-
Notifications
You must be signed in to change notification settings - Fork 23
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
feat(storage): add hostname cases to SignedURL tests #89
Conversation
BrennaEpp
commented
Jan 30, 2024
•
edited
Loading
edited
- If a library does not have a certain option (eg. emulator host), in the tests it may set something that takes the next precedence or vice-versa, or skip tests that set it. From least to most precedence: emulator host -> endpoint on client -> hostname in options
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. You should work with other language owners to get this propagated into their repos with the necessary changes to the test code.
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
storage/v1/v4_signatures.json
Outdated
"expiration": 10, | ||
"timestamp": "2019-02-01T09:00:00Z", | ||
"expectedUrl": "https://storage.domain.com:443/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=8cd0d479a88fb7d791a2dcc8fc5b5f020ca817eeef5b5a5cb3260eb63cf47ecd271faa238d0fa31efca35bc2a9244bd122178c520749f922c0235726a5a6be099bf4f33a0d54187eee2e0208964c2a13104b03e235cdeb4f07b3eb566b8a33259cf7540a3fe823be601ace2a54a79acd6834cb646380c4cfc7ef0fd95d3ebbc1f97d840f6fe1dceed4269ecb4e91ff7e6633f38adab82049a965968367b9e7c362cec868d804bd42abbb6d2e837ce5d45ee9e1d92c7acc09623acaae3df6128ca15f9f80bb6543944e8c997f691c35113b9e9f44e86fd343524343b08dd8f887685588acc103e0b432f24912e7e1c63e086aeed1890e41b75beb64164fe6bfcf", | ||
"universeDomain": "domain.com:443", |
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.
Should the port be included as part of the universe domain?
That's not what libraries in general are expecting.
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.
Ah, good point! I became over-eager as ports are accepted in endpoints and we had a bug in the Go library related to that. Removed the port from the universe domain.
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.
If I run into something while implementing these, I'll let you know.
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.
@BrennaEpp I've left a couple of questions here, thanks!
"expectedUrl": "http://localhost:8080/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=66c8c0371ca7d933a0d50f110abdf4fc7e3e379329134f272ebe4aa8100ccd5f21cd56ca5ccffae5ed36c8d6840e7cac80c2e7d786cd85b10d0faea34cddf09d2e7eb7f5c7c53934e4bf8f5cd654bc3c1b5ee9e3f8ca2189cd225b445bb866563fc4bd0d0b4d116111655611d12ec18f2d854fd7142d9afcc977dbd8f6d0524e4170506abf2b119bbe00d17697321d225162fabddb4ddae77781b4f3277a6b6fccfeb47d70b88537e5efb416001274aaeb1535b5aae757c997edc66d03898a5d08f767313d018d10992981d00e2a18ed9a6839b8a1ac7b3be1cab2e9511ba91e14a786443b59e9f21e1ae74a2c60106180646a764531fbe1fcd9c1e40550e56e", | ||
"scheme": "http", | ||
"hostname": "localhost:8080", | ||
"expectedCanonicalRequest": "GET\n/test-bucket/test-object\nX-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\nhost:storage.googleapis.com\n\nhost\nUNSIGNED-PAYLOAD", |
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.
Shouldn't the bit host:storage.googleapis.com
in expectedCanonicalRequest
be host:localhost:8080
?
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.
You are correct (here and elsewhere); I just found out that we don't test this field in Go (we'd have to go into the internals of a function to test). I will update and send a PR for review
"expectedUrl": "http://localhost:8080/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=66c8c0371ca7d933a0d50f110abdf4fc7e3e379329134f272ebe4aa8100ccd5f21cd56ca5ccffae5ed36c8d6840e7cac80c2e7d786cd85b10d0faea34cddf09d2e7eb7f5c7c53934e4bf8f5cd654bc3c1b5ee9e3f8ca2189cd225b445bb866563fc4bd0d0b4d116111655611d12ec18f2d854fd7142d9afcc977dbd8f6d0524e4170506abf2b119bbe00d17697321d225162fabddb4ddae77781b4f3277a6b6fccfeb47d70b88537e5efb416001274aaeb1535b5aae757c997edc66d03898a5d08f767313d018d10992981d00e2a18ed9a6839b8a1ac7b3be1cab2e9511ba91e14a786443b59e9f21e1ae74a2c60106180646a764531fbe1fcd9c1e40550e56e", | ||
"scheme": "http", | ||
"clientEndpoint": "http://localhost:8080", | ||
"expectedCanonicalRequest": "GET\n/test-bucket/test-object\nX-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\nhost:storage.googleapis.com\n\nhost\nUNSIGNED-PAYLOAD", |
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.
And here as well?
"timestamp": "2019-02-01T09:00:00Z", | ||
"expectedUrl": "https://xyz.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=53b20003ff2c552b3194a6bccc25024663662392554b3334e989e2704f3a0308455eaacf45c335a78c0186a5cf8eef78bf5781a7267465d28a35c9e1291f87ff340e9ee40b3b9bdce70561bf000887ce38ccd7d2445a8749453960a8f11d37576dfd5942f92d6f4527bbeffb90526b5de9653b6ca16136e9f19bcb65d984ddaf22c4ade45d6a168bb4752a43de33ab121206f50d994612824407711bff720cb1b207b61b613c44c85d3ce16dc4fc6eba24e494e176b0780d0ab85a800b13fcbf31434ddf51992efae1efde330ebda0617d1c20078ef22a4f10a7bcbed961237442d9a8db78d7aeb777a4994b50efdd41e07c4966e912a30f92a7426f207e9545", | ||
"emulatorHostname": "https://xyz.googleapis.com", | ||
"expectedCanonicalRequest": "GET\n/test-bucket/test-object\nX-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\nhost:storage.googleapis.com\n\nhost\nUNSIGNED-PAYLOAD", |
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.
And here it should be xyz.googleapis.com
?