We recently heard from several companies that performance is a concern, and this patch helps address that. Google had a patch that added "equivalence classes" to the ciphers string, but it was kind of incomplete. And the cipher string syntax is already cluttered. So while adding this as an option, only in the code isn't as comprehensive, it's cleaner and can be supported in the conf file.

@richsalz I'm curious what you think is incomplete about the equipreference groups.

This design potentially runs into issues if new ciphers are added in front. I believe the list does skip unknown ciphers, so that clears the immediate extensibility failure, but one could imagine, e.g., SCSV ciphers being in front. It also breaks if, for whatever other reason, none of the ChaCha ciphers are suitable. Maybe all common ChaCha ciphers are for the wrong version. Or maybe they're all ECDHE and you have no curves in common.

(Oh, sorry, scratch the second comment. I'd missed that you do pull in the rest of the ciphers. Though looking at the first cipher is still a little fragile.)

As I recall, you document that the equiv class means you can't sort after it? I forget.

Ah, that's true. Adding an equivalence class disables most opcodes. But it's pretty clear how to make them work. I've been meaning to do it but the need hadn't really come up yet.

Small nit to pick: please drop _FOR_MOBILE / ForMobile... the library has no way of knowing anyway.

(Oh, sorry, scratch the second comment. I'd missed that you do pull in the rest of the ciphers. Though looking at the first cipher is still a little fragile.)

This just reorders the server ciphers. I've seen other patches for similar functionality that discard all other ciphers, and as you point out that won't work. Most (all?) mobile clients indicate their preference for ChaCha by putting it at the beginning of their cipher list, so that's the key we look at.

SCSV is only present in client hellos; it won't matter how the server ciphers are ordered. I am not aware of any client that puts SCSV in the front.

Small nit to pick: please drop _FOR_MOBILE / ForMobile... the library has no way of knowing anyway.

It was to indicate intended use of the option; it was a last minute addition.

The last commit fixes the nits found thus far.

If this could be merged into 1.1.1, then many people don't need to struggle with maintaining a patch...

Note that it needs re-base.

Does the fact that it's two commits mean that this in fact is nominated for 1.1.0? I for one would vote against it in present state, because it's new feature [specifically visible from source code]. Note "in present state" as I for one would consider even implicit prioritization acceptable, i.e. with no enable flag. But that would be surely considered too radical by others :-)

I think it's hard to see how this could be a bugfix, and therefore it's master-only.

@InfoHunter suggested that it might be nice to add to 1.1.0; a comment on the use of sk_reserve indicated that it should be done separately() so as to allow a 1.1.0 backport. And there's PR #3922, which would also be a backport of a feature.

I am not convinced that this is a good long-term interface. It feels somewhat like a quick hack, and we have a few too many of these as a legacy. Perhaps implementing cipher equivalence classes makes more sense, that way the server can order lists of classes that ignore the distinction between AESGCM and CHACHA, or even between AES128 and AES256, and between SHA2-256 and SHA2-384 and chooses the first class in which there is a common cipher. Within that class we can then go with the client preference or the server preference. That is the server preference bit becomes two bits whose class to choose, and whose cipher to choose within that class.

I think adding equivalence classes and further complicating that syntax is also not optimal. This is a relatively clean, although not generic, solution to a big existing problem: Mobile wants chacha and desktop wants AES.

@richsalz I think adding equivalence classes and further complicating that syntax is also not optimal. This is a relatively clean, although not generic, solution to a big existing problem: Mobile wants chacha and desktop wants AES.
The syntax need not be made more complicated, this could be an opportunity to simplify it. We can add an "@v=1" to the front of the cipher string to indicate a brand new syntax that better reflects 20 years of experience. The "not generic" part is precisely the problem here. This is a stop-gap, not an architectural feature. And yet we may be stuck with it for a long time once introduced. We should I think at least discuss alternative designs.

I understand the issue; nobody would talk about it before, when the equivalence classes issue was raised multiple times. I'd rather see site-wide cipher profiles (from RedHat) as a better solution. But nobody is stepping forward to do that -- in fact, attempts to address this, in many ways, have been ignored or rebuffed so far.

Tnis code should go into the next release.

Often doing nothing is better than doing something that lacks a solid design. If this issue needs to be addressed, let's prioritize it and put on the roadmap, and see some design proposals before code is written. This is a major part of the public API, one that many users find confusing, and this is probably as good a time as any to address it. We've managed without a ChaCha optimization for some time, it can wait if need be.

Disagree, and we'll leave it at that.

There is strong demand for this capability ... and pushing it off simply means we end up with a whole range of approaches that get put into production to achieve the same effect - leading to slower updates and more custom patches for something that is relatively straight forward and focused. It isn't generalised - but we also haven't seen strong demand for a generalised solution.

@vdukhovni do you feel strongly enough to -1 this?
I'm ready to approve it - but if you'd prefer to take it to an OMC vote I'm fine with that too.

Tweaked the documentation.
Fixed to use sk_SSL_CIPHER_new_reserve()

@t-j-h @vdukhovni do you feel strongly enough to -1 this? I'm ready to approve it - but if you'd prefer to take it to an OMC vote I'm fine with that too.

I feel strongly that whoever does feel the itch strongly enough to actually address an issue is on the hook for doing it right and not saddling the project with a legacy of short-term work-arounds. So once we have a pull request it should be high-quality design, not just high-quality implementation.

In this case, I don't think an up/down on vote on whether to merge the change would really address my concerns. I'd like to suggest we take a short pause to consider a more ambitious design that makes it easier to optimize the cipher order using a syntax that most mortals can understand and one that avoids out-of-band control bits (like prioritize chacha) and puts all the controls in the cipher string.

Perhaps the author of this PR is willing to propose a more radical overhaul after considering how this is solved in BoringSSL, GnuTLS, TLS toolkits in other languages, ... and proposing something that is realistic and combines some of the best features of the existing approaches.

Given the nature of the request; it is doubtful that it could be completed in any reasonable about of time; and would likely miss the 1.1.1 timeframe. The existing cipher specification is arcane and easily broken; especially where forward and backward compatibility are required.
This option is not incompatible with any possible proposed preference list mechanism that might be implemented in the future.

TL;DR: no.

If no one steps forward to tackle a more comprehensive solution I'll add approval to this tomorrow so @paulidale or @dot-asm can merge.

@tmshort Given the nature of the request; it is doubtful that it could be completed in any reasonable about of time; and would likely miss the 1.1.1 timeframe. The existing cipher specification is arcane and easily broken; especially where forward and backward compatibility are required. This option is not incompatible with any possible proposed preference list mechanism that might be implemented in the future.

This is disappointing, we should not be in such a hurry that we introduce long-term API extensions that are layered on top of weak foundations, rather than revisit the design. Sure someone might do something better "some day", but "some day" rarely comes. The time to ask for better care is when the patient is opened up and the surgeon has the scalpel in hand.

The current design with internal stacks manipulated by a series of push/pop operations that move around groups of ciphers while maintaining the internal order of the groups, and putting removed ciphers back in the order in which they removed is much too arcane. Translating a preference for (kECDHE, kEDH, kRSA) with (aRSA, aECDSA) with (AESGCM, AES, ...) ordered by cipher strength into a cipherlist requires fancy gymnastics.

It should instead be possible to state that all other things being equal one prefers some key exchange algorithms over (or the same as) others, some public key algorithms over (or the same as) others, some bulk ciphers over others, ... and be able to set up equivalences which might go with the client's preference when the server otherwise does not care. It is time to create a new cipherlist string syntax, and the need to address new use-cases is a good time to take that on. Surely we can do better than what we've got, and do so in fact in a reasonable amount of time.

@t-j-h ?

Merged to master.