Merge bitcoin/bitcoin#32423: rpc: Undeprecate rpcuser/rpcpassword, store all credentials hashed in memory

e49a727 rpc: Avoid join-split roundtrip for user:pass for auth credentials (Vasil Dimov)
98ff38a rpc: Perform HTTP user:pass split once in `RPCAuthorized` (laanwj)
879a17b rpc: Store all credentials hashed in memory (laanwj)
4ab9bed rpc: Undeprecate rpcuser/rpcpassword, change message to security warning (laanwj)

Pull request description:

  This PR does two things:

  ### Undeprecate rpcuser/rpcpassword, change message to security warning

  Back in 2015, in bitcoin/bitcoin#7044, we added configuration option `rpcauth` for multiple RPC users. At the same time the old settings for single-user configuration `rpcuser` and `rpcpassword` were "soon" to be deprecated.

  The main reason for this deprecation is that while `rpcpassword` stores the password in plain text, `rpcauth` stores a hash, so it doesn't appear in the configuration in plain text.

  As the options are still in active use, actually removing them is expected to be a hassle to many, and it's not clear that is worth it. As for the security risk, in many kinds of setups (no wallet, containerized, single-user-single-application, local-only, etc) it is an unlikely point of escalation.

  In the end, it is good to encourage secure practices, but it is the responsibility of the user. Log a clear warning but remove the deprecation notice (this is also the only place where the options appear as deprecated, they were never marked as such in the -help output).

  <hr>

  ### Store all credentials hashed in memory

  This gets rid of the special-casing of `strRPCUserColonPass` by hashing cookies as well as manually provided `-rpcuser`/`-rpcpassword` with a random salt before storing them.

  Also take the opportunity to modernize the surrounding code a bit. There should be no end-user visible differences in behavior.

  <hr>

  Closes #29240.

ACKs for top commit:
  1440000bytes:
    utACK bitcoin/bitcoin@e49a727
  janb84:
    reACK bitcoin/bitcoin@e49a727
  vasild:
    ACK e49a727

Tree-SHA512: 7162848ada4545bc07b5843d1ab6fb7e31fb26de8d6385464b7c166491cd122eac2ec5e70887c414fc136600482df8277dc0cc0541d7b7cf62c4f72e25bb6145

Author: ryanofsky

src/httprpc.cpp

@@ -69,8 +69,6 @@ class HTTPRPCTimerInterface : public RPCTimerInterface
 };
 
 
-/* Pre-base64-encoded authentication token */
-static std::string strRPCUserColonPass;
 /* Stored RPC timer interface (for unregistration) */
 static std::unique_ptr<HTTPRPCTimerInterface> httpRPCTimerInterface;
 /* List of -rpcauth values */
@@ -101,31 +99,21 @@ static void JSONErrorReply(HTTPRequest* req, UniValue objError, const JSONRPCReq
 
 //This function checks username and password against -rpcauth
 //entries from config file.
-static bool multiUserAuthorized(std::string strUserPass)
+static bool CheckUserAuthorized(std::string_view user, std::string_view pass)
 {
-    if (strUserPass.find(':') == std::string::npos) {
-        return false;
-    }
-    std::string strUser = strUserPass.substr(0, strUserPass.find(':'));
-    std::string strPass = strUserPass.substr(strUserPass.find(':') + 1);
-
-    for (const auto& vFields : g_rpcauth) {
-        std::string strName = vFields[0];
-        if (!TimingResistantEqual(strName, strUser)) {
+    for (const auto& fields : g_rpcauth) {
+        if (!TimingResistantEqual(std::string_view(fields[0]), user)) {
             continue;
         }
 
-        std::string strSalt = vFields[1];
-        std::string strHash = vFields[2];
+        const std::string& salt = fields[1];
+        const std::string& hash = fields[2];
 
-        static const unsigned int KEY_SIZE = 32;
-        unsigned char out[KEY_SIZE];
+        std::array<unsigned char, CHMAC_SHA256::OUTPUT_SIZE> out;
+        CHMAC_SHA256(UCharCast(salt.data()), salt.size()).Write(UCharCast(pass.data()), pass.size()).Finalize(out.data());
+        std::string hash_from_pass = HexStr(out);
 
-        CHMAC_SHA256(reinterpret_cast<const unsigned char*>(strSalt.data()), strSalt.size()).Write(reinterpret_cast<const unsigned char*>(strPass.data()), strPass.size()).Finalize(out);
-        std::vector<unsigned char> hexvec(out, out+KEY_SIZE);
-        std::string strHashFromPass = HexStr(hexvec);
-
-        if (TimingResistantEqual(strHashFromPass, strHash)) {
+        if (TimingResistantEqual(hash_from_pass, hash)) {
             return true;
         }
     }
@@ -142,15 +130,14 @@ static bool RPCAuthorized(const std::string& strAuth, std::string& strAuthUserna
     if (!userpass_data) return false;
     strUserPass.assign(userpass_data->begin(), userpass_data->end());
 
-    if (strUserPass.find(':') != std::string::npos)
-        strAuthUsernameOut = strUserPass.substr(0, strUserPass.find(':'));
-
-    // Check if authorized under single-user field.
-    // (strRPCUserColonPass is empty when -norpccookiefile is specified).
-    if (!strRPCUserColonPass.empty() && TimingResistantEqual(strUserPass, strRPCUserColonPass)) {
-        return true;
+    size_t colon_pos = strUserPass.find(':');
+    if (colon_pos == std::string::npos) {
+        return false; // Invalid basic auth.
     }
-    return multiUserAuthorized(strUserPass);
+    std::string user = strUserPass.substr(0, colon_pos);
+    std::string pass = strUserPass.substr(colon_pos + 1);
+    strAuthUsernameOut = user;
+    return CheckUserAuthorized(user, pass);
 }
 
 static bool HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req)
@@ -291,6 +278,9 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req)
 
 static bool InitRPCAuthentication()
 {
+    std::string user;
+    std::string pass;
+
     if (gArgs.GetArg("-rpcpassword", "") == "")
     {
         std::optional<fs::perms> cookie_perms{std::nullopt};
@@ -304,18 +294,36 @@ static bool InitRPCAuthentication()
             cookie_perms = *perm_opt;
         }
 
-        assert(strRPCUserColonPass.empty()); // Only support initializing once
-        if (!GenerateAuthCookie(&strRPCUserColonPass, cookie_perms)) {
+        switch (GenerateAuthCookie(cookie_perms, user, pass)) {
+        case GenerateAuthCookieResult::ERR:
             return false;
-        }
-        if (strRPCUserColonPass.empty()) {
+        case GenerateAuthCookieResult::DISABLED:
             LogInfo("RPC authentication cookie file generation is disabled.");
-        } else {
+            break;
+        case GenerateAuthCookieResult::OK:
             LogInfo("Using random cookie authentication.");
+            break;
         }
     } else {
-        LogPrintf("Config options rpcuser and rpcpassword will soon be deprecated. Locally-run instances may remove rpcuser to use cookie-based auth, or may be replaced with rpcauth. Please see share/rpcauth for rpcauth auth generation.\n");
-        strRPCUserColonPass = gArgs.GetArg("-rpcuser", "") + ":" + gArgs.GetArg("-rpcpassword", "");
+        LogInfo("Using rpcuser/rpcpassword authentication.");
+        LogWarning("The use of rpcuser/rpcpassword is less secure, because credentials are configured in plain text. It is recommended that locally-run instances switch to cookie-based auth, or otherwise to use hashed rpcauth credentials. See share/rpcauth in the source directory for more information.");
+        user = gArgs.GetArg("-rpcuser", "");
+        pass = gArgs.GetArg("-rpcpassword", "");
+    }
+
+    // If there is a plaintext credential, hash it with a random salt before storage.
+    if (!user.empty() || !pass.empty()) {
+        // Generate a random 16 byte hex salt.
+        std::array<unsigned char, 16> raw_salt;
+        GetStrongRandBytes(raw_salt);
+        std::string salt = HexStr(raw_salt);
+
+        // Compute HMAC.
+        std::array<unsigned char, CHMAC_SHA256::OUTPUT_SIZE> out;
+        CHMAC_SHA256(UCharCast(salt.data()), salt.size()).Write(UCharCast(pass.data()), pass.size()).Finalize(out.data());
+        std::string hash = HexStr(out);
+
+        g_rpcauth.push_back({user, salt, hash});
     }
 
     if (!gArgs.GetArgs("-rpcauth").empty()) {

src/rpc/request.cpp

@@ -97,50 +97,52 @@ static fs::path GetAuthCookieFile(bool temp=false)
 
 static bool g_generated_cookie = false;
 
-bool GenerateAuthCookie(std::string* cookie_out, std::optional<fs::perms> cookie_perms)
+GenerateAuthCookieResult GenerateAuthCookie(const std::optional<fs::perms>& cookie_perms,
+                                            std::string& user,
+                                            std::string& pass)
 {
     const size_t COOKIE_SIZE = 32;
     unsigned char rand_pwd[COOKIE_SIZE];
     GetRandBytes(rand_pwd);
-    std::string cookie = COOKIEAUTH_USER + ":" + HexStr(rand_pwd);
+    const std::string rand_pwd_hex{HexStr(rand_pwd)};
 
     /** the umask determines what permissions are used to create this file -
      * these are set to 0077 in common/system.cpp.
      */
     std::ofstream file;
     fs::path filepath_tmp = GetAuthCookieFile(true);
     if (filepath_tmp.empty()) {
-        return true; // -norpccookiefile
+        return GenerateAuthCookieResult::DISABLED; // -norpccookiefile
     }
     file.open(filepath_tmp);
     if (!file.is_open()) {
         LogWarning("Unable to open cookie authentication file %s for writing", fs::PathToString(filepath_tmp));
-        return false;
+        return GenerateAuthCookieResult::ERR;
     }
-    file << cookie;
+    file << COOKIEAUTH_USER << ":" << rand_pwd_hex;
     file.close();
 
     fs::path filepath = GetAuthCookieFile(false);
     if (!RenameOver(filepath_tmp, filepath)) {
         LogWarning("Unable to rename cookie authentication file %s to %s", fs::PathToString(filepath_tmp), fs::PathToString(filepath));
-        return false;
+        return GenerateAuthCookieResult::ERR;
     }
     if (cookie_perms) {
         std::error_code code;
         fs::permissions(filepath, cookie_perms.value(), fs::perm_options::replace, code);
         if (code) {
             LogWarning("Unable to set permissions on cookie authentication file %s", fs::PathToString(filepath));
-            return false;
+            return GenerateAuthCookieResult::ERR;
         }
     }
 
     g_generated_cookie = true;
     LogInfo("Generated RPC authentication cookie %s\n", fs::PathToString(filepath));
     LogInfo("Permissions used for cookie: %s\n", PermsToSymbolicString(fs::status(filepath).permissions()));
 
-    if (cookie_out)
-        *cookie_out = cookie;
-    return true;
+    user = COOKIEAUTH_USER;
+    pass = rand_pwd_hex;
+    return GenerateAuthCookieResult::OK;
 }
 
 bool GetAuthCookie(std::string *cookie_out)

src/rpc/request.h

@@ -23,8 +23,25 @@ UniValue JSONRPCRequestObj(const std::string& strMethod, const UniValue& params,
 UniValue JSONRPCReplyObj(UniValue result, UniValue error, std::optional<UniValue> id, JSONRPCVersion jsonrpc_version);
 UniValue JSONRPCError(int code, const std::string& message);
 
-/** Generate a new RPC authentication cookie and write it to disk */
-bool GenerateAuthCookie(std::string* cookie_out, std::optional<fs::perms> cookie_perms=std::nullopt);
+enum class GenerateAuthCookieResult : uint8_t {
+    DISABLED, // -norpccookiefile
+    ERR,
+    OK,
+};
+
+/**
+ * Generate a new RPC authentication cookie and write it to disk
+ * @param[in] cookie_perms Filesystem permissions to use for the cookie file.
+ * @param[out] user Generated username, only set if `OK` is returned.
+ * @param[out] pass Generated password, only set if `OK` is returned.
+ * @retval GenerateAuthCookieResult::DISABLED Authentication via cookie is disabled.
+ * @retval GenerateAuthCookieResult::ERROR Error occurred, auth data could not be saved to disk.
+ * @retval GenerateAuthCookieResult::OK Auth data was generated, saved to disk and in `user` and `pass`.
+ */
+GenerateAuthCookieResult GenerateAuthCookie(const std::optional<fs::perms>& cookie_perms,
+                                            std::string& user,
+                                            std::string& pass);
+
 /** Read the RPC authentication cookie from disk */
 bool GetAuthCookie(std::string *cookie_out);
 /** Delete RPC authentication cookie from disk */