Scope OS-keyring access to the default install path (fixes real-credential wipe by tests)
Root cause of today's repeated logouts: the keyring entry (service kigi / oauth/kimi-code) is global per OS user, but the enable gate keyed off ENV VARS while an AuthManager's identity is its constructor path. Integration test binaries (compiled without cfg(test), no KIGI_SHARE_DIR in env) constructed managers on tempdirs whose remove_scope() then deleted the developer's REAL keychain credential — the unified log shows 13 such wipes in one day, one per test run. Structural fix: keyring participation is now a property of the manager's own path. AuthManager captures keyring_path_scoped at construction (path == default ~/.kigi/auth.json) and every keyring touch — the constructor read, update()'s write, remove_scope()'s delete — requires it, with the dynamic keyring_enabled() gate (env kill-switch, cfg(test) mock toggle) layered on top. A tempdir-rooted manager can no longer read, write, or delete the global entry no matter what process type it runs in. Regression test tempdir_manager_never_touches_global_keyring pins the incident: a foreign manager's logout must leave the (mock) keyring entry intact. Keyring behavior tests keep constructor-read coverage via a thread-local path-scope test seam.
This commit is contained in:
@@ -112,6 +112,37 @@ const PERMANENT_FAILURE_TTL: StdDuration = StdDuration::from_secs(300);
|
|||||||
/// `attempted_tombstone_key`, when a tombstone is stored), never co-held. Never hold
|
/// `attempted_tombstone_key`, when a tombstone is stored), never co-held. Never hold
|
||||||
/// a `parking_lot` guard across `.await`. Refreshers return [`RefreshOutcome`]
|
/// a `parking_lot` guard across `.await`. Refreshers return [`RefreshOutcome`]
|
||||||
/// for `refresh_chain` to apply.
|
/// for `refresh_chain` to apply.
|
||||||
|
/// Whether a manager rooted at `path` may use the OS keyring for `scope`.
|
||||||
|
///
|
||||||
|
/// The keyring entry (`service kigi / oauth/kimi-code`) is global per OS
|
||||||
|
/// user, so exactly one auth.json location can own it: the default install
|
||||||
|
/// path. Everything else (tempdir tests, `KIGI_SHARE_DIR` profiles,
|
||||||
|
/// `KIGI_AUTH_PATH` overrides) is file-scoped. The dynamic
|
||||||
|
/// [`keyring_enabled`] gate (env kill-switch, cfg(test) mock toggle) layers
|
||||||
|
/// on top at each call site.
|
||||||
|
fn keyring_path_scoped_for(path: &Path, scope: &str) -> bool {
|
||||||
|
#[cfg(test)]
|
||||||
|
if TEST_FORCE_KEYRING_PATH_SCOPE.with(|flag| flag.get()) {
|
||||||
|
return scope == KIMI_CODE_OAUTH_SCOPE;
|
||||||
|
}
|
||||||
|
scope == KIMI_CODE_OAUTH_SCOPE && path == kigi_config::default_kigi_home().join("auth.json")
|
||||||
|
}
|
||||||
|
|
||||||
|
// Test seam: pretend managers on this thread are rooted at the default
|
||||||
|
// install so keyring behavior can be exercised against the mock keyring
|
||||||
|
// from a tempdir. Thread-local for the same reason as the mock-keyring
|
||||||
|
// toggle: no leakage into concurrently running persistence tests.
|
||||||
|
#[cfg(test)]
|
||||||
|
thread_local! {
|
||||||
|
static TEST_FORCE_KEYRING_PATH_SCOPE: std::cell::Cell<bool> =
|
||||||
|
const { std::cell::Cell::new(false) };
|
||||||
|
}
|
||||||
|
|
||||||
|
#[cfg(test)]
|
||||||
|
pub(crate) fn set_test_force_keyring_path_scope(on: bool) {
|
||||||
|
TEST_FORCE_KEYRING_PATH_SCOPE.with(|flag| flag.set(on));
|
||||||
|
}
|
||||||
|
|
||||||
pub struct AuthManager {
|
pub struct AuthManager {
|
||||||
/// In-memory bearer. Mutate via [`Self::with_inner_write`] or
|
/// In-memory bearer. Mutate via [`Self::with_inner_write`] or
|
||||||
/// [`Self::refresh_chain`]; the closure helpers' sync return type
|
/// [`Self::refresh_chain`]; the closure helpers' sync return type
|
||||||
@@ -119,6 +150,15 @@ pub struct AuthManager {
|
|||||||
inner: Arc<RwLock<Option<KimiAuth>>>,
|
inner: Arc<RwLock<Option<KimiAuth>>>,
|
||||||
path: PathBuf,
|
path: PathBuf,
|
||||||
scope: String,
|
scope: String,
|
||||||
|
/// Whether THIS manager may touch the OS keyring. The keyring entry is
|
||||||
|
/// global per user, so it can only mirror the credential of the default
|
||||||
|
/// install (`~/.kigi/auth.json`). Managers rooted anywhere else —
|
||||||
|
/// integration-test tempdirs, alternate profiles, `KIGI_AUTH_PATH` —
|
||||||
|
/// must never read, write, or DELETE it: before this guard, every
|
||||||
|
/// `cargo test` run wiped the developer's real login via
|
||||||
|
/// `remove_scope`'s keyring delete. [`keyring_enabled`] (env
|
||||||
|
/// kill-switch / cfg(test) mock toggle) still gates dynamically on top.
|
||||||
|
keyring_path_scoped: bool,
|
||||||
kimi_code_config: KimiCodeConfig,
|
kimi_code_config: KimiCodeConfig,
|
||||||
refresher: RwLock<Option<Arc<dyn TokenRefresher>>>,
|
refresher: RwLock<Option<Arc<dyn TokenRefresher>>>,
|
||||||
/// Idempotency guard for `configure_refresher` so double-calls
|
/// Idempotency guard for `configure_refresher` so double-calls
|
||||||
@@ -273,7 +313,8 @@ impl AuthManager {
|
|||||||
.unwrap_or_else(|_| kigi_home.join("auth.json"));
|
.unwrap_or_else(|_| kigi_home.join("auth.json"));
|
||||||
|
|
||||||
// Keyring first (PRD F1): the session credential's primary store.
|
// Keyring first (PRD F1): the session credential's primary store.
|
||||||
if scope == KIMI_CODE_OAUTH_SCOPE
|
if keyring_path_scoped_for(&path, &scope)
|
||||||
|
&& keyring_enabled()
|
||||||
&& let KeyringRead::Found(auth) = keyring_read_session()
|
&& let KeyringRead::Found(auth) = keyring_read_session()
|
||||||
{
|
{
|
||||||
kigi_log::unified_log::info(
|
kigi_log::unified_log::info(
|
||||||
@@ -356,6 +397,7 @@ impl AuthManager {
|
|||||||
) -> Self {
|
) -> Self {
|
||||||
Self {
|
Self {
|
||||||
inner: Arc::new(RwLock::new(inner)),
|
inner: Arc::new(RwLock::new(inner)),
|
||||||
|
keyring_path_scoped: keyring_path_scoped_for(&path, &scope),
|
||||||
path,
|
path,
|
||||||
scope,
|
scope,
|
||||||
kimi_code_config,
|
kimi_code_config,
|
||||||
@@ -402,8 +444,11 @@ impl AuthManager {
|
|||||||
fn remove_scope_impl(&self, scope: &str) -> std::io::Result<()> {
|
fn remove_scope_impl(&self, scope: &str) -> std::io::Result<()> {
|
||||||
// Session credentials also live in the system keyring (primary
|
// Session credentials also live in the system keyring (primary
|
||||||
// store); drop that copy first so a file-side failure can't leave
|
// store); drop that copy first so a file-side failure can't leave
|
||||||
// the token behind.
|
// the token behind. Gated on `keyring_scoped`: only the default
|
||||||
|
// install owns the (global) keyring entry — a tempdir-rooted
|
||||||
|
// manager deleting it would log the real user out.
|
||||||
if scope == KIMI_CODE_OAUTH_SCOPE
|
if scope == KIMI_CODE_OAUTH_SCOPE
|
||||||
|
&& self.keyring_path_scoped
|
||||||
&& let Err(e) = keyring_delete_session()
|
&& let Err(e) = keyring_delete_session()
|
||||||
{
|
{
|
||||||
tracing::warn!(error = %e, "auth: failed to remove session credential from keyring");
|
tracing::warn!(error = %e, "auth: failed to remove session credential from keyring");
|
||||||
@@ -637,7 +682,7 @@ impl AuthManager {
|
|||||||
let update_started = std::time::Instant::now();
|
let update_started = std::time::Instant::now();
|
||||||
|
|
||||||
// Keyring first (PRD F1): the session credential's primary store.
|
// Keyring first (PRD F1): the session credential's primary store.
|
||||||
if self.scope == KIMI_CODE_OAUTH_SCOPE && keyring_enabled() {
|
if self.keyring_path_scoped && keyring_enabled() {
|
||||||
match keyring_write_session(&auth) {
|
match keyring_write_session(&auth) {
|
||||||
Ok(()) => {
|
Ok(()) => {
|
||||||
let elapsed_ms = update_started.elapsed().as_millis() as u64;
|
let elapsed_ms = update_started.elapsed().as_millis() as u64;
|
||||||
|
|||||||
@@ -328,15 +328,28 @@ mod keyring_integration {
|
|||||||
disable_mock_keyring_for_test, enable_mock_keyring_for_test, keyring_read_session,
|
disable_mock_keyring_for_test, enable_mock_keyring_for_test, keyring_read_session,
|
||||||
};
|
};
|
||||||
|
|
||||||
|
/// Tempdir manager pretending to be the default install (thread-local
|
||||||
|
/// test seam) so keyring behavior — including constructor-time keyring
|
||||||
|
/// reads — is exercisable against the mock keyring.
|
||||||
|
fn mgr_keyring_scoped() -> (tempfile::TempDir, Arc<AuthManager>) {
|
||||||
|
// The path scope is captured at construction, so force it first.
|
||||||
|
crate::auth::manager::set_test_force_keyring_path_scope(true);
|
||||||
|
let dir = tempfile::tempdir().unwrap();
|
||||||
|
let m = Arc::new(AuthManager::new(dir.path(), KimiCodeConfig::default()));
|
||||||
|
(dir, m)
|
||||||
|
}
|
||||||
|
|
||||||
struct MockKeyringGuard;
|
struct MockKeyringGuard;
|
||||||
impl MockKeyringGuard {
|
impl MockKeyringGuard {
|
||||||
fn enable() -> Self {
|
fn enable() -> Self {
|
||||||
enable_mock_keyring_for_test();
|
enable_mock_keyring_for_test();
|
||||||
|
crate::auth::manager::set_test_force_keyring_path_scope(true);
|
||||||
Self
|
Self
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
impl Drop for MockKeyringGuard {
|
impl Drop for MockKeyringGuard {
|
||||||
fn drop(&mut self) {
|
fn drop(&mut self) {
|
||||||
|
crate::auth::manager::set_test_force_keyring_path_scope(false);
|
||||||
disable_mock_keyring_for_test();
|
disable_mock_keyring_for_test();
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -347,7 +360,7 @@ mod keyring_integration {
|
|||||||
#[serial_test::serial(kigi_keyring)]
|
#[serial_test::serial(kigi_keyring)]
|
||||||
async fn update_prefers_keyring_and_logout_clears_it() {
|
async fn update_prefers_keyring_and_logout_clears_it() {
|
||||||
let _guard = MockKeyringGuard::enable();
|
let _guard = MockKeyringGuard::enable();
|
||||||
let (dir, m) = mgr();
|
let (dir, m) = mgr_keyring_scoped();
|
||||||
m.update(session("at-kr", "rt-kr", 3600, 3600))
|
m.update(session("at-kr", "rt-kr", 3600, 3600))
|
||||||
.await
|
.await
|
||||||
.unwrap();
|
.unwrap();
|
||||||
@@ -375,12 +388,52 @@ mod keyring_integration {
|
|||||||
));
|
));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Regression for the 2026-07-17 credential wipe: a manager rooted
|
||||||
|
/// OUTSIDE the default install (a tempdir — exactly what integration
|
||||||
|
/// tests construct) must never write to or DELETE the global keyring
|
||||||
|
/// entry. Before the path-scope guard, every `cargo test` run wiped the
|
||||||
|
/// developer's real login via `remove_scope`'s keyring delete.
|
||||||
|
#[tokio::test]
|
||||||
|
#[serial_test::serial(kigi_keyring)]
|
||||||
|
async fn tempdir_manager_never_touches_global_keyring() {
|
||||||
|
let _guard = MockKeyringGuard::enable();
|
||||||
|
// Seed the "real user's" credential via a default-scoped manager.
|
||||||
|
let (_scoped_dir, scoped) = mgr_keyring_scoped();
|
||||||
|
scoped
|
||||||
|
.update(session("at-real", "rt-real", 3600, 3600))
|
||||||
|
.await
|
||||||
|
.unwrap();
|
||||||
|
|
||||||
|
// A tempdir manager WITHOUT the path scope — the integration-test
|
||||||
|
// shape. The mock keyring stays enabled: only the path scope
|
||||||
|
// distinguishes it from the real install.
|
||||||
|
crate::auth::manager::set_test_force_keyring_path_scope(false);
|
||||||
|
let (dir, foreign) = mgr();
|
||||||
|
foreign
|
||||||
|
.update(session("at-foreign", "rt-foreign", 3600, 3600))
|
||||||
|
.await
|
||||||
|
.unwrap();
|
||||||
|
assert!(
|
||||||
|
dir.path().join("auth.json").exists(),
|
||||||
|
"foreign manager must write to its own file, not the keyring"
|
||||||
|
);
|
||||||
|
|
||||||
|
// Its logout must not destroy the global entry.
|
||||||
|
foreign.clear().unwrap();
|
||||||
|
match keyring_read_session() {
|
||||||
|
crate::auth::storage::KeyringRead::Found(auth) => {
|
||||||
|
assert_eq!(auth.key, "at-real", "real credential must survive");
|
||||||
|
}
|
||||||
|
other => panic!("global keyring entry destroyed by a tempdir manager: {other:?}"),
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/// A stale file copy left from fallback days is stripped on the next
|
/// A stale file copy left from fallback days is stripped on the next
|
||||||
/// keyring write, and the keyring copy wins on reads.
|
/// keyring write, and the keyring copy wins on reads.
|
||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
#[serial_test::serial(kigi_keyring)]
|
#[serial_test::serial(kigi_keyring)]
|
||||||
async fn keyring_write_strips_stale_file_copy() {
|
async fn keyring_write_strips_stale_file_copy() {
|
||||||
let (dir, m) = mgr();
|
let (dir, m) = mgr_keyring_scoped();
|
||||||
// Keyring disabled: first write lands in the file.
|
// Keyring disabled: first write lands in the file.
|
||||||
m.update(session("at-file", "rt-file", 3600, 3600))
|
m.update(session("at-file", "rt-file", 3600, 3600))
|
||||||
.await
|
.await
|
||||||
|
|||||||
Reference in New Issue
Block a user