init: Add option for rpccookie permissions

aureleoules commented at 9:23 am on September 14, 2022: member

Adds a bitcoind launch option -rpccookieperms to configure the file permissions of the cookie. Can be used like ./src/bitcoind -rpccookieperms=0660

Closes #25270.

fanquake commented at 9:32 am on September 14, 2022: member

https://github.com/bitcoin/bitcoin/pull/26088/checks?check_run_id=8346907823:

0The locale dependent function strtol(...) appears to be used:
1src/httprpc.cpp:            const auto perms{strtol(cookie_perms_arg->c_str(), &cookie_perms_end, 8)};
2
3Unnecessary locale depedence can cause bugs that are very tricky to isolate and fix. Please avoid using locale dependent functions if possible.

aureleoules force-pushed on Sep 14, 2022

aureleoules commented at 9:43 am on September 14, 2022: member

Fixed lint error.

aureleoules force-pushed on Sep 14, 2022

in src/rpc/request.cpp:101 in aa5afedd51 outdated

 97@@ -98,6 +98,15 @@ bool GenerateAuthCookie(std::string *cookie_out)
 98     file << cookie;
 99     file.close();
100 
101+    if(cookie_perms) {

brunoerg commented at 5:01 pm on September 14, 2022:

nit:

0    if (cookie_perms) {

in src/httprpc.cpp:259 in aa5afedd51 outdated

255         LogPrintf("Using random cookie authentication.\n");
256-        if (!GenerateAuthCookie(&strRPCUserColonPass)) {
257+
258+        std::optional<fs::perms> cookie_perms;
259+        auto cookie_perms_arg{gArgs.GetArg("-rpccookieperms")};
260+        if(cookie_perms_arg) {

kristapsk commented at 5:08 pm on September 14, 2022:

nit

0        if (cookie_perms_arg) {

in src/httprpc.cpp:261 in aa5afedd51 outdated

257+
258+        std::optional<fs::perms> cookie_perms;
259+        auto cookie_perms_arg{gArgs.GetArg("-rpccookieperms")};
260+        if(cookie_perms_arg) {
261+            auto perms{StringToOctal(*cookie_perms_arg)};
262+            if(!perms) {

kristapsk commented at 5:08 pm on September 14, 2022:

nit

0            if (!perms) {

jonatack commented at 5:47 pm on September 14, 2022:

@aureleoules running clang-format on your changes before pushing avoids these little issues; see /contrib/devtools/clang-format-diff.py for more info

aureleoules commented at 6:28 pm on September 14, 2022:

Yes I forgot to run it… Fixed the issues by running it.

aureleoules force-pushed on Sep 14, 2022

kristapsk approved

kristapsk commented at 6:59 pm on September 14, 2022: contributor

ACK 857d46e47a18265eb229a1ea20064d634786bc72

ghost commented at 11:50 am on September 15, 2022: none

This will work only for Linux. Added a few lines in src/rpc/request.cpp from an example on https://en.cppreference.com/w/cpp/filesystem/permissions to test and print permissions in logs:

 0+void perms(fs::perms p)
 1+{
 2+   std::cout << ((p & fs::perms::owner_read) != fs::perms::none ? "r" : "-")
 3+             << ((p & fs::perms::owner_write) != fs::perms::none ? "w" : "-")
 4+              << ((p & fs::perms::owner_exec) != fs::perms::none ? "x" : "-")
 5+              << ((p & fs::perms::group_read) != fs::perms::none ? "r" : "-")
 6+              << ((p & fs::perms::group_write) != fs::perms::none ? "w" : "-")
 7+              << ((p & fs::perms::group_exec) != fs::perms::none ? "x" : "-")
 8+              << ((p & fs::perms::others_read) != fs::perms::none ? "r" : "-")
 9+              << ((p & fs::perms::others_write) != fs::perms::none ? "w" : "-")
10+              << ((p & fs::perms::others_exec) != fs::perms::none ? "x" : "-")
11+              << '\n';
12+}
13
14bool GenerateAuthCookie(std::string* cookie_out, std::optional<fs::perms> cookie_perms)
15{
16  const size_t COOKIE_SIZE = 32;
17  unsigned char rand_pwd[COOKIE_SIZE];
18  GetRandBytes(rand_pwd);
19  std::string cookie = COOKIEAUTH_USER + ":" + HexStr(rand_pwd);
20
21  /** the umask determines what permissions are used to create this file -
22   * these are set to 077 in init.cpp unless overridden with -sysperms.
23   */
24  std::ofstream file;
25  fs::path filepath_tmp = GetAuthCookieFile(true);
26  file.open(filepath_tmp);
27  if (!file.is_open()) {
28      LogPrintf("Unable to open cookie authentication file %s for writing\n", fs::PathToString(filepath_tmp));
29      return false;
30  }
31  file << cookie;
32  file.close();
33
34  if (cookie_perms) {
35      std::error_code code;
36      std::filesystem::permissions(filepath_tmp, *cookie_perms, std::filesystem::perm_options::replace, code);
37      if (code) {
38          LogPrintf("Unable to set permissions on cookie authentication file %s\n", fs::PathToString(filepath_tmp));
39          return false;
40      }
41  }
42
43  fs::path filepath = GetAuthCookieFile(false);
44  if (!RenameOver(filepath_tmp, filepath)) {
45      LogPrintf("Unable to rename cookie authentication file %s to %s\n", fs::PathToString(filepath_tmp), fs::PathToString(filepath));
46      return false;
47  }
48  LogPrintf("Generated RPC authentication cookie %s\n", fs::PathToString(filepath));
49+ LogPrintf("Permissions used for cookie: ");
50+ perms(fs::status(filepath).permissions());
51
52  if (cookie_out)
53      *cookie_out = cookie;
54  return true;
55}

Windows:

 0.\bitcoind.exe
 1
 22022-09-15T09:57:30Z Using random cookie authentication.
 32022-09-15T09:57:30Z Generated RPC authentication cookie C:\Users\test\AppData\Roaming\Bitcoin\signet\.cookie
 42022-09-15T09:57:30Z Permissions used for cookie: rw-rw-rw-
 5
 6.\bitcoind.exe -rpccookieperms=0660
 7
 82022-09-15T09:51:36Z Using random cookie authentication.
 92022-09-15T09:51:36Z Generated RPC authentication cookie C:\Users\test\AppData\Roaming\Bitcoin\signet\.cookie
102022-09-15T09:51:36Z Permissions used for cookie: rw-rw-rw-

Fedora:

 0bitcoind
 1
 22022-09-15T11:34:59Z Using random cookie authentication.
 32022-09-15T11:34:59Z Generated RPC authentication cookie /home/test/.bitcoin/regtest/.cookie
 42022-09-15T11:34:59Z Permissions used for cookie: rw-------
 5
 6bitcoind -rpccookieperms=0660
 7
 82022-09-15T11:39:37Z Using random cookie authentication.
 92022-09-15T11:39:37Z Generated RPC authentication cookie /home/test/.bitcoin/regtest/.cookie
102022-09-15T11:39:37Z Permissions used for cookie: rw-rw----

init: Add option for rpccookie permissions

Adds a bitcoind launch option `-rpccookieperms` to configure
the file permissions of the cookie.

cc0d0aeca6

aureleoules force-pushed on Sep 16, 2022

aureleoules commented at 7:11 pm on September 16, 2022: member

Added the log message with a slightly modified version. Thanks @1440000bytes.

unknown commented at 7:34 pm on September 16, 2022: none

ACK https://github.com/bitcoin/bitcoin/pull/26088/commits/cc0d0aeca684c73c8f8538ca67ccc6866b786b25

I don’t have the time time to test it furtther

DrahtBot commented at 3:42 am on September 23, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	1440000bytes
Stale ACK	kristapsk

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

dongcarl commented at 4:11 pm on November 10, 2022: contributor

I’m wondering: would it be perhaps simpler to just state in the documentation that you should run bitcoind with a umask of 0002 and specify -sysperms if you want group access to the cookie?

A consideration here is that now all bitcoind created files will be group accessible including wallet files, but given that the group has access to the cookie, if they have the wallet password they can still dumpwallet, so the UNIX permissions don’t matter as much (or is just a defense-in-depth thing). It seems that #17127 also solves this as well.

In any case, this is something that I run into time and again when deploying Bitcoin Core alongside other services, so I’d love to see some documentation on it.

fanquake commented at 4:24 pm on February 6, 2023: member

@aureleoules did you want to reponds to Carls thoughts here?

It seems that #17127 also solves this as well.

It’s probably likey that 17127 will be merged shortly.

aureleoules commented at 5:12 pm on February 6, 2023: member

I don’t have a preference as I don’t have much experience using this feature. Happy to close this PR if #17127 gets merged.

fanquake commented at 10:45 am on February 7, 2023: member

Happy to close this PR if #17127 gets merged.

17127 has been merged. Closing this.

fanquake closed this on Feb 7, 2023

willcl-ark commented at 9:51 am on April 19, 2023: contributor

@aureleoules @fanquake

I think this could be re-opened to solve #25270 if @aureleoules is happy to pick it up again, otherwise I’d be happy to?

Whilst #17127 has given us more sane and secure defaults, permitting group members to access the cookie without resorting to -startupnotify wortkarounds or similar seems reasonable to me.

One the one hand any processes with access to the cookie could just as well be running as the same user as bitcoind on the host, as the cookie essentially gives them full access to funds. And if users took that approach this change wouldn’t be necessary. But I also understand people’s desire for compartmentalisation and running different processes as different users, who share a group.

If we don’t want to include this feature (but I think we should) then we should opt to close #25270 also.

aureleoules commented at 3:04 pm on April 19, 2023: member

I don’t have much time to work on Core so feel free to pick this up @willcl-ark.

in src/httprpc.cpp:261 in cc0d0aeca6

257+
258+        std::optional<fs::perms> cookie_perms;
259+        auto cookie_perms_arg{gArgs.GetArg("-rpccookieperms")};
260+        if (cookie_perms_arg) {
261+            auto perms{StringToOctal(*cookie_perms_arg)};
262+            if (!perms) {

luke-jr commented at 6:48 pm on July 26, 2023:

Maybe check that cookie_perms_arg isn’t a 1/parameterless.

willcl-ark commented at 9:53 am on July 27, 2023:

Thanks for the reminder on this luke, as I said I’d take it up previously. I took these changes in opening #28167

in src/rpc/request.cpp:111 in cc0d0aeca6

110@@ -98,12 +111,22 @@ bool GenerateAuthCookie(std::string *cookie_out)
111     file << cookie;

luke-jr commented at 6:51 pm on July 26, 2023:

Should probably set permissions before writing to the file (but after opening it, so we already have write access), just in case they are more restrictive?

in src/rpc/request.h:13 in cc0d0aeca6

 6@@ -7,8 +7,10 @@
 7 #define BITCOIN_RPC_REQUEST_H
 8 
 9 #include <any>
10+#include <optional>
11 #include <string>
12 
13+#include <fs.h>

luke-jr commented at 6:53 pm on July 26, 2023:

Rebase, <util/fs.h>

in src/rpc/request.cpp:129 in cc0d0aeca6

124     if (!RenameOver(filepath_tmp, filepath)) {
125         LogPrintf("Unable to rename cookie authentication file %s to %s\n", fs::PathToString(filepath_tmp), fs::PathToString(filepath));
126         return false;
127     }
128     LogPrintf("Generated RPC authentication cookie %s\n", fs::PathToString(filepath));
129+    LogPrintf("Permissions used for cookie: %s\n", perms_to_str(fs::status(filepath).permissions()));

luke-jr commented at 6:55 pm on July 26, 2023:

0    LogPrintf("Permissions used for cookie%s: %s\n",
1              cookie_perms ? " (set by -rpccookieperms)" : "",
2              perms_to_str(fs::status(filepath).permissions()));

init: Add option for rpccookie permissions #26088

Reviews

Conflicts