RPC: Added additional config option for multiple RPC users.

instagibbs commented at 7:51 pm on November 17, 2015: member

This pull adds an additional config option, “rpcauth” to allow multiple different users to use different credentials for login.

Motivation: In business settings there are often multiple users accessing a particular core instance, using wallet functionality. Instead of all users sharing the same login name and password, it is desired to have each user generate their own secret password, and have a hashed and salted version added to bitcoin.conf by the admin. Currently there is only one name and password, and it is stored in plaintext. This pull attempts to do just this and will be followed by an additional audit logging pull to enable admins to assign blame to spends and other irreversible actions.

The config option comes in the format:

rpcauth=USERNAME:SALT$HASH

Where:

USERNAME is the desired username. Name does not have to be unique.
SALT is the salt for the HMAC_SHA256 function
HASH is a hex string that is the result of the HMAC_SHA256 function on the user’s secret password plus the SALT as the key.

A “canonical” password generating python script has been supplied in share/rpcuser. From the client-side, one connects using the standard -rpcuser/-rpcpassword options.

luke-jr commented at 7:56 pm on November 17, 2015: member

Please explain how your vision accounts for multi-wallet use. It seems desirable to have different users access different wallets (or perhaps even limited to specific accounts within a wallet), but your plan above does not seem to account for that possibility.

gmaxwell commented at 8:13 pm on November 17, 2015: contributor

@luke-jr That seems orthogonal to me. Presumably if we eventually wanted to layer access controls on top of this in the future (though we’ve shed away from that in the past), this appears completely compatible with doing so… but this is useful without that, just from an credential management perspective.

in src/httprpc.cpp: in 24edc3be74 outdated

106+            unsigned int KEY_SIZE = 32;
107+            unsigned int KEY_HEX_SIZE = 64;
108+            unsigned char *out = new unsigned char[KEY_SIZE]; 
109+            char outhex[KEY_HEX_SIZE];
110+
111+            if (PKCS5_PBKDF2_HMAC(strPass.c_str(), strPass.size(),

sipa commented at 8:25 pm on November 17, 2015:

I think we really want to avoid adding OpenSSL dependencies. But it shouldn’t be hard to add PBKDF2 (we already have a builtin HMAC-SHA256 implementation).

gmaxwell commented at 11:44 pm on November 17, 2015:

We’re in that boat for the wallet encryption already.

sipa commented at 11:45 pm on November 17, 2015:

See #5949.

jonasschnelli commented at 7:50 am on November 18, 2015:

This affects also --disable-wallet compilation (httprpc.cpp). Agree with @sipa. There is no reason to use openssl lump for that.

in src/httprpc.cpp: in 24edc3be74 outdated

115+                    sprintf(outhex+(i*2), "%02x", out[i]);
116+                }
117+                std::string strHashFromPass = std::string(outhex, KEY_HEX_SIZE);
118+
119+                if (TimingResistantEqual(strHashFromPass, strHash)) {
120+                    return true;

sipa commented at 8:31 pm on November 17, 2015:

Early return will break the timing resistance property. It’s probably better to maintain a boleean return variable, and use |= to mix in the result.

instagibbs commented at 0:05 am on November 18, 2015:

Could you elaborate on what sort of timing info will leak? (unless you mean the function should try every single name/pass combination, and not call the continue on line 98 as well)

sipa commented at 0:35 am on November 18, 2015:

Eh, it could leak which entry was matched… but I guess that’s not considered very sensitive information.

I withdraw this comment :)

dcousens commented at 0:30 am on November 18, 2015: contributor

~~[weak] concept NACK, IMHO should be removing RPC complexity, not adding it.~~

edit: This could trivially be added externally, and would be less than 100-200 lines of code for a simply forwarding/authentication/logging HTTP server.

edit2: See after @gmaxwell’s answer

gmaxwell commented at 1:24 am on November 18, 2015: contributor

@dcousens: that could be said of any functionality; thats really no excuse for having an auth setup which will be an immediate failure in any security audit. (And for context, I asked instagibbs to work on something like this after hearing from multiple parties that they switched from bitcoin core to an API provider for a list of reasons that included this; I’d like to get all of these reasons fixed.)

I don’t see why you see this as increasing the complexity of RPC: it changes credential storage, not the RPC– other than timing it’s presence is externally indistinguishable.

I think we should phase out the old method of configuration and support only this and cookie auth; but thats incompatible so it should probably be phased in across multiple versions.

dcousens commented at 2:11 am on November 18, 2015: contributor

To clarify, the above was a weak NACK; and I may have jumped the gun on complexity, as after a code review, only ~20-30 lines was added code, the rest is tests.

If the alternative configuration method is [eventually] removed, then I see no issue with this and its an easy feature win for those who need it. Concept ACK, utACK

:+1:

jonasschnelli commented at 7:55 am on November 18, 2015: contributor

Concept ACK. I think keeping the credentials in bitcoin.conf (instead of a .passwd like file) is fine for a first step.

Nice work! Thanks for directly include a RPC test.

jonasschnelli added the label RPC on Nov 18, 2015

instagibbs commented at 12:22 pm on November 18, 2015: member

@paveljanik @jonasschnelli If we ever get around to deprecating the original rpc credential method I think that’d be a good time to move it.

dcousens commented at 1:57 pm on November 18, 2015: contributor

Is the salting and hashing really worth it? An admin could easily listen on the wire for the pre-stretched password. I feel like this is a false sense of security for what is meant to be a local (127.0.0.1) only interface? While we’re at it, why not just abandon the password?

The authentication itself is just basic HTTP authentication AFAIK. This is no better than just a single token. Clearer intent and security implications IMHO.

instagibbs commented at 2:29 pm on November 18, 2015: member

@dcousens salting and hashing protects the passwords in the case of an adversary getting a hold of the password file and forging irreversible requests immediately, but not an catching it across the wire, no. I still see value in that, especially for basic audit purposes.

dcousens commented at 2:41 pm on November 18, 2015: contributor

getting hold of the password file and forging irreversible requests immediately

I’d be willing to bet that the attacker will likely already be able to do this, be it through watching the wire or simply adding the auth settings to the bitcoin.conf. It could be RO, but, in that case, why wouldn’t it just be inaccessible.

If that sole property is desirable, then sure, add it. But it isn’t reflective of the existing security model.

especially for basic audit purposes.

~~I’m not sure how user:password vs token relates to auditing?~~ It relates to printing user:action in the logs, whereas token would expose the secret too. In this event, you could just use hash(token), but, I see the usability aspect of this.

gmaxwell commented at 2:45 pm on November 18, 2015: contributor

@dcousens: consider, a couple weeks ago I learned sipa’s rpcpassword while he was showing me something in his bitcoin.conf on his screen.

Hashed passwords were the norm for system authentication decades before encrypted transports. :)

