Opened 9 years ago

Closed 8 years ago

#4753 enhancement closed fixed (fixed)

Support cred plugins in the "twistd conch" command line

Reported by: Jean-Paul Calderone Owned by: Ying Li
Priority: normal Milestone:
Component: conch Keywords:
Cc: Branch: branches/conch-cred-plugin-4753
branch-diff, diff-cov, branch-cov, buildbot
Author: cyli

Description

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

Change History (29)

comment:1 Changed 9 years ago by Glyph

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

comment:2 Changed 9 years ago by <automation>

Owner: z3p deleted

comment:3 Changed 8 years ago by Ying Li

Author: cyli
Branch: branches/conch-cred-plugin-4753

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

comment:4 Changed 8 years ago by Ying Li

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

comment:5 Changed 8 years ago by Ying Li

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

comment:6 Changed 8 years ago by Ying Li

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

comment:8 Changed 8 years ago by Glyph

Keywords: review removed
Owner: set to Ying Li

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!

comment:9 Changed 8 years ago by 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.

comment:10 in reply to:  9 Changed 8 years ago by 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?

comment:11 Changed 8 years ago by Ying Li

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

comment:12 in reply to:  11 Changed 8 years ago by 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.

comment:13 Changed 8 years ago by 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.

comment:14 in reply to:  13 Changed 8 years ago by 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.

comment:15 Changed 8 years ago by 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.

comment:16 Changed 8 years ago by Ying Li

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

comment:17 Changed 8 years ago by 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

comment:18 Changed 8 years ago by Ying Li

Forgot to include a ticket number in 33668

comment:19 Changed 8 years ago by Ying Li

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

comment:20 Changed 8 years ago by Ying Li

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

comment:21 Changed 8 years ago by Ying Li

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

comment:22 Changed 8 years ago by Ying Li

Keywords: review added; easy removed
Owner: Ying Li deleted

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

comment:23 Changed 8 years ago by Ying Li

Keywords: easy added

comment:24 Changed 8 years ago by Thijs Triemstra

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

comment:25 Changed 8 years ago by Ying Li

Fixed typo in 33672

comment:26 Changed 8 years ago by Ying Li

Keywords: review easy removed

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

comment:27 Changed 8 years ago by Ying Li

Keywords: review added

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

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

Keywords: review removed
Owner: set to Ying Li

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.

comment:29 Changed 8 years ago by Ying Li

Resolution: fixed
Status: newclosed

(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

Note: See TracTickets for help on using tickets.