Exit broker on reload if acl_file or password_file detect invalid input

Also exit if a plugin returns an error code from the RELOAD event.

Closes #3350.

ChangeLog.txt

@@ -3,6 +3,21 @@
 
 # Broker
 
+## Deprecations
+- The acl_file option is deprecated in favour of the acl-file plugin, which is
+  the same code but moved into a plugin. The acl_file option will be removed
+  in 3.0.
+- The password_file option is deprecated in favour of the password-file plugin,
+  which is the same code but moved into a plugin. The password_file option will
+  be removed in 3.0.
+- The per_listener_settings option is deprecated in favour of the new listener
+  specific options. The per_listener_settings option will be removed in 3.0.
+
+## Behaviour changes
+
+- acl_file and password_file will produce an error on invalid input when
+  reloading the config, causing the broker to quit.
+
 ## Protocol related
 - Add support for broker created topic aliases. Topics are allocated on a
   first come first serve basis.
@@ -136,8 +151,10 @@
   tick callback again.
 
 # Plugins
+- Add acl-file plugin.
+- Add password-file plugin.
 - Add persist-sqlite plugin.
-- Add sparkplug aware plugin.
+- Add sparkplug-aware plugin.
 
 # Dynamic security plugin
 - Add ability to deny wildcard subscriptions for a role to the dynsec plugin.

plugins/acl-file/acl_parse.c

@@ -315,6 +315,7 @@ static void acl__free_entries(struct acl__entry *entry)
 	}
 }
 
+
 void acl_file__cleanup(struct acl_file_data *data)
 {
 	struct acl__user *user, *user_tmp;
@@ -332,3 +333,15 @@ void acl_file__cleanup(struct acl_file_data *data)
 	acl__free_entries(data->acl_patterns);
 	data->acl_patterns = NULL;
 }
+
+
+int acl_file__reload(int event, void *event_data, void *userdata)
+{
+	struct acl_file_data *data = userdata;
+
+	UNUSED(event);
+	UNUSED(event_data);
+
+	acl_file__cleanup(data);
+	return acl_file__parse(data);
+}

plugins/acl-file/plugin.c

@@ -60,6 +60,8 @@ int mosquitto_plugin_init(mosquitto_plugin_id_t *identifier, void **user_data, s
 
 	rc = mosquitto_callback_register(mosq_pid, MOSQ_EVT_ACL_CHECK, acl_file__check, NULL, data);
 	if(rc) return rc;
+	rc = mosquitto_callback_register(mosq_pid, MOSQ_EVT_RELOAD, acl_file__reload, NULL, data);
+	if(rc) return rc;
 
 	return MOSQ_ERR_SUCCESS;
 }
@@ -73,6 +75,7 @@ int mosquitto_plugin_cleanup(void *user_data, struct mosquitto_opt *options, int
 	UNUSED(option_count);
 
 	mosquitto_callback_unregister(mosq_pid, MOSQ_EVT_ACL_CHECK, acl_file__check, NULL);
+	mosquitto_callback_unregister(mosq_pid, MOSQ_EVT_RELOAD, acl_file__reload, NULL);
 
 	acl_file__cleanup(data);
 

plugins/password-file/password_parse.c

@@ -56,18 +56,18 @@ int password_file__parse(struct password_file_data *data)
 			if(username){
 				username = mosquitto_trimblanks(username);
 				if(strlen(username) > 65535){
-					mosquitto_log_printf(MOSQ_LOG_NOTICE, "password-file: Warning: Invalid line in password file '%s', username too long.", data->password_file);
-					continue;
+					mosquitto_log_printf(MOSQ_LOG_ERR, "password-file: Error: Invalid line in password file '%s', username too long.", data->password_file);
+					return MOSQ_ERR_INVAL;
 				}
 				if(strlen(username) <= 0){
-					mosquitto_log_printf(MOSQ_LOG_NOTICE, "password-file: Warning: Empty username in password file '%s', ignoring.", data->password_file);
-					continue;
+					mosquitto_log_printf(MOSQ_LOG_ERR, "password-file: Error: Empty username in password file '%s'.", data->password_file);
+					return MOSQ_ERR_INVAL;
 				}
 
 				HASH_FIND(hh, data->unpwd, username, strlen(username), unpwd);
 				if(unpwd){
-					mosquitto_log_printf(MOSQ_LOG_NOTICE, "password-file: Error: Duplicate user '%s' in password file '%s', ignoring.", username, data->password_file);
-					continue;
+					mosquitto_log_printf(MOSQ_LOG_ERR, "password-file: Error: Duplicate user '%s' in password file '%s'.", username, data->password_file);
+					return MOSQ_ERR_INVAL;
 				}
 
 				unpwd = mosquitto_calloc(1, sizeof(struct mosquitto__unpwd));
@@ -89,28 +89,29 @@ int password_file__parse(struct password_file_data *data)
 					password = mosquitto_trimblanks(password);
 
 					if(strlen(password) > 65535){
-						mosquitto_log_printf(MOSQ_LOG_NOTICE, "password-file: Warning: Invalid line in password file '%s', password too long.", data->password_file);
+						mosquitto_log_printf(MOSQ_LOG_ERR, "password-file: Error: Invalid line in password file '%s', password too long.", data->password_file);
 						mosquitto_FREE(unpwd->username);
 						mosquitto_FREE(unpwd);
-						continue;
+						return MOSQ_ERR_INVAL;
 					}
 
 					if(mosquitto_pw_new(&unpwd->pw, MOSQ_PW_DEFAULT)
 							|| mosquitto_pw_decode(unpwd->pw, password)){
 
-						mosquitto_log_printf(MOSQ_LOG_NOTICE, "password-file: Warning: Unable to decode line in password file '%s'.", data->password_file);
+						mosquitto_log_printf(MOSQ_LOG_ERR, "password-file: Error: Unable to decode line in password file '%s'.", data->password_file);
 						mosquitto_pw_cleanup(unpwd->pw);
 						mosquitto_FREE(unpwd->username);
 						mosquitto_FREE(unpwd);
-						continue;
+						return MOSQ_ERR_INVAL;
 					}
 
 					HASH_ADD_KEYPTR(hh, data->unpwd, unpwd->username, strlen(unpwd->username), unpwd);
 				}else{
-					mosquitto_log_printf(MOSQ_LOG_NOTICE, "password-file: Warning: Invalid line in password file '%s': %s", data->password_file, buf);
+					mosquitto_log_printf(MOSQ_LOG_ERR, "password-file: Error: Invalid line in password file '%s': %s", data->password_file, buf);
 					mosquitto_pw_cleanup(unpwd->pw);
 					mosquitto_FREE(unpwd->username);
 					mosquitto_FREE(unpwd);
+					return MOSQ_ERR_INVAL;
 				}
 			}
 		}
@@ -135,3 +136,15 @@ void password_file__cleanup(struct password_file_data *data)
 		mosquitto_FREE(u);
 	}
 }
+
+
+int password_file__reload(int event, void *event_data, void *userdata)
+{
+	struct password_file_data *data = userdata;
+
+	UNUSED(event);
+	UNUSED(event_data);
+
+	password_file__cleanup(data);
+	return password_file__parse(data);
+}

plugins/password-file/plugin.c

@@ -61,6 +61,9 @@ int mosquitto_plugin_init(mosquitto_plugin_id_t *identifier, void **user_data, s
 	rc = mosquitto_callback_register(mosq_pid, MOSQ_EVT_BASIC_AUTH, password_file__check, NULL, data);
 	if(rc) return rc;
 
+	rc = mosquitto_callback_register(mosq_pid, MOSQ_EVT_RELOAD, password_file__reload, NULL, data);
+	if(rc) return rc;
+
 	return MOSQ_ERR_SUCCESS;
 }
 
@@ -73,6 +76,7 @@ int mosquitto_plugin_cleanup(void *user_data, struct mosquitto_opt *options, int
 	UNUSED(option_count);
 
 	mosquitto_callback_unregister(mosq_pid, MOSQ_EVT_BASIC_AUTH, password_file__check, NULL);
+	mosquitto_callback_unregister(mosq_pid, MOSQ_EVT_RELOAD, password_file__reload, NULL);
 	password_file__cleanup(data);
 
 	return MOSQ_ERR_SUCCESS;

src/acl_file.h

@@ -46,6 +46,7 @@ struct acl_file_data{
 
 int acl_file__parse(struct acl_file_data *data);
 int acl_file__check(int event, void *event_data, void *userdata);
+int acl_file__reload(int event, void *event_data, void *userdata);
 void acl_file__cleanup(struct acl_file_data *data);
 
 #endif

src/loop.c

@@ -224,7 +224,9 @@ int mosquitto_main_loop(struct mosquitto__listener_sock *listensock, int listens
 		}
 #endif
 
-		signal__flag_check();
+		rc = signal__flag_check();
+		if(rc) return rc;
+
 #if defined(WITH_WEBSOCKETS) && WITH_WEBSOCKETS == WS_IS_LWS
 		for(int i=0; i<db.config->listener_count; i++){
 			/* Extremely hacky, should be using the lws provided external poll

src/mosquitto_broker_internal.h

@@ -853,7 +853,7 @@ int plugin__handle_message_in(struct mosquitto *context, struct mosquitto_base_m
 int plugin__handle_message_out(struct mosquitto *context, struct mosquitto_base_msg *base_msg);
 int plugin__handle_subscribe(struct mosquitto *context, struct mosquitto_subscription *sub);
 int plugin__handle_unsubscribe(struct mosquitto *context, struct mosquitto_subscription *sub);
-void plugin__handle_reload(void);
+int plugin__handle_reload(void);
 void plugin__handle_tick(void);
 int plugin__callback_unregister_all(mosquitto_plugin_id_t *identifier);
 void plugin_persist__handle_restore(void);
@@ -944,7 +944,7 @@ void session_expiry__send_all(void);
  * Signals
  * ============================================================ */
 void signal__setup(void);
-void signal__flag_check(void);
+int signal__flag_check(void);
 
 /* ============================================================
  * Window service and signal related functions

src/password_file.h

@@ -34,6 +34,7 @@ struct password_file_data{
 
 int password_file__parse(struct password_file_data *data);
 int password_file__check(int event, void *event_data, void *userdata);
+int password_file__reload(int event, void *event_data, void *userdata);
 void password_file__cleanup(struct password_file_data *data);
 
 #endif

src/plugin_reload.c

@@ -25,7 +25,7 @@ Contributors:
 #include "utlist.h"
 
 
-static void plugin__handle_reload_single(struct mosquitto__security_options *opts)
+static int plugin__handle_reload_single(struct mosquitto__security_options *opts)
 {
 	struct mosquitto_evt_reload event_data;
 	struct mosquitto__callback *cb_base, *cb_next;
@@ -34,24 +34,36 @@ static void plugin__handle_reload_single(struct mosquitto__security_options *opt
 
 	// Using DL_FOREACH_SAFE here, as reload callbacks might unregister themself
 	DL_FOREACH_SAFE(opts->plugin_callbacks.reload, cb_base, cb_next){
-		cb_base->cb(MOSQ_EVT_RELOAD, &event_data, cb_base->userdata);
+		int rc = cb_base->cb(MOSQ_EVT_RELOAD, &event_data, cb_base->userdata);
+		if(rc){
+			log__printf(NULL, MOSQ_LOG_ERR, "Error: Plugin %s produced error on reload: %s",
+					cb_base->identifier->plugin_name?cb_base->identifier->plugin_name:"",
+					mosquitto_strerror(rc));
+
+			return rc;
+		}
 	}
+	return MOSQ_ERR_SUCCESS;
 }
 
 
-void plugin__handle_reload(void)
+int plugin__handle_reload(void)
 {
 	struct mosquitto__security_options *opts;
+	int rc;
 
 	/* Global plugins */
-	plugin__handle_reload_single(&db.config->security_options);
+	rc = plugin__handle_reload_single(&db.config->security_options);
+	if(rc) return rc;
 
 	if(db.config->per_listener_settings){
 		for(int i=0; i<db.config->listener_count; i++){
 			opts = db.config->listeners[i].security_options;
 			if(opts && opts->plugin_callbacks.reload){
-				plugin__handle_reload_single(opts);
+				rc = plugin__handle_reload_single(opts);
+				if(rc) return rc;
 			}
 		}
 	}
+	return MOSQ_ERR_SUCCESS;
 }

src/signals.c

@@ -90,7 +90,7 @@ void signal__setup(void)
 #endif
 }
 
-void signal__flag_check(void)
+int signal__flag_check(void)
 {
 #ifdef WITH_PERSISTENCE
 	if(flag_db_backup){
@@ -104,13 +104,17 @@ void signal__flag_check(void)
 		flag_log_rotate = false;
 	}
 	if(flag_reload){
+		int rc;
 		log__printf(NULL, MOSQ_LOG_INFO, "Reloading config.");
 		config__read(db.config, true);
 		listeners__reload_all_certificates();
-		plugin__handle_reload();
+		rc = plugin__handle_reload();
+		if(rc) return rc;
 		mosquitto_security_cleanup(true);
-		mosquitto_security_init(true);
-		mosquitto_security_apply_default();
+		rc = mosquitto_security_init(true);
+		if(rc) return rc;
+		rc = mosquitto_security_apply_default();
+		if(rc) return rc;
 		log__close(db.config);
 		log__init(db.config);
 		keepalive__cleanup();
@@ -132,6 +136,8 @@ void signal__flag_check(void)
 		flag_xtreport = false;
 	}
 #endif
+
+	return MOSQ_ERR_SUCCESS;
 }
 
 /*

test/broker/23-security-acl-file-reload.py

@@ -0,0 +1,60 @@
+#!/usr/bin/env python3
+
+from mosq_test_helper import *
+import signal
+
+def write_config_default(filename, port):
+    with open(filename, 'w') as f:
+        f.write("listener %d\n" % (port))
+        f.write(f"acl_file {port}.acl\n")
+        f.write("allow_anonymous true\n")
+
+
+def write_config_plugin(filename, port):
+    with open(filename, 'w') as f:
+        f.write("listener %d\n" % (port))
+        f.write(f"plugin {mosq_test.get_build_root()}/plugins/acl-file/mosquitto_acl_file.so\n")
+        f.write(f"plugin_opt_acl_file {port}.acl\n")
+        f.write("allow_anonymous true\n")
+
+
+def do_test(write_config_func):
+    port = mosq_test.get_port()
+    conf_file = os.path.basename(__file__).replace('.py', '.conf')
+    write_config_func(conf_file, port)
+
+    rc = 1
+    connect_packet = mosq_test.gen_connect("acl-change-test")
+    connack_packet = mosq_test.gen_connack(rc=0)
+
+    with open(f"{port}.acl", "wt") as f:
+        f.write("topic readwrite a/#")
+
+    broker = mosq_test.start_broker(filename=os.path.basename(__file__), use_conf=True, port=port)
+
+    try:
+        sock = mosq_test.do_client_connect(connect_packet, connack_packet, port=port)
+        sock.close()
+
+        with open(f"{port}.acl", "wt") as f:
+            f.write("topic readwrite a#")
+
+        broker.send_signal(signal.SIGHUP)
+        # Broker should terminate
+        if mosq_test.wait_for_subprocess(broker) == 0 and broker.returncode == 3:
+            rc = 0
+    except mosq_test.TestError:
+        pass
+    except Exception as err:
+        print(err)
+    finally:
+        os.remove(conf_file)
+        os.remove(f"{port}.acl")
+        broker.terminate()
+        (stdo, stde) = broker.communicate()
+        if rc:
+            print(stde.decode('utf-8'))
+            exit(rc)
+
+do_test(write_config_default)
+do_test(write_config_plugin)

test/broker/23-security-password-file-reload.py

@@ -0,0 +1,60 @@
+#!/usr/bin/env python3
+
+from mosq_test_helper import *
+import signal
+
+def write_config_default(filename, port):
+    with open(filename, 'w') as f:
+        f.write("listener %d\n" % (port))
+        f.write(f"password_file {port}.password\n")
+        f.write("allow_anonymous true\n")
+
+
+def write_config_plugin(filename, port):
+    with open(filename, 'w') as f:
+        f.write("listener %d\n" % (port))
+        f.write(f"plugin {mosq_test.get_build_root()}/plugins/password-file/mosquitto_password_file.so\n")
+        f.write(f"plugin_opt_password_file {port}.password\n")
+        f.write("allow_anonymous true\n")
+
+
+def do_test(write_config_func):
+    port = mosq_test.get_port()
+    conf_file = os.path.basename(__file__).replace('.py', '.conf')
+    write_config_func(conf_file, port)
+
+    rc = 1
+    connect_packet = mosq_test.gen_connect("password-change-test")
+    connack_packet = mosq_test.gen_connack(rc=0)
+
+    with open(f"{port}.password", "wt") as f:
+        f.write("test:$7$1000$97ozvObcN5zP4MGzYUw4uRp8+mPQbThrHOX69vdHHNVwV4iZf2K2X23FS7weilZMKeV+9oLHdilybmpXcFApYg==$WlM0jUhsiQNQJe4IDt5K1rmtAdaenWGdntswJmDkp74W9pdrt/+RdIK3YaJ09o3pD1xbtokXq933bQh+CrjA4Q==\n")
+
+    broker = mosq_test.start_broker(filename=os.path.basename(__file__), use_conf=True, port=port)
+
+    try:
+        sock = mosq_test.do_client_connect(connect_packet, connack_packet, port=port)
+        sock.close()
+
+        with open(f"{port}.password", "wt") as f:
+            f.write("test:bad\n")
+
+        broker.send_signal(signal.SIGHUP)
+        # Broker should terminate
+        if mosq_test.wait_for_subprocess(broker) == 0 and broker.returncode == 3:
+            rc = 0
+    except mosq_test.TestError:
+        pass
+    except Exception as err:
+        print(err)
+    finally:
+        os.remove(conf_file)
+        os.remove(f"{port}.password")
+        broker.terminate()
+        (stdo, stde) = broker.communicate()
+        if rc:
+            print(stde.decode('utf-8'))
+            exit(rc)
+
+do_test(write_config_default)
+do_test(write_config_plugin)

test/broker/Makefile

@@ -18,7 +18,7 @@ test-compile :
 ptest : test-compile
 	./test.py
 
-test : test-compile msg_sequence_test 01 02 03 04 05 06 07 08 09 10 11 12 13 14 15 16 17 20 21 22
+test : test-compile msg_sequence_test 01 02 03 04 05 06 07 08 09 10 11 12 13 14 15 16 17 20 21 22 23
 
 msg_sequence_test:
 	./msg_sequence_test.py
@@ -353,3 +353,6 @@ endif
 	./22-http-api-api.py
 	./22-http-api-file.py
 	./22-http-api-tls.py
+
+23:
+	./23-security-acl-file-reload.py

test/broker/test.py

@@ -296,6 +296,8 @@ tests = [
     (2, './22-http-api-api.py'),
     (2, './22-http-api-file.py'),
     (2, './22-http-api-tls.py'),
+
+    (2, './23-security-acl-file-reload.py'),
 ]
 
 if __name__ == "__main__":