From a888155d595c57dc52f15d7b4eedf890206ec98f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A8le=20Oul=C3=A8s?= Date: Mon, 8 Jan 2024 15:02:44 +0000 Subject: [PATCH 1/6] httprpc: allow specifying rpccookie permissions in octal Co-authored-by: Will Clark Github-Pull: #28167 Rebased-From: 7456c3a5067b03972ab6159030944ee5ae81e21a --- src/util/fs_helpers.cpp | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/util/fs_helpers.cpp b/src/util/fs_helpers.cpp index 41c8fe3b8f..71322a0548 100644 --- a/src/util/fs_helpers.cpp +++ b/src/util/fs_helpers.cpp @@ -294,6 +294,23 @@ std::string PermsToSymbolicString(fs::perms p) return perm_str; } +static std::optional StringToOctal(const std::string& str) +{ + unsigned ret = 0; + for (char c : str) { + if (c < '0' || c > '7') return std::nullopt; + ret = (ret << 3) | (c - '0'); + } + return ret; +} + +static auto ConvertPermsToOctal(const std::string& str) noexcept -> std::optional +{ + // Don't permit setting special bits as they're not relevant to cookie files + if (str.length() == 3) return StringToOctal(str); + return std::nullopt; +} + std::optional InterpretPermString(const std::string& s) { if (s == "owner") { @@ -305,6 +322,8 @@ std::optional InterpretPermString(const std::string& s) return fs::perms::owner_read | fs::perms::owner_write | fs::perms::group_read | fs::perms::others_read; + } else if (auto octal_perms = ConvertPermsToOctal(s)) { + return static_cast(*octal_perms); } else { return std::nullopt; } From 0a4e77a361bf028d41e8f6080b41c021f5353541 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Thu, 16 May 2024 20:02:44 +0000 Subject: [PATCH 2/6] QA: rpc_users: Extend rpccookieperms test to octal values --- test/functional/rpc_users.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/test/functional/rpc_users.py b/test/functional/rpc_users.py index 96cd9b793c..950fb2d529 100755 --- a/test/functional/rpc_users.py +++ b/test/functional/rpc_users.py @@ -88,7 +88,14 @@ class HTTPBasicsTest(BitcoinTestFramework): assert_equal(401, call_with_auth(node, user + 'wrong', password + 'wrong').status) def test_rpccookieperms(self): - p = {"owner": 0o600, "group": 0o640, "all": 0o644} + p = { + "owner": 0o600, + "group": 0o640, + "all": 0o644, + "440": 0o440, + "640": 0o640, + "444": 0o444, + } if platform.system() == 'Windows': self.log.info(f"Skip cookie file permissions checks as OS detected as: {platform.system()=}") @@ -118,7 +125,7 @@ class HTTPBasicsTest(BitcoinTestFramework): test_perm(None) self.log.info('Check custom cookie permissions') - for perm in ["owner", "group", "all"]: + for perm in p.keys(): test_perm(perm) def test_norpccookiefile(self, node0_cookie_path): From d307da8781c5bd1113287991ce89107f93c12a66 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Thu, 25 Jan 2024 17:36:26 +0000 Subject: [PATCH 3/6] rpccookieperms: Allow setting setxid/sticky bits Simply for backward compatibility --- src/util/fs_helpers.cpp | 3 +-- test/functional/rpc_users.py | 5 +++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/util/fs_helpers.cpp b/src/util/fs_helpers.cpp index 71322a0548..6f3bd5800e 100644 --- a/src/util/fs_helpers.cpp +++ b/src/util/fs_helpers.cpp @@ -306,8 +306,7 @@ static std::optional StringToOctal(const std::string& str) static auto ConvertPermsToOctal(const std::string& str) noexcept -> std::optional { - // Don't permit setting special bits as they're not relevant to cookie files - if (str.length() == 3) return StringToOctal(str); + if ((str.length() == 3) || (str.length() == 4)) return StringToOctal(str); return std::nullopt; } diff --git a/test/functional/rpc_users.py b/test/functional/rpc_users.py index 950fb2d529..46ca8866c7 100755 --- a/test/functional/rpc_users.py +++ b/test/functional/rpc_users.py @@ -93,8 +93,9 @@ class HTTPBasicsTest(BitcoinTestFramework): "group": 0o640, "all": 0o644, "440": 0o440, - "640": 0o640, + "0640": 0o640, "444": 0o444, + "1660": 0o1660, } if platform.system() == 'Windows': @@ -104,7 +105,7 @@ class HTTPBasicsTest(BitcoinTestFramework): self.log.info('Check cookie file permissions can be set using -rpccookieperms') cookie_file_path = self.nodes[1].chain_path / '.cookie' - PERM_BITS_UMASK = 0o777 + PERM_BITS_UMASK = 0o7777 def test_perm(perm: Optional[str]): if not perm: From 051c276b67b35b73298a408258dbfa9515a19815 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Thu, 25 Jan 2024 19:50:26 +0000 Subject: [PATCH 4/6] Skip changing permissions entirely if -rpccookieperms=0 specified --- src/httprpc.cpp | 16 +++++++++++----- test/functional/rpc_users.py | 23 ++++++++++++++++++++++- 2 files changed, 33 insertions(+), 6 deletions(-) diff --git a/src/httprpc.cpp b/src/httprpc.cpp index c4297b8280..b84b105b4f 100644 --- a/src/httprpc.cpp +++ b/src/httprpc.cpp @@ -296,12 +296,18 @@ static bool InitRPCAuthentication() std::optional cookie_perms{std::nullopt}; auto cookie_perms_arg{gArgs.GetArg("-rpccookieperms")}; if (cookie_perms_arg) { - auto perm_opt = InterpretPermString(*cookie_perms_arg); - if (!perm_opt) { - LogError("Invalid -rpccookieperms=%s; must be one of 'owner', 'group', or 'all'.", *cookie_perms_arg); - return false; + if (*cookie_perms_arg == "0") { + cookie_perms = std::nullopt; + } else if (cookie_perms_arg->empty() || *cookie_perms_arg == "1") { + // leave at default + } else { + auto perm_opt = InterpretPermString(*cookie_perms_arg); + if (!perm_opt) { + LogError("Invalid -rpccookieperms=%s; must be one of 'owner', 'group', or 'all'.", *cookie_perms_arg); + return false; + } + cookie_perms = *perm_opt; } - cookie_perms = *perm_opt; } assert(strRPCUserColonPass.empty()); // Only support initializing once diff --git a/test/functional/rpc_users.py b/test/functional/rpc_users.py index 46ca8866c7..c19ad4202e 100755 --- a/test/functional/rpc_users.py +++ b/test/functional/rpc_users.py @@ -118,17 +118,38 @@ class HTTPBasicsTest(BitcoinTestFramework): actual_perms = file_stat.st_mode & PERM_BITS_UMASK expected_perms = p[perm] assert_equal(expected_perms, actual_perms) + return actual_perms # Remove any leftover rpc{user|password} config options from previous tests self.nodes[1].replace_in_config([("rpcuser", "#rpcuser"), ("rpcpassword", "#rpcpassword")]) self.log.info('Check default cookie permission') - test_perm(None) + default_perms = test_perm(None) self.log.info('Check custom cookie permissions') for perm in p.keys(): test_perm(perm) + self.log.info('Check leaving cookie permissions alone') + unassigned_perms = os.stat(self.nodes[1].chain_path / 'debug.log').st_mode & PERM_BITS_UMASK + self.restart_node(1, extra_args=["-rpccookieperms=0"]) + actual_perms = os.stat(cookie_file_path).st_mode & PERM_BITS_UMASK + assert_equal(unassigned_perms, actual_perms) + self.restart_node(1, extra_args=["-norpccookieperms"]) + actual_perms = os.stat(cookie_file_path).st_mode & PERM_BITS_UMASK + assert_equal(unassigned_perms, actual_perms) + + self.log.info('Check -norpccookieperms -rpccookieperms') + self.restart_node(1, extra_args=["-rpccookieperms=0", "-rpccookieperms=1"]) + actual_perms = os.stat(cookie_file_path).st_mode & PERM_BITS_UMASK + assert_equal(default_perms, actual_perms) + self.restart_node(1, extra_args=["-norpccookieperms", "-rpccookieperms"]) + actual_perms = os.stat(cookie_file_path).st_mode & PERM_BITS_UMASK + assert_equal(default_perms, actual_perms) + self.restart_node(1, extra_args=["-rpccookieperms=1660", "-norpccookieperms", "-rpccookieperms"]) + actual_perms = os.stat(cookie_file_path).st_mode & PERM_BITS_UMASK + assert_equal(default_perms, actual_perms) + def test_norpccookiefile(self, node0_cookie_path): assert self.nodes[0].is_node_stopped(), "We expect previous test to stopped the node" assert not node0_cookie_path.exists() From 783fbafb67c747a44ed1d1904ad133825697e7af Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Mon, 22 Apr 2024 13:16:41 +0000 Subject: [PATCH 5/6] Bugfix: RPC: Attempt to delete cookie tmp before creating it May be needed if we ended up with a read-only tmp file --- src/rpc/request.cpp | 5 +++++ test/functional/rpc_users.py | 3 +++ 2 files changed, 8 insertions(+) diff --git a/src/rpc/request.cpp b/src/rpc/request.cpp index 91f1487a40..d198682457 100644 --- a/src/rpc/request.cpp +++ b/src/rpc/request.cpp @@ -112,6 +112,11 @@ bool GenerateAuthCookie(std::string* cookie_out, std::optional cookie if (filepath_tmp.empty()) { return true; // -norpccookiefile } + try { + fs::remove(filepath_tmp); + } catch (const fs::filesystem_error& e) { + // ignore + } file.open(filepath_tmp); if (!file.is_open()) { LogWarning("Unable to open cookie authentication file %s for writing", fs::PathToString(filepath_tmp)); diff --git a/test/functional/rpc_users.py b/test/functional/rpc_users.py index c19ad4202e..a0c32a360a 100755 --- a/test/functional/rpc_users.py +++ b/test/functional/rpc_users.py @@ -200,7 +200,10 @@ class HTTPBasicsTest(BitcoinTestFramework): cookie_path = self.nodes[0].chain_path / ".cookie" cookie_path_tmp = self.nodes[0].chain_path / ".cookie.tmp" cookie_path_tmp.mkdir() + cookie_path_tmp_subdir = cookie_path_tmp / "subdir" + cookie_path_tmp_subdir.mkdir() self.nodes[0].assert_start_raises_init_error(expected_msg=init_error) + cookie_path_tmp_subdir.rmdir() cookie_path_tmp.rmdir() assert not cookie_path.exists() self.restart_node(0) From e57bd40ca9ca20ae4d2ab8e72f14171a6a6129e4 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Sat, 11 May 2024 15:54:24 +0000 Subject: [PATCH 6/6] RPC: Delete cookie file before replacing it Unclear if this is the best thing to do, but due to v26.1.knots20240325 creating it read-only, it is somewhat necessary for now --- src/rpc/request.cpp | 5 +++++ test/functional/rpc_users.py | 12 ++++++++++++ 2 files changed, 17 insertions(+) diff --git a/src/rpc/request.cpp b/src/rpc/request.cpp index d198682457..444768b25c 100644 --- a/src/rpc/request.cpp +++ b/src/rpc/request.cpp @@ -126,6 +126,11 @@ bool GenerateAuthCookie(std::string* cookie_out, std::optional cookie file.close(); fs::path filepath = GetAuthCookieFile(false); + try { + fs::remove(filepath); + } catch (const fs::filesystem_error& e) { + // ignore + } if (!RenameOver(filepath_tmp, filepath)) { LogWarning("Unable to rename cookie authentication file %s to %s", fs::PathToString(filepath_tmp), fs::PathToString(filepath)); return false; diff --git a/test/functional/rpc_users.py b/test/functional/rpc_users.py index a0c32a360a..d7ea09f3c6 100755 --- a/test/functional/rpc_users.py +++ b/test/functional/rpc_users.py @@ -210,6 +210,18 @@ class HTTPBasicsTest(BitcoinTestFramework): assert cookie_path.exists() self.stop_node(0) + cookie_path.mkdir() + cookie_path_subdir = cookie_path / "subdir" + cookie_path_subdir.mkdir() + self.nodes[0].assert_start_raises_init_error(expected_msg=init_error) + cookie_path_subdir.rmdir() + cookie_path.rmdir() + + self.log.info('Check that a non-writable cookie file will get replaced gracefully') + cookie_path.mkdir(mode=1) + self.restart_node(0) + self.stop_node(0) + self.test_rpccookieperms() self.test_norpccookiefile(cookie_path)