Repository: rusoto/rusoto (language: rust)

Issue:
Presigned URL generates invalid signature for non S3 Endpoints
I am trying to replicate the implementation of authentication for [`aws-iam-authenticator`](https://github.com/kubernetes-sigs/aws-iam-authenticator#api-authorization-from-outside-a-cluster) for use with an EKS cluster. I noticed that using `Request::generate_presigned_url` always generated invalid signatures in the URL.

I went to look at the source code of Botocore and noticed that for S3 pre-signed URLs, the payload used is always [`UNSIGNED-PAYLOAD`](https://github.com/boto/botocore/blob/12086f1c31986287bd4e0ed385358a9af48faf0f/botocore/auth.py#L582). 

For other non-S3 endpoints that uses the `SigV4QueryAuth` class, the `SigV4QueryAuth` does not override the `payload` method of the base `SigV4Auth` class and the `payload` method does sign the payload, even when it's empty.

I have a [patch](https://github.com/rusoto/rusoto/compare/master...basisai:presigned-payload) in my fork to replicate this behaviour and I can confirm that the presigned URL now work.

Should I open a PR with my patch, or is there a better way to support this use-case?

See the [generic AWS docs](https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html) for pre-signed URL.


Discussion / hints:

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index cf1031dbdcf..0f538784b9a 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,8 +1,11 @@
 # Rusoto changes
 
 ## [Unreleased]
+
 - Exclude `test_resources` in cargo manifest
 - upgrades botocore version to from `1.12.156` to `1.12.163`
+- (Breaking Change) Fix invalid signatures on presigned URLs by adding a new
+  `should_sha256_sign_payload` argument to `SignedRequest::generate_presigned_url`.
 
 ## [0.39.0] - 2019-05-19
 
diff --git a/rusoto/core/src/signature.rs b/rusoto/core/src/signature.rs
index 6e49bf9dcdc..e757f714465 100644
--- a/rusoto/core/src/signature.rs
+++ b/rusoto/core/src/signature.rs
@@ -6,6 +6,7 @@
 //! If needed, the request will be re-issued to a temporary redirect endpoint.  This can happen with
 //! newly created S3 buckets not in us-standard/us-east-1.
 
+use std::borrow::Cow;
 use std::collections::btree_map::Entry;
 use std::collections::BTreeMap;
 use std::fmt;
@@ -27,6 +28,12 @@ use crate::param::{Params, ServiceParams};
 use crate::region::Region;
 use crate::stream::ByteStream;
 
+/// Payload string to use for unsigned payload
+pub static UNSIGNED_PAYLOAD: &str = "UNSIGNED-PAYLOAD";
+/// Payload string to use for signed empty payload
+pub static EMPTY_SHA256_HASH: &str =
+    "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855";
+
 /// Possible payloads included in a `SignedRequest`.
 pub enum SignedRequestPayload {
     /// Transfer payload in a single chunk
@@ -234,11 +241,15 @@ impl SignedRequest {
         self.params = params;
     }
 
-    /// http://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-query-string-auth.html
+    /// Generate a Presigned URL for AWS
+    ///
+    /// See the [documentation](https://docs.aws.amazon.com/general/latest/gr/sigv4_signing.html)
+    /// for more information.
     pub fn generate_presigned_url(
         &mut self,
         creds: &AwsCredentials,
         expires_in: &Duration,
+        should_sha256_sign_payload: bool,
     ) -> String {
         debug!("Presigning request URL");
 
@@ -260,9 +271,9 @@ impl SignedRequest {
         self.remove_header("Content-Type");
 
         if let Some(ref token) = *creds.token() {
-            self.remove_header("x-amz-security-token");
+            self.remove_header("X-Amz-Security-Token");
             self.params
-                .put("x-amz-security-token", encode_uri_strict(token));
+                .put("X-Amz-Security-Token", encode_uri_strict(token));
         }
 
         self.remove_header("X-Amz-Algorithm");
@@ -299,6 +310,23 @@ impl SignedRequest {
         debug!("signed_headers: {:?}", signed_headers);
         debug!("canonical_query_string: {:?}", self.canonical_query_string);
 
+        let payload = if should_sha256_sign_payload {
+            match self.payload {
+                None => {
+                    Cow::Borrowed(EMPTY_SHA256_HASH)
+                }
+                Some(SignedRequestPayload::Buffer(ref payload)) => {
+                    let (digest, _len) = digest_payload(&payload);
+                    Cow::Owned(digest)
+                }
+                Some(SignedRequestPayload::Stream(ref _stream)) => {
+                    Cow::Borrowed(UNSIGNED_PAYLOAD)
+                }
+            }
+        } else {
+            Cow::Borrowed(UNSIGNED_PAYLOAD)
+        };
+
         let canonical_request = format!(
             "{}\n{}\n{}\n{}\n{}\n{}",
             &self.method,
@@ -306,7 +334,7 @@ impl SignedRequest {
             self.canonical_query_string,
             canonical_headers,
             &signed_headers,
-            "UNSIGNED-PAYLOAD"
+            payload
         );
 
         debug!("canonical_request: {:?}", canonical_request);
@@ -412,9 +440,9 @@ impl SignedRequest {
                     self.canonical_query_string,
                     canonical_headers,
                     signed_headers,
-                    &to_hexdigest("")
+                    EMPTY_SHA256_HASH
                 );
-                (Some(to_hexdigest("")), Some(0))
+                (Some(Cow::Borrowed(EMPTY_SHA256_HASH)), Some(0))
             }
             Some(SignedRequestPayload::Buffer(ref payload)) => {
                 let (digest, len) = digest_payload(&payload);
@@ -427,7 +455,7 @@ impl SignedRequest {
                     signed_headers,
                     &digest
                 );
-                (Some(digest), Some(len))
+                (Some(Cow::Owned(digest)), Some(len))
             }
             Some(SignedRequestPayload::Stream(ref stream)) => {
                 canonical_request = format!(
@@ -437,9 +465,9 @@ impl SignedRequest {
                     self.canonical_query_string,
                     canonical_headers,
                     signed_headers,
-                    "UNSIGNED-PAYLOAD"
+                    UNSIGNED_PAYLOAD
                 );
-                (Some("UNSIGNED-PAYLOAD".to_owned()), stream.size_hint())
+                (Some(Cow::Borrowed(UNSIGNED_PAYLOAD)), stream.size_hint())
             }
         };
 
diff --git a/rusoto/services/s3/src/custom/util.rs b/rusoto/services/s3/src/custom/util.rs
index 7fc3212d664..5e10ec77994 100644
--- a/rusoto/services/s3/src/custom/util.rs
+++ b/rusoto/services/s3/src/custom/util.rs
@@ -109,7 +109,7 @@ impl PreSignedRequest for GetObjectRequest {
         );
 
         request.set_params(params);
-        request.generate_presigned_url(credentials, &option.expires_in)
+        request.generate_presigned_url(credentials, &option.expires_in, false)
     }
 }
 
@@ -156,7 +156,7 @@ impl PreSignedRequest for PutObjectRequest {
             }
         }
 
-        request.generate_presigned_url(credentials, &option.expires_in)
+        request.generate_presigned_url(credentials, &option.expires_in, false)
     }
 }
 
@@ -178,7 +178,7 @@ impl PreSignedRequest for DeleteObjectRequest {
         );
 
         request.set_params(params);
-        request.generate_presigned_url(credentials, &option.expires_in)
+        request.generate_presigned_url(credentials, &option.expires_in, false)
     }
 }
 
@@ -201,6 +201,6 @@ impl PreSignedRequest for UploadPartRequest {
             request_payer, "x-amz-request-payer";
         );
 
-        request.generate_presigned_url(credentials, &option.expires_in)
+        request.generate_presigned_url(credentials, &option.expires_in, false)
     }
 }


Test patch:
diff --git a/integration_tests/Cargo.toml b/integration_tests/Cargo.toml
index 6bb4edcafbc..fe6952c9683 100644
--- a/integration_tests/Cargo.toml
+++ b/integration_tests/Cargo.toml
@@ -635,6 +635,7 @@ http = "0.1.17"
 
 [features]
 all = [
+		"core",
 		"acm",
 		"acm-pca",
 		"alexaforbusiness",
@@ -785,6 +786,7 @@ all = [
 		"apigatewayv2",
 		"ram"
 	]
+core = []
 acm = ["rusoto_acm"]
 acm-pca = ["rusoto_acm_pca"]
 alexaforbusiness = ["rusoto_alexaforbusiness"]
diff --git a/integration_tests/tests/core.rs b/integration_tests/tests/core.rs
new file mode 100644
index 00000000000..8235ac95ee6
--- /dev/null
+++ b/integration_tests/tests/core.rs
@@ -0,0 +1,38 @@
+#![cfg(feature = "core")]
+
+extern crate futures;
+extern crate reqwest;
+extern crate rusoto_core;
+
+use futures::Future;
+
+use rusoto_core::credential::{DefaultCredentialsProvider, ProvideAwsCredentials};
+use rusoto_core::param::{Params, ServiceParams};
+use rusoto_core::signature::SignedRequest;
+use rusoto_core::Region;
+
+#[test]
+fn get_caller_identity_presigned() {
+    let provider = DefaultCredentialsProvider::new().unwrap();
+    let credentials = provider.credentials().wait().unwrap();
+
+    let mut request = SignedRequest::new("GET", "sts", &Region::UsEast1, "/");
+    let mut params = Params::new();
+    params.put("Action", "GetCallerIdentity");
+    params.put("Version", "2011-06-15");
+    request.set_params(params);
+    request.add_header("x-test-header", "foobar");
+    let url =
+        request.generate_presigned_url(&credentials, &std::time::Duration::from_secs(60), true);
+
+    let client = reqwest::Client::new();
+    let response = client
+        .get(&url)
+        .header("x-test-header", "foobar")
+        .send()
+        .expect("to succeed");
+    assert!(
+        response.status().is_success(),
+        "presigned url should succeed when used"
+    );
+}