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.

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

[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

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

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

👍

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.

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

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.

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

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.

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

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

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

Moved the python script to share directory.

Concept ACK

utACK

Concept ACK +1 , finally !!!!

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

squashed

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

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.

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

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.

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

@MarcoFalke done

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

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

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

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

@instagibbs warning LGTM 👍

re-ACK

Tested ACK

ACK