Opened 13 years ago

Closed 13 years ago

#16262 closed New feature (fixed)

ValueError when authenticating

Reported by: Mitar Owned by: nobody
Component: contrib.auth Version: 1.3
Severity: Normal Keywords: auth backend
Cc: mmitar@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When using additional custom authentication backend (for crypt-like password hashes) I am getting such error from Django authentication backend if it is first in the order:

File "/usr/lib/pymodules/python2.6/django/contrib/auth/models.py" in check_password
  40.     algo, salt, hsh = enc_password.split('$')

Exception Type: ValueError at /admin/
Exception Value: too many values to unpack

This is normal, as crypt-based password hashes have one more $ in there. But I would like that Django backend is first so that it is tries first as most of the users are using Django authentication backend.

I am proposing that this exception is simply catched and authentication backend returns None. I am attaching a patch for this.

Attachments (2)

auth-backend.patch (374 bytes ) - added by Mitar 13 years ago.
auth-models.patch (741 bytes ) - added by Mitar 13 years ago.

Download all attachments as: .zip

Change History (18)

by Mitar, 13 years ago

Attachment: auth-backend.patch added

comment:1 by Mitar, 13 years ago

Has patch: set

Maybe also example backend in the documentation could include such except clause. I see it is as a good practice not to propagate authentication errors to the user.

comment:2 by Stephen Burrows, 13 years ago

Keywords: auth backend added
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

This seems like a valid issue, but the patch needs to be from the root of the django directory for two reasons: 1) so that it can be applied easily, and 2) so that people can tell what file(s) exactly you're patching. Also, from your traceback it appears that the ValueError is in contrib/auth/models.py - why not catch the ValueError there?

comment:3 by Stephen Burrows, 13 years ago

I can confirm that this happens; it seems like check_password should simply return False if the password is not correctly formatted. https://code.djangoproject.com/browser/django/trunk/django/contrib/auth/models.py#L37

by Mitar, 13 years ago

Attachment: auth-models.patch added

comment:4 by Mitar, 13 years ago

Patch needs improvement: unset

Added new patch which fixes check_password function.

comment:5 by Stephen Burrows, 13 years ago

Looks fine to me, but I'd feel strange RFCing it since I accepted it.

comment:6 by Mitar, 13 years ago

I do not understand? Should I RFC it? Or what's the idea? (I am not familiar with Django triage process.)

comment:7 by Aymeric Augustin, 13 years ago

Anyone but the author can review a patch and, if it fulfills Django's standards, mark the ticket as RFC.

melinath decided not to do it, probably because (s)he wanted someone to look into this issue.

So right now, you need someone else to review your patch.

comment:8 by Jannis Leidel, 13 years ago

Needs tests: set

I don't see at all how crypt based passwords could have more than two dollar signs, please elaborate and provide a test.

comment:9 by Mitar, 13 years ago

Of course they have. The syntax is $algorithm$salt$hash. But it is not really important here. Important is that check does not fail for any input hash string with an exception, but with returning True or False.

I do not think tests are necessary here as we are just making function more robust for any type of the input.

comment:10 by Jannis Leidel, 13 years ago

Resolution: worksforme
Status: newclosed
Last edited 13 years ago by Jannis Leidel (previous) (diff)

comment:11 by Mitar, 13 years ago

Resolution: worksforme
Status: closedreopened

Can you please read my initial ticket description. What I am explaining is that I would like to have a chain of backends (as it is documented that I can have) and some of them can have different password/hash scheme (completely different to what it is in models.py). Now, the problem is, that Django's check_password throws an exception (instead of returning False) when encountering a password/hash scheme it does not know how to parse, preventing other backends to run. So this is clearly a bug as: check_password is throwing an exception when the API says it will return only True or False and as it practically makes chains of backends useless. Of couse, backends could use some other field/database to authenticate, but they could also use the same field in the database, extending the range of possible hashing algorithms/schemes. And this is currently not possible because of the lacking simple try/except in the function.

I really do not understand what is problem with adding a simple try/except around which would make things more robust, extendable and thus usable? What is argument against adding it?

