Code

Opened 3 years ago

Closed 3 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 3 years ago.
auth-models.patch (741 bytes) - added by mitar 3 years ago.

Download all attachments as: .zip

Change History (18)

Changed 3 years ago by mitar

comment:1 Changed 3 years ago by mitar

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 Changed 3 years ago by melinath

  • Keywords auth backend added
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

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 Changed 3 years ago by melinath

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

Changed 3 years ago by mitar

comment:4 Changed 3 years ago by mitar

  • Patch needs improvement unset

Added new patch which fixes check_password function.

comment:5 Changed 3 years ago by melinath

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

comment:6 Changed 3 years ago by mitar

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

comment:7 Changed 3 years ago by aaugustin

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 Changed 3 years ago by jezdez

  • 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 Changed 3 years ago by 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.

comment:10 Changed 3 years ago by jezdez

  • Resolution set to worksforme
  • Status changed from new to closed
Version 0, edited 3 years ago by jezdez (next)

comment:11 Changed 3 years ago by mitar

  • Resolution worksforme deleted
  • Status changed from closed to reopened

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 Changed 3 years ago by jezdez

  • Resolution set to needsinfo
  • Status changed from reopened to closed

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 follow-up: Changed 3 years ago by mitar

  • Resolution needsinfo deleted
  • Status changed from closed to reopened

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

comment:14 in reply to: ↑ 13 Changed 3 years ago by jezdez

  • Needs documentation set
  • Triage Stage changed from Accepted to Design decision needed
  • Type changed from Bug to New 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 Changed 3 years ago by jezdez

  • Triage Stage changed from Design decision needed to Accepted

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

comment:16 Changed 3 years ago by jezdez

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.