-
Notifications
You must be signed in to change notification settings - Fork 6
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
wolfSSL support added #28
Conversation
Does it make sense to expose WolfSSL, and if yes, wouldn't the wrapper best be RIOT-independent? (Although even a indep. wrapper needs this).
Reason I ask are DTLS sockets: RIOT abstracts DTLS into those, and has credman which'd be interesting to wrap, but I don't see where direct WolfSSL wrappers are helpful (yet?).
|
wolfSSL does much more than just DTLS. Also the DTLS sockets aren't completely abstracted (yet), the way I understand it there's still a need to open and configure the socket with wolfSSL and sock_tls/sock_dtls then only takes care of the read/write. |
What setup steps are there that are not done via credman? For the WolfCrypt parts, may libcose be a good place to wrap them?
|
There are several (optional) things that credman doesn't do, especially when it comes to using wolfSSL with a crypto IC so the keys aren't stored in RIOT itself but in a secure storage device. Also things like setting ciphersuites don't seem to be supported at the moment. Is there any downside of wrapping something that can also be accessed by sock_tls/credman? |
Having a way to create WolfSSL functionality is fine. The direction I'm worried about is that that'd happen in riot-wrappers (which admittedly this PR doesn't necessitate, but it'd be an obvious way this could continue) -- and doing all of that for RIOT alone might be difficult to do long-term. The way I think WolfSSL should be wrapped is:
Is this the direction you'd like to go with this? (I've briefly checked whether WolfSSL crates are on crates.io already; my impression is that the wolfssl-sys could be fine, while the wolfssl crate is probably not -- but that's from a short glance and many heuristics AKA gut feeling). |
riot-headers.h
Outdated
@@ -184,6 +184,14 @@ | |||
#include "ws281x.h" | |||
#endif | |||
|
|||
/* wolfSSL */ | |||
#ifdef MODULE_WOLFSSL |
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.
#ifdef MODULE_WOLFSSL | |
#if defined(MODULE_WOLFSSL) && !defined(IS_C2RUST) |
While RIOT is "sloppy" about API and ABI, WolfSSL being a regular library should have all its interfaces bindgen-safe. If we start exposing external library's symbols (which I think we'll need to, because a regular -sys crate won't know the build flags and include paths the way we know them), making them bindgen-only should become best practice where possible.
That wolfssl-sys crate does not support cross compiling at all. It will always attempt to compile for the host architecture, causing all kinds of issues. Just so I understand it correctly: So you don't want whatever wolfSSL wrapper I might come up with in rust-riot-wrappers but suggest it should be an independent crate? That crate should then make use of either this PR or the wolfssl-sys one (if there is no cross-compiling involved)? |
Yes, that'd be the idea; riot-wrappers would then pull in the
crate-to-be if there is, for example, a way to set up a DTLS socket with
DTLS options that excede what's in credman.
Looking at the headers you're including, this might already be the case
soon: Anything from sock_tls.h would still go into riot-wrappers, but
anything from wolfssl/ could be in its own crate.
Procedurally, if it makes things easier, we can also start this off in a
module of riot-wrappers, and split it off in the course of advancing the
-wrappers PR. But let's not wait too long with using -sys as well, for
that will guarantee we're staying honest to wrapping a library and not
just some OS's view of that library.
This all doesn't mean that all of WolfSSL will need to be wrapped, the
API surface can still be the one that you need, but it'll be in a place
where all users of WolfSSL through Rust can come together, rather than
staying in a niche where it's hard to find people for sustainable
maintenance.
|
By the way, the changes for here look good; merging now only depends on CI's OK, and your resolve to stick with this even though it got more complex than you hoped ;-) (although I maintain that it won't be much more effort to maintain this the way I suggest than by just lugging it in with -wrappers). |
We have already started with the development in rust-riot-wrappers and it was really helpful to have such a base already. Unfortunately when we put it in a separate crate it won't really be maintained in the future by anyone, that's what I am a little worried about. So if someone wants to add new features the only way will be to fork it and then continue the work on it, which (from my experience) usually means that there will be several forks which all address individual problems. Of course simply putting the burden of maintaining something on another person (in this case you) isn't an acceptable solution either. So simply starting a new crate and then abandoning it might be the best choice we have at this point, the other option would be not publishing it at all, which is probably even worse. |
If support for WolfSSL is just added here for sake of having the code in there, and there are no actual use cases, things will rot (and eventually be removed unless active users are around to keep things up to date) either way. As for starting a crate and abandoning it, there are options that make it easier:
Note that due to whatever you're doing with |
This pulls in several fixes from these repositories: * riot-sys: * Add `random` module to `riot-headers` (RIOT-OS/rust-riot-sys#26) * README: fix 2 typos (RIOT-OS/rust-riot-sys#31) * bindgen: Use 0.64 (RIOT-OS/rust-riot-sys#30) * wolfSSL support added (RIOT-OS/rust-riot-sys#28) * extern-types: Generate replacement types dynamically (RIOT-OS/rust-riot-sys#27) * Extern types: Add netq_t, make them large (RIOT-OS/rust-riot-sys#25) * doc: Suppress warnings about things C2Rust does not uphold (RIOT-OS/rust-riot-sys#23) * export macro_DAC_LINE (RIOT-OS/rust-riot-sys#22) * Add BINDGEN_OUTPUT_FILE export (RIOT-OS/rust-riot-sys#21) * riot-wrappers: * DAC: Add wrapper around RIOTs DAC-interface (RIOT-OS/rust-riot-wrappers#36) * saul: Compatibly rename G* variants (RIOT-OS/rust-riot-wrappers#50) * tests: Add test for auto-init when auto-init debug is active (RIOT-OS/rust-riot-wrappers#48) * gcoap: Provide link encoder (RIOT-OS/rust-riot-wrappers#47) * Drop SUIT support in riot-wrappers (RIOT-OS/rust-riot-wrappers#44) * Add an auto init module (RIOT-OS/rust-riot-wrappers#45) * gcoap: Allow registration without scope for 'static listeners (RIOT-OS/rust-riot-wrappers#43) Through direct dependency changes (the bindgen update), the number of transitive dependencies could be reduced.
19495: Rust: Update dependencies r=maribu a=chrysn ### Contribution description This updates both the RIOT-specific and generic dependencies of Rust examples and modules. ### Testing procedure * Green CI should do ### Issues/PRs references Copying from one of the commits with some sed: * riot-sys: * RIOT-OS/rust-riot-sys#26 * RIOT-OS/rust-riot-sys#31 * RIOT-OS/rust-riot-sys#30 * RIOT-OS/rust-riot-sys#28 * RIOT-OS/rust-riot-sys#27 * RIOT-OS/rust-riot-sys#25 * RIOT-OS/rust-riot-sys#23 * RIOT-OS/rust-riot-sys#22 * RIOT-OS/rust-riot-sys#21 * riot-wrappers: * RIOT-OS/rust-riot-wrappers#36 * RIOT-OS/rust-riot-wrappers#50 * RIOT-OS/rust-riot-wrappers#48 * RIOT-OS/rust-riot-wrappers#47 * RIOT-OS/rust-riot-wrappers#44 * RIOT-OS/rust-riot-wrappers#45 * RIOT-OS/rust-riot-wrappers#43 ### How to do similar PRs Updating the RIOT-related dependencies (which here also updated bindgen because the dependency of riot-sys changed): ``` $ for x in **/Cargo.lock; do cargo update --manifest-path=${x%.lock}.toml --package riot-sys --package riot-wrappers; done ``` Updating everything (should never really be needed, b/c if something has a concrete dependency change it'd say so, but dependencies generally get better over time): ``` $ for x in **/Cargo.lock; do cargo update --manifest-path=${x%.lock}.toml; done ``` Creating the commit message: ``` $ git log --first-parent --format='%s (%b)' 9c29faf55d4c14d2d7f55f8df5059c52af4e5317..e4973a6ee88427f702dac41b3dce4fd6b6b9689e | sed 's/Merges: //' | sed 's/^/ * /' ``` git shortlog unfortunately doesn't show the merges the way I prefer linking them. The versions in the command line can be taken from `git diff --text` and looking for the riot-sys or riot-wrappers line, respectively. Co-authored-by: chrysn <chrysn@fsfe.org>
19495: Rust: Update dependencies r=chrysn a=chrysn ### Contribution description This updates both the RIOT-specific and generic dependencies of Rust examples and modules. It also follows a deprecation from the G unit renaming originally done in #19292. ### Testing procedure * Green CI should do ### Issues/PRs references Copying from one of the commits with some sed: * riot-sys: * RIOT-OS/rust-riot-sys#26 * RIOT-OS/rust-riot-sys#31 * RIOT-OS/rust-riot-sys#30 * RIOT-OS/rust-riot-sys#28 * RIOT-OS/rust-riot-sys#27 * RIOT-OS/rust-riot-sys#25 * RIOT-OS/rust-riot-sys#23 * RIOT-OS/rust-riot-sys#22 * RIOT-OS/rust-riot-sys#21 * riot-wrappers: * RIOT-OS/rust-riot-wrappers#36 * RIOT-OS/rust-riot-wrappers#50 * RIOT-OS/rust-riot-wrappers#48 * RIOT-OS/rust-riot-wrappers#47 * RIOT-OS/rust-riot-wrappers#44 * RIOT-OS/rust-riot-wrappers#45 * RIOT-OS/rust-riot-wrappers#43 * (later, when the mistake became apparent) RIOT-OS/rust-riot-wrappers#55 ### How to do similar PRs Updating the RIOT-related dependencies (which here also updated bindgen because the dependency of riot-sys changed): ``` $ for x in **/Cargo.lock; do cargo update --manifest-path=${x%.lock}.toml --package riot-sys --package riot-wrappers; done ``` Updating everything (should never really be needed, b/c if something has a concrete dependency change it'd say so, but dependencies generally get better over time): ``` $ for x in **/Cargo.lock; do cargo update --manifest-path=${x%.lock}.toml; done ``` Creating the commit message: ``` $ git log --first-parent --format='%s (%b)' 9c29faf55d4c14d2d7f55f8df5059c52af4e5317..e4973a6ee88427f702dac41b3dce4fd6b6b9689e | sed 's/Merges: //' | sed 's/^/ * /' ``` git shortlog unfortunately doesn't show the merges the way I prefer linking them. The versions in the command line can be taken from `git diff --text` and looking for the riot-sys or riot-wrappers line, respectively. Co-authored-by: chrysn <chrysn@fsfe.org>
This pulls in several fixes from these repositories: * riot-sys: * Add `random` module to `riot-headers` (RIOT-OS/rust-riot-sys#26) * README: fix 2 typos (RIOT-OS/rust-riot-sys#31) * bindgen: Use 0.64 (RIOT-OS/rust-riot-sys#30) * wolfSSL support added (RIOT-OS/rust-riot-sys#28) * extern-types: Generate replacement types dynamically (RIOT-OS/rust-riot-sys#27) * Extern types: Add netq_t, make them large (RIOT-OS/rust-riot-sys#25) * doc: Suppress warnings about things C2Rust does not uphold (RIOT-OS/rust-riot-sys#23) * export macro_DAC_LINE (RIOT-OS/rust-riot-sys#22) * Add BINDGEN_OUTPUT_FILE export (RIOT-OS/rust-riot-sys#21) * riot-wrappers: * DAC: Add wrapper around RIOTs DAC-interface (RIOT-OS/rust-riot-wrappers#36) * saul: Compatibly rename G* variants (RIOT-OS/rust-riot-wrappers#50) * tests: Add test for auto-init when auto-init debug is active (RIOT-OS/rust-riot-wrappers#48) * gcoap: Provide link encoder (RIOT-OS/rust-riot-wrappers#47) * Drop SUIT support in riot-wrappers (RIOT-OS/rust-riot-wrappers#44) * Add an auto init module (RIOT-OS/rust-riot-wrappers#45) * gcoap: Allow registration without scope for 'static listeners (RIOT-OS/rust-riot-wrappers#43) Through direct dependency changes (the bindgen update), the number of transitive dependencies could be reduced. (cherry picked from commit de41031)
19505: Rust: Update dependencies [backport 2023.04] r=maribu a=maribu # Backport of #19495 ### Contribution description This updates both the RIOT-specific and generic dependencies of Rust examples and modules. It also follows a deprecation from the G unit renaming originally done in #19292. ### Testing procedure * Green CI should do ### Issues/PRs references Copying from one of the commits with some sed: * riot-sys: * RIOT-OS/rust-riot-sys#26 * RIOT-OS/rust-riot-sys#31 * RIOT-OS/rust-riot-sys#30 * RIOT-OS/rust-riot-sys#28 * RIOT-OS/rust-riot-sys#27 * RIOT-OS/rust-riot-sys#25 * RIOT-OS/rust-riot-sys#23 * RIOT-OS/rust-riot-sys#22 * RIOT-OS/rust-riot-sys#21 * riot-wrappers: * RIOT-OS/rust-riot-wrappers#36 * RIOT-OS/rust-riot-wrappers#50 * RIOT-OS/rust-riot-wrappers#48 * RIOT-OS/rust-riot-wrappers#47 * RIOT-OS/rust-riot-wrappers#44 * RIOT-OS/rust-riot-wrappers#45 * RIOT-OS/rust-riot-wrappers#43 * (later, when the mistake became apparent) RIOT-OS/rust-riot-wrappers#55 ### How to do similar PRs Updating the RIOT-related dependencies (which here also updated bindgen because the dependency of riot-sys changed): ``` $ for x in **/Cargo.lock; do cargo update --manifest-path=${x%.lock}.toml --package riot-sys --package riot-wrappers; done ``` Updating everything (should never really be needed, b/c if something has a concrete dependency change it'd say so, but dependencies generally get better over time): ``` $ for x in **/Cargo.lock; do cargo update --manifest-path=${x%.lock}.toml; done ``` Creating the commit message: ``` $ git log --first-parent --format='%s (%b)' 9c29faf55d4c14d2d7f55f8df5059c52af4e5317..e4973a6ee88427f702dac41b3dce4fd6b6b9689e | sed 's/Merges: //' | sed 's/^/ * /' ``` git shortlog unfortunately doesn't show the merges the way I prefer linking them. The versions in the command line can be taken from `git diff --text` and looking for the riot-sys or riot-wrappers line, respectively. Co-authored-by: chrysn <chrysn@fsfe.org>
19505: Rust: Update dependencies [backport 2023.04] r=maribu a=maribu # Backport of #19495 ### Contribution description This updates both the RIOT-specific and generic dependencies of Rust examples and modules. It also follows a deprecation from the G unit renaming originally done in #19292. ### Testing procedure * Green CI should do ### Issues/PRs references Copying from one of the commits with some sed: * riot-sys: * RIOT-OS/rust-riot-sys#26 * RIOT-OS/rust-riot-sys#31 * RIOT-OS/rust-riot-sys#30 * RIOT-OS/rust-riot-sys#28 * RIOT-OS/rust-riot-sys#27 * RIOT-OS/rust-riot-sys#25 * RIOT-OS/rust-riot-sys#23 * RIOT-OS/rust-riot-sys#22 * RIOT-OS/rust-riot-sys#21 * riot-wrappers: * RIOT-OS/rust-riot-wrappers#36 * RIOT-OS/rust-riot-wrappers#50 * RIOT-OS/rust-riot-wrappers#48 * RIOT-OS/rust-riot-wrappers#47 * RIOT-OS/rust-riot-wrappers#44 * RIOT-OS/rust-riot-wrappers#45 * RIOT-OS/rust-riot-wrappers#43 * (later, when the mistake became apparent) RIOT-OS/rust-riot-wrappers#55 ### How to do similar PRs Updating the RIOT-related dependencies (which here also updated bindgen because the dependency of riot-sys changed): ``` $ for x in **/Cargo.lock; do cargo update --manifest-path=${x%.lock}.toml --package riot-sys --package riot-wrappers; done ``` Updating everything (should never really be needed, b/c if something has a concrete dependency change it'd say so, but dependencies generally get better over time): ``` $ for x in **/Cargo.lock; do cargo update --manifest-path=${x%.lock}.toml; done ``` Creating the commit message: ``` $ git log --first-parent --format='%s (%b)' 9c29faf55d4c14d2d7f55f8df5059c52af4e5317..e4973a6ee88427f702dac41b3dce4fd6b6b9689e | sed 's/Merges: //' | sed 's/^/ * /' ``` git shortlog unfortunately doesn't show the merges the way I prefer linking them. The versions in the command line can be taken from `git diff --text` and looking for the riot-sys or riot-wrappers line, respectively. 19509: cpu/cc26xx_cc13xx: Fix bogus array-bound warning [backport 2023.04] r=maribu a=maribu # Backport of #19504 ### Contribution description GCC 12 create a bogus array out of bounds warning as it assumes that because there is special handling for `uart == 0` and `uart == 1`, `uart` can indeed be `1`. There is an `assert(uart < UART_NUMOF)` above that would blow up prior to any out of bounds access. In any case, optimizing out the special handling of `uart == 1` for when `UART_NUMOF == 1` likely improves the generated code and fixes the warning. /home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:88:8: error: array subscript 1 is above array bounds of 'uart_isr_ctx_t[1]' [-Werror=array-bounds] 88 | ctx[uart].rx_cb = rx_cb; | ~~~^~~~~~ /home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:52:23: note: while referencing 'ctx' 52 | static uart_isr_ctx_t ctx[UART_NUMOF]; | ^~~ /home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:89:8: error: array subscript 1 is above array bounds of 'uart_isr_ctx_t[1]' [-Werror=array-bounds] 89 | ctx[uart].arg = arg; | ~~~^~~~~~ /home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:52:23: note: while referencing 'ctx' 52 | static uart_isr_ctx_t ctx[UART_NUMOF]; | ^~~ ### Testing procedure The actual change is a pretty obvious one-liner, so that code review and a green CI should be sufficient. If not, running any UART example app without regression should do. ### Issues/PRs references None 19510: tools/openocd: Fix handling of OPENOCD_CMD_RESET_HALT [backport 2023.04] r=maribu a=maribu # Backport of #19506 ### Contribution description The OPENOCD_CMD_RESET_HALT was not longer correctly passed to the script. This fixes the issue. ### Testing procedure Flashing of e.g. the `cc2650-launchpad` with upstream OpenOCD should work again. ### Issues/PRs references The change was added to #19050 after testing the PR and before merging. I'm not sure if the fix never worked because of this, or if behavior of `target-export-variables` or GNU Make changed. Co-authored-by: chrysn <chrysn@fsfe.org> Co-authored-by: Marian Buschsieweke <marian.buschsieweke@ovgu.de>
This pulls in several fixes from these repositories: * riot-sys: * Add `random` module to `riot-headers` (RIOT-OS/rust-riot-sys#26) * README: fix 2 typos (RIOT-OS/rust-riot-sys#31) * bindgen: Use 0.64 (RIOT-OS/rust-riot-sys#30) * wolfSSL support added (RIOT-OS/rust-riot-sys#28) * extern-types: Generate replacement types dynamically (RIOT-OS/rust-riot-sys#27) * Extern types: Add netq_t, make them large (RIOT-OS/rust-riot-sys#25) * doc: Suppress warnings about things C2Rust does not uphold (RIOT-OS/rust-riot-sys#23) * export macro_DAC_LINE (RIOT-OS/rust-riot-sys#22) * Add BINDGEN_OUTPUT_FILE export (RIOT-OS/rust-riot-sys#21) * riot-wrappers: * DAC: Add wrapper around RIOTs DAC-interface (RIOT-OS/rust-riot-wrappers#36) * saul: Compatibly rename G* variants (RIOT-OS/rust-riot-wrappers#50) * tests: Add test for auto-init when auto-init debug is active (RIOT-OS/rust-riot-wrappers#48) * gcoap: Provide link encoder (RIOT-OS/rust-riot-wrappers#47) * Drop SUIT support in riot-wrappers (RIOT-OS/rust-riot-wrappers#44) * Add an auto init module (RIOT-OS/rust-riot-wrappers#45) * gcoap: Allow registration without scope for 'static listeners (RIOT-OS/rust-riot-wrappers#43) Through direct dependency changes (the bindgen update), the number of transitive dependencies could be reduced.
This is the first step to add support for wolfSSL in Rust on RIOT-OS. This has been tested using DTLS 1.3 and using SHA256. There is a work-in-progress wrapper that this sets the base for.