-
Notifications
You must be signed in to change notification settings - Fork 123
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
Use AWSKMSClientBuilder #6
Changes from all commits
0b9164a
4a3ae70
801cab5
d58e32c
bebe70d
40f8624
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
package com.amazonaws.crypto.examples; | ||
|
||
import com.amazonaws.encryptionsdk.AwsCrypto; | ||
import com.amazonaws.encryptionsdk.CryptoResult; | ||
import com.amazonaws.encryptionsdk.kms.KmsMasterKey; | ||
import com.amazonaws.encryptionsdk.kms.KmsMasterKeyProvider; | ||
import com.amazonaws.encryptionsdk.kms.KmsMasterKeyProviderBuilder; | ||
|
||
import java.util.Collections; | ||
import java.util.Map; | ||
|
||
/** | ||
* <p> | ||
* Encrypts and then decrypts a string under a KMS key | ||
* | ||
* <p> | ||
* Arguments: | ||
* <ol> | ||
* <li>KMS Key Arn | ||
* <li>String to encrypt | ||
* </ol> | ||
*/ | ||
public class BuilderExample { | ||
|
||
private static String keyArn; | ||
private static String data; | ||
|
||
public static void main(final String[] args) { | ||
keyArn = args[0]; | ||
data = args[1]; | ||
|
||
// Instantiate the SDK | ||
final AwsCrypto crypto = new AwsCrypto(); | ||
|
||
// Set up the KmsMasterKeyProvider using the defaults provided by the KmsMasterKeyProviderBuilder | ||
final KmsMasterKeyProvider prov = KmsMasterKeyProviderBuilder.standard() | ||
.withKeyId(keyArn) | ||
.build(); | ||
|
||
// Encrypt the data | ||
// | ||
// Most encrypted data should have associated encryption context | ||
// to protect integrity. Here, we'll just use a placeholder value. | ||
// | ||
// For more information see: | ||
// blogs.aws.amazon.com/security/post/Tx2LZ6WBJJANTNW/How-to-Protect-the-Integrity-of-Your-Encrypted-Data-by-Using-AWS-Key-Management | ||
final Map<String, String> context = Collections.singletonMap("Example", "String"); | ||
|
||
final String ciphertext = crypto.encryptString(prov, data, context).getResult(); | ||
System.out.println("Ciphertext: " + ciphertext); | ||
|
||
// Decrypt the data | ||
final CryptoResult<String, KmsMasterKey> decryptResult = crypto.decryptString(prov, ciphertext); | ||
// We need to check the encryption context (and ideally key) to ensure that | ||
// this was the ciphertext we expected | ||
if (!decryptResult.getMasterKeyIds().get(0).equals(keyArn)) { | ||
throw new IllegalStateException("Wrong key id!"); | ||
} | ||
|
||
// The SDK may add information to the encryption context, so we check to ensure | ||
// that all of our values are present | ||
for (final Map.Entry<String, String> e : context.entrySet()) { | ||
if (!e.getValue().equals(decryptResult.getEncryptionContext().get(e.getKey()))) { | ||
throw new IllegalStateException("Wrong Encryption Context!"); | ||
} | ||
} | ||
|
||
// Now that we know we have the correct data, we can output it. | ||
System.out.println("Decrypted: " + decryptResult.getResult()); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -131,10 +131,19 @@ public KmsMasterKeyProvider(final AWSKMS kms, final Region region, final List<St | |
kms_ = kms; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should I'd also suggest a |
||
region_ = region; | ||
regionName_ = region.getName(); | ||
kms_.setRegion(region); | ||
// When kms_ is created by the builder, it is immutable and will throw an exception when setting something | ||
try { | ||
kms_.setRegion(region); | ||
} catch (UnsupportedOperationException e) { | ||
// Ignored. Not a problem, region should already be set by the builder. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
} | ||
keyIds_ = new ArrayList<>(keyIds); | ||
} | ||
|
||
public static KmsMasterKeyProviderBuilder builder() { | ||
return KmsMasterKeyProviderBuilder.standard(); | ||
} | ||
|
||
/** | ||
* Returns "aws-kms" | ||
*/ | ||
|
@@ -215,6 +224,8 @@ public void addGrantToken(final String grantToken) { | |
* Configures this provider to use a custom endpoint. Sets the underlying {@link Region} object | ||
* to {@code null}, and instructs the internal KMS client to use the specified {@code endPoint} | ||
* and {@code regionName}. | ||
* | ||
* Note: This method will not work if this object was created by {@link KmsMasterKeyProviderBuilder}. | ||
*/ | ||
public void setCustomEndpoint(final String regionName, final String endPoint) { | ||
if (kms_ instanceof AWSKMSClient) { | ||
|
@@ -231,6 +242,8 @@ public void setCustomEndpoint(final String regionName, final String endPoint) { | |
* Set the AWS region of the AWS KMS service for access to the master key. This method simply | ||
* calls the same method of the underlying {@link AWSKMSClient} | ||
* | ||
* Note: This method will not work if this object was created by {@link KmsMasterKeyProviderBuilder}. | ||
* | ||
* @param region | ||
* string containing the region. | ||
*/ | ||
|
@@ -244,7 +257,7 @@ public Region getRegion() { | |
return region_; | ||
} | ||
|
||
private static Region getStartingRegion(final String keyArn) { | ||
static Region getStartingRegion(final String keyArn) { | ||
final String region = parseRegionfromKeyArn(keyArn); | ||
if (region != null) { | ||
return Region.getRegion(Regions.fromName(region)); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,125 @@ | ||
package com.amazonaws.encryptionsdk.kms; | ||
|
||
import com.amazonaws.ClientConfigurationFactory; | ||
import com.amazonaws.client.AwsSyncClientParams; | ||
import com.amazonaws.client.builder.AwsClientBuilder; | ||
import com.amazonaws.regions.AwsRegionProvider; | ||
import com.amazonaws.regions.DefaultAwsRegionProviderChain; | ||
import com.amazonaws.regions.Region; | ||
import com.amazonaws.regions.RegionUtils; | ||
import com.amazonaws.regions.Regions; | ||
import com.amazonaws.services.kms.AWSKMSClient; | ||
import com.amazonaws.services.kms.AWSKMSClientBuilder; | ||
|
||
import java.util.ArrayList; | ||
import java.util.Collections; | ||
import java.util.List; | ||
|
||
/** | ||
* Used to build a {@link KmsMasterKeyProvider} which will use the defaults found in the relevant provider chains. | ||
* | ||
* If a region is not defined and no region can be found in the region provider chain, then | ||
* {@link Regions#DEFAULT_REGION} is used. | ||
* <pre> | ||
* KmsMasterKeyProvider keyProvider = KmsMasterKeyProviderBuilder.defaultProvider(); | ||
* </pre> | ||
* | ||
* Example, specifying a region: | ||
* <pre> | ||
* KmsMasterKeyProvider keyProvider = KmsMasterKeyProviderBuilder.standard() | ||
* .withDefaultRegion("us-east-1") | ||
* .build(); | ||
* </pre> | ||
*/ | ||
public class KmsMasterKeyProviderBuilder extends AwsClientBuilder<KmsMasterKeyProviderBuilder, KmsMasterKeyProvider> { | ||
|
||
private List<String> keyIds; | ||
|
||
/** | ||
* @return a new {@link KmsMasterKeyProviderBuilder} with the defaults set. | ||
*/ | ||
public static KmsMasterKeyProviderBuilder standard() { | ||
return new KmsMasterKeyProviderBuilder(); | ||
} | ||
|
||
/** | ||
* @return a KmsMasterKeyProvider using the {@link AWSKMSClientBuilder} defaults, and an empty key ID list. | ||
*/ | ||
public static KmsMasterKeyProvider defaultProvider() { | ||
return standard().build(); | ||
} | ||
|
||
private KmsMasterKeyProviderBuilder() { | ||
super(new ClientConfigurationFactory()); | ||
} | ||
|
||
/** | ||
* Sets the default region to be used by the client. Overrides any previously set region. | ||
*/ | ||
public KmsMasterKeyProviderBuilder withDefaultRegion(Region region) { | ||
return withRegion(region.getName()); | ||
} | ||
|
||
/** | ||
* Sets the region using a keyId. This will override any previously set regions. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should only infer the region from a key ID if the user hasn't manually configured a region, to avoid confusion. The rule is that we'd use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This sounds good to me. Should we throw an exception if the user tries to use I'm leaning towards being explicit, and throwing an exception so that there is no unexpected behavior on the user's end. But I could be swayed. |
||
*/ | ||
public KmsMasterKeyProviderBuilder withKeyId(String keyId) { | ||
withKeyIds(Collections.singletonList(keyId)); | ||
|
||
withDefaultRegion(KmsMasterKeyProvider.getStartingRegion(keyId)); | ||
|
||
return this; | ||
} | ||
|
||
/** | ||
* Adds {@code keyIds}, but does not use them to set the region. | ||
*/ | ||
public KmsMasterKeyProviderBuilder withKeyIds(List<String> keyIds) { | ||
if (this.keyIds == null) { | ||
this.keyIds = new ArrayList<>(); | ||
} | ||
|
||
this.keyIds.addAll(keyIds); | ||
|
||
return this; | ||
} | ||
|
||
/** | ||
* Builds a {@link KmsMasterKeyProvider} using the information it was built with, or with defaults where necessary. | ||
*/ | ||
@Override | ||
public KmsMasterKeyProvider build() { | ||
return build(getSyncClientParams()); | ||
} | ||
|
||
private KmsMasterKeyProvider build(AwsSyncClientParams clientParams) { | ||
keyIds = (keyIds == null) ? Collections.emptyList() : keyIds; | ||
|
||
AWSKMSClient client = new AWSKMSClient(clientParams.getCredentialsProvider(), | ||
clientParams.getClientConfiguration(), | ||
clientParams.getRequestMetricCollector()); | ||
|
||
Region region = determineRegion(); | ||
|
||
return new KmsMasterKeyProvider(client, | ||
region, | ||
keyIds); | ||
} | ||
|
||
private Region determineRegion() { | ||
Region region = RegionUtils.getRegion(this.getRegion()); | ||
|
||
final AwsRegionProvider regionProvider = new DefaultAwsRegionProviderChain(); | ||
|
||
if (region != null) { | ||
return region; | ||
} else { | ||
final String regionName = regionProvider.getRegion(); | ||
if (regionName != null) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. instead of using US_WEST_2 as default, could we use Regions.getCurrentRegion()? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where do you see us using us-west-2 as default? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks like a fallback in the case where the SDK's default region selection logic fails, which is reasonable enough. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @SalusaSecondus, /**
* The default region that new customers in the US are encouraged to use
* when using AWS services for the first time.
*/
public static final Regions DEFAULT_REGION = US_WEST_2; @bdonlan agree with you. looks good to me. |
||
return Region.getRegion(Regions.fromName(regionName)); | ||
} else { | ||
return Region.getRegion(Regions.DEFAULT_REGION); | ||
} | ||
} | ||
} | ||
} |
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.
Examples are great however we should prefer unit test cases as they assert the functionality as part of the build and also show the usage of it.
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.
Yes, and we should also just update existing examples to use the new API instead of adding a new bespoke example.
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.
@bdonlan,
Since you are the contributor and know more about this project than I do.
I have commented it in other PR and asking the same question here again.
Do you think we should put examples as part of the main sdk library,
or we should either create separate maven modules or keep them under src/test?
Usually the tendency is that being it an example, developers can put less secured code and insensitive logging or other kinds of code which could be prone to security threats. e.g. example providing public methods or utils which could reveal secrets. This is the one example I can think of right now however there could be more.
just a thought.
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.
Generally I think if we're checking in example code we should make sure it's production quality example code that we want people to imitate. We should also avoid pulling in new dependencies for examples in the main repository. For the more complex example you refer to, since it involves a lot of moving parts (a client component, a server in lambda, a cloudformation template...) it might be preferable to split it off to avoid confusion - or maybe have it as a separate maven subproject in a subdirectory? I'm not enough of a maven expert to know if that is a good approach here though.
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.
agree with you.
I have maven expertise and will create PR so that you can review if it makes sense.
actually aws-encryption-sdk-java dir will have a pom.xml which links to poms inside 2 subdirectories. It looks something like:
this way, the renaming of repo (aws-encryption-sdk-java) will not be required. And the current and future users will not be affected.
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.
created #32