Skip to content
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

Add feature flag for enabling Mbed TLS support #39

Merged
merged 3 commits into from
Nov 16, 2024

Conversation

sgoll
Copy link
Contributor

@sgoll sgoll commented Nov 15, 2024

Description

This adds a feature flag mbedtls that builds open62541 with Mbed TLS support for encryption. We bundle the Mbed TLS sources alongside the sources of open62541 to support all existing build targets.

@sgoll
Copy link
Contributor Author

sgoll commented Nov 15, 2024

This builds on #37 and #38 and should only be merged afterwards.

@sgoll
Copy link
Contributor Author

sgoll commented Nov 15, 2024

We bundle the Mbed TLS sources alongside the sources of open62541 to support all existing build targets.

We should eventually revisit this decision. We could try to move the bundled source code into separate crates to reduce the crate size when encryption support is not actually needed.

Comment on lines +175 to +178
enum Encryption {
MbedTls,
OpenSsl,
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This and other parts of the code prepare for the eventual introduction of OpenSSL support as well, but for now I am reluctant to enable this because it requires further tests—and we can probably not bundle OpenSSL as well, so that the set of target platforms will probably be a reduced on, on a best-effort basis. Not ideal.

@uklotzde
Copy link
Collaborator

We bundle the Mbed TLS sources alongside the sources of open62541 to support all existing build targets.

We should eventually revisit this decision. We could try to move the bundled source code into separate crates to reduce the crate size when encryption support is not actually needed.

Sound like a plan. The size of this crate is already huge.

How much does the package size increase when adding mbed TLS?

@sgoll
Copy link
Contributor Author

sgoll commented Nov 16, 2024

How much does the package size increase when adding mbed TLS?

  • Before (1fdb3c4): Packaged 350 files, 7.5MiB (1.1MiB compressed)
  • With this PR: Packaged 713 files, 15.7MiB (2.6MiB compressed)

So we are more or less doubling the package size.

@sgoll sgoll marked this pull request as ready for review November 16, 2024 02:44
@sgoll sgoll requested a review from uklotzde November 16, 2024 03:15
@uklotzde
Copy link
Collaborator

Adding this feature is convenient.

But is it worth the huge increase in package size? Each update of either open62541 or mbedtls will create a new BLOB with lots of redundant/unchanged data.

@sgoll
Copy link
Contributor Author

sgoll commented Nov 16, 2024

But is it worth the huge increase in package size? Each update of either open62541 or mbedtls will create a new BLOB with lots of redundant/unchanged data.

That is correct. My plan is to integrate encryption support first in a way that works, then look for any other issues that we might need to solve to get encrypted OPC UA communication working in Rust. When this sort-of proof of concept is okay, we can try to split off the bundled source code into separate crates.1

Footnotes

  1. I have no idea how that will work out but I would like to solve this problem separately.

Copy link
Collaborator

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

Let's start with this baseline.

@uklotzde uklotzde merged commit efa30b5 into HMIProject:main Nov 16, 2024
13 checks passed
uklotzde pushed a commit that referenced this pull request Nov 19, 2024
## Description

This is a follow-up to #39 and includes additional bindings, some of
which are only available with feature flag `mbedtls`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants