Reflect special `DEFAULT` behavior in ciphers(1) by AloisMahdal · Pull Request #5428 · openssl/openssl

AloisMahdal · 2018-02-21T15:56:38Z

Actual behavior of DEFAULT is different than currently described.
Rather than actinf as cipher string, DEFAULT cannot be combined using
logical operators, etc.

Fixes #5420.

Checklist

documentation is added or updated

richsalz · 2018-02-21T15:58:29Z

@AloisMahdal we need an individual CLA from you (we have a global corporate RedHat one). Thanks.

AloisMahdal · 2018-02-21T16:31:23Z

(iCLA sent.)

Note that this PR does not address directly the fact that DEFAULTz is equivalent to DEFAULT:z, but it does rule out the assumption that it's treated the same as other "cipher lists". In that sense, it's a bug freeze-in (as @vdukhovni points in #5421).

richsalz · 2018-02-21T17:50:11Z

See #5429

richsalz · 2018-02-21T17:54:17Z

open/close to kick the CLA-bot.

richsalz · 2018-02-21T20:37:05Z

How about this minor wording changes:
The cipher list can be prefixed with B keyword, which enables the default
cipher list as defined below. [[delete, it appears below]Content of this list is determined at compile time and
normally corresponds to BALL:!COMPLEMENTOFDEFAULT:!eNULL.]] Unlike cipher
strings, this prefix may not be combined with other strings using B<+> or
B<-> character. For example, B<DEFAULT+DES> is not a valid cipher list.

vdukhovni · 2018-02-22T00:55:31Z

Pretty close, but <-> is not a "combining" character, it is only special as the first character of a cipherlist element (-RC4) but clearly not in the middle (RC4-SHA).

AloisMahdal · 2018-02-22T10:18:47Z

Updated:

moved "default" explanation to next paragraph,
added "the",
removed mention of - as combining character,
rebased on master.

richsalz

two minor changes. thanks for doing this. i will close mine.

richsalz · 2018-02-22T18:21:05Z

doc/man1/ciphers.pod

"The cipher list"

richsalz · 2018-02-22T18:21:20Z

doc/man1/ciphers.pod

Unlike the cipher string ...

AloisMahdal · 2018-02-23T13:26:26Z

Thanks for feedback. (Need to read up something on usage of "the".) Updated: addded the's, re-wrapped and rebased on master.

vdukhovni · 2018-02-23T13:46:52Z

doc/man1/ciphers.pod

s/B<DEFAULT>/the B<DEFAULT>/

Fixed, thanks.

vdukhovni · 2018-02-23T13:48:28Z

doc/man1/ciphers.pod

s/Unlike the cipher string/Unlike other cipher strings/

Wouldn't that imply DEFAULT is a cipher string? Because it's not... (at least that's what I'm trying to say)

vdukhovni · 2018-02-23T13:49:10Z

doc/man1/ciphers.pod

s/valid cipher list/valid cipher string/

Mind you, the real story is more subtle (back to the BUGS issue), since DEFAULT+FOO is surprisingly instead "DEFAULT:+FOO", which would put FOO last (for any value of FOO). So the users who specify "DEFAULT+FOO" do not get anything like what they probably intended. If that's described, then it goes into a BUGS section, and must not be described as a stable interface.

Fixed the the. See below for comment on BUGS.

vdukhovni · 2018-02-23T13:57:50Z

doc/man1/ciphers.pod

s/Content/The content/

Fixed, thanks.

Actual behavior of DEFAULT is different than currently described. Rather than actinf as cipher string, DEFAULT cannot be combined using logical operators, etc. Fixes #5420.

AloisMahdal · 2018-02-23T19:56:44Z

Fixed most issues (i was not sure about the the vs. other.

Anyway, I agree with @vdukhovni that it's not a good idea to describe this as stable interface. I opened (yet) another PR #5455 where I described it as part of BUGS section (I added it following convention described in man-pages(7).)

vdukhovni · 2018-02-23T20:57:51Z

doc/man1/ciphers.pod

 See L<SSL_CTX_set_security_level> for a description of what each level means.

+The cipher list can be prefixed with the B<DEFAULT> keyword, which enables
+the default cipher list as defined below.  Unlike the cipher string,


The text here really does need to change. The simplest change is from 'Unlike the' to 'Unlike other'. If you want to provide more detail, you could explain that all other cipher strings are "+" separated conjunctions (AND) of basic cipher strings, which represent a single cipher or class of ciphers sharing a common elementary property. The "DEFAULT" keywork is not such a cipher string, it is instead a built-in cipher list definition that can be a starting point for further refinement, but is not combinable with "+", and must come first.

Re s/Unlike the/Unlike other/, the way things are expressed here, DEFAULT isn't a cipher string, but rather a prefix. Seen that way, 'other' would be wrong, wouldn't it? Although, I might want to see that changed to 'Unlike cipher strings, ...'

richsalz · 2018-03-07T20:28:38Z

@AloisMahdal , @vdukhovni any update?

vdukhovni · 2018-03-07T21:35:19Z

Just English grammar nits in the docs.

AloisMahdal · 2018-03-08T11:59:55Z

Actually, based on @vdukhovni's feedback I've opened also #5455, which takes another approach to solve this #5420 issue by describing the behavior in BUGS section.

My hope is that the community will choose which approach is better and merge one PR and close the other.

levitte · 2018-03-29T10:19:31Z

It seems that this is preferred to #5455. Please see @vdukhovni's commentary there to do some adjustments here.

levitte · 2018-04-19T12:48:44Z

This PR seems to have gone stale. I could see this being approved with the following editorial fixup:

diff --git a/doc/man1/ciphers.pod b/doc/man1/ciphers.pod
index 6da316db89..751518d1c2 100644
--- a/doc/man1/ciphers.pod
+++ b/doc/man1/ciphers.pod
@@ -157,9 +157,9 @@ level to B<n>, which should be a number between zero and five, inclusive.
 See L<SSL_CTX_set_security_level> for a description of what each level means.
 
 The cipher list can be prefixed with the B<DEFAULT> keyword, which enables
-the default cipher list as defined below.  Unlike the cipher string,
+the default cipher list as defined below.  Unlike cipher strings,
 this prefix may not be combined with other strings using B<+> character.
-For example, B<DEFAULT+DES> is not a valid cipher string.
+For example, B<DEFAULT+DES> is not valid.
 
 The content of the default list is determined at compile time and normally
 corresponds to B<ALL:!COMPLEMENTOFDEFAULT:!eNULL>.

What say @openssl?

richsalz · 2018-04-19T13:07:41Z

Yes, please, merge this with your change. +1

richsalz

Merge and do your fix.

levitte · 2018-04-19T13:37:04Z

Merged into master with mentioned editorial changes.

c190506 Reflect special DEFAULT behavior in ciphers(1)

Actual behavior of DEFAULT is different than currently described. Rather than actinf as cipher string, DEFAULT cannot be combined using logical operators, etc. Fixes #5420. Reviewed-by: Rich Salz <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from #5428)

openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Feb 21, 2018

richsalz mentioned this pull request Feb 21, 2018

Try to explain DEFAULT in ciphers #5429

Closed

richsalz closed this Feb 21, 2018

richsalz reopened this Feb 21, 2018

openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Feb 21, 2018

richsalz suggested changes Feb 22, 2018

View reviewed changes

vdukhovni reviewed Feb 23, 2018

View reviewed changes

Reflect special DEFAULT behavior in ciphers(1)

bdb9661

Actual behavior of DEFAULT is different than currently described. Rather than actinf as cipher string, DEFAULT cannot be combined using logical operators, etc. Fixes #5420.

AloisMahdal mentioned this pull request Feb 23, 2018

Describe bug in DEFAULT "cipher string" behavior #5455

Closed

1 task

vdukhovni reviewed Feb 23, 2018

View reviewed changes

mattcaswell added this to the 1.1.1 milestone Mar 20, 2018

richsalz approved these changes Apr 19, 2018

View reviewed changes

levitte closed this Apr 19, 2018

AloisMahdal deleted the default_as_pfx branch April 20, 2018 11:48

Uh oh!

Comments

Conversation

AloisMahdal commented Feb 21, 2018

Checklist

Uh oh!

richsalz commented Feb 21, 2018

Uh oh!

AloisMahdal commented Feb 21, 2018

Uh oh!

richsalz commented Feb 21, 2018

Uh oh!

richsalz commented Feb 21, 2018

Uh oh!

richsalz commented Feb 21, 2018

Uh oh!

vdukhovni commented Feb 22, 2018

Uh oh!

AloisMahdal commented Feb 22, 2018

Uh oh!

richsalz 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!

AloisMahdal commented Feb 23, 2018

Uh oh!

vdukhovni Feb 23, 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!

Choose a reason for hiding this comment

Uh oh!

AloisMahdal commented Feb 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richsalz commented Mar 7, 2018

Uh oh!

vdukhovni commented Mar 7, 2018

Uh oh!

AloisMahdal commented Mar 8, 2018

Uh oh!

levitte commented Mar 29, 2018

Uh oh!

levitte commented Apr 19, 2018

Uh oh!

richsalz commented Apr 19, 2018

Uh oh!

richsalz left a comment

Choose a reason for hiding this comment

Uh oh!

levitte commented Apr 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

vdukhovni Feb 23, 2018 •

edited

Loading

AloisMahdal commented Feb 23, 2018 •

edited

Loading