-
Notifications
You must be signed in to change notification settings - Fork 151
Generate dummy mlkem bindings if mlkem.h is missing #471
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
base: master
Are you sure you want to change the base?
Conversation
boring-sys/src/mlkem_dummy.rs
Outdated
|
|
||
| fn fail() { | ||
| unsafe { | ||
| ERR_put_error( |
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.
This only records an error, right? Doesn't halt the execution.
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.
Yeah. I've renamed it to just put_error.
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.
Shouldn't we panic and halt execution to prevent the thing you worried about?
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.
The void functions I'm worried about panic via unimplemented!(), which will halt the execution one way or another (unwind or abort). Other functions that can report errors do it in the normal way by pushing an error on the stack and returning a failure code (these functions are documented to return 1 on success and 0 otherwise). I wanted this to be most gracefully-degrading as possible.
36a547e to
fb69e23
Compare
fb69e23 to
d636b97
Compare
boring-sys/src/mlkem_dummy.rs
Outdated
| _seed: *const u8, | ||
| _seed_len: usize, | ||
| ) -> ::std::os::raw::c_int { | ||
| put_error(); |
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.
We should panic here too because MLKEM1024_private_key_from_seed doesn't error if you give it the right parameters and so an error check could be omitted.
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.
Sigh, another C API being one cut corner away from UB and vulnerabilities. I've slapped abort() on all of them.
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.
🙏
boring-sys/src/mlkem_dummy.rs
Outdated
| _ciphertext_len: usize, | ||
| _private_key: *const MLKEM1024_private_key, | ||
| ) -> ::std::os::raw::c_int { | ||
| put_error(); |
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.
Same.
boring-sys/src/mlkem_dummy.rs
Outdated
| _seed_len: usize, | ||
| ) -> ::std::os::raw::c_int { | ||
| put_error(); | ||
| 0 |
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.
Same.
boring-sys/src/mlkem_dummy.rs
Outdated
| _private_key: *const MLKEM768_private_key, | ||
| ) -> ::std::os::raw::c_int { | ||
| put_error(); | ||
| 0 |
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.
Same.
boring-sys/src/mlkem_dummy.rs
Outdated
| public_key: *const MLKEM768_public_key, | ||
| ) -> ::std::os::raw::c_int { | ||
| put_error(); | ||
| 0 |
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.
Same.
boring-sys/src/mlkem_dummy.rs
Outdated
| public_key: *const MLKEM1024_public_key, | ||
| ) -> ::std::os::raw::c_int { | ||
| put_error(); | ||
| 0 |
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.
Same.
d636b97 to
d3fb091
Compare
d3fb091 to
b8a20c8
Compare
Includes #470
If C lacks MLKEM APIs, and the
mlkemCargo feature isn't requiring a real MLKEM support,boring-syswill inject an always-failing fallback to allow Rust bindings to fail at run time instead of compile time.It uses a glob to add the fallback, because Rust always prefers real definitions over globs, so the real definitions will be used if they're available.