From 0df1ed268fbbd67b6fb7aa1b1a0688ec2cd4df90 Mon Sep 17 00:00:00 2001 From: Nilesh Choudhary Date: Fri, 7 Aug 2026 15:29:49 +0100 Subject: [PATCH] Harden regional authority validation Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0d07c9a2-f21b-44ee-8b3f-7e0589f1abe9 --- .../msal4j/AadInstanceDiscoveryProvider.java | 18 +- .../aad/msal4j/AadInstanceDiscoveryTest.java | 164 +++++++++++++++++- .../aad/msal4j/RegionDiscoveryTest.java | 8 +- 3 files changed, 175 insertions(+), 15 deletions(-) diff --git a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AadInstanceDiscoveryProvider.java b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AadInstanceDiscoveryProvider.java index 06faf9b5..9f057c44 100644 --- a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AadInstanceDiscoveryProvider.java +++ b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AadInstanceDiscoveryProvider.java @@ -28,9 +28,9 @@ class AadInstanceDiscoveryProvider { private static final String REGION_NAME = "REGION_NAME"; private static final int PORT_NOT_SET = -1; - //Azure region names are lowercase alphanumeric characters and hyphens, starting with a letter (westus, east-us-2, etc.). - //Regions are used to build authority hosts, so anything outside of that set could produce a malformed URL - private static final Pattern VALID_REGION = Pattern.compile("^[a-z][a-z0-9-]*$"); + //Azure region names form a single DNS label: lowercase alphanumeric characters and internal hyphens, + //starting with a letter and ending with an alphanumeric character (westus, east-us-2, etc.). + private static final Pattern VALID_REGION = Pattern.compile("^[a-z](?:[a-z0-9-]{0,61}[a-z0-9])?$"); // For information of the current api-version refer: https://docs.microsoft.com/en-us/azure/virtual-machines/windows/instance-metadata-service#versioning private static final String DEFAULT_API_VERSION = "2021-02-01"; @@ -194,8 +194,8 @@ static void cacheRegionInstanceMetadata(String originalHost, String regionalHost } /** - * Checks a region against the Azure region naming convention: lowercase alphanumeric characters - * and hyphens, starting with a letter. + * Checks a region against the Azure region naming convention and DNS-label constraints required + * when it is used to construct a regional authority host. */ static boolean isValidRegion(String region) { return region != null && VALID_REGION.matcher(region).matches(); @@ -330,8 +330,8 @@ static String discoverRegion(MsalRequest msalRequest, ServiceBundle serviceBundl CurrentRequest currentRequest = serviceBundle.getServerSideTelemetry().getCurrentRequest(); //Check if the REGION_NAME environment variable has a value for the region - if (System.getenv(REGION_NAME) != null) { - String region = System.getenv(REGION_NAME); + String region = getRegionName(); + if (region != null) { LOG.info("Region found in environment variable: {}", region); //An autodetected region that does not follow the Azure region naming convention is treated as a @@ -402,6 +402,10 @@ static String discoverRegion(MsalRequest msalRequest, ServiceBundle serviceBundl return detectedRegion; } + static String getRegionName() { + return System.getenv(REGION_NAME); + } + /** * Parses the region from the IMDS {@code /compute} JSON response body, reading the * {@code location} field. Returns {@code null} when the body is blank, the diff --git a/msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/AadInstanceDiscoveryTest.java b/msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/AadInstanceDiscoveryTest.java index 6c53bd39..18f15f57 100644 --- a/msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/AadInstanceDiscoveryTest.java +++ b/msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/AadInstanceDiscoveryTest.java @@ -8,14 +8,22 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestInstance; import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; +import org.junit.jupiter.params.provider.ValueSource; import org.mockito.MockedStatic; import org.mockito.junit.jupiter.MockitoExtension; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.Mockito.CALLS_REAL_METHODS; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.verify; import java.net.URI; import java.net.URL; @@ -139,15 +147,16 @@ void aadInstanceDiscoveryTest_AutoDetectRegion_NoRegionDetected() throws Excepti } } - @Test - void aadInstanceDiscoveryTest_RegionSetByDeveloper_invalidRegion_throws() { - - //A region containing a dot would add an unexpected subdomain to the authority host, so it must be rejected - // as soon as it is set, rather than deferred to first use + @ParameterizedTest + @ValueSource(strings = { + "east.us", "east/us", "../evil", "eastus:443", "east@us", "east us", "east\tus", "EastUS", + "east%2eus", "\uFF45astus", "eastus-", + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}) + void aadInstanceDiscoveryTest_RegionSetByDeveloper_invalidRegion_throws(String region) { MsalClientException ex = assertThrows(MsalClientException.class, () -> ConfidentialClientApplication.builder("client_id", ClientCredentialFactory.createFromSecret("secret")) .aadInstanceDiscoveryResponse(instanceDiscoveryValidResponse) - .azureRegion("east.us")); + .azureRegion(region)); assertEquals(AuthenticationErrorCode.INVALID_REGION, ex.errorCode()); } @@ -180,6 +189,83 @@ void aadInstanceDiscoveryTest_AutoDetectRegion_invalidRegionDetected_fallsBackTo } } + @ParameterizedTest + @ValueSource(strings = { + "east.us", "east/us", "../evil", "eastus:443", "east@us", "east us", "EastUS", "east%2eus", + "\uFF45astus", "eastus-", + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}) + void aadInstanceDiscoveryTest_InvalidEnvironmentRegion_fallsBackToGlobalWithoutCaching(String region) + throws Exception { + IHttpClient httpClient = mock(IHttpClient.class); + org.mockito.Mockito.when(httpClient.send(any(HttpRequest.class))).thenReturn(instanceDiscoveryResponse()); + + ConfidentialClientApplication app = autoDetectApplication(httpClient); + MsalRequest msalRequest = clientCredentialRequest(app); + URL authority = new URL(app.authority()); + + try (MockedStatic mocked = mockStatic(AadInstanceDiscoveryProvider.class, + CALLS_REAL_METHODS)) { + mocked.when(AadInstanceDiscoveryProvider::getRegionName).thenReturn(region); + + InstanceDiscoveryMetadataEntry entry = AadInstanceDiscoveryProvider.getMetadataEntry( + authority, false, msalRequest, app.serviceBundle()); + + verify(httpClient).send(argThat(request -> request.url().getHost().equals("login.microsoftonline.com"))); + assertGlobalFallback(app, entry, region); + } + } + + @ParameterizedTest + @ValueSource(strings = { + "east.us", "east/us", "../evil", "eastus:443", "east@us", "east us", "EastUS", "east%2eus", + "\uFF45astus", "eastus-", + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}) + void aadInstanceDiscoveryTest_InvalidImdsRegion_fallsBackToGlobalWithoutCaching(String region) throws Exception { + IHttpClient httpClient = mock(IHttpClient.class); + HttpResponse imdsResponse = new HttpResponse().statusCode(HttpStatus.HTTP_OK) + .body("{\"location\":\"" + region + "\"}"); + mockImdsAndInstanceDiscoveryResponses(httpClient, imdsResponse); + + ConfidentialClientApplication app = autoDetectApplication(httpClient); + MsalRequest msalRequest = clientCredentialRequest(app); + URL authority = new URL(app.authority()); + + try (MockedStatic mocked = mockStatic(AadInstanceDiscoveryProvider.class, + CALLS_REAL_METHODS)) { + mocked.when(AadInstanceDiscoveryProvider::getRegionName).thenReturn(null); + + InstanceDiscoveryMetadataEntry entry = AadInstanceDiscoveryProvider.getMetadataEntry( + authority, false, msalRequest, app.serviceBundle()); + + verify(httpClient).send(argThat(request -> request.url().toString().startsWith( + "http://169.254.169.254/metadata/instance/compute"))); + assertGlobalFallback(app, entry, region); + } + } + + @Test + void aadInstanceDiscoveryTest_MalformedImdsResponse_fallsBackToGlobalWithoutCaching() throws Exception { + IHttpClient httpClient = mock(IHttpClient.class); + HttpResponse imdsResponse = new HttpResponse().statusCode(HttpStatus.HTTP_OK).body("{ invalid json"); + mockImdsAndInstanceDiscoveryResponses(httpClient, imdsResponse); + + ConfidentialClientApplication app = autoDetectApplication(httpClient); + MsalRequest msalRequest = clientCredentialRequest(app); + URL authority = new URL(app.authority()); + + try (MockedStatic mocked = mockStatic(AadInstanceDiscoveryProvider.class, + CALLS_REAL_METHODS)) { + mocked.when(AadInstanceDiscoveryProvider::getRegionName).thenReturn(null); + + InstanceDiscoveryMetadataEntry entry = AadInstanceDiscoveryProvider.getMetadataEntry( + authority, false, msalRequest, app.serviceBundle()); + + verify(httpClient).send(argThat(request -> request.url().toString().startsWith( + "http://169.254.169.254/metadata/instance/compute"))); + assertGlobalFallback(app, entry, null); + } + } + @Test void aadInstanceDiscoveryTest_RegionSetByDeveloper_validRegion_buildsRegionalHost() throws Exception { @@ -209,6 +295,72 @@ void aadInstanceDiscoveryTest_RegionSetByDeveloper_validRegion_buildsRegionalHos } } + @ParameterizedTest + @CsvSource(value = { + "https://login.chinacloudapi.cn/my_tenant|eastus.login.chinacloudapi.cn", + "https://login.microsoftonline.us/my_tenant|eastus.login.microsoftonline.us"}, delimiter = '|') + void aadInstanceDiscoveryTest_RegionSetByDeveloper_sovereignAuthorityBuildsRegionalHost( + String authorityValue, String expectedHost) throws Exception { + ConfidentialClientApplication app = ConfidentialClientApplication.builder( + "client_id", ClientCredentialFactory.createFromSecret("secret")) + .authority(authorityValue) + .azureRegion("eastus") + .build(); + + MsalRequest msalRequest = clientCredentialRequest(app); + URL authority = new URL(app.authority()); + AadInstanceDiscoveryResponse expectedResponse = JsonHelper.convertJsonStringToJsonSerializableObject( + instanceDiscoveryValidResponse, AadInstanceDiscoveryResponse::fromJson); + + try (MockedStatic mocked = mockStatic(AadInstanceDiscoveryProvider.class, + CALLS_REAL_METHODS)) { + mocked.when(() -> AadInstanceDiscoveryProvider.discoverRegion(msalRequest, + app.serviceBundle())).thenReturn(null); + mocked.when(() -> AadInstanceDiscoveryProvider.sendInstanceDiscoveryRequest(authority, + msalRequest, app.serviceBundle())).thenReturn(expectedResponse); + + InstanceDiscoveryMetadataEntry entry = AadInstanceDiscoveryProvider.getMetadataEntry( + authority, false, msalRequest, app.serviceBundle()); + + assertEquals(expectedHost, entry.preferredNetwork()); + assertEquals(authority.getHost(), entry.preferredCache()); + } + } + + private ConfidentialClientApplication autoDetectApplication(IHttpClient httpClient) { + ConfidentialClientApplication.Builder builder = ConfidentialClientApplication.builder( + "client_id", ClientCredentialFactory.createFromSecret("secret")) + .autoDetectRegion(true); + if (httpClient != null) { + builder.httpClient(httpClient); + } + return builder.build(); + } + + private void mockImdsAndInstanceDiscoveryResponses(IHttpClient httpClient, HttpResponse imdsResponse) + throws Exception { + org.mockito.Mockito.when(httpClient.send(any(HttpRequest.class))).thenAnswer(invocation -> { + HttpRequest request = invocation.getArgument(0); + return request.url().toString().startsWith("http://169.254.169.254/metadata/instance/compute") ? + imdsResponse : instanceDiscoveryResponse(); + }); + } + + private HttpResponse instanceDiscoveryResponse() { + return new HttpResponse().statusCode(HttpStatus.HTTP_OK).body(instanceDiscoveryValidResponse); + } + + private void assertGlobalFallback(ConfidentialClientApplication app, InstanceDiscoveryMetadataEntry entry, + String invalidRegion) { + assertValidResponse(entry); + assertNull(app.azureRegion()); + if (invalidRegion != null) { + assertFalse(AadInstanceDiscoveryProvider.cache.containsKey(invalidRegion)); + assertFalse(AadInstanceDiscoveryProvider.cache.keySet().stream() + .anyMatch(host -> host.contains(invalidRegion))); + } + } + private MsalRequest clientCredentialRequest(ConfidentialClientApplication app) { //Regions are only used by the client credential flow, see AadInstanceDiscoveryProvider.shouldUseRegionalEndpoint ClientCredentialParameters parameters = ClientCredentialParameters.builder( diff --git a/msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/RegionDiscoveryTest.java b/msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/RegionDiscoveryTest.java index 83964cd1..4e43c388 100644 --- a/msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/RegionDiscoveryTest.java +++ b/msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/RegionDiscoveryTest.java @@ -61,7 +61,9 @@ void parseRegionFromImdsResponse_emptyBody_returnsNull() { } @ParameterizedTest - @ValueSource(strings = {"eastus", "westus2", "east-us-2", "centralus", "a", "a1", "a-1"}) + @ValueSource(strings = { + "eastus", "westus2", "east-us-2", "centralus", "a", "a1", "a-1", + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}) void isValidRegion_validRegionNames_returnsTrue(String region) { assertTrue(AadInstanceDiscoveryProvider.isValidRegion(region)); } @@ -75,8 +77,10 @@ void isValidRegion_validRegionNames_returnsTrue(String region) { "east us", //whitespace "1eastus", //does not start with a letter "-eastus", //does not start with a letter + "eastus-", //DNS labels cannot end with a hyphen "east_us", //underscore - "east$us"}) //other special characters + "east$us", //other special characters + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}) //longer than a DNS label void isValidRegion_invalidRegionNames_returnsFalse(String region) { assertFalse(AadInstanceDiscoveryProvider.isValidRegion(region)); }