From 539c1b9bcb12ad4525e71f9ebbdc76245f2001ea Mon Sep 17 00:00:00 2001 From: "Roger A. Light" Date: Thu, 26 Sep 2019 11:13:18 +0100 Subject: [PATCH] Tests for zero length client id, plus fix for one case where it was allowed --- ChangeLog.txt | 4 + src/conf.c | 1 + test/broker/01-connect-invalid-id-0.py | 2 +- test/broker/01-connect-zero-length-id.py | 102 +++++++++++++++++++++++ test/broker/Makefile | 1 + test/broker/test.py | 1 + 6 files changed, 110 insertions(+), 1 deletion(-) create mode 100755 test/broker/01-connect-zero-length-id.py diff --git a/ChangeLog.txt b/ChangeLog.txt index 468ba715..65d94ad7 100644 --- a/ChangeLog.txt +++ b/ChangeLog.txt @@ -1,3 +1,7 @@ +Broker: +- allow_zero_length_clientid wasn't being used by the default listener, making + it impossible to turn this option off. + 1.6.7 - 20190925 ================ diff --git a/src/conf.c b/src/conf.c index 53c622cf..1b9708d2 100644 --- a/src/conf.c +++ b/src/conf.c @@ -529,6 +529,7 @@ int config__parse_args(struct mosquitto_db *db, struct mosquitto__config *config config->listeners[config->listener_count-1].security_options.auth_plugin_configs = config->default_listener.security_options.auth_plugin_configs; config->listeners[config->listener_count-1].security_options.auth_plugin_config_count = config->default_listener.security_options.auth_plugin_config_count; config->listeners[config->listener_count-1].security_options.allow_anonymous = config->default_listener.security_options.allow_anonymous; + config->listeners[config->listener_count-1].security_options.allow_zero_length_clientid = config->default_listener.security_options.allow_zero_length_clientid; } /* Default to drop to mosquitto user if we are privileged and no user specified. */ diff --git a/test/broker/01-connect-invalid-id-0.py b/test/broker/01-connect-invalid-id-0.py index 07dfcf74..39d580cd 100755 --- a/test/broker/01-connect-invalid-id-0.py +++ b/test/broker/01-connect-invalid-id-0.py @@ -1,7 +1,7 @@ #!/usr/bin/env python3 # Test whether a CONNECT with a zero length client id results in the correct CONNACK packet. - +# MQTT V3.1 only - zero length is invalid. from mosq_test_helper import * rc = 1 diff --git a/test/broker/01-connect-zero-length-id.py b/test/broker/01-connect-zero-length-id.py new file mode 100755 index 00000000..e9c6d8fe --- /dev/null +++ b/test/broker/01-connect-zero-length-id.py @@ -0,0 +1,102 @@ +#!/usr/bin/env python3 + +# Test whether a CONNECT with a zero length client id results in the correct behaviour. + +# MQTT v3.1.1 - zero length is allowed, unless allow_zero_length_clientid is false, and unless clean_start is False. +# MQTT v5.0 - zero length is allowed, unless allow_zero_length_clientid is false + +from mosq_test_helper import * + +def write_config(filename, port1, port2, per_listener, allow_zero): + with open(filename, 'w') as f: + f.write("per_listener_settings %s\n" % (per_listener)) + f.write("port %d\n" % (port2)) + f.write("allow_zero_length_clientid %s\n" % (allow_zero)) + f.write("listener %d\n" % (port1)) + f.write("allow_zero_length_clientid %s\n" % (allow_zero)) + + +def do_test(per_listener, proto_ver, clean_start, allow_zero, client_port, expect_fail): + conf_file = os.path.basename(__file__).replace('.py', '.conf') + write_config(conf_file, port1, port2, per_listener, allow_zero) + + rc = 1 + keepalive = 10 + connect_packet = mosq_test.gen_connect("", keepalive=keepalive, proto_ver=proto_ver, clean_session=clean_start) + if proto_ver == 4: + if expect_fail == True: + connack_packet = mosq_test.gen_connack(rc=2, proto_ver=proto_ver) + else: + connack_packet = mosq_test.gen_connack(rc=0, proto_ver=proto_ver) + else: + if expect_fail == True: + connack_packet = mosq_test.gen_connack(rc=128, proto_ver=proto_ver, properties=None) + else: + props = mqtt5_props.gen_string_prop(mqtt5_props.PROP_ASSIGNED_CLIENT_IDENTIFIER, "auto-xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx") + connack_packet = mosq_test.gen_connack(rc=0, proto_ver=proto_ver, properties=props) + # Remove the "xxxx" part - this means the front part of the packet + # is correct (so remaining length etc. is correct), but we don't + # need to match against the random id. + connack_packet = connack_packet[:-36] + + broker = mosq_test.start_broker(filename=os.path.basename(__file__), port=port1, use_conf=True) + + try: + sock = mosq_test.do_client_connect(connect_packet, connack_packet, port=client_port) + sock.close() + rc = 0 + finally: + broker.terminate() + broker.wait() + (stdo, stde) = broker.communicate() + os.remove(conf_file) + if rc: + print(stde.decode('utf-8')) + print("per_listener:%s proto_ver:%d client_port:%d clean_start:%d allow_zero:%s" % (per_listener, proto_ver, client_port, clean_start, allow_zero)) + exit(rc) + + +(port1, port2) = mosq_test.get_port(2) + +test_v4 = True +test_v5 = True + +if test_v4 == True: + do_test(per_listener="false", proto_ver=4, client_port=port1, clean_start=True, allow_zero="true", expect_fail=False) + do_test(per_listener="false", proto_ver=4, client_port=port1, clean_start=True, allow_zero="false", expect_fail=True) + do_test(per_listener="false", proto_ver=4, client_port=port1, clean_start=False, allow_zero="true", expect_fail=True) + do_test(per_listener="false", proto_ver=4, client_port=port1, clean_start=False, allow_zero="false", expect_fail=True) + do_test(per_listener="true", proto_ver=4, client_port=port1, clean_start=True, allow_zero="true", expect_fail=False) + do_test(per_listener="true", proto_ver=4, client_port=port1, clean_start=True, allow_zero="false", expect_fail=True) + do_test(per_listener="true", proto_ver=4, client_port=port1, clean_start=False, allow_zero="true", expect_fail=True) + do_test(per_listener="true", proto_ver=4, client_port=port1, clean_start=False, allow_zero="false", expect_fail=True) + + do_test(per_listener="false", proto_ver=4, client_port=port2, clean_start=True, allow_zero="true", expect_fail=False) + do_test(per_listener="false", proto_ver=4, client_port=port2, clean_start=True, allow_zero="false", expect_fail=True) + do_test(per_listener="false", proto_ver=4, client_port=port2, clean_start=False, allow_zero="true", expect_fail=True) + do_test(per_listener="false", proto_ver=4, client_port=port2, clean_start=False, allow_zero="false", expect_fail=True) + do_test(per_listener="true", proto_ver=4, client_port=port2, clean_start=True, allow_zero="true", expect_fail=False) + do_test(per_listener="true", proto_ver=4, client_port=port2, clean_start=True, allow_zero="false", expect_fail=True) + do_test(per_listener="true", proto_ver=4, client_port=port2, clean_start=False, allow_zero="true", expect_fail=True) + do_test(per_listener="true", proto_ver=4, client_port=port2, clean_start=False, allow_zero="false", expect_fail=True) + +if test_v5 == True: + do_test(per_listener="false", proto_ver=5, client_port=port1, clean_start=True, allow_zero="true", expect_fail=False) + do_test(per_listener="false", proto_ver=5, client_port=port1, clean_start=True, allow_zero="false", expect_fail=True) + do_test(per_listener="false", proto_ver=5, client_port=port1, clean_start=False, allow_zero="true", expect_fail=False) + do_test(per_listener="false", proto_ver=5, client_port=port1, clean_start=False, allow_zero="false", expect_fail=True) + do_test(per_listener="true", proto_ver=5, client_port=port1, clean_start=True, allow_zero="true", expect_fail=False) + do_test(per_listener="true", proto_ver=5, client_port=port1, clean_start=True, allow_zero="false", expect_fail=True) + do_test(per_listener="true", proto_ver=5, client_port=port1, clean_start=False, allow_zero="true", expect_fail=False) + do_test(per_listener="true", proto_ver=5, client_port=port1, clean_start=False, allow_zero="false", expect_fail=True) + + do_test(per_listener="false", proto_ver=5, client_port=port2, clean_start=True, allow_zero="true", expect_fail=False) + do_test(per_listener="false", proto_ver=5, client_port=port2, clean_start=True, allow_zero="false", expect_fail=True) + do_test(per_listener="false", proto_ver=5, client_port=port2, clean_start=False, allow_zero="true", expect_fail=False) + do_test(per_listener="false", proto_ver=5, client_port=port2, clean_start=False, allow_zero="false", expect_fail=True) + do_test(per_listener="true", proto_ver=5, client_port=port2, clean_start=True, allow_zero="true", expect_fail=False) + do_test(per_listener="true", proto_ver=5, client_port=port2, clean_start=True, allow_zero="false", expect_fail=True) + do_test(per_listener="true", proto_ver=5, client_port=port2, clean_start=False, allow_zero="true", expect_fail=False) + do_test(per_listener="true", proto_ver=5, client_port=port2, clean_start=False, allow_zero="false", expect_fail=True) + +exit(0) diff --git a/test/broker/Makefile b/test/broker/Makefile index e4c32c63..3b78a1d2 100644 --- a/test/broker/Makefile +++ b/test/broker/Makefile @@ -44,6 +44,7 @@ ifeq ($(WITH_TLS),yes) else ./01-connect-uname-password-success-no-tls.py endif + ./01-connect-zero-length-id.py 02 : diff --git a/test/broker/test.py b/test/broker/test.py index 7a2f9f66..1fff9b7a 100755 --- a/test/broker/test.py +++ b/test/broker/test.py @@ -25,6 +25,7 @@ tests = [ (1, './01-connect-uname-password-denied.py'), (1, './01-connect-uname-password-success.py'), (1, './01-connect-uname-pwd-no-flag.py'), + (2, './01-connect-zero-length-id.py'), (1, './02-shared-qos0-v5.py'), (1, './02-subhier-crash.py'),