rpc: Support -rpcauthfile argument #20407

promag commented at 12:01 AM on November 17, 2020: member

This argument is similar to -rpcauth but takes the value from the specified file content and can be specified from the command line.

Fixes #20387.

fanquake added the label RPC/REST/ZMQ on Nov 17, 2020

maflcko commented at 7:33 AM on November 17, 2020: member

How is this different from includeconf with the rpcauth in it as the only content?

promag commented at 8:48 AM on November 17, 2020: member

@MarcoFalke I thought about suggesting that but main issue is that -includeconf can't be specified in the command line. Also the file pointed by -includeconf can have more configurations.

Maybe @ruimarinho and @NicolasDorier can weight on the the approach.

rpc: Refactor to process -rpcauth once 6272dcd603

rpc: Support -rpcauthfile argument

This argument is similar to -rpcauth but takes the value from the
specified file content

10ef7321f0

rpcauth: Support storing credentials in a file

To be used with -rpcauthfile

ff5d7fa1e4

promag force-pushed on Nov 17, 2020

in src/httprpc.cpp:259 in ff5d7fa1e4

 255 |      {
 256 |          LogPrintf("Using rpcauth authentication.\n");
 257 | +        for (std::string rpcauth : gArgs.GetArgs("-rpcauth")) {
 258 | +            std::vector<std::string> fields;
 259 | +            boost::split(fields, rpcauth, boost::is_any_of(":$"));
 260 | +            if (fields.size() == 3) g_rpcauth.push_back(fields);

ryanofsky commented at 1:15 PM on November 17, 2020:

I guess it's a preexisting problem, but lack of any error feedback when the syntax is wrong seems like a usability nightmare

promag commented at 1:40 PM on November 17, 2020:

Yeah it is, can be improved.

promag commented at 2:25 PM on November 17, 2020:

Actually I think I'll do it in this branch, -rpcauth values are now processed once.

schildbach commented at 5:24 PM on November 17, 2020:

Such an -rpccookiegenerate=false option is what I had in mind in #20387. I would prefer that exactly for the simplicity of using an arbitrary string as cookie (which I can share via Docker's secret mechanism).

luke-jr commented at 11:28 PM on November 24, 2020:

Syntax isn't necessarily wrong. It could be a 4th field added (eg, Knots uses it to restrict access to the named wallet).

A debug log line can't hurt though.

ryanofsky commented at 1:37 PM on November 17, 2020: contributor

This seems like a reasonable option to add.

Allowing -includeconf to be specified on the command line would also be reasonable way to help address #20387.

However, both of these approaches seem more cumbersome to use and implement than the -norpccookiegenerate option suggested #20387 (comment).

It seems like that option could be implemented with simple logic like:

bool have_cookie = GetBoolArg("-rpccookiegenerate", true) ? GenerateAuthCookie(&cookie) : GetAuthCookie(&cookie);

and unlike the rpcauth value, the cookie value would be automatically picked up by bitcoin-cli client and potentially the electrs client. There wouldn't be a need to generate username, password, and hashed auth strings, pass the hashed auth string to the server, and pass the username and password strings to each client separately. The cookie value could be any random string with no need for hashing at all and placed in a common location where it is available to the server and every client that needs to read it.

promag commented at 1:43 PM on November 17, 2020: member

Your approach doesn't support multiple credentials. I also see the cookie as a local authentication mechanism.

laanwj commented at 10:29 AM on November 18, 2020: member

Kind of meh on adding yet another RPC authentication method that's only a slight variation from the other ones, but as long as it (and all combinatorial options it opens up) are tested sufficiently I'm not strongly against it either.

promag commented at 11:36 AM on November 18, 2020: member

@laanwj while @ryanofsky approach is indeed simple (just misses the detail of not erasing cookie file on shutdown) it doesn't support different users with different whitelists. Also note that @ryanofsky approach changes cookie semantic - the cookie is available while the daemon is running. I'd even limit cookie authentication to loopback interface.

the cookie value would be automatically picked up by bitcoin-cli client and potentially the electrs client. @ryanofsky executing bitcoin-cli on the bitcoind container will always works - cookie is there. And for electrs it might be easier to just use config `cookie=USER:PASSWORD".

There wouldn't be a need to generate username, password, and hashed auth strings, pass the hashed auth string to the server, and pass the username and password strings to each client separately. The cookie value could be any random string with no need for hashing at all and placed in a common location where it is available to the server and every client that needs to read it. @ryanofsky this is not really necessary. Looks like we should just add support for bearer authentication in addition to basic authentication. In this case we could support -rpcauth=user:token and -rpcauthfile=user:path and then clients could just send Authorization: Bearer <token> header (the server would be able to identify the user and check its whitelist).

It just sounds reasonable to me to support different credentials and whitelists and make this easy to setup with docker containers and secrets.

ryanofsky commented at 12:40 PM on November 18, 2020: contributor

@promag sure, and thanks for implementing this. I just suggested the -rpccookiegenerate boolean option because it seemed to directly address the reported issue, because it seemed simple to implement as a ~one-line change, and because it seemed like a simpler configuration to set up: not requiring any scripting or hashing, not requiring any client configuration if using bitcoin-cli or using the default cookie path with another client.

Also, it's been said a few times that -norpccookiegenerate wouldn't support multiple credentials, but this would be a easy restriction to lift now or in the future by replacing GetArg("-rpccookiefile") with GetArgs("-rpccookiefile") and std::string with std::set<std::string> a few places.

I do agree with you that it would probably make sense to stop allowing cookie authentication from non-local connections by default. I also think the -rpcauthfile option implemented here would be a useful thing to have (though allowing -includeconf on the command line could be more flexible). I can see how there might be security concerns and potential for insecure misconfiguration with cookie files since the secret isn't hashed. But I think all the solutions that have been discussed have been reasonable.

promag commented at 12:48 PM on November 18, 2020: member

I'd say the advantage of -rpcauthfile is to have a file with just the token and so more suitable as a shared secret.

promag commented at 12:49 PM on November 18, 2020: member

I do agree with you that it would probably make sense to stop allowing cookie authentication from non-local connections by default.

Ok, this can be submitted on a separate branch.

in src/httprpc.cpp:269 in 10ef7321f0 outdated

 265 | +            if (!file.is_open()) continue;
 266 | +            std::string rpcauth;
 267 | +            std::getline(file, rpcauth);
 268 | +            std::vector<std::string> fields;
 269 | +            boost::split(fields, rpcauth, boost::is_any_of(":$"));
 270 | +            if (fields.size() == 3) g_rpcauth.push_back(fields);

ryanofsky commented at 12:55 PM on November 18, 2020:

In commit "rpc: Support -rpcauthfile argument" (10ef7321f0dab8cb7dfe79f7c0f9d886d8fe0097)

I think this should refuse to load if the rpcauthfile isn't formatted correctly. Logging and error and returning false might be sufficient to do this.

I think an invalid -rpcauth option above should also trigger an error, or at least report an error, but it's pre-existing code so not directly related

promag commented at 1:18 PM on November 18, 2020:

I think this should refuse to load

You mean the process should exit?

ryanofsky commented at 1:26 PM on November 18, 2020:

re: #20407 (review)

I think this should refuse to load

You mean the process should exit?

I would exit, but don't think it would be crazy not to exit, do think it would be crazy to silently ignore a configuration error giving no feedback. I am presuming returning false exits, but didn't check.

luke-jr commented at 11:29 PM on November 24, 2020:

See above, it's not necessarily an error

promag commented at 11:38 PM on November 24, 2020:

@luke-jr please review #20461, this one should rebase with that.

in share/rpcauth/rpcauth.py:45 in ff5d7fa1e4

  43 | -    print('rpcauth={0}:{1}${2}'.format(args.username, salt, password_hmac))
  44 | -    print('Your password:\n{0}'.format(args.password))
  45 | +    if args.output:
  46 | +        file = open(args.output, "x")
  47 | +        file.write('{0}:{1}${2}'.format(args.username, salt, password_hmac))
  48 | +        print('Your password:\n{0}'.format(args.password))

ryanofsky commented at 12:58 PM on November 18, 2020:

In commit "rpcauth: Support storing credentials in a file" (ff5d7fa1e4c340657110436a29aecff85de6eb80)

Same line is printed in else branch, could dedup.

in share/rpcauth/rpcauth.py:43 in ff5d7fa1e4

  38 | @@ -38,9 +39,14 @@ def main():
  39 |      salt = generate_salt(16)
  40 |      password_hmac = password_to_hmac(salt, args.password)
  41 |  
  42 | -    print('String to be appended to bitcoin.conf:')
  43 | -    print('rpcauth={0}:{1}${2}'.format(args.username, salt, password_hmac))
  44 | -    print('Your password:\n{0}'.format(args.password))
  45 | +    if args.output:
  46 | +        file = open(args.output, "x")

ryanofsky commented at 1:00 PM on November 18, 2020:

In commit "rpcauth: Support storing credentials in a file" (ff5d7fa1e4c340657110436a29aecff85de6eb80)

Doesn't really matter but

with open(args.output, "x") as file:
   file.write(...)

Is slightly better style because it ensures that the file is closed immediately after the write.

Also may wish to avoid file variable name and use something like f or fp since file is a builtin function.

luke-jr commented at 11:26 PM on November 24, 2020:

Mode should probably be 'a'

promag commented at 11:37 PM on November 24, 2020:

I was thinking it should fail if the file already exists.

luke-jr commented at 12:05 AM on November 25, 2020:

Then how would you build a file with many lines?

luke-jr commented at 2:27 AM on November 30, 2020:

Needs to specify encoding

        file = open(args.output, "x", encoding="utf8")

ryanofsky approved

ryanofsky commented at 1:02 PM on November 18, 2020: contributor

Code review ACK ff5d7fa1e4c340657110436a29aecff85de6eb80

DrahtBot commented at 10:17 PM on November 23, 2020: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20461 (rpc: Validate -rpcauth arguments by promag)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

luke-jr changes_requested

promag marked this as a draft on Nov 25, 2020

in src/httprpc.cpp:253 in ff5d7fa1e4

 249 | @@ -256,9 +250,24 @@ static bool InitRPCAuthentication()
 250 |          LogPrintf("Config options rpcuser and rpcpassword will soon be deprecated. Locally-run instances may remove rpcuser to use cookie-based auth, or may be replaced with rpcauth. Please see share/rpcauth for rpcauth auth generation.\n");
 251 |          strRPCUserColonPass = gArgs.GetArg("-rpcuser", "") + ":" + gArgs.GetArg("-rpcpassword", "");
 252 |      }
 253 | -    if (gArgs.GetArg("-rpcauth","") != "")
 254 | +    if (gArgs.GetArg("-rpcauth", "") != "" || gArgs.GetArg("-rpcauthfile", "") != "")

luke-jr commented at 1:17 AM on August 31, 2021:

This breaks the ability to set -rpcauth= to void all prior -rpcauth parameters.

DrahtBot added the label Needs rebase on Dec 13, 2021

DrahtBot commented at 11:19 PM on December 13, 2021: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>

DrahtBot commented at 1:07 PM on March 21, 2022: contributor

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

fanquake commented at 10:47 AM on December 6, 2022: member

This has been in draft for ~ 2 years now. So I'm going to close it. Feel free to comment / ping if you'd like this re-opend.

fanquake closed this on Dec 6, 2022

schildbach commented at 12:58 PM on December 6, 2022: contributor

I'd vote for re-open. The underlying issue that bitcoind is inherently difficult to use with Docker secrets, because it can't read secrets from a file, is still present. See #20387.

fanquake commented at 1:00 PM on December 6, 2022: member

There's no point re-opening, if the author isn't working on it. If that's the case, we can mark it up for grabs, and someone else can pick it up (they could also just do that now).

bitcoin locked this on Dec 6, 2023