laanwj commented at 3:24 pm on November 18, 2015: member

Concept ACK Hashed passwords in the configuration file is definitely an improvement.

in contrib/rpcuser/rpcuser.py: in 24edc3be74 outdated

0@@ -0,0 +1,41 @@
1+#!/usr/bin/env python3

laanwj commented at 3:28 pm on November 18, 2015:

nit: If this script is supposed to be used by end users, it should be in share/ (I think) and be installed with make install. Contrib is usually for obscure things only useful for developers.

MarcoFalke commented at 3:41 pm on November 18, 2015:

And mention in the readme how this should be called. E.g. ./rpcuser testUserName.

instagibbs commented at 3:51 pm on November 18, 2015:

I don’t see any related scripts in ‘share’ nor a readme in the folder.

instagibbs commented at 4:33 pm on November 18, 2015: member

Axed the key stretching done as per discussion in bitcoin-core-dev IRC, removed openssl dependency. Updated description.

instagibbs commented at 10:58 pm on November 19, 2015: member

Moved the python script to share directory.

jtimon commented at 2:46 pm on November 20, 2015: contributor

Concept ACK

dcousens commented at 4:43 pm on November 20, 2015: contributor

utACK

6coind commented at 6:49 pm on November 20, 2015: none

Concept ACK +1 , finally !!!!

gmaxwell commented at 0:16 am on November 23, 2015: contributor

@instagibbs mind squashing some of this to get the EVP stuff out of the history?

instagibbs force-pushed on Nov 23, 2015

instagibbs commented at 0:21 am on November 23, 2015: member

squashed

gmaxwell commented at 6:34 pm on November 23, 2015: contributor

Hm. Python3 dep on the gen util. How hard would that be to avoid?

gavinandresen commented at 7:01 pm on November 23, 2015: contributor

New command-line or config-file options should be documented in the –help output (see init.cpp).

How does this interact with the automatic random cookie authentication in InitRPCAuthentication? I’d expect if I use -rpcauth that would be the only authentication method available…

And are changes to bitcoin-cli needed to add this new functionality?

In general, it makes me nervous to have two very different ways of accomplishing the same thing (-rpcauth and -rcpuser/-rpcpassword). Deprecating -rpcuser/-rpcpassword and moving to -rpcauth would be better, but even if deprecation doesn’t happen in-memory conversion of any existing -rcpuser/-rpcpassword to the new scheme as one of the first things done at startup in any code that deals with those options should be less bug-prone.

instagibbs commented at 7:08 pm on November 23, 2015: member

@gmaxwell I’ve added a few more lines to make it work for either. @gavinandresen To the connecting bitcoin-cli, the interface is just the same. That was intentional. And I’m in agreement about (eventual?) deprecation. It’d be quite easy to do after this. I’ll add some documentation to init.cpp, sure.

gmaxwell commented at 7:08 pm on November 23, 2015: contributor

At least I was thinking that rpcuser/rpcpassword should be deprecated, but wasn’t expecting that to happen right this instant.

For cookie auth, it can be useful to have both– the reason is that cookie auth can be used seamlessly by local applications. But the correct way to achieve that is probably a soft opt disable for cookie auth triggered by having static authentication; I guess.

MarcoFalke commented at 7:19 pm on November 23, 2015: member

@instagibbs Mind to add the missing “-rpcauth” help message in init.cpp?

instagibbs commented at 7:46 pm on November 23, 2015: member

@MarcoFalke done

instagibbs force-pushed on Nov 23, 2015

dcousens commented at 1:29 am on November 24, 2015: contributor

@gmaxwell we can at least add a warning for -rpcuser straight away?

gmaxwell commented at 1:50 am on November 24, 2015: contributor

@dcousens seems reasonable to me, also a good way to get uses moving to the cookie approach as applicable.

instagibbs commented at 4:38 pm on November 24, 2015: member

@gavinandresen gave it some more thought, and I think it’s most straight forward to throw a warning for now, then completely replace as a next step. By the time I get some intermediate step to work, it’s just easier to replace everything in my opinion. Would that be acceptable?

instagibbs force-pushed on Nov 27, 2015

instagibbs commented at 4:53 pm on November 27, 2015: member

@dcousens: added a warning. Is this along the lines of what you were thinking?

in src/init.cpp: in 633110e3c0 outdated

481@@ -482,6 +482,7 @@ std::string HelpMessage(HelpMessageMode mode)
482     strUsage += HelpMessageOpt("-rpcbind=<addr>", _("Bind to given address to listen for JSON-RPC connections. Use [host]:port notation for IPv6. This option can be specified multiple times (default: bind to all interfaces)"));
483     strUsage += HelpMessageOpt("-rpcuser=<user>", _("Username for JSON-RPC connections"));
484     strUsage += HelpMessageOpt("-rpcpassword=<pw>", _("Password for JSON-RPC connections"));
485+    strUsage += HelpMessageOpt("-rpcauth=<userpw>", _("Username and hashed password for JSON-RPC connections. The field <userpw> comes in the format: <USERNAME>:<SALT>$<HASH>. A canonical python script is included in share/rpcuser"));

gwillen commented at 0:01 am on November 28, 2015:

Add ‘This option can be specified multiple times.’ for clarity?

gwillen commented at 0:02 am on November 28, 2015: contributor

utACK with one nit in the help text (see above.)

instagibbs force-pushed on Nov 28, 2015

in src/httprpc.cpp: in bbbd4e5cf5 outdated

74@@ -72,6 +75,51 @@ static void JSONErrorReply(HTTPRequest* req, const UniValue& objError, const Uni
75     req->WriteReply(nStatus, strReply);
76 }
77 
78+//This function checks username and password against -rpcauth
79+//entries from config file.
80+static bool multiUserAuthorized(std::string strUserPass)
81+{    
82+    std::string strUser = strUserPass.substr(0, strUserPass.find(":"));
83+    std::string strPass = strUserPass.substr(strUserPass.find(":") + 1);

sipa commented at 12:10 pm on November 28, 2015:

What happens when a client sends an authentication string that does not contain a ‘:’? My guess is that find returns npos == -1, which means that both strPass and strUser will be equal to strUserPass in that case. It looks harmless, but certainly unintuitive and perhaps not well-defined?

in src/httprpc.cpp: in bbbd4e5cf5 outdated

106+            unsigned char *out = new unsigned char[KEY_SIZE]; 
107+            char outhex[KEY_HEX_SIZE];
108+            
109+            CHMAC_SHA256(reinterpret_cast<const unsigned char*>(strSalt.c_str()), strSalt.size()).Write(reinterpret_cast<const unsigned char*>(strPass.c_str()), strPass.size()).Finalize(out);
110+            for (int i=0;i<(int)KEY_SIZE;i++) {
111+                sprintf(outhex+(i*2), "%02x", out[i]);

sipa commented at 12:11 pm on November 28, 2015:

You can use HexStr() from utilstrencodings.h here.

instagibbs commented at 11:13 pm on November 28, 2015:

Thanks for the tip.

instagibbs force-pushed on Nov 28, 2015

gwillen commented at 4:29 am on November 29, 2015: contributor

Seems like all mentioned issues are now fixed. I’d love to see this get merged before the 0.12 freeze.

Added additional config option for multiple RPC users. d52fbf00e3

instagibbs force-pushed on Nov 29, 2015

dcousens commented at 4:11 am on November 30, 2015: contributor

@instagibbs warning LGTM :+1:

dcousens commented at 4:11 am on November 30, 2015: contributor

re-ACK

btcdrak commented at 5:10 pm on November 30, 2015: contributor

Tested ACK

gmaxwell commented at 5:37 pm on November 30, 2015: contributor

ACK

gmaxwell merged this on Nov 30, 2015

gmaxwell closed this on Nov 30, 2015

gmaxwell referenced this in commit 438ee59839 on Nov 30, 2015

zkbot referenced this in commit f1aeaec471 on Mar 21, 2018

zkbot referenced this in commit 4fc490c430 on Dec 4, 2019

zkbot referenced this in commit 868c63f92d on Dec 4, 2019

MarcoFalke locked this on Sep 8, 2021

RPC: Added additional config option for multiple RPC users. #7044