Add 'zero_byte_username_is_anonymous' config option

There are many clients out there who set the 'username' flag in connect and then set an empty username. The default is to not have two methods of signalling anonymous clients, but this can be overriden. #91
halfgaar · Mar 16, 2024 · 60a66a3 · 60a66a3
1 parent 877a8fa
commit 60a66a3
Show file tree

Hide file tree

Showing 13 changed files with 130 additions and 24 deletions.
diff --git a/FlashMQTests/tst_maintests.cpp b/FlashMQTests/tst_maintests.cpp
@@ -2347,6 +2347,20 @@ listen {
         ConnAckData ackData = ack.parseConnAckData();
         QCOMPARE(ackData.reasonCode, ReasonCodes::NotAuthorized);
     }
+
+    // Test empty password for existing user. It should not think it's anonymous.
+    {
+        FlashMQTestClient client;
+        client.start();
+        client.connectClient(ProtocolVersion::Mqtt5, false, 120, [](Connect &connect) {
+                connect.username = "one";
+                connect.password = "";
+            }, 2883); // allow_anonymous true global setting.
+
+        auto ack = client.receivedPackets.front();
+        ConnAckData ackData = ack.parseConnAckData();
+        QCOMPARE(ackData.reasonCode, ReasonCodes::NotAuthorized);
+    }
 }
 
 void MainTests::testOverrideAllowAnonymousToFalse()
@@ -2360,6 +2374,7 @@ void MainTests::testOverrideAllowAnonymousToFalse()
     ConfFileTemp confFile;
     confFile.writeLine(formatString("mosquitto_password_file %s", passwd_file.getFilePath().c_str()));
     confFile.writeLine("allow_anonymous true");
+    confFile.writeLine("zero_byte_username_is_anonymous true");
     confFile.writeLine(R"(
 listen {
         protocol mqtt
@@ -2427,6 +2442,20 @@ listen {
         QCOMPARE(ackData.reasonCode, ReasonCodes::Success);
     }
 
+    // Test 'zero_byte_username_is_anonymous true'
+    {
+        FlashMQTestClient client;
+        client.start();
+        client.connectClient(ProtocolVersion::Mqtt311, false, 120, [](Connect &connect) {
+                connect.username = "";
+                connect.password = "wrong";
+            }, 2884);
+
+        auto ack = client.receivedPackets.front();
+        ConnAckData ackData = ack.parseConnAckData();
+        QCOMPARE(ackData.reasonCode, ReasonCodes::Success);
+    }
+
     {
         FlashMQTestClient client;
         client.start();

diff --git a/bridgeconfig.cpp b/bridgeconfig.cpp
@@ -171,6 +171,9 @@ void BridgeConfig::isValid()
     if (clientidPrefix.length() > 10)
         throw ConfigFileException("clientidPrefix can't be longer than 10");
 
+    if (protocolVersion <= ProtocolVersion::Mqtt311 && remote_password.has_value() && !remote_username.has_value())
+        throw ConfigFileException("MQTT 3.1.1 and lower require a username when you set a password.");
+
     setClientId();
 }
 

diff --git a/bridgeconfig.h b/bridgeconfig.h
@@ -4,6 +4,7 @@
 #include <memory>
 #include <string>
 #include <vector>
+#include <optional>
 #include "session.h"
 #include "dnsresolver.h"
 #include "sslctxmanager.h"
@@ -40,9 +41,9 @@ class BridgeConfig
     ProtocolVersion protocolVersion = ProtocolVersion::Mqtt311;
     bool bridgeProtocolBit = true;
     std::string clientidPrefix = "fmqbridge";
-    std::string local_username;
-    std::string remote_username;
-    std::string remote_password;
+    std::optional<std::string> local_username;
+    std::optional<std::string> remote_username;
+    std::optional<std::string> remote_password;
     bool remoteCleanStart = true;
     uint32_t remoteSessionExpiryInterval = 0;
     bool localCleanStart = true;

diff --git a/client.cpp b/client.cpp
@@ -687,7 +687,7 @@ void Client::setBridgeState(std::shared_ptr<BridgeState> bridgeState)
         this->address = bridgeState->c.address;
         this->clean_start = bridgeState->c.localCleanStart;
         this->clientid = bridgeState->c.getClientid();
-        this->username = bridgeState->c.local_username;
+        this->username = bridgeState->c.local_username.value_or(std::string());
         this->keepalive = bridgeState->c.keepalive;
 
         // Not setting maxOutgoingTopicAliasValue, because that must remain 0 until the other side says (in the connack) we can uses aliases.

diff --git a/configfileparser.cpp b/configfileparser.cpp
@@ -206,6 +206,7 @@ ConfigFileParser::ConfigFileParser(const std::string &path) :
     validKeys.insert("rebuild_subscription_tree_interval_seconds");
     validKeys.insert("minimum_wildcard_subscription_depth");
     validKeys.insert("wildcard_subscription_deny_mode");
+    validKeys.insert("zero_byte_username_is_anonymous");
 
     validListenKeys.insert("port");
     validListenKeys.insert("protocol");
@@ -991,6 +992,11 @@ void ConfigFileParser::loadFile(bool test)
                     else
                         throw ConfigFileException(formatString("Value '%s' for '%s' is invalid.", value.c_str(), key.c_str()));
                 }
+
+                if (testKeyValidity(key, "zero_byte_username_is_anonymous", validKeys))
+                {
+                    tmpSettings.zeroByteUsernameIsAnonymous = stringTruthiness(value);
+                }
             }
         }
         catch (std::invalid_argument &ex) // catch for the stoi()

