feat: Support PKCE #2435

amanda-tarafa · 2023-06-19T14:08:09Z

amanda-tarafa · 2023-06-19T14:08:57Z

@jskeet I will ad a few tests to this etc. but just wanted to check with you that this approach looks good.

jskeet

I think I basically follow this. Various comments, but nothing huge.

jskeet · 2023-06-19T15:01:48Z

Src/Support/Google.Apis.Auth/OAuth2/Flows/IPkceAuthorizationCodeFlow.cs

+        /// The redirect URI for the authorization code request.
+        /// </param>
+        /// <param name="codeVerifier">
+        /// The code verifier associated to the code challenge that should be included


Maybe highlight that this is an out parameter?

jskeet · 2023-06-19T15:02:15Z

Src/Support/Google.Apis.Auth/OAuth2/Flows/IPkceAuthorizationCodeFlow.cs

+        /// in the returned <see cref="AuthorizationCodeRequestUrl"/>.
+        /// </param>
+        /// <returns>An <see cref="AuthorizationCodeRequestUrl"/> subclass instance that includes the code challenge
+        /// and code challange method associated to <paramref name="codeVerifier"/>.</returns>


challange => challenge and possibly "associated to" => "associated with"

Done, both.

jskeet · 2023-06-19T15:02:49Z

Src/Support/Google.Apis.Auth/OAuth2/Flows/IPkceAuthorizationCodeFlow.cs

+        /// and code challange method associated to <paramref name="codeVerifier"/>.</returns>
+        AuthorizationCodeRequestUrl CreateAuthorizationCodeRequest(string redirectUri, out string codeVerifier);
+
+        /// <summary>Asynchronously exchanges code with a token.</summary>


"code with a token" => "an authorization code for an access token"

Done here, and from where I copied this.

jskeet · 2023-06-19T15:03:27Z

Src/Support/Google.Apis.Auth/OAuth2/Flows/IPkceAuthorizationCodeFlow.cs

+        /// <param name="code">Authorization code received from the authorization server.</param>
+        /// <param name="codeVerifier">
+        /// The PKCE code verifier to send along the exchange request.
+        /// You obtained the code verifier when calling <see cref="CreateAuthorizationCodeRequest(string, out string)"/>


Let's revisit this sentence when the rest is ready to go.

OK, I'm adding a TODO so we don't forget.

Right, how about:

The PKCE code verifier to include in the exchange request. When called by the authentication library, this will be the same value specified by the <code>codeVerifier</code> out parameter in an earlier call to CreateAuthorizationCodeRequest.

Yes, better, done.

jskeet · 2023-06-19T15:03:38Z

Src/Support/Google.Apis.Auth/OAuth2/Flows/IPkceAuthorizationCodeFlow.cs

@@ -0,0 +1,62 @@
+/*
+Copyright 2013 Google Inc


2023, here and below

jskeet · 2023-06-19T15:06:25Z

Src/Support/Google.Apis.Auth/OAuth2/Flows/PkceGoogleAuthorizationCodeFlow.cs

+            {
+                byte[] codeVerifierAsciiBytes = Encoding.ASCII.GetBytes(codeVerifier);
+                byte[] hashedCodeVerifier;
+                using (var sha256 = SHA256.Create())


Simplify the using statement? (That would allow us to declare hashedCodeVerifier at the point of assignment.)

jskeet · 2023-06-19T15:06:52Z

Src/Support/Google.Apis.Auth/OAuth2/Flows/PkceGoogleAuthorizationCodeFlow.cs

+            request.CodeChallengeMethod = effectiveChallengeMethod;
+        }
+
+        private static string Base64UrlEncode(byte[] data)


Just checking that we don't have this anywhere else?

Actually we do, and it did ring a bell when I wrote it.

jskeet · 2023-06-19T15:07:20Z

Src/Support/Google.Apis.Auth/OAuth2/Flows/PkceGoogleAuthorizationCodeFlow.cs

+        private static string GenerateCodeVerifier()
+        {
+            byte[] data = new byte[CodeVerifierRandomNumberLengthBytes];
+            using(RandomNumberGenerator randomNumberGenerator = RandomNumberGenerator.Create())


Nit: space after using, or better, let's just use a simplified using statement.

Yep, just we really don't need it until the end, but that shouldn't be a problem really.

jskeet · 2023-06-19T15:10:37Z

Src/Support/Google.Apis.Auth/OAuth2/GoogleWebAuthorizationBroker.cs

+        /// The scopes which indicate the Google API access your application is requesting.
+        /// </param>
+        /// <param name="user">The user to authorize.</param>
+        /// <param name="disablePkce">


I'd prefer to have usePkce if possible - 1) it avoids the term "disable", and 2) it's more positive. I'm okay with this if it's for consistency with other languages' already-released packages though.

No, it's fine, I'll swap it.

My intention was mostly to make it explicit that not using PKCE should be the exemption.

jskeet · 2023-06-19T15:11:20Z

Src/Support/Google.Apis.Auth/OAuth2/Requests/GoogleAuthorizationCodeRequestUrl.cs

@@ -86,8 +103,8 @@ public class GoogleAuthorizationCodeRequestUrl : AuthorizationCodeRequestUrl
        /// The name of this parameter is used only for the constructor and will not end up in the resultant query
        /// string.
        /// </remarks>
-        [Google.Apis.Util.RequestParameterAttribute("user_defined_query_params",
-            Google.Apis.Util.RequestParameterType.UserDefinedQueries)]
+        [RequestParameter("user_defined_query_params",


Unwrap line?

amanda-tarafa

All comments addressed and tests added.
I've also split in 4 commits to make it easier to review.
@jskeet PTAL, thanks!

amanda-tarafa · 2023-06-19T21:24:30Z

Src/Support/Google.Apis.Auth/OAuth2/Flows/IPkceAuthorizationCodeFlow.cs

@@ -0,0 +1,62 @@
+/*
+Copyright 2013 Google Inc


amanda-tarafa · 2023-06-19T21:25:51Z

Src/Support/Google.Apis.Auth/OAuth2/Flows/IPkceAuthorizationCodeFlow.cs

+        /// The redirect URI for the authorization code request.
+        /// </param>
+        /// <param name="codeVerifier">
+        /// The code verifier associated to the code challenge that should be included


amanda-tarafa · 2023-06-19T21:26:27Z

Src/Support/Google.Apis.Auth/OAuth2/Flows/IPkceAuthorizationCodeFlow.cs

+        /// in the returned <see cref="AuthorizationCodeRequestUrl"/>.
+        /// </param>
+        /// <returns>An <see cref="AuthorizationCodeRequestUrl"/> subclass instance that includes the code challenge
+        /// and code challange method associated to <paramref name="codeVerifier"/>.</returns>


Done, both.

amanda-tarafa · 2023-06-19T21:27:56Z

Src/Support/Google.Apis.Auth/OAuth2/Flows/IPkceAuthorizationCodeFlow.cs

+        /// and code challange method associated to <paramref name="codeVerifier"/>.</returns>
+        AuthorizationCodeRequestUrl CreateAuthorizationCodeRequest(string redirectUri, out string codeVerifier);
+
+        /// <summary>Asynchronously exchanges code with a token.</summary>


Done here, and from where I copied this.

amanda-tarafa · 2023-06-19T21:29:09Z

Src/Support/Google.Apis.Auth/OAuth2/Flows/IPkceAuthorizationCodeFlow.cs

+        /// <param name="code">Authorization code received from the authorization server.</param>
+        /// <param name="codeVerifier">
+        /// The PKCE code verifier to send along the exchange request.
+        /// You obtained the code verifier when calling <see cref="CreateAuthorizationCodeRequest(string, out string)"/>


OK, I'm adding a TODO so we don't forget.

amanda-tarafa · 2023-06-19T21:36:45Z

Src/Support/Google.Apis.Auth/OAuth2/Flows/PkceGoogleAuthorizationCodeFlow.cs

+            {
+                byte[] codeVerifierAsciiBytes = Encoding.ASCII.GetBytes(codeVerifier);
+                byte[] hashedCodeVerifier;
+                using (var sha256 = SHA256.Create())


amanda-tarafa · 2023-06-19T21:41:00Z

Src/Support/Google.Apis.Auth/OAuth2/Flows/PkceGoogleAuthorizationCodeFlow.cs

+                effectiveChallenge = Base64UrlEncode(hashedCodeVerifier);
+                effectiveChallengeMethod = ChallengeMethodSha256;
+            }
+            catch


Yep, there's this one case on .NET 4.61, but I'll be more explicit about the exception: https://learn.microsoft.com/en-us/dotnet/api/system.security.cryptography.sha256.create

amanda-tarafa · 2023-06-19T21:42:07Z

Src/Support/Google.Apis.Auth/OAuth2/Flows/PkceGoogleAuthorizationCodeFlow.cs

+            request.CodeChallengeMethod = effectiveChallengeMethod;
+        }
+
+        private static string Base64UrlEncode(byte[] data)


Actually we do, and it did ring a bell when I wrote it.

amanda-tarafa · 2023-06-19T21:45:11Z

Src/Support/Google.Apis.Auth/OAuth2/GoogleWebAuthorizationBroker.cs

+        /// The scopes which indicate the Google API access your application is requesting.
+        /// </param>
+        /// <param name="user">The user to authorize.</param>
+        /// <param name="disablePkce">


No, it's fine, I'll swap it.

My intention was mostly to make it explicit that not using PKCE should be the exemption.

amanda-tarafa · 2023-06-19T21:49:04Z

Src/Support/Google.Apis.Auth/OAuth2/Requests/GoogleAuthorizationCodeRequestUrl.cs

@@ -86,8 +103,8 @@ public class GoogleAuthorizationCodeRequestUrl : AuthorizationCodeRequestUrl
        /// The name of this parameter is used only for the constructor and will not end up in the resultant query
        /// string.
        /// </remarks>
-        [Google.Apis.Util.RequestParameterAttribute("user_defined_query_params",
-            Google.Apis.Util.RequestParameterType.UserDefinedQueries)]
+        [RequestParameter("user_defined_query_params",


jskeet

Looks good.

jskeet · 2023-06-20T10:37:09Z

Src/Support/Google.Apis.Auth/OAuth2/Flows/IPkceAuthorizationCodeFlow.cs

+        /// <param name="code">Authorization code received from the authorization server.</param>
+        /// <param name="codeVerifier">
+        /// The PKCE code verifier to send along the exchange request.
+        /// You obtained the code verifier when calling <see cref="CreateAuthorizationCodeRequest(string, out string)"/>


Right, how about:

The PKCE code verifier to include in the exchange request. When called by the authentication library, this will be the same value specified by the <code>codeVerifier</code> out parameter in an earlier call to CreateAuthorizationCodeRequest.

jskeet · 2023-06-20T10:39:36Z

Src/Support/Google.Apis.Auth/OAuth2/Flows/PkceGoogleAuthorizationCodeFlow.cs

+            using RandomNumberGenerator randomNumberGenerator = RandomNumberGenerator.Create();
+            randomNumberGenerator.GetBytes(data);
+
+            // The code verifier alphabet does not allow +, / or =, so we Base64URL encode.


I think it's more that base64-URL-encoding is simply how the code verifier is specified in the RFC.

Yes, true, it just doesn't say so anywhere in our docs, just what the alphabet is, so I unconsciously followed along. Changing.

jskeet · 2023-06-20T10:40:09Z

Src/Support/Google.Apis.Auth/OAuth2/Flows/PkceGoogleAuthorizationCodeFlow.cs

+                effectiveChallenge = TokenEncodingHelpers.UrlSafeBase64Encode(hashedCodeVerifier);
+                effectiveChallengeMethod = ChallengeMethodSha256;
+            }
+            catch (TargetInvocationException)


Possibly add a comment here as you did in the GitHub conversation to explain why this might happen?

This PKCE flow is not used in this commit.

amanda-tarafa

Comments addressed, thanks!

I'll merge on green.

amanda-tarafa · 2023-06-20T11:04:05Z

Src/Support/Google.Apis.Auth/OAuth2/Flows/IPkceAuthorizationCodeFlow.cs

+        /// <param name="code">Authorization code received from the authorization server.</param>
+        /// <param name="codeVerifier">
+        /// The PKCE code verifier to send along the exchange request.
+        /// You obtained the code verifier when calling <see cref="CreateAuthorizationCodeRequest(string, out string)"/>


Yes, better, done.

amanda-tarafa · 2023-06-20T11:05:37Z

Src/Support/Google.Apis.Auth/OAuth2/Flows/PkceGoogleAuthorizationCodeFlow.cs

+            using RandomNumberGenerator randomNumberGenerator = RandomNumberGenerator.Create();
+            randomNumberGenerator.GetBytes(data);
+
+            // The code verifier alphabet does not allow +, / or =, so we Base64URL encode.


Yes, true, it just doesn't say so anywhere in our docs, just what the alphabet is, so I unconsciously followed along. Changing.

amanda-tarafa · 2023-06-20T11:08:11Z

Src/Support/Google.Apis.Auth/OAuth2/Flows/PkceGoogleAuthorizationCodeFlow.cs

+                effectiveChallenge = TokenEncodingHelpers.UrlSafeBase64Encode(hashedCodeVerifier);
+                effectiveChallengeMethod = ChallengeMethodSha256;
+            }
+            catch (TargetInvocationException)


clundin25 · 2023-06-20T15:40:43Z

@amanda-tarafa this is great, thanks!

Updates support version: 1.61.0 - beta02 -> 1.61.0 Features - Improvements for date/time parsing. - googleapis#2441 - googleapis#2432 - googleapis#2429 - googleapis#2435 PKCE support. - googleapis#2394 Which improves impersonation support. - googleapis#2349 and googleapis#2379 Which improve error reported when ADC is not configured.

Updates support version: 1.61.0 - beta02 -> 1.61.0 Features - Improvements for date/time parsing. - #2441 - #2432 - #2429 - #2435 PKCE support. - #2394 Which improves impersonation support. - #2349 and #2379 Which improve error reported when ADC is not configured.

amanda-tarafa requested a review from jskeet June 19, 2023 14:08

amanda-tarafa requested a review from a team as a code owner June 19, 2023 14:08

jskeet reviewed Jun 19, 2023

View reviewed changes

refactor: Prepares existing code for PKCE support.

b8fb3ff

amanda-tarafa force-pushed the pkce branch from d675d5e to d8898d5 Compare June 20, 2023 09:51

amanda-tarafa commented Jun 20, 2023

View reviewed changes

jskeet approved these changes Jun 20, 2023

View reviewed changes

amanda-tarafa added 3 commits June 20, 2023 12:09

feat: Implements a PKCE flow.

1442197

This PKCE flow is not used in this commit.

feat: Makes AuthorizationCodeInstalledApp support PKCE.

bd4deea

feat: Makes GoogleWebAuthorizationBroker use PKCE by default

4c266a0

amanda-tarafa force-pushed the pkce branch from d8898d5 to 4c266a0 Compare June 20, 2023 11:10

amanda-tarafa commented Jun 20, 2023

View reviewed changes

amanda-tarafa merged commit abf24a9 into googleapis:main Jun 20, 2023

amanda-tarafa deleted the pkce branch June 20, 2023 11:12

amanda-tarafa mentioned this pull request Jun 26, 2023

Update version: 1.60.0 -> 1.61.0 #2442

Merged

feat: Support PKCE #2435

feat: Support PKCE #2435

Conversation

amanda-tarafa commented Jun 19, 2023

amanda-tarafa commented Jun 19, 2023

jskeet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanda-tarafa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jskeet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanda-tarafa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clundin25 commented Jun 20, 2023