Upgrade rand dependency, use os_rng for seed generation
#683
Conversation
|
👋 Thanks for assigning @tankyleo as a reviewer! |
tnull
force-pushed
the
2025-10-use-os-rng
branch
from
f139d39 to
42b7d21
Compare
We bump our rand dependency to the latest stable version.
The previously-used `thread_rng` should be fine, but `os_rng` is guaranteed to block until there is sufficient entropy available (e.g., after startup), which might slightly improve security here.
tnull
force-pushed
the
2025-10-use-os-rng
branch
from
42b7d21 to
e173997
Compare
|
Flaky CI is unrelated. |
| impl EntropySource for RandEntropySource { | ||
| fn fill_bytes(&self, buffer: &mut [u8]) { | ||
| rand::thread_rng().fill_bytes(buffer); | ||
| rand::rng().fill_bytes(buffer); |
There was a problem hiding this comment.
In the next commit, I would have used OsRng here too since it's used for cryptographic nonces.
There was a problem hiding this comment.
I am conscious of the performance tradeoff here as this is a frequent operation, maybe OsRng would be overkill ? Cryptographic nonces have stronger requirements than the identifiers we use ThreadRng for elsewhere
There was a problem hiding this comment.
Yes, that's why I kept it with the thread RNG, which should be perfectly fine for cryptographic operations. I guess if we'd want to be super cautious we could look into periodic re-seeding via ReseedingRng, but not even sure if that is warranted. WDYT?
There was a problem hiding this comment.
Looked into the current reseed behaviour, it's already automatically called every 64kB, I think we are good there.
They do recommend to explicitly reseed on any fork, but I don't think we ever fork right ? If so I think I am good with the current choice.
There was a problem hiding this comment.
They do recommend to explicitly reseed on any fork, but I don't think we ever fork right ? If so I think I am good with the current choice.
Yeah, was also tripping over that and considered reseeding on any new thread spawned by tokio. But, IIUC, that should happen automatically in the rng() call.
| OsRng.try_fill_bytes(&mut key).map_err(|e| { | ||
| log_error!(logger, "Failed to generate entropy: {}", e); | ||
| std::io::Error::new(std::io::ErrorKind::Other, "Failed to generate seed bytes") | ||
| })?; |
There was a problem hiding this comment.
If we propagate the Err from the OsRng here should we also propagate the Err in generate_entropy_mnemonic above ?
There was a problem hiding this comment.
I guess I am looking for consistency here, but maybe the difference is a public API function vs a function internal to the crate ?
There was a problem hiding this comment.
If we propagate the
Errfrom theOsRnghere should we also propagate theErringenerate_entropy_mnemonicabove ?
Well, we would need to introduce an enum error type just for this case, which seems like overkill. It could even be argued that we should to panic in this case, as according to the docs of OsRng failing is only expected in case of system misconfiguration and we fail to actually provide sufficient entropy, i.e., every operation after this could be considered unsafe.
There was a problem hiding this comment.
hmm ok would you consider panicking here ? sounds to me like OsRng returning an Error is very rare, but if it ever does, user should probably stop what they are doing and consider running ldk-node on a different machine.
There was a problem hiding this comment.
hmm ok would you consider panicking here ? sounds to me like
OsRngreturning anErroris very rare, but if it ever does, user should probably stop what they are doing and consider runningldk-nodeon a different machine.
Well, the other path will happen on build only, so instead of panicking we'd just be erroring out before we did anything really.
3 participants
We bump our
randdependency to the latest stable version.Additionally, we now use
os_rngfor seed/mnemonic generation. While the previously-usedthread_rngshould be totally fine, butos_rngis guaranteed to block until there is sufficient entropy available (e.g., after startup), which might slightly improve security here.