fix(audit-2026-06-11/revocation-lag): evict PrincipalCache on credential change
The PrincipalCache (token-hash -> resolved Principal, 60s TTL) had no eviction hook, so deactivation / password change / API-key revocation flipped the DB rows but the cache kept serving the stale principal for up to the TTL window. Closes the audit's PrincipalCache revocation-lag Medium and greens authz::deactivating_user_revokes_their_api_keys. - PrincipalCache::evict_user(user_id) + evict_token(hash). - One shared cache Arc threaded into AuthState / AdminsState / ApiKeysState (a per-state cache would let the middleware's own copy keep authenticating a revoked principal). - Evict on: deactivation, password change (both evict_user), logout (evict_token, precise), single API-key delete (evict_user). - H-E1 note: DB-write invalidation stays best-effort by design (a blip must not undo the rotation); the cache evict is what makes it take effect on the next request. - Renamed bearer_and_cookie_produce_same_principal -> session_token_and_api_key_produce_same_principal (cookie auth was removed in C-1; the test never tested cookies). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -45,6 +45,11 @@ pub struct AdminsState {
|
|||||||
/// Capability gate: every endpoint here requires
|
/// Capability gate: every endpoint here requires
|
||||||
/// `InstanceManageUsers` (owner / admin).
|
/// `InstanceManageUsers` (owner / admin).
|
||||||
pub authz: Arc<dyn AuthzRepo>,
|
pub authz: Arc<dyn AuthzRepo>,
|
||||||
|
/// Audit 2026-06-11 (PrincipalCache revocation-lag) — evicted on
|
||||||
|
/// password change / deactivation so a just-revoked session or
|
||||||
|
/// API key stops authenticating immediately instead of lingering
|
||||||
|
/// for the cache TTL.
|
||||||
|
pub principal_cache: Arc<crate::auth_middleware::PrincipalCache>,
|
||||||
}
|
}
|
||||||
|
|
||||||
pub fn admins_router(state: AdminsState) -> Router {
|
pub fn admins_router(state: AdminsState) -> Router {
|
||||||
@@ -254,6 +259,12 @@ async fn patch_admin(
|
|||||||
tracing::error!(?err, "failed to expire api keys on password change");
|
tracing::error!(?err, "failed to expire api keys on password change");
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
// Audit 2026-06-11 (PrincipalCache revocation-lag) — the DB
|
||||||
|
// writes above are best-effort by design (a blip must not undo
|
||||||
|
// the rotation), so evicting the in-process cache is what makes
|
||||||
|
// the rotation take effect on the very next request rather than
|
||||||
|
// after the cache TTL.
|
||||||
|
state.principal_cache.evict_user(id);
|
||||||
}
|
}
|
||||||
|
|
||||||
if let Some(email_patch) = input.email.as_ref() {
|
if let Some(email_patch) = input.email.as_ref() {
|
||||||
@@ -323,6 +334,11 @@ async fn patch_admin(
|
|||||||
tracing::error!(?err, "failed to expire api keys for deactivated admin");
|
tracing::error!(?err, "failed to expire api keys for deactivated admin");
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
// Audit 2026-06-11 (PrincipalCache revocation-lag) — evict
|
||||||
|
// cached principals so a deactivated admin's session / API
|
||||||
|
// keys stop authenticating immediately instead of lingering
|
||||||
|
// for the cache TTL window.
|
||||||
|
state.principal_cache.evict_user(id);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -39,6 +39,10 @@ const NAME_MAX: usize = 64;
|
|||||||
#[derive(Clone)]
|
#[derive(Clone)]
|
||||||
pub struct ApiKeysState {
|
pub struct ApiKeysState {
|
||||||
pub keys: Arc<dyn ApiKeyRepository>,
|
pub keys: Arc<dyn ApiKeyRepository>,
|
||||||
|
/// Audit 2026-06-11 (PrincipalCache revocation-lag) — evicted when
|
||||||
|
/// a user deletes one of their own keys so it stops authenticating
|
||||||
|
/// from cache immediately rather than after the cache TTL.
|
||||||
|
pub principal_cache: Arc<crate::auth_middleware::PrincipalCache>,
|
||||||
}
|
}
|
||||||
|
|
||||||
pub fn api_keys_router(state: ApiKeysState) -> Router {
|
pub fn api_keys_router(state: ApiKeysState) -> Router {
|
||||||
@@ -160,6 +164,12 @@ async fn delete_key(
|
|||||||
// we deliberately don't leak the distinction.
|
// we deliberately don't leak the distinction.
|
||||||
return Err(ApiKeysError::NotFound(id));
|
return Err(ApiKeysError::NotFound(id));
|
||||||
}
|
}
|
||||||
|
// Audit 2026-06-11 (PrincipalCache revocation-lag) — the cache is
|
||||||
|
// keyed by token hash, which we can't derive from the key id, so
|
||||||
|
// evict every cached principal for this user. That also drops their
|
||||||
|
// session entries; re-auth on the next request is the right cost for
|
||||||
|
// a deliberate key revocation.
|
||||||
|
state.principal_cache.evict_user(principal.user_id);
|
||||||
Ok(StatusCode::NO_CONTENT)
|
Ok(StatusCode::NO_CONTENT)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -195,6 +195,11 @@ async fn logout(State(state): State<AuthState>, req: Request<Body>) -> Response
|
|||||||
if let Err(err) = state.sessions.delete(&hash).await {
|
if let Err(err) = state.sessions.delete(&hash).await {
|
||||||
tracing::error!(?err, "admin_sessions delete failed");
|
tracing::error!(?err, "admin_sessions delete failed");
|
||||||
}
|
}
|
||||||
|
// Audit 2026-06-11 (PrincipalCache revocation-lag) — drop just
|
||||||
|
// this token from the cache so the logged-out session can't keep
|
||||||
|
// authenticating for the cache TTL. Other sessions for the same
|
||||||
|
// user are left untouched.
|
||||||
|
state.principal_cache.evict_token(&hash);
|
||||||
}
|
}
|
||||||
StatusCode::NO_CONTENT.into_response()
|
StatusCode::NO_CONTENT.into_response()
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -92,6 +92,32 @@ impl PrincipalCache {
|
|||||||
}
|
}
|
||||||
guard.insert(token_hash, (Instant::now(), principal));
|
guard.insert(token_hash, (Instant::now(), principal));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Evict every cached principal belonging to `user_id`.
|
||||||
|
///
|
||||||
|
/// Audit 2026-06-11 (PrincipalCache revocation-lag, Medium) — the
|
||||||
|
/// revocation paths (`set_active(false)`, password change,
|
||||||
|
/// API-key delete) flip the backing DB rows, but a cached principal
|
||||||
|
/// keeps authenticating for up to [`PRINCIPAL_CACHE_TTL`] unless it
|
||||||
|
/// is evicted. Callers invoke this right after the DB mutation so a
|
||||||
|
/// just-revoked credential stops working on the next request. The
|
||||||
|
/// cache value carries the resolved [`Principal`], so we can target
|
||||||
|
/// the user without knowing which token hashes belong to them.
|
||||||
|
pub fn evict_user(&self, user_id: AdminUserId) {
|
||||||
|
let Ok(mut guard) = self.inner.lock() else {
|
||||||
|
return;
|
||||||
|
};
|
||||||
|
guard.retain(|_, (_, p)| p.user_id != user_id);
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Evict a single cached entry by its token hash. Used on logout,
|
||||||
|
/// where exactly one session token is being invalidated and we
|
||||||
|
/// don't want to disturb the user's other live sessions.
|
||||||
|
pub fn evict_token(&self, token_hash: &str) {
|
||||||
|
if let Ok(mut guard) = self.inner.lock() {
|
||||||
|
guard.remove(token_hash);
|
||||||
|
}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Prefix on the wire that selects the API-key path. The body that
|
/// Prefix on the wire that selects the API-key path. The body that
|
||||||
@@ -471,4 +497,46 @@ mod tests {
|
|||||||
assert!(p.scopes.is_none());
|
assert!(p.scopes.is_none());
|
||||||
assert!(p.app_binding.is_none());
|
assert!(p.app_binding.is_none());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
fn principal_for(user_id: AdminUserId) -> Principal {
|
||||||
|
Principal {
|
||||||
|
user_id,
|
||||||
|
instance_role: InstanceRole::Owner,
|
||||||
|
scopes: None,
|
||||||
|
app_binding: None,
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn evict_user_drops_only_that_users_entries() {
|
||||||
|
// Audit 2026-06-11 (PrincipalCache revocation-lag).
|
||||||
|
let cache = PrincipalCache::new();
|
||||||
|
let alice = AdminUserId::new();
|
||||||
|
let bob = AdminUserId::new();
|
||||||
|
cache.insert("hash-alice-1".into(), principal_for(alice));
|
||||||
|
cache.insert("hash-alice-2".into(), principal_for(alice));
|
||||||
|
cache.insert("hash-bob-1".into(), principal_for(bob));
|
||||||
|
|
||||||
|
cache.evict_user(alice);
|
||||||
|
|
||||||
|
assert!(cache.get("hash-alice-1").is_none());
|
||||||
|
assert!(cache.get("hash-alice-2").is_none());
|
||||||
|
// Bob is untouched.
|
||||||
|
assert!(cache.get("hash-bob-1").is_some());
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn evict_token_drops_only_that_entry() {
|
||||||
|
let cache = PrincipalCache::new();
|
||||||
|
let alice = AdminUserId::new();
|
||||||
|
cache.insert("hash-1".into(), principal_for(alice));
|
||||||
|
cache.insert("hash-2".into(), principal_for(alice));
|
||||||
|
|
||||||
|
cache.evict_token("hash-1");
|
||||||
|
|
||||||
|
// Logout drops exactly one session; the user's other live
|
||||||
|
// session stays cached.
|
||||||
|
assert!(cache.get("hash-1").is_none());
|
||||||
|
assert!(cache.get("hash-2").is_some());
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -498,12 +498,19 @@ pub async fn build_app(
|
|||||||
.and_then(|s| s.parse::<usize>().ok())
|
.and_then(|s| s.parse::<usize>().ok())
|
||||||
.filter(|n| *n > 0)
|
.filter(|n| *n > 0)
|
||||||
.unwrap_or(2);
|
.unwrap_or(2);
|
||||||
|
// Audit 2026-06-11 (PrincipalCache revocation-lag) — one shared
|
||||||
|
// cache instance threaded into every state that performs auth or
|
||||||
|
// revocation, so an evict on the revocation side is visible to the
|
||||||
|
// middleware's resolve side. A per-state cache would let a revoked
|
||||||
|
// principal keep authenticating from the middleware's own copy.
|
||||||
|
let principal_cache =
|
||||||
|
Arc::new(picloud_manager_core::auth_middleware::PrincipalCache::new());
|
||||||
let auth_state = AuthState {
|
let auth_state = AuthState {
|
||||||
users: auth.users.clone(),
|
users: auth.users.clone(),
|
||||||
sessions: auth.sessions.clone(),
|
sessions: auth.sessions.clone(),
|
||||||
keys: auth.keys.clone(),
|
keys: auth.keys.clone(),
|
||||||
ttl: auth.ttl,
|
ttl: auth.ttl,
|
||||||
principal_cache: Arc::new(picloud_manager_core::auth_middleware::PrincipalCache::new()),
|
principal_cache: principal_cache.clone(),
|
||||||
login_rate_limiter: Arc::new(
|
login_rate_limiter: Arc::new(
|
||||||
picloud_manager_core::login_rate_limit::LoginRateLimiter::new(),
|
picloud_manager_core::login_rate_limit::LoginRateLimiter::new(),
|
||||||
),
|
),
|
||||||
@@ -514,6 +521,7 @@ pub async fn build_app(
|
|||||||
sessions: auth.sessions,
|
sessions: auth.sessions,
|
||||||
keys: auth.keys.clone(),
|
keys: auth.keys.clone(),
|
||||||
authz: authz.clone(),
|
authz: authz.clone(),
|
||||||
|
principal_cache: principal_cache.clone(),
|
||||||
};
|
};
|
||||||
let app_members_state = AppMembersState {
|
let app_members_state = AppMembersState {
|
||||||
apps: apps_state.apps.clone(),
|
apps: apps_state.apps.clone(),
|
||||||
@@ -526,7 +534,10 @@ pub async fn build_app(
|
|||||||
authz: authz.clone(),
|
authz: authz.clone(),
|
||||||
users: users.clone(),
|
users: users.clone(),
|
||||||
};
|
};
|
||||||
let api_keys_state = ApiKeysState { keys: auth.keys };
|
let api_keys_state = ApiKeysState {
|
||||||
|
keys: auth.keys,
|
||||||
|
principal_cache: principal_cache.clone(),
|
||||||
|
};
|
||||||
|
|
||||||
// /admin/auth/login + /logout are unguarded by design (login is how
|
// /admin/auth/login + /logout are unguarded by design (login is how
|
||||||
// you get in). /admin/auth/me applies the middleware internally so
|
// you get in). /admin/auth/me applies the middleware internally so
|
||||||
|
|||||||
@@ -380,12 +380,14 @@ async fn member_can_only_touch_apps_they_belong_to(pool: PgPool) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// ----------------------------------------------------------------------------
|
// ----------------------------------------------------------------------------
|
||||||
// 5. Bearer pic_ + cookie produce the same Principal
|
// 5. Session-token bearer + pic_ API-key bearer produce the same Principal
|
||||||
|
// (cookie auth was removed in the audit 2026-06-11 C-1 fix; both paths
|
||||||
|
// now travel through the Authorization header).
|
||||||
// ----------------------------------------------------------------------------
|
// ----------------------------------------------------------------------------
|
||||||
|
|
||||||
#[ignore = "needs DATABASE_URL pointing at a running Postgres"]
|
#[ignore = "needs DATABASE_URL pointing at a running Postgres"]
|
||||||
#[sqlx::test(migrations = "../manager-core/migrations")]
|
#[sqlx::test(migrations = "../manager-core/migrations")]
|
||||||
async fn bearer_and_cookie_produce_same_principal(pool: PgPool) {
|
async fn session_token_and_api_key_produce_same_principal(pool: PgPool) {
|
||||||
let s = boot(pool).await;
|
let s = boot(pool).await;
|
||||||
let session_token = login_token(&s.server, "owner", "owner-pw").await;
|
let session_token = login_token(&s.server, "owner", "owner-pw").await;
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user