Support cred plugins in the "twistd conch" command line

[Jean-Paul Calderone]

It would be really great, for all of the standard reasons, to be able to specify the source of credentials material for conch. This would be extra cool, since it might eventually lead to a conch server that can be run as a user other than root (currently root is required for several reasons, among them that credentials are sometimes checked via PAM or the shadow database).

[Glyph]

Should we have a separate ticket for 'allow starting conch server without root privileges'?

[Ying Li]

(In [33042]) Branching to 'conch-cred-plugin-4753'

[Ying Li]

(In [33549]) Branching to 'conch-cred-plugin-4753'

[Ying Li]

(In [33550]) Add cred plugin to conch refs #4753

[Ying Li]

(In [33552]) Fix for python 2.5 (with context not supported) refs #4753

[Ying Li]

[buildbot results: ​http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/conch-cred-plugin-4753]

[Glyph]

Looks reasonably good, just a couple of things need fixing:

1. When --auth is passed, a user should be able to presume that the authentication mechanisms that they specify are the only mechanisms that will be honored; it might be a security flaw to allow otherwise. (For example, you might want to run a locked-down server.) The default set of UNIX, publickey, and maybe PAM should remain the default, for compatibility, but if the user specifies a different mechanism they shouldn't be used.
2. The default set of authentication mechanisms really needs to be documented, since without that the user doesn't know what they're getting in terms of who can log in.
3. You can do this in a separate ticket, but the SSH public key and PAM checkers do not have corresponding strcred plugins, which means that the only way to get those features is to rely on the default (i.e. the old bad way, not the new good way). The relevant plugin machinery is twisted.cred.strcred and ICheckerFactory plugins, which you can see in twisted/plugins/cred_*; it should be pretty easy to add, but smaller tickets are always better.

Thanks!

[Jean-Paul Calderone]

One thing to be aware of, perhaps, is that the PAM stuff probably doesn't work, or doesn't work well enough to use outside of a toy setting. It makes Twisted calls from the wrong thread and exposes an API that almost cannot be implemented correctly. Trace how pamConversion is defined and used and you'll see the problems.

[Glyph]

Replying to exarkun:

One thing to be aware of, perhaps, is that the PAM stuff probably doesn't work, or doesn't work well enough to use outside of a toy setting. It makes Twisted calls from the wrong thread and exposes an API that almost cannot be implemented correctly. Trace how pamConversion is defined and used and you'll see the problems.

Is there an existing ticket about this?

[Ying Li]

Does that mean I should not make a PAM strcred plugin?

[Glyph]

Replying to cyli:

Does that mean I should not make a PAM strcred plugin?

Yeah, let's leave that out. If, as exarkun says, it's making Twisted calls from the wrong thread, it should never be enabled. You should also make a separate ticket for fixing the existing PAM integration, and mention that that ticket should add a strcred plugin for it.

[Ying Li]

Note: there seems to be a UNIX Password checker in twisted.conch.checkers.py, as well as a UNIX checker in twisted.cred.checkers.py. I've opened #5518 about eventually removing this.

[Ying Li]

Sorry, correction - twisted.plugins.cred_unix.py

Replying to cyli:

Note: there seems to be a UNIX Password checker in twisted.conch.checkers.py, as well as a UNIX checker in twisted.cred.checkers.py. I've opened #5518 about eventually removing this.

[Ying Li]

There are already two PAM tickets:

#3728 seems to refer to the PAM issues that exarkun mentioned, and #2970 is a ticket for making a PAM checker factory.

[Ying Li]

(In [33666]) Default checkers should only be auth methods if --auth paramter isn't set. Also add SSH cred plugin. refs #4753

[Ying Li]

(In [33667]) Attempts to fix 2 bugs: key error bug for those machines with PAM, and import error when the Crypto module is not available. refs #4753

[Ying Li]

Forgot to include a ticket number in ​33668

[Ying Li]

(In [33669]) Attempt to fix key bug in test_tap, and also an attempt to fix doc bug. refs #4753

[Ying Li]

(In [33670]) SSH cred plugin also needs pyasn1 - check for that before running cred plugin test. refs #4753

[Ying Li]

(In [33671]) Remove debug prints. refs #4753

[Ying Li]

​buildbot results I don't think the doc builder errors are mine.

[Thijs Triemstra]

There's a typo in test_strcrd.py: skip = "PyCryto is not available"

[Ying Li]

Fixed typo in ​33672

[Ying Li]

Meh, lack of tests to make sure that --auth actually overrides the auth methods that seem to be default in SSHUserAuthServer

[Ying Li]

After discussion with exarkun, we're going to add support for anonymous, etc. in a different ticket

[Jean-Paul Calderone]

Thanks for working on this. I'm really happy to see this feature in conch now. :)

1. in twisted/conch/tap.py
   1. Options.longdesc is defined using funny indentation; can you line up the "s?
   2. makeService should have a docstring
2. in twisted/conch/test/test_tap.py
   1. MakeServiceTest.setUp tries to call file.close, but it's missing some ()s. Also, it might be better to just use twisted.python.filepath.FilePath.setContent to write out this file.
   2. The formatting of the multiline return in test_authFailure is a little gross. Can you split that up into multiple statements? Also, maybe TestCase.assertFailure would be helpful here.
   3. return statement in test_authSuccess is also maybe worth splitting up.
3. in twisted/test/test_strcred.py
   1. The lines that got reformatted to be less than 80 columns don't match how most of Twisted is formatted. Can you make the 2nd line aligned with the open paren of the function call, or put the newline after the open paren and have all the args on the 2nd line.
4. The news fragment should follow the style guidelines on ​http://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles

There were some other indentation-related changes that I didn't comment on specifically, but it would be nice (but not mandatory for this ticket) to fix them.

These points are all pretty simple, please fix and then merge.

[Ying Li]

(In [33696]) Merge conch-cred-plugin-4753: support cred plugins in conch

Author: cyli Reviewer: glyph, exarkun Fixes: #4753

Changed twisted.conch.tap so that twistd conch now supports cred plugins