Add jitterentropy as RNG #4325

ghost · 2024-08-24T18:06:56Z

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.

randombit

So some significant skepticism here (ala #4309) but at least this is included in a few widely used Linux distros, so I'm relatively open to merging something that supports this source but I would be quite against doing so where the output of jitter is xored with the system RNG. This should either be its own RNG type, that is used if the application specifically wants to use it, or (I think much more appropriately) it should be an EntropySource that is used to feed userspace RNGs.

src/lib/jitter/info.txt

src/lib/jitter/jitter.cpp

src/lib/jitter/jitter.h

src/lib/rng/system_rng/system_rng.cpp

src/lib/jitter/info.txt

src/lib/jitter/jitter.cpp

coveralls · 2024-08-26T22:26:10Z

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

ghost · 2024-09-01T08:55:32Z

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.

randombit

OK looking better now, thanks for addressing the previous round. Some additional comments outside of the changed code so GH won’t let me comment

It might be useful to (also) expose Jitter_RNG as an entropy source, in the similar way as we do for System_RNG and Processor_RNG
Please also update the rng documentation
Depending on your use case you may want to also expose this to the C wrapper in botan_rng_init

src/lib/rng/jitter_rng/jitter.h

src/lib/rng/jitter_rng/jitter_rng.h

src/lib/rng/jitter_rng/jitter.cpp

src/lib/rng/jitter_rng/jitter_rng.h

src/lib/rng/jitter_rng/jitter.cpp

src/tests/test_jitter_rng.cpp

ghost · 2024-09-07T10:26:44Z

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 · 2024-09-08T09:15:33Z

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.

src/lib/rng/jitter_rng/jitter_rng.cpp

src/lib/entropy/entropy_srcs.cpp

src/lib/rng/jitter_rng/jitter_rng.cpp

src/lib/rng/jitter_rng/jitter_rng.h

src/lib/rng/jitter_rng/jitter_rng.cpp

ghost · 2024-09-21T07:30:26Z

Please (re-)review.

src/lib/rng/jitter_rng/jitter_rng.h

src/lib/rng/jitter_rng/jitter_rng.cpp

src/lib/rng/jitter_rng/jitter_rng.h

reneme

Here's another round of polishing. Along with a few nits and style/consistency suggestions, I have a left an idea for compacting/improving the test code.

We should also enable this module in at least a few CI targets to ensure sustainability. Since it introduces a new (optional) dependency it'll require a bit of CI script mangling. No worries, I'll send you a patch for it next week.

news.rst

src/lib/rng/jitter_rng/jitter_rng.cpp

src/tests/test_jitter_rng.cpp

dirkz · 2024-10-05T07:31:58Z

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.

Delta-dev-99 · 2024-10-05T12:47:21Z

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.

reneme

A few more minor nits. Most notably, the BOTAN_ASSERT suggestions are purely optional, in my opinion.

Also a suggestion for an implementation of RandomNumberGenerator::clear() that might (or might not) make sense for this adapter.

Generally, I think this converged nicely! Thanks for your patience. :)

src/lib/rng/jitter_rng/jitter_rng.cpp

src/lib/rng/jitter_rng/jitter_rng.h

src/lib/rng/jitter_rng/jitter_rng.cpp

src/lib/rng/jitter_rng/jitter_rng.h

doc/api_ref/rng.rst

src/tests/test_jitter_rng.cpp

dirkz · 2024-10-08T18:41:02Z

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

reneme

Thank you! This looks good now.

Still to do on our side:

CI integration (probably for the coverage, valgrind and sanitizer builds ?)

On the note of CI: jitterentropy-library (standalone, not the daemon) doesn't seem to exist as a package for ubuntu. Or am I missing something? For testing locally, I built it from source. This isn't a big deal as the library is small, but I'd like to omit this on CI if possible.

src/tests/test_jitter_rng.cpp

reneme · 2024-10-09T08:43:31Z

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.

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

Currently Ubuntu does not ship with a package for the jitterentropy-library. Therefore, this pulls the sources from the maintainer's GitHub and builds/installs it from source. The library is tiny, so there's no significant overhead in time, but of course the additional build dependency on another repository is a bummer.

dirkz · 2024-10-09T10:58:25Z

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.

randombit

LGTM

reneme · 2024-10-09T15:08:37Z

@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.

randombit · 2024-10-10T06:22:26Z

Seems low risk to me so fine for 3.6

ghost force-pushed the jitter branch from a3bd7f8 to cae3144 Compare August 24, 2024 18:10

randombit requested changes Aug 26, 2024

View reviewed changes

ghost marked this pull request as draft August 27, 2024 06:34

ghost force-pushed the jitter branch from cae3144 to 331af13 Compare September 1, 2024 07:28

ghost changed the title ~~Add jitter entropy to the System RNG (BOTAN_TARGET_OS_HAS_CCRANDOM)~~ Add jitterentropy as RNG Sep 1, 2024

ghost force-pushed the jitter branch 3 times, most recently from 1e9180c to d56064d Compare September 1, 2024 08:51

ghost marked this pull request as ready for review September 1, 2024 08:55

ghost requested a review from randombit September 1, 2024 08:56

randombit requested changes Sep 1, 2024

View reviewed changes

ghost requested a review from randombit September 8, 2024 09:13

