init: Add option for rpccookie permissions #26088

aureleoules · 2022-09-14T09:23:49Z

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 · 2022-09-14T09:32:44Z

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

The locale dependent function strtol(...) appears to be used:
src/httprpc.cpp:            const auto perms{strtol(cookie_perms_arg->c_str(), &cookie_perms_end, 8)};

Unnecessary locale depedence can cause bugs that are very tricky to isolate and fix. Please avoid using locale dependent functions if possible.

aureleoules · 2022-09-14T09:43:39Z

Fixed lint error.

src/rpc/request.cpp

src/httprpc.cpp

kristapsk

ACK 857d46e

ghost · 2022-09-15T11:50:43Z

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:

diff

+void perms(fs::perms p)
+{
+   std::cout << ((p & fs::perms::owner_read) != fs::perms::none ? "r" : "-")
+             << ((p & fs::perms::owner_write) != fs::perms::none ? "w" : "-")
+              << ((p & fs::perms::owner_exec) != fs::perms::none ? "x" : "-")
+              << ((p & fs::perms::group_read) != fs::perms::none ? "r" : "-")
+              << ((p & fs::perms::group_write) != fs::perms::none ? "w" : "-")
+              << ((p & fs::perms::group_exec) != fs::perms::none ? "x" : "-")
+              << ((p & fs::perms::others_read) != fs::perms::none ? "r" : "-")
+              << ((p & fs::perms::others_write) != fs::perms::none ? "w" : "-")
+              << ((p & fs::perms::others_exec) != fs::perms::none ? "x" : "-")
+              << '\n';
+}

bool GenerateAuthCookie(std::string* cookie_out, std::optional<fs::perms> cookie_perms)
{
  const size_t COOKIE_SIZE = 32;
  unsigned char rand_pwd[COOKIE_SIZE];
  GetRandBytes(rand_pwd);
  std::string cookie = COOKIEAUTH_USER + ":" + HexStr(rand_pwd);

  /** the umask determines what permissions are used to create this file -
   * these are set to 077 in init.cpp unless overridden with -sysperms.
   */
  std::ofstream file;
  fs::path filepath_tmp = GetAuthCookieFile(true);
  file.open(filepath_tmp);
  if (!file.is_open()) {
      LogPrintf("Unable to open cookie authentication file %s for writing\n", fs::PathToString(filepath_tmp));
      return false;
  }
  file << cookie;
  file.close();

  if (cookie_perms) {
      std::error_code code;
      std::filesystem::permissions(filepath_tmp, *cookie_perms, std::filesystem::perm_options::replace, code);
      if (code) {
          LogPrintf("Unable to set permissions on cookie authentication file %s\n", fs::PathToString(filepath_tmp));
          return false;
      }
  }

  fs::path filepath = GetAuthCookieFile(false);
  if (!RenameOver(filepath_tmp, filepath)) {
      LogPrintf("Unable to rename cookie authentication file %s to %s\n", fs::PathToString(filepath_tmp), fs::PathToString(filepath));
      return false;
  }
  LogPrintf("Generated RPC authentication cookie %s\n", fs::PathToString(filepath));
+ LogPrintf("Permissions used for cookie: ");
+ perms(fs::status(filepath).permissions());

  if (cookie_out)
      *cookie_out = cookie;
  return true;
}

Windows:

.\bitcoind.exe

2022-09-15T09:57:30Z Using random cookie authentication.
2022-09-15T09:57:30Z Generated RPC authentication cookie C:\Users\test\AppData\Roaming\Bitcoin\signet\.cookie
2022-09-15T09:57:30Z Permissions used for cookie: rw-rw-rw-

.\bitcoind.exe -rpccookieperms=0660

2022-09-15T09:51:36Z Using random cookie authentication.
2022-09-15T09:51:36Z Generated RPC authentication cookie C:\Users\test\AppData\Roaming\Bitcoin\signet\.cookie
2022-09-15T09:51:36Z Permissions used for cookie: rw-rw-rw-

Fedora:

bitcoind

2022-09-15T11:34:59Z Using random cookie authentication.
2022-09-15T11:34:59Z Generated RPC authentication cookie /home/test/.bitcoin/regtest/.cookie
2022-09-15T11:34:59Z Permissions used for cookie: rw-------

bitcoind -rpccookieperms=0660

2022-09-15T11:39:37Z Using random cookie authentication.
2022-09-15T11:39:37Z Generated RPC authentication cookie /home/test/.bitcoin/regtest/.cookie
2022-09-15T11:39:37Z Permissions used for cookie: rw-rw----

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

aureleoules · 2022-09-16T19:11:47Z

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

ghost

ACK cc0d0ae

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

DrahtBot · 2022-09-23T03:42:54Z

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 · 2022-11-10T16:11:51Z

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 · 2023-02-06T16:24:18Z

@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 · 2023-02-06T17:12:28Z

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 · 2023-02-07T10:45:03Z

Happy to close this PR if #17127 gets merged.

17127 has been merged. Closing this.

willcl-ark · 2023-04-19T09:51:55Z

@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 · 2023-04-19T15:04:51Z

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

luke-jr · 2023-07-26T18:48:24Z

src/httprpc.cpp

+        auto cookie_perms_arg{gArgs.GetArg("-rpccookieperms")};
+        if (cookie_perms_arg) {
+            auto perms{StringToOctal(*cookie_perms_arg)};
+            if (!perms) {


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

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

luke-jr · 2023-07-26T18:51:52Z

src/rpc/request.cpp

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


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?

luke-jr · 2023-07-26T18:53:40Z

src/rpc/request.h

 #include <string>

+#include <fs.h>


Rebase, <util/fs.h>

luke-jr · 2023-07-26T18:55:51Z

src/rpc/request.cpp

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


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

Adds a bitcoind launch option `-rpccookieperms` to configure the file permissions of the cookie. Github-Pull: bitcoin#26088 Rebased-From: cc0d0ae

73f0a6c doc: detail -rpccookieperms option (willcl-ark) d2afa26 test: add rpccookieperms test (willcl-ark) f467aed init: add option for rpccookie permissions (willcl-ark) 7df03f1 util: add perm string helper functions (willcl-ark) Pull request description: This PR picks up #26088 by aureleoules which adds a bitcoind launch option `-rpccookieperms` to set the file permissions of the cookie generated by bitcoin core. Example usage to make the generated cookie group-readable: `./src/bitcoind -rpccookieperms=group`. Accepted values for `-rpccookieperms` are `[owner|group|all]`. We let `fs::perms` handle platform-specific permissions changes. ACKs for top commit: achow101: ACK 73f0a6c ryanofsky: Code review ACK 73f0a6c. Main change since last review is no longer throwing a skip exception in the rpc test on windows, so other checks can run after it, and overall test result is passing, not skipped. Also were clarifying renames and documentation improvements. tdb3: cr ACK 73f0a6c Tree-SHA512: e800d59a44aca10e1c58ca69bf3fdde9f6ccf5eab4b7b962645af6d6bc0cfa3a357701e409c8c60d8d7744fcd33a91e77ada11790aa88cd7811ef60fab86ab11

aureleoules force-pushed the 2022-09-rpccookie-perms branch 2 times, most recently from 54f4839 to 8d7a71b Compare September 14, 2022 09:39

aureleoules force-pushed the 2022-09-rpccookie-perms branch from 8d7a71b to aa5afed Compare September 14, 2022 09:46

brunoerg reviewed Sep 14, 2022

View reviewed changes

src/rpc/request.cpp Outdated Show resolved Hide resolved

kristapsk reviewed Sep 14, 2022

View reviewed changes

src/httprpc.cpp Outdated Show resolved Hide resolved

src/httprpc.cpp Outdated Show resolved Hide resolved

aureleoules force-pushed the 2022-09-rpccookie-perms branch from aa5afed to 857d46e Compare September 14, 2022 17:28

kristapsk approved these changes Sep 14, 2022

View reviewed changes

init: Add option for rpccookie permissions

cc0d0ae

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

aureleoules force-pushed the 2022-09-rpccookie-perms branch from 857d46e to cc0d0ae Compare September 16, 2022 19:10

ghost reviewed Sep 16, 2022

View reviewed changes

kristapsk mentioned this pull request Oct 13, 2022

[BUG] BTC RPC Explorer breaks on Bitcoin Core restart raspibolt/raspibolt#848

Closed

fanquake closed this Feb 7, 2023

willcl-ark mentioned this pull request Mar 15, 2023

Allow groups of accounts to access the RPC cookie file #25270

Closed

luke-jr reviewed Jul 26, 2023

View reviewed changes

willcl-ark mentioned this pull request Jul 27, 2023

init: Add option for rpccookie permissions (replace 26088) #28167

Merged

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Aug 16, 2023

init: Add option for rpccookie permissions

bfe9843

Adds a bitcoind launch option `-rpccookieperms` to configure the file permissions of the cookie. Github-Pull: bitcoin#26088 Rebased-From: cc0d0ae

bitcoin locked and limited conversation to collaborators Jul 26, 2024

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

init: Add option for rpccookie permissions #26088

init: Add option for rpccookie permissions #26088

Uh oh!

Conversation

aureleoules commented Sep 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Sep 14, 2022

Uh oh!

aureleoules commented Sep 14, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kristapsk left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 15, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aureleoules commented Sep 16, 2022

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

dongcarl commented Nov 10, 2022

Uh oh!

fanquake commented Feb 6, 2023

Uh oh!

aureleoules commented Feb 6, 2023

Uh oh!

fanquake commented Feb 7, 2023

Uh oh!

willcl-ark commented Apr 19, 2023

Uh oh!

aureleoules commented Apr 19, 2023

Uh oh!

luke-jr Jul 26, 2023

Choose a reason for hiding this comment

Uh oh!

willcl-ark Jul 27, 2023

Choose a reason for hiding this comment

Uh oh!

luke-jr Jul 26, 2023

Choose a reason for hiding this comment

Uh oh!

luke-jr Jul 26, 2023

Choose a reason for hiding this comment

Uh oh!

luke-jr Jul 26, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aureleoules commented Sep 14, 2022 •

edited

Loading

ghost commented Sep 15, 2022 •

edited by ghost

Loading

DrahtBot commented Sep 23, 2022 •

edited

Loading