Opened 18 years ago

Closed 18 years ago

#3316 closed enhancement (fixed)

Adding `crypt' to list of password hashes for legacy apps.

Reported by: axiak@… Owned by: Adrian Holovaty
Component: Contrib apps Version: dev
Severity: normal Keywords:
Cc: dev@…, pythonmailing@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Crypt is still around here and there. When porting from a legacy app to django having crypt support is invaluable. This 3-line patch will add support to the auth library.

Attachments (3)

models.py.patch (471 bytes ) - added by axiak@… 18 years ago.
Patch to add crypt support to auth hashes
auth_crypt.patch (582 bytes ) - added by axiak@… 18 years ago.
Update to patch as per mir's comments.
ticket_3316_docs.patch (585 bytes ) - added by axiak@… 18 years ago.
The change in docs.

Download all attachments as: .zip

Change History (14)

by axiak@…, 18 years ago

Attachment: models.py.patch added

Patch to add crypt support to auth hashes

comment:1 by mir@…, 18 years ago

Has patch: set
Needs documentation: set
Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedDesign decision needed
Version: SVN

Hey, thanks for contributing. I have a few notes on your patch:

  • The crypt library is only available under Unix. I'm not sure about the policy regarding this.
  • Please don't add your login into the patch. It would make the code unreadable if every patch author did this.

by axiak@…, 18 years ago

Attachment: auth_crypt.patch added

Update to patch as per mir's comments.

comment:2 by Malcolm Tredinnick, 18 years ago

I'm probably in favour of this, since it does help with porting existing apps across to Django (you don't need to recreate all passwords). Feels like we should document why the functionality is there, though; just not sure where to do that (in the code or in a doc file somewhere).

Also, for anybody else trying to work out where this applies (since the patch is ambiguous), it is a patch against django/contrib/auth/models.py.

When things have quietened down a bit on the developer's list, we should bring this up for discussion (including the reasoning why it is useful). It is a change in direction, but that is probably just because we haven't thought of it before.

comment:3 by anonymous, 18 years ago

Patch needs improvement: unset

comment:4 by Simon G. <dev@…>, 18 years ago

Cc: dev@… added

for what it's worth, I'm +1 on this too - the whole point of the auth framework hashing is that it's extendible to other hash formats (this is why we have md5 and sha1). Crypt is a fairly common hash, and I'm sure a lot of people would find it useful.

However, I'm not sure if the if..elif..elif.. approach is best. It's a shame that the hash function in crypt isn't standardised (with "new()" and hexdigest(), or we could make this entirely future proof by doing something like:

try:
   hasher = __import__(algo)
   return hsh == hasher.new(salt+raw_password).hexdigest()

except ImportError:
   raise ValueError, "Got unknown password algorithm type (%s) in password." % algo

This way the you could use any hashing method you want.

comment:5 by axiak@…, 18 years ago

That does have the disadvantage of importing something from the database...

comment:6 by Simon G. <dev@…>, 18 years ago

Yes, I was really just thinking out loud, but I don't think there'll be any security issues - If someone tries to smuggle in some code in the "algo" slot, then import will fail (e.g. my hashing algorithm is called 'md5;print "fudge"') and raise an ImportError.

An attacker could create a new module (with new() doing something malicious), but this would require them to get the module into an importable location, and get the hashing algo to match that - I *think* this is impossible.

I could be wrong (shared hosting situations come to mind).

If we did want to go down this path, then you could always have a SAFE_HASHERS list, which could be added to in settings.py

comment:7 by Malcolm Tredinnick, 18 years ago

Triage Stage: Design decision neededReady for checkin

Simon, your dynamic import approach is dangerous for just the reasons you suggest: you are trusting externally supplied data as part of the security path. History suggests that as smart as programmers are, debugging is twice as hard and we will miss some sneaky way of smuggling in a bad import or having the bad import used in combination with a compromised machine to capture passwords (I have no idea how, but that's the point -- we can't think of everything). White-listing the permitted methods, whether through if...elif blocks or a list is the safer way.

I don't think the if...elif sequence is that bad. We need to have slightly different actions in each case and it's only around a dozen lines for the whole block. The alternative would be to look up the method in a table and dispatch to a separate function each time, which will end up with even more lines of code and harder to read. If that if-block gets to twice the size, we think of splitting it up. For now, I'm not unhappy with this.

I'm pretty happy with the patch. I'll apply it when I get a chance.

comment:8 by Marek Kubica <pythonmailing@…>, 18 years ago

Cc: pythonmailing@… added

I've found some pure Python implementation of crypt here and there. Althought I have never tried it, it may be worth including (in case import crypt fails with an ImportError) so that would work on Windows as well. Or maybe it's not that important - who uses crypt on Windows anyway? - but being platform independent as far as possible always seemed to me like a good idea.

comment:9 by Malcolm Tredinnick, 18 years ago

I don't think it's worth adding the software fallbacks. Both of the modules you point to admit to having problems in their code (and the one in the so-called public domain makes me nervous about the license -- it's not actually a good idea to release stuff into the public domain because it doesn't provide any legal protections at all).

Crypt is a Unix-based feature. So it's not really going to be useful on Windows setups and it will mean we're maintaining even more encryption methods: real crypt, Python crypt, etc... Fitting in with the existing server makes some sense, since password strings can be shared. Trying to port it to platforms where it isn't a used format, not so much.

by axiak@…, 18 years ago

Attachment: ticket_3316_docs.patch added

The change in docs.

comment:10 by anonymous, 18 years ago

Needs documentation: unset

comment:11 by Malcolm Tredinnick, 18 years ago

Resolution: fixed
Status: newclosed

(In [5073]) Fixed #3316 -- Added support for crypt hashing of passwords, mostly to support
easy porting from existing Unix-based legacy apps. Thanks, axiak@….

Note: See TracTickets for help on using tickets.
Back to Top