New mbedtls_x509_crt_parse_der_with_ext_cb() routine #3243

ndilieto · 2020-04-25T12:49:34Z

Description

This routine is functionally equivalent to mbedtls_x509_crt_parse_der(),
but it accepts an additional callback function which it calls with
every unsupported certificate extension.

Proposed solution to #3241

Signed-off-by: Nicola Di Lieto [email protected]

Status

READY

Requires Backporting

NO

Migrations

NO

This routine is functionally equivalent to mbedtls_x509_crt_parse_der(), but it accepts an additional callback function which it calls with every unsupported certificate extension. Proposed solution to Mbed-TLS#3241 Signed-off-by: Nicola Di Lieto <[email protected]>

ualpn requires mbedTLS to be configured and built with MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION which is not the default and can be a security risk. Therefore make BR2_PACKAGE_UACME_UALPN depend on BR2_PACKAGE_OPENSSL || BR2_PACKAGE_GNUTLS. Fixes http://autobuild.buildroot.net/results/d241121f8155bad9b6b25c16234576abb7fc940b See also ndilieto/uacme#23 Mbed-TLS/mbedtls#3241 Mbed-TLS/mbedtls#3243 http://lists.busybox.net/pipermail/buildroot/2020-April/281059.html http://lists.busybox.net/pipermail/buildroot/2020-April/281108.html Signed-off-by: Nicola Di Lieto <[email protected]> Signed-off-by: Thomas Petazzoni <[email protected]>

gilles-peskine-arm

Thank you for your contribution! I think the overall design is fine, it's simple and quite flexible. I left some individual comments on the interface, the documentation and the code.

We will need test cases for the new feature and a changelog entry in ChangeLog.d.

include/mbedtls/x509_crt.h

library/x509_crt.c

As suggested in Mbed-TLS#3243 (comment) Co-authored-by: Gilles Peskine <[email protected]> Signed-off-by: Nicola Di Lieto <[email protected]>

added make_copy parameter as suggested in Mbed-TLS#3243 (comment) Co-authored-by: Gilles Peskine <[email protected]> Signed-off-by: Nicola Di Lieto <[email protected]>

library/x509_crt.c

gilles-peskine-arm · 2020-05-28T11:55:05Z

Can you please edit the commits to have a signed-off-by line? Unfortunately applying GitHub suggestions doesn't do this. Also please give them a meaningful subject line. It would be better to squash the related ones together, too (e.g. “Minor documentation improvements”).

Co-authored-by: Gilles Peskine <[email protected]> Signed-off-by: Nicola Di Lieto <[email protected]>

new name: mbedtls_x509_crt_parse_der_with_ext_cb Co-authored-by: Gilles Peskine <[email protected]> Signed-off-by: Nicola Di Lieto <[email protected]>

As suggested in Mbed-TLS#3243 (comment) Co-authored-by: Gilles Peskine <[email protected]> Signed-off-by: Nicola Di Lieto <[email protected]>

added make_copy parameter as suggested in Mbed-TLS#3243 (comment) Co-authored-by: Gilles Peskine <[email protected]> Signed-off-by: Nicola Di Lieto <[email protected]>

ndilieto · 2020-05-28T15:26:07Z

Can you please edit the commits to have a signed-off-by line?

Done, and also squashed/rebased the trivial ones.

Please. let me know what else you expect me to do to get this merged.

mpg

Looks good to me.

Regarding the make_copy parameter, I understand and share Gilles's hesitations, but I don't have anything better to suggest, and I think we'll get an opportunity to clean things up when preparing 3.0, so I don't think those hesitation should prevent us from approving this.

ndilieto · 2020-06-11T05:55:38Z

Hi, many thanks for merging this. One last question, in which future mbedTLS release will this be included? I ask so that I can include this in uacme - see ndilieto/uacme#23

mpg · 2020-06-11T07:14:54Z

You're welcome, and thank you for contributing in the first place! This will be included in Mbed TLS 2.23.0, which should be released at the beginning of July.

