From f26d2a4eb470667ec0ad20b31f27b200ced33b72 Mon Sep 17 00:00:00 2001 From: Rod Persky <770327+Rod-Persky@users.noreply.github.com> Date: Wed, 3 Jun 2026 16:59:34 +1000 Subject: [PATCH 1/4] [bash_tacplus]: Propagate SSH-supplied TraceId into TACACS+ command authorization Implements the HLD in sonic-net/SONiC#2358. Accepts a client-supplied `TraceId` env var via OpenSSH AcceptEnv, marks it readonly in the user's initial interactive bash, validates the value, and attaches it as a `TraceId=` attribute on per-command TACACS+ authorization requests. Behavior is unchanged when TraceId is absent or invalid. How to verify it: - Build sonic-vs and SSH with `SendEnv TraceId` set to a valid value; confirm authorization requests include the TraceId attribute and that invalid/oversized values are silently dropped. - Run the bash_tacplus unit tests under src/tacacs/bash_tacplus/unittest/. Signed-off-by: Rod Persky <770327+Rod-Persky@users.noreply.github.com> --- build_debian.sh | 4 + files/image_config/bash/bash.bashrc | 5 + src/tacacs/bash_tacplus/bash_tacplus.c | 55 +++++++++++ .../bash_tacplus/unittest/mock_helper.c | 17 ++++ .../bash_tacplus/unittest/mock_helper.h | 7 ++ .../bash_tacplus/unittest/plugin_test.c | 99 +++++++++++++++++++ 6 files changed, 187 insertions(+) diff --git a/build_debian.sh b/build_debian.sh index 680358f8d6c..ca30118edeb 100755 --- a/build_debian.sh +++ b/build_debian.sh @@ -521,6 +521,10 @@ set /files/etc/ssh/sshd_config/#comment[following-sibling::*[1][self::AllowAgent save quit EOF +# Allow TACACS+ authorization to receive a client-supplied trace ID. +if ! sudo grep -Eq '^[[:space:]]*AcceptEnv[[:space:]]+([^#[:space:]]+[[:space:]]+)*TraceId([[:space:]]|$)' "$FILESYSTEM_ROOT/etc/ssh/sshd_config"; then + echo "AcceptEnv TraceId" | sudo tee -a "$FILESYSTEM_ROOT/etc/ssh/sshd_config" > /dev/null +fi # Configure sshd to listen for v4 and v6 connections sudo sed -i 's/^#ListenAddress 0.0.0.0/ListenAddress 0.0.0.0/' $FILESYSTEM_ROOT/etc/ssh/sshd_config sudo sed -i 's/^#ListenAddress ::/ListenAddress ::/' $FILESYSTEM_ROOT/etc/ssh/sshd_config diff --git a/files/image_config/bash/bash.bashrc b/files/image_config/bash/bash.bashrc index 5de4b40c3d6..62275a1de00 100644 --- a/files/image_config/bash/bash.bashrc +++ b/files/image_config/bash/bash.bashrc @@ -6,6 +6,11 @@ # If not running interactively, don't do anything [ -z "$PS1" ] && return +# Best-effort guard for the SSH-supplied TACACS+ TraceId in this shell. +if [ -n "${TraceId+x}" ]; then + readonly TraceId +fi + # check the window size after each command and, if necessary, # update the values of LINES and COLUMNS. shopt -s checkwinsize diff --git a/src/tacacs/bash_tacplus/bash_tacplus.c b/src/tacacs/bash_tacplus/bash_tacplus.c index 7bc4e10ac7c..f5a5fe388a7 100644 --- a/src/tacacs/bash_tacplus/bash_tacplus.c +++ b/src/tacacs/bash_tacplus/bash_tacplus.c @@ -1,4 +1,5 @@ #include +#include #include #include #include @@ -20,6 +21,13 @@ /* Remote IP address size */ #define REMOTE_ADDRESS_SIZE 64 +/* TACACS+ attributes are encoded as key=value strings with a 255 byte limit. */ +#define TACACS_ATTR_MAX_SIZE 255 +#define TRACE_ID_ENV_VARIABLE "TraceId" +#define TRACE_ID_ATTR_NAME "traceid" +#define TRACE_ID_ATTR_PREFIX_SIZE (sizeof(TRACE_ID_ATTR_NAME "=") - 1) +#define TRACE_ID_VALUE_SIZE (TACACS_ATTR_MAX_SIZE - TRACE_ID_ATTR_PREFIX_SIZE + 1) + /* Return value for is_local_user method */ #define IS_LOCAL_USER 0 #define IS_REMOTE_USER 1 @@ -110,6 +118,48 @@ void output_debug(const char *format, ...) syslog(LOG_DEBUG, TACACS_LOG_FORMAT, logBuffer); } +/* + * Check whether a TraceId character is safe to send as a TACACS+ attribute. + */ +int is_valid_trace_id_char(char value) +{ + unsigned char ch = (unsigned char)value; + return isalnum(ch) || ch == '.' || ch == '_' || ch == ':' || ch == '-'; +} + +/* + * Get SSH supplied TraceId for TACACS+ authorization. + */ +int get_trace_id(char *dst, size_t size) +{ + const char *trace_id; + size_t trace_id_len, i; + + if (size == 0) { + return 0; + } + + trace_id = getenv(TRACE_ID_ENV_VARIABLE); + if (trace_id == NULL || trace_id[0] == '\0') { + return 0; + } + + trace_id_len = strlen(trace_id); + if (trace_id_len >= size) { + output_debug("TraceId ignored: value exceeds TACACS+ attribute size limit\n"); + return 0; + } + + for (i = 0; i < trace_id_len; i++) { + if (!is_valid_trace_id_char(trace_id[i])) { + output_debug("TraceId ignored: invalid character at offset %zu\n", i); + return 0; + } + } + + snprintf(dst, size, "%s", trace_id); + return 1; +} /* * Send authorization message. @@ -130,6 +180,7 @@ int send_authorization_message( int retval; struct areply re; int i; + char trace_id[TRACE_ID_VALUE_SIZE]; attr=(struct tac_attrib *)xcalloc(1, sizeof(struct tac_attrib)); @@ -138,6 +189,10 @@ int send_authorization_message( tac_add_attrib(&attr, "protocol", "ssh"); tac_add_attrib(&attr, "service", "shell"); + if (get_trace_id(trace_id, sizeof(trace_id))) { + tac_add_attrib(&attr, TRACE_ID_ATTR_NAME, trace_id); + } + tac_add_attrib(&attr, "cmd", (char*)cmd); for(i=1; i +#include #include #include #include @@ -99,6 +100,99 @@ void testcase_tacacs_authorization_success() { CU_ASSERT_EQUAL(result, 0); } +/* Test send_authorization_message adds TraceId attribute when present */ +void testcase_send_authorization_message_trace_id() { + char *testargv[2]; + testargv[0] = "arg1"; + testargv[1] = "arg2"; + + reset_mock_tac_attrs(); + set_test_scenario(TEST_SCEANRIO_CONNECTION_SEND_SUCCESS_RESULT); + setenv("TraceId", "trace-123:abc.def", 1); + + int result = send_authorization_message(0, "test_user", "tty0", "test_host", 42, "test_command", testargv, 2); + + CU_ASSERT_EQUAL(result, 0); + CU_ASSERT_EQUAL(mock_tac_trace_id_attr_count, 1); + CU_ASSERT_STRING_EQUAL(mock_tac_trace_id_attr_value, "trace-123:abc.def"); + + unsetenv("TraceId"); +} + +/* Test send_authorization_message skips TraceId attribute when not present */ +void testcase_send_authorization_message_without_trace_id() { + char *testargv[2]; + testargv[0] = "arg1"; + testargv[1] = "arg2"; + + reset_mock_tac_attrs(); + set_test_scenario(TEST_SCEANRIO_CONNECTION_SEND_SUCCESS_RESULT); + unsetenv("TraceId"); + + int result = send_authorization_message(0, "test_user", "tty0", "test_host", 42, "test_command", testargv, 2); + + CU_ASSERT_EQUAL(result, 0); + CU_ASSERT_EQUAL(mock_tac_trace_id_attr_count, 0); +} + +/* Test send_authorization_message skips empty TraceId values */ +void testcase_send_authorization_message_empty_trace_id() { + char *testargv[2]; + testargv[0] = "arg1"; + testargv[1] = "arg2"; + + reset_mock_tac_attrs(); + set_test_scenario(TEST_SCEANRIO_CONNECTION_SEND_SUCCESS_RESULT); + setenv("TraceId", "", 1); + + int result = send_authorization_message(0, "test_user", "tty0", "test_host", 42, "test_command", testargv, 2); + + CU_ASSERT_EQUAL(result, 0); + CU_ASSERT_EQUAL(mock_tac_trace_id_attr_count, 0); + + unsetenv("TraceId"); +} + +/* Test send_authorization_message skips unsafe TraceId values */ +void testcase_send_authorization_message_invalid_trace_id() { + char *testargv[2]; + testargv[0] = "arg1"; + testargv[1] = "arg2"; + + reset_mock_tac_attrs(); + set_test_scenario(TEST_SCEANRIO_CONNECTION_SEND_SUCCESS_RESULT); + setenv("TraceId", "trace\n123", 1); + + int result = send_authorization_message(0, "test_user", "tty0", "test_host", 42, "test_command", testargv, 2); + + CU_ASSERT_EQUAL(result, 0); + CU_ASSERT_EQUAL(mock_tac_trace_id_attr_count, 0); + + unsetenv("TraceId"); +} + +/* Test send_authorization_message skips oversized TraceId values */ +void testcase_send_authorization_message_long_trace_id() { + char *testargv[2]; + char trace_id[249]; + testargv[0] = "arg1"; + testargv[1] = "arg2"; + + memset(trace_id, 'a', sizeof(trace_id) - 1); + trace_id[sizeof(trace_id) - 1] = '\0'; + + reset_mock_tac_attrs(); + set_test_scenario(TEST_SCEANRIO_CONNECTION_SEND_SUCCESS_RESULT); + setenv("TraceId", trace_id, 1); + + int result = send_authorization_message(0, "test_user", "tty0", "test_host", 42, "test_command", testargv, 2); + + CU_ASSERT_EQUAL(result, 0); + CU_ASSERT_EQUAL(mock_tac_trace_id_attr_count, 0); + + unsetenv("TraceId"); +} + /* Test authorization_with_host_and_tty get success case */ void testcase_authorization_with_host_and_tty_success() { char *testargv[2]; @@ -235,6 +329,11 @@ int main(void) { || !CU_add_test(ste, "Test testcase_tacacs_authorization_read_failed()...\n", testcase_tacacs_authorization_read_failed) || !CU_add_test(ste, "Test testcase_tacacs_authorization_denined()...\n", testcase_tacacs_authorization_denined) || !CU_add_test(ste, "Test testcase_tacacs_authorization_success()...\n", testcase_tacacs_authorization_success) + || !CU_add_test(ste, "Test testcase_send_authorization_message_trace_id()...\n", testcase_send_authorization_message_trace_id) + || !CU_add_test(ste, "Test testcase_send_authorization_message_without_trace_id()...\n", testcase_send_authorization_message_without_trace_id) + || !CU_add_test(ste, "Test testcase_send_authorization_message_empty_trace_id()...\n", testcase_send_authorization_message_empty_trace_id) + || !CU_add_test(ste, "Test testcase_send_authorization_message_invalid_trace_id()...\n", testcase_send_authorization_message_invalid_trace_id) + || !CU_add_test(ste, "Test testcase_send_authorization_message_long_trace_id()...\n", testcase_send_authorization_message_long_trace_id) || !CU_add_test(ste, "Test testcase_authorization_with_host_and_tty_success()...\n", testcase_authorization_with_host_and_tty_success) || !CU_add_test(ste, "Test testcase_check_and_load_changed_tacacs_config()...\n", testcase_check_and_load_changed_tacacs_config) || !CU_add_test(ste, "Test testcase_on_shell_execve_success()...\n", testcase_on_shell_execve_success) From 612b85c6f37efcc0f38da0835b71286745694852 Mon Sep 17 00:00:00 2001 From: Rod Persky <770327+Rod-Persky@users.noreply.github.com> Date: Fri, 5 Jun 2026 13:05:36 +1000 Subject: [PATCH 2/4] Updates for copilot PR review Signed-off-by: Rod Persky <770327+Rod-Persky@users.noreply.github.com> --- src/tacacs/bash_tacplus/bash_tacplus.c | 4 +- .../bash_tacplus/unittest/mock_helper.c | 4 +- .../bash_tacplus/unittest/plugin_test.c | 62 ++++++++++++++++--- 3 files changed, 58 insertions(+), 12 deletions(-) diff --git a/src/tacacs/bash_tacplus/bash_tacplus.c b/src/tacacs/bash_tacplus/bash_tacplus.c index f5a5fe388a7..4a5dde3c504 100644 --- a/src/tacacs/bash_tacplus/bash_tacplus.c +++ b/src/tacacs/bash_tacplus/bash_tacplus.c @@ -121,7 +121,7 @@ void output_debug(const char *format, ...) /* * Check whether a TraceId character is safe to send as a TACACS+ attribute. */ -int is_valid_trace_id_char(char value) +static int is_valid_trace_id_char(char value) { unsigned char ch = (unsigned char)value; return isalnum(ch) || ch == '.' || ch == '_' || ch == ':' || ch == '-'; @@ -130,7 +130,7 @@ int is_valid_trace_id_char(char value) /* * Get SSH supplied TraceId for TACACS+ authorization. */ -int get_trace_id(char *dst, size_t size) +static int get_trace_id(char *dst, size_t size) { const char *trace_id; size_t trace_id_len, i; diff --git a/src/tacacs/bash_tacplus/unittest/mock_helper.c b/src/tacacs/bash_tacplus/unittest/mock_helper.c index 2979e56360b..39885f90f71 100644 --- a/src/tacacs/bash_tacplus/unittest/mock_helper.c +++ b/src/tacacs/bash_tacplus/unittest/mock_helper.c @@ -12,6 +12,8 @@ #include "mock_helper.h" +#define TRACE_ID_ATTR_NAME "traceid" + // define BASH_PLUGIN_UT_DEBUG to output UT debug message. #if defined (BASH_PLUGIN_UT_DEBUG) #define debug_printf printf @@ -123,7 +125,7 @@ void *xcalloc(size_t count, size_t size) /* Mock tac_free_attrib method */ void tac_add_attrib(struct tac_attrib **attr, char *attrname, char *attrvalue) { - if (strcmp(attrname, "TraceId") == 0) + if (strcmp(attrname, TRACE_ID_ATTR_NAME) == 0) { mock_tac_trace_id_attr_count++; snprintf(mock_tac_trace_id_attr_value, sizeof(mock_tac_trace_id_attr_value), "%s", attrvalue); diff --git a/src/tacacs/bash_tacplus/unittest/plugin_test.c b/src/tacacs/bash_tacplus/unittest/plugin_test.c index 8eb0fd8825f..e4d42c92a6c 100644 --- a/src/tacacs/bash_tacplus/unittest/plugin_test.c +++ b/src/tacacs/bash_tacplus/unittest/plugin_test.c @@ -6,6 +6,8 @@ #include "mock_helper.h" #include +#define TRACE_ID_ENV_VARIABLE "TraceId" + #define IS_LOCAL_USER 0 #define IS_REMOTE_USER 1 #define ERROR_CHECK_LOCAL_USER 2 @@ -23,6 +25,36 @@ int start_up() { return 0; } +typedef struct { + char *value; + int was_set; +} trace_id_env_state_t; + +static void save_trace_id_env(trace_id_env_state_t *state) +{ + const char *value = getenv(TRACE_ID_ENV_VARIABLE); + + state->value = NULL; + state->was_set = value != NULL; + if (value != NULL) { + state->value = strdup(value); + } +} + +static void restore_trace_id_env(trace_id_env_state_t *state) +{ + if (state->was_set) { + setenv(TRACE_ID_ENV_VARIABLE, state->value, 1); + } + else { + unsetenv(TRACE_ID_ENV_VARIABLE); + } + + free(state->value); + state->value = NULL; + state->was_set = 0; +} + /* Test tacacs_authorization all tacacs server connect failed case */ void testcase_tacacs_authorization_all_failed() { char *testargv[2]; @@ -103,12 +135,14 @@ void testcase_tacacs_authorization_success() { /* Test send_authorization_message adds TraceId attribute when present */ void testcase_send_authorization_message_trace_id() { char *testargv[2]; + trace_id_env_state_t trace_id_env; testargv[0] = "arg1"; testargv[1] = "arg2"; + save_trace_id_env(&trace_id_env); reset_mock_tac_attrs(); set_test_scenario(TEST_SCEANRIO_CONNECTION_SEND_SUCCESS_RESULT); - setenv("TraceId", "trace-123:abc.def", 1); + setenv(TRACE_ID_ENV_VARIABLE, "trace-123:abc.def", 1); int result = send_authorization_message(0, "test_user", "tty0", "test_host", 42, "test_command", testargv, 2); @@ -116,64 +150,73 @@ void testcase_send_authorization_message_trace_id() { CU_ASSERT_EQUAL(mock_tac_trace_id_attr_count, 1); CU_ASSERT_STRING_EQUAL(mock_tac_trace_id_attr_value, "trace-123:abc.def"); - unsetenv("TraceId"); + restore_trace_id_env(&trace_id_env); } /* Test send_authorization_message skips TraceId attribute when not present */ void testcase_send_authorization_message_without_trace_id() { char *testargv[2]; + trace_id_env_state_t trace_id_env; testargv[0] = "arg1"; testargv[1] = "arg2"; + save_trace_id_env(&trace_id_env); reset_mock_tac_attrs(); set_test_scenario(TEST_SCEANRIO_CONNECTION_SEND_SUCCESS_RESULT); - unsetenv("TraceId"); + unsetenv(TRACE_ID_ENV_VARIABLE); int result = send_authorization_message(0, "test_user", "tty0", "test_host", 42, "test_command", testargv, 2); CU_ASSERT_EQUAL(result, 0); CU_ASSERT_EQUAL(mock_tac_trace_id_attr_count, 0); + + restore_trace_id_env(&trace_id_env); } /* Test send_authorization_message skips empty TraceId values */ void testcase_send_authorization_message_empty_trace_id() { char *testargv[2]; + trace_id_env_state_t trace_id_env; testargv[0] = "arg1"; testargv[1] = "arg2"; + save_trace_id_env(&trace_id_env); reset_mock_tac_attrs(); set_test_scenario(TEST_SCEANRIO_CONNECTION_SEND_SUCCESS_RESULT); - setenv("TraceId", "", 1); + setenv(TRACE_ID_ENV_VARIABLE, "", 1); int result = send_authorization_message(0, "test_user", "tty0", "test_host", 42, "test_command", testargv, 2); CU_ASSERT_EQUAL(result, 0); CU_ASSERT_EQUAL(mock_tac_trace_id_attr_count, 0); - unsetenv("TraceId"); + restore_trace_id_env(&trace_id_env); } /* Test send_authorization_message skips unsafe TraceId values */ void testcase_send_authorization_message_invalid_trace_id() { char *testargv[2]; + trace_id_env_state_t trace_id_env; testargv[0] = "arg1"; testargv[1] = "arg2"; + save_trace_id_env(&trace_id_env); reset_mock_tac_attrs(); set_test_scenario(TEST_SCEANRIO_CONNECTION_SEND_SUCCESS_RESULT); - setenv("TraceId", "trace\n123", 1); + setenv(TRACE_ID_ENV_VARIABLE, "trace\n123", 1); int result = send_authorization_message(0, "test_user", "tty0", "test_host", 42, "test_command", testargv, 2); CU_ASSERT_EQUAL(result, 0); CU_ASSERT_EQUAL(mock_tac_trace_id_attr_count, 0); - unsetenv("TraceId"); + restore_trace_id_env(&trace_id_env); } /* Test send_authorization_message skips oversized TraceId values */ void testcase_send_authorization_message_long_trace_id() { char *testargv[2]; + trace_id_env_state_t trace_id_env; char trace_id[249]; testargv[0] = "arg1"; testargv[1] = "arg2"; @@ -181,16 +224,17 @@ void testcase_send_authorization_message_long_trace_id() { memset(trace_id, 'a', sizeof(trace_id) - 1); trace_id[sizeof(trace_id) - 1] = '\0'; + save_trace_id_env(&trace_id_env); reset_mock_tac_attrs(); set_test_scenario(TEST_SCEANRIO_CONNECTION_SEND_SUCCESS_RESULT); - setenv("TraceId", trace_id, 1); + setenv(TRACE_ID_ENV_VARIABLE, trace_id, 1); int result = send_authorization_message(0, "test_user", "tty0", "test_host", 42, "test_command", testargv, 2); CU_ASSERT_EQUAL(result, 0); CU_ASSERT_EQUAL(mock_tac_trace_id_attr_count, 0); - unsetenv("TraceId"); + restore_trace_id_env(&trace_id_env); } /* Test authorization_with_host_and_tty get success case */ From 01688967dd760b5c92fb957d8797e754e79d76e3 Mon Sep 17 00:00:00 2001 From: Rod Persky <770327+Rod-Persky@users.noreply.github.com> Date: Thu, 9 Jul 2026 11:59:51 +1000 Subject: [PATCH 3/4] [feature] update SSH_CLIENT_TRACEID support for TACACS+ authorization Signed-off-by: Rod Persky <770327+Rod-Persky@users.noreply.github.com> --- build_debian.sh | 4 +- files/image_config/bash/bash.bashrc | 6 +-- .../yang_model_tests/tests_config/tacacs.json | 3 +- .../yang-models/sonic-system-tacacs.yang | 6 +++ src/tacacs/bash_tacplus/bash_tacplus.c | 14 ++++--- .../bash_tacplus/unittest/plugin_test.c | 41 ++++++++++++++++++- ...dd-traceid-authorization-config-flag.patch | 37 +++++++++++++++++ src/tacacs/pam/Makefile | 1 + 8 files changed, 100 insertions(+), 12 deletions(-) create mode 100644 src/tacacs/pam/0011-Add-traceid-authorization-config-flag.patch diff --git a/build_debian.sh b/build_debian.sh index b6b85a24ac7..cbe5f026fdf 100755 --- a/build_debian.sh +++ b/build_debian.sh @@ -519,8 +519,8 @@ save quit EOF # Allow TACACS+ authorization to receive a client-supplied trace ID. -if ! sudo grep -Eq '^[[:space:]]*AcceptEnv[[:space:]]+([^#[:space:]]+[[:space:]]+)*TraceId([[:space:]]|$)' "$FILESYSTEM_ROOT/etc/ssh/sshd_config"; then - echo "AcceptEnv TraceId" | sudo tee -a "$FILESYSTEM_ROOT/etc/ssh/sshd_config" > /dev/null +if ! sudo grep -Eq '^[[:space:]]*AcceptEnv[[:space:]]+([^#[:space:]]+[[:space:]]+)*SSH_CLIENT_TRACEID([[:space:]]|$)' "$FILESYSTEM_ROOT/etc/ssh/sshd_config"; then + echo "AcceptEnv SSH_CLIENT_TRACEID" | sudo tee -a "$FILESYSTEM_ROOT/etc/ssh/sshd_config" > /dev/null fi # Configure sshd to listen for v4 and v6 connections sudo sed -i 's/^#ListenAddress 0.0.0.0/ListenAddress 0.0.0.0/' $FILESYSTEM_ROOT/etc/ssh/sshd_config diff --git a/files/image_config/bash/bash.bashrc b/files/image_config/bash/bash.bashrc index 62275a1de00..550cd0e0b83 100644 --- a/files/image_config/bash/bash.bashrc +++ b/files/image_config/bash/bash.bashrc @@ -6,9 +6,9 @@ # If not running interactively, don't do anything [ -z "$PS1" ] && return -# Best-effort guard for the SSH-supplied TACACS+ TraceId in this shell. -if [ -n "${TraceId+x}" ]; then - readonly TraceId +# Best-effort guard for the SSH-supplied TACACS+ trace ID in this shell. +if [ -n "${SSH_CLIENT_TRACEID+x}" ]; then + readonly SSH_CLIENT_TRACEID fi # check the window size after each command and, if necessary, diff --git a/src/sonic-yang-models/tests/yang_model_tests/tests_config/tacacs.json b/src/sonic-yang-models/tests/yang_model_tests/tests_config/tacacs.json index 29081312159..8e79d145124 100644 --- a/src/sonic-yang-models/tests/yang_model_tests/tests_config/tacacs.json +++ b/src/sonic-yang-models/tests/yang_model_tests/tests_config/tacacs.json @@ -22,7 +22,8 @@ "auth_type": "chap", "timeout": 5, "passkey": "dellsonic", - "src_intf": "Ethernet0" + "src_intf": "Ethernet0", + "traceid_authorization": true } } } diff --git a/src/sonic-yang-models/yang-models/sonic-system-tacacs.yang b/src/sonic-yang-models/yang-models/sonic-system-tacacs.yang index 1bf3306ca2c..3fa8fb6b88f 100644 --- a/src/sonic-yang-models/yang-models/sonic-system-tacacs.yang +++ b/src/sonic-yang-models/yang-models/sonic-system-tacacs.yang @@ -185,6 +185,12 @@ module sonic-system-tacacs { } description "Source interface whose IP address is used for outgoing TACACS+ packets."; } + + leaf traceid_authorization { + type boolean; + default true; + description "Enable adding validated SSH TraceId values to TACACS+ command authorization requests."; + } } } } diff --git a/src/tacacs/bash_tacplus/bash_tacplus.c b/src/tacacs/bash_tacplus/bash_tacplus.c index 4a5dde3c504..9ec5d518873 100644 --- a/src/tacacs/bash_tacplus/bash_tacplus.c +++ b/src/tacacs/bash_tacplus/bash_tacplus.c @@ -23,7 +23,7 @@ /* TACACS+ attributes are encoded as key=value strings with a 255 byte limit. */ #define TACACS_ATTR_MAX_SIZE 255 -#define TRACE_ID_ENV_VARIABLE "TraceId" +#define TRACE_ID_ENV_VARIABLE "SSH_CLIENT_TRACEID" #define TRACE_ID_ATTR_NAME "traceid" #define TRACE_ID_ATTR_PREFIX_SIZE (sizeof(TRACE_ID_ATTR_NAME "=") - 1) #define TRACE_ID_VALUE_SIZE (TACACS_ATTR_MAX_SIZE - TRACE_ID_ATTR_PREFIX_SIZE + 1) @@ -128,7 +128,7 @@ static int is_valid_trace_id_char(char value) } /* - * Get SSH supplied TraceId for TACACS+ authorization. + * Get SSH supplied trace ID for TACACS+ authorization. */ static int get_trace_id(char *dst, size_t size) { @@ -146,13 +146,13 @@ static int get_trace_id(char *dst, size_t size) trace_id_len = strlen(trace_id); if (trace_id_len >= size) { - output_debug("TraceId ignored: value exceeds TACACS+ attribute size limit\n"); + output_debug("SSH_CLIENT_TRACEID ignored: value exceeds TACACS+ attribute size limit\n"); return 0; } for (i = 0; i < trace_id_len; i++) { if (!is_valid_trace_id_char(trace_id[i])) { - output_debug("TraceId ignored: invalid character at offset %zu\n", i); + output_debug("SSH_CLIENT_TRACEID ignored: invalid character at offset %zu\n", i); return 0; } } @@ -189,7 +189,7 @@ int send_authorization_message( tac_add_attrib(&attr, "protocol", "ssh"); tac_add_attrib(&attr, "service", "shell"); - if (get_trace_id(trace_id, sizeof(trace_id))) { + if ((tacacs_ctrl & TRACE_ID_AUTHORIZATION_FLAG) && get_trace_id(trace_id, sizeof(trace_id))) { tac_add_attrib(&attr, TRACE_ID_ATTR_NAME, trace_id); } @@ -402,6 +402,10 @@ void load_tacacs_config() output_debug("Local per-command authorization enabled.\n"); } + if (tacacs_ctrl & TRACE_ID_AUTHORIZATION_FLAG) { + output_debug("TACACS+ TraceId authorization attribute enabled.\n"); + } + if (tacacs_ctrl & PAM_TAC_DEBUG) { output_debug("TACACS+ debug enabled.\n"); } diff --git a/src/tacacs/bash_tacplus/unittest/plugin_test.c b/src/tacacs/bash_tacplus/unittest/plugin_test.c index e4d42c92a6c..212ff24a87c 100644 --- a/src/tacacs/bash_tacplus/unittest/plugin_test.c +++ b/src/tacacs/bash_tacplus/unittest/plugin_test.c @@ -6,7 +6,7 @@ #include "mock_helper.h" #include -#define TRACE_ID_ENV_VARIABLE "TraceId" +#define TRACE_ID_ENV_VARIABLE "SSH_CLIENT_TRACEID" #define IS_LOCAL_USER 0 #define IS_REMOTE_USER 1 @@ -136,12 +136,14 @@ void testcase_tacacs_authorization_success() { void testcase_send_authorization_message_trace_id() { char *testargv[2]; trace_id_env_state_t trace_id_env; + int saved_tacacs_ctrl = tacacs_ctrl; testargv[0] = "arg1"; testargv[1] = "arg2"; save_trace_id_env(&trace_id_env); reset_mock_tac_attrs(); set_test_scenario(TEST_SCEANRIO_CONNECTION_SEND_SUCCESS_RESULT); + tacacs_ctrl = PAM_TAC_DEBUG | TRACE_ID_AUTHORIZATION_FLAG; setenv(TRACE_ID_ENV_VARIABLE, "trace-123:abc.def", 1); int result = send_authorization_message(0, "test_user", "tty0", "test_host", 42, "test_command", testargv, 2); @@ -150,6 +152,30 @@ void testcase_send_authorization_message_trace_id() { CU_ASSERT_EQUAL(mock_tac_trace_id_attr_count, 1); CU_ASSERT_STRING_EQUAL(mock_tac_trace_id_attr_value, "trace-123:abc.def"); + tacacs_ctrl = saved_tacacs_ctrl; + restore_trace_id_env(&trace_id_env); +} + +/* Test send_authorization_message skips TraceId when traceid authorization is disabled */ +void testcase_send_authorization_message_trace_id_disabled() { + char *testargv[2]; + trace_id_env_state_t trace_id_env; + int saved_tacacs_ctrl = tacacs_ctrl; + testargv[0] = "arg1"; + testargv[1] = "arg2"; + + save_trace_id_env(&trace_id_env); + reset_mock_tac_attrs(); + set_test_scenario(TEST_SCEANRIO_CONNECTION_SEND_SUCCESS_RESULT); + tacacs_ctrl = PAM_TAC_DEBUG; + setenv(TRACE_ID_ENV_VARIABLE, "trace-123:abc.def", 1); + + int result = send_authorization_message(0, "test_user", "tty0", "test_host", 42, "test_command", testargv, 2); + + CU_ASSERT_EQUAL(result, 0); + CU_ASSERT_EQUAL(mock_tac_trace_id_attr_count, 0); + + tacacs_ctrl = saved_tacacs_ctrl; restore_trace_id_env(&trace_id_env); } @@ -157,12 +183,14 @@ void testcase_send_authorization_message_trace_id() { void testcase_send_authorization_message_without_trace_id() { char *testargv[2]; trace_id_env_state_t trace_id_env; + int saved_tacacs_ctrl = tacacs_ctrl; testargv[0] = "arg1"; testargv[1] = "arg2"; save_trace_id_env(&trace_id_env); reset_mock_tac_attrs(); set_test_scenario(TEST_SCEANRIO_CONNECTION_SEND_SUCCESS_RESULT); + tacacs_ctrl = PAM_TAC_DEBUG | TRACE_ID_AUTHORIZATION_FLAG; unsetenv(TRACE_ID_ENV_VARIABLE); int result = send_authorization_message(0, "test_user", "tty0", "test_host", 42, "test_command", testargv, 2); @@ -170,6 +198,7 @@ void testcase_send_authorization_message_without_trace_id() { CU_ASSERT_EQUAL(result, 0); CU_ASSERT_EQUAL(mock_tac_trace_id_attr_count, 0); + tacacs_ctrl = saved_tacacs_ctrl; restore_trace_id_env(&trace_id_env); } @@ -177,12 +206,14 @@ void testcase_send_authorization_message_without_trace_id() { void testcase_send_authorization_message_empty_trace_id() { char *testargv[2]; trace_id_env_state_t trace_id_env; + int saved_tacacs_ctrl = tacacs_ctrl; testargv[0] = "arg1"; testargv[1] = "arg2"; save_trace_id_env(&trace_id_env); reset_mock_tac_attrs(); set_test_scenario(TEST_SCEANRIO_CONNECTION_SEND_SUCCESS_RESULT); + tacacs_ctrl = PAM_TAC_DEBUG | TRACE_ID_AUTHORIZATION_FLAG; setenv(TRACE_ID_ENV_VARIABLE, "", 1); int result = send_authorization_message(0, "test_user", "tty0", "test_host", 42, "test_command", testargv, 2); @@ -190,6 +221,7 @@ void testcase_send_authorization_message_empty_trace_id() { CU_ASSERT_EQUAL(result, 0); CU_ASSERT_EQUAL(mock_tac_trace_id_attr_count, 0); + tacacs_ctrl = saved_tacacs_ctrl; restore_trace_id_env(&trace_id_env); } @@ -197,12 +229,14 @@ void testcase_send_authorization_message_empty_trace_id() { void testcase_send_authorization_message_invalid_trace_id() { char *testargv[2]; trace_id_env_state_t trace_id_env; + int saved_tacacs_ctrl = tacacs_ctrl; testargv[0] = "arg1"; testargv[1] = "arg2"; save_trace_id_env(&trace_id_env); reset_mock_tac_attrs(); set_test_scenario(TEST_SCEANRIO_CONNECTION_SEND_SUCCESS_RESULT); + tacacs_ctrl = PAM_TAC_DEBUG | TRACE_ID_AUTHORIZATION_FLAG; setenv(TRACE_ID_ENV_VARIABLE, "trace\n123", 1); int result = send_authorization_message(0, "test_user", "tty0", "test_host", 42, "test_command", testargv, 2); @@ -210,6 +244,7 @@ void testcase_send_authorization_message_invalid_trace_id() { CU_ASSERT_EQUAL(result, 0); CU_ASSERT_EQUAL(mock_tac_trace_id_attr_count, 0); + tacacs_ctrl = saved_tacacs_ctrl; restore_trace_id_env(&trace_id_env); } @@ -217,6 +252,7 @@ void testcase_send_authorization_message_invalid_trace_id() { void testcase_send_authorization_message_long_trace_id() { char *testargv[2]; trace_id_env_state_t trace_id_env; + int saved_tacacs_ctrl = tacacs_ctrl; char trace_id[249]; testargv[0] = "arg1"; testargv[1] = "arg2"; @@ -227,6 +263,7 @@ void testcase_send_authorization_message_long_trace_id() { save_trace_id_env(&trace_id_env); reset_mock_tac_attrs(); set_test_scenario(TEST_SCEANRIO_CONNECTION_SEND_SUCCESS_RESULT); + tacacs_ctrl = PAM_TAC_DEBUG | TRACE_ID_AUTHORIZATION_FLAG; setenv(TRACE_ID_ENV_VARIABLE, trace_id, 1); int result = send_authorization_message(0, "test_user", "tty0", "test_host", 42, "test_command", testargv, 2); @@ -234,6 +271,7 @@ void testcase_send_authorization_message_long_trace_id() { CU_ASSERT_EQUAL(result, 0); CU_ASSERT_EQUAL(mock_tac_trace_id_attr_count, 0); + tacacs_ctrl = saved_tacacs_ctrl; restore_trace_id_env(&trace_id_env); } @@ -374,6 +412,7 @@ int main(void) { || !CU_add_test(ste, "Test testcase_tacacs_authorization_denined()...\n", testcase_tacacs_authorization_denined) || !CU_add_test(ste, "Test testcase_tacacs_authorization_success()...\n", testcase_tacacs_authorization_success) || !CU_add_test(ste, "Test testcase_send_authorization_message_trace_id()...\n", testcase_send_authorization_message_trace_id) + || !CU_add_test(ste, "Test testcase_send_authorization_message_trace_id_disabled()...\n", testcase_send_authorization_message_trace_id_disabled) || !CU_add_test(ste, "Test testcase_send_authorization_message_without_trace_id()...\n", testcase_send_authorization_message_without_trace_id) || !CU_add_test(ste, "Test testcase_send_authorization_message_empty_trace_id()...\n", testcase_send_authorization_message_empty_trace_id) || !CU_add_test(ste, "Test testcase_send_authorization_message_invalid_trace_id()...\n", testcase_send_authorization_message_invalid_trace_id) diff --git a/src/tacacs/pam/0011-Add-traceid-authorization-config-flag.patch b/src/tacacs/pam/0011-Add-traceid-authorization-config-flag.patch new file mode 100644 index 00000000000..7f235b52c53 --- /dev/null +++ b/src/tacacs/pam/0011-Add-traceid-authorization-config-flag.patch @@ -0,0 +1,37 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Rod Persky +Date: Thu, 9 Jul 2026 00:00:00 +0000 +Subject: [PATCH] Add TraceId authorization config flag + +--- + support.c | 2 ++ + support.h | 1 + + 2 files changed, 3 insertions(+) + +diff --git a/support.c b/support.c +index 81f3466..0000000 100644 +--- a/support.c ++++ b/support.c +@@ -414,6 +414,8 @@ int _pam_parse_arg (const char *arg, char* current_secret, uint current_secret_b + ctrl |= AUTHORIZATION_FLAG_LOCAL; + } else if (!strcmp (arg, "tacacs_authorization")) { + ctrl |= AUTHORIZATION_FLAG_TACACS; ++ } else if (!strcmp (arg, "traceid_authorization")) { ++ ctrl |= TRACE_ID_AUTHORIZATION_FLAG; + } else { + _pam_log (LOG_WARNING, "unrecognized option: %s", arg); + } +diff --git a/support.h b/support.h +index 1989530..0000000 100644 +--- a/support.h ++++ b/support.h +@@ -43,6 +43,7 @@ + /* authorization setting flag */ + #define AUTHORIZATION_FLAG_LOCAL 0x40 + #define AUTHORIZATION_FLAG_TACACS 0x80 ++#define TRACE_ID_AUTHORIZATION_FLAG 0x100 + + typedef struct { + struct addrinfo *addr; +-- +2.17.1.windows.2 diff --git a/src/tacacs/pam/Makefile b/src/tacacs/pam/Makefile index 411856d8e55..8b57285562e 100644 --- a/src/tacacs/pam/Makefile +++ b/src/tacacs/pam/Makefile @@ -26,6 +26,7 @@ $(addprefix $(DEST)/, $(MAIN_TARGET)): $(DEST)/% : git apply ../0008-Extract-tacacs-support-functions-into-library.patch git apply ../0009-Add-setting-flag-for-authorization-and-accounting.patch git apply ../0010-handle-bad-password-set-by-sshd.patch + git apply ../0011-Add-traceid-authorization-config-flag.patch ifeq ($(CROSS_BUILD_ENVIRON), y) dpkg-buildpackage -rfakeroot -b -us -uc -a$(CONFIGURED_ARCH) -Pcross,nocheck -j$(SONIC_CONFIG_MAKE_JOBS) --admindir $(SONIC_DPKG_ADMINDIR) From 865a8b90b28463c2cb5adb2c3a895e15098862d2 Mon Sep 17 00:00:00 2001 From: Rod Persky <770327+Rod-Persky@users.noreply.github.com> Date: Fri, 10 Jul 2026 17:32:49 +1000 Subject: [PATCH 4/4] Update from copilot review, and added support for | trace id from dotnet Signed-off-by: Rod Persky <770327+Rod-Persky@users.noreply.github.com> --- src/tacacs/bash_tacplus/bash_tacplus.c | 19 ++++++++++++------- .../bash_tacplus/unittest/plugin_test.c | 11 ++++++----- 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/src/tacacs/bash_tacplus/bash_tacplus.c b/src/tacacs/bash_tacplus/bash_tacplus.c index 9ec5d518873..8737388a9e3 100644 --- a/src/tacacs/bash_tacplus/bash_tacplus.c +++ b/src/tacacs/bash_tacplus/bash_tacplus.c @@ -1,5 +1,4 @@ #include -#include #include #include #include @@ -123,8 +122,11 @@ void output_debug(const char *format, ...) */ static int is_valid_trace_id_char(char value) { - unsigned char ch = (unsigned char)value; - return isalnum(ch) || ch == '.' || ch == '_' || ch == ':' || ch == '-'; + return (value >= 'A' && value <= 'Z') || + (value >= 'a' && value <= 'z') || + (value >= '0' && value <= '9') || + value == '.' || value == '_' || value == ':' || + value == '-' || value == '|'; } /* @@ -133,6 +135,7 @@ static int is_valid_trace_id_char(char value) static int get_trace_id(char *dst, size_t size) { const char *trace_id; + const char *trace_id_end; size_t trace_id_len, i; if (size == 0) { @@ -144,15 +147,17 @@ static int get_trace_id(char *dst, size_t size) return 0; } - trace_id_len = strlen(trace_id); - if (trace_id_len >= size) { - output_debug("SSH_CLIENT_TRACEID ignored: value exceeds TACACS+ attribute size limit\n"); + trace_id_end = memchr(trace_id, '\0', size); + if (trace_id_end == NULL) { + output_debug("%s ignored: value exceeds TACACS+ attribute size limit\n", TRACE_ID_ENV_VARIABLE); return 0; } + trace_id_len = trace_id_end - trace_id; + for (i = 0; i < trace_id_len; i++) { if (!is_valid_trace_id_char(trace_id[i])) { - output_debug("SSH_CLIENT_TRACEID ignored: invalid character at offset %zu\n", i); + output_debug("%s ignored: invalid character at offset %zu\n", TRACE_ID_ENV_VARIABLE, i); return 0; } } diff --git a/src/tacacs/bash_tacplus/unittest/plugin_test.c b/src/tacacs/bash_tacplus/unittest/plugin_test.c index 212ff24a87c..9fed17069e9 100644 --- a/src/tacacs/bash_tacplus/unittest/plugin_test.c +++ b/src/tacacs/bash_tacplus/unittest/plugin_test.c @@ -35,15 +35,16 @@ static void save_trace_id_env(trace_id_env_state_t *state) const char *value = getenv(TRACE_ID_ENV_VARIABLE); state->value = NULL; - state->was_set = value != NULL; - if (value != NULL) { + state->was_set = 0; + if (value != NULL) { state->value = strdup(value); + state->was_set = state->value != NULL; } } static void restore_trace_id_env(trace_id_env_state_t *state) { - if (state->was_set) { + if (state->was_set && state->value != NULL) { setenv(TRACE_ID_ENV_VARIABLE, state->value, 1); } else { @@ -144,13 +145,13 @@ void testcase_send_authorization_message_trace_id() { reset_mock_tac_attrs(); set_test_scenario(TEST_SCEANRIO_CONNECTION_SEND_SUCCESS_RESULT); tacacs_ctrl = PAM_TAC_DEBUG | TRACE_ID_AUTHORIZATION_FLAG; - setenv(TRACE_ID_ENV_VARIABLE, "trace-123:abc.def", 1); + setenv(TRACE_ID_ENV_VARIABLE, "|trace-123:abc.def.", 1); int result = send_authorization_message(0, "test_user", "tty0", "test_host", 42, "test_command", testargv, 2); CU_ASSERT_EQUAL(result, 0); CU_ASSERT_EQUAL(mock_tac_trace_id_attr_count, 1); - CU_ASSERT_STRING_EQUAL(mock_tac_trace_id_attr_value, "trace-123:abc.def"); + CU_ASSERT_STRING_EQUAL(mock_tac_trace_id_attr_value, "|trace-123:abc.def."); tacacs_ctrl = saved_tacacs_ctrl; restore_trace_id_env(&trace_id_env);