Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#7717 enhancement closed fixed (fixed)

Support for diffie-hellman-group14-sha1 key exchange in conch

Reported by: Ian Moore Owned by: Colin Watson
Priority: normal Milestone:
Component: conch Keywords:
Cc: z3p, Adi Roiban, Antoine Martin, mkuron, Colin Watson Branch: branches/ssh-diffie-hellman-group14-sha1-7717-4
branch-diff, diff-cov, branch-cov, buildbot
Author: imoore76, adiroiban

Description

Hello,

Some sshd implimentations leave only diffie-hellman-group14-sha1 enabled. I would like to see this supported by conch.

Attachments (12)

7717.patch (13.7 KB) - added by Ian Moore 3 years ago.
7717-v2.patch (12.8 KB) - added by Ian Moore 3 years ago.
7717-v3.patch (14.2 KB) - added by Ian Moore 3 years ago.
7717-v4.patch (25.3 KB) - added by Ian Moore 3 years ago.
7717-v5.patch (25.2 KB) - added by Ian Moore 3 years ago.
7717-v6.patch (24.6 KB) - added by Ian Moore 3 years ago.
7717-v7.patch (26.8 KB) - added by Colin Watson 2 years ago.
7717-v8.patch (27.5 KB) - added by Colin Watson 2 years ago.
7717-v9.patch (28.1 KB) - added by Colin Watson 2 years ago.
7717-v8-to-v9.patch (7.6 KB) - added by Colin Watson 2 years ago.
7717-v10.patch (28.3 KB) - added by Colin Watson 2 years ago.
7717-v8-to-v10.patch (7.8 KB) - added by Colin Watson 2 years ago.

Download all attachments as: .zip

Change History (60)

comment:1 Changed 3 years ago by DefaultCC Plugin

Cc: z3p added

comment:2 Changed 3 years ago by Ian Moore

I have already added this to a local copy of twisted and I'm going to contribute the code to the project. Just going through the process here.

comment:3 Changed 3 years ago by Adi Roiban

Cc: Adi Roiban added

Great. Looking forward for the review.

For any question regarding the review process, don't hesitate to ask for help!

You can also review #7672 which is for diffie-hellman-group-exchange-sha256

comment:4 Changed 3 years ago by Ian Moore

Keywords: diffie-hellman-group-exchange-sha256 added
Summary: Support for diffie-hellman-group14-sha1 key exchange in conchSupport for diffie-hellman-group14-sha1 and diffie-hellman-group-exchange-sha256 keyex algos in conch

Working on a merge of the two incorporating suggestions of reviewer in #7672.

comment:5 Changed 3 years ago by Adi Roiban

Try to not do to many thinks in a single ticket / patch, as it will complicate the review process... but is ok to use the suggestions from the previous code :)

comment:6 Changed 3 years ago by Jean-Paul Calderone

Summary: Support for diffie-hellman-group14-sha1 and diffie-hellman-group-exchange-sha256 keyex algos in conchSupport for diffie-hellman-group14-sha1 key exchange in conch

Try to not do to many thinks in a single ticket

Yes indeed. Specifically, try to do exactly one thing in one ticket. :) If you need "and" to describe your change it might be too broad. Please leave the sha256 changes for #7672.

comment:7 Changed 3 years ago by Ian Moore

Keywords: diffie-hellman-group-exchange-sha256 removed

ok

Changed 3 years ago by Ian Moore

Attachment: 7717.patch added

comment:8 Changed 3 years ago by Ian Moore

Keywords: review added
Owner: Ian Moore deleted

Patch attached. Please review.

comment:9 Changed 3 years ago by Adi Roiban

Keywords: review removed
Owner: set to Ian Moore

Many thanks for the patch. Changes looks good... just a few comments to make them perfect :)

Please note that I am a junior reviewer and that my comments might be wrong.


For twisted/conch/ssh/transport.py

  • While dhPrime and dhGenerator instance variables should be documented, I think that we can make them private... is there any reason for them to be public?
  • I think that you will also need to document their types in docstring.

Generoator typo.

  • Also, try not to use short names like algo... as we will end up with all kind of strange abbreviation. That text is written once, but read many times. I think it is worth dedicated a little bit of extra time for excellent docstrings.
  • This comment does not say much and I can infer it by reading the code.
    # Set up diffie hellman key exchange 
    

maybe we can leave without this comment... there is always the risk to forget updating the comment together with the code and then you end up with a WTF comment.

  • At line 973 there is an extra newline... do we need it?
  • At line 1192 I think that there sould be a space after the comma ['diffie-hellman-group1-sha1', 'diffie-hellman-group14-sha1']:

