Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#7998 enhancement closed fixed (fixed)

twisted.conch.ssh.keys should be ported to Python 3

Reported by: hawkowl Owned by: hawkowl
Priority: normal Milestone: Python-3.x
Component: conch Keywords:
Cc: z3p Branch: branches/conch-ssh-keys-py3-7998-3
branch-diff, diff-cov, branch-cov, buildbot
Author: hawkowl

Description


Change History (21)

comment:1 Changed 2 years ago by DefaultCC Plugin

Cc: z3p added

comment:2 Changed 2 years ago by hawkowl

Author: hawkowl
Branch: branches/conch-ssh-keys-py3-7998

(In [45461]) Branching to conch-ssh-keys-py3-7998.

comment:3 Changed 2 years ago by hawkowl

Keywords: review added

Builders spun, please review.

comment:4 Changed 2 years ago by Adi Roiban

Keywords: review removed
Owner: set to hawkowl

Sorry for the delay.

Many thanks for the patience and for working on this!


In twisted/conch/ssh/common.py instead of ord(bn[0:1]) not not just use bn[0] ?

For _fastpow # evil evil maybe we can update the code with a proper comment. Proper formatting and a full sentence explaing what is going on there.


In twisted/conch/ssh/keys.py raise BadKeyError("%s is not b'RSA' or b'DSA'" % (kind,)). Maybe the error should be more generic. Later we might want to add support for other key types and then this error might get out of sync. Maybe use just 'Unknown key type %s' % (kind,)

For if type in ('RSA', 'DSA'): why not use b'RSA ? Why native string?

In blob and privateBlob why not use bytes for all data keys?

For for _toString_LSH ... it uses native string keys... but when importing from LSH it uses bytes?

Same story for many other places. Do we use bytes for data keys or native strings ? I am confused :(

In test_fromPrivateBlob the data is compared agains a dict with keys as native string. Why?


In sign you raise an error for uknown key type but verify is left unchanged. Why? I think that these changes should be done in a separte ticket as they will fix some ugly errors and they need a separate release note. I am +1 for these changes as I already use them on my fork.


It looks like this also ports the cred_sshkeys plugin... but is not included in the relase notes. I think that this need a release note...

Either update the current news fragment or create a separate ticket...

Is twisted/plugins/cred_sshkeys.py and twisted.test.test_strcred part of dist3.py ? I could not see it so in the end, maybe we need a separate ticket for the cred_sshkeys changes.


Please check my comments and resubmit.

Many thanks!

comment:5 Changed 2 years ago by hawkowl

Branch: branches/conch-ssh-keys-py3-7998branches/conch-ssh-keys-py3-7998-2

(In [45686]) Branching to conch-ssh-keys-py3-7998-2.

comment:6 Changed 2 years ago by hawkowl

Branch: branches/conch-ssh-keys-py3-7998-2branches/conch-ssh-keys-py3-7998-3

(In [46185]) Branching to conch-ssh-keys-py3-7998-3.

comment:7 Changed 2 years ago by hawkowl

Keywords: review added
Owner: hawkowl deleted

Builders are looking good.

  • The [0:1] thing is because if you just do [0] on a bytes in Python 3, you get an int back, not a bytes.
  • Fixed the mixing of str/bytes in most places, except in the instance where it is interacting with Python identifiers.
  • Ported the strcred stuff properly. Don't think it needs a ticket, it's what enables the conch ssh stuff to be used by cred.

Please review.

comment:8 Changed 2 years ago by Adi Roiban

Keywords: review removed
Owner: set to hawkowl

Thanks for the update.

I am -1 for all text to bytes conversions.

While RSA/DSA primitive data is binary the meta-data around those numbers is text and should be human readable.

Many of the RSA/DSA ssh key serializations are designed to be printables with human readable meta-data.

So I think that type, keyType RSA/DSA key components names, fromString and toString should be convert to str/Unicode rather than bytes.

In the cryptography library, RSA/DSA key components have non-bytes names and they are class members, valid Python variable names.

If you don't agree and you think that all these names should be bytes, let me know and I will try to find another person to review this.


Do we need the fingerpring as bytes ?

It is suposted to be a human readable / printable representation of the SSH key.


Do we really need the type and typeSSH as bytes?

I think both are supposed to be human readables / printable so they should be text.

Can you please also document the rtype for def type(self): ?

Same for key components. Those components names are names given by us, to help us identify them so they are supposted to be human readables and printables.

What is the point of having non-printables key components or SSH key types?


toString() is strange... as now it does something like toBytes

I think that it should return Unicode. Some key format support arbitrary header values which in the RFC are specififed as Unicode... and encoded as utf-8.

We don't yet support but and there are no tests for such keys, but they can exists.

... same for fromString()

passphrase can and should be bytes :)

sign/verify can return bytes... and should :)


Why delete objectType on Python3 ? I can guess why, but I think that it need a comment.


