Use alternative string trimming in auth service

Previous string trimming was changing the strings supplied by MQ to be null-terminated.  MQ uses fixed-width strings, and the changes to the data could cause problems in the queue manager.

Author: arthurbarr

authservice/mqhtpass/Makefile

@@ -33,12 +33,18 @@ CFLAGS += -std=gnu11 -fPIC -Wall ${CFLAGS.${ARCH}}
 LIB_APR = -L/usr/lib64 -lapr-1 -laprutil-1
 LIB_MQ = -L/opt/mqm/lib64 -lmqm_r
 
-all: $(BUILD_DIR)/mqhtpass.so $(BUILD_DIR)/htpass_test
+all: $(BUILD_DIR)/mqhtpass.so $(BUILD_DIR)/htpass_test $(BUILD_DIR)/log_test
 
 $(BUILD_DIR)/log.o : $(SRC_DIR)/log.c $(SRC_DIR)/log.h
 	mkdir -p ${dir $@}
 	gcc $(CFLAGS) -c $(SRC_DIR)/log.c -o $@
 
+$(BUILD_DIR)/log_test : $(BUILD_DIR)/log.o
+	mkdir -p ${dir $@}
+	gcc $(CFLAGS) $(SRC_DIR)/log_test.c $^ -o $@
+# Run Logging tests, and print log if they fail	
+	$@ || (cat log_test*.log && exit 1)
+
 $(BUILD_DIR)/htpass.o : $(SRC_DIR)/htpass.c $(SRC_DIR)/htpass.h
 	mkdir -p ${dir $@}
 	gcc $(CFLAGS) -c $(SRC_DIR)/htpass.c -I /usr/include/apr-1 -o $@

authservice/mqhtpass/src/log.c

@@ -1,5 +1,5 @@
 /*
-© Copyright IBM Corporation 2021
+© Copyright IBM Corporation 2021, 2022
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -124,7 +124,7 @@ void log_printf(const char *source_file, int source_line, const char *level, con
     if (strftime(date_buf, sizeof date_buf, "%FT%T", utc))
     {
        // Round microseconds down to milliseconds, for consistency
-       cur += snprintf(cur, end-cur, ", \"ibm_datetime\":\"%s.%03ldZ\"", date_buf, now.tv_usec / 1000);
+       cur += snprintf(cur, end-cur, ", \"ibm_datetime\":\"%s.%03ldZ\"", date_buf, now.tv_usec / (long)1000);
     }
     cur += snprintf(cur, end-cur, ", \"ibm_processId\":\"%d\"", pid);
     cur += snprintf(cur, end-cur, ", \"host\":\"%s\"", hostname);
@@ -146,7 +146,17 @@ void log_printf(const char *source_file, int source_line, const char *level, con
 
     // Important: Just do one file write, to prevent problems with multi-threading.
     // This only works if the log message is not too long for the buffer.
-    fprintf(fp, buf);
+    fprintf(fp, "%s", buf);
   }
 }
 
+int trimmed_len(char *s, int max_len)
+{
+  int i;
+  for (i = max_len - 1; i >= 0; i--)
+  {
+    if (s[i] != ' ')
+      break;
+  }
+  return i+1;
+}

authservice/mqhtpass/src/log.h

@@ -1,5 +1,5 @@
 /*
-© Copyright IBM Corporation 2021
+© Copyright IBM Corporation 2021, 2022
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -59,5 +59,12 @@ void log_close();
  */
 #define log_debugf(format,...) log_printf(__FILE__, __LINE__, "DEBUG", format, ##__VA_ARGS__)
 
+/**
+ * Return the length of the string when trimmed of trailing spaces.
+ * IBM MQ uses fixed length strings, so this function can be used to print
+ * a trimmed version of a string using the "%.*s" printf format string.
+ * For example, `log_printf("%.*s", trimmed_len(fw_str, 48), fw_str)`
+ */
+int trimmed_len(char *s, int);
 
 #endif

authservice/mqhtpass/src/log_test.c

