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

can't create presigned url with canned acl, can't limit content type #1692

Closed
congbaoyangrou opened this issue Apr 30, 2022 · 8 comments
Closed
Assignees
Labels
closed-for-staleness guidance Question that needs advice or information.

Comments

@congbaoyangrou
Copy link

congbaoyangrou commented Apr 30, 2022

Describe the bug

  1. don't know how to set canned ACL in presigned url.
  2. even if I set content type in PutObjectInput, the SignedHeaders doesn't contain it.
  3. misleading error and different results in different clients.

Expected Behavior

when using the presigned url, a file should be uploaded successfully.

Current Behavior

1.when I set acl:types.ObjectCannedACLPublicRead in PutObjectInput,the client should set x-amz-acl in header,which is not expected. it should be one of presigned url's query params.(I found this issue aws/aws-sdk-java-v2#1849 exactly the same as my confusion, but I don't know how to override it in go)
2. set content type="image/png" in PutObjectInput that limit didn't appear in signedheaders, so the client can successfully upload a txt file with content type text/plain(it returns HTTP status 200)
3. got NotImplemented error, I found that I didn't set the content length. the "NotImplemented error" is misleading.
the same presigned URL can be used in go client, but in PostMan, it returns the SignatureDoesNotMatch error. Copy the curl code exported by postman,it works too. why?

Reproduction Steps

server-side code:
generate upload URL

preReq, err := s.presvc.PresignPutObject(context.Background(), &s3.PutObjectInput{
		Bucket: aws.String(bucket),
		Key:    aws.String(key),
		ContentType: aws.String("image/png"),
	}, s3.WithPresignExpires(time.Hour))

client-side 1:

stats, _ := os.Stat(path)
f, _ := os.Open(path)
req, err := http.NewRequest("PUT", uploadURL, f)
// we can't change this 2 lines ,as client are not under our control, they got resigned URL from java server before.
req.Header.Set("Content-Type", "image/png")
req.Header.Set("Cache-Control", "public, max-age=31536000")
req.ContentLength = stats.Size()
response, err := http.DefaultClient.Do(req)

it works.

client-side 2:
use Postman
I copy that URL in postman, set the method to PUT, in the body, I choose binary and upload a file, then I get a SignatureDoesNotMatch error.
(when i set acl in putObjectInput and generate a presigned URL, i can use postman to upload a file with header X-Amz-acl)

client-side 3:
just use the curl code exported by postMan in client-side 2, it works. why?it both works with or without Content-Type header.
curl --location --request PUT 'https://<my-bucket>.s3.us-east-1.amazonaws.com/<my-key>?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=<xxx>%2F20220430%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20220430T103611Z&X-Amz-Expires=3600&X-Amz-SignedHeaders=host&x-id=PutObject&X-Amz-Signature=7f9a9a6fd1652ad483aadbe87fe7d20c564ddbb209bfccf8aaa92a2e5a4cb8fa ' \ --header 'Content-Type: image/png' \ --data-binary '@path/redis.png'

BTW: I don't know how to set that object's ACL in presigned URL, the options just contain a few like WithPresignExpires.

I searched in test.go, go examples, document in AWS, but they just told me how to generate a resigned URL, never told me how to use it.I see this issue: #1134 it seems not to work(if that means I should inclued headers that X-Amz-SignedHeaders provided when I use that presinged URL), that can't explain the failure in postman.

Possible Solution

No response

Additional Information/Context

No response

AWS Go SDK V2 Module Versions Used

github.com/aws/aws-sdk-go-v2 v1.16.3
github.com/aws/aws-sdk-go-v2/credentials v1.12.0
github.com/aws/aws-sdk-go-v2/service/s3 v1.26.7

Compiler and Version used

1.18

Operating System and version

macOs monterey

@congbaoyangrou congbaoyangrou added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Apr 30, 2022
@congbaoyangrou congbaoyangrou changed the title can not use presigned url to upload file can not create presigned url with canned acl Apr 30, 2022
@congbaoyangrou congbaoyangrou changed the title can not create presigned url with canned acl can't create presigned url with canned acl, can't limit content type Apr 30, 2022
@vudh1 vudh1 self-assigned this May 24, 2022
@congbaoyangrou
Copy link
Author

congbaoyangrou commented Jun 16, 2022

btw i solved this by using raw presign:

httpSigner := v4.NewSigner()
	uri := "xxx"
	req, _ := http.NewRequest("PUT", uri, nil)
	params := url.Values{
		"X-Amz-Credential": {fmt.Sprintf("%s/%s/us-east-1/s3/aws4_request", AccessKeyID, time.Now().Format("20060102"))},
		"X-Amz-Expires":    {"3600"},
		"x-id":             {"PutObject"},
		"x-amz-acl":        {"public-read"},
	}
	req.Header.Set("Content-Type", mimeType)
	req.URL.RawQuery = params.Encode()
	auth, _ := s.auth.Retrieve(ctx)
	u, _, e := httpSigner.PresignHTTP(ctx, auth, req, "UNSIGNED-PAYLOAD", "s3", "us-east-1", time.Now())
	

@vudh1 vudh1 removed their assignment Aug 25, 2022
@RanVaknin RanVaknin added p2 This is a standard priority issue m Effort estimation: medium labels Nov 14, 2022
@RanVaknin RanVaknin self-assigned this Jan 9, 2023
@RanVaknin RanVaknin added investigating This issue is being investigated and/or work is in progress to resolve the issue. and removed needs-triage This issue or PR still needs to be triaged. labels Feb 14, 2023
@tyliggity
Copy link

This works and is simpler than doing the whole thing raw: https://stackoverflow.com/a/74585074/13383986

@elousiv
Copy link

elousiv commented Dec 1, 2023

Are there any updates on this? Without the ability to set Conditions so we can set content type and content length range, similar to JS SDK and Boto, there is a security issue. Meaning, anyone with a valid presigned URL can upload a large file of any content type. Why is this not being investigated?

@tyliggity
Copy link

Are there any updates on this? Without the ability to set Conditions so we can set content type and content length range, similar to JS SDK and Boto, there is a security issue. Meaning, anyone with a valid presigned URL can upload a large file of any content type. Why is this not being investigated?

The workaround is to manually set the headers (and they are case sensitive) as shown here: https://stackoverflow.com/a/74585074/13383986

But it's still a bit ridiculous that this workaround is necessary at all considering it does seem like a tiny fix 🤷‍♂️

@elousiv
Copy link

elousiv commented Dec 1, 2023

Are there any updates on this? Without the ability to set Conditions so we can set content type and content length range, similar to JS SDK and Boto, there is a security issue. Meaning, anyone with a valid presigned URL can upload a large file of any content type. Why is this not being investigated?

The workaround is to manually set the headers (and they are case sensitive) as shown here: https://stackoverflow.com/a/74585074/13383986

But it's still a bit ridiculous that this workaround is necessary at all considering it does seem like a tiny fix 🤷‍♂️

@tyliggity thanks for following up with a workaround! While this does address ACL and content type it doesn't address (unless i'm missing something of course) the ability to set conditions such as content-length-range. Can that be sent as a header as well?

@RanVaknin
Copy link
Contributor

RanVaknin commented Dec 18, 2023

Hi all,

I'm able to set the content-type on the object I'm uploading via the presigned URL:

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

	client := s3.NewFromConfig(cfg)

	presigner := s3.NewPresignClient(client)

	data := bytes.NewBuffer([]byte("hello"))

	presignObject, err := presigner.PresignPutObject(context.TODO(), &s3.PutObjectInput{
		Bucket: aws.String(bucketName),
		Key:    aws.String("sample4.txt"),
		Body:   data,
		ContentType: aws.String("image/png"),
	})
	if err != nil {
		panic(err)
	}

	fmt.Println(presignObject.URL)
	

	request, err := http.NewRequest(presignObject.Method, presignObject.URL, data)
	if err != nil {
		panic(err)
	}

	// add all the signed headers from the presigned URL to the headers to the request.
	for k, vs := range presignObject.SignedHeader {
		for _, v := range vs {
			request.Header.Add(k, v)
		}
	}

	res, err := http.DefaultClient.Do(request)
	if err != nil {
		panic(err.Error())
	}

	fmt.Println(res.Status)

	bodyBytes, err := io.ReadAll(res.Body)
	if err != nil {
		panic(err.Error())
	}
	res.Body.Close()

	bodyString := string(bodyBytes)
	fmt.Println(bodyString)
}

You can log the signing proccess and see that the presigned URL does sign the content type as a header:

SDK 2023/12/18 13:02:05 DEBUG Request Signature:
---[ CANONICAL STRING  ]-----------------------------
PUT
/sample4.txt
X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=REDACTED%2F20231218%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20231218T210205Z&X-Amz-Expires=900&X-Amz-SignedHeaders=content-length%3Bcontent-type%3Bhost&x-id=PutObject
content-length:5
content-type:image/png
host:testbucket.s3.us-east-1.amazonaws.com

content-length;content-type;host
UNSIGNED-PAYLOAD
---[ STRING TO SIGN ]--------------------------------
AWS4-HMAC-SHA256
20231218T210205Z
20231218/us-east-1/s3/aws4_request
d1fc129b1dafe8b5f914c8d765298567657153b87b57e19d05e550528d58b927
---[ SIGNED URL ]------------------------------------
https://testbucket.s3.us-east-1.amazonaws.com/sample4.txt?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=REDACTED%2F20231218%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20231218T210205Z&X-Amz-Expires=900&X-Amz-SignedHeaders=content-length%3Bcontent-type%3Bhost&x-id=PutObject&X-Amz-Signature=REDACTED
-----------------------------------------------------
https://testbucket.s3.us-east-1.amazonaws.com/sample4.txt?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=REDACTED%2F20231218%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=REDACTED&X-Amz-Expires=900&X-Amz-SignedHeaders=content-length%3Bcontent-type%3Bhost&x-id=PutObject&X-Amz-Signature=REDACTED
200 OK

This will result in my text file uploaded with the defined content-type image/png.
image

I understand that you are looking to restrict files based on the provided content type but unfortunately the S3 server itself does not enforce the content-type for presigned URLs. I think this is for flexibility reasons.

As far as content-length-range goes, that is a feature of a POST policy, and does not have support with Presigned Put requests. The S3 server does however enforce content-length.
Thanks,
Ran~

@RanVaknin RanVaknin added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. m Effort estimation: medium labels Dec 18, 2023
Copy link

This issue has not received a response in 1 week. If you want to keep this issue open, please just leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Dec 21, 2023
@lucix-aws lucix-aws added guidance Question that needs advice or information. closed-for-staleness and removed bug This issue is a bug. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. closing-soon This issue will automatically close in 4 days unless further comments are made. p2 This is a standard priority issue labels Dec 26, 2023
@lucix-aws lucix-aws closed this as not planned Won't fix, can't repro, duplicate, stale Dec 26, 2023
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-for-staleness guidance Question that needs advice or information.
Projects
None yet
Development

No branches or pull requests

6 participants