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 wolfSSL esp-tls and Certificate Bundle Support #7936

Merged

Conversation

gojimmypi
Copy link
Contributor

Description

Part of espressif/esp-idf#13966, this PR adds support for Certificate Bundles as used in the Espressif esp-tls layer.

There are currently no local wolfSSL examples that utilize this new functionality. I have an example app as well as required modifications needed to the ESP-IDF in my v5.2.2 branch of the ESP-IDF.

Related to:

Further information can be found in the included wolfcrypt/src/port/Espressif/esp_crt_bundle/README.md file.

The enclosed cacrt_all.pem, cacrt_deprecated.pem, and cacrt_local.pem files are copies of those file used by the mbedtls/esp_crt_bundle and should ideally be kept in sync with subsequent updates to the ESP-IDF.

Additional PR submissions coming soon will include updates to the KConfig and CMakeLists.txt files for existing wolfSSL examples. See my components/wolfssl for WIP.

My intention is to have common component files for all examples with settings controlled primarily by KConfig and sdkconfig.default files. The main reference source will be the template example.

Fixes zd#

Does not fix, but see related related tickets including 18469 and 18228.

Testing

How did you test?

Tested only in Espressif environment and a quick make clean && make

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

@gojimmypi gojimmypi self-assigned this Sep 3, 2024
@gojimmypi
Copy link
Contributor Author

Jenkins retest this please


/* Returns ESP_OK if there are no zero serial numbers in the bundle,
* OR there may be zeros, but */
static CB_INLINE int wolfssl_found_zero_serial()
Copy link
Contributor

Choose a reason for hiding this comment

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

wolfssl_found_zero_serial(void)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch. added void

* 0 if the cert has a non-zero serial number
* < 0 for error wolfssl\wolfcrypt\error-crypt.h values */
static CB_INLINE int wolfssl_is_zero_serial_number(const uint8_t *der_cert,
int sz) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Brace on new line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missed that one; moved and aligned params

{
char stringVaue[X509_MAX_SUBJECT_LEN];
#ifdef WOLFSSL_DEBUG_CERT_BUNDLE
char before_str[32];
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use CTC_DATE_SIZE instead of hard coded 32?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good idea. added.

#endif /* WOLFSSL_DEBUG_CERT_BUNDLE && WOLFSSL_DEBUG_IGNORE_ASN_TIME */

#ifdef CONFIG_WOLFSSL_DEBUG_CERT_BUNDLE
void print_cert_subject_and_issuer(WOLFSSL_X509_STORE_CTX* store) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Brace new line. Move int i declaration to top of function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. Also added NULL check for store.

* We'll proceed by assiging `cert` to each of the respective items in
* bundle as we attempt to find the desired cert: */
if (ret == WOLFSSL_SUCCESS) {
_cert_bundled_loaded = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

I see where this is set but not where it is used.

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 is intended for end users. Added wolfssl_cert_bundle_loaded() to file: esp_crt_bundle.h .

Also renamed removed stray middle 'd' from _cert_bundled_loaded .

WOLFSSL_X509_STORE_CTX* store)
{
#ifdef WOLFSSL_DEBUG_CERT_BUNDLE
char before_str[32];
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe CTC_DATE_SIZE instead of 32?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, good idea. Added.

child = crt;

#if 0
/* It's OK for a trusted cert to have a weak signature hash alg.
Copy link
Contributor

Choose a reason for hiding this comment

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

Dead code? Does this need to be here still?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need. Removed.

int middle =0;
size_t name_len = 0;
size_t key_len = 0;
bool crt_found = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't use bool or true/false. Use int and 1/0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

int ret = -1;
int start = 0;
int end = 0;
int middle =0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Space after =

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, always appreciated, fixed. I also updated the order.

@dgarske dgarske removed their assignment Sep 3, 2024
gojimmypi added a commit to gojimmypi/wolfssl that referenced this pull request Sep 3, 2024
@gojimmypi gojimmypi force-pushed the pr-add-espressif-esp-tls-cert-bundle branch from e478203 to 808cced Compare September 3, 2024 22:33
@dgarske
Copy link
Contributor

dgarske commented Sep 4, 2024

Retest this please. Scan-build timeout

@gojimmypi gojimmypi force-pushed the pr-add-espressif-esp-tls-cert-bundle branch from 808cced to ac57452 Compare September 4, 2024 16:38
dgarske
dgarske previously approved these changes Sep 4, 2024
Copy link
Contributor

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

Approved. Over to PR cap.

@dgarske dgarske assigned wolfSSL-Bot and unassigned dgarske Sep 4, 2024
@gojimmypi
Copy link
Contributor Author

Known issues:

Here are some key points in addition to the new README.md file included in this PR.

PlatformIO

Update / clarification: Although the esp-tls is fully supported in the Espressif ESP-IDF with this PR, including Certificate Bundles, using the PlatformIO with the Certificate Bundles is not supported at this time.

Other esp-tls functionality should work just fine in either environment.

It appears the problem in the PlatformIO environment is that they have their own build system: similar but different than the build system used in ESP-IDF. In particular, it seems the gen_crt_bundle.py python script in the CMakeFiles.txt is not executed as desired at build time. This manifests as this error:

*** [.pio\build\esp32dev\.pio\build\esp32dev\x509_crt_bundle_wolfssl.S.o] 
Source `.pio\build\esp32dev\x509_crt_bundle_wolfssl.S' not found, 
needed by target `.pio\build\esp32dev\.pio\build\esp32dev\x509_crt_bundle_wolfssl.S.o'.

Here's a similar problem, not involving wolfSSL: https://community.platformio.org/t/esp-idf-new-project-build-error/23858/2

A workaround is to either disable Bundle Support, or select Do not use the default certificate bundle.

Use the PlatformIO pio run -t menuconfig:

Component config  ---> 
  wolfSSL  --->
    wolfSSL ESP-TLS  --->
      Certificate Bundle  --->
        Default certificate bundle options 

image

This is only a problem with PlatformIO. I'll be working on alternatives that will be included in a possible future PR.

PlatformIO Python cryptography package

The PlatformIO Python environment may be missing the cryptography package that is needed to pre-parse and sort the CA Certificated in the Bundle. A script such as this one can be used to ensure the package is installed:

Import("env")
import subprocess
import sys

# Function to install a package
def install(package):
    subprocess.check_call([sys.executable, "-m", "pip", "install", package])

try:
    # Check if cryptography is installed
    import cryptography
except ImportError:
    # If not installed, install it
    print("cryptography package not found. Installing...")
    install("cryptography")

Examples using "howsmyssl": TLS 1.3 not fully implemented

Additionally, I'd like to point out that the Espressif examples make frequent use of the https://www.howsmyssl.com/ web site. During development a variety of problems were encountered with connections to that web site from the ESP32 examples. Later confirmed in jmhodges/howsmyssl#716 (comment) is that TLS 1.3 is not fully supported. I've opened espressif/esp-idf#14496 to improve visibility to this known issue.

The problem here is that when using wolfSSL in the esp-tls layer of the ESP-IDF, and forcing only TLS 1.3, the example apps will report a connection error. This is not an issue with wolfSSL.

@gojimmypi gojimmypi changed the title Add wolfSSL esp-tls Certificate Bundle Support Add wolfSSL esp-tls and Certificate Bundle Support Sep 6, 2024
Copy link
Contributor

@douzzer douzzer left a comment

Choose a reason for hiding this comment

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

there's one whitespace infraction (inline-commented).

also you might want to fix up some overlong lines:

overlong lines added:
wolfcrypt/src/port/Espressif/esp_crt_bundle/esp_crt_bundle.c:489             ESP_LOGCBI(TAG, "Failed to extract subject or issuer at index %d\n", i);
wolfcrypt/src/port/Espressif/esp_crt_bundle/esp_crt_bundle.c:784             ESP_LOGCBV(TAG, "Item = %d; start: %d, end: %d", middle, start, end );
wolfssl/wolfcrypt/port/Espressif/esp_crt_bundle.h:30  * https://docs.espressif.com/projects/esp-idf/en/stable/esp32/api-reference/protocols/esp_tls.html
wolfssl/wolfcrypt/port/Espressif/esp_crt_bundle.h:62  * Attach and enable use of a bundle for certificate verification through a verification callback.
wolfssl/wolfcrypt/port/Espressif/esp_crt_bundle.h:63  * If no specific bundle has been set through esp_crt_bundle_set() it will default to the
wolfssl/wolfcrypt/port/Espressif/esp_crt_bundle.h:66  * Note this must be visibile for both the regular bundles, as well as the "none" option.
wolfssl/wolfcrypt/port/Espressif/esp_crt_bundle.h:73  *             - Other   if an error occured or an action must be taken by the calling process.

(the line numbers may not match exactly, because the test was on a rebase.)


## Getting Started

Use the `idf.py menuconfig`,
Copy link
Contributor

Choose a reason for hiding this comment

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

trailing whitespace on this line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

eek. I usually don't need to worry about trailing whitespace. today I needed to edit my "ignore pattern". May have been an added feature in recent update in Visual Studio:

image

Otherwise, I completely missed the long lines. Good catch! I've updated the code. There's one extra file this time around, oddly a space missing between two words in a different README.md file.

@gojimmypi
Copy link
Contributor Author

I've applied quite a lot of updates since the original PR from a couple of week ago & squashed commits .

The changes address some things that came about while testing on various flavors of ESP32.

In particular, the entire RSA math HW has been overhauled to have improved. There's also some new metrics.

A memory leak was found and corrected. There's still a memory leak with the ESP32-C2 http example as-found in v5.2 that is only manifested in continual loops (see test app). The problem is not in any of the changes in this PR, nor even any part of wolfSSL.

This has been tested on the 8 different ESP32 flavors (classic,C2(ESP8684)/C3,C6,S2,S3,S6). There's a known issue with ESP8266 config related to #8015.

There's a known problem with the ESP32-C2 WiFi. See espressif/esp-idf#11703 (comment):

Please try the suggestion here: espressif/esp-idf#10782 (comment)

[when your board] is using a 26 MHz XTAL. If you set the XTAL to 26 MHz in menuconfig, then Wi-Fi will work correctly and you also won't need to set the baud rate to 74800 when running idf.py monitor.

@gojimmypi
Copy link
Contributor Author

gojimmypi commented Sep 25, 2024

I've applied code review changes & have tests re-running on all 8 of 9 devices. The ESP8266 remains problematic, somewhat related to these changes, but only with regards to default application settings.

Reminder: There's no planned support for wolfSSL in the esp-tls layer of the ESP8266 RTOS Software Development Kit at this time. We could have a fork that supports it if needed. Not sure what the long term plans are for that ESP8266 SDK.

Today, there's only full support of wolfSSL in the esp-tls layer in my v5.2.2 fork. I've not yet created an ESP-IDF upstream PR. Lastest PlatformIO release is still v5.2.2.20240904b

The Espressif ESP-IDF v5.2.3 was released yesterday.

Additional, separate implementations and PRs will be needed for each Espressif ESP-IDF version: 5.0, 5.1, etc.

References and Notes

My next planned update is for v5.3.1. Edit: See my_531

See gojimmypi/esp-idf#1 regarding building an ESP-IDF release for PlatformIO. It is not very intuitive.

See also my https://github.com/gojimmypi/github-actions/tree/release_idf mentioned in that discussion.

Edit: Reminder for PlatformIO release as noted in gojimmypi/esp-idf#1 (comment):

Fork the IDF version you want to use. Do your changes. Add a valid package.json and version.txt in the root. Create a release.

For instance: https://github.com/gojimmypi/esp-idf/releases/tag/v5.3.1.20240925a created with gojimmypi/esp-idf@f656a0c (although it should be ~70 MB, not 1.6 GB)

Edit: See gojimmypi/esp-idf#1 (comment) regarding the oversized release. The workflow file needed to be updated like this:

name: Create zip file with recursive source clone for release

on:
  push:
    tags:
      - v*

jobs:
  release_zips:
    name: Create release zip file
    runs-on: ubuntu-20.04
    steps:
      - name: Create a recursive clone source zip
        uses: gojimmypi/github-actions/release_zips@release_idf
        env:
            RELEASE_PROJECT_NAME: ESP-IDF
            GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

Note in particular it points to uses: gojimmypi/github-actions/release_zips@release_idf here on my release_idf branch.

Documentation to more clearly explain this process is currently WIP, but should generally be found in the README.md of each respective custom ESP-IDF branch.

@gojimmypi
Copy link
Contributor Author

Jenkins retest this please.

dgarske
dgarske previously approved these changes Sep 25, 2024
wolfcrypt/src/port/Espressif/esp32_mp.c Outdated Show resolved Hide resolved
wolfcrypt/src/port/Espressif/esp_crt_bundle/README.md Outdated Show resolved Hide resolved
* to be utilized by the Espressif ESP-IDF, specifically the esp-tls layer.
*
* See:
* https://docs.espressif.com/projects/esp-idf/en/stable/esp32/api-reference/protocols/esp_tls.html
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this link could go in the documentation or manual rather than in the main wolfssl bundle. The site looks hosted in Poland which is okayish for some users, but the certificate on the site has CN listed as the country.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found 59 instances of docs.espressif.com in 49 files.

I'll remove or change the links in this PR.

Ok to have a separate PR for the remainder or would you prefer they are included here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update: a total of 112 instances in 72 files that reference espressif.com in a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update[2]: a total of 206 instances in 102 files, if including README.md files in addition to *.c and *.h files.

Copy link
Contributor

Choose a reason for hiding this comment

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

separate PR's is ok

@gojimmypi
Copy link
Contributor Author

Thanks @JacobBarthelmeh for the code review. Some excellent ideas. See my dev branch for these changes. Once Jenkins is happy, I'll squash the commits in this PR.

@gojimmypi
Copy link
Contributor Author

All ESP32 devices passing wolfssl_test on my test jig.

image

The ESP8266 failure was indeed just a config issue, needing #define FP_MAX_BITS MIN_FFDHE_FP_MAX_BITS.

The esp-tls layer is still not supported on the ESP8266, but at least now the project file compiles and works as before.

My updates to examples will be in a separate PR.

@JacobBarthelmeh JacobBarthelmeh merged commit b96e73f into wolfSSL:master Sep 27, 2024
137 checks passed
@gojimmypi gojimmypi deleted the pr-add-espressif-esp-tls-cert-bundle branch October 9, 2024 18:08
jefferyq2 added a commit to jefferyq2/wolfssl that referenced this pull request Oct 22, 2024
…ls-cert-bundle

Add wolfSSL esp-tls and Certificate Bundle Support
jefferyq2 added a commit to jefferyq2/wolfssl that referenced this pull request Oct 26, 2024
…ls-cert-bundle

Add wolfSSL esp-tls and Certificate Bundle Support
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.

5 participants