Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add TryAllDecryptionKeys flag to whether decrypt if no key IDs match #3128

Merged
merged 1 commit into from
Feb 19, 2025

Conversation

pmaytak
Copy link
Contributor

@pmaytak pmaytak commented Feb 13, 2025

Fixes #3129

  • Added a new property TryAllDecryptionKeys to TokenValidationParameters to indicate whether all decryption keys should be tried during token decryption when a key is not matched to the token 'kid' or if the token 'kid' is empty. This property defaults to true. [1] [2]
  • Updated the GetContentEncryptionKeys and DecryptToken methods to use the new TryAllDecryptionKeys property for determining when to try all decryption keys. [1] [2]
  • Also updates the new validation code path.

The precedence of decryption methods:

  • TokenValidationParameters.TokenDecryptionKeyResolver, if set.
  • TokenValidationParameters.TokenDecryptionKey, if set and key ID matches.
  • TokenValidationParameters.TokenDecryptionKeys, if set and contains any keys where key ID matches.
  • If TokenValidationParameters.TryAllDecryptionKeys is set to 'true', TokenValidationParameters.TokenDecryptionKey, TokenValidationParameters.TokenDecryptionKeys, and TokenDecryptionKeys from configuration.

Note: I will add more tests if this approach is agreed on.

@pmaytak pmaytak requested a review from a team as a code owner February 13, 2025 08:09
@pmaytak pmaytak force-pushed the pmaytak/decrypt-keyid branch from 55ad3e3 to 35aaac2 Compare February 13, 2025 11:35
@pmaytak pmaytak requested a review from Copilot February 13, 2025 11:35

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 8 changed files in this pull request and generated no comments.

Files not reviewed (3)
  • src/Microsoft.IdentityModel.Tokens/PublicAPI.Unshipped.txt: Language not supported
  • src/Microsoft.IdentityModel.Tokens/LogMessages.cs: Evaluated as low risk
  • test/Microsoft.IdentityModel.JsonWebTokens.Tests/JsonWebTokenHandlerTests.cs: Evaluated as low risk
@jennyf19
Copy link
Collaborator

Any performance tests needed given the additional logic for key handling?

@pmaytak
Copy link
Contributor Author

pmaytak commented Feb 14, 2025

Any performance tests needed given the additional logic for key handling?

Don't think so. It's just one extra bool flag check.

Copy link
Contributor

@jmprieur jmprieur left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks @pmaytak

There are formatting issues (IDE0055) that need to be addressed, probably in a different PRs (see at the bottom of the PR, as you have not touched that file)

@pmaytak pmaytak force-pushed the pmaytak/decrypt-keyid branch from 35aaac2 to f9daa79 Compare February 19, 2025 11:01
Copy link

Summary

Summary
Generated on: 2/19/2025 - 11:14:26 AM
Coverage date: 2/19/2025 - 11:04:38 AM - 2/19/2025 - 11:14:01 AM
Parser: MultiReport (60x Cobertura)
Assemblies: 1
Classes: 7
Files: 2
Line coverage: 80.3% (620 of 772)
Covered lines: 620
Uncovered lines: 152
Coverable lines: 772
Total lines: 483
Branch coverage: 67.8% (228 of 336)
Covered branches: 228
Total branches: 336
Method coverage: Feature is only available for sponsors

Coverage

Microsoft.IdentityModel.JsonWebTokens - 80.3%
Name Line Branch
Microsoft.IdentityModel.JsonWebTokens 80.3% 67.8%
Microsoft.IdentityModel.JsonWebTokens.JwtTokenUtilities 100%
System.Text.RegularExpressions.Generated 80.3% 67.8%
System.Text.RegularExpressions.Generated 80.3% 67.8%
System.Text.RegularExpressions.Generated.<RegexGenerator_g>F12A1AEDDDFE32BA
DF4DBFF323AF1BCB48B9F9721B7CD3E05F5E034CF225E3DF8__CreateJweRegex_1
79.2% 68%
System.Text.RegularExpressions.Generated.<RegexGenerator_g>F12A1AEDDDFE32BA
DF4DBFF323AF1BCB48B9F9721B7CD3E05F5E034CF225E3DF8__CreateJwsRegex_0
81.4% 67.6%
System.Text.RegularExpressions.Generated.<RegexGenerator_g>F334844C618E00D3
CEC5D3FE0D00CF0141BBEE98635313BB2CB8D3921464CE05A__CreateJweRegex_1
79.2% 68%
System.Text.RegularExpressions.Generated.<RegexGenerator_g>F334844C618E00D3
CEC5D3FE0D00CF0141BBEE98635313BB2CB8D3921464CE05A__CreateJwsRegex_0
81.4% 67.6%

@pmaytak pmaytak merged commit 6a9e823 into dev Feb 19, 2025
6 checks passed
@pmaytak pmaytak deleted the pmaytak/decrypt-keyid branch February 19, 2025 11:32
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.

[Bug] Token is decrypted using all keys if no key ID matches
3 participants