Skip to content

Commit

Permalink
Defining Keyring interface and implementing RawKeyring.
Browse files Browse the repository at this point in the history
*Issue #, if available:* #102

*Description of changes:*

This change defines the Keyring interface and an implementation of a RawKeyring which supports both AES and RSA.

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.
  • Loading branch information
WesleyRosenblum committed Nov 7, 2019
1 parent 2fe1f0d commit 4bbe029
Show file tree
Hide file tree
Showing 9 changed files with 321 additions and 38 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -131,9 +131,11 @@ public DefaultCryptoMaterialsManager(MasterKeyProvider<?> mkp) {
}

return DecryptionMaterials.newBuilder()
.setDataKey(dataKey)
.setTrailingSignatureKey(pubKey)
.build();
.setAlgorithm(request.getAlgorithm())
.setCleartextDataKey(dataKey.getKey())
.setMasterKey(dataKey.getMasterKey())
.setTrailingSignatureKey(pubKey)
.build();
}

private PublicKey deserializeTrailingKeyFromEc(CryptoAlgorithm algo, String pubKey) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@

import com.amazonaws.encryptionsdk.CryptoAlgorithm;
import com.amazonaws.encryptionsdk.CryptoMaterialsManager;
import com.amazonaws.encryptionsdk.DataKey;
import com.amazonaws.encryptionsdk.DefaultCryptoMaterialsManager;
import com.amazonaws.encryptionsdk.MasterKey;
import com.amazonaws.encryptionsdk.MasterKeyProvider;
Expand Down Expand Up @@ -61,7 +60,8 @@ public class DecryptionHandler<K extends MasterKey<K>> implements MessageCryptoH

private CryptoHandler contentCryptoHandler_;

