Opened 9 years ago

Closed 9 years ago

#3871 release blocker: regression closed fixed (fixed)

`twistd conch´ always fails due to an unhandled AttributeError; conch server cannot be started.

Reported by: Jean-Paul Calderone Owned by:
Priority: highest Milestone: Twisted-9.0
Component: conch Keywords:
Cc: Glyph, Michael Hudson-Doyle Branch: branches/conch-tap-fix-3871-2
branch-diff, diff-cov, branch-cov, buildbot
Author: therve

Description

exarkun@boson:~$ twistd -n conch
/home/exarkun/Projects/Divmod/trunk/Nevow/nevow/static.py:35: DeprecationWarning: twisted.web.error.NoResource is deprecated since Twisted 9.0.  See twisted.web.resource.NoResource.
  dangerousPathError = error.NoResource("Invalid request URL.")
Traceback (most recent call last):
  File "/home/exarkun/Projects/Twisted/trunk/bin/twistd", line 19, in <module>
    run()
  File "/home/exarkun/Projects/Twisted/trunk/twisted/scripts/twistd.py", line 27, in run
    app.run(runApp, ServerOptions)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/application/app.py", line 690, in run
    runApp(config)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/scripts/twistd.py", line 23, in runApp
    _SomeApplicationRunner(config).run()
  File "/home/exarkun/Projects/Twisted/trunk/twisted/application/app.py", line 411, in run
    self.application = self.createOrGetApplication()
  File "/home/exarkun/Projects/Twisted/trunk/twisted/application/app.py", line 489, in createOrGetApplication
    ser = plg.makeService(self.config.subOptions)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/conch/tap.py", line 40, in makeService
    t.portal.registerChecker(checkers.PluggableAuthenticationModulesChecker())
AttributeError: 'module' object has no attribute 'PluggableAuthenticationModulesChecker'
exarkun@boson:~$ 

It seems most likely that this was introduced by #2682 (although I don't believe PAM-based authentication was working prior to that either, however the failure mode was silent afaict).

Attachments (1)

fix (1.5 KB) - added by Jean-Paul Calderone 9 years ago.

Download all attachments as: .zip

Change History (26)

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

This seems to be what happened.

  • Ages, ago, PluggableAuthenticationModulesChecker moved out of twisted.conch.checkers and into twisted.cred.checkers.
  • Along with it, twisted.conch.pamauth moved into twisted.cred.
  • At that point, twisted.conch.tap became wrong, as it continued to think these things were in twisted.conch.
  • However, the failure was silent, since it manifest as the plugin always detecting PAM as unavailable (since it continued to import twisted.conch.pamauth via twisted.conch.checkers). It handled the case of PAM being missing by not setting up a PAM checker.
  • #2682 changed twisted.conch.checkers to import twisted.cred.pamauth instead of twisted.conch.pamauth. This seems to have been a change with no unit test. It was also pointless, since nothing in twisted.conch.checkers uses pamauth. And of course, it was also detrimental, as it made a silent failure that wasn't fatal into a fatal failure that prevents the server from being run at all.

Attaching a patch which demonstrates one way this can be remedied. Of course it is not suitable for inclusion as-is, as it has no test coverage.

Changed 9 years ago by Jean-Paul Calderone

Attachment: fix added

comment:2 Changed 9 years ago by Jean-Paul Calderone

Milestone: Twisted-8.2+1

Ah, and as this is a regression from 8.2, we really ought to fix it for 9.0.

comment:3 Changed 9 years ago by Glyph

Cc: Glyph added

comment:4 Changed 9 years ago by therve

Owner: changed from z3p to therve

comment:5 Changed 9 years ago by therve

Author: therve
Branch: branches/conch-tap-fix-3871

(In [27034]) Branching to 'conch-tap-fix-3871'

comment:6 Changed 9 years ago by therve

Keywords: review added
Owner: therve deleted

Please review!

comment:7 Changed 9 years ago by Jean-Paul Calderone

Owner: set to Jean-Paul Calderone
Status: newassigned

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

Keywords: review removed
Owner: changed from Jean-Paul Calderone to therve
Status: assignednew

Thanks for working on this.

  1. twisted/conch/tap.py could have a test-case-name now
  2. test_makeServicePamAuth should use a set or something when comparing portal.checkers.keys(), to avoid relying on dict ordering.
  3. It would be cool to have a non-skipped test for the without-pamauth case. Right now if I break the implementation and uninstall python-pam, nothing turns red.

comment:9 Changed 9 years ago by ralphm

exarkun beat me, so here some additional small comments:

  • There is spurious white space here.
  • It would be nice to have a comment on how PAM is actually detected to be available. I get confused by secondary ImportErrors.

comment:10 Changed 9 years ago by therve

Keywords: review added
Owner: therve deleted

Everything handled, I think.

comment:11 Changed 9 years ago by Michael Hudson-Doyle

Not a review, but this line:

+ L{ta.makeService} returns a L{TCPServer} instance, linked to

contains a typo.

comment:12 Changed 9 years ago by therve

Thanks, fixed!

comment:13 Changed 9 years ago by Michael Hudson-Doyle

Keywords: review removed

A few comments (entirely about the test, it seems):

  1. Is test_makeServicePamAuth a good enough test for the problem as described (i.e. 'twistd conch' doesn't work)? Please at least add another test that monkey patches tap.pmauth to None.
  2. "L{tap.makeService} returns a L{TCPServer} instance, linked to L{OpenSSHFactory} whose" from the docstring is missing an article (probably "an", i.e. "L{tap.makeService} returns a L{TCPServer} instance, linked to an L{OpenSSHFactory} whose")
  3. test_makeServicePamAuth should use self.patch, not doing that stuff by hand.
  4. A comment explaining why the monkey-patching is there would be a definite bonus.
  5. The verification part of the test is testing considerably more than the docstring of the test method suggests, for example that the port is 22 and testing the entire set of checkers rather than just that the PAM one is there. Could you break this up into a few testcases instead?

comment:14 Changed 9 years ago by Michael Hudson-Doyle

Cc: Michael Hudson-Doyle added

comment:15 Changed 9 years ago by therve

Owner: set to therve

comment:16 Changed 9 years ago by therve

Keywords: review added
Owner: therve deleted

Thanks a lot for the review, everything handled in r27091 I think.

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

Keywords: review removed
Owner: set to therve

twisted.conch.test.test_tap.MakeServiceTest.test_checkersPamAuth fails.

comment:18 Changed 9 years ago by Jean-Paul Calderone

Here's the failure:

===============================================================================
[ERROR]: twisted.conch.test.test_tap.MakeServiceTest.test_checkersPamAuth

Traceback (most recent call last):
  File "/home/exarkun/Projects/Twisted/trunk/twisted/conch/test/test_tap.py", line 49, in test_checkersPamAuth
    service = tap.makeService(config)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/conch/tap.py", line 42, in makeService
    t.portal.registerChecker(PluggableAuthenticationModulesChecker())
exceptions.NameError: global name 'PluggableAuthenticationModulesChecker' is not defined
-------------------------------------------------------------------------------

comment:19 Changed 9 years ago by therve

Keywords: review added
Owner: therve deleted

Hum, sorry about that, I didn't test without PyPAM installed.

comment:20 Changed 9 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to therve

Windows had a test failure:

===============================================================================
[ERROR]: twisted.conch.test.test_tap

Traceback (most recent call last):
  File "c:\buildbot\twisted\WXP32-full2.4-scmikes-select\Twisted\twisted\trial\runner.py", line 552, in loadPackage
    module = modinfo.load()
  File "c:\buildbot\twisted\WXP32-full2.4-scmikes-select\Twisted\twisted\python\modules.py", line 380, in load
    return self.pathEntry.pythonPath.moduleLoader(self.name)
  File "c:\buildbot\twisted\WXP32-full2.4-scmikes-select\Twisted\twisted\python\reflect.py", line 462, in namedAny
    topLevelPackage = _importAndCheckStack(trialname)
  File "c:\buildbot\twisted\WXP32-full2.4-scmikes-select\Twisted\twisted\conch\test\test_tap.py", line 8, in ?
    from twisted.conch import tap
  File "c:\buildbot\twisted\WXP32-full2.4-scmikes-select\Twisted\twisted\conch\tap.py", line 9, in ?
    from twisted.conch import checkers, unix
  File "c:\buildbot\twisted\WXP32-full2.4-scmikes-select\Twisted\twisted\conch\unix.py", line 17, in ?
    import fcntl, tty
exceptions.ImportError: No module named fcntl
-------------------------------------------------------------------------------

:/

comment:21 Changed 9 years ago by therve

Branch: branches/conch-tap-fix-3871branches/conch-tap-fix-3871-2

(In [27093]) Branching to 'conch-tap-fix-3871-2'

comment:22 Changed 9 years ago by therve

Keywords: review added
Owner: therve deleted

Sigh. It should be ok.

comment:23 Changed 9 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to therve

Seems to be working (not really, but working enough for this ticket). Shame there are so many Conch cross-platform stumbling blocks. :( Please merge.

comment:24 Changed 9 years ago by therve

Resolution: fixed
Status: newclosed

(In [27097]) Merge conch-tap-fix-3871-2

Author: therve Reviewers: exarkun, mwh Fixes #3871

Fixes a bad import in twisted.conch.tap preventing from running it at all and add some test coverage for it.

comment:25 Changed 8 years ago by <automation>

Owner: therve deleted
Note: See TracTickets for help on using tickets.