HADOOP-19197. S3A: Support AWS KMS Encryption Context #6874
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🎊 +1 overall
This message was automatically generated. |
|
@virajjasani @steveloughran do you need me to provide any more information to review this PR? |
ahmarsuhail
requested changes
Jul 15, 2024
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AUtils.java
Outdated
Show resolved
Hide resolved
...p-aws/src/main/java/org/apache/hadoop/fs/s3a/auth/delegation/EncryptionSecretOperations.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestSSEConfiguration.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AUtils.java
Outdated
Show resolved
Hide resolved
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.hadoop.fs.s3a; |
There was a problem hiding this comment.
your ITests are not very useful as they don't assert on anything. I also don't think you need 3 new ITests that extend AbstractTestS3AEncryption.
I recommend that you add a single ITest, that writes an object with encryption context. parameterize it for SSE-KMS default key, SS-KMS customer key, DSSE-KMS. Then do a GET on the object and assert on the encryption context.
There was a problem hiding this comment.
I will create just a single ITest class. Regarding the assertion, I couldn't find any S3 API that allow me to get the object's encryption context, HeadObject, for example, returns only ServerSideEncryption and SSEKMSKeyId.
The way that I found to test it is add a condition to the IAM Role policy or KMS Key policy requiring the encryption context to be set, like:
"Condition": {
"StringEquals": {
"kms:EncryptionContext:project": "hadoop",
"kms:EncryptionContext:jira": "HADOOP-19197"
}
}
Let me know if you know a way to get the encryption context from S3 or if you have a better idea on how to test it.
There was a problem hiding this comment.
I executed mvn -Dparallel-tests -DtestsThreadCount=16 clean verify in us-east-1, and I updated the PR description adding this information and explained how I tested that the encryption context is set in the S3 objects.
steveloughran
requested changes
Jul 15, 2024
There was a problem hiding this comment.
-
Create a new EncryptionContext class in fs.s3a.impl.encryption for this stuff, S3AUtils is too big and while leaving it alone helps backports, there's no need to make things worse. This will isolate changes better.
-
the issues related to EncryptionSecretOperations and secrets without a context matter. Is it that always the context must be non-null, but that it may be empty?
-
add unit test to verify round trip of key=value pairs through the base64 encoding and the delegation token marshalling?
-
Can you look at HADOOP-18708: Support S3 Client Side Encryption(CSE) With AWS SDK V2 #6884 to see if there are opportunities to share code, risks of conflict etc. And ideally, just review it in general to see what you think as it is 'close'
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AUtils.java
Outdated
Show resolved
Hide resolved
| @@ -76,7 +76,7 @@ public void testSessionTokenDecode() throws Throwable { | |||
| renewer, | |||
| new URI("s3a://anything/"), | |||
| new MarshalledCredentials("a", "b", ""), | |||
| new EncryptionSecrets(S3AEncryptionMethods.SSE_S3, ""), | |||
| new EncryptionSecrets(S3AEncryptionMethods.SSE_S3, "", ""), | |||
There was a problem hiding this comment.
extend test to verify the secrets are round tripped correctly
There was a problem hiding this comment.
Not sure if this is was you meant, but in the next revision I will add values to the key and encryption context instead of empty string. Then I will assert that they match the valyes in the decoded identifier.
I will do the same in testSessionTokenIdentifierRoundTrip()
|
@steveloughran, regarding #6884, I didn't find any risks of conflict with this PR, but I think that the new code in I also added a comment to that PR asking if the new properties need to be added to |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
The checks are failing because they are trying to compile three classes that I removed in the second commit. I don't know how to make it not use them. |
needs a squashed commit. let me review the changes first |
...ws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEncryptionSSEKMSWithEncryptionContext.java
Show resolved
Hide resolved
Add the property fs.s3a.encryption.context that allow users to specify the AWS KMS Encryption Context to be used in S3A. The value of the encryption context is a key/value string that will be Base64 encoded and set in the parameter ssekmsEncryptionContext from the S3 client. Contributed by Raphael Azzolini
raphaelazzolini
force-pushed
the
feature/hadoop-19197
branch
from
4de0f65 to
898b883
Compare
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
approved these changes
Jul 23, 2024
KeeProMise
pushed a commit
to KeeProMise/hadoop
that referenced
this pull request
Sep 9, 2024
The new property fs.s3a.encryption.context allow users to specify the AWS KMS Encryption Context to be used in S3A. The value of the encryption context is a key/value string that will be Base64 encoded and set in the parameter ssekmsEncryptionContext from the S3 client. Contributed by Raphael Azzolini
Hexiaoqiao
pushed a commit
to Hexiaoqiao/hadoop
that referenced
this pull request
Sep 12, 2024
The new property fs.s3a.encryption.context allow users to specify the AWS KMS Encryption Context to be used in S3A. The value of the encryption context is a key/value string that will be Base64 encoded and set in the parameter ssekmsEncryptionContext from the S3 client. Contributed by Raphael Azzolini
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add the property fs.s3a.encryption.context that allow users to specify the AWS KMS Encryption Context to be used in S3A.
The value of the encryption context is a key/value string that will be Base64 encoded and set in the parameter ssekmsEncryptionContext from the S3 client.
Contributed by Raphael Azzolini
Description of PR
This code change adds a new property to S3A: fs.s3a.encryption.context\
The property's value accepts a set of key/value attributes to be set on S3's encryption context. The value of the property will be base64 encoded and set in the parameter ssekmsEncryptionContext from the S3 client.
How was this patch tested?
Tested in us-east-1 with
mvn -Dparallel-tests -DtestsThreadCount=16 clean verifyI added a new test
ITestS3AEncryptionSSEKMSWithEncryptionContext. However, S3's head-object response doesn't contain the object encryption key. Therefore, I enabled CloudTrails data logs in my bucket to verify that the tests were passing the encryption context to the request.I added this property to
auth-keys.xmlThen I executed the following tests:
Finally, I verified in the CloudTrail logs, that the encryption context was set for both
aws:kmsandaws:kms:dsse.I also executed the test with the following statement in my KMS key:
When using that statement, tests without encryption context fail, and the new test will pass only if the given key-pair is set in
fs.s3a.encryption.context.For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?