wgtmac commented on code in PR #616:
URL: https://github.com/apache/iceberg-cpp/pull/616#discussion_r3180619427
##########
src/iceberg/catalog/rest/auth/auth_managers.cc:
##########
@@ -62,11 +62,15 @@ std::string InferAuthType(
}
AuthManagerRegistry CreateDefaultRegistry() {
- return {
+ AuthManagerRegistry registry = {
{AuthProperties::kAuthTypeNone, MakeNoopAuthManager},
{AuthProperties::kAuthTypeBasic, MakeBasicAuthManager},
{AuthProperties::kAuthTypeOAuth2, MakeOAuth2Manager},
};
+#ifdef ICEBERG_BUILD_SIGV4
Review Comment:
ditto, we don't need to use this macro here.
##########
cmake_modules/IcebergThirdpartyToolchain.cmake:
##########
@@ -531,3 +531,21 @@ endif()
if(ICEBERG_BUILD_REST)
resolve_cpr_dependency()
endif()
+
+# ----------------------------------------------------------------------
+# AWS SDK for C++
+
+function(resolve_aws_sdk_dependency)
+ find_package(AWSSDK REQUIRED COMPONENTS core)
+ list(APPEND ICEBERG_SYSTEM_DEPENDENCIES AWSSDK)
Review Comment:
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.
##########
src/iceberg/catalog/rest/auth/auth_manager_internal.h:
##########
@@ -47,4 +47,11 @@ Result<std::unique_ptr<AuthManager>> MakeOAuth2Manager(
std::string_view name,
const std::unordered_map<std::string, std::string>& properties);
+#ifdef ICEBERG_BUILD_SIGV4
+/// \brief Create a SigV4 authentication manager with a delegate.
+Result<std::unique_ptr<AuthManager>> MakeSigV4AuthManager(
Review Comment:
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.
##########
.github/workflows/cpp-linter.yml:
##########
@@ -38,13 +38,25 @@ jobs:
- name: Install dependencies
shell: bash
run: sudo apt-get update && sudo apt-get install -y
libcurl4-openssl-dev
+ - name: Cache vcpkg packages
+ uses: actions/cache@v4
+ id: vcpkg-cache
+ with:
+ path: /usr/local/share/vcpkg/installed
+ key: vcpkg-x64-linux-aws-sdk-cpp-core-${{
hashFiles('.github/workflows/cpp-linter.yml') }}
+ - name: Install AWS SDK via vcpkg
+ if: steps.vcpkg-cache.outputs.cache-hit != 'true'
+ shell: bash
+ run: vcpkg install aws-sdk-cpp[core]:x64-linux
- name: Run build
env:
CC: gcc-14
CXX: g++-14
run: |
mkdir build && cd build
- cmake .. -G Ninja -DCMAKE_EXPORT_COMPILE_COMMANDS=ON
+ cmake .. -G Ninja -DCMAKE_EXPORT_COMPILE_COMMANDS=ON \
+ -DICEBERG_BUILD_SIGV4=ON \
Review Comment:
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
##########
CMakeLists.txt:
##########
@@ -45,6 +45,7 @@ option(ICEBERG_BUILD_TESTS "Build tests" ON)
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)
Review Comment:
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`.
##########
src/iceberg/catalog/rest/auth/auth_session.h:
##########
@@ -23,6 +23,7 @@
#include <string>
#include <unordered_map>
+#include "iceberg/catalog/rest/endpoint.h"
Review Comment:
I would suggest moving `enum HttpMethod` and `struct HttpRequest` to a
dedicated `http_request.h` so we don't need to include endpoint.h here.
##########
src/iceberg/catalog/rest/auth/auth_session.h:
##########
@@ -32,25 +33,37 @@
namespace iceberg::rest::auth {
+/// \brief An outgoing HTTP request passed through an AuthSession. Mirrors the
+/// HTTPRequest type used by the Java reference implementation so signing
+/// implementations like SigV4 can operate on method, url, headers, and body
+/// as a single value.
+struct ICEBERG_REST_EXPORT HTTPRequest {
+ HttpMethod method = HttpMethod::kGet;
+ std::string url;
+ std::unordered_map<std::string, std::string> headers;
+ std::string body;
+};
+
/// \brief An authentication session that can authenticate outgoing HTTP
requests.
class ICEBERG_REST_EXPORT AuthSession {
public:
virtual ~AuthSession() = default;
- /// \brief Authenticate the given request headers.
+ /// \brief Authenticate an outgoing HTTP request.
///
- /// This method adds authentication information (e.g., Authorization header)
- /// to the provided headers map. The implementation should be idempotent.
+ /// Returns a new request with authentication information (e.g., an
+ /// Authorization header) added. Implementations must be idempotent and must
+ /// not mutate the input request.
///
- /// \param[in,out] headers The headers map to add authentication information
to.
- /// \return Status indicating success or one of the following errors:
+ /// \param request The request to authenticate.
+ /// \return The authenticated request on success, or one of:
/// - AuthenticationFailed: General authentication failure (invalid
credentials,
/// etc.)
/// - TokenExpired: Authentication token has expired and needs
refresh
/// - NotAuthorized: Not authenticated (401)
/// - 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 Result<HTTPRequest> Authenticate(const HTTPRequest& request) = 0;
Review Comment:
I agree that this change makes sense.
##########
src/iceberg/catalog/rest/http_client.cc:
##########
@@ -68,27 +70,37 @@ namespace {
/// \brief Default error type for unparseable REST responses.
constexpr std::string_view kRestExceptionType = "RESTException";
-/// \brief Prepare headers for an HTTP request.
-Result<cpr::Header> BuildHeaders(
- const std::unordered_map<std::string, std::string>& request_headers,
+/// \brief Merge default headers with per-request headers (per-request wins).
+std::unordered_map<std::string, std::string> MergeHeaders(
const std::unordered_map<std::string, std::string>& default_headers,
- auth::AuthSession& session) {
- std::unordered_map<std::string, std::string> headers(default_headers);
+ const std::unordered_map<std::string, std::string>& request_headers) {
+ std::unordered_map<std::string, std::string> merged(default_headers);
for (const auto& [key, val] : request_headers) {
- headers.insert_or_assign(key, val);
+ merged.insert_or_assign(key, val);
}
- ICEBERG_RETURN_UNEXPECTED(session.Authenticate(headers));
- return cpr::Header(headers.begin(), headers.end());
+ return merged;
+}
+
+cpr::Header ToCprHeader(const auth::HTTPRequest& request) {
+ return {request.headers.begin(), request.headers.end()};
}
-/// \brief Converts a map of string key-value pairs to cpr::Parameters.
-cpr::Parameters GetParameters(
+/// \brief Append URL-encoded query parameters to a URL, sorted by key.
+Result<std::string> AppendQueryString(
+ const std::string& base_url,
const std::unordered_map<std::string, std::string>& params) {
- cpr::Parameters cpr_params;
- for (const auto& [key, val] : params) {
- 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 + "?";
Review Comment:
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.
##########
src/iceberg/test/sigv4_auth_test.cc:
##########
@@ -0,0 +1,536 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include <string>
+#include <unordered_map>
+
+#include <aws/core/Aws.h>
+#include <aws/core/auth/AWSCredentialsProvider.h>
+#include <gtest/gtest.h>
+
+#include "iceberg/catalog/rest/auth/auth_managers.h"
+#include "iceberg/catalog/rest/auth/auth_properties.h"
+#include "iceberg/catalog/rest/auth/auth_session.h"
+#include "iceberg/catalog/rest/auth/sigv4_auth_manager.h"
+#include "iceberg/catalog/rest/http_client.h"
+#include "iceberg/table_identifier.h"
+#include "iceberg/test/matchers.h"
+
+namespace iceberg::rest::auth {
+
+class SigV4AuthTest : public ::testing::Test {
+ protected:
+ static void SetUpTestSuite() {
+ static bool initialized = false;
+ if (!initialized) {
+ Aws::SDKOptions options;
+ Aws::InitAPI(options);
+ initialized = true;
+ }
+ }
+
+ HttpClient client_{{}};
+
+ std::unordered_map<std::string, std::string> MakeSigV4Properties() {
+ return {
+ {AuthProperties::kAuthType, "sigv4"},
+ {AuthProperties::kSigV4SigningRegion, "us-east-1"},
+ {AuthProperties::kSigV4SigningName, "execute-api"},
+ {AuthProperties::kSigV4AccessKeyId, "AKIAIOSFODNN7EXAMPLE"},
+ {AuthProperties::kSigV4SecretAccessKey,
+ "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY"},
+ };
+ }
+};
+
+TEST_F(SigV4AuthTest, LoadSigV4AuthManager) {
+ auto properties = MakeSigV4Properties();
+ auto manager_result = AuthManagers::Load("test-catalog", properties);
+ ASSERT_THAT(manager_result, IsOk());
+}
+
+TEST_F(SigV4AuthTest, CatalogSessionProducesSession) {
+ auto properties = MakeSigV4Properties();
+ auto manager_result = AuthManagers::Load("test-catalog", properties);
+ ASSERT_THAT(manager_result, IsOk());
+
+ auto session_result = manager_result.value()->CatalogSession(client_,
properties);
+ ASSERT_THAT(session_result, IsOk());
+}
+
+TEST_F(SigV4AuthTest, AuthenticateAddsAuthorizationHeader) {
+ auto properties = MakeSigV4Properties();
+ auto manager_result = AuthManagers::Load("test-catalog", properties);
+ ASSERT_THAT(manager_result, IsOk());
+
+ auto session_result = manager_result.value()->CatalogSession(client_,
properties);
+ ASSERT_THAT(session_result, IsOk());
+
+ HTTPRequest request{.method = HttpMethod::kGet, .url =
"https://example.com/v1/config"};
+ auto auth_result = session_result.value()->Authenticate(request);
+ ASSERT_THAT(auth_result, IsOk());
+ const auto& headers = auth_result.value().headers;
+
+ EXPECT_NE(headers.find("authorization"), headers.end());
+ EXPECT_TRUE(headers.at("authorization").starts_with("AWS4-HMAC-SHA256"));
+ EXPECT_NE(headers.find("x-amz-date"), headers.end());
+}
+
+TEST_F(SigV4AuthTest, AuthenticateWithPostBody) {
+ auto properties = MakeSigV4Properties();
+ auto manager_result = AuthManagers::Load("test-catalog", properties);
+ ASSERT_THAT(manager_result, IsOk());
+
+ auto session_result = manager_result.value()->CatalogSession(client_,
properties);
+ ASSERT_THAT(session_result, IsOk());
+
+ HTTPRequest request{.method = HttpMethod::kPost,
+ .url = "https://example.com/v1/namespaces";,
+ .headers = {{"Content-Type", "application/json"}},
+ .body = R"({"namespace":["ns1"]})"};
+ auto auth_result = session_result.value()->Authenticate(request);
+ ASSERT_THAT(auth_result, IsOk());
+ const auto& headers = auth_result.value().headers;
+
+ EXPECT_NE(headers.find("authorization"), headers.end());
+ EXPECT_TRUE(headers.at("authorization").starts_with("AWS4-HMAC-SHA256"));
+}
+
+TEST_F(SigV4AuthTest, DelegateAuthorizationHeaderRelocated) {
+ auto properties = MakeSigV4Properties();
+ properties[AuthProperties::kToken.key()] = "my-oauth-token";
+ properties[AuthProperties::kSigV4DelegateAuthType] = "oauth2";
+
+ auto manager_result = AuthManagers::Load("test-catalog", properties);
+ ASSERT_THAT(manager_result, IsOk());
+
+ auto session_result = manager_result.value()->CatalogSession(client_,
properties);
+ ASSERT_THAT(session_result, IsOk());
+
+ HTTPRequest request{.method = HttpMethod::kGet, .url =
"https://example.com/v1/config"};
+ auto auth_result = session_result.value()->Authenticate(request);
+ ASSERT_THAT(auth_result, IsOk());
+ const auto& headers = auth_result.value().headers;
+
+ EXPECT_NE(headers.find("authorization"), headers.end());
+ EXPECT_TRUE(headers.at("authorization").starts_with("AWS4-HMAC-SHA256"));
+ EXPECT_NE(headers.find("original-authorization"), headers.end());
+ EXPECT_EQ(headers.at("original-authorization"), "Bearer my-oauth-token");
+}
+
+TEST_F(SigV4AuthTest, AuthenticateWithSessionToken) {
+ auto properties = MakeSigV4Properties();
+ properties[AuthProperties::kSigV4SessionToken] = "FwoGZXIvYXdzEBYaDHqa0";
+
+ auto manager_result = AuthManagers::Load("test-catalog", properties);
+ ASSERT_THAT(manager_result, IsOk());
+
+ auto session_result = manager_result.value()->CatalogSession(client_,
properties);
+ ASSERT_THAT(session_result, IsOk());
+
+ HTTPRequest request{.method = HttpMethod::kGet, .url =
"https://example.com/v1/config"};
+ auto auth_result = session_result.value()->Authenticate(request);
+ ASSERT_THAT(auth_result, IsOk());
+ const auto& headers = auth_result.value().headers;
+
+ EXPECT_NE(headers.find("authorization"), headers.end());
+ EXPECT_NE(headers.find("x-amz-security-token"), headers.end());
+ EXPECT_EQ(headers.at("x-amz-security-token"), "FwoGZXIvYXdzEBYaDHqa0");
+}
+
+TEST_F(SigV4AuthTest, CustomSigningNameAndRegion) {
+ auto properties = MakeSigV4Properties();
+ properties[AuthProperties::kSigV4SigningRegion] = "eu-west-1";
+ properties[AuthProperties::kSigV4SigningName] = "custom-service";
+
+ auto manager_result = AuthManagers::Load("test-catalog", properties);
+ ASSERT_THAT(manager_result, IsOk());
+
+ auto session_result = manager_result.value()->CatalogSession(client_,
properties);
+ ASSERT_THAT(session_result, IsOk());
+
+ HTTPRequest request{.method = HttpMethod::kGet, .url =
"https://example.com/v1/config"};
+ auto auth_result = session_result.value()->Authenticate(request);
+ ASSERT_THAT(auth_result, IsOk());
+ const auto& headers = auth_result.value().headers;
+
+ auto auth_it = headers.find("authorization");
+ ASSERT_NE(auth_it, headers.end());
+ EXPECT_TRUE(auth_it->second.find("eu-west-1") != std::string::npos);
+ EXPECT_TRUE(auth_it->second.find("custom-service") != std::string::npos);
+}
+
+TEST_F(SigV4AuthTest, AuthTypeCaseInsensitive) {
+ for (const auto& auth_type : {"SIGV4", "SigV4", "sigV4"}) {
+ auto properties = MakeSigV4Properties();
+ properties[AuthProperties::kAuthType] = auth_type;
+ EXPECT_THAT(AuthManagers::Load("test-catalog", properties), IsOk())
+ << "Failed for auth type: " << auth_type;
+ }
+}
+
+TEST_F(SigV4AuthTest, DelegateDefaultsToOAuth2NoAuth) {
+ auto properties = MakeSigV4Properties();
+ auto manager_result = AuthManagers::Load("test-catalog", properties);
+ ASSERT_THAT(manager_result, IsOk());
+
+ auto session_result = manager_result.value()->CatalogSession(client_,
properties);
+ ASSERT_THAT(session_result, IsOk());
+
+ HTTPRequest request{.method = HttpMethod::kGet, .url =
"https://example.com/v1/config"};
+ auto auth_result = session_result.value()->Authenticate(request);
+ ASSERT_THAT(auth_result, IsOk());
+ const auto& headers = auth_result.value().headers;
+
+ EXPECT_EQ(headers.find("original-authorization"), headers.end());
+}
+
+TEST_F(SigV4AuthTest, TableSessionInheritsProperties) {
+ auto properties = MakeSigV4Properties();
+ auto manager_result = AuthManagers::Load("test-catalog", properties);
+ ASSERT_THAT(manager_result, IsOk());
+
+ auto catalog_session = manager_result.value()->CatalogSession(client_,
properties);
+ ASSERT_THAT(catalog_session, IsOk());
+
+ iceberg::TableIdentifier table_id{.ns = iceberg::Namespace{{"ns1"}}, .name =
"table1"};
+ std::unordered_map<std::string, std::string> table_props;
+ auto table_session = manager_result.value()->TableSession(table_id,
table_props,
+
catalog_session.value());
+ ASSERT_THAT(table_session, IsOk());
+
+ HTTPRequest request{.method = HttpMethod::kGet,
+ .url = "https://example.com/v1/ns1/tables/table1"};
+ auto auth_result = table_session.value()->Authenticate(request);
+ ASSERT_THAT(auth_result, IsOk());
+ EXPECT_NE(auth_result.value().headers.find("authorization"),
+ auth_result.value().headers.end());
+}
+
+// ---------- Tests ported from Java TestRESTSigV4AuthSession ----------
+
+// Java: authenticateWithoutBody
Review Comment:
```suggestion
```
Let's remove comments like this and below.
##########
src/iceberg/catalog/rest/auth/sigv4_auth_manager.cc:
##########
@@ -0,0 +1,320 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "iceberg/catalog/rest/auth/sigv4_auth_manager.h"
+
+#include <sstream>
+
+#include <aws/core/Aws.h>
+#include <aws/core/auth/AWSAuthSigner.h>
+#include <aws/core/auth/AWSCredentialsProvider.h>
+#include <aws/core/auth/AWSCredentialsProviderChain.h>
+#include <aws/core/client/ClientConfiguration.h>
+#include <aws/core/http/standard/StandardHttpRequest.h>
+#include <aws/core/utils/HashingUtils.h>
+
+#include "iceberg/catalog/rest/auth/auth_manager_internal.h"
+#include "iceberg/catalog/rest/auth/auth_managers.h"
+#include "iceberg/catalog/rest/auth/auth_properties.h"
+#include "iceberg/catalog/rest/endpoint.h"
+#include "iceberg/util/macros.h"
+#include "iceberg/util/string_util.h"
+
+namespace iceberg::rest::auth {
+
+namespace {
+
+/// \brief Ensures AWS SDK is initialized exactly once per process.
+/// ShutdownAPI is intentionally never called (leak-by-design) to avoid
+/// static destruction order issues with objects that may outlive shutdown.
+class AwsSdkGuard {
+ public:
+ static void EnsureInitialized() {
+ static AwsSdkGuard instance;
+ (void)instance;
+ }
+
+ private:
+ AwsSdkGuard() {
+ Aws::SDKOptions options;
+ Aws::InitAPI(options);
+ }
+};
+
+Aws::Http::HttpMethod ToAwsMethod(HttpMethod method) {
+ switch (method) {
+ case HttpMethod::kGet:
+ return Aws::Http::HttpMethod::HTTP_GET;
+ case HttpMethod::kPost:
+ return Aws::Http::HttpMethod::HTTP_POST;
+ case HttpMethod::kPut:
+ return Aws::Http::HttpMethod::HTTP_PUT;
+ case HttpMethod::kDelete:
+ return Aws::Http::HttpMethod::HTTP_DELETE;
+ case HttpMethod::kHead:
+ return Aws::Http::HttpMethod::HTTP_HEAD;
+ }
+ return Aws::Http::HttpMethod::HTTP_GET;
+}
+
+std::unordered_map<std::string, std::string> MergeProperties(
+ const std::unordered_map<std::string, std::string>& base,
+ const std::unordered_map<std::string, std::string>& overrides) {
+ auto merged = base;
+ for (const auto& [key, value] : overrides) {
+ merged.insert_or_assign(key, value);
+ }
+ return merged;
+}
+
+/// Matches Java RESTSigV4AuthSession: canonical headers carry
+/// Base64(SHA256(body)), canonical request trailer uses hex.
+class RestSigV4Signer : public Aws::Client::AWSAuthV4Signer {
+ public:
+ RestSigV4Signer(const std::shared_ptr<Aws::Auth::AWSCredentialsProvider>&
creds,
+ const char* service_name, const Aws::String& region)
+ : Aws::Client::AWSAuthV4Signer(creds, service_name, region,
+ PayloadSigningPolicy::Always,
+ /*urlEscapePath=*/false) {
+ // Skip the signer's hex overwrite of x-amz-content-sha256 so canonical
+ // headers see the caller's Base64; ComputePayloadHash still feeds hex
+ // into the canonical request trailer.
+ m_includeSha256HashHeader = false;
+ }
+};
+
+} // namespace
+
+// ---- SigV4AuthSession ----
+
+SigV4AuthSession::SigV4AuthSession(
+ std::shared_ptr<AuthSession> delegate, std::string signing_region,
+ std::string signing_name,
+ std::shared_ptr<Aws::Auth::AWSCredentialsProvider> credentials_provider,
+ std::unordered_map<std::string, std::string> effective_properties)
+ : delegate_(std::move(delegate)),
+ signing_region_(std::move(signing_region)),
+ signing_name_(std::move(signing_name)),
+ credentials_provider_(std::move(credentials_provider)),
+ signer_(std::make_unique<RestSigV4Signer>(
+ credentials_provider_, signing_name_.c_str(),
signing_region_.c_str())),
+ effective_properties_(std::move(effective_properties)) {}
+
+SigV4AuthSession::~SigV4AuthSession() = default;
+
+Result<HTTPRequest> SigV4AuthSession::Authenticate(const HTTPRequest& request)
{
+ ICEBERG_ASSIGN_OR_RAISE(auto delegate_request,
delegate_->Authenticate(request));
+ const auto& original_headers = delegate_request.headers;
+
+ std::unordered_map<std::string, std::string> signing_headers;
+ for (const auto& [name, value] : original_headers) {
+ if (StringUtils::EqualsIgnoreCase(name, "Authorization")) {
+ signing_headers[std::string(kRelocatedHeaderPrefix) + name] = value;
+ } else {
+ signing_headers[name] = value;
+ }
+ }
+
+ Aws::Http::URI aws_uri(delegate_request.url.c_str());
+ auto aws_request =
std::make_shared<Aws::Http::Standard::StandardHttpRequest>(
+ aws_uri, ToAwsMethod(delegate_request.method));
+ for (const auto& [name, value] : signing_headers) {
+ aws_request->SetHeaderValue(Aws::String(name.c_str()),
Aws::String(value.c_str()));
+ }
+
+ // Empty body: hex EMPTY_BODY_SHA256 (Java parity workaround for the signer
+ // computing an invalid checksum on empty bodies). Non-empty: Base64.
+ if (delegate_request.body.empty()) {
+ aws_request->SetHeaderValue("x-amz-content-sha256",
Aws::String(kEmptyBodySha256));
+ } else {
+ auto body_stream =
+ Aws::MakeShared<std::stringstream>("SigV4Body", delegate_request.body);
+ aws_request->AddContentBody(body_stream);
+ auto sha256 = Aws::Utils::HashingUtils::CalculateSHA256(
+ Aws::String(delegate_request.body.data(),
delegate_request.body.size()));
+ aws_request->SetHeaderValue("x-amz-content-sha256",
+
Aws::Utils::HashingUtils::Base64Encode(sha256));
+ }
+
+ if (!signer_->SignRequest(*aws_request)) {
+ return std::unexpected<Error>(Error{.kind =
ErrorKind::kAuthenticationFailed,
+ .message = "SigV4 signing failed"});
+ }
+
+ HTTPRequest signed_request{.method = delegate_request.method,
+ .url = std::move(delegate_request.url),
+ .headers = {},
+ .body = std::move(delegate_request.body)};
+ for (const auto& [aws_name, aws_value] : aws_request->GetHeaders()) {
+ std::string name(aws_name.c_str(), aws_name.size());
+ std::string value(aws_value.c_str(), aws_value.size());
+ for (const auto& [orig_name, orig_value] : original_headers) {
+ if (StringUtils::EqualsIgnoreCase(orig_name, name) && orig_value !=
value) {
+ signed_request.headers[std::string(kRelocatedHeaderPrefix) +
orig_name] =
+ orig_value;
+ break;
+ }
+ }
+ signed_request.headers[std::move(name)] = std::move(value);
+ }
+
+ return signed_request;
+}
+
+Status SigV4AuthSession::Close() { return delegate_->Close(); }
+
+// ---- SigV4AuthManager ----
+
+SigV4AuthManager::SigV4AuthManager(std::unique_ptr<AuthManager> delegate)
+ : delegate_(std::move(delegate)) {}
+
+SigV4AuthManager::~SigV4AuthManager() = default;
+
+Result<std::shared_ptr<AuthSession>> SigV4AuthManager::InitSession(
+ HttpClient& init_client,
+ const std::unordered_map<std::string, std::string>& properties) {
+ AwsSdkGuard::EnsureInitialized();
+ ICEBERG_ASSIGN_OR_RAISE(auto delegate_session,
+ delegate_->InitSession(init_client, properties));
+ return WrapSession(std::move(delegate_session), properties);
+}
+
+Result<std::shared_ptr<AuthSession>> SigV4AuthManager::CatalogSession(
+ HttpClient& shared_client,
+ const std::unordered_map<std::string, std::string>& properties) {
+ AwsSdkGuard::EnsureInitialized();
+ ICEBERG_ASSIGN_OR_RAISE(auto delegate_session,
+ delegate_->CatalogSession(shared_client,
properties));
+ return WrapSession(std::move(delegate_session), properties);
+}
+
+Result<std::shared_ptr<AuthSession>> SigV4AuthManager::ContextualSession(
+ const std::unordered_map<std::string, std::string>& context,
+ std::shared_ptr<AuthSession> parent) {
+ auto sigv4_parent =
std::dynamic_pointer_cast<SigV4AuthSession>(std::move(parent));
+ ICEBERG_PRECHECK(sigv4_parent != nullptr,
+ "SigV4AuthManager parent must be a SigV4AuthSession");
+
+ ICEBERG_ASSIGN_OR_RAISE(auto delegate_session, delegate_->ContextualSession(
+ context,
sigv4_parent->delegate()));
+
+ auto merged = MergeProperties(sigv4_parent->effective_properties(), context);
+ return WrapSession(std::move(delegate_session), std::move(merged));
+}
+
+Result<std::shared_ptr<AuthSession>> SigV4AuthManager::TableSession(
+ const TableIdentifier& table,
+ const std::unordered_map<std::string, std::string>& properties,
+ std::shared_ptr<AuthSession> parent) {
+ auto sigv4_parent =
std::dynamic_pointer_cast<SigV4AuthSession>(std::move(parent));
+ ICEBERG_PRECHECK(sigv4_parent != nullptr,
+ "SigV4AuthManager parent must be a SigV4AuthSession");
+
+ ICEBERG_ASSIGN_OR_RAISE(
+ auto delegate_session,
+ delegate_->TableSession(table, properties, sigv4_parent->delegate()));
+
+ auto merged = MergeProperties(sigv4_parent->effective_properties(),
properties);
Review Comment:
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?
##########
src/iceberg/catalog/rest/CMakeLists.txt:
##########
@@ -63,4 +74,12 @@ add_iceberg_lib(iceberg_rest
SHARED_INSTALL_INTERFACE_LIBS
${ICEBERG_REST_SHARED_INSTALL_INTERFACE_LIBS})
+if(ICEBERG_BUILD_SIGV4)
+ foreach(LIB iceberg_rest_static iceberg_rest_shared)
+ if(TARGET ${LIB})
+ target_compile_definitions(${LIB} PUBLIC ICEBERG_BUILD_SIGV4)
+ endif()
+ endforeach()
+endif()
+
iceberg_install_all_headers(iceberg/catalog/rest)
Review Comment:
Currently `sigv4_auth_manager.h` is installed regardless of the
ICEBERG_BUILD_SIGV4 option value.
##########
src/iceberg/catalog/rest/auth/auth_session.h:
##########
@@ -32,25 +33,37 @@
namespace iceberg::rest::auth {
+/// \brief An outgoing HTTP request passed through an AuthSession. Mirrors the
+/// HTTPRequest type used by the Java reference implementation so signing
+/// implementations like SigV4 can operate on method, url, headers, and body
+/// as a single value.
+struct ICEBERG_REST_EXPORT HTTPRequest {
Review Comment:
Let's rename it to `HttpRequest`.
##########
src/iceberg/catalog/rest/auth/auth_session.h:
##########
@@ -32,25 +33,37 @@
namespace iceberg::rest::auth {
+/// \brief An outgoing HTTP request passed through an AuthSession. Mirrors the
+/// HTTPRequest type used by the Java reference implementation so signing
+/// implementations like SigV4 can operate on method, url, headers, and body
+/// as a single value.
+struct ICEBERG_REST_EXPORT HTTPRequest {
Review Comment:
BTW, it looks weird that its namespace is `auth`.
##########
src/iceberg/catalog/rest/auth/sigv4_auth_manager.cc:
##########
@@ -0,0 +1,320 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "iceberg/catalog/rest/auth/sigv4_auth_manager.h"
+
+#include <sstream>
+
+#include <aws/core/Aws.h>
+#include <aws/core/auth/AWSAuthSigner.h>
+#include <aws/core/auth/AWSCredentialsProvider.h>
+#include <aws/core/auth/AWSCredentialsProviderChain.h>
+#include <aws/core/client/ClientConfiguration.h>
+#include <aws/core/http/standard/StandardHttpRequest.h>
+#include <aws/core/utils/HashingUtils.h>
+
+#include "iceberg/catalog/rest/auth/auth_manager_internal.h"
+#include "iceberg/catalog/rest/auth/auth_managers.h"
+#include "iceberg/catalog/rest/auth/auth_properties.h"
+#include "iceberg/catalog/rest/endpoint.h"
+#include "iceberg/util/macros.h"
+#include "iceberg/util/string_util.h"
+
+namespace iceberg::rest::auth {
+
+namespace {
+
+/// \brief Ensures AWS SDK is initialized exactly once per process.
+/// ShutdownAPI is intentionally never called (leak-by-design) to avoid
Review Comment:
Is this the recommended approach?
##########
src/iceberg/catalog/rest/auth/sigv4_auth_manager.h:
##########
@@ -0,0 +1,132 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+#include <memory>
+#include <string>
+#include <unordered_map>
+
+#include "iceberg/catalog/rest/auth/auth_manager.h"
+#include "iceberg/catalog/rest/auth/auth_session.h"
+#include "iceberg/catalog/rest/iceberg_rest_export.h"
+#include "iceberg/result.h"
+
+namespace Aws::Auth {
+class AWSCredentialsProvider;
+} // namespace Aws::Auth
+
+namespace Aws::Client {
+class AWSAuthV4Signer;
+} // namespace Aws::Client
+
+namespace iceberg::rest::auth {
+
+/// \brief An AuthSession that signs requests with AWS SigV4.
+///
+/// The request is first authenticated by the delegate AuthSession (e.g.,
OAuth2),
+/// then signed with SigV4. In case of conflicting headers, the Authorization
header
+/// set by the delegate is relocated with an "Original-" prefix, then included
in
+/// the canonical headers to sign.
+///
+/// See https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_sigv.html
+///
+/// Thread safety: Authenticate() is thread-safe as long as the delegate
+/// session is.
+class ICEBERG_REST_EXPORT SigV4AuthSession : public AuthSession {
+ public:
+ /// SHA-256 hash of empty string, used for requests with no body.
+ static constexpr std::string_view kEmptyBodySha256 =
+ "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855";
+
+ /// Prefix prepended to relocated headers that conflict with SigV4-signed
headers.
+ static constexpr std::string_view kRelocatedHeaderPrefix = "Original-";
+
+ SigV4AuthSession(
+ std::shared_ptr<AuthSession> delegate, std::string signing_region,
+ std::string signing_name,
+ std::shared_ptr<Aws::Auth::AWSCredentialsProvider> credentials_provider,
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
