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
Expand Up @@ -16,9 +16,13 @@
//@ model import java.util.Arrays;
//@ model import java.nio.charset.StandardCharsets;

import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;

//@ nullable_by_default
public interface EncryptedDataKey {

Charset PROVIDER_ENCODING = StandardCharsets.UTF_8;

//@// An EncryptedDataKey object abstractly contains 3 pieces of data.
//@// These are represented by 3 byte arrays:
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,227 @@
/*
* 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 java.util.Objects.requireNonNull;
import static org.apache.commons.lang3.Validate.isTrue;

/**
* 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 algorithmSuite;
private SecretKey plaintextDataKey;
private final PublicKey verificationKey;
private final Map<String, String> encryptionContext;
private final KeyringTrace keyringTrace;

private DecryptionMaterials(Builder b) {
requireNonNull(b.algorithmSuite, "algorithmSuite is required");
requireNonNull(b.keyringTrace, "keyringTrace is required");
requireNonNull(b.encryptionContext, "encryptionContext is required");
validatePlaintextDataKey(b.algorithmSuite, b.plaintextDataKey);
validateVerificationKey(b.algorithmSuite, b.verificationKey);

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

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

/**
* Returns true if a plaintext data key has been populated.
*
* @return True if plaintext data key is populated, false otherwise.
*/
public boolean hasPlaintextDataKey() {
return this.plaintextDataKey != null;
}

/**
* A data key to be used as input for encryption.
*
* @return The plaintext data key.
* @throws IllegalStateException if plaintext data key has not been populated.
*/
public SecretKey getPlaintextDataKey() throws IllegalStateException {
if (!hasPlaintextDataKey()) {
throw new IllegalStateException("plaintextDataKey has not been populated");
}
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 (hasPlaintextDataKey()) {
throw new IllegalStateException("plaintextDataKey was already populated");
}
requireNonNull(plaintextDataKey, "plaintextDataKey is required");
requireNonNull(keyringTraceEntry, "keyringTraceEntry is required");
validatePlaintextDataKey(algorithmSuite, plaintextDataKey);
this.plaintextDataKey = plaintextDataKey;
keyringTrace.add(keyringTraceEntry);
}

/**
* Returns true if verification key has been populated.
*
* @return True if verification key is populated, false otherwise.
*/
public boolean hasVerificationKey() {
return verificationKey != null;
}

/**
* The verification key used for signature verification.
*
* @return The verification key.
* @throws IllegalStateException if a verification key has not been populated.
*/
public PublicKey getVerificationKey() throws IllegalStateException {
if (!hasVerificationKey()) {
throw new IllegalStateException(String.format(
"Signature verification is not supported by AlgorithmSuite %s", algorithmSuite.name()));
}
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 algorithmSuite == that.algorithmSuite &&
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(algorithmSuite, 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 algorithmSuite, SecretKey plaintextDataKey) throws IllegalArgumentException {
if (plaintextDataKey != null) {
isTrue(algorithmSuite.getDataKeyLength() == plaintextDataKey.getEncoded().length,
String.format("Incorrect key length. Expected %s but got %s",
algorithmSuite.getDataKeyLength(), plaintextDataKey.getEncoded().length));
isTrue(algorithmSuite.getDataKeyAlgo().equalsIgnoreCase(plaintextDataKey.getAlgorithm()),
String.format("Incorrect key algorithm. Expected %s but got %s",
algorithmSuite.getDataKeyAlgo(), plaintextDataKey.getAlgorithm()));
}
}

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

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

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

private Builder(DecryptionMaterials result) {
this.algorithmSuite = result.algorithmSuite;
this.plaintextDataKey = result.plaintextDataKey;
this.verificationKey = result.verificationKey;
this.encryptionContext = result.encryptionContext;
this.keyringTrace = result.keyringTrace;
}

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

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

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

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

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

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