Defining Keyring interface, RawAesKeyring and RawRsaKeyring. #142

WesleyRosenblum · 2019-11-11T22:40:03Z

Issue #, if available: #102

Description of changes:

This change defines the Keyring interface, RawAesKeyring and RawRsaKeyring.

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.

*Issue #, if available:* #102 *Description of changes:* This change defines the Keyring interface, RawAesKeyring and RawRsaKeyring. 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.

WesleyRosenblum · 2019-11-11T22:42:07Z

Some debate on the visibility of RawAesKeyring and RawRsaKeyring is contained here: 4bbe029#r35839832

SalusaSecondus · 2019-11-11T23:02:50Z

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

+     * @param wrappingKey  The AES key input to AES-GCM to encrypt plaintext data keys.
+     * @return The {@link Keyring}
+     */
+    static Keyring rawAes(String keyNamespace, String keyName, SecretKey wrappingKey) {


As noted in other discussion, please move factory methods off of this interface.

I'll move these to a StandardKeyrings class instead

SalusaSecondus · 2019-11-11T23:30:55Z

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

+        keyringTrace.add(keyNamespace, keyName,
+                KeyringTraceFlag.ENCRYPTED_DATA_KEY,
+                KeyringTraceFlag.SIGNED_ENCRYPTION_CONTEXT,
+                KeyringTraceFlag.VERIFIED_ENCRYPTION_CONTEXT);


Why is "VERIFIED" set on Encrypt?

I misunderstood this issue, I'll fix it

SalusaSecondus · 2019-11-11T23:31:05Z

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

+    void traceOnDecrypt(KeyringTrace keyringTrace) {
+        keyringTrace.add(keyNamespace, keyName,
+                KeyringTraceFlag.DECRYPTED_DATA_KEY,
+                KeyringTraceFlag.SIGNED_ENCRYPTION_CONTEXT,


Why is "SIGNED" set on decrypt?

Same as above

SalusaSecondus · 2019-11-11T23:34:14Z

src/test/java/com/amazonaws/encryptionsdk/keyrings/RawAesKeyringTest.java

+import static org.junit.Assert.assertTrue;
+
+@RunWith(MockitoJUnitRunner.class)
+public class RawAesKeyringTest {


Please add explicit test for the generate case

SalusaSecondus · 2019-11-11T23:48:02Z

src/test/java/com/amazonaws/encryptionsdk/keyrings/RawKeyringTest.java

+import static org.mockito.Mockito.when;
+
+@RunWith(MockitoJUnitRunner.class)
+public class RawKeyringTest {


I admit this is me not fully trusting mocks, but I'd like to see at least some of these tests duplicated with either (or both) of the AES/RSA keyring tests. Those could extend an abstract base test case which implements most of the tests in a generic way.

The problem is that mocks work exactly how we expect/tell them to rather than how reality might. It's important to know that the logic below works when actually implemented in addition to at the abstract level when mocked out.

I've added some tests to the RawAesKeyringTest and RawRsaKeyringTest that work without mocks

SalusaSecondus

LGTM. Please be sure to get a second review by one of our keyring experts.

seebees

Some quick feedback to start us off :)

seebees · 2019-11-13T18:02:17Z

src/main/java/com/amazonaws/encryptionsdk/model/DecryptionMaterials.java

        }

-        public Builder setDataKey(DataKey<?> dataKey) {
-            this.dataKey = dataKey;
+        public Builder setCleartextDataKey(SecretKey cleartextDataKey) {


the other implementations call this the 'unencrypted data key' and the spec uses plaintext. I would pick one of these :)

At this point, the materials should verify that the data key conforms to algorithm specification.
The encrypt materials make sure that this can not be set more than once.
I would do that here as well.

These set methods generally also build the Keyring Trace.
This makes it harder to return materials with incorrect keyring traces.

the other implementations call this the 'unencrypted data key' and the spec uses plaintext. I would pick one of these :)

I'll go with plaintextDataKey to match the spec

At this point, the materials should verify that the data key conforms to algorithm specification.

What do you mean by "At this point"? During the construction of DecryptionMaterials? The plaintextDataKey would be null at this point, it is set in onDecrypt in the Keyring, which uses the Algorithm in the DecryptionMaterials to create the plaintextDataKey:

decryptionMaterials.setCleartextDataKey( new SecretKeySpec(decryptedKey, decryptionMaterials.getAlgorithm().getDataKeyAlgo())); traceOnDecrypt(decryptionMaterials.getKeyringTrace());

The encrypt materials make sure that this can not be set more than once.
I would do that here as well.

This is just the builder, the actual setter for setCleartextDataKey throws an IllegalStateException if it has already been set.

These set methods generally also build the Keyring Trace.
This makes it harder to return materials with incorrect keyring traces.

I went back and forth on that, I wasn't happy with how it turned out when I did it that way, but let me see what I can do

+1 on using plaintextDataKey.

The plaintextDataKey would be null at this point, it is set in onDecrypt in the Keyring, which uses the Algorithm in the DecryptionMaterials to create the plaintextDataKey:

I think that we want to ensure that anyone who builds DecryptionMaterials, if they set a plaintextDataKey, can only successfully do so if the plaintextDataKey being set matches the algorithmSuite already set in the decryptionMaterials. i.e. let's make it harder for custom keyrings to create and return a set of materials which are not valid.

Agreed, I'm adding validation to DecryptionMaterials and EncryptionMaterials in both the setPlaintextDataKey and in the Builder.build methods so that it will be impossible to construct an invalid DecryptionMaterials/EncryptionMaterials

Actually, after discussing with @mattsb42-aws, I'm going to hold off on renaming cleartextDataKey, since that will break existing clients that have written their own CryptoMaterialsManager. Also I will only add the validation to the new setCleartextDataKey methods so that existing clients using the builder will not be broken with new validation requirements

Discussed with @seebees, I'm going to try defining new versions of DecryptionMaterials and EncryptionMaterials for exclusive use in Keyrings

src/main/java/com/amazonaws/encryptionsdk/model/DecryptionMaterials.java

seebees · 2019-11-13T18:06:19Z

src/main/java/com/amazonaws/encryptionsdk/model/EncryptionMaterials.java

@@ -75,6 +75,14 @@ public SecretKey getCleartextDataKey() {
        return cleartextDataKey;
    }

+    public void setCleartextDataKey(SecretKey cleartextDataKey) {
+        if(this.cleartextDataKey != null) {


I would validate the length, similar to the decrypt comment above.

lavaleri · 2019-11-13T19:28:13Z

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

+            }
+        }
+
+        LOGGER.warning("Could not decrypt any data keys");


In the case that onDecrypt gets an empty EDK list the correct behavior is to return. We should not log this warning in that case.

I am also unsure if we want to log a warning here generally. What is the current behavior with the MK/MKP setup?

In the case that onDecrypt gets an empty EDK list the correct behavior is to return. We should not log this warning in that case.

Good catch

I am also unsure if we want to log a warning here generally. What is the current behavior with the MK/MKP setup?

The Java ESDK basically has no logging anywhere. The current behavior with MK/MKP is to throw a CannotUnwrapDataKeyException if no key can be decrypted, but exception throwing is not specified in the spec so I didn't replicate that.

lavaleri · 2019-11-13T20:14:17Z

src/test/java/com/amazonaws/encryptionsdk/keyrings/RawAesKeyringTest.java

+
+        keyring.onEncrypt(encryptionMaterials);
+
+        assertEquals(1, encryptionMaterials.getEncryptedDataKeys().size());


RawAES/RSAKeyrings have different behavior for creating edks (specifically how the AES Keyring serializes the provider Info), so I think we should explicitly test that behavior in the RawAES/RSAKeyring tests by checking the fields of the returned EDKs.

I've added tests that check that the provider ID is as expected and the provider Information has the keyname as a prefix. I don't want to validate beyond that though, since the JceKeyCipher is separately tested and I want that to remain independent.

lavaleri · 2019-11-13T20:32:09Z

src/test/java/com/amazonaws/encryptionsdk/keyrings/RawAesKeyringTest.java

+        assertEquals(2, encryptionMaterials.getKeyringTrace().getEntries().get(0).getFlags().size());
+        assertTrue(encryptionMaterials.getKeyringTrace().getEntries().get(0).getFlags().contains(KeyringTraceFlag.ENCRYPTED_DATA_KEY));
+        assertTrue(encryptionMaterials.getKeyringTrace().getEntries().get(0).getFlags().contains(KeyringTraceFlag.SIGNED_ENCRYPTION_CONTEXT));


This might be overkill, but we can also check that this is the only trace, and that it does not include the "GENERATED" flag.

I do already check that the keyring trace has only 1 entry, and that keyring trace has only 2 flags and those flags are ENCRYPTED_DATA_KEY and SIGNED_ENCRYPTION_CONTEXT

lavaleri · 2019-11-13T20:33:33Z

src/test/java/com/amazonaws/encryptionsdk/keyrings/RawKeyringTest.java

+    @Test
+    public void testDecryptNoValidDataKey() {


The behavior is the same, but I would also add a test for when given an empty EDK list.

lavaleri · 2019-11-13T20:42:32Z

src/main/java/com/amazonaws/encryptionsdk/model/DecryptionMaterials.java

        }

-        public Builder setDataKey(DataKey<?> dataKey) {
-            this.dataKey = dataKey;
+        public Builder setCleartextDataKey(SecretKey cleartextDataKey) {


+1 on using plaintextDataKey.

The plaintextDataKey would be null at this point, it is set in onDecrypt in the Keyring, which uses the Algorithm in the DecryptionMaterials to create the plaintextDataKey:

I think that we want to ensure that anyone who builds DecryptionMaterials, if they set a plaintextDataKey, can only successfully do so if the plaintextDataKey being set matches the algorithmSuite already set in the decryptionMaterials. i.e. let's make it harder for custom keyrings to create and return a set of materials which are not valid.

src/main/java/com/amazonaws/encryptionsdk/internal/DecryptionHandler.java

To maintain backward compatibility with MasterKey/MasterKeyProviders, new EncryptionMaterials and DecryptionMaterials classes are defined for use in Keyrings, so they can include names inline with the spec and additional validation. This change also adds test dependencies for JUnit5.

seebees · 2019-11-18T20:13:47Z

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

+        if (o == null || getClass() != o.getClass()) return false;
+        DecryptionMaterials that = (DecryptionMaterials) o;
+        return algorithm == that.algorithm &&
+                Objects.equals(plaintextDataKey, that.plaintextDataKey) &&


Is this constant time? In the JS ESDK, we touch the plaintextDataKey in a similar way. And it is also true that given the interface it does not really matter. However, we still went to the trouble of making the function constant time to avoid cognitive load on security engineers.

SecretKeySpec relies on MessageDigest.isEqual for byte array comparison, which has been timing safe since Java 6 Update 17: https://www.oracle.com/technetwork/java/javase/6u17-141447.html

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

seebees · 2019-11-18T20:17:42Z

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

+    }
+
+    public PublicKey getVerificationKey() {
+        return verificationKey;


If verificationKey is null because the algorithm is not signed should this throw an error or just return null?

Throwing an error would likely reduce incorrect usage of this class and EncryptionMaterials, so I've gone ahead and made the optional plaintextDataKey, verificationKey, and signingKey throw IllegalStateException if they are not populated. I've also added has* methods so that these properties can be checked if they are populated.

seebees · 2019-11-18T20:19:37Z

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

+    }
+
+    public SecretKey getPlaintextDataKey() {
+        return plaintextDataKey;


If the plaintextDataKey is not yet set, is it an error to request it? Or is it just null?

Throws an IllegalStateException now

seebees · 2019-11-18T20:31:59Z

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

+        notNull(encryptedDataKey, "encryptedDataKey is required");
+        notNull(keyringTraceEntry, "keyringTraceEntry is required");
+        encryptedDataKeys.add(encryptedDataKey);
+        keyringTrace.add(keyringTraceEntry);


I think that there should be some verification that the keyring trace is valid . i.e. the flag DECRYPTED_DATA_KEY is not valid when adding an encrypted data key :(

I would prefer to leave that out of these classes. A user could make their own keyring implementations and decide they don't care about keyring traces, so they shouldn't need to populate it with some flags they don't necessarily care about.

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

seebees · 2019-11-18T20:33:22Z

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

+     * A data key to be used as input for encryption.
+     */
+    public SecretKey getPlaintextDataKey() {
+        return plaintextDataKey;


This may not be as important for decrypt, but trying to get the plaintext data key before it is set is really bad.

Throws an IllegalStateException now

robin-aws · 2019-11-21T18:37:52Z

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

+import java.util.Objects;
+
+import static org.apache.commons.lang3.Validate.isTrue;
+import static org.apache.commons.lang3.Validate.notNull;


Why not java.lang.Objects.requireNonNull?

(Asked with full awareness that this is probably the idiom for the library since before Java 1.7 :)

I've been using Validate.notNull since forever so I hadn't seen requireNonNull before :-) I'll switch it.

Actually I'd like to start using Lombok at some point to handle this stuff with annotations, thoughts on that?

I've been very wary about using Lombok in our open source projects, since it introduces another dependency. There are known interaction issues between Lombok and Kotlin too, and given the choice I'd prefer to support Kotlin better. :)

robin-aws · 2019-11-21T18:47:14Z

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

+            this.keyringTrace = result.keyringTrace;
+        }
+
+        public Builder setAlgorithmSuite(CryptoAlgorithm algorithmSuite) {


The Java SDK v2 style in these builders is to omit the set, so this should just be algorithmSuite.

See for example: https://docs.aws.amazon.com/sdk-for-java/v2/developer-guide/examples-sqs-messages.html

That's a good example to follow, I'll make the change

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

robin-aws · 2019-11-21T18:52:29Z

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

+public interface Keyring {
+
+    /**
+     * Generate a data key if not present and encrypt it using any available wrapping key


The phrase "any available wrapping key" isn't particularly meaningful for me there, especially because some keyrings will encrypt with multiple keys.

Changed it to be more general

robin-aws · 2019-11-21T19:16:33Z

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

+     * decryption of data keys using the provided wrapping key.
+     *
+     * @param keyNamespace A UTF-8 encoded value that, together with the key name, identifies the wrapping key.
+     * @param keyName      A UTF-8 encoded value that, together with the key namespace, identifies the wrapping key.


Nit-pick: these are Java Strings, which means they are internally represented in UTF-16. And that's not really relevant anyway because the underlying JceKeyCipher class will call keyName.getBytes(KEY_NAME_ENCODING) (where KEY_NAME_ENCODING == "UTF-8") and pass on the keyNamespace as a String. "A UTF-8 encoded value" really only makes sense if you're describing a byte[].

I do realize the JceKeyCipher class has the same misleading comments, so perhaps you can punt fixing this for a separate PR for consistency.

Yeah I took that from the RawAesKeyring spec. Should it change there too?

I'll fix it here for now

per updating the spec: I think that an argument can be made that using Java strings and then erroring if there is an issue converting the string to a UTF-8 encoded byte[] later is still following the behavior defined by the specification.

However, there is the issue that in the Java implementation it is possible to initialize keyrings which are unable to produce EDKs (at least for our "standard" keyrings). Is it considered being out of line with the spec to call something a "keyring" if it is unable to perform the behaviors of a keyring? Or is the fact that such a keyring only results in errors mean it's still "in line" with the spec (following the philosophy that: if you can't follow the spec behavior, you MUST fail). For now it is my opinion that we go with the latter approach. Thoughts?

Interesting point - the specification should probably say "Unicode string" instead of "UTF-8 encoded value". It would be silly to use byte arrays everywhere instead of the language-native String type.

robin-aws · 2019-11-21T19:24:21Z

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

+/**
+ * Contains the cryptographic materials needed for a decryption operation with Keyrings.
+ */
+public final class DecryptionMaterials {


I worry a bit that having two Encryption/DecryptionMaterials classes in different packages might be a little confusing. The specification assumes they are the same data structure, and using the same name for different structures there would be REALLY confusing and/or verbose.

My specification PR (awslabs/aws-encryption-sdk-specification#56) deals with part of this issue by introducing a Data Key Materials structure that's used at this level.

I'll leave this for now and we can revisit it after getting some clarity on the spec. I originally had it as the same data structure but there were issues with backwards compatibility that made that undesirable.

I definitely prefer using two different data structures, but if you're going ahead with this I would also cut an issue to track the deviation from the spec - it won't be consistent with either the current version or my PR.

robin-aws

LGTM! My only ask is that we track the deviation from the spec w.r.t. the two different *Materials classes.

WesleyRosenblum requested a review from SalusaSecondus November 11, 2019 22:44

SalusaSecondus suggested changes Nov 11, 2019

View reviewed changes

Moving factory methods to StandardKeyrings and correcting RawAes trace.

b65226b

SalusaSecondus previously approved these changes Nov 12, 2019

View reviewed changes

Adding additional tests for RawAesKeyring and RawRsaKeyring

48c9cdd

WesleyRosenblum dismissed SalusaSecondus’s stale review via 48c9cdd November 13, 2019 00:42

WesleyRosenblum requested review from lavaleri, mattsb42-aws and ttjsu-aws November 13, 2019 00:45

seebees reviewed Nov 13, 2019

View reviewed changes

lavaleri reviewed Nov 13, 2019

View reviewed changes

WesleyRosenblum added 4 commits November 14, 2019 15:16

Minor formatting fixes

8310ae2

Fixing comments and migrating KeyringTraceTest to JUnit5

61d992c

Renaming algorithm to algorithmSuite

f3a9aa6

seebees reviewed Nov 18, 2019

View reviewed changes

Making optional materials properties throw exceptions if not populated.

ebb6166

robin-aws requested changes Nov 21, 2019

View reviewed changes

Using Objects.requireNonNull and renaming builder methods

443d3d2

robin-aws approved these changes Nov 21, 2019

View reviewed changes

WesleyRosenblum merged commit 36958b8 into keyring Nov 21, 2019

WesleyRosenblum deleted the rawkeyring branch November 21, 2019 21:57

mattsb42-aws mentioned this pull request Dec 10, 2019

add keyrings #102

Open

15 tasks


		keyring.onEncrypt(encryptionMaterials);

		assertEquals(1, encryptionMaterials.getEncryptedDataKeys().size());

Defining Keyring interface, RawAesKeyring and RawRsaKeyring. #142

Defining Keyring interface, RawAesKeyring and RawRsaKeyring. #142

Conversation

WesleyRosenblum commented Nov 11, 2019

Check any applicable:

WesleyRosenblum commented Nov 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SalusaSecondus left a comment

Choose a reason for hiding this comment

seebees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WesleyRosenblum Nov 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WesleyRosenblum Nov 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WesleyRosenblum Nov 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robin-aws left a comment

Choose a reason for hiding this comment

WesleyRosenblum Nov 13, 2019 •

edited

Loading

WesleyRosenblum Nov 13, 2019 •

edited

Loading

WesleyRosenblum Nov 21, 2019 •

edited

Loading