@@ -0,0 +1,120 @@
+/*
+© Copyright IBM Corporation 2022
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include "log.h"
+
+// Headers for multi-threaded tests
+#include <pthread.h>
+
+// Start a test and log the function name
+#define test_start() printf("=== RUN: %s\n", __func__)
+
+// Indicate test has passed
+#define test_pass() printf("--- PASS: %s\n", __func__)
+
+// The length of strings used in the tests
+#define STR_LEN 5
+
+// Indicate test has failed
+void test_fail(const char *test_name)
+{
+  printf("--- FAIL: %s\n", test_name);
+  exit(1);
+}
+
+// Print a fixed-width string in hexadecimal
+void print_hex(char fw_string[STR_LEN])
+{
+  printf("[");
+  for (int i=0; i<STR_LEN; i++)
+  {
+    printf("%02x", fw_string[i]);
+    if (i < STR_LEN-1)
+      printf(",");
+  }
+  printf("]");
+}
+
+// ----------------------------------------------------------------------------
+// Tests for string manipulation
+// ----------------------------------------------------------------------------
+
+void test_trimmed_len(const char *test_name, char fw_string[STR_LEN], int expected_len)
+{
+  printf("=== RUN: %s\n", test_name);
+  int len;
+  // Create a copy of the fixed-width string
+  char fw_string2[STR_LEN];
+  memcpy(fw_string2, fw_string, STR_LEN * sizeof(char));
+  // Call the function under test
+  len = trimmed_len(fw_string, STR_LEN);
+  // Check the result is correct
+  if (len != expected_len)
+  {
+    printf("%s: Expected result to be %d; got %d\n", __func__, expected_len, len);
+    test_fail(test_name);
+  }
+  // Check that the original string has not been changed
+  for (int i=0; i<STR_LEN; i++)
+  {
+    if (fw_string[i] != fw_string2[i])
+    {
+      printf("%c-%c\n", fw_string[i], fw_string2[i]);
+      printf("%s: Expected string to be identical to input hex ", __func__);
+      print_hex(fw_string2);
+      printf("; got hex ");
+      print_hex(fw_string);
+      printf("\n");
+      test_fail(test_name);
+    }
+  }
+  printf("--- PASS: %s\n", test_name);
+}
+
+void test_trimmed_len_normal()
+{
+  char fw_string[STR_LEN] = {'a','b','c',' ',' '};
+  test_trimmed_len(__func__, fw_string, 3);
+}
+
+void test_trimmed_len_full()
+{
+  char fw_string[STR_LEN] = {'a','b','c','d','e'};
+  test_trimmed_len(__func__, fw_string, 5);
+}
+
+void test_trimmed_len_empty()
+{
+  char fw_string[STR_LEN] = {' ',' ',' ',' ',' '};
+  test_trimmed_len(__func__, fw_string, 0);
+}
+
+// ----------------------------------------------------------------------------
+
+int main()
+{
+  // Turn on debugging for the tests
+  setenv("DEBUG", "true", true);
+  log_init("log_test.log");
+  test_trimmed_len_normal();
+  test_trimmed_len_full();
+  test_trimmed_len_empty();
+  log_close();
+}

authservice/mqhtpass/src/mqhtpass.c

@@ -34,8 +34,6 @@ static MQZ_TERM_AUTHORITY mqhtpass_terminate;
 #define HTPASSWD_FILE "/etc/mqm/mq.htpasswd"
 #define NAME "MQ Advanced for Developers custom authentication service"
 
-static char *trim(char *s);
-
 /**
  * Initialization and entrypoint for the dynamically loaded
  * authorization installable service. It registers the addresses of the
@@ -80,7 +78,7 @@ void MQENTRY MQStart(
   {
     log_infof("Initializing %s", NAME);
   }
-  log_debugf("MQStart options=%s qmgr=%s", ((Options == MQZIO_SECONDARY) ? "Secondary" : "Primary"), trim(QMgrName));
+  log_debugf("MQStart options=%s qmgr=%.*s", ((Options == MQZIO_SECONDARY) ? "Secondary" : "Primary"), trimmed_len(QMgrName, MQ_Q_MGR_NAME_LENGTH), QMgrName);
 
   if (!htpass_valid_file(HTPASSWD_FILE))
   {
@@ -176,11 +174,14 @@ static void MQENTRY mqhtpass_authenticate_user_csp(
     // Tell the queue manager to continue trying other authorization services, as they might have the user.
     *pContinuation = MQZCI_CONTINUE;
     log_debugf(
-        "User authentication failed due to invalid user.  user=%s effuser=%s applname=%s csp_user=%s cc=%d reason=%d",
-        trim(pIdentityContext->UserIdentifier),
-        trim(pApplicationContext->EffectiveUserID),
-        trim(pApplicationContext->ApplName),
-        trim(csp_user),
+        "User authentication failed due to invalid user.  user=%.*s effuser=%.*s applname=%.*s csp_user=%s cc=%d reason=%d",
+        trimmed_len(pIdentityContext->UserIdentifier, MQ_USER_ID_LENGTH),
+        pIdentityContext->UserIdentifier,
+        trimmed_len(pApplicationContext->EffectiveUserID, MQ_USER_ID_LENGTH),
+        pApplicationContext->EffectiveUserID,
+        trimmed_len(pApplicationContext->ApplName, MQ_APPL_NAME_LENGTH),
+        pApplicationContext->ApplName,
+        csp_user,
         *pCompCode,
         *pReason);
   }
@@ -192,11 +193,14 @@ static void MQENTRY mqhtpass_authenticate_user_csp(
     // Tell the queue manager to stop trying other authorization services.
     *pContinuation = MQZCI_STOP;
     log_debugf(
-        "User authentication failed due to invalid password.  user=%s effuser=%s applname=%s csp_user=%s cc=%d reason=%d",
-        trim(pIdentityContext->UserIdentifier),
-        trim(pApplicationContext->EffectiveUserID),
-        trim(pApplicationContext->ApplName),
-        trim(csp_user),
+        "User authentication failed due to invalid password.  user=%.*s effuser=%.*s applname=%.*s csp_user=%s cc=%d reason=%d",
+        trimmed_len(pIdentityContext->UserIdentifier, MQ_USER_ID_LENGTH),
+        pIdentityContext->UserIdentifier,
+        trimmed_len(pApplicationContext->EffectiveUserID, MQ_USER_ID_LENGTH),
+        pApplicationContext->EffectiveUserID,
+        trimmed_len(pApplicationContext->ApplName, MQ_APPL_NAME_LENGTH),
+        pApplicationContext->ApplName,
+        csp_user,
         *pCompCode,
         *pReason);
   }
@@ -275,11 +279,14 @@ static void MQENTRY mqhtpass_authenticate_user(
       else
       {
         log_debugf(
-            "User authentication failed user=%s effuser=%s applname=%s cspuser=%s cc=%d reason=%d",
-            trim(pIdentityContext->UserIdentifier),
-            trim(pApplicationContext->EffectiveUserID),
-            trim(pApplicationContext->ApplName),
-            trim(spuser),
+            "User authentication failed user=%.*s effuser=%.*s applname=%.*s cspuser=%s cc=%d reason=%d",
+            trimmed_len(pIdentityContext->UserIdentifier, MQ_USER_ID_LENGTH),
+            pIdentityContext->UserIdentifier,
+            trimmed_len(pApplicationContext->EffectiveUserID, MQ_USER_ID_LENGTH),
+            pApplicationContext->EffectiveUserID,
+            trimmed_len(pApplicationContext->ApplName, MQ_APPL_NAME_LENGTH),
+            pApplicationContext->ApplName,
+            spuser,
             *pCompCode,
             *pReason);
       }
@@ -333,18 +340,3 @@ static void MQENTRY mqhtpass_terminate(
   *pReason = MQRC_NONE;
 }
 
-/**
- * Remove trailing spaces from a string.
- */
-static char *trim(char *s)
-{
-  int i;
-  for (i = strlen(s) - 1; i >= 0; i--)
-  {
-    if (s[i] == ' ')
-      s[i] = 0;
-    else
-      break;
-  }
-  return s;
-}