ggershinsky commented on PR #18337: URL: https://github.com/apache/iceberg/pull/18337#issuecomment-6012261904
> Should this go into this PR or a follow-up? It touches more than this change. Agreed, best done as a separate PR. > Today KeyManagementClient.generateKey(String wrappingKeyId) has no length argument, and AwsKeyManagementClient takes its key length from the kms.data-key-spec catalog property (default AES_256, i.e. 32 bytes, while encryption.data-key-length defaults to 16). So with the defaults the two already differ. To make clients honor encryption.kek-length, the interface needs a way to receive the length (for example a new method generateKey(String wrappingKeyId, int keyLength), with the existing one deprecated per the iceberg-core deprecation policy). Is that what you have in mind? SGTM - both adding a new method, and deprecating the current one. > Should encryption.kek-length default to the current 16 bytes, with valid values 16/24/32 like encryption.data-key-length? I think the KEK length default can be 32 bytes - compared to the DEKs, the balance between encryption strength and throughput is shifted to the strength for KEKs. > On the Hive path the table properties are currently passed by hand (key-id and data-key-length from HMS). Both this and the toggle property would need to follow whichever approach is chosen there. Yes, these parameters define the encryption behavior, and therefore should therefore be made tamper-proof (a good way to do it is to fetch them from a trusted Catalog service, instead of a metadata.json file in an untrusted storage). -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
