Make PBKDF hash field optional#477
Conversation
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
|
@frankdavid Thanks for the contribution! I'll review this a little bit later today. Can you rebase onto the default branch in the meantime in preparation for merge? |
jbaublitz
left a comment
jbaublitz
left a comment
There was a problem hiding this comment.
Just two code simplification requests. I believe you also mentioned that other fields may be optional. Did it turn out that we only need to wrap the hash in an Option because all other fields can easily represent the default?
| let hash_cstring = self | ||
| .hash | ||
| .as_ref() | ||
| .map(|h| CString::new(h.as_bytes()).map_err(LibcryptErr::NullError)) |
There was a problem hiding this comment.
I believe this can be simplified with the to_cstring! macro.
| let hash = if v.hash.is_null() { | ||
| None | ||
| } else { | ||
| Some(from_str_ptr!(v.hash)?.to_string()) | ||
| }; |
There was a problem hiding this comment.
I believe this block can be simplified with the ptr_to_option! macro in combination with the from_str_ptr! macro and transpose.
I looked into it and the main issue is that the backing C API does not have official sentinel values. The integer fields contain 0 when the value is "empty" but I felt it's quite risky to interpret 0 as missing (what if the backing implementation starts using 0 as a non-empty value?). With the current way, the Rust wrapper just lets the 0s through without interpreting them. In order to move to |
jbaublitz
left a comment
jbaublitz
left a comment
There was a problem hiding this comment.
I 100% agree and have generally tended toward that approach throughout the library as you can probably see. Thanks for making the revisions. I'll approve and merge.
3 participants
Fixes #473
The hash field is not always populated. The library should not try to create a String instance if hash is null.