Move away from node-keytar to electron's SafeStorage API

[hl662]

Closes #161

node-keytar has been deprecated, thus requiring us to figure out a way to do OS-based credentials. Electron's SafeStorage API uses Chromium's oscrypt under the hood to encrypt and decrypt strings using the client OS's specific credential managers. That left us with just a persistent storage solution to look for, which electron-store fulfills.

Moving straight to SafeStorage and removing node-keytar entirely would essentially clear out a user's sessions, and require them to perform single sign on again. Instead, we can slowly migrate to SafeStorage, by moving the tokens used in node-keytar over to electron-store the first time the auth client retrieves their refreshToken. Thus, no side-effects will happen, giving clients time to upgrade their auth-clients modules without hassle.

[aruniverse]

fyi @MichaelBelousov @johnnyd710 @karolis-zukauskas @GintV can you guys take a look to make sure we dont break you

[MichaelBelousov]

fyi @MichaelBelousov @johnnyd710 @!karolis-zukauskas @!GintV can you guys take a look to make sure we dont break you

we actually just found that a PR we've tried to get in but is hanging in CI is likely caused by keytar, we're going to test if this patch fixes it, and will assess whether this breaks us. In our case, it's OK if it causes the user to log in again.

cc @johnnyd710

[johnnyd710]

I quickly tried patching this PR into our application, but I get (replacing the name of our app with "appname"):

no such file or direcory, mkdir C:\Users\John.DiMatteo\AppData\Roaming\@appname\app-nodejs\Config\iTwinJs:app-name:https:.

Btw, we override the electron userdata directory shortly after our app starts up to C:\Users\John.DiMatteo\AppData\Roaming\Bentley\appname.

Also, like Mike said, we are having issues with keytar in our CI, so it would be great if we could get rid of it completely. I see you still use it to migrate the old keys over. Could you make that optional?

[ben-polinsky]

I quickly tried patching this PR into our application, but I get (replacing the name of our app with "appname"):

no such file or direcory, mkdir C:\Users\John.DiMatteo\AppData\Roaming\@appname\app-nodejs\Config\iTwinJs:app-name:https:.

Btw, we override the electron userdata directory shortly after our app starts up to C:\Users\John.DiMatteo\AppData\Roaming\Bentley\appname.

Also, like Mike said, we are having issues with keytar in our CI, so it would be great if we could get rid of it completely. I see you still use it to migrate the old keys over. Could you make that optional?

@hl662 could you take a look at the first issue and also the failing integration tests?

@johnnyd710 I see. It's going to be tough to do in a non-breaking (semver) way. Wondering if it's worth it for us to keep the migration. Was your build previously passing with keytar installed?

[hl662]

Hmm yeah, that error was also popping up in the CI build for windows. It might be an issue with the filename being too long, thus windows is splitting the filename in half - one part is a folder and the other part is the actual .json containing the credentials. I can shorten the file name. As for dropping node-keytar, we could... Just that the side effect of this would be any app that bumps itwin/electron-authorization will just have to sign in again.

[hl662]

Hmm yeah, that error was also popping up in the CI build for windows. It might be an issue with the filename being too long, thus windows is splitting the filename in half - one part is a folder and the other part is the actual .json containing the credentials. I can shorten the file name. As for dropping node-keytar, we could... Just that the side effect of this would be any app that bumps itwin/electron-authorization will just have to sign in again.

scratch what I said above, it failed because with the .json filename, part of it has the issuer_url which has http:/ - the / tells all operating systems to create a folder with one half of the filename, and the other half is the actual file. This side effect is fine on Linux and Mac, but on Windows, it fails cause windows paths use \. Still gonna shorten the file name, probably gonna cut the issuer_url from it.

[ben-polinsky]

As for dropping node-keytar, we could... Just that the side effect of this would be any app that bumps itwin/electron-authorization will just have to sign in again.

This is true. I really liked the idea users wouldn't have to, though.

[MichaelBelousov]

