Opened 5 years ago

Closed 5 years ago

#3984 defect closed fixed (fixed)

twisted.conch.checkers.SSHPublicKeyDatabase can't find user's private key

Reported by: esteve Owned by:
Priority: normal Milestone:
Component: conch Keywords:
Cc: bs1984@… Branch: branches/key-database-3984
(diff, github, buildbot, log)
Author: bshi Launchpad Bug:

Description

User's private key can't be found because SSHPublicKeyDatabase#checkKey doesn't expand user's home directory properly. Adding a print statement or a call to log.msg shows that, when user foo tries SSH to a Conch service, SSHPublicKeyDatabase looks for its private key in /home/foo/foo/.ssh

Attachments (3)

3984-sshpublickeydatabase-userdir-lookup.patch (3.5 KB) - added by esteve 5 years ago.
3984-sshpublickeydatabase-userdir-lookup-2.patch (7.1 KB) - added by esteve 5 years ago.
27989-Fix-key-checker.patch (3.5 KB) - added by bshi 5 years ago.
Bug fix against SVN 27989

Download all attachments as: .zip

Change History (19)

comment:1 Changed 5 years ago by esteve

  • Owner z3p deleted

comment:2 Changed 5 years ago by esteve

  • Keywords review added

This patch changes SSHPublicKeyDatabase#checkKey to use ~user in os.path.expanduser to return, e.g., /home/user, instead of /home/user/user. Tests were also changed to use the full path to the ssh directory ($HOME/.ssh).

I've also pushed a branch to Launchpad:

https://code.launchpad.net/~esteve/twisted/3984-sshpublickeydatabase-userdir-lookup

comment:3 Changed 5 years ago by glyph

  • Keywords review removed
  • Owner set to esteve

Thanks for noticing this and contributing the patch.

It appears to fix the issue and at least nominally test it, but it seems like it would be very easy to change the code or test a little bit so that it was no longer covered.

  1. Some test's docstring somewhere should talk about how the .ssh directory is located. The docstring of SSHPublicKeyDatabase itself should also be a little more specific.
  2. MockOS.expanduser should change so that it gives something different than a static path regardless of its input. You can currently replace the input to the expanduser call in checkKey to anything at all (even a non-string) and the test still passes.
  3. self.sshDir in the test case should be set up by doing something like FilePath(self.mktemp()).child(".ssh"); avoid the use of os.path when you can.

comment:4 Changed 5 years ago by esteve

  • Status changed from new to assigned

comment:5 Changed 5 years ago by esteve

  • Keywords review added

Thanks Glyph for you review.

I just uploaded a new version of the patch and pushed its changes to Launchpad. I changed MockOS to return different user home directory depending on the argument passed. I haven't added tests for MockOS#expanduser, because I preferred to know if I was on the right track before going any further.

I also changed the test suite to use FilePath instead of os.path.

comment:6 Changed 5 years ago by exarkun

  • Owner esteve deleted
  • Status changed from assigned to new

comment:7 Changed 5 years ago by exarkun

  • Owner set to exarkun
  • Status changed from new to assigned

comment:8 Changed 5 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to esteve
  • Status changed from assigned to new
  1. MockOS.users needs to be an instance attribute. As implemented now, every MockOS instance shares a single users dict. That will have some Consequences. :)
  2. Since SSHPublicKeyDatabase.checkKey already uses the pwd module, maybe it would be simpler to implement and test a solution based on that. The pwd module will tell you the home directory of a user, and this won't involve string parsing or as much path mangling to fake in the tests. Plus, there's twisted.python.fakepwd to make it extra simple.
  3. In the test setup, if you set self.mockos.path to FilePath(self.mktemp()) first and construct the rest of the paths based on that, you won't need sshDir.parent().parent().parent() :)
  4. I'm puzzled by the changes to MockOS.setuid and MockOS.setgid. What purpose do they serve?

Changed 5 years ago by bshi

Bug fix against SVN 27989

comment:9 Changed 5 years ago by bshi

  • Keywords review added
  • Owner esteve deleted

Anyone working on this? Please review proposed patch to resolve this attached as 27989-Fix-key-checker.patch which incorporates the suggestions in the most recent review.

After skimming the OpenSSH documentation, refactored checkKey() slightly and added a new class method to get a list of authorized_key files. Idea here is that API users can override for custom configurations.

comment:10 Changed 5 years ago by bshi

  • Cc bs1984@… added

comment:11 Changed 5 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/key-database-3984

(In [27990]) Branching to 'key-database-3984'

comment:12 Changed 5 years ago by exarkun

  • Author changed from exarkun to bshi
  • Keywords review removed
  • Owner set to exarkun
  • Status changed from new to assigned

Thanks for picking this up bshi. The patch looks great. I'm going to make a few minor adjustments and commit it.

comment:13 Changed 5 years ago by exarkun

(In [27992]) two blank lines between methods; update copyright header; convert todo in docstring to a note about the implementation's behavior; use FilePath instead of os.path

refs #3984

comment:14 Changed 5 years ago by exarkun

(In [27993]) news

refs #3984

comment:15 Changed 5 years ago by exarkun

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

(In [27994]) Merge key-database-3984

Author: bshi
Reviewer: exarkun
Fixes: #3984

Fix SSHPublicKeyDatabase so that it looks for authorized_keys files in
the right place.

comment:16 Changed 4 years ago by <automation>

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