alamb commented on code in PR #790:
URL:
https://github.com/apache/arrow-rs-object-store/pull/790#discussion_r3547969576
##########
src/aws/credential.rs:
##########
@@ -262,7 +258,14 @@ impl<'a> AwsAuthorizer<'a> {
.insert(&REQUEST_PAYER_HEADER,
REQUEST_PAYER_HEADER_VALUE.clone());
}
- let (signed_headers, canonical_headers) =
canonicalize_headers(request.headers());
+ // SigV4 must sign the `host` header, but the actual Host header is
managed by the HTTP
+ // transport (hyper derives it from the request URL). We therefore add
`host` to a
+ // throwaway copy of the headers used only to build the signature,
rather than pinning it
+ // on the request.
Review Comment:
I personally found this comment hard to understand. Mostly because it
asserted that the actual Host header is managed by the transport when I think
it is more accurate to say reqwest will add the `Host` header if not already
present. It also doesn't mention the redirect problem this PR is trying to
solve
So maybe something more like
```suggestion
// the SigV4 must include a value for `host`, but the actual Host
header may need to change
// due to a redirect. Therefore do not override the Host header for
the http
// request, let the client set it.
```
##########
src/aws/credential.rs:
##########
@@ -262,7 +258,14 @@ impl<'a> AwsAuthorizer<'a> {
.insert(&REQUEST_PAYER_HEADER,
REQUEST_PAYER_HEADER_VALUE.clone());
}
- let (signed_headers, canonical_headers) =
canonicalize_headers(request.headers());
+ // SigV4 must sign the `host` header, but the actual Host header is
managed by the HTTP
+ // transport (hyper derives it from the request URL). We therefore add
`host` to a
+ // throwaway copy of the headers used only to build the signature,
rather than pinning it
+ // on the request.
+ let host = &url[url::Position::BeforeHost..url::Position::AfterPort];
+ let mut headers_to_sign = request.headers().clone();
Review Comment:
I personally think this clone/sign pattern is confusing -- it took me a
while to grok what this is doing and realize the net effect of this is to
remove the HOST from the headers in reqwest
I think it would be both clearer and more performant (no `clone`) to do
something more like
```rust
request.headers_mut().insert(HOST, host);
let sig = compute_signature(request.headers())?;
request.headers_mut().remove(HOST);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
