Refactor JceMasterKey to extract logic to be shared by raw keyrings. #139

WesleyRosenblum · 2019-11-01T00:21:50Z

Issue #, if available: #102

Description of changes:

In anticipation of the RawAesKeyring and RawRsaKeyring needing logic currently embedded in the JceMasterKey, this change extracts that logic into the JceKeyCipher class so it may be shared.

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

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

*Issue #, if available:* #102 *Description of changes:* In anticipation of the RawAesKeyring and RawRsaKeyring needing logic currently embedded in the JceMasterKey, this change extracts that logic into the JceKeyCipher class so it may be shared. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. - [ ] Were any files moved? Moving files changes their URL, which breaks all hyperlinks to the files.

…f SecretKey

…o Utils

WesleyRosenblum · 2019-11-08T19:18:50Z

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

+            if (tagLen != TAG_LENGTH) {
+                throw new InvalidKeyException(String.format("Authentication tag length must be %s", TAG_LENGTH));
+            }
+
+            if (nonceLen != NONCE_LENGTH) {
+                throw new InvalidKeyException(String.format("Initialization vector (IV) length must be %s", NONCE_LENGTH));
+            }


This is the only part of this PR that isn't a refactor. I made this change to bring this in line with the RawAesKeyring spec

SalusaSecondus · 2019-11-08T19:25:55Z

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

+            final byte[] provInfo = new byte[keyNameBytes.length + wData.extraInfo.length];
+            System.arraycopy(keyNameBytes, 0, provInfo, 0, keyNameBytes.length);
+            System.arraycopy(wData.extraInfo, 0, provInfo, keyNameBytes.length, wData.extraInfo.length);


If wData.extraInfo.length==0, then we should short-circuit this to avoid extra data-copies and allocation.

SalusaSecondus · 2019-11-08T19:27:55Z

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

+
+    private static byte[] specToBytes(final GCMParameterSpec spec) {
+        final byte[] nonce = spec.getIV();
+        final ByteArrayOutputStream baos = new ByteArrayOutputStream();


NitPick: Since we know exactly how long this will be, it's probably more efficient to use a ByteBuffer than a ByteArrayOutputStream.

Changed to ByteBuffer, it simplifies the code as well

SalusaSecondus · 2019-11-08T19:29:41Z

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

+            dis.readFully(nonce);
+            return new GCMParameterSpec(tagLen, nonce);
+        } catch (final IOException ex) {
+            throw new AssertionError("Impossible exception", ex);


Really impossible? I suspect that if the data is too short you'll get an IOException. I'd just rethrow it as an InvalidKeyException.

Now that I'm using a ByteBuffer it is impossible (ByteBuffer doesn't throw IOExceptions). I'm explicitly checking the data size against the nonce length now.

SalusaSecondus · 2019-11-08T19:29:57Z

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

+    }
+
+    private static GCMParameterSpec bytesToSpec(final byte[] data, final int offset) throws InvalidKeyException {
+        final ByteArrayInputStream bais = new ByteArrayInputStream(data, offset, data.length - offset);


NitPick: Same comment about ByteBuffer probably being more efficient.

SalusaSecondus · 2019-11-08T19:35:15Z

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

+    private static final int NONCE_LENGTH = 12;
+    private static final int TAG_LENGTH = 128;
+    private static final String TRANSFORMATION = "AES/GCM/NoPadding";
+    private final SecureRandom rnd = new SecureRandom();


Remove this and use Utils.getSecureRandom() exactly when you need it. (This avoids both creating more instances than you need and thread-contention.)

Yeah, I didn't see Utils.getSecureRandom until recently and forgot to go back and update this change

SalusaSecondus · 2019-11-08T19:35:49Z

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

+    WrappingData buildWrappingCipher(final Key key, final Map<String, String> encryptionContext)
+            throws GeneralSecurityException {
+        final byte[] nonce = new byte[NONCE_LENGTH];
+        rnd.nextBytes(nonce);


Use Utils.getSecureRandom().nextBytes(nonce);

SalusaSecondus · 2019-11-08T19:38:38Z

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

+        // We require BouncyCastle to avoid some bugs in the default Java implementation
+        // of OAEP.


This is an old comment and should be removed.

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

SalusaSecondus · 2019-11-08T19:39:50Z

src/main/java/com/amazonaws/encryptionsdk/jce/JceMasterKey.java

-    private static final Logger LOGGER = Logger.getLogger(JceMasterKey.class.getName());
-    private static final byte[] EMPTY_ARRAY = new byte[0];
-
+public class JceMasterKey extends MasterKey<JceMasterKey> {
    private final SecureRandom rnd = new SecureRandom();


Remove this.

WesleyRosenblum

Made suggested changes

SalusaSecondus · 2019-11-08T21:23:40Z

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

-                throw new InvalidKeyException(String.format("Authentication tag length must be %s", TAG_LENGTH));
-            }
+        final int tagLen = buffer.getInt();
+        final int nonceLen = buffer.getInt();


If the length is too short, youBufferUnderflowException which should be caught and rethrown as an InvalidKeyException.

Defined a SPEC_LENGTH constant that I validate against

SalusaSecondus

LGTM

* Add a basic example for encrypting and decrypting with a KMS CMK (#136) * *Issue #, if available:* #108 *Description of changes:* Add a basic example for encrypting and decrypting with a KMS CMK. 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. * Add test and Maven plugin to include examples directory as test source * Update docs in prep for 1.6.1 (#133) * Update docs in prep for 1.6.1 * Actually bump version for release * Fix for new versions of gpg * Refactor JceMasterKey to extract logic to be shared by raw keyrings. (#139) * Refactor JceMasterKey to extract logic to be shared by raw keyrings. *Issue #, if available:* #102 *Description of changes:* In anticipation of the RawAesKeyring and RawRsaKeyring needing logic currently embedded in the JceMasterKey, this change extracts that logic into the JceKeyCipher class so it may be shared. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. - [ ] Were any files moved? Moving files changes their URL, which breaks all hyperlinks to the files. * fix: The final frame can not be larger than the Frame Length (#166) * Add validation to ensure the length of the final frame in the final frame header does not exceed the frame size specified in the message header. * Validate that frame length is positive for framed data * Reverting removal of variable frame length code * Reverting removal of variable frame length code * Fix spacing after if Co-authored-by: SalusaSecondus <[email protected]> Co-authored-by: Greg Rubin <[email protected]>

WesleyRosenblum requested a review from SalusaSecondus November 1, 2019 00:22

WesleyRosenblum added 6 commits October 31, 2019 18:45

Remove requirement that wrappingKey and unwrappingKey be non null

78e956b

Simplifying JceKeyCipher methods

28ec657

Changing encryptKey and decryptKey methods to use key bytes instead o…

66b3838

…f SecretKey

Removed unused imports

f789a23

Adding validation to AesGcm key cipher and moving ArrayPrefixEquals t…

7e2dde6

…o Utils

Set provider ID correctly in the encrypted data key.

c6bd363

WesleyRosenblum commented Nov 8, 2019

View reviewed changes

SalusaSecondus suggested changes Nov 8, 2019

View reviewed changes

Using ByteByffer instead of ByteArrayInput/OutputStreams

9d248fb

WesleyRosenblum commented Nov 8, 2019

View reviewed changes

SalusaSecondus suggested changes Nov 8, 2019

View reviewed changes

Ensure spec length is correct

209c7ec

SalusaSecondus approved these changes Nov 8, 2019

View reviewed changes

WesleyRosenblum merged commit 4fdc309 into master Nov 8, 2019

WesleyRosenblum deleted the jcekeycipher branch February 12, 2020 02:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor JceMasterKey to extract logic to be shared by raw keyrings. #139

Refactor JceMasterKey to extract logic to be shared by raw keyrings. #139

WesleyRosenblum commented Nov 1, 2019

WesleyRosenblum Nov 8, 2019

SalusaSecondus Nov 8, 2019

WesleyRosenblum Nov 8, 2019

SalusaSecondus Nov 8, 2019

WesleyRosenblum Nov 8, 2019

SalusaSecondus Nov 8, 2019

WesleyRosenblum Nov 8, 2019

SalusaSecondus Nov 8, 2019

WesleyRosenblum Nov 8, 2019

SalusaSecondus Nov 8, 2019

WesleyRosenblum Nov 8, 2019

SalusaSecondus Nov 8, 2019

WesleyRosenblum Nov 8, 2019

SalusaSecondus Nov 8, 2019

WesleyRosenblum Nov 8, 2019

SalusaSecondus Nov 8, 2019

WesleyRosenblum Nov 8, 2019

WesleyRosenblum left a comment

SalusaSecondus Nov 8, 2019

WesleyRosenblum Nov 8, 2019

SalusaSecondus left a comment

		// We require BouncyCastle to avoid some bugs in the default Java implementation
		// of OAEP.

Refactor JceMasterKey to extract logic to be shared by raw keyrings. #139

Refactor JceMasterKey to extract logic to be shared by raw keyrings. #139

Conversation

WesleyRosenblum commented Nov 1, 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

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 left a 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