Opened 9 years ago

Closed 6 years ago

#3242 enhancement closed fixed (fixed)

use python 2.5 'spwd' module instead of z3p secret 'shadow' module when available

Reported by: Glyph Owned by: z3p
Priority: high Milestone:
Component: conch Keywords:
Cc: therve, Michael Hudson-Doyle, Thijs Triemstra, jesstess Branch: branches/spwd-3242-7
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun, z3p

Description

It might even make sense to get rid of the 'shadow' module and just include a 2.3 backport of spwd within Twisted, but I'll leave it up to you to make another ticket. However, given that this functionality is actually maintained somewhere, let's use the maintained version...

Change History (41)

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

I would expect the PAM functionality to almost entirely obsolete any direct support of /etc/passwd or /etc/shadow. Is this something we actually need to continue to maintain?

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

Also, whatever third-party module is used should be documented as a dependency. (See #1633)

comment:3 in reply to:  1 ; Changed 9 years ago by jkk

Replying to exarkun:

I would expect the PAM functionality to almost entirely obsolete any direct support of /etc/passwd or /etc/shadow. Is this something we actually need to continue to maintain?

It will happen eventually, but currently PyPAM is in Alpha stage and it doesn't work under python 2.5 .I wouldn't yet entirely rely on it as the only authentication module supported.

comment:4 in reply to:  3 Changed 9 years ago by jkk

Replying to jkk:

Replying to exarkun:

I would expect the PAM functionality to almost entirely obsolete any direct support of /etc/passwd or /etc/shadow. Is this something we actually need to continue to maintain?

It will happen eventually, but currently PyPAM is in Alpha stage and it doesn't work under python 2.5 .I wouldn't yet entirely rely on it as the only authentication module supported.

Ok, so it will work on python 2.5.I've just sent a necessary fix to PyPAM maintainer.

comment:5 Changed 8 years ago by z3p

Author: z3p
Branch: branches/spwd-3242

(In [26271]) Branching to 'spwd-3242'

comment:6 Changed 8 years ago by z3p

Keywords: review added
Owner: z3p deleted
Priority: normalhigh

Ready for review, I guess.

comment:7 Changed 8 years ago by Glyph

Keywords: review removed
Owner: set to z3p

You should write some tests that mock the appropriate functions (seteuid, setegid, getspwnam) here. Ideally you could break it up so that the front-end/back-end separation with UNIXPasswordDatabase is done via an attribute that is an (optional, for compatibility purposes) argument to its constructor or something.

comment:8 Changed 8 years ago by z3p

Keywords: review added
Owner: z3p deleted

I refactored that class to take an option argument of getpwnam functions, and some tests for that class and the new functions.

comment:9 Changed 8 years ago by Glyph

Keywords: review removed
Owner: set to z3p

Erm... I don't actually see any of that in the branch. I only see changes to twisted/conch/checkers.py, as a matter of fact. Is there another branch with the wrong ticket number on it somewhere? If so, want to merge forward with a correct number so the 'branch' field picks it up?

comment:10 Changed 8 years ago by z3p

Keywords: review added
Owner: z3p deleted

Oh, looks like the commit didn't actually make it into SVN. Should be there now.

comment:11 Changed 8 years ago by Glyph

Keywords: review removed
Owner: set to z3p

Conflicts in both files when merged to trunk. Looks like you need that merge forward anyway :).

comment:12 Changed 8 years ago by z3p

Branch: branches/spwd-3242branches/spwd-3242-2

(In [26921]) Branching to 'spwd-3242-2'

comment:13 Changed 8 years ago by z3p

Keywords: review added
Owner: changed from z3p to Glyph

This time for sure!

comment:14 Changed 8 years ago by Glyph

Keywords: review removed
Owner: changed from Glyph to z3p

