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 support for SSL/TLS client authentication #169

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

rvdgracht
Copy link

Add support for mutual TLS authentication. This is the preferred method of authentication for bosch-iot-suite and the only one that allows you to keep the authenticator in a (f)TPM.

Optionally, an Openssl engine can be configured if required for access to the ssl private key.

@rvdgracht rvdgracht changed the title Add support for SSL/TLS client authentication authentication Add support for SSL/TLS client authentication Jan 31, 2024
Add support for mutual TLS authentication. This is the preferred method
of authentication for bosch-iot-suite and the only one that allows you to
keep the authenticator in a (f)TPM.

Optionally, an Openssl engine can be configured if required for access to
the ssl private key.

Signed-off-by: Robin van der Gracht <[email protected]>
@rvdgracht
Copy link
Author

When using an ssl engine with a slow secure key storage, i.e. OPTEE with pkcs11 TA on a stm32mp151 setting up a TLS connection can take some time (I've seen 5 to 50 seconds). Because rauc-hawkbit-updater creates a new connection to the server for EVERY status update and poll, this can be cumbersome. For that I've opened a different pull request with a change that keeps the connection open between request. See PR #170

@Bastian-Krause
Copy link
Member

Did you see #166? My understanding is that this PR is quite similar.

Copy link
Member

@Bastian-Krause Bastian-Krause left a comment

Choose a reason for hiding this comment

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

The changes look good to me, although very similar to #166.

This does not support HTTP streaming installation (stream_bundle = true), right? #166 seems to support this, although I don't really see how. I would have expected that the tls-cert, tls-key, tls-ca arguments would be set in RAUC's InstallBundle(). Would you be willing to add client cert authentication support for HTTP streaming installations?

#166 also contains tests for client cert authentication. It would like to have them included here.

If you don't use the HTTP streaming feature, cherry-picking the test without streaming support would be a way forward. #166 could then be rebased to add HTTP streaming support later.

@rvdgracht
Copy link
Author

rvdgracht commented Feb 20, 2024

Did you see #166? My understanding is that this PR is quite similar.

Yes. At the last moment when I was creating the PR.
I decided to create a separate PR anyway since #166 seemed stale, and had no support for pkcs11.

Would you be willing to add client cert authentication support for HTTP streaming installations?

We're not actually using the streaming installation feature, but I added it anyway.
I added the mtls test as well.

The mtls test is heavily based on the work of @flobz (Florain Bezannier).
Cherry picking his patch wasn't possible, since my API is slightly different and because I wanted to use key/cert supplied through the arguments of InstallBundle() instead of externally through a test fixture.
I took the liberty of chopping Florians patch up in more manageble/reviewable chunks.
I wanted to give Florian credit and added his signed-off to paches where I haven't (or barely) changed his code, I hope thats ok.

@flobz
Copy link

flobz commented Feb 20, 2024

#166 isn't stale I'm waiting for @Bastian-Krause final review :)

@Bastian-Krause
Copy link
Member

@rvdgracht Thanks for adding streaming support and testing. Do you want to have a look at the failing tests or should I?

@Bastian-Krause
Copy link
Member

The overall approach looks good to me, once the test failures and the heap-use-after-free are solved, I can fix up some minor Python formatting and maybe simplify a thing or two. Then, this should be ready.

@Bastian-Krause
Copy link
Member

Bastian-Krause commented Mar 11, 2024

I tend to reviewing/merging this instead of #166: the implementation seems more straight forward and the commits are more comprehensible.

@flobz What do you think of this? Would this work for you, too? Is there anything missing in comparison to #166?

libcairo2-dev is required for building the pycairo wheel which is
a dependency for pygobject.

Signed-off-by: Robin van der Gracht <[email protected]>
Dictionaries can't contain multiple items with the same key. This prevents
removal of multiple options from the same config section.

Signed-off-by: Florian Bezannier <[email protected]>
Signed-off-by: Robin van der Gracht <[email protected]>
This option makes Hawkbit take the x-forward headers into account.

Signed-off-by: Florian Bezannier <[email protected]>
Signed-off-by: Robin van der Gracht <[email protected]>
Signed-off-by: Florian Bezannier <[email protected]>
Signed-off-by: Robin van der Gracht <[email protected]>
Signed-off-by: Florian Bezannier <[email protected]>
Signed-off-by: Robin van der Gracht <[email protected]>
Based on the work of Florain Bezannier.
Changes are that the client key and certificate are now provided to the
rauc_dbus_dummy by rauc-hawkbit-updater through arguments of the
InstallBundle method call (for streaming installations).
This also removes the need for a separate mtls rauc_dbus_dummy fixture.

Signed-off-by: Robin van der Gracht <[email protected]>
@rvdgracht
Copy link
Author

@Bastian-Krause I found and fixed the heap-use-after-free.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants