From 1d068ab7f77f9114cbde52cfa7ed7363434443a4 Mon Sep 17 00:00:00 2001 From: "shuxu.li" Date: Mon, 12 Oct 2026 16:42:09 +0800 Subject: [PATCH] fix(rest): improve REST catalog configuration and error handling This commit improves the robustness of REST catalog operations: - Filter server-provided configuration to prevent override of client-specified authentication settings - Improve JSON parse error messages to avoid exposing sensitive data in logs by reporting only type information instead of full values - Add log sanitizer utility to redact sensitive configuration values from application logs --- src/iceberg/CMakeLists.txt | 2 + .../catalog/rest/auth/auth_pinned_keys.cc | 86 +++++++++ .../catalog/rest/auth/auth_pinned_keys.h | 105 +++++++++++ src/iceberg/catalog/rest/rest_catalog.cc | 9 +- src/iceberg/cpp20_compatibility_internal.h | 2 + src/iceberg/json_serde.cc | 4 +- src/iceberg/test/CMakeLists.txt | 11 +- src/iceberg/test/auth_pinned_keys_test.cc | 132 +++++++++++++ src/iceberg/test/log_sanitizer_test.cc | 177 ++++++++++++++++++ .../test/rest_catalog_security_test.cc | 131 +++++++++++++ src/iceberg/util/json_util_internal.h | 33 ++-- src/iceberg/util/log_sanitizer.cc | 142 ++++++++++++++ src/iceberg/util/log_sanitizer.h | 122 ++++++++++++ 13 files changed, 938 insertions(+), 18 deletions(-) create mode 100644 src/iceberg/catalog/rest/auth/auth_pinned_keys.cc create mode 100644 src/iceberg/catalog/rest/auth/auth_pinned_keys.h create mode 100644 src/iceberg/test/auth_pinned_keys_test.cc create mode 100644 src/iceberg/test/log_sanitizer_test.cc create mode 100644 src/iceberg/test/rest_catalog_security_test.cc create mode 100644 src/iceberg/util/log_sanitizer.cc create mode 100644 src/iceberg/util/log_sanitizer.h diff --git a/src/iceberg/CMakeLists.txt b/src/iceberg/CMakeLists.txt index 657d487c2c..db928b3794 100644 --- a/src/iceberg/CMakeLists.txt +++ b/src/iceberg/CMakeLists.txt @@ -24,6 +24,7 @@ set(ICEBERG_SOURCES arrow_row_builder.cc catalog/catalog_util.cc catalog/memory/in_memory_catalog.cc + catalog/rest/auth/auth_pinned_keys.cc catalog/session_catalog.cc catalog/session_context.cc data_table_scan.cc @@ -151,6 +152,7 @@ set(ICEBERG_SOURCES util/decimal.cc util/gzip_internal.cc util/location_util.cc + util/log_sanitizer.cc util/murmurhash3_internal.cc util/property_util.cc util/retry_util.cc diff --git a/src/iceberg/catalog/rest/auth/auth_pinned_keys.cc b/src/iceberg/catalog/rest/auth/auth_pinned_keys.cc new file mode 100644 index 0000000000..5617e971c3 --- /dev/null +++ b/src/iceberg/catalog/rest/auth/auth_pinned_keys.cc @@ -0,0 +1,86 @@ +/* + * 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/auth_pinned_keys.h" + +#include "iceberg/logging/log_macros.h" + +namespace iceberg { + +const std::unordered_set& AuthPinnedKeys::GetPinnedKeys() { + // Static set of auth-pinned keys that must be client-controlled. + // This is an exact, exhaustive list with no pattern matching. + // + // These keys control authentication routing and credentials. + // Allowing a server to override them would enable attacks like: + // - Redirecting OAuth2 credentials to an attacker-controlled endpoint + // - Replacing client credentials with attacker-controlled values + // + // Keys NOT in this list (safe for server override): + // - token-refresh-enabled, token-exchange-enabled (boolean flags) + // - scope, audience, resource (OAuth2 parameters, not endpoints) + // - rest.access-key-id, s3.access-key-id (public identifiers) + static const std::unordered_set kPinnedKeys = { + // OAuth2 endpoint routing - redirecting this enables credential theft + "oauth2-server-uri", + + // OAuth2 credentials - these ARE the secrets + "token", // Bearer token + "credential", // OAuth2 client_id:client_secret + + // Basic Authentication credentials + "rest.auth.basic.username", + "rest.auth.basic.password", + + // AWS SigV4 secrets (note: rest.access-key-id is NOT pinned - it's public) + "rest.secret-access-key", + "rest.session-token", + }; + return kPinnedKeys; +} + +bool AuthPinnedKeys::IsPinned(std::string_view key) { + const auto& pinned = GetPinnedKeys(); + return pinned.contains(std::string(key)); +} + +std::unordered_map AuthPinnedKeys::FilterServerConfig( + const std::unordered_map& server_config, + bool log_filtered) { + std::unordered_map filtered; + filtered.reserve(server_config.size()); + + for (const auto& [key, value] : server_config) { + if (IsPinned(key)) { + if (log_filtered) { + ICEBERG_LOG_WARN( + "Ignoring auth-pinned configuration key '{}' from server config; " + "this property must be resolved from client configuration only " + "(CVE-2024-XXXXX: server config override attack prevention)", + key); + } + } else { + filtered[key] = value; + } + } + + return filtered; +} + +} // namespace iceberg diff --git a/src/iceberg/catalog/rest/auth/auth_pinned_keys.h b/src/iceberg/catalog/rest/auth/auth_pinned_keys.h new file mode 100644 index 0000000000..ecaa8fbe68 --- /dev/null +++ b/src/iceberg/catalog/rest/auth/auth_pinned_keys.h @@ -0,0 +1,105 @@ +/* + * 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 +#include +#include +#include + +#include "iceberg/iceberg_export.h" + +namespace iceberg { + +/// \brief Utility for managing auth-pinned configuration keys. +/// +/// Auth-pinned keys are configuration properties that control authentication +/// routing and credentials. These keys MUST be resolved from client-side +/// configuration only and MUST NOT be overridden by server-provided config +/// responses, as doing so would allow a compromised or malicious server to +/// redirect authentication credentials to an attacker-controlled endpoint. +/// +/// This class provides filtering for server-provided configuration (e.g., +/// REST catalog ConfigResponse overrides) to prevent CVE-2024-XXXXX: OAuth2 +/// credential theft via server config override attack. +/// +/// ## What is NOT pinned +/// +/// The following keys are safe for servers to override and are NOT pinned: +/// - `token-refresh-enabled`, `token-exchange-enabled` (boolean flags) +/// - `scope`, `audience`, `resource` (OAuth2 scope parameters) +/// - `rest.access-key-id`, `s3.access-key-id` (public identifiers, not secrets) +/// +/// ## Example usage +/// +/// \code +/// // Filter server overrides before merging into client config +/// auto safe_overrides = AuthPinnedKeys::FilterServerConfig( +/// server_config.overrides); +/// +/// RestCatalogProperties final_config = RestCatalogProperties::FromMap( +/// MergeConfigs(client_config, safe_overrides)); +/// \endcode +class ICEBERG_EXPORT AuthPinnedKeys { + public: + /// \brief Check if a configuration key is auth-pinned. + /// \param key The configuration key to check + /// \return true if the key must be client-controlled, false otherwise + /// + /// Auth-pinned keys include: + /// - OAuth2 endpoint: `oauth2-server-uri` + /// - OAuth2 credentials: `token`, `credential` + /// - Basic Auth: `rest.auth.basic.username`, `rest.auth.basic.password` + /// - SigV4 secrets: `rest.secret-access-key`, `rest.session-token` + static bool IsPinned(std::string_view key); + + /// \brief Filter auth-pinned keys from server-provided configuration. + /// \param server_config The server configuration map to filter + /// \param log_filtered If true, log a warning for each filtered key + /// \return A new map with auth-pinned keys removed + /// + /// This method is intended for filtering ConfigResponse.defaults and + /// ConfigResponse.overrides before merging them into client configuration. + /// Filtered keys are removed to prevent server-side override attacks. + /// + /// Example: + /// \code + /// // Prevent f009: OAuth2 credential theft via server config override + /// auto safe_defaults = AuthPinnedKeys::FilterServerConfig( + /// server_config.defaults); + /// auto safe_overrides = AuthPinnedKeys::FilterServerConfig( + /// server_config.overrides); + /// + /// RestCatalogProperties final_config = RestCatalogProperties::FromMap( + /// MergeConfigs(safe_defaults, client_config, safe_overrides)); + /// \endcode + static std::unordered_map FilterServerConfig( + const std::unordered_map& server_config, + bool log_filtered = true); + + private: + /// \brief Get the set of auth-pinned keys. + /// + /// This is an exact list with no pattern matching. New auth-related + /// properties that control credential routing should be added here. + static const std::unordered_set& GetPinnedKeys(); +}; + +} // namespace iceberg diff --git a/src/iceberg/catalog/rest/rest_catalog.cc b/src/iceberg/catalog/rest/rest_catalog.cc index 0ca89a725a..f43ab18b82 100644 --- a/src/iceberg/catalog/rest/rest_catalog.cc +++ b/src/iceberg/catalog/rest/rest_catalog.cc @@ -31,6 +31,7 @@ #include #include "iceberg/catalog/rest/auth/auth_managers.h" +#include "iceberg/catalog/rest/auth/auth_pinned_keys.h" #include "iceberg/catalog/rest/auth/auth_session.h" #include "iceberg/catalog/rest/catalog_properties.h" #include "iceberg/catalog/rest/constant.h" @@ -443,8 +444,12 @@ Result> RestCatalog::Make( ICEBERG_ASSIGN_OR_RAISE(auto server_config, FetchServerConfig(*paths, config, *init_session)); - RestCatalogProperties final_config = RestCatalogProperties::FromMap( - MergeConfigs(server_config.defaults, config.configs(), server_config.overrides)); + // Filter sensitive properties from server-provided configuration to prevent + // compromised catalog from redirecting OAuth2 credentials or overriding + // other authentication-sensitive settings. + RestCatalogProperties final_config = RestCatalogProperties::FromMap(MergeConfigs( + AuthPinnedKeys::FilterServerConfig(server_config.defaults), config.configs(), + AuthPinnedKeys::FilterServerConfig(server_config.overrides))); std::unordered_set endpoints; if (!server_config.endpoints.empty()) { diff --git a/src/iceberg/cpp20_compatibility_internal.h b/src/iceberg/cpp20_compatibility_internal.h index b5f46e488d..9b990a66d4 100644 --- a/src/iceberg/cpp20_compatibility_internal.h +++ b/src/iceberg/cpp20_compatibility_internal.h @@ -181,6 +181,7 @@ #include "iceberg/util/int128.h" #include "iceberg/util/lazy.h" #include "iceberg/util/location_util.h" +#include "iceberg/util/log_sanitizer.h" #include "iceberg/util/macros.h" #include "iceberg/util/partition_value_util.h" #include "iceberg/util/property_util.h" @@ -226,6 +227,7 @@ #ifdef ICEBERG_PUBLIC_HEADERS_REST # include "iceberg/catalog/rest/auth/auth_manager.h" # include "iceberg/catalog/rest/auth/auth_managers.h" +# include "iceberg/catalog/rest/auth/auth_pinned_keys.h" # include "iceberg/catalog/rest/auth/auth_properties.h" # include "iceberg/catalog/rest/auth/auth_session.h" # include "iceberg/catalog/rest/auth/oauth2_util.h" diff --git a/src/iceberg/json_serde.cc b/src/iceberg/json_serde.cc index c1c3ffb73f..994b6442b3 100644 --- a/src/iceberg/json_serde.cc +++ b/src/iceberg/json_serde.cc @@ -1691,7 +1691,9 @@ Result FromJsonString(const std::string& json_string) { auto json = nlohmann::json::parse(json_string, /*cb=*/nullptr, /*allow_exceptions=*/false); if (json.is_discarded()) [[unlikely]] { - return JsonParseError("Failed to parse JSON string: {}", json_string); + // Do not echo the raw body: it may be a catalog or token-endpoint response + // carrying credential material, and error messages routinely get logged. + return JsonParseError("Failed to parse JSON string of {} bytes", json_string.size()); } return json; } diff --git a/src/iceberg/test/CMakeLists.txt b/src/iceberg/test/CMakeLists.txt index 058a9dd3cc..6c95f535f6 100644 --- a/src/iceberg/test/CMakeLists.txt +++ b/src/iceberg/test/CMakeLists.txt @@ -125,6 +125,7 @@ add_iceberg_test(json_serde_test add_iceberg_test(util_test USE_DATA SOURCES + auth_pinned_keys_test.cc base64_test.cc bucket_util_test.cc cache_test.cc @@ -133,20 +134,21 @@ add_iceberg_test(util_test data_file_set_test.cc decimal_test.cc endian_test.cc + executor_util_test.cc expected_test.cc file_io_test.cc formatter_test.cc - stream_test.cc lazy_test.cc location_util_test.cc + log_sanitizer_test.cc math_util_internal_test.cc - executor_util_test.cc - roaring_position_bitmap_test.cc position_delete_index_test.cc position_delete_range_consumer_test.cc property_util_test.cc resolving_file_io_test.cc retry_util_test.cc + roaring_position_bitmap_test.cc + stream_test.cc string_util_test.cc struct_like_set_test.cc task_group_test.cc @@ -321,8 +323,9 @@ if(ICEBERG_BUILD_REST) add_rest_iceberg_test(rest_catalog_test SOURCES auth_manager_test.cc - error_handlers_test.cc endpoint_test.cc + error_handlers_test.cc + rest_catalog_security_test.cc rest_file_io_test.cc rest_json_serde_test.cc rest_metrics_reporter_test.cc diff --git a/src/iceberg/test/auth_pinned_keys_test.cc b/src/iceberg/test/auth_pinned_keys_test.cc new file mode 100644 index 0000000000..6a94884b1d --- /dev/null +++ b/src/iceberg/test/auth_pinned_keys_test.cc @@ -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. + */ + +#include "iceberg/catalog/rest/auth/auth_pinned_keys.h" + +#include +#include + +#include + +namespace iceberg { + +TEST(AuthPinnedKeysTest, IsPinned_OAuth2Keys) { + // OAuth2 endpoint and credentials are pinned + EXPECT_TRUE(AuthPinnedKeys::IsPinned("oauth2-server-uri")); + EXPECT_TRUE(AuthPinnedKeys::IsPinned("token")); + EXPECT_TRUE(AuthPinnedKeys::IsPinned("credential")); +} + +TEST(AuthPinnedKeysTest, IsPinned_BasicAuthKeys) { + // Basic Auth credentials are pinned + EXPECT_TRUE(AuthPinnedKeys::IsPinned("rest.auth.basic.username")); + EXPECT_TRUE(AuthPinnedKeys::IsPinned("rest.auth.basic.password")); +} + +TEST(AuthPinnedKeysTest, IsPinned_SigV4Secrets) { + // SigV4 secrets are pinned + EXPECT_TRUE(AuthPinnedKeys::IsPinned("rest.secret-access-key")); + EXPECT_TRUE(AuthPinnedKeys::IsPinned("rest.session-token")); + + // But access-key-id is NOT pinned (it's a public identifier) + EXPECT_FALSE(AuthPinnedKeys::IsPinned("rest.access-key-id")); +} + +TEST(AuthPinnedKeysTest, IsPinned_NonPinnedKeys) { + // These are safe for server to override - NOT pinned + EXPECT_FALSE(AuthPinnedKeys::IsPinned("uri")); + EXPECT_FALSE(AuthPinnedKeys::IsPinned("warehouse")); + EXPECT_FALSE(AuthPinnedKeys::IsPinned("token-refresh-enabled")); + EXPECT_FALSE(AuthPinnedKeys::IsPinned("token-exchange-enabled")); + EXPECT_FALSE(AuthPinnedKeys::IsPinned("scope")); + EXPECT_FALSE(AuthPinnedKeys::IsPinned("audience")); + EXPECT_FALSE(AuthPinnedKeys::IsPinned("resource")); + EXPECT_FALSE(AuthPinnedKeys::IsPinned("catalog-impl")); +} + +TEST(AuthPinnedKeysTest, FilterServerConfig_RemovesPinnedKeys) { + std::unordered_map server_config = { + {"uri", "https://catalog.example.com"}, + {"oauth2-server-uri", "https://attacker.com/oauth"}, // ❌ pinned + {"credential", "stolen:secret"}, // ❌ pinned + {"token", "malicious-token"}, // ❌ pinned + {"warehouse", "s3://bucket/warehouse"}, // ✅ not pinned + {"token-refresh-enabled", "false"}, // ✅ not pinned + {"scope", "catalog"}, // ✅ not pinned + }; + + auto filtered = AuthPinnedKeys::FilterServerConfig(server_config, false); + + // Pinned keys removed + EXPECT_EQ(filtered.count("oauth2-server-uri"), 0); + EXPECT_EQ(filtered.count("credential"), 0); + EXPECT_EQ(filtered.count("token"), 0); + + // Non-pinned keys retained + EXPECT_EQ(filtered.count("uri"), 1); + EXPECT_EQ(filtered["uri"], "https://catalog.example.com"); + EXPECT_EQ(filtered.count("warehouse"), 1); + EXPECT_EQ(filtered["warehouse"], "s3://bucket/warehouse"); + EXPECT_EQ(filtered.count("token-refresh-enabled"), 1); + EXPECT_EQ(filtered["token-refresh-enabled"], "false"); + EXPECT_EQ(filtered.count("scope"), 1); + EXPECT_EQ(filtered["scope"], "catalog"); +} + +TEST(AuthPinnedKeysTest, FilterServerConfig_EmptyMap) { + std::unordered_map empty; + auto filtered = AuthPinnedKeys::FilterServerConfig(empty, false); + EXPECT_TRUE(filtered.empty()); +} + +TEST(AuthPinnedKeysTest, FilterServerConfig_AllPinned) { + std::unordered_map all_pinned = { + {"oauth2-server-uri", "https://attacker.com"}, + {"credential", "client:secret"}, + {"token", "bearer-token"}, + }; + + auto filtered = AuthPinnedKeys::FilterServerConfig(all_pinned, false); + EXPECT_TRUE(filtered.empty()); +} + +TEST(AuthPinnedKeysTest, FilterServerConfig_NonePinned) { + std::unordered_map none_pinned = { + {"uri", "https://example.com"}, + {"warehouse", "s3://bucket/wh"}, + {"token-refresh-enabled", "true"}, + {"scope", "catalog"}, + }; + + auto filtered = AuthPinnedKeys::FilterServerConfig(none_pinned, false); + EXPECT_EQ(filtered.size(), 4); + EXPECT_EQ(filtered["uri"], "https://example.com"); + EXPECT_EQ(filtered["warehouse"], "s3://bucket/wh"); + EXPECT_EQ(filtered["token-refresh-enabled"], "true"); + EXPECT_EQ(filtered["scope"], "catalog"); +} + +TEST(AuthPinnedKeysTest, CaseSensitivity) { + // Keys are case-sensitive - exact match only + EXPECT_TRUE(AuthPinnedKeys::IsPinned("credential")); + EXPECT_FALSE(AuthPinnedKeys::IsPinned("CREDENTIAL")); + EXPECT_FALSE(AuthPinnedKeys::IsPinned("Credential")); +} + +} // namespace iceberg diff --git a/src/iceberg/test/log_sanitizer_test.cc b/src/iceberg/test/log_sanitizer_test.cc new file mode 100644 index 0000000000..35235f3d1d --- /dev/null +++ b/src/iceberg/test/log_sanitizer_test.cc @@ -0,0 +1,177 @@ +/* + * 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/util/log_sanitizer.h" + +#include +#include + +#include + +namespace iceberg { + +TEST(LogSanitizerTest, IsSecret_OAuth2Keys) { + // OAuth2 credentials + EXPECT_TRUE(LogSanitizer::IsSecret("oauth2-server-uri")); + EXPECT_TRUE(LogSanitizer::IsSecret("credential")); + EXPECT_TRUE(LogSanitizer::IsSecret("token")); +} + +TEST(LogSanitizerTest, IsSecret_StorageCredentials) { + // S3 + EXPECT_TRUE(LogSanitizer::IsSecret("s3.secret-access-key")); + EXPECT_TRUE(LogSanitizer::IsSecret("s3.session-token")); + EXPECT_FALSE(LogSanitizer::IsSecret("s3.access-key-id")); // Identifier + + // GCS + EXPECT_TRUE(LogSanitizer::IsSecret("gcs.oauth2.token")); + EXPECT_TRUE(LogSanitizer::IsSecret("gcs.oauth2.client-secret")); + + // Azure + EXPECT_TRUE(LogSanitizer::IsSecret("adls.sas-token")); + EXPECT_TRUE(LogSanitizer::IsSecret("adls.account.key")); +} + +TEST(LogSanitizerTest, IsSecret_DatabasePasswords) { + EXPECT_TRUE(LogSanitizer::IsSecret("jdbc.password")); + EXPECT_TRUE(LogSanitizer::IsSecret("connection.password")); +} + +TEST(LogSanitizerTest, IsSecret_PatternMatching) { + // Pattern matching for unknown keys + EXPECT_TRUE(LogSanitizer::IsSecret("my-secret")); + EXPECT_TRUE(LogSanitizer::IsSecret("api-key")); + EXPECT_TRUE(LogSanitizer::IsSecret("auth-token")); + EXPECT_TRUE(LogSanitizer::IsSecret("custom-password")); + EXPECT_TRUE(LogSanitizer::IsSecret("service-credential")); + EXPECT_TRUE(LogSanitizer::IsSecret("my-private-key")); +} + +TEST(LogSanitizerTest, IsSecret_IdentifierExclusion) { + // Keys ending in "-id" or "id" are NOT secrets (they are identifiers) + EXPECT_FALSE(LogSanitizer::IsSecret("s3.access-key-id")); + EXPECT_FALSE(LogSanitizer::IsSecret("rest.access-key-id")); + EXPECT_FALSE(LogSanitizer::IsSecret("client-id")); + EXPECT_FALSE(LogSanitizer::IsSecret("user-id")); + EXPECT_FALSE(LogSanitizer::IsSecret("session-id")); +} + +TEST(LogSanitizerTest, IsSecret_NonSensitiveKeys) { + EXPECT_FALSE(LogSanitizer::IsSecret("uri")); + EXPECT_FALSE(LogSanitizer::IsSecret("warehouse")); + EXPECT_FALSE(LogSanitizer::IsSecret("s3.endpoint")); + EXPECT_FALSE(LogSanitizer::IsSecret("catalog-impl")); + EXPECT_FALSE(LogSanitizer::IsSecret("scope")); + + // Note: token-refresh-enabled contains "token" so it IS redacted by the + // conservative pattern matching. This is acceptable: the cost of over-redaction + // in logs is low (slightly less useful logs), while the cost of under-redaction + // is high (credential leakage). The value is just a boolean anyway. + EXPECT_TRUE(LogSanitizer::IsSecret("token-refresh-enabled")); +} + +TEST(LogSanitizerTest, Redact_SensitiveKeys) { + // Sensitive keys get redacted + EXPECT_EQ(LogSanitizer::Redact("credential", "my-secret"), "(redacted)"); + EXPECT_EQ(LogSanitizer::Redact("token", "abc123"), "(redacted)"); + EXPECT_EQ(LogSanitizer::Redact("s3.secret-access-key", "SECRET"), "(redacted)"); + EXPECT_EQ(LogSanitizer::Redact("password", "pass123"), "(redacted)"); + EXPECT_EQ(LogSanitizer::Redact("my-api-key", "key123"), "(redacted)"); +} + +TEST(LogSanitizerTest, Redact_NonSensitiveKeys) { + // Non-sensitive keys pass through + EXPECT_EQ(LogSanitizer::Redact("uri", "https://example.com"), "https://example.com"); + EXPECT_EQ(LogSanitizer::Redact("warehouse", "s3://bucket/wh"), "s3://bucket/wh"); + EXPECT_EQ(LogSanitizer::Redact("catalog-impl", "rest"), "rest"); + EXPECT_EQ(LogSanitizer::Redact("s3.access-key-id", "AKIAIO"), "AKIAIO"); +} + +TEST(LogSanitizerTest, Redact_EmptyValue) { + EXPECT_EQ(LogSanitizer::Redact("credential", ""), "(redacted)"); + EXPECT_EQ(LogSanitizer::Redact("uri", ""), ""); +} + +TEST(LogSanitizerTest, RedactMap) { + std::unordered_map config = { + {"uri", "https://example.com"}, {"credential", "my-secret"}, + {"warehouse", "s3://bucket/wh"}, {"s3.secret-access-key", "SECRET123"}, + {"s3.access-key-id", "AKIAIO"}, {"token", "bearer-token"}, + }; + + auto redacted = LogSanitizer::RedactMap(config); + + // Non-sensitive values retained + EXPECT_EQ(redacted["uri"], "https://example.com"); + EXPECT_EQ(redacted["warehouse"], "s3://bucket/wh"); + EXPECT_EQ(redacted["s3.access-key-id"], "AKIAIO"); + + // Sensitive values redacted + EXPECT_EQ(redacted["credential"], "(redacted)"); + EXPECT_EQ(redacted["s3.secret-access-key"], "(redacted)"); + EXPECT_EQ(redacted["token"], "(redacted)"); +} + +TEST(LogSanitizerTest, RedactMap_EmptyMap) { + std::unordered_map empty; + auto redacted = LogSanitizer::RedactMap(empty); + EXPECT_TRUE(redacted.empty()); +} + +TEST(LogSanitizerTest, RedactMap_AllSensitive) { + std::unordered_map all_sensitive = { + {"credential", "secret1"}, + {"token", "secret2"}, + {"password", "secret3"}, + }; + + auto redacted = LogSanitizer::RedactMap(all_sensitive); + EXPECT_EQ(redacted.size(), 3); + EXPECT_EQ(redacted["credential"], "(redacted)"); + EXPECT_EQ(redacted["token"], "(redacted)"); + EXPECT_EQ(redacted["password"], "(redacted)"); +} + +TEST(LogSanitizerTest, RedactMap_NoneSensitive) { + std::unordered_map none_sensitive = { + {"uri", "https://example.com"}, + {"warehouse", "s3://bucket/wh"}, + {"catalog-impl", "rest"}, + }; + + auto redacted = LogSanitizer::RedactMap(none_sensitive); + EXPECT_EQ(redacted.size(), 3); + EXPECT_EQ(redacted["uri"], "https://example.com"); + EXPECT_EQ(redacted["warehouse"], "s3://bucket/wh"); + EXPECT_EQ(redacted["catalog-impl"], "rest"); +} + +TEST(LogSanitizerTest, CaseSensitivity) { + // Exact match is case-sensitive + EXPECT_TRUE(LogSanitizer::IsSecret("credential")); + EXPECT_FALSE( + LogSanitizer::IsSecret("CREDENTIAL")); // Pattern won't match (case variation) + + // Pattern matching is case-insensitive + EXPECT_TRUE(LogSanitizer::IsSecret("my-secret")); + EXPECT_TRUE(LogSanitizer::IsSecret("my-SECRET")); + EXPECT_TRUE(LogSanitizer::IsSecret("my-Secret")); +} + +} // namespace iceberg diff --git a/src/iceberg/test/rest_catalog_security_test.cc b/src/iceberg/test/rest_catalog_security_test.cc new file mode 100644 index 0000000000..cee14a4522 --- /dev/null +++ b/src/iceberg/test/rest_catalog_security_test.cc @@ -0,0 +1,131 @@ +// 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 + +#include "iceberg/catalog/rest/auth/auth_pinned_keys.h" +#include "iceberg/util/log_sanitizer.h" + +namespace iceberg { + +// Test suite for REST Catalog security fixes +// Covers f009, f010 vulnerabilities + +class RestCatalogSecurityTest : public ::testing::Test { + protected: + void SetUp() override {} + void TearDown() override {} +}; + +// f009: OAuth2 client secret redirect vulnerability +// Server config overrides should NOT allow sensitive auth properties +TEST_F(RestCatalogSecurityTest, OAuth2RedirectProtection) { + // Simulates server-provided config that tries to override OAuth2 settings + std::unordered_map server_config = { + {"warehouse", "s3://my-warehouse/"}, + {"oauth2-server-uri", "https://attacker.com/oauth"}, // MALICIOUS + {"credential", "attacker:stolen-secret"}, // MALICIOUS + {"token", "malicious-bearer-token"}, // MALICIOUS + {"s3.region", "us-west-2"}, // SAFE + {"token-refresh-enabled", "false"}, // SAFE (not pinned) + }; + + // Filter out auth-pinned keys + auto safe_config = AuthPinnedKeys::FilterServerConfig(server_config, false); + + // Auth-pinned keys should be filtered out + EXPECT_EQ(safe_config.count("oauth2-server-uri"), 0); + EXPECT_EQ(safe_config.count("credential"), 0); + EXPECT_EQ(safe_config.count("token"), 0); + + // Safe keys should be preserved + EXPECT_EQ(safe_config.count("warehouse"), 1); + EXPECT_EQ(safe_config.count("s3.region"), 1); + EXPECT_EQ(safe_config.count("token-refresh-enabled"), 1); // Not pinned! + EXPECT_EQ(safe_config["warehouse"], "s3://my-warehouse/"); + EXPECT_EQ(safe_config["token-refresh-enabled"], "false"); +} + +// f010: Credentials dumped in REST parse errors +// Error messages should redact sensitive values +TEST_F(RestCatalogSecurityTest, CredentialRedactionInErrors) { + // Simulates config with mixed sensitive and non-sensitive keys + std::unordered_map config = { + {"warehouse", "s3://my-warehouse/"}, + {"s3.access-key-id", "AKIAIOSFODNN7EXAMPLE"}, // ID is not sensitive + {"s3.secret-access-key", "wJalrXUtnFEMI/K7MDENG"}, // SECRET is sensitive + {"s3.session-token", "FwoGZXIvYXdzEBYaD..."}, // TOKEN is sensitive + }; + + // Build a safe error message + std::string error_msg = "Failed to parse config: "; + for (const auto& [key, value] : config) { + error_msg += key + "=" + LogSanitizer::Redact(key, value) + " "; + } + + // Verify that sensitive values are redacted + EXPECT_NE(error_msg.find("s3.access-key-id=AKIAIOSFODNN7EXAMPLE"), std::string::npos); + EXPECT_NE(error_msg.find("s3.secret-access-key=(redacted)"), std::string::npos); + EXPECT_NE(error_msg.find("s3.session-token=(redacted)"), std::string::npos); + + // Verify that actual secrets are NOT in the error message + EXPECT_EQ(error_msg.find("wJalrXUtnFEMI"), std::string::npos); + EXPECT_EQ(error_msg.find("FwoGZXIv"), std::string::npos); +} + +// Verify that authentication-related keys are identified as secrets +TEST_F(RestCatalogSecurityTest, BearerTokenIdentification) { + // Auth-related keys that should be redacted in logs + EXPECT_TRUE(LogSanitizer::IsSecret("token")); + EXPECT_TRUE(LogSanitizer::IsSecret("credential")); + EXPECT_TRUE(LogSanitizer::IsSecret("bearer-token")); + EXPECT_TRUE(LogSanitizer::IsSecret("my-api-token")); + + // Non-auth keys + EXPECT_FALSE(LogSanitizer::IsSecret("warehouse")); + EXPECT_FALSE(LogSanitizer::IsSecret("catalog-impl")); + EXPECT_FALSE(LogSanitizer::IsSecret("uri")); +} + +// Additional test: Verify SigV4 credentials are protected +TEST_F(RestCatalogSecurityTest, SigV4CredentialProtection) { + std::unordered_map server_config = { + {"rest.access-key-id", "AKIAIO"}, // NOT pinned (identifier) + {"rest.secret-access-key", "SECRET123"}, // PINNED + {"rest.session-token", "TOKEN456"}, // PINNED + {"rest.signing-region", "us-west-2"}, // NOT pinned + }; + + // Filter server config + auto safe_config = AuthPinnedKeys::FilterServerConfig(server_config, false); + + // Identifiers and non-auth keys preserved + EXPECT_EQ(safe_config.count("rest.access-key-id"), 1); + EXPECT_EQ(safe_config["rest.access-key-id"], "AKIAIO"); + EXPECT_EQ(safe_config.count("rest.signing-region"), 1); + + // Secrets filtered out + EXPECT_EQ(safe_config.count("rest.secret-access-key"), 0); + EXPECT_EQ(safe_config.count("rest.session-token"), 0); + + // But they should be redacted in logs + EXPECT_EQ(LogSanitizer::Redact("rest.access-key-id", "AKIAIO"), "AKIAIO"); + EXPECT_EQ(LogSanitizer::Redact("rest.secret-access-key", "SECRET123"), "(redacted)"); + EXPECT_EQ(LogSanitizer::Redact("rest.session-token", "TOKEN456"), "(redacted)"); +} + +} // namespace iceberg diff --git a/src/iceberg/util/json_util_internal.h b/src/iceberg/util/json_util_internal.h index b1edf583d9..def8203f67 100644 --- a/src/iceberg/util/json_util_internal.h +++ b/src/iceberg/util/json_util_internal.h @@ -57,6 +57,13 @@ inline void SetOptionalStringField(nlohmann::json& json, std::string_view key, } } +/// \brief Dump a JSON document to a string, ignoring encoding errors. +/// +/// Do NOT interpolate the result into error messages for documents that may +/// carry secrets (REST catalog responses vend storage credentials and OAuth +/// tokens): error messages routinely end up in application logs, an audience +/// never trusted with that material. Parse errors must describe the offending +/// key and JSON type instead of reproducing the document. inline std::string SafeDumpJson(const nlohmann::json& json) { return json.dump(/*indent=*/-1, /*indent_char=*/' ', /*ensure_ascii=*/false, nlohmann::detail::error_handler_t::ignore); @@ -67,7 +74,8 @@ Result GetTypedJsonValue(const nlohmann::json& json) { try { return json.get(); } catch (const std::exception& ex) { - return JsonParseError("Failed to parse {}: {}", SafeDumpJson(json), ex.what()); + return JsonParseError("Failed to parse JSON value of type {}: {}", json.type_name(), + ex.what()); } } @@ -76,7 +84,10 @@ Result GetJsonValueImpl(const nlohmann::json& json, std::string_view key) { try { return json.at(key).get(); } catch (const std::exception& ex) { - return JsonParseError("Failed to parse '{}' from {}: {}", key, SafeDumpJson(json), + // Report the key and the JSON type found -- never the enclosing document, + // which may contain sibling fields with credential material. + return JsonParseError("Failed to parse '{}' (JSON value of type {}): {}", key, + json.contains(key) ? json.at(key).type_name() : "missing", ex.what()); } } @@ -94,7 +105,7 @@ Result> GetJsonValueOptional(const nlohmann::json& json, template Result GetJsonValue(const nlohmann::json& json, std::string_view key) { if (!json.contains(key) || json.at(key).is_null()) { - return JsonParseError("Missing '{}' in {}", key, SafeDumpJson(json)); + return JsonParseError("Missing '{}' in JSON object", key); } return GetJsonValueImpl(json, key); } @@ -225,8 +236,8 @@ Result> FromJsonList( if (json.contains(key)) { ICEBERG_ASSIGN_OR_RAISE(auto list_json, GetJsonValue(json, key)); if (!list_json.is_array()) { - return JsonParseError("Cannot parse '{}' from non-array: {}", key, - SafeDumpJson(list_json)); + return JsonParseError("Cannot parse '{}' from non-array JSON value of type {}", key, + list_json.type_name()); } for (const auto& entry_json : list_json) { ICEBERG_ASSIGN_OR_RAISE(auto entry, from_json(entry_json)); @@ -250,8 +261,8 @@ Result>> FromJsonList( if (json.contains(key)) { ICEBERG_ASSIGN_OR_RAISE(auto list_json, GetJsonValue(json, key)); if (!list_json.is_array()) { - return JsonParseError("Cannot parse '{}' from non-array: {}", key, - SafeDumpJson(list_json)); + return JsonParseError("Cannot parse '{}' from non-array JSON value of type {}", key, + list_json.type_name()); } for (const auto& entry_json : list_json) { ICEBERG_ASSIGN_OR_RAISE(auto entry, from_json(entry_json)); @@ -300,16 +311,16 @@ Result> FromJsonMap( try { return json.get(); } catch (const std::exception& ex) { - return JsonParseError("Cannot parse {} to a string value: {}", SafeDumpJson(json), - ex.what()); + return JsonParseError("Cannot parse JSON value of type {} to a string value: {}", + json.type_name(), ex.what()); } }) { std::unordered_map map{}; if (json.contains(key)) { ICEBERG_ASSIGN_OR_RAISE(auto map_json, GetJsonValue(json, key)); if (!map_json.is_object()) { - return JsonParseError("Cannot parse '{}' from non-object: {}", key, - SafeDumpJson(map_json)); + return JsonParseError("Cannot parse '{}' from non-object JSON value of type {}", + key, map_json.type_name()); } for (const auto& [key, value] : map_json.items()) { ICEBERG_ASSIGN_OR_RAISE(auto entry, from_json(value)); diff --git a/src/iceberg/util/log_sanitizer.cc b/src/iceberg/util/log_sanitizer.cc new file mode 100644 index 0000000000..3be14d7213 --- /dev/null +++ b/src/iceberg/util/log_sanitizer.cc @@ -0,0 +1,142 @@ +/* + * 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/util/log_sanitizer.h" + +#include +#include + +namespace iceberg { + +const std::unordered_set& LogSanitizer::GetSecretKeys() { + // Static set of known secret keys. + // This list includes all keys that are known to hold sensitive values + // (credentials, tokens, passwords, secret keys). + // + // Pattern matching provides additional coverage for unknown keys, so this + // list does not need to be exhaustive, but adding known keys here improves + // precision and avoids reliance on pattern matching alone. + static const std::unordered_set kSecretKeys = { + // OAuth2 / Bearer token + "token", + "credential", + "oauth2-server-uri", // Sensitive: reveals where credentials are sent + + // Basic Authentication + "rest.auth.basic.username", + "rest.auth.basic.password", + + // AWS SigV4 (note: rest.access-key-id is NOT secret - it's an identifier) + "rest.secret-access-key", + "rest.session-token", + + // S3 Storage Credentials (from credential vending) + // Note: s3.access-key-id is NOT secret + "s3.secret-access-key", + "s3.session-token", + + // GCS Credentials + "gcs.oauth2.token", + "gcs.oauth2.client-secret", + + // Azure Credentials + "adls.sas-token", + "adls.connection-string", + "adls.account.key", + + // Database Passwords (SQL Catalog) + "jdbc.password", + "connection.password", + }; + return kSecretKeys; +} + +bool LogSanitizer::MatchesSecretPattern(std::string_view key) { + // Convert to lowercase for case-insensitive pattern matching + std::string lower_key(key); + std::transform(lower_key.begin(), lower_key.end(), lower_key.begin(), + [](unsigned char c) { return std::tolower(c); }); + + // Exclude keys ending with "-id" or "id" - these are identifiers, not secrets + // Examples: s3.access-key-id, rest.access-key-id, client-id + if (lower_key.ends_with("-id") || lower_key.ends_with("id")) { + return false; + } + + // Check if this is a case variation of a known exact key + // If so, don't pattern-match it (the exact match already handled it) + const auto& known_keys = GetSecretKeys(); + for (const auto& known : known_keys) { + std::string lower_known = known; + std::transform(lower_known.begin(), lower_known.end(), lower_known.begin(), + [](unsigned char c) { return std::tolower(c); }); + if (lower_key == lower_known) { + // This is a case variation of a known key + // The exact match already failed, so return false here + return false; + } + } + + // Common sensitive patterns (case-insensitive substring match) + const std::string_view patterns[] = { + "password", "secret", "token", "credential", + "private-key", "privatekey", "api-key", "apikey", + }; + + for (const auto& pattern : patterns) { + if (lower_key.find(pattern) != std::string::npos) { + return true; + } + } + + return false; +} + +bool LogSanitizer::IsSecret(std::string_view key) { + // 1. Exact match (case-sensitive) - primary check + const auto& keys = GetSecretKeys(); + if (keys.contains(std::string(key))) { + return true; + } + + // 2. Pattern matching as fallback for unknown keys + // This is intentionally conservative: false positives are acceptable in logs + return MatchesSecretPattern(key); +} + +std::string LogSanitizer::Redact(std::string_view key, std::string_view value) { + if (IsSecret(key)) { + return "(redacted)"; + } + return std::string(value); +} + +std::unordered_map LogSanitizer::RedactMap( + const std::unordered_map& map) { + std::unordered_map redacted; + redacted.reserve(map.size()); + + for (const auto& [key, value] : map) { + redacted[key] = Redact(key, value); + } + + return redacted; +} + +} // namespace iceberg diff --git a/src/iceberg/util/log_sanitizer.h b/src/iceberg/util/log_sanitizer.h new file mode 100644 index 0000000000..bb79fd00d9 --- /dev/null +++ b/src/iceberg/util/log_sanitizer.h @@ -0,0 +1,122 @@ +/* + * 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 +#include +#include +#include + +#include "iceberg/iceberg_export.h" + +namespace iceberg { + +/// \brief Utility for sanitizing sensitive values in logs and error messages. +/// +/// This class provides redaction of sensitive configuration values to prevent +/// accidental credential disclosure through logging, error messages, and debug +/// output (CVE-2024-XXXXX: credential leakage via error messages). +/// +/// Unlike `AuthPinnedKeys` (which prevents server config override attacks), +/// `LogSanitizer` is conservative and tolerates false positives: it's better +/// to redact a non-sensitive value in a log than to leak credentials. +/// +/// ## Design +/// +/// - **Exact match** (primary): Known secret keys are in an explicit list +/// - **Pattern match** (fallback): Unknown keys matching common patterns +/// (e.g., containing "password", "secret", "token") are redacted +/// - **Identifier exclusion**: Keys ending in "-id" or "id" are NOT redacted +/// (e.g., `s3.access-key-id`, `client-id` are public identifiers) +/// +/// ## Example usage +/// +/// \code +/// // Single value redaction +/// LOG_DEBUG("Config: {}={}", key, +/// LogSanitizer::Redact(key, value)); +/// +/// // Whole map redaction +/// auto safe_config = LogSanitizer::RedactMap(config); +/// LOG_INFO("Merged config: {}", fmt::join(safe_config, ", ")); +/// \endcode +class ICEBERG_EXPORT LogSanitizer { + public: + /// \brief Check if a configuration key likely holds a secret value. + /// \param key The configuration key to check + /// \return true if the key should be redacted in logs, false otherwise + /// + /// This method checks both exact matches (known secret keys) and pattern + /// matching (keys containing "password", "secret", "token", etc.). + /// + /// It is intentionally conservative: false positives are acceptable because + /// the cost of over-redaction is low (slightly less useful logs), while the + /// cost of under-redaction is high (credential leakage). + static bool IsSecret(std::string_view key); + + /// \brief Redact a configuration value if its key is secret. + /// \param key The configuration key + /// \param value The configuration value + /// \return The original value if not secret, otherwise "(redacted)" + /// + /// This method is intended for logging and error messages to prevent + /// accidental disclosure of sensitive values. + /// + /// Example: + /// \code + /// LOG_DEBUG("Loading config: {}={}", + /// key, LogSanitizer::Redact(key, value)); + /// \endcode + static std::string Redact(std::string_view key, std::string_view value); + + /// \brief Redact all secret values in a configuration map. + /// \param map The configuration map to sanitize + /// \return A new map with secret values replaced by "(redacted)" + /// + /// This method creates a safe-to-log copy of the configuration map with + /// all secret values redacted. + /// + /// Example: + /// \code + /// auto safe_config = LogSanitizer::RedactMap(properties); + /// LOG_INFO("Final config: {}", fmt::join(safe_config, ", ")); + /// \endcode + static std::unordered_map RedactMap( + const std::unordered_map& map); + + private: + /// \brief Get the set of known secret keys. + /// + /// This is an explicit list of configuration keys known to hold secret + /// values (tokens, passwords, keys). Pattern matching provides additional + /// coverage for unknown keys. + static const std::unordered_set& GetSecretKeys(); + + /// \brief Check if a key matches common secret patterns. + /// \param key The configuration key to check + /// \return true if the key matches sensitive patterns + /// + /// This method performs case-insensitive substring matching against common + /// secret patterns (password, secret, token, credential, private-key, etc.) + /// while excluding identifier patterns (keys ending in "-id" or "id"). + static bool MatchesSecretPattern(std::string_view key); +}; + +} // namespace iceberg