This deprecated option is no longer needed following the merge of Mbed-TLS#3243 The removed option also affected the handling of critical unsupported certificate policies. Therefore also pass the certificate policies extension to the callback supplied to mbedtls_x509_crt_parse_der_with_ext_cb() if it contains unsupported policies, which allows the callback to fully replicate the behaviour of the removed option.

This deprecated option is no longer needed following the merge of Mbed-TLS#3243 The removed option also affected the handling of critical unsupported certificate policies. Therefore also pass the certificate policies extension to the callback supplied to mbedtls_x509_crt_parse_der_with_ext_cb() if it contains unsupported policies, which allows the callback to fully replicate the behaviour of the removed option. Signed-off-by: Nicola Di Lieto <[email protected]>

The mbedtls_x509_crt_parse_der_with_ext_cb function (available in mbedTLS 2.23.0 and later) allows parsing the "id-pe-acmeIdentifier" certificate extension without having to configure the deprecated MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION. Fixes #23 See also Mbed-TLS/mbedtls#3243

ualpn requires mbedTLS to be configured and built with MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION which is not the default and can be a security risk. Therefore make BR2_PACKAGE_UACME_UALPN depend on BR2_PACKAGE_OPENSSL || BR2_PACKAGE_GNUTLS. Fixes http://autobuild.buildroot.net/results/d241121f8155bad9b6b25c16234576abb7fc940b See also ndilieto/uacme#23 Mbed-TLS/mbedtls#3241 Mbed-TLS/mbedtls#3243 http://lists.busybox.net/pipermail/buildroot/2020-April/281059.html http://lists.busybox.net/pipermail/buildroot/2020-April/281108.html Signed-off-by: Nicola Di Lieto <[email protected]> Signed-off-by: Thomas Petazzoni <[email protected]> (cherry picked from commit 96c3b52) Signed-off-by: Peter Korsgaard <[email protected]>

This can be replaced with a version check, as the new function was merged upstream: Mbed-TLS/mbedtls#3243 (comment)

Following the update to mbedTLS 2.28.0 in commit 0f8aab0, ualpn can work with mbedTLS without restrictions. References https://git.buildroot.net/buildroot/commit?id=96c3b52132b41716ca445b4c73a1a8886c26e5ee ndilieto/uacme#23 (comment) ndilieto/uacme@bbee626 Mbed-TLS/mbedtls#3243 Signed-off-by: Nicola Di Lieto <[email protected]> Signed-off-by: Peter Korsgaard <[email protected]>

Following the update to mbedTLS 2.28.0 in commit 0f8aab0, ualpn can work with mbedTLS without restrictions. References https://git.buildroot.net/buildroot/commit?id=96c3b52132b41716ca445b4c73a1a8886c26e5ee ndilieto/uacme#23 (comment) ndilieto/uacme@bbee626 Mbed-TLS/mbedtls#3243 Signed-off-by: Nicola Di Lieto <[email protected]> Signed-off-by: Peter Korsgaard <[email protected]> (cherry picked from commit 6c7b469) Signed-off-by: Peter Korsgaard <[email protected]>

This was referenced Apr 26, 2020

RFC8737 and MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION #3241

Closed

Allow unsupported extensions in runtime #1425

Closed

danh-arm added enhancement needs-review Every commit must be reviewed by at least two team members, labels Apr 27, 2020

danh-arm assigned gilles-peskine-arm Apr 27, 2020

danh-arm added the needs-design-approval label Apr 27, 2020

gilles-peskine-arm self-requested a review April 27, 2020 10:30

danh-arm added this to the May 2020 Sprint milestone May 14, 2020

gilles-peskine-arm requested changes May 27, 2020

View reviewed changes

gilles-peskine-arm added approved for design needs-work and removed needs-design-approval needs-review Every commit must be reviewed by at least two team members, labels May 27, 2020

ndilieto added a commit to ndilieto/mbedtls that referenced this pull request May 28, 2020

mbedtls_x509_crt_ext_cb_t definition changed

4c3cb7a

As suggested in Mbed-TLS#3243 (comment) Co-authored-by: Gilles Peskine <[email protected]> Signed-off-by: Nicola Di Lieto <[email protected]>

hanno-becker reviewed May 28, 2020

View reviewed changes

library/x509_crt.c Outdated Show resolved Hide resolved

hanno-becker reviewed May 28, 2020

View reviewed changes

library/x509_crt.c Outdated Show resolved Hide resolved

gilles-peskine-arm added the needs-ci Needs to pass CI tests label May 28, 2020

ndilieto and others added 4 commits May 28, 2020 17:17

Minor style and documentation improvements

6e24980

Co-authored-by: Gilles Peskine <[email protected]> Signed-off-by: Nicola Di Lieto <[email protected]>

Rename mbedtls_x509_crt_parse_der_ext

fde98f7

new name: mbedtls_x509_crt_parse_der_with_ext_cb Co-authored-by: Gilles Peskine <[email protected]> Signed-off-by: Nicola Di Lieto <[email protected]>

mbedtls_x509_crt_ext_cb_t definition changed

fae25a1

As suggested in Mbed-TLS#3243 (comment) Co-authored-by: Gilles Peskine <[email protected]> Signed-off-by: Nicola Di Lieto <[email protected]>

mbedtls_x509_crt_parse_der_with_ext_cb enhancement

4dbe567

added make_copy parameter as suggested in Mbed-TLS#3243 (comment) Co-authored-by: Gilles Peskine <[email protected]> Signed-off-by: Nicola Di Lieto <[email protected]>

ndilieto force-pushed the development branch from 7717e4f to 2e6097c Compare May 28, 2020 15:23

ndilieto requested a review from hanno-becker June 2, 2020 13:50

mpg approved these changes Jun 10, 2020

View reviewed changes

mpg added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Jun 10, 2020

mpg merged commit 87a51aa into Mbed-TLS:development Jun 10, 2020

ndilieto mentioned this pull request Jun 11, 2020

Pass "certificate policies" extension to callback #3419

Merged

gilles-peskine-arm mentioned this pull request Aug 13, 2020

Allow mbed library to ignore critical extensions #3564

Closed

chris-jones-arm mentioned this pull request Apr 20, 2021

Remove MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION #4378

Closed

TRodziewicz mentioned this pull request May 11, 2021

Remove MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION #4477

Merged

3 tasks

ndilieto mentioned this pull request Feb 17, 2022

Build failure due to MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION ndilieto/uacme#23

Closed

ndilieto added a commit to ndilieto/uacme that referenced this pull request Feb 20, 2022

Simplify mbedtls_x509_crt_parse_der_with_ext_cb check

7f1ffbd

This can be replaced with a version check, as the new function was merged upstream: Mbed-TLS/mbedtls#3243 (comment)

daverodgman mentioned this pull request Apr 8, 2022

Added parsing of v3 extension subject key identifier #2018

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New mbedtls_x509_crt_parse_der_with_ext_cb() routine #3243

New mbedtls_x509_crt_parse_der_with_ext_cb() routine #3243

ndilieto commented Apr 25, 2020 •

edited

Loading

gilles-peskine-arm left a comment

gilles-peskine-arm commented May 28, 2020

ndilieto commented May 28, 2020

mpg left a comment

ndilieto commented Jun 11, 2020

mpg commented Jun 11, 2020

New mbedtls_x509_crt_parse_der_with_ext_cb() routine #3243

New mbedtls_x509_crt_parse_der_with_ext_cb() routine #3243

Conversation

ndilieto commented Apr 25, 2020 • edited Loading

Description

Status

Requires Backporting

Migrations

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented May 28, 2020

ndilieto commented May 28, 2020

mpg left a comment

Choose a reason for hiding this comment

ndilieto commented Jun 11, 2020

mpg commented Jun 11, 2020

ndilieto commented Apr 25, 2020 •

edited

Loading