Repository navigation
Conversation
Let a subject-token validation profile list the JWT algorithms it accepts instead of pinning one through a separate argument of verify_subject_token_jwt. Native profiles keep their single configured algorithm; external profiles default to RS256 and ES256 and can be widened per issuer with with_allowed_algorithms. The header alg is checked against the list before any key lookup. An empty list, or one that contains an HMAC algorithm, is rejected as an invalid profile. Part of openstack-experimental#1636. SecurityImpact Signed-off-by: Thinh Phuc Nguyen <nguyenthinhphuc1901@gmail.com>
Signed-off-by: Thinh Phuc Nguyen <nguyenthinhphuc1901@gmail.com>
gtema
left a comment
gtema
left a comment
There was a problem hiding this comment.
There are some nits with potential improvements/hardenings, but I am fine if you address them in the follow-up.
There was a problem hiding this comment.
https://github.com/openstack-experimental/keystone/blob/main/doc/src/contributor/crypto-inventory.md should be updated stating RS256/ES256 are used as default
| fn is_symmetric(algorithm: Algorithm) -> bool { | ||
| matches!( | ||
| algorithm, | ||
| Algorithm::HS256 | Algorithm::HS384 | Algorithm::HS512 |
There was a problem hiding this comment.
There is a theoretical gap here, if a new HS algorithm is added it would slip through. Instead a positive allowlist (RS*, PS*, ES*, EdDSA) would be safer
| /// A profile with exactly one algorithm reports a mismatch with | ||
| /// [`TokenVerificationError::AlgorithmMismatch`], as the native profiles | ||
| /// always have. | ||
| fn resolve_profile_decoding_key( |
There was a problem hiding this comment.
resolve_profile_decoding_key checks alg header against the profile's list, decoding_key_for finds JWK by kid and calls DecodingKey::from_jwk(jwk), verify_subject_token_jwt calls Validation::new(alg) with the header alg. The issue is that neither of those is comparing the JWK's own kty or alg with the header alg. A token can claim ES256, while it's kid points at an RSA key. The crates/jwt-crypto/src/backend.rs:new_verifier does the check, but we should have a test verifying this behavior (rejection on alg mismatch in this particular flow) to prevent regressions. So add the two tests to the PR or a follow-up: ES256 token against an RSA-only JWKS, and RS256 token against an EC-only JWKS. Both should return an error.
f26e4da
Summary
Step 1 of #1636. A subject-token validation profile now carries the JWT algorithms it accepts.
with_allowed_algorithmssets the list for an issuer.algis checked against the list before key lookup. An algorithm outside the list is rejected withAlgorithmNotAllowed. With a single-entry list the existingAlgorithmMismatcherror is kept.verify_subject_token_jwtno longer takesexpected_algorithm. The native constructors take the signing algorithm instead.The identity provider has no algorithm list yet, so nothing sets a non-default list in this PR. That comes with the verifier switch in step 3.
Test plan
alg: none, HS256 and an out-of-list algorithm are rejected. PS256 and EdDSA are accepted when configured. An empty list or an HMAC entry is an invalid profile. The default list is RS256 and ES256.cargo test -p openstack-keystone-core --lib oauth2_clientcargo clippy -p openstack-keystone-core --all-targetsSecurity review checklist
This changes token verification: which
algvalues a profile accepts. Native behaviour and errors are unchanged. Nothing reads scope, delegation, credentials or OPA input, so the checklist items are N/A.