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

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

Closed
2 tasks done
ncw opened this issue Aug 7, 2024 · 5 comments · Fixed by #2781
Closed
2 tasks done

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

ncw opened this issue Aug 7, 2024 · 5 comments · Fixed by #2781
Assignees
Labels
documentation This is a problem with documentation. p3 This is a minor priority issue queued This issues is on the AWS team's backlog v1-v2-inconsistency v1-v2-inconsistency Behavior has changed from v1 to v2, or feature is missing altogether

Comments

@ncw
Copy link

ncw commented Aug 7, 2024

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

// Specifies the customer-provided encryption key that you originally provided for
// Amazon S3 to encrypt the data before storing it. This value is used to decrypt
// the object when recovering it and must match the one used when storing the data.
// The key must be appropriate for use with the algorithm specified in the
// x-amz-server-side-encryption-customer-algorithm header.
//
// If you encrypt an object by using server-side encryption with customer-provided
// encryption keys (SSE-C) when you store the object in Amazon S3, then when you
// GET the object, you must use the following headers:
//
// - x-amz-server-side-encryption-customer-algorithm
//
// - x-amz-server-side-encryption-customer-key
//
// - x-amz-server-side-encryption-customer-key-MD5
//
// For more information about SSE-C, see [Server-Side Encryption (Using Customer-Provided Encryption Keys)] in the Amazon S3 User Guide.
//
// This functionality is not supported for directory buckets.
//
// [Server-Side Encryption (Using Customer-Provided Encryption Keys)]: https://docs.aws.amazon.com/AmazonS3/latest/dev/ServerSideEncryptionCustomerKeys.html
SSECustomerKey *string

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

@ncw ncw added needs-triage This issue or PR still needs to be triaged. v1-v2-inconsistency v1-v2-inconsistency Behavior has changed from v1 to v2, or feature is missing altogether labels Aug 7, 2024
ncw added a commit to rclone/rclone that referenced this issue Aug 7, 2024
The new SDK apparently keeds the customer key to be base64 encoded
where the old one did that for you automatically.

See: aws/aws-sdk-go-v2#2736
See: https://forum.rclone.org/t/new-s3-backend-help-testing-needed/47139/3
@RanVaknin RanVaknin self-assigned this Aug 13, 2024
@RanVaknin
Copy link
Contributor

RanVaknin commented Aug 13, 2024

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~

@RanVaknin RanVaknin added feature-request A feature should be added or improved. needs-review This issue or pull request needs review from a core team member. p3 This is a minor priority issue and removed needs-triage This issue or PR still needs to be triaged. labels Aug 13, 2024
@ncw
Copy link
Author

ncw commented Aug 14, 2024

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 rclone/rclone#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 RanVaknin added queued This issues is on the AWS team's backlog and removed needs-review This issue or pull request needs review from a core team member. labels Aug 14, 2024
@RanVaknin
Copy link
Contributor

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
Copy link
Author

ncw commented Aug 15, 2024

Perfect - thanks @RanVaknin

@lucix-aws lucix-aws added documentation This is a problem with documentation. and removed feature-request A feature should be added or improved. labels Aug 26, 2024
@lucix-aws lucix-aws changed the title MIGRATION ISSUE: s3: SSECustomerKey needs to be base64 encoded in the new SDK MIGRATION ISSUE: s3: document that SSECustomerKey now needs to be base64 encoded Aug 26, 2024
@lucix-aws lucix-aws linked a pull request Sep 11, 2024 that will close this issue
Copy link

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.

lostb1t pushed a commit to lostb1t/rclone that referenced this issue Sep 15, 2024
The new SDK apparently keeds the customer key to be base64 encoded
where the old one did that for you automatically.

See: aws/aws-sdk-go-v2#2736
See: https://forum.rclone.org/t/new-s3-backend-help-testing-needed/47139/3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation This is a problem with documentation. p3 This is a minor priority issue queued This issues is on the AWS team's backlog v1-v2-inconsistency v1-v2-inconsistency Behavior has changed from v1 to v2, or feature is missing altogether
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants