Opened 5 years ago

Closed 5 years ago

#4266 defect closed fixed (fixed)

Conch ssh client auth order doesn't honour preferredOrder

Reported by: alanfranzoni Owned by:
Priority: normal Milestone:
Component: conch Keywords:
Cc: jesstess, alanfranzoni Branch: branches/userauth-order-4266
(diff, github, buildbot, log)
Author: jesstess Launchpad Bug:

Description

Problem lies in twisted.conch.ssh.userauth.py, class SSHUserAuthClient

when trying to auth, ordering doesn't work as expected; if preferredorder is ["a", "b"] and server return as auth methods , "a"?, the client will try to use, in order, ["x", "a"]. This happens because orderByPreference in ssh_USERAUTH_FAILURE() returns -1 when a method is NOT within preferredOrder, and util.dsu will then put this method to the beginning of the resulting canContinue list.

The test wasn't really suited to check for that error because the auth method "afirstmethod" did not exist on the client, and this just made the client skip that method because it couldn't fine auth_afirstmethod, and go on with the password.

The attached patch contains a modifed test which triggers the issue and a proposed fix.

Attachments (2)

twisted_ssh_userauth.patch (1.4 KB) - added by alanfranzoni 5 years ago.
ssh client userauth correct preferredOrder patch
userauth-2.patch (2.2 KB) - added by alanfranzoni 5 years ago.

Download all attachments as: .zip

Change History (13)

Changed 5 years ago by alanfranzoni

ssh client userauth correct preferredOrder patch

comment:1 Changed 5 years ago by exarkun

  • Keywords review added
  • Owner z3p deleted

comment:2 Changed 5 years ago by jesstess

  • Owner set to jesstess

comment:3 Changed 5 years ago by jesstess

  • Author set to jesstess
  • Branch set to branches/userauth-order-4266

(In [28160]) Branching to 'userauth-order-4266'

comment:4 Changed 5 years ago by jesstess

(In [28161]) Apply twisted_ssh_userauth.patch by alanfranzoni.

refs #4266

comment:5 Changed 5 years ago by jesstess

  • Cc jesstess added
  • Keywords review removed
  • Owner changed from jesstess to alanfranzoni

Thanks for the patch, alanfranzoni. The patch looks good and the branch passes on my test machine. A few comments:

  • orderByPreference would benefit from a doc string, including epytext markup for it's parameter, and ssh_USERAUTH_FAILURE could also use markup for it's parameters and return value. Docstrings and markup are described in the coding standard.
  • Since userauth.py and test_userauth.py are getting modified, can you bump the copyrights on them?
  • Can you add a NEWS file for the defect, as described in ReviewProcess?

Branch userauth-order-4266 now exists for this change. Please make subsequent diffs against this branch. Thanks again!

comment:6 Changed 5 years ago by alanfranzoni

  • Keywords review added
  • Owner changed from alanfranzoni to jesstess

Here it is, I hope I have understood the way the conch client works :-)

Changed 5 years ago by alanfranzoni

comment:7 Changed 5 years ago by jesstess

  • Keywords review removed

comment:8 Changed 5 years ago by jesstess

(In [28170]) Apply userauth-2.patch and add a bit up epytext markup.

refs #4266

comment:9 Changed 5 years ago by jesstess

  • Cc alanfranzoni added

comment:10 Changed 5 years ago by jesstess

  • Resolution set to fixed
  • Status changed from new to closed

(In [28171]) Merge userauth-order-4266

Author: alanfranzoni
Reviewer: jesstess
Fixes: #4266

twisted.conch.ssh.SSHUserAuthClient now honors preferredOrder when
authenticating.

comment:11 Changed 4 years ago by <automation>

  • Owner jesstess deleted
Note: See TracTickets for help on using tickets.