For twisted/conch/test/test_transport.py

  • For test_KEXDH_INIT_GROUP14 I see a lot of code duplication with GROUP1. Maybe that you can extract the common parts into a function, with changes parts as parameters
  • For test_KEXINIT_group14, I find it hard to read the test and understand what is going on. I prefer to have the lines from a test split in Arrange,Act,Assert pattern, in this way it much easier to read what exactly is tested and what are the preconditions.
  • For test_KEXINIT_group14 the test is similar to test_KEXINIT_group1... the docstring is wrong.
  • For test_KEXINIT_group14, i know that it is a copy of test_KEXINIT_group1 but reading self.proto.dataReceived(self.transport.value()) i have a WTF comment... what is actually written there? It is all a big mystery. What is that '\x99' * 64. I think that some comments might help here

Thanks again for the patch!

As described in rfc 4253 this is a MUST so this ticket is much appreciated!

Changed 3 years ago by Ian Moore

Attachment: 7717-v2.patch added

comment:10 Changed 3 years ago by Ian Moore

Keywords: review added

Hello,

Thank you for the review!


For twisted/conch/ssh/transport.py

  • I'm not aware of any mechanism to make dhPrime and dhGenerator private. I think you are referring to DH_PRIME, DH_PRIME14, etc. module variables. DH_PRIME was already a module level variable and referred to by test_transport.py and test_ssh.py. I just left DH_PRIME a module level variable and added DH_PRIME_14 et al at module level to be consistant. I do agree with the original author in DH_PRIME etc. being module level as they really should be constants if python supported constants. Please let me know your thoughts on this. If I am misunderstanding, I appologize and please elaborate on the public / private comment. I am somewhat new to python.
  • dhPrime and dhGenerator types (Long) are now documented
  • Generoator typo fixed :)
  • "algo" expanded to "algorithm"
  • Comment regarding diffie-helman key exchange setup removed.
  • Newline at 973 removed
  • Space after comma at 1192 added

For twisted/conch/test/test_transport.py

  • test_KEXDH_INIT_GROUP1 and test_KEXDH_INIT_GROUP14 have been modified as described so that only one function is used with parameters changing the tests.
  • test_KEXINIT_group14 and test_KEXINIT_group1 have been consolidated into test_KEXINIT with parameters modifying the tests. I have tried to split the logic as you have described.
  • I don't believe the docstring is wrong. It is a key exchange test like test_KEXINIT_groupexchange except using the group1 or group14 algorithm. Please elaborate on what you might like to see here.
  • Re test_KEXINIT_group14 & test_KEXINIT_group1 comments - .dataReceived(self.transport.value()) is replicating the client recieving data from the server. The line containing '\x99' * 64 is some low-level ssh protocol umm... stuff :), is the level of detail describing this really needed? It does not align with the level of comment detail in the rest of the test module. Though I will add it if required.

Thank you for all your comments!

comment:11 Changed 3 years ago by Adi Roiban

Changes looks good.

I have also did a quick test with OpenSSH and it looks good.


They still need a review from a core developer.

A core developer would also need to trigger a full buildbot test for this patch.

I'm not aware of any mechanism to make dhPrime and dhGenerator private

Just name then _dhPrime and _dhGenerator, and they are private, from the point of view of Twisted public API :)

I don't believe the docstring is wrong.

True . My bad.


Not sure if test_KEXINIT should be written in this way, or have a dedicated method... but I don't think that this needs to be changed.

I know that comments in the other tests are not great, but this is not an excuse to continue writing mysterious tests :).

Regarding .dataReceived(self.transport.value()) I find it hard to know what exact value is written... what value is currently on the transport?

Maybe it should be extracted into a method named self.gotClientInitRequest()

Now for the line containing '\x99' * 64. Is that stuff important for the test? Does the method under test has any influence over that stuff? And in case it is important for the test, does it help to know that it is some low level stuff?

From what I can see (by reading the souce code, not the test) I think that this test that DH private key is generated with a value which can be represented with a single byte ? 4 bytes for the length, 1 byte for the value and remaining values just padding. Maybe this can also be extracted into a method and add all this info in the docstring.

It is not required to add comments for old code, so comments regarding the tests are just "nice to have".

There are no tests for the test code, so from my point of view, the tests should be easy to read and reduce any risk of writing tests which are wrong.

Thanks again!

comment:12 Changed 3 years ago by Ian Moore

Owner: changed from Ian Moore to Adi Roiban

