Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate wallet passwords as UTF-8 #1205

Open
michaelsproul opened this issue May 27, 2020 · 4 comments
Open

Validate wallet passwords as UTF-8 #1205

michaelsproul opened this issue May 27, 2020 · 4 comments

Comments

@michaelsproul
Copy link
Member

Description

Presently we accept arbitrary bytes from a file as a password, which doesn't seem like the most intuitive UX, particularly when we want to treat it like text and strip out newline characters as per #1175. Making the PlainText type contain an actual String would make the newline stripping a lot cleaner too.

@michaelsproul michaelsproul added code-quality RFC Request for comment labels May 27, 2020
@michaelsproul
Copy link
Member Author

For reference, newline stripping becomes as straightforward as:

fn main() {
    println!("{:?}", "mysupersecurepassword\r\n".trim_end_matches(|c| c == '\r' || c == '\n'));
    println!("{:?}", "mysupersecurepassword\r\n\r\n\r\r".trim_end_matches(|c| c == '\r' || c == '\n'));
}

Playground Link

Which is simpler than working with bytes as in #1199

@paulhauner
Copy link
Member

Makes sense to me!

@michaelsproul michaelsproul changed the title Consider validating wallet passwords as UTF-8 Validate wallet passwords as UTF-8 May 28, 2020
@michaelsproul michaelsproul added good first issue Good for newcomers and removed RFC Request for comment labels May 28, 2020
@roynalnaruto
Copy link
Contributor

Description

Presently we accept arbitrary bytes from a file as a password, which doesn't seem like the most intuitive UX, particularly when we want to treat it like text and strip out newline characters as per #1175. Making the PlainText type contain an actual String would make the newline stripping a lot cleaner too.

Oh, I saw this issue only after you had merged the previous PR. Anyway, I have made the update to support UTF-8 here

@realbigsean
Copy link
Member

I had a couple questions about this after looking at #2070

  • is there any reason not to use ZeroizeString everywhere?
  • is there any reason not to remove PlainText entirely (are there any places we may still may want a zeroizing Vec<u8>)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants