Remove default port from host when calculating signature #1618

s12v · 2017-10-27T19:47:03Z

Stumbled upon this issue when working on elasticsearch signatures (elastic/beats#5466). Default elasticsearch port is 9200, I had to include "non-standart" 443 to hostname, and then it calculates wrong signature.

jasdel · 2017-11-01T20:06:59Z

Thanks for creating this PR @s12v. I'm looking into the v4 signing spec to verify that this will not go against any of the v4 specification.

jasdel · 2017-11-01T20:22:45Z

From the V4 signing specification and test suite i'm not seeing any mention of excluding port from the Host header when computing the signature's canonical string. And per the HTTP RFC the port should be included in the header if its non standard.

I'm curious if the failure you're seeing because the a "standard" port of 443 (TLS) is being included in the hostname.

jasdel · 2017-11-01T20:42:33Z

Related to aws/aws-cli#2883

s12v · 2017-11-01T21:49:02Z

@jasdel, makes sense, thanks for looking into it.
Host header includes port, so SDK has to modify header? Or maybe just calculate correct signature. I'm wondering how does it work in the java sdk...

s12v · 2017-11-01T22:22:10Z

@jasdel, found it: https://github.com/aws/aws-sdk-java/blob/1.11.221/aws-java-sdk-core/src/main/java/com/amazonaws/auth/AWS4Signer.java#L533-L545

s12v · 2017-11-01T23:39:39Z

I changed the code quite a bit and moved tests to functional_test.go, please have a look

jasdel

Thanks for updating this PR! Stripping off the port conditionally looks good. Just a couple comments about checking if the request value is not nil. And if you ran into situations where this was possible.

jasdel · 2017-11-02T17:43:39Z

aws/signer/v4/v4.go

@@ -696,6 +698,81 @@ func (ctx *signingCtx) removePresign() {
 	ctx.Query.Del("X-Amz-SignedHeaders")
 }

+// Set ctx.Request.Host to host:port for non-standard ports, and to host for explicit standart ports
+func (ctx *signingCtx) sanitizeHost() {
+	if ctx.Request == nil || ctx.Request.URL == nil {


did you run into situations where the ctx.Request value was nil? I think this value is required for the signer to function. In that case i think it would be better to fail loudly instead of silently bypass the nil value.

@jasdel, I assume it's always set, but I don't know :) Removing the check.

What do you think about ctx.Request.URL? Should be also always set?

jasdel · 2017-11-02T17:44:05Z

aws/signer/v4/v4.go

+
+// Returns host without port number
+func hostname(req *http.Request) string {
+	if req == nil {


Same question as above in sanitizeHost with nil request value.

jasdel · 2017-11-02T17:45:09Z

aws/signer/v4/v4.go

+	return aws.URLHostname(req.URL)
+}
+
+// Copy of url.stripPort()


could you update this to mention directly that this was copied for the Go standard library url package. Would also be also to include the go version too.

jasdel · 2017-11-02T17:45:16Z

aws/signer/v4/v4.go

+	return hostport[:colon]
+}
+
+// Copy of url.portOnly()


ditto as above

s12v · 2017-11-02T20:01:55Z

@jasdel, changed.

I'm thinking about one more thing: current implementation modifies ctx.request.Host when needed. It's possible to achieve the same result in func (ctx *signingCtx) buildCanonicalHeaders(..), without touching the original request. Which is probably better... what do you think?

Please check the next commit to see what I mean.

jasdel · 2017-11-02T22:55:23Z

I was thinking about this as well. I think the original request must be modified because if the HTTP request sent doesn't use the exact Host header value the signature will fail. I don't think the Go HTTP client automatically strip off the port in the Host header. At least httputil.DumpRequest shows it with the port on the host header. I've not looked at this in wireshark yet though.

https://play.golang.org/p/Z7opUaTA-F

If the Go HTTP client doesn't automatically strip off the "standard" port form the Host header I think this needs to be performed by the SDK.

Maybe a better place for this logic would be when the request is created instead of the signer. The signer wouldn't need any special logic if the Host value on the HTTPRequest is already formatted "correctly".

s12v · 2017-11-02T23:27:23Z

I was thinking about this as well. I think the original request must be modified because if the HTTP request sent doesn't use the exact Host header value the signature will fail

I think it will not fail (at least it was working for me with AWS ES). Probably, on the receiving side default port is removed before signing. However, it's not explicitly documented anywhere, and maybe it even works differently for different services... no idea.

But I agree, that on the other hand, it's better to have signature and host header consistent.
If you'd like, I can revert the last commit.

Maybe a better place for this logic would be when the request is created instead of the signer.

Not sure, the signer accepts http.Request.

jasdel · 2017-11-06T18:44:57Z

Thanks for the update @s12v I think with ES you're right the port is probably being removed automatically server side before signature verification. We probably cannot rely on this for all services though. I've heard mentions about standard port in host header causing failures for some services.

Thanks a lot for taking the time and putting together this PR by the way!

Maybe a better place for this logic would be when the request is created instead of the signer.

Not sure, the signer accepts http.Request.

Good point. This logic would be required in the signer anyways to handle the case a user uses the signer directly with a http.Request. So we know the logic needs to per performed in the signer for the Sign and Presign maybe case. From a library structural point of view it would also be good if the SDK's internals generated the request.Request's Host header correctly.

jasdel · 2017-11-06T18:55:25Z

To Resolve this how about we move the stripDefaultPort et al functions/logic to the request package, and export stripDefaultPort named as SanitizeHostForHeader. The SDK's request.New could use this when building a new SDK request. Setting the Request's HTTPRequest.Host member.

For the use case where a user manually calls v4.Signer.Sign|Presign, the methods should use request.SanitizeHostForHeader to update the http.Request's Host member.

I think this would allow the logic to be performed in both request building, and ensure the Host header is constructed correctly for anonymous requests, (aka unsigned).

s12v · 2017-11-06T22:53:33Z

@jasdel, sounds good, moved to request

jasdel · 2017-11-14T20:46:17Z

Thanks for putting this PR together @s12v. the change looks good. I'll go ahead and merge this in. It will be tagged in our next release.

s12v · 2018-02-20T21:14:43Z

@jasdel, it seems the issue with default ports exists in aws-sdk-go-v2, do you have any plans to move the code there?

jasdel · 2018-02-20T23:18:56Z

Thanks for letting us know about this issue in the v2 SDK @s12v. Would you mind creating a issue in the v2 for this. We'll get that issue ported over.

Remove port from host when calculating signature

3546215

Sanitize host, move tests

e8ed4e4

s12v added 6 commits November 2, 2017 00:40

rename tests

3fb7ab9

Don't extract host unless needed

78d18fd

better comment

d7f78ff

1.7 compatibility

b67f4f0

port is usually empty

7de5504

code style

a8b4020

jasdel suggested changes Nov 2, 2017

View reviewed changes

code review changes

832938e

Move logic to buildCanonicalHeaders, don't modify the original request

de117e8

s12v changed the title ~~Remove port from host when calculating signature~~ Remove default port from host when calculating signature Nov 2, 2017

s12v and others added 4 commits November 6, 2017 23:43

Move logic to request package, add tests

30c89b7

revert

a903462

Merge branch 'master' into 1537_remove_port_from_hostport

1b6f008

Merge branch 'master' into 1537_remove_port_from_hostport

a2e4df2

jasdel approved these changes Nov 14, 2017

View reviewed changes

jasdel merged commit 3f5879a into aws:master Nov 14, 2017

jasdel added a commit that referenced this pull request Nov 14, 2017

Update change log for #1618

08818f4

awstools mentioned this pull request Nov 14, 2017

v1.12.27 #1644

Merged

s12v mentioned this pull request Feb 26, 2018

Signer: remove default ports (port from V1) aws/aws-sdk-go-v2#132

Closed

jasdel mentioned this pull request Jan 19, 2021

Initial Pass at an IO layer for the Rust SDK smithy-lang/smithy-rs#107

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove default port from host when calculating signature #1618

Remove default port from host when calculating signature #1618

s12v commented Oct 27, 2017

jasdel commented Nov 1, 2017

jasdel commented Nov 1, 2017

jasdel commented Nov 1, 2017

s12v commented Nov 1, 2017 •

edited

Loading

s12v commented Nov 1, 2017

s12v commented Nov 1, 2017

jasdel left a comment

jasdel Nov 2, 2017

s12v Nov 2, 2017 •

edited

Loading

jasdel Nov 2, 2017

jasdel Nov 2, 2017

jasdel Nov 2, 2017

s12v commented Nov 2, 2017 •

edited

Loading

jasdel commented Nov 2, 2017 •

edited

Loading

s12v commented Nov 2, 2017 •

edited

Loading

jasdel commented Nov 6, 2017

jasdel commented Nov 6, 2017

s12v commented Nov 6, 2017

jasdel commented Nov 14, 2017

s12v commented Feb 20, 2018 •

edited

Loading

jasdel commented Feb 20, 2018

Remove default port from host when calculating signature #1618

Remove default port from host when calculating signature #1618

Conversation

s12v commented Oct 27, 2017

jasdel commented Nov 1, 2017

jasdel commented Nov 1, 2017

jasdel commented Nov 1, 2017

s12v commented Nov 1, 2017 • edited Loading

s12v commented Nov 1, 2017

s12v commented Nov 1, 2017

jasdel left a comment

Choose a reason for hiding this comment

jasdel Nov 2, 2017

Choose a reason for hiding this comment

s12v Nov 2, 2017 • edited Loading

Choose a reason for hiding this comment

jasdel Nov 2, 2017

Choose a reason for hiding this comment

jasdel Nov 2, 2017

Choose a reason for hiding this comment

jasdel Nov 2, 2017

Choose a reason for hiding this comment

s12v commented Nov 2, 2017 • edited Loading

jasdel commented Nov 2, 2017 • edited Loading

s12v commented Nov 2, 2017 • edited Loading

jasdel commented Nov 6, 2017

jasdel commented Nov 6, 2017

s12v commented Nov 6, 2017

jasdel commented Nov 14, 2017

s12v commented Feb 20, 2018 • edited Loading

jasdel commented Feb 20, 2018

s12v commented Nov 1, 2017 •

edited

Loading

s12v Nov 2, 2017 •

edited

Loading

s12v commented Nov 2, 2017 •

edited

Loading

jasdel commented Nov 2, 2017 •

edited

Loading

s12v commented Nov 2, 2017 •

edited

Loading

s12v commented Feb 20, 2018 •

edited

Loading