[DNM] fix: Allow project_id in credential update patch - #1085
Draft
gtema wants to merge 1 commit into
Draft
Conversation
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 <noreply@anthropic.com>
Signed-off-by: Artem Goncharov <artem.goncharov@gmail.com>
gtema
marked this pull request as draft
July 24, 2026 07:14
|
🦢 Load Test Results Goose Attack ReportPlan Overview
Request Metrics
Response Time Metrics
Status Code Metrics
Transaction Metrics
Scenario Metrics
|
|
🧪 Tempest Identity Compatibility Results (advisory, non-blocking) rust Failed test IDs |
|
Bencher Report| Project | keystone |
| Branch | claude/github-issue-1044-dvbtyi |
| Testbed | ubuntu-latest |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result nanoseconds (ns) (Result Δ%) | Upper Boundary nanoseconds (ns) (Limit %) |
|---|---|---|---|
| Command_Serde/apply/remove | 📈 view plot 🚷 view threshold | 86,531.00 ns(-63.09%)Baseline: 234,440.47 ns | 787,620.58 ns (10.99%) |
| Command_Serde/apply/set | 📈 view plot 🚷 view threshold | 94,933.00 ns(-66.42%)Baseline: 282,733.00 ns | 1,265,870.25 ns (7.50%) |
| Command_Serde/pack/delete | 📈 view plot 🚷 view threshold | 125.39 ns(+4.74%)Baseline: 119.72 ns | 152.38 ns (82.29%) |
| Command_Serde/pack/delete_index | 📈 view plot 🚷 view threshold | 114.25 ns(+7.97%)Baseline: 105.81 ns | 133.18 ns (85.79%) |
| Command_Serde/pack/set | 📈 view plot 🚷 view threshold | 192.44 ns(+0.12%)Baseline: 192.21 ns | 245.60 ns (78.35%) |
| Command_Serde/pack/set_index | 📈 view plot 🚷 view threshold | 114.35 ns(+8.07%)Baseline: 105.82 ns | 132.80 ns (86.11%) |
| Command_Serde/unpack/delete | 📈 view plot 🚷 view threshold | 221.25 ns(+18.14%)Baseline: 187.28 ns | 251.48 ns (87.98%) |
| Command_Serde/unpack/delete_index | 📈 view plot 🚷 view threshold | 180.59 ns(+15.28%)Baseline: 156.65 ns | 205.46 ns (87.89%) |
| Command_Serde/unpack/set | 📈 view plot 🚷 view threshold | 282.62 ns(+7.15%)Baseline: 263.77 ns | 344.43 ns (82.05%) |
| Command_Serde/unpack/set_index | 📈 view plot 🚷 view threshold | 179.46 ns(+16.42%)Baseline: 154.15 ns | 202.68 ns (88.54%) |
| Payload_encryption/pack/remove_cmd | 📈 view plot 🚷 view threshold | 119.47 ns(+5.62%)Baseline: 113.11 ns | 145.02 ns (82.38%) |
| Payload_encryption/pack/set_cmd | 📈 view plot 🚷 view threshold | 193.16 ns(-2.12%)Baseline: 197.35 ns | 263.16 ns (73.40%) |
| Payload_encryption/unpack/remove_cmd | 📈 view plot 🚷 view threshold | 226.12 ns(+13.99%)Baseline: 198.36 ns | 267.87 ns (84.41%) |
| Payload_encryption/unpack/set_cmd | 📈 view plot 🚷 view threshold | 286.37 ns(+3.12%)Baseline: 277.71 ns | 367.45 ns (77.93%) |
| Raft_1Node_Latency/prefix/1node | 📈 view plot 🚷 view threshold | 4,681,200.00 ns(+76.97%)Baseline: 2,645,185.62 ns | 6,079,561.64 ns (77.00%) |
| Raft_1Node_Latency/read/1node | 📈 view plot 🚷 view threshold | 34,201.00 ns(-12.02%)Baseline: 38,872.00 ns | 54,612.64 ns (62.62%) |
| Raft_1Node_Latency/remove/1node | 📈 view plot 🚷 view threshold | 246,850.00 ns(-62.19%)Baseline: 652,827.19 ns | 2,515,025.93 ns (9.82%) |
| Raft_1Node_Latency/write/1node | 📈 view plot 🚷 view threshold | 271,910.00 ns(-61.40%)Baseline: 704,401.25 ns | 3,209,812.88 ns (8.47%) |
| build_snapshot/default | 📈 view plot 🚷 view threshold | 114,500.00 ns(-4.12%)Baseline: 119,419.84 ns | 176,740.09 ns (64.78%) |
| fernet token/project | 📈 view plot 🚷 view threshold | 1,553.60 ns(+10.08%)Baseline: 1,411.36 ns | 1,740.82 ns (89.25%) |
| get_data_keyspace | 📈 view plot 🚷 view threshold | 0.35 ns(+11.76%)Baseline: 0.31 ns | 0.38 ns (92.67%) |
| get_db | 📈 view plot 🚷 view threshold | 0.35 ns(+11.53%)Baseline: 0.32 ns | 0.38 ns (92.64%) |
| get_fernet_token_timestamp/project | 📈 view plot 🚷 view threshold | 152.03 ns(+7.32%)Baseline: 141.66 ns | 180.00 ns (84.46%) |
| get_keyspace | 📈 view plot 🚷 view threshold | 4.74 ns(-9.41%)Baseline: 5.23 ns | 11.51 ns (41.18%) |
gtema
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
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 noreply@anthropic.com
Signed-off-by: Artem Goncharov artem.goncharov@gmail.com