You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "shangxinli (via GitHub)" <gi...@apache.org> on 2026/10/16 18:50:46 UTC
[PR] feat: add KeyManagementClient, KmsRegistry and InMemoryKms [iceberg-cpp]
shangxinli opened a new pull request, #1031:
URL: https://github.com/apache/iceberg-cpp/pull/1031
## Summary
Step 5 of #988. Adds the KMS client interface and the registry that creates one from catalog properties, plus an in-memory client for tests and local development.
- `KeyManagementClient` (core): wraps and unwraps secret keys with a master key held in a KMS, mirroring Java's interface including the optional in-KMS key generation path. The core library stays free of crypto dependencies — this is an interface only.
- `KmsRegistry` (core): resolves the catalog's KMS client from the same property names as Java, `encryption.kms-type` (`aws`, `gcp`, `azure`) or `encryption.kms-impl` (a name registered with `KmsRegistry::Register`). Built-in clients register themselves when their library is linked; applications register custom ones before creating catalogs. Returns a null client when neither property is set, so an unencrypted catalog is not an error.
- `InMemoryKms` (bundle, `ICEBERG_ENCRYPTION`): master keys held in memory, wire compatible with Java's `MemoryMockKMS` — AES-GCM wrap under the master key with no AAD. Master keys can also come from catalog properties as `encryption.in-memory-kms.key.<id>` = Base64.
Stacked on #1030 (step 4), which `InMemoryKms` uses for the AES-GCM primitives; the first commit here is that PR. Review only the second commit, or wait for #1030 to land.
## Java parity
Ported against `KeyManagementClient.java` and `EncryptionUtil.createKmsClient`, with `MemoryMockKMS` / `UnitestKMS` as the reference for the in-memory client. Deliberate differences, both flagged in the code:
- Java resolves `encryption.kms-impl` to a class name and instantiates it reflectively. C++ has no reflection, so the registry maps a name to a factory; the property keys and their meaning are unchanged.
- Java accepts a fourth type, `aliyun`. #988 scopes the built-in clients to AWS, GCP and Azure, so `aliyun` is not built in here; the doc comment says to register one and select it through `encryption.kms-impl`. The type list is otherwise identical.
One behavior is matched on purpose rather than "improved": a property that is present but empty counts as set, as in Java, so a misconfigured catalog fails instead of silently loading with no KMS.
## Test plan
- [x] `encryption_test` passes: registry loads a custom implementation and initializes it with the catalog properties, resolves a built-in type case-insensitively, and rejects setting both type and impl, an unsupported type, an unregistered name, and a present-but-empty value.
- [x] `crypto_test` passes: `InMemoryKms` unwraps a key encryption key that Iceberg Java's `UnitestKMS` wrapped under its `keyA` master key, which is what makes the Java wire compatibility claim testable rather than a C++ round trip. The same works through `KmsRegistry` with a Base64 master key supplied as a catalog property.
- [x] `cpp20_compatibility_test` passes. `key_management_client.h` is core and unconditional; `in_memory_kms.h` is bundle plus encryption, so it joins `aes_gcm_stream.h` behind `ICEBERG_PUBLIC_HEADERS_ENCRYPTION`.
- [x] `pre-commit` clean, including the generated public-header check.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: add KeyManagementClient, KmsRegistry and InMemoryKms [iceberg-cpp]
Posted by "shangxinli (via GitHub)" <gi...@apache.org>.
shangxinli commented on PR #1031:
URL: https://github.com/apache/iceberg-cpp/pull/1031#issuecomment-6450914515
@wgtmac CI is green and the Load empty-factory fix is in — would appreciate a look when you have time.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: add KeyManagementClient, KmsRegistry and InMemoryKms [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1031:
URL: https://github.com/apache/iceberg-cpp/pull/1031#discussion_r4334879654
##########
src/iceberg/encryption/key_management_client.cc:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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/encryption/key_management_client.h"
+
+#include <mutex>
+
+#include "iceberg/util/macros.h"
+#include "iceberg/util/string_util.h"
+
+namespace iceberg {
+
+namespace {
+
+struct RegistryState {
+ std::mutex mutex;
+ std::unordered_map<std::string, KmsRegistry::Factory> factories;
+};
+
+RegistryState& State() {
+ static RegistryState state;
+ return state;
+}
+
+} // namespace
+
+void KmsRegistry::Register(std::string name, Factory factory) {
+ auto& state = State();
+ std::lock_guard lock(state.mutex);
+ state.factories.insert_or_assign(std::move(name), std::move(factory));
+}
+
+Result<std::shared_ptr<KeyManagementClient>> KmsRegistry::Load(
+ std::string_view name, const Properties& properties) {
+ Factory factory;
+ {
+ auto& state = State();
+ std::lock_guard lock(state.mutex);
+ auto it = state.factories.find(std::string(name));
+ if (it == state.factories.end()) {
+ return NotFound(
+ "KMS client not found: {}. Built-in clients must be enabled at build time "
+ "(ICEBERG_KMS_AWS, ICEBERG_KMS_GCP, ICEBERG_KMS_AZURE); custom clients must be "
+ "registered with KmsRegistry::Register",
+ name);
+ }
+ factory = it->second;
+ }
+ ICEBERG_ASSIGN_OR_RAISE(auto client, factory());
Review Comment:
Could Load reject an empty factory before invoking it, or document a non-empty factory as a Register precondition? An empty std::function currently throws std::bad_function_call instead of returning a Result error.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: add KeyManagementClient, KmsRegistry and InMemoryKms [iceberg-cpp]
Posted by "shangxinli (via GitHub)" <gi...@apache.org>.
shangxinli commented on code in PR #1031:
URL: https://github.com/apache/iceberg-cpp/pull/1031#discussion_r4350376352
##########
src/iceberg/encryption/key_management_client.cc:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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/encryption/key_management_client.h"
+
+#include <mutex>
+
+#include "iceberg/util/macros.h"
+#include "iceberg/util/string_util.h"
+
+namespace iceberg {
+
+namespace {
+
+struct RegistryState {
+ std::mutex mutex;
+ std::unordered_map<std::string, KmsRegistry::Factory> factories;
+};
+
+RegistryState& State() {
+ static RegistryState state;
+ return state;
+}
+
+} // namespace
+
+void KmsRegistry::Register(std::string name, Factory factory) {
+ auto& state = State();
+ std::lock_guard lock(state.mutex);
+ state.factories.insert_or_assign(std::move(name), std::move(factory));
+}
+
+Result<std::shared_ptr<KeyManagementClient>> KmsRegistry::Load(
+ std::string_view name, const Properties& properties) {
+ Factory factory;
+ {
+ auto& state = State();
+ std::lock_guard lock(state.mutex);
+ auto it = state.factories.find(std::string(name));
+ if (it == state.factories.end()) {
+ return NotFound(
+ "KMS client not found: {}. Built-in clients must be enabled at build time "
+ "(ICEBERG_KMS_AWS, ICEBERG_KMS_GCP, ICEBERG_KMS_AZURE); custom clients must be "
+ "registered with KmsRegistry::Register",
+ name);
+ }
+ factory = it->second;
+ }
+ ICEBERG_ASSIGN_OR_RAISE(auto client, factory());
Review Comment:
Load now rejects an empty factory before invoking it, with a regression test.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: add KeyManagementClient, KmsRegistry and InMemoryKms [iceberg-cpp]
Posted by "shangxinli (via GitHub)" <gi...@apache.org>.
shangxinli commented on code in PR #1031:
URL: https://github.com/apache/iceberg-cpp/pull/1031#discussion_r4350512013
##########
src/iceberg/encryption/key_management_client.cc:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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/encryption/key_management_client.h"
+
+#include <mutex>
+
+#include "iceberg/util/macros.h"
+#include "iceberg/util/string_util.h"
+
+namespace iceberg {
+
+namespace {
+
+struct RegistryState {
+ std::mutex mutex;
+ std::unordered_map<std::string, KmsRegistry::Factory> factories;
+};
+
+RegistryState& State() {
+ static RegistryState state;
+ return state;
+}
+
+} // namespace
+
+void KmsRegistry::Register(std::string name, Factory factory) {
+ auto& state = State();
+ std::lock_guard lock(state.mutex);
+ state.factories.insert_or_assign(std::move(name), std::move(factory));
+}
+
+Result<std::shared_ptr<KeyManagementClient>> KmsRegistry::Load(
+ std::string_view name, const Properties& properties) {
+ Factory factory;
+ {
+ auto& state = State();
+ std::lock_guard lock(state.mutex);
+ auto it = state.factories.find(std::string(name));
+ if (it == state.factories.end()) {
+ return NotFound(
+ "KMS client not found: {}. Built-in clients must be enabled at build time "
+ "(ICEBERG_KMS_AWS, ICEBERG_KMS_GCP, ICEBERG_KMS_AZURE); custom clients must be "
+ "registered with KmsRegistry::Register",
+ name);
+ }
+ factory = it->second;
+ }
+ ICEBERG_ASSIGN_OR_RAISE(auto client, factory());
Review Comment:
Good catch - Load now rejects an empty factory with an invalid-argument error instead of invoking it, with a regression test that fails with bad_function_call without the guard.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org