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 35967da7..245e3dd1 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 @@ -15,6 +15,7 @@ import java.util.Map; import java.util.HashMap; import java.util.concurrent.*; +import java.util.regex.Pattern; class AadInstanceDiscoveryProvider { @@ -27,6 +28,10 @@ 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-]*$"); + // 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"; private static final String IMDS_ENDPOINT = "http://169.254.169.254/metadata/instance/compute?api-version=" + DEFAULT_API_VERSION; @@ -98,8 +103,10 @@ static InstanceDiscoveryMetadataEntry getMetadataEntry(URL authorityUrl, && detectedRegion != null) { LOG.debug("Region autodetection found {}, this region will be used for future calls.", detectedRegion); + //The regional host is built first so that a region which cannot be used is not stored on the + // application, where it would affect every later request made by that application + host = getRegionalizedHost(authorityUrl.getHost(), detectedRegion); ((AbstractClientApplicationBase) msalRequest.application()).azureRegion = detectedRegion; - host = getRegionalizedHost(authorityUrl.getHost(), ((AbstractClientApplicationBase) msalRequest.application()).azureRegion()); } cacheRegionInstanceMetadata(authorityUrl.getHost(), host); @@ -186,6 +193,24 @@ static void cacheRegionInstanceMetadata(String originalHost, String regionalHost cache.putIfAbsent(regionalHost, new InstanceDiscoveryMetadataEntry(regionalHost, originalHost, aliases)); } + /** + * Checks a region against the Azure region naming convention: lowercase alphanumeric characters + * and hyphens, starting with a letter. + */ + static boolean isValidRegion(String region) { + return region != null && VALID_REGION.matcher(region).matches(); + } + + /** + * Regions which do not follow the Azure region naming convention are rejected, as they would otherwise be + * used to build a malformed authority host and lead to failed or misdirected token requests. + */ + private static MsalClientException invalidRegionException(String region) { + return new MsalClientException(String.format("Invalid region '%s': region must start with a lowercase " + + "letter and contain only lowercase alphanumeric characters and hyphens", region), + AuthenticationErrorCode.INVALID_REGION); + } + private static String getRegionalizedHost(String host, String region) { String regionalizedHost; @@ -193,6 +218,12 @@ private static String getRegionalizedHost(String host, String region) { return host; } + //Last check before the region is used to build a host: regions from any source, such as one set by an + // app developer, must follow the Azure region naming convention + if (!isValidRegion(region)) { + throw invalidRegionException(region); + } + //Cached calls may already have the regionalized authority, so if region info is already in the host just return as-is if (host.contains(region)) { return host; @@ -303,10 +334,19 @@ static String discoverRegion(MsalRequest msalRequest, ServiceBundle serviceBundl //Check if the REGION_NAME environment variable has a value for the region if (System.getenv(REGION_NAME) != null) { - LOG.info("Region found in environment variable: {}", System.getenv(REGION_NAME)); + String region = System.getenv(REGION_NAME); + LOG.info("Region found in environment variable: {}", region); + + //A region is used to build an authority host, so an unexpected value is rejected rather than + // used to make requests against a malformed URL + if (!isValidRegion(region)) { + currentRequest.regionSource(RegionTelemetry.REGION_SOURCE_FAILED_AUTODETECT.telemetryValue); + throw invalidRegionException(region); + } + currentRequest.regionSource(RegionTelemetry.REGION_SOURCE_ENV_VARIABLE.telemetryValue); - return System.getenv(REGION_NAME); + return region; } //Check the IMDS endpoint to retrieve current region (will only work if application is running in an Azure VM) @@ -316,21 +356,23 @@ static String discoverRegion(MsalRequest msalRequest, ServiceBundle serviceBundl ExecutorService executor = Executors.newSingleThreadExecutor(); Future future = executor.submit(() -> executeRequest(IMDS_ENDPOINT, headers, msalRequest, serviceBundle)); + String detectedRegion = null; + try { LOG.info("Starting call to IMDS endpoint."); IHttpResponse httpResponse = future.get(IMDS_TIMEOUT, IMDS_TIMEOUT_UNIT); //If call to IMDS endpoint was successful, parse the region from the JSON response body if (httpResponse.statusCode() == HttpStatus.HTTP_OK) { - String region = parseRegionFromImdsResponse(httpResponse.body()); - if (!StringHelper.isBlank(region)) { - LOG.info("Region retrieved from IMDS endpoint: {}", region); - currentRequest.regionSource(RegionTelemetry.REGION_SOURCE_IMDS.telemetryValue); + detectedRegion = parseRegionFromImdsResponse(httpResponse.body()); + } - return region; - } + if (!StringHelper.isBlank(detectedRegion)) { + LOG.info("Region retrieved from IMDS endpoint: {}", detectedRegion); + currentRequest.regionSource(RegionTelemetry.REGION_SOURCE_IMDS.telemetryValue); + } else { + LOG.warn("Call to local IMDS failed with status code: {}, or response was empty or missing a region", httpResponse.statusCode()); + currentRequest.regionSource(RegionTelemetry.REGION_SOURCE_FAILED_AUTODETECT.telemetryValue); } - LOG.warn("Call to local IMDS failed with status code: {}, or response was empty or missing a region", httpResponse.statusCode()); - currentRequest.regionSource(RegionTelemetry.REGION_SOURCE_FAILED_AUTODETECT.telemetryValue); } catch (Exception ex) { // handle other exceptions //IMDS call failed, cannot find region @@ -344,7 +386,19 @@ static String discoverRegion(MsalRequest msalRequest, ServiceBundle serviceBundl executor.shutdownNow(); } - return null; + if (StringHelper.isBlank(detectedRegion)) { + return null; + } + + //A region is used to build an authority host, so an unexpected value from IMDS is rejected rather than + // used to make requests against a malformed URL. Validation happens outside of the try/catch above so + // that it is not mistaken for an IMDS communication failure and silently swallowed + if (!isValidRegion(detectedRegion)) { + currentRequest.regionSource(RegionTelemetry.REGION_SOURCE_FAILED_AUTODETECT.telemetryValue); + throw invalidRegionException(detectedRegion); + } + + return detectedRegion; } /** diff --git a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AbstractClientApplicationBase.java b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AbstractClientApplicationBase.java index f381d808..e31ce327 100644 --- a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AbstractClientApplicationBase.java +++ b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AbstractClientApplicationBase.java @@ -421,6 +421,11 @@ public T autoDetectRegion(boolean val) { * Regions must be valid Azure regions and their short names should be used, such as 'westus' for the West US Azure region, * 'centralus' for the Central US Azure region, etc. *

+ * Because the region is used to build the authority host, it must follow the Azure region naming convention: + * lowercase alphanumeric characters and hyphens, starting with a letter. A region which does not will cause a + * {@link MsalClientException} with the error code {@link AuthenticationErrorCode#INVALID_REGION} when a token + * request first attempts to use it. + *

* Although you can set a specific region here and enable autodetection with {@link AbstractClientApplicationBase#autoDetectRegion} at the same time * the specific region set here will take priority over the autodetected region if there is a mismatch. *

diff --git a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AuthenticationErrorCode.java b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AuthenticationErrorCode.java index d5fba3df..f5136012 100644 --- a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AuthenticationErrorCode.java +++ b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AuthenticationErrorCode.java @@ -166,4 +166,13 @@ public class AuthenticationErrorCode { * This is returned by the instance discovery endpoint when the provided authority host is unknown. */ public static final String INVALID_INSTANCE = "invalid_instance"; + + /** + * Indicates that a region does not follow the Azure region naming convention, which is lowercase alphanumeric + * characters and hyphens starting with a letter, such as 'westus' or 'east-us-2'. Regions are used to build + * authority hosts, so any other value could result in requests being made against a malformed URL. The region + * may have been set with the azureRegion API, or detected from the REGION_NAME environment variable or the + * IMDS endpoint. For more details, see https://aka.ms/msal4j-azure-regions + */ + public static final String INVALID_REGION = "invalid_region"; } 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 f28aec53..525f4df6 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 @@ -11,6 +11,7 @@ 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.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.CALLS_REAL_METHODS; @@ -18,6 +19,7 @@ import java.net.URI; import java.net.URL; +import java.util.Collections; @ExtendWith(MockitoExtension.class) @TestInstance(TestInstance.Lifecycle.PER_CLASS) @@ -137,6 +139,89 @@ void aadInstanceDiscoveryTest_AutoDetectRegion_NoRegionDetected() throws Excepti } } + @Test + void aadInstanceDiscoveryTest_RegionSetByDeveloper_invalidRegion_throws() throws Exception { + + ConfidentialClientApplication app = ConfidentialClientApplication.builder( + "client_id", ClientCredentialFactory.createFromSecret("secret")) + .aadInstanceDiscoveryResponse(instanceDiscoveryValidResponse) + .azureRegion("east.us") + .build(); + + MsalRequest msalRequest = clientCredentialRequest(app); + URL authority = new URL(app.authority()); + + //A region containing a dot would add an unexpected subdomain to the authority host, so it must be rejected + // before it is used to build a URL + MsalClientException ex = assertThrows(MsalClientException.class, () -> + AadInstanceDiscoveryProvider.getMetadataEntry(authority, false, msalRequest, app.serviceBundle())); + + assertEquals(AuthenticationErrorCode.INVALID_REGION, ex.errorCode()); + } + + @Test + void aadInstanceDiscoveryTest_AutoDetectRegion_invalidRegionDetected_throws() throws Exception { + + ConfidentialClientApplication app = ConfidentialClientApplication.builder( + "client_id", ClientCredentialFactory.createFromSecret("secret")) + .autoDetectRegion(true) + .build(); + + MsalRequest msalRequest = clientCredentialRequest(app); + URL authority = new URL(app.authority()); + + try (MockedStatic mocked = mockStatic(AadInstanceDiscoveryProvider.class, CALLS_REAL_METHODS)) { + + mocked.when(() -> AadInstanceDiscoveryProvider.discoverRegion(msalRequest, + app.serviceBundle())).thenReturn("east.us"); + + MsalClientException ex = assertThrows(MsalClientException.class, () -> + AadInstanceDiscoveryProvider.getMetadataEntry(authority, false, msalRequest, app.serviceBundle())); + + assertEquals(AuthenticationErrorCode.INVALID_REGION, ex.errorCode()); + //An unusable region must not be stored on the application, where it would affect every later request + assertNull(app.azureRegion()); + } + } + + @Test + void aadInstanceDiscoveryTest_RegionSetByDeveloper_validRegion_buildsRegionalHost() throws Exception { + + ConfidentialClientApplication app = ConfidentialClientApplication.builder( + "client_id", ClientCredentialFactory.createFromSecret("secret")) + .authority("https://login.microsoftonline.com/my_tenant") + .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("eastus.login.microsoft.com", entry.preferredNetwork()); + assertEquals("login.microsoftonline.com", entry.preferredCache()); + } + } + + private MsalRequest clientCredentialRequest(ConfidentialClientApplication app) { + //Regions are only used by the client credential flow, see AadInstanceDiscoveryProvider.shouldUseRegionalEndpoint + ClientCredentialParameters parameters = ClientCredentialParameters.builder( + Collections.singleton("scopes")).build(); + + return new ClientCredentialRequest(parameters, app, + new RequestContext(app, PublicApi.ACQUIRE_TOKEN_FOR_CLIENT, parameters), null); + } + void assertValidResponse(InstanceDiscoveryMetadataEntry entry) { assertEquals(entry.preferredNetwork(), "login.microsoftonline.com"); assertEquals(entry.preferredCache(), "login.windows.net"); 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 563837ff..83964cd1 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 @@ -5,9 +5,14 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestInstance; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.NullAndEmptySource; +import org.junit.jupiter.params.provider.ValueSource; 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.assertTrue; @TestInstance(TestInstance.Lifecycle.PER_CLASS) class RegionDiscoveryTest { @@ -54,4 +59,38 @@ void parseRegionFromImdsResponse_emptyBody_returnsNull() { assertNull(AadInstanceDiscoveryProvider.parseRegionFromImdsResponse("")); assertNull(AadInstanceDiscoveryProvider.parseRegionFromImdsResponse(null)); } + + @ParameterizedTest + @ValueSource(strings = {"eastus", "westus2", "east-us-2", "centralus", "a", "a1", "a-1"}) + void isValidRegion_validRegionNames_returnsTrue(String region) { + assertTrue(AadInstanceDiscoveryProvider.isValidRegion(region)); + } + + @ParameterizedTest + @ValueSource(strings = { + "east.us", //dot would add a subdomain to the authority host + "east/us", //slash would add a path segment to the authority URL + "../evil", //path traversal attempt + "EastUS", //uppercase characters + "east us", //whitespace + "1eastus", //does not start with a letter + "-eastus", //does not start with a letter + "east_us", //underscore + "east$us"}) //other special characters + void isValidRegion_invalidRegionNames_returnsFalse(String region) { + assertFalse(AadInstanceDiscoveryProvider.isValidRegion(region)); + } + + @ParameterizedTest + @NullAndEmptySource + void isValidRegion_nullOrEmpty_returnsFalse(String region) { + assertFalse(AadInstanceDiscoveryProvider.isValidRegion(region)); + } + + @ParameterizedTest + @ValueSource(strings = {"West US", "West US 2"}) + void isValidRegion_displayName_returnsFalse(String region) { + //Regions must be given as their short name ("westus"), not their display name ("West US") + assertFalse(AadInstanceDiscoveryProvider.isValidRegion(region)); + } }