feat: ComputeCredentials implements IBlobSigner #2308

amanda-tarafa · 2023-01-16T15:12:41Z

No description provided.

amanda-tarafa · 2023-01-16T15:13:30Z

I've set the do not merge so that I can properly test this code with Storage URL signing before merging.
But this shouls be ready for review.

jskeet

Generally fine - some nits, but nothing particularly significant. Happy to discuss the property vs method for the service account email.

jskeet · 2023-01-16T16:43:56Z

Src/Support/Google.Apis.Auth.Tests/OAuth2/ComputeCredentialTests.cs

+                {
+                    Content = new StringContent(dummyServiceAccountEmail)
+                });
+        }


Nit: indent

Yep, this was fixed on a later commit on the same PR.

jskeet · 2023-01-16T16:45:49Z

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

+        /// instance if that happens so fetching the service account default email is re-attempted.
+        /// </para>
+        /// </remarks>
+        public Task<string> DefaultServiceAccountEmailTask => _defaultServiceAccountEmailCache.Value;


It feels slightly odd to have a property that kicks of an HTTP request.

Could we make this a method? It can still be lazy, but GetDefaultServiceAccountEmailAsync() feels a little cleaner to me.

Yep, agreed.

jskeet · 2023-01-16T16:46:39Z

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

+        /// The effective Compute Engine default service account email URL.
+        /// This takes account of the GCE_METADATA_HOST environment variable.
+        /// </summary>
+        internal static string EffectiveComputeDefaultServiceAccountEmailUrl => GetEffectiveMetadataUrl(ComputeDefaultServiceAccountEmailSuffix, DefaultMetadataServerUrl + ComputeDefaultServiceAccountEmailSuffix);


Wrap after =>?

jskeet · 2023-01-16T16:49:37Z

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

+
+        /// <summary>
+        /// Creates and HTTP form from <paramref name="request"/> and posts it to <paramref name="url"/>.
+        /// If <paramref name="authenticationHeaderValue"/> is not null, it's value it's included as the


"it's value it's => "its value is"

jskeet · 2023-01-16T16:50:05Z

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

+            {
+                Content = ParameterUtils.CreateFormUrlEncodedContent(request)
+            };
+            httpRequest.Headers.Authorization = authenticationHeaderValue;


Is this still okay if authenticationHeaderValue is null?

Oh nice catch. It is OK now because there's no default value for the Auth header, but best to only change it if there's something to change it to.

jskeet · 2023-01-16T16:53:58Z

Src/Support/Google.Apis.Auth.Tests/OAuth2/ComputeCredentialTests.cs

+            var initializer = new ComputeCredential.Initializer("http://will.be.ignored", "http://will.be.ignored")
+            {
+                HttpClientFactory = new MockHttpClientFactory(messageHandler),
+                Clock= clock


Nit: space before =

jskeet · 2023-01-16T16:54:38Z

Src/Support/Google.Apis.Auth.Tests/OAuth2/GoogleCredentialTests.cs

@@ -693,14 +693,5 @@ public async Task SignBlobAsync()
            var signature = await credential.SignBlobAsync(Encoding.ASCII.GetBytes("toSign"));
            Assert.Equal("VvQqmCec5sESTXmt3ojPodmqZhV30MffzuyCvz6DatyW4uO9IMLWbUmynPYNNnn6w0EIVV0CyrARd56VLxgL+DVdsX726W3dfY13nNJo7i8PyV8R2i8yRLimqqraa1fDb29V2n+FsS0OzBPibDscViQHVlu0PBRApsVsjGG+eB4=", signature);
        }
-
-        [Fact]
-        public async Task SignBlobAsync_UnsupportedCredential()


Keep the test, just change the credential type?

jskeet · 2023-01-16T16:55:09Z

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

+        /// For instance, to perform IAM API calls for signing blobs of data.
+        /// </summary>
+        /// <remarks>Lazy to build one HtppClient only if it is needed.</remarks>
+        private readonly Lazy<ConfigurableHttpClient> _authenticatedHtppClient;


Htpp => Http

