Fix Canonical URI encoding during request signing (#16)

Fix Canonical URI encoding during request signing - resolves #193
cognitect-labs · Nov 28, 2022 · 5ebdfc8 · 5ebdfc8
1 parent 8d52391
commit 5ebdfc8
Show file tree

Hide file tree

Showing 6 changed files with 184 additions and 25 deletions.
diff --git a/deps.edn b/deps.edn
@@ -16,7 +16,8 @@
                                            org.slf4j/slf4j-reload4j    {:mvn/version "2.0.3"}
                                            http-kit/http-kit           {:mvn/version "2.6.0"}
                                            com.cognitect.aws/endpoints {:mvn/version "1.1.12.321"}
-                                           com.cognitect.aws/s3        {:mvn/version "822.2.1145.0"}}}
+                                           com.cognitect.aws/s3        {:mvn/version "822.2.1145.0"}
+                                           com.amazonaws/aws-java-sdk  {:mvn/version "1.12.344"}}}
            :test            {:extra-deps {io.github.cognitect-labs/test-runner {:git/tag "v0.5.1" :git/sha "dfb30dd"}}
                              :main-opts  ["-m" "cognitect.test-runner"]}
            :examples        {:extra-paths ["examples" "examples/resources" "dev/resources"]

diff --git a/src/cognitect/aws/signers.clj b/src/cognitect/aws/signers.clj
@@ -53,17 +53,30 @@
   (-> request-method name str/upper-case))
 
 (defn- canonical-uri
-  [{:keys [uri]}]
-  (let [encoded-path (-> uri
-                         (str/replace #"//+" "/") ; (URI.) throws Exception on '//'.
-                         (str/replace #"\s" "%20"); (URI.) throws Exception on space.
-                         (URI.)
-                         (.normalize)
-                         (.getPath)
-                         (uri-encode "/"))]
-    (if (.isEmpty ^String encoded-path)
+  [{:keys [uri]} {:keys [double-encode? normalize-uri?]}]
+  (let [[path _query] (str/split uri #"\?")
+        ^String encoded-path (-> path
+                                 (cond-> double-encode? (uri-encode "/"))
+                                 (str/replace #"^//+" "/") ; (URI.) throws Exception on '//' at beginning of string.
+                                 (str/replace #"\s" "%20"); (URI.) throws Exception on space.
+                                 (URI.)
+                                 (cond-> normalize-uri? (.normalize))
+                                 (.getPath)
+                                 (uri-encode "/"))]
+    (cond
+      (.isEmpty encoded-path)
       "/"
-      encoded-path)))
+
+      ;; https://github.com/aws/aws-sdk-java/blob/fd409de/aws-java-sdk-core/src/main/java/com/amazonaws/auth/AbstractAWSSigner.java#L392-L397
+      ;; Normalization can leave a trailing slash at the end of the resource path,
+      ;; even if the input path doesn't end with one. Example input: /foo/bar/.
+      ;; Remove the trailing slash if the input path doesn't end with one.
+      (and (not= encoded-path "/")
+           (str/ends-with? encoded-path "/")
+           (not (str/ends-with? path "/")))
+      (.substring encoded-path 0 (dec (.length encoded-path)))
+
+      :else encoded-path)))
 
 (defn- canonical-query-string
   [{:keys [uri query-string]}]
@@ -105,18 +118,18 @@
   (util/hex-encode (util/sha-256 (:body request))))
 
 (defn canonical-request
-  [{:keys [headers] :as request}]
+  [{:keys [headers] :as request} opts]
   (str/join "\n" [(canonical-method request)
-                  (canonical-uri request)
+                  (canonical-uri request opts)
                   (canonical-query-string request)
                   (canonical-headers-string request)
                   (signed-headers request)
                   (or (get headers "x-amz-content-sha256")
                       (hashed-body request))]))
 
 (defn string-to-sign
-  [request auth-info]
-  (let [bytes (.getBytes ^String (canonical-request request))]
+  [request auth-info opts]
+  (let [bytes (.getBytes ^String (canonical-request request opts))]
     (str/join "\n" ["AWS4-HMAC-SHA256"
                     (get-in request [:headers "x-amz-date"])
                     (credential-scope auth-info request)
@@ -133,13 +146,13 @@
       (util/hmac-sha-256 "aws4_request")))
 
 (defn signature
-  [auth-info request]
+  [auth-info request opts]
   (util/hex-encode
    (util/hmac-sha-256 (signing-key request auth-info)
-                      (string-to-sign request auth-info))))
+                      (string-to-sign request auth-info opts))))
 
 (defn v4-sign-http-request
-  [service endpoint credentials http-request & {:keys [content-sha256-header?]}]
+  [service endpoint credentials http-request & {:keys [content-sha256-header? double-url-encode? normalize-uri-paths?]}]
   (let [{:keys [:aws/access-key-id :aws/secret-access-key :aws/session-token]} credentials
         auth-info      {:access-key-id     access-key-id
                         :secret-access-key secret-access-key
@@ -156,16 +169,27 @@
                       (:access-key-id auth-info)
                       (credential-scope auth-info req)
                       (signed-headers req)
-                      (signature auth-info req)))))
-
+                      (signature auth-info req {:double-encode? double-url-encode?
+                                                :normalize-uri? normalize-uri-paths?})))))
+
+;; https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html
+;;
+;; Each path segment must be URI-encoded twice (except for Amazon S3 which only gets URI-encoded once).
+;;
+;; Normalize URI paths according to RFC 3986.
+;; In exception to this, you do not normalize URI paths for requests to Amazon S3
 (defmethod sign-http-request "v4"
   [service endpoint credentials http-request]
-  (v4-sign-http-request service endpoint credentials http-request))
+  (v4-sign-http-request service endpoint credentials http-request
+                        :double-url-encode? true
+                        :normalize-uri-paths? true))
 
 (defmethod sign-http-request "s3"
   [service endpoint credentials http-request]
-  (v4-sign-http-request service endpoint credentials http-request :content-sha256-header? true))
+  (v4-sign-http-request service endpoint credentials http-request
+                        :content-sha256-header? true))
 
 (defmethod sign-http-request "s3v4"
   [service endpoint credentials http-request]
-  (v4-sign-http-request service endpoint credentials http-request :content-sha256-header? true))
+  (v4-sign-http-request service endpoint credentials http-request
+                        :content-sha256-header? true))
diff --git a/src/cognitect/aws/util.clj b/src/cognitect/aws/util.clj
@@ -184,7 +184,9 @@
   [^String s]
   (-> s
       (URLEncoder/encode "UTF-8")
-      (.replace "+" "%20")))
+      ;; https://github.com/aws/aws-sdk-java/blob/fd409de/aws-java-sdk-core/src/main/java/com/amazonaws/util/SdkHttpUtils.java#L77-L91
+      (.replace "+" "%20")
+      (.replace "*" "%2A")))
 
 (defn query-string
   "Create a query string from a list of parameters. Values must all be

diff --git a/test/src/cognitect/aws/generators.clj b/test/src/cognitect/aws/generators.clj
@@ -0,0 +1,42 @@
+(ns cognitect.aws.generators
+  (:require [clojure.string :as str]
+            [clojure.test.check.generators :as gen]
+            [cognitect.aws.util :as util]))
+
+;; see cognitect.aws.protocols.rest/serialize-uri
+;; we want to mimic inputs to cognitect.aws.signers/sign-http-request
+(defn ^:private serialize-path-part
+  [part]
+  (-> part
+      (util/url-encode)
+      (str/replace "%2F" "/")
+      (str/replace "%7E" "~")))
+
+(defn gen-service
+  [sig-ver]
+  (gen/let [service-name (gen/such-that seq gen/string-alphanumeric)]
+    {:metadata {:signatureVersion sig-ver
+                :signingName service-name}}))
+
+(def gen-request
+  (gen/let [host (gen/fmap #(str % ".com") (gen/such-that seq gen/string-alphanumeric))
+            path-parts (gen/vector (gen/fmap serialize-path-part (gen/such-that (complement str/blank?) gen/string-ascii)) 1 10)
+            path-separator (gen/elements ["/" "//"])
+            query-ks (gen/vector (gen/such-that seq gen/string-alphanumeric))
+            query-vs (gen/vector (gen/such-that seq gen/string-alphanumeric) (count query-ks))
+            method (gen/elements [:get :post])
+            ;; https://github.com/aws/aws-sdk-java/blob/d35b018/aws-java-sdk-core/src/main/java/com/amazonaws/auth/internal/SignerKey.java#L30-L34
+            ;; date must be >1 day past epoch and <= today
+            ;; 1668999574880 == 2022-11-20
+            epoch (gen/large-integer* {:min 86400000 :max 1668999574880})
+            body gen/string]
+    {:request-method method
+     :body           (.getBytes ^String body "UTF-8")
+     :headers        {"x-amz-date" (util/format-date util/x-amz-date-format (java.util.Date. epoch))
+                      "host"       host}
+     :uri            (str path-separator
+                          (str/join path-separator path-parts)
+                          (when (seq query-ks)
+                            (str "?" (str/join "&" (map #(str %1 "=" %2)
+                                                        query-ks
+                                                        query-vs)))))}))
diff --git a/test/src/cognitect/aws/jdk.clj b/test/src/cognitect/aws/jdk.clj
@@ -0,0 +1,60 @@
+(ns cognitect.aws.jdk
+  (:require [clojure.java.io :as io]
+            [clojure.string :as str]
+            [cognitect.aws.util :as util])
+  (:import [java.net URI]
+           [com.amazonaws DefaultRequest]
+           [com.amazonaws.auth AWS4Signer]
+           [com.amazonaws.auth BasicAWSCredentials]
+           [com.amazonaws.http HttpMethodName]
+           [com.amazonaws.services.s3.internal AWSS3V4Signer]
+           [com.amazonaws.services.s3.request S3HandlerContextKeys]))
+
+(defn ^:private ->http-method
+  [request]
+  (HttpMethodName/fromValue (name (:request-method request))))
+
+(defn ^:private ->x-amz-date
+  [request]
+  (->> (get-in request [:headers "x-amz-date"])
+       (util/parse-date util/x-amz-date-format)))
+
+(defn ^:private ->basic-credentials
+  [credentials]
+  (BasicAWSCredentials. (:aws/access-key-id credentials)
+                        (:aws/secret-access-key credentials)))
+
+(defn ^:private ->default-request
+  [service request]
+  (let [[path query] (str/split (:uri request) #"\?" 2)
+        req (doto (DefaultRequest. (get-in service [:metadata :signingName]))
+              (.setHttpMethod (->http-method request))
+              (.setContent (io/input-stream (:body request)))
+              (.setEndpoint (URI. (str "https://" (get-in request [:headers "host"]))))
+              (.setResourcePath (str/replace path #"^//+" "/")))]
+    (when (seq query)
+      (doseq [[k v] (map #(str/split % #"=") (str/split query #"&"))]
+        (.addParameter req k v)))
+    req))
+
+(defn v4-jdk-signed-request
+  [service credentials request]
+  (let [basic-credentials (->basic-credentials credentials)
+        req               (->default-request service request)
+        signer            (doto (AWS4Signer.)
+                            (.setServiceName (get-in service [:metadata :signingName]))
+                            (.setOverrideDate (->x-amz-date request)))]
+    (.sign signer req basic-credentials)
+    req))
+
+(defn s3v4-jdk-signed-request
+  [service credentials request]
+  (let [basic-credentials (->basic-credentials credentials)
+        req               (doto (->default-request service request)
+                            (.addHandlerContext S3HandlerContextKeys/IS_PAYLOAD_SIGNING_ENABLED true)
+                            (.addHandlerContext S3HandlerContextKeys/IS_CHUNKED_ENCODING_DISABLED true))
+        signer            (doto (AWSS3V4Signer.)
+                            (.setServiceName (get-in service [:metadata :signingName]))
+                            (.setOverrideDate (->x-amz-date request)))]
+    (.sign signer req basic-credentials)
+    req))
diff --git a/test/src/cognitect/aws/signers_test.clj b/test/src/cognitect/aws/signers_test.clj
@@ -8,6 +8,10 @@
             [clojure.spec.test.alpha :as stest]
             [clojure.string :as str]
             [clojure.test :as t :refer [deftest is testing]]
+            [clojure.test.check.clojure-test :refer [defspec]]
+            [clojure.test.check.properties :as prop]
+            [cognitect.aws.jdk :as jdk]
+            [cognitect.aws.generators :as g]
             [cognitect.aws.signers :as signers])
   (:import [java.io ByteArrayInputStream]
            [org.apache.commons.io.input BOMInputStream]))
@@ -142,9 +146,35 @@
       (is (true? (-> res :clojure.spec.test.check/ret :result))
           res))))
 
+(defspec aws-v4-signer-parity 1000
+  (prop/for-all [service (g/gen-service "v4")
+                 request g/gen-request]
+    (let [signed-request (signers/sign-http-request service {:region "us-east-1"} credentials request)
+          jdk-signed-request (jdk/v4-jdk-signed-request service credentials request)]
+      (is (= (get-in signed-request [:headers "authorization"])
+             (get (.getHeaders jdk-signed-request) "Authorization"))))))
+
+(defspec aws-s3v4-signer-parity 1000
+  (prop/for-all [service (g/gen-service "s3v4")
+                 request g/gen-request]
+    (let [signed-request (signers/sign-http-request service {:region "us-east-1"} credentials request)
+          jdk-signed-request (jdk/s3v4-jdk-signed-request service credentials request)]
+      (is (= (get-in signed-request [:headers "authorization"])
+             (get (.getHeaders jdk-signed-request) "Authorization"))))))
+
+(defspec aws-s3-signer-parity 1000
+  (prop/for-all [service (g/gen-service "s3")
+                 request g/gen-request]
+    (let [signed-request (signers/sign-http-request service {:region "us-east-1"} credentials request)
+          ;; https://docs.aws.amazon.com/AmazonS3/latest/userguide/UsingAWSSDK.html
+          ;; If you are sending direct REST calls to Amazon S3, you must modify your application to use the Signature Version 4 signing process.
+          jdk-signed-request (jdk/s3v4-jdk-signed-request service credentials request)]
+      (is (= (get-in signed-request [:headers "authorization"])
+             (get (.getHeaders jdk-signed-request) "Authorization"))))))
+
 (comment
   (t/run-tests)
 
   (sub-directories (io/file (io/resource "aws-sig-v4-test-suite")))
 
-  (read-tests (io/file (io/resource "aws-sig-v4-test-suite"))))
+  (read-tests (io/file (io/resource "aws-sig-v4-test-suite"))))