Merge pull request #218 from scality/fix/gcs-presign-colon-encoding

Fix GCS presigned URL signature for multipart part uploads

Author: matthiasL-scality

lua/compute_aws_s3_signature.lua

@@ -344,7 +344,10 @@ elseif signature_mode == "PRESIGN_PUT" then
 
   local expires = ngx.time() + 3600
   local aws_secret_key = os.getenv('AWS_SECRET_ACCESS_KEY')
-  local canonicalized_resource = "/" .. ngx.var.aws_tgt_bucket .. "/" .. ngx.var.encoded_key
+  -- GCS normalises ':' to '%3A' when computing StringToSign for presigned URLs;
+  -- encode it here so the signature matches what GCS will verify.
+  local url_safe_key = ngx.var.encoded_key:gsub(':', '%%3A')
+  local canonicalized_resource = "/" .. ngx.var.aws_tgt_bucket .. "/" .. url_safe_key
   -- Sign as PUT with no Content-MD5 and no Content-Type (presigned, not proxied).
   local string_to_sign = "PUT\n\n\n" .. expires .. "\n" .. canonicalized_resource
   local aws_signature = ngx.encode_base64(ngx.hmac_sha1(aws_secret_key, string_to_sign))
@@ -373,17 +376,26 @@ elseif signature_mode == "PRESIGN_PART" then
   end
   local expires     = ngx.time() + 3600
   local aws_secret_key = os.getenv('AWS_SECRET_ACCESS_KEY')
-  -- GCS V2 requires URL-encoded values in the canonical resource (unlike AWS V2 which uses raw values).
-  -- See: https://cloud.google.com/storage/docs/access-control/signed-urls-v2
-  -- ngx.var.arg_* may return a raw (already percent-encoded) or decoded value depending on the nginx
-  -- version. Normalize by unescaping first, then re-escaping to avoid double-encoding (%2B → %252B).
+  -- GCS normalises ':' to '%3A' when computing StringToSign for presigned URLs;
+  -- encode it here so the signature matches what GCS will verify.
+  local url_safe_key = ngx.var.encoded_key:gsub(':', '%%3A')
+  -- Normalise uploadId: ngx.var.arg_* may be pre-encoded or raw depending on the nginx
+  -- version; unescape then re-escape to avoid double-encoding (%2B → %252B).
   local escaped_upload_id = ngx.escape_uri(ngx.unescape_uri(upload_id))
-  -- subresources must appear in canonical resource (alphabetical: partNumber < uploadId)
-  local canonicalized_resource = "/" .. ngx.var.aws_tgt_bucket .. "/" .. ngx.var.encoded_key ..
-    "?partNumber=" .. part_number .. "&uploadId=" .. escaped_upload_id
+  -- GCS does NOT include ?partNumber=N&uploadId=X in the canonical resource for presigned
+  -- part PUTs. Standard AWS S3-compatible backends (cloudserver, Scaleway S3) do include
+  -- them per the V2 spec. Detect GCS from ENDPOINT_URL to pick the right behaviour.
+  local endpoint_url = os.getenv('ENDPOINT_URL') or ''
+  local canonicalized_resource
+  if endpoint_url:find('googleapis', 1, true) then
+    canonicalized_resource = "/" .. ngx.var.aws_tgt_bucket .. "/" .. url_safe_key
+  else
+    canonicalized_resource = "/" .. ngx.var.aws_tgt_bucket .. "/" .. url_safe_key ..
+      "?partNumber=" .. part_number .. "&uploadId=" .. escaped_upload_id
+  end
   local string_to_sign = "PUT\n\n\n" .. expires .. "\n" .. canonicalized_resource
   local aws_signature = ngx.encode_base64(ngx.hmac_sha1(aws_secret_key, string_to_sign))
-  local presigned_url = ngx.var.redirect_endpoint .. "/" .. ngx.var.aws_tgt_bucket .. "/" .. ngx.var.encoded_key ..
+  local presigned_url = ngx.var.redirect_endpoint .. "/" .. ngx.var.aws_tgt_bucket .. "/" .. url_safe_key ..
     "?partNumber=" .. part_number ..
     "&uploadId=" .. escaped_upload_id ..
     "&AWSAccessKeyId=" .. ngx.escape_uri(ngx.var.aws_access_key) ..

tests/end2end/test_presign_upload.py

@@ -8,6 +8,7 @@
 """
 
 import hashlib
+from urllib.parse import urlparse
 
 import requests
 
@@ -55,6 +56,17 @@ def test_presign_upload_file_reachable_after_direct_put(session, artifacts_url):
     assert dl.content == data
 
 
+def test_presign_upload_encodes_colons_in_url(session, artifacts_url):
+    """Presigned URL path must use %3A for ':' — GCS normalises ':' to '%3A'
+    when computing StringToSign, so a literal ':' causes SignatureDoesNotMatch."""
+    resp = session.get(f'{artifacts_url}/presign-upload/{STAGING_BUILD}/colon-check.txt')
+    assert resp.status_code == 200
+    url = resp.text.strip()
+    path = urlparse(url).path
+    assert '%3A' in path, f"Expected '%3A' in presigned URL path, got: {path!r}"
+    assert ':' not in path, f"Raw ':' in presigned URL path causes SignatureDoesNotMatch on GCS"
+
+
 def test_presign_upload_rejects_non_get(session, artifacts_url):
     """Only GET is allowed on /presign-upload/; other methods return 400."""
     url = f'{artifacts_url}/presign-upload/{STAGING_BUILD}/file.txt'
@@ -94,24 +106,39 @@ def test_presign_upload_part_returns_url(session, artifacts_url):
 
 def test_presign_upload_part_encodes_special_chars_in_upload_id(session, artifacts_url):
     """uploadId containing +, / and = (GCS-style base64) is percent-encoded in the presigned URL."""
-    # Use a synthetic uploadId with base64 special characters. The PRESIGN_PART
-    # endpoint does not validate that the uploadId corresponds to a real upload,
-    # so we can inject one directly to verify the encoding behaviour without
-    # needing a backend that generates such IDs.
     special_upload_id = 'abc+def/ghi=jkl'
     resp = session.get(
         f'{artifacts_url}/presign-upload-part/{STAGING_BUILD}/presign/encoding-test.bin',
         params={'partNumber': 1, 'uploadId': special_upload_id},
     )
     assert resp.status_code == 200, f'{resp.status_code} {resp.text}'
     url = resp.text.strip()
-    # The uploadId must appear percent-encoded in the presigned URL so that GCS
-    # can reconstruct the canonical resource from the URL literally (GCS V2 spec).
     assert 'uploadId=abc%2Bdef%2Fghi%3Djkl' in url, (
         f'Expected uploadId to be percent-encoded in presigned URL, got: {url!r}'
     )
 
 
+def test_presign_upload_part_encodes_colons_in_url(session, artifacts_url):
+    """Presigned part URL path must use %3A for ':' — GCS normalises ':' to '%3A'
+    when computing StringToSign, so a literal ':' causes SignatureDoesNotMatch."""
+    upload_id = multipart_initiate(session, artifacts_url, STAGING_BUILD, 'presign/colon-part.bin')
+    try:
+        resp = session.get(
+            f'{artifacts_url}/presign-upload-part/{STAGING_BUILD}/presign/colon-part.bin',
+            params={'partNumber': 1, 'uploadId': upload_id},
+        )
+        assert resp.status_code == 200, f'{resp.status_code} {resp.text}'
+        url = resp.text.strip()
+        path = urlparse(url).path
+        assert '%3A' in path, f"Expected '%3A' in presigned part URL path, got: {path!r}"
+        assert ':' not in path, f"Raw ':' in presigned part URL path causes SignatureDoesNotMatch on GCS"
+    finally:
+        session.delete(
+            f'{artifacts_url}/upload-multipart/abort/{STAGING_BUILD}/presign/colon-part.bin',
+            params={'uploadId': upload_id},
+        )
+
+
 def test_presign_multipart_full_round_trip(session, artifacts_url):
     """Initiate via nginx, upload parts directly to S3, complete via nginx."""
     build = STAGING_BUILD