Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION #4477

Merged
merged 3 commits into from
Jun 10, 2021
Merged

Remove MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION #4477

merged 3 commits into from
Jun 10, 2021

Conversation

TRodziewicz
Copy link
Contributor

Signed-off-by: TRodziewicz [email protected]

Description

The config option MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION is now redundant in the library (as of #3243) as there is a more flexible runtime option for dealing with unsupported extensions: mbedtls_x509_crt_parse_der_with_ext_cb.

Fixes: #4378

Status

IN DEVELOPMENT

Requires Backporting

NO?

Migrations

NO

Todos

  • Tests
  • Documentation
  • Changelog updated

@TRodziewicz TRodziewicz self-assigned this May 11, 2021
@TRodziewicz TRodziewicz added component-x509 enhancement mbedtls-3 needs-ci Needs to pass CI tests size-s Estimated task size: small (~2d) needs-work labels May 11, 2021
@TRodziewicz TRodziewicz added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review needs-work needs-ci Needs to pass CI tests and removed needs-ci Needs to pass CI tests needs-work needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels May 12, 2021
@TRodziewicz TRodziewicz added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review and removed needs-ci Needs to pass CI tests labels May 12, 2021
davidhorstmann-arm
davidhorstmann-arm requested changes May 12, 2021
View reviewed changes
Copy link
Contributor

@davidhorstmann-arm davidhorstmann-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small nit, otherwise LGTM.

ChangeLog.d/issue4378.txt Outdated Show resolved Hide resolved
davidhorstmann-arm
davidhorstmann-arm previously approved these changes May 12, 2021
View reviewed changes
Copy link
Contributor

@davidhorstmann-arm davidhorstmann-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

ronald-cron-arm
ronald-cron-arm previously requested changes May 17, 2021
View reviewed changes
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rebase to avoid the first merge commit.

@ronald-cron-arm ronald-cron-arm removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels May 17, 2021
@mpg
Copy link
Contributor

mpg commented Jun 7, 2021

@gilles-peskine-arm @ronald-cron-arm is one of you planning to review this, or should it be labeled "needs: reviewer"?

@gilles-peskine-arm
Copy link
Contributor

It isn't on my queue now (but I may get around to it later if nobody else does).

@gilles-peskine-arm gilles-peskine-arm added the needs-reviewer This PR needs someone to pick it up for review label Jun 7, 2021
@ronald-cron-arm
Copy link
Contributor

Not especially on my list, either.

@ronald-cron-arm ronald-cron-arm removed their request for review June 7, 2021 12:54
mpg
mpg requested changes Jun 9, 2021
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, except for what looks like a mistake in conflict resolution, and the usual suggestions for improving the wording of the migration guide.

include/mbedtls/config.h Outdated
@@ -1909,16 +1909,14 @@
#define MBEDTLS_VERSION_FEATURES

/**
* \def MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION
* \def MBEDTLS_X509_ALLOW_EXTENSIONS_NON_V3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This option was removed in #4581, we don't want to re-introduce it here. This is probably an issue with conflict resolution in the merge commit.

docs/3.0-migration-guide.d/remove_MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION.md Outdated
@@ -0,0 +1,13 @@
Remove the X509 parser sensitivity control for an unknown critical extension from config.h
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It think it's clearer to say "Remove the config option MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION"

docs/3.0-migration-guide.d/remove_MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION.md Outdated
Remove the X509 parser sensitivity control for an unknown critical extension from config.h
------------------------------------------------------------------------------------------

It affects users who use the `MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should provide a bit more context here. Suggested text: "This change does not affect users of the default configuration; it only affect users who enable this option. The X.509 standard says that implementation must reject critical extensions that they don't recognize, and this is is what Mbed TLS does by default. This option allowed to continue parsing those certificates but didn't provide a convenient way to handle those extensions." Then keep the second paragraph.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@mpg mpg added needs-work and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Jun 9, 2021
@TRodziewicz TRodziewicz force-pushed the Remove__X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION branch from ba1197c to 8dd46ab Compare June 9, 2021 11:25
@TRodziewicz TRodziewicz force-pushed the Remove__X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION branch from 8dd46ab to 0ea2576 Compare June 9, 2021 11:32
@TRodziewicz TRodziewicz added needs-ci Needs to pass CI tests and removed needs-work labels Jun 9, 2021
davidhorstmann-arm
davidhorstmann-arm approved these changes Jun 9, 2021
View reviewed changes
Copy link
Contributor

@davidhorstmann-arm davidhorstmann-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - cheers!

mpg
mpg approved these changes Jun 10, 2021
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing my feedback. Looks good to me now.

@mpg mpg dismissed ronald-cron-arm’s stale review June 10, 2021 07:12

Feedback has been addressed

@mpg mpg added approved Design and code approved - may be waiting for CI or backports and removed needs-ci Needs to pass CI tests labels Jun 10, 2021
@mpg mpg merged commit 44eea8f into Mbed-TLS:development Jun 10, 2021
@torkleyy torkleyy mentioned this pull request Oct 6, 2023
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gilles-peskine-arm gilles-peskine-arm gilles-peskine-arm left review comments

@mpg mpg mpg approved these changes

@davidhorstmann-arm davidhorstmann-arm davidhorstmann-arm approved these changes

@ronald-cron-arm ronald-cron-arm ronald-cron-arm left review comments

Assignees

@TRodziewicz TRodziewicz

Labels
approved Design and code approved - may be waiting for CI or backports component-x509 enhancement size-s Estimated task size: small (~2d)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION
7 participants
@TRodziewicz @mpg @gilles-peskine-arm @ronald-cron-arm @davidhorstmann-arm @yanesca @tomasz-rodziewicz-mobica