[tests] Use cookie auth instead of rpcuser and rpcpassword #10533
pull achow101 wants to merge 3 commits into bitcoin:master from achow101:tests-use-cookie-auth changing 6 files +98 −31-
achow101 commented at 6:22 pm on June 5, 2017: memberSince rpcuser and rpcpassword are now deprecated, replace them with cookie auth.
-
in test/functional/test_framework/util.py:195 in 7727344b47 outdated
199+ 200+def get_datadir_path(dirname, n): 201+ return os.path.join(dirname, "node"+str(n)) 202+ 203+def get_auth_cookie(datadir, n): 204+ with open(os.path.join(os.path.join(datadir, "regtest"), ".cookie"), 'r') as f:
sipa commented at 6:24 pm on June 5, 2017:can be written asos.path.join(datadir, "regtest", ".cookie")
(and elsewhere)MarcoFalke added the label Tests on Jun 5, 2017achow101 force-pushed on Jun 5, 2017achow101 force-pushed on Jun 5, 2017in test/functional/p2p-segwit.py:1505 in 5d26d02c5f outdated
1501@@ -1502,14 +1502,14 @@ def test_upgrade_after_activation(self, node, node_id): 1502 sync_blocks(self.nodes) 1503 1504 # Make sure that this peer thinks segwit has activated. 1505- assert(get_bip9_status(node, 'segwit')['status'] == "active") 1506+ assert(get_bip9_status(self.nodes[node_id], 'segwit')['status'] == "active")
sipa commented at 7:03 am on June 6, 2017:Would anode = self.nodes[node_id]
work as well?
achow101 commented at 5:25 pm on June 6, 2017:It would, but I don’t see why that would be any better.laanwj commented at 8:51 am on June 6, 2017: memberAgree on doing this by default.
I do think we should keep one regression test around for rpcuser/rpcpasswd as long as that functionality is still supported, though.
achow101 force-pushed on Jun 6, 2017achow101 commented at 8:22 pm on June 6, 2017: memberI added a test for rpcuser and rpcpassword.sipa commented at 9:54 pm on June 7, 2017: memberutACK; the first two commits should probably be squashed togethersdaftuar commented at 4:00 pm on June 8, 2017: memberConcept ACK, though the pruning.py test fails for me – I assume it needs an update similar to p2p-segwit.py.in test/functional/test_framework/util.py:236 in ba2d7592a3 outdated
230@@ -215,7 +231,9 @@ def wait_for_bitcoind_start(process, url, i): 231 if process.poll() is not None: 232 raise Exception('bitcoind exited with status %i during initialization' % process.returncode) 233 try: 234- rpc = get_rpc_proxy(url, i) 235+ # Check if .cookie file to be created 236+ rpc = get_rpc_proxy(rpc_url(datadir, i), i) 237+ logger.info(rpc.url)
jnewbery commented at 5:28 pm on June 8, 2017:This should be a debug log (logger.debug()
)
achow101 commented at 6:25 pm on June 8, 2017:that’s a mistake from my earlier debugging. removed.in test/functional/test_framework/util.py:207 in ba2d7592a3 outdated
210+ with open(os.path.join(datadir, "bitcoin.conf"), 'r') as f: 211+ user = None 212+ password = None 213+ for line in f: 214+ if line.startswith("rpcuser="): 215+ user = line.split("=")[1].strip("\n")
jnewbery commented at 5:30 pm on June 8, 2017:Perhaps assert if there are multiple “rpcuser=” lines? Same for “rpcpassword=” lines
achow101 commented at 6:25 pm on June 8, 2017:donein test/functional/multi_rpc.py:129 in ba2d7592a3 outdated
124+ 125+ conn = http.client.HTTPConnection(url.hostname, url.port) 126+ conn.connect() 127+ conn.request('POST', '/', '{"method": "getbestblockhash"}', headers) 128+ resp = conn.getresponse() 129+ assert_equal(resp.status==401, False)
jnewbery commented at 5:33 pm on June 8, 2017:It’d be better to assert directly on the status, ie:
assert_equal(resp.status, 200)
achow101 commented at 6:25 pm on June 8, 2017:donein test/functional/multi_rpc.py:140 in ba2d7592a3 outdated
135+ 136+ conn = http.client.HTTPConnection(url.hostname, url.port) 137+ conn.connect() 138+ conn.request('POST', '/', '{"method": "getbestblockhash"}', headers) 139+ resp = conn.getresponse() 140+ assert_equal(resp.status==401, True)
jnewbery commented at 5:35 pm on June 8, 2017:Better to assert directly on the status, ie:
assert_equal(resp.status, 401)
This will cause the received status to be written out in the assert if the assert fails.
achow101 commented at 6:25 pm on June 8, 2017:donein test/functional/multi_rpc.py:26 in ba2d7592a3 outdated
22 def setup_chain(self): 23 super().setup_chain() 24 #Append rpcauth to bitcoin.conf before initialization 25 rpcauth = "rpcauth=rt:93648e835a54c573682c2eb19f882535$7681e9c5b74bdd85e78166031d2058e1069b3ed7ed967c93fc63abba06f31144" 26 rpcauth2 = "rpcauth=rt2:f8607b1a88861fac29dfccf9b52ff9f$ff36a0c23c8c62b4846112e50fa888416e94c17bfd4c42f88fd8f55ec6a3137e" 27+ rpcuser = "rpcuser=rpcuser"
jnewbery commented at 5:38 pm on June 8, 2017:Recommend that you add some unicode characters to the username/password to test that we don’t regress unicode support at some point.
achow101 commented at 6:29 pm on June 8, 2017:done
laanwj commented at 6:29 pm on June 8, 2017:Yes, this was the reason why we use the funny unicode characters in the rpcuser/pass in the current code. It needs to be tested at some point.jnewbery commented at 5:42 pm on June 8, 2017: memberConcept ACK. It definitely makes sense to have our test framework use the mainline expected authentication method (and have a regression test for username/password as before).
However, I’d very much prefer if this happens after #10082. That introduces a TestNode class that will encapsulate all the properties and state for a node under test. I think that makes the test framework a lot clearer and prevents having to pass variables into functions in utils.py.
I’ve added a few review comments inline.
Replace cookie auth in tests
Since rpcuser and rpcpassword are now deprecated, replace them with cookie auth. Fix test failures with cookie auth
achow101 force-pushed on Jun 8, 2017achow101 force-pushed on Jun 8, 2017achow101 force-pushed on Jun 8, 2017achow101 force-pushed on Jun 8, 2017achow101 force-pushed on Jun 8, 2017Add test for rpcuser/rpcpassword 3ec5ad88e6achow101 force-pushed on Jun 9, 2017sipa commented at 0:10 am on June 13, 2017: memberutACK 3ec5ad88e67bba74c795575d52c97fd2c7e880c1in test/functional/test_framework/util.py:195 in 3ec5ad88e6 outdated
197+ return os.path.join(dirname, "node"+str(n)) 198 199-def rpc_url(i, rpchost=None): 200- rpc_u, rpc_p = rpc_auth_pair(i) 201+def get_auth_cookie(datadir, n): 202+ if os.path.isfile(os.path.join(datadir, "regtest", ".cookie")):
MarcoFalke commented at 11:46 am on June 18, 2017:I think you can not check for the cookie file first. This file is always created and thus, the code in the
else
block is dead code.You might want to check for
rpcuser
first?in test/functional/test_framework/util.py:211 in 3ec5ad88e6 outdated
213+ assert user is None # Ensure that there is only one rpcuser line 214+ user = line.split("=")[1].strip("\n") 215+ if line.startswith("rpcpassword="): 216+ assert password is None # Ensure that there is only one rpcpassword line 217+ password = line.split("=")[1].strip("\n") 218+ if user is None and password is None:
MarcoFalke commented at 11:47 am on June 18, 2017:booleanor
instead ofand
?in test/functional/p2p-segwit.py:1947 in 3ec5ad88e6 outdated
1943@@ -1944,7 +1944,7 @@ def run_test(self): 1944 self.test_signature_version_1() 1945 self.test_non_standard_witness() 1946 sync_blocks(self.nodes) 1947- self.test_upgrade_after_activation(self.nodes[2], 2) 1948+ self.test_upgrade_after_activation(2)
MarcoFalke commented at 11:53 am on June 18, 2017:Please name primitive types. If you don’t want to clutter the scope with variable names, you could always just use named args.MarcoFalke changes_requestedMarcoFalke commented at 11:54 am on June 18, 2017: memberutACK 3ec5ad88e67bba74c795575d52c97fd2c7e880c1, but needs feedback addressed.Check for rpcuser/rpcpassword first then for cookie
Better to check that rpcuser and rpcpassword exist then to check for the cookie in the test framework. Name an argument for consistency in p2p-segwit.py
achow101 commented at 5:35 pm on June 18, 2017: memberAddressed @MarcoFalke’s commentsin test/functional/test_framework/util.py:214 in 279fde58e3
217+ split_userpass = userpass.split(':') 218+ user = split_userpass[0] 219+ password = split_userpass[1] 220+ if user is None or password is None: 221+ raise ValueError("No RPC credentials") 222+ return user, password
MarcoFalke commented at 8:28 am on June 21, 2017:I still don’t understand how this could pass travis. As mentioned earlier, the
.cookie
file is always created, even when rpcuser, rpcpassword is in thebitcoin.conf
. Thus, the current function should always “fall through” to read the cookie file (even if the rpcuser and rpcpassword was read). Maybe travis has just slow IO and the cookie is not yet created, but we shouldn’t rely on this hardware detail for the test to pass.I’d suggest you move the return statement into each body of the if blocks.
Specifically,
0with open('conf'): 1 for line in f: 2 # Read user and password ... 3 if user is None or password is None: 4 raise ValueError("No RPC credentials") 5 return user, password 6 7# ... 8 9with open('cookie'): 10 # Read and split 11 return split_userpass[0], split_userpass[1]
achow101 commented at 8:36 am on June 21, 2017:The.cookie
file is never created if rpcuser and rpcpassword are specified in the.conf
file. You can check the behavior yourself. All of the tests pass locally for me too.
MarcoFalke commented at 9:03 am on June 21, 2017:Guess you are right. I must have messed up something locally when testing this. Let me check …
MarcoFalke commented at 9:27 am on June 21, 2017:Sorry, I thought that only specifying
rpcuser
in the conf is enough for authentication, when in fact only specifyingrpcpassword
is enough. Cf. https://github.com/bitcoin/bitcoin/blob/1ad3d4e1261f4a444d982a1470c257c78233bda3/src/httprpc.cpp#L226So your previous code is right.
MarcoFalke commented at 8:30 am on June 21, 2017: memberPlease squash after addressing my feedback.MarcoFalke approvedMarcoFalke commented at 9:28 am on June 21, 2017: memberTested ACK 3ec5ad88e67bba74c795575d52c97fd2c7e880c1 (not current HEAD)laanwj merged this on Jun 21, 2017laanwj closed this on Jun 21, 2017
laanwj referenced this in commit d083bd9b9c on Jun 21, 2017PastaPastaPasta referenced this in commit ae22a072bd on Jul 6, 2019PastaPastaPasta referenced this in commit 35d082cabb on Jul 8, 2019PastaPastaPasta referenced this in commit a4e91fa4d3 on Jul 9, 2019PastaPastaPasta referenced this in commit 7802c82e31 on Jul 9, 2019barrystyle referenced this in commit 2c1549b1cf on Jan 22, 2020furszy referenced this in commit 0255df35a6 on Apr 26, 2021DrahtBot locked this on Sep 8, 2021
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: 2025-01-22 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me