Summary:

NIS backend doesn't support MD5 passwords

Review Request #647 — Created Nov. 28, 2008 and submitted

Information
Owner:	will_lentz
Repository:	Review Board SVN (deprecated)
Branch:
Bugs:
Depends On:
Reviewers
Groups:	reviewboard
People:

Description

The current NIS login code only supports DES passwords.  This change adds support for MD5 passwords.
(Our NIS server automatically uses MD5 once a password reaches a certain length..)

Testing Done

Just to be sure, are we absolutely positive this doesn't break existing NIS support?

/trunk/reviewboard/accounts/backends.py (Diff revision 1)

No need for parens. Also, you can just use [:3].

Change Summary:

OK - I made the suggested changes.

For 9/10 of our users the original unmodified code works fine.  The tenth user had a long password, so our NIS server uses an MD5 password.  The modified code now works for all 10 users, so I don't think this change breaks any existing uses.

Also, according to http://en.wikipedia.org/wiki/Crypt_(Unix) only an MD5 password will use a prefix of $1$.

Diff:

Revision 2 (+6 -1)

Show changes

/trunk/reviewboard/accounts/backends.py

/trunk/reviewboard/accounts/backends.py (Diff revision 2)

Looking at this again, I think we're doing too much work. Wouldn't this make more sense:

if original_crypted.startswith("$1$"):
    salt = original_crypted
else:
    salt = original_crypted[:2]

will_lentz Dec. 7, 2008, 12:54 p.m.

That's cool.  I didn't know crypt would work if you passed the entire encrypted password as the salt.  In my diff, for an MD5 encrypted password of:
  '$1$YeNsbWdH$wvOF8JdqsoiLix754LTW90'
I passing in a salt of:
  '$1$YeNsbWdH'
instead of your suggestion (which works too) of:
  '$1$YeNsbWdH$wvOF8JdqsoiLix754LTW90'
Since we can just pass in the entire password, how about the following change for line 18 (remove 4 characters)?
  new_crypted = crypt.crypt(password, original_crypted)
I tested the above change with original DES and MD5 password users and it works for me.  All the suggested changes work, though, so I'm happy with any of them.  Let me know what patch you prefer..

chipx86 Dec. 7, 2008, 12:59 p.m.

If this works, then that's great. Less good is preferable :) Let's take the latter patch, and if things break (sure hope not) we can always go back and use the more conservative approach.

Change Summary:

Incorporated feedback from diff version #2.

Diff:

Revision 3 (+1 -1)

Show changes

/trunk/reviewboard/accounts/backends.py

Ship it!

Looks great. Nice and simple. Thanks!

Committed as r1634.