This PR adds tests for rpcwhitelistdefault. The implementation is a continuation of this PR.
Applied suggestions to include the tests in rpc_whitelist.py and to use a single node.
PR covers three test cases:
I didn’t add tests for rpcwhitelistdefault = 0 with user permissions since that is already tested here: rpc_whitelist.py#L77.
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.
11+import urllib.parse
12 from test_framework.test_framework import BitcoinTestFramework
13 from test_framework.util import (
14 assert_equal,
15 str_to_b64str,
16+ get_datadir_path,
nit: See https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#style-guidelines
If more than one name from a module is needed, use lexicographically sorted multi-line imports in order to reduce the possibility of potential merge conflicts.
29+ auth_value = str_to_b64str('{}:{}'.format(user[0], user[3]))
30+ headers = {"Authorization": "Basic " + auth_value}
31 conn = http.client.HTTPConnection(url.hostname, url.port)
32 conn.connect()
33- conn.request('POST', '/', '{"method": "' + method + '"}', headers)
34+ conn.request('POST', '/', '{{"method": "{}"}}'.format(method), headers)
nit: See https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#style-guidelines
Use f’{x}’ for string formatting in preference to ‘{}’.format(x) or ‘%s’ % x.
22+
23+def rpccall(node, user, method, password=None):
24 url = urllib.parse.urlparse(node.url)
25- headers = {"Authorization": "Basic " + str_to_b64str('{}:{}'.format(user[0], user[3]))}
26+ if password is not None:
27+ auth_value = str_to_b64str('{}:{}'.format(user, password))
This seems to be using the argument user as either a string or a list depending on whether or not password was provided, which seems less straightforward than using a consistent type. Unless I’m missing something, the password argument might not be needed at all, since the user list contains the plaintext password. See:
112+ all authorized users (with rpcauth entries) are allowed to access all RPC methods
113+ """
114+ self.log.info("Testing rpcwhitelistdefault=0 with no specified permissions")
115+ with open(os.path.join(get_datadir_path(self.options.tmpdir, 0), "bitcoin.conf"), 'a', encoding='utf8') as f:
116+ f.write("rpcwhitelistdefault=0\n")
117+ f.write("rpcauth=user3:50358aa884c841648e0700b073c32b2e$b73e95fff0748cc0b517859d2ca47d9bac1aa78231f3e48fa9222b612bd2083e\n")
rpcauth lines, it might be better to expand the existing users list. This would apply to adjacent methods as well.
@tdb3 I’ve pushed an update. Adding the users to the existing list is a better approach. However, some tests are now failing because users are whitelisted in the run_test function, while the new tests are designed to test instances when a user is not whitelisted.
I thought clearing the bitcoin.conf file before writing the new test would solve this issue, but it doesn’t. Below is a sample output for the bitcoin.conf file in the test_rpcwhitelist_default_0_without_whitelist function:
0rpcwhitelistdefault=0
1rpcauth=user1:50358aa884c841............................................................
2rpcauth=user2:8650ba41296f6..........................................
3rpcauth=user3:50358aa8..................................................
Despite having new permissions, users are still whitelisted, and thus this assertion would fail:
assert_equal(200, rpccall(self.nodes[0], user[0], "getbestblockhash", user[3]).status)
Why would this happen? The new tests are still reading permissions from therun_testfunction and not the new bitcoin.conf
with open(os.path.join(get_datadir_path(self.options.tmpdir, 0), "bitcoin.conf"), 'a', encoding='utf8') as f creates a new bitcoin.conf file inside bitcoin but outside node0 directory. the bitcoin.conf file inside node0 directory where the actual permissions are read from is unchanged. See L65 for example.
with open(self.nodes[0].datadir_path / "bitcoin.conf", "a", encoding="utf8") as f:
Thank you for picking this up. Enhancing testing for of the whitelist capability is important to notice regressions associated with this security-minded feature.
Concept ACK. Looks like there could be opportunities to enhance the existing approach taken. Some inline comments were left.
Built and ran all functional tests (all passed).
Checked that the following comments were addressed from PR #17805.
test_framework.util importsself.setup_clean_chain = False could be added in https://github.com/bitcoin/bitcoin/blob/94836b3d96f7fe9ed4225bf7ada961ca5dd554e8/test/functional/rpc_whitelist.py#L35-L36specififed typo fixedHTTP_FORBIDDEN instead of 403) could increase readability, but I’m not particularly partial either way. If it’s decided to factor response codes into constants, there probably should be a follow up PR to refactor response codes used in lines in this file predating this PR. Changing the previous instances of 200/403 in this PR would seem to be combining two separate goals (adding new tests, refactoring).
Thank you for picking this up. Enhancing testing for of the whitelist capability is important to notice regressions associated with this security-minded feature.
Concept ACK. Looks like there could be opportunities to enhance the existing approach taken. Some inline comments were left.
Built and ran all functional tests (all passed).
Checked that the following comments were addressed from PR #17805.
- The additional tests appear to be integrated into rpc_whitelist.py
- Tests were organized into different methods
- num_nodes used is 1
- Trailing comma included in
test_framework.utilimports- nit: Looks like the original comment @jonatack to include explicit
self.setup_clean_chain = Falsecould be added in https://github.com/bitcoin/bitcoin/blob/94836b3d96f7fe9ed4225bf7ada961ca5dd554e8/test/functional/rpc_whitelist.py#L35-L36specififedtypo fixed- nit: Seems like @jonatack’s original comment to create constants for HTTP codes (e.g.
HTTP_FORBIDDENinstead of 403) could increase readability, but I’m not particularly partial either way. If it’s decided to factor response codes into constants, there probably should be a follow up PR to refactor response codes used in lines in this file predating this PR. Changing the previous instances of 200/403 in this PR would seem to be combining two separate goals (adding new tests, refactoring).
Thanks a lot for the review, I didn’t implement the requested format for HTTP codes because I wanted to maintain consistency with the already existing tests. However, for readability, I agree with your suggestion. I think the right approach would be to address this in a follow-up PR and also refactor these codes for some of the other tests files as well.
🚧 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.
168+ self.log.info("[" + user[0] + "]: Testing a permitted permission (" + permission + ")")
169+ assert_equal(403, rpccall(self.nodes[0], user[0], "getblockchaininfo", user[3]).status)
170+
171+
172+ def test_whitelistdefault_1_with_specified_permission(self):
173+ self.log.info("Testing rpcwhitelistdefault=1 with specified permissions")
with specified
Extra space between
24 url = urllib.parse.urlparse(node.url)
25- headers = {"Authorization": "Basic " + str_to_b64str('{}:{}'.format(user[0], user[3]))}
26+ if password is not None:
27+ auth_value = str_to_b64str('{}:{}'.format(user, password))
28+ else:
29+ auth_value = str_to_b64str('{}:{}'.format(user[0], user[3]))
Same string formatting nit can be applied here as well.
0Use f'{x}' for string formatting in preference to '{}'.format(x) or '%s' % x.
101@@ -92,5 +102,93 @@ def run_test(self):
102 self.log.info("Strange test 5")
103 assert_equal(200, rpccall(self.nodes[0], self.strange_users[4], "getblockcount").status)
run_test.
181+ permissions = user[2].replace(" ", "").split(",")
182+ i = 0
183+ while i < len(permissions):
184+ if permissions[i] == '':
185+ permissions.pop(i)
186+ i += 1
getPermissions(user) function.
123+ if permissions[i] == '':
124+ permissions.pop(i)
125+ i += 1
126+ for permission in permissions:
127+ assert_equal(200, rpccall(self.nodes[0], user[0], permission, user[3]).status)
128+ assert_equal(200, rpccall(self.nodes[0], user[0], "getbestblockhash", user[3]).status)
This test fails at this line, throwing 403 instead.
02024-05-08T10:36:33.356000Z TestFramework (INFO): Testing rpcwhitelistdefault=0 with no specified permissions
12024-05-08T10:36:34.102000Z TestFramework (ERROR): Assertion failed
2Traceback (most recent call last):
3 File "/Users/aither/projects/bitcoin/test/functional/test_framework/test_framework.py", line 132, in main
4 self.run_test()
5 File "/Users/aither/projects/bitcoin/test/functional/rpc_whitelist.py", line 106, in run_test
6 self.test_rpcwhitelist_default_0_without_whitelist()
7 File "/Users/aither/projects/bitcoin/test/functional/rpc_whitelist.py", line 128, in test_rpcwhitelist_default_0_without_whitelist
8 assert_equal(200, rpccall(self.nodes[0], user[0], "getbestblockhash", user[3]).status)
9 File "/Users/aither/projects/bitcoin/test/functional/test_framework/util.py", line 74, in assert_equal
10 raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
11AssertionError: not(200 == 403)
109+ self.test_whitelistdefault_1_with_specified_permission()
110+
111+
112+ def test_rpcwhitelist_default_0_without_whitelist(self):
113+ self.log.info("Testing rpcwhitelistdefault=0 with no specified permissions")
114+ with open(os.path.join(self.nodes[0].cwd, "bitcoin.conf"), "w", encoding="utf8") as f:
Thanks for adding these tests, concept ACK.
Seems like this is in progress because there is test failure and a debugger is added. Added few suggestions for now, will review again when the tests pass.
Thanks for adding these tests, concept ACK.
Seems like this is in progress because there is test failure and a debugger is added. Added few suggestions for now, will review again when the tests pass.
Thanks for the review! The initial tests passed successfully. But I pushed an update that implements the suggested alternative approach from this discussion
Unfortunately, the tests are currently failing with this new approach, as explained here.
While I’m still working on refining the suggested approach, I’ve decided to revert the PR to its original state (with passing tests) for now. To keep things functional while I continue working on the alternative approach in a separate branch on my fork
99@@ -92,5 +100,79 @@ def run_test(self):
100 self.log.info("Strange test 5")
101 assert_equal(200, rpccall(self.nodes[0], self.strange_users[4], "getblockcount").status)
102
103+ #rpcwhitelistdefaulttest
18-import http.client
19-import urllib.parse
20
21-def rpccall(node, user, method):
22+
23+def rpccall(node, user, method, password=None):
b8be5d4: do we need this commit? aren’t all the final changes in the next commit 875af7d.
if not, for the code added in b8be5d4 and later removed in 875af7d - you could edit b8be5d4 instead. diff and commits would be easier to review.
35 resp = conn.getresponse()
36 conn.close()
37 return resp
38
39
40+def getPermissions(user):
140+ with open(self.nodes[0].datadir_path / "bitcoin.conf", "a", encoding="utf8") as f:
141+ f.write("\nrpcwhitelistdefault=1\n")
142+ f.write("rpcwhitelist=__cookie__:getblockcount,getblockchaininfo,getmempoolinfo,stop\n")
143+ self.restart_node(0)
144+
145+ assert_equal(403, rpccall(self.nodes[0], self.users[2], "getblockcount").status)
test_rpcwhitelistdefault_0_no_permissions() and test_rpcwhitelistdefault_1_no_permissions()so that it would be easier to follow that not whitelisted user can access any method in the former case and cannot access any method in the latter case.
COMMON_PERMISSIONS = ["getbestblockhash", "getblockchaininfo"] to be used by both functions.
132+ with open(self.nodes[0].datadir_path / "bitcoin.conf", 'r', encoding='utf8') as f:
133+ lines = f.readlines()
134+ with open(self.nodes[0].datadir_path / "bitcoin.conf", 'w', encoding='utf8') as f:
135+ for line in lines:
136+ if "rpcwhitelistdefault=0" not in line:
137+ f.write(line)
9f635812: you could combine write and append of the file in 1 place.
0with open(self.nodes[0].datadir_path / "bitcoin.conf", 'w', encoding='utf8') as f:
1 for line in lines:
2 if "rpcwhitelistdefault=0" in line:
3 f.write("rpcwhitelistdefault=1\n")
4 else:
5 f.write(line)
6 f.write("rpcwhitelist=__cookie__:getblockcount,getblockchaininfo,getmempoolinfo,stop\n")
149+ """
150+ * rpcwhitelistdefault=1
151+ * Permissions:
152+ (user1): getbestblockhash,getblockcount
153+ (user2): getblockcount
154+ Expected result: * users can only acess whitelisted methods
156+ for user in self.users:
157+ permissions = getPermissions(user)
158+ for permission in permissions:
159+ self.log.info("[" + user[0] + "]: Testing a permitted permission (" + permission + ")")
160+ assert_equal(200, rpccall(self.nodes[0], user, permission).status)
161+ assert_equal(403, rpccall(self.nodes[0], user, "getblockchaininfo").status)
self.never_allowed permission set instead of “getblockchaininfo”.
37+
38+
39 class RPCWhitelistTest(BitcoinTestFramework):
40 def set_test_params(self):
41 self.num_nodes = 1
42+ self.setup_clean_chain = False
47@@ -35,7 +48,8 @@ def run_test(self):
48 # 3 => Password Plaintext
49 self.users = [
50 ["user1", "50358aa884c841648e0700b073c32b2e$b73e95fff0748cc0b517859d2ca47d9bac1aa78231f3e48fa9222b612bd2083e", "getbestblockhash,getblockcount,", "12345"],
51- ["user2", "8650ba41296f62092377a38547f361de$4620db7ba063ef4e2f7249853e9f3c5c3592a9619a759e3e6f1c63f2e22f1d21", "getblockcount", "54321"]
52+ ["user2", "8650ba41296f62092377a38547f361de$4620db7ba063ef4e2f7249853e9f3c5c3592a9619a759e3e6f1c63f2e22f1d21", "getblockcount", "54321"],
53+ ["user3", "ab02e4fb22ef4ab004cca217a49ee8d2$90dd09b08edd12d552d9d8a5ada838dcef2ac587789fa7e9c47f5990e80cdf93"," ", "password123"]
9f635812: style nit:
0 ["user3", "ab02e4fb22ef4ab004cca217a49ee8d2$90dd09b08edd12d552d9d8a5ada838dcef2ac587789fa7e9c47f5990e80cdf93", "", "password123"]
122+ """
123+ * rpcwhitelistdefault=1
124+ * No Permissions defined
125+ Expected result: * user3 (not whitelisted) can not access any method
126+ """
127+ # Rewrite file configurations
replace_in_config can be used here?