Skip to content

http_aws_sigv4: add additional verbose log statements to sigv4 calculation #16952

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from

Conversation

nbaws
Copy link
Contributor

@nbaws nbaws commented Apr 3, 2025

To use curl as a tool for troubleshooting SigV4 signing, it is useful to have the 'Canonical Request', 'String To Sign' and 'Signature' calculations output. This trivial patch changes the behaviour from 'only logging one field via library debug' to 'logging in verbose mode for all 3 fields'.

@vszakats
Copy link
Member

vszakats commented Apr 3, 2025

Could this mean potentially logging secrets in verbose mode? I'm thinking this may have been a reason to make this a debug only output.

@testclutch
Copy link

Analysis of PR #16952 at 943c99f7:

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

Generated by Testclutch

@nbaws
Copy link
Contributor Author

nbaws commented Apr 3, 2025

There are 3 sensitive components input as part of an AWS SigV4 calculation:

  • Access Key ID
  • Secret Access Key
  • Session Token

In the header output that is already produced in verbose mode, the Access Key ID and Session Token (REDACTED) is already printed verbatim in the HTTP headers.

> Authorization: AWS4-HMAC-SHA256 Credential=ASIAQREDACTED20250403/ap-southeast-2/vpc-lattice-svcs/aws4_request, SignedHeaders=accept;host;x-amz-content-sha256;x-amz-date;x-amz-security-token, Signature=d01b12468873fa4be8e0379b981248fd17f2c6b071e7f5c83c1e4ee51552b2f7
> X-Amz-Date: 20250403T224011Z
> User-Agent: curl/8.13.1-DEV
> x-amz-security-token: IQo...REDACTED

The secret access key is never printed, nor is being printed in this patch. The signature is not sensitive, as it is a sha256 hash of the stringToSign calculation, which includes a sha256 hash of the payload, which is already printed by curl in verbose mode.

Even though I agree this content is sensitive, it is no more sensitive than is already being printed in the current verbose mode implementation. This patch also aligns with the behaviour of the AWS SDK and python3 boto libraries, which generate StringToSign and CanonicalRequest debug strings in the same format in verbose mode.

@vszakats
Copy link
Member

vszakats commented Apr 4, 2025

Ah, indeed, canonical_request already appeared as a header.

What woud you think of making the outputs single lines, e.g.:

 infof(data, "aws_sigv4: Canonical request: '%s'", canonical request);
[...]
 infof(data, "aws_sigv4: String to sign: '%s'", str_to_sign);

@nbaws
Copy link
Contributor Author

nbaws commented Apr 4, 2025

Ah, indeed, canonical_request already appeared as a header.

What woud you think of making the outputs single lines, e.g.:

 infof(data, "aws_sigv4: Canonical request: '%s'", canonical request);
[...]
 infof(data, "aws_sigv4: String to sign: '%s'", str_to_sign);

Makes sense to me - fixed :)

Just for your understanding, the rationale for enclosing in [] is because I was trying to remove the ambiguity when printing them. They contain cr/lf and this should hopefully make it very clear which is the string, and which is the debug print statement contents. When comparing them (such as when you are troubleshooting signing issues), being able to be accurate as to the exact string contents is really important.

Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good idea!

@bagder
Copy link
Member

bagder commented Apr 4, 2025

This might be helpful already for #16927

@bagder bagder closed this in c871dcb Apr 4, 2025
@bagder
Copy link
Member

bagder commented Apr 4, 2025

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants