From 196ed3df3568474b2c42a6b7f5e3d353b87c76b6 Mon Sep 17 00:00:00 2001 From: Apoorv Kothari Date: Fri, 7 Jul 2023 14:57:57 -0700 Subject: [PATCH] feedback --- error/s2n_errno.c | 4 ++-- error/s2n_errno.h | 2 +- tests/unit/s2n_ktls_test.c | 10 ++++----- tls/s2n_ktls.c | 44 ++++++++++++++++++++------------------ tls/s2n_ktls.h | 10 ++++----- 5 files changed, 36 insertions(+), 34 deletions(-) diff --git a/error/s2n_errno.c b/error/s2n_errno.c index a80185e859e..b85a00e96e7 100644 --- a/error/s2n_errno.c +++ b/error/s2n_errno.c @@ -290,9 +290,9 @@ static const char *no_such_error = "Internal s2n error"; ERR_ENTRY(S2N_ERR_INTERNAL_LIBCRYPTO_ERROR, "An internal error has occurred in the libcrypto API") \ ERR_ENTRY(S2N_ERR_NO_RENEGOTIATION, "Only secure, server-initiated renegotiation is supported") \ ERR_ENTRY(S2N_ERR_APP_DATA_BLOCKED, "Blocked on application data during handshake") \ - ERR_ENTRY(S2N_ERR_KTLS_ALREADY_ENABLED, "kTLS is already enabled on the socket") \ ERR_ENTRY(S2N_ERR_KTLS_MANAGED_IO, "kTLS cannot be enabled while custom I/O is configured for the connection") \ - ERR_ENTRY(S2N_ERR_KTLS_HANDSHAKE_NOT_COMPLETE, "kTLS can only be enabled once the handshake is completed") \ + ERR_ENTRY(S2N_ERR_KTLS_ALREADY_ENABLED, "kTLS is already enabled on the socket") \ + ERR_ENTRY(S2N_ERR_HANDSHAKE_NOT_COMPLETE, "Operation is only allowed after the handshake is complete") \ ERR_ENTRY(S2N_ERR_KTLS_UNSUPPORTED_PLATFORM, "kTLS is unsupported on this platform") \ ERR_ENTRY(S2N_ERR_KTLS_UNSUPPORTED_CONN, "kTLS is unsupported for this connection") \ ERR_ENTRY(S2N_ERR_KTLS_ULP, "An error occurred when attempting to configure the socket for kTLS. Ensure the 'tls' kernel module is enabled.") \ diff --git a/error/s2n_errno.h b/error/s2n_errno.h index 725b42b2e6b..0e360158442 100644 --- a/error/s2n_errno.h +++ b/error/s2n_errno.h @@ -306,9 +306,9 @@ typedef enum { S2N_ERR_SECRET_SCHEDULE_STATE, S2N_ERR_CERT_OWNERSHIP, S2N_ERR_INTERNAL_LIBCRYPTO_ERROR, + S2N_ERR_HANDSHAKE_NOT_COMPLETE, S2N_ERR_KTLS_ALREADY_ENABLED, S2N_ERR_KTLS_MANAGED_IO, - S2N_ERR_KTLS_HANDSHAKE_NOT_COMPLETE, S2N_ERR_KTLS_UNSUPPORTED_PLATFORM, S2N_ERR_KTLS_UNSUPPORTED_CONN, S2N_ERR_KTLS_ULP, diff --git a/tests/unit/s2n_ktls_test.c b/tests/unit/s2n_ktls_test.c index fcd0e487811..51297f272aa 100644 --- a/tests/unit/s2n_ktls_test.c +++ b/tests/unit/s2n_ktls_test.c @@ -94,8 +94,8 @@ int main(int argc, char **argv) int fd = 1; EXPECT_OK(s2n_test_configure_mock_ktls_connection(server_conn, fd, false)); - EXPECT_FAILURE_WITH_ERRNO(s2n_connection_ktls_enable_send(server_conn), S2N_ERR_KTLS_HANDSHAKE_NOT_COMPLETE); - EXPECT_FAILURE_WITH_ERRNO(s2n_connection_ktls_enable_recv(server_conn), S2N_ERR_KTLS_HANDSHAKE_NOT_COMPLETE); + EXPECT_FAILURE_WITH_ERRNO(s2n_connection_ktls_enable_send(server_conn), S2N_ERR_HANDSHAKE_NOT_COMPLETE); + EXPECT_FAILURE_WITH_ERRNO(s2n_connection_ktls_enable_recv(server_conn), S2N_ERR_HANDSHAKE_NOT_COMPLETE); }; /* s2n_connection_ktls_enable */ @@ -110,7 +110,7 @@ int main(int argc, char **argv) EXPECT_FAILURE_WITH_ERRNO(s2n_connection_ktls_enable_recv(server_conn), S2N_ERR_KTLS_DISABLED_FOR_TEST); }; - /* ktls already enabled */ + /* ktls already enabled is a noop and returns success */ { DEFER_CLEANUP(struct s2n_connection *server_conn = s2n_connection_new(S2N_SERVER), s2n_connection_ptr_free); @@ -118,10 +118,10 @@ int main(int argc, char **argv) EXPECT_OK(s2n_test_configure_mock_ktls_connection(server_conn, fd, true)); server_conn->ktls_send_enabled = true; - EXPECT_FAILURE_WITH_ERRNO(s2n_connection_ktls_enable_send(server_conn), S2N_ERR_KTLS_ALREADY_ENABLED); + EXPECT_SUCCESS(s2n_connection_ktls_enable_send(server_conn)); server_conn->ktls_recv_enabled = true; - EXPECT_FAILURE_WITH_ERRNO(s2n_connection_ktls_enable_recv(server_conn), S2N_ERR_KTLS_ALREADY_ENABLED); + EXPECT_SUCCESS(s2n_connection_ktls_enable_recv(server_conn)); }; /* unsupported protocols */ diff --git a/tls/s2n_ktls.c b/tls/s2n_ktls.c index 6a77eda2b3a..786add2f914 100644 --- a/tls/s2n_ktls.c +++ b/tls/s2n_ktls.c @@ -42,7 +42,7 @@ S2N_RESULT s2n_ktls_validate(struct s2n_connection *conn, s2n_ktls_mode ktls_mod /* kTLS enable should only be called once the handshake has completed. */ if (!is_handshake_complete(conn)) { - RESULT_BAIL(S2N_ERR_KTLS_HANDSHAKE_NOT_COMPLETE); + RESULT_BAIL(S2N_ERR_HANDSHAKE_NOT_COMPLETE); } /* TODO support TLS 1.3 @@ -76,6 +76,9 @@ S2N_RESULT s2n_ktls_validate(struct s2n_connection *conn, s2n_ktls_mode ktls_mod /* The input stuffer should be empty before enabling kTLS. */ RESULT_ENSURE(s2n_stuffer_data_available(&conn->in) == 0, S2N_ERR_RECORD_STUFFER_NEEDS_DRAINING); + break; + default: + RESULT_BAIL(S2N_ERR_SAFETY); break; } @@ -88,15 +91,9 @@ S2N_RESULT s2n_ktls_retrieve_file_descriptor(struct s2n_connection *conn, s2n_kt RESULT_ENSURE_REF(fd); if (ktls_mode == S2N_KTLS_MODE_RECV) { - /* retrieve the receive fd */ - RESULT_ENSURE_REF(conn->recv_io_context); - const struct s2n_socket_read_io_context *peer_socket_ctx = conn->recv_io_context; - *fd = peer_socket_ctx->fd; + RESULT_GUARD_POSIX(s2n_connection_get_read_fd(conn, fd)); } else if (ktls_mode == S2N_KTLS_MODE_SEND) { - /* retrieve the send fd */ - RESULT_ENSURE_REF(conn->send_io_context); - const struct s2n_socket_write_io_context *peer_socket_ctx = conn->send_io_context; - *fd = peer_socket_ctx->fd; + RESULT_GUARD_POSIX(s2n_connection_get_write_fd(conn, fd)); } return S2N_RESULT_OK; @@ -124,14 +121,11 @@ static S2N_RESULT s2n_ktls_configure_socket(struct s2n_connection *conn, s2n_ktl int ret = setsockopt(fd, S2N_SOL_TCP, S2N_TCP_ULP, S2N_TLS_ULP_NAME, S2N_TLS_ULP_NAME_SIZE); if (ret != 0) { - /* EEXIST: https://man7.org/linux/man-pages/man3/errno.3.html - * - * TCP_ULP has already been enabled on the socket so the operation is a noop. - * Since its possible to call this twice, once for TX and once for RX, consider - * the noop a success. */ - if (errno != EEXIST) { - RESULT_BAIL(S2N_ERR_KTLS_ULP); - } + /* https://man7.org/linux/man-pages/man3/errno.3.html + * EEXIST indicates that TCP_ULP has already been enabled on the socket. + * This is a noop and therefore safe to ignore. + */ + RESULT_ENSURE(errno == EEXIST, S2N_ERR_KTLS_ULP); } return S2N_RESULT_OK; @@ -154,8 +148,12 @@ int s2n_connection_ktls_enable_send(struct s2n_connection *conn) } POSIX_GUARD_RESULT(s2n_ktls_validate(conn, S2N_KTLS_MODE_SEND)); - - POSIX_GUARD_RESULT(s2n_ktls_configure_socket(conn, S2N_KTLS_MODE_SEND)); + s2n_result res = s2n_ktls_configure_socket(conn, S2N_KTLS_MODE_SEND); + if (s2n_errno == S2N_ERR_KTLS_ALREADY_ENABLED) { + return S2N_SUCCESS; + } else { + POSIX_GUARD_RESULT(res); + } return S2N_SUCCESS; } @@ -169,8 +167,12 @@ int s2n_connection_ktls_enable_recv(struct s2n_connection *conn) } POSIX_GUARD_RESULT(s2n_ktls_validate(conn, S2N_KTLS_MODE_RECV)); - - POSIX_GUARD_RESULT(s2n_ktls_configure_socket(conn, S2N_KTLS_MODE_RECV)); + s2n_result res = s2n_ktls_configure_socket(conn, S2N_KTLS_MODE_RECV); + if (s2n_errno == S2N_ERR_KTLS_ALREADY_ENABLED) { + return S2N_SUCCESS; + } else { + POSIX_GUARD_RESULT(res); + } return S2N_SUCCESS; } diff --git a/tls/s2n_ktls.h b/tls/s2n_ktls.h index ba2151a7e50..7b521ff1ae9 100644 --- a/tls/s2n_ktls.h +++ b/tls/s2n_ktls.h @@ -18,11 +18,11 @@ #include "tls/s2n_config.h" /* Define headers needed to enable and use kTLS. - * * - * * The inline header definitions are required to compile kTLS specific code. - * * kTLS has been tested on linux. For all other platforms, kTLS is marked as - * * unsuppored, and will return an un-supported error. - * */ + * + * The inline header definitions are required to compile kTLS specific code. + * kTLS has been tested on linux. For all other platforms, kTLS is marked as + * unsupported, and will return an unsupported error. + */ #if defined(__linux__) #define S2N_KTLS_SUPPORTED true #include "tls/s2n_ktls_linux.h"