Add support for unsigned payloads in aws #3741

trueleo · 2023-02-21T11:38:59Z

Which issue does this PR close?

Closes #3737 .

Rationale for this change

Explained in #3737

What changes are included in this PR?

Adds with_unsigned_payload configuration option to AmazonS3Builder.
Adds sign_payload bool to S3Config and RequestSigner.
If sign_payload is set to true it will work as before and set a SHA256 hash to x-amz-content-sha256. Otherwise it will set it as "UNSIGNED_PAYLOAD"

Are there any user-facing changes?

AmazonS3Builder has a new method named with_unsigned_payload

roeap

We should add the appropriate key to AmazonS3ConfigKey, and corresponding parsing (FromStr) and serialisation (AsRef) methods, such that the new option can be configured via the storage options interface.

trueleo · 2023-02-22T08:53:51Z

@roeap How do we add integration test for this option.

roeap · 2023-02-22T09:28:52Z

How do we add integration test for this option.

There are some tests for these options, you could add a case in that test.

arrow-rs/object_store/src/aws/mod.rs

Lines 1194 to 1218 in ceba128

    
           fn s3_test_config_from_typed_map() { 
        
               let aws_access_key_id = "object_store:fake_access_key_id".to_string(); 
        
               let aws_secret_access_key = "object_store:fake_secret_key".to_string(); 
        
               let aws_default_region = "object_store:fake_default_region".to_string(); 
        
               let aws_endpoint = "object_store:fake_endpoint".to_string(); 
        
               let aws_session_token = "object_store:fake_session_token".to_string(); 
        
               let options = HashMap::from([ 
        
                   (AmazonS3ConfigKey::AccessKeyId, aws_access_key_id.clone()), 
        
                   (AmazonS3ConfigKey::SecretAccessKey, aws_secret_access_key), 
        
                   (AmazonS3ConfigKey::DefaultRegion, aws_default_region.clone()), 
        
                   (AmazonS3ConfigKey::Endpoint, aws_endpoint.clone()), 
        
                   (AmazonS3ConfigKey::Token, aws_session_token.clone()), 
        
               ]); 
        
               let builder = AmazonS3Builder::new() 
        
                   .try_with_options(&options) 
        
                   .unwrap() 
        
                   .try_with_option(AmazonS3ConfigKey::SecretAccessKey, "new-secret-key") 
        
                   .unwrap(); 
        
               assert_eq!(builder.access_key_id.unwrap(), aws_access_key_id.as_str()); 
        
               assert_eq!(builder.secret_access_key.unwrap(), "new-secret-key"); 
        
               assert_eq!(builder.region.unwrap(), aws_default_region); 
        
               assert_eq!(builder.endpoint.unwrap(), aws_endpoint); 
        
               assert_eq!(builder.token.unwrap(), aws_session_token); 
        
           }

roeap

Thanks! Just a few minor comments, but overall I think this looks good!

object_store/src/aws/credential.rs

roeap · 2023-02-23T22:26:52Z

object_store/src/aws/mod.rs

@@ -822,6 +837,13 @@ impl AmazonS3Builder {
        self
    }

+    /// Sets the client to not include payload checksum in signature calculation.
+    /// See [unsigned payload option](https://docs.aws.amazon.com/AmazonS3/latest/API/sig-v4-header-based-auth.html)
+    pub fn with_unsigned_payload(mut self) -> Self {


in other cases for boolean options, we pass in the actual parameter, so that true and false can explicitly be set. While not overly critical, I think its better to be consistent,.

Making this change but for reference an existing option with_imdsv1_fallback does not take boolean argument.

I am not sure what general convention are around this.

object_store/src/aws/mod.rs

roeap

LGTM.

tustvold

This looks good to me, thank you. Apologies for the slow review

tustvold · 2023-02-27T12:58:15Z

object_store/src/aws/mod.rs

@@ -1222,6 +1252,14 @@ mod tests {
        stream_get(&integration).await;
    }

+    #[tokio::test]
+    async fn s3_test_unsigned_payload() {


This appears to race with s3_test above, it likely needs to be rolled into the same test

Oh right, will it be better to add something like https://crates.io/crates/serial_test

I would rather avoid additional test dependencies if possible. I see no issue with just merging this into the same test as above, the stack trace will make it clear what test failed

tustvold · 2023-02-27T23:02:21Z

I'm afraid I messed up the sequencing and so this missed the release today... Too many plates... Thank you once again

ursabot · 2023-02-27T23:08:06Z

Benchmark runs are scheduled for baseline = 5cc0f9b and contender = e7eb304. e7eb304 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

Add support for unsigned payloads in aws

84e2f3a

github-actions bot added the object-store Object Store Interface label Feb 21, 2023

roeap reviewed Feb 22, 2023

View reviewed changes

Add unsigned payload to AmazonS3ConfigKey

ceba128

trueleo added 4 commits February 22, 2023 15:17

Link to aws doc

bd98018

Add env test

f5caf55

Add test

f94a20d

Add integration test

5bfb186

trueleo requested a review from roeap February 22, 2023 14:56

roeap reviewed Feb 23, 2023

View reviewed changes

trueleo added 4 commits February 24, 2023 13:45

Take boolean argument

8b2579b

Fix doc

a8f9460

Merge branch 'master' into unsigned_s3_request

3c644c1

Clippy fixes

31bf8f2

roeap approved these changes Feb 24, 2023

View reviewed changes

tustvold approved these changes Feb 27, 2023

View reviewed changes

tustvold reviewed Feb 27, 2023

View reviewed changes

trueleo added 2 commits February 27, 2023 20:01

Merge into s3 test

f164df2

Merge branch 'master' into unsigned_s3_request

b17574d

tustvold mentioned this pull request Feb 27, 2023

Release object_store 0.5.5 (next release after 0.5.4) #3768

Closed

tustvold merged commit e7eb304 into apache:master Feb 27, 2023

trueleo deleted the unsigned_s3_request branch February 28, 2023 10:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for unsigned payloads in aws #3741

Add support for unsigned payloads in aws #3741

trueleo commented Feb 21, 2023

roeap left a comment

trueleo commented Feb 22, 2023

roeap commented Feb 22, 2023

roeap left a comment

roeap Feb 23, 2023

trueleo Feb 24, 2023 •

edited

Loading

roeap left a comment

tustvold left a comment

tustvold Feb 27, 2023

trueleo Feb 27, 2023

tustvold Feb 27, 2023 •

edited

Loading

tustvold commented Feb 27, 2023

ursabot commented Feb 27, 2023

Add support for unsigned payloads in aws #3741

Add support for unsigned payloads in aws #3741

Conversation

trueleo commented Feb 21, 2023

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

roeap left a comment

Choose a reason for hiding this comment

trueleo commented Feb 22, 2023

roeap commented Feb 22, 2023

roeap left a comment

Choose a reason for hiding this comment

roeap Feb 23, 2023

Choose a reason for hiding this comment

trueleo Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

roeap left a comment

Choose a reason for hiding this comment

tustvold left a comment

Choose a reason for hiding this comment

tustvold Feb 27, 2023

Choose a reason for hiding this comment

trueleo Feb 27, 2023

Choose a reason for hiding this comment

tustvold Feb 27, 2023 • edited Loading

Choose a reason for hiding this comment

tustvold commented Feb 27, 2023

ursabot commented Feb 27, 2023

trueleo Feb 24, 2023 •

edited

Loading

tustvold Feb 27, 2023 •

edited

Loading