[thinhphucnguyen]

Summary

Adds the pieces to verify an external JWT (the subject_token of the token exchange, part of #945 / #1018) against its issuer.

• extract_issuer reads iss from the unverified payload. It is only used to pick an entry from the trusted issuers, verify_jwt still checks the signed iss.
• verify_external_jwt checks the issuer against the trusted list before any network call. The keys come from the issuer's jwks_url if it has one, otherwise from OIDC discovery. An empty audience list is rejected.
• New OidcError variants: MalformedToken, MissingIssuer, UntrustedIssuer and AudienceRequired.

Nothing calls verify_external_jwt yet, it will be wired into the token exchange endpoint next, so it is #[allow(dead_code)] for now.

Done as part of the BU EC528 class project (ec528-fall26/keystone-ng#6).

Test plan

21 new unit tests in oidc_utils: valid token, tampered payload and signature, wrong issuer, untrusted issuer, missing or non-string iss, malformed tokens, empty audience list, static jwks_url preferred over discovery, discovery issuer mismatch, and no network call for an untrusted issuer.

• cargo test -p openstack-keystone --lib federation::api::oidc_utils: 63 passed, 0 failed

Security review checklist

Required if this diff touches authentication, scope, delegation, tokens,
credentials, EC2, trusts, application credentials, or OPA policy input.
Delete this section entirely if it doesn't apply. See
doc/src/contributor/security-model.md
§7 for the full context behind each item.

This adds JWT signature verification only. It does not read or build scope, delegation, Credentials or OPA input, and nothing calls it yet. Answers below are N/A for that reason.

• Does any delegation/authorization decision read the scope
  (project_id, ScopeInfo) where it should read the chain
  (authentication_context(), delegation object)? (I1/I2)
  N/A, no scope or delegation is read.
• New delegation-sensitive policy rule? Is the fact it needs projected
  onto Credentials from the chain, and does the rule anchor on
  delegated_project_id + carry the scope-drift tripwire? (I2/I3)
  N/A, no policy changes.
• New scope shape or redemption path for a delegated auth? Are effective
  roles still bounded by the delegation? Added a test that a restricted
  delegation cannot exceed its roles via the new path? (I4)
  N/A, no new scope shape or redemption path.
• New ScopeInfo variant or auth method? Updated
  validate_scope_boundaries(), calculate_effective_roles(),
  fully_resolved(), and Credentials::try_from — all without adding a
  wildcard _ => arm? (I5, Gate J)
  N/A, no new variant or auth method. The only new match arms are in From<OidcError> for KeystoneApiError.
• New lookup by a client-derivable key (like sha256(access))? Does it
  re-assert type/shape after fetch? (I6)
  N/A. The issuer is client-supplied, but it is matched against an allowlist before any lookup or network call.
• Does any new data reaching OPA include secrets/decrypted blobs? (I7,
  Gate I)
  N/A, nothing reaches OPA.
• New list/collection endpoint? Does it re-check each item with the
  per-item read policy? (I8)
  N/A, no endpoint.
• Same endpoint: can an unprivileged caller request the whole collection
  and leave the per-item pass to sort it out? Is the value the
  collection policy gates on the same one the driver actually filters
  by? (I8a)
  N/A, no endpoint.
• Does the change let a narrow auth method be broadened by a
  request-supplied scope? (I5)
  N/A, no scope handling.
• Are there negative tests proving the escape is blocked, not just
  positive tests proving the happy path works?
  Yes, for the checks this code adds: tampered payload and signature, wrong issuer, untrusted issuer, missing or non-string iss, malformed tokens.
• Does the test drive ValidatedSecurityContext::new_for_scope()
  end-to-end, not just the inner helper (calculate_effective_roles,
  validate_scope_boundaries) in isolation?
  N/A, no security context is built here. This will matter when the token exchange endpoint is wired in.

[gtema]

Questions:

* Where should the trusted issuer list come from? My guess is `bound_issuer` on the identity provider record.

yes, bound_issuer is the natural source

* `verify_jwt` requires `sub` and `aud`. Is that right for RFC 8693 subject tokens?

the RFC does not require sub/aud on the subject token, but we need them: sub is necessary for the mapping, aud is safer. We definitely should ensure aud doesn't get optional by accident

* Should this use discovery or the static `jwks_url` / `jwt_validation_pubkeys` on the IdP?

should support both with preferring static keys to save a roundtrip

[gtema]

accepting here the empty audiences results in a complete skip audience verification (see build_validation above). For the token exchange this is the confused-deputy vulnerabilities. Let's through an error when audiences is empty to prevent own mistakes

[thinhphucnguyen]

Got it! I changed it so that verify_external_jwt now returns an AudienceRequired error instead of skipping the aud check. A test is also written to check it.

[gtema]

Perhaps it is more logical to use the trusted issuer rather than simply the issuer as a claim from the token. We do verify it above, so it looks more sane to use the verified/trusted issuer

[thinhphucnguyen]

Fixed. verify_jwt now gets the allowlisted issuer, not the string from the token.

[gtema]

let's check for JwtDecode error here explicitly. The tampered-n approach is also fragile, because a bad modulus may fail in DecodingKey::from_jwk rather than at signature verification.

[thinhphucnguyen]

I changed it so that the it now uses a valid second RSA key under the same kid and asserts JwtDecode, so it can’t pass because of a bad modulus.

[gtema]

you asked whether it should implement validation with static keys only, or also with discovery. The code now only implements static keys, so you should either change the PR description or also implement discovery

[gtema]

you need to drop "merge" commit, rebase the branch and then force-push. Merge commits are failing the commit message check and should not be used since they break change logs. Unfortunately there is no way to disable it in the UI

[thinhphucnguyen]

you asked whether it should implement validation with static keys only, or also with discovery. The code now only implements static keys, so you should either change the PR description or also implement discovery

Thank you for the feedback! Both are are in. In the code, verify_external_jwt uses the issuer's jwks_url if it has one, and if not it does discovery (the match on trusted.jwks_url in oidc_utils.rs). There's a test for each path.

I also changed the dead_code attributes back to allow, added the blank line in error.rs, and rebased so the merge commit is gone.

[gtema]

so is the discovery of the jwks deferred to a separate PR? If so please state this in the PR

[thinhphucnguyen]

so is the discovery of the jwks deferred to a separate PR? If so please state this in the PR

It's not deferred, it's in this PR. verify_external_jwt uses the issuer's jwks_url if it has one, and if not it runs discovery on the issuer and takes the keys from jwks_uri (the match on trusted.jwks_url). verify_external_jwt_prefers_static_jwks_url_over_discovery covers the static path, and verify_external_jwt_accepts_valid_token goes through discovery.

If you meant something else, tell me which line and I'll take a look asap!

[gtema]

ah right, I missed that from previous reviews. Now we have a potential issue with caching (or technically with lack hereof), but that is something for the followup.