aws_sigv4: merge repeated headers in canonical request

[austinmoore-]

When multiple headers share the same name, AWS SigV4 expects them to be merged into a single header line in the canonical request, with values comma-delimited in the order they appeared.

This PR fixes the issue by combining repeated headers while creating the canonical request.

The fix has been verified with a few real S3 requests, along with the added libcurl test.

---

Detailed example:

curl -X PUT --verbose --silent \
    --aws-sigv4 "aws:amz:us-east-1:s3" --user "$AWS_ACCESS_KEY_ID:$AWS_SECRET_ACCESS_KEY" \
    -H "x-amz-meta-test: test2" \
    -H "x-amz-meta-test: test1" \
    --data-binary @testfile \
    "https://examplebucket.s3.amazonaws.com/testfile"

The canonical request that curl generates looks like:

PUT
/1B

host:examplebucket.s3.amazonaws.com
x-amz-content-sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855
x-amz-date:20250317T054922Z
x-amz-meta-test:test2
x-amz-meta-test:test1

host;x-amz-content-sha256;x-amz-date;x-amz-meta-test
e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855

AWS will respond with a SignatureDoesNotMatch error because from the above request, it will generate the following as the canonical request:

PUT
/1B

host:examplebucket.s3.amazonaws.com
x-amz-content-sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855
x-amz-date:20250317T054922Z
x-amz-meta-test:test2,test1

host;x-amz-content-sha256;x-amz-date;x-amz-meta-test
e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855

[testclutch]

Analysis of PR #16743 at bb50f7c7:

Test ../../tests/http/test_07_upload.py::TestUpload::test_07_42a_upload_disconnect[h2] failed, which has NOT been flaky recently, so there could be a real issue in this PR.

Test 1978 failed, which has NOT been flaky recently, so there could be a real issue in this PR.

Generated by Testclutch

[vszakats]

Analysis of PR #16743 at bb50f7c7:

Test ../../tests/http/test_07_upload.py::TestUpload::test_07_42a_upload_disconnect[h2] failed, which has NOT been flaky recently, so there could be a real issue in this PR.

Test 1978 failed, which has NOT been flaky recently, so there could be a real issue in this PR.

Generated by Testclutch

This seems legit:

test 1978...[HTTP AWS_SIGV4 canonical request duplicate header test]
 143 functions found, but only fail 25 (17.48%)
** MEMORY FAILURE
Leak detected: memory still allocated: 39 bytes
At 55a2208a0328, there's 16 bytes.
 allocated by /home/runner/work/curl/curl/lib/slist.c:68
At 55a2208a02f8, there's 23 bytes.
 allocated by /home/runner/work/curl/curl/lib/slist.c:94
LIMIT /home/runner/work/curl/curl/lib/slist.c:94 strdup reached memlimit
 1978: torture FAILED: function number 11 in test.
 invoke with "-t11" to repeat this single case.

https://github.com/curl/curl/actions/runs/13892739184/job/38867352025?pr=16743#step:41:4062

[austinmoore-]

Ack, 1978 is the test that I added for this. I won't be able to look at it today, but I'll take a look to see what I missed freeing soon.

[austinmoore-]

I was incorrectly creating the header list in the test, which has been addressed in the latest commit.

I'm still unsure about the test_07_42a_upload_disconnect[h2] failure. From what I can tell, there is no aws_sigv4 functionality in that test. I'm not super familiar with curl's source, but the changes in this PR are isolated to aws_sigv4 functionality, and AFAIK shouldn't have any direct affect on HTTP2 functionality.

[austinmoore-]

The job runner for Windows / mingw, CM clang-x86_64 gnutls (pull_request) exceeded the maximum execution time.

---

Also, I'm squashing the fix commit.

[bagder]

Lovely take and bonus points for the awesome test case! Just some minor remarks from me.

[bagder]

If colon_next is NULL here, this goes wrong.

[austinmoore-]

I can't find a way that we'd get a header without a colon in here. By this point, all the headers should have a :. I've added that to the function doc so that it's easier for folks to realize if the DEBUGASSERT fails while making a change, what the function is expecting and that they may need to update it.

However, if you'd prefer that it be changed to something like

if(!colon_next)
    return /* some CURLcode */

I'm happy to change it to that. Just lmk if you'd prefer that and which error code is best here (maybe CURLE_BAD_FUNCTION_ARGUMENT?)

[austinmoore-]

Updated to use dynbuf in place of malloc/strcpy. I've also added a more headers to the test case.

Regarding the test case, let me know if its preferred they be split out. I went with the thinking of "test case for this fix" instead of "test case for failure situations" but I'm happy to split it if the latter is preferred.

[bagder]

Thanks!