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

PKey._write_private_key uses old identifier from cryptography #741

Closed
virlos opened this Issue May 9, 2016 · 6 comments

Comments

Projects
None yet
3 participants
@virlos

virlos commented May 9, 2016

Cryptography changed BestEncryption to BestAvailableEncryption in pyca/cryptography@199dc27#diff-f567fb1e66bd7ded34d4717d684339b7R188.

This results in a failure to write the key to a file with paramiko 2.0.0:

import paramiko
from StringIO import StringIO as S
key = paramiko.RSAKey.generate(bits=1024)
key.write_private_key(S(), password='hello')
Traceback (most recent call last):
File "", line 1, in
File "/usr/local/lib/python2.7/dist-packages/paramiko/rsakey.py", line 142, in write_private_key
password=password
File "/usr/local/lib/python2.7/dist-packages/paramiko/pkey.py", line 345, in _write_private_key
encryption = serialization.BestEncryption(password)
AttributeError: 'module' object has no attribute 'BestEncryption'
from cryptography.hazmat.primitives import serialization as s
dir(s)
['BestAvailableEncryption', 'Encoding', 'Enum', 'KeySerializationEncryption', 'NoEncryption', 'PrivateFormat', 'PublicFormat', 'UnsupportedAlgorithm', 'builtins', 'doc', 'file', 'name', 'package', '_load_ssh_dss_public_key', '_load_ssh_ecdsa_public_key', '_load_ssh_rsa_public_key', '_read_next_mpint', '_read_next_string', 'abc', 'absolute_import', 'base64', 'division', 'dsa', 'ec', 'load_der_private_key', 'load_der_public_key', 'load_pem_private_key', 'load_pem_public_key', 'load_ssh_public_key', 'print_function', 'rsa', 'six', 'struct', 'utils']
paramiko.version
'2.0.0'
import cryptography
cryptography.version
'1.3.2'

@bitprophet

This comment has been minimized.

Member

bitprophet commented May 12, 2016

Thanks, not sure how this got overlooked unless it's related to 1e7849b (but the commit you linked is apparently present in crypto 0.8 and up, so even our old requirement should have been API consistent...)

@bitprophet

This comment has been minimized.

Member

bitprophet commented May 12, 2016

Also I'm assuming this is evidence of a hole in the test suite, perhaps there aren't enough (any?) tests for the writing of keys, only reading. Old code is old 😩

@bitprophet bitprophet added this to the 1.16.2 / 1.17.1 / 2.0.1 milestone May 12, 2016

@bitprophet bitprophet modified the milestones: 1.16.2 / 1.17.1 / 2.0.1, 1.16.3 et al Jun 21, 2016

alexpilotti added a commit to alexpilotti/paramiko that referenced this issue Aug 23, 2016

Fixes cryptography compatibility
Writing and reading password protected private keys fails with
cryptography >= 0.8.

Closes bug paramiko#741

alexpilotti added a commit to alexpilotti/paramiko that referenced this issue Aug 23, 2016

Fixes cryptography compatibility
Writing and reading password protected private keys fails with
cryptography >= 0.8.

This patch fixes the issue by updating changed class names and
adding support for AES-256-CBC.

Closes bug paramiko#741
@alexpilotti

This comment has been minimized.

alexpilotti commented Aug 23, 2016

Decryption fails as well due to missing support for AES-256-CBC.
being both trivial fixes, I combined them in a single patch in PR #800.

@alexpilotti

This comment has been minimized.

alexpilotti commented Aug 23, 2016

There's also a PR for this issue here: #772, so I closed #800 to avoid duplicates.

alexpilotti added a commit to alexpilotti/paramiko that referenced this issue Aug 23, 2016

Fixes cryptography compatibility
Writing and reading password protected private keys fails with
cryptography >= 0.8.

This patch fixes the issue by updating changed class names and
adding support for AES-256-CBC.

Closes bug paramiko#741

@bitprophet bitprophet modified the milestones: 1.17.3 et al, 1.17.4 etc Dec 9, 2016

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jun 7, 2017

There's about 3 different tickets that deal with this now. Search, people! Search! sobbing noises

Also, 2 of them muddy the waters by adding AES-256-CBC support, which I'd prefer to be a separate addition in the master/feature branch, because maintainership. The third had some (partially my fault) confusion around whether to handle the Cryptography API change with kid gloves or not.

Thus, none of them are one-click merge-able; I am hand-picking the necessary bits and then attempting to credit everybody involved.


Workflow:

  • #912 added tests, which fail against 2.0 HEAD. Pulled those in first.
  • Added the fix that passes password into _write_private_key from _write_private_key_file, and now the tests surface the actual API breakage, which otherwise goes undetected due to logic branching.
  • Fixing the Crypto API, now I'm seeing why folks kept putting AES-256-CBC in...somehow the file is appearing as that, despite the tests only re-writing the fixture file they read in. Which is supposed to be DES-EDE3-CBC, an already-supported cipher. Wat.
  • OK, seems to be because Cryptography's openssl backend defaults to that particular cipher (if my super quick glance is right, it's literally what "best available encryption" means - a specific cipher name is stored that is manually curated over time).
    • So even though we read a DES-EDE3-CBC file, we are writing an AES-256-CBC one. That's mildly irritating but probably worth considering a distinct issue, especially since one assumes we can consider it a security improvement.
  • I'm a little wary of why we need to replicate that information (keysize, blocksize, mode) inside Paramiko - i.e. I wonder if the entirety of _CIPHER_TABLE and its single blob of references in this file, are legacy-ish and should be axed. Shouldn't it be available inside Cryptography in some form? Or are these very-specific-to-us choices about how to parameterize these ciphers? Still feels bad. Will dig.
  • With the cipher info inserted, test pass, as expected.
@bitprophet

This comment has been minimized.

Member

bitprophet commented Jun 7, 2017

Regardless of the "why do we need to tell ourselves about how to run these ciphers" question, I think I am now mostly ok with considering the entirety of #912 a 2.0-level bugfix. Being unable to read the very files we write is quite bugfix-y and it's not like this truly constitutes "new functionality" and/or much of a stability risk.

dkhapun pushed a commit to cyberx-labs/paramiko that referenced this issue Jun 7, 2018

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