oauth: implement RFC 7636 PKCE in OAuth client

mjcheetham · mjcheetham · commit 5f13ab2d3332 · 2020-04-22T17:08:09.000+01:00
Implement the Proof Key for Code Exchange (RFC 7636) specification in
the OAuth2 client.
diff --git a/Git-Credential-Manager.sln.DotSettings b/Git-Credential-Manager.sln.DotSettings
@@ -1,2 +1,3 @@
 ﻿<wpf:ResourceDictionary xml:space="preserve" xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:s="clr-namespace:System;assembly=mscorlib" xmlns:ss="urn:shemas-jetbrains-com:settings-storage-xaml" xmlns:wpf="http://schemas.microsoft.com/winfx/2006/xaml/presentation">
-	<s:String x:Key="/Default/Environment/Hierarchy/Build/SolBuilderDuo/UseMsbuildSolutionBuilder/@EntryValue">No</s:String></wpf:ResourceDictionary>
+	<s:String x:Key="/Default/Environment/Hierarchy/Build/SolBuilderDuo/UseMsbuildSolutionBuilder/@EntryValue">No</s:String>
+	<s:Boolean x:Key="/Default/UserDictionary/Words/=PKCE/@EntryIndexedValue">True</s:Boolean></wpf:ResourceDictionary>
diff --git a/src/shared/GitHub/GitHubAuthentication.cs b/src/shared/GitHub/GitHubAuthentication.cs
@@ -203,9 +203,9 @@ public async Task<OAuth2TokenResult> GetOAuthTokenAsync(Uri targetUri, IEnumerab
                 // Write message to the terminal (if any is attached) for some feedback that we're waiting for a web response
                 Context.Terminal.WriteLine("info: please complete authentication in your browser...");
 
-                string authCode = await oauthClient.GetAuthorizationCodeAsync(scopes, browser, CancellationToken.None);
+                OAuth2AuthorizationCodeResult authCodeResult = await oauthClient.GetAuthorizationCodeAsync(scopes, browser, CancellationToken.None);
 
-                return await oauthClient.GetTokenByAuthorizationCodeAsync(authCode, CancellationToken.None);
+                return await oauthClient.GetTokenByAuthorizationCodeAsync(authCodeResult, CancellationToken.None);
             }
             else
             {
diff --git a/src/shared/Microsoft.Git.CredentialManager.Tests/Authentication/OAuth2ClientTests.cs b/src/shared/Microsoft.Git.CredentialManager.Tests/Authentication/OAuth2ClientTests.cs
@@ -39,9 +39,9 @@ public async Task OAuth2Client_GetAuthorizationCodeAsync()
 
             OAuth2Client client = CreateClient(httpHandler, endpoints);
 
-            string actualAuthCode = await client.GetAuthorizationCodeAsync(expectedScopes, browser, CancellationToken.None);
+            OAuth2AuthorizationCodeResult result = await client.GetAuthorizationCodeAsync(expectedScopes, browser, CancellationToken.None);
 
-            Assert.Equal(expectedAuthCode, actualAuthCode);
+            Assert.Equal(expectedAuthCode, result.Code);
         }
 
         [Fact]
@@ -88,7 +88,7 @@ public async Task OAuth2Client_GetTokenByAuthorizationCodeAsync()
             string[] expectedScopes = {"read", "write", "delete"};
 
             OAuth2Application app = CreateTestApplication();
-            app.AuthCodes[authCode] = expectedScopes;
+            app.AuthGrants.Add(new OAuth2Application.AuthCodeGrant(authCode, expectedScopes));
 
             var server = new TestOAuth2Server(endpoints);
             server.RegisterApplication(app);
@@ -98,7 +98,8 @@ public async Task OAuth2Client_GetTokenByAuthorizationCodeAsync()
 
             OAuth2Client client = CreateClient(httpHandler, endpoints);
 
-            OAuth2TokenResult result = await client.GetTokenByAuthorizationCodeAsync(authCode, CancellationToken.None);
+            var authCodeResult = new OAuth2AuthorizationCodeResult(authCode);
+            OAuth2TokenResult result = await client.GetTokenByAuthorizationCodeAsync(authCodeResult, CancellationToken.None);
 
             Assert.NotNull(result);
             Assert.Equal(expectedScopes, result.Scopes);
@@ -217,9 +218,10 @@ public async Task OAuth2Client_E2E_InteractiveWebFlowAndRefresh()
 
             OAuth2Client client = CreateClient(httpHandler, endpoints);
 
-            string authCode = await client.GetAuthorizationCodeAsync(expectedScopes, browser, CancellationToken.None);
+            OAuth2AuthorizationCodeResult authCodeResult = await client.GetAuthorizationCodeAsync(
+                expectedScopes, browser, CancellationToken.None);
 
-            OAuth2TokenResult result1 = await client.GetTokenByAuthorizationCodeAsync(authCode, CancellationToken.None);
+            OAuth2TokenResult result1 = await client.GetTokenByAuthorizationCodeAsync(authCodeResult, CancellationToken.None);
 
             Assert.NotNull(result1);
             Assert.Equal(expectedScopes, result1.Scopes);
@@ -296,11 +298,11 @@ public async Task OAuth2Client_E2E_DeviceFlowAndRefresh()
             RedirectUris = new[] {TestRedirectUri}
         };
 
-        private static OAuth2Client CreateClient(HttpMessageHandler httpHandler, OAuth2ServerEndpoints endpoints, IOAuth2NonceGenerator generator = null)
+        private static OAuth2Client CreateClient(HttpMessageHandler httpHandler, OAuth2ServerEndpoints endpoints, IOAuth2CodeGenerator generator = null)
         {
             return new OAuth2Client(new HttpClient(httpHandler), endpoints, TestClientId, TestRedirectUri, TestClientSecret)
             {
-                NonceGenerator = generator
+                CodeGenerator = generator
             };
         }
 
