Opened 11 years ago
Closed 10 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 11 years ago by
comment:2 Changed 11 years ago by
Owner: | z3p deleted |
---|
comment:3 Changed 11 years ago by
Author: | → cyli |
---|---|
Branch: | → branches/conch-cred-plugin-4753 |
(In [33042]) Branching to 'conch-cred-plugin-4753'
comment:6 Changed 10 years ago by
comment:7 Changed 10 years ago by
Keywords: | review easy added |
---|
comment:8 Changed 10 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Ying Li |
Looks reasonably good, just a couple of things need fixing:
- 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. - 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.
- 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
andICheckerFactory
plugins, which you can see intwisted/plugins/cred_*
; it should be pretty easy to add, but smaller tickets are always better.
Thanks!
comment:9 follow-up: 10 Changed 10 years ago by
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 Changed 10 years ago by
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 follow-up: 12 Changed 10 years ago by
Does that mean I should not make a PAM strcred plugin?
comment:12 Changed 10 years ago by
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 follow-up: 14 Changed 10 years ago by
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 Changed 10 years ago by
comment:15 Changed 10 years ago by
comment:16 Changed 10 years ago by
comment:17 Changed 10 years ago by
comment:19 Changed 10 years ago by
comment:20 Changed 10 years ago by
comment:22 Changed 10 years ago by
Keywords: | review added; easy removed |
---|---|
Owner: | Ying Li deleted |
buildbot results I don't think the doc builder errors are mine.
comment:23 Changed 10 years ago by
Keywords: | easy added |
---|
comment:24 Changed 10 years ago by
There's a typo in test_strcrd.py: skip = "PyCryto is not available"
comment:26 Changed 10 years ago by
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 10 years ago by
Keywords: | review added |
---|
After discussion with exarkun, we're going to add support for anonymous, etc. in a different ticket
comment:28 Changed 10 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Ying Li |
Thanks for working on this. I'm really happy to see this feature in conch now. :)
- in twisted/conch/tap.py
Options.longdesc
is defined using funny indentation; can you line up the "s?makeService
should have a docstring
- in twisted/conch/test/test_tap.py
MakeServiceTest.setUp
tries to call file.close, but it's missing some ()s. Also, it might be better to just usetwisted.python.filepath.FilePath.setContent
to write out this file.- The formatting of the multiline return in
test_authFailure
is a little gross. Can you split that up into multiple statements? Also, maybeTestCase.assertFailure
would be helpful here. - return statement in
test_authSuccess
is also maybe worth splitting up.
- in twisted/test/test_strcred.py
- 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.
- 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 10 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
Should we have a separate ticket for 'allow starting conch server without root privileges'?