Keyring Ergonomics

[WesleyRosenblum]

One goal of the AWS Encryption SDK is to make doing the right thing easy, and doing the wrong thing difficult/impossible.

After we release keyrings, it will be very difficult to change any of the keyring APIs without breaking existing customers. Thus, we should endeavor to design the ergonomics of keyrings to achieve this goal before we release them. In this issue, we'll discuss the proposed changes to the ESDK to support keyrings, make sure that they achieve this goal, and improve our design if they don't.

In this issue, we introduce some of the major keyring-related changes, along with some potential improvements/alternative ergonomics. Please feel free to comment and add more areas for improvement.

StandardKeyrings

The only way to instantiate the built-in keyrings in the ESDK is by using one of the static factory methods in the StandardKeyrings class. All built-in keyring implementation classes are package-private so they can evolve with the assurance that no client is directly instantiating them.

The most commonly used factory method in StandardKeyrings will likely be

public static Keyring kms(KmsClientSupplier clientSupplier, 
                          List<String> grantTokens, 
                          List<String> keyIds, 
                          String generator)

This method requires customers to instantiate a KmsClientSupplier (using a KmsClientSupplier.Builder), supply optional grantTokens and keyIds, and an optional or required generator ARN.

Proposal: We could add an additional factory method to StandardKeyrings that uses a default KmsClientSupplier and eliminates some of the optional arguments, such as:

public static Keyring kms(String generator) 

Proposal: We can replace factory methods with methods that return builders for each keyring type. This would allow optional inputs without too many methods:

public static KmsKeyringBuilder kms()
public static RawRsaKeyringBuilder rawRsa()
public static RawAesKeyringBuilder rawAes()
public static MultiKeyringBuilder multi()

AwsCryptoConfig

AwsCrypto is the main entry point to the Java ESDK. Almost all existing public methods in this class have been deprecated and replaced by methods that take in AwsCryptoConfig, such as:

public AwsCryptoResult<byte[]> encryptData(final AwsCryptoConfig config,
                                               final byte[] plaintext) 

AwsCryptoConfig is instantiated with a AwsCryptoConfig.Builder that gives users the option to seta CryptoMaterialsManager, Keyring, and Encryption Context. This reduces the number of non-deprecated public methods in AwsCrypto from about 30 to 5.

EncryptRequest / DecryptRequest

AwsCrypto is the main entry point to the Java ESDK. Almost all existing public methods in this class have been deprecated and replaced by methods that take in specific request types, such as:

public AwsCryptoResult<byte[]> encrypt(final EncryptRequest request)
public AwsCryptoResult<byte[]> decrypt(final DecryptRequest request)
public long estimateCiphertextSize(final EstimateCiphertextSizeRequest request)
public AwsCryptoOutputStream createEncryptingOutputStream(final CreateEncryptingOutputStreamRequest request)

Each of these types are constructed with a builder to easily allow for optional parameters. This matches the pattern that the AWS SDK uses.

Keyring Interface and Encryption/Decryption Materials

The Keyring interface defines two void methods:

void onEncrypt(EncryptionMaterials encryptionMaterials);
void onDecrypt(DecryptionMaterials decryptionMaterials, List<? extends EncryptedDataKey> encryptedDataKeys)

We've changed the materials classes to make some fields mutable, including the cleartextDataKey and the list of encryptedDataKeys.

Proposal: Make EncryptionMaterials and DecryptionMaterials immutable, and require users to reinstantiate whenever they change. To do this, we need to change the return type of the keyring interface methods from void to EncryptionMaterials and DecryptionMaterials. We can also introduce a new abstract KeyringBase class to reduce the amount of data each keyring implementation needs to pass back and forth. This class would look something like this:

public abstract class KeyringBase implements Keyring {

    @Override
    public EncryptionMaterials onEncrypt(EncryptionMaterials encryptionMaterials) {
        OnEncryptResult onEncryptResult = onEncryptImpl(encryptionMaterials);

        return encryptionMaterials.toBuilder()
                .setEncryptedDataKeys(onEncryptResult.getEncryptedDataKeys())
                .setCleartextDataKey(onEncryptResult.getCleartextDataKey())
                .setKeyringTrace(onEncryptResult.getKeyringTrace())
                .build();
    }

    @Override
    public DecryptionMaterials onDecrypt(DecryptionMaterials decryptionMaterials, List<? extends EncryptedDataKey> encryptedDataKeys) {
        OnDecryptResult onDecryptResult = onDecryptImpl(decryptionMaterials, encryptedDataKeys);
        
        return decryptionMaterials.toBuilder()
                .setCleartextDataKey(onDecryptResult.getCleartextDataKey())
                .setKeyringTrace(onDecryptResult.getKeyringTrace())
    }

    public abstract OnEncryptResult onEncryptImpl(EncryptionMaterials encryptionMaterials);
    public abstract OnDecryptResult onDecryptImpl(DecryptionMaterials encryptionMaterials, List<? extends EncryptedDataKey> encryptedDataKeys);
}

[robin-aws]

+1 for making EncryptionMaterials and DecryptionMaterials immutable (and see awslabs/aws-encryption-sdk-specification#66). Is the result from onEncrypt expected to include the encrypted data keys passed in, or just those created by this keyring?

[robin-aws]

public static Keyring kms(String generator) 

Nit: can it be something like generatorKeyId?

Regarding AwsCryptoConfig, it sounds like there's no point to having a separate AwsCrypto class any more, if every method is passed a config object. Why not instantiate AwsCrypto using an AwsCryptoBuilder that has all the config that AwsCryptoConfig would have?

[WesleyRosenblum]

Is the result from onEncrypt expected to include the encrypted data keys passed in, or just those created by this keyring?

It is expected to include the edks passed in as well. That's kind of why it seems more natural to me for it to be mutable. If I make the materials immutable, I would prefer to make a KeyringBase abstract class that doesn't return the materials class, but something more minimal and will then merge the result with the input materials to produce new materials. So you'd have something like this:

public abstract class KeyringBase implements Keyring {

    @Override
    public EncryptionMaterials onEncrypt(EncryptionMaterials encryptionMaterials) {
        OnEncryptResult onEncryptResult = onEncryptImpl(encryptionMaterials);

        return encryptionMaterials.toBuilder()
                .setEncryptedDataKeys(onEncryptResult.getEncryptedDataKeys())
                .setCleartextDataKey(onEncryptResult.getCleartextDataKey())
                .setKeyringTrace(onEncryptResult.getKeyringTrace())
                .build();
    }

    @Override
    public DecryptionMaterials onDecrypt(DecryptionMaterials decryptionMaterials, List<? extends EncryptedDataKey> encryptedDataKeys) {
        OnDecryptResult onDecryptResult = onDecryptImpl(decryptionMaterials, encryptedDataKeys);
        
        return decryptionMaterials.toBuilder()
                .setCleartextDataKey(onDecryptResult.getCleartextDataKey())
                .setKeyringTrace(onDecryptResult.getKeyringTrace())
    }

    public abstract OnEncryptResult onEncryptImpl(EncryptionMaterials encryptionMaterials);
    public abstract OnDecryptResult onDecryptImpl(DecryptionMaterials encryptionMaterials, List<? extends EncryptedDataKey> encryptedDataKeys);
}

I believe that would be less error-prone for users to implement (they would just extend KeyringBase instead of implementing Keyring), and would not violate the point @mattsb42-aws made in Spec Issue 66 (I think))

[WesleyRosenblum]

Regarding AwsCryptoConfig, it sounds like there's no point to having a separate AwsCrypto class any more, if every method is passed a config object. Why not instantiate AwsCrypto using an AwsCryptoBuilder that has all the config that AwsCryptoConfig would have?

I like this, though my concern is that then the EncryptionContext would be a member variable of AwsCrypto, which would prohibit re-use of an AwsCrypto instance across multiple encryption operations and could lead to misuse if a user tried to re-use it. I suppose I could move EncryptionContext back to being an input variable, though then we'd either have to have an additional method to handle the empty EncryptionContext case, or be ok with requiring an EncryptionContext.

Or maybe I should go the other way, and have encryptData and decryptData take in EncryptDataRequest and DecryptDataRequest types. That would match other AWS SDKs

[mattsb42-aws]

in no particular order:

immutable materials

I like it. Do it. :) More thoughts on how and exactly what their API should be later.

StandardKeyrings

I don't really understand the appeal of StandardKeyrings or what it actually buys us, but I'm willing to chalk that up to language ergonomics and defer to others who are more familiar with Java idioms.

onEncrypt vs onEncryptImpl

Maybe I'm missing something, but this looks to me like a fragile hack that would be better served by better helpers on the encryption/decryption materials.

For example, rather than having to do this to get a new decryption materials:

decryptionMaterials.toBuilder()
                .setCleartextDataKey(onDecryptResult.getCleartextDataKey())
                .setKeyringTrace(onDecryptResult.getKeyringTrace())

What if instead I were to simply say:

decryptionMaterials.withCleartextDataKey(cleartextDataKey, keyringTraceEntry)

This lets us leave the management of encryption/decryption materials to the materials rather than attempting to have something else control them.

We have a limited number of additive actions that we want keyrings to be able to do, and none of those actions involve simply adding a single thing. If we say "just turn it into a builder and add some stuff", how do we make sure that, say, every EDK added also includes a corresponding keyring trace entry?

These are mutating rather than assembling, but this is the general sort of pattern I mean:

https://github.com/aws/aws-encryption-sdk-python/blob/952bce44db630754de6a0dad451f72665f83b1c7/src/aws_encryption_sdk/materials_managers/__init__.py#L266-L284

AwsCryptoConfig

I'm not opposed to this, per se, but I do think we should consider our motivations and the 90% use-case.

This is actually similar to the API that we use in the DDB Encryption Client for Python:

https://github.com/aws/aws-dynamodb-encryption-python/blob/1d247525ba133a6ea09cbf0702d2fb5fa67ea843/src/dynamodb_encryption_sdk/encrypted/item.py#L40-L41

If you want to call this function directly, you have to manually build a CryptoConfig object for each request. This also carries through to the higher-level helper clients, letting you pass in a custom per-request CryptoConfig to override the default configuration for that helper instance. However, the 90% use-case that we expect is for people to a) use the higher-level helper clients and b) configure them once and use them normally throughout.

If we want to simplify the request (and I'm on board with that), we should make sure that we're not making things much worse for the 90% in order to make things simpler for the 10%.

Re the contents of a hypothetical AwsCryptoConfig, I'm going to argue that the encryption context MUST NOT be part of that config. The encryption context needs to be handled with the same granularity as the plaintext.

What that effectively leaves us with is "a config thing that can contain either a CMM or a keyring, (or a MKP?), and Does The Right Thing". That doesn't sound much better to me than requiring callers to manually wrap their keyrings in DefaultCryptoMaterialsProvider.

[WesleyRosenblum]

AwsCryptoConfig -> EncryptRequest/DecryptRequest

Although it might be slightly overkill, I've decided to go with individual request types for each method. So instead of AwsCryptoConfig, we now have:

• EncryptRequest
• DecryptRequest
• EstimateCiphertextSizeRequest
• CreateDecryptingInputStreamRequest / CreateDecryptingOutputStreamRequest
• CreateEncryptingInputStreamRequest / Create EncryptingOutputStreamRequest

Each of these types are constructed with a builder to easily allow for optional parameters. This matches the pattern that the AWS SDK uses.

Immutable Materials & onEncrypt vs onEncryptImpl (KeyringBase)

I recognize the benefits of immutable materials, but I'm not still not entirely happy with the usability of them, even with withCleartextDataKey type helper methods. In KMSKeyring for example, a ClearTextDataKey is generated, which results in new EncryptionMaterials, then that key is encrypted, which results in new EncryptionMaterials, and then there is a loop where all other data keys are encrypted, creating new EncryptionMaterials each time. It seems like a lot of different EncryptionMaterials to keep track of, which is why I proposed the abstract KeyringBase, so that the author of a custom Keyring only has to work with a mutable OnEncryptResult/OnDecryptResult and doesn't have to keep recreating EncryptionMaterials.

[praus]

Great work Wesley! Here are a couple improvements I can think of.

• I like StandardKeyrings class. It's convenient and Java-idiomatic to have all StandardKeyrings in one class. However, I would be inclined to make newBuilder() methods on standard keyrings public so that StandardKeyrings class is not mandatory to use.
• I would rename generator to something like generatorCmkId or similar. generator is too ... generic :).
• Please document keyIds argument better to make it clear what it's used for. Currently, it says "A list of strings identifying KMS CMKs, in ARN, CMK Alias, or ARN Alias format." which doesn't tell me much.
• I would strongly prefer having an immutable class that represents a KMS CMK identifier instead of passing around strings. Something like AwsKmsCmkId. It should have some lightweight validation. Like, don't validate region and partition, but validate that if the user is supplying an ARN, it looks vaguely valid etc. If the user is not supplying an ARN, check that a UUID is being supplied. Honestly, even if AwsKmsCmkId just wrapped an instance of String and made sure it's not null and empty, it would be useful.
• I like the idea of providing builders. For KMS keyring I would provide two methods:
  public static Keyring kms(AwsKmsCmkId generatorCmkId) and public static KmsKeyringBuilder kms(). If someone wants to supply grants, their use case is most likely more complex so using a builder is completely warranted.
• I'm inclined to agree with Matt KeyringBase is not worth it. Utility methods like withCleartextDataKey should do the job just as well.

[WesleyRosenblum]

I would be inclined to make newBuilder() methods on standard keyrings public so that StandardKeyrings class is not mandatory to use.

I was trying to avoid having any of the standard keyrings be Public classes to give us the flexibility to change them later on knowing no client was referencing them. Working on the ESDK I’ve become very conservative around making classes and methods public as the many public classes and methods that reference MasterKeys has made it more challenging to update things while maintaining backward compatibility.

I've added separate KmsKeyringBuilder, RawAesKeyringBuilder and RawRsaKeyringBuilder classes that can be instantiated by using the methods in StandardKeyrings or by using the standard() factory method defined on each Builder

[WesleyRosenblum]

Thanks for the suggestions, they have been incorporated into the keyring branch.