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

S3 SSECustomerKey no longer working since 3.498.0 #5863

Closed
3 tasks done
john-yick opened this issue Mar 7, 2024 · 7 comments
Closed
3 tasks done

S3 SSECustomerKey no longer working since 3.498.0 #5863

john-yick opened this issue Mar 7, 2024 · 7 comments
Assignees
Labels
bug This issue is a bug. p2 This is a standard priority issue

Comments

@john-yick
Copy link

john-yick commented Mar 7, 2024

Checkboxes for prior research

Describe the bug

Versions prior to 3.498.0 seem to work as expected for SSECustomerKey, but something within 3.498.0 has broken it for us.

I have crafted a small script below that works fine for 3.496.0 which we have running in production. But updating to any AWS SDK version higher than 3.496.0 causes an 403 Access Denied error to be thrown.

SDK version number

@aws-sdk/[email protected]

Which JavaScript Runtime is this issue in?

Node.js

Details of the browser/Node.js/ReactNative version

v20.7.0

Reproduction Steps

Run the following code for a file that exists with SSECustomerKey encryption

Variables you will need to provide

  • customerManagedKey
  • key
  • bucket
const crypto = require("crypto");
const { S3Client, GetObjectCommand } = require("@aws-sdk/client-s3");
const { fromEnv } = require("@aws-sdk/credential-providers");

const SSECustomerAlgorithm = "AES256";
const SSECustomerKey = crypto
  .createHash("md5")
  .update(customerManagedKey)
  .digest("hex");
const SSECustomerKeyMD5 = crypto
  .createHash("md5")
  .update(SSECustomerKey)
  .digest("hex");

const s3Client = new S3Client({
  region: process.env.AWS_REGION,
  credentials: fromEnv(),
});

// Works under 3.496.0
// Broken under 3.498.0
async function main() {
  try {
    const s3GetObjectResponse = await s3Client.send(
      new GetObjectCommand({
        Bucket: bucket,
        Key: key,
        SSECustomerAlgorithm: SSECustomerAlgorithm,
        SSECustomerKey: SSECustomerKey,
        SSECustomerKeyMD5: SSECustomerKeyMD5,
      })
    );
    const data = new Promise((resolve, reject) => {
      s3GetObjectResponse.Body.pipe(res);
      s3GetObjectResponse.Body.on("error", (err) => {
        reject(err);
      });
      s3GetObjectResponse.Body.on("close", () => {
        resolve();
      });
    });
    console.log(data);
  } catch (err) {
    console.log(err);
  }
}

main();

Observed Behavior

An error is thrown on any version of the JS SDK 3.498.0 or higher

Expected Behavior

The GetObjectCommand should download the object to local as expected

Possible Solution

No response

Additional Information/Context

No response

@john-yick john-yick added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 7, 2024
@RanVaknin RanVaknin self-assigned this Mar 7, 2024
@RanVaknin RanVaknin added p1 This is a high priority issue and removed needs-triage This issue or PR still needs to be triaged. labels Mar 7, 2024
@RanVaknin
Copy link
Contributor

Hi @john-yick ,

Thanks for letting us know.
I'll look into with priority.

Thanks,
Ran~

@RanVaknin RanVaknin added the investigating Issue is being investigated and/or work is in progress to resolve the issue. label Mar 7, 2024
@RanVaknin
Copy link
Contributor

Hi @john-yick ,

Is there a reason you were providing the key as a hexadecimal string?

const SSECustomerKey = crypto
  .createHash("md5")
  .update(customerManagedKey)
  .digest("hex");

This is breaking because of a change I made to the SSEC middleware here. The reason it's breaking is because S3 expects the SSECustomerKey to be provided as a binary that has been base64 encoded.

The reason I made the change was that before the change, you needed to provide the key as binary, and the SDK would base64 encode it for you, which is not following the documentation.
After the change, you can either specify the key as a raw binary (to support the old behavior) or you could base64 encode it yourself (which follows the documentation).

To explain what is happening in the old implementation with your code:
When you provide a hex string as the SSECustomerKey, the function treats the hex string as if it were UTF-8 encoded text, which leads to an inaccurate binary representation for the purposes of encryption key encoding. This misinterpreted binary is then turned into a base64 string for the SSECustomerKey. Additionally, an MD5 hash of this incorrect binary is made and also turned into base64 for the SSECustomerKeyMD5.

I'm not sure why this has passed the service side validations on the S3 side, but if you have some more input as to why you were providing the key as a hex string in the first place maybe it can shed some light on the service side behavior.

At any rate, I'm working on a fix to support the old (seemingly incorrect) behavior.
Thanks,
Ran~

@RanVaknin RanVaknin added the response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. label Mar 7, 2024
@john-yick
Copy link
Author

john-yick commented Mar 8, 2024

@RanVaknin I am unsure why it converts into a Hex value as the implementation our side is quite old. But the code has been working for quite a long time, enough for us to have over 200k objects encrypted with this SSECustomerKey.

We always assumed that it was correct as the response from S3 was valid and we could track the objects from S3's web console also.

For the SSECustomerKeyMD5 we calculate it within our code based off the Hex string that is the SSECustomerKey and pass it to the S3Client. Is the client ignoring this now and trying to re-calculate it?

Thanks you for your support.
John

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. label Mar 9, 2024
@RanVaknin RanVaknin added pending-release This issue will be fixed by an approved PR that hasn't been released yet. and removed investigating Issue is being investigated and/or work is in progress to resolve the issue. labels Mar 14, 2024
@RanVaknin
Copy link
Contributor

Hi @john-yick ,

Can you please wait for tomorrow's release and update your SDK version and try again?

Thanks~
Ran

@RanVaknin RanVaknin added response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. p2 This is a standard priority issue and removed pending-release This issue will be fixed by an approved PR that hasn't been released yet. p1 This is a high priority issue labels Mar 18, 2024
@john-yick
Copy link
Author

@RanVaknin Sure thing, will give it a try tomorrow and let you know how it goes.

Thanks,
John

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. label Mar 20, 2024
@john-yick
Copy link
Author

@RanVaknin I can confirm that the issue has been resolved 👍

Thanks for your assistance,
John

Copy link

github-actions bot commented Apr 4, 2024

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug This issue is a bug. p2 This is a standard priority issue
Projects
None yet
Development

No branches or pull requests

2 participants