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

Adds recipe for salt and hash a password using ring crate #322

Merged
merged 1 commit into from Oct 9, 2017

Conversation

Projects
None yet
2 participants
@j-haj
Copy link
Contributor

j-haj commented Oct 8, 2017

Adds recipe for #288

  • Slightly changes the wording on the example (explicitly mentioning PBKDF2)
  • Uses PBKDF2 key derivation function
  • Verifies password with hash

cc #213

fixes #288

@budziq
Copy link
Collaborator

budziq left a comment

@j-haj That is an excellent example!
Just some minor stylistic suggestions below and we are ready to merge!

  • also please run rustfmt on the example.
let mut salt = [0u8; CREDENTIAL_LEN];
let mut pbkdf2_hash = [0u8; CREDENTIAL_LEN];
rng.fill(&mut salt)?;

This comment has been minimized.

@budziq

budziq Oct 8, 2017

Collaborator

salt generation is a crucial step that can be easily missed in this example. I would suggest adding comment above // Generate salt and mention it ind the textual description above (with links to fill)

This comment has been minimized.

@j-haj

j-haj Oct 9, 2017

Author Contributor

Excellent point! I updated the discussion and modified the code organization to make the salt generation stand out

fn run() -> Result<()> {
let rng = rand::SystemRandom::new();
static DIGEST_ALG: &'static digest::Algorithm = &digest::SHA512;

This comment has been minimized.

@budziq

budziq Oct 8, 2017

Collaborator

I would just use &digest::SHA512 directly everywhere. I feel Its less noisy and easier to understand that way.

This comment has been minimized.

@j-haj

j-haj Oct 9, 2017

Author Contributor

Agreed -- good suggestion

<a name="ex-pbkdf2"></a>
## Salt and hash a password with PBKDF2

[![ring-badge]][ring] [![cat-cryptography-badge]][cat-cryptography]

This comment has been minimized.

@budziq

budziq Oct 8, 2017

Collaborator

hmm the data_encoding crate is actually besides the point of the example but we might still add it to the crate list. I'll leave the decision up to you.

This comment has been minimized.

@j-haj

j-haj Oct 9, 2017

Author Contributor

Added

use ring::rand::SecureRandom;
fn run() -> Result<()> {
let rng = rand::SystemRandom::new();

This comment has been minimized.

@budziq

budziq Oct 8, 2017

Collaborator

I would put the rng initiation below the consts declarations

This comment has been minimized.

@j-haj

j-haj Oct 9, 2017

Author Contributor

Done

let password = "password";
pbkdf2::derive(DIGEST_ALG, N_ITER, &salt, password.as_bytes(),
&mut pbkdf2_hash);
println!("PBKDF2 hash: {}", HEXUPPER.encode(&pbkdf2_hash));

This comment has been minimized.

@budziq

budziq Oct 8, 2017

Collaborator

lets also printout the salt here

rng.fill(&mut salt)?;
// Hash password
let password = "password";

This comment has been minimized.

@budziq

budziq Oct 8, 2017

Collaborator

it might be easier to spot if we use value other than variable name. how about something based on https://xkcd.com/936/ ;)

This comment has been minimized.

@j-haj

j-haj Oct 9, 2017

Author Contributor

Updated! If only I was as witty as xkcd...

let hash_verification = pbkdf2::verify(DIGEST_ALG, N_ITER, &salt,
password.as_bytes(), &pbkdf2_hash);
match hash_verification {
Ok(_) => println!("Successfully verified password"),

This comment has been minimized.

@budziq

budziq Oct 8, 2017

Collaborator

How about we provide two separate calls to verify one with correct password and second one with bogus one each succeeded by call to assert!(hash_verification.is_ok()); instead of the match and println!?

I'm not sure what would be nicer here. So I'll let you decide.

This comment has been minimized.

@j-haj

j-haj Oct 9, 2017

Author Contributor

I added the asserts, however on the failing assert I do assert!(!should_fail.is_ok()); so we don't have to worry about a failing assert. I have encountered differing opinions on this approach, so I am happy to remove the negation if you prefer!

@j-haj

This comment has been minimized.

Copy link
Contributor Author

j-haj commented Oct 9, 2017

Excellent feedback as always. Just want to run everything through rustfmt and I'll update the PR.

@j-haj j-haj force-pushed the j-haj:master branch from 03b6c35 to 58226e1 Oct 9, 2017

Adds recipe for salt and hash a password using ring crate
* Uses PBKDF2 key derivation function
* Verifies password with hash

@j-haj j-haj force-pushed the j-haj:master branch from 58226e1 to eacf144 Oct 9, 2017

@j-haj

This comment has been minimized.

Copy link
Contributor Author

j-haj commented Oct 9, 2017

One other thing I wanted to run by you was the formatting on some of the function calls to derive/verify. Rustfmt reformatted two of them into the argument column formatting seen in the example (for consistency I added the third). I like this formatting but it makes the example a bit longer in terms of line count. I'm happy to flatten things out if you prefer!

@budziq budziq merged commit 45b7857 into rust-lang-nursery:master Oct 9, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@budziq

This comment has been minimized.

Copy link
Collaborator

budziq commented Oct 9, 2017

Thanks @j-haj ! That is very fine work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.