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

Add enhanced presign request feature #1477

Closed
wants to merge 14 commits into from

Conversation

cjyclaire
Copy link
Contributor

@cjyclaire cjyclaire commented Apr 5, 2017

This PR adds support for Aws::S3::PresignedRequest that allows more flexible and customized presigned behaviors, refractored based on aws-sigv4 gem and provides helpful #headers for bucket policy work arounds etc..

Documentations and tests(rspec & smoke) both have been addressed in the PR, here is a sample usage:

req = Aws::S3::PresignedRequest.new(:put_object,
  bucket: 'bktname',
  key: 'key',
  acl: 'private',
  cache_control: 'max-age=20000'
  headers: {
    'foo' => 'bar',
    'cache_control' => 'max-age=20000'
  }
)

req.uri
# => #<URI::HTTPS https://bktname. ... .&X-Amz-SignedHeaders=host%3Bx-amz-acl%3Bx-amz-canche-control%3Bx-amz-foo...

req.headers
# => {"x-amz-acl"=>"private", "x-amz-canche-control" => "max-age=20000", "x-amz-foo"=>"bar"}

Related issues, PRs and feature requests:
#874, #1152, #1384
#1228, #1403 ( #1400, #1401)
#2098

cc:\ @awood45 @trevorrowe

Copy link
Member

@trevorrowe trevorrowe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My review is incomplete, but I feel like the interface needs to be reworked to support correct serialization of structured header values and structured querystring parameters. I'm undecided what S3 client operations need to be supported. We need to better define the expected interface and features.

# You should only set this value for testing.
def initialize(method, params = {})
@bucket, @key = bucket_and_key(params)
@client = params.delete(:client) || Aws::S3::Client.new
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do this in multiple places, so this pattern is not new (inject or construct a client), but we do not actually require a client. We actually need a region, and a set of credentials. The client object in inconsequential. It seems like a better solution would be to have a public interface for getting the default region, and a public interface for getting default credentials.

The client configuration can be updated to use these interfaces, so the behavior remains consistent, and then the code can be reduced down to injecting an optional region and or credentials.

def initialize(method, params = {})
@bucket, @key = bucket_and_key(params)
@client = params.delete(:client) || Aws::S3::Client.new
@http_method = method[/[^_]+/].upcase
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can determine this correctly for every API operation by simply using the API object.

Aws::S3::Client.api.operation(method).http_method

This has the benefit of it will work for more than object operations.

@expires = expires_in(params)
@virtual_host = !!params.delete(:virtual_host)
@secure = params.delete(:secure)
@headers = build_headers(params)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation of build_headers is limited to only support those params that have x-amz-* prefixes. As such you will run into issues with things like x-amz-metadata-* headers that are given as hashes normally, but this method will require them to be given as strings with metadata-{key} prefixes.

It would be ideal to preserve the same interface we use when making client calls.

@cjyclaire
Copy link
Contributor Author

Still in progress, yet have finished refractor using client request handler in constructing urls.

@cjyclaire
Copy link
Contributor Author

cjyclaire commented Apr 18, 2017

Based on the discussion our team had off-line, we will work on a more generalized solution that brings much more benefits and flexibility in the long run (involving client level, request level etc.). And that is a feature has been prioritized in our backlog, and will be shipped through Modularization Ruby SDK which is currently under preview release and will be in GA in the near future. So this PR will not be merged.

Meanwhile, aws-sigv4 gem is available for general, flexible and customized presign behavior and signer usage, with fully documented source code.

This PR will still be kept opened for a while for feedbacks and thoughts! : )

@awood45 awood45 force-pushed the master branch 2 times, most recently from cc532a5 to 6e56849 Compare June 29, 2017 21:49
@oyeanuj
Copy link

oyeanuj commented Jul 7, 2017

@cjyclaire I am using v3 and the code below, but wondering how can I add a custom header to my PUT request. Adding it on the client-side throws an error, so it seems like I must do it while creating the pre-signed url?

signer = Aws::S3::Presigner.new(client: s3)
self.presigned_url = signer.presigned_url(
  :put_object,
  bucket:	self.bucket,
  key:	self.file_key
)

So, is it possible right now in v3? If not, what would be the equivalent code for the code above with the gem mentioned above?

Thank you!

@cjyclaire
Copy link
Contributor Author

@oyeanuj Sure, you can do it both in V2 and V3, yet I'd recommend using the aws-sigv4 gem with code snippets as below:

require `aws-sigv4`

# a s3 client at region 'us-west-2'
signer = Aws::Sigv4::Signer.new( 
  service: 's3', 
  region: 'us-west-2', 
  credentials_provider: client.config.credentials, 
  uri_escape_path: false, 
)

# create presigned url for an object with bucket 'a-fancy-bucket' and key 'hello_world' 
url = signer.presign_url( 
  http_method: 'PUT', 
  url:'https://a-fancy-bucket.s3-us-west-2.amazonaws.com/hello_world', 
  headers: {                                                                                                                             
     "x-amz-foo" => "bar"                                                                                       
  } 
  body_digest: 'UNSIGNED-PAYLOAD', 
) 

# making request
body = ...
Net::HTTP.start(url.host) do |http|
  http.send_request("PUT", url.request_uri, body, { 
    "x-amz-foo" => "bar", 
  })
end
# => #<Net::HTTPOK 200 OK readbody=true>

@oyeanuj
Copy link

oyeanuj commented Jul 9, 2017

@cjyclaire Thanks! Just curious, is there an advantage of using aws-sigv4 over v3 aws-sdk-s3? FWIW, I need the url just to pass to the client side to make the request.

@cjyclaire cjyclaire added needs-major-version Can only be considered for the next major release and removed work-in-progress labels Sep 17, 2019
@ajredniwja ajredniwja removed the v3 label Feb 26, 2020
@mullermp
Copy link
Contributor

Closing - I think this solution solves this case? #874 (comment)

@mullermp mullermp closed this May 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-major-version Can only be considered for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants