New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bcrypt dependency version should be 3.1.3 #990

Closed
pghmcfc opened this Issue Jun 11, 2017 · 6 comments

Comments

Projects
None yet
3 participants
@pghmcfc

pghmcfc commented Jun 11, 2017

Earlier versions don't support ignore_few_rounds parameter for kdf():

$ /usr/bin/python2 ./test.py --no-sftp --no-big-file
............................................................................E...............................................................................
======================================================================
ERROR: test_ed25519 (tests.test_pkey.KeyTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/builddir/build/BUILD/paramiko-2.2.0/tests/test_pkey.py", line 456, in test_ed25519
    test_path('test_ed25519_password.key'), b'abc123'
  File "/builddir/build/BUILD/paramiko-2.2.0/paramiko/pkey.py", line 205, in from_private_key_file
    key = cls(filename=filename, password=password)
  File "/builddir/build/BUILD/paramiko-2.2.0/paramiko/ed25519key.py", line 59, in __init__
    signing_key = self._parse_signing_key_data(data, password)
  File "/builddir/build/BUILD/paramiko-2.2.0/paramiko/ed25519key.py", line 122, in _parse_signing_key_data
    ignore_few_rounds=True,
TypeError: kdf() got an unexpected keyword argument 'ignore_few_rounds'

----------------------------------------------------------------------
Ran 156 tests in 21.618s

FAILED (errors=1)

That was with 3.1.2.

@ploxiln

This comment has been minimized.

Contributor

ploxiln commented Jun 11, 2017

Thanks for the report!
cc @alex

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jun 12, 2017

LOL @ adding features in a bugfix release 😞 That kwarg seems to have been added at the same time as the warnings it mutes, going by their changelog on PyPI. Unfortunately this means if we just axe it, users will presumably get annoying warnings on bcrypt 3.1.3+.

Requiring users to be on bcrypt 3.1.3 or newer doesn't seem super awful to me (esp given it's a brand new requirement as of Paramiko 2.2.0). Otherwise we have to do something mildly gross like a try / except TypeError fallback to the non-ignore_few_rounds invocation of kdf(). Which then could mask real TypeErrors lower down.

So I'm thinking we just bump the setup.py requirement for bcrypt up to 3.1.3. If there are no counterarguments I'll do that soon.

@ploxiln

This comment has been minimized.

Contributor

ploxiln commented Jun 12, 2017

Another thing I've done before is change the logging level for a particular module, e.g.

logging.getLogger("bcrypt").setLevel(logging.ERROR)

I'm not sure if that would work in this case, but if it did, it would allow to remove use of ignore_few_rounds while still preventing the warnings.

It does seem like bumping the requirement to bcrypt==3.1.3 isn't too bad, probably the simplest and best way to go.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jun 12, 2017

In my limited experience there's log-level warnings and then there's use of the warnings module which is wholly distinct...not sure which bcrypt is using in this case.

ploxiln added a commit to ploxiln/paramiko that referenced this issue Jun 13, 2017

@ploxiln

This comment has been minimized.

Contributor

ploxiln commented Jun 13, 2017

bcrypt requirement bumped in just-released paramiko-2.2.1

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jun 14, 2017

Oh yea, I neglected to close the related tickets, my bad - thanks!

@bitprophet bitprophet closed this Jun 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment