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

pull laanwj wants to merge 4 commits into bitcoin:master from laanwj:2025-05-remove-rpcpassword-deprecation changing 3 files +75 −48
  1. laanwj commented at 9:31 am on May 6, 2025: member

    This PR does two things:

    Undeprecate rpcuser/rpcpassword, change message to security warning

    Back in 2015, in #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).

    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.

    Closes #29240.

  2. laanwj added the label RPC/REST/ZMQ on May 6, 2025
  3. DrahtBot commented at 9:31 am on May 6, 2025: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32423.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK Sjors
    Stale ACK janb84, vasild

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  4. laanwj force-pushed on May 6, 2025
  5. in src/httprpc.cpp:318 in 1b28314259 outdated
    313@@ -314,7 +314,8 @@ static bool InitRPCAuthentication()
    314             LogInfo("Using random cookie authentication.");
    315         }
    316     } else {
    317-        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");
    318+        LogInfo("Using rpcuser/rpcpassword authentication.");
    319+        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 for more information.\n");
    


    maflcko commented at 9:46 am on May 6, 2025:
    nit in 1b28314259952b70071eeeae11867e921729c4c7: Can drop the trailing newline, if you retouch.
  6. in src/httprpc.cpp:329 in 2392b43247 outdated
    328+        GetStrongRandBytes(raw_salt);
    329+        std::string salt = HexStr(raw_salt);
    330+
    331+        // Compute HMAC.
    332+        std::array<unsigned char,  CHMAC_SHA256::OUTPUT_SIZE> out;
    333+        CHMAC_SHA256(reinterpret_cast<const unsigned char*>(salt.data()), salt.size()).Write(reinterpret_cast<const unsigned char*>(pass.data()), pass.size()).Finalize(out.data());
    


    maflcko commented at 9:51 am on May 6, 2025:
    nit in 2392b4324777e002dc5363bac7f85cc6f8586818: For new code, UCharCast may be better than a “raw” cast, but only if you re-touch.
  7. maflcko commented at 9:56 am on May 6, 2025: member
    concept ack
  8. in src/httprpc.cpp:102 in 2392b43247 outdated
     98@@ -101,31 +99,27 @@ static void JSONErrorReply(HTTPRequest* req, UniValue objError, const JSONRPCReq
     99 
    100 //This function checks username and password against -rpcauth
    101 //entries from config file.
    102-static bool multiUserAuthorized(std::string strUserPass)
    103+static bool CheckUserAuthorized(std::string user_pass)
    


    laanwj commented at 10:14 am on May 6, 2025:
    self-nit: This could be std::string_view as well. Or const std::string& at least.
  9. laanwj force-pushed on May 6, 2025
  10. laanwj commented at 10:28 am on May 6, 2025: member
    2392b4324777e002dc5363bac7f85cc6f8586818 → 3b0366d1775d6c00844ff0c90542814bf641bb18 Use UCharCast, no newline on log message, pass in std::string_view, some & code formatting
  11. in src/httprpc.cpp:104 in 3b0366d177 outdated
     98@@ -101,31 +99,27 @@ static void JSONErrorReply(HTTPRequest* req, UniValue objError, const JSONRPCReq
     99 
    100 //This function checks username and password against -rpcauth
    101 //entries from config file.
    102-static bool multiUserAuthorized(std::string strUserPass)
    103+static bool CheckUserAuthorized(std::string_view user_pass)
    104 {
    105-    if (strUserPass.find(':') == std::string::npos) {
    106+    if (user_pass.find(':') == std::string::npos) {
    


    laanwj commented at 11:43 am on May 6, 2025:

    i intentionally kept this as-is (eg converting to use Split slightly changes semantics), but thinking of it, calling user_pass.find(':')three times in a row is maybe a bit much.

    Edit: heck, in the calling function it already does user_pass.find(':'), might as well do it there and pass in user and password seperately.

  12. Sjors commented at 12:14 pm on May 6, 2025: member

    Hah, I was planning on doing this undeprecating this morning, but couldn’t find the deprecation message, so I figured someone already did it.

    Concept ACK

  13. laanwj force-pushed on May 6, 2025
  14. yancyribbens commented at 4:59 pm on May 6, 2025: contributor

    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

    This probably applies to testnet and regtest too.

  15. laanwj commented at 10:15 am on May 8, 2025: member

    This probably applies to testnet and regtest too.

    Yes, for quick testing, not having to use an external script to create credentials is more convenient too. Usually cookie-based auth will suffice, but not always when dealing with external applications that need a username/password.

  16. janb84 commented at 11:48 am on May 8, 2025: contributor

    tACK 3acfc07

    • code review ✅
    • compiled, run all tests ✅
    • tested on regtest with user/pass & did remote curl call to test rpc user&pass func. ✅
  17. DrahtBot requested review from Sjors on May 8, 2025
  18. in src/httprpc.cpp:306 in 3acfc071c3 outdated
    306         }
    307     } else {
    308-        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");
    309-        strRPCUserColonPass = gArgs.GetArg("-rpcuser", "") + ":" + gArgs.GetArg("-rpcpassword", "");
    310+        LogInfo("Using rpcuser/rpcpassword authentication.");
    311+        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 for more information.");
    


    vasild commented at 11:47 am on May 9, 2025:
    It is like this in master, but the meaning of share/rpcauth may not be obvious. Especially considering that some distros may not install it in their packages. Is not installed by our cmake --install BUILDDIR either. Maybe change to “See share/rpcauth in the source directory for more information”.

    vasild commented at 11:51 am on May 9, 2025:

    laanwj commented at 2:23 pm on May 9, 2025:
    Maybe; we never added the deprecation notice there, so i’m not convinced i should touch that now.

    laanwj commented at 2:28 pm on May 9, 2025:
    Agree.
  19. in src/httprpc.cpp:326 in 3acfc071c3 outdated
    326+        std::array<unsigned char, 16> raw_salt;
    327+        GetStrongRandBytes(raw_salt);
    328+        std::string salt = HexStr(raw_salt);
    329+
    330+        // Compute HMAC.
    331+        std::array<unsigned char,  CHMAC_SHA256::OUTPUT_SIZE> out;
    


    vasild commented at 12:19 pm on May 9, 2025:

    nit:

    0        std::array<unsigned char, CHMAC_SHA256::OUTPUT_SIZE> out;
    
  20. in src/httprpc.cpp:104 in 3acfc071c3 outdated
    109-    std::string strPass = strUserPass.substr(strUserPass.find(':') + 1);
    110-
    111-    for (const auto& vFields : g_rpcauth) {
    112-        std::string strName = vFields[0];
    113-        if (!TimingResistantEqual(strName, strUser)) {
    114+    for (const auto& fields : g_rpcauth) {
    


    vasild commented at 12:26 pm on May 9, 2025:

    Just noting: the way this works (unchanged by this PR) is that it allows duplicate usernames with different passwords (e.g. joe:123 and joe:456) and would allow login with either password (123 or 456).

    (just a comment, feel free to resolve right away)


    laanwj commented at 2:25 pm on May 9, 2025:
    Yes, this was and is still alllowed. Not sure it’s a problem, if people want multiple passwords for one username they can do so, it doesn’t open any security hole. In any case doesn’t seem in scope of this change, which intentionally doesn’t change user-visible behavior.
  21. in src/httprpc.cpp:318 in 3acfc071c3 outdated
    304         } else {
    305             LogInfo("Using random cookie authentication.");
    306         }
    307     } else {
    308-        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");
    309-        strRPCUserColonPass = gArgs.GetArg("-rpcuser", "") + ":" + gArgs.GetArg("-rpcpassword", "");
    


    vasild commented at 12:46 pm on May 9, 2025:

    I did not test to confirm this, just read the code, but before this PR if -rpcuser=joe -rpcpassword=abc:def was configured, it would have been possible to login with user joe:abc and password def.

    (just a comment, feel free to resolve right away)


    laanwj commented at 2:26 pm on May 9, 2025:
    Agree, it’s no longer possible to have : in -rpcpassword after this due to the choice to split them using Split(). I could change that code, though : in -rpcuser should really be invalid as the spec doesn’t allow it.
  22. in src/httprpc.cpp:318 in 3acfc071c3 outdated
    318+        if (fields.size() != 2) {
    319+            LogError("Unable to parse RPC credentials. The configured rpcuser or rpcpassword cannot contain a \":\".");
    320+            return false;
    321+        }
    322+        const std::string& user = fields[0];
    323+        const std::string& pass = fields[1];
    


    vasild commented at 1:25 pm on May 9, 2025:

    This looks like a new restriction. Could it introduce a breakage for users that have -rpcpassword=contains:colon?

    It is sub-optimal to join/craft the string “user:pass” and to split/parse it a few lines later. Better store the user and pass in separate variables, like this:

      0diff --git i/src/httprpc.cpp w/src/httprpc.cpp
      1index ed5a0253c6..493f1b0320 100644
      2--- i/src/httprpc.cpp
      3+++ w/src/httprpc.cpp
      4@@ -275,13 +275,14 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req)
      5     }
      6     return true;
      7 }
      8 
      9 static bool InitRPCAuthentication()
     10 {
     11-    std::string user_colon_pass;
     12+    std::string user;
     13+    std::string pass;
     14 
     15     if (gArgs.GetArg("-rpcpassword", "") == "")
     16     {
     17         std::optional<fs::perms> cookie_perms{std::nullopt};
     18         auto cookie_perms_arg{gArgs.GetArg("-rpccookieperms")};
     19         if (cookie_perms_arg) {
     20@@ -290,36 +291,31 @@ static bool InitRPCAuthentication()
     21                 LogError("Invalid -rpccookieperms=%s; must be one of 'owner', 'group', or 'all'.", *cookie_perms_arg);
     22                 return false;
     23             }
     24             cookie_perms = *perm_opt;
     25         }
     26 
     27-        if (!GenerateAuthCookie(&user_colon_pass, cookie_perms)) {
     28+        switch (GenerateAuthCookie(cookie_perms, user, pass)) {
     29+        case GenerateAuthCookieResult::ERROR:
     30             return false;
     31-        }
     32-        if (user_colon_pass.empty()) {
     33+        case GenerateAuthCookieResult::DISABLED:
     34             LogInfo("RPC authentication cookie file generation is disabled.");
     35-        } else {
     36+            break;
     37+        case GenerateAuthCookieResult::OK:
     38             LogInfo("Using random cookie authentication.");
     39+            break;
     40         }
     41     } else {
     42         LogInfo("Using rpcuser/rpcpassword authentication.");
     43         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 for more information.");
     44-        user_colon_pass = gArgs.GetArg("-rpcuser", "") + ":" + gArgs.GetArg("-rpcpassword", "");
     45+        user = gArgs.GetArg("-rpcuser", "");
     46+        pass = gArgs.GetArg("-rpcpassword", "");
     47     }
     48 
     49     // If there is a plaintext credential, hash it with a random salt before storage.
     50-    if (!user_colon_pass.empty()) {
     51-        std::vector<std::string> fields{SplitString(user_colon_pass, ':')};
     52-        if (fields.size() != 2) {
     53-            LogError("Unable to parse RPC credentials. The configured rpcuser or rpcpassword cannot contain a \":\".");
     54-            return false;
     55-        }
     56-        const std::string& user = fields[0];
     57-        const std::string& pass = fields[1];
     58-
     59+    if (!user.empty() || !pass.empty()) {
     60         // Generate a random 16 byte hex salt.
     61         std::array<unsigned char, 16> raw_salt;
     62         GetStrongRandBytes(raw_salt);
     63         std::string salt = HexStr(raw_salt);
     64 
     65         // Compute HMAC.
     66diff --git i/src/rpc/request.cpp w/src/rpc/request.cpp
     67index 30f4a4f51b..249c6cb621 100644
     68--- i/src/rpc/request.cpp
     69+++ w/src/rpc/request.cpp
     70@@ -94,56 +94,59 @@ static fs::path GetAuthCookieFile(bool temp=false)
     71     }
     72     return AbsPathForConfigVal(gArgs, arg);
     73 }
     74 
     75 static bool g_generated_cookie = false;
     76 
     77-bool GenerateAuthCookie(std::string* cookie_out, std::optional<fs::perms> cookie_perms)
     78+GenerateAuthCookieResult GenerateAuthCookie(const std::optional<fs::perms>& cookie_perms,
     79+                                            std::string& user,
     80+                                            std::string& pass)
     81 {
     82     const size_t COOKIE_SIZE = 32;
     83     unsigned char rand_pwd[COOKIE_SIZE];
     84     GetRandBytes(rand_pwd);
     85-    std::string cookie = COOKIEAUTH_USER + ":" + HexStr(rand_pwd);
     86+    const std::string rand_pwd_hex{HexStr(rand_pwd)};
     87+    std::string cookie = COOKIEAUTH_USER + ":" + rand_pwd_hex;
     88 
     89     /** the umask determines what permissions are used to create this file -
     90      * these are set to 0077 in common/system.cpp.
     91      */
     92     std::ofstream file;
     93     fs::path filepath_tmp = GetAuthCookieFile(true);
     94     if (filepath_tmp.empty()) {
     95-        return true; // -norpccookiefile
     96+        return GenerateAuthCookieResult::DISABLED; // -norpccookiefile
     97     }
     98     file.open(filepath_tmp);
     99     if (!file.is_open()) {
    100         LogWarning("Unable to open cookie authentication file %s for writing", fs::PathToString(filepath_tmp));
    101-        return false;
    102+        return GenerateAuthCookieResult::ERROR;
    103     }
    104     file << cookie;
    105     file.close();
    106 
    107     fs::path filepath = GetAuthCookieFile(false);
    108     if (!RenameOver(filepath_tmp, filepath)) {
    109         LogWarning("Unable to rename cookie authentication file %s to %s", fs::PathToString(filepath_tmp), fs::PathToString(filepath));
    110-        return false;
    111+        return GenerateAuthCookieResult::ERROR;
    112     }
    113     if (cookie_perms) {
    114         std::error_code code;
    115         fs::permissions(filepath, cookie_perms.value(), fs::perm_options::replace, code);
    116         if (code) {
    117             LogWarning("Unable to set permissions on cookie authentication file %s", fs::PathToString(filepath));
    118-            return false;
    119+            return GenerateAuthCookieResult::ERROR;
    120         }
    121     }
    122 
    123     g_generated_cookie = true;
    124     LogInfo("Generated RPC authentication cookie %s\n", fs::PathToString(filepath));
    125     LogInfo("Permissions used for cookie: %s\n", PermsToSymbolicString(fs::status(filepath).permissions()));
    126 
    127-    if (cookie_out)
    128-        *cookie_out = cookie;
    129-    return true;
    130+    user = COOKIEAUTH_USER;
    131+    pass = rand_pwd_hex;
    132+    return GenerateAuthCookieResult::OK;
    133 }
    134 
    135 bool GetAuthCookie(std::string *cookie_out)
    136 {
    137     std::ifstream file;
    138     std::string cookie;
    139diff --git i/src/rpc/request.h w/src/rpc/request.h
    140index 24887e8691..e93df02ba6 100644
    141--- i/src/rpc/request.h
    142+++ w/src/rpc/request.h
    143@@ -20,14 +20,31 @@ enum class JSONRPCVersion {
    144 
    145 /** JSON-RPC 2.0 request, only used in bitcoin-cli **/
    146 UniValue JSONRPCRequestObj(const std::string& strMethod, const UniValue& params, const UniValue& id);
    147 UniValue JSONRPCReplyObj(UniValue result, UniValue error, std::optional<UniValue> id, JSONRPCVersion jsonrpc_version);
    148 UniValue JSONRPCError(int code, const std::string& message);
    149 
    150-/** Generate a new RPC authentication cookie and write it to disk */
    151-bool GenerateAuthCookie(std::string* cookie_out, std::optional<fs::perms> cookie_perms=std::nullopt);
    152+enum class GenerateAuthCookieResult : uint8_t {
    153+    DISABLED, // -norpccookiefile
    154+    ERROR,
    155+    OK,
    156+};
    157+
    158+/**
    159+ * Generate a new RPC authentication cookie and write it to disk
    160+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] cookie_perms Filesystem permissions to use for the cookie file.
    161+ * [@param](/bitcoin-bitcoin/contributor/param/)[out] user Generated username, only set if `OK` is returned.
    162+ * [@param](/bitcoin-bitcoin/contributor/param/)[out] pass Generated password, only set if `OK` is returned.
    163+ * [@retval](/bitcoin-bitcoin/contributor/retval/) GenerateAuthCookieResult::DISABLED Authentication via cookie is disabled.
    164+ * [@retval](/bitcoin-bitcoin/contributor/retval/) GenerateAuthCookieResult::ERROR Error occurred, auth data could not be saved to disk.
    165+ * [@retval](/bitcoin-bitcoin/contributor/retval/) GenerateAuthCookieResult::OK Auth data was generated, saved to disk and in `user` and `pass`.
    166+ */
    167+GenerateAuthCookieResult GenerateAuthCookie(const std::optional<fs::perms>& cookie_perms,
    168+                                            std::string& user,
    169+                                            std::string& pass);
    170+
    171 /** Read the RPC authentication cookie from disk */
    172 bool GetAuthCookie(std::string *cookie_out);
    173 /** Delete RPC authentication cookie from disk */
    174 void DeleteAuthCookie();
    175 /** Parse JSON-RPC batch reply into a vector */
    176 std::vector<UniValue> JSONRPCProcessBatchReply(const UniValue& in);
    

    laanwj commented at 2:27 pm on May 9, 2025:
    i did that initially but it was a much larger change than i wanted. The current PR’s behavior is easy to review. It seems this works fine, and keeps everything the same with the only exception the : in rpcpassword but that gives a clear error at least.

    laanwj commented at 2:40 pm on May 9, 2025:

    Anyhow, i’ll cherry-pick this, thanks, hope i won’t get the opposite comment now that this is changing too much 😓

    Edit: done So after this, even -rpcuser can contain : again, although that makes it impossible to authenticate. This is disallowed by the spec so not an important edge case.

  23. vasild commented at 1:27 pm on May 9, 2025: contributor

    Almost ACK 3acfc071c3445e943069b2778bbc5c74f702ef36

    Only unsure about The configured rpcuser or rpcpassword cannot contain a ":", see the comment below.

  24. rpc: Undeprecate rpcuser/rpcpassword, change message to security warning
    Back in 2015, in #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.
    
    Closes #29240.
    4ab9bedee9
  25. rpc: 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.
    879a17bcb1
  26. rpc: Perform HTTP user:pass split once in `RPCAuthorized` 98ff38a6f1
  27. laanwj force-pushed on May 9, 2025
  28. rpc: Avoid join-split roundtrip for user:pass for auth credentials e49a7274a2
  29. laanwj force-pushed on May 9, 2025

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-05-11 18:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me