Skip to content

mbedtls: allow storing certificates in filesystem #13863

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

Closed

Conversation

facchinm
Copy link
Contributor

@facchinm facchinm commented Nov 4, 2020

Summary of changes

This patch adds TLSSocketWrapper::set_root_ca_cert_path to allow using a (filesystem) path instead than an xxd generated array to store CA certificates.
If filesystem support is enabled in mbed_config.h or systemwide we try invoking mbedtls_x509_crt_parse_path which returns the number of certificates it was able to parse (or < 0 if it failed).

Impact of changes

Migration actions required

Documentation


Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[x] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[x] Tests / results supplied as part of this PR

Reviewers


@facchinm facchinm force-pushed the mbedtls_certificates_in_filesystem branch from 128203d to 38f9ba0 Compare November 4, 2020 16:46
@ciarmcom ciarmcom requested a review from a team November 4, 2020 17:04
@ciarmcom
Copy link
Member

ciarmcom commented Nov 4, 2020

@facchinm, thank you for your changes.
@ARMmbed/mbed-os-maintainers please review.

@0xc0170 0xc0170 requested review from a team November 5, 2020 09:32
@Patater
Copy link
Contributor

Patater commented Nov 6, 2020

Why not add a new nsapi_error_t TLSSocketWrapper::set_root_ca_cert(const char *root_ca_path) method? I believe we are OK with that sort of API extension. As is, it seems pretty awkward to use the buffer passing method with a length of 0 to mean look it up in the filesystem unless you have MBEDTLS_FS_IO disabled in which case use it as a buffer actually.

Also, this will need tests added before we could accept this sort of PR.

@facchinm facchinm force-pushed the mbedtls_certificates_in_filesystem branch 3 times, most recently from 55d740e to 7595a47 Compare November 9, 2020 09:14
@facchinm
Copy link
Contributor Author

facchinm commented Nov 9, 2020

@Patater you are right, it was quite awkward. But the overload nsapi_error_t TLSSocketWrapper::set_root_ca_cert(const char *root_ca_path) is already there and requires a \0 terminated string ca_cert array (which is exactly the same API with automatic length).
I reimplemented using set_root_ca_cert_path as signature, is it better? Adding the tests later today

@facchinm facchinm force-pushed the mbedtls_certificates_in_filesystem branch 2 times, most recently from 0f4b69a to d91a7c4 Compare November 9, 2020 15:00
@facchinm facchinm force-pushed the mbedtls_certificates_in_filesystem branch from d91a7c4 to 2bdd2f2 Compare November 10, 2020 16:08
@facchinm
Copy link
Contributor Author

@Patater I added both a unit test and a greentea test. Should the dependency on MBEDTLS_FS_IO in the latter be moved one level up (like here )? Anything else I can do?

@facchinm facchinm changed the title [RFC] mbedtls: allow storing certificates in filesystem mbedtls: allow storing certificates in filesystem Nov 11, 2020
@facchinm facchinm force-pushed the mbedtls_certificates_in_filesystem branch from 2bdd2f2 to 5effb92 Compare November 12, 2020 11:54
@jeromecoutant
Copy link
Collaborator

Ping

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 3, 2020

@ARMmbed/mbed-os-security Can you please review?

@jeromecoutant
Copy link
Collaborator

@0xc0170 maybe we can start CI in // ...?

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 9, 2020

CI started

@mbed-ci
Copy link

mbed-ci commented Dec 9, 2020

Jenkins CI Test : ❌ FAILED

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-test ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️
jenkins-ci/mbed-os-ci_cloud-client-pytest

@mergify mergify bot added needs: work and removed needs: CI labels Dec 9, 2020
@ciarmcom ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Jul 9, 2021
@ciarmcom ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Jul 16, 2021
@ciarmcom ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Jul 25, 2021
@ciarmcom ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Aug 6, 2021
@ciarmcom ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Aug 15, 2021
@ciarmcom ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Aug 22, 2021
@ciarmcom ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Aug 29, 2021
@ciarmcom ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Sep 5, 2021
@ciarmcom ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Sep 18, 2021
@ciarmcom ciarmcom added the stale Stale Pull Request label Sep 26, 2021
@0xc0170 0xc0170 removed the stale Stale Pull Request label Sep 29, 2021
@0xc0170
Copy link
Contributor

0xc0170 commented Sep 30, 2021

@facchinm I think it will be better to create a new feature request for updating tls to 3.0, I am not aware of any plans to update for this new major version (this version contains the fix required here).

I'll close this pull request for now.

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.

8 participants