Tentatively marking this for 1.1.1, since I believe it is a bugfix.

This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago

I'm about to push an update, and will put some commentary here first.

The main diff from the original version of the PR is:

diff --git a/ssl/statem/extensions_srvr.c b/ssl/statem/extensions_srvr.c
index 9cddb35a84..6b3b33e239 100644
--- a/ssl/statem/extensions_srvr.c
+++ b/ssl/statem/extensions_srvr.c
@@ -1616,8 +1616,11 @@ EXT_RETURN tls_construct_stoc_key_share(SSL *s, WPACKET *pkt,
         }
         return EXT_RETURN_NOT_SENT;
     }
-    if ((s->ext.psk_kex_mode & TLSEXT_KEX_MODE_FLAG_KE_DHE) == 0) {
-        /* Explicitly not doing DHE; don't send key share */
+    if (s->hit && (s->ext.psk_kex_mode & TLSEXT_KEX_MODE_FLAG_KE_DHE) == 0) {
+        /*
+         * PSK ('hit') and explicitly not doing DHE (if the client sent the
+         * DHE option we always take it); don't send key share.
+         */
         return EXT_RETURN_NOT_SENT;
     }

There is also some fallout in the test suite -- the original version of the PR changed a success to an expected failure, which turns out to have been detecting exactly this bug. It can go back to its original (success) formulation, though I updated the commentary slightly.

I'm also rebasing onto a fresh master since it's been more than a month.

David had in essence asked two questions, and I'll answer them separately.

We can use s->hit to tell if a PSK is being used. For TLS 1.3, the relevant parts are inside tls_parse_ctos_psk(); it is important to note that this is not called as part of the main extension processing flow, but instead is specifically called (along with tls_parse_ctos_psk_kex_modes()) in ssl_get_prev_session(). The internal-cache stateless and stateful ticket processing is done inline here, but we give the application callbacks psk_find_session_callback (new) and psk_server_callback (legacy) [0] a chance to find a session before trying the internal processing. Regardless of where we find a session (from callback or ticket processing), either ssl_get_prev_session() succeeds and we use that session, so it's a "hit", the session is not usable and is removed from the SSL, or ssl_get_prev_session() returns a fatal error and we abort the handshake. So, if we use the PSK-derived session, it is a hit, and we can safely rely on s->hit to reflect that a PSK is being used.

In order to tell which key-exchange mode is being used, we have to jump through some contortions. We rely in a couple ways on the fact that there are only two defined modes at present (pure-psk and psk+DHE). Our server always implements psk+DHE and prefers it if possible, and our server only allows using pure-PSK if SSL_OP_ALLOW_NO_DHE_KEX is set. To reiterate, because of our server preference, if the client offered psk+DHE that is what we will use. So we can only get into this scenario if the client doesn't offer psk+DHE. If the client only offers pure-psk and the option is not set (or if the client offers only things we don't recognize, etc.), then s->ext.psk_kex_mode == 0 and we abort the handshake at the top of tls_parse_ctos_psk() and would never get this far. So if we made it this far and we are using psk-only, that means that the client offered only pure-psk (and maybe some things we don't recognize) and SSL_OP_ALLOW_NO_DHE_KEX is set. If we are using psk-only, TLSEXT_KEX_MODE_FLAG_KE_DHE is not set, since if it was set we'd be doing psk_DHE. Thus, checking that (s->ext.psk_kex_mode & TLSEXT_KEX_MODE_FLAG_KE_DHE) == 0 produces the correct result: there are only the two flags and thus four possible values. If neither flag is set, the handshake has failed already. If the psk+DHE flag is set, we're doing DHE (which is what that code checks), and the only other option is pure-PSK.

[0] The two named callbacks are the only ones relevant for session resumption on the server. Other psk-adjacent functionality exposed to the application includes: psk_client_callback (used to retrieve a session on the client to offer for potential rejection, which as client-only is not relevant for server operation), cert->psk_identity_hint (used in constructing the pre-1.3 ServerKeyExchange), session->psk_identity_hint (to hold the value received in the ServerKeyExchange), psk_identity (set when constructing the ClientKeyExchange, itself based on the output of the psk_client_callback), and psk_use_session_cb (the new way to provide a session on a client as a candidate for resumption, which again as client-only is not relevant here).

(and the CI pointed out that the make update commit had to be refreshed)

@t8m I'm not sure if you were personally interested or just relaying the results of OTC triage, but this should be ready now.

It actually cherry-picks without code conflicts, but runs into the SSLfatal() API change around function codes (and needs a make update to allocate a code for final_psk()). Which is maybe borderline on whether it merits a new PR or not; I'm happy to go either way.

IMO that requires a new PR.

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.

Merged to master, thanks for the review.
The 1.1.1 version is in #15252 , so removing the 1.1.1 label and closing this one as complete.