diff --git a/man/flashmq.1 b/man/flashmq.1
@@ -5,7 +5,7 @@
 \\$2 \(la\\$1\(ra\\$3
 ..
 .if \n(.g .mso www.tmac
-.TH flashmq 1 "16 November 2023" "" ""
+.TH flashmq 1 "4 March 2024" "" ""
 .SH NAME
 flashmq \- A fast light-weight scalable MQTT server
 .SH SYNOPSIS

diff --git a/man/flashmq.conf.5 b/man/flashmq.conf.5
@@ -5,7 +5,7 @@
 \\$2 \(la\\$1\(ra\\$3
 ..
 .if \n(.g .mso www.tmac
-.TH flashmq.conf 5 "2 March 2024" "" ""
+.TH flashmq.conf 5 "16 March 2024" "" ""
 .SH NAME
 flashmq.conf \- FlashMQ configuration file format
 .SH SYNOPSIS
@@ -119,6 +119,17 @@ This option can be overriden on a per-listener basis; see
 .URL #listen__allow_anonymous listener.allow_anonymous
 \&.
 
+Default value: \*(T<false\*(T>
+.TP 
+\*(T<\fBzero_byte_username_is_anonymous\fR\*(T> \fItrue/false\fR 
+The proper way to signal an anonymous client is by setting the 'username present' flag in the CONNECT packet to 0, which in MQTT3 also demands the absence of a password. However, there are also clients out there that set the 'username present' flag to 1 and then give an empty username. This is an undesirable situation, because it means there are two ways to identify an anonymous client.
+
+Anonymous clients are not authenticated against a loaded plugin when \*(T<\fBallow_anonymous\fR\*(T> is true. With this option enabled, that means users with empty string as usernames also aren't.
+
+With this option disabled, clients connecting with an empty username will be reject with 'bad username or password' as MQTT error code.
+
+The default is to be unambigious, but this can be overridden with this option.
+
 Default value: \*(T<false\*(T>
 .TP 
 \*(T<\fBrlimit_nofile\fR\*(T> \fInumber\fR 

diff --git a/man/flashmq.conf.5.dbk5 b/man/flashmq.conf.5.dbk5
@@ -240,6 +240,29 @@
           </para>
         </listitem>
       </varlistentry>
+      <varlistentry xml:id="zero_byte_username_is_anonymous">
+        <term><option>zero_byte_username_is_anonymous</option> <replaceable>true/false</replaceable></term>
+        <listitem>
+          <para>
+            The proper way to signal an anonymous client is by setting the 'username present' flag in the CONNECT packet to 0, which in MQTT3 also demands the absence of a password. However, there are also clients out there that set the 'username present' flag to 1 and then give an empty username. This is an undesirable situation, because it means there are two ways to identify an anonymous client.
+          </para>
+
+          <para>
+            Anonymous clients are not authenticated against a loaded plugin when <option>allow_anonymous</option> is true. With this option enabled, that means users with empty string as usernames also aren't.
+          </para>
+
+          <para>
+            With this option disabled, clients connecting with an empty username will be reject with 'bad username or password' as MQTT error code.
+          </para>
+
+          <para>
+            The default is to be unambigious, but this can be overridden with this option.
+          </para>
+          <para>
+            Default value: <literal>false</literal>
+          </para>
+        </listitem>
+      </varlistentry>
       <varlistentry xml:id="rlimit_nofile">
         <term><option>rlimit_nofile</option> <replaceable>number</replaceable></term>
         <listitem>

diff --git a/man/flashmq.conf.5.html b/man/flashmq.conf.5.html
@@ -296,6 +296,29 @@
         </dd>
 
 
+        <dt id="zero_byte_username_is_anonymous"><code class="option">zero_byte_username_is_anonymous</code> <code class="replaceable">true/false</code><a class="hash-anchor" href="#zero_byte_username_is_anonymous">#</a></dt>
+        <dd>
+          <p>
+            The proper way to signal an anonymous client is by setting the 'username present' flag in the CONNECT packet to 0, which in MQTT3 also demands the absence of a password. However, there are also clients out there that set the 'username present' flag to 1 and then give an empty username. This is an undesirable situation, because it means there are two ways to identify an anonymous client.
+          </p>
+
+          <p>
+            Anonymous clients are not authenticated against a loaded plugin when <code class="option">allow_anonymous</code> is true. With this option enabled, that means users with empty string as usernames also aren't.
+          </p>
+
+          <p>
+            With this option disabled, clients connecting with an empty username will be reject with 'bad username or password' as MQTT error code.
+          </p>
+
+          <p>
+            The default is to be unambigious, but this can be overridden with this option.
+          </p>
+          <p>
+            Default value: <code class="literal">false</code>
+          </p>
+        </dd>
+
+
         <dt id="rlimit_nofile"><code class="option">rlimit_nofile</code> <code class="replaceable">number</code><a class="hash-anchor" href="#rlimit_nofile">#</a></dt>
         <dd>
           <p>

diff --git a/mqttpacket.cpp b/mqttpacket.cpp
@@ -263,8 +263,8 @@ MqttPacket::MqttPacket(const Connect &connect) :
     writeByte(protocolVersionByte);
 
     uint8_t flags = connect.clean_start << 1;
-    flags |= !connect.username.empty() << 7;
-    flags |= !connect.password.empty() << 6;
+    flags |= static_cast<unsigned int>(connect.username.has_value()) << 7;
+    flags |= static_cast<unsigned int>(connect.password.has_value()) << 6;
 
     if (connect.will)
     {
@@ -295,10 +295,10 @@ MqttPacket::MqttPacket(const Connect &connect) :
         writeString(connect.will->payload);
     }
 
-    if (!connect.username.empty())
-        writeString(connect.username);
-    if (!connect.password.empty())
-        writeString(connect.password);
+    if (connect.username.has_value())
+        writeString(connect.username.value());
+    if (connect.password.has_value())
+        writeString(connect.password.value());
 
     calculateRemainingLength();
 }
@@ -679,11 +679,22 @@ ConnectData MqttPacket::parseConnectData()
 
     if (user_name_flag)
     {
+        // Usernames must be UTF-8, but we defer that check so we can give proper a CONNACK, and continue parsing.
         result.username = readBytesToString(false);
-        result.willpublish.username = result.username.value();
 
         if (result.username.value().empty())
-            throw ProtocolError("Username flagged as present, but it's 0 bytes.", ReasonCodes::MalformedPacket);
+        {
+            if (settings.zeroByteUsernameIsAnonymous)
+                result.username.reset();
+            else
+                throw ProtocolError("Attempting anonymous login with zero byte username. See config option 'zero_byte_username_is_anonymous'.",
+                                    ReasonCodes::BadUserNameOrPassword);
+        }
+    }
+
+    if (result.username)
+    {
+        result.willpublish.username = result.username.value();
 
         if (!settings.allowUnsafeUsernameChars && containsDangerousCharacters(result.username.value()))
             throw ProtocolError(formatString("Username '%s' contains unsafe characters and 'allow_unsafe_username_chars' is false.", result.username.value().c_str()),
@@ -692,15 +703,12 @@ ConnectData MqttPacket::parseConnectData()
 
     if (result.password_flag)
     {
-        if (this->protocolVersion <= ProtocolVersion::Mqtt311 && !result.username)
+        if (this->protocolVersion <= ProtocolVersion::Mqtt311 && !user_name_flag)
         {
             throw ProtocolError("MQTT 3.1.1: If the User Name Flag is set to 0, the Password Flag MUST be set to 0.", ReasonCodes::MalformedPacket);
         }
 
         result.password = readBytesToString(false);
-
-        if (result.password.empty())
-            throw ProtocolError("Password flagged as present, but it's 0 bytes.", ReasonCodes::MalformedPacket);
     }
 
     return  result;

diff --git a/settings.h b/settings.h
@@ -91,6 +91,7 @@ class Settings
     SharedSubscriptionTargeting sharedSubscriptionTargeting = SharedSubscriptionTargeting::RoundRobin;
     uint16_t minimumWildcardSubscriptionDepth = 0;
     WildcardSubscriptionDenyMode wildcardSubscriptionDenyMode = WildcardSubscriptionDenyMode::DenyAll;
+    bool zeroByteUsernameIsAnonymous = false;
     std::list<std::shared_ptr<Listener>> listeners; // Default one is created later, when none are defined.
 
     std::list<Network> setRealIpFrom;

diff --git a/types.cpp b/types.cpp
@@ -471,11 +471,11 @@ size_t Connect::getLengthWithoutFixedHeader() const
         result += will->payload.length() + 2;
     }
 
-    if (!username.empty())
-        result += username.size() + 2;
+    if (username.has_value())
+        result += username->size() + 2;
 
-    if (!password.empty())
-        result += password.size() + 2;
+    if (password.has_value())
+        result += password->size() + 2;
 
     return result;
 }

diff --git a/types.h b/types.h
@@ -17,6 +17,7 @@ See LICENSE for license details.
 #include <memory>
 #include <chrono>
 #include <vector>
+#include <optional>
 
 #include "forward_declarations.h"
 
@@ -311,8 +312,8 @@ struct Connect
     bool clean_start = true;
     bool bridgeProtocolBit = false;
     std::string clientid;
-    std::string username;
-    std::string password;
+    std::optional<std::string> username;
+    std::optional<std::string> password;
     uint16_t keepalive = 60;
     std::shared_ptr<WillPublish> will;
     std::shared_ptr<Mqtt5PropertyBuilder> propertyBuilder;