Deprecate public use of the RAND_DRBG_USED_FLAGS define by mspncp · Pull Request #7190 · openssl/openssl

mspncp · 2018-09-11T22:41:11Z

The new DRBG API added the aforementioned define. However, it
is used internally only and having it defined publicly does not
serve any purpose except causing potential version compatibilty
problems.

mspncp · 2018-09-11T22:43:47Z

Should I add Fixes #7182 or should this wait until the define is removed?

paulidale · 2018-09-11T23:02:52Z

include/openssl/rand_drbg.h

Would it make more sense to wrapper this ~~and the define above~~ with an deprecated #ifdef?

Scratch the prior define. It is necessary.

Hmmm, how do you mean that? The flag is used internally and that will not change. Only the define will be moved from the public header into a private one or to drbg_lib.c, see #7182 (comment).

Ok, one could deprecate it and use a slightly renamed constant internally, as @mattcaswell suggested in #7182 (comment). But is it worth the effort?

I'd favour the final approach -- leave the current define there with its current value but wrapped:

#if OPENSSL_API_COMPAT < 0x10200000L # define RAND_DRBG_USED_FLAGS (RAND_DRBG_FLAG_CTR_NO_DF) #endif

Then add a new define to an internal header file that takes over the duties of the one. RAND_DRBG_ALL_FLAGS?

We can't deprecate something in a header by leaving it there. If we need it internally and we want to deprecate it, it should move to an internal header file.

Ok, convinced. I'll do it and use your name suggestion. But it's too late in the night now for me to scratch my head whether the internal define should go into drbg_lib.c only or into an internal header...

I'm happy to grab this one and run with it.

I'd go for drbg_lib.c since it isn't used anywhere else.
It also doesn't need to be a #define. const unsigned int?
The name could then be kept but in lower case.

mspncp · 2018-09-12T05:58:14Z

Reworked the pr following @paulidale's suggestions. Squashed the fixups, since it's a small pr.

mspncp · 2018-09-12T06:05:43Z

Note: new commit message

    Replace the public RAND_DRBG_USED_FLAGS #define by an internal constant

    The new DRBG API added the aforementioned #define. However, it is
    used internally only and having it defined publicly does not serve
    any purpose except causing potential version compatibility problems.

    Fixes #7182

The new DRBG API added the aforementioned #define. However, it is used internally only and having it defined publicly does not serve any purpose except causing potential version compatibility problems. Fixes openssl#7182

mattcaswell

Approved. Got to be the shortest lived symbol from introduction to deprecation.

mattcaswell · 2018-09-12T08:37:05Z

Should I add Fixes #7182 or should this wait until the define is removed?

I would add it. The deprecation in the header file is enough of a prompt for us to come back later and remove it.

mspncp · 2018-09-12T09:50:21Z

I would add it. The deprecation in the header file is enough of a prompt for us to come back later and remove it.

Agreed. In fact, I already added it in fb08daa after addressing @paulidale's comments.

mspncp · 2018-09-12T09:51:35Z

Approved. Got to be the shortest lived symbol from introduction to deprecation.

I like scoring records ;-)

mspncp · 2018-09-12T09:54:04Z

Thanks for your approval, Matt. I'll wait for Pauli's feedback nevertheless, since it was his suggestion.

paulidale

One formatting nit but approved either way.

paulidale · 2018-09-12T21:08:47Z

crypto/rand/drbg_lib.c

+static const unsigned int rand_drbg_used_flags =
+    RAND_DRBG_FLAG_CTR_NO_DF |
+    /* add new entries before this line */
+    0;


These two lines should go I think.

i.e. either

static const unsigned int rand_drbg_used_flags = RAND_DRBG_FLAG_CTR_NO_DF ;

or everything on one line (if it fits);
Future items being added with the | leading the line.

The original intention was to be diff-friendly, i.e., adding one flag will produce only a single line diff. But I understand that it violates the coding style guidelines, so I'll change it.

Diffs don't handle things that move across files well.
Still approved.

… constant

mspncp · 2018-09-12T21:38:46Z

approved either way.

It looks like you did not press the [Approve] button yet.

paulidale · 2018-09-12T21:42:50Z

Sorry about missing the radio button selection.

The new DRBG API added the aforementioned #define. However, it is used internally only and having it defined publicly does not serve any purpose except causing potential version compatibility problems. Fixes #7182 Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from #7190)

The new DRBG API added the aforementioned #define. However, it is used internally only and having it defined publicly does not serve any purpose except causing potential version compatibility problems. Fixes #7182 Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from #7190) (cherry picked from commit c402e94)

mspncp · 2018-09-12T21:49:56Z

Merged, thanks!

mspncp added branch: master Applies to master branch branch: 1.1.1 Applies to OpenSSL_1_1_1-stable branch (EOL) labels Sep 11, 2018

mspncp mentioned this pull request Sep 11, 2018

RAND_DRBG_USED_FLAGS is problematic for ABI compat #7182

Closed

paulidale reviewed Sep 11, 2018

View reviewed changes

mspncp force-pushed the pr-deprecate-rand-drbg-used-flags branch from 372cb69 to a09ddda Compare September 12, 2018 05:55

mspncp force-pushed the pr-deprecate-rand-drbg-used-flags branch from a09ddda to 47649fe Compare September 12, 2018 06:04

mspncp force-pushed the pr-deprecate-rand-drbg-used-flags branch from 47649fe to fb08daa Compare September 12, 2018 06:16

mattcaswell approved these changes Sep 12, 2018

View reviewed changes

mattcaswell added the approval: done This pull request has the required number of approvals label Sep 12, 2018

paulidale reviewed Sep 12, 2018

View reviewed changes

fixup! Replace the public RAND_DRBG_USED_FLAGS #define by an internal…

11cf93a

… constant

paulidale approved these changes Sep 12, 2018

View reviewed changes

mspncp closed this Sep 12, 2018

mspncp mentioned this pull request Sep 12, 2018

Added DRBG_HMAC & DRBG_HASH #6779

Closed

2 tasks

mspncp added this to the 1.1.1a milestone Oct 23, 2018

mspncp deleted the pr-deprecate-rand-drbg-used-flags branch October 28, 2018 19:57

Uh oh!

Conversation

mspncp commented Sep 11, 2018

Uh oh!

mspncp commented Sep 11, 2018

Uh oh!

paulidale Sep 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mspncp commented Sep 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mspncp commented Sep 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattcaswell left a comment

Choose a reason for hiding this comment

Uh oh!

mattcaswell commented Sep 12, 2018

Uh oh!

mspncp commented Sep 12, 2018

Uh oh!

mspncp commented Sep 12, 2018

Uh oh!

mspncp commented Sep 12, 2018

Uh oh!

paulidale left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mspncp commented Sep 12, 2018

Uh oh!

paulidale commented Sep 12, 2018

Uh oh!

mspncp commented Sep 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

paulidale Sep 11, 2018 •

edited

Loading

mspncp commented Sep 12, 2018 •

edited

Loading

mspncp commented Sep 12, 2018 •

edited

Loading