refactor(rand): deprecate internal DRBG implementation#5775
refactor(rand): deprecate internal DRBG implementation#5775kaukabrizvi merged 13 commits intoaws:mainfrom
Conversation
maddeleine
left a comment
There was a problem hiding this comment.
I think you should break this PR up into the minimal number of file changes needed to remove the custom DRBG. Then the second PR can be all the documentation changes needed. As it is, 38 files is a lot, and you want your reviewers to be able to focus on the change that really matters, the custom DRBG removal.
| /* Generates random data (every size between 1 and 5120 bytes) and performs | ||
| * basic pattern tests on the resulting output | ||
| */ | ||
| static S2N_RESULT s2n_basic_pattern_tests(S2N_RESULT (*s2n_get_random_data_cb)(struct s2n_blob *blob)) |
There was a problem hiding this comment.
For this test file in particular, I'm not sure that we should be deleting all these tests. The thing is, even if we're moving our randomness to the libcrypto, it still might be good to keep these tests around, no matter where we're getting our randomness.
Do they all still pass if you just don't delete any test in this file?
There was a problem hiding this comment.
No, they don't all pass if you keep the file unchanged because many tests call APIs that were deleted in this PR. There were a few tests that can be kept/slightly modified to still assert important properties of libcrypto randomness. I added back tests for s2n_public_random() range correctness, thread uniqueness, fork uniqueness (parent/child get different data), basic generate uniqueness, and urandom fd validation/re-open.
Goal
This PR removes our custom DRBG implementation.
Why
s2n historically maintained a custom DRBG to provide fast randomness and distinct public/private streams. Modern backends now provide secure and performant randomness implementations, allowing s2n to rely on the libcrypto directly. Removing the custom DRBG simplifies the codebase, reduces maintenance overhead, and defers randomness to the libraries responsible for handling and maintaining it.
It also resolves #4348, #3455, #4827, #3240.
How
In #5726, we began deferring randomness to the libcrypto when it had separate streams of public/private randomness. For AWS-LC versions prior to v1.68.0, before a public randomness API was added, we now just call RAND_priv_bytes and RAND_bytes. We expect most customers that build s2n from source to also build AWS-LC from source, and for customers who pin their LC version, this behavior would be identical to what we curently do for randomness in FIPS mode. The public/private stream separation provided in newer AWS-LC versions is a defense-in-depth measure. Customers requiring this guarantee can update to AWS-LC ≥1.68.0. For OpenSSL 1.0.2, LibreSSL, and BoringSSL we defer to system randomness by reading from dev/urandom. This fulfils the randomness requirements listed in the TLS 1.3 RFC while also being faster than calling their own random methods. It also fulfills the public/private stream concern since each call obtains fresh entropy directly from the kernel CSPRNG.
The PR also removes dead code, which was only referenced by the DRBG module including:
Callouts
RAND_bytes()for both public randomness andRAND_priv_bytes(wraps RAND_bytes) for private randomness (no explicit stream separation; treated as defense-in-depth only).s2n_rand_set_callbacks,s2n_cleanup, ands2n_cleanup_threadare retained as no-ops for backwards compatibility. These public APIs are now marked deprecated and retained only for backwards compatibility.Testing
This change primarily removes code and delegates randomness generation to libcrypto or the OS. As a result, correctness of the RNG itself (including fork-safety and uniqueness guarantees) is now the responsibility of the underlying libcrypto implementation.
Testing therefore focuses on validating the integration points rather than re-testing RNG behavior:
s2n_random_test.cverifies that randomness path selection behaves as expected (vias2n_use_libcrypto_rand()feature probes).s2n_libcrypto_test.cvalidates the randomness delegation assumptions for each libcrypto.I also ran a performance test locally with OpenSSL 1.0.2 and observed a 0.1% increase in handshake duration. This would be OS dependent but is always faster than calling OpenSSL 1.0.2 RAND-bytes.
Related
Related: #5726
Resolves: #4348, #3455, #4827, #3240
release summary: Removes s2n's internal DRBG and delegates randomness generation to libcrypto when supported.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.