Fix & update documentation about RAND_priv_bytes() #6514

romen · 2018-06-18T16:21:52Z

Stemming from #6501 (comment) this PR updates the documentation about RAND_priv_bytes() and then adds missing documentation about BN_priv_rand() and BN_priv_rand_range():

documentation is updated
documentation is added

Pinging @kroeckx , @richsalz and @mspncp for comments: bear in mind that I'm not entirely familiar with pod syntax, so please double check for newbie errors!

kroeckx · 2018-06-18T16:29:42Z

At first look this all looks fine.

romen · 2018-06-18T16:35:54Z

doc/man3/BN_rand.pod

+same difference between RAND_bytes() and RAND_priv_bytes().
+
+The PRNG must be seeded prior to calling BN_rand(), BN_rand_range(),
+BN_priv_rand(), or BN_priv_rand_range().


@kroeckx Is the above paragraph still relevant? Or does it come from pre-1.1.0?

@mspncp , can you remove the "ready" label, so that we can wait for comments from @kroeckx about this?

I have the feeling these 2 lines in 1.1.1 and 1.1.0 are not relevant anymore as the PRNG is automatically seeded for 3rd party applications upon initializing the library, am I wrong?

@richsalz, please wait for the outstanding feedback from @kroeckx before merging (If you prefer, I can do the final merge, too. Just let me know.)

As far as I know, since 1.1.0 we will try to initialize on first use. So this is not relevant for those versions. But only 1.1.1 has the priv functions.

RAND_bytes has a comment about generating an error when it's not seeded. I wonder if we should add something like that here too.

@kroeckx I should have fixed this and the suggestion about adding entries in the HISTORY sections in 7bd4945.
Can you please review them again?

richsalz

Looks good to me. "make doc-nits" will find many errors BTW.

mspncp

LGTM. Just one nit, which does not need to be fixed.

mspncp · 2018-06-18T17:59:26Z

doc/man3/BN_rand.pod

@@ -37,7 +43,13 @@ If B<bits> is 1 then B<top> cannot also be B<BN_RAND_FLG_TOPTWO>.
 BN_rand_range() generates a cryptographically strong pseudo-random
 number B<rnd> in the range 0 E<lt>= B<rnd> E<lt> B<range>.

-The PRNG must be seeded prior to calling BN_rand() or BN_rand_range().
+BN_priv_rand() and BN_priv_rand_range() have, respectively, the same


Maybe move ",respectively" to the end? (that saves a comma.)

@mspncp I'll fix it right away, thanks for the feedback!

@mspncp done!

romen · 2018-06-18T19:17:53Z

@richsalz about make doc-nits, if I run it I don't get any output, I also tried to directly run util/find-doc-nits but my Perl-foo is lower than 0, so I couldn't really figure out which combination of boolean flags I should use to get useful and relevant output!

Could you please elaborate a bit on how to use it so that I can be more effective in improving the documentation in the future?

mspncp · 2018-06-18T19:21:53Z

I think, Rich's remark about make doc-nits was just a general hint. If you call it manually, you can save a CI roundtrip, because the CI's will run it anyway. If make doc-nits is silent, everything is fine.

richsalz · 2018-06-18T19:23:58Z

I will not merge, and leave it to you @mspncp; thanks. @romen thanks for the update, and Matthias was right about 'make doc-nits' It's all good!

kroeckx · 2018-06-18T19:57:44Z

You should probably add a HISTORY section saying that the priv functions were added in 1.1.1

kaduk · 2018-06-18T19:59:58Z

doc/man3/RAND_bytes.pod

+be used for generating values that should remain private. If using the
+default RAND_METHOD, this function uses a separate instance of the PRNG
+so that a compromise of the global generator will not affect the secrecy
+of such values.


Is it really the "global generator" anymore? Perhaps "a compromise of the PRNG used for public-facing values will not affect the secrecy of values from the private generator"?

@kaduk fixed in 7bd4945
@kroeckx does it sound good to you?

How about the following formulation?

... this function uses a separate "private" PRNG instance so that a compromise of the "public" PRNG instance will not affect the secrecy of these private values.