Your feedback makes sense. Before I begin changing some things, I wanted to check with you.

  • dhPrime and dhGenerator will be renamed to _dhPrime and _dhGenerator.
  • You're probably right. test_KEXINIT should probably not be written this way. I will move it to a helper and call it from test_KEXINIT_group1 and test_KEXINIT_group14 methods.
  • I will also attempt to comment this helper function the best I can. Should I lean towards inline comments for this? E.g.
    def _kexInitDH(kexAlgorithm, dhPrime, dhGenerator):
        """
        Test that a KEXINIT packet with a group1 or group14 key exchange
        results valid key setup and that diffie-hellman prime and
        generator are set correctly.
        """
        self.proto.supportedKeyExchanges = [kexAlgorithm]

        # Imitate reception of server key exchange request contained
        # in data returned by self.transport.value()
        self.proto.dataReceived(self.transport.value())

        # Private key should be appropriate length and padded
        self.assertEqual(common.MP(self.proto.x)[5:], '\x99' * 64)

        # Data sent to server should be a transport.MSG_KEXDH_INIT
        # message containing our public key
        self.assertEqual(self.packets,
                          [(transport.MSG_KEXDH_INIT, self.proto.e)])

        # self.proto._dhPrime and self.proto._dhGenerator should be
        # set correctly based on the negotiated key exchange algorithm
        self.assertEqual(self.proto._dhPrime, dhPrime)
        self.assertEqual(self.proto._dhGenerator, dhGenerator)


    def test_KEXINIT_group1(self):
        """
        _kexInitDH test for a group1 key exchange.
        """
        self._kexInitDH('diffie-hellman-group1-sha1', transport.DH_PRIME,
                transport.DH_GENERATOR)

.. or should this be summed up in the method comment?

comment:13 Changed 3 years ago by Adi Roiban

Owner: changed from Adi Roiban to Ian Moore

I don't think that self.assertEqual(self.proto._dhPrime, dhPrime) need an inline comment. This is the main part of the test, the test's docstring should describe this.

Inline comments are ok, but even better is create a Test Specific Language where the comment and code is replaced with a method call. The method name (and method docstring) should document all the low level actions.

For me the following comment is still close to a WTF momment.

# Private key should be appropriate length and padded
self.assertEqual(common.MP(self.proto.x)[5:], '\x99' * 64)

'appropriate length` is very generic. While the call to packMP gives a hing about how padding is tested, I find it hard to discover how length is tested.

Now... documented this might be hard, so there is no need to fix it here.

Thanks!

comment:14 Changed 3 years ago by Jean-Paul Calderone

Author: exarkun
Branch: branches/ssh-diffie-hellman-group14-sha1-7717

(In [43487]) Branching to 'ssh-diffie-hellman-group14-sha1-7717'

comment:15 Changed 3 years ago by Jean-Paul Calderone

(In [43488]) Apply 7717-v2.patch

refs #7717

comment:16 Changed 3 years ago by Ian Moore

Actually it appears that it only tests length. I thought it was padded based on the '\x99' repeat, but this is just a result of monkey patching randbytes.secureRandom in the test module. I'm not even sure this is really needed.

randbytes.secureRandom is patched to return '\x99' * len passed to function.

def secureRandom(len):
    """
    Return a consistent entropy value
    """
    return '\x99' * len

So in transport.py, you have:

self.proto.x = common.MP(_generateX(randbytes.secureRandom, 512))

... the whole thing could be shortened to something like..

self.assertEqual(common.MP(Util.number.bytes_to_long('\x99' * 64))[5:],
   '\x99' * 64)

As far as I can tell, this assertion will always be true. I'm not sure what the original author intended to test. _generateX must be passed 512 to get 64 bytes. Other than that, I don't know.

Changed 3 years ago by Ian Moore

Attachment: 7717-v3.patch added

comment:17 Changed 3 years ago by Jean-Paul Calderone

Branch: branches/ssh-diffie-hellman-group14-sha1-7717branches/ssh-diffie-hellman-group14-sha1-7717-2

(In [43489]) Branching to 'ssh-diffie-hellman-group14-sha1-7717-2'

comment:18 Changed 3 years ago by Jean-Paul Calderone

(In [43490]) Apply 7717-v3.patch

refs #7717

comment:19 Changed 3 years ago by Jean-Paul Calderone

Author: exarkunimoore76
Keywords: review removed

Thanks imoore76 and adiroiban.

