Skip to content

Commit ee45d33

Browse files
committed
Polish REST SigV4 auth integration
Simplify AWS SDK wiring, remove premature session-cache plumbing, tighten credential validation, and strengthen SigV4 tests.
1 parent 2e68f1f commit ee45d33

27 files changed

Lines changed: 733 additions & 1332 deletions

‎.github/workflows/aws_test.yml‎

Lines changed: 10 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,6 @@
1515
# specific language governing permissions and limitations
1616
# under the License.
1717

18-
# AWS-related tests. ICEBERG_S3 and ICEBERG_SIGV4 are exercised individually and
19-
# together; with both on, ICEBERG_AWSSDK_SOURCE defaults to BUNDLED so SigV4
20-
# reuses Arrow's bundled aws-cpp-sdk-core — a single AWS SDK is linked (no ODR).
2118
name: AWS Tests
2219

2320
on:
@@ -50,33 +47,26 @@ jobs:
5047
fail-fast: false
5148
matrix:
5249
include:
53-
- title: AMD64 Ubuntu 24.04, S3
50+
- title: Ubuntu 24.04, S3 + SigV4, bundled AWS SDK
5451
runs-on: ubuntu-24.04
5552
CC: gcc-14
5653
CXX: g++-14
5754
s3: "ON"
58-
sigv4: "OFF"
59-
- title: AMD64 Ubuntu 24.04, SigV4
60-
runs-on: ubuntu-24.04
61-
CC: gcc-14
62-
CXX: g++-14
63-
s3: "OFF"
6455
sigv4: "ON"
65-
aws-sdk-features: core
66-
- title: AMD64 Ubuntu 24.04, S3 + SigV4
56+
bundle_awssdk: "ON"
57+
- title: Ubuntu 24.04, S3 + SigV4, system AWS SDK
6758
runs-on: ubuntu-24.04
6859
CC: gcc-14
6960
CXX: g++-14
7061
s3: "ON"
7162
sigv4: "ON"
72-
# Arrow's S3 filesystem consumes this same AWS SDK, so it needs the
73-
# S3-related components in addition to core (config is required by
74-
# Arrow's FindAWSSDKAlt).
63+
bundle_awssdk: "OFF"
7564
aws-sdk-features: core,config,s3,identity-management,sts,transfer
76-
- title: AArch64 macOS 26, S3
65+
- title: macOS 26 ARM64, S3, bundled AWS SDK
7766
runs-on: macos-26
7867
s3: "ON"
7968
sigv4: "OFF"
69+
bundle_awssdk: "ON"
8070
env:
8171
ICEBERG_TEST_S3_URI: s3://iceberg-test
8272
AWS_ACCESS_KEY_ID: minio
@@ -94,14 +84,14 @@ jobs:
9484
shell: bash
9585
run: sudo apt-get update && sudo apt-get install -y libcurl4-openssl-dev
9686
- name: Cache vcpkg packages
97-
if: ${{ matrix.sigv4 == 'ON' && matrix.s3 == 'OFF' }}
87+
if: ${{ startsWith(matrix.runs-on, 'ubuntu') && matrix.bundle_awssdk == 'OFF' }}
9888
uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830 # v4.3.0
9989
id: vcpkg-cache
10090
with:
10191
path: /usr/local/share/vcpkg/installed
10292
key: vcpkg-x64-linux-aws-sdk-cpp-s3-${{ matrix.s3 }}-sigv4-${{ matrix.sigv4 }}-${{ hashFiles('.github/workflows/aws_test.yml') }}
10393
- name: Install AWS SDK via vcpkg
104-
if: ${{ matrix.sigv4 == 'ON' && matrix.s3 == 'OFF' && steps.vcpkg-cache.outputs.cache-hit != 'true' }}
94+
if: ${{ startsWith(matrix.runs-on, 'ubuntu') && matrix.bundle_awssdk == 'OFF' && steps.vcpkg-cache.outputs.cache-hit != 'true' }}
10595
shell: bash
10696
# Retry to ride out transient GitHub/mirror download failures (504s).
10797
run: |
@@ -126,8 +116,8 @@ jobs:
126116
- name: Build and test Iceberg
127117
shell: bash
128118
env:
129-
CMAKE_TOOLCHAIN_FILE: ${{ matrix.sigv4 == 'ON' && matrix.s3 == 'OFF' && '/usr/local/share/vcpkg/scripts/buildsystems/vcpkg.cmake' || '' }}
130-
run: ci/scripts/build_iceberg.sh "$(pwd)" OFF OFF ${{ matrix.s3 }} ${{ matrix.sigv4 }}
119+
CMAKE_TOOLCHAIN_FILE: ${{ startsWith(matrix.runs-on, 'ubuntu') && matrix.bundle_awssdk == 'OFF' && '/usr/local/share/vcpkg/scripts/buildsystems/vcpkg.cmake' || '' }}
120+
run: ci/scripts/build_iceberg.sh "$(pwd)" OFF OFF ${{ matrix.s3 }} ${{ matrix.sigv4 }} ${{ matrix.bundle_awssdk }}
131121

132122
# Exercise the Meson build with SigV4 enabled (resolves aws-cpp-sdk-core via
133123
# its CMake config, not pkg-config whose Cflags force -std=c++11).

‎CMakeLists.txt‎

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -53,13 +53,8 @@ option(ICEBERG_SQL_SQLITE "Build the SQLite connector for the SQL catalog" OFF)
5353
option(ICEBERG_SQL_POSTGRESQL "Build the PostgreSQL connector for the SQL catalog" OFF)
5454
option(ICEBERG_SQL_MYSQL "Build the MySQL connector for the SQL catalog" OFF)
5555
option(ICEBERG_S3 "Build with S3 support" OFF)
56-
option(ICEBERG_SIGV4 "Build SigV4 authentication support (requires AWS SDK)" OFF)
57-
set(ICEBERG_AWSSDK_SOURCE
58-
"AUTO"
59-
CACHE STRING "AWS SDK source for SigV4: AUTO (reuse Arrow's bundled AWS SDK when \
60-
ICEBERG_S3 is ON, otherwise SYSTEM), SYSTEM (find an installed AWS SDK), or \
61-
BUNDLED (reuse Arrow's bundled AWS SDK; requires ICEBERG_S3)")
62-
set_property(CACHE ICEBERG_AWSSDK_SOURCE PROPERTY STRINGS AUTO SYSTEM BUNDLED)
56+
option(ICEBERG_SIGV4 "Build with SigV4 support" OFF)
57+
option(ICEBERG_BUNDLE_AWSSDK "Bundle AWS SDK for S3/SigV4 support" ON)
6358
option(ICEBERG_ENABLE_ASAN "Enable Address Sanitizer" OFF)
6459
option(ICEBERG_ENABLE_UBSAN "Enable Undefined Behavior Sanitizer" OFF)
6560

@@ -83,12 +78,6 @@ if(ICEBERG_BUILD_REST_INTEGRATION_TESTS AND WIN32)
8378
message(WARNING "Cannot build rest integration test on Windows, turning it off.")
8479
endif()
8580

86-
# ICEBERG_S3 requires ICEBERG_BUILD_BUNDLE
87-
if(NOT ICEBERG_BUILD_BUNDLE AND ICEBERG_S3)
88-
set(ICEBERG_S3 OFF)
89-
message(STATUS "ICEBERG_S3 is disabled because ICEBERG_BUILD_BUNDLE is OFF")
90-
endif()
91-
9281
include(CMakeParseArguments)
9382
include(IcebergBuildUtils)
9483
include(IcebergSanitizer)

‎ci/scripts/build_iceberg.sh‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
# specific language governing permissions and limitations
1818
# under the License.
1919
#
20-
# Usage: build_iceberg.sh <source_dir> [rest_integration_tests=OFF] [sccache=OFF] [s3=OFF] [sigv4=OFF]
20+
# Usage: build_iceberg.sh <source_dir> [rest_integration_tests=OFF] [sccache=OFF] [s3=OFF] [sigv4=OFF] [bundle_awssdk=ON]
2121

2222
set -eux
2323

@@ -27,6 +27,7 @@ build_rest_integration_test=${2:-OFF}
2727
build_enable_sccache=${3:-OFF}
2828
build_enable_s3=${4:-OFF}
2929
build_enable_sigv4=${5:-OFF}
30+
build_bundle_awssdk=${6:-ON}
3031
run_tests=${ICEBERG_RUN_TESTS:-ON}
3132

3233
mkdir ${build_dir}
@@ -56,12 +57,17 @@ else
5657
CMAKE_ARGS+=("-DICEBERG_SIGV4=OFF")
5758
fi
5859

60+
if [[ "${build_bundle_awssdk}" == "ON" ]]; then
61+
CMAKE_ARGS+=("-DICEBERG_BUNDLE_AWSSDK=ON")
62+
else
63+
CMAKE_ARGS+=("-DICEBERG_BUNDLE_AWSSDK=OFF")
64+
fi
65+
5966
if is_windows; then
6067
CMAKE_ARGS+=("-DCMAKE_TOOLCHAIN_FILE=C:/vcpkg/scripts/buildsystems/vcpkg.cmake")
6168
CMAKE_ARGS+=("-DCMAKE_BUILD_TYPE=Release")
6269
else
6370
# Pass an externally provided toolchain (e.g. vcpkg for the SigV4 job)
64-
# explicitly instead of relying on CMake >= 3.21 reading the env var.
6571
if [[ -n "${CMAKE_TOOLCHAIN_FILE:-}" ]]; then
6672
CMAKE_ARGS+=("-DCMAKE_TOOLCHAIN_FILE=${CMAKE_TOOLCHAIN_FILE}")
6773
fi

‎cmake_modules/IcebergThirdpartyToolchain.cmake‎

Lines changed: 48 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -19,23 +19,49 @@
1919
# third party libraries.
2020
set(ICEBERG_SYSTEM_DEPENDENCIES)
2121
set(ICEBERG_ARROW_INSTALL_INTERFACE_LIBS)
22-
23-
if(ICEBERG_SIGV4)
24-
set(ICEBERG_AWSSDK_SOURCE_RESOLVED "${ICEBERG_AWSSDK_SOURCE}")
25-
if(ICEBERG_AWSSDK_SOURCE_RESOLVED STREQUAL "AUTO")
26-
if(ICEBERG_S3)
27-
set(ICEBERG_AWSSDK_SOURCE_RESOLVED "BUNDLED")
28-
else()
29-
set(ICEBERG_AWSSDK_SOURCE_RESOLVED "SYSTEM")
30-
endif()
22+
set(ICEBERG_AWSSDK_BUNDLED FALSE)
23+
if(ICEBERG_S3 AND ICEBERG_BUNDLE_AWSSDK)
24+
if(NOT ICEBERG_BUILD_BUNDLE)
25+
message(FATAL_ERROR "ICEBERG_BUNDLE_AWSSDK requires ICEBERG_BUILD_BUNDLE to be ON")
3126
endif()
32-
if(ICEBERG_AWSSDK_SOURCE_RESOLVED STREQUAL "BUNDLED" AND NOT ICEBERG_S3)
33-
message(FATAL_ERROR "ICEBERG_AWSSDK_SOURCE=BUNDLED requires ICEBERG_S3=ON: "
34-
"the bundled AWS SDK is provided by Arrow's S3 support.")
27+
set(ICEBERG_AWSSDK_BUNDLED TRUE)
28+
endif()
29+
30+
set(ICEBERG_AWSSDK_COMPONENTS)
31+
if(NOT ICEBERG_AWSSDK_BUNDLED)
32+
if(ICEBERG_S3)
33+
list(APPEND
34+
ICEBERG_AWSSDK_COMPONENTS
35+
core
36+
config
37+
s3
38+
transfer
39+
identity-management
40+
sts)
41+
elseif(ICEBERG_SIGV4)
42+
list(APPEND ICEBERG_AWSSDK_COMPONENTS core)
3543
endif()
36-
message(STATUS "AWS SDK source for SigV4: ${ICEBERG_AWSSDK_SOURCE_RESOLVED}")
3744
endif()
3845

46+
# ----------------------------------------------------------------------
47+
# AWS SDK for C++
48+
49+
function(resolve_aws_sdk_dependency)
50+
if(NOT ICEBERG_AWSSDK_COMPONENTS)
51+
return()
52+
endif()
53+
find_package(AWSSDK REQUIRED COMPONENTS ${ICEBERG_AWSSDK_COMPONENTS})
54+
list(APPEND ICEBERG_SYSTEM_DEPENDENCIES AWSSDK)
55+
set(ICEBERG_SYSTEM_DEPENDENCIES
56+
${ICEBERG_SYSTEM_DEPENDENCIES}
57+
PARENT_SCOPE)
58+
# Forwarded to find_dependency(AWSSDK ...) in iceberg-config.cmake.in so
59+
# downstream installed builds load the same AWS SDK targets.
60+
set(ICEBERG_FIND_EXTRA_ARGS_AWSSDK
61+
"COMPONENTS;${ICEBERG_AWSSDK_COMPONENTS}"
62+
PARENT_SCOPE)
63+
endfunction()
64+
3965
# ----------------------------------------------------------------------
4066
# Versions and URLs for toolchain builds
4167
#
@@ -126,12 +152,10 @@ function(resolve_arrow_dependency)
126152
set(ARROW_RUNTIME_SIMD_LEVEL "NONE")
127153
set(ARROW_POSITION_INDEPENDENT_CODE ON)
128154
set(ARROW_DEPENDENCY_SOURCE "BUNDLED")
129-
if(ICEBERG_S3
130-
AND ICEBERG_SIGV4
131-
AND ICEBERG_AWSSDK_SOURCE_RESOLVED STREQUAL "SYSTEM")
155+
set(ARROW_WITH_ZLIB ON)
156+
if(ICEBERG_S3 AND NOT ICEBERG_AWSSDK_BUNDLED)
132157
set(AWSSDK_SOURCE "SYSTEM")
133158
endif()
134-
set(ARROW_WITH_ZLIB ON)
135159
set(ZLIB_SOURCE "SYSTEM")
136160
set(ARROW_VERBOSE_THIRDPARTY_BUILD OFF)
137161
set(CMAKE_CXX_STANDARD 20)
@@ -641,6 +665,13 @@ resolve_nanoarrow_dependency()
641665
resolve_croaring_dependency()
642666
resolve_nlohmann_json_dependency()
643667

668+
if(ICEBERG_S3 OR ICEBERG_SIGV4)
669+
if(ICEBERG_SIGV4 AND NOT ICEBERG_BUILD_REST)
670+
message(FATAL_ERROR "ICEBERG_SIGV4 requires ICEBERG_BUILD_REST to be ON")
671+
endif()
672+
resolve_aws_sdk_dependency()
673+
endif()
674+
644675
if(ICEBERG_BUILD_BUNDLE)
645676
resolve_arrow_dependency()
646677
resolve_avro_dependency()
@@ -654,30 +685,3 @@ endif()
654685
if(ICEBERG_BUILD_SQL_CATALOG)
655686
resolve_sql_catalog_dependencies()
656687
endif()
657-
658-
# ----------------------------------------------------------------------
659-
# AWS SDK for C++
660-
661-
function(resolve_aws_sdk_dependency)
662-
if(ICEBERG_AWSSDK_SOURCE_RESOLVED STREQUAL "BUNDLED")
663-
message(STATUS "SigV4 reuses Arrow's bundled AWS SDK (aws-cpp-sdk-core)")
664-
return()
665-
endif()
666-
find_package(AWSSDK REQUIRED COMPONENTS core)
667-
list(APPEND ICEBERG_SYSTEM_DEPENDENCIES AWSSDK)
668-
set(ICEBERG_SYSTEM_DEPENDENCIES
669-
${ICEBERG_SYSTEM_DEPENDENCIES}
670-
PARENT_SCOPE)
671-
# Forwarded to find_dependency(AWSSDK ...) in iceberg-config.cmake.in so
672-
# downstream installed builds load aws-cpp-sdk-core via AWSSDK_FIND_COMPONENTS.
673-
set(ICEBERG_FIND_EXTRA_ARGS_AWSSDK
674-
"COMPONENTS;core"
675-
PARENT_SCOPE)
676-
endfunction()
677-
678-
if(ICEBERG_SIGV4)
679-
if(NOT ICEBERG_BUILD_REST)
680-
message(FATAL_ERROR "ICEBERG_SIGV4 requires ICEBERG_BUILD_REST to be ON")
681-
endif()
682-
resolve_aws_sdk_dependency()
683-
endif()

‎meson.options‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,6 @@ option(
4444
value: 'disabled',
4545
)
4646

47-
# Resolves a system-installed AWS SDK via its CMake config; the bundled-AWS
48-
# path (ICEBERG_AWSSDK_SOURCE=BUNDLED) is CMake-only.
4947
option(
5048
'sigv4',
5149
type: 'feature',

‎src/iceberg/catalog/rest/CMakeLists.txt‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -23,13 +23,12 @@ set(ICEBERG_REST_SOURCES
2323
auth/auth_properties.cc
2424
auth/auth_session.cc
2525
auth/oauth2_util.cc
26-
auth/sigv4_auth_manager.cc
26+
auth/sigv4_manager.cc
2727
auth/token_refresh_scheduler.cc
2828
catalog_properties.cc
2929
endpoint.cc
3030
error_handlers.cc
3131
http_client.cc
32-
http_request.cc
3332
json_serde.cc
3433
resource_paths.cc
3534
rest_catalog.cc
@@ -58,7 +57,7 @@ list(APPEND
5857
if(ICEBERG_SIGV4)
5958
list(APPEND ICEBERG_REST_STATIC_BUILD_INTERFACE_LIBS aws-cpp-sdk-core)
6059
list(APPEND ICEBERG_REST_SHARED_BUILD_INTERFACE_LIBS aws-cpp-sdk-core)
61-
if(ICEBERG_AWSSDK_SOURCE_RESOLVED STREQUAL "SYSTEM")
60+
if(NOT ICEBERG_AWSSDK_BUNDLED)
6261
list(APPEND ICEBERG_REST_STATIC_INSTALL_INTERFACE_LIBS aws-cpp-sdk-core)
6362
list(APPEND ICEBERG_REST_SHARED_INSTALL_INTERFACE_LIBS aws-cpp-sdk-core)
6463
endif()
@@ -76,12 +75,16 @@ add_iceberg_lib(iceberg_rest
7675
SHARED_INSTALL_INTERFACE_LIBS
7776
${ICEBERG_REST_SHARED_INSTALL_INTERFACE_LIBS})
7877

79-
if(ICEBERG_SIGV4)
80-
foreach(LIB iceberg_rest_static iceberg_rest_shared)
81-
if(TARGET ${LIB})
82-
target_compile_definitions(${LIB} PUBLIC ICEBERG_SIGV4)
78+
foreach(LIB iceberg_rest_static iceberg_rest_shared)
79+
if(TARGET ${LIB})
80+
if(ICEBERG_SIGV4)
81+
target_compile_definitions(${LIB}
82+
PUBLIC "$<BUILD_INTERFACE:ICEBERG_SIGV4_ENABLED=1>")
83+
else()
84+
target_compile_definitions(${LIB}
85+
PUBLIC "$<BUILD_INTERFACE:ICEBERG_SIGV4_ENABLED=0>")
8386
endif()
84-
endforeach()
85-
endif()
87+
endif()
88+
endforeach()
8689

8790
iceberg_install_all_headers(iceberg/catalog/rest)

‎src/iceberg/catalog/rest/auth/auth_manager.cc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,8 @@ Result<std::shared_ptr<AuthSession>> AuthManager::InitSession(
3838
}
3939

4040
Result<std::shared_ptr<AuthSession>> AuthManager::ContextualSession(
41-
[[maybe_unused]] const SessionContext& context, std::shared_ptr<AuthSession> parent) {
41+
[[maybe_unused]] const std::unordered_map<std::string, std::string>& context,
42+
std::shared_ptr<AuthSession> parent) {
4243
// By default, return the parent session as-is
4344
return parent;
4445
}

‎src/iceberg/catalog/rest/auth/auth_manager.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@
2323
#include <string>
2424
#include <unordered_map>
2525

26-
#include "iceberg/catalog/rest/auth/session_context.h"
2726
#include "iceberg/catalog/rest/iceberg_rest_export.h"
2827
#include "iceberg/catalog/rest/type_fwd.h"
2928
#include "iceberg/result.h"
@@ -71,12 +70,13 @@ class ICEBERG_REST_EXPORT AuthManager {
7170
/// This method is used by SessionCatalog to create sessions for different contexts
7271
/// (e.g., different users or tenants).
7372
///
74-
/// \param context Per-session properties and credentials.
73+
/// \param context Context properties (e.g., user credentials, tenant info).
7574
/// \param parent Catalog session to inherit from or return as-is.
7675
/// \return A context-specific session, or the parent session if no context-specific
7776
/// session is needed, or an error if session creation fails.
7877
virtual Result<std::shared_ptr<AuthSession>> ContextualSession(
79-
const SessionContext& context, std::shared_ptr<AuthSession> parent);
78+
const std::unordered_map<std::string, std::string>& context,
79+
std::shared_ptr<AuthSession> parent);
8080

8181
/// \brief Create or reuse a session scoped to a single table/view.
8282
///

‎src/iceberg/catalog/rest/auth/auth_properties.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,10 @@ class ICEBERG_REST_EXPORT AuthProperties : public ConfigBase<AuthProperties> {
5959
inline static const std::string kSigV4Enabled = "rest.sigv4-enabled";
6060
inline static const std::string kSigV4DelegateAuthType =
6161
"rest.auth.sigv4.delegate-auth-type";
62+
63+
/// SigV4 signing region. If unset, SigV4 resolves the signing region from
64+
/// AWS environment/profile configuration and fails if no region can be
65+
/// resolved.
6266
inline static const std::string kSigV4SigningRegion = "rest.signing-region";
6367
inline static const std::string kSigV4SigningName = "rest.signing-name";
6468
inline static const std::string kSigV4SigningNameDefault = "execute-api";

‎src/iceberg/catalog/rest/auth/auth_session.cc‎

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -43,12 +43,11 @@ class DefaultAuthSession : public AuthSession {
4343
explicit DefaultAuthSession(std::unordered_map<std::string, std::string> headers)
4444
: headers_(std::move(headers)) {}
4545

46-
Result<HttpRequest> Authenticate(const HttpRequest& request) override {
47-
HttpRequest authenticated = request;
46+
Result<HttpRequest> Authenticate(HttpRequest request) override {
4847
for (const auto& [key, value] : headers_) {
49-
authenticated.headers.try_emplace(key, value);
48+
request.headers.try_emplace(key, value);
5049
}
51-
return authenticated;
50+
return request;
5251
}
5352

5453
private:
@@ -78,13 +77,12 @@ class OAuth2AuthSession : public AuthSession,
7877
return session;
7978
}
8079

81-
Result<HttpRequest> Authenticate(const HttpRequest& request) override {
82-
HttpRequest authenticated = request;
80+
Result<HttpRequest> Authenticate(HttpRequest request) override {
8381
std::shared_lock lock(mutex_);
8482
for (const auto& [key, value] : headers_) {
85-
authenticated.headers.try_emplace(key, value);
83+
request.headers.try_emplace(key, value);
8684
}
87-
return authenticated;
85+
return request;
8886
}
8987

9088
Status Close() override { return CloseImpl(); }

0 commit comments

Comments
 (0)