-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
HADOOP-18850 Enable dual-layer server-side encryption with AWS KMS keys #6140
Conversation
Bucket with DSSE encryption enabled.
|
💔 -1 overall
This message was automatically generated. |
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.
had a quick review; one request for production code as that factory method is getting complex; minor cleanups for tests.
will need docs too
@@ -94,6 +96,13 @@ public static void assertEncrypted(S3AFileSystem fs, | |||
kmsKeyArn, | |||
md.ssekmsKeyId()); | |||
break; | |||
case DSSE_KMS: | |||
assertEquals("Wrong algorithm in " + details, |
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.
prefer assertJ for all new test cases. save time by doing it first time round
*/ | ||
private void skipIfBucketNotKmsEncrypted() throws IOException { | ||
S3AFileSystem fs = getFileSystem(); | ||
Path path = path(getMethodName() + "find-encryption-algo"); |
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.
just use methodPath()
String kmsKey = S3AUtils.getS3EncryptionKey(getTestBucketName(c), c); | ||
// skip the test if DSSE-KMS or KMS key not set. | ||
if (StringUtils.isBlank(kmsKey)) { | ||
skip(S3_ENCRYPTION_KEY + " is not set for " + |
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.
maybe S3ATestUtils.skipIfEncryptionNotSet(); else assume(string, bool)
@@ -354,6 +358,10 @@ private void putEncryptionParameters(PutObjectRequest.Builder putObjectRequestBu | |||
// Set the KMS key if present, else S3 uses AWS managed key. | |||
EncryptionSecretOperations.getSSEAwsKMSKey(encryptionSecrets) | |||
.ifPresent(kmsKey -> putObjectRequestBuilder.ssekmsKeyId(kmsKey)); | |||
} else if (S3AEncryptionMethods.DSSE_KMS == algorithm) { |
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.
how about we move to a switch() here and add a default for unknown stuff
🎊 +1 overall
This message was automatically generated. |
Re-tested with latest revision |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
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.
looks good; some minor changes but +1 pending those
try { | ||
skipIfEncryptionNotSet(c, DSSE_KMS); | ||
} catch (IOException e) { | ||
throw new RuntimeException(e); |
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 this was production i'd ask for an UncheckedIOException(); not so worried here
|
||
4. As a result, S3 will only decode the data if the user has been granted access to the key. | ||
|
||
|
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.
can you add a "further reading" link to the aws docs on this; its a complicated topic
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.
done
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Re-tested with latest revision |
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.
LGTM
+1
…KMS keys (apache#6140) Contributed by Viraj Jasani
…KMS keys (apache#6140) Contributed by Viraj Jasani
Jira: HADOOP-18850