A few more comments:

  1. twisted/conch/ssh/transport.py
    1. The default values for _dhPrime and _dhGenerator are strings. This will be confusing if they ever get used since the actual, useful values for these attributes are integers.
    2. Please document the types of things using @type ... (rather than using prose in the @ivar)
    3. "Generator" has a particular meaning in Python. Please make the docs clear (and perhaps rename _dhGenerator) that this thing has nothing to do with the usual idea of Python generators and is about something else.
    4. Since _dhPrime and _dhGenerator are tightly coupled, perhaps these values should be represented by a single object. For example, there could be DiffieHellmanGroup1 and DiffieHellmanGroup14 objects with prime and generator atributes. Or a ModularExponentialGroup class that accepts prime and generator initializer arguments (this could eventually have methods for the various ops, _MPpow, etc, supplying the right prime and generator values).
    5. It would be nice to define strings like 'diffie-hellman-group1-sha1' as constants using twisted.python.constants.Values and then use the resulting symbolic names throughout - instead of repeatedly spelling out the string literal.
    6. Similarly, instead of writing ... in ['diffie-hellman-group1-sha1','diffie-hellman-group14-sha1'] throughout, it would be nice to define the groups that are supported in one place. If the MODP DH group parameters are defined using objects as suggested above, then the single canonical definition might be a dictionary mapping names of groups to those objects. This provides a natural way to select the right group parameters as well (look up the requested kexAlg in that dict).
    7. There's a mis-indented line at the end of the docstring for _ssh_KEXDH_INIT.
  2. twisted/conch/test/test_transport.py
    1. The parameters to _kexDHInit should be documented.
    2. Same for _kexInitDH.
    3. Ideally, test methods should make one assertion. Multiple calls to assertEqual result in a test which might have multiple problems but can only report one (the first assertEqual to fail will stop the test).
    4. I made a few whitespace fixes in r43491 you may want to note for future patches.
  3. It would be nice to formalize the OpenSSH interop testing. Conch has some existing tests that run an OpenSSH - mostly in twisted.conch.test.test_conch. These are not particularly wonderful tests but it is nice to have automated test coverage for the interop. Using the -o KexAlgorithms=... flag to ssh I think it should be possible to explicitly demostrate Conch and OpenSSH are working together with each of these groups.
  4. I checked the v3 patch into a branch. Feel free to generate future patches against that branch instead of against trunk.

comment:20 Changed 3 years ago by Ian Moore

Owner: changed from Ian Moore to Jean-Paul Calderone

How do you feel about a KexAlgorithms class in transport.py with a private kex algorithm map dict and @classmethods like getDHPrime(kexAlgorithm), getSupportedAlgorithms(), getDHGenerator(..), etc., then calling KexAlgorithms.getDHPrime(self.kexAlgo) etc.. in the transport classes? This moves all kex algorithm definition to its own class and lends itself to easily adding diffie-hellman-group-exchange-sha256 support in the future, which I hope to do.

Changed 3 years ago by Ian Moore

Attachment: 7717-v4.patch added

comment:21 Changed 3 years ago by Ian Moore

Keywords: review added

So here's what I did:

twisted/conch/ssh/transport.py

  • Removed DH_* module level variables
  • Created KexAlgorithms class with key exchange algorithm definitions and supporting static methods
  • Removed _dhPrime and _dhGenerator from SSHTransportBase and replaced references with lookup functions from KexAlgorithms class
  • Removed all references that use a specific key exchange algorithm name from code and comments. Logic forking is now based on a key exchange attributed called fixedGroup.
  • SSHTransportBase.supportedKeyExchanges is now generated from KexAlgorithms.getSupportedKeyExchanges to keep all key algorithm names and definitions in one place.

twisted/conch/test/test_transport.py

  • Updated docs as suggested
  • References to transport.DH_PRIME changed to use transport.KexAlgorithms methods
  • References to transport instance _dhPrime and _dhGenerator vars changed to use transport.KexAlgorithms methods
  • Only one assertion per test

twisted/conch/test/test_ssh.py

  • References to transport.DH_PRIME changed to use transport.KexAlgorithms methods

twisted/conch/test/test_conch.py

  • Added OpenSSH interop tests for key exchange algorithms as suggested.

The only place actual key exchange algorithm names are used are in the tests and their definitions. I guess I could have written it so that SSHTransportBase has a single kexAlgorithmObj object. There's 10 other ways to do all of this - I just chose the one I chose :)

comment:22 Changed 3 years ago by Adi Roiban

Just a few comments regarding the new KexAlgorithms class/container.

Do you like 'KexAlgorithms' name? I always find it hard to read and see if this is not KeyAlgorithms ... why not go for the full KeyExchangeAlgorithms?

KexAlgorithms should inherit from object, so that we can keep compatibility with Python 2.7

Key exchange algorithm names can be misleading I don't understand this docstring... and it does not have a full stop.

isFixedGroup, getDHPrime, getDHGenerator:

  • All make use of the private KexAlgorithms class members. Why not make them class methods?
  • All take kexAlgo as arumment. Why not make them instace methods?
  • Why do we need them as static?

In this way, instead of KexAlgorithms.getDHGenerator(self.kexAlg) we would have only self.kexAlg.getDHGnerator() or just self.kexAlg.dh_generator where dh_generator could be a property based on getDHGenerator and where self.kexAlg = KexAlgorithms(NEGOCIATED_ALGORITHM)

I like the way _kex_algo_map is structured, but I don't know what to say about the missing primes from diffie-hellman-group-exchange-sha1 ... this algorithm uses factory.getDHPrimes(bits).

