Skip to content

Comments

Try to explain DEFAULT in ciphers#5429

Closed
richsalz wants to merge 1 commit intoopenssl:masterfrom
richsalz:gh5420
Closed

Try to explain DEFAULT in ciphers#5429
richsalz wants to merge 1 commit intoopenssl:masterfrom
richsalz:gh5420

Conversation

@richsalz
Copy link
Contributor

Fixes #5420. Alternate version for #5428.

@richsalz richsalz added branch: master Applies to master branch branch: 1.0.2 Applies to OpenSSL_1_0_2-stable branch (EOL) 1.1.0 approval: review pending This pull request needs review by a committer labels Feb 21, 2018
@richsalz richsalz self-assigned this Feb 21, 2018
Copy link
Contributor

Choose a reason for hiding this comment

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

Not just "cannot be prefixed": even DEFAULT+ECDH will not behave as other cipher string would (ie. select what is in DEFAULT and uses ECDH).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is not what + means. Try these:

openssl ciphers -v DEFAULT | tail
openssl ciphers -v DEFAULT+RSA | tail

It does not do an AND. It does a "move to end of the list."

Copy link
Member

Choose a reason for hiding this comment

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

You can do RSA+AES, where + does mean and. DEFAULT+AES does not work because of the bug. On the other hand ALL+RSA:!COMPLEMENTOFDEFAULT:!eNULL works.

Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC from comments by people who have read the code, they are taken as cipher string, not cipher name. ie, DEFAULT-ECDH means what DEFAULT:-ECDH would, although -ECDH is not a cipher name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I guess so. The terminology in this page is poor.

Copy link
Contributor

Choose a reason for hiding this comment

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

s/by/be/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tnx.
Updated commit pushed.

Thanks to Thomas Mraz and Alois Mahdal for helping with this.
@nickthetait
Copy link
Contributor

For this case would providing a warning help or add to the confusion? Something like unknown cipherstring 'z' was ignored

@richsalz
Copy link
Contributor Author

So the docs that say "move to the end, but adds no ciphers" are wrong.

@AloisMahdal
Copy link
Contributor

+ on start of cipher string (move to the end of list) is different than + as operator to join cipher strings (intersection, or and). None of them work with DEFAULT. That, so far, can be understood (with some effort) from the doc. It actually also provides somewhat viable syntax.

However, due to the strange behavior, DEFAULT+RSA will actually be understood as the former, so the latter is not possible. This probably extends to other cases that one could derive from the text.

IMO easiest way to understand is to just realize that DEFAULT just does not behave as "cipher string", but a completely different beast. Trying to cram its behavior into current terminology just brings up exceptions. Hence my other PR.

@richsalz
Copy link
Contributor Author

#5428 is better, closing this.

@richsalz richsalz closed this Feb 22, 2018
@richsalz richsalz deleted the gh5420 branch February 22, 2018 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval: review pending This pull request needs review by a committer 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.

4 participants