From f99402dfc18f9ac60f34506a399df34d83b0759d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Rodr=C3=ADguez?= Date: Thu, 17 Jun 2021 20:03:35 -0400 Subject: [PATCH] [core] bearerTokenAuthenticationPolicy shouldn't sign HTTP URLs (#15785) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Following our approach in other languages, for example, Python’s: https://github.com/Azure/azure-sdk-for-python/blob/d27b0f6a49afa85d99994f052188d192a10c5f02/sdk/containerregistry/azure-containerregistry/azure/containerregistry/_helpers.py#L81 Fixes #14311 --- .../bearerTokenAuthenticationPolicy.ts | 6 ++++ .../bearerTokenAuthenticationPolicyTests.ts | 26 +++++++++++++++-- sdk/core/core-http/test/serviceClientTests.ts | 28 +++++++++---------- .../bearerTokenAuthenticationPolicy.ts | 6 ++++ .../bearerTokenAuthenticationPolicy.spec.ts | 22 +++++++++++++++ .../unit/logsQueryClient.unittest.spec.ts | 6 ++-- 6 files changed, 75 insertions(+), 19 deletions(-) diff --git a/sdk/core/core-http/src/policies/bearerTokenAuthenticationPolicy.ts b/sdk/core/core-http/src/policies/bearerTokenAuthenticationPolicy.ts index 998b9a1395a4..78e5d9957f85 100644 --- a/sdk/core/core-http/src/policies/bearerTokenAuthenticationPolicy.ts +++ b/sdk/core/core-http/src/policies/bearerTokenAuthenticationPolicy.ts @@ -240,6 +240,12 @@ export function bearerTokenAuthenticationPolicy( } public async sendRequest(webResource: WebResourceLike): Promise { + if (!webResource.url.toLowerCase().startsWith("https://")) { + throw new Error( + "Bearer token authentication is not permitted for non-TLS protected (non-https) URLs." + ); + } + const { token } = await getToken({ abortSignal: webResource.abortSignal, tracingOptions: { diff --git a/sdk/core/core-http/test/policies/bearerTokenAuthenticationPolicyTests.ts b/sdk/core/core-http/test/policies/bearerTokenAuthenticationPolicyTests.ts index c7ecb2bc7132..d5ad4cb4783f 100644 --- a/sdk/core/core-http/test/policies/bearerTokenAuthenticationPolicyTests.ts +++ b/sdk/core/core-http/test/policies/bearerTokenAuthenticationPolicyTests.ts @@ -183,8 +183,30 @@ describe("BearerTokenAuthenticationPolicy", function() { assert.equal(expireDelayMs + 2 * getTokenDelay, Date.now() - startTime, exceptionMessage); }); - function createRequest(operationSpec?: OperationSpec): WebResource { - const request = new WebResource(); + it("throws if the target URI doesn't start with 'https'", async () => { + const expiresOn = Date.now() + 1000 * 60; // One minute later. + const credential = new MockRefreshAzureCredential(expiresOn); + const request = createRequest(undefined, "http://azure.com"); + const policy = createBearerTokenPolicy("test-scope", credential); + + let error: Error | undefined; + try { + await policy.sendRequest(request); + } catch (e) { + error = e; + } + + assert.equal( + error?.message, + "Bearer token authentication is not permitted for non-TLS protected (non-https) URLs." + ); + }); + + function createRequest( + operationSpec?: OperationSpec, + uri: string = "https://azure.com" + ): WebResource { + const request = new WebResource(uri); request.operationSpec = operationSpec; return request; } diff --git a/sdk/core/core-http/test/serviceClientTests.ts b/sdk/core/core-http/test/serviceClientTests.ts index 1ce8529128f5..3d3fe3ceb494 100644 --- a/sdk/core/core-http/test/serviceClientTests.ts +++ b/sdk/core/core-http/test/serviceClientTests.ts @@ -44,7 +44,7 @@ describe("ServiceClient", function() { const testOperationSpec: OperationSpec = { httpMethod: "GET", - baseUrl: "httpbin.org", + baseUrl: "https://bin.org", serializer: new Serializer(), headerParameters: [ { @@ -217,7 +217,7 @@ describe("ServiceClient", function() { }, { httpMethod: "GET", - baseUrl: "httpbin.org", + baseUrl: "https://bin.org", serializer: new Serializer(), headerParameters: [ { @@ -271,7 +271,7 @@ describe("ServiceClient", function() { {}, { httpMethod: "GET", - baseUrl: "httpbin.org", + baseUrl: "https://bin.org", serializer: new Serializer(), headerParameters: [], responses: { @@ -385,7 +385,7 @@ describe("ServiceClient", function() { }); it("should serialize collection with empty multi query parameter", async function() { - await testSendOperationRequest([], QueryCollectionFormat.Multi, true, "httpbin.org"); + await testSendOperationRequest([], QueryCollectionFormat.Multi, true, "https://bin.org"); }); it("should apply withCredentials to requests", async function() { @@ -406,7 +406,7 @@ describe("ServiceClient", function() { { serializer: new Serializer(), httpMethod: "GET", - baseUrl: "httpbin.org", + baseUrl: "https://bin.org", responses: { 200: {} } } ); @@ -423,7 +423,7 @@ describe("ServiceClient", function() { { serializer: new Serializer(), httpMethod: "GET", - baseUrl: "httpbin.org", + baseUrl: "https://bin.org", responses: { 200: {} } } ); @@ -454,7 +454,7 @@ describe("ServiceClient", function() { { serializer: new Serializer(), httpMethod: "GET", - baseUrl: "httpbin.org", + baseUrl: "https://bin.org", responses: { 200: { bodyMapper: { @@ -494,7 +494,7 @@ describe("ServiceClient", function() { { serializer: new Serializer(), httpMethod: "GET", - baseUrl: "httpbin.org", + baseUrl: "https://bin.org", responses: {} } ); @@ -521,7 +521,7 @@ describe("ServiceClient", function() { { serializer: new Serializer(), httpMethod: "GET", - baseUrl: "httpbin.org", + baseUrl: "https://bin.org", responses: {} } ); @@ -547,7 +547,7 @@ describe("ServiceClient", function() { { serializer: new Serializer(), httpMethod: "GET", - baseUrl: "httpbin.org", + baseUrl: "https://bin.org", responses: {} } ); @@ -576,7 +576,7 @@ describe("ServiceClient", function() { { serializer: new Serializer(), httpMethod: "GET", - baseUrl: "httpbin.org", + baseUrl: "https://bin.org", responses: {} } ); @@ -607,7 +607,7 @@ describe("ServiceClient", function() { { serializer: new Serializer(), httpMethod: "GET", - baseUrl: "httpbin.org", + baseUrl: "https://bin.org", responses: {} } ); @@ -1960,7 +1960,7 @@ describe("ServiceClient", function() { bodyMapper: BodyMapper } }, - baseUrl: "httpbin.org", + baseUrl: "https://bin.org", serializer }; @@ -2032,7 +2032,7 @@ async function testSendOperationRequest( }, { httpMethod: "GET", - baseUrl: "httpbin.org", + baseUrl: "https://bin.org", serializer: new Serializer(), queryParameters: [ { diff --git a/sdk/core/core-rest-pipeline/src/policies/bearerTokenAuthenticationPolicy.ts b/sdk/core/core-rest-pipeline/src/policies/bearerTokenAuthenticationPolicy.ts index 82ed0fda700b..37cfc549389b 100644 --- a/sdk/core/core-rest-pipeline/src/policies/bearerTokenAuthenticationPolicy.ts +++ b/sdk/core/core-rest-pipeline/src/policies/bearerTokenAuthenticationPolicy.ts @@ -155,6 +155,12 @@ export function bearerTokenAuthenticationPolicy( * - Retrieve a token with the challenge information, then re-send the request. */ async sendRequest(request: PipelineRequest, next: SendRequest): Promise { + if (!request.url.toLowerCase().startsWith("https://")) { + throw new Error( + "Bearer token authentication is not permitted for non-TLS protected (non-https) URLs." + ); + } + await callbacks.authorizeRequest({ scopes: Array.isArray(scopes) ? scopes : [scopes], request, diff --git a/sdk/core/core-rest-pipeline/test/bearerTokenAuthenticationPolicy.spec.ts b/sdk/core/core-rest-pipeline/test/bearerTokenAuthenticationPolicy.spec.ts index cdc12a215d95..bcdc26be5b6c 100644 --- a/sdk/core/core-rest-pipeline/test/bearerTokenAuthenticationPolicy.spec.ts +++ b/sdk/core/core-rest-pipeline/test/bearerTokenAuthenticationPolicy.spec.ts @@ -223,6 +223,28 @@ describe("BearerTokenAuthenticationPolicy", function() { assert.equal(expireDelayMs + 2 * getTokenDelay, Date.now() - startTime, exceptionMessage); }); + it("throws if the target URI doesn't start with 'https'", async () => { + const expireDelayMs = defaultRefreshWindow + 5000; + const tokenExpiration = Date.now() + expireDelayMs; + const credential = new MockRefreshAzureCredential(tokenExpiration); + + const request = createPipelineRequest({ url: "http://example.com" }); + const policy = createBearerTokenPolicy("test-scope", credential); + const next = sinon.stub, ReturnType>(); + + let error: Error | undefined; + try { + await policy.sendRequest(request, next); + } catch (e) { + error = e; + } + + assert.equal( + error?.message, + "Bearer token authentication is not permitted for non-TLS protected (non-https) URLs." + ); + }); + function createBearerTokenPolicy( scopes: string | string[], credential: TokenCredential diff --git a/sdk/monitor/monitor-query/test/internal/unit/logsQueryClient.unittest.spec.ts b/sdk/monitor/monitor-query/test/internal/unit/logsQueryClient.unittest.spec.ts index 1b711961993e..f70b5b193123 100644 --- a/sdk/monitor/monitor-query/test/internal/unit/logsQueryClient.unittest.spec.ts +++ b/sdk/monitor/monitor-query/test/internal/unit/logsQueryClient.unittest.spec.ts @@ -29,12 +29,12 @@ describe("LogsQueryClient unit tests", () => { }; const client = new LogsQueryClient(tokenCredential, { - endpoint: "customEndpoint1", + endpoint: "https://customEndpoint1", scopes: "customScopes1" }); - assert.equal(client["_logAnalytics"].$host, "customEndpoint1"); - assert.equal(client["_logAnalytics"]["baseUri"], "customEndpoint1"); + assert.equal(client["_logAnalytics"].$host, "https://customEndpoint1"); + assert.equal(client["_logAnalytics"]["baseUri"], "https://customEndpoint1"); try { await client.queryLogs("workspaceId", "query", Durations.last5Minutes);