randombit reviewed Sep 8, 2024

View reviewed changes

ghost force-pushed the jitter branch from 7d0d2dc to b60a8be Compare September 11, 2024 08:52

This comment was marked as resolved.

Sign in to view

ghost requested a review from randombit September 11, 2024 11:34

This comment was marked as resolved.

Sign in to view

ghost force-pushed the jitter branch 3 times, most recently from c7cb815 to d7754ed Compare September 21, 2024 07:26

Delta-dev-99 reviewed Sep 26, 2024

View reviewed changes

src/lib/rng/jitter_rng/jitter_rng.h Outdated Show resolved Hide resolved

ghost force-pushed the jitter branch from d7754ed to 812e156 Compare September 26, 2024 07:50

Delta-dev-99 suggested changes Sep 27, 2024

View reviewed changes

src/lib/rng/jitter_rng/jitter_rng.h Outdated Show resolved Hide resolved

src/lib/rng/jitter_rng/jitter_rng.h Outdated Show resolved Hide resolved

src/lib/rng/jitter_rng/jitter_rng.cpp Outdated Show resolved Hide resolved

dirkz force-pushed the jitter branch from 812e156 to fe7ec4e Compare September 27, 2024 07:36

Delta-dev-99 reviewed Sep 28, 2024

View reviewed changes

src/lib/rng/jitter_rng/jitter_rng.h Outdated Show resolved Hide resolved

dirkz force-pushed the jitter branch from fe7ec4e to cc43d40 Compare September 28, 2024 07:46

reneme requested changes Oct 2, 2024

View reviewed changes

dirkz force-pushed the jitter branch from cc43d40 to 61927c6 Compare October 5, 2024 07:26

dirkz force-pushed the jitter branch from 61927c6 to 9404ef2 Compare October 7, 2024 05:32

reneme reviewed Oct 7, 2024

View reviewed changes

src/tests/test_jitter_rng.cpp Outdated Show resolved Hide resolved

src/tests/test_jitter_rng.cpp Outdated Show resolved Hide resolved

src/tests/test_jitter_rng.cpp Show resolved Hide resolved

dirkz force-pushed the jitter branch 2 times, most recently from 6aafa62 to 57ccd78 Compare October 8, 2024 18:39

reneme approved these changes Oct 9, 2024

View reviewed changes

src/tests/test_jitter_rng.cpp Show resolved Hide resolved

reneme reviewed Oct 9, 2024

View reviewed changes

src/tests/test_jitter_rng.cpp Show resolved Hide resolved

CPU jitter RNG and entropy source

5f0d4eb

dirkz force-pushed the jitter branch from 57ccd78 to 5f0d4eb Compare October 9, 2024 10:50

randombit approved these changes Oct 9, 2024

View reviewed changes

randombit merged commit 0d8c1dd into randombit:master Oct 10, 2024
40 checks passed

reneme mentioned this pull request Oct 11, 2024

TPM2: Basic bindings in FFI & Python #4361

Merged

reneme mentioned this pull request Oct 25, 2024

Add jitter_rng module to crypto documentation sehlen-bsi/botan-docs#251

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add jitterentropy as RNG #4325

Add jitterentropy as RNG #4325

ghost commented Aug 24, 2024

randombit left a comment

coveralls commented Aug 26, 2024 •

edited

Loading

ghost commented Sep 1, 2024

randombit left a comment

ghost commented Sep 7, 2024 •

edited by ghost

Loading

ghost commented Sep 8, 2024

This comment was marked as resolved.

This comment was marked as resolved.

ghost commented Sep 21, 2024

reneme left a comment

dirkz commented Oct 5, 2024

Delta-dev-99 commented Oct 5, 2024

reneme left a comment

dirkz commented Oct 8, 2024

reneme left a comment •

edited

Loading

reneme commented Oct 9, 2024 •

edited

Loading

dirkz commented Oct 9, 2024

randombit left a comment

reneme commented Oct 9, 2024

randombit commented Oct 10, 2024

Add jitterentropy as RNG #4325

Add jitterentropy as RNG #4325

Conversation

ghost commented Aug 24, 2024

randombit left a comment

Choose a reason for hiding this comment

coveralls commented Aug 26, 2024 • edited Loading

ghost commented Sep 1, 2024

randombit left a comment

Choose a reason for hiding this comment

ghost commented Sep 7, 2024 • edited by ghost Loading

ghost commented Sep 8, 2024

This comment was marked as resolved.

This comment was marked as resolved.

ghost commented Sep 21, 2024

reneme left a comment

Choose a reason for hiding this comment

dirkz commented Oct 5, 2024

Delta-dev-99 commented Oct 5, 2024

reneme left a comment

Choose a reason for hiding this comment

dirkz commented Oct 8, 2024

reneme left a comment • edited Loading

Choose a reason for hiding this comment

reneme commented Oct 9, 2024 • edited Loading

dirkz commented Oct 9, 2024

randombit left a comment

Choose a reason for hiding this comment

reneme commented Oct 9, 2024

randombit commented Oct 10, 2024

coveralls commented Aug 26, 2024 •

edited

Loading

ghost commented Sep 7, 2024 •

edited by ghost

Loading

reneme left a comment •

edited

Loading

reneme commented Oct 9, 2024 •

edited

Loading