You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/05/06 10:56:27 UTC
[GitHub] [iceberg] JiJiTang opened a new pull request, #4714: Add AwsKmsClient for table encryption KMS client implementation
JiJiTang opened a new pull request, #4714:
URL: https://github.com/apache/iceberg/pull/4714
Aws KMS client implementation:
- Support data key generation using AWS KMS, configurable via property `ENABLE_KEY_GENERATION`
--
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
[GitHub] [iceberg] ggershinsky commented on pull request #4714: [AWS]Add AwsKmsClient for table encryption KMS client implementation
Posted by GitBox <gi...@apache.org>.
ggershinsky commented on PR #4714:
URL: https://github.com/apache/iceberg/pull/4714#issuecomment-1122037631
not any users, but org security experts, see eg our earlier docs at https://spark.apache.org/docs/latest/sql-data-sources-parquet.html#kms-client . Let us think a bit more about this PR.
--
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
[GitHub] [iceberg] JiJiTang commented on a diff in pull request #4714: [AWS]Add AwsKmsClient for table encryption KMS client implementation
Posted by GitBox <gi...@apache.org>.
JiJiTang commented on code in PR #4714:
URL: https://github.com/apache/iceberg/pull/4714#discussion_r867984567
##########
aws/src/main/java/org/apache/iceberg/aws/AwsKmsClient.java:
##########
@@ -0,0 +1,101 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.aws;
+
+import java.nio.ByteBuffer;
+import java.util.Base64;
+import java.util.Map;
+import org.apache.iceberg.encryption.KmsClient;
+import software.amazon.awssdk.core.SdkBytes;
+import software.amazon.awssdk.services.kms.model.DataKeySpec;
+import software.amazon.awssdk.services.kms.model.DecryptRequest;
+import software.amazon.awssdk.services.kms.model.DecryptResponse;
+import software.amazon.awssdk.services.kms.model.EncryptRequest;
+import software.amazon.awssdk.services.kms.model.EncryptResponse;
+import software.amazon.awssdk.services.kms.model.GenerateDataKeyRequest;
+import software.amazon.awssdk.services.kms.model.GenerateDataKeyResponse;
+
+public class AwsKmsClient implements KmsClient {
+ /*
+ * property for enabling data key generation using AWS KMS service, by default it's enabled,
+ * please set this property to "false" to disable key generation by AWS KMS server.
+ */
+ public static final String ENABLE_KEY_GENERATION = "kms.client.aws.key.generation.enabled";
+ // property for specifying data key generation key spec: AES_256 or AES_128
Review Comment:
@flyrain , updated.
--
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
[GitHub] [iceberg] JiJiTang commented on pull request #4714: Add AwsKmsClient for table encryption KMS client implementation
Posted by GitBox <gi...@apache.org>.
JiJiTang commented on PR #4714:
URL: https://github.com/apache/iceberg/pull/4714#issuecomment-1119494288
@ggershinsky for review
--
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
[GitHub] [iceberg] ggershinsky commented on pull request #4714: [AWS]Add AwsKmsClient for table encryption KMS client implementation
Posted by GitBox <gi...@apache.org>.
ggershinsky commented on PR #4714:
URL: https://github.com/apache/iceberg/pull/4714#issuecomment-1121409812
Yep, a KMS client doesn't receive catalog properties (which makes sense, because a KMS and a Catalog are two unrelated services). It can be configured via table properties and via custom channels. If the default AWS KMS client works out of box for a typical environment, then maybe we can start without custom channels; allowing users to override this as needed.
--
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
[GitHub] [iceberg] flyrain commented on a diff in pull request #4714: [AWS]Add AwsKmsClient for table encryption KMS client implementation
Posted by GitBox <gi...@apache.org>.
flyrain commented on code in PR #4714:
URL: https://github.com/apache/iceberg/pull/4714#discussion_r867004263
##########
aws/src/main/java/org/apache/iceberg/aws/AwsKmsClient.java:
##########
@@ -0,0 +1,101 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.aws;
+
+import java.nio.ByteBuffer;
+import java.util.Base64;
+import java.util.Map;
+import org.apache.iceberg.encryption.KmsClient;
+import software.amazon.awssdk.core.SdkBytes;
+import software.amazon.awssdk.services.kms.model.DataKeySpec;
+import software.amazon.awssdk.services.kms.model.DecryptRequest;
+import software.amazon.awssdk.services.kms.model.DecryptResponse;
+import software.amazon.awssdk.services.kms.model.EncryptRequest;
+import software.amazon.awssdk.services.kms.model.EncryptResponse;
+import software.amazon.awssdk.services.kms.model.GenerateDataKeyRequest;
+import software.amazon.awssdk.services.kms.model.GenerateDataKeyResponse;
+
+public class AwsKmsClient implements KmsClient {
+ /*
+ * property for enabling data key generation using AWS KMS service, by default it's enabled,
+ * please set this property to "false" to disable key generation by AWS KMS server.
+ */
+ public static final String ENABLE_KEY_GENERATION = "kms.client.aws.key.generation.enabled";
+ // property for specifying data key generation key spec: AES_256 or AES_128
Review Comment:
Nit: suggest an empty line above the comment.
--
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
[GitHub] [iceberg] ggershinsky commented on pull request #4714: [AWS]Add AwsKmsClient for table encryption KMS client implementation
Posted by GitBox <gi...@apache.org>.
ggershinsky commented on PR #4714:
URL: https://github.com/apache/iceberg/pull/4714#issuecomment-1119616871
cc @jackye1995 @RussellSpitzer @flyrain
--
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
[GitHub] [iceberg] danielcweeks commented on pull request #4714: [AWS]Add AwsKmsClient for table encryption KMS client implementation
Posted by GitBox <gi...@apache.org>.
danielcweeks commented on PR #4714:
URL: https://github.com/apache/iceberg/pull/4714#issuecomment-1121304920
Hey @JiJiTang I'm hoping you can clarify a little more about this implementation and the use case. A few things confuse me a little:
1. This appears to be a standalone kms client implementation, but I don't see how anyone could use this directly without bringing custom code (i.e. we don't dynamically load any of the aws clients individually).
2. This doesn't appear to be implemented in the same pattern as the AWSClientFactory (I see there is a factory method on this implementation, but that's quite different from how other clients are accessed).
There are a lot of different configurations with AWS clients and I don't think the intent is to make Iceberg support every option, so it might be better to just maintain your own client factory with customizations like this.
If we were to support key generation, it would probably need to be integrated with the AWSClientFactory implementations and configurable via a property, but this seems to be getting into pretty narrow use cases that add additional complexity.
--
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
[GitHub] [iceberg] JiJiTang commented on pull request #4714: [AWS]Add AwsKmsClient for table encryption KMS client implementation
Posted by GitBox <gi...@apache.org>.
JiJiTang commented on PR #4714:
URL: https://github.com/apache/iceberg/pull/4714#issuecomment-1121361248
@danielcweeks , thanks VM for the good comments. Please find my comments as the following:
1. Default AWS KMS client created by default AWSClientFactory relies on normal AWS credential provider which works out of box for typical AWS environment without the need of customoisation.
2. I didn't use `AwsClientFactories.from` because AWSClientFactory relies on catalog properties for initialisation but Iceberg Encryption KMS client `initialise` method only brings table properties not catalog properties in the scope.
IMHO, support key generation or not should be per table configuration and configured via table properties, not sure we should configure via AWSClientFactory, cc @ggershinsky for more inputs.
--
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