Skip to content
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

Secrets are now managed by native OS credential store. #1618

Merged
merged 2 commits into from Jun 7, 2019

Conversation

Projects
None yet
3 participants
@tonyanziano
Copy link
Contributor

commented Jun 5, 2019

Resolves #1464


Please try to break this :)

Bot secrets are now stored by your OS in your native credential store:

Windows: Credential Manager
Mac: Keychain
Linux: Secret Service API / libsecret

image

image

NOTE #1: Due to the fact that we now use keytar which is a native node addon, it has to be built properly for Electron when launching and running the app, and must be built for Node when running the tests (Jest environment).

I couldn't figure out a way to have it easily built for both environments, so if you decide to flip back and forth between running tests and launching the app, then you will need to use the scripts in the root/package.json file:

  • rebuild:keytar:electron: use when you want to run the app
  • rebuild:keytar:node: use when you want to run the tests

Let me know if you think this process could be improved or better documented.


NOTE #2: The library used to rebuild keytar for Electron, electron-rebuild, does not play nicely with Lerna monorepos, and so keytar had to be added as a dependency to the root package.json file. This should not have any adverse effects other than there being an extra line in the file. I opened an issue against electron-rebuild and it can be found here.

@justinwilaby

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

This looks good. I think we should consider code that removes the store bot secrets from the server.json with this rollout.

@cwhitten - thoughts?

@tonyanziano

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

@justinwilaby bot secrets are currently stored in bots.json via the BotInfo type, unless you are talking about something else. This PR has code that removes that property from those entries.

@cwhitten

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

I think we need to do that before we can submit for the security & privacy reviews. @tonyanziano what do you think?

@tonyanziano

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

I thought that removing the property would be sufficient, and that further writes to bots.json would then omit the property, but that doesn't seem to be the case after testing it just now.

I can add some code that purges any secrets from bots.json before loading / writing.

@tonyanziano tonyanziano force-pushed the toanzian/#1464-secrets branch from e7e3f68 to 76acf69 Jun 5, 2019

return botsJson.bots;
return botsJson.bots.map(bot => {
delete bot.secret;
return bot;

This comment has been minimized.

Copy link
@tonyanziano

tonyanziano Jun 5, 2019

Author Contributor

this code purges the secret property from any BotInfo objects when we first load bots from disk

@cwhitten
Copy link
Member

left a comment

Verified working on Mac! Nice work

@cwhitten cwhitten merged commit ca9d735 into master Jun 7, 2019

3 checks passed

Emulator-CI-PR #63213 succeeded
Details
[experimental]Upload-package.json #63214 succeeded
Details
license/cla All CLA requirements met.
Details

@cwhitten cwhitten deleted the toanzian/#1464-secrets branch Jun 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.