Skip to content

feat(auth): implement SigV4 authentication for REST catalog#616

Merged
wgtmac merged 23 commits into
apache:mainfrom
plusplusjiajia:sigv4
Jun 14, 2026
Merged

feat(auth): implement SigV4 authentication for REST catalog#616
wgtmac merged 23 commits into
apache:mainfrom
plusplusjiajia:sigv4

Conversation

@plusplusjiajia

Copy link
Copy Markdown
Member

Implement AWS SigV4 authentication for the REST catalog client, following Java's RESTSigV4AuthManager and RESTSigV4AuthSession.

  • Extend AuthSession::Authenticate() with HTTPRequestContext (method, url, body) for SigV4 request signing
  • Add SigV4AuthSession: delegate-first auth → relocate conflicting Authorization header → sign with AWS SDK
  • Add SigV4AuthManager: wraps delegate AuthManager (default OAuth2), resolves credentials from properties or default chain
  • Body hash matches Java's SignerChecksumParams output: empty body → hex EMPTY_BODY_SHA256; non-empty body → Base64(SHA256(body))

@plusplusjiajia plusplusjiajia force-pushed the sigv4 branch 9 times, most recently from d1c0732 to 4326282 Compare April 11, 2026 11:01
Comment thread src/iceberg/catalog/rest/auth/auth_managers.cc Outdated
Comment thread .github/workflows/cpp-linter.yml Outdated
Comment thread src/iceberg/catalog/rest/auth/sigv4_auth_manager.cc Outdated
Comment thread src/iceberg/catalog/rest/auth/sigv4_auth_manager.cc Outdated
Comment thread src/iceberg/catalog/rest/auth/sigv4_auth_manager.cc Outdated
Comment thread src/iceberg/catalog/rest/auth/sigv4_auth_manager.cc Outdated
Comment thread src/iceberg/catalog/rest/auth/sigv4_auth_manager.h Outdated
Comment thread src/iceberg/test/auth_manager_test.cc Outdated
Comment thread src/iceberg/catalog/rest/auth/auth_session.h Outdated
Comment thread src/iceberg/catalog/rest/auth/auth_properties.h
Comment thread src/iceberg/catalog/rest/auth/auth_properties.h Outdated
Comment thread src/iceberg/catalog/rest/auth/auth_properties.h Outdated
Comment thread src/iceberg/catalog/rest/auth/auth_managers.cc Outdated
Comment thread src/iceberg/catalog/rest/auth/auth_managers.cc Outdated
Comment thread src/iceberg/catalog/rest/auth/sigv4_auth_manager.cc Outdated
@plusplusjiajia plusplusjiajia force-pushed the sigv4 branch 2 times, most recently from 8f61d0e to 655be23 Compare April 15, 2026 04:23
Comment thread src/iceberg/catalog/rest/auth/sigv4_auth_manager.cc Outdated
Comment thread src/iceberg/catalog/rest/auth/sigv4_auth_manager.cc Outdated
Comment thread src/iceberg/catalog/rest/auth/sigv4_auth_manager.cc Outdated
Comment thread src/iceberg/catalog/rest/http_client.cc Outdated

@wgtmac wgtmac left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for adding this! I have just completed the architectural review and didn't fully review the sigv4 manager yet. I have some preliminary questions here:

  • Should we also be compatible to the legacy rest.sigv4-enabled=true config (and others) when creating a auth manager?
  • How is this tested e2e? Any chance to have an integration test?

Comment thread cmake_modules/IcebergThirdpartyToolchain.cmake Outdated
Comment thread src/iceberg/catalog/rest/auth/auth_manager_internal.h
Comment thread src/iceberg/catalog/rest/auth/auth_managers.cc Outdated
Comment thread .github/workflows/cpp-linter.yml Outdated
Comment thread CMakeLists.txt Outdated
Comment thread src/iceberg/catalog/rest/http_client.cc
Comment thread src/iceberg/catalog/rest/auth/sigv4_auth_manager_internal.h Outdated
Comment thread src/iceberg/catalog/rest/auth/sigv4_auth_manager.cc Outdated
Comment thread src/iceberg/test/sigv4_auth_test.cc Outdated
Comment thread src/iceberg/catalog/rest/auth/sigv4_auth_manager.cc Outdated
@plusplusjiajia

Copy link
Copy Markdown
Member Author

Thanks for adding this! I have just completed the architectural review and didn't fully review the sigv4 manager yet. I have some preliminary questions here:

  • Should we also be compatible to the legacy rest.sigv4-enabled=true config (and others) when creating a auth manager?
  • How is this tested e2e? Any chance to have an integration test?

@wgtmac Thanks for taking a look!
Legacy rest.sigv4-enabled: Java does carry a deprecation alias here. I'd rather not pull that over into a fresh client — iceberg-cpp has no historical users to preserve, and inheriting a property name Java is already trying to retire feels like the wrong direction. Happy to revisit if there's a concrete cross-runtime config-sharing case.
E2E: Java's TestRESTSigV4AuthSession is also header-only — it checks the headers the signer produces but never sends a real HTTP request. The first 12 of our 21 cases are direct ports, so we're at parity with Java's actual coverage.

@plusplusjiajia plusplusjiajia force-pushed the sigv4 branch 4 times, most recently from cb4387f to 6fda557 Compare May 17, 2026 02:35
Comment thread src/iceberg/catalog/rest/rest_catalog.cc Outdated
@wgtmac

wgtmac commented Jun 13, 2026

Copy link
Copy Markdown
Member

Thanks for the update! I'll go through it again and perhaps polish some design on my end. Have you ever tested it against a real AWS environment? How can we ensure it works and won't break in the future?

Simplify AWS SDK wiring, remove premature session-cache plumbing, tighten credential validation, and strengthen SigV4 tests.

@wgtmac wgtmac left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for working on this PR, @plusplusjiajia!

I have pushed a commit for cleanup. I think this PR still has some design issues, especially around REST catalog session and table-level auth. I have to admit that current REST catalog design has some obvious drawbacks which make it hard to support contextual sessions. So let’s keep this PR focused for now. I may follow up with a separate PR to address the design issues around REST catalog.

@wgtmac wgtmac merged commit 577c8e4 into apache:main Jun 14, 2026
21 checks passed
@plusplusjiajia

Copy link
Copy Markdown
Member Author

Thanks @wgtmac for the thorough review and the polish pass, and everyone else for the feedback along the way — much appreciated!

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.

6 participants