rpc: Implement random-cookie based authentication #6388

pull laanwj wants to merge 2 commits into bitcoin:master from laanwj:2015_07_cookie_monsters changing 5 files +120 −27
  1. laanwj commented at 1:07 pm on July 7, 2015: member

    When no -rpcpassword is specified, use a special ‘cookie’ file for authentication. This file is generated with random content when the daemon starts, and deleted when it exits. Read access to this file controls who can access through RPC. By default this file is stored in the data directory but it be overridden with -rpccookiefile.

    This is similar to Tor CookieAuthentication: see https://www.torproject.org/docs/tor-manual.html.en

    Alternative to #6258. Like that pull, this allows running bitcoind without any manual configuration. However, daemons should ideally never write to their configuration files, so I prefer this solution.

  2. laanwj added the label RPC on Jul 7, 2015
  3. in src/rpcprotocol.cpp: in 492ee66d29 outdated
    288@@ -287,3 +289,68 @@ UniValue JSONRPCError(int code, const string& message)
    289     error.push_back(Pair("message", message));
    290     return error;
    291 }
    292+
    293+/** Username used when cookie authentication is in use (arbitrary, only for
    


    laanwj commented at 1:29 pm on July 7, 2015:
    Not convinced that this code actually belongs in rpcprotocol, it is here because that file is shared between bitcoin-cli and bitcoind server, but conceptually it exists on a higher level than the protocol. Feel free to suggest a better place.

    jonasschnelli commented at 6:42 pm on July 7, 2015:
    I think rpcprotocol.h/.cpp is a good place for this extension. A new cookieauth.h/cpp file would be possible but i think is more compact and neatly arranged if it stays in rpcprotocol.h/cpp

    laanwj commented at 11:58 am on July 8, 2015:
    Making a new file pair e.g. rpcauthcookie sounds good to me too. But I don’t feel strongly about it either. The low-level code from rpcprotocol will be unnecessary with the libevent-based HTTP server (#5677), so this will at least leave something there :)
  4. in src/bitcoin-cli.cpp: in 492ee66d29 outdated
    104-                GetConfigFile().string().c_str()));
    105+    std::string strRPCUserColonPass;
    106+    if (mapArgs["-rpcpassword"] == "") {
    107+        /* Try fall back to cookie-based authentication if no password is provided */
    108+        if (!GetAuthCookie(&strRPCUserColonPass)) {
    109+            throw runtime_error(strprintf(
    


    laanwj commented at 3:49 pm on July 7, 2015:
    This error should be changed. It will be shown if bitcoin-cli is used while bitcoind is not running, so it’s confusing to tell the user to just set a password (which will indeed make this message go away, but just enough to get to connection refused :-).

    jonasschnelli commented at 6:46 pm on July 7, 2015:
    I think this if block from L100-L112 should go down to L126 (after the connection has been set up). It would make more sense to first check if we can connect to bitcoind, then check if we have some credentials available.

    laanwj commented at 11:51 am on July 8, 2015:
    Agreed. Thought this through a bit and that’d be better. if it can connect, and there is no cookie file, this message still makes sense.
  5. ajweiss commented at 5:22 pm on July 7, 2015: contributor
    Nice! Reviewing this…
  6. ajweiss commented at 6:01 pm on July 7, 2015: contributor
    I spent some time trying to break this, and was unable to do so. This grinds off one of bitcoind’s most annoying burrs, very nice! One thing to note is that some of the init scripts in the contrib directory have checks for a set rpcpassword, so when this goes in those should be removed.
  7. in src/rpcserver.cpp: in 492ee66d29 outdated
    596@@ -597,28 +597,16 @@ void StartRPCThreads()
    597         strAllowed += subnet.ToString() + " ";
    598     LogPrint("rpc", "Allowing RPC connections from: %s\n", strAllowed);
    599 
    600-    strRPCUserColonPass = mapArgs["-rpcuser"] + ":" + mapArgs["-rpcpassword"];
    601     if (((mapArgs["-rpcpassword"] == "") ||
    602          (mapArgs["-rpcuser"] == mapArgs["-rpcpassword"])) && Params().RequireRPCPassword())
    


    jonasschnelli commented at 6:39 pm on July 7, 2015:
    I think this if needs to be rewritten because at the moment the cookie system does not work in regtest.

    laanwj commented at 12:03 pm on July 8, 2015:
    I’d personally love to get rid of Params().RequireRPCPassword(), also for consistency reasons. But I think it’s unrelated to this pull - on regtest, setting an empty password is currently just allowed, so takes precedence.

    jonasschnelli commented at 12:14 pm on July 8, 2015:

    Hmm… but the following way don’t work.

    jonasschnelli$ ./src/bitcoind --regtest --datadir=/tmp/dummy2 --printtoconsole (fresh datadir)

    0jonasschnelli$ ./src/bitcoin-cli --regtest --datadir=/tmp/dummy2/ help
    1error: You must set rpcpassword=<password> in the configuration file:
    2/tmp/dummy2/bitcoin.conf
    3If the file does not exist, create it with owner-readable-only file permissions.
    

    laanwj commented at 12:24 pm on July 8, 2015:
    Right, so bitcoin-cli should mind that flag as well (for now). Thanks.

    laanwj commented at 12:28 pm on July 8, 2015:
    Bleh, that’d mean moving the flag to BaseParams…
  8. jonasschnelli commented at 6:46 pm on July 7, 2015: contributor
    Nice work! Tested ACK (nit: doesn’t work in regtest atm).
  9. laanwj referenced this in commit 8a1ce57ffe on Jul 8, 2015
  10. laanwj referenced this in commit fb569395b8 on Jul 8, 2015
  11. in src/bitcoin-cli.cpp: in 3f84f46a90 outdated
    114+    std::string strRPCUserColonPass;
    115+    if (mapArgs["-rpcpassword"] == "") {
    116+        // Try fall back to cookie-based authentication if no password is provided
    117+        if (!GetAuthCookie(&strRPCUserColonPass)) {
    118+            throw runtime_error(strprintf(
    119+                _("You must set rpcpassword=<password> in the configuration file:\n%s\n"
    


    sipa commented at 4:32 pm on July 9, 2015:
    Mention in this help text that the ability to write to [authfilepath] would also solve the problem? Now it may be confusing as being unable to write a file results in a help text mentioning some unrelated config option.

    laanwj commented at 12:25 pm on July 10, 2015:

    This message triggers if it is unable to read from the file, e.g. if it doesn’t exist, but is somehow able to connect. This is a very strange situation, but it won’t be solved by ability to write anywhere.

    (bitcoind won’t even start if no rpcpassword is set but it is unable to write the cookie file)

  12. in src/rpcprotocol.cpp: in 3f84f46a90 outdated
    313+    /** the umask determines what permissions are used to create this file -
    314+     * these are set to 077 in init.cpp unless overridden with -sysperms.
    315+     */
    316+    std::ofstream file;
    317+    boost::filesystem::path filepath = GetAuthCookieFile();
    318+    file.open(filepath.string().c_str());
    


    sipa commented at 4:33 pm on July 9, 2015:

    No logic to prevent too wide permissions on that file?

    EDIT: nevermind, umask


    laanwj commented at 12:28 pm on July 10, 2015:
    Right: even for the wallet permissions we rely on the umask.
  13. laanwj referenced this in commit 85ee55b5c3 on Jul 10, 2015
  14. rpc: Implement random-cookie based authentication
    When no `-rpcpassword` is specified, use a special 'cookie' file for
    authentication. This file is generated with random content when the
    daemon starts, and deleted when it exits. Read access to this file
    controls who can access through RPC. By default this file is stored in
    the data directory but it be overriden with `-rpccookiefile`.
    
    This is similar to Tor CookieAuthentication: see
    https://www.torproject.org/docs/tor-manual.html.en
    
    Alternative to #6258. Like that pull, this allows running bitcoind
    without any manual configuration. However, daemons should ideally never write to
    their configuration files, so I prefer this solution.
    71cbeaad9a
  15. in src/rpcserver.cpp: in 3f84f46a90 outdated
    619-                "", CClientUIInterface::MSG_ERROR | CClientUIInterface::SECURE);
    620-        StartShutdown();
    621-        return;
    622+        LogPrintf("No rpcpassword set - using random cookie authentication\n");
    623+        if (!GenerateAuthCookie(&strRPCUserColonPass)) {
    624+            StartShutdown();
    


    unknown commented at 4:35 am on July 13, 2015:
    Would it make sense to move the rpcpassword warning into here, instead of removing it entirely? This would give the user warning if GenerateAuthCookie() fails to write to disk for example.

    laanwj commented at 10:22 am on July 13, 2015:
    GenerateAuthCookie already logs an error when it is unable to create the file. The data directory must be writable (otherwise creating the lock file will fail, earlier), so it is an extremely rare error. IMO it doesn’t need an extensive translated message.

    laanwj commented at 10:37 am on July 13, 2015:
    I think it is reasonable to show an error here before shutting down, but can just as well be a standard “Initialization failed, check the error log for details”.
  16. laanwj force-pushed on Jul 13, 2015
  17. laanwj commented at 11:12 am on July 13, 2015: member
    Rebased against #6398, added generic UI error message before shutting down when writing cookie fails.
  18. doc: mention RPC random cookie authentication in release notes 0937290553
  19. laanwj force-pushed on Jul 13, 2015
  20. laanwj merged this on Jul 14, 2015
  21. laanwj closed this on Jul 14, 2015

  22. laanwj referenced this in commit fd5dfda939 on Jul 14, 2015
  23. zkbot referenced this in commit 94f427a211 on Jan 18, 2017
  24. AllanDoensen referenced this in commit 3735c9332c on Apr 23, 2017
  25. MarcoFalke locked this on Sep 8, 2021

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: 2024-11-21 18:12 UTC

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