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

pull willcl-ark wants to merge 4 commits into bitcoin:master from willcl-ark:2023-07-rpccookie-perms changing 8 files +133 −11
  1. willcl-ark commented at 9:52 am on July 27, 2023: member

    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.

  2. DrahtBot commented at 9:52 am on July 27, 2023: contributor

    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.

  3. in src/httprpc.cpp:244 in a8e92784ba outdated
    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;
    


    luke-jr commented at 8:13 pm on August 5, 2023:
    Not sure StringToOctal is the right place for this check.

    luke-jr commented at 8:14 pm on August 5, 2023:
    Isn’t the leading 0 optional? (Also the user might want to setgid or something weird)

    willcl-ark commented at 1:30 pm on August 9, 2023:
    Seperated sanitzing checks and str->octal converstion in b4b0d2adc9

    willcl-ark commented at 1:31 pm on August 9, 2023:
    b4b0d2adc9 now handles optional special mode bit
  4. luke-jr changes_requested
  5. willcl-ark force-pushed on Aug 9, 2023
  6. DrahtBot added the label CI failed on Aug 9, 2023
  7. in src/httprpc.cpp:255 in b4b0d2adc9 outdated
    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);
    


    luke-jr commented at 6:12 pm on August 10, 2023:
    I don’t think we need to prepend the zero here.

    willcl-ark commented at 2:24 pm on November 20, 2023:
    Ok removed the 0 padding in 1b0fa12fa5
  8. DrahtBot removed the label CI failed on Aug 18, 2023
  9. DrahtBot added the label CI failed on Oct 25, 2023
  10. DrahtBot commented at 7:20 pm on October 25, 2023: contributor
    Are you still working on this?
  11. willcl-ark force-pushed on Nov 20, 2023
  12. willcl-ark force-pushed on Nov 20, 2023
  13. willcl-ark commented at 2:24 pm on November 20, 2023: member
    Updated to address luke’s comment and added a test to ensure rpccookieperms are being applied
  14. willcl-ark force-pushed on Nov 21, 2023
  15. willcl-ark force-pushed on Nov 21, 2023
  16. willcl-ark force-pushed on Nov 21, 2023
  17. willcl-ark force-pushed on Nov 21, 2023
  18. DrahtBot removed the label CI failed on Nov 21, 2023
  19. willcl-ark force-pushed on Nov 21, 2023
  20. willcl-ark commented at 11:36 am on November 21, 2023: member
    Rebased now that #28905 is merged.
  21. DrahtBot added the label Needs rebase on Dec 1, 2023
  22. willcl-ark force-pushed on Dec 7, 2023
  23. DrahtBot removed the label Needs rebase on Dec 7, 2023
  24. in src/rpc/request.cpp:90 in f15f4b3c4b outdated
    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';
    


    luke-jr commented at 1:29 am on December 29, 2023:
    This won’t work if multiple bits are set

    luke-jr commented at 1:31 am on December 29, 2023:
    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)
  25. kristapsk commented at 9:30 am on December 29, 2023: contributor
    Should the permissions that can be set limited here? There is no reason to ever set +x on an RPC cookie file.
  26. luke-jr commented at 11:50 pm on December 30, 2023: member
    I’m not sure there’s a reason to ever set +w either… maybe it should just be -rpccookieaccess user/group/all or something
  27. kristapsk commented at 1:10 am on December 31, 2023: contributor

    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.

  28. willcl-ark force-pushed on Jan 8, 2024
  29. willcl-ark commented at 12:17 pm on January 8, 2024: member

    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.

  30. kristapsk commented at 1:37 pm on January 8, 2024: contributor

    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.

    Only three permissions that makes sense IMO are 600, 640 and 644.

  31. willcl-ark commented at 1:50 pm on January 8, 2024: member

    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.

    Only three permissions that makes sense IMO are 600, 640 and 644.

    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…

  32. kristapsk commented at 1:52 pm on January 8, 2024: contributor

    I don’t think enabling write perms on a cookie (ever?!) that bitcoind has created makes sense?

    Ahh, yes, 400, 440, 444.

  33. willcl-ark force-pushed on Jan 9, 2024
  34. willcl-ark force-pushed on Jan 9, 2024
  35. DrahtBot added the label CI failed on Jan 12, 2024
  36. willcl-ark force-pushed on Jan 15, 2024
  37. willcl-ark commented at 1:21 pm on January 15, 2024: member
    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.
  38. DrahtBot removed the label CI failed on Jan 15, 2024
  39. jamesob commented at 4:57 pm on January 16, 2024: member
    Approach ACK; seems like a reasonable feature reasonably implemented. Will do a more formal review soon.
  40. willcl-ark added the label RPC/REST/ZMQ on Jan 24, 2024
  41. in src/httprpc.cpp:266 in d38e54425f outdated
    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};
    


    luke-jr commented at 4:08 am on January 25, 2024:
    I dislike using auto both here and the default. It could conceivably not be a type that supports all permission masks.

    willcl-ark commented at 9:48 am on January 25, 2024:
    Changed to fs::perms here and the default in e838d7dcc6c835dcb936dd3b691842aaa43f22ad
  42. in src/rpc/request.cpp:101 in d38e54425f outdated
    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));
    


    luke-jr commented at 4:10 am on January 25, 2024:
    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?

    willcl-ark commented at 9:50 am on January 25, 2024:
    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.

    luke-jr commented at 5:07 pm on January 25, 2024:

    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).

  43. in test/functional/rpc_users.py:115 in d38e54425f outdated
    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
    


    luke-jr commented at 4:11 am on January 25, 2024:
    Missing a space in the indentation

    willcl-ark commented at 9:50 am on January 25, 2024:
    Thanks! My formatter is having some difficulty with diff/range-based formatting at the moment. Fixed.
  44. in test/functional/rpc_users.py:128 in d38e54425f outdated
    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]
    


    luke-jr commented at 4:12 am on January 25, 2024:

    Feels like a hack

    0        for perm in "440", "640", "444":
    1            test_perm(perm)
    

    willcl-ark commented at 9:52 am on January 25, 2024:
    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
  45. willcl-ark force-pushed on Jan 25, 2024
  46. willcl-ark force-pushed on Jan 25, 2024
  47. DrahtBot added the label CI failed on Jan 25, 2024
  48. DrahtBot commented at 9:53 am on January 25, 2024: contributor

    🚧 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/20852902254

  49. DrahtBot removed the label CI failed on Jan 25, 2024
  50. luke-jr commented at 6:11 pm on January 25, 2024: member
    Actually, why wouldn’t fs::permissions work on Windows?
  51. willcl-ark force-pushed on Jan 26, 2024
  52. willcl-ark force-pushed on Jan 26, 2024
  53. luke-jr referenced this in commit 78458d2bce on Jan 26, 2024
  54. luke-jr referenced this in commit e48a51febf on Jan 26, 2024
  55. luke-jr referenced this in commit 1bf30db0f2 on Jan 26, 2024
  56. luke-jr referenced this in commit a257ebd621 on Jan 26, 2024
  57. luke-jr commented at 4:28 pm on January 26, 2024: member

    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.

  58. willcl-ark force-pushed on Feb 5, 2024
  59. willcl-ark force-pushed on Feb 5, 2024
  60. willcl-ark commented at 11:30 am on February 5, 2024: member

    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.

  61. DrahtBot added the label CI failed on Feb 5, 2024
  62. willcl-ark force-pushed on Feb 5, 2024
  63. DrahtBot removed the label CI failed on Feb 5, 2024
  64. tdb3 commented at 2:14 am on February 22, 2024: contributor

    Test ACK ce9df2aba3e98ba7f2a576d587ba8e59bf341083.

    Great and appreciated addition.

    Performed basic manual testing, receiving all successful/expected results.

    1. Confirmed default of 400 perms for the .cookie file when no CLI args or config file parameters are provided.
    2. Confirmed CLI arg of -rpccookieperms=440 results in 440 permissions.
    3. Confirmed bitcoin.conf with rpccookieperms=440 results in 440 permissions.
    4. Confirmed empty, non-number, and > 3 octal number is detected and error provided (expected).

    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.

  65. DrahtBot requested review from jamesob on Feb 22, 2024
  66. Retropex referenced this in commit d15bcf38ff on Mar 28, 2024
  67. Retropex referenced this in commit 56d8725c4a on Mar 28, 2024
  68. Retropex referenced this in commit a428f71348 on Mar 28, 2024
  69. Retropex referenced this in commit c3bf4ef27d on Mar 28, 2024
  70. luke-jr commented at 4:33 pm on March 29, 2024: member

    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.

  71. willcl-ark force-pushed on Apr 3, 2024
  72. willcl-ark commented at 10:28 am on April 3, 2024: member

    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 400 permissions, 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.

  73. DrahtBot added the label CI failed on Apr 3, 2024
  74. DrahtBot removed the label CI failed on Apr 5, 2024
  75. luke-jr commented at 10:59 pm on April 21, 2024: member

    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?

  76. willcl-ark force-pushed on Apr 22, 2024
  77. willcl-ark commented at 11:30 am on April 22, 2024: member

    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

  78. willcl-ark force-pushed on Apr 22, 2024
  79. DrahtBot added the label CI failed on Apr 22, 2024
  80. DrahtBot commented at 12:12 pm on April 22, 2024: contributor

    🚧 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/24098949757

  81. willcl-ark force-pushed on Apr 22, 2024
  82. in doc/init.md:38 in 805577312c outdated
    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
    


    luke-jr commented at 12:58 pm on April 22, 2024:

    While in here, maybe fix the typo

    0By default the cookie is stored in the data directory, but its location can be
    

    willcl-ark commented at 8:31 am on May 20, 2024:
    done
  83. in src/httprpc.cpp:273 in 805577312c outdated
    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);
    


    luke-jr commented at 1:02 pm on April 22, 2024:
    We already print settings somewhere else, and this isn’t where the actual permissions are being changed.

    willcl-ark commented at 8:32 am on May 20, 2024:
    Removed from this location
  84. in src/init.cpp:653 in 805577312c outdated
    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);
    


    luke-jr commented at 1:03 pm on April 22, 2024:
    nit: “all” seems better than “others” here IMO
  85. DrahtBot removed the label CI failed on Apr 22, 2024
  86. DrahtBot added the label Needs rebase on May 7, 2024
  87. willcl-ark force-pushed on May 7, 2024
  88. DrahtBot removed the label Needs rebase on May 7, 2024
  89. in src/httprpc.cpp:264 in 740f343859 outdated
    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");
    


    maflcko commented at 3:31 pm on May 17, 2024:
    nit: For new code it would be good to use LogInfo, not the deprecated and confusing alias LogPrintf.

    willcl-ark commented at 8:32 am on May 20, 2024:
    Thanks, taken here and elsewhere it was being used.
  90. willcl-ark force-pushed on May 20, 2024
  91. in src/init.cpp:653 in fba815a4e6 outdated
    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);
    


    luke-jr commented at 5:41 pm on May 20, 2024:
    %s

    willcl-ark commented at 3:02 pm on May 21, 2024:
    Fixed in 5a20b078c3a418103601ee6eb88e5dba8e6d9863
  92. in src/util/fs_helpers.h:76 in fba815a4e6 outdated
    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+};
    


    luke-jr commented at 6:39 pm on May 20, 2024:
    This feels overengineered. Why can’t we stick to fs::perms internally and just map the string in a function?

    willcl-ark commented at 3:01 pm on May 21, 2024:
    Now simplified in d8d47ab395b8fa341e0aac2fe01a4b9488af5347
  93. willcl-ark force-pushed on May 21, 2024
  94. willcl-ark force-pushed on May 21, 2024
  95. in src/util/fs_helpers.cpp:307 in 02e31263f3 outdated
    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;
    


    luke-jr commented at 3:50 pm on May 21, 2024:
    IMO it was better to leave group/others as read-only, no write access

    willcl-ark commented at 9:20 am on May 22, 2024:

    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.

  96. in src/util/fs_helpers.h:66 in 02e31263f3 outdated
    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);
    


    luke-jr commented at 4:38 pm on May 21, 2024:
    const has no meaning here, keep it only in the actual function
  97. willcl-ark force-pushed on May 22, 2024
  98. DrahtBot added the label CI failed on May 22, 2024
  99. luke-jr commented at 1:29 am on May 23, 2024: member
    QA failed
  100. willcl-ark force-pushed on May 24, 2024
  101. DrahtBot removed the label CI failed on May 24, 2024
  102. in src/httprpc.h:14 in 35df853464 outdated
     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};
    


    ryanofsky commented at 3:28 pm on June 5, 2024:

    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.


    willcl-ark commented at 12:37 pm on June 24, 2024:
    Dropped the new constant leaving the current behaviour as default, unless an optional fs::perms is provided in 7ad5349a2ec
  103. in src/httprpc.cpp:257 in ae7ec049e5 outdated
    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
    


    ryanofsky commented at 4:48 pm on June 5, 2024:

    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.


    willcl-ark commented at 12:36 pm on June 24, 2024:
    Thanks, now done in 7ad5349a2ec
  104. in src/rpc/request.cpp:84 in ae7ec049e5 outdated
    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)
    


    ryanofsky commented at 4:49 pm on June 5, 2024:

    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.


    willcl-ark commented at 12:37 pm on June 24, 2024:
    Taken in 7ad5349a2ec
  105. ryanofsky approved
  106. ryanofsky commented at 5:14 pm on June 5, 2024: contributor

    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.

  107. DrahtBot requested review from tdb3 on Jun 5, 2024
  108. in test/functional/rpc_users.py:118 in ab3245f335 outdated
    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)
    


    tdb3 commented at 1:56 am on June 8, 2024:

    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.


    willcl-ark commented at 12:38 pm on June 24, 2024:
    Thanks, i took this simplification in 7ad5349a2ec
  109. tdb3 commented at 2:08 am on June 8, 2024: contributor

    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
    
  110. luke-jr referenced this in commit 0b30e67701 on Jun 13, 2024
  111. luke-jr referenced this in commit 8ad18ef583 on Jun 13, 2024
  112. luke-jr referenced this in commit 3c65404ecb on Jun 13, 2024
  113. luke-jr referenced this in commit ba25becb04 on Jun 13, 2024
  114. luke-jr referenced this in commit a1b1d3348b on Jun 13, 2024
  115. luke-jr referenced this in commit a6d297ae7b on Jun 13, 2024
  116. willcl-ark force-pushed on Jun 24, 2024
  117. in src/rpc/request.cpp:106 in 7ad5349a2e outdated
    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;
    


    ryanofsky commented at 11:00 pm on June 24, 2024:

    In commit “init: add option for rpccookie permissions” (7ad5349a2ec04399c99c8cbf71c8d3b4627132f8)

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


    willcl-ark commented at 9:14 am on June 27, 2024:
    Moved in 9eff5601059
  118. ryanofsky approved
  119. ryanofsky commented at 11:07 pm on June 24, 2024: contributor
    Code review ACK e4798038c00e787023b9dedc907966a08592d79f. Seems very clean now with the test simplification and without the win32 stuff.
  120. DrahtBot requested review from tdb3 on Jun 24, 2024
  121. in src/rpc/request.h:22 in 7ad5349a2e outdated
    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);
    


    tdb3 commented at 1:02 am on June 25, 2024:

    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.


    willcl-ark commented at 9:14 am on June 27, 2024:
    Added in 49d5bfdd7e7
  122. tdb3 approved
  123. tdb3 commented at 1:15 am on June 25, 2024: contributor

    re ACK e4798038c00e787023b9dedc907966a08592d79f

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

  124. in test/functional/rpc_users.py:92 in 31cc8bc305 outdated
    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':
    


    achow101 commented at 5:35 pm on June 26, 2024:

    In 31cc8bc305103baf2c9979d86ef1b43dec5628cb “test: add rpccookieperms test”

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


    willcl-ark commented at 9:14 am on June 27, 2024:
    Thanks, updated in 49d5bfdd7e7
  125. in test/functional/rpc_users.py:93 in 31cc8bc305 outdated
    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=}")
    


    achow101 commented at 5:41 pm on June 26, 2024:

    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.


    tdb3 commented at 2:48 am on June 27, 2024:
    Good catch. Might also be good to print a log message before returning for awareness.

    willcl-ark commented at 9:14 am on June 27, 2024:
    Oh no! fixed in 49d5bfdd7e7
  126. in src/util/fs_helpers.h:67 in 38af55e285 outdated
    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);
    


    achow101 commented at 5:43 pm on June 26, 2024:

    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.


    willcl-ark commented at 9:14 am on June 27, 2024:
    I agree on reflection, gave them new names in 49d5bfdd7e7
  127. in src/rpc/request.cpp:93 in 7ad5349a2e outdated
    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.
    


    achow101 commented at 5:45 pm on June 26, 2024:

    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.


    ryanofsky commented at 7:13 pm on June 26, 2024:

    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 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 (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.


    willcl-ark commented at 9:15 am on June 27, 2024:
    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.
  128. willcl-ark force-pushed on Jun 27, 2024
  129. in src/util/fs_helpers.h:77 in 4f6b00c94b outdated
    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);
    


    tdb3 commented at 1:21 pm on June 27, 2024:

    nit: spelling

    0- InterpretPermSting
    1+ InterpretPermString
    

    willcl-ark commented at 1:55 pm on June 27, 2024:
    fixed
  130. tdb3 approved
  131. tdb3 commented at 1:33 pm on June 27, 2024: contributor

    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.

  132. DrahtBot requested review from ryanofsky on Jun 27, 2024
  133. util: add perm string helper functions
    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.
    7df03f1a92
  134. willcl-ark force-pushed on Jun 27, 2024
  135. init: add option for rpccookie permissions
    Add a bitcoind launch option `-rpccookieperms` to configure the file
    permissions of the cookie on Unix systems.
    f467aede78
  136. test: add rpccookieperms test
    Tests various perms on non-Windows OSes
    d2afa2690c
  137. doc: detail -rpccookieperms option
    Co-authored-by: tdb3 <106488469+tdb3@users.noreply.github.com>
    73f0a6cbd0
  138. willcl-ark force-pushed on Jun 27, 2024
  139. DrahtBot commented at 2:16 pm on June 27, 2024: contributor

    🚧 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

  140. DrahtBot added the label CI failed on Jun 27, 2024
  141. DrahtBot removed the label CI failed on Jun 27, 2024
  142. achow101 commented at 6:20 pm on June 27, 2024: member
    ACK 73f0a6cbd0b628675028fbd5a37eff8115e7ccfe
  143. DrahtBot requested review from tdb3 on Jun 27, 2024
  144. ryanofsky approved
  145. ryanofsky commented at 8:46 pm on June 27, 2024: contributor
    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.
  146. tdb3 approved
  147. tdb3 commented at 9:32 pm on June 27, 2024: contributor
    cr ACK 73f0a6cbd0b628675028fbd5a37eff8115e7ccfe
  148. ryanofsky merged this on Jun 27, 2024
  149. ryanofsky closed this on Jun 27, 2024

  150. Sjors commented at 10:33 am on July 2, 2024: member

    Nice! Will try this out soon.

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


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-23 15:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me