Improvements to htpasswd code following review

Improved multi-threading, including new test
This commit is contained in:
Arthur Barr
2020-11-18 10:05:03 +00:00
committed by Arthur J Barr
parent 5fd9fc5e26
commit 4257f6a199
7 changed files with 262 additions and 210 deletions

View File

@@ -133,8 +133,7 @@ RUN mkdir /opt/mqm \
&& chown -R 1001:root /opt/mqm/*
COPY authservice/ /opt/app-root/src/authservice/
WORKDIR /opt/app-root/src/authservice/mqhtpass
RUN make mqhtpass.so
RUN make htpass_test
RUN make all
###############################################################################
# Add default developer config
@@ -157,7 +156,7 @@ LABEL io.k8s.description="Simplify, accelerate and facilitate the reliable excha
LABEL base-image=$BASE_IMAGE
LABEL base-image-release=$BASE_TAG
USER 0
COPY --from=cbuilder /opt/app-root/src/authservice/mqhtpass/mqhtpass.so /opt/mqm/lib64/
COPY --from=cbuilder /opt/app-root/src/authservice/mqhtpass/build/mqhtpass.so /opt/mqm/lib64/
COPY etc/mqm/*.ini /etc/mqm/
COPY etc/mqm/mq.htpasswd /etc/mqm/
RUN chmod 0660 /etc/mqm/mq.htpasswd

View File

@@ -12,31 +12,39 @@
# See the License for the specific language governing permissions and
# limitations under the License.
# Where to find the sample test.
# This Makefile expects the following to be installed:
# - gcc
# - ldd
# - MQ SDK (mqm_r library, plus header files)
# - Apache Portable Runtime (apr-1 and aprutil-1 libraries, plus header files)
SRC_DIR = src
BUILD_DIR = ./build
# Flags passed to the C compiler.
CFLAGS += -std=c11 -fPIC -Wall
# Flags passed to the C compiler. Need to use gnu11 to get POSIX functions needed for file locking.
CFLAGS += -std=gnu11 -fPIC -Wall -m64
log.o : $(SRC_DIR)/log.c $(SRC_DIR)/log.h
gcc $(CFLAGS) -c $(SRC_DIR)/log.c
LIB_APR = -L/usr/lib64 -lapr-1 -laprutil-1
LIB_MQ = -L/opt/mqm/lib64 -lmqm_r
htpass.o : $(SRC_DIR)/htpass.c $(SRC_DIR)/htpass.h
gcc $(CFLAGS) -c -I /usr/include/apr-1 $^
all: $(BUILD_DIR)/mqhtpass.so $(BUILD_DIR)/htpass_test
htpass_test.o : htpass.o log.o
gcc $(CFLAGS) -I /usr/include/apr-1 -L/usr/lib64 -lapr-1 -laprutil-1 $(SRC_DIR)/htpass_test.c $^ -o htpass_test
$(BUILD_DIR)/log.o : $(SRC_DIR)/log.c $(SRC_DIR)/log.h
mkdir -p ${dir $@}
gcc $(CFLAGS) -c $(SRC_DIR)/log.c -o $@
htpass_test : htpass_test.o
./htpass_test
$(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 $@
mqhtpass.so : log.o htpass.o
gcc $(CFLAGS) -I/opt/mqm/inc -D_REENTRANT -L/usr/lib64 -lapr-1 -laprutil-1 -L/opt/mqm/lib64 -lmqm_r -Wl,-rpath,/opt/mqm/lib64 -Wl,-rpath,/usr/lib64 -shared $(SRC_DIR)/mqhtpass.c $^ -o $@
$(BUILD_DIR)/htpass_test : $(BUILD_DIR)/htpass.o $(BUILD_DIR)/log.o
mkdir -p ${dir $@}
gcc $(CFLAGS) $(LIB_APR) -lpthread $(SRC_DIR)/htpass_test.c $^ -o $@
# Run HTPasswd tests, and print log if they fail
$@ || (cat htpass_test*.log && exit 1)
$(BUILD_DIR)/mqhtpass.so : $(BUILD_DIR)/log.o $(BUILD_DIR)/htpass.o
mkdir -p ${dir $@}
# NOTE: rpath for libapr will be different on Ubuntu
gcc $(CFLAGS) -I/opt/mqm/inc -D_REENTRANT $(LIB_APR) $(LIB_MQ) -Wl,-rpath,/opt/mqm/lib64 -Wl,-rpath,/usr/lib64 -shared $(SRC_DIR)/mqhtpass.c $^ -o $@
ldd $@
mqhtpass_unittest.o : $(SRC_DIR)/mqhtpass_test.cc \
$(SRC_DIR)/sample1.h $(FUSED_GTEST_H)
gcc $(CFLAGS) -c $(SRC_DIR)/mqhtpass_test.cc
mqhtpass_unittest : mqhtpass.so mqhtpass_test.o gtest-all.o gtest_main.o
gcc $(CFLAGS) $^ -o $@

View File

@@ -25,9 +25,9 @@ limitations under the License.
#include <apr_errno.h>
#include <apr_md5.h>
char * find_hash(char*, char*);
char *find_hash(char *, char *);
char * find_hash(char *filename, char *user)
char *find_hash(char *filename, char *user)
{
bool found = false;
FILE *fp;
@@ -45,27 +45,26 @@ char * find_hash(char *filename, char *user)
char *line = malloc(line_size);
while (fgets(line, line_size, fp) != NULL)
{
huser = strtok(line, ":");
if (strcmp(user, huser) == 0)
char *saveptr;
// Need to use strtok_r to be safe for multiple threads
huser = strtok_r(line, ":", &saveptr);
if (huser && (strcmp(user, huser) == 0))
{
hash = strtok(NULL, " \r\n\t");
// Make a duplicate of the string, because we'll be keeping it
hash = strdup(strtok_r(NULL, " \r\n\t", &saveptr));
found = true;
break;
}
}
fclose(fp);
// if (line)
// free(line);
// if (huser)
// free(huser);
// if (encPassword)
// free(encPassword);
if (line)
free(line);
}
if (!found)
{
hash = NULL;
}
return(hash);
return (hash);
}
bool htpass_authenticate_user(char *filename, char *user, char *password)
@@ -76,73 +75,18 @@ bool htpass_authenticate_user(char *filename, char *user, char *password)
// Supports multiple hashing algorithms, but we should only be using bcrypt
apr_status_t status = apr_password_validate(password, hash);
// status is usually either APR_SUCCESS or APR_EMISMATCH
if (status == APR_SUCCESS) {
if (status == APR_SUCCESS)
{
result = true;
log_debugf("Correct password supplied. user=%s", user);
} else {
}
else
{
log_debugf("Incorrect password supplied. user=%s", user);
}
return(result);
return (result);
}
// bool htpass_authenticate_user(char *filename, char *user, char *password)
// {
// bool result = false;
// FILE *fp;
// // char line[1024];
// char *huser;
// char *hash;
// // size_t len = 0;
// // size_t read;
// // int valid = -1;
// fp = fopen(filename, "r");
// if (fp == NULL)
// {
// log_errorf("Error %d opening htpasswd file '%s'", errno, filename);
// }
// if (fp)
// {
// const size_t line_size = 1024;
// char *line = malloc(line_size);
// while (fgets(line, line_size, fp) != NULL)
// {
// huser = strtok(line, ":");
// if (strcmp(user, huser) == 0)
// {
// hash = strtok(NULL, " \r\n\t");
// log_debugf("Matched user in htpasswd file: user=%s hash=%s*", huser, hash);
// // Use the Apache Portable Runtime utilities to validate the password against the hash.
// // Supports multiple hashing algorithms, but we should only be using bcrypt
// apr_status_t status = apr_password_validate(password, hash);
// // status is usually either APR_SUCCESS or APR_EMISMATCH
// if (status == APR_SUCCESS) {
// result = true;
// log_debugf("Correct password supplied. user=%s", huser);
// } else {
// log_debugf("Incorrect password supplied. user=%s", huser);
// }
// // Break out of the loop, as we've found the right user
// break;
// // TODO: Do we need to free(hash)?
// }
// else
// {
// log_debugf("Read incorrect user in htpassword: user=%s", huser);
// }
// }
// fclose(fp);
// // if (line)
// // free(line);
// // if (huser)
// // free(huser);
// // if (encPassword)
// // free(encPassword);
// }
// return result;
// }
bool htpass_valid_user(char *filename, char *user)
{
char *hash = find_hash(filename, user);
@@ -151,5 +95,5 @@ bool htpass_valid_user(char *filename, char *user)
{
valid = true;
}
return(valid);
return (valid);
}

View File

@@ -14,58 +14,172 @@ See the License for the specific language governing permissions and
limitations under the License.
*/
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include "log.h"
#include "htpass.h"
void test_fail(const char *test_name) {
printf("Failed test %s\n", test_name);
// 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__)
// Indicate test has failed
void test_fail(const char *test_name)
{
printf("--- FAIL: %s\n", test_name);
exit(1);
}
// ----------------------------------------------------------------------------
// Simple tests
// ----------------------------------------------------------------------------
void test_htpass_authenticate_user_fred_valid()
{
test_start();
int ok = htpass_authenticate_user("./src/htpass_test.htpasswd", "fred", "passw0rd");
printf("%s: fred - %d\n", __func__, ok);
if (!ok) test_fail(__func__);
if (!ok)
test_fail(__func__);
test_pass();
}
void test_htpass_authenticate_user_fred_invalid1()
{
test_start();
int ok = htpass_authenticate_user("./src/htpass_test.htpasswd", "fred", "passw0rd ");
printf("%s: fred - %d\n", __func__, ok);
if (ok) test_fail(__func__);
if (ok)
test_fail(__func__);
test_pass();
}
void test_htpass_authenticate_user_fred_invalid2()
{
test_start();
int ok = htpass_authenticate_user("./src/htpass_test.htpasswd", "fred", "");
printf("%s: fred - %d\n", __func__, ok);
if (ok) test_fail(__func__);
if (ok)
test_fail(__func__);
test_pass();
}
void test_htpass_authenticate_user_fred_invalid3()
{
test_start();
int ok = htpass_authenticate_user("./src/htpass_test.htpasswd", "fred", "clearlywrong");
printf("%s: fred - %d\n", __func__, ok);
if (ok) test_fail(__func__);
if (ok)
test_fail(__func__);
test_pass();
}
void test_htpass_authenticate_user_barney_valid()
{
test_start();
int ok = htpass_authenticate_user("./src/htpass_test.htpasswd", "barney", "s3cret");
printf("%s: barney - %d\n", __func__, ok);
if (!ok) test_fail(__func__);
if (!ok)
test_fail(__func__);
test_pass();
}
// ----------------------------------------------------------------------------
// Multi-threaded test
// ----------------------------------------------------------------------------
#define NUM_THREADS 5
// Number of tests to perform per thread. Higher numbers are more likely to trigger timing issue.
#define NUM_TESTS_PER_THREAD 1000
// Maximum number of JSON errors to report (log can get flooded)
#define MAX_JSON_ERRORS 10
// Authenticate multiple users, multiple times
void *authenticate_many_times(void *p)
{
for (int i = 0; i < NUM_TESTS_PER_THREAD; i++)
{
int ok = htpass_authenticate_user("./src/htpass_test.htpasswd", "barney", "s3cret");
if (!ok)
test_fail(__func__);
ok = htpass_authenticate_user("./src/htpass_test.htpasswd", "fred", "passw0rd");
if (!ok)
test_fail(__func__);
}
pthread_exit(NULL);
}
void check_log_file_valid(char *filename)
{
int errors = 0;
printf("--- Checking log file is valid\n");
// Check that the JSON log file isn't corrupted
FILE *log = fopen(filename, "r");
if (log == NULL)
{
test_fail(__func__);
}
const size_t line_size = 1024;
char *line = malloc(line_size);
while (fgets(line, line_size, log) != NULL)
{
if ((line[0] != '{') && (errors < MAX_JSON_ERRORS))
{
printf("*** Invalid JSON detected: %s\n", line);
errors++;
}
}
fclose(log);
}
// Test authenticate_user with multiple threads, each doing many authentications
void test_htpass_authenticate_user_multithreaded(char *logfile)
{
pthread_t threads[NUM_THREADS];
int rc;
test_start();
// Re-initialize the log to use a file for the multi-threaded test
log_init(logfile);
for (int i = 0; i < NUM_THREADS; i++)
{
printf("Creating thread %d\n", i);
rc = pthread_create(&threads[i], NULL, authenticate_many_times, NULL);
if (rc)
{
printf("Error: Unable to create thread, %d\n", rc);
test_fail(__func__);
}
}
// Wait for all the threads to complete
for (int i = 0; i < NUM_THREADS; i++)
{
pthread_join(threads[i], NULL);
}
check_log_file_valid(logfile);
test_pass();
}
// ----------------------------------------------------------------------------
int main()
{
log_init_file(stdout);
printf("TESTING BEGINS\n");
// Turn on debugging for the tests
setenv("DEBUG", "true", true);
log_init("htpass_test.log");
test_htpass_authenticate_user_fred_valid();
test_htpass_authenticate_user_fred_invalid1();
test_htpass_authenticate_user_fred_invalid2();
test_htpass_authenticate_user_fred_invalid3();
test_htpass_authenticate_user_barney_valid();
log_close();
// Call multi-threaded test last, because it re-initializes the log to use a file
test_htpass_authenticate_user_multithreaded("htpass_test_multithreaded.log");
}

View File

@@ -14,19 +14,33 @@ See the License for the specific language governing permissions and
limitations under the License.
*/
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <stdarg.h>
#include <string.h>
#include <time.h>
#include <sys/time.h>
#include <unistd.h>
#define LOG_LEVEL_INFO 0
#define LOG_LEVEL_ERROR 1
#define LOG_LEVEL_DEBUG 2
FILE *fp = NULL;
int pid;
bool debug = false;
/**
* Determine whether debugging is enabled or not, using an environment variable.
*/
void init_debug(){
char *debug_env = getenv("DEBUG");
if (debug_env != NULL)
{
// Enable debug logging if the DEBUG environment variable is set
if (strncmp(debug_env, "true", 4) || strncmp(debug_env, "1", 1))
{
debug = true;
}
}
}
int log_init(char *filename)
{
@@ -44,12 +58,14 @@ int log_init(char *filename)
result = 1;
}
}
init_debug();
return result;
}
void log_init_file(FILE *f)
{
fp = f;
init_debug();
}
void log_close()
@@ -65,102 +81,46 @@ void log_printf(const char *source_file, int source_line, const char *level, con
{
if (fp)
{
char buff[70];
// If this is a DEBUG message, and debugging is off
if ((strncmp(level, "DEBUG", 5) == 0) && !debug)
{
return;
}
char buf[1024] = "";
char *cur = buf;
char* const end = buf + sizeof buf;
char date_buf[70];
struct tm *utc;
time_t t;
struct timeval now;
gettimeofday(&now, NULL);
t = now.tv_sec;
t = time(NULL);
utc = gmtime(&t);
fprintf(fp, "{");
fprintf(fp, "\"loglevel\":\"%s\"", level);
cur += snprintf(cur, end-cur, "{");
cur += snprintf(cur, end-cur, "\"loglevel\":\"%s\"", level);
// Print ISO-8601 time and date
if (strftime(buff, sizeof buff, "%FT%T", utc))
if (strftime(date_buf, sizeof date_buf, "%FT%T", utc))
{
fprintf(fp, ", \"ibm_datetime\":\"%s.%3ld", buff, now.tv_usec);
// Round microseconds down to milliseconds, for consistency
cur += snprintf(cur, end-cur, ", \"ibm_datetime\":\"%s.%03ldZ", date_buf, now.tv_usec / 1000);
}
fprintf(fp, ", \"ibm_processId\":\"%d\"", pid);
fprintf(fp, ", \"module\":\"%s:%d\"", source_file, source_line);
fprintf(fp, ", \"message\":\"");
cur += snprintf(cur, end-cur, ", \"ibm_processId\":\"%d\"", pid);
cur += snprintf(cur, end-cur, ", \"module\":\"%s:%d\"", source_file, source_line);
cur += snprintf(cur, end-cur, ", \"message\":\"");
// Print log message, using varargs
va_list args;
va_start(args, format);
vfprintf(fp, format, args);
cur += vsnprintf(cur, end-cur, format, args);
va_end(args);
fprintf(fp, "\"}\n");
cur += snprintf(cur, end-cur, "\"}\n");
// 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);
}
}
/**
* Writes a message to the log file, using the specified type, based on a printf format string.
*/
// void log_printf(const char *level, const char *format, va_list args)
// {
// // FindSize();
// if (fp)
// {
// char buff[70];
// struct tm *utc;
// time_t t;
// struct timeval now;
// gettimeofday(&now, NULL);
// t = now.tv_sec;
// // Print ISO-8601 time and date
// t = time(NULL);
// utc = gmtime(&t);
// fprintf(fp, "{");
// fprintf(fp, "\"loglevel\":\"%s\"", level);
// if (strftime(buff, sizeof buff, "%FT%T", utc))
// {
// fprintf(fp, ", \"ibm_datetime\":\"%s.%3ld", buff, now.tv_usec);
// }
// fprintf(fp, ", \"ibm_processId\": \"%d\"", pid);
// fprintf(fp, ", \"message\":\"");
// // Print log message, using varargs
// // va_list args;
// // va_start(args, format);
// vfprintf(fp, format, args);
// // va_end(args);
// fprintf(fp, "\"}\n");
// }
// }
// void log_errorf(const char *format, ...)
// {
// va_list args;
// va_start(args, format);
// log_printf("ERROR", format, args);
// va_end(args);
// }
// void log_infof(const char *format, ...)
// {
// va_list args;
// va_start(args, format);
// log_printf("INFO", format, args);
// va_end(args);
// }
// void log_debugf(const char *format, ...)
// {
// va_list args;
// va_start(args, format);
// log_printf("DEBUG", format, args);
// va_end(args);
// }
// void log_debugf2(const char *source_file, const char *source_line, const char *format, ...)
// {
// va_list args;
// va_start(args, format);
// log_printf(source_line, format, args);
// va_end(args);
// }

View File

@@ -17,10 +17,13 @@ limitations under the License.
#ifndef _LOG_H
#define _LOG_H
/**
* Initialize the log to use the given file name.
*/
int log_init(char *);
/**
* Initialize the log wih an existing file handle
* Initialize the log with an existing file handle.
*/
void log_init_file(FILE *);

View File

@@ -64,12 +64,16 @@ void MQENTRY MQStart(
{
MQLONG CC = MQCC_OK;
MQLONG Reason = MQRC_NONE;
int log_rc = 0;
CC = log_init(LOG_FILE);
if (CC == MQCC_OK)
log_rc = log_init(LOG_FILE);
if (log_rc != 0)
{
log_infof("MQStart options=%s qmgr=%s", ((Options == MQZIO_SECONDARY) ? "Secondary" : "Primary"), trim(QMgrName));
CC = MQCC_FAILED;
Reason = MQRC_INITIALIZATION_FAILED;
}
log_infof("MQStart options=%s qmgr=%s", ((Options == MQZIO_SECONDARY) ? "Secondary" : "Primary"), trim(QMgrName));
/************************************************************************/
/* Initialize the entry point vectors. This is performed for both */
/* global and process initialisation, i.e whatever the value of the */
@@ -126,6 +130,13 @@ static void MQENTRY mqhtpass_authenticate_user(
{
char *spuser = NULL;
char *sppass = NULL;
// By default, return a warning, which indicates to MQ that this
// authorization service hasn't authenticated the user.
*pCompCode = MQCC_WARNING;
*pReason = MQRC_NONE;
// By default, tell the queue manager to continue trying other
// authorization services.
*pContinuation = MQZCI_CONTINUE;
if ((pSecurityParms->AuthenticationType) == MQCSP_AUTH_USER_ID_AND_PWD)
{
@@ -133,9 +144,21 @@ static void MQENTRY mqhtpass_authenticate_user(
// Firstly, create null-terminated strings from the user credentials in the MQ CSP object
spuser = malloc(pSecurityParms->CSPUserIdLength + 1);
if (!spuser)
{
log_errorf("Unable to allocate memory");
return;
}
strncpy(spuser, pSecurityParms->CSPUserIdPtr, pSecurityParms->CSPUserIdLength);
spuser[pSecurityParms->CSPUserIdLength] = 0;
sppass = malloc(pSecurityParms->CSPPasswordLength + 1);
if (!sppass)
{
log_errorf("Unable to allocate memory");
if (spuser)
free(spuser);
return;
}
strncpy(sppass, pSecurityParms->CSPPasswordPtr, pSecurityParms->CSPPasswordLength);
sppass[pSecurityParms->CSPPasswordLength] = 0;
log_debugf("%s with CSP user set. user=%s", __func__, spuser);
@@ -151,11 +174,6 @@ static void MQENTRY mqhtpass_authenticate_user(
}
else
{
// Return a warning, which indicates to MQ that this authorization service hasn't
// authenticated the user, but other authorization services should be tried as well.
*pCompCode = MQCC_WARNING;
*pReason = MQRC_NONE;
*pContinuation = MQZCI_CONTINUE;
log_debugf(
"Failed to authenticate user=%s effuser=%s applname=%s cspuser=%s cc=%d reason=%d",
pIdentityContext->UserIdentifier,
@@ -165,13 +183,20 @@ static void MQENTRY mqhtpass_authenticate_user(
*pCompCode,
*pReason);
}
free(spuser);
free(sppass);
if (spuser)
free(spuser);
if (sppass)
free(sppass);
}
else
{
// Password not supplied, so just check that the user ID is valid
spuser = malloc(sizeof(PMQCHAR12));
spuser = malloc(sizeof(PMQCHAR12) + 1);
if (!sppass)
{
log_errorf("Unable to allocate memory");
return;
}
strncpy(spuser, pApplicationContext->EffectiveUserID, strlen(pApplicationContext->EffectiveUserID));
spuser[sizeof(PMQCHAR12)] = 0;
log_debugf("%s without CSP user set. effectiveuid=%s", __func__, spuser);
@@ -185,9 +210,6 @@ static void MQENTRY mqhtpass_authenticate_user(
}
else
{
*pCompCode = MQCC_WARNING;
*pReason = MQRC_NONE;
*pContinuation = MQZCI_CONTINUE;
log_debugf(
"Invalid user=%s effuser=%s applname=%s cspuser=%s cc=%d reason=%d",
pIdentityContext->UserIdentifier,
@@ -197,6 +219,8 @@ static void MQENTRY mqhtpass_authenticate_user(
*pCompCode,
*pReason);
}
if (spuser)
free(spuser);
}
return;
}
@@ -216,7 +240,7 @@ static void MQENTRY mqhtpass_free_user(
PMQLONG pCompCode,
PMQLONG pReason)
{
log_infof("mqhtpass_freeuser()");
log_debugf("mqhtpass_freeuser()");
*pCompCode = MQCC_WARNING;
*pReason = MQRC_NONE;
*pContinuation = MQZCI_CONTINUE;
@@ -236,8 +260,8 @@ static void MQENTRY mqhtpass_term_auth(
PMQLONG pCompCode,
PMQLONG pReason)
{
log_infof("mqhtpass_term_auth()");
if ((Options & MQZTO_PRIMARY) == MQZTO_PRIMARY)
log_debugf("mqhtpass_term_auth()");
if (Options == MQZTO_PRIMARY)
{
log_close();
}