Skip to content

fix: sigv4 auth to use base64-encoded content sha256 and custom canonical request#3120

Open
plusplusjiajia wants to merge 3 commits intoapache:mainfrom
plusplusjiajia:fix-sigv4-auth
Open

fix: sigv4 auth to use base64-encoded content sha256 and custom canonical request#3120
plusplusjiajia wants to merge 3 commits intoapache:mainfrom
plusplusjiajia:fix-sigv4-auth

Conversation

@plusplusjiajia
Copy link
Member

@plusplusjiajia plusplusjiajia commented Mar 4, 2026

Rationale for this change

This PR fixes the SigV4 request signing implementation in pyiceberg to align with the behavior of the [Iceberg Java SDK]
The SigV4 canonical request must use the hex-encoded SHA-256 of the payload for signature computation, while the x-amz-content-sha256 header uses base64 encoding. The default botocore.auth.SigV4Auth uses the x-amz-content-sha256 header value directly in the canonical request, which is incorrect when the header is base64-encoded.

Are these changes tested?

Yes

Are there any user-facing changes?

No

@plusplusjiajia plusplusjiajia changed the title Fix SigV4 auth to use base64-encoded content SHA256 and custom canonical request fix: sigv4 auth to use base64-encoded content sha256 and custom canonical request Mar 5, 2026
@JingsongLi
Copy link

Hi @kevinjqliu Can you take a look?

@kevinjqliu
Copy link
Contributor

thanks for the PR! i understand this is to align with the java SigV4 implementation. Could you help me understand the specific scenario in which this is currently breaking? (i dont know much about sigv4)

Copy link
Contributor

@kevinjqliu kevinjqliu left a comment

Choose a reason for hiding this comment

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

reading up on SigV4 java implementation

but some initial comments

def canonical_request(self, request: Any) -> str:
cr = super().canonical_request(request)
# Replace the last line (body_checksum) with hex-encoded payload hash.
return cr.rsplit("\n", 1)[0] + "\n" + self.payload(request)
Copy link
Contributor

Choose a reason for hiding this comment

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

using rsplit feels fragile, can we reuse the same logic from canonical_request but just replace the last part with self.payload(request)

Copy link
Contributor

Choose a reason for hiding this comment

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

im confused why the _IcebergSigV4Auth class is needed

Copy link
Member Author

Choose a reason for hiding this comment

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

using rsplit feels fragile, can we reuse the same logic from canonical_request but just replace the last part with self.payload(request)

Good point! Updated to reuse the logic from botocore's implement instead of using rsplit. The override now explicitly builds the canonical request and always uses self.payload(request) (hex-encoded) for the body checksum, regardless of the x-amz-content-sha256 header value.

Copy link
Member Author

Choose a reason for hiding this comment

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

im confused why the _IcebergSigV4Auth class is needed

The _IcebergSigV4Auth class is needed because botocore's SigV4Auth.canonical_request uses the X-Amz-Content-SHA256 header value directly as the body checksum when present. Since we set this header to base64, we need to override canonical_request to always use hex-encoded self.payload(request) instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

ideally we should test against an integration test before merging. unit tests can only do so much

Copy link
Member Author

Choose a reason for hiding this comment

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

ideally we should test against an integration test before merging. unit tests can only do so much

Agreed, integration tests would definitely give more confidence here. Currently there are no SigV4-related integration tests in the project — the existing integration tests under tests/integration/ use a docker-compose based REST catalog without SigV4 enabled. The Java implementation also tests SigV4 with rather than integration tests against a real service.
One challenge with adding SigV4 integration tests to CI is that it would require real AWS credentials (access key / secret key), which might not be straightforward to set up in the open-source CI environment. On my end, I've been able to verify this change against a REST catalog server built with the Iceberg Java SDK. I'd really appreciate any suggestions you might have on how best to approach this.

@kevinjqliu
Copy link
Contributor

Ok so from what ive gathered