This is true. I really liked the idea users wouldn't have to, though.

agreed. We were hoping more for a runtime option to not access keytar at all. We might have to patch one in ourselves if there is none, because it seems to hang in our CI builds and we haven't determined why yet. Ideally we'd reproduce the issue in a Linux VM and figure out why but haven't been able to yet and for us it'll be faster potentially to just stub out keytar. Up to you if you want to expose configuration for us to avoid it.

[ben-polinsky]

This is true. I really liked the idea users wouldn't have to, though.

agreed. We were hoping more for a runtime option to not access keytar at all. We might have to patch one in ourselves if there is none, because it seems to hang in our CI builds and we haven't determined why yet. Ideally we'd reproduce the issue in a Linux VM and figure out why but haven't been able to yet and for us it'll be faster potentially to just stub out keytar. Up to you if you want to expose configuration for us to avoid it.

Ah, so your CI is failing at runtime? I had assumed during install/build. If that's the case, we can attempt to conditionally import based on an environment variable.

[MichaelBelousov]

Ah, so your CI is failing at runtime? I had assumed during install/build. If that's the case, we can attempt to conditionally import based on an environment variable.

that would be fine for us. env var or some sort of configuration API would work for us.

[johnnyd710]

FYI, I was able to get this working with our app - it just required us to polyfill some electron utilities (like safeStorage) since our authentication happens on a separate node process that doesn't have access to electron, but that's our problem, not yours.

[hl662]

electron-store creates the storage file in different locations, depending on whether the Store was initialized in the main Electron process or the renderer process - see explanation here. This explains why the integration tests were hanging - the RefreshTokenStore in the test file and the one from ElectronMainAuthorization pointed to different places. Solution was to manually resolve the path, depending on the OS.

[hl662]

@MichaelBelousov @johnnyd710 can you review? If your app overriding the electron userData folder causes the client to behave wonky, I could add a config option to overwrite the tokenStore location for electronMainAuth

[johnnyd710]

@MichaelBelousov @johnnyd710 can you review? If your app overriding the electron userData folder causes the client to behave wonky, I could add a config option to overwrite the tokenStore location for electronMainAuth

Yeah, it's a bit weird that there is a difference between the location we store config and settings, and the location you use for the token store. If you could provide us an option to override the tokenStore, that would be ideal.

[hl662]

@MichaelBelousov @johnnyd710 can you review? If your app overriding the electron userData folder causes the client to behave wonky, I could add a config option to overwrite the tokenStore location for electronMainAuth

Yeah, it's a bit weird that there is a difference between the location we store config and settings, and the location you use for the token store. If you could provide us an option to override the tokenStore, that would be ideal.

I added a config option to ElectronMainAuthorizationConfiguration

[johnnyd710]

tested and working with our app (with the node process patches/polyfills I mentioned before, since we don't have electron)

[ben-polinsky]

@MichaelBelousov @johnnyd710 can you review? If your app overriding the electron userData folder causes the client to behave wonky, I could add a config option to overwrite the tokenStore location for electronMainAuth

Yeah, it's a bit weird that there is a difference between the location we store config and settings, and the location you use for the token store. If you could provide us an option to override the tokenStore, that would be ideal.

I added a config option to ElectronMainAuthorizationConfiguration

Could you add this to the readme.md for the electron package?

[hl662]

@MichaelBelousov @johnnyd710 can you review? If your app overriding the electron userData folder causes the client to behave wonky, I could add a config option to overwrite the tokenStore location for electronMainAuth

Yeah, it's a bit weird that there is a difference between the location we store config and settings, and the location you use for the token store. If you could provide us an option to override the tokenStore, that would be ideal.

I added a config option to ElectronMainAuthorizationConfiguration

Could you add this to the readme.md for the electron package?

Added, also updated the README to remove references to keytar. safeStorage does want to use lib-secret and gnome-keyring on linux as discussed here, so I just replaced keytar with safeStorage.