Skip to content
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

UnsoundPureKES and DirectSerialise API #504

Merged
merged 7 commits into from
Nov 6, 2024

Conversation

tdammers
Copy link
Contributor

@tdammers tdammers commented Oct 16, 2024

Description

This introduces two changes that are needed for introducing mlocked KES into ouroboros-consensus and implementing a KES agent:

  • The DirectSerialise API, an abstraction that allows us to send key data over a socket connection directly from mlocked memory, without using any intermediate variables on the GHC heap that might leak secrets to disk
  • Reinstating the non-mlocked KES API as UnsoundPureKES; this is necessary for a minimally disruptive migration path in ouroboros-consensus. We will use this API to keep the existing code, loading KES keys from disk, available, while adding KES agent connectivity (which will use mlocked memory throughout) as an alternative. Until all non-mlocked KES usage has been phased out, we will need to keep the UnsoundPureKES API around.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated
  • All visible changes are prepended to the latest section of a CHANGELOG.md for the affected packages.
    New section is never added with the code changes. (See RELEASING.md)
  • When applicable, versions are updated in .cabal and CHANGELOG.md files according to the
    versioning process.
  • The version bounds in .cabal files for all affected packages are updated.
    If you change the bounds in a cabal file, that package itself must have a version increase. (See RELEASING.md)
  • Self-reviewed the diff

@tdammers tdammers changed the title Reinstate non-mlocked KES as UnsoundPureKES UnsoundPureKES and DirectSerialise API Oct 16, 2024
@tdammers tdammers force-pushed the tdammers/mlocked-kes-for-consensus branch from dee6ddf to 5d87496 Compare October 16, 2024 07:33
@tdammers tdammers marked this pull request as ready for review October 16, 2024 07:35
@tdammers tdammers requested a review from lehins as a code owner October 16, 2024 07:35
@lehins
Copy link
Collaborator

lehins commented Oct 17, 2024

Is that a replacement for #317 or an extension of it? Before I waste time on reviewing both I'd like to know what the actual plan is.

@tdammers
Copy link
Contributor Author

This one includes the changes from #317, and, unlike #317, has already been rebased. So as long as you're fine having all the changes in a single PR, you can ignore #317 and just review this one.

Copy link
Collaborator

@lehins lehins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My apologies for taking so long to review this work.

It looks great! I only had a few suggestions, but nothing critical.

cardano-crypto-class/src/Cardano/Crypto/KES/Simple.hs Outdated Show resolved Hide resolved
cardano-crypto-class/src/Cardano/Crypto/KES/Simple.hs Outdated Show resolved Hide resolved
cardano-crypto-class/src/Cardano/Crypto/KES/Simple.hs Outdated Show resolved Hide resolved
cardano-crypto-class/src/Cardano/Crypto/KES/Simple.hs Outdated Show resolved Hide resolved
cardano-crypto-class/src/Cardano/Crypto/KES/Simple.hs Outdated Show resolved Hide resolved
cardano-crypto-class/src/Cardano/Crypto/KES/Mock.hs Outdated Show resolved Hide resolved
cardano-crypto-class/src/Cardano/Crypto/KES/CompactSum.hs Outdated Show resolved Hide resolved
@lehins lehins mentioned this pull request Oct 26, 2024
@lehins
Copy link
Collaborator

lehins commented Oct 30, 2024

I just noticed that Arbitrary instances for VerKeyKES and SigKES are gone. Could you please bring them back in this PR.

instance KESAlgorithm v => Arbitrary (VerKeyKES v) where
  arbitrary = deriveVerKeyKES <$> arbitrary

instance (KESAlgorithm v, ContextKES v ~ (), Signable v ~ SignableRepresentation)
      => Arbitrary (SigKES v) where
  arbitrary = do
    a <- arbitrary :: Gen Message
    sk <- arbitrary
    let sig = signKES () 0 a sk
    return sig

FYI. These arbitrary instances do not need to deriveVerKeyKES or signKES

All they need is to generate concrete types that contain random data with the expected length. So it would be sufficient to generate random bytestring and deserialize it into VerKeyKES and SigKES

@lehins
Copy link
Collaborator

lehins commented Oct 31, 2024

Here are the missing Arbitrary instances that should work:

instance KESAlgorithm v => Arbitrary (VerKeyKES v) where
  arbitrary = do
    bs <- genByteString (fromInteger (natVal (Proxy @(SizeVerKeyKES v))))
    case rawDeserialiseVerKeyKES bs of
      Nothing -> error "Impossible: the size of VerKeyKES is specified statically"
      Just vk -> pure vk

instance KESAlgorithm v => Arbitrary (SigKES v) where
  arbitrary = do
    bs <- genByteString (fromInteger (natVal (Proxy @(SizeSigKES v))))
    case rawDeserialiseSigKES bs of
      Nothing -> error "Impossible: the size of SigKES is specified statically"
      Just vk -> pure vk

instance UnsoundPureKESAlgorithm v => Arbitrary (UnsoundPureSignKeyKES v) where
  arbitrary = unsoundPureGenKeyKES <$> arbitrarySeedOfSize seedSize
    where
      seedSize = seedSizeKES (Proxy :: Proxy v)

@tdammers
Copy link
Contributor Author

tdammers commented Oct 31, 2024

Here are the missing Arbitrary instances that should work:

instance KESAlgorithm v => Arbitrary (VerKeyKES v) where
  arbitrary = do
    bs <- genByteString (fromInteger (natVal (Proxy @(SizeVerKeyKES v))))
    case rawDeserialiseVerKeyKES bs of
      Nothing -> error "Impossible: the size of VerKeyKES is specified statically"
      Just vk -> pure vk

instance KESAlgorithm v => Arbitrary (SigKES v) where
  arbitrary = do
    bs <- genByteString (fromInteger (natVal (Proxy @(SizeSigKES v))))
    case rawDeserialiseSigKES bs of
      Nothing -> error "Impossible: the size of SigKES is specified statically"
      Just vk -> pure vk

instance UnsoundPureKESAlgorithm v => Arbitrary (UnsoundPureSignKeyKES v) where
  arbitrary = unsoundPureGenKeyKES <$> arbitrarySeedOfSize seedSize
    where
      seedSize = seedSizeKES (Proxy :: Proxy v)

This hinges on the assumption that raw serialization works the way it currently does, i.e., just pack the raw key data into the buffer in binary form. But this is not part of the DirectSerialise contract - we do promise that a valid value can be serialized, and that the serialization of a valid value will deserialize cleanly, but we do not guarantee that any sequence of bytes that has the right length will be a valid serialization of a valid value.

In fact, the assumption does not hold for MockKES: here, the serialization format of a signkey is the serialization of the VerKey, plus a 64-bit word indicating the current KES evolution. If that evolution is larger than the maximum number of evolutions (as indicated by the type), then you get an expired key, and since the maximum number of evolutions is going to be very small compared to the maximum value of a 64-bit word, we would have to run a massive number of tests to reach those interesting code paths even once.

We can, however, generate a random seed and then go through unsoundPureGenKeyKES - seeds are, by definition, arbitrary binary data, so this will always produce a meaningful key. Downside is that the evolution will always be 0, so to get better coverage, we may have to evolve it a random number of times (up to the max evolution).

@lehins
Copy link
Collaborator

lehins commented Oct 31, 2024

we do not guarantee that any sequence of bytes that has the right length will be a valid serialization of a valid value.

We can provide Arbitrary instance through serialization for individual KES algorithms, it doesn't have to be catch all for all algorithms. So, we can still use serialization to provide instances. However, if you have better ideas on how to reinstate Arbitrary instances, then by all means, it doesn't really matter how as long as we can still use it for testing things like serialization

Copy link
Collaborator

@lehins lehins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Few more comments with respect to recent changes

cardano-crypto-class/src/Cardano/Crypto/DirectSerialise.hs Outdated Show resolved Hide resolved
cardano-crypto-class/src/Cardano/Crypto/DirectSerialise.hs Outdated Show resolved Hide resolved
cardano-crypto-class/src/Cardano/Crypto/KES/Mock.hs Outdated Show resolved Hide resolved
cardano-crypto-class/src/Cardano/Crypto/DirectSerialise.hs Outdated Show resolved Hide resolved
@tdammers
Copy link
Contributor Author

tdammers commented Nov 4, 2024

However, if you have better ideas on how to reinstate Arbitrary instances, then by all means, it doesn't really matter how as long as we can still use it for testing things like serialization

IMO the simplest way would be to generate an arbitrary seed, use the unsound KES API to generate a sign key from that, and then derive a verkey and/or signature. This will work for any KES algorithm, and it's completely agnostic of the serialization format.

Note however that we already have machinery in place for testing ser/deser. I could of course rewrite all that to use Arbitrary instances and the unsound KES API, but we'd still need the withSK machinery for other tests, and once we remove the unsound KES API again, we'll just need to put all that code back.

I'll provide the Arbitrary instances; whether you also want the ser/deser tests rewritten is your call.

@lehins
Copy link
Collaborator

lehins commented Nov 4, 2024

IMO the simplest way would be to generate an arbitrary seed, use the unsound KES API to generate a sign key from that, and then derive a verkey and/or signature. This will work for any KES algorithm, and it's completely agnostic of the serialization format.

Whatever route you think is the easiest way, go for it. We just to make sure we don't loose those Arbitrary instances, because they are needed for testing serialization

@tdammers tdammers force-pushed the tdammers/mlocked-kes-for-consensus branch from 12c4077 to a182417 Compare November 5, 2024 07:52
@tdammers tdammers merged commit 391a2c5 into master Nov 6, 2024
24 checks passed
@tdammers tdammers deleted the tdammers/mlocked-kes-for-consensus branch November 6, 2024 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants