botan icon indicating copy to clipboard operation
botan copied to clipboard

Add jitter entropy to the System RNG (BOTAN_TARGET_OS_HAS_CCRANDOM)

Open planckdz opened this issue 1 year ago • 1 comments

Add jitterentropy-library to Botan as a module. When enabled, the system RNG can use this additional entropy and depends on jitterentropy-library.

So far I have implemented this only for BOTAN_TARGET_OS_HAS_CCRANDOM for Botan 3, which got configured fairly automatically for my macOS machine.

If this is something that is useful for Botan. I can easily provide implementation for other systems that I have access to, (windows and ubuntu, plus macOS/Android with arc4random). Plus port it to the Botan 2 branch.

I did not format system_rng.cpp yet because most changes would be in existing code (using vs code which should pick up .clang-format).

There is an integration project (using submodules for Botan and the jitter library), in case this is of use for someone. It probably works on linux too, but I'm not sure it the jitter will actually be used there because of the usage of BOTAN_TARGET_OS_HAS_CCRANDOM. At least with tests for Botan 2, a WSL ubuntu picked BOTAN_TARGET_OS_HAS_GETRANDOM here for System_RNG_Impl.

planckdz avatar Aug 24 '24 18:08 planckdz

Coverage Status

coverage: 90.985% (-0.02%) from 91.004% when pulling 5323fa8eba6686ccff2efc4fef7a97aa53d1a5a2 on plancksecurity:jitter into 1fe3150f597053313ecd2b22127ddd9a097c81ca on randombit:master.

coveralls avatar Aug 26 '24 22:08 coveralls

I rewrote this as an RNG and implemented all feedback points, plus a very basic test to make sure code gets executed. Please re-check and let me know.

ghost avatar Sep 01 '24 08:09 ghost

I have addressed all issues except:

  • Formatting (I'm using clang-format but it looks like the CI check is using different parameters, and I cannot reproduce that format, even using clang-format-17)
  • Entropy source
  • FFI (DONE)

Will tackle those next.

ghost avatar Sep 07 '24 10:09 ghost

I have not figured out the formatting issue. If you have a script somewhere in the repo, please tell. I responded to every feedback point, please let me know if I misunderstood. There's now a jitter RNG and jitter entropy source.

ghost avatar Sep 08 '24 09:09 ghost

I have not figured out the formatting issue. If you have a script somewhere in the repo, please tell. I responded to every feedback point, please let me know if I misunderstood. There's now a jitter RNG and jitter entropy source.

Formatted, using run_clang_format.py and clang-format-mp-17.

ghost avatar Sep 11 '24 09:09 ghost

TODO: Comply to doc/dev_ref/contributing.rst.

Update: I thought I had to add python tests, but nothing to do here from my point of view since no actual changes to ffi.h. So DONE.

ghost avatar Sep 14 '24 08:09 ghost

Please (re-)review.

ghost avatar Sep 21 '24 07:09 ghost

Thank you very much for the great feedback, I have pushed a new update. Especially the declaration/definition of Jitter_RNG_Internal may require another look. Please let me know.

dirkz avatar Oct 05 '24 07:10 dirkz

About the declaration/definition of Jitter_RNG_Internal:

Using anonymous namespace in headers is strongly discouraged. It generates a warning on my computer (Ubuntu x86_64 using GCC 13.2). I don't think it's harmful in this case because there's only a forward declaration in there but I don't see any benefits either. The default visibility is already hidden.

Personally I think the best would be to just use an attribute to hide the nested internal class declaration, but this will probably not work on windows because there's no way to disable __dclspec(dllexport) of a nested definition of an exported class. I'm not sure if __dclspec(dllexport) propagates to nested type definitions though. I will test it.

Considering this, the best compromise will probably be to just make the internal class a standalone declaration. It won't generate symbols and we'll have to deal with a bit of namespace pollution.

Delta-dev-99 avatar Oct 05 '24 12:10 Delta-dev-99

Again, thank you very much for your feedback. I have implemented all of it. Please re-check.

dirkz avatar Oct 08 '24 18:10 dirkz

Regarding CI: Here a patch that enables running your test in (some) CI configurations: https://github.com/reneme/botan/pull/6/commits/f22d6b894c821593eedf4641964b14d34c86be56 You might need to rebase to the latest master branch before this applies. Also note: https://github.com/randombit/botan/pull/4325#discussion_r1793084723 Please cherry-pick my patch and update this pull request.

@randombit I had to build jitterentropy from source, because Ubuntu doesn't provide a package for it. 😞

reneme avatar Oct 09 '24 08:10 reneme

Regarding CI: Here a patch that enables running your test in (some) CI configurations: reneme@f22d6b8 You might need to rebase to the latest master branch before this applies. Also note: #4325 (comment) Please cherry-pick my patch and update this pull request.

Rebased to current master and your CI change cherry-picked.

dirkz avatar Oct 09 '24 10:10 dirkz

@randombit Your call regarding merging before or after the 3.6.0 release. Given that this is entirely inside an optional module, there's probably little chance for general surprises here.

reneme avatar Oct 09 '24 15:10 reneme

Seems low risk to me so fine for 3.6

randombit avatar Oct 10 '24 06:10 randombit