Skip to content
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

Remove default port from host when calculating signature #1618

Merged
merged 14 commits into from
Nov 14, 2017
69 changes: 69 additions & 0 deletions aws/signer/v4/functional_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -164,3 +164,72 @@ func TestStandaloneSign_CustomURIEscape(t *testing.T) {
t.Errorf("expect %v, got %v", e, a)
}
}

func TestStandaloneSign_StandardHttpsPort(t *testing.T) {
expectedHost := "estest.us-east-1.es.amazonaws.com"
expectedSig := "AWS4-HMAC-SHA256 Credential=AKID/19700101/us-east-1/es/aws4_request, SignedHeaders=host;x-amz-date;x-amz-security-token, Signature=e573fc9aa3a156b720976419319be98fb2824a3abc2ddd895ecb1d1611c6a82d"
urlWithStandardPort := "https://estest.us-east-1.es.amazonaws.com:443/_search"

signer := v4.NewSigner(unit.Session.Config.Credentials)
req, _ := http.NewRequest("GET", urlWithStandardPort, nil)
_, err := signer.Sign(req, nil, "es", "us-east-1", time.Unix(0, 0))
if err != nil {
t.Fatalf("expect no error, got %v", err)
}

actualHost := req.Host
if e, a := expectedHost, actualHost; e != a {
t.Errorf("expect %v, got %v", e, a)
}

actual := req.Header.Get("Authorization")
if e, a := expectedSig, actual; e != a {
t.Errorf("expect %v, got %v", e, a)
}
}

func TestStandaloneSign_NonStandardHttpPort(t *testing.T) {
expectedHost := "example.com:9200"
expectedSig := "AWS4-HMAC-SHA256 Credential=AKID/19700101/us-east-1/es/aws4_request, SignedHeaders=host;x-amz-date;x-amz-security-token, Signature=cd9d926a460f8d3b58b57beadbd87666dc667e014c0afaa4cea37b2867f51b4f"
urlWithNonStandardHttpPort := "http://example.com:9200/_search"

signer := v4.NewSigner(unit.Session.Config.Credentials)
req, _ := http.NewRequest("GET", urlWithNonStandardHttpPort, nil)
_, err := signer.Sign(req, nil, "es", "us-east-1", time.Unix(0, 0))
if err != nil {
t.Fatalf("expect no error, got %v", err)
}

actualHost := req.Host
if e, a := expectedHost, actualHost; e != a {
t.Errorf("expect %v, got %v", e, a)
}

actual := req.Header.Get("Authorization")
if e, a := expectedSig, actual; e != a {
t.Errorf("expect %v, got %v", e, a)
}
}

func TestStandaloneSign_NonStandardHttpsPort(t *testing.T) {
expectedHost := "example.com:9200"
expectedSig := "AWS4-HMAC-SHA256 Credential=AKID/19700101/us-east-1/es/aws4_request, SignedHeaders=host;x-amz-date;x-amz-security-token, Signature=cd9d926a460f8d3b58b57beadbd87666dc667e014c0afaa4cea37b2867f51b4f"
urlWithNonStandardHttpsPort := "https://example.com:9200/_search"

signer := v4.NewSigner(unit.Session.Config.Credentials)
req, _ := http.NewRequest("GET", urlWithNonStandardHttpsPort, nil)
_, err := signer.Sign(req, nil, "es", "us-east-1", time.Unix(0, 0))
if err != nil {
t.Fatalf("expect no error, got %v", err)
}

actualHost := req.Host
if e, a := expectedHost, actualHost; e != a {
t.Errorf("expect %v, got %v", e, a)
}

actual := req.Header.Get("Authorization")
if e, a := expectedSig, actual; e != a {
t.Errorf("expect %v, got %v", e, a)
}
}
77 changes: 77 additions & 0 deletions aws/signer/v4/v4.go
Original file line number Diff line number Diff line change
Expand Up @@ -504,6 +504,8 @@ func (ctx *signingCtx) build(disableHeaderHoisting bool) {

ctx.buildBodyDigest()

ctx.sanitizeHost()

unsignedHeaders := ctx.Request.Header
if ctx.isPresign {
if !disableHeaderHoisting {
Expand Down Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@s12v s12v Nov 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

return
}

var port string
if ctx.Request.Host != "" {
port = portOnly(ctx.Request.Host)
} else {
port = portOnly(ctx.Request.URL.Host)
}

if isUsingNonDefaultPort(ctx.Request.URL.Scheme, port) {
ctx.Request.Host = hostname(ctx.Request) + ":" + port
} else if port != "" { // remove standard port, if set explicitly
ctx.Request.Host = hostname(ctx.Request)
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as above in sanitizeHost with nil request value.

return ""
}

if req.Host != "" {
return stripPort(req.Host)
}

return aws.URLHostname(req.URL)
}

// Copy of url.stripPort()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

func stripPort(hostport string) string {
colon := strings.IndexByte(hostport, ':')
if colon == -1 {
return hostport
}
if i := strings.IndexByte(hostport, ']'); i != -1 {
return strings.TrimPrefix(hostport[:i], "[")
}
return hostport[:colon]
}

// Copy of url.portOnly()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto as above

func portOnly(hostport string) string {
colon := strings.IndexByte(hostport, ':')
if colon == -1 {
return ""
}
if i := strings.Index(hostport, "]:"); i != -1 {
return hostport[i+len("]:"):]
}
if strings.Contains(hostport, "]") {
return ""
}
return hostport[colon+len(":"):]
}

// Returns true if the specified URI is using a non-standard port
// (i.e. any port other than 80 for HTTP URIs or any port other than 443 for HTTPS URIs)
func isUsingNonDefaultPort(scheme, port string) bool {
if port == "" {
return false
}

lowerCaseScheme := strings.ToLower(scheme)
if (lowerCaseScheme == "http" && port == "80") || (lowerCaseScheme == "https" && port == "443") {
return false
}

return true;
}

func makeHmac(key []byte, data []byte) []byte {
hash := hmac.New(sha256.New, key)
hash.Write(data)
Expand Down