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 but no ContentLength results in URL that doesn't include ContentType in the signed headers #2063

Closed
ieguinoa opened this issue Mar 22, 2023 · 6 comments
Assignees
Labels
bug This issue is a bug. closing-soon This issue will automatically close in 4 days unless further comments are made. p3 This is a minor priority issue response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days.

Comments

@ieguinoa
Copy link

Describe the bug

Signing a PUT URL with a ContentType value results in a signed URL without Content-Type included in the signed headers unless a ContentLength is also specified.

Expected Behavior

When signing a request with ContentType but no ContentLength I expect to have the signed URL containing content-type but no content-length in the

Current Behavior

There is no content-type in the signed headers (X-Amz-SignedHeaders). The type header is silently removed from the request.

Reproduction Steps

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

Possible Solution

The problem is here, where it silently removes the content-type when content-length is == 0.
Ideally there should be a way to remove this RemoveContentTypeHeader middleware from the stack, or define a value of content-length that could escape that condition, may not be a clean solution but content-length=-1 could be a value that the user can specify to indicate they know the type but not the length of the content.

Additional Information/Context

This bug was reported before #1475 but no solution was given for the case when you don't know the length value, so you don't want to specify that header at all.

AWS Go SDK V2 Module Versions Used

v1.17.6

Compiler and Version used

1.20

Operating System and version

Ventura 13.2.1

@ieguinoa ieguinoa added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 22, 2023
@RanVaknin
Copy link
Contributor

Hi @ieguinoa ,

The problem is here, where it silently removes the content-type when content-length is == 0.

This is by design, this isn't a bug. Like I mentioned before in the issue you have linked, you must to supply content length to presigned url request if you are specifying content-type.

Ran

@RanVaknin RanVaknin self-assigned this Mar 22, 2023
@RanVaknin RanVaknin added 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 Mar 22, 2023
@RanVaknin RanVaknin assigned RanVaknin and isaiahvita and unassigned RanVaknin Mar 22, 2023
@ieguinoa
Copy link
Author

Hi @RanVaknin and @isaiahvita,

Thanks for the response.
I think the solution you proposed here does not apply to our case.
In our use case we don't know the contentLength value (therefore we can't use any fixed value and sign the url with it) but we do know the content type, and we want to enforce it during the uploading by adding it to the signed contents so we can be sure the uploader will upload content with that type.
Is there a way to make use of the sdk for this case?

Ignacio

@isaiahvita
Copy link
Contributor

isaiahvita commented Mar 26, 2023

Hi @ieguinoa

Thanks for the engagement. When it comes to presigned URLs we tend to bias towards removing potentially extraneous headers that would make it harder for customers to configure. Conceptually, presigned URLs are meant to be self-contained requests that will "just work" without need for modification.
While we agree that your use case seems like a valid one, we are going to have to be careful about outright removal of the RemoveContentTypeHeader middleware from this operation due to backwards compatibility concerns. We will keep your use case in mind as we reevaluate this in the future.

In the meantime, you can actually customize this behavior for your particular use case by providing a custom client option

package main

import (
	"bytes"
	"context"
	"log"
	"net/http"
	"github.com/aws/aws-sdk-go-v2/aws"
	"github.com/aws/aws-sdk-go-v2/config"
	"github.com/aws/aws-sdk-go-v2/service/s3"
	smithyhttp "github.com/aws/smithy-go/transport/http"
)


func main() {

	ctx := context.Background()
	cfg, err := config.LoadDefaultConfig(ctx, config.WithRegion("us-west-2"), config.WithClientLogMode(aws.LogRequest|aws.LogSigning))

	if err != nil {
		log.Fatalf("unable to load SDK config, %v", err)
	}

	s3Client := s3.NewFromConfig(cfg)
	presignClient := s3.NewPresignClient(s3Client)
	requestBody := []byte("foo")

	presignResp, _ := presignClient.PresignPutObject(ctx, &s3.PutObjectInput{
		Bucket: aws.String("PLACEHOLDER_BUCKET"),
		Key:    aws.String("PLACEHOLDER_FILENAME"),
	},
	func(o *s3.PresignOptions) {
		o.ClientOptions = append(o.ClientOptions, func(o *s3.Options) {
			 o.APIOptions = append(o.APIOptions, smithyhttp.AddHeaderValue("Content-Type", "text/plain"))
		})
	})

	req, _ := http.NewRequest(http.MethodPut, presignResp.URL, bytes.NewBuffer(requestBody))
	req.Header.Set("Content-Type", "text/plain")
	client := &http.Client{}
	client.Do(req)

}

@RanVaknin RanVaknin added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed needs-review This issue or pull request needs review from a core team member. labels Mar 28, 2023
@github-actions
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 Mar 29, 2023
@ieguinoa
Copy link
Author

Thanks for the response @isaiahvita, that approach did the trick for now.

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

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. closing-soon This issue will automatically close in 4 days unless further comments are made. 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

3 participants