Closed ghost closed 1 month ago
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.
I have addressed all issues except:
clang-format
but it looks like the CI check is using different parameters, and I cannot reproduce that format, even using clang-format-17)Will tackle those next.
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.
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
.
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.
Please (re-)review.
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.
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.
Again, thank you very much for your feedback. I have implemented all of it. Please re-check.
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. 😞
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 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.
Seems low risk to me so fine for 3.6
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 pickedBOTAN_TARGET_OS_HAS_GETRANDOM
here forSystem_RNG_Impl
.