Add proposal for crypto building block

[ItalyPaleAle]

Ported from dapr/dapr#4508

[mukundansundar]

Should subtle crypto ops be available via dapr CLI?

[ItalyPaleAle]

Should subtle crypto ops be available via dapr CLI?

I don't see why not. But keep in mind the high-level ops are probably going to be gRPC-only.

[yaron2]

Should subtle crypto ops be available via dapr CLI?

I don't see why not. But keep in mind the high-level ops are probably going to be gRPC-only.

This might conflict with our criteria to make APIs stable, in guaranteeing they have both gRPC and HTTP implementations. @mukundansundar do we have that listed in API maturity criteria right now?

[mukundansundar]

Should subtle crypto ops be available via dapr CLI?

I don't see why not. But keep in mind the high-level ops are probably going to be gRPC-only.

This might conflict with our criteria to make APIs stable, in guaranteeing they have both gRPC and HTTP implementations. @mukundansundar do we have that listed in API maturity criteria right now?

@yaron2

Currently in the API guidelines the requirements for a new API are for proposals

• Proposed contract for the API
  • HTTP and gRPC APIs should be consistent in behavior and user experience.

further below for Alpha APIs

• Both HTTP and gRPC protocols should be supported for the new API

Both of these are recommendations that they should be consistent and supported. Not a strong requirement.

[ItalyPaleAle]

@yaron2 This might conflict with our criteria to make APIs stable, in guaranteeing they have both gRPC and HTTP implementations. @mukundansundar do we have that listed in API maturity criteria right now?

Note that subtle operations are supported for both HTTP and gRPC and what I am writing below ONLY applies to the high-level encryption/decryption APIs

The rationale for only supporting gRPC is in the proposal:

These APIs will only be available over gRPC, because they will work over a stream of data from the client. An HTTP implementation would require keeping the entire message in-memory, which is not acceptable.

Imagine the situation where you're encrypting (or decrypting) a very large file (multiple MBs or GBs). If you were to do that via HTTP, you'd have to send your entire blob to the runtime, then wait for the runtime to process it, and then read the response back. Even in the best-case scenario, where the runtime flushes the data as soon as it's processed, you still have an amount of data equal to the length of the message in memory at some point.

Using gRPC we are able to use 2-way streaming. The Dapr SDK would chunk the data and send each chunk to the runtime. The runtime processes the chunk and sends it back (encrypted or decrypted). The runtime will never need to keep in memory more data than the size of a chunk (64KB in the proposal for the high-level encryption scheme). That's MUCH better than possibly holding GBs in memory. The Dapr Encryption Scheme proposed verifies the data chunk-by-chunk, so even during decryption, we are never flushing un-verified data to the caller and this is considered safe.

So my point on not supporting HTTP is essentially to protect users from themselves, as there will be the temptation of sending large files over HTTP and that will cause memory issues.

This is not a problem for subtle APIs because they're not meant to be used to encrypt large chunks of data. In some cases it's not even possible (e.g. when using RSA you're limited to a handful of bytes).

PS: The guidelines use should to indicate that both HTTP and gRPC should be supported, but it's not a strict requirement. During discussion, we highlighted a bunch of scenarios where it would be not advisable to support both protocols, and I believe this is one of them (most scenarios involved the need for streaming)

[berndverst]

I support this.

[olitomlinson]

I support the introduction of this building block

[ItalyPaleAle]

@johnewart and I had a review with the "crypto board" at Microsoft to advise on the design of the high-level encryption scheme. Overall we got approval from them, albeit with some small tweaks. I have applied a few of them already, and will complete the work by EOW.

The crypto board did not comment on the APIs being exposed, as that's purely a Dapr decision, just the design of the encryption scheme, the keys and algorithms used, etc.

[ItalyPaleAle]

Completed updating the document with feedback from the crypto board. The change was in the headers of the Dapr encryption scheme, which is now a JSON object that covers both the "plain-text" header and the "binary" header.

[artursouza]

LGTM overall. Just some feedback:

1. Give an example of a crypto component's YAML.
2. Add HTTP High-Level API too - even with the perf penalty. It can be handy for local development and debugging.
3. The high level API should be done first:
   1. It will "wow" most users and our target audience does not really want to deal with low level APIs. I would even argue that people using low level crypto APIs might not consider Dapr at all.
   2. It is a smaller exposed surface for us to quickly add to the SDKs.

[artursouza]

One more point, we need a reference to the encryption key in the high-level API's payload, so old data can be decrypted after key rotation. Asking users to manage key rotation would make the high-level API not as useful.

[ItalyPaleAle]

I added the HTTP APIs for high-level crypto, although their use is strongly discouraged (and limited by Dapr's max-http-request-size).

As for this:

One more point, we need a reference to the encryption key in the high-level API's payload, so old data can be decrypted after key rotation. Asking users to manage key rotation would make the high-level API not as useful.

I have thought about that, but I am not sure how I feel about this.

It's pretty common for users to have to provide the key (or at least its name) when performing decryption operations. None of the schemes that the Dapr Encryption Scheme v1 is based on reference the key name in the message.

In the current API design, users are required to pass the name of the key used to decrypt the data in the request, and that seems to be the usual pattern. If users think that they will need to rotate keys, they should keep a reference to the key used stored alongside the data

Additionally, the name of the key is dependent on where it's stored. The same key could have different names when stored locally and when in Azure Storage Key Vault.

I don't see how knowing the key in the message would help with key rotation. In all cases, users are required to keep around the old key if they want to continue decrypting the messages encrypted with that key (and this is a usual pattern).

Also worth pointing out that key rotation should be less of a concern with the Dapr encryption scheme, which can be used to safely encrypt as many messages as one wants (unlike the current Dapr state store encryption which is limited to 2^33 messages ever).

If anything, we should consider implementing a method that re-encrypts files (which means just re-encrypting the first few bytes to re-wrap the file key).

[yaron2]

The same key could have different names when stored locally and when in Azure Storage

Why the reference to Azure Storage specifically?

[ItalyPaleAle]

Why the reference to Azure Storage specifically?

Sorry, that was a lapse. I meant Azure Key Vault, and fixed my message now

[ItalyPaleAle]

@artursouza I thought more about your ask to include the key ID into the manifest, and how I could make that work, and I am still not convinced.

I could find 3 problems:

1. "Hardcoding" the key name makes the solution not portable. Users would not be allowed to switch where the key is stored, for example migrating from a K8s secret to Azure Key Vault, or vice-versa.
2. Especially when using asymmetric ciphers, it should be totally acceptable for the keys to be stored in different places. For example, app A only encrypts messages, so it only needs the public part of the key: that can be stored in a JWKS document retrieved from a URL. While app B which can decrypt messages needs the private part, which may be stored in a vault.
3. The key's name in vaults is not necessarily stable. For example, in Azure Key Vault each key can have multiple "versions", and each version could have the same key or a different one. We support this in Dapr Crypto, by allowing specifying the key version after the name, for example mykey/version1 (versions in AKV are actually SHA hashes, so not something the user can type). If you don't specify a version, it defaults to latest. This causes two issues:
   • If the latest version is "version1": mykey, mykey/latest and mykey/version1 all map to the same key.
   • mykey/latest and mykey are possibly moving targets: they do not necessarily identify one specific key.
   • We can implement a "resolver" that converts "latest" to a specific version, but that requires performing an extra network call which is very slow.

Thoughts? If not, are we ok to get this proposal officially approved?

[artursouza]

@artursouza I thought more about your ask to include the key ID into the manifest, and how I could make that work, and I am still not convinced.

I could find 3 problems:

1. "Hardcoding" the key name makes the solution not portable. Users would not be allowed to switch where the key is stored, for example migrating from a K8s secret to Azure Key Vault, or vice-versa.

2. Especially when using asymmetric ciphers, it should be totally acceptable for the keys to be stored in different places. For example, app A only encrypts messages, so it only needs the public part of the key: that can be stored in a JWKS document retrieved from a URL. While app B which can decrypt messages needs the private part, which may be stored in a vault.

3. The key's name in vaults is not necessarily stable. For example, in Azure Key Vault each key can have multiple "versions", and each version could have the same key or a different one. We support this in Dapr Crypto, by allowing specifying the key version after the name, for example mykey/version1 (versions in AKV are actually SHA hashes, so not something the user can type). If you don't specify a version, it defaults to latest. This causes two issues:

   • If the latest version is "version1": mykey, mykey/latest and mykey/version1 all map to the same key.
   • mykey/latest and mykey are possibly moving targets: they do not necessarily identify one specific key.
   • We can implement a "resolver" that converts "latest" to a specific version, but that requires performing an extra network call which is very slow.

Thoughts? If not, are we ok to get this proposal officially approved?

1. The reference to the key should be portable, just like we made it portable in the secrets store.
2. Any additional information to fetch the key should be included in the metadata too.
3. Adding the version is required too.

We might need additional metadata in the crypto components to handle key references, maybe create aliases that are portable.

My point being that having a high level building block without metadata to enable key rotation dramatically reduces the value add for this API. If there is significant work for users to implement key rotation on top of this API, they might decide not to use it at all.

[ItalyPaleAle]

@artursouza and I synced offline on the issue with "key binding" and we believe we have a solution that addresses the concerns each of us had.

1. The key reference is added to the metadata of the document by default.
2. The "Decrypt" method has an optional argument that allows overriding the key that may be present in the document being decrypted.
3. The "Encrypt" method has 2 optional arguments:
   • key_unbound bool: when true, does not include the key reference in the document. In this case, the "Decrypt" method must be called with the key reference. The default is false (i.e. the key reference is included)
   • key_binding string: allows storing a different key reference in the encrypted document. By default the key reference is the name of the key used to encrypt the document, but if the decryption key is different (e.g. encryption is performed with a public key in a JWKS, and decryption with a private key in AKV), then this allows the user to specify the reference.

Naming is hard, let me know if you have better suggestions on the names :)

I've updated the proposal for the high-level design: b713672

[artursouza]

The only thing is to use the new names instead from the thread we had.

excludeDecryptionKey bool
decryptionKey string

[ItalyPaleAle]

The change is in place:

• decryptionKey
• omitDecryptionKeyName

[yaron2]

+1 binding

[ItalyPaleAle]

FYI I have made a small change to the proposal to clarify the algorithms support for key wrapping in the high-level scheme.

1. Explained that AES-KW will require using 256-bit keys. This is because our file key (that will be wrapped) is 256-bit in length, and AES-KW thus requires using 256-bit wrapping keys too (this is per RFC 3394)
2. Removed AES-GCM-SIV which was listed as a "maybe", and will not be implemented in the near future. No vault supports it, it's not implemented in the "local crypto", and it was not recommended by the MS crypto board during the review.

[mukundansundar]

+1 binding

Addressed