-
Notifications
You must be signed in to change notification settings - Fork 73
KMS Encryption Plugin #1543
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
KMS Encryption Plugin #1543
Conversation
…ionIntegration test to use the key management utility instead of manually updating tables
| - AWS KMS key with appropriate permissions | ||
| - Database table to store encryption metadata | ||
| - AWS credentials configured (via IAM roles, profiles, or environment variables) | ||
| - **JSqlParser 4.5.x dependency** - Required for SQL parsing and analysis |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're in the SQL parsing business now!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No choice... that is the only way to tell if a column requires encryption (at the moment)
|
|
||
| ```sql | ||
| -- Key storage table (must be created first due to foreign key) | ||
| CREATE TABLE key_storage ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we generally rely on user connecting to a database which has this table? What if I have a user with multiple schemas available to me, where this table needs to be? Also of course, what is multiple tables with the same name exist in different schemas? Could there be a any confusion as to what metadata table is relevant to them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we have tables prefixed with like "aws_" or configurable via properties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, implementation detail... could be in an aws schema
| // Configure encryption for sensitive columns | ||
| keyManagementUtility.initializeEncryptionForColumn("customers", "ssn", masterKeyArn); | ||
| keyManagementUtility.initializeEncryptionForColumn("customers", "credit_card", masterKeyArn); | ||
| keyManagementUtility.initializeEncryptionForColumn("customers", "phone", masterKeyArn); | ||
| keyManagementUtility.initializeEncryptionForColumn("customers", "address", masterKeyArn); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like to make sure there is no sneaky insert of unencrypted data by mistake the table should have a trigger which looks for some KMS-related prefixes in the values inserted and reject if it doesn't find them. Maybe we should recommend customers setting something up. I believe KMS-encrypted strings have specific look to them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will cover all possible paths how the data might end up inserted into the table, such as via PL/SQL function or copied from another table...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is certainly a valid concern. Since this will work with MySQL as well we may have to figure something out.
|
|
||
| ## Limitations | ||
|
|
||
| - Currently supports string data types for encryption |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume ORDER BY or LIKE are not much useful since server side cannot figure it out.
However, if I do group by encrypted column, will it work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, won't work. You can't do any sorting, indexing etc on an encrypted column. You would need another column with say the last 4 digits if the ssn to do equaliity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the same value is re-written to the encrypted column, is it going to have a different cyphertext?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will have a different cypher text. The encryption is non-determistic.
| optionalImplementation("io.opentelemetry:opentelemetry-api:1.52.0") | ||
| optionalImplementation("io.opentelemetry:opentelemetry-sdk:1.52.0") | ||
| optionalImplementation("io.opentelemetry:opentelemetry-sdk-metrics:1.52.0") | ||
| optionalImplementation("com.github.jsqlparser:jsqlparser:4.5") // JSqlParser SQL parser (Java 8 compatible) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have we done a security review / audit for this dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, but I may replace it with my own.
…ma to the key metadata tables
…t the key metadata is store in
Summary
Plugin to enable encrypting columns on the client
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.