feat: add tx encryption key rotation - #2373
Conversation
| // Reserved for future attestation evidence beyond the validator signatures, e.g. a TEE quote, | ||
| // signature chain, compose hash, app measurement, epoch, or accepted measurement set. | ||
| reserved 6 to 9; | ||
| reserved 5 to 9; |
There was a problem hiding this comment.
FYI we shouldn't reserve for future use.
Protobuf already supports adding new fields in a non-breaking fashion. reserved is used to protect removed fields. i.e. when removing a field in a backwards compatible way, replace it with reserved to prevent someone reusing that same field number, which would break the schema.
Mirko-von-Leipzig
left a comment
There was a problem hiding this comment.
I have some questions; without having reviewed things at all - I'm waiting on some internal consensus on how the whole operation might work.
- This aligns key rotation with the epoch. The epoch length is dictated by node storage requires. Should we be combining these concepts, or should the key epoch be independent?
- Do we need a way to rotate the master key? I feel like this just shuffles the security boundary around a little, while cosmetically rotating the public key. But I'm no security expert; perhaps this is somehow sufficient.
The concepts can be independent (for example defining the key epoch
We probably need to have a way of rotating the master key, as it's single point of failure (a leak means all epoch keys become derivable). What the current epoch rotation does give is containment for the derived keys used during operation. |
|
Without having looked at the code too much, a few comments:
I don't think we need to make this automatic and rotate keys on every epoch (I'm assuming we define epoch as
So, this PR would basically set up all the structure needed for key rotation so that user interfaces are more or less stable, but the actual key rotation we can implement later. Separately, do we already expect witnesses for submit transaction / submit batch to be encrypted? If not, I'd implement that first because this is a higher priority and would impact the client directly. |
huitseeker
left a comment
There was a problem hiding this comment.
This PR adds useful wire types for current and optional next keys, but it also builds automatic epoch rotation, secret export, and key archival that we do not need for the next step. I suggest narrowing it to the key schedule contract.
The validator should accept keys from its provider, attest the current and optional next schedule as one value, and decrypt by caller supplied key ID under provider-owned current and grace rules.
Shared node code should verify the schedule against trusted chain state.
| /// The ciphertext is a serialized [`SealedMessage`]. | ||
| async fn decrypt_transaction_inputs( | ||
| &self, | ||
| epoch: u16, |
There was a problem hiding this comment.
Could this take the caller's key_id instead of an epoch? The later submission envelope identifies the advertised key, while this API guesses between epoch and epoch - 1. That cannot reject a scheduled key before activation or enforce a manual current/grace policy independently of storage epochs.
| let public_key = self.key_for_epoch(epoch).public_key(); | ||
| TransactionEncryptionKeyInfo { | ||
| scheme: Self::scheme_id(), | ||
| key_id: public_key.to_commitment().to_bytes()[..4].to_vec(), |
There was a problem hiding this comment.
Could key_id stay as provider owned opaque bytes, with this provider using the full public key commitment? The four byte prefix can collide, so decryption may select the wrong key even before automatic derivation is replaced with manual rotation.
| // `role_suffix` is a single `0` byte when the key is attested as the current key, or a `1` | ||
| // byte followed by `rotation_block_num` (4 bytes little-endian) when the key is attested as a | ||
| // scheduled next key, binding the rotation block to the signature. The canonical construction | ||
| // is `miden_validator::attestation_commitment`. |
There was a problem hiding this comment.
Could the canonical verifier live in shared node code, likely miden-node-proto?
This helper only builds a commitment inside miden-validator; every verification in this PR is handwritten in validator tests. Clients otherwise have to duplicate transcript parsing, signature checks, trusted-validator checks, and current/next role handling.
|
|
||
| // The key that replaces `current_key` at `rotation_block_num`. Encryption keys rotate every | ||
| // epoch, so this is populated except in the degenerate case where no next epoch exists. | ||
| optional NextTransactionEncryptionKey next_key = 2; |
There was a problem hiding this comment.
Because next_key is optional and each key is signed separately, an untrusted RPC can remove a scheduled next key while the current signature still verifies. Could the attestation also bind the complete current/optional-next schedule? Manual rotation needs clients to distinguish "nothing scheduled" from a stripped schedule.
| associated_data: &[u8], | ||
| ) -> anyhow::Result<Vec<u8>>; | ||
|
|
||
| /// Returns the secret key material of the given epoch's encryption key for archival, or `None` |
There was a problem hiding this comment.
Could we remove secret export and SQLite archival from this PR? A decryption provider should keep secret bytes private and expose only operations. The current API rules out an operation-only TEE, KMS, or HSM provider and stores plaintext key material in the validator database.
| block_header BLOB NOT NULL | ||
| ) WITHOUT ROWID; | ||
|
|
||
| CREATE TABLE encryption_keys ( |
There was a problem hiding this comment.
Could this table and the related schema change be removed with key archival? If archival remains, it needs a new 002_... migration. Editing 001_initial.sql changes the version 1 schema hash, so databases created by the current release fail version_check() before migration can run.
| } | ||
| false | ||
| }, | ||
| () = tokio::time::sleep(retry_delay), if retry_pending => true, |
There was a problem hiding this comment.
Could the automatic rotation worker be removed in favor of an explicit rotation operation at an epoch boundary? The current retry loop also has a bug. Each tip update recreates the 30 second sleep, so frequent blocks can prevent a pending retry forever.
| already trust from the chain and reconstruct the encryption key with miden-crypto. The exact attestation payload is | ||
| documented on the `ValidatorKeyAttestation` proto message. Note that this scheme does not hide transaction inputs from | ||
| holders of the shared encryption secret (currently the network operator and every validator) and provides no forward | ||
| secrecy. The attestation proves which validator vouched for the key but does not prove freshness: a replayed older |
There was a problem hiding this comment.
Could the response bind the current key's activation block and provide one shared check against trusted chain state?
A valid old current_key response can otherwise be replayed forever, so fetching again does not tell a client whether the key is still current.
|
@juan518munoz I've opened #2382 as a simplification proposal. |
Summary
Continues the transaction input encryption work from #2342 (part of #2319) by making the shared transaction encryption key rotate perpetually, once per epoch, with no operator action and no validator coordination.
Per-epoch key derivation
The secret configured via
--encryption-key.hex/--encryption-key.kms-ciphertextis now a master secret rather than the encryption key itself. Each epoch's X25519 key is derived deterministically from it:Since every validator holds the same master secret, all validators derive identical keys for every epoch and transition to the new key at each boundary independently. The key provider owns the derivation and is now epoch-aware, including a grace window on decryption so submissions sealed just before a boundary remain decryptable after it.
Changelog