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

Complete implementation of MbedTLS as backend #528

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

Conversation

huitema
Copy link
Collaborator

@huitema huitema commented May 16, 2024

No description provided.

@barracuda156
Copy link

@huitema How is this going?

@huitema huitema requested a review from kazuho June 2, 2024 15:19
@huitema
Copy link
Collaborator Author

huitema commented Jun 2, 2024

I think this PR is ready. It allows using MbedTLS as a self-sufficient back end, including for functions like certificate verification. I would like review of the code that derives the server's public key from the list of certificates. In all the tests, the size of the list is 1, so the assumption that the first certificate is good works. But if the list contains more certificates, we probably have some extra work to do.

@huitema
Copy link
Collaborator Author

huitema commented Jun 3, 2024

@kazuho in the test assets, do we have example of certificate chains containing more than 1 certificate?

@doublex doublex mentioned this pull request Aug 16, 2024
@doublex
Copy link

doublex commented Aug 18, 2024

@kazuho
Are there any reasons against this pull request?
An alternative to Openssl would be really helpful for embedded systems.

@doublex
Copy link

doublex commented Sep 10, 2024

The patch works for me (tested in production). Some issues:

a) Memory leak, this is never free'd:

static int mbedtls_verify_certificate(ptls_verify_certificate_t *_self, ptls_t *tls, const char *server_name,
    mbedtls_x509_crt* chain_head = (mbedtls_x509_crt*)malloc(sizeof(mbedtls_x509_crt));

int ptls_mbedtls_init_verify_certificate(ptls_context_t* ptls_ctx, char const* buffer, size_t len)
    mbedtls_x509_crt* chain_head = (mbedtls_x509_crt*)malloc(sizeof(mbedtls_x509_crt));

b) Unused variable:
https://github.com/huitema/picotls/blob/complete-mbedtls-backend/lib/mbedtls_sign.c#L406

c) Maybe a typo (mbedssl -> mbedtls):
ptls_mbedssl_init_verify_certificate_complete() -> ptls_mbedtls_init_verify_certificate_complete()

@doublex
Copy link

doublex commented Sep 10, 2024

Patch to fix the memory-leak:

*** a/mbedtls_sign.c    2024-09-10 16:56:25.685199324 +0200
--- b/mbedtls_sign.c    2024-09-10 16:56:05.489162611 +0200
*************** static int mbedtls_verify_certificate(pt
*** 1250,1255 ****
--- 1250,1256 ----
  
      if (chain_head != NULL) {
          mbedtls_x509_crt_free(chain_head);
+         free(chain_head);
      }
      return ret;
  }
*************** void ptls_mbedtls_dispose_verify_certifi
*** 1390,1395 ****
--- 1391,1397 ----
      if (verifier != NULL) {
          if (verifier->trust_ca != NULL) {
              mbedtls_x509_crt_free(verifier->trust_ca);
+             free(verifier->trust_ca);
              verifier->trust_ca = NULL;
          }
          if (verifier->trust_crl != NULL) {

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.

3 participants