10241-fix-ssh-key-negotiation

[cocobear]

Scope and purpose

I'm using SSHServerTransport connect to my server, it failed, then I found there is a problem in ssh key negotiation.

Take a look at these piece of code in SSHTransportBase:

        # These are the server directions
        outs = [encSC, macSC, compSC]
        ins = [encCS, macSC, compCS]

It using macSC twice and macCS never, the ins variable should contains client side algs, macs, but it was initialized with macSC(which represent server side mac).

Contributor Checklist:

• The associated ticket in Trac is here: https://twistedmatrix.com/trac/ticket/10241#ticket

• I ran tox -e lint to format my patch to meet the Twisted Coding Standard

• I have created a newsfragment in src/twisted/newsfragments/ (see: News files)

• The title of the PR starts with the associated Trac ticket number (without the # character).

• I have updated the automated tests and checked that all checks for the PR are green.

• I have submitted the associated Trac ticket for review by adding the word review to the keywords field in Trac, and putting a link to this PR in the comment; it shows up in https://twisted.reviews/ now.

• The merge commit will use the below format
  The first line is automatically generated by GitHub based on PR ID and branch name.
  The other lines generated by GitHub should be replaced.

[cocobear]

ok

[adiroiban]

Thanks @cocobear for the update.

The changes looks good. Even if there are no automated tests, I think that this can be merged.

But to merge this pr, it needs a newsfragment which will announce the change for the next public release.

Let me know if you need help with creating or writing the text for the newsfragment file.

Thanks again!

[cocobear]

Thanks @cocobear for the update.

The changes looks good. Even if there are no automated tests, I think that this can be merged.

But to merge this pr, it needs a newsfragment which will announce the change for the next public release.

Let me know if you need help with creating or writing the text for the newsfragment file.

Thanks again!

add a newsfragment .

[cocobear]

@adiroiban @graingert When will this pr can be merged?

[adiroiban]

Sorry for the delay. I am not very happy with the release note information.
The current text should like a commit message and not a release note message.

Also, since there are no automated tests, can you provide an example of a manual test?

The current code look like an obvious mistake and your fix looks right... but is best to double check it with a test.
At least with a manual tests if we don't have automated tests

Cheers

[adiroiban]

By reading the release notes, someone can decide if they are affected by a certain bug and they should know whether an upgrade is required or not.

For the release notes, maybe something like this

SSHTransportBase.ssh_KEXINIT now uses the remote peer preferred MAC list for negotiation. In previous versions  it was only using the local preferred MAC list.

[cocobear]

gg

manual test

What kind of manual test should I provide? can you give me a example link?

[cocobear]

@adiroiban update newsfragment

[twm]

I've kicked the builds a few times but it seems that this is consistently a problem on pypy-3.6-alldeps-nocov-posix. I suggest investigating that failure, which may prove to be a PyPy issue that should be reported upstream, or perhaps irrelevant as we will eventually drop 3.6.

(For the record I do not understand what this PR changes so I am not reviewing it. I am merely pushing automated buttons.)

[adiroiban]

Here is the original test run for latest trunk - https://github.com/twisted/twisted/runs/4235367874?check_suite_focus=true

I have re-triggered it now at I got the same failures at https://github.com/twisted/twisted/runs/4421611362?check_suite_focus=true

So this PR is OK. I will see if I can merge it and we can look at the py3.6 issues in a separate PR.

At the same time py3.6 is EOL on 23 Dec 2021 so I a not sure how we are going to support pypy3.6... I guess that is best to just drop support for pypy3.6 and focus all the remaining effort on pypy3.7

[adiroiban]

All good. Thanks.

It would have been nice to have a tests for this.
But I don't have time for that now and I think that is safe to merge it as it it.

Thanks!