Skip to content

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

Merged
merged 1 commit into from
Jul 18, 2018

Conversation

andresag01
Copy link

@andresag01 andresag01 commented Aug 2, 2017

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 default
implementation of the mbedtls_time() function provided by mbed TLS is
sufficient 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

  • Tests
  • Documentation

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.

@andresag01
Copy link
Author

@sbutcher-arm @yanesca @RonEld please review

@andresag01
Copy link
Author

Please note that I have created this PR against mbed-os-example-tls to fix the problems introduced by merging this PR

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 13, 2017

@andresag01 status of this PR?

@adbridge
Copy link
Contributor

bump @andresag01

@andresag01
Copy link
Author

@adbridge @0xc0170: This PR is pending review by the mbed TLS team.

@yanesca @k-stachowiak @AndrzejKurek @sbutcher-arm

@cmonr
Copy link
Contributor

cmonr commented Feb 1, 2018

@andresag01, is this PR still pending review by @yanesca @k-stachowiak @AndrzejKurek @sbutcher-arm, or has it been superseded by something else?

@andresag01
Copy link
Author

@cmonr: My understanding is that it is pending review by @mazimkhan and @hanno-arm.

@cmonr
Copy link
Contributor

cmonr commented Feb 1, 2018

@andresag01 Thanks for the clarification.

@hanno-arm @mazimkhan, feel free to correct us if this is not the case.

@cmonr
Copy link
Contributor

cmonr commented Feb 23, 2018

@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?

@cmonr
Copy link
Contributor

cmonr commented Mar 6, 2018

I'm going to close this since there hasn't been any movement in this PR for over a month.
@mazimkhan, @hanno-arm, feel free to reopen with reviews.

@simonbutcher
Copy link
Contributor

@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.

@simonbutcher
Copy link
Contributor

@cmonr - Could you please re-open this PR so we can get it merged? And also remove the 'DRAFT' title?

@cmonr cmonr changed the title DRAFT: Integrate mbed OS RTC with mbed TLS Integrate mbed OS RTC with mbed TLS May 30, 2018
@cmonr cmonr reopened this May 30, 2018
@cmonr
Copy link
Contributor

cmonr commented May 30, 2018

@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.

@k-stachowiak
Copy link
Contributor

@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.
In order to submit a review I need to be set as a reviewer. @cmonr Could you or someone else from the Mbed OS team add me as a reviewer here?

@simonbutcher
Copy link
Contributor

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.

@simonbutcher
Copy link
Contributor

simonbutcher commented Jul 5, 2018

@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.

@cmonr cmonr requested review from simonbutcher and removed request for mazimkhan and hanno-becker July 5, 2018 18:48
@k-stachowiak
Copy link
Contributor

@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.

@cmonr
Copy link
Contributor

cmonr commented Jul 12, 2018

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.
@andresag01 andresag01 force-pushed the iotssl-1594-rtc-integration branch from 538fd62 to 5ced8e4 Compare July 16, 2018 20:02
@andresag01
Copy link
Author

@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...

@k-stachowiak
Copy link
Contributor

k-stachowiak commented Jul 17, 2018

@andresag01 My aim was to check if the proposed code builds. I took the hashing example, deployed mbed-os tree based on this branch, but rebased to the then current master, and built the application for K64F and for LPC1768. I checked two things: 1) that both cases build, 2) that the proper source is generated, by placing an #error directive in the parts of the source that I expected to be compiled in the respective cases.

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.

@andresag01
Copy link
Author

@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.

@cmonr
Copy link
Contributor

cmonr commented Jul 18, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Jul 18, 2018

Build : SUCCESS

Build number : 2628
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/4846/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci
Copy link

mbed-ci commented Jul 18, 2018

@mbed-ci
Copy link

mbed-ci commented Jul 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants