-
Notifications
You must be signed in to change notification settings - Fork 365
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Data-driven V4 URL signing tests, and implementation changes #2882
Conversation
@@ -109,7 +109,8 @@ private struct SigningState | |||
string expiryText = expirySeconds.ToString(CultureInfo.InvariantCulture); | |||
|
|||
string clientEmail = blobSigner.Id; | |||
string credentialScope = $"{datestamp}/auto/gcs/goog4_request"; | |||
// TODO: This storage used to be gcs. Check it! | |||
string credentialScope = $"{datestamp}/auto/storage/goog4_request"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Following gsutil implementation of "storage".
"datestamp": "20190201", | ||
"region": "auto", | ||
"credentialScope": "20190201/auto/storage/goog4_request", | ||
"expectedUrl": "https://storage.googleapis.com/test-bucket/test-object?x-goog-algorithm=GOOG4-RSA-SHA256&x-goog-credential=test-iam-credentials%40dummy-project-id.iam.gserviceaccount.com%2F20190201%2Fauto%2Fstorage%2Fgoog4_request&x-goog-date=20190201T090000Z&x-goog-expires=10&x-goog-signedheaders=host&x-goog-signature=95e6a13d43a1d1962e667f17397f2b80ac9bdd1669210d5e08e0135df9dff4e56113485dbe429ca2266487b9d1796ebdee2d7cf682a6ef3bb9fbb4c351686fba90d7b621cf1c4eb1fdf126460dd25fa0837dfdde0a9fd98662ce60844c458448fb2b352c203d9969cb74efa4bdb742287744a4f2308afa4af0e0773f55e32e92973619249214b97283b2daa14195244444e33f938138d1e5f561088ce8011f4986dda33a556412594db7c12fc40e1ff3f1bedeb7a42f5bcda0b9567f17f65855f65071fabb88ea12371877f3f77f10e1466fff6ff6973b74a933322ff0949ce357e20abe96c3dd5cfab42c9c83e740a4d32b9e11e146f0eb3404d2e975896f74" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Query parameter name casing discussion should be reopened. I comment on the bug and in the document related to this PR internally.
91c1c3e
to
1163f27
Compare
Initially the test description is in our repo, but is likely to be somewhere common in the future.
It turns out we hardly need any of the V2 code in V4, so most of that has been moved into V2Signer.cs. All we need is the trimming part.
A fix for this is working its way through to production
Ready for review now :) This doesn't quite complete the V4 work - and I want to add more data-driven tests afterwards - but it's getting there. I'm rerunning all integration tests and snippets locally. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as far as I can see.
(The test fails at the moment, but I'm creating the PR for sharing purposes.)