Core: Add support for encryption.kms-type with aws/azure/gcp#15272
Core: Add support for encryption.kms-type with aws/azure/gcp#15272kevinjqliu merged 2 commits intoapache:mainfrom
Conversation
ebyhr
force-pushed
the
ebi/kms
branch
2 times, most recently
from
532decb to
96095f8
Compare
|
cc @ggershinsky |
docs/docs/encryption.md
Outdated
| Two parameters are required to activate encryption of a table: | ||
|
|
||
| 1. Catalog property `encryption.kms-impl`, that specifies the class path for a client of a KMS ("key management service"). | ||
| 1. Catalog property `encryption.kms-type`, that accepts `aws`, `azure` or `gcp`, or catalog property `encryption.kms-impl`, that specifies the class path for a client of a KMS ("key management service"). |
There was a problem hiding this comment.
what happens when both type and impl are specified and they are different, do we have assertions ?
There was a problem hiding this comment.
It's disallowed at EncryptionUtil.java L52 and this PR has a test to verify the failure.
There was a problem hiding this comment.
aws is specified and the impl is org.apache.iceberg.aws.AwsKeyManagementClient should we fail ?
i was thinking more of case gcp is specified and org.apache.iceberg.aws.AwsKeyManagementClient is specified
| kmsImpl = | ||
| switch (kmsType.toLowerCase(Locale.ROOT)) { | ||
| case CatalogProperties.ENCRYPTION_KMS_TYPE_AWS -> | ||
| CatalogProperties.ENCRYPTION_KMS_IMPL_AWS; | ||
| case CatalogProperties.ENCRYPTION_KMS_TYPE_AZURE -> | ||
| CatalogProperties.ENCRYPTION_KMS_IMPL_AZURE; | ||
| case CatalogProperties.ENCRYPTION_KMS_TYPE_GCP -> | ||
| CatalogProperties.ENCRYPTION_KMS_IMPL_GCP; | ||
| default -> throw new IllegalStateException("Unsupported KMS type: " + kmsType); | ||
| }; |
There was a problem hiding this comment.
Have we give a thought about tables which span across multiple cloud providers ? do we need something like ResolvingFileIO eq ?
There was a problem hiding this comment.
Currently, we have three single-cloud implementations, built-in the Iceberg code. When a patch for a new KMS client (for a widely used platform) is sent and merged in Iceberg, a new type would be added here.
A multi-cloud KMS client sounds better fitted for the custom encryption.kms-impl parameter; but if there is a popular usecase, then no reason not to have it as a new type.
There was a problem hiding this comment.
hopefully the github glitch is resolved; sorry if this appears twice:
An updated comment wrt the ResolvingFileIO. Looks like it infers the type from the file location prefix (like "s3" and others in the SCHEME_TO_FILE_IO map). This is less applicable to encryption keys.
There was a problem hiding this comment.
Would it make sense to make this pluggable? I've seen requests related to for example Hashicorp Vault (#14451), a pluggable key manager could be beneficial to integrate any 3rd party key management solution into Iceberg, as long as there's a working client implementation for it.
There was a problem hiding this comment.
The other (encryption.kms-impl) parameter is designed for a pluggable support of the 3rd party KMS systems.
We need it since there is a lot of KMS options out there, basically an undefined number.
The encryption.kms-type parameter is only for a few highly popular KMS platforms that Iceberg plans to actively support, inc security leak fixes, version updates, bug fixes etc.
There was a problem hiding this comment.
Thanks Gidon for the clarification!
|
Thanks @ebyhr , it's good to have this capability sooner rather than later. |
docs/docs/encryption.md
Outdated
| Two parameters are required to activate encryption of a table: | ||
|
|
||
| 1. Catalog property `encryption.kms-impl`, that specifies the class path for a client of a KMS ("key management service"). | ||
| 1. Catalog property `encryption.kms-type`, that accepts `aws`, `azure` or `gcp`, or catalog property `encryption.kms-impl`, that specifies the class path for a client of a KMS ("key management service"). |
There was a problem hiding this comment.
This is the first item that follows "Two parameters are required to activate encryption of a table:". The change makes the requirement somewhat less clear. We probably want something like
"1. Catalog property that specifies the KMS ("key management service"). It can be either encryption.kms-type for pre-defined KMS clients (aws, azure or gcp) or encryption.kms-impl with the client class path for custom KMS clients.
| kmsImpl = | ||
| switch (kmsType.toLowerCase(Locale.ROOT)) { | ||
| case CatalogProperties.ENCRYPTION_KMS_TYPE_AWS -> | ||
| CatalogProperties.ENCRYPTION_KMS_IMPL_AWS; | ||
| case CatalogProperties.ENCRYPTION_KMS_TYPE_AZURE -> | ||
| CatalogProperties.ENCRYPTION_KMS_IMPL_AZURE; | ||
| case CatalogProperties.ENCRYPTION_KMS_TYPE_GCP -> | ||
| CatalogProperties.ENCRYPTION_KMS_IMPL_GCP; | ||
| default -> throw new IllegalStateException("Unsupported KMS type: " + kmsType); | ||
| }; |
There was a problem hiding this comment.
Currently, we have three single-cloud implementations, built-in the Iceberg code. When a patch for a new KMS client (for a widely used platform) is sent and merged in Iceberg, a new type would be added here.
A multi-cloud KMS client sounds better fitted for the custom encryption.kms-impl parameter; but if there is a popular usecase, then no reason not to have it as a new type.
6 participants
Set
encryption.kms-implinternally based onencryption.kms-typevalue: