rpc: Support -rpcauthfile argument by promag · Pull Request #20407 · bitcoin/bitcoin

promag · 2020-11-17T00:01:03Z

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.

maflcko · 2020-11-17T07:33:29Z

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

promag · 2020-11-17T08:48:39Z

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

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

To be used with -rpcauthfile

ryanofsky

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.

ryanofsky · 2020-11-17T13:15:51Z

src/httprpc.cpp

+        for (std::string rpcauth : gArgs.GetArgs("-rpcauth")) {
+            std::vector<std::string> fields;
+            boost::split(fields, rpcauth, boost::is_any_of(":$"));
+            if (fields.size() == 3) g_rpcauth.push_back(fields);


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

Yeah it is, can be improved.

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

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

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.

promag · 2020-11-17T13:43:01Z

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

laanwj · 2020-11-18T10:29:40Z

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 · 2020-11-18T11:36:22Z

@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 · 2020-11-18T12:40:03Z

@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 · 2020-11-18T12:48:40Z

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 · 2020-11-18T12:49:46Z

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.

ryanofsky

Code review ACK ff5d7fa

ryanofsky · 2020-11-18T12:55:08Z

src/httprpc.cpp

+            std::getline(file, rpcauth);
+            std::vector<std::string> fields;
+            boost::split(fields, rpcauth, boost::is_any_of(":$"));
+            if (fields.size() == 3) g_rpcauth.push_back(fields);


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

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

I think this should refuse to load

You mean the process should exit?

re: #20407 (comment)

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.

See above, it's not necessarily an error

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

ryanofsky · 2020-11-18T12:58:39Z

share/rpcauth/rpcauth.py

+    if args.output:
+        file = open(args.output, "x")
+        file.write('{0}:{1}${2}'.format(args.username, salt, password_hmac))
+        print('Your password:\n{0}'.format(args.password))


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

Same line is printed in else branch, could dedup.

ryanofsky · 2020-11-18T13:00:57Z

share/rpcauth/rpcauth.py

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


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

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.

DrahtBot · 2020-11-23T22:17:12Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

rpc: Validate -rpcauth arguments #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 · 2020-11-24T23:26:08Z

share/rpcauth/rpcauth.py

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


Mode should probably be 'a'

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

Then how would you build a file with many lines?

luke-jr · 2020-11-24T23:28:12Z

src/httprpc.cpp

+        for (std::string rpcauth : gArgs.GetArgs("-rpcauth")) {
+            std::vector<std::string> fields;
+            boost::split(fields, rpcauth, boost::is_any_of(":$"));
+            if (fields.size() == 3) g_rpcauth.push_back(fields);


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.

luke-jr · 2020-11-24T23:29:48Z

src/httprpc.cpp

+            std::getline(file, rpcauth);
+            std::vector<std::string> fields;
+            boost::split(fields, rpcauth, boost::is_any_of(":$"));
+            if (fields.size() == 3) g_rpcauth.push_back(fields);


See above, it's not necessarily an error

luke-jr · 2020-11-30T02:27:08Z

share/rpcauth/rpcauth.py

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


Needs to specify encoding

Suggested change

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

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

luke-jr · 2021-08-31T01:17:39Z

src/httprpc.cpp

        strRPCUserColonPass = gArgs.GetArg("-rpcuser", "") + ":" + gArgs.GetArg("-rpcpassword", "");
    }
-    if (gArgs.GetArg("-rpcauth","") != "")
+    if (gArgs.GetArg("-rpcauth", "") != "" || gArgs.GetArg("-rpcauthfile", "") != "")


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

DrahtBot · 2021-12-13T23:19:04Z

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

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

DrahtBot · 2022-03-21T13:07:02Z

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 · 2022-12-06T10:47:34Z

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.

schildbach · 2022-12-06T12:58:16Z

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 · 2022-12-06T13:00:58Z

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

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

promag added 3 commits November 17, 2020 12:56

rpc: Refactor to process -rpcauth once

6272dcd

rpc: Support -rpcauthfile argument

10ef732

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

rpcauth: Support storing credentials in a file

ff5d7fa

To be used with -rpcauthfile

promag force-pushed the 2020-11-rpcauthfile branch from 8a3b62c to ff5d7fa Compare November 17, 2020 12:56

ryanofsky reviewed Nov 17, 2020

View reviewed changes

promag mentioned this pull request Nov 17, 2020

Bitcoind to use RPC authentication cookie from file #20387

Closed

ryanofsky approved these changes Nov 18, 2020

View reviewed changes

DrahtBot mentioned this pull request Nov 23, 2020

rpc: Validate -rpcauth arguments #20461

Merged

luke-jr suggested changes Nov 24, 2020

View reviewed changes

promag marked this pull request as draft November 25, 2020 00:09

luke-jr reviewed Nov 30, 2020

View reviewed changes

luke-jr reviewed Aug 31, 2021

View reviewed changes

DrahtBot added the Needs rebase label Dec 13, 2021

fanquake closed this Dec 6, 2022

bitcoin locked and limited conversation to collaborators Dec 6, 2023

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

Conversation

promag commented Nov 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Nov 17, 2020

Uh oh!

promag commented Nov 17, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Nov 17, 2020

Uh oh!

laanwj commented Nov 18, 2020

Uh oh!

promag commented Nov 18, 2020

Uh oh!

ryanofsky commented Nov 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Nov 18, 2020

Uh oh!

promag commented Nov 18, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Nov 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Nov 23, 2020

Conflicts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Dec 13, 2021

Uh oh!

DrahtBot commented Mar 21, 2022

Uh oh!

fanquake commented Dec 6, 2022

Uh oh!

schildbach commented Dec 6, 2022

Uh oh!

fanquake commented Dec 6, 2022

Uh oh!

promag commented Nov 17, 2020 •

edited

Loading

ryanofsky commented Nov 18, 2020 •

edited

Loading

ryanofsky Nov 18, 2020 •

edited

Loading