From 54922df73b32453cf139d1501b8c848ee18eddb8 Mon Sep 17 00:00:00 2001 From: Artem Goncharov Date: Fri, 24 Jul 2026 08:50:54 +0200 Subject: [PATCH] fix: Allow project_id in credential update patch CredentialUpdate (api-types and core-types) was missing the project_id field despite ADR 0019 and identity/credential/update.rego documenting it as updatable. A PATCH /v3/credentials/{id} carrying project_id was silently ignored end-to-end (API type had no field to deserialize it into, so the SQL driver never wrote it), which is why tempest's test_credentials_create_get_update_delete failed against keystone-rs (issue #1044): it updates project_id and asserts the new value sticks. Plumb project_id through CredentialUpdate in both wire and provider types, the API-to-provider conversion, and the SQL driver's update query. Add regression coverage at the SQL-driver, handler-mock, and real-OPA-policy layers, including the delegation-boundary denial (moves_project_out_of_scope) that was previously untestable because the patch could never carry project_id. - Allow EC2 access value to change on update The provider layer rejected any change to blob.access on credential update, citing CVE-2020-12691, breaking tempest's test_credentials_create_get_update_delete (issue #1044): it updates a credential's access/secret pair and expects 200 OK. keystone-py's own update handler (keystone/api/credentials.py::_validate_blob_update_keys) only enforces immutability on trust_id/app_cred_id/access_token_id/ access_id -- and access_id is not a real blob key anywhere in keystone-py (the EC2 access value is stored under access), so that part of the check has always been a permanent no-op upstream. keystone-py has therefore always allowed access to change freely on update, leaving the credential's SHA-256-derived id (computed once at Create) unchanged -- a known quirk, not a security control. Drop the Rust-only "access immutable" guard to match this verified upstream behavior, keeping the trust_id/app_cred_id/access_token_id delegation-metadata guard intact. Update ADR 0019 and the handler docstring accordingly, and replace the stale rejection test with one asserting access can change. Assisted-by: Claude Signed-off-by: Artem Goncharov --- crates/api-types/src/v3/credential.rs | 6 + crates/api-types/src/v3/credential_conv.rs | 1 + .../core-types/src/credential/credential.rs | 5 + crates/core/src/credential/service.rs | 37 +++-- .../src/credential/update.rs | 26 +++ .../keystone/src/api/v3/credential/update.rs | 150 ++++++++++++++++-- doc/src/adr/0019-credentials.md | 14 +- 7 files changed, 210 insertions(+), 29 deletions(-) diff --git a/crates/api-types/src/v3/credential.rs b/crates/api-types/src/v3/credential.rs index 0e57a0646..f38778fcb 100644 --- a/crates/api-types/src/v3/credential.rs +++ b/crates/api-types/src/v3/credential.rs @@ -159,6 +159,12 @@ pub struct CredentialUpdate { #[serde(skip_serializing_if = "Option::is_none")] pub blob: Option, + /// New project association. + #[cfg_attr(feature = "builder", builder(default))] + #[serde(skip_serializing_if = "Option::is_none")] + #[cfg_attr(feature = "validate", validate(length(max = 64)))] + pub project_id: Option, + /// New credential type. #[cfg_attr(feature = "builder", builder(default))] #[serde(skip_serializing_if = "Option::is_none")] diff --git a/crates/api-types/src/v3/credential_conv.rs b/crates/api-types/src/v3/credential_conv.rs index 59df8247c..231cad3d2 100644 --- a/crates/api-types/src/v3/credential_conv.rs +++ b/crates/api-types/src/v3/credential_conv.rs @@ -61,6 +61,7 @@ impl From for provider_types::CredentialUpda let credential = value.credential; Self { blob: credential.blob, + project_id: credential.project_id, r#type: credential.r#type, } } diff --git a/crates/core-types/src/credential/credential.rs b/crates/core-types/src/credential/credential.rs index c24f2c7ce..c4978062d 100644 --- a/crates/core-types/src/credential/credential.rs +++ b/crates/core-types/src/credential/credential.rs @@ -110,6 +110,11 @@ pub struct CredentialUpdate { #[builder(default)] pub blob: Option, + /// New project association. + #[builder(default)] + #[validate(length(max = 64))] + pub project_id: Option, + /// New credential type. #[builder(default)] #[validate(length(min = 1, max = 255))] diff --git a/crates/core/src/credential/service.rs b/crates/core/src/credential/service.rs index cc1f75621..1c1e53775 100644 --- a/crates/core/src/credential/service.rs +++ b/crates/core/src/credential/service.rs @@ -30,10 +30,6 @@ use crate::credential::{CredentialApi, CredentialProviderError, backend::Credent use crate::events::AuditDispatchError; use crate::plugin_manager::PluginManagerApi; -/// The EC2 `blob` field that is immutable on update and must always be -/// present and unchanged (ADR 0019 §2, Update). -const IMMUTABLE_ACCESS_FIELD: &str = "access"; - /// Delegation-metadata fields inside the EC2 `blob` (ADR 0019 §1). These are /// never client-settable: they are derived server-side from the creating /// request's own [`AuthenticationContext`] (OSSA-2026-005 / CVE-2026-33551) @@ -264,12 +260,6 @@ impl CredentialApi for CredentialService { let old_val: Value = serde_json::from_str(&existing.blob)?; let mut new_val: Value = serde_json::from_str(new_blob)?; - if old_val.get(IMMUTABLE_ACCESS_FIELD) != new_val.get(IMMUTABLE_ACCESS_FIELD) { - return Err(CredentialProviderError::ImmutableField( - IMMUTABLE_ACCESS_FIELD.to_string(), - )); - } - // Delegation metadata is server-managed (see // `stamp_ec2_delegation_metadata`): the caller's patch is not // expected to resupply it, so a missing field carries the @@ -505,8 +495,17 @@ mod tests { assert_eq!(created.id, expected_id); } + /// Regression test (GitHub issue #1044): keystone-py's update handler + /// (`keystone/api/credentials.py::_validate_blob_update_keys`) only + /// actually enforces immutability on `trust_id`/`app_cred_id`/ + /// `access_token_id`/`access_id` -- and `access_id` is not a real blob + /// key (the EC2 access value is stored under `access`), so that check is + /// permanently a no-op and keystone-py allows `access` to change freely + /// on update, with the credential's `id` (computed once at create time) + /// left as-is. Tempest's `test_credentials_create_get_update_delete` + /// relies on exactly this and fails if `access` is rejected here. #[tokio::test] - async fn test_update_rejects_change_to_immutable_access_field() { + async fn test_update_allows_changing_access_field() { let state = get_mocked_state(None, None).await; let mut backend = MockCredentialBackend::default(); backend.expect_get_credential().returning(|_, _| { @@ -519,6 +518,16 @@ mod tests { extra: None, })) }); + backend.expect_update_credential().returning(|_, id, _| { + Ok(Credential { + id: id.to_string(), + user_id: "user_id".into(), + project_id: Some("project_id".into()), + blob: r#"{"access":"AKIA_NEW","secret":"s"}"#.into(), + r#type: "ec2".into(), + extra: None, + }) + }); let provider = create_provider(backend); let rec = CredentialUpdate { @@ -526,11 +535,11 @@ mod tests { ..Default::default() }; - let err = provider + let updated = provider .update_credential(&ExecutionContext::internal(&state), "cred_id", rec) .await - .unwrap_err(); - assert!(matches!(err, CredentialProviderError::ImmutableField(f) if f == "access")); + .unwrap(); + assert_eq!(updated.blob, r#"{"access":"AKIA_NEW","secret":"s"}"#); } #[tokio::test] diff --git a/crates/credential-driver-sql/src/credential/update.rs b/crates/credential-driver-sql/src/credential/update.rs index e9ebb0e67..2731770e1 100644 --- a/crates/credential-driver-sql/src/credential/update.rs +++ b/crates/credential-driver-sql/src/credential/update.rs @@ -46,6 +46,9 @@ pub async fn update( if let Some(new_type) = rec.r#type { active.r#type = Set(new_type); } + if let Some(new_project_id) = rec.project_id { + active.project_id = Set(Some(new_project_id)); + } if let Some(new_blob) = rec.blob { let repo = FernetKeyRepository::new(cfg.credential.key_repository.clone()); let keys = repo.load(cfg.credential.insecure_allow_null_key).await?; @@ -127,6 +130,7 @@ mod tests { let rec = CredentialUpdate { blob: None, + project_id: None, r#type: Some("ec2".into()), }; let updated = update(&cfg, &db, "cred-1", rec).await.unwrap(); @@ -134,6 +138,27 @@ mod tests { assert_eq!(updated.blob, "original-secret"); } + #[tokio::test] + async fn test_update_project_id() { + let key_dir = tempfile::tempdir().unwrap(); + let cfg = test_config(key_dir.path()); + let db = test_db().await; + FernetKeyRepository::new(key_dir.path().to_path_buf()) + .setup() + .await + .unwrap(); + insert_credential(&cfg, &db, "cred-1", b"original-secret").await; + + let rec = CredentialUpdate { + blob: None, + project_id: Some("new-project".into()), + r#type: None, + }; + let updated = update(&cfg, &db, "cred-1", rec).await.unwrap(); + assert_eq!(updated.project_id.as_deref(), Some("new-project")); + assert_eq!(updated.blob, "original-secret"); + } + #[tokio::test] async fn test_update_blob_reencrypts_with_current_primary_key() { let key_dir = tempfile::tempdir().unwrap(); @@ -147,6 +172,7 @@ mod tests { let rec = CredentialUpdate { blob: Some("new-secret".into()), + project_id: None, r#type: None, }; let updated = update(&cfg, &db, "cred-1", rec).await.unwrap(); diff --git a/crates/keystone/src/api/v3/credential/update.rs b/crates/keystone/src/api/v3/credential/update.rs index a92f89dca..9944a3d4c 100644 --- a/crates/keystone/src/api/v3/credential/update.rs +++ b/crates/keystone/src/api/v3/credential/update.rs @@ -28,9 +28,12 @@ use openstack_keystone_core::auth::ExecutionContext; /// Update an existing credential. /// -/// Immutable fields (`user_id`, and — within `blob` — `access`, `trust_id`, +/// Immutable fields (`user_id`, and — within `blob` — `trust_id`, /// `app_cred_id`, `access_token_id`) are enforced by the provider layer -/// (CVE-2020-12691); this handler only wires the request through. +/// (CVE-2020-12691); this handler only wires the request through. The EC2 +/// `access` value itself is *not* immutable (matches keystone-py, see +/// `crates/core/src/credential/service.rs`), even though it no longer +/// matches the credential's SHA-256-derived `id` after such a change. #[utoipa::path( patch, path = "/{credential_id}", @@ -281,6 +284,91 @@ mod tests { assert_eq!(updated.credential.id, "bar"); } + /// Regression test for the tempest gap (`test_credentials_create_get_ + /// update_delete`, GitHub issue #1044): `project_id` in the PATCH body + /// must actually reach the provider's `CredentialUpdate` and come back + /// in the response, not silently get dropped. + #[tokio::test] + async fn test_update_project_id() { + let mut credential_mock = MockCredentialProvider::default(); + + credential_mock + .expect_get_credential() + .withf(|_, id: &'_ str| id == "bar") + .returning(|_, _| { + Ok(Some( + CredentialBuilder::default() + .id("bar") + .blob(r#"{"access":"AKIA","secret":"OLD"}"#) + .r#type("ec2") + .user_id("uid") + .project_id("p1") + .build() + .unwrap(), + )) + }); + + credential_mock + .expect_update_credential() + .withf( + |_, + id: &'_ str, + rec: &openstack_keystone_core_types::credential::CredentialUpdate| { + id == "bar" && rec.project_id.as_deref() == Some("p2") + }, + ) + .returning(|_, _, _| { + Ok(CredentialBuilder::default() + .id("bar") + .blob(r#"{"access":"AKIA","secret":"NEW"}"#) + .r#type("ec2") + .user_id("uid") + .project_id("p2") + .build() + .unwrap()) + }); + + let vsc = test_fixture_scoped(); + let state = get_mocked_state( + Provider::mocked_builder().mock_credential(credential_mock), + true, + None, + ) + .await; + + let mut api = openapi_router() + .layer(TraceLayer::new_for_http()) + .with_state(state); + + let update_req = CredentialUpdateRequest { + credential: CredentialUpdateBuilder::default() + .blob(r#"{"access":"AKIA","secret":"NEW"}"#) + .project_id("p2") + .build() + .unwrap(), + }; + + let response = api + .as_service() + .oneshot( + Request::builder() + .method("PATCH") + .header(http::header::CONTENT_TYPE, "application/json") + .uri("/bar") + .extension(vsc) + .body(Body::from(serde_json::to_string(&update_req).unwrap())) + .unwrap(), + ) + .await + .unwrap(); + + assert_eq!(response.status(), StatusCode::OK); + + let body = response.into_body().collect().await.unwrap().to_bytes(); + let updated: ApiCredentialResponse = serde_json::from_slice(&body).unwrap(); + assert_eq!(updated.credential.project_id.as_deref(), Some("p2")); + } + #[tokio::test] async fn test_update_not_found() { let mut credential_mock = MockCredentialProvider::default(); @@ -380,17 +468,30 @@ mod tests { async fn update_request( vsc: ValidatedSecurityContext, provider_builder: ProviderBuilder, + ) -> StatusCode { + update_request_with_patch(vsc, provider_builder, None).await + } + + /// Like `update_request`, but lets the patch carry a `project_id` -- + /// the only way to drive `moves_project_out_of_scope` + /// (`policy/credential/update.rego`) through this handler. + async fn update_request_with_patch( + vsc: ValidatedSecurityContext, + provider_builder: ProviderBuilder, + patch_project_id: Option<&str>, ) -> StatusCode { let (state, _opa_guard) = get_state_with_real_policy(provider_builder).await; let mut api = openapi_router() .layer(TraceLayer::new_for_http()) .with_state(state); + let mut builder = CredentialUpdateBuilder::default(); + builder.blob(r#"{"seed":"NEW"}"#); + if let Some(project_id) = patch_project_id { + builder.project_id(project_id); + } let update_req = CredentialUpdateRequest { - credential: CredentialUpdateBuilder::default() - .blob(r#"{"seed":"NEW"}"#) - .build() - .unwrap(), + credential: builder.build().unwrap(), }; api.as_service() @@ -429,12 +530,7 @@ mod tests { } /// OSSA-2026-015: a delegated caller updating a credential already - /// bound to its own delegation project is allowed. Note: - /// `moves_project_out_of_scope` in the policy is currently - /// unreachable via this handler -- `CredentialUpdate` - /// (`crates/api-types/src/v3/credential.rs`) has no `project_id` - /// field, so the update patch this handler builds can never carry - /// one for the policy to inspect. + /// bound to its own delegation project is allowed. #[tokio::test] async fn delegated_caller_updating_credential_bound_to_own_project_is_allowed() { let status = update_request( @@ -456,5 +552,35 @@ mod tests { .await; assert_eq!(status, StatusCode::FORBIDDEN); } + + /// OSSA-2026-015: a delegated caller must not be able to use + /// `project_id` in the patch to move a credential it owns out of the + /// delegation's own project (`moves_project_out_of_scope` in + /// `policy/credential/update.rego`). + #[tokio::test] + async fn delegated_caller_moving_credential_out_of_own_project_is_denied() { + let status = update_request_with_patch( + restricted_app_cred_vsc("u1", "p1"), + provider_with_credential("u1", Some("p1")), + Some("p2"), + ) + .await; + assert_eq!(status, StatusCode::FORBIDDEN); + } + + /// A non-delegated caller updating their own credential may set + /// `project_id` in the patch -- this is the regression covered by + /// tempest's `test_credentials_create_get_update_delete`, which + /// updates a credential's `project_id` and expects it to stick. + #[tokio::test] + async fn owner_updating_own_credential_project_id_is_allowed() { + let status = update_request_with_patch( + member_vsc("u1", "p1", &["member"]), + provider_with_credential("u1", None), + Some("p2"), + ) + .await; + assert_eq!(status, StatusCode::OK); + } } } diff --git a/doc/src/adr/0019-credentials.md b/doc/src/adr/0019-credentials.md index 5e4af52cb..9c5c2f210 100644 --- a/doc/src/adr/0019-credentials.md +++ b/doc/src/adr/0019-credentials.md @@ -255,9 +255,17 @@ escape hatch. - **Updatable**: `type`, `blob`, `project_id`. - **Immutable**: `user_id` and `project_id` may not be changed to point at a user or project the acting user has no access to (CVE-2020-12691). Within the - `blob`, the following fields are additionally immutable: `access` (the EC2 - access key — changing it would desynchronize the record from its - SHA-256-derived `id`), `trust_id`, `app_cred_id`, and `access_token_id`. + `blob`, the following fields are additionally immutable: `trust_id`, + `app_cred_id`, and `access_token_id`. The EC2 access key (`blob.access`) is + **not** immutable, matching keystone-py's actual behaviour: its update + handler only ever enforces immutability on `trust_id`/`app_cred_id`/ + `access_token_id`/`access_id`, and `access_id` is not a real blob field (the + access value is stored under `access`), so that part of the check is a + permanent no-op upstream. Changing `access` leaves the credential's + SHA-256-derived `id` unchanged (it is computed once at Create and never + recomputed), so the row becomes reachable via the *old* access value's hash + with the *new* access value in its blob -- a known keystone-py quirk, not a + Rust-specific behaviour. - **Process**: Updating the `blob` triggers automatic re-encryption with the current Primary Key and updates `key_hash`.