ValueError when authenticating

[Mitar]

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.

[Mitar]

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.

[Stephen Burrows]

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?

[Stephen Burrows]

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

[Mitar]

Added new patch which fixes check_password function.

[Stephen Burrows]

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

[Mitar]

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

[Aymeric Augustin]

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.

[Jannis Leidel]

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

[Mitar]

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.

[Jannis Leidel]

Well, there are clearly only two dollar signs in https://code.djangoproject.com/browser/django/trunk/django/contrib/auth/models.py?rev=16022#L251

[Mitar]

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.

[Jannis Leidel]

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.

[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.

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.)

[Jannis Leidel]

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.

[Jannis Leidel]

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

[Jannis Leidel]

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.