Skip to content
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.

Sign up for GitHub

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

Early check for dependency when set kmsKeyId #398

Closed
wants to merge 3 commits into from
Closed

Early check for dependency when set kmsKeyId #398

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
51cb94a
Early check for dependency when set kmsKeyId
hieuwu Oct 31, 2024
1acd568
Move check to proper place
hieuwu Oct 31, 2024
264bff8
Require dependency & remove unneeded check
hieuwu Nov 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/main/java/software/amazon/encryption/s3/S3EncryptionClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1139,6 +1139,11 @@ public S3EncryptionClient build() {
.secureRandom(_secureRandom)
.build();
} else if (_kmsKeyId != null) {
try {
Class.forName("software.amazon.awssdk.services.kms.KmsClient");
} catch (ClassNotFoundException e) {
throw new RuntimeException("software.amazon.awssdk:kms is required to set up with KMS key", e);
}
KmsClient kmsClient = KmsClient.builder()
.credentialsProvider(_awsCredentialsProvider)
.region(_region)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think this check is sufficient to prevent the exception.
The KMS Client is an import of this class.

I think the Java Compiled Byte Code will require KMS,
even if a KMS key is not passed,
because the Java class has references to the objects.

But I could be wrong;
Maybe the Java compiler is fancier than I thought,
and masks the KMS objects unless this if statement is encountered?

@hieuwu did you run a test that proved that this mitigates your issue?

Because the current testing of the S3EC will not cover this.

Copy link
Contributor Author

@hieuwu hieuwu Nov 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@texastony I got the same issue when run the app. As kms is dependency of S3EC, I would expect that it should fail at Compile Time. However the build is still successful, the exception is thrown only we start running the app or test.

Screenshot 2024-11-01 at 07 12 42

I have removed the check. Also I have tried making software.amazon.awssdk:kms not optional, the lib works as expected, no exception thrown and we do not need to add additional dependency.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect this is the correct fix.
I'll approve CI run.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you Tony

Expand Down