Made all changes suggested. There is one failing test, ./test/recipes/80-test_ssl_old.t, but I haven't been able to figure out how to run it standalone to see where its failing:

../test/recipes/80-test_ssl_old.t                (Wstat: 256 Tests: 6 Failed: 1)
  Failed test:  3
  Non-zero exit status: 1

How do I run the .t tests?

Try make TESTS=test_ssl_old V=1 test to get verbose output

Is this enough code that I need to sign a CLA? If it is, I need to get our lawyer's OK :-(. They will OK it, I just need to get the process moving. If I don't need a CLA, I won't bother them.

Thanks for #7759 (comment), I fixed the tests, PTAL.

I'm not on the OMC, but this was tagged with need-cla, so I think you need to go through the process...

All non-trivial contributions need a CCLA and ICLA on file (in general) so getting that sorted out is a prerequisite for progressing.

This contribution is non-trivial.

Kicked CLA process into gear. Looks like it will be rubber stamped.

CLA signed and emailed.

All green! @mattcaswell I made all your requested changes.

Placing this temporarily on hold while we sort out a CLA issue.

Removed the "hold" - the CLA issues are now resolved. @sam-github - please could you update your commits to show your ibm email as the author and then force push?

OK, renamed the counting function, changed to rsam@ email address. Thanks for your patience with the CLA process.

Another thought I had on this: with TLSv1.3 it may be intentional to set the TLSv1.2 ciphersuites to be empty (e.g. if you never plan to negotiation TLSv1.2). With the implementation as it currently is that's (sort of) fine - you can do that, but you do still get an error return - even though what you were trying to do "worked" (the TLSv1.2 ciphersuites are set to the empty list). It's kind of weird that this function still changes the cipher list to empty even while signalling an error - normally if a function returns an error you expect no change to have been made (but that's the way it has always worked I guess). Perhaps there should be some discussion of this in the docs?

This also introduces a problem with the "ciphers" command line application. Say you only want to see the TLSv1.3 ciphersuites. So you might think the following would work (and it did in 1.1.1/1.1.1a) - but it doesn't after this PR.

Before the PR:

$ openssl ciphers -v "empty"
TLS_AES_256_GCM_SHA384  TLSv1.3 Kx=any      Au=any  Enc=AESGCM(256) Mac=AEAD
TLS_CHACHA20_POLY1305_SHA256 TLSv1.3 Kx=any      Au=any  Enc=CHACHA20/POLY1305(256) Mac=AEAD
TLS_AES_128_GCM_SHA256  TLSv1.3 Kx=any      Au=any  Enc=AESGCM(128) Mac=AEAD

After the PR:

$ openssl ciphers -v "empty"
Error in cipher list
140661759513024:error:1410D0B9:SSL routines:SSL_CTX_set_cipher_list:no cipher match:ssl/ssl_lib.c:2572:

I think there are two seperate issues here:

1. https://www.openssl.org/docs/faq.html#MISC8 --- 1.1.1 should be ABI compatible with 1.1.0, but should it also be API compatible? That is, should it behave the same? My reading is "yes", so this change fixes a bug.
2. Does the 1.1.1 API work well. I agree with @mattcaswell 's points. Both failing, AND having a side-effect is undesireable. Also, the current behaviours are a bit weird.

Some thoughts, no particular order.

openssl ciphers -v none is undocumented: Usage: ciphers [options]

openssl ciphers -tls1_3 -v -s is a bit more obvious than openssl ciphers -v some-invalid-string

I don't understand why -tls1_3 in the absence of -s does nothing, I would have expected -tls1_3 to do what only -tls1_3 -s did. In fairness, behaviour is documented in the full man page, but not the usage.

clearing cipher_list or ciphersuites should not be an error, but there is no specific api to do either now, unless you count set_cipher_list("") and set_ciphersuites("")

It surprised me that set_ciphersuites(":") is an error, and this behaviour has some odd effects in the openssl ciphers command:

~/s % openssl ciphers -ciphersuites ":"
Error setting TLSv1.3 ciphersuites

I'd expect that to empty the ciphersuites. " An empty list is permissible." (docs) doesn't really say what an empty list looks like.

openssl ciphers -ciphersuites "" works, because set_ciphersuites() has special handling for a zero-length string, "" is an "empty list".

~/s % openssl ciphers ":"
TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_SHA256:TLS_AES_128_GCM_SHA256
~/s % openssl ciphers -ciphersuites "" ":"
Error in cipher list

Current behaviour. Kindof odd that ":" is not a cipher list error in the first case, but is in the second.

This PR makes a cipher list of ":" always an error, as it was in 1.1.0, but I think it would be better to always be NOT an error (in openssl 1.2). I'd also expect a ciphersuites argument of ":" to NOT be an error (equivalent to "" now).

Stepping back, I'm not sure why set_ciphersuites() was created for TLSv1.3 only. The docs state it as fact, but don't give rationale.

Is it because set_cipher_list has a complex legacy string format, and you didn't want to change anything about its behaviour? That would make sense, but I don't understand why set_ciphersuites can't be used with TLSv1.2 and below ciphers -- of course, not with magic characters, but only containing full, exact, cipher suite names, in preference order, as it does for TLSv1.3.

Equivalently, I'm not sure why set_cipher_list() can't be used to set TLSv1.3 ciphers. Is it because you want people to stop using it, but didn't want to break API compatibility, so you just added the new/TLSv1.3 cipher suites to the new API, as a gentle nudge?

OK, I found #5359

Summary: it was desired that TLSv1.3 suites not be disabled except very intentionally.

I think the splitting was incomplete, since they are all going in the same list, set_cipher_list() is counting TLSv1.3 suites. I suggest openssl 1.2 should make set_cipher_list and set_ciphersuites behave more similarly:

• set_ciphersuites should accept ":" and " : " as same meaning as ""
• set_cipher_list should clear all TLSv1.2 ciphersuites and NOT error for a list of "" and " : " and ":" (same set of "empty list" strings as supported by set_ciphersuites)
• set_cipher_list should error if given a non-empty list argument that results in no TLSv1.2 cipher suites, and should not modify the configured suites when it errors

set_ciphersuites should accept ":" and " : " as same meaning as ""
set_cipher_list should clear all TLSv1.2 ciphersuites and NOT error for a list of "" and " : " and ":" (same set of "empty list" strings as supported by set_ciphersuites)
set_cipher_list should error if given a non-empty list argument that results in no TLSv1.2 cipher suites, and should not modify the configured suites when it errors

I'm wondering if there is any reason why we can't make these three changes now (in master and 1.1.1)? The first one definitely shouldn't break anything. The second I suppose could in theory...but it seems unlikely. I'm also struggling to think of a usage scenario that the third change would break.

I'd be in favour of just fixing this issue as noted - i.e. correctly "empty" string handling inconsistencies.

I'd be in favour of just fixing this issue as noted - i.e. correctly "empty" string handling inconsistencies.

I'm not really sure what you mean by that. There are a number of inconsistencies here around empty string handling. This PR fixes one, but introduces more around the output of the "ciphers" command.

I was responding to "I'm wondering if there is any reason why we can't make these three changes now (in master and 1.1.1)?"

Introducing error return with side-effect would be very ugly. I do not think this PR makes much sense without actually doing all the three mentioned changes.

@t8m did someone propose introducing error-return-with-side-effect? I think everything @mattcaswell or I suggested does the opposite.

@mattcaswell Backwards compatibility in the face of change is hard to gauge, you know your user base, but for example I was testing yesterday and found #2801 broke https://github.com/mkg20001/ursa/blob/0999dcf62cced5102913f925cb4fcfabdd480dfb/test/native.js#L688. I would call this unnecessary reliance on OpenSSL internals, but someone else might call it an API incompatibility.

I'm also struggling to think of a usage scenario that the third change would break.

Your example!

openssl ciphers -v "empty"

See also issue #7196, which was the previous round of folks being affected by this. I agree with @sam-github that this is the more compatible change. In OpenSSL 1.1.0, callers could check the result of SSL_set_cipherlist to see if there were errors in their TLS 1.2 cipher configuration. Implicitly adding TLS 1.3 ciphers does not change whether or not there were errors.

These sorts of things do require judgement calls, but, with the benefit of hindsight and now several known affected projects, I think it's clear this PR is the more compatible decision.

As another data point, this PR is closer to the strategy BoringSSL took. We preserve the fact that SSL_set_cipherlist("bogus") reports failure. (We actually go further and treat 1.2 and 1.3 ciphers as totally orthogonal, so SSL_get_ciphers is also unchanged.) We did not have to update a single caller to support this strategy.

(Specifically #7196 (comment), which suggested the behavior as this PR, though with a different implementation.)

@sam-github what is the status of this PR?

@mattcaswell I'm not sure, the conversation seemed to suggest that instead of this, much more wide spread changes were wanted.

If that's not the case, and its agreed that this is on its way to landing, I'll keep working on the tests.

Aside: I worked around the backwards incompatibility in node.js by always calling set_ciphersuites:

let ciphers = (options.ciphers || tls.DEFAULT_CIPHERS).split(':');
let cipherList = ciphers.filter(_ => !_.match(/^TLS_/)).join(':');
let cipherSuites = ciphers.filter(_ => _.match(/^TLS_/)).join(':');

// Set cipher suites first, so that setCiphers() will error if there are
// neither TLSv1.2 nor TLSv1.3 cipher suites enabled.
c.context.setCipherSuites(cipherSuites);
c.context.setCiphers(cipherList);

if (cipherSuites === '' && c.context.getMaxProto() > TLS1_2_VERSION
    && c.context.getMinProto() < TLS1_3_VERSION)
  c.context.setMaxProto(TLS1_2_VERSION);

if (cipherList === '' && c.context.getMinProto() < TLS1_3_VERSION
    && c.context.getMaxProto() > TLS1_2_VERSION)
  c.context.setMinProto(TLS1_3_VERSION);

IMO this change should proceed.

I'll address your comments tomorrow @mattcaswell

@mattcaswell please take a look

Pushed. Thanks.