so right now we have KexAlgorithm.getDHPrime() and factory.getDHPrimes(bits) ... which might be confusing. It would be nice to get primes for all algorithm from a single place. No need to do this in this patch but it would be good to keep this in mind.

Thanks!

Changed 3 years ago by Ian Moore

Attachment: 7717-v5.patch added

Changed 3 years ago by Ian Moore

Attachment: 7717-v6.patch added

comment:23 Changed 3 years ago by Ian Moore

Hello,

  • I'd rather like to keep the KexAlgorithms name. kex and key are common throughout the code and openssh config.
  • This class now inherits from object.
  • I removed the comment. I remember that I was going to type something, but got lost in thought / work :)
  • You make a good point about these methods being @class vs @static. I've changed them to @classmethod
  • kexAlg is set as a string like keyAlg throughout the code and it makes sense to me this way. I guess I could have added some instance var like kexAlgObj or kexAlgInstance, but I'm not sure how many changes I can make before this ticket becomes + " .. and refactor conch/ssh/transport.py".
  • The primes are not present for diffie-hellman-group-exchange-sha1 because in a key exchange negotiation using this algorithm a prime/generator group is chosen based on a requested size. As designed, this is only present in OpenSSHFactory where getPrimes() calls another method to read from OpenSSH's moduli file which contains this data. If getDHPrime is called when this algorithm is used, something is wrong. I've added a brief explanation regarding this to the KexAlgorithms docstring.
  • Seeing that multiple calls to KexAlgorithms.getDHPrime() were a little too verbose, factory.getDHPrime() returns a tuple, and both transport classes already held the generator and prime in instance variables self.g and self.p, I've changed KexAlgorithms.getDHPrime() to return a tuple and these are stored in self.g and self.p respectively - just like when factory.getDHPrime() is used.

Thank you

comment:24 Changed 3 years ago by Ian Moore

Keywords: KeyExchange diffie-hellman-group14-sha1 reviewreview KeyExchange diffie-hellman-group14-sha1 review

Hello,

I'm just wondering what the status of this is and when I may expect to see it committed to trunk.

Thank you! --Ian

comment:25 Changed 3 years ago by Ian Moore

Keywords: KeyExchange diffie-hellman-group14-sha1 reviewreview KeyExchange diffie-hellman-group14-sha1 review

Hello,

I'm just wondering what the status of this is and when I may expect to see it committed to trunk.

Thank you! --Ian

comment:26 Changed 3 years ago by Adi Roiban

Sorry for the delay. I was expecting a senior developer to finalize the review.

Many thanks for the work on this!

I think that changes look good, but they need a senior developer to review and merge them.


I still think that it should be better to instantiate a KexAlgorithms object with the negotiated algorithm... as commented before and forget about self.kexAlg but I agree that this can be done in a separate ticket. I am willing to contribute with that patch :)

# In _kexDHInit we would have:

self.kexAlgorithm = KexAlgorithms(kexAlgorithm)

# and then used as
self.kexAlgorithm.getDHPrime()
self.kexAlgorithm.isFixedGroup()


Just a side not while reading existing code:

Maybe supportedKeyExchanges should be refactored into a getter as it should not have a big performance impact.

@property
def supportedKeyExchanges(self):
   return KexAlgorithms.getSupportedKeyExchanges(self.factory)

I find it much better than the hack from SSHFactory protocol builder.


Just a side note while doing the review and reading the existing code.

As a separate ticket, maybe self.kexAlg and self.supportedKeyExchanges should be made private as I don't see that they are used by any external objects and I think that they should be private stuff without allowing arbitrary access to them.


I would not consider OpenSSH as a state of the art project in term of variable names, API design, code readability ... and test coverage.

You write a code once and read it multiple times :)


For the side notes, if you find them valid, I would volunteer to work on the patch.


Thanks!

comment:27 Changed 3 years ago by Adi Roiban

Some test are failing... see buildbot results... in particular

https://buildbot.twistedmatrix.com/builders/debian6-x86_64-py2.6/builds/742

https://buildbot.twistedmatrix.com/builders/debian-easy-py2.6-epoll/builds/2856/steps/epoll/logs/test.log

the failure message and log is not of great help

[ERROR]
Traceback (most recent call last):
Failure: twisted.conch.error.ConchError: ('exit code was not 0: 255', None)

twisted.conch.test.test_conch.OpenSSHKeyExchangeTestCase.test_DH_GROUP1
twisted.conch.test.test_conch.OpenSSHKeyExchangeTestCase.test_DH_GROUP14
twisted.conch.test.test_conch.OpenSSHKeyExchangeTestCase.test_DH_GROUP_EXCHANGE_SHA1

Same tests pass on other systems... ex

https://buildbot.twistedmatrix.com/builders/trusty64-py2.7-poll/builds/4491/steps/poll/logs/stdio

They also pass on my Ubuntu 14.04

So it might be due to different versions of OpenSSH

We will need to dig deeper to see what went wrong.

comment:28 Changed 3 years ago by Antoine Martin

Cc: Antoine Martin added

comment:29 Changed 3 years ago by mkuron

Cc: mkuron added

comment:30 Changed 3 years ago by Glyph

Keywords: KeyExchange diffie-hellman-group14-sha1 removed

We use keywords for workflow, not ad-hoc search stuff, so I'm removing the irrelevant keywords.

comment:31 Changed 3 years ago by Glyph

Owner: changed from Jean-Paul Calderone to Ian Moore

Hi imoore76,

Thanks for your contribution, and for your patience.

There are a few blocking issues:

  1. the removal of the public DH_PRIME and DH_GENERATOR constants. The compatibility policy forbids doing this without a deprecation period.
  2. the test failures and coding standard issues on the buildbot need to be addressed
  3. there are a few epytext errors
    1. ; :: and then indent means "fixed-width preformatted code" not "indented region", so the description of _kex_algo_map is a big code block. If you want to do a bulleted list, see the epytext documentation for lists: http://epydoc.sourceforge.net/manual-epytext.html#lists - but, if you go forward to the optional feedback I've got, you can avoid elaborate formatting by just using an instance :).
    2. There's an @rtype missing a following : in getSupportedExchanges
    3. _kexInitDH has spaces before the :s in its @param declarations.

Since this has been through a couple of other reviewers who have already had a lot of suggestions, I'll try to keep my own aesthetic commentary to a minimum. But here are a few points:

  1. It's not quite true (as adiroiban says) that instantiating KexAlgorithm "can be done in a separate ticket". This ticket puts the KexAlgorithms class into the public API, with all its existing classmethods. So please consider that if this is not the API we want, it will need to go through a deprecation cycle.
  2. I can appreciate that refactoring all of conch to properly treat key exchange algorithms as objects and call methods on them is a bunch of unnecessary scope creep. However, the KexAlgorithms class sticks out from the API a bit oddly: it doesn't represent anything, you can't really meaningfully instantiate it, yet it offers a public constructor and users could inherit from it. So it's exposing more API surface which needs to be deprecated.
  3. It's manipulating its own data structures internally as dictionaries full of sets of keys which have to be documented by an ad-hoc convention in a docstring, rather than just saying "It's a L{Foo}".
  4. It makes an already extremely long module (>1600 lines) even longer.

I propose that, to resolve these issues, you instead make a new module, twisted.conch.ssh.kex which has global functions that KeyAlgorithms now exposes as class methods. Make a new twisted.conch.ssh.kex._Algorithm class, give it preference, fixedGroup, prime, and generator attributes. Making classes with attributes is easier to document, and has bonuses on PyPy, where it will recognize that what you've actually got is a struct, and (if you don't go accessing __dict__ all over the place) automatically optimize its memory layout. It can't necessarily do that for actual dictionaries. Also, it builds towards the class which can eventually be used to refactor the innards of the transport interface later, rather than creating more chaff that just needs to be deprecated to get there.

Again, thanks a lot for your contribution!

comment:32 Changed 3 years ago by Glyph

Keywords: review removed

Oops. Actual review comment above, obviously.

comment:33 Changed 2 years ago by Adi Roiban

Colin Watson has proposed this patch https://github.com/twisted/twisted/pull/57

Changed 2 years ago by Colin Watson

Attachment: 7717-v7.patch added

comment:34 Changed 2 years ago by Colin Watson

Cc: Colin Watson added
Keywords: review added

Since this ticket seems to be stalled, and it's at least arguably a prerequisite for https://twistedmatrix.com/trac/ticket/7672, I decided to pick this up and apply glyph's suggestions. You can grab them from the 7717-v7.patch attachment or from https://github.com/twisted/twisted/pull/57.

Should all the interfaces in twisted.conch.ssh.kex perhaps be underscore-prefixed to indicate that they're internal? And if there's anything else I can help to tidy up, I'm willing.

comment:35 Changed 2 years ago by hawkowl

Owner: Ian Moore deleted

comment:36 Changed 2 years ago by Adi Roiban

Author: imoore76adiroiban, imoore76
Branch: branches/ssh-diffie-hellman-group14-sha1-7717-2branches/ssh-diffie-hellman-group14-sha1-7717-3

(In [45556]) Branching to ssh-diffie-hellman-group14-sha1-7717-3.

comment:37 Changed 2 years ago by Adi Roiban

Here is my review

I have sent the patch to buildbot... test are green with a single exception:

https://buildbot.twistedmatrix.com/builders/twistedchecker/builds/3348/steps/run-twistedchecker/logs/new%20twistedchecker%20errors


Is Paul Swartz the maintainer for the new ssh/kex module? Does Twisted dev process still have the concept of a module maintainer?


