Skip to content
This repository has been archived by the owner on Sep 20, 2023. It is now read-only.

DRG interface improvements #323

Closed
wants to merge 13 commits into from
Closed

Conversation

infinity0
Copy link

This PR adds a PRG typeclass that supports initialising from a seed (which the system DRG cannot implement), and exposes raw functionality in ChaCha so other people can implement their own PRGs.

Motivation: I'm writing a network simulator that can run simulations deterministically with arbitrary suspend-resume capabilities. To do this, I need to be able to suspend the state of the PRG and write it to disk, then read it later. I appreciate this might be a controversial feature so didn't include it in this PR, but have merely exposed the functionality needed to easily write my own PRG.

If you like, I can also add a commit that includes a ChaChaPureInsecure instance of PRG that uses ByteString and derives (Show, Read, Generic), etc for serialisation purposes. (Using ScrubbedBytes is also possible for more security, but is more work as you have to write some of the instances by hand, I didn't get around to it yet.)

@infinity0 infinity0 force-pushed the master branch 2 times, most recently from beecd66 to 080a57e Compare June 8, 2020 12:58
@ocheron
Copy link
Contributor

ocheron commented Jun 10, 2020

For now I looked only at the ChaCha changes.
You generalize StateSimple to arbitrary ByteArray instances.
This has 2 consequences:
(1) It can't ensure that the input to generateSimple has expected length, so the C code may read past the buffer end
(2) It exposes state internals which are architecture dependent

While for (1) the class in Data.ByteArray.Sized could help, for (2) maybe explicit functions to serialize/deserialize StateSimple in a format with defined and stable endianness would be preferable.

@infinity0
Copy link
Author

@ocheron Thanks for the comment, I overlooked the arch-dependent-ness of it. How about 75d7876 ?

@infinity0
Copy link
Author

@ocheron Hi, mind taking another look?

Copy link
Contributor

@ocheron ocheron left a comment

Choose a reason for hiding this comment

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

I'm not super familiar with the random part but think this is ok to add, with a few review comments.

Crypto/Cipher/ChaCha.hs Show resolved Hide resolved
Crypto/Internal/ByteArray.hs Outdated Show resolved Hide resolved
module Crypto.Random.Types
(
MonadRandom(..)
, MonadPseudoRandom
, DRG(..)
, PRG(..)
, prgNewEntropy
, prgNew
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer not to expose prgNewEntropy unless absolutely necessary. This function relies on its argument always returning requested length.

Copy link
Author

@infinity0 infinity0 Jun 22, 2020

Choose a reason for hiding this comment

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

I can remove it, but it's really just a convenience wrapper around prgNewSeed, a typeclass method of the new PRG class I'm adding in this PR. Anyone can recreate prgNewEntropy themselves, so I'm not sure hiding it achieves anything in terms of avoiding misuse.

For prgNewSeed, I basically copied the type signature of the existing ChaCha.initializeSimple. Its implementation checks the length of the seed, but the type signature does not make this obvious. So I could modify both of these to return a CryptoFailable StateSimple/gen instead - and then this type would propagate through into prgNewEntropy. How does that sound? That should make it much more obvious that anyone implementing prgNewSeed should explicitly check the length.

Copy link
Author

Choose a reason for hiding this comment

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

For example, in 9922075

Copy link
Contributor

Choose a reason for hiding this comment

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

I still think the real convenience wrapper is prgNew, without a CryptoFailable result.
MonadRandom is supposed to always return the requested length.
So if the PRG and MonadRandom instances are implemented correctly, a result will be generated.

Misuse comes only with prgNewEntropy.
People not wanting getRandomBytes, or wanting length larger than minimum, can call the PRG methods directly.
They will know best if the length requirement is always met or not.

Copy link
Author

Choose a reason for hiding this comment

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

In the interests of getting this PR merged, I don't want to continue arguing over a minor point, and so I've un-exposed prgNewEntropy as you wanted, and made prgNew auto-unwrap the CryptoFailable.

However, I still believe the old version is OK and more helpful for users. In Haskell, typeclass instances are chosen by the caller, so I don't see a fundamental usage/abuse/security difference between:

  1. letting the user implicitly select the instance of MonadRandom class in prgNew, vs
  2. letting the user explicitly select the getEntropy function in prgNewEntropy

Either of these could return the wrong length, but (a) the programmer can easily check the function they're using behaves correctly and (b) since 9922075 we mandate a length check in prgNewSeed.

Arguably, the explicit version (2) is easier to check - with MonadRandom (1) you have to look up the instance definition first, which could be a redirection to another function (e.g. MonadRandom IO), so it takes two steps. This is why in my own code, I prefer to use getEntropy explicitly and avoid the implicit MonadRandom IO instance.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't want to merge initializeSimpleErr like this.
Bytes after the 40th are ignored and don't contribute to the DRG.

Copy link
Author

Choose a reason for hiding this comment

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

@ocheron It is the same as the current behaviour of initializeSimple - as you can see in the diff I did not change the behaviour. How do you want to fix it, and shouldn't it be done in a separate PR?

Crypto/Random/Types.hs Outdated Show resolved Hide resolved
infinity0 added a commit to infinity0/cryptonite that referenced this pull request Jun 30, 2020
@infinity0
Copy link
Author

ping @ocheron

@infinity0
Copy link
Author

ping @ocheron I am still interested in getting this PR merged, can you elaborate on the next steps?

@ocheron
Copy link
Contributor

ocheron commented Oct 1, 2020

Sorry I don't do Haskell anymore and have left the project.

@infinity0
Copy link
Author

@ocheron OK, thanks for the response and best of luck with whatever you're doing now!

@vincenthz can you take over and have a look please?

@infinity0
Copy link
Author

Ping @vincenthz again - I've also added a generic prgFork implementation that addresses #15.

@vincenthz
Copy link
Member

archiving repository

@vincenthz vincenthz closed this Sep 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants