Add API to generate NewSessionTicket on demand by kaduk · Pull Request #11416 · openssl/openssl

kaduk · 2020-03-26T22:55:14Z

Add a TLS-1.3-server-only API to allow the application to request that a NewSessionTicket message be generated the next time we are about to start writing a data record.

Fixes: #10069

Checklist

documentation is added or updated
tests are added or updated

An 'if' clause was nestled against a previous closing brace as it if was an 'else if', but should properly stand on its own line.

This API requests that the TLS stack generate a (TLS 1.3) NewSessionTicket message the next time it is safe to do so (i.e., we do not have other data pending write, which could be mid-record). For efficiency, defer actually generating/writing the ticket until there is other data to write, to avoid producing server-to-client traffic when not needed.

Run a normal handshake and then request some extra tickets, checking that the new_session_cb is called the expected number of times. Since the tickets are generated in the same way as other tickets, there should not be a need to verify that these specific ones can be used to resume. Run the test with both zero and a non-zero number of tickets issued in the initial handshake.

kaduk · 2020-03-26T22:59:20Z

ssl/statem/statem_srvr.c

         */
-        if (s->hit || s->num_tickets <= s->sent_tickets) {
+        if (s->ext.extra_tickets_expected > 0) {
+            s->ext.extra_tickets_expected--;


This chunk illustrates an annoying decision I had to make about the semantics of the new field -- num_tickets is a fixed configuration value, and sent_tickets is an accurate counter over the lifetime of the relevant portion of the connection, but we both increment and decrement extra_tickets_expected in the current implementation. If we were to make it increment-only, then we'd have to do a three-variable check with s->sent_tickets vs s->num_tickets + s->ext.extra_tickets_expected that's a little harder to read, and that might bleed out into more places in the code. I don't have a strong opinion, but took this route because I liked having the semantics of s->ext.extra_tickets_expected > 0 indicating a pending request.

Would conditioning on having completed the handshake make sense?

That would probably make sense, yes. Thanks!

Unfortunately, neither SSL_in_init() nor SSL_is_init_finished() suffice for this call site (at least in my current model for the API), since in order to write new handshake messages (tickets) we go back "in init", and for the case when we want to write more than one ticket in a row, we have to stay there for multiple passes through this logic. So when I tried it, the "send two tickets in a row" test only sent one ticket.

SSL_IS_FIRST_HANDSHAKE might do what you need to figure out whether the original handshake has completed or not

I added the check here, though later testing indicates that in a normal 1.3 handshake SSL_IS_FIRST_HANDSHAKE() will return 0 by the time we are writing tickets. (Which makes sense, since it's looking at finished lengths, and NST happens after Finished.)

kaduk · 2020-03-26T23:00:49Z

test/sslapitest.c

+            || !TEST_false(SSL_read_ex(clientssl, buf, sizeof(buf), &nbytes))
+            || !TEST_int_eq(4, new_called))
+        goto end;
+


I'd love to also be able to test the logic that delays writing tickets while there's a partial write pending, but as far as I can tell the mempacket BIO that we use in this test harness does not have a way to leave an incomplete write, at present.

We have bio_s_always_retry() does that help? See test_key_update_in_write for an example

It does; thanks!

I'll note for posterity that adding this test revealed a bug in the code that checked if writes were pending that mis-grouped the boolean expressions.

kaduk · 2020-03-26T23:04:33Z

ssl/ssl_lib.c


+int SSL_new_session_ticket(SSL *s)
+{
+	if (SSL_in_init(s) || !s->server || !SSL_IS_TLS13(s))


I feel like there should be another thing to check here, but this matches the documentation and I am not sure what the other check would be...

During the early data phase we come out of init for a while - but the first handshake has not yet completed. Therefore we might want a check of SSL_IS_FIRST_HANDSHAKE here (which returns true if still in the first handshake regardless of the "init" value).

That is almost certainly the other thing I was thinking of; thanks!

kaduk · 2020-03-26T23:18:05Z

ssl/statem/statem_srvr.c


    case TLS_ST_SW_SESSION_TICKET:
-        if (SSL_IS_TLS13(s) && s->sent_tickets == 0) {
+        if (SSL_IS_TLS13(s) && s->sent_tickets == 0 && s->ext.extra_tickets_expected == 0) {


This check worries me a bit, since it implicitly relies on the invariant that s->ext.extra_tickets_expected is zero during the initial handshake. (A potential design to implement "only send tickets on first application data" on top of this would violate that invariant.)

Perhaps an ossl_assert here to check this invariant is appropriate?

I don't think I can actually assert anything at this location, since (as mentioned above) SSL_IS_FIRST_HANDSHAKE() is false already by this point. Preventing calls to SSL_new_session_ticket() should be enough, though if we do end up adding an option to defer the initial batch of tickets until the first write we'll need to check whether sent_tickets should be updated for all tickets (not just initial ones)

paulidale

Looks okay, and I'd approve if my TLS 1.3 knowledge were sufficiently advanced.

CI failures are relevant.

paulidale · 2020-04-01T02:47:18Z

ssl/statem/statem_srvr.c

         */
-        if (s->hit || s->num_tickets <= s->sent_tickets) {
+        if (s->ext.extra_tickets_expected > 0) {
+            s->ext.extra_tickets_expected--;


Would conditioning on having completed the handshake make sense?

kaduk · 2020-04-02T17:13:43Z

I'd approve if my TLS 1.3 knowledge were sufficiently advanced.

I expect to ask Matt for review, but only after 3.0.0alpha1 is out -- this change is lower priority than the release process.

kaduk · 2020-04-03T17:55:00Z

Travis error was just a timeout:

Still running (42 of 60): make _tests
The job exceeded the maximum time limit for jobs, and has been terminated.

kaduk · 2020-04-20T19:46:07Z

@mattcaswell now that #11520 is closed, is it appropriate to put a bit of time into looking at this one?

kaduk · 2020-04-20T19:57:01Z

Oops, I just saw #11470 (comment), so nevermind

kaduk · 2020-04-27T23:28:02Z

@mattcaswell do you think you'll want to review this one?

mattcaswell

I finally found some time to look at this.

mattcaswell · 2020-04-28T10:22:08Z

doc/man3/SSL_CTX_set_num_tickets.pod

+starting a new write operation, so that it is bundled with other application
+data being written and properly aligned to a record boundary.
+SSL_new_session_ticket() can be called more than once to request additional
+tickets be sent; all such requests are queued until and the multiple tickets


"queued until". Doesn't quite make sense

It was once "queued until it is safe to do so" but had a bad edit; fixed.

doc/man3/SSL_CTX_set_num_tickets.pod

mattcaswell · 2020-04-28T10:25:04Z

ssl/ssl_lib.c


+int SSL_new_session_ticket(SSL *s)
+{
+	if (SSL_in_init(s) || !s->server || !SSL_IS_TLS13(s))


During the early data phase we come out of init for a while - but the first handshake has not yet completed. Therefore we might want a check of SSL_IS_FIRST_HANDSHAKE here (which returns true if still in the first handshake regardless of the "init" value).

mattcaswell · 2020-04-28T10:29:18Z

ssl/statem/statem_srvr.c


    case TLS_ST_SW_SESSION_TICKET:
-        if (SSL_IS_TLS13(s) && s->sent_tickets == 0) {
+        if (SSL_IS_TLS13(s) && s->sent_tickets == 0 && s->ext.extra_tickets_expected == 0) {


Perhaps an ossl_assert here to check this invariant is appropriate?

mattcaswell · 2020-04-28T10:30:53Z

test/sslapitest.c

+            || !TEST_false(SSL_read_ex(clientssl, buf, sizeof(buf), &nbytes))
+            || !TEST_int_eq(idx * 2 + 4, new_called)
+            /* || !TEST_int_eq(sizeof(buf), nbytes)*/
+            /* || !TEST_int_eq(c, buf[0]) */


Why the commented out code?

Same debugging artifact -- I can't check the value that I read when the read fails because the write was zero-length!

mattcaswell · 2020-04-28T10:32:39Z

test/sslapitest.c

+
+    /* Now try a (real) write to actually send the tickets */
+    c = '1';
+    if (!TEST_true(SSL_write_ex(serverssl, &c, 0, &nbytes))


You talk about doing it again below using "dummy" writes...but this also looks like a dummy write

Seems to be an artifact of some changes I made while debugging the initial work; it should be a real 1-byte write now.

mattcaswell · 2020-04-28T10:34:09Z

test/sslapitest.c

+    new_called = 0;
+    do_cache = 1;
+
+    if (!setup_ticket_test(0, idx, &sctx, &cctx))


Should we also be testing this with stateful tickets (i.e. the first param here set to 1)?

I do recall deciding to not test stateful tickets, but I don't remember why, so it clearly can't have been a good reason.
(added.)

kaduk · 2020-04-28T22:47:34Z

Thanks for all the comments! These are really good and will give me some more bits to work on for the next update. (SSL_IS_FIRST_HANDSHAKE() is already looking promising; I'm not sure why I failed to find that when I first looked for it.)
With respect to the "dummy writes", they should be asking to write zero bytes, which is why I called them "dummy".

Test the write-pending case.

kaduk

I'm about to push a pile of fixup commits, but I suspect that it makes more sense for me to rebase+squash, adjusting commit message wording as needed, instead of just trying to review the fixups. Let me know if you want me to rebase+squash.

kaduk · 2020-04-29T17:04:27Z

doc/man3/SSL_CTX_set_num_tickets.pod

+starting a new write operation, so that it is bundled with other application
+data being written and properly aligned to a record boundary.
+SSL_new_session_ticket() can be called more than once to request additional
+tickets be sent; all such requests are queued until and the multiple tickets


It was once "queued until it is safe to do so" but had a bad edit; fixed.

kaduk · 2020-04-29T17:04:49Z

ssl/ssl_lib.c


+int SSL_new_session_ticket(SSL *s)
+{
+	if (SSL_in_init(s) || !s->server || !SSL_IS_TLS13(s))


That is almost certainly the other thing I was thinking of; thanks!

kaduk · 2020-04-29T17:06:43Z

ssl/statem/statem_srvr.c

         */
-        if (s->hit || s->num_tickets <= s->sent_tickets) {
+        if (s->ext.extra_tickets_expected > 0) {
+            s->ext.extra_tickets_expected--;


I added the check here, though later testing indicates that in a normal 1.3 handshake SSL_IS_FIRST_HANDSHAKE() will return 0 by the time we are writing tickets. (Which makes sense, since it's looking at finished lengths, and NST happens after Finished.)

kaduk · 2020-04-29T17:09:07Z

ssl/statem/statem_srvr.c


    case TLS_ST_SW_SESSION_TICKET:
-        if (SSL_IS_TLS13(s) && s->sent_tickets == 0) {
+        if (SSL_IS_TLS13(s) && s->sent_tickets == 0 && s->ext.extra_tickets_expected == 0) {


I don't think I can actually assert anything at this location, since (as mentioned above) SSL_IS_FIRST_HANDSHAKE() is false already by this point. Preventing calls to SSL_new_session_ticket() should be enough, though if we do end up adding an option to defer the initial batch of tickets until the first write we'll need to check whether sent_tickets should be updated for all tickets (not just initial ones)

kaduk · 2020-04-29T17:09:52Z

test/sslapitest.c

+    new_called = 0;
+    do_cache = 1;
+
+    if (!setup_ticket_test(0, idx, &sctx, &cctx))


I do recall deciding to not test stateful tickets, but I don't remember why, so it clearly can't have been a good reason.
(added.)

kaduk · 2020-04-29T17:10:30Z

test/sslapitest.c

+
+    /* Now try a (real) write to actually send the tickets */
+    c = '1';
+    if (!TEST_true(SSL_write_ex(serverssl, &c, 0, &nbytes))


Seems to be an artifact of some changes I made while debugging the initial work; it should be a real 1-byte write now.

kaduk · 2020-04-29T17:10:55Z

test/sslapitest.c

+            || !TEST_false(SSL_read_ex(clientssl, buf, sizeof(buf), &nbytes))
+            || !TEST_int_eq(idx * 2 + 4, new_called)
+            /* || !TEST_int_eq(sizeof(buf), nbytes)*/
+            /* || !TEST_int_eq(c, buf[0]) */


Same debugging artifact -- I can't check the value that I read when the read fails because the write was zero-length!

kaduk · 2020-04-29T17:11:08Z

test/sslapitest.c

+            || !TEST_false(SSL_read_ex(clientssl, buf, sizeof(buf), &nbytes))
+            || !TEST_int_eq(4, new_called))
+        goto end;
+


It does; thanks!

mattcaswell

This looks good. Just a handful of minor comments. Travis red cross is not relevant.

mattcaswell · 2020-04-30T08:25:16Z

doc/man3/SSL_CTX_set_num_tickets.pod

+indicates only that the request to send a ticket was processed, not that the
+ticket itself was sent.  To be notified when the ticket itself is sent, a
+new-session callback can be registered with L<SSL_CTX_sess_set_new_cb(3)> that
+will be invoked as the ticket or tickets are generated.


It's kind of implied, but perhaps make it clearer that this is only valid on the server side - you can't use this on the client side to request the server send you a ticket!

mattcaswell · 2020-04-30T08:27:31Z

ssl/record/rec_layer_s3.c

@@ -387,7 +387,8 @@ int ssl3_write_bytes(SSL *s, int type, const void *buf_, size_t len,
     * If we are supposed to be sending a KeyUpdate then go into init unless we
     * have writes pending - in which case we should finish doing that first.


Update this comment to say we also go into init if we have pending tickets to send?

mattcaswell · 2020-04-30T08:32:26Z

ssl/statem/statem_srvr.c

        }
+        if (s->ext.extra_tickets_expected > 0) {
+            st->hand_state = TLS_ST_SW_SESSION_TICKET;
+            s->ext.extra_tickets_expected--;


Is it feasible to move this decrement into the ticket processing itself? I've tried (not 100% successfully) to avoid side effects in the transition functions as much as possible.

Sure; I had gone this route since I didn't want to put the "random side effect" into the ticket-generation function, but avoiding side-effects in the transition functions seems like a better thing to prioritize.

mattcaswell · 2020-04-30T08:33:45Z

ssl/statem/statem_srvr.c

         */
-        if (s->hit || s->num_tickets <= s->sent_tickets) {
+        if (!SSL_IS_FIRST_HANDSHAKE(s) && s->ext.extra_tickets_expected > 0) {
+            s->ext.extra_tickets_expected--;


Similarly here it would be good to avoid side effects if possible.

kaduk · 2020-04-30T21:31:05Z

New (final?) fixup commit pushed.

This API requests that the TLS stack generate a (TLS 1.3) NewSessionTicket message the next time it is safe to do so (i.e., we do not have other data pending write, which could be mid-record). For efficiency, defer actually generating/writing the ticket until there is other data to write, to avoid producing server-to-client traffic when not needed. Reviewed-by: Matt Caswell <[email protected]> (Merged from openssl#11416) (cherry picked from commit 3bfacb5)

Run a normal handshake and then request some extra tickets, checking that the new_session_cb is called the expected number of times. Since the tickets are generated in the same way as other tickets, there should not be a need to verify that these specific ones can be used to resume. Run the test with both zero and a non-zero number of tickets issued in the initial handshake. Reviewed-by: Matt Caswell <[email protected]> (Merged from openssl#11416) (cherry picked from commit f0049b8)