TLS 1.3 server: don't send NewSessionTicket (session resumption is not implemented)

[EdouardMALOT]

Summary

The TLS 1.3 server sends a NewSessionTicket after every completed handshake, but session resumption is not implemented: the server can never honor the ticket it just issued.

What _nx_secure_tls_send_newsessionticket() actually sends: ticket_lifetime = 604800 (the RFC maximum, 7 days), a random ticket_age_add, and the hardcoded string "NewSessionTicket" as ticket identity. No server-side state is created. The ticket is a stub.

What happens when a client caches that ticket and offers it on its next connection depends on the build:

• With NX_SECURE_ENABLE_PSK_CIPHERSUITES: _nx_secure_tls_process_clienthello_psk_extension() rejects any PSK identity with a non-zero obfuscated age (/* Only support external PSKs (no session resumption). */) with NX_SECURE_TLS_BAD_CLIENTHELLO_PSK_EXTENSION and a fatal alert. Since ticket_age_add is random, a replayed ticket's obfuscated age is never zero in practice. Observed in the field with Java (JSSE) clients: every other connection fails. Connection N succeeds and caches the ticket, connection N+1 replays it and is aborted, JSSE drops the cached ticket, connection N+2 succeeds, and so on.
• Without it: the pre_shared_key extension is ignored and the handshake completes as a full handshake. No failure, but every TLS 1.3 client is handed a 7-day stub ticket it will cache and uselessly replay on every connection.

Changes

• nx_secure/src/nx_secure_tls_1_3_server_handshake.c — do not send NewSessionTicket after the client Finished. The message is optional per RFC 8446 §4.6.1 ("the server MAY send"); not advertising resumption is the correct behavior as long as it is not implemented.

