Skip to content

Use of masterkey misleading #417

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
davidsowerby opened this issue Aug 12, 2020 · 12 comments
Closed

Use of masterkey misleading #417

davidsowerby opened this issue Aug 12, 2020 · 12 comments

Comments

@davidsowerby
Copy link

In the Readme, there is a section which says:

It's possible to add other parameters to work with your instance of Parse Server:-

  await Parse().initialize(
        keyApplicationId,
        keyParseServerUrl,
        masterKey: keyParseMasterKey, // Required for Back4App and others
        clientKey: keyParseClientKey, // Required for some setups
        debug: true, // When enabled, prints logs to console
        liveQueryUrl: keyLiveQueryUrl, // Required if using LiveQuery 
        autoSendSessionId: true, // Required for authentication and ACL
        securityContext: securityContext, // Again, required for some setups
	coreStore: await CoreStoreSharedPrefsImp.getInstance()); // Local data storage method. Will use SharedPreferences instead of Sembast as an internal DB

I think this is misleading, and may encourage people to use their Back4App masterkey outside the server, which IMO is a substantial security risk.

Would it be better to say:

 masterKey: keyParseAPIKey, // Required for Back4App (use Rest API Key) and others
@RodrigoSMarques
Copy link
Contributor

You are right it is not necessary. I will do a PR by updating the documentation. Thank you!

@fischerscode
Copy link
Contributor

I think we can close this now, as #474 is part of the currently released version.

@nstrelow
Copy link
Contributor

nstrelow commented Nov 20, 2020

@davidsowerby I am using Back4app and I am not able to get a connection to the server without using the masterkey.
I tried rest, client, javascript keys instead of masterkey. No luck.

Is there a configuration option I am missing?
EDIT: Got it workinh by ONLY using the client key AS clientKey

@davidsowerby
Copy link
Author

@nstrelow glad you got it working. Not sure if you will need the Rest Key at some point but this is my set up. (There's a bit of code behind this to select environment, and load the secrets file)

The secrets.json file (not committed to repository) [keys are not real!], and I use a local parse server installation for dev and test:

{
  "dev": {
    "applicationId": "test",
    "clientId": "test",
    "restId": "test",
    "serverUrl": "http://localhost:1337/parse/"
  },
  "test": {
    "applicationId": "test",
    "clientId": "test",
    "restId": "test",
    "serverUrl": "http://localhost:1337/parse/"
  },
  "qa": {
    "applicationId": "RXK098mdfsjhsdfgnhngsdfua;e,l2X",
    "clientId": "asQ88uWvZv6KlNtgGQ88uWvZv6KlNtgG",
    "restId": "K098mdfsjhsdK098mdfsjhsdK098mdfsjhsdK098mdfsjhsd",
    "serverUrl": "https://parseapi.back4app.com/"
  },
  "prod": {
    "applicationId": "TwerweewdddoeooemskskskskoIdoDDhbSq",
    "clientId": "5kVjW2USn6SIvQnskjsaoiuewnewrtnzcWjE1voJCg",
    "restId": "Ip3dJ59qbWsdfsfsdfsddddpPVNlfQzSpfiRPJjHpA",
    "serverUrl": "https://parseapi.back4app.com/"
  },
}

And to configure Back4App

  @override
  connect() async {
    final config = await Back4AppConfig().config(env);
    _parse = await Parse().initialize(
      config.appId,
      config.serverUrl,
      autoSendSessionId: true,
      // Required for authentication and ACL
      debug: true,
      // When enabled, prints logs to console
      coreStore: coreStore,
      masterKey: config.restId,
      clientKey: config.clientId,
    );
  }

I remember struggling with this when I started with Back4App - I am wondering now whether it might be worth me publishing a small package ("back4app-connect") to do this. What do you think?

@davidsowerby
Copy link
Author

@fisherscode I see that following text has been added:

⚠️ Please note that the master key should only be used in safe environments and never on client side ‼️ Using this package on a server should be fine.

That is correct of course, but the example still states:

await Parse().initialize(
        keyApplicationId,
        keyParseServerUrl,
        **masterKey: keyParseMasterKey, // Required for Back4App and others**
        clientKey: keyParseClientKey, // Required for some setups
        debug: true, // When enabled, prints logs to console
        liveQueryUrl: keyLiveQueryUrl, // Required if using LiveQuery 
        autoSendSessionId: true, // Required for authentication and ACL
        securityContext: securityContext, // Again, required for some setups
	coreStore: CoreStoreMemoryImp()); // Non persistent mode (default): Sdk will store everything in memmore instead of using Sembast as an internal DB.

Should it not just advise which key to use for Back4App? It is pretty unlikely this package will be used on anything but a client - although I would love to write cloud functions in Dart rather than JS!

@fischerscode
Copy link
Contributor

@davidsowerby
You are properly right, we should not recommend using the master key there.

Why do you have to specify a master key?
For me it works fine without a master key specified. (Just tested it using back4app.)
(I've used the example app and modified it to only use the client key.)

@davidsowerby
Copy link
Author

I'm not absolutely sure - it's a while since I set this up. The test you did - did that include executing a cloud function? (It's the only thing I can think of that might need it)

@fischerscode
Copy link
Contributor

Executing a simple cloud function (just return a string) worked for me.

@fischerscode
Copy link
Contributor

Maybe the issue was fixed in a recent commit.

@davidsowerby
Copy link
Author

Mm, a bit of a mystery. I definitely had a problem with it, but from my Git log, that code is at least 4 months old, so something could have changed. Perhaps just take the master key out of the example? The main point is not to encourage people into poor security.

@fischerscode
Copy link
Contributor

@davidsowerby
I've created #519 to remove the master key from the example and the documentation.

@davidsowerby
Copy link
Author

Ok, I'll close this one then (see #519)

fischerscode added a commit that referenced this issue Nov 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants