aws / aws-sdk-go-v2

AWS SDK for the Go programming language.
https://aws.github.io/aws-sdk-go-v2/docs/
Apache License 2.0
2.58k stars 622 forks source link

MIGRATION ISSUE: s3: document that SSECustomerKey now needs to be base64 encoded #2736

Closed ncw closed 1 day ago

ncw commented 1 month ago

Pre-Migration Checklist

Go Version Used

go 1.22

Describe the Migration Issue

In the s3 package github.com/aws/aws-sdk-go-v2/service/s3 the input to the SSECustomerKey field in s3.HeadObjectInput, s3.CopyObjectInput, s3.GetObjectInput and s3.PutObjectInput (there may be others but I tested those ones) needs to base64 encoded, whereas in the v1 SDK it did not.

This is not mentioned in the docs, eg

https://github.com/aws/aws-sdk-go-v2/blob/5f159bb2f5f5d8a9c9160869ccdda35be050930f/service/s3/api_op_GetObject.go#L340-L361

And it is not mentioned in the Migration Guide

So I can only assume it is either a bug or an undocumented change.

Code Comparison

No response

Observed Differences/Errors

SDKv1 sends this (note the X-Amz-Server-Side-Encryption-Customer-Key here is a test key so not sensitive)

2024/08/07 09:56:52 DEBUG : HEAD /README.md HTTP/1.1
Host: rclone-sse-c.s3.eu-west-2.amazonaws.com
User-Agent: rclone/v1.67.0
Authorization: XXXX
X-Amz-Content-Sha256: e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855
X-Amz-Date: 20240807T085652Z
X-Amz-Server-Side-Encryption-Customer-Algorithm: AES256
X-Amz-Server-Side-Encryption-Customer-Key: Y3puOHFyYlVzVC81eTVIcjJpOTNJbVdtSVFMQ1pMT0w=
X-Amz-Server-Side-Encryption-Customer-Key-Md5: ME4ss65LcXQBY2CynVdZyA==

whereas SDKv2 sends this

2024/08/07 09:55:35 DEBUG : HEAD /README.md HTTP/1.1
Host: rclone-sse-c.s3.eu-west-2.amazonaws.com
User-Agent: rclone/v1.68.0-beta.8139.5727beb2b.fix-4989-s3-aws-sdk-v2
Accept-Encoding: identity
Amz-Sdk-Invocation-Id: fe5db3e3-e062-4fa9-9b6e-ab485fb7f99e
Amz-Sdk-Request: attempt=1; max=10
Authorization: XXXX
X-Amz-Content-Sha256: e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855
X-Amz-Date: 20240807T085535Z
X-Amz-Server-Side-Encryption-Customer-Algorithm: AES256
X-Amz-Server-Side-Encryption-Customer-Key: czn8qrbUsT/5y5Hr2i93ImWmIQLCZLOL
X-Amz-Server-Side-Encryption-Customer-Key-Md5: ME4ss65LcXQBY2CynVdZyA==

Which produces this error

operation error S3: HeadObject, https response error StatusCode: 400, RequestID: HBZC24PYAY1HMHBZ, HostID: tDJl5G2Gad6ga16/FB7AG90D5xbSM5LnzFh6/ppUpacGVFLFX6Svs6IEgFDUq8YfsktQ3XhwJMhPAklOoRjQMQ==, api error BadRequest: Bad Request

You can see quite clearly that the X-Amz-Server-Side-Encryption-Customer-Key in the SDKv1 is a base64 encoded version of that sent by the SDKv2

Additional Context

No response

RanVaknin commented 1 month ago

Hi @ncw ,

Thank you for reaching out. I can confirm that the behavior between v1 and v2 changed. Specifically, in v1 we had a customization that handled all the encoding for you. In the newer SDKs we are moving away from customizations and are trying to mirror more closely the specification given to us by the API model itself.

Unfortunately, as you have pointed out, the doc string (which is generated from S3's documentation model), does not specify that the SSE-C fields must be encoded. This is indeed confusing. S3 did supplement their standard API docs with a bit of documentation specifically about SSE-C where they are mentioning that these fields should be base64 encoded.

In essence, if something is not described in the S3 API model as a "should be base 64 encoded", the SDK does not "know" it needs to do the encoding.

I will review this with the team, and see what is the best course of action for this.

Thanks again, Ran~

ncw commented 4 weeks ago

Thanks @RanVaknin

I think a lot of users keep their SSE keys as base64 anyway as they can have non printable characters in - this is what we found in https://github.com/rclone/rclone/issues/6400 anyway.

That makes me think that a note on the docs is all that is required here. I understand that may be harder than it sounds given the auto generated nature of the docs!

RanVaknin commented 4 weeks ago

Hi @ncw ,

I 100% agree with you. This needs to be better documented. Unfortunately like you mentioned, the doc string itself that the SDK presents is 1 to 1 mirroring the API docs from S3 which do not mention the need for encoding.

We have decided to add a section in our migration guide specifically about this change in behavior.

Thanks again for reaching out. Ran~

ncw commented 4 weeks ago

Perfect - thanks @RanVaknin

github-actions[bot] commented 1 day ago

This issue is now closed. Comments on closed issues are hard for our team to see. If you need more assistance, please open a new issue that references this one.