rpc: Implement random-cookie based authentication #6388

Merged
merged 2 commits into from Jul 14, 2015

Conversation

Projects
None yet
4 participants
@laanwj
Member

laanwj commented Jul 7, 2015

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.

@@ -287,3 +289,68 @@ UniValue JSONRPCError(int code, const string& message)
error.push_back(Pair("message", message));
return error;
}
+
+/** Username used when cookie authentication is in use (arbitrary, only for

This comment has been minimized.

@laanwj

laanwj Jul 7, 2015

Member

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.

@laanwj

laanwj Jul 7, 2015

Member

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.

This comment has been minimized.

@jonasschnelli

jonasschnelli Jul 7, 2015

Member

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

@jonasschnelli

jonasschnelli Jul 7, 2015

Member

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

This comment has been minimized.

@laanwj

laanwj Jul 8, 2015

Member

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 :)

@laanwj

laanwj Jul 8, 2015

Member

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 :)

@laanwj

View changes

src/bitcoin-cli.cpp
+ if (mapArgs["-rpcpassword"] == "") {
+ /* Try fall back to cookie-based authentication if no password is provided */
+ if (!GetAuthCookie(&strRPCUserColonPass)) {
+ throw runtime_error(strprintf(

This comment has been minimized.

@laanwj

laanwj Jul 7, 2015

Member

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 :-).

@laanwj

laanwj Jul 7, 2015

Member

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 :-).

This comment has been minimized.

@jonasschnelli

jonasschnelli Jul 7, 2015

Member

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.

@jonasschnelli

jonasschnelli Jul 7, 2015

Member

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.

This comment has been minimized.

@laanwj

laanwj Jul 8, 2015

Member

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.

@laanwj

laanwj Jul 8, 2015

Member

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.

@ajweiss

This comment has been minimized.

Show comment
Hide comment
@ajweiss

ajweiss Jul 7, 2015

Contributor

Nice! Reviewing this...

Contributor

ajweiss commented Jul 7, 2015

Nice! Reviewing this...

@ajweiss

This comment has been minimized.

Show comment
Hide comment
@ajweiss

ajweiss Jul 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.

Contributor

ajweiss commented Jul 7, 2015

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.

@jonasschnelli

View changes

src/rpcserver.cpp
@@ -597,28 +597,16 @@ void StartRPCThreads()
strAllowed += subnet.ToString() + " ";
LogPrint("rpc", "Allowing RPC connections from: %s\n", strAllowed);
- strRPCUserColonPass = mapArgs["-rpcuser"] + ":" + mapArgs["-rpcpassword"];
if (((mapArgs["-rpcpassword"] == "") ||
(mapArgs["-rpcuser"] == mapArgs["-rpcpassword"])) && Params().RequireRPCPassword())

This comment has been minimized.

@jonasschnelli

jonasschnelli Jul 7, 2015

Member

I think this if needs to be rewritten because at the moment the cookie system does not work in regtest.

@jonasschnelli

jonasschnelli Jul 7, 2015

Member

I think this if needs to be rewritten because at the moment the cookie system does not work in regtest.

This comment has been minimized.

@laanwj

laanwj Jul 8, 2015

Member

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.

@laanwj

laanwj Jul 8, 2015

Member

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.

This comment has been minimized.

@jonasschnelli

jonasschnelli Jul 8, 2015

Member

Hmm... but the following way don't work.

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

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

jonasschnelli Jul 8, 2015

Member

Hmm... but the following way don't work.

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

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

This comment has been minimized.

@laanwj

laanwj Jul 8, 2015

Member

Right, so bitcoin-cli should mind that flag as well (for now). Thanks.

@laanwj

laanwj Jul 8, 2015

Member

Right, so bitcoin-cli should mind that flag as well (for now). Thanks.

This comment has been minimized.

@laanwj

laanwj Jul 8, 2015

Member

Bleh, that'd mean moving the flag to BaseParams...

@laanwj

laanwj Jul 8, 2015

Member

Bleh, that'd mean moving the flag to BaseParams...

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jul 7, 2015

Member

Nice work!
Tested ACK (nit: doesn't work in regtest atm).

Member

jonasschnelli commented Jul 7, 2015

Nice work!
Tested ACK (nit: doesn't work in regtest atm).

+ // Try fall back to cookie-based authentication if no password is provided
+ if (!GetAuthCookie(&strRPCUserColonPass)) {
+ throw runtime_error(strprintf(
+ _("You must set rpcpassword=<password> in the configuration file:\n%s\n"

This comment has been minimized.

@sipa

sipa Jul 9, 2015

Member

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.

@sipa

sipa Jul 9, 2015

Member

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.

This comment has been minimized.

@laanwj

laanwj Jul 10, 2015

Member

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)

@laanwj

laanwj Jul 10, 2015

Member

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)

+ */
+ std::ofstream file;
+ boost::filesystem::path filepath = GetAuthCookieFile();
+ file.open(filepath.string().c_str());

This comment has been minimized.

@sipa

sipa Jul 9, 2015

Member

No logic to prevent too wide permissions on that file?

EDIT: nevermind, umask

@sipa

sipa Jul 9, 2015

Member

No logic to prevent too wide permissions on that file?

EDIT: nevermind, umask

This comment has been minimized.

@laanwj

laanwj Jul 10, 2015

Member

Right: even for the wallet permissions we rely on the umask.

@laanwj

laanwj Jul 10, 2015

Member

Right: even for the wallet permissions we rely on the umask.

laanwj added a commit to laanwj/bitcoin that referenced this pull request Jul 10, 2015

rpc: Remove chain-specific RequireRPCPassword
I've never liked the chain-specific exception to having to set a
password. It gives issues with #6388 which makes it valid to
set no password in every case (as it enables random cookie authentication).

This pull removes the flag, so that all chains are regarded the same.

It also removes the username==password test, which doesn't provide any
substantial extra security.
@ghost

View changes

src/rpcserver.cpp
- return;
+ LogPrintf("No rpcpassword set - using random cookie authentication\n");
+ if (!GenerateAuthCookie(&strRPCUserColonPass)) {
+ StartShutdown();

This comment has been minimized.

@ghost

ghost Jul 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.

@ghost

ghost Jul 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.

This comment has been minimized.

@laanwj

laanwj Jul 13, 2015

Member

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

laanwj Jul 13, 2015

Member

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.

This comment has been minimized.

@laanwj

laanwj Jul 13, 2015

Member

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".

@laanwj

laanwj Jul 13, 2015

Member

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".

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.
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 13, 2015

Member

Rebased against #6398, added generic UI error message before shutting down when writing cookie fails.

Member

laanwj commented Jul 13, 2015

Rebased against #6398, added generic UI error message before shutting down when writing cookie fails.

@laanwj laanwj merged commit 0937290 into bitcoin:master Jul 14, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jul 14, 2015

Merge pull request #6388
0937290 doc: mention RPC random cookie authentication in release notes (Wladimir J. van der Laan)
71cbeaa rpc: Implement random-cookie based authentication (Wladimir J. van der Laan)

@VoidingWarranties VoidingWarranties referenced this pull request in NebulousLabs/Sia Apr 14, 2016

Closed

API authentication #1065

zkbot added a commit to zcash/zcash that referenced this pull request Jan 18, 2017

Auto merge of #1999 - str4d:1950-random-cookie-rpc-auth, r=ebfull
rpc: Implement random-cookie based authentication

Cherry-picked from bitcoin/bitcoin#6388.

Closes #1950.

AllanDoensen referenced this pull request in AllanDoensen/BitcoinUnlimited Apr 23, 2017

rpc: Remove chain-specific RequireRPCPassword
I've never liked the chain-specific exception to having to set a
password. It gives issues with #6388 which makes it valid to
set no password in every case (as it enables random cookie authentication).

This pull removes the flag, so that all chains are regarded the same.

It also removes the username==password test, which doesn't provide any
substantial extra security.

@dgenr8 dgenr8 referenced this pull request in bitcoinxt/bitcoinxt Jun 5, 2018

Merged

Cherry-pick updates to contrib/linearize #441

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment