Drop passlib #14
Drop passlib #14
Conversation
| sb = bytes(salt, "utf-8") | ||
| pb = bytes(password, "utf-8") |
There was a problem hiding this comment.
The original code looks like:
| sb = bytes(salt, "utf-8") | |
| pb = bytes(password, "utf-8") | |
| sb = bytes(salt, "ascii") | |
| pb = bytes(password, "iso-8859-1") |
salt and password are strings. A str decoded into bytes with "utf-8" encoding will not necessarily have the same bytes as "iso-8859-1". If the string has anything beyond ASCII (ASCII has codepoints 0-127, and iso-8859-1 also has codepoints 128-255), then utf-8 would use two bytes instead of iso-8859-1's single byte.
(See: https://stackoverflow.com/a/7048780)
I'm wondering about backwards compatibility and why you changed this. I'm guessing passlib effectively used utf-8, right? A quick glance at the (very convoluted) passlib sources suggests it used utf-8 by default when loading the htpasswd file. Since we didn't use the encoding param of the HtpasswdFile class, we effectively always used that default. Did you run into any failed tests that triggered changing these lines?
There was a problem hiding this comment.
The code hadn't been touched in 10 years, so I was thinking most modern system would be using utf-8. I tested the toto password using the ascii and utf-8 encodings and they produced the same hash, but that might have been just luck because they are ascii characters and had it been accented characters in the password, it might not produce the correct result.
I could expand the range of characters tested in the password to hashing too.
There was a problem hiding this comment.
Let's punt on this one. Since we've been using utf-8 by default any way, this probably isn't a backwards compatibility issue. We can add tests if someone files an issue about character encoding issues.
| # User not found. | ||
| return None |
There was a problem hiding this comment.
It looks like passlib had a dummy feature that made user-not-found errors take a little bit longer to return, so that someone can't use timing to distinguish bad password from bad user. Do we care about that?
There was a problem hiding this comment.
I didn't know passlib did that, but yes, it could be reasonable to add a random timeout. Does 300-500ms sound reasonable?
There was a problem hiding this comment.
Looks like the dummy method just hashes a fake password ignoring the result.
|
Now that this is merged, we need to update the lockfile in st2.git to finish removing passlib. |
3 participants
This PR is follow on work from StackStorm/st2#6350 which removes passlib from st2's requirements.