diff --git a/lua/compute_aws_s3_signature.lua b/lua/compute_aws_s3_signature.lua index d4603e3..c5b795d 100644 --- a/lua/compute_aws_s3_signature.lua +++ b/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) .. diff --git a/tests/end2end/test_presign_upload.py b/tests/end2end/test_presign_upload.py index 242c066..27b7b3f 100644 --- a/tests/end2end/test_presign_upload.py +++ b/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,10 +106,6 @@ 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', @@ -105,13 +113,32 @@ def test_presign_upload_part_encodes_special_chars_in_upload_id(session, artifac ) 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