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

Presigned URL for PUT with ContentType doesn't include ContentType in the signed headers #1475

Closed
vangent opened this issue Oct 28, 2021 · 10 comments
Assignees
Labels
bug This issue is a bug.

Comments

@vangent
Copy link

vangent commented Oct 28, 2021

Confirm by changing [ ] to [x] below to ensure that it's a bug:

Describe the bug
Signing a PUT URL with a ContentType results in a URL without Content-Type included in the signed headers, so the ContentType is not enforced.

Version of AWS SDK for Go?
github.com/aws/aws-sdk-go-v2/service/s3 v1.17.0

Version of Go (go version)?
latest

To Reproduce (observed behavior)

  const contentType = "text/plain"
  in := &s3v2.PutObjectInput{
          Bucket: aws.String(bucket),
          Key:    aws.String(key),
          ContentType: aws.String(contentType),
  }
  p, _ := s3v2.NewPresignClient(client).PresignPutObject(ctx, in)
  fmt.Println(p.URL)

Expected behavior

V1 produced a signed URL that included content-type in the X-Amz-SignedHeaders.
V2 does not.

As a result, using the signed header from V2 doesn't enforce the content type.

Example URL from V1 (some of the URL elided):

https://go-cloud-testing.s3.us-west-1.amazonaws.com/blob-for-signing?...X-Amz-SignedHeaders=content-type%3Bhost&X-Amz-Signature=cf1572571f8ebd5f5f47b0487d2fea44e6ffc33766fc3719bf5ee61dad9c0e3b     

Example URL from V2:

https://go-cloud-testing.s3.us-west-1.amazonaws.com/blob-for-signing?...X-Amz-SignedHeaders=host&x-id=PutObject&X-Amz-Signature=10df84e7fc6715cd212c53d7cb85248a41e3714e0602fc8075183587c90857e6       

Additional context
V1 code is here:
https://github.com/google/go-cloud/blob/master/blob/s3blob/s3blob.go#L750

@vangent vangent added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Oct 28, 2021
@KaibaLopez KaibaLopez added needs-review This issue or pull request needs review from a core team member. and removed needs-triage This issue or PR still needs to be triaged. labels Nov 5, 2021
@KaibaLopez
Copy link
Contributor

hi @vangent ,
Thanks for pointing this out to us, yea this is definitely a bug, pretty sure it's happening somewhere here...
We'll get this fixed as fast as we can.

@KaibaLopez KaibaLopez removed the needs-review This issue or pull request needs review from a core team member. label Nov 5, 2021
@github-actions
Copy link

github-actions bot commented Nov 6, 2022

We have noticed this issue has not received attention in 1 year. We will close this issue for now. If you think this is in error, please feel free to comment and reopen the issue.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Nov 6, 2022
@vangent
Copy link
Author

vangent commented Nov 7, 2022

Please fix this.

@RanVaknin
Copy link
Contributor

Hi @vangent ,

First of all I'd like to apologize for the long wait. The person who assigned it to themselves left the company and this issue fell between the cracks. This is not the kind of experience we want users to have with any AWS product, so for that I am sincerely sorry.

To answer your question Content-Type will not be set unless Content-Length is also set, or is not 0. (Its 0 by default)
There is a specific utility function that omits that header if Content length is not present in the params.

// RemoveContentTypeHeader removes content-type header if
// content length is unset or equal to zero.
func RemoveContentTypeHeader(stack *middleware.Stack) error {
	return stack.Build.Add(&removeContentTypeHeader{}, middleware.After)
}

Here is how I created the presigned URL:

func main() {

	cfg, err := config.LoadDefaultConfig(context.TODO(), config.WithRegion("us-east-1"), config.WithClientLogMode(aws.LogResponseWithBody|aws.LogRequestWithBody))
	if err != nil {
		panic(err)
	}

	presigner := s3.NewPresignClient(client)

	presignPutObject, err := presigner.PresignPutObject(context.TODO(), &s3.PutObjectInput{
		Bucket:        aws.String(myBucket),
		Key:           aws.String(myKey),
		ContentType:   aws.String(MyContentType),
		ContentLength: 1,
	})
	if err != nil {
		panic(err)
	}
	fmt.Println(presignPutObject.URL)
	
	// output: 
	// https://REDACTED.s3.us-east-1.amazonaws.com/REDACTED?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=REDACTED&X-Amz-Date=20221107T212750Z&X-Amz-Expires=900&X-Amz-SignedHeaders=content-length%3Bcontent-type%3Bhost&x-id=PutObject&X-Amz-Signature=REDACTED

Let me know if this helps.
All the best,
Ran~

@RanVaknin RanVaknin added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Nov 7, 2022
@RanVaknin RanVaknin self-assigned this Nov 7, 2022
@vangent
Copy link
Author

vangent commented Nov 8, 2022

Thanks for getting back to this!

So, I think that explains why the implementation does what it does, but it doesn't explain why. Maybe there's a good reason, but can you explain why the ContentType should not be enforced unless there is a non-zero ContentLength?

@vangent
Copy link
Author

vangent commented Nov 8, 2022

BTW, aws/aws-sdk-java-v2#2520 is a similar bug with a regression from V1.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Nov 9, 2022
@RanVaknin
Copy link
Contributor

@vangent ,

Unfortunately I'm not entirely sure. My best guess would be that this is related to some newer S3 related requirements specifying this behavior.

I take it that my previous answer solved the issue at hand so I feel confident we can close this thread. If you have any other issues please feel free to open another issue and I'll do my best to address it as soon as possible.

Thanks again!
Ran~

@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.

@henryson
Copy link

henryson commented Sep 27, 2023

I have a similar problem. I get a CORS error when adding Content-Type to the signed URL PUT request. Followed your advice to add ContentLength to the PutObjectCommand when generating the signed URL and I can see that header in the browser, but the Content-Type is still missing in X-Amz-SignedHeaders.
I run nodejs on the server with @aws-sdk/client-s3 (v3)

`
import { getSignedUrl } from '@aws-sdk/s3-request-presigner'
import config from "../config/config"

const client = new S3({
region: "eu-north-1",
endpoint: config.myUrl,
credentials: {
accessKeyId: config.myAccess,
secretAccessKey: config.mySecret,
}
})

const signedUrl = async (args: any) => {
const fileId = cuid()
const filetype = args.filetype
const filename = args.filename
.replace(/([Ì]|[^0-9a-öA-Ö.\s])/g, '')
.normalize('NFKD')
.replace(/([\u0300-\u036f]|[^0-9a-zA-Z.\s])/g, '')
const command = new PutObjectCommand({ Key: fileId + "/" + filename, Bucket: config.myBucket, ContentType: filetype, ContentLength: 1 })
const url = await getSignedUrl(client, command, { expiresIn: 15 * 60 })
return { id: fileId, name: filename, url, type: filetype }
}
`

Any idea?

Cheers, Josef

Hi @vangent ,

First of all I'd like to apologize for the long wait. The person who assigned it to themselves left the company and this issue fell between the cracks. This is not the kind of experience we want users to have with any AWS product, so for that I am sincerely sorry.

To answer your question Content-Type will not be set unless Content-Length is also set, or is not 0. (Its 0 by default) There is a specific utility function that omits that header if Content length is not present in the params.

@TheRedSpy15
Copy link

Pinging @RanVaknin based on the github action notification and that it doesn't appear, anyone will see @henryson comment otherwise

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
Development

No branches or pull requests

5 participants