jskeet · 2023-01-16T16:55:31Z

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

@@ -234,6 +244,13 @@ private async Task<string> GetDefaultServiceAccountEmail()
            return await response.Content.ReadAsStringAsync().ConfigureAwait(false);
        }

+        private ConfigurableHttpClient BuildAuthenticatedHttpClient()
+        {
+            var httpClietnArgs = BuildCreateHtppClientArgs();


Clietn => Client

jskeet · 2023-01-16T16:56:13Z

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

+        /// </remarks>
+        public async Task<string> SignBlobAsync(byte[] blob, CancellationToken cancellationToken = default)
+        {
+            var request = new IamSignBlobRequest


Possibly unwrap to a single line?

amanda-tarafa

All comments adressed on the last commit. PTAL

amanda-tarafa · 2023-01-16T21:31:18Z

Src/Support/Google.Apis.Auth.Tests/OAuth2/ComputeCredentialTests.cs

+                {
+                    Content = new StringContent(dummyServiceAccountEmail)
+                });
+        }


Yep, this was fixed on a later commit on the same PR.

amanda-tarafa · 2023-01-16T21:34:34Z

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

+        /// instance if that happens so fetching the service account default email is re-attempted.
+        /// </para>
+        /// </remarks>
+        public Task<string> DefaultServiceAccountEmailTask => _defaultServiceAccountEmailCache.Value;


Yep, agreed.

amanda-tarafa · 2023-01-16T21:35:19Z

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

+        /// The effective Compute Engine default service account email URL.
+        /// This takes account of the GCE_METADATA_HOST environment variable.
+        /// </summary>
+        internal static string EffectiveComputeDefaultServiceAccountEmailUrl => GetEffectiveMetadataUrl(ComputeDefaultServiceAccountEmailSuffix, DefaultMetadataServerUrl + ComputeDefaultServiceAccountEmailSuffix);


amanda-tarafa · 2023-01-16T21:38:13Z

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

+
+        /// <summary>
+        /// Creates and HTTP form from <paramref name="request"/> and posts it to <paramref name="url"/>.
+        /// If <paramref name="authenticationHeaderValue"/> is not null, it's value it's included as the


amanda-tarafa · 2023-01-16T21:39:02Z

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

+            {
+                Content = ParameterUtils.CreateFormUrlEncodedContent(request)
+            };
+            httpRequest.Headers.Authorization = authenticationHeaderValue;


Oh nice catch. It is OK now because there's no default value for the Auth header, but best to only change it if there's something to change it to.

amanda-tarafa · 2023-01-16T22:12:29Z

Src/Support/Google.Apis.Auth.Tests/OAuth2/ComputeCredentialTests.cs

+            var initializer = new ComputeCredential.Initializer("http://will.be.ignored", "http://will.be.ignored")
+            {
+                HttpClientFactory = new MockHttpClientFactory(messageHandler),
+                Clock= clock


amanda-tarafa · 2023-01-16T22:12:49Z

Src/Support/Google.Apis.Auth.Tests/OAuth2/GoogleCredentialTests.cs

@@ -693,14 +693,5 @@ public async Task SignBlobAsync()
            var signature = await credential.SignBlobAsync(Encoding.ASCII.GetBytes("toSign"));
            Assert.Equal("VvQqmCec5sESTXmt3ojPodmqZhV30MffzuyCvz6DatyW4uO9IMLWbUmynPYNNnn6w0EIVV0CyrARd56VLxgL+DVdsX726W3dfY13nNJo7i8PyV8R2i8yRLimqqraa1fDb29V2n+FsS0OzBPibDscViQHVlu0PBRApsVsjGG+eB4=", signature);
        }
-
-        [Fact]
-        public async Task SignBlobAsync_UnsupportedCredential()


amanda-tarafa · 2023-01-16T22:16:32Z

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

+        /// For instance, to perform IAM API calls for signing blobs of data.
+        /// </summary>
+        /// <remarks>Lazy to build one HtppClient only if it is needed.</remarks>
+        private readonly Lazy<ConfigurableHttpClient> _authenticatedHtppClient;


amanda-tarafa · 2023-01-16T22:17:19Z

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

@@ -234,6 +244,13 @@ private async Task<string> GetDefaultServiceAccountEmail()
            return await response.Content.ReadAsStringAsync().ConfigureAwait(false);
        }

+        private ConfigurableHttpClient BuildAuthenticatedHttpClient()
+        {
+            var httpClietnArgs = BuildCreateHtppClientArgs();


amanda-tarafa · 2023-01-16T22:18:41Z

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

+        /// </remarks>
+        public async Task<string> SignBlobAsync(byte[] blob, CancellationToken cancellationToken = default)
+        {
+            var request = new IamSignBlobRequest


jskeet

Another couple of tiny suggestions, but basically all good.

jskeet · 2023-01-17T09:03:03Z

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

+        /// </para>
+        /// <para>
+        /// Note that if, when fetching this value, an exception is thrown, the exception is cached and
+        /// will be rethrown every time this property is accessed. You can create a new <see cref="ComputeCredential"/>


the exception is cached and will be rethrown by the task returned by any future call to this method

Ah yes, true.

jskeet · 2023-01-17T09:03:33Z

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

+        /// The private key associated with the Compute service account is not known locally
+        /// by a ComputeCredential. Signing happens by executing a request to the IAM Credentials API
+        /// which increases latency and counts towards IAM Credentials API quotas. Aditionally, the first
+        /// time a ComputeCredential is used to sign data, a request to the metadata server is execute to


execute => executed (or just "made")

jskeet · 2023-01-17T09:05:00Z

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

+        public async Task<string> SignBlobAsync(byte[] blob, CancellationToken cancellationToken = default)
+        {
+            var request = new IamSignBlobRequest { Payload = blob };
+            var signBlobUrl = string.Format(GoogleAuthConsts.IamSignEndpointFormatString, await GetDefaultServiceAccountEmailAsync(cancellationToken).ConfigureAwait(false));


I wonder whether for the sake of both readability and debugging it's worth separating this out:

var request = new IamSignBlobRequest { Payload = blob }; var serviceAccountEmail = await GetDefaultServiceAccountEmailAsync(cancellationToken).ConfigureAwait(false); var signBlobUrl = string.Format(GoogleAuthConsts.IamSignEndpointFormatString, serviceAccountEmail); ...

Yep, will do that.

amanda-tarafa

Latest comments addressed.

I'll keep the PR open until I've run all Storage tests locally. I'll flag any changes if needed.

amanda-tarafa · 2023-01-18T12:26:35Z

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

+        /// </para>
+        /// <para>
+        /// Note that if, when fetching this value, an exception is thrown, the exception is cached and
+        /// will be rethrown every time this property is accessed. You can create a new <see cref="ComputeCredential"/>


Ah yes, true.

amanda-tarafa · 2023-01-18T12:28:38Z

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

+        /// The private key associated with the Compute service account is not known locally
+        /// by a ComputeCredential. Signing happens by executing a request to the IAM Credentials API
+        /// which increases latency and counts towards IAM Credentials API quotas. Aditionally, the first
+        /// time a ComputeCredential is used to sign data, a request to the metadata server is execute to


amanda-tarafa · 2023-01-18T12:29:12Z

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

+        public async Task<string> SignBlobAsync(byte[] blob, CancellationToken cancellationToken = default)
+        {
+            var request = new IamSignBlobRequest { Payload = blob };
+            var signBlobUrl = string.Format(GoogleAuthConsts.IamSignEndpointFormatString, await GetDefaultServiceAccountEmailAsync(cancellationToken).ConfigureAwait(false));


Yep, will do that.

- This has been commented for a while. - It was not a unit test to start with as detecting Compute residency executes requests against the metadata server.

Only TokeRequestExtensions.ExecuteAsync remains as it was public. But it does rely on the newly generalized ones.

Useful for credentials that require differently configured HttpClients but with the same base configuration.

For which, fetches service account email associated to a ComputeCredential. Closes googleapis#1857 Towards googleapis/google-cloud-dotnet#8413

And support libraries from 1.59.0-alpha01 => 1.59.0 Features: googleapis#2308 ComputeCredential implements IBlobSigner

And support libraries from 1.59.0-alpha01 => 1.59.0 Features: - googleapis#2308 ComputeCredential implements IBlobSigner

And support libraries from 1.59.0-alpha01 => 1.59.0 Features: - #2308 ComputeCredential implements IBlobSigner

amanda-tarafa added the do not merge label Jan 16, 2023

amanda-tarafa requested a review from jskeet January 16, 2023 15:12

amanda-tarafa requested a review from a team as a code owner January 16, 2023 15:12

jskeet approved these changes Jan 16, 2023

View reviewed changes

amanda-tarafa force-pushed the signer-credentials branch from 9f767d6 to c9211a3 Compare January 16, 2023 23:17

amanda-tarafa commented Jan 16, 2023

View reviewed changes

amanda-tarafa mentioned this pull request Jan 16, 2023

feat: Improve credential support for URL signing. googleapis/google-cloud-dotnet#9555

Merged

jskeet approved these changes Jan 17, 2023

View reviewed changes

amanda-tarafa force-pushed the signer-credentials branch from c9211a3 to 4c57b70 Compare January 18, 2023 12:32

amanda-tarafa commented Jan 18, 2023

View reviewed changes

tests: Remove commented out test detecting Compute residency.

ff2b7ec

- This has been commented for a while. - It was not a unit test to start with as detecting Compute residency executes requests against the metadata server.

amanda-tarafa force-pushed the signer-credentials branch 2 times, most recently from f59d8e6 to 98a104c Compare January 19, 2023 11:50

amanda-tarafa removed the do not merge label Jan 19, 2023

amanda-tarafa added 4 commits January 19, 2023 11:55

refactor: Generalize HTTP requests helpers.

edb30dd

Only TokeRequestExtensions.ExecuteAsync remains as it was public. But it does rely on the newly generalized ones.

refactor: Generalize IAM Sign Blob request and response types.

ace0670

refactor: Extract HTTP creation args to a method on ServiceCredential.

2eec1d9

Useful for credentials that require differently configured HttpClients but with the same base configuration.

feat: ComputeCredential implements IBlobSigner.

5b2d850

For which, fetches service account email associated to a ComputeCredential. Closes googleapis#1857 Towards googleapis/google-cloud-dotnet#8413

amanda-tarafa force-pushed the signer-credentials branch from 98a104c to 5b2d850 Compare January 19, 2023 11:56

amanda-tarafa merged commit 8d3534d into googleapis:main Jan 19, 2023

amanda-tarafa deleted the signer-credentials branch January 19, 2023 11:59

amanda-tarafa added a commit to amanda-tarafa/google-api-dotnet-client that referenced this pull request Jan 19, 2023

Update version: 1.58.0 => 1.59.0

cc6f50e

And support libraries from 1.59.0-alpha01 => 1.59.0 Features: googleapis#2308 ComputeCredential implements IBlobSigner

amanda-tarafa added a commit to amanda-tarafa/google-api-dotnet-client that referenced this pull request Jan 19, 2023

Update version: 1.58.0 => 1.59.0

4a94d71

And support libraries from 1.59.0-alpha01 => 1.59.0 Features: - googleapis#2308 ComputeCredential implements IBlobSigner

amanda-tarafa mentioned this pull request Jan 19, 2023

Update version: 1.58.0 => 1.59.0 #2314

Merged

amanda-tarafa added a commit that referenced this pull request Jan 19, 2023

Update version: 1.58.0 => 1.59.0

03aed9e

And support libraries from 1.59.0-alpha01 => 1.59.0 Features: - #2308 ComputeCredential implements IBlobSigner

feat: ComputeCredentials implements IBlobSigner #2308

feat: ComputeCredentials implements IBlobSigner #2308

Conversation

amanda-tarafa commented Jan 16, 2023

amanda-tarafa commented Jan 16, 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

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

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