Add a builder API for constructing KMS MKPs #40
Conversation
The current KMS MKP constructors use legacy (@deprecated) KMS client constructors, and are likely to break at some point in the future. In addition, they also mutate the client that is passed in, and have an unfortunate combinatorial explosion of argument types. This change introduces a new builder API for constructing KMS MKPs, and deprecates the old one. In addition, it adds support for decrypting KMS keys from multiple regions with the same MKP, bringing us to feature parity with the Python SDK. For now, the semantics of code using the old constructors is unchanged, but it's likely that we'll want to remove these constructors the next time we make a breaking change to our APIs.
|
Note that this change preserves the behavior that |
This failed in cases where the default region isn't configured anywhere (e.g. in our travis builds...)
| } | ||
|
|
||
| if (defaultRegion_ == null) { | ||
| throw new AwsCryptoException("Can't use non-ARN key identifiers or aliases when no default region is set"); |
There was a problem hiding this comment.
We should probably complain on build() actually - since the list of key IDs for encrypt can't change after construction using the builder, and this isn't relevant on decrypt.
| AWSKMS getClient(String regionName); | ||
| } | ||
|
|
||
| public static class Builder implements Cloneable { |
There was a problem hiding this comment.
I generally like making internal Builders final.
| * @param tokens | ||
| * @return | ||
| */ | ||
| public Builder withGrantTokens(List<String> tokens) { |
There was a problem hiding this comment.
GrantTokens are fundamentally regional and should be treated that way. There is no value (and some negative) to passing them into the wrong region.
| */ | ||
| @Deprecated | ||
| @Override | ||
| public void setGrantTokens(final List<String> grantTokens) { |
There was a problem hiding this comment.
As noted elsewhere, GrantTokens are regional things.
| private String defaultRegion_ = null; | ||
| private RegionalClientSupplier regionalClientSupplier_ = null; | ||
| private AWSKMSClientBuilder templateBuilder_ = null; | ||
| private List<String> keyIds_ = new ArrayList<>(); |
There was a problem hiding this comment.
This should be copied when .clone() is invoked. A shallow copy here will likely cause problems.
| public static class Builder implements Cloneable { | ||
| private String defaultRegion_ = null; | ||
| private RegionalClientSupplier regionalClientSupplier_ = null; | ||
| private AWSKMSClientBuilder templateBuilder_ = null; |
There was a problem hiding this comment.
This should be copied when .clone() is invoked. A shallow copy here will likely cause problems.
| private RegionalClientSupplier regionalClientSupplier_ = null; | ||
| private AWSKMSClientBuilder templateBuilder_ = null; | ||
| private List<String> keyIds_ = new ArrayList<>(); | ||
| private List<String> grantTokens_ = new ArrayList<>(); |
There was a problem hiding this comment.
This should be copied when .clone() is invoked. A shallow copy here will likely cause problems.
|
|
||
| package com.amazonaws.encryptionsdk.kms; | ||
|
|
||
| import javax.crypto.SecretKey; |
There was a problem hiding this comment.
Are these new imports used anywhere?
| } | ||
|
|
||
| /** | ||
| * Provides a custom factory function that will vend KMS clients. This is provided for advanced use cases which |
There was a problem hiding this comment.
Hmm, I like this. This would be good to add to the Python client too; currently we just always create a standard client.
| } | ||
|
|
||
| if (defaultRegion_ == null) { | ||
| throw new AwsCryptoException("Can't use non-ARN key identifiers or aliases when no default region is set"); |
|
|
||
| public Builder clone() { | ||
| try { | ||
| Builder cloned = (Builder)super.clone(); |
There was a problem hiding this comment.
NitPick: space after the cast
The current KMS MKP constructors use legacy (@deprecated) KMS client
constructors, and are likely to break at some point in the future. In addition,
they also mutate the client that is passed in, and have an unfortunate
combinatorial explosion of argument types.
This change introduces a new builder API for constructing KMS MKPs, and
deprecates the old one. In addition, it adds support for decrypting KMS keys
from multiple regions with the same MKP, bringing us to feature parity with the
Python SDK.
For now, the semantics of code using the old constructors is unchanged, but
it's likely that we'll want to remove these constructors the next time we make
a breaking change to our APIs.