Warning: Can't synchronize with repository "(default)" (Unsupported version control system "svn": No module named svn). Look in the Trac log for more information.

Ticket #1281 (closed defect: fixed)

Opened 11 years ago

Last modified 10 years ago

[PATCH][TEST] identity throws exceptions for encrypted passwords with unicode characters

Reported by: Felix.Schwarz Owned by: anonymous
Priority: normal Milestone: 1.0.2
Component: Identity Version: 1.0.1
Severity: normal Keywords:
Cc:

Description

The current identity encryption code does only work with 8 bit Python strings, not with unicode strings.

...
  File "/home/fs/jk/site-packages/TurboGears-1.0.1-py2.3.egg/turbogears/identity/visitor.py", line 165, in record_request
    identity= self.identity_from_request(visit.key)
  File "/home/fs/jk/site-packages/TurboGears-1.0.1-py2.3.egg/turbogears/identity/visitor.py", line 89, in identity_from_request
    identity= source(visit_key)
  File "/home/fs/jk/site-packages/TurboGears-1.0.1-py2.3.egg/turbogears/identity/visitor.py", line 146, in identity_from_form
    identity= self.provider.validate_identity( user_name, pw, visit_key )
  File "/home/fs/jk/site-packages/TurboGears-1.0.1-py2.3.egg/turbogears/identity/soprovider.py", line 224, in validate_identity
    if not self.validate_password(user, user_name, password):
  File "/home/fs/jk/site-packages/TurboGears-1.0.1-py2.3.egg/turbogears/identity/soprovider.py", line 249, in validate_password
    return user.password == self.encrypt_password(password)
  File "/home/fs/jk/site-packages/TurboGears-1.0.1-py2.3.egg/turbogears/identity/soprovider.py", line 189, in <lambda>
    self.encrypt_password= lambda pw: md5.new(pw).hexdigest()
UnicodeEncodeError: 'ascii' codec can't encode character u'\xf6' in position 4: ordinal not in range(128)

I know hashing/encryption works on the byte level and has no notation for "unicode". Therefore Python's unicode strings must be converted to old 8 bit strings.

But using unicode characters in your password makes it much stronger and in many countries, characters as 'ä', 'ö' etc. are used very often in common words. Therefore users want to use these characters in there passwords.

IMHO the obvious strategy to fix this defect is to add a condition statement before actually hashing the password.

if isinstance(pw, unicode):
    pw = pw.encode('UTF-8')

The problem must be solved in TurboGears because the exception is thrown before any user written code gets the chance to inspect/change the parameters.

I will attach a patch for the test suite which uncovers the problem.

Attachments

unicode_with_encrypted_passwords.patch Download (2.9 KB) - added by Felix.Schwarz 11 years ago.
identity_unicode.patch Download (3.8 KB) - added by plewis 11 years ago.
identity_unicode_v2.patch Download (5.4 KB) - added by Felix.Schwarz 11 years ago.
revised patch version which solves the mentioned problems
identity_unicode_tests.patch Download (3.1 KB) - added by plewis 11 years ago.
identity_unicode_v3.patch Download (8.5 KB) - added by Felix.Schwarz 11 years ago.

Change History

Changed 11 years ago by Felix.Schwarz

comment:1 Changed 11 years ago by Felix.Schwarz

This test suite patch includes my patch für #1264 as this issue is a blocker for me on Python 2.3.

Please not that there are some very rough places in the patch: I'm removing IdentityVisitPlugins? manually from a private list. If anyone has a better solution for this problem (update the encryption setting), please change the patch accordingly. Maybe a new interface is needed...

Furthermore another test is failing due to this test case patch. I think this is because the encryption setting is not reverted back to its original value properly.

comment:2 Changed 11 years ago by plewis

I think the identity_unicode patch fixes the problem. All identity tests still pass.

Changed 11 years ago by plewis

comment:3 Changed 11 years ago by plewis

  • Summary changed from identity throws exceptions for encrypted passwords with unicode characters to [PATCH][TEST] identity throws exceptions for encrypted passwords with unicode characters

comment:4 Changed 11 years ago by Felix.Schwarz

I have some comments about your patch:

  1. You change the encoding of test_identity.py to "latin-1". I'm not sure what the general policy is here but IMHO we should use ASCII or UTF-8. Everything is only a solution for some people and UTF-8 unifies everything.
  2. Your changes to saprovider.py do not have a test.
  3. IMHO your change to test_user_password_raw is wrong (setting the encryption to None). If there is no encryption, you could use 'user.password = ...' as well. The point is to test the set_password_raw.
  4. I'm not sure that multiple assertions are a good thing in test cases. What about adding new ones?

comment:5 Changed 11 years ago by Felix.Schwarz

Some more comments

  1. Your patch does not follow the patch guidelines (diff was not created in the project root but in turbogears/identity).
  2. The encryption function is duplicated in soprovider and saprovider.

Changed 11 years ago by Felix.Schwarz

revised patch version which solves the mentioned problems

comment:6 Changed 11 years ago by plewis

made changes to tests as requested (in identity_unicode_tests.patch)

  1. now UTF-8 encoded
  2. test to test_user_password was wrong. Meant to be testing just plain password setting without an encryption algorithim. This test should be in the new patch.
  3. removed multiple assertions inside of a test by writing new tests.

I don't see 2. and 3. in the identity_unicode_v2.patch, so I thought I would submit it anyways.

Changed 11 years ago by plewis

comment:7 Changed 11 years ago by Felix.Schwarz

Nice patch. We should try to merge identity_unicode_tests.patch with identity_unicode_v2.patch and come up with one unified patch. Maybe I get to this tomorrow.

Btw: Encoding unconditionally to UTF-8 is bad (there are test failures in my private acceptance tests). We should do this conditionally although I don't know how to make a unit test for that.

Changed 11 years ago by Felix.Schwarz

comment:8 Changed 11 years ago by Felix.Schwarz

I attached a unified patch which integrates plewis' changes. Basically, it uses his test cases from identity_unicode_tests.patch and my changes in the turbogears.identity code from identity_unicode_v2.patch.

There was on additional change:

  • The password is now only encoded as UTF-8 it is a unicode string. 8 bit Python strings won't get encoded. I added an additional test case for it.

Minor changes:

  • some small fixes for pep 8 compatibility (coding style)
  • docstring additions in the new test cases
  • one minor docstring fix (s/sha/md5/)

plewis: Is this patch okay with you? Ready for commit? :-)

comment:9 Changed 11 years ago by Felix.Schwarz

Forgot to mention: Diffed against r2573 now.

comment:10 Changed 11 years ago by plewis

Looks good to me. I applied the v3 patch, and all identity tests pass.

comment:11 follow-up: ↓ 12 Changed 11 years ago by jorge.vargas

haven't test it but it seems like a good fix, I saw we can commit it.

comment:12 in reply to: ↑ 11 Changed 11 years ago by jorge.vargas

Replying to jorge.vargas:

haven't test it but it seems like a good fix, I say we can commit it.

comment:13 Changed 10 years ago by alberto

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

Applied at [2826]. Thanks! :) (and sorry for the delay)

Alberto

Note: See TracTickets for help on using tickets.