Opened 4 years ago

Closed 3 years ago

#7144 enhancement closed fixed (fixed)

More generic SSH public key checker

Reported by: Ying Li Owned by: Ying Li
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/new-ssh-key-checker-7144-2
branch-diff, diff-cov, branch-cov, buildbot
Author: cyli

Description

As per discussions with exarkun, https://github.com/cyli/ess/blob/ee505696664eef6ae5e308a5be2a3e7704b4e924/ess/checkers.py introduces a more general version of an SSH checker that does not depend on authorized key files and makes use of Key objects themselves. It provides an interface that matches the avatar ID to an iterable of Keys.

It is not exactly API compatible with the existing twisted.conch.checkers.SSHPublicKeyDatabase, due to not providing a getAuthorizedKeysFiles method or checkKey method. So SSHPublicKeyDatabase should be deprecated, and this checker added as an entirely new API.

It should also remove runAsEffectiveUser - exarkun suggests opening a new ticket after this one to put in a sidecar process in conch, if necessary, that has permissions to read privileged files instead.

Change History (19)

comment:1 Changed 4 years ago by Ying Li

Author: cyli
Branch: branches/new-ssh-key-checker-7144

(In [42217]) Branching to new-ssh-key-checker-7144.

comment:2 Changed 4 years ago by Ying Li

Keywords: review added
Owner: Ying Li deleted

comment:3 Changed 4 years ago by Ying Li

Oops. :( This is actually a duplicate of #3701, so I closed that one.

comment:4 Changed 4 years ago by Ying Li

Branch: branches/new-ssh-key-checker-7144branches/new-ssh-key-checker-7144-2

(In [42439]) Branching to new-ssh-key-checker-7144-2.

comment:5 Changed 4 years ago by Ying Li

Merging forward since there were some conflicts with #7002

comment:6 Changed 4 years ago by Adi Roiban

Thanks for the work on this.

Code looks great.

Please note that I am just starting to do reviews so my comments might be stupid / bad / personal preferences.

This is why I will not remove the review flag.

I am just trying to help with the review queue.

Not sure what is twisted policy but maybe docstring senteces should end with a full stop.


twisted/conch/checkers.py


In readAuthorizedKeyFile is it ok to silently ignore bad lines ?

I see that _keysFromFilepaths is at least logging something.

I see there is a test for this behavior but maybe this should be also stated in function's docstring.


Do you think that UNIXAuthorizedKeysFiles will be used on system without pwd or with custom pwd implementation?

Is twisted.python.fakepwd.UserDatabase used in production ?

I understand that injecting pwd is of great help with testing, but maybe this can be kept private and not a public constuctor argument.


Not sure what is twisted policy for this:

InMemoryKeyMapping maybe self.mapping could be made private. Same comment for the other new classes.


Is AuthorizedKeysFilesMapping really required ?

I am ok with UNIXAuthorizedKeysFiles which is a standard method for Unix ... but for other system I assume that requirements are much generic and I am not sure how many usecases could be solved with AuthorizedKeysFilesMapping.


Maybe parsekey could be renamed to keyParser ?


test_endpoints.py

            mapping = dict([(k,[Key.fromString(v).public()])
                        for k, v in users.iteritems()])
        checker = SSHPublicKeyChecker(InMemoryKeyMapping(mapping))

Not sure what is twisted policy regarding this kind of code in tests. While this is smart / cute I find it harder to read. Since there are no tests to check the tests I prefer to have maximum readability in test.


twisted/conch/test/test_checkers.py

UNIXAuthorizedKeysFilesTestCase.setUp() maybe authorizedKeys.setContent('key 1\nkey 2') should not be added to all tests since this make individual tests harder to read.

For example

        def test_ignoresNonexistantFile(self):
        """
        L{checkers.AuthorizedKeysFilesMapping.getAuthorizedKeys} returns only
        the keys in C{~/.ssh/authorized_keys} and C{~/.ssh/authorized_keys2}
        if they exist
        """

Talks about authorized_keys and authorized_keys2 but test itself does not contain any reference to those files

Thanks!

comment:7 in reply to:  6 Changed 4 years ago by Ying Li

Hi adiroiban!

Thank you so much for the review! I'm not entirely sure what best practices are either here unless they're explicitly documented, so I've made some changes and expressed opinions on other points which may not be correct.


Not sure what is twisted policy but maybe docstring senteces should end with a full stop.

I don't think there is a policy on this (https://twistedmatrix.com/documents/current/core/development/policy/coding-standard.html#auto9), but it is a good idea. I've changed everything so that the docstrings end with a full stop, but the param descriptions may or may not, since they seem like partial sentences.


In readAuthorizedKeyFile is it ok to silently ignore bad lines ?

I see that _keysFromFilepaths is at least logging something.

I see there is a test for this behavior but maybe this should be also stated in function's docstring.

I've updated readAuthorizedKeyFile to also log when a parse has failed, but I've also added this behavior to all the docstrings.


Do you think that UNIXAuthorizedKeysFiles will be used on system without pwd or with custom pwd implementation?

Is twisted.python.fakepwd.UserDatabase used in production ?

I understand that injecting pwd is of great help with testing, but maybe this can be kept private and not a public constuctor argument.

No, in general, probably pwd will be used, and the fakepwd.UserDatabase is not. I've changed all the instance variables to be public, but I think Twisted general prefers named keyword arguments in the constructor for injection, rather than mangling private vars later even for testing? I haven't seen this in the coding standard anywhere, I've just noticed the trend though. Not sure if this is correct.


Not sure what is twisted policy for this:

InMemoryKeyMapping maybe self.mapping could be made private. Same comment for the other new classes.

Done


Is AuthorizedKeysFilesMapping really required ?

I am ok with UNIXAuthorizedKeysFiles which is a standard method for Unix ... but for other system I assume that requirements are much generic and I am not sure how many usecases could be solved with AuthorizedKeysFilesMapping.

It could be useful if you want to map SSH access for usernames that don't correspond to users on the system. If you wanted SFTP read access, for instance, where the files can be served up to a user not on the system. Or for manhole access for certain users only.


Maybe parsekey could be renamed to keyParser ?

I generally pick verbs for functions and nouns for objects - as far as I can tell the twisted code generally follows this pattern, but I haven't found anything that explicitly said so. I stayed with parsekey for now, but am happy to be overruled.


            mapping = dict([(k,[Key.fromString(v).public()])
                        for k, v in users.iteritems()])
        checker = SSHPublicKeyChecker(InMemoryKeyMapping(mapping))

Not sure what is twisted policy regarding this kind of code in tests. While this is smart / cute I find it harder to read. Since there are no tests to check the tests I prefer to have maximum readability in test.

I am also not sure what the policy was. I've been told that best practices in python in general are list/dictionary comprehensions rather than for loops to set the values of a dictionary - can someone weigh in on this?


UNIXAuthorizedKeysFilesTestCase.setUp() maybe authorizedKeys.setContent('key 1\nkey 2') should not be added to all tests since this make individual tests harder to read.

For example

        def test_ignoresNonexistantFile(self):
        """
        L{checkers.AuthorizedKeysFilesMapping.getAuthorizedKeys} returns only
        the keys in C{~/.ssh/authorized_keys} and C{~/.ssh/authorized_keys2}
        if they exist
        """

Talks about authorized_keys and authorized_keys2 but test itself does not contain any reference to those files

Hmm... I agree that one would have to check setUp to actually see the content. But one would also have to check setUp to see where that variable came from, as well, and the line between repeating code and readability has to be drawn somewhere.

I see your point though, and as a compromise, I've also added the instance variable 'expectedKeys' in the setUp function, and the tests now refer to that instead of ['key 1', 'key 2'] explicitly, so that at least when reading the tests, one doesn't have to know that 'key 1' and 'key 2' were written to the authorized key file.

comment:8 Changed 4 years ago by Adi Roiban

Maybe parsekey could be renamed to keyParser ?

I generally pick verbs for functions and nouns for objects - as far as I can tell the twisted code generally follows this pattern, but I haven't found anything that explicitly said so. I stayed with parsekey for now, but am happy to be overruled.

Then, maybe parseKey ... with camel case.


All other changes and comments looks great. Thanks! Should be good to merge.

This still needs a review from a committer :(

comment:9 in reply to:  8 Changed 4 years ago by Ying Li

Then, maybe parseKey ... with camel case.

Done. :) Thank you for all your thorough comments!

comment:10 Changed 3 years ago by Alex Gaynor

Keywords: review removed
Owner: set to Ying Li

A few notes:

  • InMemoryKeyMapping - what do you think about naming this InMemorySSHKeyDatabase or similar?
  • readAuthorizedKeyFile - I'm not wild about the bare except: here
  • _keysFromFilepaths - still not wild abotu the bare except:
  • AuthorizedKeysFilesMapping - what do you think about moving this into a seperate ticket? It doesn't exist yet, and it's a feature addition on a ticket that's nominally a refactoring.
  • UNIXAuthorizedKeysFiles.getAuthorizedKeys - this bare except: definitely shouldn't be necessary
  • SSHPublicKeyChecker._checkKey - bare except:
  • SSHPublicKeyChecker._verifyKey - bare except:
  • twisted/conch/test/test_checkers.py - Please use either io.BytesIO or io.StringIO, whichever is appropriate
  • What do you think about rewriting SSHPublicKeyDatabase to use this stuff, which should let us delete a lot of code?

Overall this looks really fantastic, and after addressing these you should feel free to merge (unless you want another review of course).

comment:11 in reply to:  10 Changed 3 years ago by Ying Li

Keywords: review added

Hey Alex! Thanks so much for reviewing! I'd love another look if you don't mind - I have a question about the first comment in particular.

  • InMemoryKeyMapping - what do you think about naming this InMemorySSHKeyDatabase or similar?

Is the reason you want to name it "...DB" because the interface is named "...DB"? I agree that they should be consistent - I'm wondering if they should both be named "...DB" or both be named "...Map" or "...Mapping", because the functionality is to map an avatar ID to an iterable of keys, whereas DB implies database or some type of extra querying. I dunno though, I'm bad at naming things.

  • readAuthorizedKeyFile - I'm not wild about the bare except: here
  • _keysFromFilepaths - still not wild abotu the bare except:

Fixed.

  • AuthorizedKeysFilesMapping - what do you think about moving this into a seperate ticket? It doesn't exist yet, and it's a feature addition on a ticket that's nominally a refactoring.

Sure, that's fine. I wanted it for ess, mainly, but a separate test makes sense. :)

  • UNIXAuthorizedKeysFiles.getAuthorizedKeys - this bare except: definitely shouldn't be necessary
  • SSHPublicKeyChecker._checkKey - bare except:
  • SSHPublicKeyChecker._verifyKey - bare except:

Fixed, I think.

  • twisted/conch/test/test_checkers.py - Please use either io.BytesIO or io.StringIO, whichever is appropriate

Fixed - picked io.StringIO, since keys probably have to be text? twisted.conch.ssh.keys.Key.fromString seems to read unicode strings just fine for RSA keys, anyway.

  • What do you think about rewriting SSHPublicKeyDatabase to use this stuff, which should let us delete a lot of code?

This new checker is not really API compatible with the previous checker (it does not allow changing to a different effective user for instance to read their authorized key files) and it cares mostly about iterables of keys, rather than iterables of key files.

Some changes could be made to make it more compatible, but (1) I'd like to do this in such a way that the changes make sense outside of the context of making it exactly API compatible with SSHPublicKeyDatabase and (2) perhaps such changes could be made in another branch?

comment:12 Changed 3 years ago by Ying Li

Owner: changed from Ying Li to Alex Gaynor

comment:13 Changed 3 years ago by Ying Li

Buildbot results: https://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/new-ssh-key-checker-7144-2

Twisted checker unfortunately seems to insist on repeating docstrings for implementations of methods that are defined in interfaces, and removing the docstring doesn't fix it either.

comment:14 Changed 3 years ago by Alex Gaynor

Keywords: review removed
Owner: changed from Alex Gaynor to Ying Li

Yes, I went with DB because of the interface, I think DB is better than mapping.

io.StringIO sounds right then.

I'm not sure how to deal with Twisted checker, but otherwise this looks good to me, thanks!

comment:15 Changed 3 years ago by Glyph

"DB" sounds to me like something that supports arbitrary querying. "Mapping" sounds like something that does lookup by a single key. And it looks like this interface is more the former than the latter. Why do you like "DB"?

comment:16 Changed 3 years ago by Alex Gaynor

Mapping to me reminds me of the Mapping ABC, whose interface this doesn't support, and which really is something different entirely. Database is commonly used to refer to other things, such as "authorized keys database", which don't support arbitrary querying.

comment:17 Changed 3 years ago by Glyph

I am OK with Database, I guess. Mapping isn't perfectly evocative either.

comment:18 Changed 3 years ago by Ying Li

Updated the name - buildbot results here: https://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/new-ssh-key-checker-7144-2 (random py3 test failure).

Will merge now. Thanks for the review!

comment:19 Changed 3 years ago by Ying Li

Resolution: fixed
Status: newclosed

(In [42801]) Merge new-ssh-key-checker-7144-2: New public ssh key checker

Author: cyli Reviewers: adiroiban, Alex Fixes: #7144

New ssh public key checker that provides a different API for specifying authorized keys. Also deprecates the previous SSHPublicKeyDatabase.

Note: See TracTickets for help on using tickets.