Fix: Add tenant ID validation in UserPrincipalManager#49631
Open
rujche wants to merge 14 commits into
Open
Conversation
…bypass
- Validate the 'tid' (tenant ID) claim in JWT tokens against the configured tenant
- Only accept tokens from the configured tenant if tenant ID is specified
- Maintains backward compatibility by skipping validation when tenant is not configured
- Fixes the authentication bypass vulnerability in UserPrincipalManager
Changes:
1. UserPrincipalManager.java:
- Added tenant ID claim validation in getValidator() method
- Extract configured tenant ID from AadAuthenticationProperties
- Reject tokens where tid doesn't match configured tenant
- Added debug logging for successful tenant validation
- Added org.springframework.util.StringUtils import
2. UserPrincipalManagerTests.java:
- Added 4 new unit tests for tenant ID validation:
* tenantIdValidationSucceedsWhenMatchingConfiguredTenant()
* tenantIdValidationFailsWhenMismatchedTenant()
* tenantIdValidationSkippedWhenNoTenantConfigured()
* tenantIdValidationSkippedWhenTenantPropertyIsEmpty()
- Added reflection helper methods for testing private methods
- All 9 tests pass successfully (5 original + 4 new)
rujche
requested review from
a team,
Netyyyy,
moarychan and
saragluna
as code owners
Add entry for the cross-tenant authentication bypass fix in AAD authentication filters to the 7.4.0-beta.1 release notes.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds JWT tid (tenant ID) claim validation to UserPrincipalManager to prevent cross-tenant token acceptance when a single tenant is intended, along with unit tests covering matching/mismatching/skip scenarios.
Changes:
- Added
tidvs configured tenant validation insideUserPrincipalManager#getValidator()’s claims verifier. - Added new unit tests covering tenant match/mismatch and validation skip behavior.
- Introduced reflection-based helpers in tests to reach private implementation details.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| sdk/spring/spring-cloud-azure-autoconfigure/src/main/java/com/azure/spring/cloud/autoconfigure/implementation/aad/filter/UserPrincipalManager.java | Adds tid claim validation against configured tenant. |
| sdk/spring/spring-cloud-azure-autoconfigure/src/test/java/com/azure/spring/cloud/autoconfigure/implementation/aad/filter/UserPrincipalManagerTests.java | Adds tests + reflection helpers to validate the new tenant-checking behavior. |
- Skip tenant ID validation for multi-tenant values (common, organizations, consumers) - Normalize tenant IDs to lowercase for case-insensitive comparison - Fix test helper method to properly accept and use JWSAlgorithm parameter - Update tests to reflect actual Spring behavior (default 'common' for unconfigured tenant) - Use 'organizations' instead of empty string for multi-tenant test scenario These changes ensure: 1. Multi-tenant applications (using common/organizations/consumers endpoints) still work 2. Tenant ID comparison is case-insensitive and normalized 3. Tests accurately reflect real-world Spring configuration defaults 4. Code is cleaner and follows best practices
- Use Locale.ROOT for security-sensitive tenant ID normalization (prevents locale-dependent behavior) - Change direct String cast to Object cast with toString() to safely handle non-String tid claims - Rename test method from tenantIdValidationSkippedWhenTenantPropertyIsEmpty() to tenantIdValidationSkippedWhenOrganizationsConfigured() for clarity - Maintains consistency with AadResourceServerConfiguration#getTrimmedTenantId patterns - Improves robustness against malicious tokens with unexpected claim types
…h rule - Reformatted normalizedTokenTid initialization to split the ternary operator across multiple lines - Ensures line 236 stays under 120 characters as enforced by Checkstyle - All 9 tests pass, BUILD SUCCESS
…n of final fields - Added UserPrincipalManager(JWKSource, AadAuthenticationProperties) package-private constructor - Updated tenant ID validation tests to use the new constructor instead of reflection - Removed setAadAuthenticationProperties() helper method that modified private final fields - Tests now avoid brittle reflection and are cleaner - All 9 tests pass, BUILD SUCCESS
…orts - Moved com.azure.spring.cloud... imports before com.nimbusds... imports - Aligned with module import-order conventions (consistent with other test classes)
rujche
changed the title
UserPrincipalManager
moarychan
reviewed
Jun 26, 2026
Member
There was a problem hiding this comment.
please help check if we extend the DefaultJWTClaimsVerifier using a seperate class, is it posible to avoid adding a new constructor
| * Multi-tenant values (common, organizations, consumers) should skip tenant ID validation. | ||
| */ | ||
| private boolean isMultiTenantValue(String normalizedTenantId) { | ||
| return "common".equals(normalizedTenantId) |
Member
There was a problem hiding this comment.
It is best to add logs to inform users that they are using a multi-tenant configuration and that tenant ID verification is not performed.
…on skip - Added debug log when multi-tenant values (common, organizations, consumers) skip tenant ID verification - Helps users understand when tenant ID validation is not performed - All 9 tests pass, BUILD SUCCESS
…erifier class - Created new AadJwtClaimsVerifier class extending DefaultJWTClaimsVerifier - Moved issuer, audience, and tenant ID validation logic into the new class - Simplifies UserPrincipalManager by delegating verification responsibilities - Eliminates need for custom constructor in UserPrincipalManager for most cases - More testable and maintainable architecture - Addressed review suggestion to use separate class for verification logic - All 9 tests pass, BUILD SUCCESS
- Make AadJwtClaimsVerifier package-private and final (internal implementation detail) - Extract issuer validation logic into AadIssuerValidator utility class to prevent duplication - Update UserPrincipalManager to use AadIssuerValidator instead of local isAadIssuer method - Add unit test tenantIdValidationSkippedWhenConsumersConfigured to cover 'consumers' multi-tenant value - Remove duplicate issuer prefix constants from UserPrincipalManager These changes improve code reusability, reduce duplication in security validation logic, and ensure consistent issuer validation across the package.
- Handle null/empty audience list gracefully with BadJWTException instead of NPE - Remove trailing whitespace from blank lines in test file
…n AadJwtClaimsVerifier
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
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.
Description
This PR fixes a cross-tenant authentication bypass vulnerability in Azure SDK for Java by implementing proper tenant ID (tid claim) validation in JWT tokens.
Problem
The AAD authentication filters (AadAuthenticationFilter and AadAppRoleStatelessAuthenticationFilter) were not validating the tenant ID claim in JWT tokens, allowing tokens from different tenants to be accepted, creating a security vulnerability.
Solution
Changes
New File: AadJwtClaimsVerifier.java
Updated: UserPrincipalManager.java
Updated: UserPrincipalManagerTests.java
Updated: CHANGELOG.md
Security Impact
Testing
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines