-
Notifications
You must be signed in to change notification settings - Fork 3k
Integrate mbed OS RTC with mbed TLS #4846
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
Conversation
Please note that I have created this PR against mbed-os-example-tls to fix the problems introduced by merging this PR |
@andresag01 status of this PR? |
bump @andresag01 |
@adbridge @0xc0170: This PR is pending review by the mbed TLS team. @yanesca @k-stachowiak @AndrzejKurek @sbutcher-arm |
@andresag01, is this PR still pending review by @yanesca @k-stachowiak @AndrzejKurek @sbutcher-arm, or has it been superseded by something else? |
@cmonr: My understanding is that it is pending review by @mazimkhan and @hanno-arm. |
@andresag01 Thanks for the clarification. @hanno-arm @mazimkhan, feel free to correct us if this is not the case. |
@hanno-arm @mazimkhan Would y'all be able to review this PR any time soon? @0xc0170 Was the reason this was marked as DNM because the PR is listed as a draft? @andresag01, is this PR still a draft? |
I'm going to close this since there hasn't been any movement in this PR for over a month. |
@k-stachowiak / @AndrzejKurek - Could you both please review, and approve this PR so we can get it in? And could one of you also please test it on an actual board also? If you don't have a board with RTC, please say so, and we'll sort something else out. Thanks. |
@cmonr - Could you please re-open this PR so we can get it merged? And also remove the 'DRAFT' title? |
@sbutcher-arm Sure thing, although it seems a bit odd to reopen a PR that's months old. But I suppose it makes sense considering the small changeset. |
@sbutcher-arm I have checked this PR on a K64F with the x509 API and I can confirm that it does the expected thing. After adding the change I get the time/date functionality working out of the box. |
Hi @andresag01 / @k-stachowiak, It's good we've done the positive test on the K64F, but have we tested this on a board that doesn't have an RTC? What's the behaviour? What builds? If you answer that question, I should be able to approve. |
@cmonr - You can remove @hanno-arm and @mazimkhan from the review list. If it's approved by myself, @RonEld and @k-stachowiak, that's enough. |
@sbutcher-arm I checked that this change(*) builds against a RTCless target "LPC1768" and I can confirm that the RTC is properly detected at the build time. (*) this PR needs to be rebased as it was forked at the time before changes in the platform setup/teardown changes and taken as is won't build, when used with the Mbed TLS examples. The change is trivial and so is the rebase, which I have done locally. |
Expecting this PR to be rebased and will then start CI |
The integration is simply to define the macro MBEDTLS_HAVE_TIME_DATE in the features/mbedtls/platform/inc/platform_mbed.h. The default implementation of the mbedtls_time() function provided by mbed TLS is sufficient to work with mbed OS because both use POSIX functions.
538fd62
to
5ced8e4
Compare
@k-stachowiak: I have rebased the PR. I quickly tested the changes (compile only) with the HEAD of mbed-os and there were no build failures (other than the expected 'no entropy' errors) with K64F or LPC1768. Could you please specify what exactly did you try and post the diff of the patch you created to fix this problem? Also, as I mentioned earlier, the error that I do expect is that the tls-client example will fail because MBEDTLS_HAVE_TIME enables time/date checks when validating the certificate. However, the example code does not correctly set the clock for the checks to succeed. For this reason, I created this patch to filter out those errors and make the example succeed. That patch is really out-of-date though... |
@andresag01 My aim was to check if the proposed code builds. I took the The only problem that I encountered was lack of the platform initialization/teardown API in the MbedTLS. Since I deployed your branch, based of an older Mbed OS version into the up to date example programs, there was an incompatibility. The aforementioned rebase solved the problem without any other patches. I did not test it with other examples (the code I was testing is limited to Mbed OS), and I did not deploy the program to the target board. |
@k-stachowiak: Ok. The rebased is done now and the examples built without issues, so I assume no further work is needed from my part. |
/morph build |
Build : SUCCESSBuild number : 2628 Triggering tests/morph test |
Exporter Build : SUCCESSBuild number : 2267 |
Test : SUCCESSBuild number : 2375 |
…ration Integrate mbed OS RTC with mbed TLS
Description
This change integrates the mbed OS Real-Time Clock (RTC) into mbed TLS.
The integration is simply to define the macro
MBEDTLS_HAVE_TIME_DATE
in the
features/mbedtls/platform/inc/platform_mbed.h
. The defaultimplementation of the
mbedtls_time()
function provided by mbed TLS issufficient to work with mbed OS because both use POSIX functions.
Status
IN DEVELOPMENT
Migrations
This patch modifies the behaviour of the X509 module in mbed TLS because now the certificate verification process will check that the certificates date/time validity is correct when it previously did not. For this to work correctly, the application needs to correctly set up the RTC with a call to
set_time()
. For example, this change causes the mbed TLS example application tls-client to fail.YES
Todos
Steps to test or reproduce
Use mbed OS as normal. When verifying certificates, now the date/time validity checks will be exercised by the mbed TLS X509 module.