-
Notifications
You must be signed in to change notification settings - Fork 58
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
Conversation
57cdbeb
to
9fa496c
Compare
@frozencemetery |
18f4a69
to
20440f6
Compare
@frozencemetery: Made a few small style changes; CI build looks like it's working. |
20440f6
to
c18244d
Compare
There was a problem hiding this 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).
b53a0ad
to
2053e7e
Compare
@frozencemetery: Got it; per our offline discussion, I made Let me know what you think. |
There was a problem hiding this 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.
4cfd7f2
to
1b3d09d
Compare
@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(); |
There was a problem hiding this comment.
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?
db6b5be
to
4a4d416
Compare
@frozencemetery Fixed; the call to HMAC from within |
8d3a56c
to
8f51ccb
Compare
8f51ccb
to
8c4ce4f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks!
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.