@mspncp sounds good to me, but I completely lack knowledge and understanding about the DRBG craziness ( 😃 ) to evaluate how accurately this reflects the default RAND_METHOD internals!

So I'll defer to a Battle Royale among you wise PRNG experts to determine what the final formulation should be!! 😆

See man RAND_DRBG(7) for a nice ascii art picture :-).

The image looks a bit distorted in my browser, but not on the console.

That's very nice!! I should check Section 7 more often, there is a lot of cool stuff there!

@kroeckx

I will squash this commit once we reach consensus - Update sentence on PRNG seeding requirement leftover from 1.0.2 @kroeckx, openssl#6514 (comment) openssl#6514 (comment) - Add history entry about BN_priv_rand and BN_priv_rand_range @kroeckx, openssl#6514 (comment) - Rephrase "global generator" in RAND_priv_bytes() doc @kaduk, openssl#6514 (comment) - Add history entry about RAND_priv_bytes() @kroeckx, openssl#6514 (comment)

romen · 2018-06-18T20:44:25Z

Once @kaduk and @kroeckx greenlight this, if none has further feedback or objections, I'll squash the two commits to make it ready for merge.

I will squash this and previous commits into a single one once we reach consensus.

romen · 2018-06-18T21:02:52Z

@richsalz the 1.1.0 label is misplaced

mspncp · 2018-06-19T04:06:53Z

Does not apply to ~~1.1.1~~ 1.1.0 as @romen already remarked.

mspncp · 2018-06-19T05:29:45Z

I'll squash the two commits to make it ready for merge.

You don't need to do the squashing, I can do it in the final merge.

For larger pull requests, it is in fact the preferred way that squashing is done by the reviewer who merges, unless there are conflicts to be resolved or you want to reword your commit message. We try to avoid force-pushing after approval, since formally, after squashing and force-pushing, the entire change set needs to be revisited. For a small pull request like yours it would be acceptable if you prefer to squash it yourself, but it is not necessary to do it.

mspncp · 2018-06-19T07:36:14Z

How about the following formulation?

Note: when I suggested the alternative formulation I wasn't aware that you had already addressed @kaduk's comment. So your pr is fine for me the way it is. (Sorry, no battle royale ;-). ) I will give @kaduk a few more hours to respond and will merge after that with the current three approvals if nobody shows up.

romen · 2018-06-19T08:12:21Z

In 509cdce , I updated the wording according to @mspncp suggestion in #6514 (comment), reformatted the history sections to use lists, and edited RAND.7 as it had a similar section about "long-term secrets".

Finally I sprinkled links to RAND.7 and RAND_DRBG.7 in RAND_bytes.3 and BN_rand.3: those section 7 manpages are so useful and beautiful they deserve to be as reachable as possible!!

romen · 2018-06-19T08:14:34Z

@mspncp

Note: when I suggested the alternative formulation I wasn't aware that you had already addressed @kaduk's comment. So your pr is fine for me the way it is.

So should I revert the rephrasing in 509cdce ?

romen

Conflict between RAND.7 and RAND_bytes.3

romen · 2018-06-19T08:21:55Z

doc/man3/BN_rand.pod

+
+An error occurs in the underlying B<RAND> functions if the CSPRNG has
+not been seeded with enough randomness to ensure an unpredictable byte
+sequence.


I mirrored the note contained in RAND_bytes.3 for

An error occurs in the underlying B functions if the CSPRNG has not been seeded with enough randomness to ensure an unpredictable byte sequence.

Reading RAND.7, to me it seems that this, and the original note, are in conflict to the following statement:

The default random generator will initialize automatically on first use and will be fully functional without having to be initialized ('seeded') explicitly. It seeds and reseeds itself automatically using trusted random sources provided by the operating system.

Should I remove the above note?

What about the one in RAND_bytes.3?

Okay, silly me, rereading it they are not in conflict. It's just remarking about the fact that whenever randomness is requested through one of those functions, the developer should always check for errors in case there wasn't enough randomness.

I would rephrase this note and the one in RAND_bytes.3 just to make the message about checking the return value more explicit for this reason!

Is this going to become something bigger? Then you might want to consider one of the following two options:

Add "WIP:" to the beginning of the pull request title so we see it is work-in-progress and will make useful comments but not attempt a formal review. (As long as it is WIP, you are free to squash and reorganize as you like)

Keep this pull request limited to RAND_priv_bytes() only and create a new WIP pull request (which can even be based on this branch initially) for all the new ideas you came up with.

I guess it's pretty much done now, when I opened it it was limited only to BN_priv_rand() and RAND_priv_bytes(), it was only after the feedback on RAND(7) and RAND_DRBG(7) that I noticed that also RAND(7) needed updating, because it had the same notice note on "long-term secrets" rather than "any private value".

Commit d246d50 too is just an improvement on some feedback I received in this PR, about including a note about errors when the CSPRNG does not have enough entropy: upon reading RAND(7) I initially misinterpreted the intention of the note when compared with the note in RAND(7) about automatic initialization and reseeding: when I (5 minutes later) realized that the note on RAND_bytes(3) was to remind API users to always check for the error return value, I made that more explicit so that other silly users like me don't fall in the same trap!

mspncp · 2018-06-19T09:15:22Z

So should I revert the rephrasing in 509cdce ?

No no, I just wanted to say that there is no need to address my commit. You pushed after my comment, did you? Or did I overlook your commit?

I will have a look at your newest changes.

romen · 2018-06-19T09:50:20Z

No no, I just wanted to say that there is no need to address my commit. You pushed after my comment, did you? Or did I overlook your commit?

Yep, you commented before I pushed, but I was unaware of your newest comment when I pushed.

mspncp

LGTM

mspncp · 2018-06-19T10:09:03Z

doc/man7/RAND.pod

@@ -64,7 +67,7 @@ L<RAND_priv_bytes(3)>,
 L<RAND_get_rand_method(3)>
 L<RAND_set_rand_method(3)>


While you're at it, would you mind adding two missing commas here?

mspncp · 2018-06-19T11:38:46Z

I will leave the pr open for a few more hours to give @richsalz and @kroeckx the opportunity to reapprove (or remain silent) before merging late this evening or tomorrow morning.

kaduk

I'm fine with the reworded formulation, thanks!
(There is also no need to address the indicated nits inline before merging.)

kaduk · 2018-06-19T14:09:35Z

doc/man3/BN_rand.pod

+
+=head1 NOTES
+
+Always check the error return value of these functions and don't take


Some man page style guidelines say to avoid contractions, though I forget if that's something we've actually adopted for OpenSSL.

kaduk · 2018-06-19T14:11:07Z

doc/man3/RAND_bytes.pod

+=head1 NOTES
+
+Always check the error return value of RAND_bytes() and
+RAND_priv_bytes() and don't take randomness for granted: an error occurs


(same here)

kaduk · 2018-06-19T14:12:35Z

doc/man7/RAND.pod

-in order to reveal as little information as possible about its internal state.
+The intention behind using a dedicated CSPRNG exclusively for private
+values is that none of its output should be visible to an attacker (e.g
+used as salt value), in order to reveal as little information as


Some grammar pedants will insist upon offsetting things like "i.e." and "e.g." with commas (or parentheses) both before and after.

"e.g." was missing the trailing point anyway, so I went ahead and also applied the "avoid contractions" suggestion.

Thanks @kaduk for spotting it!

I noticed that the e.g. in was missing the trailing point anyway, so I went ahead and also committed the "avoid contractions" suggestion.

richsalz · 2018-06-19T14:57:32Z

Reconfirm, this is ready to merge.

mspncp · 2018-06-19T15:03:14Z

Well then, let's go ;-)

Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Kurt Roeckx <kurt@roeckx.be> Reviewed-by: Ben Kaduk <kaduk@mit.edu> Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com> (Merged from #6514)

mspncp · 2018-06-19T15:10:47Z

Merge, thank you very much!

(Travis failure was unrelated)

richsalz · 2018-06-19T15:12:59Z

@romen, @mspncp, etc., thank you all very much for working through this! I think this had one of the highest ratio's of discussion/line of any PR. :)

romen · 2018-06-19T15:17:55Z

@richsalz are you complaining I talk too much???? 😛

Anyway, thank you all, I learned a lot by working on this PR and especially from all your feedback!

mspncp · 2018-06-19T15:19:05Z

No, he is teasing me that I'm being too pedantic ;-)

richsalz · 2018-06-19T15:33:47Z

No, I'm thanking everyone for sticking with this, even though it became much bigger then I think we all first thought it would be.

romen mentioned this pull request Jun 18, 2018

Coordinate blinding for EC_POINT #6501

Closed

romen commented Jun 18, 2018

View reviewed changes

richsalz approved these changes Jun 18, 2018

View reviewed changes

richsalz self-assigned this Jun 18, 2018

richsalz added branch: master Merge to master branch 1.1.0 approval: review pending This pull request needs review by a committer labels Jun 18, 2018

mspncp approved these changes Jun 18, 2018

View reviewed changes

mspncp added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jun 18, 2018

Fix & update documentation about RAND_priv_bytes()

ed10e19

romen force-pushed the priv_rand_docfix branch from cab0fb7 to ed10e19 Compare June 18, 2018 18:43

mspncp removed the approval: done This pull request has the required number of approvals label Jun 18, 2018

mspncp assigned mspncp and unassigned richsalz Jun 18, 2018

kaduk reviewed Jun 18, 2018

View reviewed changes

mspncp mentioned this pull request Jun 18, 2018

Improve the output of make doc-nits #6517

Closed

[will-squash] Update Copyright year in doc/man3/BN-rand.pod

4fdeba6

I will squash this and previous commits into a single one once we reach consensus.

kroeckx approved these changes Jun 18, 2018

View reviewed changes

mspncp added branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch and removed 1.1.0 labels Jun 19, 2018

[fixup] rephrase, restyle, edit RAND.7

509cdce

romen commented Jun 19, 2018

View reviewed changes

romen added 2 commits June 19, 2018 11:36

[fixup] make comments about error checking more explicit

e5cde2e

[fixup] fix links to external manpages

d246d50

mspncp approved these changes Jun 19, 2018

View reviewed changes

[fixup] add missing commas

f443e19

kaduk approved these changes Jun 19, 2018

View reviewed changes

[fixup] address minor grammar nits spotted by @kaduk

f26704c

I noticed that the e.g. in was missing the trailing point anyway, so I went ahead and also committed the "avoid contractions" suggestion.

richsalz approved these changes Jun 19, 2018

View reviewed changes

richsalz added the approval: done This pull request has the required number of approvals label Jun 19, 2018

mspncp closed this Jun 19, 2018

		@@ -64,7 +67,7 @@ L<RAND_priv_bytes(3)>,
		L<RAND_get_rand_method(3)>
		L<RAND_set_rand_method(3)>


		=head1 NOTES

		Always check the error return value of these functions and don't take

Fix & update documentation about RAND_priv_bytes() #6514

Fix & update documentation about RAND_priv_bytes() #6514

Conversation

romen commented Jun 18, 2018

kroeckx commented Jun 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richsalz left a comment

Choose a reason for hiding this comment

mspncp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romen commented Jun 18, 2018

mspncp commented Jun 18, 2018

richsalz commented Jun 18, 2018

kroeckx commented Jun 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romen commented Jun 18, 2018

romen commented Jun 18, 2018

mspncp commented Jun 19, 2018 • edited

mspncp commented Jun 19, 2018

mspncp commented Jun 19, 2018

romen commented Jun 19, 2018

romen commented Jun 19, 2018

romen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mspncp commented Jun 19, 2018

romen commented Jun 19, 2018

mspncp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mspncp commented Jun 19, 2018

kaduk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richsalz commented Jun 19, 2018

mspncp commented Jun 19, 2018

mspncp commented Jun 19, 2018

richsalz commented Jun 19, 2018

romen commented Jun 19, 2018

mspncp commented Jun 19, 2018

richsalz commented Jun 19, 2018

mspncp commented Jun 19, 2018 •

edited