From b1aaa60699ad6510625b16540179badd5b10f763 Mon Sep 17 00:00:00 2001 From: Garrett D'Amore Date: Sun, 14 Apr 2024 16:33:38 -0700 Subject: [PATCH] fixes #1588 TLS should log errors This isn't complete, but it should go much further in assisting debugging TLS related errors. --- src/supplemental/tls/mbedtls/tls.c | 40 +++++++++++++++++++++- src/supplemental/tls/tls_common.c | 55 ++++++++++++++++-------------- src/supplemental/tls/tls_test.c | 52 ++++++++++++++++++++-------- src/testing/certs.c | 23 +++++++++++++ src/testing/nuts.h | 1 + tests/tls.c | 19 ++++++----- 6 files changed, 140 insertions(+), 50 deletions(-) diff --git a/src/supplemental/tls/mbedtls/tls.c b/src/supplemental/tls/mbedtls/tls.c index 03fd231bc..8f09558a3 100644 --- a/src/supplemental/tls/mbedtls/tls.c +++ b/src/supplemental/tls/mbedtls/tls.c @@ -17,6 +17,7 @@ #include "mbedtls/version.h" // Must be first in order to pick up version #include "mbedtls/error.h" +#include "nng/nng.h" // mbedTLS renamed this header for 2.4.0. #if MBEDTLS_VERSION_MAJOR > 2 || MBEDTLS_VERSION_MINOR >= 4 @@ -99,6 +100,22 @@ tls_random(void *arg, unsigned char *buf, size_t sz) #endif } +static void +tls_log_err(const char *msgid, const char *context, int errnum) +{ + char errbuf[256]; + mbedtls_strerror(errnum, errbuf, sizeof(errbuf)); + nng_log_err(msgid, "%s: %s", context, errbuf); +} + +static void +tls_log_warn(const char *msgid, const char *context, int errnum) +{ + char errbuf[256]; + mbedtls_strerror(errnum, errbuf, sizeof(errbuf)); + nng_log_warn(msgid, "%s: %s", context, errbuf); +} + // tls_mk_err converts an mbed error to an NNG error. static struct { int tls; @@ -193,6 +210,8 @@ conn_init(nng_tls_engine_conn *ec, void *tls, nng_tls_engine_config *cfg) mbedtls_ssl_set_bio(&ec->ctx, tls, net_send, net_recv, NULL); if ((rv = mbedtls_ssl_setup(&ec->ctx, &cfg->cfg_ctx)) != 0) { + tls_log_warn( + "NNG-TLS-CONN-FAIL", "Failed to setup TLS connection", rv); return (tls_mk_err(rv)); } @@ -266,6 +285,7 @@ conn_handshake(nng_tls_engine_conn *ec) return (0); default: + tls_log_warn("NNG-TLS-HANDSHAKE", "TLS handshake failed", rv); return (tls_mk_err(rv)); } } @@ -393,8 +413,10 @@ config_init(nng_tls_engine_config *cfg, enum nng_tls_mode mode) rv = mbedtls_ssl_config_defaults(&cfg->cfg_ctx, ssl_mode, MBEDTLS_SSL_TRANSPORT_STREAM, MBEDTLS_SSL_PRESET_DEFAULT); if (rv != 0) { + tls_log_err("NNG-TLS-CONFIG-INIT-FAIL", + "Failed to initialize TLS configuration", rv); config_fini(cfg); - return (rv); + return (tls_mk_err(rv)); } mbedtls_ssl_conf_authmode(&cfg->cfg_ctx, auth_mode); @@ -461,12 +483,16 @@ config_ca_chain(nng_tls_engine_config *cfg, const char *certs, const char *crl) pem = (const uint8_t *) certs; len = strlen(certs) + 1; if ((rv = mbedtls_x509_crt_parse(&cfg->ca_certs, pem, len)) != 0) { + tls_log_err("NNG-TLS-CA-FAIL", + "Failed to parse CA certificate(s)", rv); return (tls_mk_err(rv)); } if (crl != NULL) { pem = (const uint8_t *) crl; len = strlen(crl) + 1; if ((rv = mbedtls_x509_crl_parse(&cfg->crl, pem, len)) != 0) { + tls_log_err("NNG-TLS-CRL-FAIL", + "Failed to parse revocation list", rv); return (tls_mk_err(rv)); } } @@ -493,6 +519,8 @@ config_own_cert(nng_tls_engine_config *cfg, const char *cert, const char *key, pem = (const uint8_t *) cert; len = strlen(cert) + 1; if ((rv = mbedtls_x509_crt_parse(&p->crt, pem, len)) != 0) { + tls_log_err("NNG-TLS-CRT-FAIL", + "Failure parsing our own certificate", rv); rv = tls_mk_err(rv); goto err; } @@ -507,12 +535,15 @@ config_own_cert(nng_tls_engine_config *cfg, const char *cert, const char *key, pass != NULL ? strlen(pass) : 0, tls_random, NULL); #endif if (rv != 0) { + tls_log_err("NNG-TLS-KEY", "Failure parsing private key", rv); rv = tls_mk_err(rv); goto err; } rv = mbedtls_ssl_conf_own_cert(&cfg->cfg_ctx, &p->crt, &p->key); if (rv != 0) { + tls_log_err("NNG-TLS-SELF", + "Failure configuring self certificate", rv); rv = tls_mk_err(rv); goto err; } @@ -536,6 +567,8 @@ config_version(nng_tls_engine_config *cfg, nng_tls_version min_ver, int maj = MBEDTLS_SSL_MAJOR_VERSION_3; if (min_ver > max_ver) { + nng_log_err("TLS-CFG-VER", + "TLS maximum version must be larger than mimumum version"); return (NNG_ENOTSUP); } switch (min_ver) { @@ -553,6 +586,8 @@ config_version(nng_tls_engine_config *cfg, nng_tls_version min_ver, v1 = MBEDTLS_SSL_MINOR_VERSION_3; break; default: + nng_log_err( + "TLS-CFG-VER", "TLS minimum version not supported"); return (NNG_ENOTSUP); } @@ -575,6 +610,8 @@ config_version(nng_tls_engine_config *cfg, nng_tls_version min_ver, // Note that this means that if we ever TLS 1.4 or 2.0, // then this will break. That's sufficiently far out // to justify not worrying about it. + nng_log_err( + "TLS-CFG-VER", "TLS maximum version not supported"); return (NNG_ENOTSUP); } @@ -629,6 +666,7 @@ nng_tls_engine_init_mbed(void) mbedtls_ctr_drbg_init(&cfg->rng_ctx); rv = mbedtls_ctr_drbg_seed(&rng_ctx, tls_get_entropy, NULL, NULL, 0); if (rv != 0) { + tls_log_err("NNG-TLS-RNG", "Failed initializing CTR DRBG", rv); nni_mtx_fini(&rng_lock); return (rv); } diff --git a/src/supplemental/tls/tls_common.c b/src/supplemental/tls/tls_common.c index 55fed7dc8..fea91e22a 100644 --- a/src/supplemental/tls/tls_common.c +++ b/src/supplemental/tls/tls_common.c @@ -43,7 +43,7 @@ static nni_atomic_ptr tls_engine; struct nng_tls_config { nng_tls_engine_config_ops ops; - const nng_tls_engine * engine; // store this so we can verify + const nng_tls_engine *engine; // store this so we can verify nni_mtx lock; int ref; int busy; @@ -55,21 +55,21 @@ struct nng_tls_config { typedef struct { nng_stream stream; nng_tls_engine_conn_ops ops; - nng_tls_config * cfg; - const nng_tls_engine * engine; + nng_tls_config *cfg; + const nng_tls_engine *engine; size_t size; - nni_aio * user_aio; // user's aio for connect/accept + nni_aio *user_aio; // user's aio for connect/accept nni_aio conn_aio; // system aio for connect/accept nni_mtx lock; bool closed; bool hs_done; nni_list send_queue; nni_list recv_queue; - nng_stream * tcp; // lower level stream + nng_stream *tcp; // lower level stream nni_aio tcp_send; // lower level send pending nni_aio tcp_recv; // lower level recv pending - uint8_t * tcp_send_buf; - uint8_t * tcp_recv_buf; + uint8_t *tcp_send_buf; + uint8_t *tcp_recv_buf; size_t tcp_recv_len; size_t tcp_recv_off; bool tcp_recv_pend; @@ -101,7 +101,7 @@ static nni_reap_list tls_conn_reap_list = { typedef struct { nng_stream_dialer ops; nng_stream_dialer *d; // underlying TCP dialer - nng_tls_config * cfg; + nng_tls_config *cfg; nni_mtx lk; // protects the config } tls_dialer; @@ -130,7 +130,7 @@ tls_dialer_free(void *arg) static void tls_conn_cb(void *arg) { - tls_conn * conn = arg; + tls_conn *conn = arg; nng_stream *tcp; int rv; @@ -171,7 +171,7 @@ tls_dialer_dial(void *arg, nng_aio *aio) { tls_dialer *d = arg; int rv; - tls_conn * conn; + tls_conn *conn; if (nni_aio_begin(aio) != 0) { return; @@ -212,7 +212,7 @@ tls_dialer_set_config(void *arg, const void *buf, size_t sz, nni_type t) { int rv; nng_tls_config *cfg; - tls_dialer * d = arg; + tls_dialer *d = arg; nng_tls_config *old; if ((rv = nni_copyin_ptr((void **) &cfg, buf, sz, t)) != 0) { @@ -235,7 +235,7 @@ tls_dialer_set_config(void *arg, const void *buf, size_t sz, nni_type t) static int tls_dialer_get_config(void *arg, void *buf, size_t *szp, nni_type t) { - tls_dialer * d = arg; + tls_dialer *d = arg; nng_tls_config *cfg; int rv; nni_mtx_lock(&d->lk); @@ -409,7 +409,7 @@ nni_tls_dialer_alloc(nng_stream_dialer **dp, const nng_url *url) typedef struct { nng_stream_listener ops; nng_stream_listener *l; - nng_tls_config * cfg; + nng_tls_config *cfg; nni_mtx lk; } tls_listener; @@ -445,7 +445,7 @@ tls_listener_accept(void *arg, nng_aio *aio) { tls_listener *l = arg; int rv; - tls_conn * conn; + tls_conn *conn; if (nni_aio_begin(aio) != 0) { return; @@ -469,7 +469,7 @@ tls_listener_set_config(void *arg, const void *buf, size_t sz, nni_type t) { int rv; nng_tls_config *cfg; - tls_listener * l = arg; + tls_listener *l = arg; nng_tls_config *old; if ((rv = nni_copyin_ptr((void **) &cfg, buf, sz, t)) != 0) { @@ -494,7 +494,7 @@ tls_listener_set_config(void *arg, const void *buf, size_t sz, nni_type t) static int tls_listener_get_config(void *arg, void *buf, size_t *szp, nni_type t) { - tls_listener * l = arg; + tls_listener *l = arg; nng_tls_config *cfg; int rv; nni_mtx_lock(&l->lk); @@ -809,7 +809,7 @@ static const nni_option tls_options[] = { static int tls_set(void *arg, const char *name, const void *buf, size_t sz, nni_type t) { - tls_conn * conn = arg; + tls_conn *conn = arg; int rv; nng_stream *tcp; @@ -837,7 +837,7 @@ tls_get(void *arg, const char *name, void *buf, size_t *szp, nni_type t) static int tls_alloc(tls_conn **conn_p, nng_tls_config *cfg, nng_aio *user_aio) { - tls_conn * conn; + tls_conn *conn; const nng_tls_engine *eng; size_t size; @@ -1066,7 +1066,7 @@ static void tls_tcp_send_cb(void *arg) { tls_conn *conn = arg; - nng_aio * aio = &conn->tcp_send; + nng_aio *aio = &conn->tcp_send; int rv; size_t count; @@ -1098,7 +1098,7 @@ static void tls_tcp_recv_cb(void *arg) { tls_conn *conn = arg; - nni_aio * aio = &conn->tcp_recv; + nni_aio *aio = &conn->tcp_recv; int rv; nni_mtx_lock(&conn->lock); @@ -1273,9 +1273,9 @@ nng_tls_config_cert_key_file( nng_tls_config *cfg, const char *path, const char *pass) { int rv; - void * data; + void *data; size_t size; - char * pem; + char *pem; if ((rv = nni_file_get(path, &data, &size)) != 0) { return (rv); @@ -1295,9 +1295,9 @@ int nng_tls_config_ca_file(nng_tls_config *cfg, const char *path) { int rv; - void * data; + void *data; size_t size; - char * pem; + char *pem; if ((rv = nni_file_get(path, &data, &size)) != 0) { return (rv); @@ -1397,7 +1397,7 @@ nng_tls_config_auth_mode(nng_tls_config *cfg, nng_tls_auth_mode mode) int nng_tls_config_alloc(nng_tls_config **cfg_p, nng_tls_mode mode) { - nng_tls_config * cfg; + nng_tls_config *cfg; const nng_tls_engine *eng; size_t size; int rv; @@ -1496,9 +1496,12 @@ int nng_tls_engine_register(const nng_tls_engine *engine) { if (engine->version != NNG_TLS_ENGINE_VERSION) { + nng_log_err("NNG-TLS-ENGINE-VER", + "TLS Engine version mismatch: %d != %d", engine->version, + NNG_TLS_ENGINE_VERSION); return (NNG_ENOTSUP); } - nni_atomic_set_ptr(&tls_engine, (void *)engine); + nni_atomic_set_ptr(&tls_engine, (void *) engine); return (0); } diff --git a/src/supplemental/tls/tls_test.c b/src/supplemental/tls/tls_test.c index 96d95fb63..4a3f6e2d2 100644 --- a/src/supplemental/tls/tls_test.c +++ b/src/supplemental/tls/tls_test.c @@ -1,5 +1,5 @@ // -// Copyright 2020 Staysail Systems, Inc. +// Copyright 2024 Staysail Systems, Inc. // // This software is supplied under the terms of the MIT License, a // copy of which should be located in the distribution where this @@ -7,6 +7,7 @@ // found online at https://opensource.org/licenses/MIT. // +#include "nng/nng.h" #include void @@ -14,6 +15,7 @@ test_tls_config_version(void) { nng_tls_config *cfg; + NUTS_ENABLE_LOG(NNG_LOG_INFO); NUTS_PASS(nng_tls_config_alloc(&cfg, NNG_TLS_MODE_SERVER)); // Verify that min ver < max ver @@ -50,8 +52,9 @@ void test_tls_conn_refused(void) { nng_stream_dialer *dialer; - nng_aio * aio; + nng_aio *aio; + NUTS_ENABLE_LOG(NNG_LOG_INFO); NUTS_PASS(nng_aio_alloc(&aio, NULL, NULL)); nng_aio_set_timeout(aio, 5000); // 5 sec @@ -69,20 +72,21 @@ void test_tls_large_message(void) { nng_stream_listener *l; - nng_stream_dialer * d; - nng_aio * aio1, *aio2; - nng_stream * s1; - nng_stream * s2; - nng_tls_config * c1; - nng_tls_config * c2; + nng_stream_dialer *d; + nng_aio *aio1, *aio2; + nng_stream *s1; + nng_stream *s2; + nng_tls_config *c1; + nng_tls_config *c2; char addr[32]; - uint8_t * buf1; - uint8_t * buf2; + uint8_t *buf1; + uint8_t *buf2; size_t size = 450001; - void * t1; - void * t2; + void *t1; + void *t2; int port; + NUTS_ENABLE_LOG(NNG_LOG_INFO); // allocate messages NUTS_ASSERT((buf1 = nng_alloc(size)) != NULL); NUTS_ASSERT((buf2 = nng_alloc(size)) != NULL); @@ -108,7 +112,7 @@ test_tls_large_message(void) NUTS_TRUE(port > 0); NUTS_TRUE(port < 65536); - snprintf(addr, sizeof (addr), "tls+tcp://127.0.0.1:%d", port); + snprintf(addr, sizeof(addr), "tls+tcp://127.0.0.1:%d", port); NUTS_PASS(nng_stream_dialer_alloc(&d, addr)); NUTS_PASS(nng_tls_config_alloc(&c2, NNG_TLS_MODE_CLIENT)); NUTS_PASS(nng_tls_config_ca_chain(c2, nuts_server_crt, NULL)); @@ -147,9 +151,29 @@ test_tls_large_message(void) nng_aio_free(aio2); } +void +test_tls_garbled_cert(void) +{ + nng_stream_listener *l; + nng_tls_config *c1; + + NUTS_ENABLE_LOG(NNG_LOG_INFO); + + // Allocate the listener first. We use a wild-card port. + NUTS_PASS(nng_stream_listener_alloc(&l, "tls+tcp://127.0.0.1:0")); + NUTS_PASS(nng_tls_config_alloc(&c1, NNG_TLS_MODE_SERVER)); + NUTS_FAIL(nng_tls_config_own_cert( + c1, nuts_garbled_crt, nuts_server_key, NULL), + NNG_ECRYPTO); + + nng_stream_listener_free(l); + nng_tls_config_free(c1); +} + TEST_LIST = { { "tls config version", test_tls_config_version }, { "tls conn refused", test_tls_conn_refused }, { "tls large message", test_tls_large_message }, + { "tls garbled cert", test_tls_garbled_cert }, { NULL, NULL }, -}; \ No newline at end of file +}; diff --git a/src/testing/certs.c b/src/testing/certs.c index e2ed94a0c..5ad44e338 100644 --- a/src/testing/certs.c +++ b/src/testing/certs.c @@ -124,3 +124,26 @@ const char *nuts_client_crt = "sxUMa5kT+zc17q57ZcgNq/sSGI3BU4b/E/8ntIwiui2xWSf/4JR6xtanih8uY5Pu\n" "QTgg9qTtFgtu4WWUP7JhreoINTw6O4/g5Z18\n" "-----END CERTIFICATE-----\n"; + +const char *nuts_garbled_crt = + "-----BEGIN CERTIFICATE-----\n" + "MIIDdzCCAl8CFEzqJgxMn+OTdw7RjLtz8FlhrQ0HMA0GCSqGSIb3DQEBCwUAMHcx\n" + "CzAJBgNVBAYTAlhYMQ8wDQYDVQQIDAZVdG9waWExETAPBgNVBAcMCFBhcmFkaXNl\n" + "MRgwFgYDVQQKDA9OTkcgVGVzdHMsIEluYy4xFDASBgNVBAsMC0NsaWVudCBDZXJ0\n" + "MRQwEgYDVQQDDAtUZXN0IENsaWVudDAgFw0yMDA1MjMxODQ1MjZaGA8yMTIwMDQy\n" + "8884NDUyNlowdzELMAkGA1UEBhMCWFgxDzANBgNVBAgMBlV0b3BpYTERMA8GA1UE\n" + "BwwIUGFyYWRpc2UxGDAWBgNVBAoMD05ORyBUZXN0cywgSW5jLjEUMBIGA1UECwwL\n" + "Q2xpZW50IENlcnQxFDASBgNVBAMMC1Rlc3QgQ2xpZW50MIIBIjANBgkqhkiG9w0B\n" + "AQEFAAOCAQ8AMIIBCgKCAQEAoHWEJXvfaHDM33AyYbJHggKOllgcvwscEnsXztIt\n" + "OK+0jO6SRFSbtye1cjtrkGVCYBjeWMcOdEiNB0pw3PceVpF/Q9ifCuaSYsJA3sPH\n" + "wi/A3G7ZTe2KCH1i26I4zyw1Bn5AzkaDDXsaht2S9PEqIBCbWo/V1pWiv4QdYmLT\n" + "/UFYJDxFpFC3iKVC+BDv9yzziyaFXOYsQJXcaq8ZRD79bNV5NFfzUih8RoasIdD4\n" + "LoamBSbbr5XzstTISus+wu1JDKgKkYMJhLGA/tdU/eOKuTDx89yO4ba23W74xeqW\n" + "JYe0wPy+krmeB5M7UA7jIvg1JXhYACxujhieMp7wcC3FPwIDAQABMA0GCSqGSIb3\n" + "DQEBCwUAA4IBAQCMTQ89YnD19bCGIdUl/z6w2yx1x1kvTYHT+SzhUprsgiuS3KT1\n" + "RZNhjf5U3Yu+B6SrJCLuylv+L2zQfmHogp3lV7bayOA7r/rVy5fdmHS+Ei1w6LDL\n" + "t8jayiRMPG4VCgaG486yI73PFpK5DXnyFqSd23TlWvNoNeVag5gjlhzG+mHZBSB2\n" + "ExpGY3SPxrKSzDqIITVPVgzjW25N8qtgLXC6HODDiViNYq1nmuoS4O80NIYAPPs6\n" + "sxUMa5kT+zc17q57ZcgNq/sSGI3BU4b/E/8ntIwiui2xWSf/4JR6xtanih8uY5Pu\n" + "QTgg9qTtFgtu4WWUP7JhreoINTw6O4/g5Z18\n" + "-----END CERTIFICATE-----\n"; diff --git a/src/testing/nuts.h b/src/testing/nuts.h index 6321b005d..f24f71369 100644 --- a/src/testing/nuts.h +++ b/src/testing/nuts.h @@ -101,6 +101,7 @@ extern const char *nuts_server_key; extern const char *nuts_server_crt; extern const char *nuts_client_key; extern const char *nuts_client_crt; +extern const char *nuts_garbled_crt; // NUTS_SUCCESS tests for NNG success. It reports the failure if it // did not. diff --git a/tests/tls.c b/tests/tls.c index 545d2da26..c3a4a46bf 100644 --- a/tests/tls.c +++ b/tests/tls.c @@ -275,6 +275,9 @@ init_listener_tls_file(nng_listener l) TestMain("TLS Transport", { static trantest tt; + nng_log_set_logger(nng_stderr_logger); + nng_log_set_level(NNG_LOG_INFO); + if (strcmp(nng_tls_engine_name(), "none") == 0) { Skip("TLS not enabled"); } @@ -332,7 +335,7 @@ TestMain("TLS Transport", { nng_socket s2; nng_listener l; nng_dialer d; - char * addr; + char *addr; So(nng_tls_register() == 0); So(nng_pair_open(&s1) == 0); @@ -454,7 +457,7 @@ TestMain("TLS Transport", { nng_socket s2; // client nng_listener l; char addr[NNG_MAXADDRLEN]; - nng_msg * msg; + nng_msg *msg; nng_pipe p; bool b; nng_dialer d; @@ -500,7 +503,7 @@ TestMain("TLS Transport", { nng_listener l; nng_dialer d; char addr[NNG_MAXADDRLEN]; - nng_msg * msg; + nng_msg *msg; nng_pipe p; bool b; @@ -576,8 +579,8 @@ TestMain("TLS Transport", { So(nng_listener_set_int(l, NNG_OPT_TCP_NODELAY, x) == NNG_EBADTYPE); // This assumes sizeof (bool) != sizeof (int) - So(nng_listener_set( - l, NNG_OPT_TCP_NODELAY, &x, sizeof(x)) == NNG_EINVAL); + So(nng_listener_set(l, NNG_OPT_TCP_NODELAY, &x, sizeof(x)) == + NNG_EINVAL); nng_dialer_close(d); nng_listener_close(l); @@ -607,8 +610,7 @@ TestMain("TLS Transport", { So(nng_dialer_create(&d, s, "tcp://127.0.0.1:4999") == 0); So(nng_dialer_get_bool(d, NNG_OPT_TCP_KEEPALIVE, &v) == 0); So(v == false); - So(nng_dialer_set_bool(d, NNG_OPT_TCP_KEEPALIVE, true) == - 0); + So(nng_dialer_set_bool(d, NNG_OPT_TCP_KEEPALIVE, true) == 0); So(nng_dialer_get_bool(d, NNG_OPT_TCP_KEEPALIVE, &v) == 0); So(v == true); So(nng_dialer_get_int(d, NNG_OPT_TCP_KEEPALIVE, &x) == @@ -618,8 +620,7 @@ TestMain("TLS Transport", { NNG_EBADTYPE); So(nng_listener_create(&l, s, "tcp://127.0.0.1:4999") == 0); - So(nng_listener_get_bool(l, NNG_OPT_TCP_KEEPALIVE, &v) == - 0); + So(nng_listener_get_bool(l, NNG_OPT_TCP_KEEPALIVE, &v) == 0); So(v == false); x = 1; So(nng_listener_set_int(l, NNG_OPT_TCP_KEEPALIVE, x) ==