Definitely an improvement. I like the large number of duplicated seteuid lines which are deleted ;-).

  1. getpwnam_functions
    1. its name violates the coding standard.
    2. it isn't documented. The class needs a docstring which has an @ivar.
    3. why have the 'None' condition at all? is the list intended to be mutable per-instance? wouldn't it be simpler to have it be a tuple, and have the signature be def __init__(self, getpwnamFunctions=(getpwnam_passwd, getpwnam_shadow))?
  2. getpwnam_passwd and getpwnam_shadow
    1. names violate the coding standard
    2. 3 blank lines between toplevel functions, please
    3. neither one has a docstring
  3. test_checkers
    1. We've been moving towards a more declarative, active-voice style of writing docstrings, one which is both briefer and more amenable to documentation generation. For example, instead of "When foo is true, bar() should baz without raising an exception.", "bar() bazzes when foo.". Avoid pronouns, except where the antecedent is clearly visible in the docstring itself. Avoid words like "should" and "verifies" and so on. Just say what the expected behavior is, not that the test verifies it or that it should be so.
    2. test_verifyCryptedPassword and test_verifyCryptedPassword_md5 have blank docstrings.
    3. test_getpwnam_passwd, test_getpwnam_shadow, test_default_checkers, test_pass_in_checkers, test_verify_password, test_fail_on_KeyError, test_fail_on_bad_password, and test_loop_through_functions all have names which violate the coding standard.
    4. assertLoggedIn and assertUnauthorizedLogin are a bit vague. "is a valid login"? What does that mean? What constitutes validity? I see a @type, but what about a @param? The bit where you need to return its result or it might not actually do anything seems pretty important, too.
    5. rather than touching a bunch of unrelated lines by changing the import from importing classes to importing modules, why not just `from twisted.conch.checkers import pwd as pwdcheckers, shadow as spwdcheckers?
    6. If you import individual modules, patch() calls can be less invasive. Since spwd and pwd aren't particularly widely used, this isn't of terribly much practical concern in this case, but it's a good habit to get into: patch as narrowly as possible. In this particular case, getpwnam and getspwnam are the only functions that are used in checkers, so you could import them directly.
    7. there are a couple of >80 character lines.
    8. please put 2 blank lines between methods and 3 between classes.
    9. test_loop_through_functions' docstring is a bit vague. Phrase it a bit more positively, i.e. "UNIXPasswordDatabase.requestAvatarId loops through each getpwnam function associated with it and returns a Deferred which fires with the result of the first one which returns a value rather than raising exception Foo."

comment:15 Changed 8 years ago by z3p

Keywords: review added
Owner: z3p deleted

Other than 3.5 and 3.6, the other comments are addressed in the branch.

3.5: I switched to importing the module because otherwise I'm importing a ton of individual things and that seemed more cluttered. If it's really a problem I can do it that way, though.

3.6: I'm not sure what this means; I'm only patching individual functions; I'm not sure how to patch more narrowly than that. Perhaps an example would help.

I'm going to put this back up for review, but if those comments need to be addressed, feel free to bounce it back.

comment:16 Changed 8 years ago by therve

Cc: therve added
Keywords: review removed
Owner: set to z3p

*

     import Crypto.Cipher.DES3
+    Crypto
 except ImportError:

Please don't do that. I know that pyflakes complain, but I'd rather see it fixed in pyflakes.

*

+        credentials = SSHPrivateKey('test', 'ssh-rsa',
+                                    keydata.publicRSA_openssh, 'foo',
+                                    keys.Key.fromString(
+                keydata.privateRSA_openssh).sign('foo'))

Indentation is broken. Maybe something like that:

        credentials = SSHPrivateKey(
            'test', 'ssh-rsa', keydata.publicRSA_openssh, 'foo',
            keys.Key.fromString(keydata.privateRSA_openssh).sign('foo'))

It's visible in a couple of place: the second indentation should always be 4 spaces, not 8.

  • +        skip = 'cannot run without crypt module'
    

Do you mean pwd here?

  • +    elif not getattr(os, 'O_NOCTTY', None):
    +        skip = 'cannot run without os.O_NOCTTY'
    

Please check for is None explicitely.

  • +    @param username: the username of the user to return the passwd database
    +    information for.
    

The second line should be indented (and the following as well).

*

+    A checker which validates users out of the UNIX password databases, or a
+    databases a compatible format.

"a database in a compatible format" ?

This is a pretty nice enhancement. I suppose it depends on #3822 before being merged?

Thanks!

comment:17 Changed 8 years ago by z3p

Branch: branches/spwd-3242-2branches/spwd-3242-3

(In [27081]) Branching to 'spwd-3242-3'

comment:18 Changed 8 years ago by z3p

Keywords: review added
Owner: z3p deleted

It didn't actually depend on #3832; that's why I had to have the os.O_NOCTTY check. Now that it's merged, I can remove that check.

Other comments are addressed, I believe.

comment:19 Changed 8 years ago by Glyph

Regarding my point 3.5, never mind. An example regarding point 3.6 is forthcoming.

comment:20 Changed 8 years ago by Glyph

Okay. So, regarding patching more narrowly - reading my comment right now, I can see it doesn't make much sense, so here's a much-expanded version.

The tests are currently passing os, checkers.shadow (i.e. "spwd"), and pwd as arguments to "patch". This means that any module that does import os (or import spwd or import pwd) will be using the replaced functions for the duration of the test.

As I said, the functions being patched in this case are unlikely to be used by trial, but, "unlikely" is the not same as "certainly not". Certainly other functions in the os module are.

This is a pre-existing problem in this test module, so I don't think it's necessarily the responsibility of this branch to fix. However, as a matter of habit, patching should always be done so that it affects as few modules as possible, to minimize the effect of future features that trial might have. (Imagine a tool which let you run every test as a different effective user-ID. I don't know why you'd want such a thing... but you could have it.)

Taking the specific example of test_getpwnamShadow, instead of this:

self.patch(checkers.shadow, 'getspnam', lambda u: (0, 1, 2, 3))
self.patch(os, "geteuid", self.mockos.geteuid)
self.patch(os, "getegid", self.mockos.getegid)
# ... et cetera ...

you could do this:

self.patch(checkers, "os", self.mockos)
class MockShadow(object):
    def getspnam(self, u):
        return (0, 1, 2, 3)
self.patch(checkers, "shadow", MockShadow())

(of course you could put MockShadow at module level)

The second example there only patches the names actually used in the checkers module, so other modules which import the same names are not affected.

comment:21 Changed 8 years ago by Michael Hudson-Doyle

I'm not sure what the status of this branch is. Was glyph's comment intended as a review?

comment:22 Changed 8 years ago by Michael Hudson-Doyle

Cc: Michael Hudson-Doyle added

comment:23 Changed 8 years ago by Glyph

My comment was an explanation of a point in an earlier review, which z3p said he didn't understand in comment 15.

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

Keywords: review removed
Owner: set to z3p

Hopefully z3p can respond to glyph's point 3.6 in comment 15 now.

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

Author: z3pexarkun, z3p
Branch: branches/spwd-3242-3branches/spwd-3242-4

(In [29696]) Branching to 'spwd-3242-4'

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

Branch: branches/spwd-3242-4branches/spwd-3242-5

(In [29711]) Branching to 'spwd-3242-5'

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

Branch: branches/spwd-3242-5branches/spwd-3242-6

(In [29836]) Branching to 'spwd-3242-6'

comment:28 Changed 6 years ago by <automation>

Owner: z3p deleted

comment:29 Changed 6 years ago by Thijs Triemstra

Cc: Thijs Triemstra added

comment:30 Changed 6 years ago by z3p

Branch: branches/spwd-3242-6branches/swpd-3242-7

(In [33143]) Branching to 'swpd-3242-7'

comment:31 Changed 6 years ago by z3p

Branch: branches/swpd-3242-7branches/spwd-3242-7

(In [33144]) Branching to 'spwd-3242-7'

comment:32 Changed 6 years ago by z3p

(In [33147]) address comment 3.6

Refs #3242

comment:33 Changed 6 years ago by z3p

Keywords: review added

comment:34 Changed 6 years ago by Glyph

Owner: set to Glyph
Status: newassigned

Reviewing.

comment:35 Changed 6 years ago by Glyph

Keywords: review removed
Owner: changed from Glyph to z3p
Status: assignednew

WELCOME BACK TO THE STAGE OF HISTORY

  1. Looks like the API documentation builder found some problems; you should fix those.
  2. assertUnauthorizedLogin incorrectly describes its @rtype as Deferred; its return type is actually NoneType. (Which is good, because new tests should not spin the reactor.)
  3. This code still uses patch an awful lot. I can see you responded to my previous review, and the patches are all much less aggressive, but in the future you may want to consider attributes looked up on instances in more places than temporarily-replaced attributes of modules or classes. (Nothing to be done for this ticket, just be aware of that in the future.)

Otherwise this looks pretty good, thanks: if you can fix points 1 and 2, I think it's ready to land!

comment:36 Changed 6 years ago by Thijs Triemstra

ShadowDatabase in t.p.fakepwd also needs an @since marker.

comment:37 Changed 6 years ago by Jean-Paul Calderone

Looks like the API documentation builder found some problems; you should fix those.

Dang. I'd really like to teach pydoctor about the standard library. :/

comment:38 Changed 6 years ago by z3p

I've addressed point 2, and added the @since marker.

I'm not sure what the right answer is for point 1. Should I untag the module in the docstring, or is there some other fix to make?

comment:39 Changed 6 years ago by z3p

Keywords: review added
Owner: z3p deleted

http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/spwd-3242-7

They're green, including the API builder (thanks to exarkun for the pointer).

comment:40 Changed 6 years ago by jesstess

Cc: jesstess added
Keywords: review removed
Owner: set to z3p

The API changes look good, the rtype change looks good, @since looks good, and I think this is ready to merge!

comment:41 Changed 6 years ago by z3p

Resolution: fixed
Status: newclosed

(In [33231]) Merge branch spwd-3242-7: use python 2.5 'spwd' module instead of z3p secret 'shadow' module when available

Author: exarkun, z3p Reviewer: glyph, therve, thijs, jesstess Fixes: #3242

Python 2.5 includes a standard library module to interact with the /etc/shadow password database. This updates Twisted to that module, rather than the module z3p hacked together many, many years ago.

Note: See TracTickets for help on using tickets.