AWS SDK v2 support #516
AWS SDK v2 support #516
Conversation
smswz
force-pushed
the
sdk-v2-review
branch
5 times, most recently
from
a3e3367 to
6820da3
Compare
There was a problem hiding this comment.
I am not sure if we are leaking information when we store certain variables.
For example, on this line:
final String decryptResponseKeyId = decryptResponse.keyId();
I understand that for readability this makes more sense but I'm not sure if adding this reference leaks information?
|
Hmmm, I wouldn't think a variable would cause information to be leaked. Maybe @lavaleri or @farleyb-amazon can provide more information? |
|
The KMS Key Id is not secret. Even if it were, we would just want to be sure to minimize the time it needs to exist in memory as much as is feasible. This extra reference won't hurt since with or without this extra reference no more references to this data will exist after this method completes, and it will be ready to be GC'd by Java as appropriate. In terms of data leakage in Java, we mainly just want to be sure that we aren't storing secret values in such that they never get GC'd, or we potentially log that information. |
src/main/java/com/amazonaws/encryptionsdk/kmsv2/AwsKmsMrkAwareMasterKey.java
Show resolved
Hide resolved
src/main/java/com/amazonaws/encryptionsdk/kmsv2/AwsKmsMrkAwareMasterKey.java
Show resolved
Hide resolved
src/main/java/com/amazonaws/encryptionsdk/kmsv2/AwsKmsMrkAwareMasterKeyProvider.java
Show resolved
Hide resolved
src/main/java/com/amazonaws/encryptionsdk/kmsv2/AwsKmsMrkAwareMasterKeyProvider.java
Show resolved
Hide resolved
| private DiscoveryFilter discoveryFilter_ = null; | ||
| private String discoveryMrkRegion_ = this.defaultRegion_; | ||
| private Region discoveryMrkRegion_ = this.defaultRegion_; | ||
|
|
||
| Builder() { | ||
| // Default access: Don't allow outside classes to extend this class | ||
| } | ||
|
|
||
| public Builder clone() { |
There was a problem hiding this comment.
This isn't blocking, but this is a good opportunity to move from Cloneable to a copy constructor pattern, i.e. implement a toBuilder() instead of this clone. The Cloneable pattern is confusing and easy to get wrong, and I believe the SDK already has customers familiar with the toBuilder() pattern.
There was a problem hiding this comment.
That's a great idea, especially since the SDKv2 clients no longer support the Cloneable pattern either. I will probably push this change to a later CR though since its more of a functional change.
src/main/java/com/amazonaws/encryptionsdk/kmsv2/KmsMasterKeyProvider.java
Outdated
Show resolved
Hide resolved
src/main/java/com/amazonaws/encryptionsdk/kmsv2/AwsKmsMrkAwareMasterKeyProvider.java
Show resolved
Hide resolved
src/main/java/com/amazonaws/encryptionsdk/kmsv2/AwsKmsMrkAwareMasterKeyProvider.java
Show resolved
Hide resolved
src/main/java/com/amazonaws/encryptionsdk/kmsv2/KmsMasterKeyProvider.java
Outdated
Show resolved
Hide resolved
src/main/java/com/amazonaws/encryptionsdk/kmsv2/KmsMasterKeyProvider.java
Outdated
Show resolved
Hide resolved
|
Dependent on aws/aws-sdk-java-v2#3024 |
|
After getting some eyes, I'll rename the package to |
1 similar comment
|
After getting some eyes, I'll rename the package to |
src/main/java/com/amazonaws/encryptionsdk/kms/AwsKmsMrkAwareMasterKey.java
Show resolved
Hide resolved
src/test/java/com/amazonaws/encryptionsdk/kmsv2/XCompatKmsDecryptTest.java
Show resolved
Hide resolved
Now supports using either v1 or v2 (or both) of the AWS SDK for Java with the encryption SDK. The new classes to support AWS SDK for Java v2 can be found in the `com.amazonaws.encryptionsdk.kmssdkv2` package.
Description of changes:
Adding support for the new version of the AWS SDK for Java. The changes will be merged into the
aws-sdk-v2branch to make it easier to compare changes. Once the source and tests are merged in to that branch, I will open a PR to mergeaws-sdk-v2intomasterBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Check any applicable: