Skip to content

fix(oauth2): Allow only asymmetric algorithms in profiles - #1642

Open
thinhphucnguyen wants to merge 2 commits into
openstack-experimental:mainfrom
thinhphucnguyen:profile-followup
Open

thinhphucnguyen wants to merge 2 commits into
openstack-experimental:mainfrom
thinhphucnguyen:profile-followup

Conversation

@thinhphucnguyen

Copy link
Copy Markdown
Contributor

Follow-up to #1639, addressing the review comments there:

  • crypto-inventory.md now says external profiles default to RS256 and ES256.
  • The HMAC denylist is replaced by a positive allowlist (RS*, PS*, ES*,
    EdDSA), so an algorithm added to jsonwebtoken later is refused until it is
    listed.
  • Added tests for an ES256 token against an RSA-only JWKS and an RS256 token
    against an EC-only JWKS. Both are rejected (InvalidKeyFormat), so no
    verifier logic changes.

Also covers HS384 in the existing HMAC test.

@gtema gtema left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A very small nit, otherwise fine

Comment thread crates/core/src/oauth2_client/verify.rs Outdated
|| profile.expected_issuers.iter().any(String::is_empty)
|| profile.allowed_algorithms.is_empty()
|| profile.allowed_algorithms.iter().copied().any(is_symmetric)
|| !profile

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change now uncovers another inconsistency - mismatch of the algo would result in InvalidProfile error with "must specify issuers and audiences" which is pretty misleading for the algo error. Let's please add another error kind (e.g., DisallowedAlgorithm)

Signed-off-by: Thinh Phuc Nguyen <nguyenthinhphuc1901@gmail.com>
Signed-off-by: Thinh Phuc Nguyen <nguyenthinhphuc1901@gmail.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants