fix(esp-tls): make the wolfSSL backend send entire client certificate… (IDFGH-12621) #13618

frankencode · 2024-04-15T23:32:26Z

… chains

This change makes the wolfSSL backend sent the complete TLS client certificate chain. This align the wolfSSL backend with the behavior of the mbedTLS backend. Some servers need the intermediate certificates to verify a client certificate. If the provided PEM file contains only a single certificate this change has no effect and the behavior will be as before.
This impacts higher level APIs to function as someone would expect. E.g.: esp_websocket_client_config_t.client_cert: when passing here a pem file containing 2 certificates (the CA's and the client's) it would be expected that both are transmitted during TLS handshake.

github-actions · 2024-04-15T23:33:07Z

	Messages
📖	🎉 Good Job! All checks are passing!

👋 Hello frankencode, we appreciate your contribution to this project!

📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more.

🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project.

Click to see more instructions ...

This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...

We do welcome contributions in the form of bug reports, feature requests and pull requests via this public GitHub repository.

This GitHub project is public mirror of our internal git repository

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved, we synchronize it into our internal git repository.
4. In the internal git repository we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
5. If the change is approved and passes the tests it is merged into the default branch.
5. On next sync from the internal git repository merged change will appear in this public GitHub repository.

Generated by 🚫 dangerJS against d2d43bb

… chains This change makes the wolfSSL backend sent the complete TLS client certificate chain. This align the wolfSSL backend with the behavior of the mbedTLS backend. Some servers need the intermediate certificates to verify a client certificate. If the provided PEM file contains only a single certificate this change has no effect and the behavior will be as before. This impacts higher level APIs to function as someone would expect. E.g.: esp_websocket_client_config_t.client_cert: when passing here a pem file containing 2 certificates (the CA's and the client's) it would be expected that both are transmitted during TLS handshake.

gojimmypi · 2024-04-16T03:05:38Z

Hi @frankencode and thank you for your interest in wolfssl.

Please note that the espressif/esp-wolfssl is stale and not managed by wolfssl as noted in espressif/esp-wolfssl#24 (comment).

We'd like to invite you to instead open pull requests or issues at wolfSSL/wolfssl.

Additionally, please note that we have wolfSSL available as a Managed Component as well as some examples using a GitHub clone of wolfssl as a component in the ESP-IDF with only a CMakeLists.txt.

AdityaHPatwardhan · 2024-05-20T12:30:47Z

components/esp-tls/esp_tls_wolfssl.c

@@ -97,7 +97,7 @@ static esp_err_t esp_load_wolfssl_verify_buffer(esp_tls_t *tls, const unsigned c
            wolf_fileformat = WOLFSSL_FILETYPE_ASN1;
        }
        if (type == FILE_TYPE_SELF_CERT) {
-            if ((*err_ret = wolfSSL_CTX_use_certificate_buffer( (WOLFSSL_CTX *)tls->priv_ctx, cert_buf, cert_len, wolf_fileformat)) == WOLFSSL_SUCCESS) {
+            if ((*err_ret = wolfSSL_CTX_use_certificate_chain_buffer_format( (WOLFSSL_CTX *)tls->priv_ctx, cert_buf, cert_len, wolf_fileformat)) == WOLFSSL_SUCCESS) {


Hi @frankencode Thanks for the PR.
Sorry for the really delayed review. I have confirmed that this change makes it same as mbedTLS.

No hurry. Good things take time.

AdityaHPatwardhan · 2024-05-20T12:31:39Z

components/esp-tls/esp_tls_wolfssl.c

@@ -310,6 +310,14 @@ static esp_err_t set_client_config(const char *hostname, size_t hostlen, esp_tls
 #endif /* CONFIG_WOLFSSL_HAVE_ALPN */
    }

+#ifdef CONFIG_WOLFSSL_HAVE_OCSP


Regarding this change. Where do you plan to add this CONFIG option ? Have you defined it in your own project?

I have same real issues building wolfssl as esp-idf component. I have a bit of hacked setup with the crappy scripts that ship with wolfssl which wildly copy around files to create an esp-idf component... That said I also added this OCSP option to the esp-wolfssl package which is still referenced in the esp-tls documentation:
espressif/esp-wolfssl#24 . The good thing about this esp-wolfssl is that it builds wolfssl from github and allows to cleanly track the upstream. At some point someone probably has to update this package to point to the latest wolfssl version. The name of the option will definitely be CONFIG_WOLFSSL_HAVE_OCSP as HAVE_OCSP is also the name used internally in wolfssl's config headers.

Ahh, okay I didn't notice this PR. Will see what can be done about the PR and updating the wolfSSL version

AdityaHPatwardhan · 2024-05-20T12:37:14Z

components/esp-tls/esp_tls_wolfssl.c

+    wolfSSL_CTX_EnableOCSP((WOLFSSL_CTX *)tls->priv_ctx, 0);
+#endif
+
+    wolfSSL_CTX_UseSNI(tls->priv_ctx, WOLFSSL_SNI_HOST_NAME, hostname, hostlen);


I think this change should go here ( i.e. under the skip_common_name option)

I'm not sure how SNI is related to CN server certificate check. In our day and age virtual hosting is common place and providing the actual Server Name (Indicator) should be a default behavior.

Yes it is the default behaviour.
If you notice that the option it is under is skip_common_name.
By default it is not set hence the default behaviour is to indicate the server name, as per your expectation.
But in some cases (e.g., when there is a local server and the CN would not match the IP of that server) then we provide an option to skip sending the server name indication so that the TLS connection becomes successful even if the server name does not match.

OK, I see mbedtls_ssl_set_hostname() has a special twist. As the documentation states: "Warning: If the hostname is not set with this function, Mbed TLS will silently skip certificate verification entirely. " Hence I moved the wolfSSL_CTX_UseSNI() call to make it behave similar when switching to the wolfSSL esp-tls backend. Check out the last commit. (I've force push updated it!-) I've tested it on my customer's app with wireshark. Works as expected for me.

AdityaHPatwardhan · 2024-05-21T09:54:52Z

components/esp-tls/esp_tls_wolfssl.c

@@ -288,6 +288,9 @@ static esp_err_t set_client_config(const char *hostname, size_t hostlen, esp_tls
            free(use_host);
            return ESP_ERR_WOLFSSL_SSL_SET_HOSTNAME_FAILED;
        }
+        /* Mimick the semantics of mbedtls_ssl_set_hostname(), which does not only set the SNI,
+           but if not set will make the mbedTLS stack skip certificate verification. */
+        wolfSSL_CTX_UseSNI(tls->priv_ctx, WOLFSSL_SNI_HOST_NAME, use_host, strlen(use_host));


Can you please check the return code in this case

AdityaHPatwardhan · 2024-05-21T09:57:06Z

components/esp-tls/esp_tls_wolfssl.c

@@ -310,6 +313,12 @@ static esp_err_t set_client_config(const char *hostname, size_t hostlen, esp_tls
 #endif /* CONFIG_WOLFSSL_HAVE_ALPN */
    }

+#ifdef CONFIG_WOLFSSL_HAVE_OCSP
+    wolfSSL_CTX_EnableOCSPStapling((WOLFSSL_CTX *)tls->priv_ctx );


Can you check the return codes for all the API?

Also, I think the order should be

wolfSSL_CTX_EnableOCSP
wolfSSL_CTX_EnableOCSPStapling
wolfSSL_UseOCSPStapling

Hi Aditya, I've updated the commit once more. I've added proper error handling and added comments to the calls. These OCSP related API calls are sadly not documented. I simply go by the usage as shown in the example client application (https://github.com/wolfSSL/wolfssl/blob/master/examples/client/client.c). And YES, I checked it actually works with wireshark of having OCSPv1 TLS extension send by client and having OCSP status forwarded by server.

Please see:

wolfSSL_CTX_EnableOCSP

wolfSSL_CTX_EnableOCSPStapling

wolfSSL_UseOCSPStapling

The web page appears to not always immediately auto-scroll to the correct location for me, so give it a few seconds, make sure the entire page loads. Ctrl-F might help navigate to the specific API documentation.

Thanks Jim, I didn't realize there are separate manual pages (I was missing the doxygen docs I used to see in other places in wolfSSL;-). I quickly checked if there is additional useful information. The "wolfSSL_CTX_EnableOCSP" man page shows an example for a different function and is missing the "WOLFSSL_OCSP_CHECKALL" flag used in the example client. The others I think I got right from reading the source code!-)

Glad to help. Having a robust integration with Espressif ESP-IDF is definitely on the roadmap for wolfSSL.

In the meantime, I've opened an issue regarding the documentation. See wolfSSL/wolfssl#7564

@gojimmypi Looks good to me. Btw. I figured I couldn't find the docs initially because that folder is not copied by your IDE/Espressif/*/setup.sh script. When I find the time I will update esp-wolfssl (or create something similar for my convenience).

Hi @frankencode - cool, thanks for taking a look. The PR was merger earlier.

yes - the script currently only copies the essential code for build and not the docs. I'm curious why you choose the script copy method? The new CMakeLists.txt allows you to point the component at a GitHub repo directory. For me, it is much easier to see and track changes with the cmake file.

I'm always interested in how others are using the wolfSSL library.

Cheers

Jim, I have to look at your new method at some later time, again. It initially turned me off that you couldn't follow the common conventions there for the example CMakeLists.txt. Components are searched for by EXTRA_COMPONENT_DIRS or similar. Paths to some local installation directories do not belong into a CMakeLists file! Also, as I told you before, with the current setup there are missing Kconfig variables which allow to select wolfSSL as esp-tls backend. And I think when I did add those I ran into many build issues... I'm currently only testing/prototyping. I surely going to clean up my build setup later-on and keep you posted when I figured out how to build wolfSSL latest version cleanly for esp-idf. So far updating esp-wolfssl looks like the best bet to me.

Thank for the feedback! You are absolutely right: the Kconfig definitely needs improvement. I'll be giving that attention soon. Let me know if there are any specifics that you think should be given priority. Perhaps you have a sample? Ideally, I'd of course like all of the wolfSSL settings to be configurable via Kconfig... but there are many.

Paths to some local installation directories do not belong into a CMakeLists file!

Although yes, there's flexibility to do this, the default behavior is to simply search the parent directories in the case of the examples and use the wolfSSL a few directories up.

In the case of a different directory, such as in my case - I have this:

workspace/wolfssl
workspace/myproject_using_wolfssl
workspace/myproject2_using_wolfssl

The value being ensuring all projects are pointing to a fixed wolfSSL git source code repository, and easily point to an alternate as desired. If there's a wolfssl directory in the myproject, then that one will be used.

I'll be sure and take a look at your EXTRA_COMPONENT_DIRS suggestion. I'm always open to making improvements.

I do believe that still works for a completely different location, as there's an Apple Homekit project doing that with wolfSSL.

Kind regards.

As far as I could observe the informal convention is to place components into "components" directories in esp-idf and have EXTRA_COMPONENT_DIRS point to it if its not a default location that is searched for. (An nobody certainly wants a custom search logic for each 3rd party component). I was initially testing my setup with the esp websocket client example: https://github.com/espressif/esp-protocols/tree/master/components/esp_websocket_client/examples/target . (But there are certainly better examples;-)

If you have some time I'd also be happy if you can review the two commits this comment section was actually meant to be about. THX.

AdityaHPatwardhan · 2024-05-23T10:22:59Z

components/esp-tls/esp_tls_wolfssl.c

@@ -310,6 +317,23 @@ static esp_err_t set_client_config(const char *hostname, size_t hostlen, esp_tls
 #endif /* CONFIG_WOLFSSL_HAVE_ALPN */
    }

+#ifdef CONFIG_WOLFSSL_HAVE_OCSP
+    /* increase error verbosity */
+    wolfSSL_CTX_EnableOCSP((WOLFSSL_CTX *)tls->priv_ctx, WOLFSSL_OCSP_CHECKALL);


Can you please add return check for this API as well.
I see in the wolfSSL manual that it is of int return type.

WOLFSSL_CTX* ctx = wolfSSL_CTX_new( method ); int options; // initialize to option constant ... int ret = wolfSSL_CTX_EnableOCSP(ctx, options); if(ret != SSL_SUCCESS){ // OCSP is not enabled }

You're right. I've added error handling to the wolfSSL_CTX_EnableOCSP(), too.

AdityaHPatwardhan · 2024-05-23T10:23:04Z

components/esp-tls/esp_tls_wolfssl.c

+        ESP_LOGE(TAG, "wolfSSL_UseOCSPStapling failed, returned %d", ret);
+        return ESP_ERR_WOLFSSL_SSL_SETUP_FAILED;
+    }
+    /* decrease error verbosity */


Why is it required to reduce the error verbosity after enabling OCSP stapling?

That is a good question I was asking myself, too. This call is something I copied from the example wolfSSL client. I thought it might be needed because I'm not registering a callback to receive status information on the OCSP progress/status. But it turns out this call has no effect on wolfSSL 5.7. The certificate manager associated with the context is the only component using this OCSP_CHECKALL flag and it cannot be reset to zero afterwards. That said I removed the last call and retested everything with wolfSSL 5.7 and I can see OCSP extension being transmitted as before. Which improves my understanding of this API and I also changed the comments accordingly.

AdityaHPatwardhan · 2024-05-23T16:40:38Z

sha=c909e58b39419dba9c0ac2878fc6d1b95db53057

AdityaHPatwardhan · 2024-05-30T04:33:09Z

components/esp-tls/esp_tls_wolfssl.c

+        /* Mimic the semantics of mbedtls_ssl_set_hostname(), which does not only set the SNI,
+           but if not set will make the mbedTLS stack skip certificate verification. */
+        if ((ret = wolfSSL_CTX_UseSNI(tls->priv_ctx, WOLFSSL_SNI_HOST_NAME, use_host, strlen(use_host))) != WOLFSSL_SUCCESS)
+        {


Suggested change

/* Mimic the semantics of mbedtls_ssl_set_hostname(), which does not only set the SNI,

but if not set will make the mbedTLS stack skip certificate verification. */

if ((ret = wolfSSL_CTX_UseSNI(tls->priv_ctx, WOLFSSL_SNI_HOST_NAME, use_host, strlen(use_host))) != WOLFSSL_SUCCESS)

{

/* Mimic the semantics of mbedtls_ssl_set_hostname(). */

if ((ret = wolfSSL_CTX_UseSNI(tls->priv_ctx, WOLFSSL_SNI_HOST_NAME, use_host, strlen(use_host))) != WOLFSSL_SUCCESS) {

Sorry I missed this syntax change previously.
I think commit needs to be updated as well, mbedTLS stack does not skip certificate verification.

Can you also check if the pre-commit passes for the MR

You can check if the pre-commit is passing with following command
pre-commit run --all-files --from-ref HEAD~8 --to-ref HEAD
I hope you have already installed pre-commits, if not please find the instructions here

@frankencode Can you please make this one change.
Thanks

I actually copied that comment from mbedtls documentation. The documentation on mbedtls_ssl_set_hostname states:

Warning: If the hostname is not set with this function, Mbed TLS will silently skip certificate verification entirely. Always set the hostname with this function - even when not using SNI!

For the esp_tls_mbedtls.c logic this means if cfg->skip_common_name is true mbedtls might do as the documenation says and skip any checks on the certificate send by the server.

But you are right, that might be an issue that needs to be raised on the mbedtls backend side and the comment is misplaced here!

I've updated the comment. Check out the new commit.

Almost all sites these days are virtually hosted and hence SNI (server name indicator TLS extension) should be enabled by default. In addition this change enables OCSP (online server status protocol) support for esp-tls clients using the wolfSSL backend. The 3 code lines enable OCSP stabling v1. By default this feature is disabled. (I will send another PR on esp-wolfssl repository to allow to enable it easily.)

AdityaHPatwardhan · 2024-05-30T12:25:52Z

sha=d2d43bbe78bdd9d62698f9c1db33d11a23ac8382

mahavirj · 2024-07-08T07:45:08Z

Merged with 7a12394 and 7e1e3df

frankencode mentioned this pull request Apr 16, 2024

change(esp-wolfssl): allow to enable OCSP support espressif/esp-wolfssl#24

Merged

espressif-bot added the Status: Opened Issue is new label Apr 16, 2024

github-actions bot changed the title ~~fix(esp-tls): make the wolfSSL backend send entire client certificate…~~ fix(esp-tls): make the wolfSSL backend send entire client certificate… (IDFGH-12621) Apr 16, 2024

espressif-bot assigned AdityaHPatwardhan Apr 22, 2024

espressif-bot added Status: In Progress Work is in progress and removed Status: Opened Issue is new labels Apr 22, 2024

AdityaHPatwardhan reviewed May 20, 2024

View reviewed changes

frankencode force-pushed the master branch from edd260e to 63c84a0 Compare May 21, 2024 09:30

AdityaHPatwardhan reviewed May 21, 2024

View reviewed changes

frankencode force-pushed the master branch from 63c84a0 to 5711493 Compare May 21, 2024 14:28

gojimmypi mentioned this pull request May 21, 2024

wolfSSL_CTX_EnableOCSP documentation update request wolfSSL/wolfssl#7564

Closed

AdityaHPatwardhan reviewed May 23, 2024

View reviewed changes

frankencode force-pushed the master branch from 5711493 to c909e58 Compare May 23, 2024 12:53

AdityaHPatwardhan approved these changes May 23, 2024

View reviewed changes

AdityaHPatwardhan added the PR-Sync-Merge Pull request sync as merge commit label May 23, 2024

AdityaHPatwardhan reviewed May 30, 2024

View reviewed changes

frankencode force-pushed the master branch from c909e58 to d2d43bb Compare May 30, 2024 11:51

AdityaHPatwardhan added the PR-Sync-Update Pull request sync fetch new changes label May 30, 2024

AdityaHPatwardhan removed the PR-Sync-Merge Pull request sync as merge commit label May 30, 2024

espressif-bot added Status: Done Issue is done internally Resolution: NA Issue resolution is unavailable and removed Status: In Progress Work is in progress labels Jun 3, 2024

gojimmypi mentioned this pull request Jun 13, 2024

Improving wolfSSL integration with the Espressif ESP-IDF (IDFGH-13019) #13966

Open

21 tasks

mahavirj closed this Jul 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(esp-tls): make the wolfSSL backend send entire client certificate… (IDFGH-12621) #13618

fix(esp-tls): make the wolfSSL backend send entire client certificate… (IDFGH-12621) #13618

frankencode commented Apr 15, 2024

github-actions bot commented Apr 15, 2024 •

edited

Loading

gojimmypi commented Apr 16, 2024 •

edited

Loading

AdityaHPatwardhan May 20, 2024

frankencode May 20, 2024

AdityaHPatwardhan May 20, 2024

frankencode May 20, 2024

AdityaHPatwardhan May 21, 2024

AdityaHPatwardhan May 20, 2024

frankencode May 20, 2024

AdityaHPatwardhan May 21, 2024

frankencode May 21, 2024

AdityaHPatwardhan May 21, 2024

AdityaHPatwardhan May 21, 2024

frankencode May 21, 2024 •

edited

Loading

gojimmypi May 21, 2024

frankencode May 21, 2024

gojimmypi May 21, 2024

frankencode May 22, 2024

gojimmypi May 22, 2024

frankencode May 22, 2024

gojimmypi May 22, 2024

frankencode May 22, 2024

AdityaHPatwardhan May 23, 2024

frankencode May 23, 2024

AdityaHPatwardhan May 23, 2024

frankencode May 23, 2024

AdityaHPatwardhan commented May 23, 2024

AdityaHPatwardhan May 30, 2024 •

edited

Loading

AdityaHPatwardhan May 30, 2024

frankencode May 30, 2024

frankencode May 30, 2024

AdityaHPatwardhan commented May 30, 2024

mahavirj commented Jul 8, 2024

fix(esp-tls): make the wolfSSL backend send entire client certificate… (IDFGH-12621) #13618

fix(esp-tls): make the wolfSSL backend send entire client certificate… (IDFGH-12621) #13618

Conversation

frankencode commented Apr 15, 2024

github-actions bot commented Apr 15, 2024 • edited Loading

gojimmypi commented Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankencode May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaHPatwardhan commented May 23, 2024

AdityaHPatwardhan May 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaHPatwardhan commented May 30, 2024

mahavirj commented Jul 8, 2024

github-actions bot commented Apr 15, 2024 •

edited

Loading

gojimmypi commented Apr 16, 2024 •

edited

Loading

frankencode May 21, 2024 •

edited

Loading

AdityaHPatwardhan May 30, 2024 •

edited

Loading