Maybe we can just rename kex.py to _kex.py and then all its content is private.

Later, if some methods can be re-used outside of the core Twisted project, we can rename or export them as public.

If whole _kex is private then we can use it in any way without having to care about the deprecation policy and will allow us to refactor it at will :)


getDHPrime return both the prime and generator...maybe it can be renamed to getPrimeAndGenerator.

I find this usage confusing kex.getDHPrime('diffie-hellman-group1-sha1')[1])

maybe we can split this into getDHPrime and getDHGenerator


_DHGroupExchangeSHA1 does not fully implement the _IAlgorithm interface.

Maybe instead of using the 'fixedGroup' flag, we can create 2 interfaces

IFixedKexAlgoritm and IDynamicKexAlgorithm ... and IKexAlgorithm for common attributes.

With these interfaces, in isFixedGroup() we can have retrun IFixedKexAlgoritm.providedBy(_getKex(kexAlgo))


I would have preferd that instead of keeping the self.kexAlg as a string, we would keep an instance of IFixedKexAlgoritm / IDynamicKexAlgorithm

then, instead for if kex.isFixedGroup(self.kexAlg) we could just use if IFixedKexAlgoritm.providedBy(self.kexAlg)


self.proto.factory.getPrimes().get(1024)[0][1]) is also confusing but I don't know how this can be improved. ... but maybe we can live with this as this is only used in tests.


In test_ssh and test_transport I don't understand the docstring for getPrimes


In test_conch I think that we can rename _kexAlgorithmExec into assertExecuteWithKexAlgorithm since this is basically an assertion method.

In test_transport, I don't think there is any advange in using underscore to mark private methods. Maybe _kexDHInit can be renamed into assertKEXDH_INITResponse

The docstrings for test_KEXDH_INIT_GROUP1 and test_KEXDH_INIT_GROUP14 does not add to much new information. Maybe rephrase it as:

KEXDH_INIT requests are processed when diffie-hellman-group1-sha1 key exchange algorithm is requested.

I think that the test's docstring should describe what feature/behaviour is tested without making any reference about how the test is done.

Also, _kexInitDH and _kexDHInit are confusing. Maybe also rename _kexInitDH and update it's docstring. What is a 'valid setup' ?


I am leaving this on the review queue so that glyph can also take a look.

Thanks for working on this!

comment:38 Changed 2 years ago by Adi Roiban

Keywords: review removed
Owner: set to Colin Watson

Hi Colin,

Do you still have time to continue working on this patch?

Since there was more than 1 month without any other review, I think that you can consider the last review valid and try to address the comments and resubmit.

Thanks!

Last edited 2 years ago by Adi Roiban (previous) (diff)

comment:39 in reply to:  37 Changed 2 years ago by Colin Watson

Replying to adiroiban:

Is Paul Swartz the maintainer for the new ssh/kex module? Does Twisted dev process still have the concept of a module maintainer?

I've just removed this, since it seems better than possibly lying.

getDHPrime return both the prime and generator...maybe it can be renamed to getPrimeAndGenerator.

I find this usage confusing kex.getDHPrime('diffie-hellman-group1-sha1')[1])

maybe we can split this into getDHPrime and getDHGenerator

I would prefer to leave this as it is despite the slightly confusing name, because it aligns with the one in twisted.conch.ssh.factory which is already public.

I would have preferd that instead of keeping the self.kexAlg as a string, we would keep an instance of IFixedKexAlgoritm / IDynamicKexAlgorithm

We could, but that seems to imply the kex algorithm interfaces being at least semi-public, doesn't it? I'm not sure that this is worth it.

self.proto.factory.getPrimes().get(1024)[0][1]) is also confusing but I don't know how this can be improved. ... but maybe we can live with this as this is only used in tests.

I at least managed to factor this out a bit.

I'm attaching a v8 patch (where I didn't specifically reply to a review comment, I addressed it). Does this look good enough to land now?

Changed 2 years ago by Colin Watson

Attachment: 7717-v8.patch added

comment:40 Changed 2 years ago by Colin Watson

Keywords: review added
Owner: changed from Colin Watson to Adi Roiban

comment:41 Changed 2 years ago by Adi Roiban

Author: adiroiban, imoore76imoore76, adiroiban
Branch: branches/ssh-diffie-hellman-group14-sha1-7717-3branches/ssh-diffie-hellman-group14-sha1-7717-4

(In [45713]) Branching to ssh-diffie-hellman-group14-sha1-7717-4.

comment:42 Changed 2 years ago by Adi Roiban

Keywords: review removed
Owner: changed from Adi Roiban to Colin Watson

Thanks for the patch and sorry for the delay.

I have applied to trunk and sent it to buildbot.

I think that the patch is in a good shape.... it just needs a little cleanup.


