Pass "certificate policies" extension to callback

[ndilieto]

Description

Pass the "certificate policies" extension to the callback supplied to mbedtls_x509_crt_parse_der_with_ext_cb() if it contains unsupported policies.

This allows the callback to fully replicate the behaviour of the deprecated MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION configuration.

Signed-off-by: Nicola Di Lieto nicola.dilieto@gmail.com

Status

READY

Requires Backporting

NO

[gilles-peskine-arm]

I'm afraid we can't remove this option now. That would be considered an incompatible change and we don't do that in minor versions of Mbed TLS. (We might make an exception for security, and even so, we still haven't removed the transition option MBEDTLS_TLS_DEFAULT_ALLOW_SHA1_IN_CERTIFICATES!) We'll definitely remove MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION in Mbed TLS 3.0.

Since we plan to release Mbed TLS 3.0 this year, I don't think it's worth it to touch MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION until then. We could change MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION to cause mbedtls_x509_crt_parse_der to invoke mbedtls_x509_crt_parse_der_with_ext_cb with an allow-everything callback, but that would make it more complex and I don't see any benefit.

[ndilieto]

Ok thanks. I understand. What about the handling of unsupported policies? Currently the "certificate policies" extension is known by mbedTLS and therefore never passed to the callback, even if it contains unsupported policies. I am not sure how common these policies are - do you think there is any value in making the callback handle them as I did in this PR, even if the deprecated option isn't removed?

[gilles-peskine-arm]

Passing some known extensions to the callback does make sense. It contradicts the current documentation for mbedtls_x509_crt_parse_der_with_ext_cb, so if we change that, we need to do it before the next release (currently scheduled for early July).

I wonder if this should be just for certificatePolicy with unknown policies, or possibly for other extensions? Maybe the documentation of mbedtls_x509_crt_parse_der_with_ext_cb could state that the callback may be called for known extensions and the exact list of extensions may change in future versions of the library? Or maybe the callback should be called for every extension?

[ndilieto]

I wonder if this should be just for certificatePolicy with unknown policies, or possibly for other extensions?

My view was only those that being potentially critical can make the parsing fail.

Maybe the documentation of mbedtls_x509_crt_parse_der_with_ext_cb could state that the callback may be called for known extensions and the exact list of extensions may change in future versions of the library?

I already changed it tn the PR to reflect the change I made: https://github.com/ARMmbed/mbedtls/pull/3419/files#diff-97f260303207ad6227a973dbe90fcc39

Or maybe the callback should be called for every extension?

I don't particularly like the sound of that, but I'm not sure.

[ndilieto]

I have reworked and force-pushed this. The deprecated option is no longer removed and I've only kept the passing of "certificate policies" to the callback if it contains unsupported policies.

[ronald-cron-arm]

Thanks for addressing my comments. LGTM.

[gilles-peskine-arm]

The code looks good to me. But please add a note about possible API evolution.