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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
238@@ -239,12 +239,37 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req)
239 return true;
240 }
241
242+static std::optional<unsigned> StringToOctal(const std::string& str)
243+{
244+ if (str.length() != 4) return std::nullopt;
StringToOctal is the right place for this check.
250+}
251+
252+static std::optional<unsigned> ConvertPermsToOctal(const std::string& str)
253+{
254+ // Padding the front with 0 sets no special modes if not provided
255+ if (str.length() == 3) return StringToOctal("0" + str);
86+static std::string perms_to_str(fs::perms p)
87+{
88+ char special = '-';
89+ if ((p & fs::perms::set_uid) != fs::perms::none) special = '4';
90+ if ((p & fs::perms::set_gid) != fs::perms::none) special = '2';
91+ if ((p & fs::perms::sticky_bit) != fs::perms::none) special = '1';
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.
Sorry for the slow response. I was away but am now back.
Thanks for the review, I agree that setting special bits is uninteresting for cookie files (I was really trying to keep the subfunctions generic in case we wanted to re-use them elsewhere). I’ve now removed ability to set special bits.
I think a little on switching to string-based settings, i.e. -rpccookieperms=group, but this quickly gets more ugly inside, as IMO the usual use-case is to be setting multiple values in the same permission. i.e. setting just user (040) means that the owner-user will NOT be able to read the cookie, as user is checked against owner before group, and if a match is found, with no read perms, the checks are exited. I feel that users are likely to want to set group in addition to user, therefore 3 digit ocal perms make more sense.
I think a little on switching to string-based settings, i.e.
-rpccookieperms=group, but this quickly gets more ugly inside, as IMO the usual use-case is to be setting multiple values in the same permission. i.e. setting justuser(040) means that the owner-user will NOT be able to read the cookie, as user is checked against owner before group, and if a match is found, with no read perms, the checks are exited. I feel that users are likely to want to setgroupin addition touser, therefore 3 digit ocal perms make more sense.
Only three permissions that makes sense IMO are 600, 640 and 644.
I think a little on switching to string-based settings, i.e.
-rpccookieperms=group, but this quickly gets more ugly inside, as IMO the usual use-case is to be setting multiple values in the same permission. i.e. setting justuser(040) means that the owner-user will NOT be able to read the cookie, as user is checked against owner before group, and if a match is found, with no read perms, the checks are exited. I feel that users are likely to want to setgroupin addition touser, therefore 3 digit ocal perms make more sense.Only three permissions that makes sense IMO are
600,640and644.
Yes. I did author a commit to be prepended the two in this PR, which changed our default cookie perms to 400 using DEFAULT_COOKIE_PERMS (with current umask it is 600), as I don’t think enabling write perms on a cookie (ever?!) that bitcoind has created makes sense? It just seemed a bit out of scope for this PR and I thought it might work best as a followup (or before) on it’s own. It could also include rejecting permissions which included writing if we wanted…
I don’t think enabling write perms on a cookie (ever?!) that bitcoind has created makes sense?
Ahh, yes, 400, 440, 444.
400, and rebased on master to try and get the unrelated CI failure to go away.
262 if (gArgs.GetArg("-rpcpassword", "") == "")
263 {
264 LogPrintf("Using random cookie authentication.\n");
265- if (!GenerateAuthCookie(&strRPCUserColonPass)) {
266+
267+ auto cookie_perms{DEFAULT_COOKIE_PERMS};
auto both here and the default. It could conceivably not be a type that supports all permission masks.
fs::perms here and the default in e838d7dcc6c835dcb936dd3b691842aaa43f22ad
100 LogPrintf("Unable to open cookie authentication file %s for writing\n", fs::PathToString(filepath_tmp));
101 return false;
102 }
103+
104+#ifdef WIN32
105+ LogPrintf("Unable to set unix file permissions on cookie using Windows systems: %s\n", fs::PathToString(filepath_tmp));
InitRPCAuthentication here, which now also returns an error if the user tries to set rpccookieperms on windows.
I don’t think Windows has strict defaults on permissions for new files, with regard to other users. This StackExchange article at least suggests the default can be manipulated manually.
Also, typically when a feature is disabled at build time, we don’t add it to argsman (except perhaps as a hidden_args, but I think that may be a bug in such cases).
110+ self.restart_node(1, extra_args=[f"-rpccookieperms={perm}"])
111+ file_stat = os.stat(cookie_file_path)
112+ actual_perms = f"{(file_stat.st_mode & PERM_BITS_UMASK):03o}"
113+ assert_equal(perm, actual_perms)
114+
115+ # Remove any leftover rpc{user|password} config options from previous tests
123+ self.log.info('Check default cookie permission')
124+ test_perm(None)
125+
126+ self.log.info('Check custom cookie permissions')
127+ test_perms = ["440", "640", "444"]
128+ [test_perm(perm) for perm in test_perms]
Feels like a hack
0 for perm in "440", "640", "444":
1 test_perm(perm)
🚧 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.
For your consideration, I have pushed a variant with some recommended changes here: https://github.com/luke-jr/bitcoin/commits/rpccookieperms-26%2Bknots/
Feel free to cherry-pick or squash into your PR as you deem best.
Thanks Luke, I agree that repeated string concatenation was not a particularly neat for creating a single string where we could simply modify in place. I’ve updated this further from your version to use a lambda in bf9b11d8f6c0af9b2c76674481e5905a00240cf5.
Re Windows. Yes, fs::permissions is “supported”, but IIUC without devolving into access control lists, it all boils down to whether the write bit is enabled. I don’t see the sense in supporting either of these things here personally. The default cookie permission should be setting the read flag on Windows, but not write. And I would prefer to keep rpccookieperms Unix only.
Test ACK ce9df2aba3e98ba7f2a576d587ba8e59bf341083.
Great and appreciated addition.
Performed basic manual testing, receiving all successful/expected results.
.cookie file when no CLI args or config file parameters are provided.-rpccookieperms=440 results in 440 permissions.rpccookieperms=440 results in 440 permissions.nit: Recommend augmenting content in doc/init.md to inform of default cookie file permissions.
Example in this commit https://github.com/tdb3/bitcoin/commit/fdd8a4e70632c74c630caaff5cc36da0c309168e (welcome to cherry pick as desired).
bitcoind --help already states default permissions of 400, so the addition to init.md is author’s discretion.
Knots 26.1 included this (with a few modifications). A user got stuck with a .cookie.tmp file that was read-only, and couldn’t start Knots because the file couldn’t be opened for writing. Which got me thinking should we even be modifying a read-only cookie file, or use it as-is? But here, we’ve been discussing only (or normative) read-only files…
Dealing with a read-only tmp file is probably outside the scope of this PR, but maybe we should reconsider if +rw might actually be the correct mode to use.
Dealing with a read-only tmp file is probably outside the scope of this PR, but maybe we should reconsider if +rw might actually be the correct mode to use. @luke-jr that seems like an unlikely situation (unless bitcoind is killed in the moment between writing the temporary cookie file and renaming it?), nonetheless, it could be addressed by changing where we apply the new more restrictive
400permissions, from the temp file to the renamed.cookie.
I don’t think the temporary cookie file existing with perms 600 for a moment is particularly problematic, so I’ve integrated the change here in 06761eb2686ffe3d7f647d8df9bb7b555bc0072a
I also tested that Bitcoin Core starts up OK like this with the new changes:
0cd $DATADIR
1touch .cookie.tmp
2chmod 600 .cookie.tmp
3./src/bitcoind
Even if the .cookie.tmp file contains old data from a previous start, the default std::ofstream open mode will overwrite the file rather than append, so after renaming the .cookie should always be valid.
My point was more that we should probably not be making cookie files read-only to begin with. So only rw-r--r--, rw-r-----, and rw------- should be supported. Bringing us back to this maybe being best as -rpccookieperms=user/group/all rather than an octal modeset.
And then how to handle if the cookie file already exists read-only… it might make sense to just use it unmodified in that scenario?
OK Luke you’ve persuaded me. I’ve re-worked this now to accept -rpccookieperms=[user|group|others].
nit: Recommend augmenting content in doc/init.md to inform of default cookie file permissions. Example in this commit https://github.com/tdb3/bitcoin/commit/fdd8a4e70632c74c630caaff5cc36da0c309168e (welcome to cherry pick as desired). bitcoind –help already states default permissions of 400, so the addition to init.md is author’s discretion.
Thansk @tdb3, I’ve taken your suggestion in b02736b2fb0ec2ac1ac9d6100e7677348394d306
🚧 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.
34@@ -35,8 +35,10 @@ it will use a special cookie file for authentication. The cookie is generated wi
35 content when the daemon starts, and deleted when it exits. Read access to this file
36 controls who can access it through RPC.
37
38-By default the cookie is stored in the data directory, but it's location can be overridden
39-with the option '-rpccookiefile'.
40+By default the cookie is stored in the data directory, but it's location can be
While in here, maybe fix the typo
0By default the cookie is stored in the data directory, but its location can be
273+ if (!perm_opt) {
274+ LogPrintf("Invalid -rpccookieperms=%s; must be one of 'owner', 'group', or 'others'.\n", *cookie_perms_arg);
275+ return false;
276+ }
277+ cookie_perms = *perm_opt;
278+ LogPrintf("Cookie permissions set to %s.\n", cookie_perms.name);
652@@ -653,6 +653,7 @@ void SetupServerArgs(ArgsManager& argsman)
653 argsman.AddArg("-rpcbind=<addr>[:port]", "Bind to given address to listen for JSON-RPC connections. Do not expose the RPC server to untrusted networks such as the public internet! This option is ignored unless -rpcallowip is also passed. Port is optional and overrides -rpcport. Use [host]:port notation for IPv6. This option can be specified multiple times (default: 127.0.0.1 and ::1 i.e., localhost)", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::RPC);
654 argsman.AddArg("-rpcdoccheck", strprintf("Throw a non-fatal error at runtime if the documentation for an RPC is incorrect (default: %u)", DEFAULT_RPC_DOC_CHECK), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::RPC);
655 argsman.AddArg("-rpccookiefile=<loc>", "Location of the auth cookie. Relative paths will be prefixed by a net-specific datadir location. (default: data dir)", ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
656+ argsman.AddArg("-rpccookieperms=<readable-by>", strprintf("Set permissions on the RPC auth cookie file so that it is readable by [owner|group|others] (default: %u)", DEFAULT_COOKIE_PERMS.name), ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
264+
265+ CookiePerms::Perm cookie_perms{DEFAULT_COOKIE_PERMS};
266+ auto cookie_perms_arg{gArgs.GetArg("-rpccookieperms")};
267+ if (cookie_perms_arg) {
268+#ifdef WIN32
269+ LogPrintf("Unable to set unix-style file permissions on cookie via -rpccookieperms on Windows systems\n");
LogInfo, not the deprecated and confusing alias LogPrintf.
649@@ -650,6 +650,7 @@ void SetupServerArgs(ArgsManager& argsman)
650 argsman.AddArg("-rpcbind=<addr>[:port]", "Bind to given address to listen for JSON-RPC connections. Do not expose the RPC server to untrusted networks such as the public internet! This option is ignored unless -rpcallowip is also passed. Port is optional and overrides -rpcport. Use [host]:port notation for IPv6. This option can be specified multiple times (default: 127.0.0.1 and ::1 i.e., localhost)", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::RPC);
651 argsman.AddArg("-rpcdoccheck", strprintf("Throw a non-fatal error at runtime if the documentation for an RPC is incorrect (default: %u)", DEFAULT_RPC_DOC_CHECK), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::RPC);
652 argsman.AddArg("-rpccookiefile=<loc>", "Location of the auth cookie. Relative paths will be prefixed by a net-specific datadir location. (default: data dir)", ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
653+ argsman.AddArg("-rpccookieperms=<readable-by>", strprintf("Set permissions on the RPC auth cookie file so that it is readable by [owner|group|all] (default: %u)", DEFAULT_COOKIE_PERMS.name), ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
%s
71+ };
72+
73+ static constexpr Perm owner {"owner", fs::perms::owner_read | fs::perms::owner_write};
74+ static constexpr Perm group {"group", owner.permissions | fs::perms::group_read};
75+ static constexpr Perm all {"all", group.permissions | fs::perms::others_read};
76+};
302+ return fs::perms::owner_read | fs::perms::owner_write |
303+ fs::perms::group_read | fs::perms::group_write;
304+ } else if (s == "all") {
305+ return fs::perms::owner_read | fs::perms::owner_write |
306+ fs::perms::group_read | fs::perms::group_write |
307+ fs::perms::others_read | fs::perms::others_write;
Reverted to RO for group and others. I had misunderstood what you meant here:
My point was more that we should probably not be making cookie files read-only to begin with. So only rw-r–r–, rw-r—–, and rw——- should be supported. Bringing us back to this maybe being best as -rpccookieperms=user/group/all rather than an octal modeset.
IMO it doesn’t matter much if users and others get rw or ro, if they can read it, they can access the RPCs.
62@@ -62,6 +63,9 @@ void ReleaseDirectoryLocks();
63 bool TryCreateDirectories(const fs::path& p);
64 fs::path GetDefaultDataDir();
65
66+std::string PermsToString(const fs::perms p);
const has no meaning here, keep it only in the actual function
5@@ -6,6 +6,12 @@
6 #define BITCOIN_HTTPRPC_H
7
8 #include <any>
9+#include <util/fs.h>
10+
11+/** Default permissions for cookie file.
12+ * Set to owner read/write, which on Windows sets the write permission.
13+ */
14+const fs::perms DEFAULT_COOKIE_PERMS{fs::perms::owner_read | fs::perms::owner_write};
In commit “rpc: add default 600 permissions for cookie file” (35df85346438258e1bb5f736e9c4d48cf59e6547)
This commit message is confusing because it sounds like it changing the permissions of the cookie file, but the diff only shows it adding a constant declaration and not changing any code?
It seems like it would be better if this commit were combined with the third commit “init: add option for rpccookie permissions” (ae7ec049e5c8d2361c7f47cad3e8c13f912f1c2a) where the new constant is actually used. It would make both commits easier to understand, IMO
EDIT: Alternately this constant could be dropped if GenerateAuthCookie took a std::optional<fs::perms> parameter instead of fs::perms, and only optionally set permissions instead of always setting them, as suggested in a later comment.
fs::perms is provided in 7ad5349a2ec
254+ auto cookie_perms_arg{gArgs.GetArg("-rpccookieperms")};
255+ if (cookie_perms_arg) {
256+#ifdef WIN32
257+ LogInfo("Unable to set unix-style file permissions on cookie via -rpccookieperms on Windows systems\n");
258+ return false;
259+#else
In commit “init: add option for rpccookie permissions” (ae7ec049e5c8d2361c7f47cad3e8c13f912f1c2a)
It would be nice to drop this ifdef special case for windows. Even though windows uses acls, the fs::permissions call should still be able to do the requested things by setting acls for the “Users” and “Everyone” groups on windows. I don’t think we need to go out of our way to second guess the library and operating system here. If the operating system or filesystem do not support permissions, or have a problem with the way they are specified, the fs::permissions call can fail with a more specific error message, so there should be no need for this special case.
80@@ -82,33 +81,38 @@ static fs::path GetAuthCookieFile(bool temp=false)
81
82 static bool g_generated_cookie = false;
83
84-bool GenerateAuthCookie(std::string *cookie_out)
85+bool GenerateAuthCookie(std::string* cookie_out, fs::perms cookie_perms)
In commit “init: add option for rpccookie permissions” (ae7ec049e5c8d2361c7f47cad3e8c13f912f1c2a)
I think it would be good to change fs::perms parameter to std::optional<fs::perms> and not call fs::permissions() unless -rpccookieperms option is specified, for backwards compatibility.
It is easy to imagine a new fs::permissions(DEFAULT_COOKIE_PERMS) call having bad side effects on platforms like windows that use acls, or maybe failing on filesystems that do not support permissions. The previous default behavior of using the the 0077 umask was already functionally the same as making this call, so I don’t see a reason to add the call when -rpccookieperms is not specified.
Code review ACK 9617e42a7b12a05ed8a0815c5a6537e1614727f5. I didn’t follow all the previous discussion, but I like the place this PR ended up accepting simple “owner” “group” and “all” settings and avoiding more complicated corner cases.
I left some suggestions which are mostly not important, though I do think it would be better to try to stay more backwards compatible and avoid setting permissions when it is not requested.
113+ conf = self.nodes[1].bitcoinconf
114+ with conf.open('r') as file:
115+ lines = file.readlines()
116+ filtered_lines = [line for line in lines if not line.startswith('rpcuser') and not line.startswith('rpcpassword')]
117+ with conf.open('w') as file:
118+ file.writelines(filtered_lines)
Unless I’m missing something, this is functionally simlar to TestNode.replace_in_config(). Using replace_in_config() instead could make things shorter by reusing code. For example:
0diff --git a/test/functional/rpc_users.py b/test/functional/rpc_users.py
1index 55cba41754..f65b13a440 100755
2--- a/test/functional/rpc_users.py
3+++ b/test/functional/rpc_users.py
4@@ -110,12 +110,7 @@ class HTTPBasicsTest(BitcoinTestFramework):
5 assert_equal(expected_perms, actual_perms)
6
7 # Remove any leftover rpc{user|password} config options from previous tests
8- conf = self.nodes[1].bitcoinconf
9- with conf.open('r') as file:
10- lines = file.readlines()
11- filtered_lines = [line for line in lines if not line.startswith('rpcuser') and not line.startswith('rpcpassword')]
12- with conf.open('w') as file:
13- file.writelines(filtered_lines)
14+ self.nodes[1].replace_in_config([("rpcuser", "#rpcuser"), ("rpcpassword", "#rpcpassword")])
15
16 self.log.info('Check default cookie permission')
17 test_perm(None)
Tried this out locally and it seemed to work well.
Approach ACK. This is looking great.
Left a comment in test_rpccookieperms().
Also manually checked cookie file permissions after starting bitcoind without -rpccookieperms and with each option (owner, group, all) respectively. Permissions were as expected:
0-rw------- 1 dev dev 75 Jun 7 21:59 .cookie
0-rw------- 1 dev dev 75 Jun 7 21:59 .cookie
0-rw-r----- 1 dev dev 75 Jun 7 22:00 .cookie
0-rw-r--r-- 1 dev dev 75 Jun 7 22:00 .cookie
107 if (!RenameOver(filepath_tmp, filepath)) {
108- LogPrintf("Unable to rename cookie authentication file %s to %s\n", fs::PathToString(filepath_tmp), fs::PathToString(filepath));
109+ LogInfo("Unable to rename cookie authentication file %s to %s\n", fs::PathToString(filepath_tmp), fs::PathToString(filepath));
110 return false;
111 }
112+ std::error_code code;
In commit “init: add option for rpccookie permissions” (7ad5349a2ec04399c99c8cbf71c8d3b4627132f8)
Could move this variable inside the if statement since it’s not used afterwards.
18 std::string JSONRPCReply(const UniValue& result, const UniValue& error, const UniValue& id);
19 UniValue JSONRPCError(int code, const std::string& message);
20
21 /** Generate a new RPC authentication cookie and write it to disk */
22-bool GenerateAuthCookie(std::string *cookie_out);
23+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.
0diff --git a/src/rpc/request.h b/src/rpc/request.h
1index b40df16631..c7e723d962 100644
2--- a/src/rpc/request.h
3+++ b/src/rpc/request.h
4@@ -19,7 +19,7 @@ std::string JSONRPCReply(const UniValue& result, const UniValue& error, const Un
5 UniValue JSONRPCError(int code, const std::string& message);
6
7 /** Generate a new RPC authentication cookie and write it to disk */
8-bool GenerateAuthCookie(std::string* cookie_out, std::optional<fs::perms> cookie_perms);
9+bool GenerateAuthCookie(std::string* cookie_out, std::optional<fs::perms> cookie_perms=std::nullopt);
10 /** Read the RPC authentication cookie from disk */
11 bool GetAuthCookie(std::string *cookie_out);
12 /** Delete RPC authentication cookie from disk */
GenerateAuthCookie() is currently used with the argument provided, so not a big deal either way.
85@@ -84,6 +86,39 @@ def test_auth(self, node, user, password):
86 self.log.info('Wrong...')
87 assert_equal(401, call_with_auth(node, user + 'wrong', password + 'wrong').status)
88
89+ def test_rpccookieperms(self):
90+ p = {"owner": 0o600, "group": 0o640, "all": 0o644}
91+
92+ if os.name == 'nt':
85@@ -84,6 +86,39 @@ def test_auth(self, node, user, password):
86 self.log.info('Wrong...')
87 assert_equal(401, call_with_auth(node, user + 'wrong', password + 'wrong').status)
88
89+ def test_rpccookieperms(self):
90+ p = {"owner": 0o600, "group": 0o640, "all": 0o644}
91+
92+ if os.name == 'nt':
93+ raise SkipTest(f"Skip cookie file permissions checks as OS detected as: {os.name=}")
In 31cc8bc305103baf2c9979d86ef1b43dec5628cb “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.
62@@ -62,6 +63,9 @@ void ReleaseDirectoryLocks();
63 bool TryCreateDirectories(const fs::path& p);
64 fs::path GetDefaultDataDir();
65
66+std::string PermsToString(fs::perms p);
67+std::optional<fs::perms> StringToPerms(const std::string& s);
In 38af55e285336cdd3f42635938f24622451703b0 “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.
88 unsigned char rand_pwd[COOKIE_SIZE];
89 GetRandBytes(rand_pwd);
90 std::string cookie = COOKIEAUTH_USER + ":" + HexStr(rand_pwd);
91
92- /** the umask determines what permissions are used to create this file -
93- * these are set to 0077 in common/system.cpp.
In 7ad5349a2ec04399c99c8cbf71c8d3b4627132f8 “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 (review)
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
owneras 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 (review). 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.
72+/** Interpret a custom permissions level string as fs::perms
73+ *
74+ * @param[in] s Permission level string
75+ * @return Permissions as fs::perms
76+ */
77+std::optional<fs::perms> InterpretPermSting(const std::string& s);
nit: spelling
0- InterpretPermSting
1+ InterpretPermString
re ACK 4f6b00c94b59c8206c77647c97f4ed188fa99401
Ran rpc_users locally (passed).
Re-ran the manual check in #28167#pullrequestreview-2105557076 (same results).
Also sanity checked on a Windows machine running Python 3.9.13 that platform.system() does return “Windows”.
Left one minor nit.
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>
🚧 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.
Nice! Will try this out soon.
Update 2024-07-11: works like a charm and allowed me to drop some ugliness from systemd config.