Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

pkg/wolfssl: Update wolfSSL to 5.5.4 and add DTLS 1.3 support #19302

Merged
merged 7 commits into from
Mar 10, 2023

Conversation

Flole998
Copy link
Contributor

Contribution description

  • Update wolfSSL to the most recent version
  • Add DTLS 1.3 support in wolfSSL

Testing procedure

  • All tests still suceed
  • Test code in rust was written to verify DTLS 1.3 is working as intended

Issues/PRs references

The patch file included in this PR is submitted to wolfSSL as wolfSSL/wolfssl#6007

@github-actions github-actions bot added the Area: pkg Area: External package ports label Feb 22, 2023
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 22, 2023
@benpicco benpicco requested a review from HendrikVE February 22, 2023 17:52
@riot-ci
Copy link

riot-ci commented Feb 22, 2023

Murdock results

✔️ PASSED

ede7302 tests/pkg_wolfssl: Update boards with insufficient memory

Success Failures Total Runtime
6882 0 6882 08m:56s

Artifacts

@Flole998
Copy link
Contributor Author

I have no clue why that test is failing when CI is running it, running it locally I don't see that issue and there is also no issue report from anybody else about this error at wolfSSL.

@benpicco
Copy link
Contributor

benpicco commented Feb 22, 2023

This one should be easy to 'fix' by adding

CFLAGS += -Wno-unused-function

to either the pkg Makefile or Makefile.wolfssl

@Flole998
Copy link
Contributor Author

This one should be easy to 'fix' by adding

CFLAGS += -Wno-unused-function

to either the pkg Makefile or Makefile.wolf

Is that really a proper fix though? I mean for obvious reasons it would turn off the error, but I'm surprised it's showing up in the first place. A brief look showed they made that declaration dependent on some preprocessor variables, probably to avoid this issue, so I'm trying to make sense of what's happening here rather than just silencing that error.

@benpicco
Copy link
Contributor

You are also enabling new WolfSSL modules in this PR, this may be related.

@Flole998
Copy link
Contributor Author

The issue appears to be an upstream-issue. I have opened wolfSSL/wolfssl#6142 for it.

@benpicco
Copy link
Contributor

If you want to move this forward you can just add

CFLAGS += -Wno-unused-function

to the pkg and remove it later if upstream has been updated (see e.g. #19328)

@Flole998
Copy link
Contributor Author

They tend to be quite fast in fixing things upstream, and I'm not in a hurry, so I'll at least wait for a response there and if they come up with a patch I'd rather drop that in the patches directory.

@Flole998
Copy link
Contributor Author

Flole998 commented Mar 1, 2023

As I said: They are quite fast, they already fixed it upstream. The patch is already included and the test is running through now. However, now the next one is failing :D

Both patches included here are aready in upstream master and when they do the next release of wolfSSL those can be dropped.

@github-actions github-actions bot added the Area: tests Area: tests and testing framework label Mar 1, 2023
10 seconds are not enough, each operation takes one second, and we do
more than 10. 20 seems to be a sane value.
@Flole998 Flole998 requested a review from miri64 as a code owner March 1, 2023 00:25
@github-actions github-actions bot added the Area: examples Area: Example Applications label Mar 1, 2023
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!
Does that mean we could now implement the sock_dtls API for wolfSSL with proper DTLS support as it was done for tinyDTLS?

@Flole998
Copy link
Contributor Author

Flole998 commented Mar 1, 2023

That was already possible before this PR. There has been an approach somewhere, and I am planning on picking that up. The advantage of this PR is, that we can support the new DTLS version 1.3, wolfSSL is apparently the only crypto library that has already implemented that.

@Flole998
Copy link
Contributor Author

Flole998 commented Mar 1, 2023

It looks like murdock didn't run for the last commit, any idea why?

@benpicco
Copy link
Contributor

benpicco commented Mar 1, 2023

What do you mean by

Test code in rust was written to verify DTLS 1.3 is working as intended

I see no Rust code as part of this pr - or do you mean you have Rust-on-RIOT code in another project that already makes use of this?

@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 1, 2023
@Flole998
Copy link
Contributor Author

Flole998 commented Mar 1, 2023

What do you mean by

Test code in rust was written to verify DTLS 1.3 is working as intended

I see no Rust code as part of this pr - or do you mean you have Rust-on-RIOT code in another project that already makes use of this?

I have changes ready for rust-riot-sys, I wanted to do one thing at a time though to prevent having PRs which depend on other PRs. There's also a rust wrapper in the making. My plan was to now get this merged, then do the rust-riot-sys change, then get the wrapper done and get that merged in rust-riot-wrappers and then get an example/test case merged here again when everything is done and the code is cleaned up.

@benpicco
Copy link
Contributor

benpicco commented Mar 1, 2023

Sounds like a plan!

bors merge

bors bot added a commit that referenced this pull request Mar 1, 2023
19302: pkg/wolfssl: Update wolfSSL to 5.5.4 and add DTLS 1.3 support r=benpicco a=Flole998



Co-authored-by: Florian Lentz <flolen@uni-bremen.de>
Co-authored-by: Flole998 <Flole998@users.noreply.github.com>
@bors
Copy link
Contributor

bors bot commented Mar 1, 2023

Build failed:

@benpicco
Copy link
Contributor

benpicco commented Mar 1, 2023

You can re-generate Makefile.ci by running make generate-Makefile.ci - but before doing that: Any idea why the ROM usage increased that much?

@Flole998
Copy link
Contributor Author

Flole998 commented Mar 2, 2023

There's one thing that puzzles me about this: The tests went through when this PR was checked and it failed when it was merged into master. So apparently something that was merged after I branched-off but before the merge attempt was made also caused increased flash usage.

Now many additional wolfSSL Features are included, see

#define HAVE_TLS_EXTENSIONS
, so we support more ciphersuites which obviously comes with a tradeoff, in this case flash usage increased.

I'll check to see how bad it really is, comparing the size from this PR against the size of master should show if this is bad or if that specific target was just marginal on flash anyways.

@benpicco
Copy link
Contributor

benpicco commented Mar 2, 2023

You can use make cosy to get a visualization of memory usage of the individual objects.

@Flole998
Copy link
Contributor Author

Flole998 commented Mar 2, 2023

I just checked and flash usage increased by 10-30kB (depending on the target). That's a lot more than expected and I will check why that is, for IoT-Devices that's a lot and not acceptable without a good explanation IMO. That should also clearly be visible when it's that much.

Maybe it's worth to get a summary of how a certain PR affects flash usage automatically, I know for some Arduino libaries that's done, that way such issues are visible before merging and we don't end up just using more and more memory and the one who goes over the limit just disables the target. If this would've been visible I would have investigated in immediately. On the other side this was about to get merged, so this huge increase would have gone in without even noticing. Long story short: When working with memory constrained devices something like that seems to make sense to prevent accidentally increasing the flash requirements significantly.

@Flole998
Copy link
Contributor Author

Flole998 commented Mar 8, 2023

So the increased flash usage is coming from various places. It is not due to additional includes or something like that. So disabling that failing board now.

@benpicco
Copy link
Contributor

benpicco commented Mar 8, 2023

It's strange that the size increases that much between versions, but I suppose wolfSSL mostly targets larger devices then.

@Flole998
Copy link
Contributor Author

Flole998 commented Mar 8, 2023

wolfSSL should run on embedded devices aswell, the increased code size is indeed unusual but we did skip quite some updates, the old version is 3 years old.

I am currently running one last generate-makefile.ci, once that is completed we can have another go at merging this.

@benpicco
Copy link
Contributor

bors merge

@bors
Copy link
Contributor

bors bot commented Mar 10, 2023

Build succeeded:

@bors bors bot merged commit 8dc8bf3 into RIOT-OS:master Mar 10, 2023
@OlegHahm
Copy link
Member

Sorry, I somehow missed the conclusion. For the nrf52dk the new version requires about 15k more of flash and about 2,7k more of RAM. That's really huge and I didn't find the explanation.

@Flole998
Copy link
Contributor Author

Sorry, I somehow missed the conclusion. For the nrf52dk the new version requires about 15k more of flash and about 2,7k more of RAM. That's really huge and I didn't find the explanation.

The conclusion is, that the increased requirements seem to be coming from basically all over the place. As the old version is about 3 years old this seems to be general growth and not something specific that's causing the increased requirements (for example some module that's now enabled by default).

@MrKevinWeiss MrKevinWeiss added this to the Release 2023.04 milestone Apr 25, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Area: examples Area: Example Applications Area: pkg Area: External package ports Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants