Skip to content

Comments

Improve documentation of behaviour of DEFAULT in the cipherstring.#5421

Closed
t8m wants to merge 1 commit intoopenssl:masterfrom
t8m:DEFAULT-doc
Closed

Improve documentation of behaviour of DEFAULT in the cipherstring.#5421
t8m wants to merge 1 commit intoopenssl:masterfrom
t8m:DEFAULT-doc

Conversation

@t8m
Copy link
Member

@t8m t8m commented Feb 20, 2018

Fixes #5420

Checklist
  • documentation is added or updated

@mattcaswell mattcaswell added branch: master Applies to master branch branch: 1.0.2 Applies to OpenSSL_1_0_2-stable branch (EOL) 1.1.0 labels Feb 20, 2018
@mattcaswell mattcaswell added the approval: review pending This pull request needs review by a committer label Feb 20, 2018
Copy link
Contributor

@richsalz richsalz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you merge.

@richsalz richsalz 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 Feb 20, 2018
@AloisMahdal
Copy link
Contributor

AloisMahdal commented Feb 20, 2018

Sorry, but "does not need to be separated from rest of the cipherstring" does not make so much sense given that the subject is a cipherstring (notice difference between cipherlist and cipherstring in man page).

Also the #5420 is about DEFAULT, not COMPLEMENTOFDEFAULT. (Edit: disregard the last sentence.)

@AloisMahdal
Copy link
Contributor

IOW, the behavior (as I observed it--the actual processing in the code might be superset of that) seems to me as completely different syntax of cipherlist.

Copy link

@vdukhovni vdukhovni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this misbehaviour should be documented as an interface contract. Mentioning it in the "BUGS" section would be OK.

@vdukhovni vdukhovni removed the approval: done This pull request has the required number of approvals label Feb 20, 2018
@vdukhovni
Copy link

In addition to opposing the documentation change (to freeze-in a bug), the real documentation lapse with "DEFAULT" is that unlike various more "primitive" cipherlist elements, it does not support additional boolean combinations, e.g. "DEFAULT+kECDHE" or similar. It is a special "macro" that can only occur at the start of the list, and is logically replaced with its definition, and then anything else that follows after the next ":".

We can document that limitation as a non-bug, is that is less likely to change, is a good part of the reason why "DEFAULT" is first. This PR is not progress as it stands IMHO.

@t8m
Copy link
Member Author

t8m commented Feb 21, 2018

Victor, would you be able to propose a different PR with your suggestions?

@t8m t8m closed this Feb 21, 2018
@t8m t8m deleted the DEFAULT-doc branch February 22, 2018 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch: master Applies to master branch branch: 1.0.2 Applies to OpenSSL_1_0_2-stable branch (EOL)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Strange behavior of DEFAULT in cipher list

5 participants