New mbedtls_x509_crt_parse_der_with_ext_cb() routine #3243
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 <nicola.dilieto@gmail.com>
This was referenced Apr 26, 2020
danh-arm
added
enhancement
needs-review
buildroot-auto-update
pushed a commit
to buildroot/buildroot
that referenced
this pull request
May 9, 2020
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 <nicola.dilieto@gmail.com> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
gilles-peskine-arm
requested changes
May 27, 2020
There was a problem hiding this comment.
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.
gilles-peskine-arm
added
approved for design
needs-work
and removed
needs-design-approval
needs-review
ndilieto
added a commit
to ndilieto/mbedtls
that referenced
this pull request
May 28, 2020
As suggested in Mbed-TLS#3243 (comment) Co-authored-by: Gilles Peskine <gilles.peskine@arm.com> Signed-off-by: Nicola Di Lieto <nicola.dilieto@gmail.com>
ndilieto
added a commit
to ndilieto/mbedtls
that referenced
this pull request
May 28, 2020
added make_copy parameter as suggested in Mbed-TLS#3243 (comment) Co-authored-by: Gilles Peskine <gilles.peskine@arm.com> Signed-off-by: Nicola Di Lieto <nicola.dilieto@gmail.com>
|
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 <gilles.peskine@arm.com> Signed-off-by: Nicola Di Lieto <nicola.dilieto@gmail.com>
new name: mbedtls_x509_crt_parse_der_with_ext_cb Co-authored-by: Gilles Peskine <gilles.peskine@arm.com> Signed-off-by: Nicola Di Lieto <nicola.dilieto@gmail.com>
As suggested in Mbed-TLS#3243 (comment) Co-authored-by: Gilles Peskine <gilles.peskine@arm.com> Signed-off-by: Nicola Di Lieto <nicola.dilieto@gmail.com>
added make_copy parameter as suggested in Mbed-TLS#3243 (comment) Co-authored-by: Gilles Peskine <gilles.peskine@arm.com> Signed-off-by: Nicola Di Lieto <nicola.dilieto@gmail.com>
Done, and also squashed/rebased the trivial ones. Please. let me know what else you expect me to do to get this merged. |
mpg
approved these changes
Jun 10, 2020
There was a problem hiding this comment.
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.
mpg
added
approved
|
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 |
|
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. |
ndilieto
added a commit
to ndilieto/mbedtls
that referenced
this pull request
Jun 11, 2020
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.
ndilieto
added a commit
to ndilieto/mbedtls
that referenced
this pull request
Jun 11, 2020
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 <nicola.dilieto@gmail.com>
ndilieto
added a commit
to ndilieto/uacme
that referenced
this pull request
Jun 21, 2020
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
buildroot-auto-update
pushed a commit
to buildroot/buildroot
that referenced
this pull request
Jul 27, 2020
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 <nicola.dilieto@gmail.com> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> (cherry picked from commit 96c3b52) Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
3 tasks
ndilieto
added a commit
to ndilieto/uacme
that referenced
this pull request
Feb 20, 2022
This can be replaced with a version check, as the new function was merged upstream: Mbed-TLS/mbedtls#3243 (comment)
woodsts
pushed a commit
to woodsts/buildroot
that referenced
this pull request
Jun 19, 2022
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 <nicola.dilieto@gmail.com> Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
damien-lemoal
pushed a commit
to damien-lemoal/buildroot
that referenced
this pull request
Jul 13, 2022
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 <nicola.dilieto@gmail.com> Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
woodsts
pushed a commit
to woodsts/buildroot
that referenced
this pull request
Jul 19, 2022
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 <nicola.dilieto@gmail.com> Signed-off-by: Peter Korsgaard <peter@korsgaard.com> (cherry picked from commit 6c7b469) Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
buildroot-auto-update
pushed a commit
to buildroot/buildroot
that referenced
this pull request
Jul 19, 2022
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 <nicola.dilieto@gmail.com> Signed-off-by: Peter Korsgaard <peter@korsgaard.com> (cherry picked from commit 6c7b469) Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 nicola.dilieto@gmail.com
Status
READY
Requires Backporting
NO
Migrations
NO