From b7e48a4d8f58e53f017e048aef221a0c2cf6663b Mon Sep 17 00:00:00 2001 From: Kenneth Giusti Date: Tue, 6 Feb 2024 14:39:18 -0500 Subject: [PATCH] Fixes #1398 - prevent force-close of TCP pseudo-connections via management TCP adaptor creates special connection instances that are used for managing the service address subscriptions. A user must not inadvertantly delete these connections or the router will malfunction. This patch prevents these connections from being force-closed via the management interface. Closes #1398 --- include/qpid/dispatch/protocol_adaptor.h | 4 +- src/adaptors/adaptor_common.c | 34 +++++++++++++---- src/adaptors/adaptor_common.h | 7 ++++ src/adaptors/http1/http1_client.c | 3 +- src/adaptors/http1/http1_server.c | 3 +- src/adaptors/http2/http2_adaptor.c | 35 ++++++++++-------- src/adaptors/reference_adaptor.c | 3 +- src/adaptors/tcp/tcp_adaptor.c | 40 +++++++++++--------- src/adaptors/tcp_lite/tcp_lite.c | 47 +++++++++++++++++------- src/router_core/agent_connection.c | 13 +++---- src/router_core/connections.c | 4 +- src/router_core/router_core_private.h | 1 + src/router_node.c | 10 ++++- 13 files changed, 134 insertions(+), 70 deletions(-) diff --git a/include/qpid/dispatch/protocol_adaptor.h b/include/qpid/dispatch/protocol_adaptor.h index 23bd1f819..1b7313f5e 100644 --- a/include/qpid/dispatch/protocol_adaptor.h +++ b/include/qpid/dispatch/protocol_adaptor.h @@ -868,7 +868,6 @@ void qdr_link_set_drained(qdr_core_t *core, qdr_link_t *link); */ qd_delivery_state_t *qdr_delivery_take_local_delivery_state(qdr_delivery_t *dlv, uint64_t *dispo); - qdr_connection_info_t *qdr_connection_info(bool is_encrypted, bool is_authenticated, bool opened, @@ -884,7 +883,8 @@ qdr_connection_info_t *qdr_connection_info(bool is_encrypted, bool ssl, const char *version, bool streaming_links, - bool connection_trunking); + bool connection_trunking, + bool allow_delete); // T: can be deleted via mgmt agent void qdr_connection_info_set_group_correlator(qdr_connection_info_t *info, const char *correlator); diff --git a/src/adaptors/adaptor_common.c b/src/adaptors/adaptor_common.c index 9510b9251..3671e6f10 100644 --- a/src/adaptors/adaptor_common.c +++ b/src/adaptors/adaptor_common.c @@ -245,18 +245,38 @@ int qd_raw_connection_write_buffers(pn_raw_connection_t *pn_raw_conn, qd_adaptor return num_buffers_written; } -char *qd_raw_conn_get_address(pn_raw_connection_t *pn_raw_conn) + +size_t qd_raw_conn_get_address_buf(pn_raw_connection_t *pn_raw_conn, char *buf, size_t buflen) { + assert(pn_raw_conn); + assert(buflen); + + buf[0] = '\0'; + const pn_netaddr_t *netaddr = pn_raw_connection_remote_addr(pn_raw_conn); - char buffer[1024]; - int len = pn_netaddr_str(netaddr, buffer, 1024); - if (len <= 1024) { - return strdup(buffer); - } else { - return strndup(buffer, 1024); + if (!netaddr) + return 0; + + int len = pn_netaddr_str(netaddr, buf, buflen); + if (len < 0) + return 0; + if (len >= buflen) { // truncated + len = buflen - 1; + buf[len] = '\0'; } + + return (size_t) len; +} + + +char *qd_raw_conn_get_address(pn_raw_connection_t *pn_raw_conn) +{ + char result[1024]; + qd_raw_conn_get_address_buf(pn_raw_conn, result, sizeof(result)); + return strdup(result); } + int qd_raw_connection_drain_write_buffers(pn_raw_connection_t *pn_raw_conn) { pn_raw_buffer_t buffs[RAW_BUFFER_BATCH]; diff --git a/src/adaptors/adaptor_common.h b/src/adaptors/adaptor_common.h index db172db75..911cb312e 100644 --- a/src/adaptors/adaptor_common.h +++ b/src/adaptors/adaptor_common.h @@ -95,6 +95,13 @@ int qd_raw_connection_write_buffers(pn_raw_connection_t *pn_raw_conn, qd_adaptor */ char *qd_raw_conn_get_address(pn_raw_connection_t *pn_raw_conn); +/** + * Get the raw connections remote address. + * Like qd_raw_conn_get_address(), but address buffer is supplied by caller. + * @return number of bytes written, zero if no address available (buf is set to the null string). + */ +size_t qd_raw_conn_get_address_buf(pn_raw_connection_t *pn_raw_conn, char *buf, size_t buflen); + /** * Drains write buffers held by proton raw connection. * @param raw_conn - The pn_raw_connection_t to which the write buffers were granted. diff --git a/src/adaptors/http1/http1_client.c b/src/adaptors/http1/http1_client.c index 830766e2b..606b5f15a 100644 --- a/src/adaptors/http1/http1_client.c +++ b/src/adaptors/http1/http1_client.c @@ -348,7 +348,8 @@ static void _setup_client_connection(qdr_http1_connection_t *hconn) false, //bool ssl, "", // peer router version, false, // streaming links - false); // connection trunking + false, // connection trunking + true); // allow mgmt agent DELETE operation hconn->qdr_conn = qdr_connection_opened(qdr_http1_adaptor->core, qdr_http1_adaptor->adaptor, diff --git a/src/adaptors/http1/http1_server.c b/src/adaptors/http1/http1_server.c index 53587a977..1128f4618 100644 --- a/src/adaptors/http1/http1_server.c +++ b/src/adaptors/http1/http1_server.c @@ -208,7 +208,8 @@ static qdr_http1_connection_t *_create_server_connection(qd_http_connector_t *co false, //bool ssl, "", // peer router version, false, // streaming links - false); // connection trunking + false, // connection trunking + true); // allow mgmt agent DELETE operation hconn->qdr_conn = qdr_connection_opened(qdr_http1_adaptor->core, qdr_http1_adaptor->adaptor, diff --git a/src/adaptors/http2/http2_adaptor.c b/src/adaptors/http2/http2_adaptor.c index 60c275d97..107073025 100644 --- a/src/adaptors/http2/http2_adaptor.c +++ b/src/adaptors/http2/http2_adaptor.c @@ -2760,22 +2760,24 @@ static int handle_incoming_http(qdr_http2_connection_t *conn) qdr_http2_connection_t *qdr_http_connection_ingress_accept(qdr_http2_connection_t* ingress_http_conn) { ingress_http_conn->remote_address = qd_raw_conn_get_address(ingress_http_conn->pn_raw_conn); - qdr_connection_info_t *info = qdr_connection_info(false, //bool is_encrypted, - false, //bool is_authenticated, - true, //bool opened, - "", //char *sasl_mechanisms, + qdr_connection_info_t *info = qdr_connection_info(false, //bool is_encrypted, + false, //bool is_authenticated, + true, //bool opened, + "", //char *sasl_mechanisms, QD_INCOMING, //qd_direction_t dir, ingress_http_conn->remote_address, //const char *host, - "", //const char *ssl_proto, - "", //const char *ssl_cipher, - "", //const char *user, + "", //const char *ssl_proto, + "", //const char *ssl_cipher, + "", //const char *user, "HttpAdaptor", //const char *container, - 0, //pn_data_t *connection_properties, - 0, //int ssl_ssf, - false, //bool ssl, - "", // peer router version, - false, // streaming links - false); // connection trunking + 0, //pn_data_t *connection_properties, + 0, //int ssl_ssf, + false, //bool ssl, + "", // peer router version, + false, // streaming links + false, // connection trunking + true); // allow mgmt agent DELETE operation + qdr_connection_t *conn = qdr_connection_opened(http2_adaptor->core, http2_adaptor->adaptor, @@ -3106,9 +3108,10 @@ qdr_http2_connection_t *qdr_http_connection_egress(qd_http_connector_t *connecto 0, //pn_data_t *connection_properties, 0, //int ssl_ssf, false, //bool ssl, - "", // peer router version, - false, // streaming links - false); // connection trunking + "", // peer router version, + false, // streaming links + false, // connection trunking + true); // allow mgmt agent DELETE operation qdr_connection_t *conn = qdr_connection_opened(http2_adaptor->core, http2_adaptor->adaptor, diff --git a/src/adaptors/reference_adaptor.c b/src/adaptors/reference_adaptor.c index 296bfcbe7..38aa02951 100644 --- a/src/adaptors/reference_adaptor.c +++ b/src/adaptors/reference_adaptor.c @@ -423,7 +423,8 @@ static void on_startup(void *context) false, //bool ssl, "", // peer router version, false, // streaming links - false); // connection trunking + false, // connection trunking + true); // allow mgmt agent DELETE operation adaptor->conn = qdr_connection_opened(adaptor->core, // core adaptor->adaptor, // protocol_adaptor diff --git a/src/adaptors/tcp/tcp_adaptor.c b/src/adaptors/tcp/tcp_adaptor.c index 125a12319..ac3520114 100644 --- a/src/adaptors/tcp/tcp_adaptor.c +++ b/src/adaptors/tcp/tcp_adaptor.c @@ -921,22 +921,23 @@ static void qdr_tcp_connection_ingress_accept(qdr_tcp_connection_t* tc) // So, we need to call pn_data_free(tcp_conn_properties). // pn_data_t *tcp_conn_properties = qdr_tcp_conn_properties(); - qdr_connection_info_t *info = qdr_connection_info(tc->require_tls, // is_encrypted, - false, // is_authenticated, - true, // opened, - "", // *sasl_mechanisms, - QD_INCOMING, // dir, - tc->remote_address, // *host, - "", // *ssl_proto, - "", // *ssl_cipher, - "", // *user, - "TcpAdaptor", // *container, - tcp_conn_properties, // *connection_properties, - 0, // ssl_ssf, - false, // ssl, - "", // peer router version, - false, // streaming links - false); // connection trunking + qdr_connection_info_t *info = qdr_connection_info(tc->require_tls, // is_encrypted, + false, // is_authenticated, + true, // opened, + "", // *sasl_mechanisms, + QD_INCOMING, // dir, + tc->remote_address, // *host, + "", // *ssl_proto, + "", // *ssl_cipher, + "", // *user, + "TcpAdaptor", // *container, + tcp_conn_properties, // *connection_properties, + 0, // ssl_ssf, + false, // ssl, + "", // peer router version, + false, // streaming links + false, // connection trunking + true); // allow mgmt agent DELETE operation pn_data_free(tcp_conn_properties); qdr_connection_t *conn = qdr_connection_opened(tcp_adaptor->core, @@ -1351,6 +1352,10 @@ static void qdr_tcp_create_server_side_connection(qdr_tcp_connection_t* tc) qd_log(LOG_TCP_ADAPTOR, QD_LOG_DEBUG, "[C%" PRIu64 "] Opening server-side core connection %s", tc->conn_id, host); + // ISSUE-1225: cannot allow user to delete tcp dispatch connections via management. The must delete the + // parent tcpConnector instead. + const bool allow_delete = !tc->is_egress_dispatcher_conn; + // // The qdr_connection_info() function makes its own copy of the passed in tcp_conn_properties. // So, we need to call pn_data_free(tcp_conn_properties) @@ -1371,7 +1376,8 @@ static void qdr_tcp_create_server_side_connection(qdr_tcp_connection_t* tc) false, // bool ssl, "", // peer router version, false, // streaming links - false); // connection trunking + false, // connection trunking + allow_delete); pn_data_free(tcp_conn_properties); qdr_connection_t *conn = qdr_connection_opened(tcp_adaptor->core, diff --git a/src/adaptors/tcp_lite/tcp_lite.c b/src/adaptors/tcp_lite/tcp_lite.c index 1663ae25f..d4f2c44fe 100644 --- a/src/adaptors/tcp_lite/tcp_lite.c +++ b/src/adaptors/tcp_lite/tcp_lite.c @@ -179,21 +179,17 @@ static pn_data_t *TL_conn_properties(void) } -static qdr_connection_t *TL_open_core_connection(uint64_t conn_id, bool incoming) +static qdr_connection_t *TL_open_core_connection(uint64_t conn_id, bool incoming, const char *host, bool allow_delete) { qdr_connection_t *conn; - - // - // The qdr_connection_info() function makes its own copy of the passed in tcp_conn_properties. - // So, we need to call pn_data_free(tcp_conn_properties) - // + pn_data_t *properties = TL_conn_properties(); qdr_connection_info_t *info = qdr_connection_info(false, // is_encrypted, false, // is_authenticated, true, // opened, "", // sasl_mechanisms, incoming ? QD_INCOMING : QD_OUTGOING, // dir, - "tcplite", // host, + host, "", // ssl_proto, "", // ssl_cipher, "", // user, @@ -203,7 +199,8 @@ static qdr_connection_t *TL_open_core_connection(uint64_t conn_id, bool incoming false, // ssl, "", // peer router version, true, // streaming links - false); // connection trunking + false, // connection trunking + allow_delete); pn_data_free(properties); conn = qdr_connection_opened(tcplite_context->core, @@ -232,7 +229,7 @@ static void TL_setup_listener(tcplite_listener_t *li) // Set up a core connection to handle all of the links and deliveries for this listener // li->common.conn_id = qd_server_allocate_connection_id(tcplite_context->server); - li->common.core_conn = TL_open_core_connection(li->common.conn_id, true); + li->common.core_conn = TL_open_core_connection(li->common.conn_id, true, "tcplistener", false); qdr_connection_set_context(li->common.core_conn, li); // @@ -272,7 +269,7 @@ static void TL_setup_connector(tcplite_connector_t *cr) // Set up a core connection to handle all of the links and deliveries for this connector // cr->common.conn_id = qd_server_allocate_connection_id(tcplite_context->server); - cr->common.core_conn = TL_open_core_connection(cr->common.conn_id, false); + cr->common.core_conn = TL_open_core_connection(cr->common.conn_id, false, "tcpConnector", false); qdr_connection_set_context(cr->common.core_conn, cr); // @@ -766,11 +763,13 @@ static void link_setup_LSIDE_IO(tcplite_connection_t *conn) tcplite_listener_t *li = (tcplite_listener_t*) conn->common.parent; qdr_terminus_t *target = qdr_terminus(0); qdr_terminus_t *source = qdr_terminus(0); + char host[64]; // for numeric remote client IP:port address qdr_terminus_set_address(target, li->adaptor_config->address); qdr_terminus_set_dynamic(source); - - conn->common.core_conn = TL_open_core_connection(conn->common.conn_id, true); + + qd_raw_conn_get_address_buf(conn->raw_conn, host, sizeof(host)); + conn->common.core_conn = TL_open_core_connection(conn->common.conn_id, true, host, true); qdr_connection_set_context(conn->common.core_conn, conn); conn->inbound_link = qdr_link_first_attach(conn->common.core_conn, QD_INCOMING, qdr_terminus(0), target, "tcp.lside.in", 0, false, 0, &conn->inbound_link_id); @@ -786,10 +785,12 @@ static void link_setup_CSIDE_IO(tcplite_connection_t *conn, qdr_delivery_t *deli { ASSERT_RAW_IO; qdr_terminus_t *target = qdr_terminus(0); + char host[64]; // for numeric remote server IP:port address qdr_terminus_set_address(target, conn->reply_to); - conn->common.core_conn = TL_open_core_connection(conn->common.conn_id, false); + qd_raw_conn_get_address_buf(conn->raw_conn, host, sizeof(host)); + conn->common.core_conn = TL_open_core_connection(conn->common.conn_id, false, host, true); qdr_connection_set_context(conn->common.core_conn, conn); conn->inbound_link = qdr_link_first_attach(conn->common.core_conn, QD_INCOMING, qdr_terminus(0), target, "tcp.cside.in", 0, false, 0, &conn->inbound_link_id); @@ -1736,8 +1737,26 @@ static void CORE_delivery_update(void *context, qdr_delivery_t *dlv, uint64_t di } -static void CORE_connection_close(void *context, qdr_connection_t *conn, qdr_error_t *error) +// Managment agent set connection admin state to deleted. +// +static void CORE_connection_close(void *context, qdr_connection_t *core_conn, qdr_error_t *error) { + tcplite_common_t *common = (tcplite_common_t*) qdr_connection_get_context(core_conn); + if (common && common->context_type == TL_CONNECTION) { + // Ignore requests to delete the pseudo connections associated with connectors/listeners - these can only be + // deleted by deleting the parent. Note we can only call pn_raw_connection_closed() if this is the proper + // proactor thread! + tcplite_connection_t *conn = (tcplite_connection_t *) common; + if (conn->raw_conn && !!(sys_thread_proactor_mode() & SYS_THREAD_PROACTOR_MODE_RAW_CONNECTION) + && conn->raw_conn == (pn_raw_connection_t *) sys_thread_proactor_context()) { + + qd_log(LOG_TCP_ADAPTOR, QD_LOG_DEBUG, "[C%" PRIu64 "] TCP connection force-closed by management", conn->common.conn_id); + if (error) { + qdr_error_copy(error, pn_raw_connection_condition(conn->raw_conn)); + } + pn_raw_connection_close(conn->raw_conn); + } + } } diff --git a/src/router_core/agent_connection.c b/src/router_core/agent_connection.c index c91bbfb86..790464c61 100644 --- a/src/router_core/agent_connection.c +++ b/src/router_core/agent_connection.c @@ -524,12 +524,9 @@ static void qdra_connection_update_set_status(qdr_core_t *core, qdr_query_t *que qd_iterator_t *admin_status_iter = qd_parse_raw(admin_state); if (qd_iterator_equal(admin_status_iter, (unsigned char*) QDR_CONNECTION_ADMIN_STATUS_DELETED)) { - // This connection has been force-closed. There are certain types of connections that cannot be - // force-closed by management because they are essential for the correct operation of the router - if (conn->role != QDR_ROLE_INTER_ROUTER - && conn->role != QDR_ROLE_EDGE_CONNECTION - // ISSUE-1225: cannot delete tcp dispatch connections - && (!conn->connection_info->host || strcmp(conn->connection_info->host, "egress-dispatch"))) { + // This connection has been force-closed. Ensure that only those connections that permit force-close by + // management are closed. + if (conn->connection_info->allow_delete) { qdr_close_connection_CT(core, conn); // This log message shows a connection is being deleted via a management request. qd_log(LOG_AGENT, @@ -542,8 +539,8 @@ static void qdra_connection_update_set_status(qdr_core_t *core, qdr_query_t *que } else { // - // You are trying to delete an inter-router connection and that is always forbidden, no matter what - // policy rights you have. + // You are trying to delete a connection that is required for proper router operation (e.g. an + // inter-router connection). That is always forbidden, no matter what policy rights you have. // query->status = QD_AMQP_FORBIDDEN; query->status.description = "You are not allowed to perform this operation."; diff --git a/src/router_core/connections.c b/src/router_core/connections.c index ef305a607..a55dbf6c6 100644 --- a/src/router_core/connections.c +++ b/src/router_core/connections.c @@ -195,7 +195,8 @@ qdr_connection_info_t *qdr_connection_info(bool is_encrypted, bool ssl, const char *version, bool streaming_links, - bool connection_trunking) + bool connection_trunking, + bool allow_delete) { qdr_connection_info_t *connection_info = new_qdr_connection_info_t(); ZERO(connection_info); @@ -228,6 +229,7 @@ qdr_connection_info_t *qdr_connection_info(bool is_encrypted, connection_info->ssl = ssl; connection_info->streaming_links = streaming_links; connection_info->connection_trunking = connection_trunking; + connection_info->allow_delete = allow_delete; sys_mutex_init(&connection_info->connection_info_lock); return connection_info; } diff --git a/src/router_core/router_core_private.h b/src/router_core/router_core_private.h index 4d822b293..d374c3f25 100644 --- a/src/router_core/router_core_private.h +++ b/src/router_core/router_core_private.h @@ -647,6 +647,7 @@ struct qdr_connection_info_t { bool opened; bool streaming_links; // will allow streaming links bool connection_trunking; // peer supports connection trunking + bool allow_delete; // T: may be deleted via management agent DELETE operation qd_direction_t dir; qdr_connection_role_t role; pn_data_t *connection_properties; diff --git a/src/router_node.c b/src/router_node.c index a0cd55181..8f5daf9e3 100644 --- a/src/router_node.c +++ b/src/router_node.c @@ -1523,7 +1523,12 @@ static void AMQP_opened_handler(qd_router_t *router, qd_connection_t *conn, bool is_ssl = true; } - + // allow_delete: There are certain types of connections that must not be force-closed by user because they are + // essential for the correct operation of the router (see ISSUE-1225). Prevent the mgmt agent from setting the admin + // status to DELETED for these types of connections. TODO(kgiusti): what other connection roles need to be + // protected?? + // + bool allow_delete = role != QDR_ROLE_INTER_ROUTER && role != QDR_ROLE_EDGE_CONNECTION; bool encrypted = tport && pn_transport_is_encrypted(tport); bool authenticated = tport && pn_transport_is_authenticated(tport); @@ -1542,7 +1547,8 @@ static void AMQP_opened_handler(qd_router_t *router, qd_connection_t *conn, bool is_ssl, rversion, streaming_links, - connection_trunking); + connection_trunking, + allow_delete); qdr_connection_info_set_group_correlator(connection_info, conn->group_correlator);