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

achow101 · 2017-06-05T18:22:15Z

Since rpcuser and rpcpassword are now deprecated, replace them with cookie auth.

sipa · 2017-06-05T18:24:45Z

test/functional/test_framework/util.py

can be written as os.path.join(datadir, "regtest", ".cookie") (and elsewhere)

sipa · 2017-06-06T07:03:55Z

test/functional/p2p-segwit.py

Would a node = self.nodes[node_id] work as well?

It would, but I don't see why that would be any better.

laanwj · 2017-06-06T08:51:25Z

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 · 2017-06-06T20:22:06Z

I added a test for rpcuser and rpcpassword.

sipa · 2017-06-07T21:54:39Z

utACK; the first two commits should probably be squashed together

sdaftuar · 2017-06-08T16:00:33Z

Concept ACK, though the pruning.py test fails for me -- I assume it needs an update similar to p2p-segwit.py.

jnewbery

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.

jnewbery · 2017-06-08T17:28:53Z

test/functional/test_framework/util.py

This should be a debug log (logger.debug())

that's a mistake from my earlier debugging. removed.

jnewbery · 2017-06-08T17:30:06Z

test/functional/test_framework/util.py

Perhaps assert if there are multiple "rpcuser=" lines? Same for "rpcpassword=" lines

jnewbery · 2017-06-08T17:33:53Z

test/functional/multi_rpc.py

It'd be better to assert directly on the status, ie:

assert_equal(resp.status, 200)

jnewbery · 2017-06-08T17:35:02Z

test/functional/multi_rpc.py

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.

jnewbery · 2017-06-08T17:38:44Z

test/functional/multi_rpc.py

Recommend that you add some unicode characters to the username/password to test that we don't regress unicode support at some point.

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.

Since rpcuser and rpcpassword are now deprecated, replace them with cookie auth. Fix test failures with cookie auth

sipa · 2017-06-13T00:10:15Z

utACK 3ec5ad8

maflcko

utACK 3ec5ad8, but needs feedback addressed.

maflcko · 2017-06-18T11:46:51Z

test/functional/test_framework/util.py

-def rpc_url(i, rpchost=None):
-    rpc_u, rpc_p = rpc_auth_pair(i)
+def get_auth_cookie(datadir, n):
+    if os.path.isfile(os.path.join(datadir, "regtest", ".cookie")):


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?

maflcko · 2017-06-18T11:47:26Z

test/functional/test_framework/util.py

+                if line.startswith("rpcpassword="):
+                    assert password is None # Ensure that there is only one rpcpassword line
+                    password = line.split("=")[1].strip("\n")
+            if user is None and password is None:


boolean or instead of and?

maflcko · 2017-06-18T11:53:43Z

test/functional/p2p-segwit.py

        self.test_non_standard_witness()
        sync_blocks(self.nodes)
-        self.test_upgrade_after_activation(self.nodes[2], 2)
+        self.test_upgrade_after_activation(2)


Please name primitive types. If you don't want to clutter the scope with variable names, you could always just use named args.

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 · 2017-06-18T17:35:50Z

Addressed @MarcoFalke's comments

maflcko · 2017-06-21T08:28:04Z

test/functional/test_framework/util.py

+            password = split_userpass[1]
+    if user is None or password is None:
+        raise ValueError("No RPC credentials")
+    return user, password


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,

with open('conf'): for line in f: # Read user and password ... if user is None or password is None: raise ValueError("No RPC credentials") return user, password # ... with open('cookie'): # Read and split return split_userpass[0], split_userpass[1]

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.

Guess you are right. I must have messed up something locally when testing this. Let me check ...

Sorry, I thought that only specifying rpcuser in the conf is enough for authentication, when in fact only specifying rpcpassword is enough. Cf.

bitcoin/src/httprpc.cpp

