Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Nov 30, 2015

Conversation

@instagibbs
Copy link
Member

instagibbs commented Nov 17, 2015

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:

  1. USERNAME is the desired username. Name does not have to be unique.
  2. SALT is the salt for the HMAC_SHA256 function
  3. 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
Copy link
Member

luke-jr commented Nov 17, 2015

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
Copy link
Contributor

gmaxwell commented Nov 17, 2015

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

@sipa
sipa reviewed Nov 17, 2015
View changes
src/httprpc.cpp Outdated
unsigned char *out = new unsigned char[KEY_SIZE];
char outhex[KEY_HEX_SIZE];

if (PKCS5_PBKDF2_HMAC(strPass.c_str(), strPass.size(),

This comment has been minimized.

Copy link
@sipa

sipa Nov 17, 2015

Member

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

This comment has been minimized.

Copy link
@gmaxwell

gmaxwell Nov 17, 2015

Contributor

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

This comment has been minimized.

Copy link
@sipa

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Nov 18, 2015

Member

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

@sipa
sipa reviewed Nov 17, 2015
View changes
src/httprpc.cpp Outdated
std::string strHashFromPass = std::string(outhex, KEY_HEX_SIZE);

if (TimingResistantEqual(strHashFromPass, strHash)) {
return true;

This comment has been minimized.

Copy link
@sipa

sipa Nov 17, 2015

Member

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.

This comment has been minimized.

Copy link
@instagibbs

instagibbs Nov 18, 2015

Author Member

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)

This comment has been minimized.

Copy link
@sipa

sipa Nov 18, 2015

Member

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

I withdraw this comment :)

@dcousens
Copy link
Contributor

dcousens commented Nov 18, 2015

[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
Copy link
Contributor

gmaxwell commented Nov 18, 2015

@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
Copy link
Contributor

dcousens commented Nov 18, 2015

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

👍

@jonasschnelli
Copy link
Member

jonasschnelli commented Nov 18, 2015

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.

@instagibbs
Copy link
Member Author

instagibbs commented Nov 18, 2015

@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
Copy link
Contributor

dcousens commented Nov 18, 2015

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
Copy link
Member Author

instagibbs commented Nov 18, 2015

@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
Copy link
Contributor

dcousens commented Nov 18, 2015

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
Copy link
Contributor

gmaxwell commented Nov 18, 2015

@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
Copy link
Member

laanwj commented Nov 18, 2015

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

@laanwj
laanwj reviewed Nov 18, 2015
View changes
contrib/rpcuser/rpcuser.py Outdated
@@ -0,0 +1,41 @@
#!/usr/bin/env python3

This comment has been minimized.

Copy link
@laanwj

laanwj Nov 18, 2015

Member

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.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Nov 18, 2015

Member

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

This comment has been minimized.

Copy link
@instagibbs

instagibbs Nov 18, 2015

Author Member

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

@instagibbs
Copy link
Member Author

instagibbs commented Nov 18, 2015

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

@instagibbs
Copy link
Member Author

instagibbs commented Nov 19, 2015

Moved the python script to share directory.

@jtimon
Copy link
Member

jtimon commented Nov 20, 2015

Concept ACK

@dcousens
Copy link
Contributor

dcousens commented Nov 20, 2015

utACK

@6coind
Copy link

6coind commented Nov 20, 2015

Concept ACK +1 , finally !!!!

@gmaxwell
Copy link
Contributor

gmaxwell commented Nov 23, 2015

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

@instagibbs instagibbs force-pushed the instagibbs:multrpc branch Nov 23, 2015
@instagibbs
Copy link
Member Author

instagibbs commented Nov 23, 2015

squashed

@gmaxwell
Copy link
Contributor

gmaxwell commented Nov 23, 2015

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

@gavinandresen
Copy link
Contributor

gavinandresen commented Nov 23, 2015

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
Copy link
Member Author

instagibbs commented Nov 23, 2015

@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
Copy link
Contributor

gmaxwell commented Nov 23, 2015

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
Copy link
Member

MarcoFalke commented Nov 23, 2015

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

@instagibbs
Copy link
Member Author

instagibbs commented Nov 23, 2015

@gmaxwell
Copy link
Contributor

gmaxwell commented Nov 24, 2015

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

@instagibbs
Copy link
Member Author

instagibbs commented Nov 24, 2015

@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 instagibbs force-pushed the instagibbs:multrpc branch Nov 27, 2015
@instagibbs
Copy link
Member Author

instagibbs commented Nov 27, 2015

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

@gwillen
gwillen reviewed Nov 28, 2015
View changes
src/init.cpp Outdated
@@ -482,6 +482,7 @@ std::string HelpMessage(HelpMessageMode mode)
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)"));
strUsage += HelpMessageOpt("-rpcuser=<user>", _("Username for JSON-RPC connections"));
strUsage += HelpMessageOpt("-rpcpassword=<pw>", _("Password for JSON-RPC connections"));
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"));

This comment has been minimized.

Copy link
@gwillen

gwillen Nov 28, 2015

Contributor

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

@gwillen
Copy link
Contributor

gwillen commented Nov 28, 2015

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

@instagibbs instagibbs force-pushed the instagibbs:multrpc branch Nov 28, 2015
@sipa
sipa reviewed Nov 28, 2015
View changes
src/httprpc.cpp Outdated
static bool multiUserAuthorized(std::string strUserPass)
{
std::string strUser = strUserPass.substr(0, strUserPass.find(":"));
std::string strPass = strUserPass.substr(strUserPass.find(":") + 1);

This comment has been minimized.

Copy link
@sipa

sipa Nov 28, 2015

Member

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?

@sipa
sipa reviewed Nov 28, 2015
View changes
src/httprpc.cpp Outdated

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);
for (int i=0;i<(int)KEY_SIZE;i++) {
sprintf(outhex+(i*2), "%02x", out[i]);

This comment has been minimized.

Copy link
@sipa

sipa Nov 28, 2015

Member

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

This comment has been minimized.

Copy link
@instagibbs

instagibbs Nov 28, 2015

Author Member

Thanks for the tip.

@instagibbs instagibbs force-pushed the instagibbs:multrpc branch Nov 28, 2015
@gwillen
Copy link
Contributor

gwillen commented Nov 29, 2015

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

@instagibbs instagibbs force-pushed the instagibbs:multrpc branch to d52fbf0 Nov 29, 2015
@dcousens
Copy link
Contributor

dcousens commented Nov 30, 2015

@instagibbs warning LGTM 👍

@dcousens
Copy link
Contributor

dcousens commented Nov 30, 2015

re-ACK

@btcdrak
Copy link
Contributor

btcdrak commented Nov 30, 2015

Tested ACK

@gmaxwell
Copy link
Contributor

gmaxwell commented Nov 30, 2015

ACK

@gmaxwell gmaxwell merged commit d52fbf0 into bitcoin:master Nov 30, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gmaxwell added a commit that referenced this pull request Nov 30, 2015
d52fbf0 Added additional config option for multiple RPC users. (Gregory Sanders)
zkbot added a commit to zcash/zcash that referenced this pull request Mar 21, 2018
Misc upstream PRs

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6077
  - Second commit only (first was already applied to 0.11.X and then reverted)
- bitcoin/bitcoin#6284
- bitcoin/bitcoin#6489
- bitcoin/bitcoin#6462
- bitcoin/bitcoin#6647
- bitcoin/bitcoin#6235
- bitcoin/bitcoin#6905
- bitcoin/bitcoin#6780
  - Excluding second commit (QT) and third commit (requires bitcoin/bitcoin#6993)
- bitcoin/bitcoin#6961
  - Excluding QT parts, and a small `src/policy/policy.cpp` change which depends on a bunch of other PRs, which we'll have to remember to come back to.
- bitcoin/bitcoin#7044
- bitcoin/bitcoin#8856
- bitcoin/bitcoin#9002

Part of #2074 and #2132.
zkbot added a commit to zcash/zcash that referenced this pull request Dec 4, 2019
Misc upstream PRs

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6077
  - Second commit only (first was already applied to 0.11.X and then reverted)
- bitcoin/bitcoin#6284
- bitcoin/bitcoin#6489
- bitcoin/bitcoin#6235
- bitcoin/bitcoin#6905
- bitcoin/bitcoin#6780
  - Excluding second commit (QT) and third commit (requires bitcoin/bitcoin#6993)
- bitcoin/bitcoin#6961
  - Excluding QT parts, and a small `src/policy/policy.cpp` change which depends on a bunch of other PRs, which we'll have to remember to come back to.
- bitcoin/bitcoin#7044
- bitcoin/bitcoin#8856
- bitcoin/bitcoin#9002

Part of #2074 and #2132.
zkbot added a commit to zcash/zcash that referenced this pull request Dec 4, 2019
Misc upstream PRs

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6077
  - Second commit only (first was already applied to 0.11.X and then reverted)
- bitcoin/bitcoin#6284
- bitcoin/bitcoin#6489
- bitcoin/bitcoin#6235
- bitcoin/bitcoin#6905
- bitcoin/bitcoin#6780
  - Excluding second commit (QT) and third commit (requires bitcoin/bitcoin#6993)
- bitcoin/bitcoin#6961
  - Excluding QT parts, and a small `src/policy/policy.cpp` change which depends on a bunch of other PRs, which we'll have to remember to come back to.
- bitcoin/bitcoin#7044
- bitcoin/bitcoin#8856
- bitcoin/bitcoin#9002

Part of #2074 and #2132.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.