https://bugzilla.wikimedia.org/show_bug.cgi?id=28419
--- Comment #23 from Rob Lanphier <ro...@wikimedia.org> 2012-03-24 20:39:31 UTC --- Sorry that this review is backed up in the queue. I happened to notice Django's latest release announcement[1] (they just switched to PBKDF2) and it reminded me about this issue. I think once we get our security engineer hired, we'll be able to take a closer look at this. This is in a particularly important area for correctness, and we're pretty conservative about futzing with it. It's also a potential migration headache for existing users (including Wikimedia), so we need to be careful about what we do. My initial feedback on the patch: it seems like going with PBKDF2 is a pretty defensible position. I think it'll still be important to respond to all of Tim's points in his original email, but I also don't think we're going to be able to wait for Tim to have the time to implement this exactly the way he envisions. Thank you Tyler for moving this forward with some code. The one thing that I think is a very important point of his original suggestions is this "2. Upgradeable: it should be possible to compute the C-type hash from the B-type hash, to allow upgrades without bothering users." Does this patch achieve this goal? I can't quite tell. One big area for improvement is the function naming. Our current function naming is tragic, and this patch compounds the error in a way that is both confusing and backwards-incompatible. The current API uses "oldCrypt" for A-type crypt strings, and "crypt" for B-type crypt strings. This patch uses "reallyOldCrypt" for A-type, "oldCrypt" for B-type crypt, and "crypt" for C-type. "really"? really? :-) I won't give you too much grief, Tyler, since you seem to have just been following the convention of an unfamiliar (and, in this case, silly) codebase, but I think this is one time that deference to tradition doesn't work. Here's my suggestion for naming functions: implement cryptTypeA(), cryptTypeB(), and cryptTypeC(). Have a deprecated "oldCrypt" call cryptTypeA(), and implement the crypt type selection logic in the new crypt(). [1] https://docs.djangoproject.com/en/dev/topics/auth/#auth-password-storage -- Configure bugmail: https://bugzilla.wikimedia.org/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug. _______________________________________________ Wikibugs-l mailing list Wikibugs-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
