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

Add support for custom credential manager extensions #2230

Merged
merged 27 commits into from
Jun 6, 2023

Conversation

rudyflores
Copy link
Contributor

@rudyflores rudyflores commented Apr 11, 2023

Proposed changes

Solves #2212

Add support for custom credential manager extensions. When a new custom credential manager extension is installed, Zowe Explorer will activate that extension and handle credentials for Zowe Profiles through the custom credential manager override set in the imperative.json file.

e.g:

  • User installs Kubernetes secrets credential manager extension
  • imperative.json is set to Secrets for Kubernetes
  • Zowe Explorer activates the extension and begins securing/storing credentials with Kubernetes secrets
  • User uninstalls Kubernetes secrets credential manager extension
  • Refresh is required for changes to happen
  • imperative.json is set to the default @zowe/cli
  • Zowe Explorer begins to secure credentials with Keytar

Release Notes

Milestone: TBD

Changelog: Added support for custom credential manager extensions in Zowe Explorer

Types of changes

What types of changes does your code introduce to Zowe Explorer?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Updates to Documentation or Tests (if none of the other choices apply)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This checklist will be used as reference for both the contributor and the reviewer

  • I have read the CONTRIBUTOR GUIDANCE wiki
  • PR title follows Conventional Commits Guidelines
  • PR Description is included
  • gif or screenshot is included if visual changes are made
  • yarn workspace vscode-extension-for-zowe vscode:prepublish has been executed
  • All checks have passed (DCO, Jenkins and Code Coverage)
  • I have added unit test and it is passing
  • I have added integration test and it is passing
  • There is coverage for the code that I have added
  • I have tested it manually and there are no regressions found
  • I have added necessary documentation (if appropriate)
  • Any PR dependencies have been merged and published (if appropriate)

Further comments

This change will also remove the old code that will default imperative.json back to @zowe/cli if the unsecure profile is in use. From now on imperative.json should reflect what plugin is installed, the default is in use, or no credential manager is needed.

Signed-off-by: Rudy Flores <[email protected]>
Signed-off-by: Rudy Flores <[email protected]>
Signed-off-by: Rudy Flores <[email protected]>
Signed-off-by: Rudy Flores <[email protected]>
@rudyflores rudyflores self-assigned this Apr 11, 2023
Signed-off-by: Rudy Flores <[email protected]>
@codecov
Copy link

codecov bot commented Apr 14, 2023

Codecov Report

Patch coverage: 98.11% and project coverage change: +0.06 🎉

Comparison is base (82fc6bf) 91.83% compared to head (ac0cd86) 91.90%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2230      +/-   ##
==========================================
+ Coverage   91.83%   91.90%   +0.06%     
==========================================
  Files          90       90              
  Lines        9201     9242      +41     
  Branches     1900     1908       +8     
==========================================
+ Hits         8450     8494      +44     
+ Misses        750      747       -3     
  Partials        1        1              
Impacted Files Coverage Δ
packages/zowe-explorer/src/utils/ProfilesUtils.ts 89.96% <97.72%> (+1.65%) ⬆️
packages/zowe-explorer/src/ZoweExplorerExtender.ts 86.36% <100.00%> (ø)
packages/zowe-explorer/src/globals.ts 98.61% <100.00%> (+2.15%) ⬆️
packages/zowe-explorer/src/shared/init.ts 97.91% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@rudyflores rudyflores marked this pull request as ready for review April 25, 2023 19:43
JillieBeanSim
JillieBeanSim previously approved these changes May 8, 2023
Copy link
Contributor

@JillieBeanSim JillieBeanSim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rudyflores for this enhancement

Would you mind adding a section in the wiki under Extenders section to explain the new addition please?

@rudyflores
Copy link
Contributor Author

JillieBeanSim
JillieBeanSim previously approved these changes May 9, 2023
packages/zowe-explorer/src/globals.ts Outdated Show resolved Hide resolved
packages/zowe-explorer/src/utils/ProfilesUtils.ts Outdated Show resolved Hide resolved
packages/zowe-explorer/src/utils/ProfilesUtils.ts Outdated Show resolved Hide resolved
@t1m0thyj
Copy link
Member

t1m0thyj commented May 18, 2023

Awesome work! Sorry for taking so long to review this PR 😢 Tested this PR by installing the K8s extension and it works 🎉
I do have a few questions:

In #2212 (comment) the following implementation was suggested:

  1. It could read the contents of the Imperative settings file, and check the mapping mentioned above to see if the associated VS Code extension is installed.

    • Setting is defined in ~/.zowe/settings/imperative.json or in team config files:
      "CredentialManager": "Secrets for Kubernetes"
  2. If the credential manager is valid and an associated extension is installed, then ZE will load and use the custom credential manager extension instead of Keytar.

  3. If the credential manager is valid and an associated extension is not installed, then ZE will prompt the user asking if they wish to install the extension from the VS Code Marketplace.

  4. If the credential manager is invalid, then ZE will prompt the user asking if they wish to reset it to the default value (@zowe/cli) which uses Keytar.

