[TACACS] Improve per-command authorization performance by read passwd…

… entry with getpwent (#16460) Improve per-command authorization performance by read passwd entry with getpwent. #### Why I did it Currently per-command authorization will check if user is remote user with getpwnam API, which will trigger tacplus-nss for authentication with TACACS server. But this is not necessary because when user login the user information already add to local passwd file. Use getpwent API can directly read from passwd file, this will improve per-command authorization performance. ##### Work item tracking - Microsoft ADO: 25104723 #### How I did it Improve per-command authorization performance by read passwd entry with getpwent. #### How to verify it Pass all UT. ### Description for the changelog Improve per-command authorization performance by read passwd entry with getpwent.
sonic-net · Oct 14, 2023 · f0d88f3 · f0d88f3
1 parent 072eaed
commit f0d88f3
Show file tree

Hide file tree

Showing 4 changed files with 118 additions and 36 deletions.
diff --git a/src/tacacs/bash_tacplus/bash_tacplus.c b/src/tacacs/bash_tacplus/bash_tacplus.c
@@ -14,8 +14,8 @@
 /* Remote user gecos prefix, which been assigned by nss_tacplus */
 #define REMOTE_USER_GECOS_PREFIX      "remote_user"
 
-/* Default value for _SC_GETPW_R_SIZE_MAX */
-#define DEFAULT_SC_GETPW_R_SIZE_MAX     1024
+/* Default value for getpwent */
+#define DEFAULT_GETPWENT_SIZE_MAX     4096
 
 /* Return value for is_local_user method */
 #define IS_LOCAL_USER              0
@@ -31,6 +31,7 @@
 /* Output syslog to mock method when build with UT */
 #if defined (BASH_PLUGIN_UT)
 #define syslog mock_syslog
+#define getpwent_r mock_getpwent_r
 #endif
 
 /* Tacacs+ log format */
@@ -350,40 +351,39 @@ int is_local_user(char *user)
     }
 
     struct passwd pwd;
-    struct passwd *pwdresult;
-    char *buf;
-    size_t bufsize = sysconf(_SC_GETPW_R_SIZE_MAX);
-    if (bufsize == -1) {
-        bufsize = DEFAULT_SC_GETPW_R_SIZE_MAX;
-    }
+    struct passwd *ppwd;
+    char buf[DEFAULT_GETPWENT_SIZE_MAX];
+    int pwdresult;
+    int result = ERROR_CHECK_LOCAL_USER;
+    setpwent();
+    while (1) {
+        pwdresult = getpwent_r(&pwd, buf, sizeof(buf), &ppwd);
+        if (pwdresult) {
+            // no more pw entry
+            break;
+        }
 
-    buf = malloc(bufsize);
-    if (buf == NULL) {
-       output_error("failed to allocate getpwnam_r buffer.\n");
-       return ERROR_CHECK_LOCAL_USER;
-    }
+        if (strcmp(ppwd->pw_name, user) != 0) {
+            continue;
+        }
 
-    int s = getpwnam_r(user, &pwd, buf, bufsize, &pwdresult);
-    int result = IS_LOCAL_USER;
-    if (pwdresult == NULL) {
-        if (s == 0)
-            output_error("get user information user failed, user: %s not found\n", user);
+        // compare passwd entry, for remote user pw_gecos will start as 'remote_user'
+        if (strncmp(ppwd->pw_gecos, REMOTE_USER_GECOS_PREFIX, strlen(REMOTE_USER_GECOS_PREFIX)) == 0) {
+            output_debug("user: %s, UID: %d, GECOS: %s is remote user.\n", user, ppwd->pw_uid, ppwd->pw_gecos);
+            result = IS_REMOTE_USER;
+        }
         else {
-            output_error("get user information failed, user: %s, errorno: %d\n", user, s);
+            output_debug("user: %s, UID: %d, GECOS: %s is local user.\n", user, ppwd->pw_uid, ppwd->pw_gecos);
+            result = IS_LOCAL_USER;
         }
-
-        result = ERROR_CHECK_LOCAL_USER;
-    }
-    else if (strncmp(pwd.pw_gecos, REMOTE_USER_GECOS_PREFIX, strlen(REMOTE_USER_GECOS_PREFIX)) == 0) {
-        output_debug("user: %s, UID: %d, GECOS: %s is remote user.\n", user, pwd.pw_uid, pwd.pw_gecos);
-        result = IS_REMOTE_USER;
+        break;
     }
-    else {
-        output_debug("user: %s, UID: %d, GECOS: %s is local user.\n", user, pwd.pw_uid, pwd.pw_gecos);
-        result = IS_LOCAL_USER;
+    endpwent();
+
+    if (result == ERROR_CHECK_LOCAL_USER) {
+        output_error("get user information user failed, user: %s not found\n", user);
     }
 
-    free(buf);
     return result;
 }
 
@@ -485,4 +485,4 @@ int on_shell_execve (char *user, int shell_level, char *cmd, char **argv)
     // return 0, so bash will continue run user command and will check user permission with linux permission check.
     output_debug("start local authorization for command %s with given arguments\n", cmd);
     return 0;
-}
+}
diff --git a/src/tacacs/bash_tacplus/unittest/mock_helper.c b/src/tacacs/bash_tacplus/unittest/mock_helper.c
@@ -3,6 +3,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <pwd.h>
 #include <CUnit/CUnit.h>
 #include <CUnit/Basic.h>
 
