Modify is_tls13_capable() to take account of the servername cb#13304
Modify is_tls13_capable() to take account of the servername cb#13304mattcaswell wants to merge 2 commits intoopenssl:masterfrom
Conversation
A servername cb may change the available certificates, so if we have one set then we cannot rely on the configured certificates to determine if we are capable of negotiating TLSv1.3 or not. Fixes openssl#13291
If an SNI callback has been set then we may have no certificuates suitable for TLSv1.3 use configured for the current SSL_CTX. This should not prevent us from negotiating TLSv1.3, since we may change the SSL_CTX by the time we need a suitable certificate.
|
1.1.1 backport is in #13305. |
t8m
left a comment
t8m
left a comment
There was a problem hiding this comment.
Approved provided the comment nit is fixed.
| * certificate type, or has PSK or a certificate callback configured. Otherwise | ||
| * returns 0. | ||
| * certificate type, or has PSK or a certificate callback configured, or has | ||
| * a servername callback configure. Otherwise returns 0. |
t8m
added
the
approval: done
|
I would prefer if this change was not necessary, given that it is adding yet more complexity to the already intertwined dependencies of handshake processing. That said, I cannot fully object, since we already have PSK callback checks, but it seems like it is going in the wrong direction. The current behavior of the servername_cb is constrained by many consumers and use cases that evolved organically to depend on particular implementation choices, and are sensitive to any reordering of handshake processing. I believe that the client_hello_cb is the appropriate place for application code that needs to affect TLS version negotiation to reside, since that callback has clear and well-defined semantics with respect to the rest of handshake processing. |
Please take a look at the comments in #13291. The nginx developers believe that the client_hello_cb does not meet their needs (apparently they tried it and abandoned it). Whilst we might prefer users to use the client_hello_cb we have not chosen to deprecate the servername_cb and this PR addresses an issue that looks like a clear bug which should be fixed IMO. |
|
24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually. |
kroeckx
added
hold: discussion
|
I've put a hold flag on it, until there is sufficient time to see that this is the way we want to do it. |
Yes, I already commented there and it looks like there was a followup to help clarify the concerns and whether new APIs can address some or all of them.
Well, it addresses an issue and introduces another, so there's a tradeoff. We in general try to provide protection against failed negotiations, in this case by not considering TLS versions that are incompatible with the configured server certificate. This is problematic when the server certificate is going to change and the new certificate would be compatible with other TLS versions, which motivates relaxing the check, but if we relax the check then someone who is going to change the certificate to one that is not compatible with TLS 1.3 will experience a handshake failure, as we negotiate 1.3 and then discover that the certificate is incompatible. So it's a tradeoff. I note that I chose my words carefully in "prefer if this change was not necessary" (implying that it may well be necessary), since our precedent for similar cases is to relax the check and leave the application code with the responsibility for performing the analogous check. |
I consider it necessary. |
|
Would alternative fix that does not return 0 from is_tls13_capable() in case no certificate is set at all would be acceptable? The reason is that running anonymous server is highly improbable anyway. Or return 0 or 1 in this case based on whether the servername_cb is set or not. |
kroeckx
removed
the
hold: discussion
|
So what are we going to do with this PR? |
It was waiting for OTC to decide what we were going to do - so not sure why you removed the OTC hold. |
|
I added the OTC hold label because it was approved, but I think it
needed to have some time for to discuss it. If you think it needs
an actual OTC decission, can someone at least start a discussion
about it?
|
mattcaswell
added
the
approval: done
Well, I'm happy with it as it is. I thought others wanted that discussion. If not then I'm happy to proceed with it as is. |
|
24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually. |
A servername cb may change the available certificates, so if we have one set then we cannot rely on the configured certificates to determine if we are capable of negotiating TLSv1.3 or not. Fixes #13291 Reviewed-by: Tomas Mraz <[email protected]> (Merged from #13304)
If an SNI callback has been set then we may have no certificuates suitable for TLSv1.3 use configured for the current SSL_CTX. This should not prevent us from negotiating TLSv1.3, since we may change the SSL_CTX by the time we need a suitable certificate. Reviewed-by: Tomas Mraz <[email protected]> (Merged from #13304)
No one objected or added the OTC hold label, so I have pushed to master. The 1.1.1 version still seems to have the hold. |
5 participants
A servername cb may change the available certificates, so if we have one
set then we cannot rely on the configured certificates to determine if we
are capable of negotiating TLSv1.3 or not.
Fixes #13291