Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 5 additions & 14 deletions nx_secure/src/nx_secure_tls_1_3_server_handshake.c

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went with option 1 in cae8841. Added a common exclude list to coverage.sh so the file is excluded across every coverage config, not just default_build_coverage — tls_1_3_enable_build_coverage and sesip_build_coverage also compile TLS 1.3. Reason lives in the file's header block, per the previous commit.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the header note in 725f44a. Three facts: uncalled on purpose, ticket is a placeholder (no state, fixed identity), pointer to the server handshake and RFC 8446 §4.6.1 for the why. CALLED BY updated to match.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in dfa4c28. Cheaper half: full TLS 1.3 handshake, then a raw TCP peek on the client socket after one periodic tick — the pre-fix NewSessionTicket lands there before anyone asks the TLS layer to receive, so the peek returns NX_SUCCESS instead of NX_NO_PACKET. Raw socket rather than session_receive because the TLS layer would consume the ticket silently. PSK-offer half deferred to the companion issue.

Heads-up on validation: I couldn't run the test locally — nested emulation on macOS/arm64 segfaults for every existing 32-bit i386 test in this suite (including nx_secure_tls_1_3_invalid_client_state), so this needs the CI's native x86_64 to actually execute.

Original file line number Diff line number Diff line change
Expand Up @@ -559,20 +559,11 @@ NX_SECURE_TLS_SERVER_STATE old_server_state;

/* Post-Auth server messages (if any) are sent here. */

/* For session resumption, send a NewSessionTicket message to allow for resumption PSK to be generated. */
status = _nx_secure_tls_allocate_handshake_packet(tls_session, packet_pool, &send_packet, wait_option);
if (status != NX_SUCCESS)
{
break;
}

/* Populate the packet with our NewSessionTicket Message. */
status = _nx_secure_tls_send_newsessionticket(tls_session, send_packet);
status = _nx_secure_tls_send_handshake_record(tls_session, send_packet, NX_SECURE_TLS_NEW_SESSION_TICKET, wait_option);
if(status != NX_SUCCESS)
{
break;
}
/* No NewSessionTicket is sent: session resumption is not
* implemented on this server, and the message is optional per
* RFC 8446 Section 4.6.1. The corresponding rejection path
* lives in _nx_secure_tls_process_clienthello_psk_extension,
* which is where resumption support would also need to land. */

