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

V4 Signer incorrect behavior when ContentLength already set #1728

Closed
matelang opened this issue Jun 14, 2022 · 1 comment · Fixed by #1743
Closed

V4 Signer incorrect behavior when ContentLength already set #1728

matelang opened this issue Jun 14, 2022 · 1 comment · Fixed by #1743
Labels
bug This issue is a bug.

Comments

@matelang
Copy link

matelang commented Jun 14, 2022

Describe the bug

Hi,

I am trying to implement an AWS SDK v2 HTTP request Signer for the OpenSearch Go client library (traditionally support was only for AWS SDK V1 signer).

I've discovered that when having a request with a Content-Length already set, the AWS SDK V2 SignerV4 (package: github.com/aws/aws-sdk-go-v2/aws/signer/v4) will incorrectly re-append the content length to the canonical request as comma separated values.

Having the following test:

	b1, err := json.Marshal(map[string]interface{}{
		"description": "this is a test",
		"timestamp":   "2020-01-16T04:32:49",
	})
	if err != nil {
		log.Error().Err(err).Msg("json ctor error")
	}
	r1, err := http.NewRequest(http.MethodPut, "https://localhost:9200/testing/_doc/1?pretty",
		strings.NewReader(string(b1)))
	r1.Header.Set("Content-Type", "application/json")
	r1.Header.Set("Content-Length", fmt.Sprintf("%d", len(b1)))
	if err != nil {
		log.Error().Err(err).Msg("http ctor error")
	}

And calling the V1 signer and V2 signer on such a http.Request, will result in different signatures being generated, due to the canonical request being altered erroneously by the V2 implementation.

Expected canonical string:

2022/06/14 17:01:23 DEBUG Request Signature:
---[ CANONICAL STRING  ]-----------------------------
PUT
/testing/_doc/1
pretty=
content-length:66
content-type:application/json
host:localhost:9200
x-amz-date:20220614T140123Z

content-length;content-type;host;x-amz-date
a333350fb0ac11b4a9efea6cdf4dc98ddc1900c24a59530ec829e1189ab8172a

Actual canonical string (note content-length header):

---[ CANONICAL STRING  ]-----------------------------
PUT
/testing/_doc/1
pretty=
content-length:66,66
content-type:application/json
host:localhost:9200
x-amz-date:20220614T140309Z

content-length;content-type;host;x-amz-date
a333350fb0ac11b4a9efea6cdf4dc98ddc1900c24a59530ec829e1189ab8172a

Expected Behavior

Don't reappend the ContentLength as comma separated values.

Current Behavior

ContentLength header value gets appended as comma separated value.

Reproduction Steps

Please see issue description.

Possible Solution

#1729

Additional Information/Context

No response

AWS Go SDK V2 Module Versions Used

github.com/aws/aws-sdk-go-v2 v1.16.5
github.com/aws/aws-sdk-go-v2/config v1.15.10

Compiler and Version used

go version go1.18.3 darwin/arm64

Operating System and version

MacOs, M1 Pro

@matelang matelang added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 14, 2022
jasdel added a commit that referenced this issue Jun 29, 2022
…1743)


Fixes the SDK's AWS SigV4 signer to not double sign the content-length header. If the Content-Length header was manually set on the http.Request, that value would be included along with the Request.ContentLength value as a common separated list when computing the string to sign.

This fix updates the signer to always ignore the content-length header, and only use the Request.ContentLength parameter. This change also matches http.Request's behavior of ignoring the Content-Length header if set.

Fixes #1728
Replaces: #1729
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@vudh1 vudh1 removed the needs-triage This issue or PR still needs to be triaged. label Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug.
Projects
None yet
2 participants