@@ -206,4 +207,33 @@ void mock_syslog(int priority, const char *format, ...)
   va_end (args);
 
   debug_printf("MOCK: syslog: %s\n", mock_syslog_message_buffer);
+}
+
+int mock_getpwent_r(struct passwd *restrict pwbuf,
+                      char *buf, size_t buflen,
+                      struct passwd **restrict pwbufp)
+{
+	static char* test_user = "test_user";
+	static char* root_user = "root";
+	static char* empty_gecos = "";
+	static char* remote_gecos = "remote_user";
+	*pwbufp = pwbuf;
+	switch (test_scenario)
+	{
+		case TEST_SCEANRIO_CONNECTION_SEND_SUCCESS_RESULT:
+		case TEST_SCEANRIO_CONNECTION_SEND_DENINED_RESULT:
+		case TEST_SCEANRIO_IS_LOCAL_USER_REMOTE:
+			pwbuf->pw_name = test_user;
+			pwbuf->pw_gecos = remote_gecos;
+			pwbuf->pw_uid = 1000;
+			return 0;
+		case TEST_SCEANRIO_IS_LOCAL_USER_ROOT:
+			pwbuf->pw_name = root_user;
+			pwbuf->pw_gecos = empty_gecos;
+			pwbuf->pw_uid = 0;
+			return 0;
+		case TEST_SCEANRIO_IS_LOCAL_USER_NOT_FOUND:
+			return 1;
+	}
+	return 1;
 }
diff --git a/src/tacacs/bash_tacplus/unittest/mock_helper.h b/src/tacacs/bash_tacplus/unittest/mock_helper.h
@@ -24,11 +24,16 @@
 /* Mock syslog buffer */
 extern char mock_syslog_message_buffer[1024];
 
-#define TEST_SCEANRIO_CONNECTION_ALL_FAILED				1
-#define TEST_SCEANRIO_CONNECTION_SEND_FAILED_RESULT			2
-#define TEST_SCEANRIO_CONNECTION_SEND_SUCCESS_READ_FAILED			3
-#define TEST_SCEANRIO_CONNECTION_SEND_DENINED_RESULT			4
-#define TEST_SCEANRIO_CONNECTION_SEND_SUCCESS_RESULT			5
+#define TEST_SCEANRIO_CONNECTION_ALL_FAILED                 1
+#define TEST_SCEANRIO_CONNECTION_SEND_FAILED_RESULT         2
+#define TEST_SCEANRIO_CONNECTION_SEND_SUCCESS_READ_FAILED   3
+#define TEST_SCEANRIO_CONNECTION_SEND_DENINED_RESULT        4
+#define TEST_SCEANRIO_CONNECTION_SEND_SUCCESS_RESULT        5
+#define TEST_SCEANRIO_LOAD_CHANGED_TACACS_CONFIG            6
+#define TEST_SCEANRIO_IS_LOCAL_USER_UNKNOWN                 7
+#define TEST_SCEANRIO_IS_LOCAL_USER_NOT_FOUND               8
+#define TEST_SCEANRIO_IS_LOCAL_USER_ROOT                    9
+#define TEST_SCEANRIO_IS_LOCAL_USER_REMOTE                  10
 
 /* Set test scenario for test*/
 void set_test_scenario(int scenario);

