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

[esteve]

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

[esteve]

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

[glyph]

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.

[esteve]

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.

[exarkun]

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?

[bshi]

Bug fix against SVN 27989

[bshi]

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.

[exarkun]

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

[exarkun]

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

[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

[exarkun]

(In [27993]) news

refs #3984

[exarkun]

(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.