feat(auth): implement SigV4 authentication for REST catalog by plusplusjiajia · Pull Request #616 · apache/iceberg-cpp

plusplusjiajia · 2026-04-11T08:47:05Z

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))

evindj · 2026-04-14T02:01:48Z

+  ICEBERG_PRECHECK(delegate_type != AuthProperties::kAuthTypeSigV4,
+                   "Cannot delegate a SigV4 auth manager to another SigV4 auth manager");


add delegate_type in error message?

add delegate_type in error message?

Good idea, done.

evindj · 2026-04-14T02:22:30Z

      - name: Install dependencies
        shell: bash
-        run: sudo apt-get update && sudo apt-get install -y libcurl4-openssl-dev
+        run: sudo apt-get update && sudo apt-get install -y libcurl4-openssl-dev ninja-build


n00b question why is ninja-build required here?

Good question — I added a build step in this PR so the linter can see the SigV4 code (needs compile_commands.json from a real build). I used cmake -G Ninja for speed and to be consistent with the other CI workflows, and Ninja is not preinstalled on ubuntu-24.04, hence the extra ninja-build package. Happy to switch to Make if you'd prefer.

If you see https://github.com/actions/runner-images/blob/main/images/ubuntu/Ubuntu2404-Readme.md, if will find Ninja is a pre-install.

Thanks @zhjwpku — you're right, Ninja is pre-installed on the ubuntu-24.04 runner. Dropped the ninja-build from the apt install step

evindj · 2026-04-14T02:27:31Z

+    if (session_token_it != properties.end() && !session_token_it->second.empty()) {
+      Aws::Auth::AWSCredentials credentials(access_key_it->second.c_str(),
+                                            secret_key_it->second.c_str(),
+                                            session_token_it->second.c_str());
+      return std::make_shared<Aws::Auth::SimpleAWSCredentialsProvider>(credentials);
+    }
+    Aws::Auth::AWSCredentials credentials(access_key_it->second.c_str(),
+                                          secret_key_it->second.c_str());
+    return std::make_shared<Aws::Auth::SimpleAWSCredentialsProvider>(credentials);


Nit: could do only one return if Credentials are created in the conditional statement.

Nit: could do only one return if Credentials are created in the conditional statement.

Nice catch, done.

evindj · 2026-04-14T02:34:46Z

+  auto it = properties.find(AuthProperties::kSigV4SigningName);
+  if (it != properties.end() && !it->second.empty()) {
+    return it->second;
+  }


if(properties.count(AuthProperties::kSigV4SigningName) > 0) { // do work }

might be a little less verbose than

auto it = properties.find(AuthProperties::kSigV4SigningName); if (it != properties.end() && !it->second.empty()) { return it->second; }

Thanks for the suggestion! I chose to keep the !it->second.empty() check on purpose — the intent is for an explicitly-empty value (e.g., a env var set to "") to also fall through to the legacy key / default.

evindj · 2026-04-14T02:47:39Z

+    const TableIdentifier& table,
+    const std::unordered_map<std::string, std::string>& properties,
+    std::shared_ptr<AuthSession> parent) {
+  auto* sigv4_parent = dynamic_cast<SigV4AuthSession*>(parent.get());


use checked_pointer_cast instead

use checked_pointer_cast instead

Done here as well.

evindj · 2026-04-14T02:48:16Z

+Result<std::shared_ptr<AuthSession>> SigV4AuthManager::ContextualSession(
+    const std::unordered_map<std::string, std::string>& context,
+    std::shared_ptr<AuthSession> parent) {
+  auto* sigv4_parent = dynamic_cast<SigV4AuthSession*>(parent.get());


use checked_pointer_cast

use checked_pointer_cast

Thanks, that's much better! Done.

evindj · 2026-04-14T02:51:39Z

+  std::string signing_region_;
+  std::string signing_name_;
+  std::shared_ptr<Aws::Auth::AWSCredentialsProvider> credentials_provider_;
+  /// Shared signer instance, matching Java's single Aws4Signer per manager.


I find this comment a bit confusing especially given that signer_ is a unique pointer that will be destroyed when SigV4AuthSession is destructed

You're right, sorry about the confusion — the "shared signer" wording was misleading since signer_ is owned per-session via unique_ptr. I've removed the comment.

evindj · 2026-04-14T02:54:50Z


  std::unordered_map<std::string, std::string> headers;
-  ASSERT_THAT(session_result.value()->Authenticate(headers), IsOk());
+  ASSERT_THAT(session_result.value()->Authenticate(headers, {}), IsOk());


it feels like Authenticate coudl accept a default value for the second parameter

Good point, thanks! Done

lishuxu · 2026-04-14T14:12:42Z

  ///         - IOError: Network or connection errors when reaching auth server
  ///         - RestError: HTTP errors from authentication service
-  virtual Status Authenticate(std::unordered_map<std::string, std::string>& headers) = 0;
+  virtual Status Authenticate(std::unordered_map<std::string, std::string>& headers,


The current design splits the request context into two separate parameters (headers as in-out + HTTPRequestContext as a separate struct).
The Java implementation uses a cleaner "request-in, request-out" pattern where authenticate() receives the full HTTPRequest and returns a new immutable request with auth headers, I'd suggest aligning with Java by introducing an HTTPRequest type that encapsulates method, url, headers, and body together, and changing the signature to:

virtual Result Authenticate(const HTTPRequest& request) = 0;

I'm open for this

Thanks @lishuxu ! Agreed — aligning with Java's request-in/request-out pattern is the right call. I'll address this in the current PR: introducing an HTTPRequest type (encapsulating method, url, headers, body), changing the signature to Result Authenticate(const HTTPRequest& request). Will push an update shortly — PTAL when it's ready.

lishuxu · 2026-04-14T14:35:38Z

+  // ---- SigV4 AWS credential entries ----
+
+  /// AWS region for SigV4 signing.
+  inline static const std::string kSigV4SigningRegion = "rest.signing-region";


We can remove the legacy key kSigV4Region/kSigV4Service

lishuxu · 2026-04-14T14:38:49Z

+  // ---- SigV4 AWS credential entries ----
+
+  /// AWS region for SigV4 signing.


The names are self-explanatory. I think we can remove them to keep the code concise.

lishuxu · 2026-04-14T14:40:50Z

  inline static const std::string kSigV4DelegateAuthType =
      "rest.auth.sigv4.delegate-auth-type";

+  // ---- SigV4 AWS credential entries ----


Nit: the // ---- SigV4 AWS credential entries ---- section header is redundant given the // ---- SigV4 entries ---- block above already covers SigV4 config. Merge them into a single section.

lishuxu · 2026-04-14T14:49:32Z

 }

+#ifdef ICEBERG_BUILD_SIGV4
+Result<std::unique_ptr<AuthManager>> MakeSigV4AuthManager(


MakeSigV4AuthManager is implemented directly in auth_managers.cc, while all other factory functions (MakeNoopAuthManager, MakeBasicAuthManager, MakeOAuth2Manager) are defined in their own translation units and only declared in auth_manager_internal.h. Suggest moving the implementation to sigv4_auth_manager.cc for consistency.

Good catch! Done.

lishuxu · 2026-04-14T15:01:22Z


 #include "iceberg/catalog/rest/auth/auth_manager_internal.h"
+#ifdef ICEBERG_BUILD_SIGV4
+#  include "iceberg/catalog/rest/auth/sigv4_auth_manager.h"


Nit: auth_properties.h should come before the #ifdef ICEBERG_BUILD_SIGV4 block to maintain alphabetical include order.

Thanks for catching it!

lishuxu · 2026-04-15T02:07:41Z

+  }
+
+  {
+    std::lock_guard<std::mutex> lock(signing_mutex_);


The mutex guards signer_->SignRequest() because AWSAuthV4Signer::SignRequest reportedly mutates internal signer state. However, signer_ is per-session (not shared across sessions), so the mutex only matters if the same SigV4AuthSession instance is called concurrently from multiple threads.

In contrast, Java's RESTSigV4AuthManager shares a single Aws4Signer across all sessions — if the Java signer were stateful, it would need synchronization there. It's worth confirming whether AWSAuthV4Signer::SignRequest actually mutates this or just uses local state — if the latter, the mutex can be removed entirely.

@lishuxu Good call — I checked the aws-sdk-cpp source (1.11.x, AWSAuthV4Signer) and you're right: for the symmetric SigV4 path we use, SignRequest does not mutate this, so the mutex is unnecessary. I've dropped it.

lishuxu · 2026-04-15T06:33:45Z

+  return std::make_shared<Aws::Auth::DefaultAWSCredentialsProviderChain>();
+}
+
+std::string SigV4AuthManager::ResolveSigningRegion(


ResolveSigningRegion manually reads AWS_REGION / AWS_DEFAULT_REGION and falls back to "us-east-1". Java delegates to DefaultAwsRegionProviderChain which also covers ~/.aws/config, EC2/ECS instance metadata. The AWS C++ SDK has an equivalent Aws::Config::EC2InstanceProfileConfigLoader and Aws::Environment::GetEnv. Consider using the SDK's built-in region resolution instead of reimplementing a subset of it.

@lishuxu Good point.Switched to: return {Aws::Client::ClientConfiguration().region.c_str()};

lishuxu · 2026-04-15T06:37:10Z

+Status SigV4AuthManager::Close() { return delegate_->Close(); }
+
+Result<std::shared_ptr<Aws::Auth::AWSCredentialsProvider>>
+SigV4AuthManager::MakeCredentialsProvider(


Java's AwsProperties.restCredentialsProvider() supports loading a custom AwsCredentialsProvider via a class name property. C++ only supports static credentials and the default chain. This is a known gap — worth a // TODO comment for future extensibility.

lishuxu · 2026-04-15T06:39:27Z

+Result<std::shared_ptr<AuthSession>> SigV4AuthManager::ContextualSession(
+    const std::unordered_map<std::string, std::string>& context,
+    std::shared_ptr<AuthSession> parent) {
+  auto sigv4_parent = internal::checked_pointer_cast<SigV4AuthSession>(std::move(parent));


checked_pointer_cast in ContextualSession/TableSession compiles to static_pointer_cast in Release builds — a wrong type silently causes UB.

You're right. Updated.

lishuxu · 2026-04-15T06:50:24Z

+    if (!first) url += "&";
+    auto ek = EncodeString(k);
+    auto ev = EncodeString(v);
+    url += (ek ? *ek : k) + "=" + (ev ? *ev : v);


AppendQueryString silently falls back to the raw key/value when EncodeString fails. If encoding fails, the URL passed to Authenticate would differ from what the server receives, causing signature verification to fail. Consider propagating the error instead:
ICEBERG_ASSIGN_OR_RAISE(auto ek, EncodeString(k));
ICEBERG_ASSIGN_OR_RAISE(auto ev, EncodeString(v));
url += ek + "=" + ev;

@lishuxu Good catch. Changed AppendQueryString to return Resultstd::string

wgtmac

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 · 2026-05-04T09:14:21Z

+
+function(resolve_aws_sdk_dependency)
+  find_package(AWSSDK REQUIRED COMPONENTS core)
+  list(APPEND ICEBERG_SYSTEM_DEPENDENCIES AWSSDK)


Here it records only AWSSDK for installed-package dependency discovery, while src/iceberg/catalog/rest/CMakeLists.txt exports aws-cpp-sdk-core in the REST install interface. The generated iceberg-config.cmake can only call find_dependency(AWSSDK) without COMPONENTS core, but AWS SDK’s CMake config loads component packages from AWSSDK_FIND_COMPONENTS. A downstream installed SigV4 build can therefore fail to find/link AWS core unless it happens to be on the default linker path.

I'd suggest to special-case find_dependency(AWSSDK COMPONENTS core) in the iceberg-config.cmake.in or otherwise export the AWS SDK dependency component-aware.

wgtmac · 2026-05-04T09:37:54Z


+#ifdef ICEBERG_BUILD_SIGV4
+/// \brief Create a SigV4 authentication manager with a delegate.
+Result<std::unique_ptr<AuthManager>> MakeSigV4AuthManager(


Where is the definition? BTW, we don't need to use macro ICEBERG_BUILD_SIGV4 everywhere. We can return Unsupported from MakeSigV4AuthManager function internally depending on this macro.

wgtmac · 2026-05-04T09:38:14Z

      {AuthProperties::kAuthTypeBasic, MakeBasicAuthManager},
      {AuthProperties::kAuthTypeOAuth2, MakeOAuth2Manager},
  };
+#ifdef ICEBERG_BUILD_SIGV4


ditto, we don't need to use this macro here.

wgtmac · 2026-05-04T09:49:14Z

          mkdir build && cd build
-          cmake .. -G Ninja -DCMAKE_EXPORT_COMPILE_COMMANDS=ON
+          cmake .. -G Ninja -DCMAKE_EXPORT_COMPILE_COMMANDS=ON \
+            -DICEBERG_BUILD_SIGV4=ON \


Why do we need changes in the file? Is it because unrecognized headers from sigv4_auth_manager.cc? Does disabling ICEBERG_BUILD_SIGV4 help in this case? I am thinking if we can add a dedicated ci workflow for aws-related stuff like S3 and SigV4

wgtmac · 2026-05-04T09:54:00Z

 option(ICEBERG_BUILD_BUNDLE "Build the battery included library" ON)
 option(ICEBERG_BUILD_REST "Build rest catalog client" ON)
 option(ICEBERG_BUILD_REST_INTEGRATION_TESTS "Build rest catalog integration tests" OFF)
+option(ICEBERG_BUILD_SIGV4 "Build SigV4 authentication support (requires AWS SDK)" OFF)


Please rebase on the latest main branch so we can see the option ICEBERG_S3. I think we should follow the same pattern to name it ICEBERG_SIGV4.

wgtmac · 2026-05-04T12:40:34Z

-    cpr_params.Add({key, val});
+  if (params.empty()) return base_url;
+  std::map<std::string, std::string> sorted(params.begin(), params.end());
+  std::string url = base_url + "?";


Here we assume that base_url will never contain & which is true for rest catalog use case but HttpClient is an exported class so it is worth adding a comment to avoid misuse.

wgtmac · 2026-05-04T12:53:19Z

+  SigV4AuthSession(
+      std::shared_ptr<AuthSession> delegate, std::string signing_region,
+      std::string signing_name,
+      std::shared_ptr<Aws::Auth::AWSCredentialsProvider> credentials_provider,


Generally it is not a good practice to expose AWS sdk internals like this. We can consider changing to sigv4_auth_manager_internal.h so we don't install it any more. This is also related to my other comment about installed headers.

wgtmac · 2026-05-04T12:55:11Z

+namespace {
+
+/// \brief Ensures AWS SDK is initialized exactly once per process.
+/// ShutdownAPI is intentionally never called (leak-by-design) to avoid


Is this the recommended approach?

wgtmac · 2026-05-04T12:59:13Z

+// ---------- Tests ported from Java TestRESTSigV4AuthSession ----------
+
+// Java: authenticateWithoutBody


Suggested change

// ---------- Tests ported from Java TestRESTSigV4AuthSession ----------

// Java: authenticateWithoutBody

Let's remove comments like this and below.

wgtmac · 2026-05-04T14:47:09Z

+      auto delegate_session,
+      delegate_->TableSession(table, properties, sigv4_parent->delegate()));
+
+  auto merged = MergeProperties(sigv4_parent->effective_properties(), properties);


Quick question: is it intentional that table sessions inherit the parent session's effective SigV4 properties, including contextual overrides?

This seems slightly different from Java's current RESTSigV4AuthManager, where tableSession merges table properties with catalogProperties. The C++ precedence of catalog < context < table looks reasonable to me, but if this is deliberate, could we document it or keep the test that makes this behavior explicit?

plusplusjiajia force-pushed the sigv4 branch 5 times, most recently from bfc98f7 to 9ddbdbd Compare April 11, 2026 09:35

feat(auth): implement SigV4 authentication for REST catalog

eaa433f

plusplusjiajia force-pushed the sigv4 branch 3 times, most recently from 915c87b to d1c0732 Compare April 11, 2026 11:00

fix(ci): enable SigV4 build in cpp-linter workflow

4326282

plusplusjiajia force-pushed the sigv4 branch from d1c0732 to 4326282 Compare April 11, 2026 11:01

evindj reviewed Apr 14, 2026

View reviewed changes

plusplusjiajia added 3 commits April 14, 2026 11:13

address review feedback

6e12fbd

add single-arg Authenticate() overload

eb74231

sigv4 x-amz-content-sha256 must be Base64 in canonical headers

6a9cd32

lishuxu reviewed Apr 14, 2026

View reviewed changes

adopt request-in/request-out Authenticate interface

abb2cf6

lishuxu reviewed Apr 14, 2026

View reviewed changes

plusplusjiajia added 2 commits April 14, 2026 23:02

move MakeSigV4AuthManager to sigv4_auth_manager.cc

4197ceb

Meson: wire SigV4 behind a feature option

f26927e

lishuxu reviewed Apr 15, 2026

View reviewed changes

plusplusjiajia force-pushed the sigv4 branch from 901f215 to 8f61d0e Compare April 15, 2026 04:17

drop unnecessary signing mutex

655be23

plusplusjiajia force-pushed the sigv4 branch from 8f61d0e to 655be23 Compare April 15, 2026 04:23

lishuxu reviewed Apr 15, 2026

View reviewed changes

plusplusjiajia added 2 commits April 15, 2026 15:25

address review feedback

39f4164

ci: drop redundant ninja-build install from cpp-linter

743a444

wgtmac reviewed May 4, 2026

View reviewed changes

		ICEBERG_PRECHECK(delegate_type != AuthProperties::kAuthTypeSigV4,
		"Cannot delegate a SigV4 auth manager to another SigV4 auth manager");

		// ---- SigV4 AWS credential entries ----

		/// AWS region for SigV4 signing.

		// ---------- Tests ported from Java TestRESTSigV4AuthSession ----------

		// Java: authenticateWithoutBody

Conversation

plusplusjiajia commented Apr 11, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment