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

fix(secrets): Reduce keychain unlock prompts on MacOS #2394

Merged
merged 11 commits into from
Jan 7, 2025

Conversation

t1m0thyj
Copy link
Member

@t1m0thyj t1m0thyj commented Dec 21, 2024

What It Does

Reduces the number of keychain unlock prompts on MacOS for simultaneous access to secrets by multiple instances of the same application.

As a security feature, MacOS always re-prompts for keychain access after a secret has been modified by another process. This is why after running zowe config secure which updates credentials using node, you get a prompt for Zowe Explorer (the VS Code extension host process) to access credentials.

Suppose there are 3 VS Code instances open: A, B, and C. Currently when a vault change event is triggered, all 3 instances attempt to access the keychain simultaneously and Mac users will see 3 keychain prompts (ouch).

By adding a mutex associated with the calling process, we can make B & C wait to access credentials until A has finished accessing them. Now Mac users will see just 1 keychain prompt (0 would be ideal but 1 is way better than 3).

How to Test

This PR can only be tested on MacOS since the issue is OS-specific:

  1. Download the keyring.node artifact and copy it into your ZE prebuilds folder.
  2. Dequarantine the file with the command xattr -d com.apple.quarantine *.node
  3. Close all VS Code windows and re-open multiple of them.
  4. Run the zowe config secure command and press Enter repeatedly to skip all prompts.
  5. Notice that only 1 prompt is shown for keychain access.

Review Checklist
I certify that I have:

Additional Comments

Copy link

codecov bot commented Dec 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.32%. Comparing base (bf77e64) to head (9040008).
Report is 13 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2394   +/-   ##
=======================================
  Coverage   91.32%   91.32%           
=======================================
  Files         638      638           
  Lines       18210    18210           
  Branches     3823     3823           
=======================================
  Hits        16630    16630           
  Misses       1579     1579           
  Partials        1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@t1m0thyj t1m0thyj force-pushed the fix/macos-keychain-prompts branch from 5ac932d to 9475e16 Compare December 21, 2024 12:16
@t1m0thyj t1m0thyj force-pushed the fix/macos-keychain-prompts branch from 45d2bc0 to 7c0a0d7 Compare December 23, 2024 13:15
@t1m0thyj t1m0thyj requested a review from traeok December 23, 2024 15:25
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.

Changes make sense to me, thanks for the fix @t1m0thyj - pretty cool that fmutex leverages the scope of the guard to auto-release the lock 😋

As discussed offline, failing tests are related to glibc segfault with ava - once workerThreads: false is added, the stages will pass. Thanks for looking into this as well!

@t1m0thyj t1m0thyj force-pushed the fix/macos-keychain-prompts branch from 0104a49 to fadf4e8 Compare December 24, 2024 13:46
@t1m0thyj t1m0thyj added the needs-ported Indicates that a PR needs to be ported (master - lts-incremental) label Dec 24, 2024
@t1m0thyj t1m0thyj marked this pull request as ready for review January 6, 2025 17:22
Copy link

github-actions bot commented Jan 6, 2025

📅 Suggested merge-by date: 1/20/2025

Copy link
Member

@gejohnston gejohnston left a comment

Choose a reason for hiding this comment

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

A small suggestion about the changelog. Otherwise ready to approve.

packages/secrets/CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: Timothy Johnson <[email protected]>
@t1m0thyj t1m0thyj requested a review from gejohnston January 6, 2025 18:49
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.

Changes LGTM! 😋

packages/secrets/core/src/os/mac/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@anaxceron anaxceron left a comment

Choose a reason for hiding this comment

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

We should add an issue or PR link to the changelog entry

packages/secrets/CHANGELOG.md Outdated Show resolved Hide resolved
Copy link

sonarqubecloud bot commented Jan 7, 2025

@t1m0thyj t1m0thyj merged commit 5f86044 into master Jan 7, 2025
60 checks passed
@t1m0thyj t1m0thyj deleted the fix/macos-keychain-prompts branch January 7, 2025 22:09
@t1m0thyj t1m0thyj added the release-patch Indicates a patch to existing code has been applied label Jan 7, 2025
Copy link

github-actions bot commented Jan 7, 2025

Release succeeded for the master branch. 🎉

The following packages have been published:

Powered by Octorelease 🚀

@t1m0thyj t1m0thyj removed the needs-ported Indicates that a PR needs to be ported (master - lts-incremental) label Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-patch Indicates a patch to existing code has been applied released
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

7 participants