feat: Add support for file-sourced external credentials. #2206

amanda-tarafa · 2022-09-09T22:47:02Z

Towards #2033.

Also refactors the existing external account credential tests to make it easier to reuse test code.

jskeet

Various nits and a few questions, but basically fine as far as I can see - I'm not a subject-matter expert on this though.

jskeet · 2022-09-12T08:28:50Z

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

+            get => new TheoryData<string, Type>
+            {
+                { DummyUrlSourcedExternalAccountCredentialFileContents, typeof(UrlSourcedExternalAccountCredential) },
+                { DummyFileSourcedExternalAccountCredentialFileContents, typeof (FileSourcedExternalAccountCredential) },


Nit: remove space after typeof. (I wouldn't have noticed it if it hadn't been for the inconsistency with the one above.

(Note: both of these comments apply twice below as well.)

jskeet · 2022-09-12T08:28:57Z

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


-        [Fact]
-        public async Task GetDefaultCredential_UrlSourcedExternalAccountCredential()
+        public static TheoryData<string, Type> ExternalAccountCredentialTestData


Just make this expression-bodied?

Done, on all three

jskeet · 2022-09-12T08:32:45Z

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

+            Assert.Contains($"subject_token={SubjectTokenText}", contentText);
+            Assert.Contains($"scope={scope}", contentText);
+
+            return new HttpResponseMessage


I'd probably do the serialization in a separate statement above, then just return new HttpResponseMessage { Content = new StringContent(content, Encoding.UTF8) }; - aside from anything else, that's simpler when debugging.

Given all the code below, it might also be worth extracting new HttpResponseMessage { Content = new StringContent(content, Encoding.UTF8) } into a helper method.

In fact, we might want a helper method to do everything - just given the access token and expiry, as that's all that really seems to vary.

jskeet · 2022-09-12T08:33:43Z

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

+                    {
+                        accessToken = ImpersonatedAccessToken,
+                        expireTime = "2020-05-13T16:00:00.045123456Z"
+                    }))


This one doesn't specify UTF-8 - is that relevant?

After extracting it does, not sure if it was default already.

jskeet · 2022-09-12T08:40:10Z

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

+        /// <inheritdoc/>
+        protected override Task<string> GetSubjectTokenAsync(CancellationToken taskCancellationToken)
+        {
+            var fileContent = File.ReadAllText(SubjectTokenFilePath);


Do we definitely want to read the file here, rather than in the constructor? What's the expected behavior if this file changes over time?

The file content can change at any time, so I think it's correct to read the file here.

Yes, it is expected that the subject token may be refreshed and the refreshed subject token should be used each time.

I had this question in general for subject tokens, regardless of whether they are URL or file or otherwise sourced, they can always change, on every access token refresh the subject tokens should be requested anew.

jskeet · 2022-09-12T08:40:38Z

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

+            }
+            else
+            {
+                var jsonResponse = NewtonsoftJsonSerializer.Instance.Deserialize<Dictionary<string, string>>(fileContent);


Are we okay with this failing with a Json.NET-specific error if the field doesn't exist, or isn't a string?

I'll wrap the exceptions from here on the parent class, as we could also get IOExceptions if the file is not present, etc. I've added tests as well for both credentials.

jpassing · 2022-09-13T00:08:02Z

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

+        /// <inheritdoc/>
+        protected override Task<string> GetSubjectTokenAsync(CancellationToken taskCancellationToken)
+        {
+            var fileContent = File.ReadAllText(SubjectTokenFilePath);


This is an async method, so you could read the file asynchronously too

There's no File.ReadAllTextAsync pre .NET 7 (which surprised me a little) and I prefered code simplicity. But yes, changing now...

jpassing · 2022-09-13T00:11:31Z

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

+        /// <inheritdoc/>
+        protected override Task<string> GetSubjectTokenAsync(CancellationToken taskCancellationToken)
+        {
+            var fileContent = File.ReadAllText(SubjectTokenFilePath);


The file content can change at any time, so I think it's correct to read the file here.

amanda-tarafa

I've addressed all review comments, PTAL.

amanda-tarafa · 2022-09-13T14:06:54Z

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


-        [Fact]
-        public async Task GetDefaultCredential_UrlSourcedExternalAccountCredential()
+        public static TheoryData<string, Type> ExternalAccountCredentialTestData


Done, on all three

amanda-tarafa · 2022-09-13T14:10:25Z

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

+        /// <inheritdoc/>
+        protected override Task<string> GetSubjectTokenAsync(CancellationToken taskCancellationToken)
+        {
+            var fileContent = File.ReadAllText(SubjectTokenFilePath);


Yes, it is expected that the subject token may be refreshed and the refreshed subject token should be used each time.

I had this question in general for subject tokens, regardless of whether they are URL or file or otherwise sourced, they can always change, on every access token refresh the subject tokens should be requested anew.

amanda-tarafa · 2022-09-13T14:21:00Z

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

+        /// <inheritdoc/>
+        protected override Task<string> GetSubjectTokenAsync(CancellationToken taskCancellationToken)
+        {
+            var fileContent = File.ReadAllText(SubjectTokenFilePath);


There's no File.ReadAllTextAsync pre .NET 7 (which surprised me a little) and I prefered code simplicity. But yes, changing now...

amanda-tarafa · 2022-09-13T14:47:07Z

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

+            }
+            else
+            {
+                var jsonResponse = NewtonsoftJsonSerializer.Instance.Deserialize<Dictionary<string, string>>(fileContent);


I'll wrap the exceptions from here on the parent class, as we could also get IOExceptions if the file is not present, etc. I've added tests as well for both credentials.

amanda-tarafa · 2022-09-13T16:27:39Z

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

+            Assert.Contains($"subject_token={SubjectTokenText}", contentText);
+            Assert.Contains($"scope={scope}", contentText);
+
+            return new HttpResponseMessage


amanda-tarafa · 2022-09-13T16:29:00Z

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

+                    {
+                        accessToken = ImpersonatedAccessToken,
+                        expireTime = "2020-05-13T16:00:00.045123456Z"
+                    }))


After extracting it does, not sure if it was default already.

jskeet · 2022-09-14T08:35:34Z

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

+            {
+                return await GetSubjectTokenAsyncImpl(taskCancellationTokne).ConfigureAwait(false);
+            }
+            catch(Exception ex)


Micro-nit: space before (

Done. I'll rebase all and merge after green! Thanks.

Towards googleapis#2033.

Features - googleapis#2228 Supports Quota Project environment variable for ADC. - googleapis#2219 Removes obsolete OOB code. BREAKING CHANGE: We have removed OOB support from the library because the feature is not supported by Google OAuth as of October 3rd 2022. See the [official announcement](https://developers.googleblog.com/2022/02/making-oauth-flows-safer.html#disallowed-oob) for more information. - googleapis#2216 Log errors when trying to reach Compute's metadata server. - googleapis#2177, googleapis#2197, googleapis#2200, googleapis#2206, googleapis#2215, googleapis#2235 Support for Workload Identity Federation and Worforce Identity Federation. - googleapis#2103 Compute credentials support explicit scopes. - googleapis#2096 Improves deserialization error handling.

Features - #2228 Supports Quota Project environment variable for ADC. - #2219 Removes obsolete OOB code. BREAKING CHANGE: We have removed OOB support from the library because the feature is not supported by Google OAuth as of October 3rd 2022. See the [official announcement](https://developers.googleblog.com/2022/02/making-oauth-flows-safer.html#disallowed-oob) for more information. - #2216 Log errors when trying to reach Compute's metadata server. - #2177, #2197, #2200, #2206, #2215, #2235 Support for Workload Identity Federation and Worforce Identity Federation. - #2103 Compute credentials support explicit scopes. - #2096 Improves deserialization error handling.

And support version: 1.58.0-beta02 => 1.58.0 Bug fixes: - googleapis#2273 Don't attempt to use an empty/null media type in batch responses - googleapis#2251 Which fixes a typo on then name of one of the AWS Credential environment variables. - googleapis#2264 Which adds a missing line break in AWS Credential canonical request. Features - googleapis#2269 Support per-request max retry configuration. - googleapis#2228 Supports Quota Project environment variable for ADC. - googleapis#2219 Removes obsolete OOB code. BREAKING CHANGE: We have removed OOB support from the library because the feature is not supported by Google OAuth as of October 3rd 2022. See the [official announcement](https://developers.googleblog.com/2022/02/making-oauth-flows-safer.html#disallowed-oob) for more information. - googleapis#2216 Log errors when trying to reach Compute's metadata server. - googleapis#2177, googleapis#2197, googleapis#2200, googleapis#2206, googleapis#2215, googleapis#2235 Support for Workload Identity Federation and Worforce Identity Federation. - googleapis#2103 Compute credentials support explicit scopes. - googleapis#2096 Improves deserialization error handling.

And support version: 1.58.0-beta02 => 1.58.0 Dependencies: - Client libraries no longer target netstandard1.0 or net40. Bug fixes: - googleapis#2273 Don't attempt to use an empty/null media type in batch responses - googleapis#2251 Which fixes a typo on then name of one of the AWS Credential environment variables. - googleapis#2264 Which adds a missing line break in AWS Credential canonical request. Features - googleapis#2269 Support per-request max retry configuration. - googleapis#2228 Supports Quota Project environment variable for ADC. - googleapis#2219 Removes obsolete OOB code. BREAKING CHANGE: We have removed OOB support from the library because the feature is not supported by Google OAuth as of October 3rd 2022. See the [official announcement](https://developers.googleblog.com/2022/02/making-oauth-flows-safer.html#disallowed-oob) for more information. - googleapis#2216 Log errors when trying to reach Compute's metadata server. - googleapis#2177, googleapis#2197, googleapis#2200, googleapis#2206, googleapis#2215, googleapis#2235 Support for Workload Identity Federation and Worforce Identity Federation. - googleapis#2103 Compute credentials support explicit scopes. - googleapis#2096 Improves deserialization error handling.

And support version: 1.58.0-beta02 => 1.58.0 Dependencies: - Client libraries no longer target netstandard1.0 or net40. Bug fixes: - #2273 Don't attempt to use an empty/null media type in batch responses - #2251 Which fixes a typo on then name of one of the AWS Credential environment variables. - #2264 Which adds a missing line break in AWS Credential canonical request. Features - #2269 Support per-request max retry configuration. - #2228 Supports Quota Project environment variable for ADC. - #2219 Removes obsolete OOB code. BREAKING CHANGE: We have removed OOB support from the library because the feature is not supported by Google OAuth as of October 3rd 2022. See the [official announcement](https://developers.googleblog.com/2022/02/making-oauth-flows-safer.html#disallowed-oob) for more information. - #2216 Log errors when trying to reach Compute's metadata server. - #2177, #2197, #2200, #2206, #2215, #2235 Support for Workload Identity Federation and Worforce Identity Federation. - #2103 Compute credentials support explicit scopes. - #2096 Improves deserialization error handling.

amanda-tarafa requested a review from jskeet September 9, 2022 22:47

amanda-tarafa requested a review from a team as a code owner September 9, 2022 22:47

jskeet approved these changes Sep 12, 2022

View reviewed changes

jpassing approved these changes Sep 13, 2022

View reviewed changes

amanda-tarafa force-pushed the wif-file-based branch from 65d548a to 829927a Compare September 13, 2022 16:27

amanda-tarafa commented Sep 13, 2022

View reviewed changes

lsirac approved these changes Sep 13, 2022

View reviewed changes

jskeet approved these changes Sep 14, 2022

View reviewed changes

feat: Add support for file-sourced external credentials.

a16f320

Towards googleapis#2033.

amanda-tarafa force-pushed the wif-file-based branch from 829927a to a16f320 Compare September 14, 2022 08:57

amanda-tarafa merged commit d584dc7 into googleapis:main Sep 14, 2022

amanda-tarafa deleted the wif-file-based branch September 14, 2022 09:01

amanda-tarafa mentioned this pull request Oct 18, 2022

Update support version: 1.57.0 -> 1.58.0-beta01 #2237

Merged

amanda-tarafa mentioned this pull request Nov 30, 2022

Update version: 1.57.0 => 1.58.0 #2274

Merged

feat: Add support for file-sourced external credentials. #2206

feat: Add support for file-sourced external credentials. #2206

Uh oh!

Conversation

amanda-tarafa commented Sep 9, 2022

Uh oh!

jskeet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amanda-tarafa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants