-
Notifications
You must be signed in to change notification settings - Fork 122
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
Changes from 8 commits
c2bcffe
b65226b
48c9cdd
ea1886d
8310ae2
61d992c
f3a9aa6
ebb6166
443d3d2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 org.apache.commons.lang3.Validate.isTrue; | ||
import static org.apache.commons.lang3.Validate.notNull; | ||
|
||
/** | ||
* Contains the cryptographic materials needed for a decryption operation with Keyrings. | ||
*/ | ||
public final class DecryptionMaterials { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
notNull(b.algorithmSuite, "algorithmSuite is required"); | ||
notNull(b.keyringTrace, "keyringTrace is required"); | ||
notNull(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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"); | ||
} | ||
notNull(plaintextDataKey, "plaintextDataKey is required"); | ||
notNull(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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) && | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 setAlgorithmSuite(CryptoAlgorithm algorithmSuite) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Java SDK v2 style in these builders is to omit the See for example: https://docs.aws.amazon.com/sdk-for-java/v2/developer-guide/examples-sqs-messages.html There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good example to follow, I'll make the change |
||
this.algorithmSuite = algorithmSuite; | ||
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); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. :)