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

Add key derivation function binding and appropriate tests #12

Merged
merged 2 commits into from
Nov 15, 2018

Conversation

mbestavros
Copy link
Contributor

@mbestavros mbestavros commented Nov 6, 2018

As the title suggests, this PR adds a binding for the PBKDF2-HMAC function in OpenSSL and configures it to match the implementation in Python-Keylime. An appropriate test is also added.

A few notes:

  • This is also mentioned in the function documentation, but it bears repeating: The only reason this binding uses SHA-1 is because Python-Keylime uses it at the moment. PyCryptodome defaults to SHA-1 as a PRF if the prf parameter is not specified, and Python-Keylime does not provide that parameter in its KDF binding.

  • The key the test compares against was generated from the Python-Keylime KDF binding using the same password and salt.

Tagging @frozencemetery and @leonjia0112 for review.

@mbestavros mbestavros force-pushed the crypto-kdf branch 3 times, most recently from 57cdbeb to 9fa496c Compare November 6, 2018 22:26
@leonjia0112
Copy link
Contributor

leonjia0112 commented Nov 7, 2018

@frozencemetery
I think you can try to delete all white space and type in space again. Otherwise, you can try rustfmt, that would solve this problem for you.

@mbestavros mbestavros force-pushed the crypto-kdf branch 3 times, most recently from 18f4a69 to 20440f6 Compare November 7, 2018 14:14
@mbestavros
Copy link
Contributor Author

@frozencemetery: Made a few small style changes; CI build looks like it's working. cargo is complaining that the call to pbkdf2_hmac isn't handling errors correctly, though it's just a warning at the moment. Not sure of the best way to handle that, though.

Copy link
Contributor

@frozencemetery frozencemetery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbestavros Basically its complaint is that pkcs5::pbkdf2_hmac has a return value which should be checked. How you do that depends a bit on the API you want to present; most likely, kdf() either needs to also return a Result<String>, or a failure brings down the system (unwrap or so).

@mbestavros mbestavros force-pushed the crypto-kdf branch 5 times, most recently from b53a0ad to 2053e7e Compare November 8, 2018 22:21
@mbestavros
Copy link
Contributor Author

mbestavros commented Nov 8, 2018

@frozencemetery: Got it; per our offline discussion, I made kdf and do_hmac pass the Result returned from OpenSSL down the line until it can be appropriately handled. Also made appropriate changes to main code to make sure the new function calls work. Figured the slight context-switch deserved its own commit.

Let me know what you think.

Copy link
Contributor

@frozencemetery frozencemetery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Squash the changes to kdf() into a single commit and shorten the second commit message, then it's ready to go.

@mbestavros mbestavros force-pushed the crypto-kdf branch 2 times, most recently from 4cfd7f2 to 1b3d09d Compare November 9, 2018 18:13
@mbestavros
Copy link
Contributor Author

@frozencemetery: Done, commits are properly separated now.

src/main.rs Outdated
@@ -84,7 +84,7 @@ fn response_function(req: Request<Body>) -> BoxFut {
let hmac = crypto::do_hmac(
common::KEY.to_string(),
challenge.unwrap().to_string(),
);
).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so this is better in that we're not panicing in the crypto layer, but this will still cause the server to panic if it fails. Can you just fail the request instead?

@mbestavros mbestavros force-pushed the crypto-kdf branch 2 times, most recently from db6b5be to 4a4d416 Compare November 14, 2018 16:02
@mbestavros
Copy link
Contributor Author

@frozencemetery Fixed; the call to HMAC from within main.rs now should fail gracefully. I used the match pattern; if there's a better way, please let me know.

@mbestavros mbestavros force-pushed the crypto-kdf branch 2 times, most recently from 8d3a56c to 8f51ccb Compare November 14, 2018 16:32
Copy link
Contributor

@frozencemetery frozencemetery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks!

@frozencemetery frozencemetery merged commit 1561312 into keylime:master Nov 15, 2018
@mbestavros mbestavros deleted the crypto-kdf branch November 19, 2018 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants