Modernize rpcauth.py #27081

pull sipa wants to merge 1 commits into bitcoin:master from sipa:202302_modernize_rpcauth_py changing 2 files +11 −14

sipa commented at 10:37 PM on February 10, 2023: member

Use Python3 constructions, and f-strings.
DrahtBot commented at 10:37 PM on February 10, 2023: contributor



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK jamesob

Ignored review codo1

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
codo1 commented at 5:43 AM on February 11, 2023: none

tACK Tested as follows: generated new userpw using an explicit password. Used it on signet through an RPC request (checked I got 'incorrect password attempt' if I changed the userpw a bit).

Type	Reviewers
ACK	jamesob
Ignored review	codo1

brunoerg commented at 9:45 PM on February 11, 2023: contributor

from CI (locally I got same error when running rpcauth-test):

Traceback (most recent call last):
  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-w64-mingw32/src/../test/util/rpcauth-test.py", line 33, in test_generate_password
    base64.urlsafe_b64decode(password)).decode('utf-8')
  File "/usr/lib/python3.10/base64.py", line 133, in urlsafe_b64decode
    return b64decode(s)
  File "/usr/lib/python3.10/base64.py", line 87, in b64decode
    return binascii.a2b_base64(s)
binascii.Error: Incorrect padding

in share/rpcauth/rpcauth.py:9 in af2e2325bc outdated

   3 | @@ -4,22 +4,20 @@
   4 |  # file COPYING or http://www.opensource.org/licenses/mit-license.php.
   5 |  
   6 |  from argparse import ArgumentParser
   7 | -from base64 import urlsafe_b64encode
   8 |  from getpass import getpass
   9 | -from os import urandom
  10 | -
  11 |  import hmac
  12 | +import secrets

brunoerg commented at 9:52 PM on February 11, 2023:

nit: from secrets import token_hex, token_urlsafe

sipa commented at 10:11 PM on February 13, 2023:

Done.

in share/rpcauth/rpcauth.py:7 in af2e2325bc outdated
```
   3 | @@ -4,22 +4,20 @@
   4 |  # file COPYING or http://www.opensource.org/licenses/mit-license.php.
   5 |  
   6 |  from argparse import ArgumentParser
   7 | -from base64 import urlsafe_b64encode
```
brunoerg commented at 3:29 PM on February 13, 2023:

Since you're replacing base64 lib to use secrets, perhaps would be good to do the same in rpcauth-test?

sipa commented at 10:10 PM on February 13, 2023:

Thanks; I've made some similar changes there.
sipa force-pushed on Feb 13, 2023
Modernize rpcauth.py and its tests e4e17907b6
sipa force-pushed on Feb 13, 2023
jamesob commented at 10:12 PM on February 13, 2023: member

Github ACK https://github.com/bitcoin/bitcoin/pull/27081/commits/e4e17907b686c25dda91e569645a8f501ca48f90
fanquake merged this on Feb 14, 2023
fanquake closed this on Feb 14, 2023
jonatack commented at 3:52 PM on February 14, 2023: contributor
tACK Tested as follows: generated new userpw using an explicit password. Used it on signet through an RPC request (checked I got 'incorrect password attempt' if I changed the userpw a bit). @codo1 If this comment can be helpful, your review is listed as ignored in #27081 (comment) and is not part of the merge commit because your ACK isn't followed by the commit hash. See these links for more information:
- https://jonatack.github.io/articles/how-to-review-pull-requests-in-bitcoin-core#ack-with-the-commit-hash
- https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#code-review
sidhujag referenced this in commit 852e17c805 on Feb 14, 2023
codo1 commented at 8:47 PM on February 14, 2023: none

@codo1 If this comment can be helpful @jonatack It is. Thank you. Part of the docs you link to need to be re-read by me.
bitcoin locked this on Feb 14, 2024

Contributors

Modernize rpcauth.py #27081

Reviews