/* If we get here, the Client Finished was processed without errors and the handshake is complete. */
tls_session -> nx_secure_tls_server_state = NX_SECURE_TLS_SERVER_STATE_HANDSHAKE_FINISHED;
Expand Down
10 changes: 9 additions & 1 deletion nx_secure/src/nx_secure_tls_send_newsessionticket.c
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,14 @@
/* for session resumption should the same client attempt another */
/* connection within the lifespan of the ticket. */
/* */
/* NOTE: this function is currently uncalled. It is retained for a */
/* future implementation of TLS 1.3 session resumption. The ticket it */
/* builds today is a placeholder: no server-side state is stored, and */
/* the ticket identity is a fixed string, so a client that replayed it */
/* would be rejected. See _nx_secure_tls_1_3_server_handshake for the */
/* reason it is not called and RFC 8446 Section 4.6.1 for the fact */
/* that sending NewSessionTicket is optional. */
/* */
/* INPUT */
/* */
/* tls_session TLS control block */
Expand All @@ -56,7 +64,7 @@
/* */
/* CALLED BY */
/* */
/* _nx_secure_tls_server_handshake TLS server state machine */
/* (none - retained for future session resumption support) */
/* */
/**************************************************************************/
#if (NX_SECURE_TLS_TLS_1_3_ENABLED)
Expand Down
11 changes: 11 additions & 0 deletions test/cmake/nx_secure/coverage.sh
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,17 @@ cd $(dirname $0)
root_path=$(cd ../../../nx_secure/src; pwd)
mkdir -p coverage_report/$1
extra_args=""
# Always excluded: retained for a future TLS 1.3 session-resumption
# implementation but currently uncalled. See the header block of
# nx_secure_tls_send_newsessionticket.c and PR #403 for the reason.
common_exclude_list="nx_secure_tls_send_newsessionticket.c"
for e in $common_exclude_list
do
for f in $(ls $root_path/$e);
do
extra_args+="-e $f "
done
done
if [ "$1" == "default_build_coverage" ];
then
exclude_list="nx*_secure_dtls_*.c \
Expand Down
1 change: 1 addition & 0 deletions test/cmake/nx_secure/regression/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ set(nx_secure_test_cases
${SOURCE_DIR}/nx_secure_test/nx_secure_tls_client_ca_select_test.c
${SOURCE_DIR}/nx_secure_test/nx_secure_tls_1_3_receive_invalid_server_handshake_message_test.c
${SOURCE_DIR}/nx_secure_test/nx_secure_tls_1_3_invalid_client_state_test.c
${SOURCE_DIR}/nx_secure_test/nx_secure_tls_1_3_no_newsessionticket_test.c
${SOURCE_DIR}/nx_secure_test/nx_secure_tls_1_3_serverhello_length_checking_test.c
${SOURCE_DIR}/nx_secure_test/nx_secure_tls_1_3_session_create_ext_test.c
${SOURCE_DIR}/nx_secure_test/nx_secure_tls_1_3_before_key_generation_test.c
Expand Down
2 changes: 2 additions & 0 deletions test/regression/nx_secure_test/netxtestcontrol.c
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,7 @@ void nx_secure_tls_1_3_clienthello_length_checking_test_application_define(void
void nx_secure_tls_1_3_handshake_fail_test_application_define(void *);
void nx_secure_tls_1_3_hello_retry_cookie_test_application_define(void *);
void nx_secure_tls_1_3_invalid_client_state_test_application_define(void *first_unused_memory);
void nx_secure_tls_1_3_no_newsessionticket_test_application_define(void *first_unused_memory);
void nx_secure_tls_1_3_key_share_test_application_define(void *);
void nx_secure_tls_1_3_provisioned_psk_test_application_define(void *);
void nx_secure_tls_1_3_receive_invalid_server_handshake_message_test_application_define(void *);
Expand Down Expand Up @@ -361,6 +362,7 @@ TEST_ENTRY test_control_tests[] =
{nx_secure_tls_1_3_handshake_fail_test_application_define, TEST_TIMEOUT_LOW},
{nx_secure_tls_1_3_hello_retry_cookie_test_application_define, TEST_TIMEOUT_LOW},
{nx_secure_tls_1_3_invalid_client_state_test_application_define, TEST_TIMEOUT_LOW},
{nx_secure_tls_1_3_no_newsessionticket_test_application_define, TEST_TIMEOUT_LOW},
{nx_secure_tls_1_3_key_share_test_application_define, TEST_TIMEOUT_LOW},
{nx_secure_tls_1_3_serverhello_length_checking_test_application_define, TEST_TIMEOUT_LOW},
{nx_secure_tls_1_3_session_create_ext_test_application_define, TEST_TIMEOUT_LOW},
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,306 @@
/***************************************************************************/
/* Copyright (c) 2024 Microsoft Corporation */
/* Copyright (c) 2026 Eclipse ThreadX contributors */
/* */
/* This program and the accompanying materials are made available under */
/* the terms of the MIT License which is available at */
/* https://opensource.org/licenses/MIT. */
/* */
/* SPDX-License-Identifier: MIT */
/***************************************************************************/

/* This test completes a TLS 1.3 handshake between a NetX client and a */
/* NetX server and asserts that the server sends no additional record */
/* after its Finished. The server previously emitted a stub */
/* NewSessionTicket here; see PR #403 for the reason it was removed. */

#include "nx_api.h"
#include "nx_secure_tls_api.h"
#include "ecc_certs.c"

extern VOID test_control_return(UINT status);


#if !defined(NX_SECURE_TLS_CLIENT_DISABLED) && !defined(NX_SECURE_TLS_SERVER_DISABLED) && defined(NX_SECURE_ENABLE_ECC_CIPHERSUITE) && (NX_SECURE_TLS_TLS_1_3_ENABLED)
#define NUM_PACKETS 24
#define PACKET_SIZE 1536
#define PACKET_POOL_SIZE (NUM_PACKETS * (PACKET_SIZE + sizeof(NX_PACKET)))
#define THREAD_STACK_SIZE 1024
#define ARP_CACHE_SIZE 1024
#define METADATA_SIZE 16000
#define CERT_BUFFER_SIZE 2048
#define SERVER_PORT 4433

/* Define the ThreadX and NetX object control blocks. */

static TX_THREAD thread_server;
static TX_THREAD thread_client;
static NX_PACKET_POOL pool_0;
static NX_IP ip_0;
static UINT error_counter;

static NX_TCP_SOCKET client_socket_0;
static NX_SECURE_TLS_SESSION tls_client_session_0;
static NX_SECURE_X509_CERT client_trusted_ca;
static NX_SECURE_X509_CERT client_remote_cert;
static NX_TCP_SOCKET server_socket_0;
static NX_SECURE_TLS_SESSION tls_server_session_0;
static NX_SECURE_X509_CERT server_local_certificate;

static ULONG pool_0_memory[PACKET_POOL_SIZE / sizeof(ULONG)];
static ULONG thread_server_stack[THREAD_STACK_SIZE / sizeof(ULONG)];
static ULONG thread_client_stack[THREAD_STACK_SIZE / sizeof(ULONG)];
static ULONG ip_0_stack[THREAD_STACK_SIZE / sizeof(ULONG)];
static ULONG arp_cache[ARP_CACHE_SIZE];
static UCHAR client_metadata[METADATA_SIZE];
static UCHAR server_metadata[METADATA_SIZE];
static UCHAR client_cert_buffer[CERT_BUFFER_SIZE];

static UCHAR tls_packet_buffer[2][4000];

extern const USHORT nx_crypto_ecc_supported_groups[];
extern const NX_CRYPTO_METHOD *nx_crypto_ecc_curves[];
extern const UINT nx_crypto_ecc_supported_groups_size;
extern const NX_SECURE_TLS_CRYPTO nx_crypto_tls_ciphers_ecc;

/* Signal from client to server: handshake done and the raw TCP receive
check has finished, so the server may tear down its session. */
static TX_SEMAPHORE semaphore_check_done;

/* Define thread prototypes. */

static VOID test_client_entry(ULONG thread_input);
static VOID test_server_entry(ULONG thread_input);
extern VOID _nx_ram_network_driver_1500(struct NX_IP_DRIVER_STRUCT *driver_req);

static VOID ERROR_COUNTER(void)
{
error_counter++;
}

#define do_something_if_fail(p) if (!(p)) { ERROR_COUNTER(); }

#ifdef CTEST
void test_application_define(void *first_unused_memory);
void test_application_define(void *first_unused_memory)
#else
VOID nx_secure_tls_1_3_no_newsessionticket_test_application_define(void *first_unused_memory)
#endif
{
UINT status;
CHAR *pointer;

error_counter = 0;

pointer = (CHAR *)first_unused_memory;

tx_thread_create(&thread_server, "thread server", test_server_entry, 0,
thread_server_stack, sizeof(thread_server_stack),
7, 7, TX_NO_TIME_SLICE, TX_AUTO_START);

tx_thread_create(&thread_client, "thread client", test_client_entry, 0,
thread_client_stack, sizeof(thread_client_stack),
8, 8, TX_NO_TIME_SLICE, TX_AUTO_START);

tx_semaphore_create(&semaphore_check_done, "semaphore check done", 0);

nx_system_initialize();

status = nx_packet_pool_create(&pool_0, "NetX Main Packet Pool", PACKET_SIZE,
pool_0_memory, PACKET_POOL_SIZE);
do_something_if_fail(status == NX_SUCCESS);

status = nx_ip_create(&ip_0, "NetX IP Instance 0", IP_ADDRESS(1, 2, 3, 4), 0xFFFFFF00UL,
&pool_0, _nx_ram_network_driver_1500,
ip_0_stack, sizeof(ip_0_stack), 1);
do_something_if_fail(status == NX_SUCCESS);

status = nx_arp_enable(&ip_0, (VOID *)arp_cache, sizeof(arp_cache));
do_something_if_fail(status == NX_SUCCESS);

status = nx_tcp_enable(&ip_0);
do_something_if_fail(status == NX_SUCCESS);

nx_secure_tls_initialize();
}

static VOID client_tls_setup(NX_SECURE_TLS_SESSION *tls_session_ptr)
{
UINT status;

memset(client_metadata, 0xFF, sizeof(client_metadata));
status = nx_secure_tls_session_create(tls_session_ptr,
&nx_crypto_tls_ciphers_ecc,
client_metadata,
sizeof(client_metadata));
do_something_if_fail(status == NX_SUCCESS);

status = nx_secure_tls_ecc_initialize(tls_session_ptr, nx_crypto_ecc_supported_groups,
nx_crypto_ecc_supported_groups_size,
nx_crypto_ecc_curves);
do_something_if_fail(status == NX_SUCCESS);

memset(&client_remote_cert, 0, sizeof(client_remote_cert));
status = nx_secure_tls_remote_certificate_allocate(tls_session_ptr,
&client_remote_cert,
client_cert_buffer,
sizeof(client_cert_buffer));
do_something_if_fail(status == NX_SUCCESS);

status = nx_secure_x509_certificate_initialize(&client_trusted_ca, ECCA4_der, ECCA4_der_len,
NX_NULL, 0, NULL, 0,
NX_SECURE_X509_KEY_TYPE_NONE);
do_something_if_fail(status == NX_SUCCESS);

status = nx_secure_tls_trusted_certificate_add(tls_session_ptr, &client_trusted_ca);
do_something_if_fail(status == NX_SUCCESS);

status = nx_secure_tls_session_packet_buffer_set(tls_session_ptr, tls_packet_buffer[0],
sizeof(tls_packet_buffer[0]));
do_something_if_fail(status == NX_SUCCESS);
}

static VOID server_tls_setup(NX_SECURE_TLS_SESSION *tls_session_ptr)
{
UINT status;

memset(server_metadata, 0xFF, sizeof(server_metadata));
status = nx_secure_tls_session_create(tls_session_ptr,
&nx_crypto_tls_ciphers_ecc,
server_metadata,
sizeof(server_metadata));
do_something_if_fail(status == NX_SUCCESS);

status = nx_secure_tls_ecc_initialize(tls_session_ptr, nx_crypto_ecc_supported_groups,
nx_crypto_ecc_supported_groups_size,
nx_crypto_ecc_curves);
do_something_if_fail(status == NX_SUCCESS);

memset(&server_local_certificate, 0, sizeof(server_local_certificate));
status = nx_secure_x509_certificate_initialize(&server_local_certificate,
ECTestServer4_der, ECTestServer4_der_len,
NX_NULL, 0, ECTestServer4_key_der,
ECTestServer4_key_der_len,
NX_SECURE_X509_KEY_TYPE_EC_DER);
do_something_if_fail(status == NX_SUCCESS);

status = nx_secure_tls_local_certificate_add(tls_session_ptr,
&server_local_certificate);
do_something_if_fail(status == NX_SUCCESS);

status = nx_secure_tls_session_packet_buffer_set(tls_session_ptr, tls_packet_buffer[1],
sizeof(tls_packet_buffer[1]));
do_something_if_fail(status == NX_SUCCESS);
}

static void test_server_entry(ULONG thread_input)
{
UINT status;

printf("NetX Secure Test: TLS 1.3 No NewSessionTicket Test..................");

status = nx_tcp_socket_create(&ip_0, &server_socket_0, "Server socket", NX_IP_NORMAL,
NX_DONT_FRAGMENT, NX_IP_TIME_TO_LIVE, 8192, NX_NULL, NX_NULL);
do_something_if_fail(status == NX_SUCCESS);

status = nx_tcp_server_socket_listen(&ip_0, SERVER_PORT, &server_socket_0, 5, NX_NULL);
do_something_if_fail(status == NX_SUCCESS);

server_tls_setup(&tls_server_session_0);

status = nx_tcp_server_socket_accept(&server_socket_0, NX_WAIT_FOREVER);
do_something_if_fail(status == NX_SUCCESS);

status = nx_secure_tls_session_start(&tls_server_session_0, &server_socket_0, NX_WAIT_FOREVER);
do_something_if_fail(status == NX_SUCCESS);

/* Wait for the client to finish its post-handshake check before we
tear down. Tearing down early would send a close_notify alert that
the client's raw TCP peek could observe. */
tx_semaphore_get(&semaphore_check_done, NX_WAIT_FOREVER);

nx_secure_tls_session_end(&tls_server_session_0, NX_IP_PERIODIC_RATE);
nx_secure_tls_session_delete(&tls_server_session_0);

nx_tcp_socket_disconnect(&server_socket_0, NX_NO_WAIT);
nx_tcp_server_socket_unaccept(&server_socket_0);
nx_tcp_server_socket_unlisten(&ip_0, SERVER_PORT);
nx_tcp_socket_delete(&server_socket_0);
}

static void test_client_entry(ULONG thread_input)
{
UINT status;
NX_PACKET *packet_ptr = NX_NULL;
NXD_ADDRESS server_address;

server_address.nxd_ip_version = NX_IP_VERSION_V4;
server_address.nxd_ip_address.v4 = IP_ADDRESS(127, 0, 0, 1);

status = nx_tcp_socket_create(&ip_0, &client_socket_0, "Client socket", NX_IP_NORMAL,
NX_DONT_FRAGMENT, NX_IP_TIME_TO_LIVE, 8192, NX_NULL, NX_NULL);
do_something_if_fail(status == NX_SUCCESS);

status = nx_tcp_client_socket_bind(&client_socket_0, NX_ANY_PORT, NX_NO_WAIT);
do_something_if_fail(status == NX_SUCCESS);

client_tls_setup(&tls_client_session_0);

status = nxd_tcp_client_socket_connect(&client_socket_0, &server_address, SERVER_PORT, NX_WAIT_FOREVER);
do_something_if_fail(status == NX_SUCCESS);

status = nx_secure_tls_session_start(&tls_client_session_0, &client_socket_0, NX_WAIT_FOREVER);
do_something_if_fail(status == NX_SUCCESS);

/* Give the server thread time to run and, in the buggy case, send its
stub NewSessionTicket after processing our Finished. One periodic
tick is far more than the server needs. */
tx_thread_sleep(NX_IP_PERIODIC_RATE);

/* Peek the raw TCP receive queue. The TLS layer has not been asked to
receive anything since the handshake, so any bytes here must have
been sent by the server after its Finished. In the fixed server
there are none. In the buggy server there is a NewSessionTicket. */
status = nx_tcp_socket_receive(&client_socket_0, &packet_ptr, NX_NO_WAIT);
if (status != NX_NO_PACKET)
{
ERROR_COUNTER();
if (packet_ptr != NX_NULL)
{
nx_packet_release(packet_ptr);
}
}

tx_semaphore_put(&semaphore_check_done);

nx_secure_tls_session_end(&tls_client_session_0, NX_IP_PERIODIC_RATE);
nx_secure_tls_session_delete(&tls_client_session_0);

nx_tcp_socket_disconnect(&client_socket_0, NX_NO_WAIT);
nx_tcp_client_socket_unbind(&client_socket_0);
nx_tcp_socket_delete(&client_socket_0);

if (error_counter)
{
printf("ERROR!\n");
test_control_return(1);
}
else
{
printf("SUCCESS!\n");
test_control_return(0);
}
}

#else
#ifdef CTEST
void test_application_define(void *first_unused_memory);
void test_application_define(void *first_unused_memory)
#else
VOID nx_secure_tls_1_3_no_newsessionticket_test_application_define(void *first_unused_memory)
#endif
{
printf("NetX Secure Test: TLS 1.3 No NewSessionTicket Test..................N/A\n");
test_control_return(3);
}
#endif