Skip to content
This repository has been archived by the owner on Nov 13, 2023. It is now read-only.

Make Keytar optional for CLIs #28

Closed
tucker01 opened this issue Sep 27, 2018 · 6 comments
Closed

Make Keytar optional for CLIs #28

tucker01 opened this issue Sep 27, 2018 · 6 comments
Assignees
Labels
enhancement New feature or request 🔥 🔥 🔥 Pure Fire - For requests that are simply awesome. release-major Indicates a major breaking change will be introduced

Comments

@tucker01
Copy link
Contributor

tucker01 commented Sep 27, 2018

To improve install experience (and encourage managing credentials/secure fields via plugins), Keytar will be optional for a CLI.

Details will be added here based on design discussions.

Depends on #21

@tucker01 tucker01 added enhancement New feature or request 🔥 🔥 🔥 Pure Fire - For requests that are simply awesome. labels Sep 27, 2018
@ghost ghost added the release-major Indicates a major breaking change will be introduced label Sep 27, 2018
@ghost
Copy link

ghost commented Sep 27, 2018

Keytar and the default credential manager will not be removed in this effort. Instead we will be extending the capability to allow secure fields to be stored insecurely in the profile yaml files.

Design

Since a CLI can now have Keytar as a truly optional dependency, we will determine if we should use the DefaultCredentialManager by looking at a boolean config in the imperative config passed. If this boolean is specified as true then the DefaultCredentialManager will be used by imperative (unless overridden by a plugin). Otherwise, secure fields will be stored in plain text in the profile yaml file.

With this change, Keytar will move from an optional dependency of imperative to a dev dependency. This is purely so that we can build the DefaultCredentialManager, which requires keytar. If the CLI implemented on imperative sets the config option to use keytar, it must also specify keytar as a dependency. If it does not, the DefaultCredentialManager will throw errors indicating that keytar is not installed. As it is coded now, this will not really require a change to the DefaultCredentialManager as it already handles this.

If a secure field is managed by a credential manager, it will now be stored with the text Managed By <name>. <name> will be Default for the DefaultCredentialManager or the name of an overriding plugin.

Tentatively: Profile yaml files should now give no permissions to the everybody user in Linux. Since usernames and passwords can now be stored here, these files should not be able to be read by just anyone.

Development Notes

To save as plaintext, the CredentialManagerFactory should remain uninitialized and a variable set indicating that plaintext should be used. The profile manager will then check this value to determine if it should be sent to the credential manager or saved in plaintext.

We can get the name of an override currently by checking the AppConfig.instance.settings.overrides.CredentialManager property. With that being said, this issue depends on #21.

@ghost ghost added the in progress label Sep 27, 2018
@tucker01 tucker01 assigned tucker01 and ghost Sep 27, 2018
@tucker01
Copy link
Contributor Author

tucker01 commented Sep 27, 2018

It does appears that we can rely on package.json, as @gejohnston mentioned (even if built into an binary/electron).

Are we good with explicitly checking just dependencies? Any reason to interrogate optional?

@tucker01
Copy link
Contributor Author

A gotcha with our current thinking is that development with a local package dependency (via npm link or file:) will not resolve keytar.

@ghost
Copy link

ghost commented Sep 27, 2018

I think we should just check dependencies. But I am not against also checking the optional dependencies as well.

And I am not sure what you mean by the second point. Could you elaborate a bit?

@tucker01
Copy link
Contributor Author

I realized this is really a non-issue as long as we have Keytar as a dev dependency in imperative.

But, the scenario was... I had removed Keytar from imperative node_modules (to test) and I have imperative npm linked to my local CLI project... the credential managers module.paths will NOT include the local CLI project node_modules (where you would have Keytar installed)... this resulted in "Keytar not found".

Again, if we have Keytar installed as a dev dependency, my scenario should work just fine.

@ghost
Copy link

ghost commented Oct 8, 2018

Closed by #35

This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request 🔥 🔥 🔥 Pure Fire - For requests that are simply awesome. release-major Indicates a major breaking change will be introduced
Projects
None yet
Development

No branches or pull requests

1 participant