_nx_secure_tls_send_newsessionticket() has no state side effects (it only writes the packet), so nothing downstream is affected; the function is kept for the day resumption support lands. The PSK extension handler is untouched, so external PSKs (age == 0) keep working. Incidentally, the removed block also contained an unchecked status (_nx_secure_tls_send_newsessionticket()'s return value was overwritten by the next call).

Possible follow-up, out of scope here: per RFC 8446 §4.2.11, a server that does not accept an offered PSK identity should skip it and fall back to a full handshake rather than abort. That would harden the handler against resumption offers from any peer.

Test plan

• TLS 1.3 server with NX_SECURE_ENABLE_PSK_CIPHERSUITES, Java (JSSE) client making repeated connections: previously every other handshake failed with a fatal alert, now all connections succeed
• TLS 1.3 client ↔ NetX server full handshake: unchanged apart from the absent (optional) NewSessionTicket record
• External PSK ciphersuites: unchanged (PSK extension handler untouched)

[fdesbiens]

Thank you — this is a well-diagnosed problem and I agree with the conclusion. I checked every claim in the description and they all hold:

The ticket really is a stub. nx_secure_tls_send_newsessionticket.c:90 sets lifetime = 604800, :98 sets age_add = NX_RAND(), and :124-125 set the ticket to the literal string "NewSessionTicket". More importantly for the safety of removing it, the function contains no writes to tls_session at all — it only fills the packet buffer. So there are no state side effects and nothing downstream can depend on it having run, exactly as you say.

The rejection path is as described. _nx_secure_tls_process_clienthello_psk_extension() returns NX_SECURE_TLS_BAD_CLIENTHELLO_PSK_EXTENSION for any age != 0 at nx_secure_tls_process_clienthello_extensions.c:1397-1401. And that error code is not present in _nx_secure_tls_map_error_to_alert(), so it falls through to the default: case and produces a fatal internal_error alert — which is precisely the Alert(internal_error) in your comment. The handler is also compiled only under NX_SECURE_ENABLE_PSK_CIPHERSUITES together with TLS 1.3 (:1269), so your two-configuration breakdown is accurate.

The unchecked status was real. In the removed block, status from _nx_secure_tls_send_newsessionticket() was overwritten by the following _nx_secure_tls_send_handshake_record() call before ever being tested. Removing the block eliminates that too.

Nothing breaks. The only test that mentions NX_SECURE_TLS_NEW_SESSION_TICKET is nx_secure_tls_1_3_invalid_client_state_test.c, and it constructs the record itself over a raw TCP socket (:263, :281) rather than relying on the server handshake to emit one, so it is unaffected. I also checked that removing the block does not orphan any locals — send_packet and packet_pool are still used throughout the function, so there is no -Werror unused-variable risk.

One detail worth adding to your write-up, because it explains why this went unnoticed for so long: NetX Duo's own TLS 1.3 client treats the message as a no-op — nx_secure_tls_1_3_client_handshake.c:339-342 is just status = NX_SUCCESS; break;. It never caches or replays a ticket, so a NetX-to-NetX test can never reproduce this. It genuinely took a third-party client to surface it.

I also considered the alternative of keeping the message but setting ticket_lifetime = 0, since RFC 8446 defines zero as "discard immediately". Not sending it is better: it is explicitly permitted by §4.6.1, it costs one fewer record, and it does not depend on the client honouring the zero. Your choice is the right one.

Two things before merge. The coverage side effect needs a decision; this leaves an entire source file with no caller. And I would not defer the PSK handler as far as "out of scope" — as I explain, this patch alone does not fix the field symptom if you have more than one version of the server in service.

[fdesbiens]

You raise the RFC 8446 §4.2.11 behaviour — a server that does not accept an offered PSK identity should ignore it and fall back to a full handshake rather than abort — as a possible follow-up, out of scope here. I would push back on the prioritisation, for one concrete reason.

This patch stops this server from handing out tickets. It does not stop a client from offering one. A JSSE client acquires its cached ticket from whichever server it last spoke to, so as soon as there is more than one version of the server in service, the symptom persists:

• A device fleet mid-rollout, where some nodes are patched and some are not: a client that gets a ticket from an unpatched node and then reaches a patched one is still aborted with a fatal alert, because the patched node still rejects age != 0.
• A load-balanced pool with any unpatched member, for the same reason.
• A rollback, where a client holds a ticket issued before the patch.

In all three the observed behaviour is unchanged from what you reported: every other connection dies. The abort is the defect that produces the visible failure; not sending the ticket removes one source of the trigger.

So the two changes are complementary, and the PSK handler is the one that makes the server robust regardless of where a ticket came from. Fixing it would also make this PR optional rather than necessary — though I would still take this PR, because advertising a capability that is not implemented is wrong on its own terms.

My ask is just that it be tracked as a companion issue rather than an aspiration, and ideally landed in the same release. Happy for it to be a separate PR.

[fdesbiens]

After this change _nx_secure_tls_send_newsessionticket() has zero callers in the library — I grepped, and the only remaining references are its own definition and the prototype at nx_secure/inc/nx_secure_tls.h:1426.

The consequence is a measurable coverage regression. Today every completed TLS 1.3 server handshake runs that function, so nx_secure_tls_send_newsessionticket.c is covered; afterwards it is 0% across the board. The NX_SECURE_TLS_NEW_SESSION_TICKET case in the client handshake (nx_secure_tls_1_3_client_handshake.c:339) also becomes unreachable in NetX-to-NetX runs.

I checked whether this breaks CI and it does not — test/cmake/nx_secure/coverage.sh runs gcovr to produce XML and HTML reports but does not enforce a threshold, so no gate fails. But we aim for 100% coverage, so a file going to zero will not do.

Two options, and I do not mind which:

1. Add nx_secure_tls_send_newsessionticket.c to the exclude_list in coverage.sh — the mechanism already exists at :22-33, where nx_secure_tls_1_3_server_handshake.c and others are excluded for default_build_coverage. This is the smallest change and keeps the reports honest.
2. Keep the function reachable from a test, if you would rather not lose the coverage — it can be called directly the way nx_secure_tls_1_3_invalid_client_state_test.c calls other internals.

Please pick one explicitly rather than leaving it to be discovered.

[fdesbiens]

This comment concerns nx_secure/src/nx_secure_tls_send_newsessionticket.c.

You note in the description that the function is kept for when resumption support lands, which is the right call. That intent should live in the source rather than only in the PR, because the next person doing a dead-code sweep will find an unreferenced global function and reasonably propose deleting it.

A line in its header block — that it is currently uncalled, retained for future session-resumption support, and that the ticket it builds is a placeholder with no server-side state — would save that round trip. It is also the natural place to record that the ticket identity is a fixed string, which is the fact that makes it unusable today.

On footprint: the function is a non-static global, so whether it survives linking depends on the consumer. The netxduo builds compile with -ffunction-sections -fdata-sections, so it will be garbage-collected there, but an application linking the static library without --gc-sections will carry it. Not a reason to change anything, just worth knowing it is not automatically free.

[fdesbiens]

Nine lines of comment replacing fourteen lines of code, and most of it narrates what used to go wrong and which third-party client exposed it. That is exactly the material your PR description and commit message should carry — and yours already does, well.

What a future reader of this function needs is shorter: that no NewSessionTicket is sent because session resumption is not implemented, that the message is optional per RFC 8446 §4.6.1, and a pointer to the PSK handler as the place that would also need changing. Two or three lines.

This is the same point I made on #401, so treat it as a general preference rather than a fresh objection: keep the reasoning that dates from the fix in the commit, and keep the source comment about the code as it now stands.

[fdesbiens]

There is no regression test. I recognise this one is harder than the others in your recent series: asserting an absence means either checking that the server emits no handshake record between Finished and application data, or driving a crafted ClientHello carrying a PSK identity with a non-zero obfuscated age and asserting the connection survives — and the latter only becomes possible once the PSK handler is fixed per finding 1.

The cheaper half is worth doing now: a TLS 1.3 server test that completes a handshake and asserts no NewSessionTicket record follows would lock in this change and would have caught the original stub being introduced. The PSK-offer test can come with the follow-up.