BTW, I am talking about cyrpt scheme and not Django scheme. And crypt scheme can have zero (for legacy) or three dollar characters. Check your local /etc/shadow file.

comment:12 by Jannis Leidel, 13 years ago

Resolution: needsinfo
Status: reopenedclosed

First of all, I read the ticket description and wasn't able to get the information you just provided, which is why I first asked you to provide a test case that demonstrates the alleged bug.

I understand that Django throws an exception for password hashes that contain more than two dollar signs, which is honestly fine by me since it proves that the stored password hash doesn't follow the password format Django requires. I was additionally thrown off by the fact that Django itself support crypt as an alternative hashing algorithm, making it even more unclear what you were talking about. So again, please provide a concrete case in which this code explodes and I'd be happy to reconsider.

comment:13 by Mitar, 13 years ago

Resolution: needsinfo
Status: closedreopened

Isn't one of Django's principles also loose coupling? I think that in this particular case it means that the password field of User model could contain something else than what Django is expecting. OK, it is normal that check_password function cannot be magical and validate the password if the field contains garbage (from its perspective). But it should also not blow up.

Crypt was just an example, the same holds also for apr1 (Apache runtime) hashing algorithm. Or any other value I would like to store in the password field of User model which might not contain exactly two $.

So concrete case: I cannot store this value $1$JDE7PpXQ$8bV7aOArT3P91NHFaI7vpg in the password field of the User model. And my other authentication backend knows what to do with that. But my authentication backend is never called because Django's one throws an exception. I also cannot store $apr1$HSSoQQ2h$1S454HzbLe/ewAAWhmSnv. to use in another of my backends.

I really do not understand why it is so hard to add that try/except around? It would allow Django users to have their own values in the password field. It is just something useful and good.

(Reopened as I have provided concrete case in which the code explodes.)

in reply to:  13 comment:14 by Jannis Leidel, 13 years ago

Needs documentation: set
Triage Stage: AcceptedDesign decision needed
Type: BugNew feature

Replying to mitar:

Isn't one of Django's principles also loose coupling? I think that in this particular case it means that the password field of User model could contain something else than what Django is expecting. OK, it is normal that check_password function cannot be magical and validate the password if the field contains garbage (from its perspective). But it should also not blow up.

Django specifically doesn't provide a pluggable interface for the password hashing algorithm, so arguing for "loose coupling" is kinda a moot point. I agree the auth system shouldn't blow up of course -- if used as expected. If you're using it in ways it wasn't meant to be used, that's a different story.

Crypt was just an example, the same holds also for apr1 (Apache runtime) hashing algorithm. Or any other value I would like to store in the password field of User model which might not contain exactly two $.

Right, in that sense it's not at all a bug but a feature request, which is part of a bigger discussion about the auth backends.

So concrete case: I cannot store this value $1$JDE7PpXQ$8bV7aOArT3P91NHFaI7vpg in the password field of the User model. And my other authentication backend knows what to do with that. But my authentication backend is never called because Django's one throws an exception. I also cannot store $apr1$HSSoQQ2h$1S454HzbLe/ewAAWhmSnv. to use in another of my backends.

Right, again this is a known limitation of the auth system, but not a "bug" you trying to make out of that. Marking as DND because I'm not sure how this should look like. Feel free to bring this topic up on the django-developers mailing list for further discussion about extending the auth backend for pluggable password hashing. Note, there might have already been a discussion about that, so please search the Groups first.

I really do not understand why it is so hard to add that try/except around? It would allow Django users to have their own values in the password field. It is just something useful and good.

Because adding a try block hides the fact that the password you're using with Django's auth system isn't officially supported by it.

comment:15 by Jannis Leidel, 13 years ago

Triage Stage: Design decision neededAccepted

After talking to some during the sprint, marking as accepted.

comment:16 by Jannis Leidel, 13 years ago

Resolution: fixed
Status: reopenedclosed

In [16456]:

Fixed #14390 and #16262 -- Moved password related functions from auth models to utils module and stopped check_password from throwing an exception. Thanks, subsume and lrekucki.

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