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.

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

Remove MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION #4378

Closed
chris-jones-arm opened this issue Apr 20, 2021 · 3 comments · Fixed by #4477
Closed

Remove MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION #4378

chris-jones-arm opened this issue Apr 20, 2021 · 3 comments · Fixed by #4477
Assignees
Labels
component-x509 enhancement good-first-issue Good for newcomers size-s Estimated task size: small (~2d)

Comments

@chris-jones-arm
Copy link
Contributor

chris-jones-arm commented Apr 20, 2021

Context

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.

This new function allows the user to pass a callback which gets called whenever an unsupported extension is encountered. The user can then react and deal with the specific extension inside the callback.


Rationale

As there is now a more flexible and capable function for handling unsupported x509 extensions it makes no sense to keep redundant compile time options in the library. In addition use of MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION could result in unsafe behaviour by completely ignoring critical extensions whereas the callback can be used to specific extensions which are known to be currently unsupported.


Work items for 3.0

  • Remove the option: MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION from config.h.
  • Remove all references to MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION in the library. The library should behave as if the option were always disabled.
  • Replace tests dependant on this option with tests using the callback instead.
  • Direct users to mbedtls_x509_crt_parse_der_with_ext_cb as a migration path, inside a changelog entry.
@chris-jones-arm chris-jones-arm added enhancement component-x509 good-first-issue Good for newcomers mbedtls-3 size-s Estimated task size: small (~2d) labels Apr 20, 2021
@mpg
Copy link
Contributor

mpg commented Apr 26, 2021

Replace tests dependant on this option with tests using the callback instead.

When I checked, there wasn't any test depending on this option, only on its negation. This is a test gap, and I suggest we close it as part of this issue, by adding a test using the callback: this will validate the migration path that we're suggesting to users.

Direct users to mbedtls_x509_crt_parse_der_with_ext_cb as a migration path, inside a changelog entry.

Briefly in the ChangeLog entry, and with some more details in the Migration Guide entry, now that it has been kick-started, see #4402

@TRodziewicz TRodziewicz self-assigned this May 11, 2021
@TRodziewicz
Copy link
Contributor

TRodziewicz commented May 11, 2021

Replace tests dependant on this option with tests using the callback instead.

When I checked, there wasn't any test depending on this option, only on its negation. This is a test gap, and I suggest we close it as part of this issue, by adding a test using the callback: this will validate the migration path that we're suggesting to users.

@mpg, I see that there are already 8 tests using the callback:
X509 CRT ASN1 (Unsupported critical extension recognized by callback)
X509 CRT ASN1 (Unsupported critical extension not recognized by callback)
X509 CRT ASN1 (Unsupported non critical extension recognized by callback)
X509 CRT ASN1 (Unsupported non critical extension not recognized by callback)
X509 CRT ASN1 (Unsupported critical policy recognized by callback)
X509 CRT ASN1 (Unsupported critical policy not recognized by callback)
X509 CRT ASN1 (Unsupported non critical policy recognized by callback)
X509 CRT ASN1 (Unsupported non critical policy not recognized by callback)

Do we still need some more?

@mpg
Copy link
Contributor

mpg commented May 12, 2021

Oh, good point, I didn't notice those tests! Nope, that's enough tests, it covers everything. So, no tests to add, and the existing tests that depend on the negation of the option can be kept unchanged (just removing the dependency). Good catch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-x509 enhancement good-first-issue Good for newcomers size-s Estimated task size: small (~2d)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants