durch / rust-s3

Rust library for interfacing with S3 API compatible services
MIT License
519 stars 198 forks source link

Rework presign_post to be correct #358

Closed urkle closed 11 months ago

urkle commented 1 year ago

@durch please provide commends on code-style choices and contracts.

Right now this is a fully working model to get presigned posts actually working based on the documentation from these pages

https://docs.aws.amazon.com/AmazonS3/latest/API/RESTObjectPOST.html https://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-HTTPPOSTConstructPolicy.html https://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-post-example.html

I even got the same exact signature when faking the dates and base64 payload as the post-example 😄 I had to fake the payload as their example has things pretty formatted and the hash is in a slightly different order.

Reference: #339

urkle commented 1 year ago

@durch Do you have any feedback on this?

One change I'm thinking about is making the field maps a Cow instead to reduce copies for largely static strings.

urkle commented 1 year ago

I pushed a fixup commit that switches to Cow for the PostPolicy. let me know if that is better and I'll squash the commits down.

FYI. I'm using this all locally in a project. It seems all of the S3 libraries for rust seem to lack PresignedPost support even amazon's.

yallxe commented 1 year ago

Any updates on this? What is the status of this PR?

urkle commented 1 year ago

@yallxe you can point your Cargo.toml to this branch to test it locally and verify it works for you as well (vs just me).

Add this section to your Cargo.toml

[patch.crates-io]
rust-s3 = { git = "https://github.com/urkle/rust-s3", branch = "feat-fix-presigned-post" }
Leonils commented 1 year ago

Hello @urkle. Nice work on this api.

One detail though, I think the documentation test does not compile. Am I wrong? Following your unit test structure, I would rather assume that

    /// Get a presigned url for posting an object to a given path
    ///
    /// # Example:
    ///
    /// ```no_run
    /// use s3::bucket::Bucket;
    /// use s3::creds::Credentials;
    /// use s3::post_policy::*;
    /// use http::HeaderMap;
    /// use http::header::HeaderName;
+    /// use std::borrow::Cow;
    ///
    /// let bucket_name = "rust-s3-test";
    /// let region = "us-east-1".parse().unwrap();
    /// let credentials = Credentials::default().unwrap();
    /// let bucket = Bucket::new(bucket_name, region, credentials).unwrap();
    ///
    /// let post_policy = PostPolicy::new(86400).condition(
    ///     PostPolicyField::Key,
-    ///     PostPolicyValue::StartsWith("user/user1/".to_string())
+    ///     PostPolicyValue::StartsWith(Cow::Borrowed("user/user1/"))
    /// );
    ///
-    /// let (url, fields) = bucket.presign_post(post_policy).unwrap();
+   /// let PresignedPost { url, fields, .. } = bucket.presign_post(post_policy).unwrap();
-    /// println!("Presigned url: {}, fields: {}", url, fields);
+    /// println!("Presigned url: {}, fields: {?:}", url, fields);
    /// ```
Leonils commented 1 year ago

Just for further context, in the AWS API,

urkle commented 1 year ago

One detail though, I think the documentation test does not compile. Am I wrong? Following your unit test structure, I would rather assume that

* the policies values are expected to be Cows smart pointers, not strings

* the result is a structure, not just a tuple, and the fields do not implement the `Display` trait

Thanks for catching this. Originally I had just strings and switched it to Cow in a followup commit but forgot to re-run example tests. And at one point it was a tuple, but I reworked it as things became more complex.

I'll update these doc examples and ensure ALL tests are passing :)

durch commented 11 months ago

@urkle wow, great work, thanks for your patience, I come back to this repo in bursts, this looks great

urkle commented 11 months ago

🕺