From 842113f85470452e3fc095f757479ce4ffdea050 Mon Sep 17 00:00:00 2001 From: augustuswm Date: Fri, 17 Jul 2026 14:25:54 -0500 Subject: [PATCH] Restrict usage of boundary less token creation --- v-api/src/context/mod.rs | 4 + v-api/src/context/user.rs | 231 +++++++++++++++++++++++++++++++- v-api/src/endpoints/api_user.rs | 35 ++++- 3 files changed, 262 insertions(+), 8 deletions(-) diff --git a/v-api/src/context/mod.rs b/v-api/src/context/mod.rs index b3941b50..6408a326 100644 --- a/v-api/src/context/mod.rs +++ b/v-api/src/context/mod.rs @@ -281,6 +281,10 @@ impl From for HttpError { tracing::info!(?error, "Failed to authenticate caller"); match error { + UserContextError::BoundaryRequired => client_error( + ClientErrorStatusCode::BAD_REQUEST, + "permission boundary must be defined for a cross-user token", + ), UserContextError::FailedToAuthenticate => client_error( ClientErrorStatusCode::UNAUTHORIZED, "Failed to authenticate", diff --git a/v-api/src/context/user.rs b/v-api/src/context/user.rs index 7131a6b0..570e68f9 100644 --- a/v-api/src/context/user.rs +++ b/v-api/src/context/user.rs @@ -41,6 +41,8 @@ pub enum BasePermissions { #[derive(Debug, Error)] pub enum UserContextError { + #[error("permission boundary must be defined for a cross-user token")] + BoundaryRequired, #[error("Failed to authenticate caller")] FailedToAuthenticate, #[error("Supplied API key is invalid")] @@ -498,14 +500,20 @@ where caller: &Caller, token: NewApiKey, api_user_id: &TypedUuid, - ) -> ResourceResult, StoreError> { + ) -> ResourceResult, UserContextError> { let can_create = caller.can(&VPermission::CreateApiKey(*api_user_id).into()); + + // If the user is requesting a token for themselves, they can grant an unbounded token. + // If the request is for a different user, then a permission boundary must be provided. + let self_token = &caller.id == api_user_id; + let has_boundary = token.permission_boundary.is_some(); let can_grant = token .permission_boundary .as_ref() .is_none_or(|p| caller.can_grant_all(p)); + let authorized = has_boundary && can_grant || !has_boundary && self_token; - if can_create && can_grant { + if can_create && authorized { Ok(ApiKeyStore::upsert(&*self.storage, token).await?) } else { resource_restricted() @@ -679,3 +687,222 @@ where }) } } + +#[cfg(test)] +mod tests { + //! Tests for the authorization checks in [`UserContext::create_api_user_token`]. + //! + //! The guard being verified is: + //! + //! ```text + //! authorized = (has_boundary && caller.can_grant_all(boundary)) + //! || (!has_boundary && caller.id == api_user_id) + //! create allowed = caller.can(CreateApiKey(api_user_id)) && authorized + //! ``` + //! + //! In other words: + //! - Every explicit boundary must contain only permissions the caller can + //! grant, regardless of who the token is for. + //! - A boundary-less key (which resolves to `BasePermissions::Full` at + //! authentication) may only be created for oneself. + //! - `CreateApiKey` authority for the target is always required. + + use std::{collections::HashMap, sync::Arc}; + + use chrono::{TimeDelta, Utc}; + use newtype_uuid::TypedUuid; + use v_model::{ + ApiKey, NewApiKey, UserId, + permissions::{Caller, Permissions}, + storage::MockApiKeyStore, + }; + + use crate::{context::test_mocks::MockStorage, permissions::VPermission}; + + use super::UserContext; + + /// Build a `UserContext` whose token store echoes back whatever key is + /// upserted, so tests can observe the persisted `permission_boundary`. + fn user_context_with_token_store() -> UserContext { + let mut token_store = MockApiKeyStore::new(); + token_store.expect_upsert().returning(move |key| { + Ok(ApiKey { + id: TypedUuid::new_v4(), + user_id: key.user_id, + key_signature: key.key_signature, + permission_boundary: key.permission_boundary, + expires_at: key.expires_at, + created_at: Utc::now(), + updated_at: Utc::now(), + deleted_at: None, + }) + }); + + let mut storage = MockStorage::new(); + storage.api_user_token_store = Some(Arc::new(token_store)); + + UserContext::new(Arc::new(storage)) + } + + fn make_caller(id: TypedUuid, permissions: Vec) -> Caller { + Caller { + id, + permissions: permissions.into(), + extensions: HashMap::default(), + } + } + + fn make_key( + user_id: TypedUuid, + permission_boundary: Option>, + ) -> NewApiKey { + NewApiKey { + id: TypedUuid::new_v4(), + user_id, + key_signature: "key-signature".to_string(), + permission_boundary, + expires_at: Utc::now() + TimeDelta::try_days(1).unwrap(), + } + } + + /// A caller delegated only `CreateApiKey(target)` (and not the target's own + /// permissions) must NOT be able to mint a boundary-less key for another + /// user. A boundary-less key resolves to `BasePermissions::Full` at + /// authentication, so this would grant the target's full permission set. + #[tokio::test] + async fn boundaryless_token_for_other_user_is_rejected() { + let ctx = user_context_with_token_store(); + let target_id = TypedUuid::new_v4(); + + let attacker = make_caller( + TypedUuid::new_v4(), + vec![VPermission::CreateApiKey(target_id)], + ); + // Far-future expiry, boundary-less => Full permissions of the target. + let mut token = make_key(target_id, None); + token.expires_at = Utc::now() + TimeDelta::try_days(3650).unwrap(); + + let result = ctx + .create_api_user_token(&attacker, token, &target_id) + .await; + + assert!( + result.is_err(), + "privilege escalation: a caller who cannot grant the target user's permissions minted \ + a boundary-less (BasePermissions::Full) key for them; persisted boundary = {:?}", + result.map(|k| k.permission_boundary), + ); + } + + /// A caller must not place a permission they cannot grant into a key's + /// boundary, even when creating a token for another user. + #[tokio::test] + async fn ungrantable_boundary_for_other_user_is_rejected() { + let ctx = user_context_with_token_store(); + let target_id = TypedUuid::new_v4(); + + let attacker = make_caller( + TypedUuid::new_v4(), + vec![VPermission::CreateApiKey(target_id)], + ); + let token = make_key(target_id, Some(vec![VPermission::ManageApiUsersAll].into())); + + let result = ctx + .create_api_user_token(&attacker, token, &target_id) + .await; + + assert!( + result.is_err(), + "caller was able to grant a permission (ManageApiUsersAll) they do not hold", + ); + } + + /// The grant check applies even when creating a token for oneself: an + /// explicit boundary must still contain only permissions the caller can + /// grant. Only a *boundary-less* self-token is exempt from the grant check. + #[tokio::test] + async fn ungrantable_boundary_for_self_is_rejected() { + let ctx = user_context_with_token_store(); + let self_id = TypedUuid::new_v4(); + + let caller = make_caller(self_id, vec![VPermission::CreateApiKey(self_id)]); + let token = make_key(self_id, Some(vec![VPermission::ManageApiUsersAll].into())); + + let result = ctx.create_api_user_token(&caller, token, &self_id).await; + + assert!( + result.is_err(), + "caller escalated by granting themselves ManageApiUsersAll (which they do not hold) \ + via a self-token boundary", + ); + } + + /// `CreateApiKey` authority for the target is always required — even for a + /// boundary-less token for oneself. + #[tokio::test] + async fn missing_create_permission_is_rejected() { + let ctx = user_context_with_token_store(); + let self_id = TypedUuid::new_v4(); + + // No CreateApiKey permission at all. + let caller = make_caller(self_id, vec![]); + let token = make_key(self_id, None); + + let result = ctx.create_api_user_token(&caller, token, &self_id).await; + + assert!( + result.is_err(), + "a token was created without CreateApiKey authority for the target", + ); + } + + /// A user may mint an unbounded (`BasePermissions::Full`) token for + /// themselves. + #[tokio::test] + async fn boundaryless_self_token_is_allowed() { + let ctx = user_context_with_token_store(); + let self_id = TypedUuid::new_v4(); + + let caller = make_caller(self_id, vec![VPermission::CreateApiKey(self_id)]); + let token = make_key(self_id, None); + + let key = ctx + .create_api_user_token(&caller, token, &self_id) + .await + .expect("a user should be able to create a boundary-less token for themselves"); + + assert_eq!( + key.permission_boundary, None, + "self-service token should be persisted boundary-less", + ); + } + + /// A caller may create a bounded token for another user as long as they can + /// grant every permission in the boundary. + #[tokio::test] + async fn grantable_boundary_for_other_user_is_allowed() { + let ctx = user_context_with_token_store(); + let target_id = TypedUuid::new_v4(); + + let caller = make_caller( + TypedUuid::new_v4(), + vec![ + VPermission::CreateApiKey(target_id), + VPermission::GetApiUser(target_id), + ], + ); + let boundary: Permissions = vec![VPermission::GetApiUser(target_id)].into(); + let token = make_key(target_id, Some(boundary.clone())); + + let key = ctx + .create_api_user_token(&caller, token, &target_id) + .await + .expect("a bounded, grantable token for another user should be allowed"); + + assert_eq!( + key.permission_boundary, + Some(boundary), + "the persisted boundary should match the requested, grantable boundary", + ); + } +} diff --git a/v-api/src/endpoints/api_user.rs b/v-api/src/endpoints/api_user.rs index a222b929..0f16ca99 100644 --- a/v-api/src/endpoints/api_user.rs +++ b/v-api/src/endpoints/api_user.rs @@ -1301,6 +1301,10 @@ mod tests { user_id: TypedUuid::new_v4(), }; + let allowed_user_path = ApiUserPath { + user_id: TypedUuid::new_v4(), + }; + let new_token = ApiKeyCreateParams { permission_boundary: None, expires_at: Utc::now() + TimeDelta::try_seconds(5 * 60).unwrap(), @@ -1325,6 +1329,27 @@ mod tests { .expect_get() .with(eq(unknown_api_user_path.user_id), eq(false)) .returning(move |_, _| Ok(None)); + api_user_store + .expect_get() + .with(eq(allowed_user_path.user_id), eq(false)) + .returning(move |_, _| { + Ok(Some(ApiUserInfo { + user: ApiUser { + id: allowed_user_path.user_id, + permissions: vec![ + VPermission::GetApiUser(allowed_user_path.user_id), + VPermission::CreateApiKey(allowed_user_path.user_id), + ] + .into(), + groups: BTreeSet::new(), + created_at: Utc::now(), + updated_at: Utc::now(), + deleted_at: None, + }, + email: None, + providers: vec![], + })) + }); let mut api_user_provider_store = MockApiUserProviderStore::new(); api_user_provider_store .expect_list() @@ -1418,14 +1443,12 @@ mod tests { assert!(resp.is_err()); assert_eq!(get_status(&resp), StatusCode::NOT_FOUND); - let user4 = mock_user(); - // 4. Succeed in creating token let success_permissions = Caller { - id: user4.id, + id: allowed_user_path.user_id, permissions: vec![ - VPermission::GetApiUser(api_user_path.user_id), - VPermission::CreateApiKey(api_user_path.user_id), + VPermission::GetApiUser(allowed_user_path.user_id), + VPermission::CreateApiKey(allowed_user_path.user_id), ] .into(), extensions: HashMap::default(), @@ -1434,7 +1457,7 @@ mod tests { let resp = create_api_user_token_inner( &ctx, success_permissions, - api_user_path, + allowed_user_path, new_token.clone(), ) .await;