Add OSCORE Support #47
Conversation
…ons to add oscore conf to context for servers and to add new recipients
…on-volatile storage
…ence_number() using Option and pattern-matching
…dd_oscore_conf() is deprecated
…of OscoreConf & OscoreRecipient
…o it's raw struct)
…formation has been added to the context
…ep track of its pointer
pulsastrix
added
enhancement
Workflow Status ReportGenerated for commit b9976cf on Thu May 15 18:27:22 UTC 2025.
In case of failure, clippy warnings and rustfmt changes (if any) will be indicated as CI check warnings in the file comparison view. Documentation: Download Coverage Report: Download Note: Online versions of documentation and coverage reports may not be available indefinitely, especially after the pull request was merged. Code Coverage ReportCoverage target is 80%. Expand to view coverage statistics
Total coverage: 53.23% |
There was a problem hiding this comment.
Hi, thanks for the contribution!
You can find my initial comments below.
Regarding the examples: Would it be possible to add an abridged version of the examples (without the EDHOC part) to the examples subdirectory or to the module-level rustdoc in oscore.rs?
That way, we can use those as (no_run) tests.
Lastly, it would also be nice if we had a runnable test case that runs both a server and a client and performs a basic request, akin to the test cases already present for UDP and DTLS.
libcoap/src/oscore.rs
Outdated
| // SAFETY: It is expected, that the user provides valid oscore_conf bytes. In case of | ||
| // failure this will return null which will result in an error being thrown. |
There was a problem hiding this comment.
This safety comment does not fully explain why the function call itself is safe.
One thing that should at least be mentioned here is that conf is copied internally in the call to coap_new_oscore_conf and therefore doesn't have to live longer than that call.
Without looking at the source code of libcoap, this is not obvious to a reader, and the required lifetime of conf is important wrt. memory safety.
In general, safety comments should mainly focus on aspects of "safety" in Rust terminology (mainly memory safety and aliasing rules) and less about regular error handling that does not affect memory safety.
In a nutshell, safety comments for unsafe blocks should be a statement of the form "these invariants are expected by the content of the unsafe block, and I have made sure that these invariants hold because of [...]".
In contrast, functions and traits declared as unsafe should have a safety section in their Rustdoc of the form "to not break Rust safety guarantees, you must uphold the following invariants if you use this piece of code".
That way, readers of the code (whether they are library users or maintainers) can quickly get an understanding of what assumptions the original author has made and what they might need to uphold themselves if they call the affected code/make changes to the code.
For example (although in most cases it doesn't have to be as verbose as this one):
| // SAFETY: It is expected, that the user provides valid oscore_conf bytes. In case of | |
| // failure this will return null which will result in an error being thrown. | |
| // SAFETY: | |
| // - The parts of the byte string referenced by conf are defensively copied if used | |
| // by the newly created oscore_conf | |
| // - conf may point to an arbitrary range of bytes (invalid data will not cause UB) | |
| // - save_seq_num_func is specifically designed to work as a callback for this | |
| // function. | |
| // - save_seq_num_func_param may be a null pointer (save_seq_num_func does | |
| // not use it). |
libcoap/src/context.rs
Outdated
| // SAFETY: Properly initialized CoapContext always has a valid raw_context that is not deleted until | ||
| // the CoapContextInner is dropped. OscoreConf raw_conf should be valid, else return an error. | ||
| // | ||
| // coap_context_oscore_server will also always free the raw_conf, regardless of the result: | ||
| // [libcoap docs](https://libcoap.net/doc/reference/4.3.5/group__oscore.html#ga71ddf56bcd6d6650f8235ee252fde47f) | ||
| unsafe { | ||
| result = coap_context_oscore_server(inner_ref.raw_context, oscore_conf.as_mut_raw_conf()?); | ||
| }; | ||
|
|
||
| // Invalidate the OscoreConf raw_conf as its freed by the call above. | ||
| oscore_conf.raw_conf_valid = false; | ||
|
|
||
| // Check whether adding the config to the context failed. | ||
| if result == 0 { | ||
| return Err(OscoreServerCreationError::Unknown); | ||
| } | ||
|
|
||
| // Add the initial_recipient (if present). | ||
| if let Some(initial_recipient) = oscore_conf.initial_recipient.clone() { | ||
| let initial_recipient = OscoreRecipient::new(initial_recipient.as_str()); |
There was a problem hiding this comment.
Wouldn't this be easier to represent by having a function OscoreConf::into_raw_conf(self) -> (*mut coap_oscore_conf_t, Option<OscoreRecipient>) that consumes the config?
That way, you can avoid managing a separate field to check that the pointer is still usable (the pointer being valid would be guaranteed/an invariant for the lifetime of the OscoreConf instance.
libcoap/src/context.rs
Outdated
| // SAFETY: If adding the recipient to the context fails we drop its underlying raw | ||
| // struct manually as it's not handled by the raw_context. There is one case where | ||
| // libcoap would already free the raw struct, which is filtered out above as we | ||
| // prevent adding a duplicate recipient_id to the context and return. | ||
| unsafe { | ||
| result = coap_new_oscore_recipient(inner_ref.raw_context, recipient.get_c_struct()); | ||
| }; | ||
|
|
||
| // Drop the raw struct if adding it failed (except for duplicate id)... | ||
| // SAFETY: This should be safe to use here as 'coap_new_oscore_recipient()' would only | ||
| // free() the raw struct on failure due to a duplicate recipient_id, which is filtered | ||
| // out above. | ||
| if result == 0 { | ||
| recipient.drop(); | ||
| return Err(OscoreRecipientError::Unknown); | ||
| } |
There was a problem hiding this comment.
Just like with the OscoreConf: have a method OscoreRecipient::into_raw(self) ->*mut coap_bin_const_t to convert the OscoreRecipient into the raw pointer, that way you don't need to manage the internal state of the raw pointer.
libcoap/src/context.rs
Outdated
| } | ||
|
|
||
| // ...or else save the recipient to keep alive the pointer to its raw struct. | ||
| inner_ref.recipients.push(recipient); |
There was a problem hiding this comment.
Raw pointers are not freed if the struct containing them is dropped, so you might not even have to keep the OscoreRecipient stored at all.
In fact, you might also get away with not having an OscoreRecipient struct at all and just taking a recipient_id: &str as arguments for this method and delete_oscore_recipient() instead.
libcoap/src/oscore.rs
Outdated
| // The user only supplies the recipients ID, we will build the recipients C struct here. | ||
| let recipient = coap_bin_const_t { | ||
| length: recipient_id.len(), | ||
| s: recipient_id.as_ptr(), | ||
| }; | ||
|
|
||
| let recipient: *mut coap_bin_const_t = Box::into_raw(Box::new(recipient)); |
There was a problem hiding this comment.
Pointers created by Box::into_raw can not necessarily be freed by libcoap.
To more more specific, providing a different global allocator to Rust code will cause calls to free() a Rust-made pointer in libcoap to fail, see https://doc.rust-lang.org/std/boxed/index.html#memory-layout
Use libcoap's own coap_new_bin_const() function instead. That way, it is guaranteed that the created pointer can also be freed by libcoap:
| // The user only supplies the recipients ID, we will build the recipients C struct here. | |
| let recipient = coap_bin_const_t { | |
| length: recipient_id.len(), | |
| s: recipient_id.as_ptr(), | |
| }; | |
| let recipient: *mut coap_bin_const_t = Box::into_raw(Box::new(recipient)); | |
| // The user only supplies the recipients ID, we will build the recipients C struct here. | |
| // SAFETY: provided pointer and length point to a valid byte string usable by coap_new_bin_const() | |
| let recipient = unsafe { coap_new_bin_const(recipient_id.as_ptr(), recipient_id.len()) }; |
EDIT: Note that if you apply this change, you also have to replace instances of Box::from_raw for this pointer with coap_delete_bin_const().
libcoap/src/oscore.rs
Outdated
| /// Drops the recipient from memory. | ||
| /// This will trigger a double free if coap_bin_const_t has already been freed! | ||
| /// WARNING: THIS SHOULD NEVER BE CALLED UNLESS YOU'RE SURE THE coap_bin_const_t HAS NOT BEEN FREED BEFORE! | ||
| pub(crate) fn drop(&self) { | ||
| // SAFETY: Currently, this is only used in 'add_new_oscore_recipient()' in case the recipient | ||
| // is not added to the context. There is currently only one exception, which is filtered out, | ||
| // because trying to add a duplicate recipient to the oscore context would already trigger a | ||
| // free() in libcoap. | ||
| unsafe { | ||
| let _ = Box::from_raw(self.get_c_struct()); | ||
| } | ||
| } |
There was a problem hiding this comment.
As indicated by your warning message, this must be marked unsafe (but also see my comments in context.rs on why you might not need the OscoreRecipient struct at all).
libcoap/src/session/client.rs
Outdated
| // SAFETY: self.raw_context is guaranteed to be valid, local_if can be null. | ||
| // OscoreConf raw_conf should be valid, else we return an error. | ||
| // | ||
| // coap_new_client_session_oscore should free the raw_conf: | ||
| // [libcoap docs](https://libcoap.net/doc/reference/4.3.5/group__oscore.html#ga65ac1a57ebc037b4d14538c8e21c28a7) | ||
| let session = unsafe { | ||
| coap_new_client_session_oscore( | ||
| ctx.as_mut_raw_context(), | ||
| std::ptr::null(), | ||
| CoapAddress::from(addr).as_raw_address(), | ||
| coap_proto_t_COAP_PROTO_UDP, | ||
| oscore_conf.as_mut_raw_conf()?, | ||
| ) | ||
| }; | ||
|
|
||
| // Invalidate the OscoreConf raw_conf as it's freed by the call above, so we don't try to | ||
| // free it again in the future, which would cause a double free(). | ||
| oscore_conf.raw_conf_valid = false; |
There was a problem hiding this comment.
Same as in context.rs: replace as_mut_raw_conf() with a method that consumes OscoreConf to avoid manually managing the state of the oscore_conf instance.
Co-authored-by: Hugo Hakim Damer <[email protected]>
Co-authored-by: Hugo Hakim Damer <[email protected]>
This pull request aims to provide support for OSCORE in libcoap-rs.
It adds a new feature flag "oscore" which has to be activated to enable all functions regarding OSCORE under which we provide the following:
We introduced two new structs OscoreConf and OscoreRecipient which are currently located within oscore.rs.
We also added new errors to allow for error-handling by the user in errors.rs.
There is also a minimal client and server example available, deriving from the official documentation.
Security was a priority and all unsafe calls have been annotated with an appropriate security notice, as is the standard for this project.