Use mlocked KES internally #1374
Open
+622
−280
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tdammers
force-pushed
the
tdammers/mlocked-kes-rebase
branch
from
d7d3c8f to
3c994f0
Compare
tdammers
force-pushed
the
tdammers/mlocked-kes-rebase
branch
from
3c994f0 to
cfddb02
Compare
tdammers
requested review from
nfrisby,
jasagredo,
amesgen,
fraser-iohk,
dnadales and
geo2a
as code owners
jasagredo
requested changes
Feb 13, 2025
| @@ -143,6 +143,9 @@ data BlockForging m blk = BlockForging { | |||
| -> [Validated (GenTx blk)] -- Transactions to include | |||
| -> IsLeader (BlockProtocol blk) -- Proof we are leader | |||
| -> m blk | |||
|
|
|||
| , finalize :: m () | |||
There was a problem hiding this comment.
What is this "finalizing"? (please add some haddocks here too)
| @@ -108,6 +109,10 @@ data family CodecConfig blk :: Type | |||
| -- avoid circular dependencies. | |||
| data family StorageConfig blk :: Type | |||
|
|
|||
| -- | Credentials needed for block forging. In eras that use KES, this will be | |||
| -- a pair of KES sign key and OpCert; in other eras, it should be 'Void'. | |||
| type family BlockForgingCredentials blk :: Type | |||
There was a problem hiding this comment.
This is defined here, and for the HFBlock, but nowhere else?
| @@ -77,6 +77,8 @@ deriving stock instance CanHardFork xs => Show (LedgerState (HardForkBlock xs) | |||
| deriving stock instance CanHardFork xs => Eq (LedgerState (HardForkBlock xs)) | |||
| deriving newtype instance CanHardFork xs => NoThunks (LedgerState (HardForkBlock xs)) | |||
|
|
|||
| type instance BlockForgingCredentials (HardForkBlock '[blk]) = BlockForgingCredentials blk | |||
| @@ -11,6 +11,7 @@ | |||
| {-# LANGUAGE RecordWildCards #-} | |||
| {-# LANGUAGE ScopedTypeVariables #-} | |||
| {-# LANGUAGE TypeApplications #-} | |||
| {-# LANGUAGE TypeFamilies #-} | |||
| hardForkFinalize :: (Monad m, All Top xs) | ||
| => NonEmptyOptNP (BlockForging m) xs -> m () | ||
| hardForkFinalize blockForging = | ||
| -- pure () |
| @@ -475,12 +476,12 @@ protocolInfoCardano paramsCardano | |||
| , length credssShelleyBased > 1 | |||
| = error "Multiple Shelley-based credentials not allowed for mainnet" | |||
| | otherwise | |||
| = assertWithMsg (validateGenesis genesisShelley) | |||
| = assertWithMsg (validateGenesis genesisShelley) $ | |||
There was a problem hiding this comment.
Suggested change
| = assertWithMsg (validateGenesis genesisShelley) $ | |
| = assertWithMsg (validateGenesis genesisShelley) |
Comment on lines
+41
to
+43
| , ProtocolParamsByron | ||
| , ProtocolParamsShelleyBased | ||
| , CheckpointsMap |
There was a problem hiding this comment.
Why are these exported? They are not new to this PR
Comment on lines
+5
to
+11
| - Change `HotKey` to manage not only KES sign keys, but also the corresponding | ||
| OpCerts. This is in preparation for KES agent connectivity: with the new | ||
| design, the KES agent will provide both KES sign keys and matching OpCerts | ||
| together, and we need to be able to dynamically replace them both together. | ||
| - Add finalizer to `HotKey`. This takes care of securely forgetting any KES | ||
| keys the HotKey may still hold, and will be called automatically when the | ||
| owning block forging terminates. |
There was a problem hiding this comment.
Isn't this done in the protocol package instead?
Comment on lines
+16
to
+18
| - The `KesKey` data type in `unstable-cardano-tools` has been renamed to | ||
| `UnsoundPureKesKey`, to reflect the fact that it uses the old, unsound KES | ||
| API (which does not use mlocking or secure forgetting). |
There was a problem hiding this comment.
We don't reflect these changes in the changelog
There was a problem hiding this comment.
I'm unsure we want to merge this as is. If we did so Consensus would be unreleasable until those other dependencies are released. I think we should wait until there are no more source-repository-package stanzas.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This changes Consensus such that mlocked KES keys are used internally.
This is important groundwork for supporting KES agents in the future. In this form, the code will still load KES keys from disk, which is unsound, but the internal machinery is ready to also accept KES keys from other sources, and once loaded, KES keys will be handled appropriately (kept in mlocked RAM at all times, securely erased when expired).
This also involves a restructuring of the
HotKeydata structure, which now manages not only a KES SignKey, but also the corresponding OpCert. This is necessary for two reasons:Supersedes #1284.