diff --git a/src/tacacs/bash_tacplus/unittest/plugin_test.c b/src/tacacs/bash_tacplus/unittest/plugin_test.c
@@ -5,6 +5,10 @@
 #include "mock_helper.h"
 #include <libtac/support.h>
 
+#define IS_LOCAL_USER              0
+#define IS_REMOTE_USER             1
+#define ERROR_CHECK_LOCAL_USER     2
+
 /* tacacs debug flag */
 extern int tacacs_ctrl;
 
@@ -112,6 +116,8 @@ void testcase_authorization_with_host_and_tty_success() {
 /* Test check_and_load_changed_tacacs_config */
 void testcase_check_and_load_changed_tacacs_config() {
 
+	set_test_scenario(TEST_SCEANRIO_LOAD_CHANGED_TACACS_CONFIG);
+
 	// test connection failed case
 	check_and_load_changed_tacacs_config();
 
@@ -171,6 +177,43 @@ void testcase_on_shell_execve_failed() {
 	CU_ASSERT_STRING_EQUAL(mock_syslog_message_buffer, "test_command not authorized by TACACS+ with given arguments, not executing\n");
 }
 
+/* Test is_local_user unknown user */
+void testcase_is_local_user_unknown() {
+	set_test_scenario(TEST_SCEANRIO_IS_LOCAL_USER_UNKNOWN);
+	int result = is_local_user("UNKNOWN");
+
+    // check unknown user is remote.
+	CU_ASSERT_EQUAL(result, IS_REMOTE_USER);
+}
+
+/* Test is_local_user not found user */
+void testcase_is_local_user_not_found() {
+	set_test_scenario(TEST_SCEANRIO_IS_LOCAL_USER_NOT_FOUND);
+	int result = is_local_user("notexist");
+
+    // check unknown user is remote.
+	CU_ASSERT_EQUAL(result, ERROR_CHECK_LOCAL_USER);
+	CU_ASSERT_STRING_EQUAL(mock_syslog_message_buffer, "get user information user failed, user: notexist not found\n");
+}
+
+/* Test is_local_user root user */
+void testcase_is_local_user_root() {
+	set_test_scenario(TEST_SCEANRIO_IS_LOCAL_USER_ROOT);
+	int result = is_local_user("root");
+
+    // check unknown user is remote.
+	CU_ASSERT_EQUAL(result, IS_LOCAL_USER);
+}
+
+/* Test is_local_user remote user */
+void testcase_is_local_user_remote() {
+	set_test_scenario(TEST_SCEANRIO_IS_LOCAL_USER_REMOTE);
+	int result = is_local_user("test_user");
+
+    // check unknown user is remote.
+	CU_ASSERT_EQUAL(result, IS_REMOTE_USER);
+}
+
 int main(void) {
   if (CUE_SUCCESS != CU_initialize_registry()) {
     return CU_get_error();
@@ -196,7 +239,11 @@ int main(void) {
 	  || !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)
 	  || !CU_add_test(ste, "Test testcase_on_shell_execve_denined()...\n", testcase_on_shell_execve_denined)
-	  || !CU_add_test(ste, "Test testcase_on_shell_execve_failed()...\n", testcase_on_shell_execve_failed)) {
+	  || !CU_add_test(ste, "Test testcase_on_shell_execve_failed()...\n", testcase_on_shell_execve_failed)
+	  || !CU_add_test(ste, "Test testcase_is_local_user_unknown()...\n", testcase_is_local_user_unknown)
+	  || !CU_add_test(ste, "Test testcase_is_local_user_not_found()...\n", testcase_is_local_user_not_found)
+	  || !CU_add_test(ste, "Test testcase_is_local_user_root()...\n", testcase_is_local_user_root)
+	  || !CU_add_test(ste, "Test testcase_is_local_user_remote()...\n", testcase_is_local_user_remote)) {
     CU_cleanup_registry();
     return CU_get_error();
   }