[Merged by Bors] - Normalize keystore passwords #1972
Conversation
|
@kirk-baird with #1928 , we are returning an error if we try encrypting using a password with control chars.
This is what the deposit-cli seems to be doing here So if the user entered a password with control characters in the deposit-cli, it seems they would get stripped and fail with the import in lighthouse. Made c5daab7 to strip the characters instead of throwing an error. Can you please check if I understood the spec correctly here? :) |
Yep I'm happy with your interpretation to strip away any control characters rather than error. Thanks for fixing this up! I had a look through |
|
Added normalization to the |
|
|
||
| let derived_key = derive_key(&password, &kdf)?; | ||
| password.retain(|c| !is_control_character(c)); |
There was a problem hiding this comment.
The comment at the top of this function
(Errors) if
passworduses utf-8 control characters.
Is out of date
| /// Returns true if the given char is a UTF-8 control character and false otherwise. | ||
| fn is_control_character(c: char) -> bool { | ||
| // 0x00 - 0x1F + 0x80 - 0x9F + 0x7F | ||
| c.is_control() |
There was a problem hiding this comment.
Having reviewed the control characters listed in UnicodeData.txt I'm satisfied that this is equivalent to the set of control characters claimed (and required by the spec). Nice!
There was a problem hiding this comment.
Haha just saw Kirk's comment. Oh well, now we've triple checked it
michaelsproul
added
waiting-on-author
michaelsproul
added
ready-for-merge
|
bors r+ |
## Issue Addressed Resolves #1879 ## Proposed Changes Do NFKD normalization for keystore passwords.
|
Pull request successfully merged into unstable. Build succeeded: |
bors
bot
changed the title
Issue Addressed
Resolves #1879
Proposed Changes
Do NFKD normalization for keystore passwords.