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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 15 additions & 14 deletions .github/workflows/secrets-sdk.yml
Original file line number Diff line number Diff line change
Expand Up @@ -38,27 +38,27 @@ jobs:
npm run build -- --target i686-pc-windows-msvc
npm run test
target: i686-pc-windows-msvc
- host: ubuntu-22.04
- host: ubuntu-latest
target: x86_64-unknown-linux-gnu
use-cross: true
build: |
set -e
CARGO=cross npm run build -- --target x86_64-unknown-linux-gnu
- host: ubuntu-22.04
- host: ubuntu-latest
target: i686-unknown-linux-gnu
use-cross: true
build: |
set -e
source scripts/configure-cross.sh i686-unknown-linux-gnu
CARGO=cross npm run build -- --target i686-unknown-linux-gnu
- host: ubuntu-22.04
- host: ubuntu-latest
target: armv7-unknown-linux-gnueabihf
use-cross: true
build: |
set -e
source scripts/configure-cross.sh armv7-unknown-linux-gnueabihf
CARGO=cross npm run build -- --target armv7-unknown-linux-gnueabihf
- host: ubuntu-22.04
- host: ubuntu-latest
target: x86_64-unknown-linux-musl
use-cross: true
build: |
Expand All @@ -67,14 +67,14 @@ jobs:
- host: macos-latest
target: aarch64-apple-darwin
build: npm run build -- --target aarch64-apple-darwin
- host: ubuntu-22.04
- host: ubuntu-latest
target: aarch64-unknown-linux-gnu
use-cross: true
build: |
set -e
source scripts/configure-cross.sh aarch64-unknown-linux-gnu
CARGO=cross npm run build -- --target aarch64-unknown-linux-gnu
- host: ubuntu-22.04
- host: ubuntu-latest
target: aarch64-unknown-linux-musl
use-cross: true
build: |
Expand Down Expand Up @@ -208,22 +208,23 @@ jobs:
- host: macos-latest
target: x86_64-apple-darwin
architecture: x64
- host: ubuntu-22.04
- host: ubuntu-latest
target: x86_64-unknown-linux-gnu
- host: ubuntu-22.04
- host: ubuntu-latest
target: x86_64-unknown-linux-musl
- host: ubuntu-22.04
- host: ubuntu-latest
target: aarch64-unknown-linux-gnu
platform: linux/arm64
- host: ubuntu-22.04
- host: ubuntu-latest
target: aarch64-unknown-linux-musl
platform: linux/arm64
- host: ubuntu-22.04
- host: ubuntu-latest
target: armv7-unknown-linux-gnueabihf
platform: linux/arm/v7
node:
- "18"
- "20"
- "22"
runs-on: ${{ matrix.settings.host }}
steps:
- uses: actions/checkout@v4
Expand All @@ -250,10 +251,10 @@ jobs:
if: ${{ matrix.settings.platform }}
- name: Test bindings
run: npm run test
if: ${{ matrix.settings.host != 'ubuntu-22.04' }}
if: ${{ matrix.settings.host != 'ubuntu-latest' }}
- name: Setup and run tests
uses: addnab/docker-run-action@v3
if: ${{ matrix.settings.host == 'ubuntu-22.04' && !endsWith(matrix.settings.target, 'musl') }}
if: ${{ matrix.settings.host == 'ubuntu-latest' && !endsWith(matrix.settings.target, 'musl') }}
with:
image: ${{ format('node:{0}-slim', matrix.node) }}
options: "-v ${{ github.workspace }}:/build -w /build --cap-add=IPC_LOCK ${{ matrix.settings.platform && format('--platform={0}', matrix.settings.platform) }}"
Expand All @@ -263,7 +264,7 @@ jobs:
cd packages/secrets && dbus-run-session -- bash scripts/linux-test.sh
- name: Setup and run tests (MUSL)
uses: addnab/docker-run-action@v3
if: ${{ matrix.settings.host == 'ubuntu-22.04' && endsWith(matrix.settings.target, 'musl') }}
if: ${{ matrix.settings.host == 'ubuntu-latest' && endsWith(matrix.settings.target, 'musl') }}
with:
image: ${{ format('node:{0}-alpine', matrix.node) }}
options: "-v ${{ github.workspace }}:/build -w /build --cap-add=IPC_LOCK ${{ matrix.settings.platform && format('--platform={0}', matrix.settings.platform) }}"
Expand Down
6 changes: 5 additions & 1 deletion packages/secrets/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

All notable changes to the Zowe Secrets SDK package will be documented in this file.

## Recent Changes

- BugFix: Reduced number of keychain unlock prompts on MacOS for simultaneous access to secrets by multiple instances of the same application.

## `8.1.2`

- BugFix: Updated dependencies for technical currency. [#2289](https://github.com/zowe/zowe-cli/pull/2289)
Expand Down Expand Up @@ -52,4 +56,4 @@ All notable changes to the Zowe Secrets SDK package will be documented in this f
## `7.18.0`

- Initial release.
- `keyring` module added for interacting with OS-specific keyring/credential vaults. See [src/keyring](src/keyring/README.md) for information on this native module and how it can be used.
- `keyring` module added for interacting with OS-specific keyring/credential vaults. See [src/keyring](src/keyring/README.md) for information on this native module and how it can be used.
12 changes: 11 additions & 1 deletion packages/secrets/core/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions packages/secrets/core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,11 @@ version = "0.48.0"
[target.'cfg(target_os = "macos")'.dependencies]
core-foundation = "0.9.3"
core-foundation-sys = "0.8.4"
fmutex = "0.1.0"

[target.'cfg(any(target_os = "freebsd", target_os = "linux"))'.dependencies]
gio = "0.18.2"
glib = "0.18.2"
glib-sys = "0.18.1"
gio = "0.18.2"
libsecret = "0.4.0"
libsecret-sys = "0.4.0"
libsecret-sys = "0.4.0"
1 change: 0 additions & 1 deletion packages/secrets/core/src/os/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ pub enum KeyringError {
#[error("[keyring] {name:?} library returned an error:\n\n{details:?}")]
Library { name: String, details: String },

#[cfg(not(target_os = "macos"))]
#[error("[keyring] An OS error has occurred:\n\n{0}")]
Os(String),

Expand Down
27 changes: 27 additions & 0 deletions packages/secrets/core/src/os/mac/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use error::Error;

use crate::os::mac::error::ERR_SEC_ITEM_NOT_FOUND;
use crate::os::mac::keychain_search::{KeychainSearch, SearchResult};
use fmutex::Guard;
use keychain::SecKeychain;

impl From<Error> for KeyringError {
Expand All @@ -22,6 +23,27 @@ impl From<Error> for KeyringError {
}
}

impl From<std::io::Error> for KeyringError {
fn from(error: std::io::Error) -> Self {
KeyringError::Os(error.to_string())
}
}

fn keyring_mutex() -> Result<Guard, KeyringError> {
// MacOS shows keychain prompt after secret has been modified by another process. We use cross-process mutex to
// block keychain access if there are multiple concurrent keychain operations invoked by the same process. This
// prevents multiple instances of the same app (e.g. VS Code) from triggering several keychain prompts at once.
let exe_path = std::env::current_exe()
.unwrap()
.to_string_lossy()
.replace(std::path::MAIN_SEPARATOR, "_");
let lock_path = std::env::temp_dir()
.join(format!("keyring_{}.lock", exe_path));
std::fs::OpenOptions::new().create(true).write(true).open(&lock_path)
.and_then(|_| fmutex::lock(lock_path))
.map_err(KeyringError::from)
}

///
/// Attempts to set a password for a given service and account.
///
Expand All @@ -38,6 +60,7 @@ pub fn set_password(
password: &String,
) -> Result<bool, KeyringError> {
let keychain = SecKeychain::default().unwrap();
let _lock = keyring_mutex().unwrap();
match keychain.set_password(service.as_str(), account.as_str(), password.as_bytes()) {
Ok(()) => Ok(true),
Err(err) => Err(KeyringError::from(err)),
Expand All @@ -56,6 +79,7 @@ pub fn set_password(
///
pub fn get_password(service: &String, account: &String) -> Result<Option<String>, KeyringError> {
let keychain = SecKeychain::default().unwrap();
let _lock = keyring_mutex().unwrap();
match keychain.find_password(service.as_str(), account.as_str()) {
Ok((pw, _)) => Ok(Some(String::from_utf8(pw.to_owned())?)),
Err(err) if err.code() == ERR_SEC_ITEM_NOT_FOUND => Ok(None),
Expand Down Expand Up @@ -83,6 +107,7 @@ pub fn find_password(service: &String) -> Result<Option<String>, KeyringError> {
}

let keychain = SecKeychain::default().unwrap();
let _lock = keyring_mutex().unwrap();
match keychain.find_password(cred_attrs[0], cred_attrs[1]) {
Ok((pw, _)) => {
let pw_str = String::from_utf8(pw.to_owned())?;
Expand All @@ -104,6 +129,7 @@ pub fn find_password(service: &String) -> Result<Option<String>, KeyringError> {
///
pub fn delete_password(service: &String, account: &String) -> Result<bool, KeyringError> {
let keychain = SecKeychain::default().unwrap();
let _lock = keyring_mutex().unwrap();
match keychain.find_password(service.as_str(), account.as_str()) {
Ok((_, item)) => {
item.delete()?;
Expand All @@ -128,6 +154,7 @@ pub fn find_credentials(
service: &String,
credentials: &mut Vec<(String, String)>,
) -> Result<bool, KeyringError> {
let _lock = keyring_mutex().unwrap();
match KeychainSearch::new()
.label(service.as_str())
.with_attrs()
Expand Down
3 changes: 2 additions & 1 deletion packages/secrets/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@
"ava": "^6.0.0"
},
"ava": {
"timeout": "3m"
"timeout": "3m",
"workerThreads": false
},
"engines": {
"node": ">=14"
Expand Down
12 changes: 11 additions & 1 deletion packages/secrets/src/keyring/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions packages/secrets/src/keyring/Cross.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@ passthrough = [
image = "ghcr.io/cross-rs/aarch64-unknown-linux-gnu:main"

[target.aarch64-unknown-linux-musl]
image = "rust:alpine"
image = "rust:alpine3.20"
pre-build = [
"wget -qO- https://musl.cc/aarch64-linux-musl-cross.tgz | tar -xzC / && export PATH=\"/aarch64-linux-musl-cross/bin:$PATH\"",
"apk add --no-cache musl-dev pkgconfig",
"apk add -p /aarch64-linux-musl-cross --initdb --arch aarch64 --allow-untrusted -X \"https://dl-cdn.alpinelinux.org/alpine/latest-stable/main/\" --no-cache --no-scripts libsecret-dev",
"apk add -p /aarch64-linux-musl-cross --initdb --arch aarch64 --allow-untrusted -X $(head -n 1 /etc/apk/repositories) --no-cache --no-scripts libsecret-dev",
"rustup target add aarch64-unknown-linux-musl"
]

Expand All @@ -33,5 +33,5 @@ image = "ghcr.io/cross-rs/i686-unknown-linux-gnu:main"
image = "ghcr.io/cross-rs/x86_64-unknown-linux-gnu:main"

[target.x86_64-unknown-linux-musl]
image = "rust:alpine"
image = "rust:alpine3.20"
pre-build = ["apk add libsecret-dev musl-dev"]
Loading