From adccfaf5d5041eb65ea96490c0f7772e1d0b81a9 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Tue, 21 Apr 2020 15:58:45 +0100 Subject: [PATCH] oauth: ensure auth grant redirect URI matches token req --- .../Authentication/OAuth2ClientTests.cs | 2 +- .../Authentication/OAuth/IOAuth2WebBrowser.cs | 2 + .../OAuth/OAuth2AuthorizationCodeResult.cs | 6 +- .../Authentication/OAuth/OAuth2Client.cs | 18 +++--- .../OAuth/OAuth2SystemWebBrowser.cs | 64 +++++++++---------- .../Objects/TestOAuth2Server.cs | 31 ++++++--- .../Objects/TestOAuth2WebBrowser.cs | 5 ++ 7 files changed, 74 insertions(+), 54 deletions(-) diff --git a/src/shared/Microsoft.Git.CredentialManager.Tests/Authentication/OAuth2ClientTests.cs b/src/shared/Microsoft.Git.CredentialManager.Tests/Authentication/OAuth2ClientTests.cs index 52938bbfd..2c668fdaa 100644 --- a/src/shared/Microsoft.Git.CredentialManager.Tests/Authentication/OAuth2ClientTests.cs +++ b/src/shared/Microsoft.Git.CredentialManager.Tests/Authentication/OAuth2ClientTests.cs @@ -98,7 +98,7 @@ public async Task OAuth2Client_GetTokenByAuthorizationCodeAsync() OAuth2Client client = CreateClient(httpHandler, endpoints); - var authCodeResult = new OAuth2AuthorizationCodeResult(authCode); + var authCodeResult = new OAuth2AuthorizationCodeResult(authCode, TestRedirectUri); OAuth2TokenResult result = await client.GetTokenByAuthorizationCodeAsync(authCodeResult, CancellationToken.None); Assert.NotNull(result); diff --git a/src/shared/Microsoft.Git.CredentialManager/Authentication/OAuth/IOAuth2WebBrowser.cs b/src/shared/Microsoft.Git.CredentialManager/Authentication/OAuth/IOAuth2WebBrowser.cs index 98fd609d9..1c73d5883 100644 --- a/src/shared/Microsoft.Git.CredentialManager/Authentication/OAuth/IOAuth2WebBrowser.cs +++ b/src/shared/Microsoft.Git.CredentialManager/Authentication/OAuth/IOAuth2WebBrowser.cs @@ -9,6 +9,8 @@ namespace Microsoft.Git.CredentialManager.Authentication.OAuth { public interface IOAuth2WebBrowser { + Uri UpdateRedirectUri(Uri uri); + Task GetAuthenticationCodeAsync(Uri authorizationUri, Uri redirectUri, CancellationToken ct); } } diff --git a/src/shared/Microsoft.Git.CredentialManager/Authentication/OAuth/OAuth2AuthorizationCodeResult.cs b/src/shared/Microsoft.Git.CredentialManager/Authentication/OAuth/OAuth2AuthorizationCodeResult.cs index 800577bf1..1ff91b282 100644 --- a/src/shared/Microsoft.Git.CredentialManager/Authentication/OAuth/OAuth2AuthorizationCodeResult.cs +++ b/src/shared/Microsoft.Git.CredentialManager/Authentication/OAuth/OAuth2AuthorizationCodeResult.cs @@ -1,18 +1,20 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT license. +using System; namespace Microsoft.Git.CredentialManager.Authentication.OAuth { public class OAuth2AuthorizationCodeResult { - public OAuth2AuthorizationCodeResult(string code, string codeVerifier = null) + public OAuth2AuthorizationCodeResult(string code, Uri redirectUri = null, string codeVerifier = null) { Code = code; + RedirectUri = redirectUri; CodeVerifier = codeVerifier; } public string Code { get; } - + public Uri RedirectUri { get; } public string CodeVerifier { get; } } } diff --git a/src/shared/Microsoft.Git.CredentialManager/Authentication/OAuth/OAuth2Client.cs b/src/shared/Microsoft.Git.CredentialManager/Authentication/OAuth/OAuth2Client.cs index 2ae835a5d..96356e5fb 100644 --- a/src/shared/Microsoft.Git.CredentialManager/Authentication/OAuth/OAuth2Client.cs +++ b/src/shared/Microsoft.Git.CredentialManager/Authentication/OAuth/OAuth2Client.cs @@ -104,9 +104,11 @@ public async Task GetAuthorizationCodeAsync(IEnum [OAuth2Constants.AuthorizationEndpoint.PkceChallengeParameter] = codeChallenge }; + Uri redirectUri = null; if (_redirectUri != null) { - queryParams[OAuth2Constants.RedirectUriParameter] = _redirectUri.ToString(); + redirectUri = browser.UpdateRedirectUri(_redirectUri); + queryParams[OAuth2Constants.RedirectUriParameter] = redirectUri.ToString(); } string scopesStr = string.Join(" ", scopes); @@ -123,12 +125,12 @@ public async Task GetAuthorizationCodeAsync(IEnum Uri authorizationUri = authorizationUriBuilder.Uri; // Open the browser at the request URI to start the authorization code grant flow. - Uri redirectUri = await browser.GetAuthenticationCodeAsync(authorizationUri, _redirectUri, ct); + Uri finalUri = await browser.GetAuthenticationCodeAsync(authorizationUri, redirectUri, ct); // Check for errors serious enough we should terminate the flow, such as if the state value returned does // not match the one we passed. This indicates a badly implemented Authorization Server, or worse, some // form of failed MITM or replay attack. - IDictionary redirectQueryParams = redirectUri.GetQueryParameters(); + IDictionary redirectQueryParams = finalUri.GetQueryParameters(); if (!redirectQueryParams.TryGetValue(OAuth2Constants.AuthorizationGrantResponse.StateParameter, out string replyState)) { throw new OAuth2Exception($"Missing '{OAuth2Constants.AuthorizationGrantResponse.StateParameter}' in response."); @@ -144,7 +146,7 @@ public async Task GetAuthorizationCodeAsync(IEnum throw new OAuth2Exception($"Missing '{OAuth2Constants.AuthorizationGrantResponse.AuthorizationCodeParameter}' in response."); } - return new OAuth2AuthorizationCodeResult(authCode, codeVerifier); + return new OAuth2AuthorizationCodeResult(authCode, redirectUri, codeVerifier); } public async Task GetDeviceCodeAsync(IEnumerable scopes, CancellationToken ct) @@ -191,14 +193,14 @@ public async Task GetTokenByAuthorizationCodeAsync(OAuth2Auth [OAuth2Constants.ClientIdParameter] = _clientId }; - if (authorizationCodeResult.CodeVerifier != null) + if (authorizationCodeResult.RedirectUri != null) { - formData[OAuth2Constants.TokenEndpoint.PkceVerifierParameter] = authorizationCodeResult.CodeVerifier; + formData[OAuth2Constants.RedirectUriParameter] = authorizationCodeResult.RedirectUri.ToString(); } - if (_redirectUri != null) + if (authorizationCodeResult.CodeVerifier != null) { - formData[OAuth2Constants.RedirectUriParameter] = _redirectUri.ToString(); + formData[OAuth2Constants.TokenEndpoint.PkceVerifierParameter] = authorizationCodeResult.CodeVerifier; } using (HttpContent requestContent = new FormUrlEncodedContent(formData)) diff --git a/src/shared/Microsoft.Git.CredentialManager/Authentication/OAuth/OAuth2SystemWebBrowser.cs b/src/shared/Microsoft.Git.CredentialManager/Authentication/OAuth/OAuth2SystemWebBrowser.cs index 1b22df22f..5f66f43f9 100644 --- a/src/shared/Microsoft.Git.CredentialManager/Authentication/OAuth/OAuth2SystemWebBrowser.cs +++ b/src/shared/Microsoft.Git.CredentialManager/Authentication/OAuth/OAuth2SystemWebBrowser.cs @@ -42,55 +42,35 @@ public OAuth2SystemWebBrowser(OAuth2WebBrowserOptions options) _options = options; } - public async Task GetAuthenticationCodeAsync(Uri authorizationUri, Uri redirectUri, CancellationToken ct) + public Uri UpdateRedirectUri(Uri uri) { - if (!redirectUri.IsLoopback) + if (!uri.IsLoopback) { - throw new ArgumentException("Only localhost is supported as a redirect URI.", nameof(redirectUri)); + throw new ArgumentException("Only localhost is supported as a redirect URI.", nameof(uri)); } // If a port has been specified use it, otherwise find a free one - if (redirectUri.IsDefaultPort) + if (uri.IsDefaultPort) { int port = GetFreeTcpPort(); - - UpdateLoopbackRedirectPort(port, ref authorizationUri, ref redirectUri); + return new UriBuilder(uri) {Port = port}.Uri; } - Task interceptTask = InterceptRequestsAsync(redirectUri, ct); - - OpenDefaultBrowser(authorizationUri); - - return await interceptTask; + return uri; } - private void UpdateLoopbackRedirectPort(int port, ref Uri authorizationUri, ref Uri redirectUri) + public async Task GetAuthenticationCodeAsync(Uri authorizationUri, Uri redirectUri, CancellationToken ct) { - IDictionary authUriQuery = authorizationUri.GetQueryParameters(); - - Uri newRedirectUri = new UriBuilder(redirectUri) {Port = port}.Uri; - - authUriQuery[OAuth2Constants.RedirectUriParameter] = newRedirectUri.ToString(); - - Uri newAuthorizationUri = new UriBuilder(authorizationUri){Query = authUriQuery.ToQueryString()}.Uri; + if (!redirectUri.IsLoopback) + { + throw new ArgumentException("Only localhost is supported as a redirect URI.", nameof(redirectUri)); + } - authorizationUri = newAuthorizationUri; - redirectUri = newRedirectUri; - } + Task interceptTask = InterceptRequestsAsync(redirectUri, ct); - private static int GetFreeTcpPort() - { - var listener = new TcpListener(IPAddress.Loopback, 0); + OpenDefaultBrowser(authorizationUri); - try - { - listener.Start(); - return ((IPEndPoint) listener.LocalEndpoint).Port; - } - finally - { - listener.Stop(); - } + return await interceptTask; } private void OpenDefaultBrowser(Uri uri) @@ -205,5 +185,21 @@ string FormatError(string format) } } } + + private static int GetFreeTcpPort() + { + var listener = new TcpListener(IPAddress.Loopback, 0); + + try + { + listener.Start(); + return ((IPEndPoint) listener.LocalEndpoint).Port; + } + finally + { + listener.Stop(); + } + } + } } diff --git a/src/shared/TestInfrastructure/Objects/TestOAuth2Server.cs b/src/shared/TestInfrastructure/Objects/TestOAuth2Server.cs index 7f45d2ca0..2b2051830 100644 --- a/src/shared/TestInfrastructure/Objects/TestOAuth2Server.cs +++ b/src/shared/TestInfrastructure/Objects/TestOAuth2Server.cs @@ -68,7 +68,8 @@ private Task OnAuthorizationEndpointAsync(HttpRequestMessag } // Redirect is optional, but if it is specified it must match a registered URI - Uri redirectUri = app.ValidateRedirect(reqQuery[OAuth2Constants.RedirectUriParameter]); + reqQuery.TryGetValue(OAuth2Constants.RedirectUriParameter, out string redirectUriStr); + Uri redirectUri = app.ValidateRedirect(redirectUriStr); // Scope is optional reqQuery.TryGetValue(OAuth2Constants.ScopeParameter, out string scopeStr); @@ -99,7 +100,7 @@ private Task OnAuthorizationEndpointAsync(HttpRequestMessag // Create the auth code grant OAuth2Application.AuthCodeGrant grant = app.CreateAuthorizationCodeGrant( - TokenGenerator, scopes, codeChallenge, codeChallengeMethod); + TokenGenerator, scopes, redirectUriStr, codeChallenge, codeChallengeMethod); var respQuery = new Dictionary { @@ -189,10 +190,12 @@ private async Task OnTokenEndpointAsync(HttpRequestMessage } formData.TryGetValue(OAuth2Constants.TokenEndpoint.PkceVerifierParameter, out string codeVerifier); + if (formData.TryGetValue(OAuth2Constants.RedirectUriParameter, out string redirectUriStr)) + { + app.ValidateRedirect(redirectUriStr); + } - app.ValidateRedirect(formData[OAuth2Constants.RedirectUriParameter]); - - tokenResp = app.CreateTokenByAuthorizationGrant(TokenGenerator, authCode, codeVerifier); + tokenResp = app.CreateTokenByAuthorizationGrant(TokenGenerator, authCode, codeVerifier, redirectUriStr); } else if (StringComparer.OrdinalIgnoreCase.Equals(grantType, OAuth2Constants.TokenEndpoint.RefreshTokenGrantType)) { @@ -287,16 +290,18 @@ public class OAuth2Application { public class AuthCodeGrant { - public AuthCodeGrant(string code, string[] scopes, + public AuthCodeGrant(string code, string[] scopes, string redirectUri = null, string codeChallenge = null, OAuth2PkceChallengeMethod codeChallengeMethod = OAuth2PkceChallengeMethod.Plain) { Code = code; Scopes = scopes; + RedirectUri = redirectUri; CodeChallenge = codeChallenge; CodeChallengeMethod = codeChallengeMethod; } public string Code { get; } public string[] Scopes { get; } + public string RedirectUri { get; } public string CodeChallenge { get; } public OAuth2PkceChallengeMethod CodeChallengeMethod { get; } } @@ -336,11 +341,11 @@ public OAuth2Application(string id) public IDictionary RefreshTokens { get; } = new Dictionary(); public AuthCodeGrant CreateAuthorizationCodeGrant(TestOAuth2ServerTokenGenerator generator, - string[] scopes, string codeChallenge, OAuth2PkceChallengeMethod codeChallengeMethod) + string[] scopes, string redirectUri, string codeChallenge, OAuth2PkceChallengeMethod codeChallengeMethod) { string code = generator.CreateAuthorizationCode(); - var grant = new AuthCodeGrant(code, scopes, codeChallenge, codeChallengeMethod); + var grant = new AuthCodeGrant(code, scopes, redirectUri, codeChallenge, codeChallengeMethod); AuthGrants.Add(grant); return grant; @@ -387,7 +392,7 @@ public bool IsDeviceCodeGrantApproved(string deviceCode) } public TokenEndpointResponseJson CreateTokenByAuthorizationGrant( - TestOAuth2ServerTokenGenerator generator, string authCode, string codeVerifier) + TestOAuth2ServerTokenGenerator generator, string authCode, string codeVerifier, string redirectUri) { var grant = AuthGrants.FirstOrDefault(x => x.Code == authCode); if (grant is null) @@ -395,6 +400,7 @@ public TokenEndpointResponseJson CreateTokenByAuthorizationGrant( throw new Exception($"Invalid authorization code '{authCode}'"); } + // Validate the grant's code challenge was constructed from the given code verifier if (!string.IsNullOrWhiteSpace(grant.CodeChallenge)) { if (string.IsNullOrWhiteSpace(codeVerifier)) @@ -431,6 +437,13 @@ public TokenEndpointResponseJson CreateTokenByAuthorizationGrant( } } + // If an explicit redirect URI was used as part of the authorization request then + // the redirect URI used for the token call must match exactly. + if (!string.IsNullOrWhiteSpace(grant.RedirectUri) && !StringComparer.Ordinal.Equals(grant.RedirectUri, redirectUri)) + { + throw new Exception("Redirect URI must match exactly the one used when requesting the authorization code."); + } + string accessToken = generator.CreateAccessToken(); string refreshToken = generator.CreateRefreshToken(); diff --git a/src/shared/TestInfrastructure/Objects/TestOAuth2WebBrowser.cs b/src/shared/TestInfrastructure/Objects/TestOAuth2WebBrowser.cs index 143593e39..2fdc00cc6 100644 --- a/src/shared/TestInfrastructure/Objects/TestOAuth2WebBrowser.cs +++ b/src/shared/TestInfrastructure/Objects/TestOAuth2WebBrowser.cs @@ -17,6 +17,11 @@ public TestOAuth2WebBrowser(HttpMessageHandler httpHandler) _httpClient = new HttpClient(httpHandler); } + public Uri UpdateRedirectUri(Uri uri) + { + return uri; + } + public async Task GetAuthenticationCodeAsync(Uri authorizationUri, Uri redirectUri, CancellationToken ct) { using (var response = await _httpClient.SendAsync(HttpMethod.Get, authorizationUri))