private DataKey<K> dataKey_;
private SecretKey dataKey_;
private K masterKey_;
private SecretKey decryptionKey_;
private CryptoAlgorithm cryptoAlgo_;
private Signature trailingSig_;
Expand Down Expand Up @@ -454,12 +454,13 @@ private void readHeaderFields(final CiphertextHeaders ciphertextHeaders) {

DecryptionMaterials result = materialsManager_.decryptMaterials(request);

dataKey_ = result.getCleartextDataKey();
//noinspection unchecked
dataKey_ = (DataKey<K>)result.getDataKey();
masterKey_ = (K)result.getMasterKey();
PublicKey trailingPublicKey = result.getTrailingSignatureKey();

try {
decryptionKey_ = cryptoAlgo_.getEncryptionKeyFromDataKey(dataKey_.getKey(), ciphertextHeaders);
decryptionKey_ = cryptoAlgo_.getEncryptionKeyFromDataKey(dataKey_, ciphertextHeaders);
} catch (final InvalidKeyException ex) {
throw new AwsCryptoException(ex);
}
Expand Down Expand Up @@ -536,7 +537,7 @@ public CiphertextHeaders getHeaders() {

@Override
public List<K> getMasterKeys() {
return Collections.singletonList(dataKey_.getMasterKey());
return Collections.singletonList(masterKey_);
}

@Override
Expand Down
73 changes: 73 additions & 0 deletions src/main/java/com/amazonaws/encryptionsdk/keyrings/Keyring.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/*
* 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.EncryptedDataKey;
import com.amazonaws.encryptionsdk.model.DecryptionMaterials;
import com.amazonaws.encryptionsdk.model.EncryptionMaterials;

import javax.crypto.SecretKey;
import java.security.PrivateKey;
import java.security.PublicKey;
import java.util.List;

/**
* Keyrings are responsible for the generation, encryption, and decryption of data keys.
*/
public interface Keyring {

/**
* Generate a data key if not present and encrypt it using any available wrapping key
*
* @param encryptionMaterials Materials needed for encryption that the keyring may modify.
*/
void onEncrypt(EncryptionMaterials encryptionMaterials);

/**
* Attempt to decrypt the encrypted data keys
*
* @param decryptionMaterials Materials needed for decryption that the keyring may modify.
* @param encryptedDataKeys List of encrypted data keys.
*/
void onDecrypt(DecryptionMaterials decryptionMaterials, List<EncryptedDataKey> encryptedDataKeys);

/**
* Constructs a {@link Keyring} which does local AES-GCM encryption
* 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.
* @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) {

This comment has been minimized.

Copy link
@mattsb42-aws

mattsb42-aws Nov 7, 2019

Member

Generally speaking, we've avoided trying to have any sort of central "registry" like this and instead opted for just providing people with the classes to create their own instances directly. I'm not sure what value this method and the rawRsa method would add, and they seem like they would just add maintenance overhead and complexity.

This comment has been minimized.

Copy link
@WesleyRosenblum

WesleyRosenblum Nov 7, 2019

Author Contributor

I thought it was more user-friendly. Instead of having to know the names of the keyring implementations, they are easily accessible in the interface. It also aligns somewhat with the spec, which calls out all the supported keyrings. It also lets us restructure the underlying implementations later on. Also, since I don't have separate RawRsaKeyring and RawAesKeyring classes, I thought this better than using the RawKeyring.aes and RawKeyring.rsa factory methods. Though if I do end up making explicit RawRsaKeyring and RawAesKeyring classes that would also address that.

I guess I'm somewhat uncomfortable for our clients to have direct access to the implementation classes and this way directs them to use the Keyring interface only in their code.

This comment has been minimized.

Copy link
@mattsb42-aws

mattsb42-aws Nov 7, 2019

Member

The "supported keyrings" in the spec just means they keyrings that we happen to provide. The keyrings that we vend should not have a different user experience than the keyrings that someone else might make.

I'm not opposed to the concept of a keyring registry, but I originally tried to do this with the Python client and could not find a solution that worked well.

I guess I'm somewhat uncomfortable for our clients to have direct access to the implementation classes

Why? We need to build them to be compliant with the spec and the spec defines their interfaces and behaviors. There is intentionally not very much leeway in what their external APIs do.

This comment has been minimized.

Copy link
@WesleyRosenblum

WesleyRosenblum Nov 7, 2019

Author Contributor

I originally tried to do this with the Python client and could not find a solution that worked well.
Do you recall what didn't work well about it?

Why? We need to build them to be compliant with the spec and the spec defines their interfaces and behaviors. There is intentionally not very much leeway in what their external APIs do.

It makes it harder to change the underlying implementations later on. For example, if we go with having separate RawRsaKeyring and RawAesKeyring classes, but later on change our mind and decide that one class can cover both of them, then we can't do that refactoring since the client is already depending directly on these implementation classes.

This comment has been minimized.

Copy link
@mattsb42-aws

mattsb42-aws Nov 7, 2019

Member

I originally tried to do this with the Python client and could not find a solution that worked well.

Do you recall what didn't work well about it?

Given that the keyrings that we vend must not be special, how does an arbitrary user creating their own keyring integrate their keyring into the keyring registry?

The crux of this is that our keyrings should not be special: the UX for a user using a keyring that we create vs using a keyring that they created vs using a keyring that some third party created should not be notably different.

It makes it harder to change the underlying implementations later on. For example, if we go with having separate RawRsaKeyring and RawAesKeyring classes, but later on change our mind and decide that one class can cover both of them, then we can't do that refactoring since the client is already depending directly on these implementation classes.

I'm actually not convinced that this is a problem. In that scenario, we would still want to retain the class structure to avoid breaking users that are expecting specific classes.

I would argue that the top-level API of "class X implements keyring Y defined in specification Z version m.n with constructor options P, Q, and R" is not something that should change. We can massage the constructor UX in backwards compatible ways, but I think that changing anything more fundamental about the public UX of the class is not something that we should do.

This comment has been minimized.

Copy link
@WesleyRosenblum

WesleyRosenblum Nov 7, 2019

Author Contributor

Given that the keyrings that we vend must not be special, how does an arbitrary user creating their own keyring integrate their keyring into the keyring registry?

I guess I don't see it as a registry, they are just Factory methods to control instantiation. If a user is creating their own keyrings they can control instantiation however they want for their keyrings, we shouldn't be concerned about that.

The crux of this is that our keyrings should not be special: the UX for a user using a keyring that we create vs using a keyring that they created vs using a keyring that some third party created should not be notably different.

Using the keyring isn't any different, its just the instantiation that is potentially different. I think that small difference is worth it to maintain more control and be more maintainable in the long run.

In that scenario, we would still want to retain the class structure to avoid breaking users that are expecting specific classes.

That is kind of the point though, the user has no access to the specific classes so we don't break them if the underlying implementations change.

I think that changing anything more fundamental about the public UX of the class is not something that we should do.

But I'm not talking about changing the public UX of the class, which is only defined by the Keyring interface and factory methods. I think leaking out these implementation classes to our users has no real benefit to the user and just restricts our freedom to make structural improvements later on.

This comment has been minimized.

Copy link
@seebees

seebees Nov 8, 2019

Contributor

Getting to the party late:

The Keyring interfaces are complicated.
For customers who want to consume the Encryption SDK,
and just get to the encrypt/decrypt function,
they are "private".
But the Keyring interface is not just for these customers.
For a customer who wants to develop their own Keyrings,
these interfaces are "public".
I'm not a huge OOP fan, but, in that world,
what if I want a different RSA Keyring that is "that but also",
you would inherit from the base class and extend, yes?

This comment has been minimized.

Copy link
@WesleyRosenblum

WesleyRosenblum Nov 8, 2019

Author Contributor

If that case you could still do something like:

public class MyCustomRawAesKeyring implements Keyring {
    
    private final Keyring baseKeyring;

    public MyCustomRawAesKeyring(String namespace, String name, SecretKey wrappingKey) {
        this.baseKeyring = Keyring.rawAes(namespace, name, wrappingKey);
    }

    @Override
    public void onEncrypt(EncryptionMaterials encryptionMaterials) {
        baseKeyring.onEncrypt(encryptionMaterials);
        //Do something else
        
    }

    @Override
    public void onDecrypt(DecryptionMaterials decryptionMaterials, List<EncryptedDataKey> encryptedDataKeys) {
        baseKeyring.onDecrypt(decryptionMaterials, encryptedDataKeys);
        //Do something else
    }
}

This comment has been minimized.

Copy link
@robin-aws

robin-aws Nov 8, 2019

Contributor

Getting to the party even later than @seebees :)

It makes it harder to change the underlying implementations later on. For example, if we go with having separate RawRsaKeyring and RawAesKeyring classes, but later on change our mind and decide that one class can cover both of them, then we can't do that refactoring since the client is already depending directly on these implementation classes.

I think that's just an argument for factories or builder methods, if we want to have that freedom.

I would argue that the top-level API of "class X implements keyring Y defined in specification Z version m.n with constructor options P, Q, and R" is not something that should change. We can massage the constructor UX in backwards compatible ways, but I think that changing anything more fundamental about the public UX of the class is not something that we should do.

Rather than the specification saying "there must exist a class RawRsaKeyring with the following properties..." I'd prefer it said "there must exist a method makeRawRsaKeyring, and the result of calling that method has the following properties". Independently of that we could say the specific class exists, if we really want to provide it as an abstract base class.

This comment has been minimized.

Copy link
@WesleyRosenblum

WesleyRosenblum Nov 9, 2019

Author Contributor

I think that's just an argument for factories or builder methods, if we want to have that freedom.

Yes, I'm arguing for static factory methods on the Keyring interface and making the RawRsaKeyring and RawAesKeyring classes package-private. I'm not sure why we wouldn't want that freedom.

This comment has been minimized.

Copy link
@mattsb42-aws

mattsb42-aws Nov 9, 2019

Member

Rather than the specification saying "there must exist a class RawRsaKeyring with the following properties..." I'd prefer it said "there must exist a method makeRawRsaKeyring, and the result of calling that method has the following properties".

To be clear, the spec says nothing about classes or methods. It is very intentionally vague about what exact form these things take because we know that not all languages that it will need to fit have the same ergonomics.

This comment has been minimized.

Copy link
@SalusaSecondus

SalusaSecondus Nov 11, 2019

Contributor

Coming in really late. I don't care strongly about whether we expose factory methods (with non-public backing classes) or just give people classes. However, I strongly believe that this factory object should not be the same as the main interface. The Keyring interface just defines how they should work. If we want to give people a place to find the standard/official ones, then we should create a second object similar to the following:

public final class StandardKeyrings {
  private StandardKeyrings() {}; // prevent instantiation
  public static Keyring factoryMethod1(...)
  public static Keyring factoryMethod1(...)
}

This comment has been minimized.

Copy link
@WesleyRosenblum

WesleyRosenblum Nov 11, 2019

Author Contributor

This is a pattern I've seen in a few places, one of which is the Java Comparator interface, which defines a static naturalOrder method on the Comparator interface for getting an instance of a NaturalOrder Comparator:

https://docs.oracle.com/javase/8/docs/api/java/util/Comparator.html#naturalOrder--

I started following this pattern on other projects where we previously had Factory classes, and replaced them with just factory methods directly on the interface, which Java 8 enabled with static methods on interfaces. I'm ok with a "StandardKeyrings" factory too, but do you have a particular reason that rolling the factory methods directly into the interface is a bad idea?

This comment has been minimized.

Copy link
@SalusaSecondus

SalusaSecondus Nov 11, 2019

Contributor

I'll admit that it is a matter of taste. For me its a line of reducing cognitive load. When people are thinking about how to use an interface (or just moving it around), they don't need to worry about the (potentially large list) of standard implementations of it. Likewise, if someone just wants an instance but doesn't care about it's actual definition (because they are treating it as an opaque object), they just care about the factory methods and not about the actual interface. This lets people look in one place and (mostly) ignore the other.

While not on an interface, we see this pattern commonly in the JDK as well (Object vs Objects, Array vs Arrays, Charset vs StandardCharsets).

This comment has been minimized.

Copy link
@WesleyRosenblum

WesleyRosenblum Nov 11, 2019

Author Contributor

Yeah, I can buy that argument too. In this case I would admit that static factory methods on interfaces hasn't caught on too widely, and since Keyrings are going to be instantiated by our users I don't want to do anything too unusual. I'll try out having a StandardKeyrings factory class

This comment has been minimized.

Copy link
@robin-aws

robin-aws Nov 19, 2019

Contributor

Rather than the specification saying "there must exist a class RawRsaKeyring with the following properties..." I'd prefer it said "there must exist a method makeRawRsaKeyring, and the result of calling that method has the following properties".

To be clear, the spec says nothing about classes or methods. It is very intentionally vague about what exact form these things take because we know that not all languages that it will need to fit have the same ergonomics.

Very true - the spec really only talks about "initialization" of a type of object without implying what form that takes. We tend to default to thinking of each type mapping directly to a class and "initialization" to constructors and not factory methods.

Perhaps we should more explicitly call out that option in a place like https://github.com/awslabs/aws-encryption-sdk-specification/blob/master/framework/structures.md? We say "these structures will usually be represented as objects" (meaning individual instances), and we could mention that types may or may not correspond to classes.

return RawKeyring.aes(keyNamespace, keyName, wrappingKey);
}

/**
* Constructs a {@link Keyring} which does local RSA encryption and decryption of data keys using the
* provided public and private keys. If {@code privateKey} is {@code null} then the returned {@link Keyring}
* can only be used for encryption.
*
* @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.
* @param publicKey The RSA public key used by this keyring to encrypt data keys.
* @param privateKey The RSA private key used by this keyring to decrypt data keys.
* @param wrappingAlgorithm The RSA algorithm to use with this keyring.
* @return The {@link Keyring}
*/
static Keyring rawRsa(String keyNamespace, String keyName, PublicKey publicKey, PrivateKey privateKey, String wrappingAlgorithm) {
return RawKeyring.rsa(keyNamespace, keyName, publicKey, privateKey, wrappingAlgorithm);
}
}
134 changes: 134 additions & 0 deletions src/main/java/com/amazonaws/encryptionsdk/keyrings/RawKeyring.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
/*
* 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.EncryptedDataKey;
import com.amazonaws.encryptionsdk.internal.JceKeyCipher;
import com.amazonaws.encryptionsdk.internal.Utils;
import com.amazonaws.encryptionsdk.model.DecryptionMaterials;
import com.amazonaws.encryptionsdk.model.EncryptionMaterials;
import com.amazonaws.encryptionsdk.model.KeyBlob;

import javax.crypto.SecretKey;
import javax.crypto.spec.SecretKeySpec;

import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
import java.security.PrivateKey;
import java.security.PublicKey;
import java.util.List;
import java.util.logging.Logger;

import static org.apache.commons.lang3.Validate.notBlank;
import static org.apache.commons.lang3.Validate.notNull;

/**
* A keyring supporting local encryption and decryption using either RSA or AES-GCM.
*/
public class RawKeyring implements Keyring {

private final String keyNamespace;
private final String keyName;
private final JceKeyCipher jceKeyCipher;
private static final Charset KEY_NAME_ENCODING = StandardCharsets.UTF_8;
private static final Logger LOGGER = Logger.getLogger(RawKeyring.class.getName());

static Keyring aes(String keyNamespace, String keyName, SecretKey wrappingKey) {

This comment has been minimized.

Copy link
@mattsb42-aws

mattsb42-aws Nov 7, 2019

Member

The approach we've taken elsewhere is to explicitly separate RawRsaKeyring and RawAesKeyring. The reasoning for this is that what they actually do is very different and some of the internal logic is very different (ex: exact key name match for RSA vs prefix and length match for AES) that we determined that it was simpler to separate them out rather than trying to have a single thing that does everything.

This comment has been minimized.

Copy link
@WesleyRosenblum

WesleyRosenblum Nov 7, 2019

Author Contributor

I started out that way, but then released I had already encapsulated everything that was different about them (save for the key trace difference mentioned below which I don't see in the spec) in JceKeyCipher.

return new RawKeyring(keyNamespace, keyName, JceKeyCipher.aesGcm(wrappingKey));
}

static Keyring rsa(String keyNamespace, String keyName, PublicKey publicKey, PrivateKey privateKey, String transformation) {
return new RawKeyring(keyNamespace, keyName, JceKeyCipher.rsa(publicKey, privateKey, transformation));
}

private RawKeyring(final String keyNamespace, final String keyName, JceKeyCipher jceKeyCipher) {
notBlank(keyNamespace, "keyNamespace is required");
notBlank(keyName, "keyName is required");
notNull(jceKeyCipher, "jceKeyCipher is required");

this.keyNamespace = keyNamespace;
this.keyName = keyName;
this.jceKeyCipher = jceKeyCipher;
}

@Override
public void onEncrypt(EncryptionMaterials encryptionMaterials) {
notNull(encryptionMaterials, "encryptionMaterials are required");

if (encryptionMaterials.getCleartextDataKey() == null) {
generateDataKey(encryptionMaterials);
}

final SecretKey cleartextDataKey = encryptionMaterials.getCleartextDataKey();

if (!cleartextDataKey.getAlgorithm().equalsIgnoreCase(encryptionMaterials.getAlgorithm().getDataKeyAlgo())) {
throw new IllegalArgumentException("Incorrect key algorithm. Expected " + cleartextDataKey.getAlgorithm()
+ " but got " + encryptionMaterials.getAlgorithm().getDataKeyAlgo());
}

final EncryptedDataKey encryptedDataKey = jceKeyCipher.encryptKey(
cleartextDataKey.getEncoded(), keyName, keyNamespace, encryptionMaterials.getEncryptionContext());
encryptionMaterials.getEncryptedDataKeys().add(new KeyBlob(encryptedDataKey));

encryptionMaterials.getKeyringTrace().add(keyNamespace, keyName, KeyringTraceFlag.ENCRYPTED_DATA_KEY);

This comment has been minimized.

Copy link
@mattsb42-aws

mattsb42-aws Nov 7, 2019

Member

The keyring trace is also different for AES vs RSA: AES adds the "signed encryption context" and "verified encryption context" flags on encrypt and decrypt respectively.

This comment has been minimized.

Copy link
@WesleyRosenblum

WesleyRosenblum Nov 7, 2019

Author Contributor

I don't see that in the spec?

This comment has been minimized.

}

@Override
public void onDecrypt(DecryptionMaterials decryptionMaterials, List<EncryptedDataKey> encryptedDataKeys) {
notNull(decryptionMaterials, "decryptionMaterials are required");
notNull(encryptedDataKeys, "encryptedDataKeys are required");

if (decryptionMaterials.getCleartextDataKey() != null) {
return;
}

final byte[] keyNameBytes = keyName.getBytes(KEY_NAME_ENCODING);

for (EncryptedDataKey encryptedDataKey : encryptedDataKeys) {
if (!keyNamespace.equals(encryptedDataKey.getProviderId())) {
continue;
}

if (!Utils.arrayPrefixEquals(encryptedDataKey.getProviderInformation(), keyNameBytes, keyNameBytes.length)) {

This comment has been minimized.

Copy link
@mattsb42-aws

mattsb42-aws Nov 7, 2019

Member

This check is also different for AES and RSA keyrings. For the RSA keyring, the provider info MUST exactly equal the key name. For AES, the provider info MUST start with the key name and be of the expected length.

continue;
}

try {
final byte[] decryptedKey = jceKeyCipher.decryptKey(
encryptedDataKey, keyName, decryptionMaterials.getEncryptionContext());
decryptionMaterials.setCleartextDataKey(
new SecretKeySpec(decryptedKey, decryptionMaterials.getAlgorithm().getDataKeyAlgo()));
decryptionMaterials.getKeyringTrace().add(keyNamespace, keyName, KeyringTraceFlag.DECRYPTED_DATA_KEY);
return;
} catch (Exception e) {
LOGGER.info("Could not decrypt key due to: " + e.getMessage());
}
}

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

private void generateDataKey(EncryptionMaterials encryptionMaterials) {
if (encryptionMaterials.getCleartextDataKey() != null) {
throw new IllegalStateException("Plaintext data key already exists");
}

final byte[] rawKey = new byte[encryptionMaterials.getAlgorithm().getDataKeyLength()];
Utils.getSecureRandom().nextBytes(rawKey);
final SecretKey key = new SecretKeySpec(rawKey, encryptionMaterials.getAlgorithm().getDataKeyAlgo());

encryptionMaterials.setCleartextDataKey(key);
encryptionMaterials.getKeyringTrace().add(keyNamespace, keyName, KeyringTraceFlag.GENERATED_DATA_KEY);
}
}
Loading

0 comments on commit 4bbe029

Please sign in to comment.