Conversation
paulidale
added
the
approval: review pending
|
Conflicts resolved, rebased to master. Ping @openssl/committers for review please. |
There was a problem hiding this comment.
If I understand it correctly, the APIs RAND_DRBG_get0_{public,private,primary}() and RAND_get0_{public,private,primary}_drbg() return two unrelated triples of random generators. For example, if
RAND_DRBG* drbg_public = RAND_DRBG_get0_public();
EVP_RAND_CTX* rand_public = RAND_get0_public_drbg(NULL);
then drbg_public->rand != rand_public. The same holds for drbg_private and drbg_primary. If that is the case (which I checked in the debugger, see example below), then your construction does not work, because it breaks the API promise in the following way: The documented behaviour of version 1.1.1 is that you can change the seed source for the default OpenSSL CSPRNG, which implements the RAND_bytes() call, by replacing the callbacks of the primary DRBG (RAND_DRBG_set_callbacks(RAND_DRBG_get0_primary(), ...)). In your setup, modifying the primary DRBG will not have an effect on RAND_bytes(), because it calls into the primary EVP_RAND_CTX.
The good news: The situation might not be entirely lost, if manage to construct the legacy DRBGs in such a way that drbg_{pub,priv,prim}->rand == rand_{pub,priv,prim}.
Example
I modified the openssl rand command as follows to verify in the debugger that drbg->rand != rand.
diff --git a/apps/rand.c b/apps/rand.c
index 1ad6894597..66d9a41308 100644
--- a/apps/rand.c
+++ b/apps/rand.c
@@ -17,6 +17,7 @@
#include <openssl/bio.h>
#include <openssl/err.h>
#include <openssl/rand.h>
+#include <openssl/rand_drbg.h>
typedef enum OPTION_choice {
OPT_ERR = -1, OPT_EOF = 0, OPT_HELP,
@@ -48,6 +49,9 @@ const OPTIONS rand_options[] = {
int rand_main(int argc, char **argv)
{
+ RAND_DRBG *drbg_pub, *drbg_priv, *drbg_prim;
+ EVP_RAND_CTX *rand_pub, *rand_priv, *rand_prim;
+
ENGINE *e = NULL;
BIO *out = NULL;
char *outfile = NULL, *prog;
@@ -109,6 +113,15 @@ int rand_main(int argc, char **argv)
out = BIO_push(b64, out);
}
+ drbg_pub = RAND_DRBG_get0_public();
+ rand_pub = RAND_get0_public_drbg(NULL);
+
+ drbg_priv = RAND_DRBG_get0_private();
+ rand_priv = RAND_get0_private_drbg(NULL);
+
+ drbg_prim = RAND_DRBG_get0_master();
+ rand_prim = RAND_get0_primary_drbg(NULL);
+
while (num > 0) {
unsigned char buf[4096];
int chunk;
|
A general remark about the documentation: The new EVP_RAND interface is intended to be more generic than the RAND_DRBG interface, which it supersedes. A DRBG is just a specific type of EVP_RAND implementation (it happens to be OpenSSL's current default implementation). Currently there are a lot of places in the documentation (in particular EVP_RAND.pod) and the source code, where the term 'DRBG' is used for an arbitrary |
This doesn't work well because RAND_DRBG allows far too much invasive control over the internals for the underlying EVP_RAND. If there are shared, we're back to having the additional layer which this PR is trying to remove. |
That's exactly what I am trying to tell you all the time: the only way to get rid of the mess is to remove the RAND_DRBG interface in 3.0 entirely, not just to deprecate it. This is a breaking change, I know, but it's a clean break and we can give good reasons for doing it. Your current approach is also a breaking change, in a very subtle way which will cause much more confusion. |
|
Okay, we're going to need an OMC vote either on: "remove RAND_DRBG complete" or "deprecate it but with slightly different behaviour". |
Yes, I agree. But preceding the vote I expect a public discussion on openssl-project. ;-) |
|
Pausing this pending the vote outcome. |
paulidale
added
hold: discussion
|
@mspncp, I'm not sure where to relocate the text in |
|
Two removal commits added -- if the current vote passes, this is roughly in the state we want. |
Sorry for not answering earlier, I was too busy yesterday. Moving RAND_DRBG(7) to EVP_RAND(7) looks like a reasonable solution. |
|
I moved some of the text to RAND(7) as well. |
paulidale
force-pushed
the
dep-rand
branch
2 times, most recently
from
e5028e0 to
cc56c2c
Compare
|
"These" as in changes and news. |
|
The last commit pushed avoids the memory leak. There is something peculiar going on and preallocating the primary DRBG avoids it. Without this fix, the leak happens on the first |
|
GitHub has truncated thread #12509 (comment) and some posts are missing for unknown reason, so I quote them here:
I'm not keen to add yet another rand prefix, for just three functions. Also, it should be |
mspncp
removed
the
severity: urgent
|
@paulidale I removed the 'urgent' label, since we missed the alpha release anyway. Could you reorganize your commits and bring them into the final form in which you intend to merge them? (Preferrably without moving the merge-base). |
|
Ugg, conflicts in |
The RAND_DRBG API did not fit well into the new provider concept as implemented by EVP_RAND and EVP_RAND_CTX. The main reason is that the RAND_DRBG API is a mixture of 'front end' and 'back end' API calls and some of its API calls are rather low-level. This holds in particular for the callback mechanism (RAND_DRBG_set_callbacks()) and the RAND_DRBG type changing mechanism (RAND_DRBG_set()). Adding a compatibility layer to continue supporting the RAND_DRBG API as a legacy API for a regular deprecation period turned out to come at the price of complicating the new provider API unnecessarily. Since the RAND_DRBG API exists only since version 1.1.1, it was decided by the OMC to drop it entirely. Other related changes: Use RNG instead of DRBG in EVP_RAND documentation. The documentation was using DRBG in places where it should have been RNG or CSRNG. Move the RAND_DRBG(7) documentation to EVP_RAND(7).
|
I've squashed it down to one commit for the removal plus a second commit for the No need to reset the approval timer for this. |
|
24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually. |
The RAND_DRBG API did not fit well into the new provider concept as implemented by EVP_RAND and EVP_RAND_CTX. The main reason is that the RAND_DRBG API is a mixture of 'front end' and 'back end' API calls and some of its API calls are rather low-level. This holds in particular for the callback mechanism (RAND_DRBG_set_callbacks()) and the RAND_DRBG type changing mechanism (RAND_DRBG_set()). Adding a compatibility layer to continue supporting the RAND_DRBG API as a legacy API for a regular deprecation period turned out to come at the price of complicating the new provider API unnecessarily. Since the RAND_DRBG API exists only since version 1.1.1, it was decided by the OMC to drop it entirely. Other related changes: Use RNG instead of DRBG in EVP_RAND documentation. The documentation was using DRBG in places where it should have been RNG or CSRNG. Move the RAND_DRBG(7) documentation to EVP_RAND(7). Reviewed-by: Matthias St. Pierre <[email protected]> (Merged from #12509)
Reviewed-by: Matthias St. Pierre <[email protected]> (Merged from #12509)
paulidale
added
approval: ready to merge
|
Merged to master. Thanks for the reviews and feedback. |
The RAND_DRBG API did not fit well into the new provider concept as implemented by EVP_RAND and EVP_RAND_CTX. The main reason is that the RAND_DRBG API is a mixture of 'front end' and 'back end' API calls and some of its API calls are rather low-level. This holds in particular for the callback mechanism (RAND_DRBG_set_callbacks()) and the RAND_DRBG type changing mechanism (RAND_DRBG_set()). Adding a compatibility layer to continue supporting the RAND_DRBG API as a legacy API for a regular deprecation period turned out to come at the price of complicating the new provider API unnecessarily. Since the RAND_DRBG API exists only since version 1.1.1, it was decided by the OMC to drop it entirely. Other related changes: Use RNG instead of DRBG in EVP_RAND documentation. The documentation was using DRBG in places where it should have been RNG or CSRNG. Move the RAND_DRBG(7) documentation to EVP_RAND(7). Reviewed-by: Matthias St. Pierre <[email protected]> (Merged from openssl#12509)
Reviewed-by: Matthias St. Pierre <[email protected]> (Merged from openssl#12509)
These are leftovers from the RAND_DRBG removal (openssl#12509).
These are leftovers from the RAND_DRBG removal (#12509). Reviewed-by: Paul Dale <[email protected]> (Merged from #12866)
6 participants
The RAND_DRBG API and implementation is convoluted and difficult. With the move to providers, it is being deprecated. The RAND APIs, however, remain unchanged.
Checklist