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

PutObjectCommandInput.SSECustomerKey type is incorrect #4736

Closed
3 tasks done
DavidZbarsky-at opened this issue May 19, 2023 · 10 comments
Closed
3 tasks done

PutObjectCommandInput.SSECustomerKey type is incorrect #4736

DavidZbarsky-at opened this issue May 19, 2023 · 10 comments
Assignees
Labels
closed-for-staleness feature-request New feature or enhancement. May require GitHub community feedback. needs-review This issue/pr needs review from an internal developer. p3 This is a minor priority issue response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days.

Comments

@DavidZbarsky-at
Copy link

Checkboxes for prior research

Describe the bug

This is a dupe of #2081 that I am reopening because the explanation didn't make sense to me. I am still seeing this issue, but I am using PutObjectCommandInput as advised, rather than PutObjectRequest. See also generated types at https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/clients/client-s3/interfaces/putobjectcommandinput.html#ssecustomerkey-7

Is there something in the codegen process that is supposed to expand the type to account for conversions?

SDK version number

@aws-sdk/client-v3@latest

Which JavaScript Runtime is this issue in?

Node.js

Details of the browser/Node.js/ReactNative version

16

Reproduction Steps

const data: PutObjectCommandInput = {
    SSECustomerKey: Buffer.from('foo')
}

Observed Behavior

Type 'Buffer' is not assignable to type 'string'.ts(2322)
models_0.d.ts(8835, 5): The expected type comes from property 'SSECustomerKey' which is declared here on type 'PutObjectCommandInput'```

### Expected Behavior

no type error

### Possible Solution

_No response_

### Additional Information/Context

_No response_
@DavidZbarsky-at DavidZbarsky-at added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels May 19, 2023
@RanVaknin RanVaknin self-assigned this May 19, 2023
@yenfryherrerafeliz
Copy link
Contributor

Hi @DavidZbarsky-at, sorry to hear about your issues. Right now, SSECustomerKey parameter is expected to be string, so that it needs to be provided as so. You can see it's definition here. However, I can mark this as a feature request, but, I would need you to provide more details about why this change is needed?, and what exactly is your use case?.

For now, for you to prevent this error from happening you can convert the buffer returned to string as follow:

const data: PutObjectCommandInput = {
    SSECustomerKey: Buffer.from('foo').toString()
}

Thanks!

@yenfryherrerafeliz yenfryherrerafeliz added response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. feature-request New feature or enhancement. May require GitHub community feedback. p3 This is a minor priority issue and removed needs-triage This issue or PR still needs to be triaged. bug This issue is a bug. labels May 19, 2023
@DavidZbarsky-at
Copy link
Author

@yenfryherrerafeliz Hi, thanks for responding.

Here is how the parameter is used:

const properties = [
{
target: "SSECustomerKey",
hash: "SSECustomerKeyMD5",
},
{
target: "CopySourceSSECustomerKey",
hash: "CopySourceSSECustomerKeyMD5",
},
];
for (const prop of properties) {
const value: SourceData | undefined = (input as any)[prop.target];
if (value) {
const valueView: Uint8Array = ArrayBuffer.isView(value)
? new Uint8Array(value.buffer, value.byteOffset, value.byteLength)
: typeof value === "string"
? options.utf8Decoder(value)
: new Uint8Array(value);

In particular, note

const valueView: Uint8Array = ArrayBuffer.isView(value)
? new Uint8Array(value.buffer, value.byteOffset, value.byteLength)
which allows using a buffer view. In fact, if you pass in a strong, it will be converted to a Uint8Array anyway.

So I'm requesting that the type be adjusted to allow passing in a Uint8Array containing the SSECustomerKey.

My application is storing keys in memory as Uint8Array already, so adding extra conversions to string (which the SDK will then convert back to Uint8Array) is just visual noise and wasted CPU cycles.

Please let me know if you have additional questions and thanks for your consideration!

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. label May 22, 2023
@yenfryherrerafeliz
Copy link
Contributor

@DavidZbarsky-at, thanks for providing this information. I will mark this feature request to be reviewed so we can evaluate this further.

Thanks!

@yenfryherrerafeliz yenfryherrerafeliz added the needs-review This issue/pr needs review from an internal developer. label May 22, 2023
@DavidZbarsky-at
Copy link
Author

Thanks! Quick note - I'd consider this to be a bug in the existing type definitions rather than a new feature. Looking forward to seeing this fixed!

@jstastny
Copy link

jstastny commented Oct 3, 2023

I ran into the same issue.

I have tried the workaround suggested in #4736 (comment), but without any luck -- when I use it, I get InvalidArgument: The secret key was invalid for the specified algorithm. error, when generating the random key with crypto.randomBytes(32).

Here is the minimal code to reproduce.

import * as s3 from '@aws-sdk/client-s3'
import { randomBytes } from 'crypto'

const keyBytes = randomBytes(32)
const key = Buffer.from(keyBytes).toString()

const client = new s3.S3Client({})

await client.send(
  new s3.PutObjectCommand({
    Bucket: 'my-testing-bucket',
    Key: 'encryption-test',
    Body: 'test',
    SSECustomerAlgorithm: 'AES256',
    SSECustomerKey: key,
  })
)

If I pass the buffer directly (without converting it to string), everything works fine, but I need to cast to any to avoid type error.

const key = Buffer.from(keyBytes) as any

@yenfryherrerafeliz
Copy link
Contributor

@DavidZbarsky-at, @jstastny, this is actually not considered as a bug. The type expected for SSECustomerKey is a string and needs to be provided as so. This type is actually defined by the service itself and the SDK just follows its directives regarding typing. We could leave this as a feature-request if you people want but it would be considered as low priority for now.

Thanks!

@yenfryherrerafeliz yenfryherrerafeliz added the response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. label Oct 17, 2023
@DavidZbarsky-at
Copy link
Author

DavidZbarsky-at commented Oct 17, 2023

@yenfryherrerafeliz Hi, did you have a chance to review the code links I posted in #4736 (comment)?

I don't think this statement is correct "The type expected for SSECustomerKey is a string".

The code handles ArrayBuffer.isView(value). Here's an updated link to the relevant stanza on mainhttps://github.com/aws/aws-sdk-js-v3/blob/51f316ea2d674b1ca78fd7236d6a606d53a88b21/packages/middleware-ssec/src/index.ts#L39-L43

If the type is provided by the service, that's fine, it just needs to be fixed upstream. Is that code public or is that something AWS-internal?

@yenfryherrerafeliz
Copy link
Contributor

Hi @DavidZbarsky-at, yes, I reviewed the code you have linked, but we expect the parameters to be provided as the service has defined them, regardless how it is handled internally. Of course we can consider this as a feature request, but to be considered a bug we need to differ from the type set by the service model. I reviewed the Java SDK and PHP SDK and they also both expect the same type. Here you can confirm from the service model that this field is marked as string. I know a wrapper here would be ideal, but this is something we could consider for future implementations.

Please let me know if you have any questions.

Thanks!

If the type is provided by the service, that's fine, it just needs to be fixed upstream. Is that code public or is that something AWS-internal?

For JS SDK here is where the service models are:
https://github.com/aws/aws-sdk-js-v3/tree/main/codegen/sdk-codegen/aws-models

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. label Oct 18, 2023
@yenfryherrerafeliz yenfryherrerafeliz added the response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. label Oct 18, 2023
@github-actions
Copy link

This issue has not received a response in 1 week. If you still think there is a problem, please leave a comment to avoid the issue from automatically closing.

@github-actions github-actions bot added closing-soon This issue will automatically close in 4 days unless further comments are made. closed-for-staleness and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Oct 25, 2023
Copy link

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 Nov 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
closed-for-staleness feature-request New feature or enhancement. May require GitHub community feedback. needs-review This issue/pr needs review from an internal developer. p3 This is a minor priority issue response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days.
Projects
None yet
Development

No branches or pull requests

4 participants