for Key.fromFile

can we have a context for this file handling?

return Class.fromString(open(filename, 'rb').read(), type, passphrase)

Please see my comments and resubmit.

Thanks!

comment:9 Changed 2 years ago by hawkowl

Keywords: review added
Owner: hawkowl deleted

Hi Adi, thanks for the review.

  • I don't think fromString/toString going to Unicode is a good idea, and I think it's slightly out of scope. Maybe we should have a "toBytes" instead, implemented in a different ticket.
  • type is now a str, sshType is unpacked from something so I'd rather not try and decode it.
  • Added a comment about ibjectType.
  • Added a context thingy.

Please review, builders spun.

comment:10 Changed 2 years ago by Adi Roiban

Hi,

Many thanks for the update.

fromString/toString are bad API so I have no hard feelings about them other than deprecating them ASAP :) ... it is just strange that they are called from/to string and in fact are just bytes.

And those strings expected by fromString/toString, are not SSH length encoded strings and are not text... but are bytes. People might have a WTF moment when using this API.


I still think that sshType should be text and decoded/encoded as US-ASCII

https://tools.ietf.org/html/rfc4253#section-6.6

Certificates and public keys are encoded as follows:

string certificate or public key format identifier byte[n] key/certificate data

https://tools.ietf.org/html/rfc4251#section-5

string:

  • Arbitrary length binary string.
  • US-ASCII is used for internal names
  • UTF-8 for text that might be displayed to the user.

ssh-rsa is an identifier not arbitrary data... and after my judgment an internal name... so it should be US-ASCII.

b'ssh-rsa' will produce the desire result only on ASCII based system... on EBCDIC system it will generate an invalid result.


I will think about fromString/toString in the morning and will come back with details.

Many thanks for working on this.

Thanks!

comment:11 Changed 2 years ago by Adi Roiban

Keywords: review removed
Owner: set to hawkowl

I am back. Since the current code does not do any explicit decoding/encoding I think that having sshType as byte is OK... but unfortunate.


************* Module twisted.conch.ssh.keys
C0301:529,0: Line too long (80/79)

in

BadKeyError("unknown key type %s" % (kind,))

kind is bytes but is interpolated as a text.

there are other similar usages.

Is that ok?


def sshType(self):

this needs a return and rtype api-doc markup.


hexiv = ''.join(['%02X' % ord(x)

should this be ?

hexiv = b''.join(['%02X' % ord(x)

Please check latest comment , fix and merge.

Thanks!

comment:12 Changed 2 years ago by hawkowl

Resolution: fixed
Status: newclosed

(In [46403]) Merge conch-ssh-keys-py3-7998-3: Port twisted.conch.ssh.keys to Python 3

Author: hawkowl Reviewer: adiroiban Fixes: #7998

comment:13 Changed 2 years ago by Thijs Triemstra

Was it intentional to remove the __version__ and version attributes from conch: http://twistedmatrix.com/trac/changeset/46403/trunk/twisted/conch/__init__.py?

comment:14 Changed 2 years ago by Adi Roiban

Resolution: fixed
Status: closedreopened

Good question.

I think that the version removal is not related to py3 port and was also removed without any deprecation warning and without any motivation.

I think that it was not intentional. I was my fault for not raising this during the review.

Many thanks for the comment.

comment:15 Changed 2 years ago by Thijs Triemstra

Can we reopen and revert the change asap?

comment:16 Changed 2 years ago by Adi Roiban

AFAIK this code was not yet released.

I have reopened the ticket. Not sure who should revert the changes.. and how.

Feel free to submit a patch which reverts the changes.... or maybe we can have a new ticket which just add back the removed version.

Feel free to join #twisted-dev and try to get this ticket on a fast track :)

Thanks!

comment:17 Changed 2 years ago by hawkowl

(In [46433]) Revert r46403: Port twisted.conch.ssh.keys to Python 3

Removed Conch's version, rolling that back.

Reopens: #7998

comment:18 Changed 2 years ago by hawkowl

Keywords: review added
Owner: hawkowl deleted
Status: reopenednew

Reverted on trunk, reverted the unintended changes. I'll make another ticket about the existence of it, as we should remove it later (properly).

Please review, builders spun.

comment:19 Changed 2 years ago by Adi Roiban

Keywords: review removed
Owner: set to hawkowl

Thanks. Please merge :)

Sorry for the bad review.

comment:20 Changed 2 years ago by hawkowl

Resolution: fixed
Status: newclosed

(In [46436]) Merge conch-ssh-keys-py3-7998-3: Port twisted.conch.ssh.keys to Python 3

Author: hawkowl Reviewer: adiroiban Fixes: #7998

comment:21 Changed 2 years ago by SamyCookie

Hi. The auto-generated module 'twisted.conch._version' is not listed in the dist3 file. Is that an intentional omission ?

Note: See TracTickets for help on using tickets.