MXS-2220 Move monitor user and password fields to internal class

Also changed the lengths of the buffers.

include/maxscale/server.hh

@@ -61,8 +61,8 @@ class SERVER
 {
 public:
     static const int MAX_ADDRESS_LEN = 1024;
-    static const int MAX_MONUSER_LEN = 1024;
+    static const int MAX_MONUSER_LEN = 512;
-    static const int MAX_MONPW_LEN = 1024;
+    static const int MAX_MONPW_LEN = 512;
     static const int MAX_VERSION_LEN = 256;
     static const int RLAG_UNDEFINED = -1;   // Default replication lag value
 
@@ -87,8 +87,6 @@ public:
     int  extra_port = -1;                     /**< Alternative monitor port if normal port fails */
 
     // Other settings
-    char monuser[MAX_MONUSER_LEN] = {'\0'};     /**< Monitor username, overrides monitor setting */
-    char monpw[MAX_MONPW_LEN] = {'\0'};         /**< Monitor password, overrides monitor setting  */
     bool proxy_protocol = false;                /**< Send proxy-protocol header to backends when connecting
                                                  *   routing sessions. */
 
@@ -468,8 +466,6 @@ extern void    server_clear_set_status_nolock(SERVER* server, uint64_t bits_to_c
 extern void    server_set_status_nolock(SERVER* server, uint64_t bit);
 extern void    server_clear_status_nolock(SERVER* server, uint64_t bit);
 extern void    server_transfer_status(SERVER* dest_server, const SERVER* source_server);
-extern void    server_add_mon_user(SERVER* server, const char* user, const char* passwd);
-extern void    server_update_credentials(SERVER* server, const char* user, const char* passwd);
 extern void     server_update_address(SERVER* server, const char* address);
 extern uint64_t server_map_status(const char* str);
 

server/core/config_runtime.cc

@@ -495,11 +495,11 @@ bool runtime_alter_server(Server* server, const char* key, const char* value)
     }
     else if (strcmp(key, CN_MONITORUSER) == 0)
     {
-        server_update_credentials(server, value, server->monpw);
+        server->set_monitor_user(value);
     }
     else if (strcmp(key, CN_MONITORPW) == 0)
     {
-        server_update_credentials(server, server->monuser, value);
+        server->set_monitor_password(value);
     }
     else if (strcmp(key, CN_PERSISTPOOLMAX) == 0)
     {

server/core/internal/server.hh

@@ -252,6 +252,27 @@ public:
      */
     bool serialize() const;
 
+    /**
+     * Update server-specific monitor username. Does not affect existing monitor connections,
+     * only new connections will use the updated username.
+     *
+     * @param username New username. Must not be too long.
+     * @return True, if value was updated
+     */
+    bool set_monitor_user(const std::string& user);
+
+    /**
+     * Update server-specific monitor password. Does not affect existing monitor connections,
+     * only new connections will use the updated password.
+     *
+     * @param password New password. Must not be too long.
+     * @return True, if value was updated
+     */
+    bool set_monitor_password(const std::string& password);
+
+    std::string monitor_user() const;
+    std::string monitor_password() const;
+
     mutable std::mutex m_lock;
     DCB**              persistent = nullptr; /**< List of unused persistent connections to the server */
 
@@ -260,21 +281,26 @@ private:
     {
         mutable std::mutex lock; /**< Protects array-like settings from concurrent access */
 
-        std::string protocol;      /**< Backend protocol module name */
+        /** All config settings in text form. This is only read and written from the admin thread
-        std::string authenticator; /**< Authenticator module name */
+         *  so no need for locking. */
+        std::vector<ConfigParameter> all_parameters;
+
+        std::string protocol;      /**< Backend protocol module name. Does not change so needs no locking. */
+        std::string authenticator; /**< Authenticator module name. Does not change so needs no locking. */
+
+        char monuser[MAX_MONUSER_LEN + 1] = {'\0'}; /**< Monitor username, overrides monitor setting */
+        char monpw[MAX_MONPW_LEN + 1] = {'\0'};     /**< Monitor password, overrides monitor setting */
+
+        long persistpoolmax = 0; /**< Maximum size of persistent connections pool */
+        long persistmaxtime = 0; /**< Maximum number of seconds connection can live */
 
         /** Disk space thresholds. Can be queried from modules at any time so access must be protected
          *  by mutex. */
         MxsDiskSpaceThreshold disk_space_limits;
-        /** All config settings in text form. This is only read and written from the admin thread
+
-         *  so no need for locking. */
-        std::vector<ConfigParameter> all_parameters;
         /** Additional custom parameters which may affect routing decisions or the monitor module.
          *  Can be queried from modules at any time so access must be protected by mutex. */
         std::map<std::string, std::string> custom_parameters;
-
-        long persistpoolmax = 0; /**< Maximum size of persistent connections pool */
-        long persistmaxtime = 0; /**< Maximum number of seconds connection can live */
     };
 
     /**
@@ -297,10 +323,10 @@ private:
         std::string version_string() const;
 
     private:
-        mutable std::mutex m_lock;                    /**< Protects against concurrent writing */
+        mutable std::mutex m_lock;                        /**< Protects against concurrent writing */
-        Version m_version_num;                        /**< Numeric version */
+        Version m_version_num;                            /**< Numeric version */
-        Type m_type = Type::MARIADB;                  /**< Server type */
+        Type m_type = Type::MARIADB;                      /**< Server type */
-        char m_version_str[MAX_VERSION_LEN] = {'\0'}; /**< Server version string */
+        char m_version_str[MAX_VERSION_LEN + 1] = {'\0'}; /**< Server version string */
     };
 
     const std::string m_name;             /**< Server config name */

server/core/monitor.cc

@@ -47,6 +47,7 @@
 #include "internal/externcmd.hh"
 #include "internal/monitor.hh"
 #include "internal/modules.hh"
+#include "internal/server.hh"
 
 /** Schema version, journals must have a matching version */
 #define MMB_SCHEMA_VERSION 2
@@ -1139,7 +1140,8 @@ static void mon_append_node_names(MXS_MONITOR* mon,
 
     while (servers && len)
     {
-        if (status == 0 || servers->server->status & status)
+        Server* server = static_cast<Server*>(servers->server);
+        if (status == 0 || server->status & status)
         {
             if (approach == CREDENTIALS_EXCLUDE)
             {
@@ -1147,32 +1149,28 @@ static void mon_append_node_names(MXS_MONITOR* mon,
                          sizeof(arr),
                          "%s[%s]:%d",
                          separator,
-                         servers->server->address,
+                         server->address,
-                         servers->server->port);
+                         server->port);
             }
             else
             {
-                const char* user;
+                string user = mon->user;
-                const char* password;
+                string password = mon->password;
-                if (*servers->server->monuser)
+                string server_specific_monuser = server->monitor_user();
+                if (!server_specific_monuser.empty())
                 {
-                    user = servers->server->monuser;
+                    user = server_specific_monuser;
-                    password = servers->server->monpw;
+                    password = server->monitor_password();
-                }
-                else
-                {
-                    user = mon->user;
-                    password = mon->password;
                 }
 
                 snprintf(arr,
                          sizeof(arr),
                          "%s%s:%s@[%s]:%d",
                          separator,
-                         user,
+                         user.c_str(),
-                         password,
+                         password.c_str(),
-                         servers->server->address,
+                         server->address,
-                         servers->server->port);
+                         server->port);
             }
 
             separator = ",";
@@ -1468,16 +1466,17 @@ mxs_connect_result_t mon_ping_or_connect_to_db(MXS_MONITOR* mon, MXS_MONITORED_S
     mxs_connect_result_t conn_result = MONITOR_CONN_REFUSED;
     if ((database->con = mysql_init(NULL)))
     {
-        char* uname = mon->user;
+        string uname = mon->user;
-        char* passwd = mon->password;
+        string passwd = mon->password;
-
+        Server* server = static_cast<Server*>(database->server); // Clean this up later.
-        if (database->server->monuser[0] && database->server->monpw[0])
+        string server_specific_monuser = server->monitor_user();
+        if (!server_specific_monuser.empty())
         {
-            uname = database->server->monuser;
+            uname = server_specific_monuser;
-            passwd = database->server->monpw;
+            passwd = server->monitor_password();
         }
 
-        char* dpwd = decrypt_password(passwd);
+        char* dpwd = decrypt_password(passwd.c_str());
 
         mysql_optionsv(database->con, MYSQL_OPT_CONNECT_TIMEOUT, (void*) &mon->connect_timeout);
         mysql_optionsv(database->con, MYSQL_OPT_READ_TIMEOUT, (void*) &mon->read_timeout);
@@ -1489,7 +1488,8 @@ mxs_connect_result_t mon_ping_or_connect_to_db(MXS_MONITOR* mon, MXS_MONITORED_S
         for (int i = 0; i < mon->connect_attempts; i++)
         {
             start = time(NULL);
-            bool result = (mxs_mysql_real_connect(database->con, database->server, uname, dpwd) != NULL);
+            bool result = (mxs_mysql_real_connect(database->con, database->server, uname.c_str(), dpwd) !=
+                           NULL);
             end = time(NULL);
 
             if (result)

server/core/server.cc

@@ -83,6 +83,7 @@ const char ERR_CANNOT_MODIFY[] = "The server is monitored, so only the maintenan
                                  "set/cleared manually. Status was not modified.";
 const char WRN_REQUEST_OVERWRITTEN[] = "Previous maintenance request was not yet read by the monitor "
                                        "and was overwritten.";
+const char ERR_TOO_LONG_CONFIG_VALUE[] = "The new value for %s is too long. Maximum length is %i characters.";
 
 // Converts Server::ConfigParam to MXS_CONFIG_PARAM and keeps them in the same order. Required for some
 // functions taking MXS_CONFIG_PARAMs as arguments.
@@ -126,23 +127,24 @@ private:
 
 /**
  * Write to char array by first zeroing any extra space. This reduces effects of concurrent reading.
+ * Concurrent writing should be prevented by the caller.
  *
- * @param dest Destination buffer. The buffer is assumed to contains at least \0 at the end.
+ * @param dest Destination buffer. The buffer is assumed to contains at least a \0 at the end.
- * @param dest_size Maximum size of destination buffer, including terminating \0.
+ * @param max_len Size of destination buffer - 1. The last element (max_len + 1) is never written to.
- * @param source Source string. A maximum of @c dest_size - 1 characters are copied.
+ * @param source Source string. A maximum of @c max_len characters are copied.
  */
-void careful_strcpy(char* dest, size_t dest_size, const std::string& source)
+void careful_strcpy(char* dest, size_t max_len, const std::string& source)
 {
     // The string may be accessed while we are updating it.
     // Take some precautions to ensure that the string cannot be completely garbled at any point.
     // Strictly speaking, this is not fool-proof as writes may not appear in order to the reader.
-    size_t old_len = strlen(dest);
     size_t new_len = source.length();
-    if (new_len >= dest_size)
+    if (new_len > max_len)
     {
-        new_len = dest_size - 1; // Need space for the \0.
+        new_len = max_len;
     }
 
+    size_t old_len = strlen(dest);
     if (new_len < old_len)
     {
         // If the new string is shorter, zero out the excess data.
@@ -233,7 +235,8 @@ Server* Server::server_alloc(const char* name, MXS_CONFIG_PARAMETER* params)
 
     if (*monuser && *monpw)
     {
-        server_add_mon_user(server, monuser, monpw);
+        server->set_monitor_user(monuser);
+        server->set_monitor_password(monpw);
     }
 
     for (MXS_CONFIG_PARAMETER* p = params; p; p = p->next)
@@ -781,54 +784,44 @@ void server_transfer_status(SERVER* dest_server, const SERVER* source_server)
     dest_server->status = source_server->status;
 }
 
-/**
+bool Server::set_monitor_user(const string& username)
- * Add a user name and password to use for monitoring the
- * state of the server.
- *
- * @param server        The server to update
- * @param user          The user name to use
- * @param passwd        The password of the user
- */
-void server_add_mon_user(SERVER* server, const char* user, const char* passwd)
 {
-    if (user != server->monuser
+    bool rval = false;
-        && snprintf(server->monuser, sizeof(server->monuser), "%s", user) > (int)sizeof(server->monuser))
+    if (username.length() <= MAX_MONUSER_LEN)
     {
-        MXS_WARNING("Truncated monitor user for server '%s', maximum username "
+        careful_strcpy(m_settings.monuser, MAX_MONUSER_LEN, username);
-                    "length is %lu characters.",
+        rval = true;
-                    server->name(),
-                    sizeof(server->monuser));
     }
-
+    else
-    if (passwd != server->monpw
-        && snprintf(server->monpw, sizeof(server->monpw), "%s", passwd) > (int)sizeof(server->monpw))
     {
-        MXS_WARNING("Truncated monitor password for server '%s', maximum password "
+        MXS_ERROR(ERR_TOO_LONG_CONFIG_VALUE, CN_MONITORUSER, MAX_MONUSER_LEN);
-                    "length is %lu characters.",
-                    server->name(),
-                    sizeof(server->monpw));
     }
+    return rval;
 }
 
-/**
+bool Server::set_monitor_password(const string& password)
- * Check and update a server definition following a configuration
- * update. Changes will not affect any current connections to this
- * server, however all new connections will use the new settings.
- *
- * If the new settings are different from those already applied to the
- * server then a message will be written to the log.
- *
- * @param server        The server to update
- * @param protocol      The new protocol for the server
- * @param user          The monitor user for the server
- * @param passwd        The password to use for the monitor user
- */
-void server_update_credentials(SERVER* server, const char* user, const char* passwd)
 {
-    if (user != NULL && passwd != NULL)
+    bool rval = false;
+    if (password.length() <= MAX_MONPW_LEN)
     {
-        server_add_mon_user(server, user, passwd);
+        careful_strcpy(m_settings.monpw, MAX_MONPW_LEN, password);
+        rval = true;
     }
+    else
+    {
+        MXS_ERROR(ERR_TOO_LONG_CONFIG_VALUE, CN_MONITORPW, MAX_MONPW_LEN);
+    }
+    return rval;
+}
+
+string Server::monitor_user() const
+{
+    return m_settings.monuser;
+}
+
+string Server::monitor_password() const
+{
+    return m_settings.monpw;
 }
 
 void Server::set_parameter(const string& name, const string& value)