Skip to content

Defining Keyring interface, RawAesKeyring and RawRsaKeyring. #142

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

Merged
merged 9 commits into from
Nov 21, 2019
19 changes: 13 additions & 6 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -53,16 +53,23 @@
</dependency>

<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
<version>2.28.1</version>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-engine</artifactId>
<version>5.5.2</version>
<scope>test</scope>
</dependency>

<dependency>
<groupId>org.junit.vintage</groupId>
<artifactId>junit-vintage-engine</artifactId>
<version>5.5.2</version>
<scope>test</scope>
</dependency>

<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<version>4.12</version>
<groupId>org.mockito</groupId>
<artifactId>mockito-junit-jupiter</artifactId>
<version>3.1.0</version>
<scope>test</scope>
</dependency>

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,189 @@
/*
* Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License"). You may not use this file except
* in compliance with the License. A copy of the License is located at
*
* http://aws.amazon.com/apache2.0
*
* or in the "license" file accompanying this file. This file is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
* specific language governing permissions and limitations under the License.
*/

package com.amazonaws.encryptionsdk.keyrings;

import com.amazonaws.encryptionsdk.CryptoAlgorithm;

import javax.crypto.SecretKey;
import java.security.PublicKey;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;

import static org.apache.commons.lang3.Validate.isTrue;
import static org.apache.commons.lang3.Validate.notNull;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not java.lang.Objects.requireNonNull?

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

Copy link
Contributor Author

@WesleyRosenblum WesleyRosenblum Nov 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private final CryptoAlgorithm algorithm;
private SecretKey plaintextDataKey;
private final PublicKey verificationKey;
private final Map<String, String> encryptionContext;
private final KeyringTrace keyringTrace;

private DecryptionMaterials(Builder b) {
notNull(b.algorithm, "algorithm is required");
notNull(b.keyringTrace, "keyringTrace is required");
validatePlaintextDataKey(b.algorithm, b.plaintextDataKey);
validateVerificationKey(b.algorithm, b.verificationKey);

algorithm = b.algorithm;
plaintextDataKey = b.plaintextDataKey;
verificationKey = b.verificationKey;
encryptionContext = b.encryptionContext;
keyringTrace = b.keyringTrace;
}

/**
* The algorithm to use for this decryption operation.
*/
public CryptoAlgorithm getAlgorithm() {
return algorithm;
}

public SecretKey getPlaintextDataKey() {
return plaintextDataKey;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throws an IllegalStateException now

}

/**
* Sets the plaintext data key. The plaintext data key must not already be populated.
*
* @param plaintextDataKey The plaintext data key.
* @param keyringTraceEntry The keyring trace entry recording this action.
*/
public void setPlaintextDataKey(SecretKey plaintextDataKey, KeyringTraceEntry keyringTraceEntry) {
if (this.plaintextDataKey != null) {
throw new IllegalStateException("plaintextDataKey was already populated");
}
notNull(plaintextDataKey, "plaintextDataKey is required");
notNull(keyringTraceEntry, "keyringTraceEntry is required");
validatePlaintextDataKey(algorithm, plaintextDataKey);
this.plaintextDataKey = plaintextDataKey;
keyringTrace.add(keyringTraceEntry);
}

public PublicKey getVerificationKey() {
return verificationKey;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

public Map<String, String> getEncryptionContext() {
return encryptionContext;
}

public KeyringTrace getKeyringTrace() {
return keyringTrace;
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
DecryptionMaterials that = (DecryptionMaterials) o;
return algorithm == that.algorithm &&
Objects.equals(plaintextDataKey, that.plaintextDataKey) &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Objects.equals(verificationKey, that.verificationKey) &&
Objects.equals(encryptionContext, that.encryptionContext) &&
Objects.equals(keyringTrace, that.keyringTrace);
}

@Override
public int hashCode() {
return Objects.hash(algorithm, plaintextDataKey, verificationKey, encryptionContext, keyringTrace);
}

public static Builder newBuilder(CryptoAlgorithm algorithm) {
return new Builder(algorithm);
}

public Builder toBuilder() {
return new Builder(this);
}

private void validatePlaintextDataKey(CryptoAlgorithm algorithm, SecretKey plaintextDataKey) throws IllegalArgumentException {
if (plaintextDataKey != null) {
isTrue(algorithm.getDataKeyLength() == plaintextDataKey.getEncoded().length,
String.format("Incorrect key length. Expected %s but got %s",
algorithm.getDataKeyLength(), plaintextDataKey.getEncoded().length));
isTrue(algorithm.getDataKeyAlgo().equalsIgnoreCase(plaintextDataKey.getAlgorithm()),
String.format("Incorrect key algorithm. Expected %s but got %s",
algorithm.getDataKeyAlgo(), plaintextDataKey.getAlgorithm()));
}
}

/**
* Validates that a verification key is specified only if and only if
* the given algorithm suite supports signature verification.
*/
private void validateVerificationKey(CryptoAlgorithm algorithm, PublicKey verificationKey) throws IllegalArgumentException {
if (algorithm.getTrailingSignatureAlgo() == null && verificationKey != null) {
throw new IllegalArgumentException(
String.format("Algorithm %s does not support signature verification", algorithm.name()));
} else if (algorithm.getTrailingSignatureAlgo() != null && verificationKey == null) {
throw new IllegalArgumentException(
String.format("Algorithm %s requires a verification key for signature verification", algorithm.name()));
}
}

public static final class Builder {
private CryptoAlgorithm algorithm;
private SecretKey plaintextDataKey;
private PublicKey verificationKey;
private Map<String, String> encryptionContext;
private KeyringTrace keyringTrace = new KeyringTrace();

private Builder(CryptoAlgorithm algorithm) {
this.algorithm = algorithm;
}

private Builder(DecryptionMaterials result) {
this.algorithm = result.getAlgorithm();
this.plaintextDataKey = result.getPlaintextDataKey();
this.verificationKey = result.getVerificationKey();
this.encryptionContext = result.getEncryptionContext();
this.keyringTrace = result.getKeyringTrace();
}

public Builder setAlgorithm(CryptoAlgorithm algorithm) {
this.algorithm = algorithm;
return this;
}

public Builder setPlaintextDataKey(SecretKey plaintextDataKey) {
this.plaintextDataKey = plaintextDataKey;
return this;
}

public Builder setVerificationKey(PublicKey verificationKey) {
this.verificationKey = verificationKey;
return this;
}

public Builder setEncryptionContext(Map<String, String> encryptionContext) {
this.encryptionContext = Collections.unmodifiableMap(new HashMap<>(encryptionContext));
return this;
}

public Builder setKeyringTrace(KeyringTrace keyringTrace) {
this.keyringTrace = keyringTrace;
return this;
}

public DecryptionMaterials build() {
return new DecryptionMaterials(this);
}
}
}
Loading