KMS client registration deprecation

[juergw]

The KMS client registration has several problems, which may lead to hard to find bugs. Since most users don't need them, we have decided to deprecate these functions to discourage their usage. But note that there are currently no plans to remove these functions in upcoming releases.

Here is a short list of the problems we have found:

• users often register their KMS client every time they want to use it. This works, but every time register is called, a client is added to a global list. And that list will grow larger and larger. So this will result in a memory leak that is hard to debug. And probably will only be a problem in production, which is bad.
• if two users in the same code-base register their KMS clients with overlapping key URI prefixes, then they may end up using the KMS client of the other user without noticing. This may also cause problems that are hard to find. It is better to avoid such side-effects.
• The key templates created with aead.CreateKMSEnvelopeAEADKeyTemplate have a different behavior than other key templates in tink. Usually, keyset.NewHandle(template) will create a new key. But templates from aead.CreateKMSEnvelopeAEADKeyTemplate will not create a new key, they will only create a key-reference to an existing key. This may be confusing to some users.

And almost all users don't really need to use KMS client registration, they can achieve the same with simpler code: for example, instead of this:

kmsClient, err := gcpkms.NewClientWithOptions(...)
if err != nil {
  ...
}
registry.RegisterKMSClient(kmsClient)
kmsEnvelopeAEADTemplate, err := aead.CreateKMSEnvelopeAEADKeyTemplate(kekURI, aead.AES128GCMKeyTemplate())
if err != nil {
  ...
}
handle, err := keyset.NewHandle(kmsEnvelopeAEADTemplate)
if err != nil {
  ...
}
envAEAD, err := aead.New(handle)
if err != nil {
  ...
}

they can just do this:

kmsClient, err := gcpkms.NewClientWithOptions(...)
if err != nil {
  ...
}
kekAEAD, err := kmsClient.GetAEAD(kekURI)
if err != nil {
    ...
}
envAEAD := aead.NewKMSEnvelopeAEAD2(aead.AES128GCMKeyTemplate(), kekAEAD)

[theory]

Thanks for the explanation. These are very real issues, so I can see why deprecation would be desirable. But I'm a bit concerned that use cases it provides for are not yet covered by other/new features, namely:

• I see no other way to associate an envelope KEK URI with a specific key. It seems to me that the KMS URI should never change for an envelope key, so I'd expect it to be encoded into the template. This change would require the mapping from key to KMS URI external to Tink -- which complicates things for the maintainer of the code and, I think, in appropriately externalizes an important security configuration from the Tink library.
• With this model, it's more likely a misconfiguration will break things. If the KMS URI is not part of the key template in the keyset, it's easier for someone to change it in the external configuration, or forget it in a new deployment, or make typos, etc. Having it be part of the template ensures it says consistent.
• This complicates things like key rotation. If I'm required to, say, switch from AWS KMS to GCP KMS, the external-to-tink config would need to account for that, too.

I really like having the envelope client config stuff as part of Tink, but acknowledge the challenges of the current design: pairing a key URI with a client is tricky! I suspect the prefix model could be improved by introducing additional rules (longest prefix match wins, replacing a client with the same prefix is an error or replaces the existing client, etc.). And it surely ought not to be the documented first implementation for most users.

But having a method to manage multiple KMS clients for multiple envelope keys is a super useful feature for more complicated implementations that need to support multiple envelope keys with different URIs and vendors.

[juergw]

Note that we don't have a plan to remove it, the depreciation is only to discourage using it if you don't need to.

I agree that a single KMS key should only be used with a single DEK template. For most users, the registration does not really help to achieve that, because they do exactly what I wrote in the example above, and they could just use the same KMS key with different DEK templates.

So can you tell me how exactly you are using Tink?

• Do you register your client(s) at startup, and have a config with serialized KMS envelope keys that you load?
• Do your keyset have just one key, or do they sometimes have several keys?
• What do you use these envelope AEADs for? Do you directly encrypt data, or do you encrypt a keyset with that? A simple code examle would be really helpful.

Note that if you use the envelope AEAD to encrypt a keyset, then you only encrypt a small amount since a keyset is fairly small, and you don't really need envelope encryption. You can directly use the AEAD from the KMS client. And if you want to move to a new kms key, you can just re-encrypt the keyset with the new KMS key.

[theory]

I agree that a single KMS key should only be used with a single DEK template. For most users, the registration does not really help to achieve that, because they do exactly what I wrote in the example above, and they could just use the same KMS key with different DEK templates.

Let's say that kekURI in your example comes from a configuration file. If it is changed in that file, suddenly the same "key" is using a different KEK to encrypt, all decryptions start failing, and new encryptions won't work with the old key. The KMSEnvelopeAEADKeyTemplate avoids that issue.

• Do you register your client(s) at startup, and have a config with serialized KMS envelope keys that you load?

Yes, the KMS envelop template lives in the keyset file that's loaded at startup. The KMS client config lives in a different config (with different prefixes for each client).

• Do your keyset have just one key, or do they sometimes have several keys?

The expectation is that there'd be several, though in general that's for the non-envelope use case. Still, the apps needs a way for multiple keys with multiple KMSes in order to facilitate key rotation when the powers that be decide to change cloud vendors (which happens more often in enterprises than is healthy, admittedly).

• What do you use these envelope AEADs for? Do you directly encrypt data, or do you encrypt a keyset with that?

I actually prefer to use non-envelope keys for performance reasons, and only encrypt the keyset itself with the KMS key. But the InfoSec team I've worked with have strongly recommended using envelope encryption for all use cases, so the app in that case would use one KMS key to decrypt the keyset, and then (at least) one other to encrypt business data with an envelope key in the keyset.

I think it might be best if the envelope key used a different KMS key than the keyset KMS key — and would have to to support multiple keys, anyway. I have an idea to enable a key for each type of entity at some point, as well, so there would be multiple keys in the keyset in that case.

The current manager pattern with prefixes, for all its flaws, enables these patterns. If it were to go away (and I appreciate there are no plans to rn), I'd have to reimplement something like it in the app itself — without the benefit of Google security engineers to look it over. :-)

A simple code examle would be really helpful.

Sure, I'll work something up soon.

Note that if you use the envelope AEAD to encrypt a keyset, then you only encrypt a small amount since a keyset is fairly small, and you don't really need envelope encryption. You can directly use the AEAD from the KMS client. And if you want to move to a new kms key, you can just re-encrypt the keyset with the new KMS key.

Yeah, that pattern seems fine to me. It's using one or more envelope keys to encrypt business data that I worry about.

[juergw]

Thanks for the response, that was helpful.

[david-bain]

I would like to add that our implementation relies heavily on the registry functionality, and seeing the depreciation warning (which errors in our CI btw by default) is concerning.

In our application, we have implemented our own version of a tink kms service for super-fast in-memory crypto. Basically we store tink keysets in a DB, and our implementation, when an encrypt or decrypt is requested, automatically pulls the relevant keyset (if not cached or cache is expired), decrypts it and puts it in a cache for use. We then perform envelope encryption with the KEK from the keyset.

For each operation, we determine which KMS to use based on the URI in metadata next to the payload. This is important as we are handling crypto for many different operations and our app doesn't know which KMS client was used to encrypt a payload prior to inspecting the payload metadata. At this point we need some method to search for the KMS client... eg. a registry.

This has worked great as we just needed to make sure our app had all the possible KMS services registered and then we could just call decrypt on a payload, and through the current registry implementation, the decrypt would look up the registry, find the correct KMS reference, get the reference AEAD, which then can pull the cache etc and actually perform the decrypt.

I don't see what is proposed to replace the registry other than me effectively creating my own version.

Can we at least remove the deprecation warning? It is causing concern with a bunch of people when our CI is failing and we can't promise a rewrite of production reliant code and we will need to have an exception in our tooling to allow this

[juergw]

I think you should consider just implementing your own "registry". All you need is a list of KMS clients and a lookup function. The implementation of the lookup is really simple, it is really just a loop: https://github.com/tink-crypto/tink-go/blob/main/core/registry/registry.go#L145

And since you probably never want to mutate the list of KMS clients while using them, you don't need any locking. So you can implement it like this:

func GetKMSClient(kmsClients []registry.KMSClient, keyURI string) (registry.KMSClient, error) {
    for _, kmsClient := range kmsClients {
        if kmsClient.Supported(keyURI) {
            return kmsClient, nil
        }
    }
    return nil, fmt.Errorf("KMS client supporting %s not found", keyURI)
}

// GetKMSAEAD returns an AEAD instance for the first KMS client that supports keyURI.
func GetKMSAEAD(kmsClients []registry.KMSClient, keyURI string) (tink.AEAD, error) {
    client, err := GetKMSClient(kmsClients, keyURI)
    if err != nil {
        return nil, err
    }
    return client.GetAEAD(keyURI)
}

[theory]

It would be nice to a non-global registry, which one can implement on one's own. But I still believe the the KMS URI should be stored with the key so that for each key it can be passed to GetKMSAEAD().

[david-bain]

Can I confirm if the kms_envelope_aead_key_manager is going to be updated or deprecated as it uses the current registry?

Also to answer your questions: Do you register your client(s) at startup, and have a config with serialized KMS envelope keys that you load? Yes we register our clients at startup once, but keysets are pulled separately as needed or via the GCPKMS package.

Do your keyset have just one key, or do they sometimes have several keys? Our keysets are rotating every week so they will eventually have 100s of keys

What do you use these envelope AEADs for? Do you directly encrypt data, or do you encrypt a keyset with that? We are directly encrypting/decrypting data as it passes through our proxy service so that our end-users don't need to. Each payload is envelope encrypted with a new DEK to ensure it is as secure as possible when it is stored at rest on a vendor server.

[juergw]

Having 100s of keys will be a problem, I think. KMS Envelope keys don't have an output prefix that identifies the key: https://github.com/tink-crypto/tink-go/blob/main/aead/aead_key_templates.go#L147 So, when you call decrypt for an AEAD of a keyset with 100s of keys with output prefix RAW, then it will have to iterate over all of them: https://github.com/tink-crypto/tink-go/blob/main/aead/aead_factory.go#L148 and if each decrypt requires an RPC call, this will take a long time.

[theory]

In your code you can manually set the prefix for envelope keys to use the TINK prefix, at least. That should solve the problem going forward, at least.

I think it was an error for it to use the RAW prefix by default. The wrapped key should definitely use RAW, but the envelope around it should have defaulted to the TINK prefix.

[juergw]

Yes, manually setting the prefix works, and it preferable if you have several keys in the keyset. But you must serialize the keyset and not re-create the keyset. Because each time you create a new keyset for the same KMS key, Tink will create a new key ID and the primitives will not be compatible.

I have added some tests to make sure that manually setting the output prefix type to TINK works. To make sure that we don't break this use-case.

[juergw]

We've decided to remove the deprecation on these functions, see https://github.com/tink-crypto/tink-go/commit/27ac04e7d76cdac3827687809fd5ad46bb6d3458.

It's better to wait with the deprecation until we have a good replacement for this.

[theory]

But you must serialize the keyset and not re-create the keyset. Because each time you create a new keyset for the same KMS key, Tink will create a new key ID and the primitives will not be compatible.

Do people do this? ISTM that creating a new keyset (or new keys) always creates new keys.