diff --git a/src/shared/Microsoft.Git.CredentialManager.Tests/Authentication/OAuth2CryptographicCodeGeneratorTests.cs b/src/shared/Microsoft.Git.CredentialManager.Tests/Authentication/OAuth2CryptographicCodeGeneratorTests.cs
@@ -0,0 +1,87 @@
+using System.Linq;
+using System.Security.Cryptography;
+using System.Text;
+using Microsoft.Git.CredentialManager.Authentication.OAuth;
+using Xunit;
+
+namespace Microsoft.Git.CredentialManager.Tests.Authentication
+{
+    public class OAuth2CryptographicCodeGeneratorTests
+    {
+        private const string ValidBase64UrlCharsNoPad = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_";
+
+        [Fact]
+        public void OAuth2CryptographicCodeGenerator_CreateNonce_IsUnique()
+        {
+            var generator = new OAuth2CryptographicCodeGenerator();
+
+            // Create a bunch of nonce values
+            var nonces = new string[32];
+            for (int i = 0; i < nonces.Length; i++)
+            {
+                nonces[i] = generator.CreateNonce();
+            }
+
+            // There should be no duplicates
+            string[] uniqueNonces = nonces.Distinct().ToArray();
+            Assert.Equal(uniqueNonces, nonces);
+        }
+
+        [Fact]
+        public void OAuth2CryptographicCodeGenerator_CreatePkceCodeVerifier_IsUniqueBase64UrlStringWithoutPaddingAndLengthBetween43And128()
+        {
+            var generator = new OAuth2CryptographicCodeGenerator();
+
+            // Create a bunch of verifiers
+            var verifiers = new string[32];
+            for (int i = 0; i < verifiers.Length; i++)
+            {
+                string v = generator.CreatePkceCodeVerifier();
+
+                // Assert the verifier is a base64url string without padding
+                char[] vs = v.ToCharArray();
+                Assert.All(vs, x => Assert.Contains(x, ValidBase64UrlCharsNoPad));
+
+                // Assert the verifier is a string of length [43, 128] (inclusive)
+                Assert.InRange(v.Length, 43, 128);
+
+                verifiers[i] = v;
+            }
+
+            // There should be no duplicates
+            string[] uniqueVerifiers = verifiers.Distinct().ToArray();
+            Assert.Equal(uniqueVerifiers, verifiers);
+        }
+
+        [Fact]
+        public void OAuth2CryptographicCodeGenerator_CreatePkceCodeChallenge_Plain_ReturnsVerifierUnchanged()
+        {
+            var generator = new OAuth2CryptographicCodeGenerator();
+
+            var verifier = generator.CreatePkceCodeVerifier();
+            var challenge = generator.CreatePkceCodeChallenge(OAuth2PkceChallengeMethod.Plain, verifier);
+
+            Assert.Equal(verifier, challenge);
+        }
+
+        [Fact]
+        public void OAuth2CryptographicCodeGenerator_CreatePkceCodeChallenge_Sha256_ReturnsBase64UrlEncodedSha256HashOfAsciiVerifier()
+        {
+            var generator = new OAuth2CryptographicCodeGenerator();
+
+            var verifier = generator.CreatePkceCodeVerifier();
+
+            byte[] verifierAsciiBytes = Encoding.ASCII.GetBytes(verifier);
+            byte[] hashedBytes;
+            using (var sha256 = SHA256.Create())
+            {
+                hashedBytes = sha256.ComputeHash(verifierAsciiBytes);
+            }
+
+            var expectedChallenge = Base64UrlConvert.Encode(hashedBytes, false);
+            var actualChallenge = generator.CreatePkceCodeChallenge(OAuth2PkceChallengeMethod.Sha256, verifier);
+
+            Assert.Equal(expectedChallenge, actualChallenge);
+        }
+    }
+}
diff --git a/src/shared/Microsoft.Git.CredentialManager.Tests/Base64UrlConvertTests.cs b/src/shared/Microsoft.Git.CredentialManager.Tests/Base64UrlConvertTests.cs
@@ -0,0 +1,37 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT license.
+using Xunit;
+
+namespace Microsoft.Git.CredentialManager.Tests
+{
+    public class Base64UrlConvertTests
+    {
+        [Theory]
+        [InlineData(new byte[0], "")]
+        [InlineData(new byte[]{4}, "BA==")]
+        [InlineData(new byte[]{4,5}, "BAU=")]
+        [InlineData(new byte[]{4,5,6}, "BAUG")]
+        [InlineData(new byte[]{4,5,6,7}, "BAUGBw==")]
+        [InlineData(new byte[]{4,5,6,7,8}, "BAUGBwg=")]
+        [InlineData(new byte[]{4,5,6,7,8,9}, "BAUGBwgJ")]
+        public void Base64UrlConvert_Encode_WithPadding(byte[] data, string expected)
+        {
+            string actual = Base64UrlConvert.Encode(data, includePadding: true);
+            Assert.Equal(expected, actual);
+        }
+
+        [Theory]
+        [InlineData(new byte[0], "")]
+        [InlineData(new byte[]{4}, "BA")]
+        [InlineData(new byte[]{4,5}, "BAU")]
+        [InlineData(new byte[]{4,5,6}, "BAUG")]
+        [InlineData(new byte[]{4,5,6,7}, "BAUGBw")]
+        [InlineData(new byte[]{4,5,6,7,8}, "BAUGBwg")]
+        [InlineData(new byte[]{4,5,6,7,8,9}, "BAUGBwgJ")]
+        public void Base64UrlConvert_Encode_WithoutPadding(byte[] data, string expected)
+        {
+            string actual = Base64UrlConvert.Encode(data, includePadding: false);
+            Assert.Equal(expected, actual);
+        }
+    }
+}
diff --git a/src/shared/Microsoft.Git.CredentialManager/Authentication/OAuth/OAuth2AuthorizationCodeResult.cs b/src/shared/Microsoft.Git.CredentialManager/Authentication/OAuth/OAuth2AuthorizationCodeResult.cs
@@ -0,0 +1,18 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT license.
+
+namespace Microsoft.Git.CredentialManager.Authentication.OAuth
+{
+    public class OAuth2AuthorizationCodeResult
+    {
+        public OAuth2AuthorizationCodeResult(string code, string codeVerifier = null)
+        {
+            Code = code;
+            CodeVerifier = codeVerifier;
+        }
+
+        public string Code { 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
@@ -2,10 +2,8 @@
 // Licensed under the MIT license.
 using System;
 using System.Collections.Generic;
-using System.Net;
 using System.Net.Http;
 using System.Net.Http.Headers;
-using System.Text;
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.Git.CredentialManager.Authentication.OAuth.Json;
@@ -26,7 +24,7 @@ public interface IOAuth2Client
         /// <param name="browser">User agent to use to start the authorization code grant flow.</param>
         /// <param name="ct">Token to cancel the operation.</param>
         /// <returns>Authorization code.</returns>
-        Task<string> GetAuthorizationCodeAsync(IEnumerable<string> scopes, IOAuth2WebBrowser browser, CancellationToken ct);
+        Task<OAuth2AuthorizationCodeResult> GetAuthorizationCodeAsync(IEnumerable<string> scopes, IOAuth2WebBrowser browser, CancellationToken ct);
 
         /// <summary>
         /// Retrieve a device code grant.
@@ -40,10 +38,10 @@ public interface IOAuth2Client
         /// <summary>
         /// Exchange an authorization code acquired from <see cref="GetAuthorizationCodeAsync"/> for an access token.
         /// </summary>
-        /// <param name="authorizationCode">Authorization code.</param>
+        /// <param name="authorizationCodeResult">Authorization code grant result.</param>
         /// <param name="ct">Token to cancel the operation.</param>
         /// <returns>Token result.</returns>
-        Task<OAuth2TokenResult> GetTokenByAuthorizationCodeAsync(string authorizationCode, CancellationToken ct);
+        Task<OAuth2TokenResult> GetTokenByAuthorizationCodeAsync(OAuth2AuthorizationCodeResult authorizationCodeResult, CancellationToken ct);
 
         /// <summary>
         /// Use a refresh token to get a new access token.
@@ -70,7 +68,7 @@ public class OAuth2Client : IOAuth2Client
         private readonly string _clientId;
         private readonly string _clientSecret;
 
-        private IOAuth2NonceGenerator _nonceGenerator;
+        private IOAuth2CodeGenerator _codeGenerator;
 
         public OAuth2Client(HttpClient httpClient, OAuth2ServerEndpoints endpoints, string clientId, Uri redirectUri = null, string clientSecret = null)
         {
@@ -81,31 +79,37 @@ public OAuth2Client(HttpClient httpClient, OAuth2ServerEndpoints endpoints, stri
             _clientSecret = clientSecret;
         }
 
-        public IOAuth2NonceGenerator NonceGenerator
+        public IOAuth2CodeGenerator CodeGenerator
         {
-            get => _nonceGenerator ?? (_nonceGenerator = new OAuth2NonceGenerator());
-            set => _nonceGenerator = value;
+            get => _codeGenerator ?? (_codeGenerator = new OAuth2CryptographicCodeGenerator());
+            set => _codeGenerator = value;
         }
 
         #region IOAuth2Client
 
-        public async Task<string> GetAuthorizationCodeAsync(IEnumerable<string> scopes, IOAuth2WebBrowser browser, CancellationToken ct)
+        public async Task<OAuth2AuthorizationCodeResult> GetAuthorizationCodeAsync(IEnumerable<string> scopes, IOAuth2WebBrowser browser, CancellationToken ct)
         {
-            string state = NonceGenerator.CreateNonce();
-            string scopesStr = string.Join(" ", scopes);
+            string state = CodeGenerator.CreateNonce();
+            string codeVerifier = CodeGenerator.CreatePkceCodeVerifier();
+            string codeChallenge = CodeGenerator.CreatePkceCodeChallenge(OAuth2PkceChallengeMethod.Sha256, codeVerifier);
 
             var queryParams = new Dictionary<string, string>
             {
-                [OAuth2Constants.AuthorizationEndpoint.ResponseTypeParameter] = OAuth2Constants.AuthorizationEndpoint.AuthorizationCodeResponseType,
+                [OAuth2Constants.AuthorizationEndpoint.ResponseTypeParameter] =
+                    OAuth2Constants.AuthorizationEndpoint.AuthorizationCodeResponseType,
                 [OAuth2Constants.ClientIdParameter] = _clientId,
                 [OAuth2Constants.AuthorizationEndpoint.StateParameter] = state,
+                [OAuth2Constants.AuthorizationEndpoint.PkceChallengeMethodParameter] =
+                    OAuth2Constants.AuthorizationEndpoint.PkceChallengeMethodS256,
+                [OAuth2Constants.AuthorizationEndpoint.PkceChallengeParameter] = codeChallenge
             };
 
             if (_redirectUri != null)
             {
                 queryParams[OAuth2Constants.RedirectUriParameter] = _redirectUri.ToString();
             }
 
+            string scopesStr = string.Join(" ", scopes);
             if (!string.IsNullOrWhiteSpace(scopesStr))
             {
                 queryParams[OAuth2Constants.ScopeParameter] = scopesStr;
@@ -140,7 +144,7 @@ public async Task<string> GetAuthorizationCodeAsync(IEnumerable<string> scopes,
                 throw new OAuth2Exception($"Missing '{OAuth2Constants.AuthorizationGrantResponse.AuthorizationCodeParameter}' in response.");
             }
 
-            return authCode;
+            return new OAuth2AuthorizationCodeResult(authCode, codeVerifier);
         }
 
         public async Task<OAuth2DeviceCodeResult> GetDeviceCodeAsync(IEnumerable<string> scopes, CancellationToken ct)
@@ -177,15 +181,21 @@ public async Task<OAuth2DeviceCodeResult> GetDeviceCodeAsync(IEnumerable<string>
             }
         }
 
-        public async Task<OAuth2TokenResult> GetTokenByAuthorizationCodeAsync(string authorizationCode, CancellationToken ct)
+        public async Task<OAuth2TokenResult> GetTokenByAuthorizationCodeAsync(OAuth2AuthorizationCodeResult authorizationCodeResult, CancellationToken ct)
         {
             var formData = new Dictionary<string, string>
             {
                 [OAuth2Constants.TokenEndpoint.GrantTypeParameter] = OAuth2Constants.TokenEndpoint.AuthorizationCodeGrantType,
-                [OAuth2Constants.TokenEndpoint.AuthorizationCodeParameter] = authorizationCode,
-                [OAuth2Constants.ClientIdParameter] = _clientId,
+                [OAuth2Constants.TokenEndpoint.AuthorizationCodeParameter] = authorizationCodeResult.Code,
+                [OAuth2Constants.TokenEndpoint.PkceVerifierParameter] = authorizationCodeResult.CodeVerifier,
+                [OAuth2Constants.ClientIdParameter] = _clientId
             };
 
+            if (authorizationCodeResult.CodeVerifier != null)
+            {
+                formData[OAuth2Constants.TokenEndpoint.PkceVerifierParameter] = authorizationCodeResult.CodeVerifier;
+            }
+
             if (_redirectUri != null)
             {
                 formData[OAuth2Constants.RedirectUriParameter] = _redirectUri.ToString();
diff --git a/src/shared/Microsoft.Git.CredentialManager/Authentication/OAuth/OAuth2Constants.cs b/src/shared/Microsoft.Git.CredentialManager/Authentication/OAuth/OAuth2Constants.cs
@@ -14,6 +14,10 @@ public static class AuthorizationEndpoint
             public const string StateParameter = "state";
             public const string AuthorizationCodeResponseType = "code";
             public const string ResponseTypeParameter = "response_type";
+            public const string PkceChallengeParameter = "code_challenge";
+            public const string PkceChallengeMethodParameter = "code_challenge_method";
+            public const string PkceChallengeMethodPlain = "plain";
+            public const string PkceChallengeMethodS256 = "S256";
         }
 
         public static class AuthorizationGrantResponse
@@ -30,7 +34,7 @@ public static class TokenEndpoint
             public const string GrantTypeParameter = "grant_type";
             public const string AuthorizationCodeGrantType = "authorization_code";
             public const string RefreshTokenGrantType = "refresh_token";
-
+            public const string PkceVerifierParameter = "code_verifier";
             public const string AuthorizationCodeParameter = "code";
             public const string RefreshTokenParameter = "refresh_token";
         }
diff --git a/src/shared/Microsoft.Git.CredentialManager/Authentication/OAuth/OAuth2CryptographicGenerator.cs b/src/shared/Microsoft.Git.CredentialManager/Authentication/OAuth/OAuth2CryptographicGenerator.cs
diff --git a/src/shared/Microsoft.Git.CredentialManager/Authentication/OAuth/OAuth2NonceGenerator.cs b/src/shared/Microsoft.Git.CredentialManager/Authentication/OAuth/OAuth2NonceGenerator.cs
diff --git a/src/shared/Microsoft.Git.CredentialManager/Base64UrlConvert.cs b/src/shared/Microsoft.Git.CredentialManager/Base64UrlConvert.cs
diff --git a/src/shared/TestInfrastructure/Objects/TestOAuth2Server.cs b/src/shared/TestInfrastructure/Objects/TestOAuth2Server.cs

Original file line number	Diff line number	Diff line change
`@@ -203,9 +203,9 @@ public async Task<OAuth2TokenResult> GetOAuthTokenAsync(Uri targetUri, IEnumerab`
`203`	`203`	`// Write message to the terminal (if any is attached) for some feedback that we're waiting for a web response`
`204`	`204`	`Context.Terminal.WriteLine("info: please complete authentication in your browser...");`
`205`	`205`
`206`		`- string authCode = await oauthClient.GetAuthorizationCodeAsync(scopes, browser, CancellationToken.None);`
	`206`	`+ OAuth2AuthorizationCodeResult authCodeResult = await oauthClient.GetAuthorizationCodeAsync(scopes, browser, CancellationToken.None);`
`207`	`207`
`208`		`- return await oauthClient.GetTokenByAuthorizationCodeAsync(authCode, CancellationToken.None);`
	`208`	`+ return await oauthClient.GetTokenByAuthorizationCodeAsync(authCodeResult, CancellationToken.None);`
`209`	`209`	`}`
`210`	`210`	`else`
`211`	`211`	`{`