This PR implements (1) and (2), but not (3) and (4). Do we still want to implement (3) and (4), and if so would we plan to do it in this PR or a separate one?

Additional Comments:

@JillieBeanSim JillieBeanSim added this to the v2.9.0 milestone May 18, 2023
@rudyflores
Copy link
Contributor Author

@t1m0thyj Regarding your comment, (1) and (2) should be implemented. As for (3) and (4), that was not really done yet since this can perhaps be addressed in a follow up PR? I think this PR is already a good starting point which we can perhaps refine to have those last two requirements implemented in order for us to provide the best experience when using custom credential managers :)

Copy link
Member

@traeok traeok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks awesome so far! I was able to store credentials in a secret on a Google Cloud k8s cluster. However, I ran into a couple issues when testing:

After I installed the K8s plugin in Zowe CLI, I expected that ZE would prompt me to install the K8s extension when VS Code is launched. But it didn't prompt me, and it also reset my CredentialManager setting in ~/.zowe/settings/imperative.json back to @zowe/cli.

Like @t1m0thyj mentioned, I had a similar issue to the one described above, except I already had the plugin and VS extension installed. After reloading my VS Code window, the credential manager reset to @zowe/cli. After reinstalling both the CLI plugin and VS Code extension, the issue was resolved. Here's how I was able to reproduce the problem:

  1. Install Kubernetes VS Code extension
  2. Install Kubernetes CLI plugin in the command line
  3. Reload window for Zowe Explorer
  4. Notice that the CredentialManager value was reset to @zowe/cli

In addition, I also noticed that the credentials are stored under a single secret on Google Cloud, and I presume this will also be the case in a local environment. Since the credentials are stored in a way where the Zowe config path is the "key" for the object, @zFernand0 and I quickly discussed a scenario where he could potentially replace my credentials in the secret object:

  1. Create a path that matches the file path my credentials are stored under
  2. Set the ZOWE_CLI_HOME environment variable to fall under this path (e.g. if my config is /a/b/c/zowe.config.json, ZOWE_CLI_HOME would be /a/b/c).
  3. Right click on a session in Zowe Explorer -> "Update Credentials"
  4. After entering in the username and password, my stored credentials will be overwritten in the secret object w/ the new credentials provided.

Could we potentially avoid this by generating a unique hash on the user's end, and then chaining that with the file path to make a unique key? Or, could we generate a unique hash and simply use that as the key?

With either proposal, we would have to store the hash/identifier somewhere (such as in the Zowe config itself), but this would prevent someone from intentionally overwriting other user's credentials in the same cluster.

@JTonda JTonda added the dev-docs documentation delivered by developer label May 25, 2023
Copy link
Member

@t1m0thyj t1m0thyj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will approve once merge conflicts are resolved 🙂

I have a question about this sequence:

  • User installs Kubernetes secrets credential manager extension
  • imperative.json is set to Secrets for Kubernetes

Is it correct that in between those steps, refresh is required for changes to happen? If so that's fine - I think in the future we could investigate handling the extensions.onDidChange event in VS Code to prompt the user to refresh ZE or reload the window.

Regarding the issues mentioned earlier:

@traeok
Copy link
Member

traeok commented May 30, 2023

moving comment to zowe/zowe-cli-secrets-for-kubernetes as it relates to the extension and not this PR

@rudyflores
Copy link
Contributor Author

Will approve once merge conflicts are resolved 🙂

I have a question about this sequence:

  • User installs Kubernetes secrets credential manager extension
  • imperative.json is set to Secrets for Kubernetes

Is it correct that in between those steps, refresh is required for changes to happen? If so that's fine - I think in the future we could investigate handling the extensions.onDidChange event in VS Code to prompt the user to refresh ZE or reload the window.

Regarding the issues mentioned earlier:

Yes that is correct, currently the user needs to restart in order to see the changes reflected. I think in the future it would be nice to find a way to prompt for a reload if a credential manager is installed/uninstalled (currently only disabling the custom credential manager prompts for reload because VS Code prompts to do so).

Regarding the bullet points:

  1. regarding that issue I think that's where we'll need to prompt for reload upon installing/uninstalling and also integrate the work discussed before regarding checking if the extension is in the VS Code marketplace and follow the CLI's configuration.
  2. In the Kubernetes world a user will store a credential per namespace and this can also be documented, just not sure where this should be documented since this is related to the credential manager and not ZE?

@rudyflores rudyflores requested review from traeok and t1m0thyj June 5, 2023 15:19
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Member

@t1m0thyj t1m0thyj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @rudyflores!

Copy link
Member

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing all my novice questions 😋
LGTM! 😋

@zFernand0 zFernand0 merged commit 155ee55 into main Jun 6, 2023
@zFernand0 zFernand0 deleted the credential-plugins-support branch June 6, 2023 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev-docs documentation delivered by developer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for custom credential manager extensions
6 participants