-
Notifications
You must be signed in to change notification settings - Fork 247
Local sealing improvements #7554
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…ious_sealed_ledger_location
This reverts commit 56ce3b0.
| ); | ||
| if (ccf.node.shuffleSealedShares !== undefined) { | ||
| ccf.node.shuffleSealedShares(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unfortunate but required as 6.X does not have shuffleSealedShares.
Without this call, nodes which join the network must wait until the next rekey before they are recoverable.
To be clear this PR always ensures that if you can read recovery shares from the ledger for yourself, you have the latest ledger secret, its just that these nodes would not be able to get recovery shares and complete recovery until the rekey on the original network.
The alternative is to use a hook on the nodes table, in which case a joining node would be recoverable after the hook's transaction is committed, which should only be after a few seconds, but this change shuffles the shares in the same transaction, and so means that there is desync risk. (modulo during migration)
|
|
||
| // Do we also need to cleanse the recovery_key_pair or will openssl handle | ||
| // that in the EVP_KEY destructor? | ||
| OPENSSL_cleanse(derived_key.data(), derived_key.size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is necessary, and I can't figure it out from the openssl docs.
| // Do we also need to cleanse the recovery_key_pair or will openssl handle | |
| // that in the EVP_KEY destructor? | |
| OPENSSL_cleanse(derived_key.data(), derived_key.size()); | |
| OPENSSL_cleanse(derived_key.data(), derived_key.size()); |
src/node/rpc/node_frontend.h
Outdated
|
|
||
| auto* sealed_recovery_keys = | ||
| args.tx.template rw<SealedRecoveryKeys>(Tables::SEALED_RECOVERY_KEYS); | ||
| sealed_recovery_keys->remove(node_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the only place where nodes get cleaned up? I couldn't find anywhere else obvious but I may be missing something :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request significantly improves the local sealing mechanism for CCF by moving from a disk-based approach to a ledger-based approach. The key changes include:
Purpose:
The PR addresses potential skew issues between the latest ledger secret and sealed secrets by ensuring synchronization through ledger storage. Nodes now seal recovery keys that allow the primary to seal ledger secrets for all nodes, enabling robust disaster recovery.
Changes:
- Replaced disk-based sealed secret storage with ledger-based storage in two new tables
- Introduced RSA key pairs for recovery, with private keys sealed using SNP-derived keys
- Enabled the primary to seal ledger secrets for other nodes using their public keys
- Updated configuration to use
enable_local_sealingandprevious_local_sealing_identityinstead of file paths
Reviewed changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/node/local_sealing.cpp | New implementation of ledger-based sealing with RSA key pairs and AES-GCM encryption |
| src/node/local_sealing.h | Simplified header exposing public API for sealing operations |
| src/service/tables/local_sealing.h | New table definitions for sealed recovery keys and sealed shares |
| include/ccf/service/local_sealing.h | Public interface defining SealedRecoveryKey structure |
| src/node/node_state.h | Integration of sealing during node join and recovery |
| src/node/share_manager.h | Share management refactored to support sealed shares alongside recovery shares |
| src/node/rpc/node_frontend.h | RPC handlers updated to store/remove sealed recovery keys and shuffle sealed shares |
| src/node/rpc/node_call_types.h | Added sealed_recovery_key field to join/create node structures |
| src/js/extensions/ccf/node.cpp | Added JS API for shuffling sealed shares |
| samples/constitutions/default/actions.js | Constitution updated to shuffle sealed shares when nodes become trusted |
| tests/infra/remote.py | Updated test infrastructure to use new configuration parameters |
| tests/infra/node.py | Removed disk-based sealed secret management code |
| tests/e2e_operations.py | Updated tests to use ledger-based approach, removed disk corruption test |
| doc/operations/recovery.rst | Comprehensive documentation of new local sealing approach with diagrams |
| doc/audit/builtin_maps.rst | Documentation of new sealed_shares and sealed_recovery_keys tables |
| CMakeLists.txt | Added local_sealing.cpp to build |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
fournet
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, thanks!
To be on the safe side, we could also document that the authenticity of the key materials encrypted to the node public key stems from its inclusion in the ledger (not just successful decryption using the sealed private key).
The current local sealing approach can end up with skew between what the latest ledger secret and the sealed secret.
The idea is that we can force these to be in sync by sealing to the ledger and unsealing from it.
The complication is that these changes must occur in the same transaction as any ledger secret updates, and so the primary must be able to seal for other nodes.
Hence nodes include a sealed private key, and corresponding public key in their network info, and the primary uses the public key to seal the ledger secret.
Thus a recovering node can derive the sealing key, read the ledger to unseal the private key and decrypt the sealed ledger secret.
This process is summarised below:
flowchart TB subgraph SNP["SNP PSP"] DK["DERIVED_KEY"] VCEK Measurement Policy["UserData (Policy)"] TCB VCEK --> DK Measurement --> DK Policy --> DK TCB --> DK end subgraph KG["Key Generation"] subgraph Sealing Key SK["Sealing Key<br/>(HKDF)"] Label["Label: <br/>CCF_LOCAL_SEALING_KEY"] DK -->|ikm| SK Label -->|info| SK end RSA["Recovery Key<br/>(RSA Key Pair)"] PubKey["Public Key"] PrivKey["Private Key"] RSA --> PubKey RSA --> PrivKey LS["Ledger secret"] LSWK["Ledger secret wrapping key"] end subgraph Sealed["Store: nodes.sealed_recovery_keys"] SPK["Sealed Private Key<br/>(AES-GCM encrypted)"] SK -->|key| SPK PrivKey --> SPK StoredPubKey["Public Key (plaintext)"] PubKey --> StoredPubKey end subgraph Shares["Store: gov.sealed_shares table"] WLS["Wrapped Ledger Secret<br/>(AES-GCM encrypted)"] LSWK -->|key| WLS LS --> WLS EWK["Encrypted Wrapping Key<br/>(per-node, RSA-OAEP encrypted)"] StoredPubKey -->|key| EWK LSWK --> EWK end subgraph Recovery["Recovery Process"] UPK["Unsealed Private Key"] SK -->|key| UPK SPK --> UPK UWK["Unsealed Wrapping Key"] UPK -->|key| UWK EWK --> UWK ULS["Unsealed Ledger Secret"] UWK -->|key| ULS WLS --> ULS end