-
Notifications
You must be signed in to change notification settings - Fork 22
Use TLS over transport for authentication of peer #227
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR adds TLS transport support to wolfHSM for secure client-server communication. The implementation extends the existing TCP transport with TLS encryption using wolfSSL, supporting both certificate-based and PSK (Pre-Shared Key) authentication methods.
Key Changes:
- New TLS transport layer built on top of TCP transport with wolfSSL integration
- Support for both mutual TLS authentication and PSK modes
- Configuration changes to enable TLS/PSK in the build system
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| port/posix/posix_transport_tls.h | Header defining TLS transport structures and function declarations |
| port/posix/posix_transport_tls.c | Implementation of TLS transport client/server functions |
| port/posix/posix_transport_tcp.c | Exposed HandleConnect function and fixed null pointer check |
| examples/posix/wh_posix_server/wh_posix_server_cfg.h | Added TLS and PSK configuration function declarations |
| examples/posix/wh_posix_server/wh_posix_server_cfg.c | Implemented TLS and PSK server configuration with certificate loading |
| examples/posix/wh_posix_server/wh_posix_server.c | Added TLS/PSK transport options to CLI |
| examples/posix/wh_posix_server/user_settings.h | Enabled TLS, TLS12, PSK, and debug settings |
| examples/posix/wh_posix_client/wh_posix_client_cfg.h | Added TLS and PSK client configuration declarations |
| examples/posix/wh_posix_client/wh_posix_client_cfg.c | Implemented TLS and PSK client configuration with certificate loading |
| examples/posix/wh_posix_client/wh_posix_client.c | Added TLS/PSK transport options to client CLI |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
e9a4e7e to
fcb0a97
Compare
bigbrett
left a comment
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.
quick initial skim - I'd like to see test coverage added to this before diving much deeper. Looks very promising though, I'm super excited about this !!!
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.
Pull Request Overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Pull Request Overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Investigating SHE tests with TLS. |
|
Random thoughts for future work: It would be really cool to have a built-in way to initialize and setup the |
4ab374d to
0272216
Compare
|
Rebased on top of recent examples and yaml changes. Assigning back to me until CI tests are passing |
22ee224 to
cebfea5
Compare
billphipps
left a comment
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.
Just capturing some notes in progress. Good conversation. Let's keep working to figure out the best path forward!
| @scan-build --exclude $(WOLFSSL_DIR)/wolfcrypt \ | ||
| --exclude $(WOLFSSL_DIR)/src \ |
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.
Were these causing issues before this PR? Recommend to add a comment explaining why it is ok to exclude wolfcrypt here. We should probably consider allowing a system library build of wolfssl for tools rather than building from source
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.
I see the --exclude's being in the main wolfHSM branch. Not sure why it would be listed as change in this PR. Maybe from the rebase onto main?
| defined(WOLFHSM_CFG_ENABLE_CLIENT) && defined(WOLFHSM_CFG_TEST_POSIX) | ||
| /* Test driver should run TCP client tests against the example server */ | ||
| ret = whTest_ClientTcp(); | ||
| #elif defined(WOLFHSM_CFG_TEST_CLIENT_ONLY_TLS) && \ |
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.
Consider combining the xxx_ONLY_TCP and xxx_ONLY_TLS into CLIENT_ONLY and use additional CFG or CFG_TEST to specify the transport. These CFG_TEST macros should be described in wh_test.h with defaults AND the wh_test.h should be included in user_settings.h and wh_config.h to allow those configurations to be overridden.
| int whPosixClient_ExampleTlsConfig(void* conf) | ||
| { | ||
| if (whPosixClient_ExampleTlsCommonConfig(conf) != WH_ERROR_OK) { | ||
| return WH_ERROR_ABORTED; |
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.
Consider returning the return value?
|
|
||
| #if defined(WOLFHSM_CFG_TEST_POSIX) && defined(WOLFHSM_CFG_ENABLE_CLIENT) | ||
| #include "port/posix/posix_transport_tcp.h" | ||
| #ifndef WOLFHSM_CFG_NO_CRYPTO |
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.
This should also be gated on whether or not TLS is actually needed since the user_settings.h may be different when TLS is needed or not.
| /* For cert manager */ | ||
| #define NO_TLS | ||
| /* Eliminates need for IO layer since we only use CM */ | ||
| #define WOLFSSL_USER_IO |
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.
These should be gated on whether the TLS transport is necessary in this build. It should be made very clear which wolfCrypt settings are required ONLY for TLS and which are the ones needed for a slim/test version of wolfHSM
| rc = wolfSSL_CTX_load_verify_buffer(ctx->ssl_ctx, ca_cert_der_2048, | ||
| sizeof_ca_cert_der_2048, | ||
| CTC_FILETYPE_ASN1); |
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.
Recommend to move CA and device certificates into the TLS transport config structure. These buffers (pointers?) or filenames would get copied into the TLS transport context and then used during the TLS transport Init() callback. This would allow you to instantiate and connect in a single wh_Client_Init() call, rather than build parts of the context out of sequence.
No description provided.