fix: Missing line break in AWS credential canonical request #2264

amanda-tarafa · 2022-11-18T00:18:20Z

No description provided.

jskeet

Wow - that change to the canonical request is astonishing, mostly in terms of it not being 100% reproducible. That's really weird.

jskeet · 2022-11-18T07:42:22Z

Src/Support/Google.Apis.Auth/OAuth2/AwsExternalAccountCredential.AwsSignedSubjectToken.cs


            [JsonProperty("url")]
-            public string Url { get; }
+            public string Url { get; set; }


This feels like more than a refactor - this feels like it could be at the heart of the problem. (i.e. maybe we weren't deserializing properly). If this actually is the relevant change, maybe separate into a different commit?

No, this is really just about testability. Production code doesn't deserialize at all, it only serializes.
Test code is now deserializing and that's why I had to make all these changes.

jskeet · 2022-11-18T07:44:30Z

Src/Support/IntegrationTests/ExternalAccountCredentialTests.cs

+                HttpResponseMessage response = await httpClient.GetAsync(assumeRoleUrl);
+                content = await response.Content.ReadAsStringAsync();
+            }
+            XmlDocument awsCredentialXml = new XmlDocument();


I'd suggest using LINQ to XML instead of XmlDocument in general - it's a much nicer API.

Yep done. It was very simple what we needed to do, but in case that changes in the future, it's better with Linq.

jskeet · 2022-11-18T07:45:53Z

Src/Support/IntegrationTests/IntegrationTests.Utils/Google.Apis.IAMCredentials.v1.cs

+        public override string Name => "iamcredentials";
+
+        /// <summary>Gets the service base URI.</summary>
+        public override string BaseUri =>


Once we've updated to the next generator version, we'll no longer generate the conditional part, FWIW. Might be worth adapting to that now, given that we're not targeting those platforms for tests anyway.

(Or maybe just leave a note somewhere to remind ourselves to update it later.)

I've filed #2265

amanda-tarafa

Comments addresed, see the last commit for the changes.

Wow - that change to the canonical request is astonishing, mostly in terms of it not being 100% reproducible. That's really weird.

Agree, the more I think about it, the more I'm convinced I made a mistake on my manual tests, but I haven't been able to find that either (I've disassembled the dll on the local nuget package that I used for the manual tests). But the fact that I can never reproduce on my manual tests and I can always reproduce on the AWS mimmicking test, makes me think that the problem is somehwere in my manual tests.

amanda-tarafa · 2022-11-18T10:02:17Z

Src/Support/Google.Apis.Auth/OAuth2/AwsExternalAccountCredential.AwsSignedSubjectToken.cs


            [JsonProperty("url")]
-            public string Url { get; }
+            public string Url { get; set; }


No, this is really just about testability. Production code doesn't deserialize at all, it only serializes.
Test code is now deserializing and that's why I had to make all these changes.

amanda-tarafa · 2022-11-18T11:05:18Z

Src/Support/IntegrationTests/ExternalAccountCredentialTests.cs

+                HttpResponseMessage response = await httpClient.GetAsync(assumeRoleUrl);
+                content = await response.Content.ReadAsStringAsync();
+            }
+            XmlDocument awsCredentialXml = new XmlDocument();


Yep done. It was very simple what we needed to do, but in case that changes in the future, it's better with Linq.

amanda-tarafa · 2022-11-18T11:23:24Z

Src/Support/IntegrationTests/IntegrationTests.Utils/Google.Apis.IAMCredentials.v1.cs

+        public override string Name => "iamcredentials";
+
+        /// <summary>Gets the service base URI.</summary>
+        public override string BaseUri =>


I've filed #2265

jskeet · 2022-11-18T11:46:41Z

Src/Support/IntegrationTests/ExternalAccountCredentialTests.cs


-            string AwsCredentialValue(string tagName) => awsCredentialXml.GetElementsByTagName(tagName)[0].InnerText;
+            string AwsCredentialValue(string tagName) =>
+                awsCredentialXml.Descendants().Where(element => element.Name.LocalName == tagName).Single().Value;


Nit: you can change .Where(xxx).Single() to .Single(xxx) to simplify it a bit.

Ah true, will change and rebase all in a couple of commits.

Fixes googleapis#2250. - I haven't been able to reproduce this bug 100%, but it happens enough without this change, and hasn't been observed with this change. - The Java implementation [includes this extra line break](https://github.com/googleapis/google-auth-library-java/blob/5e5fe411af88f00fd7fe09b1f4ca4d5d9417ecab/oauth2_http/java/com/google/auth/oauth2/AwsRequestSigner.java#L177). - From looking at the [signature spec](https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html) (see point 4), is still not entirely clear that this extra line break at the end of the canonical headers is actually specified in the requirements.

amanda-tarafa · 2022-11-18T14:00:19Z

Src/Support/Google.Apis.Auth/OAuth2/AwsExternalAccountCredential.AwsSignedSubjectToken.cs

                        canonicalRequest.Append(headerPair.Value);
                        canonicalRequest.Append(newLine);
                    }
+                    canonicalRequest.Append(newLine);


OK, I got what was wrong with my tests:

I was previously adding the line breaks before appending the next element instead of after appending the previous one, so...

The code in line 152 looked like canonicalRequest.Append(newLine).Append(signedHeaders); which added the missing line break.

But the code in line 134 looked like canonicalRequest.Append(newLine).Append(verificationRequestUrl.GetQueryStringForAwsCanonicalRequest()).Append(newLine); which is pretty ugly (appending line breaks before and after on account of the for loop adding the headers right after).

So I changed the code to add the line breaks after each element, not realizing that I was removing this line break with that. And I managed to cherry pick that change out of the last (and possibly more) local nuget package that I created, which I did rougly two hours before merging the original PR. I'm sorry about this! But I'm glad I understand what happened.

The unit test that I have added will catch any such changes. And I've also run my manual tests with the code on this PR, and it's good.

Bug fixes: - 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.

Bug fixes: - #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.

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.

refactor: Visibility and setability changes for testing purposes only.

b4ec065

amanda-tarafa requested a review from jskeet November 18, 2022 00:18

amanda-tarafa requested a review from a team as a code owner November 18, 2022 00:18

jskeet approved these changes Nov 18, 2022

View reviewed changes

amanda-tarafa commented Nov 18, 2022

View reviewed changes

jskeet approved these changes Nov 18, 2022

View reviewed changes

amanda-tarafa force-pushed the aws-issue branch from 6dfd3a7 to 6fefebe Compare November 18, 2022 11:51

amanda-tarafa commented Nov 18, 2022

View reviewed changes

amanda-tarafa merged commit d46ec9b into googleapis:main Nov 18, 2022

amanda-tarafa deleted the aws-issue branch November 18, 2022 14:44

amanda-tarafa mentioned this pull request Nov 22, 2022

Update support version: 1.58.0-beta01 -> 1.58.0-beta02 #2270

Merged

amanda-tarafa mentioned this pull request Nov 30, 2022

Update version: 1.57.0 => 1.58.0 #2274

Merged

fix: Missing line break in AWS credential canonical request #2264

fix: Missing line break in AWS credential canonical request #2264

Uh oh!

Conversation

amanda-tarafa commented Nov 18, 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants