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

willcl-ark · 2023-07-27T09:52:15Z

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.

DrahtBot · 2023-07-27T09:52:18Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	achow101, ryanofsky, tdb3
Approach ACK	jamesob

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)

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.

src/httprpc.cpp

DrahtBot · 2023-10-25T19:20:28Z

Are you still working on this?

willcl-ark · 2023-11-20T14:24:58Z

Updated to address luke's comment and added a test to ensure rpccookieperms are being applied

willcl-ark · 2023-11-21T11:36:55Z

Rebased now that #28905 is merged.

luke-jr · 2023-12-29T01:29:05Z

src/rpc/request.cpp

+    if ((p & fs::perms::set_uid)    != fs::perms::none) special = '4';
+    if ((p & fs::perms::set_gid)    != fs::perms::none) special = '2';
+    if ((p & fs::perms::sticky_bit) != fs::perms::none) special = '1';


This won't work if multiple bits are set

These bits seem to typically manifest by replacing the 'x' field with either 's' (setuid/setgid+executable), 'S' (setuid/setgid WITHOUT executable), 't' (sticky+exec), or 'T' (sticky NO exec)

kristapsk · 2023-12-29T09:30:35Z

Should the permissions that can be set limited here? There is no reason to ever set +x on an RPC cookie file.

luke-jr · 2023-12-30T23:50:42Z

I'm not sure there's a reason to ever set +w either... maybe it should just be -rpccookieaccess user/group/all or something

kristapsk · 2023-12-31T01:10:56Z

I'm not sure there's a reason to ever set +w either... maybe it should just be -rpccookieaccess user/group/all or something

Agree, makes sense. Giving read access to other users is the only use case of this functionality I know of.

Set owner read/write permissions on cookie files by default. On Windows this will cause the write bit to be set. Github-Pull: bitcoin#28167 Rebased-From: 35df853

Helpers to convert from pemissions to strings and visa-versa. Github-Pull: bitcoin#28167 Rebased-From: e7c7bdc

Add a bitcoind launch option `-rpccookieperms` to configure the file permissions of the cookie on Unix systems. Github-Pull: bitcoin#28167 Rebased-From: ae7ec04

Tests various perms on non-Windows OSes Github-Pull: bitcoin#28167 Rebased-From: ab3245f

Github-Pull: bitcoin#28167 Rebased-From: 36816dc

Co-authored-by: Will Clark <will@256k1.dev> Github-Pull: bitcoin#28167 Rebased-From: 7456c3a

ryanofsky

Code review ACK e479803. Seems very clean now with the test simplification and without the win32 stuff.

ryanofsky · 2024-06-24T23:00:31Z

src/rpc/request.cpp

        return false;
    }
+    std::error_code code;


In commit "init: add option for rpccookie permissions" (7ad5349)

Could move this variable inside the if statement since it's not used afterwards.

Moved in 9eff560

tdb3

re ACK e479803

Ran rpc_users locally (passed).
Re-ran the manual check in #28167 (review) (same results).
Left one minor nit.

tdb3 · 2024-06-25T01:02:34Z

src/rpc/request.h


 UniValue JSONRPCRequestObj(const std::string& strMethod, const UniValue& params, const UniValue& id);
 UniValue JSONRPCReplyObj(const UniValue& result, const UniValue& error, const UniValue& id);
 std::string JSONRPCReply(const UniValue& result, const UniValue& error, const UniValue& id);
 UniValue JSONRPCError(int code, const std::string& message);

 /** Generate a new RPC authentication cookie and write it to disk */
-bool GenerateAuthCookie(std::string *cookie_out);
+bool GenerateAuthCookie(std::string* cookie_out, std::optional<fs::perms> cookie_perms);


nit: If touching this file again, may want to consider defining a default argument for cookie_perms. Since std::optional is used, I'm assuming the intent is not to force the caller to provide an optional object.

diff --git a/src/rpc/request.h b/src/rpc/request.h index b40df16631..c7e723d962 100644 --- a/src/rpc/request.h +++ b/src/rpc/request.h @@ -19,7 +19,7 @@ std::string JSONRPCReply(const UniValue& result, const UniValue& error, const Un UniValue JSONRPCError(int code, const std::string& message); /** Generate a new RPC authentication cookie and write it to disk */ -bool GenerateAuthCookie(std::string* cookie_out, std::optional<fs::perms> cookie_perms); +bool GenerateAuthCookie(std::string* cookie_out, std::optional<fs::perms> cookie_perms=std::nullopt); /** Read the RPC authentication cookie from disk */ bool GetAuthCookie(std::string *cookie_out); /** Delete RPC authentication cookie from disk */

GenerateAuthCookie() is currently used with the argument provided, so not a big deal either way.

Added in 49d5bfd

achow101 · 2024-06-26T17:35:31Z

test/functional/rpc_users.py

+    def test_rpccookieperms(self):
+        p = {"owner": 0o600, "group": 0o640, "all": 0o644}
+
+        if os.name == 'nt':


In 31cc8bc "test: add rpccookieperms test"

Since #29034, the preferred convention is to use platform.system() rather than os.name.

Thanks, updated in 49d5bfd

achow101 · 2024-06-26T17:41:26Z

test/functional/rpc_users.py

+        p = {"owner": 0o600, "group": 0o640, "all": 0o644}
+
+        if os.name == 'nt':
+            raise SkipTest(f"Skip cookie file permissions checks as OS detected as: {os.name=}")


In 31cc8bc "test: add rpccookieperms test"

This is incorrect. It causes the entire test file to be marked as skipped (different exit code, marked differently in the test runner output) even though everything else ran. The correct thing here is to skip this individual case, which can be achieved by simply returning.

Good catch. Might also be good to print a log message before returning for awareness.

Oh no! fixed in 49d5bfd

achow101 · 2024-06-26T17:43:03Z

src/util/fs_helpers.h

+std::string PermsToString(fs::perms p);
+std::optional<fs::perms> StringToPerms(const std::string& s);


In 38af55e "util: add PermsToString and StringToPerms"

nit: This naming is a little bit confusing to me since it suggests that these functions are inverses of each other, but they're not as their string outputs are not compatible.

I agree on reflection, gave them new names in 49d5bfd

achow101 · 2024-06-26T17:45:40Z

src/rpc/request.cpp

-    /** the umask determines what permissions are used to create this file -
-     * these are set to 0077 in common/system.cpp.


In 7ad5349 "init: add option for rpccookie permissions"

This comment is removed, but the umask is still what sets the permissions in the default case.

Since we can set the permissions explicitly in this function, I think it would make sense to always set them with owner as default rather than relying on the umask that happens somewhere else.

re: #28167 (comment)

This comment is removed, but the umask is still what sets the permissions in the default case.

Good catch, it would be good to add back the comment.

Since we can set the permissions explicitly in this function, I think it would make sense to always set them with owner as default rather than relying on the umask that happens somewhere else.

The PR was actually doing this originally, but I suggested doing the opposite in #28167 (comment). It makes sense to me that when -rpccookieperms option is not specified, bitcoind would not set permissions on this file, just like it is not setting permissions on other files.

It seems fragile to rely on the fs::permissions call doing the right thing on all filesystems, when its behavior is not specified anywhere and we have never relied on it before. The documentation points to some odd ways it can be behave like "changing some bits may automatically change others" and we don't know what unusual things it may do on fuse filesystems or filesystems that use ACLs.

I've un-removed the comment in 9eff560, as I agree with @ryanofsky that not changing the existant behaviour is the "safer" default, and new behaviour can be introduced with the new option.

tdb3

re ACK 4f6b00c

Ran rpc_users locally (passed).
Re-ran the manual check in #28167 (review) (same results).
Also sanity checked on a Windows machine running Python 3.9.13 that platform.system() does return "Windows".
Left one minor nit.

src/util/fs_helpers.h

PermsToSymbolicString will convert from fs::perms to string type 'rwxrwxrwx'. InterpretPermString will convert from a user-supplied "perm string" such as 'owner', 'group' or 'all, into appropriate fs::perms.

Add a bitcoind launch option `-rpccookieperms` to configure the file permissions of the cookie on Unix systems.

Tests various perms on non-Windows OSes

Co-authored-by: tdb3 <106488469+tdb3@users.noreply.github.com>

DrahtBot · 2024-06-27T14:16:19Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/26763232631}

achow101 · 2024-06-27T18:20:44Z

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

Sjors · 2024-07-02T10:33:13Z

Nice! Will try this out soon.

Update 2024-07-11: works like a charm and allowed me to drop some ugliness from systemd config.

willcl-ark mentioned this pull request Jul 27, 2023

init: Add option for rpccookie permissions #26088

Closed

DrahtBot mentioned this pull request Jul 27, 2023

Support JSON-RPC 2.0 when requested by client #27101

Merged

luke-jr suggested changes Aug 5, 2023

View reviewed changes

src/httprpc.cpp Outdated Show resolved Hide resolved

src/httprpc.cpp Outdated Show resolved Hide resolved

willcl-ark force-pushed the 2023-07-rpccookie-perms branch from a8e9278 to b4b0d2a Compare August 9, 2023 13:26

DrahtBot added the CI failed label Aug 9, 2023

luke-jr reviewed Aug 10, 2023

View reviewed changes

src/httprpc.cpp Outdated Show resolved Hide resolved

DrahtBot removed the CI failed label Aug 18, 2023

DrahtBot added the CI failed label Oct 25, 2023

DrahtBot mentioned this pull request Nov 3, 2023

rpc: keep .cookie file if it was not generated #28784

Merged

willcl-ark force-pushed the 2023-07-rpccookie-perms branch 2 times, most recently from 49e7859 to 4b469d3 Compare November 20, 2023 14:24

willcl-ark force-pushed the 2023-07-rpccookie-perms branch 4 times, most recently from ee4ead3 to 65d43b1 Compare November 21, 2023 09:46

DrahtBot removed the CI failed label Nov 21, 2023

willcl-ark force-pushed the 2023-07-rpccookie-perms branch from 65d43b1 to 4cb56cf Compare November 21, 2023 11:35

DrahtBot added the Needs rebase label Dec 1, 2023

willcl-ark force-pushed the 2023-07-rpccookie-perms branch from 4cb56cf to f15f4b3 Compare December 7, 2023 11:46

willcl-ark mentioned this pull request Dec 7, 2023

/home/bitcoin/.bitcoin permissions vs. /data/bitcoin raspibolt/raspibolt#1347

Closed

DrahtBot removed the Needs rebase label Dec 7, 2023

luke-jr reviewed Dec 29, 2023

View reviewed changes

willcl-ark force-pushed the 2023-07-rpccookie-perms branch from f15f4b3 to ad750b3 Compare January 8, 2024 12:10

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 13, 2024

util: add PermsToString and StringToPerms

8ad18ef

Helpers to convert from pemissions to strings and visa-versa. Github-Pull: bitcoin#28167 Rebased-From: e7c7bdc

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 13, 2024

test: add rpccookieperms test

ba25bec

Tests various perms on non-Windows OSes Github-Pull: bitcoin#28167 Rebased-From: ab3245f

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 13, 2024

util: add fs helpers to convert perms to str and octal

a1b1d33

Github-Pull: bitcoin#28167 Rebased-From: 36816dc

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 13, 2024

httprpc: allow specifying rpccookie permissions in octal

a6d297a

Co-authored-by: Will Clark <will@256k1.dev> Github-Pull: bitcoin#28167 Rebased-From: 7456c3a

willcl-ark force-pushed the 2023-07-rpccookie-perms branch from 9617e42 to e479803 Compare June 24, 2024 12:36

ryanofsky approved these changes Jun 24, 2024

View reviewed changes

DrahtBot requested a review from tdb3 June 24, 2024 23:07

tdb3 approved these changes Jun 25, 2024

View reviewed changes

DrahtBot mentioned this pull request Jun 25, 2024

RFC: Instanced logs #30338

Open

achow101 reviewed Jun 26, 2024

View reviewed changes

willcl-ark force-pushed the 2023-07-rpccookie-perms branch from e479803 to 4f6b00c Compare June 27, 2024 09:11

tdb3 approved these changes Jun 27, 2024

View reviewed changes

src/util/fs_helpers.h Outdated Show resolved Hide resolved

DrahtBot requested a review from ryanofsky June 27, 2024 13:33

util: add perm string helper functions

7df03f1

PermsToSymbolicString will convert from fs::perms to string type 'rwxrwxrwx'. InterpretPermString will convert from a user-supplied "perm string" such as 'owner', 'group' or 'all, into appropriate fs::perms.

willcl-ark force-pushed the 2023-07-rpccookie-perms branch from 4f6b00c to 8802579 Compare June 27, 2024 13:55

willcl-ark and others added 3 commits June 27, 2024 15:08

init: add option for rpccookie permissions

f467aed

Add a bitcoind launch option `-rpccookieperms` to configure the file permissions of the cookie on Unix systems.

test: add rpccookieperms test

d2afa26

Tests various perms on non-Windows OSes

doc: detail -rpccookieperms option

73f0a6c

Co-authored-by: tdb3 <106488469+tdb3@users.noreply.github.com>

willcl-ark force-pushed the 2023-07-rpccookie-perms branch from 8802579 to 73f0a6c Compare June 27, 2024 14:16

DrahtBot added CI failed and removed CI failed labels Jun 27, 2024

DrahtBot requested a review from tdb3 June 27, 2024 18:20

ryanofsky approved these changes Jun 27, 2024

View reviewed changes

tdb3 approved these changes Jun 27, 2024

View reviewed changes

ryanofsky merged commit d38dbaa into bitcoin:master Jun 27, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

willcl-ark commented Jul 27, 2023 •

edited

Loading

DrahtBot commented Jul 27, 2023 •

edited

Loading

DrahtBot commented Oct 25, 2023

willcl-ark commented Nov 20, 2023

willcl-ark commented Nov 21, 2023

luke-jr Dec 29, 2023

luke-jr Dec 29, 2023

kristapsk commented Dec 29, 2023

luke-jr commented Dec 30, 2023

kristapsk commented Dec 31, 2023

ryanofsky left a comment

ryanofsky Jun 24, 2024

willcl-ark Jun 27, 2024

tdb3 left a comment

tdb3 Jun 25, 2024 •

edited

Loading

willcl-ark Jun 27, 2024

achow101 Jun 26, 2024

willcl-ark Jun 27, 2024

achow101 Jun 26, 2024

tdb3 Jun 27, 2024

willcl-ark Jun 27, 2024

achow101 Jun 26, 2024

willcl-ark Jun 27, 2024

achow101 Jun 26, 2024

ryanofsky Jun 26, 2024

willcl-ark Jun 27, 2024

tdb3 left a comment

DrahtBot commented Jun 27, 2024

achow101 commented Jun 27, 2024

ryanofsky left a comment

tdb3 left a comment

Sjors commented Jul 2, 2024 •

edited

Loading

		std::string PermsToString(fs::perms p);
		std::optional<fs::perms> StringToPerms(const std::string& s);

		/** the umask determines what permissions are used to create this file -
		* these are set to 0077 in common/system.cpp.

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

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

Conversation

willcl-ark commented Jul 27, 2023 • edited Loading

DrahtBot commented Jul 27, 2023 • edited Loading

Code Coverage

Reviews

Conflicts

DrahtBot commented Oct 25, 2023

willcl-ark commented Nov 20, 2023

willcl-ark commented Nov 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristapsk commented Dec 29, 2023

luke-jr commented Dec 30, 2023

kristapsk commented Dec 31, 2023

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdb3 left a comment

Choose a reason for hiding this comment

tdb3 Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdb3 left a comment

Choose a reason for hiding this comment

DrahtBot commented Jun 27, 2024

achow101 commented Jun 27, 2024

ryanofsky left a comment

Choose a reason for hiding this comment

tdb3 left a comment

Choose a reason for hiding this comment

Sjors commented Jul 2, 2024 • edited Loading

willcl-ark commented Jul 27, 2023 •

edited

Loading

DrahtBot commented Jul 27, 2023 •

edited

Loading

tdb3 Jun 25, 2024 •

edited

Loading

Sjors commented Jul 2, 2024 •

edited

Loading