Opened 8 years ago

Closed 3 years ago

#3701 enhancement closed duplicate (duplicate)

Add a checker for ISSHPrivateKey which parameterizes the location of the keys

Reported by: Jean-Paul Calderone Owned by: Ying Li
Priority: normal Milestone:
Component: conch Keywords:
Cc: Branch: branches/parameterize-sshkey-checker-3701-2
branch-diff, diff-cov, branch-cov, buildbot
Author: cyli

Description

SSHPublicKeyDatabase works for a UNIX account-style system. It contains code which falls into one of two categories, though:

  1. code for finding the authorized keys for a given account
  2. code for checking those keys against the supplied credentials

It would be nice if the former could be parameterized somehow, allowing the latter to be re-used. For example, this would allow a simple in-memory key database.

Change History (37)

comment:1 Changed 8 years ago by Jean-Paul Calderone

To be even a bit more specific, most of what I'm looking for is a callback very much along the lines of checkKey - but for which is documented and tested to be overrideable by a subclass. I initially hoped that it also wouldn't need to check blobs, but on further thought I see that may not be feasible - it is necessary to find the right key for a user who may have several. It would be nice if ISSHPrivateKey specified a Key rather than a string blob, though. Comparing strings always makes me quite sad. :)

comment:2 Changed 8 years ago by Jonathan Lange

I have code for this (I think) sitting around, not yet public.

comment:3 Changed 6 years ago by <automation>

Owner: z3p deleted

comment:4 Changed 5 years ago by Ying Li

Author: cyli
Branch: branches/parameterize-sshkey-checker-3701

(In [33745]) Branching to 'parameterize-sshkey-checker-3701'

comment:5 Changed 5 years ago by Ying Li

(In [33769]) Add a base SSH public key database class, and a replacement for SSHPublicKeyDatabase that is (mostly) backwards compatible.

refs #3701

comment:6 Changed 5 years ago by Ying Li

(In [33782]) Fix some backwards-compatibility bugs in the tests

refs #3701

comment:7 Changed 5 years ago by Ying Li

Test coverage is spotty in some ways:

  • Even though SSHPublicKeyDatabaseTestCase.test_requestAvatarIdInvalidKey is supposed to test what happens when checkKey returns false, this is never tested in the UNIXAccountPublicKeyDatabaseTestCase the credentials are invalid (no signature) - the order in which things are checked is inverted from SSHPublicKeyDatabaseTestCase. In any case, only one thing that can fail should probably be tested.
  • There are never any tested authorized_keys file access errors, which means that certain code paths in checkKey aren't tested. But more importantly, test_requestAvatarIdNormalizeException is only verifies that ValidPublicKey gets normalized.

comment:8 Changed 5 years ago by Ying Li

Correction, it tests that BadKeyError gets normalized

comment:9 Changed 5 years ago by Ying Li

(In [33799]) Improve test coverage for existing SSHPublicKeyDatabaseTestCase tests

refs #3701

comment:10 Changed 5 years ago by Ying Li

(In [33800]) Change naming of new checkers to XXXPublicKeyChecker, rather than XXXPublicKeyDatabase

refs #3701

comment:11 Changed 5 years ago by Ying Li

(In [33809]) Add a InMemorySSHPublicKeyChecker and add tests for it and BaseSSHPublicKeyChecker

refs #3701

comment:12 Changed 5 years ago by Ying Li

(In [33810]) Add news file and add @since tags

refs #3701

comment:13 Changed 5 years ago by Ying Li

(In [33811]) Skip new test cases if conch dependencies are not met.

refs #3701

comment:14 Changed 5 years ago by Ying Li

(In [33821]) Fix another test that should not do anything if conch dependencies are not available

refs #3701

comment:15 Changed 5 years ago by Ying Li

(In [33822]) More test fixing

refs #3701

comment:16 Changed 5 years ago by Ying Li

Keywords: review added

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

Keywords: review removed
Owner: set to Ying Li

Thanks very much for working on this. Sorry about the terrible review latency. :(

  1. Design Concerns
    1. The interface looks a little off to me. UNIXAccountPublicKeyChecker.validateKey doesn't even use the publicKey parameter. That can work because the key and the credentials object represent the same information. It might be better if the interface didn't need this redundancy, though. The base class could provide two things (for checkers that need tow different levels of functionality):
      1. A helper for checking whether two keys are the same (this would replace the key == publicKey in one implementation of validateKey and the base64.decodestring(l2[1]) == credentials.blob in the other). Any subclass could use this instead of re-implementing key comparison.
      2. A hook for returning an iterator of keys which are acceptable for a particular user. The base class can call this, iterate the result, and compare each key to the one presented. I would expect this to simplify UNIXAccountPublicKeyChecker by moving all of the non-path-searching code out of it into the base. This would make any class that doesn't need special key lookup logic simpler (consider a checker that finds keys by their fingerprint instead of by the attempted username; such an implementation wouldn't benefit from this hook, hence the helper for key comparison suggested above.
    2. Or, from another perspective, UNIXAccountPublicKeyChecker.checkKey still mixes up the two concerns mentioned in the ticket description, so the refactoring is incomplete.
  2. Documentation Concerns
    1. Conch is sorely lacking in howto-style documentation. Can you add a short howto with an example of writing a custom checker based on this new functionality?
    2. Some things are missing docstrings (maybe irrelevant, depending on how design concerns are addressed):
      1. BaseSSHPublicKeyChecker.requestAvatarId
      2. InMemorySSHPublicKeyChecker.validateKey
      3. UNIXAccountPublicKeyChecker.validateKey
  3. Localized Concerns
    1. self.keyDictionary = keyDictionary or {} is a bit of an anti-pattern. It has surprising behavior when keyDictionary == {}.
    2. InMemorySSHPublicKeyChecker.validateKey probably doesn't need the exception handling anymore? Even if it did, the bare except with no handling is, of course, terrible, and Conch should stop doing things like this.
    3. All calls to log.err should now pass a second argument. UNIXAccountPublicKeyChecker.validateKey needs to be adjusted to do this.

comment:18 Changed 4 years ago by Ying Li

Branch: branches/parameterize-sshkey-checker-3701branches/parameterize-sshkey-checker-3701-2

(In [37645]) Branching to 'parameterize-sshkey-checker-3701-2'

comment:19 Changed 4 years ago by Ying Li

(In [37647]) Merge changes forward. refs #3701

comment:20 Changed 4 years ago by Ying Li

(In [37683]) Update docstrings on keys. refs #3701

comment:21 Changed 4 years ago by Ying Li

(In [37686]) General functions to get authorized keys and to check the provided key. refs #3701

comment:22 Changed 4 years ago by Ying Li

(In [37715]) Remove unused code and fix broken SSHPublicKeyDatabase tests. refs #3701

comment:23 Changed 4 years ago by Ying Li

(In [37716]) Add test for getAuthorizedKeysFiles. refs #3701

comment:24 Changed 4 years ago by Ying Li

(In [37763]) New ssh key checker that takes a callable to get keys to compare credentials against. refs #3701

comment:25 Changed 4 years ago by Ying Li

(In [37769]) Add support for multiline/multikey strings. refs #3701

comment:26 Changed 4 years ago by Ying Li

(In [37771]) Add a callable that generates keys from unix .ssh authorized key files. refs #3701

comment:27 Changed 4 years ago by Ying Li

(In [37775]) Remove spwd support, since we're not supporting 2.5 anymore. refs #3701

comment:28 Changed 4 years ago by Ying Li

(In [37776]) changed my mind, don't remove spwd. refs #3701

comment:29 Changed 4 years ago by Ying Li

(In [37780]) Remove publicKeysFromStrings, because a blob string cannot be on multiple lines. So only include the filepath helper in the checkers. refs #3701

comment:30 Changed 4 years ago by Ying Li

(In [37781]) Conch custom checker documentation. refs #3701

comment:31 Changed 4 years ago by Ying Li

(In [37782]) topfile. refs #3701

comment:32 Changed 4 years ago by Ying Li

Keywords: review added
Owner: Ying Li deleted

So after a ton of poking, I've ended up with something very similar to checkKey, but passed as a parameter.

But I'm thinking perhaps we should just go back to the checkKey/generateAuthorizedKeys abstraction, except that generateAuthorizedKeys will not be a public method, and that checkKey instead of being overridable is a callable that gets passed into the __init__ function.

The positive of the way it is right now (where the callable that gets passed into the __init__ function is a generator function that produces valid keys to compare against is that it sort of forces comparison between Key objects, rather than blobs.

Please weigh in.

Also updated the documentation, which did already have an example of an in-memory key checker.

comment:33 Changed 4 years ago by Ying Li

I think SVN/buildbot repo is broken. =/ Last commit should be r37783. And buildbot keeps building either a very old revision or getting bzr bugs.

comment:34 Changed 4 years ago by Ying Li

But on my local machine tests passes for regular python 2.7, python 3.3, and the checker tests pass for python 2.7 without mods (but I get twisted.test.test_twisted.RealZopeInterfaceTests.test_zope36 failure even though zope.interface is installed)

comment:35 Changed 4 years ago by Ying Li

Looks like buildbot is fixed. So r37793 fixes some twistedchecker docstring issues. new buildbot results

comment:36 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Ying Li
  1. topfile missing verb.
  2. If you are moving dependencySkip to the toplevel, you can remove the redundant checks on the TestCases. You could even just rename it to skip. You've also removed the cryptSkip from HelperTests.
  3. The various setUp method could use a docstring.
  4. The assertions in test_keysFromFilepathsInvalidPermissionsWithOwnerIds seem somewhat divorced from what the docstring claims.
  5. "This generator never ends - will produce FakeKey objects until 20," is self-contradictory.
  6. The assertTrues could use a descriptive message.
  7. test_obscuresErrors's FakeKey has a bogus docstring.
  8. UserAuthorizedKeysGeneratorTestCase doesn't appear to use MockOS except as a fancy place to store a filename.
  9. C{None}/C{str}: I'd be inclined to spell this L{str} or L{None}. (It isn't obvious what / is supposed to mean)
  10. publicKeysFromFilepaths:
    • it doesn't take absolute filenames.
    • The comment about multiple keys would be clearer if it said something affirmative, rather than '... but ...'
    • What happens if there are comments in any of the files? I think the current code will raise an exception, but that probably isn't desired.
  11. Every time I've read Obscure, I've read it as an adjective, rather than a verb.
  12. demo_checkers.tac doesn't exist.
  13. As exarkun said, having some narrative how-to style documentation would be nice.
    • Also, we've started to add tests for examples (but doing that is optional).

comment:37 Changed 3 years ago by Ying Li

Resolution: duplicate
Status: newclosed

Oops. :( I opened a duplicate ticket, because somehow I forgot about this one. Will close this as a duplicate of #7144, since that one is up for review now.

Note: See TracTickets for help on using tickets.