I have also tried to review this in the context of #7672 but since there was no much activity into #7672 in the last year.

I think that the new code is good and it would not be hard to implement #7672 on top of it.

For #7672 we will also need a _kex.getHashMethod(kexAlgo) but this new helper should not be implemented in this patch.


I think that not raising raise error.ConchError('bad kexalg: %s' % self.kexAlg) is a good thing.

We should check for a valid kex algorithm when we set it, not each time we use it.

As a follow up, maybe we should convert kexAlg into a property and have a setter which will raise an error when try to set an unsupported algorithm.


For now we can keep the interfaces private and just use the isFixedGroup helper... later we can expose them. I am happy that for now isFixedGroup is implemented using the interfaces logic :)


I don't like this

self.g, self.p = _kex.getDHPrime(self.kexAlg)

  1. g and p are bad names. we should not use abbreviations but rather try to use full names which describe the usage/purpose of this variable.
  1. In case we really need them as instance variable, they should be private ex self._dhGenerator and self._dhPrimes
  1. Do we realy need to make them instance variables? why not use local variables? just dhGenerator and dhPrimes

I would prefer to go for local variables.


Minor comment.

Can we rename kexes to keyExchanges ?

Can we rename kexAlgo to kexAlgorithm ?... in _kex.py and the new tests?

When we go for abbreviation we end up with kexAlg and kexAlgo ... as a name for the same thing.


dh_generator, dh_prime = self.proto.factory.getPrimes().get(1024)[0]

should be

dhGenerator, dhPrime = self.proto.factory.getPrimes().get(1024)[0]

to comply with Twisted coding standard


Please review my latest comments... and also check the buildbot results.

You can resubmit a patch on top of the ssh-diffie-hellman-group14-sha1-7717-4. Hope that soon we will have a git based workflow.

Looking forward to have it merged.

Many thanks for your work!

Changed 2 years ago by Colin Watson

Attachment: 7717-v9.patch added

Changed 2 years ago by Colin Watson

Attachment: 7717-v8-to-v9.patch added

Changed 2 years ago by Colin Watson

Attachment: 7717-v10.patch added

Changed 2 years ago by Colin Watson

Attachment: 7717-v8-to-v10.patch added

comment:43 Changed 2 years ago by Colin Watson

Keywords: review added
Owner: changed from Colin Watson to Adi Roiban

Indeed, I've been putting off any further work on #7672 until we're finished with this.

As I explained on IRC, these are already documented instance variables, so I don't think we should change them as part of this branch. Also, they do need to be instance variables and not local variables: in the case of group exchange, they include real state rather than being statically derived from the algorithm name.

I've attached both a combined patch and a diff from the current branch. Please could you have another look? Hopefully it's good this time!

comment:44 Changed 2 years ago by Adi Roiban

Keywords: review removed
Owner: changed from Adi Roiban to Colin Watson

Thanks for the patch.

It looks good.

I have applied it and sent it to buildbot.

I have made a few style changes..... twistedchecker still complains about self.p and self.g but we should live with that.


After this is merged, I think that we should deprecate the DH_GENERATOR and DH_PRIME constants.

Do you think think that they are usefull in some other place?

I am talking about this line:

DH_GENERATOR, DH_PRIME = _kex.getDHPrime("diffie-hellman-group1-sha1")

For the new _kex.getDHPrime, why not rename it to _kex.getDHGeneratorAndPrime as it returns both the prime part and the generator part. This is new code (also private) and we should be free to use better names, without having to care about legacy code.


Some minor comments, just to help me understand these changes.... we can refator this later.

Why do we really need _kex.getDHPrime ?

whey not just use ?

kex = _kex.getKex('some-name')
kex.generator
kex.prime

same for isFixedGroup ... why not just use something like:

kex = _kex.getKex('some-name')
kex.fixedGroup  # True or False.

Please check my comment related to _kex.getDHPrime renaming.

Please also check my minor changes https://github.com/twisted/twisted/commit/3fe817cbd9fcd674d8d2522ed5640f95884da569

If you are OK with the renaming I can also do the rename and merge.

Thanks!

comment:45 Changed 2 years ago by Adi Roiban

Resolution: fixed
Status: newclosed

(In [45812]) Merge ssh-diffie-hellman-group14-sha1-7717-4: Support diffie-hellman-group14-sha1 key exchange in conch.ssh.

Authors: cjwatson, imoore76 Reviewer: adiroiban Fixes: #7717

comment:46 Changed 2 years ago by Adi Roiban

Colin was not able to comment due to the spam filter. He confirmed that my changes are OK and they can be merged.

Many thanks for your help.

I am looking forward for reviewing #7672

Thanks!

comment:47 Changed 2 years ago by Glyph

adiroiban: I checked the spam filter and did not find anything. What is colin's username?

Note: See TracTickets for help on using tickets.