Skip to content
Draft
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
6 changes: 6 additions & 0 deletions crates/api-types/src/v3/credential.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,12 @@ pub struct CredentialUpdate {
#[serde(skip_serializing_if = "Option::is_none")]
pub blob: Option<String>,

/// 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<String>,

/// New credential type.
#[cfg_attr(feature = "builder", builder(default))]
#[serde(skip_serializing_if = "Option::is_none")]
Expand Down
1 change: 1 addition & 0 deletions crates/api-types/src/v3/credential_conv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ impl From<api_types::CredentialUpdateRequest> for provider_types::CredentialUpda
let credential = value.credential;
Self {
blob: credential.blob,
project_id: credential.project_id,
r#type: credential.r#type,
}
}
Expand Down
5 changes: 5 additions & 0 deletions crates/core-types/src/credential/credential.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,11 @@ pub struct CredentialUpdate {
#[builder(default)]
pub blob: Option<String>,

/// New project association.
#[builder(default)]
#[validate(length(max = 64))]
pub project_id: Option<String>,

/// New credential type.
#[builder(default)]
#[validate(length(min = 1, max = 255))]
Expand Down
37 changes: 23 additions & 14 deletions crates/core/src/credential/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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(|_, _| {
Expand All @@ -519,18 +518,28 @@ 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 {
blob: Some(r#"{"access":"AKIA_NEW","secret":"s"}"#.into()),
..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]
Expand Down
26 changes: 26 additions & 0 deletions crates/credential-driver-sql/src/credential/update.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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?;
Expand Down Expand Up @@ -127,13 +130,35 @@ 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();
assert_eq!(updated.r#type, "ec2");
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();
Expand All @@ -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();
Expand Down
150 changes: 138 additions & 12 deletions crates/keystone/src/api/v3/credential/update.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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}",
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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(
Expand All @@ -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);
}
}
}
14 changes: 11 additions & 3 deletions doc/src/adr/0019-credentials.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`.

Expand Down
Loading