SigV4 is AWS's request signing mechanism. We're addressing the scenario where both SigV4 and OAuth are required for REST catalog authentication.
The REST catalog requires an OAuth token, and AWS requires the request to also be SigV4-signed. The request is first authenticated by the delegate auth session, which stamps the OAuth bearer token into the Authorization header. That header is then relocated to Original-Authorization before SigV4 signs the request — freeing up Authorization for the SigV4 signature while keeping the OAuth token present and covered by the HMAC.

Are you running your own IRC on AWS? i think glue IRC doesnt need the oauth part

Comment on lines +725 to +727
if request.body:
body_bytes = request.body.encode("utf-8") if isinstance(request.body, str) else request.body
content_sha256_header = base64.b64encode(hashlib.sha256(body_bytes).digest()).decode()
Copy link
Contributor

Choose a reason for hiding this comment

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

i dont think we need to base64 encode this. the java implementation doesnt do this. neither does the python reference implementation

Copy link
Member Author

@plusplusjiajia plusplusjiajia Mar 21, 2026

Choose a reason for hiding this comment

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

i dont think we need to base64 encode this. the java implementation doesnt do this. neither does the python reference implementation

Thanks for the review! Really appreciate you taking the time to look through this carefully.
However, I believe the Java implementation actually does use base64 for non-empty bodies. We can see TestRESTSigV4AuthSession.java#L174 : the expected value for x-amz-content-sha256 is "yc5oAKPWjHY4sW8XQq0l/3aNrrXJKBycVFNnDEGMfww=" (base64), not hex. This is because Java uses RESTSigV4AuthSession.java#L100-L104 which produces base64-encoded checksums. Only empty bodies use hex as a RESTSigV4AuthSession.java#L119-L121 (see also the TestRESTSigV4AuthSession.java#L118).

@kevinjqliu
Copy link
Contributor

@plusplusjiajia
Copy link
Member Author

thanks for the PR! i understand this is to align with the java SigV4 implementation. Could you help me understand the specific scenario in which this is currently breaking? (i dont know much about sigv4)

That's a great question — let me walk through the context.
The root cause is in how the Java Iceberg SDK computes the x-amz-content-sha256 header. It uses AWS SDK v2's SignerChecksumParams with Algorithm.SHA256 and sets the checksumHeaderName to X-Amz-Content-SHA256 . Internally, the AWS SDK's
applies BinaryUtils.toBase64() to the checksum before writing it into the specified header — this is part of the flexible checksum mechanism rather than standard SigV4 behavior.
So the base64 encoding in x-amz-content-sha256 is essentially a side effect of Java Iceberg leveraging the flexible checksum API. For empty bodies, the Java side already has a RESTSigV4AuthSession.java#L119-L121 to override this with the standard hex value, but for non-empty bodies, the base64 value is left as-is (confirmed by the TestRESTSigV4AuthSession.java#L174 ).
Since x-amz-content-sha256 is a signed header, its value participates in the canonical request construction. When a REST catalog server built with the Java Iceberg SDK verifies incoming signatures, it expects the same base64-encoded value. If the Python client sends a hex-encoded value instead, the canonical headers won't match during server-side signature verification, resulting in a signature mismatch.
This PR aligns the Python implementation with the Java SDK's current behavior to ensure interoperability. That said, I agree it would be worth discussing whether the Java side should also be updated to use standard hex encoding — but that would need to be a coordinated change across both implementations. Happy to hear your thoughts on this!

@plusplusjiajia
Copy link
Member Author

Ok so from what ive gathered

SigV4 is AWS's request signing mechanism. We're addressing the scenario where both SigV4 and OAuth are required for REST catalog authentication. The REST catalog requires an OAuth token, and AWS requires the request to also be SigV4-signed. The request is first authenticated by the delegate auth session, which stamps the OAuth bearer token into the Authorization header. That header is then relocated to Original-Authorization before SigV4 signs the request — freeing up Authorization for the SigV4 signature while keeping the OAuth token present and covered by the HMAC.

Are you running your own IRC on AWS? i think glue IRC doesnt need the oauth part

We're planning to set up our own REST catalog with SigV4 signing. We don't use OAuth — just SigV4 for request authentication. This PR ensures the Python client produces signatures compatible with the Java Iceberg SDK's SigV4 implementation on the server side.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants