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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
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.
<!--174a7506f384e20aa4161008e828411d-->
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;
Not sure StringToOctal is the right place for this check.
Isn't the leading 0 optional? (Also the user might want to setgid or something weird)
Seperated sanitzing checks and str->octal converstion in b4b0d2adc9
b4b0d2adc9 now handles optional special mode bit
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);
I don't think we need to prepend the zero here.
Ok removed the 0 padding in 1b0fa12fa5
Are you still working on this?
Updated to address luke's comment and added a test to ensure rpccookieperms are being applied
Rebased now that #28905 is merged.
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';
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)
Should the permissions that can be set limited here? There is no reason to ever set +x on an RPC cookie file.
I'm not sure there's a reason to ever set +w either... maybe it should just be -rpccookieaccess user/group/all or something
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.
OK I added a commit to change default cookie perms to 400, and rebased on master to try and get the unrelated CI failure to go away.
Approach ACK; seems like a reasonable feature reasonably implemented. Will do a more formal review soon.
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};
I dislike using auto both here and the default. It could conceivably not be a type that supports all permission masks.
Changed to 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));
Feels weird to log this unconditionally. Maybe the option should just be unavailable (and implicitly error) on Windows, and hard-code setting it user-read-only here?
The cookie should be read-only on Windows by deafult, so I've moved the logging up into 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
Missing a space in the indentation
Thanks! My formatter is having some difficulty with diff/range-based formatting at the moment. Fixed.
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
for perm in "440", "640", "444":
test_perm(perm)
IMO list comprehensions are fine, but seeing as I voided half the point of using one (creating a one-liner), so took the change to a regular for loop in 78f59b4a26b19011d53d9330454f383b47c063eb
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 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.
<sub>Debug: https://github.com/bitcoin/bitcoin/runs/20852902254</sub>
Actually, why wouldn't fs::permissions work on Windows?
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:
cd $DATADIR
touch .cookie.tmp
chmod 600 .cookie.tmp
./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
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 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.
<sub>Debug: https://github.com/bitcoin/bitcoin/runs/24098949757</sub>
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
By default the cookie is stored in the data directory, but its location can be
done
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);
We already print settings somewhere else, and this isn't where the actual permissions are being changed.
Removed from this location
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);
nit: "all" seems better than "others" here IMO
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");
nit: For new code it would be good to use LogInfo, not the deprecated and confusing alias LogPrintf.
Thanks, taken here and elsewhere it was being used.
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
Fixed in 5a20b078c3a418103601ee6eb88e5dba8e6d9863
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 | +};
This feels overengineered. Why can't we stick to fs::perms internally and just map the string in a function?
Now simplified in d8d47ab395b8fa341e0aac2fe01a4b9488af5347
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;
IMO it was better to leave group/others as read-only, no write access
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
QA failed
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.
Dropped the new constant leaving the current behaviour as default, unless an optional 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.
Thanks, now done in 7ad5349a2ec
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.
Taken in 7ad5349a2ec
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:
diff --git a/test/functional/rpc_users.py b/test/functional/rpc_users.py
index 55cba41754..f65b13a440 100755
--- a/test/functional/rpc_users.py
+++ b/test/functional/rpc_users.py
@@ -110,12 +110,7 @@ class HTTPBasicsTest(BitcoinTestFramework):
assert_equal(expected_perms, actual_perms)
# Remove any leftover rpc{user|password} config options from previous tests
- conf = self.nodes[1].bitcoinconf
- with conf.open('r') as file:
- lines = file.readlines()
- filtered_lines = [line for line in lines if not line.startswith('rpcuser') and not line.startswith('rpcpassword')]
- with conf.open('w') as file:
- file.writelines(filtered_lines)
+ self.nodes[1].replace_in_config([("rpcuser", "#rpcuser"), ("rpcpassword", "#rpcpassword")])
self.log.info('Check default cookie permission')
test_perm(None)
Tried this out locally and it seemed to work well.
Thanks, i took this simplification in 7ad5349a2ec
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:
-rw------- 1 dev dev 75 Jun 7 21:59 .cookie
-rw------- 1 dev dev 75 Jun 7 21:59 .cookie
-rw-r----- 1 dev dev 75 Jun 7 22:00 .cookie
-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.
Moved in 9eff5601059
Code review ACK e4798038c00e787023b9dedc907966a08592d79f. Seems very clean now with the test simplification and without the win32 stuff.
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.
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 49d5bfdd7e7
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':
Thanks, updated in 49d5bfdd7e7
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.
Good catch. Might also be good to print a log message before returning for awareness.
Oh no! fixed in 49d5bfdd7e7
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.
I agree on reflection, gave them new names in 49d5bfdd7e7
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.
I've un-removed the comment in 9eff5601059, 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.
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
- InterpretPermSting
+ InterpretPermString
fixed
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>
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 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.
<sub>Debug: https://github.com/bitcoin/bitcoin/runs/26763232631</sub>
ACK 73f0a6cbd0b628675028fbd5a37eff8115e7ccfe
Code review ACK 73f0a6cbd0b628675028fbd5a37eff8115e7ccfe. 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.
cr ACK 73f0a6cbd0b628675028fbd5a37eff8115e7ccfe
Nice! Will try this out soon.
Update 2024-07-11: works like a charm and allowed me to drop some ugliness from systemd config.