Defining the KMS Keyring #147

WesleyRosenblum · 2019-11-22T03:18:53Z

Description of changes:

This change contains the initial implementation of the KMS Keyring, as well as a refactoring of the existing KMS logic into a DataKeyEncryptionDao that is shared between the KMS Keyring and the KMS Master Key.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Check any applicable:

Were any files moved? Moving files changes their URL, which breaks all hyperlinks to the files.

*Description of changes:* Defining the KMS Keyring. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. # Check any applicable: - [ ] Were any files moved? Moving files changes their URL, which breaks all hyperlinks to the files.

lavaleri

Couple of questions, but otherwise the KMS keyring logic LGTM. Note that awslabs/aws-encryption-sdk-specification#50 is still open (specifies behavior with client supplier when a region can't be grabbed from the arn), and could impact this PR if more changes are suggested.

You should also get this reviewed by someone with more Java experience, and more familiar with the current MKP behavior.

lavaleri · 2019-11-25T19:38:18Z

src/main/java/com/amazonaws/encryptionsdk/kms/KmsMasterKey.java

+                        result.getPlaintextDataKey(),
+                        edk.getEncryptedDataKey(),
+                        edk.getProviderInformation(), this);
+            } catch (final AwsCryptoException | MismatchedDataKeyException ex) {


Do we have a unit test that ensures this behavior is maintained? (I can't seem to find one, or other unit tests for KMSMasterKey :()

KMSMasterKey is covered by LegacyKMSMasterKeyProviderTests. I've added a separate KmsMasterKeyTest to cover the case of a MismatchedDataKeyException.

lavaleri · 2019-11-25T19:43:04Z

src/main/java/com/amazonaws/encryptionsdk/kms/KmsUtils.java

+    }
+
+    private static boolean isKeyAlias(String arn) {
+        return arn.startsWith(ALIAS_PREFIX) && !arn.contains(":");


Where does the !arn.contains(":") requirement come from?

I was thinking I wanted to return false for an alias ARN, that still contains the ALIAS_PREFIX (though not as a prefix). But since an alias ARN would not have that as a prefix, checking for : shouldn't be necessary.

lavaleri · 2019-11-25T19:51:59Z

src/main/java/com/amazonaws/encryptionsdk/keyrings/KmsKeyring.java

+                encryptionMaterials.getPlaintextDataKey(), encryptionMaterials.getEncryptionContext());
+
+        encryptionMaterials.addEncryptedDataKey(encryptedDataKey,
+                new KeyringTraceEntry(KMS_PROVIDER_ID, keyId, KeyringTraceFlag.ENCRYPTED_DATA_KEY, KeyringTraceFlag.SIGNED_ENCRYPTION_CONTEXT));


Did we determine whether this should be the keyId the user inputs, or the keyId returned from the KMS result (and is it in the spec/do we have an issue cut to update the spec)?

I think we were leaning toward the keyId the user inputs, which is what I went with, but it wasn't settled. Either way, the spec still just says "generator" which is wrong, I've created an issue: awslabs/aws-encryption-sdk-specification#60

src/main/java/com/amazonaws/encryptionsdk/kms/KmsUtils.java

SalusaSecondus

I did not review the tests. Minor issues found, but nothing major.

SalusaSecondus · 2019-12-16T19:53:52Z

src/main/java/com/amazonaws/encryptionsdk/exception/MismatchedDataKeyException.java

+ * This exception is thrown when the key used by KMS to decrypt a data key does not
+ * match the provider information contained within the encrypted data key.
+ */
+public class MismatchedDataKeyException extends IllegalStateException {


I don't think that this is technically an IllegalStateException. According to the documentation.

Signals that a method has been invoked at an illegal or inappropriate time. In other words, the Java environment or Java application is not in an appropriate state for the requested operation.

Originally I wanted this to not be an AwsCryptoException so I could handle it differently, and I was thinking it indicated KMS was in a bad state, so IllegalStateException seemed ok. Now looking over the code again, I can just make it extend AwsCryptoException. I added another unit test for KmsKeyring to ensure the behavior I wanted is maintained for this exception.

src/main/java/com/amazonaws/encryptionsdk/keyrings/KmsKeyring.java

SalusaSecondus · 2019-12-16T19:58:48Z

src/main/java/com/amazonaws/encryptionsdk/keyrings/KmsKeyring.java

+                .filter(edk -> edk.getProviderId().equals(KMS_PROVIDER_ID))
+                .map(edk -> new String(edk.getProviderInformation(), PROVIDER_ENCODING))
+                .allMatch(KmsUtils::isArnWellFormed)) {
+            throw new MalformedArnException("encryptedDataKeys contains a malformed ARN");


Why should a single malformed ARN cause decryption failure? So long as we can find a single valid ARN and decrypt it, we should succeed.

My thinking was that since decryption failures are ignored, as a user of this method I would like the method to fail fast if I am passing in bad inputs. Otherwise I would not understand why a particular key was not being used for decryption.

We should be flexible in what we accept (when it doesn't conflict with security). What if a newer version of the ESDK is delivered which supports a new form of ARNs? Older versions should still be able to decrypt data associated with the old-style of understood ARNs.

If no data keys can be decrypted, then this error should be propagated up. That's useful.

I disagree. The question is, how did such a value get in the the EDK in the first place?
I guess the question is "what is bad input"? if someone were to create a new KMS Keyring, that is not quite right and sometimes puts the wrong thing in the ProviderID, is this an example of a bad input?

@seebees which point are you disagreeing with?

SalusaSecondus · 2019-12-16T20:00:30Z

src/main/java/com/amazonaws/encryptionsdk/keyrings/KmsKeyring.java

+            throw new MalformedArnException("encryptedDataKeys contains a malformed ARN");
+        }
+
+        for (EncryptedDataKey encryptedDataKey : encryptedDataKeys) {


This is a potential O(n^2) operation. We iterate over each EDK and then in okToDecrypt iterate over every keyId we have configured. This seems like a potentially inefficient way to do this.

I'll put all the configured keys into a HashSet to improve performance

SalusaSecondus · 2019-12-16T20:04:30Z

src/main/java/com/amazonaws/encryptionsdk/kms/KmsClientSupplier.java

+         * @return The proxy
+         */
+        private AWSKMS newCachingProxy(AWSKMS client, String regionId) {
+            return (AWSKMS) Proxy.newProxyInstance(


It's worth keeping an eye on this. Proxies can cause performance problems sometimes as they depend on Java reflection APIs.

Sure, though I hope any proxy overhead would be insignificant compared to the actual service call to KMS.

Will address after decision is made on supporting raw key IDs

WesleyRosenblum requested review from lavaleri, mattsb42-aws, robin-aws and seebees November 22, 2019 03:19

lavaleri reviewed Nov 25, 2019

View reviewed changes

Adding test for MismatchedDataKey and updating StandardKeyrings

b8d146e

mattsb42-aws mentioned this pull request Dec 10, 2019

add keyrings #102

Open

15 tasks

Adding builder for KmsClientSupplier to support easy client setup

4f8f518

seebees previously requested changes Dec 11, 2019

View reviewed changes

src/main/java/com/amazonaws/encryptionsdk/kms/KmsUtils.java Show resolved Hide resolved

WesleyRosenblum added 2 commits December 11, 2019 16:43

Support raw key IDs in KmsMasterKey

25952ee

Reducing visibility of KmsKeyring

ff95acb

SalusaSecondus suggested changes Dec 16, 2019

View reviewed changes

WesleyRosenblum added 5 commits December 16, 2019 13:17

Don't attempt encryption with a null generator

05fae28

Make MismatchedDataKeyException an AwsCryptoException

aa48443

Optimizing O(n^2) operation in onDecrypt

ca42177

Making defensive copy of keyIds

55c2f3c

Skip malformed arns in OnDecrypt instead of failing

01e33d1

SalusaSecondus previously approved these changes Dec 18, 2019

View reviewed changes

Merge branch 'keyring' into kms

a5abeb6

WesleyRosenblum dismissed SalusaSecondus’s stale review via a5abeb6 December 18, 2019 21:38

WesleyRosenblum removed request for mattsb42-aws and robin-aws December 18, 2019 21:57

SalusaSecondus approved these changes Dec 18, 2019

View reviewed changes

WesleyRosenblum merged commit 77f46c2 into keyring Dec 18, 2019

WesleyRosenblum deleted the kms branch December 18, 2019 22:05

Defining the KMS Keyring #147

Defining the KMS Keyring #147

Uh oh!

Conversation

WesleyRosenblum commented Nov 22, 2019

Check any applicable:

Uh oh!

lavaleri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SalusaSecondus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!