[tests] Use cookie auth instead of rpcuser and rpcpassword

achow101 commented at 6:22 pm on June 5, 2017: member

Since 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 as os.path.join(datadir, "regtest", ".cookie") (and elsewhere)

MarcoFalke added the label Tests on Jun 5, 2017

achow101 force-pushed on Jun 5, 2017

in 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 a node = 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: member

Agree 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, 2017

achow101 commented at 8:22 pm on June 6, 2017: member

I added a test for rpcuser and rpcpassword.

sipa commented at 9:54 pm on June 7, 2017: member

utACK; the first two commits should probably be squashed together

sdaftuar commented at 4:00 pm on June 8, 2017: member

Concept 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:

done

in 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:

done

in 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:

done

in 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: member

Concept 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

c53c9831ee

achow101 force-pushed on Jun 8, 2017

Add test for rpcuser/rpcpassword 3ec5ad88e6

achow101 force-pushed on Jun 9, 2017

sipa commented at 0:10 am on June 13, 2017: member

utACK 3ec5ad88e67bba74c795575d52c97fd2c7e880c1

in 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:

boolean or instead of and?

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_requested

MarcoFalke commented at 11:54 am on June 18, 2017: member

utACK 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

279fde58e3

achow101 commented at 5:35 pm on June 18, 2017: member

Addressed @MarcoFalke’s comments

in 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 the bitcoin.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 specifying rpcpassword is enough. Cf. https://github.com/bitcoin/bitcoin/blob/1ad3d4e1261f4a444d982a1470c257c78233bda3/src/httprpc.cpp#L226

So your previous code is right.

MarcoFalke commented at 8:30 am on June 21, 2017: member

Please squash after addressing my feedback.

MarcoFalke approved

MarcoFalke commented at 9:28 am on June 21, 2017: member

Tested ACK 3ec5ad88e67bba74c795575d52c97fd2c7e880c1 (not current HEAD)

laanwj merged this on Jun 21, 2017

laanwj closed this on Jun 21, 2017

laanwj referenced this in commit d083bd9b9c on Jun 21, 2017

PastaPastaPasta referenced this in commit ae22a072bd on Jul 6, 2019

PastaPastaPasta referenced this in commit 35d082cabb on Jul 8, 2019

PastaPastaPasta referenced this in commit a4e91fa4d3 on Jul 9, 2019

PastaPastaPasta referenced this in commit 7802c82e31 on Jul 9, 2019

barrystyle referenced this in commit 2c1549b1cf on Jan 22, 2020

furszy referenced this in commit 0255df35a6 on Apr 26, 2021

DrahtBot locked this on Sep 8, 2021

[tests] Use cookie auth instead of rpcuser and rpcpassword #10533