Line 226 in 1ad3d4e

} else {

So your previous code is right.

maflcko · 2017-06-21T08:30:31Z

Please squash after addressing my feedback.

maflcko · 2017-06-21T09:28:00Z

Tested ACK 3ec5ad8 (not current HEAD)

279fde5 Check for rpcuser/rpcpassword first then for cookie (Andrew Chow) 3ec5ad8 Add test for rpcuser/rpcpassword (Andrew Chow) c53c983 Replace cookie auth in tests (Andrew Chow) Tree-SHA512: 21efb84c87080a895cac8a7fe4766738c34eebe9686c7d10af1bf91ed4ae422e2d5dbbebffd00d34744eb6bb2d0195ea3aca86deebf085bbdeeb1d8b474241ed

…pcpassword 279fde5 Check for rpcuser/rpcpassword first then for cookie (Andrew Chow) 3ec5ad8 Add test for rpcuser/rpcpassword (Andrew Chow) c53c983 Replace cookie auth in tests (Andrew Chow) Tree-SHA512: 21efb84c87080a895cac8a7fe4766738c34eebe9686c7d10af1bf91ed4ae422e2d5dbbebffd00d34744eb6bb2d0195ea3aca86deebf085bbdeeb1d8b474241ed

8817d76 Add config changes to release notes (random-zebra) 988d428 [tests] Unit tests for -testnet/-regtest in [test]/[regtest] sections (Anthony Towns) 7b72630 ArgsManager: special handling for -regtest and -testnet (Anthony Towns) 51c4aa1 [tests] Unit tests for network-specific config entries (Anthony Towns) 30f3bab ArgsManager: Warn when ignoring network-specific config setting (Anthony Towns) 1bddffe ArgsManager: limit some options to only apply on mainnet when in default section (Anthony Towns) ef505ea [Tests] Fix and re-activate rpc_users.py functional test (random-zebra) 0eacce7 [RPC] Add rpcauth startup flag for multiple RPC users (random-zebra) e79ff3c [Tests] Fix and re-activate feature_config_args functional test (random-zebra) 3fcaaa4 Replace cookie auth in tests (random-zebra) b907f33 rpc: Generate auth cookie in hex instead of base64 (random-zebra) bf443a9 [tests] Use regtest section in functional tests configs (Anthony Towns) 7857bcd [tests] Unit tests for config file sections (Anthony Towns) 4cf2ee6 ArgsManager: support config file sections (Anthony Towns) 2658771 ArgsManager: drop m_negated_args (Anthony Towns) 4ee69b5 ArgsManager: keep command line and config file arguments separate (random-zebra) 4f416b8 [tests] Add additional unit tests for -nofoo edge cases (Anthony Towns) 5d1200b [tests] Check GetChainName works with config entries (Anthony Towns) 157da7c [tests] Add unit tests for ReadConfigStream (Anthony Towns) 23a4633 ReadConfigStream: assume the stream is good (Anthony Towns) 56ea59e Separate out ReadConfigStream from ReadConfigFile (Anthony Towns) a3438a2 Test datadir in conf file exists (MeshCollider) 459c4ad [tests] Add unit tests for GetChainName (Anthony Towns) 0ab3e99 Move ChainNameFromCommandLine into ArgsManager, rename to GetChainName (Anthony Towns) 1ea7ce6 Globals: Decouple GetConfigFile and ReadConfigFile from global mapArgs (random-zebra) Pull request description: It is now possible for a single configuration file to set different options for different networks. This is done by using sections or by prefixing the option with the network, such as: ``` main.uacomment=pivx test.uacomment=pivx-testnet regtest.uacomment=regtest [main] mempoolsize=300 [test] mempoolsize=100 [regtest] mempoolsize=20 ``` The `addnode=`, `connect=`, `port=`, `bind=`, `rpcport=`, `rpcbind=`, and `wallet=` options will only apply to mainnet when specified in the configuration file, unless a network is specified. Also - fix cookie-based authentication for the functional tests, and re-enable `feature_config_args.py` - add `-rpcauth` startup flag, for multiple RPC users, and re-enable `rpc_users.py`. Backports relevant commits from: - bitcoin#8856 Globals: Decouple GetConfigFile and ReadConfigFile from global mapArgs - bitcoin#11829 Test datadir specified in conf file exists - bitcoin#12878 Config handling refactoring in preparation for network-specific sections - bitcoin#11862 Network specific conf sections - bitcoin#10533 [tests] Use cookie auth instead of rpcuser and rpcpassword - bitcoin#8858 Generate auth cookie in hex instead of base64 ACKs for top commit: Fuzzbawls: Tested ACK 8817d76 furszy: utACK 8817d76 Tree-SHA512: 0d23dbf3d254b186a5378576601cf43f8322abe036b0b135a39b22e13ffa2e299cb1323160d87c7d8284e6aaa229c47f54c8a3a3ebf07cc298e644fb8bd69dc0

sipa reviewed Jun 5, 2017

View reviewed changes

test/functional/test_framework/util.py Outdated

Copy link

Member

sipa Jun 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be written as os.path.join(datadir, "regtest", ".cookie") (and elsewhere)

maflcko added the Tests label Jun 5, 2017

achow101 force-pushed the tests-use-cookie-auth branch 2 times, most recently from 01e186b to 5d26d02 Compare June 5, 2017 23:07

sipa reviewed Jun 6, 2017

View reviewed changes

achow101 force-pushed the tests-use-cookie-auth branch from 5d26d02 to aec5644 Compare June 6, 2017 18:43

jnewbery reviewed Jun 8, 2017

View reviewed changes

Replace cookie auth in tests

c53c983

Since rpcuser and rpcpassword are now deprecated, replace them with cookie auth. Fix test failures with cookie auth

achow101 force-pushed the tests-use-cookie-auth branch 5 times, most recently from 3e5840a to 9d3b314 Compare June 8, 2017 23:32

Add test for rpcuser/rpcpassword

3ec5ad8

achow101 force-pushed the tests-use-cookie-auth branch from 9d3b314 to 3ec5ad8 Compare June 9, 2017 00:19

maflcko suggested changes Jun 18, 2017

View reviewed changes

Check for rpcuser/rpcpassword first then for cookie

279fde5

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

maflcko reviewed Jun 21, 2017

View reviewed changes

maflcko approved these changes Jun 21, 2017

View reviewed changes

laanwj merged commit 279fde5 into bitcoin:master Jun 21, 2017

laanwj mentioned this pull request Jun 21, 2017

Move stop/start functions from utils.py into BitcoinTestFramework #10556

Merged

lontivero mentioned this pull request Mar 7, 2019

Uses RPC cookie file instead of rpcuser:rpcpassword WalletWasabi/WalletWasabi#1210

Merged

random-zebra mentioned this pull request Apr 19, 2021

[Core] Network specific configuration sections PIVX-Project/PIVX#2324

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

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

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

Uh oh!

Conversation

achow101 commented Jun 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jun 6, 2017

Uh oh!

achow101 commented Jun 6, 2017

Uh oh!

sipa commented Jun 7, 2017

Uh oh!

sdaftuar commented Jun 8, 2017

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Jun 13, 2017

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jun 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jun 21, 2017

Uh oh!

maflcko commented Jun 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants