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

Backport 2.16: Always revoke certificate on CRL #3562

Conversation

raoulstrackx
Copy link
Contributor

@raoulstrackx raoulstrackx commented Aug 13, 2020

Description

backport of #3433
RFC5280 does not state that the revocationDate should be checked.

In addition, when no time source is available (i.e., when MBEDTLS_HAVE_TIME_DATE is not defined), mbedtls_x509_time_is_past always returns 0. This results in the CRL not being checked at all.

https://tools.ietf.org/html/rfc5280

Status

READY

Migrations

If there is any API change, what's the incentive and logic for it.

NO

@gilles-peskine-arm gilles-peskine-arm added bug component-x509 needs-review Every commit must be reviewed by at least two team members, labels Aug 13, 2020
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

One change is needed for 2.16 and 2.7. Other than that looks good to me.

@@ -1094,6 +1094,16 @@ component_test_null_entropy () {
make test
}

component_test_no_date_time () {
msg "build: default config without MBEDTLS_HAVE_TIME_DATE"
scripts/config.py unset MBEDTLS_HAVE_TIME_DATE
Copy link
Contributor

Choose a reason for hiding this comment

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

config.py needs to be config.pl in 2.16 and 2.7.

@raoulstrackx raoulstrackx force-pushed the raoul/verify_crl_without_time_bp2.16 branch from 0e75c4b to 2789382 Compare August 14, 2020 07:20
mpg
mpg previously approved these changes Aug 14, 2020
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.

@mpg mpg added the needs-ci Needs to pass CI tests label Aug 14, 2020
@gilles-peskine-arm gilles-peskine-arm removed needs-ci Needs to pass CI tests needs-review Every commit must be reviewed by at least two team members, labels Aug 14, 2020
@gilles-peskine-arm gilles-peskine-arm changed the title Always revoke certificate on CRL Backport 2.16: Always revoke certificate on CRL Aug 14, 2020
@mpg mpg added the approved Design and code approved - may be waiting for CI or backports label Aug 17, 2020
@gilles-peskine-arm gilles-peskine-arm added needs-work and removed approved Design and code approved - may be waiting for CI or backports labels Aug 19, 2020

Unverified

No user is associated with the committer email.
RFC5280 does not state that the `revocationDate` should be checked.

In addition, when no time source is available (i.e., when MBEDTLS_HAVE_TIME_DATE is not defined), `mbedtls_x509_time_is_past` always returns 0. This results in the CRL not being checked at all.

https://tools.ietf.org/html/rfc5280
Signed-off-by: Raoul Strackx <[email protected]>
@raoulstrackx raoulstrackx dismissed stale reviews from gilles-peskine-arm and mpg via 75475d8 August 26, 2020 09:43
@raoulstrackx raoulstrackx force-pushed the raoul/verify_crl_without_time_bp2.16 branch from 2789382 to 75475d8 Compare August 26, 2020 09:43
@mpg mpg self-requested a review August 26, 2020 09:53
@mpg mpg self-assigned this Aug 26, 2020
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.

LGTM, thanks for resolving the conflicts.

@mpg mpg added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Aug 26, 2020
@mpg mpg added the needs-ci Needs to pass CI tests label Aug 26, 2020
@mpg mpg removed the needs-ci Needs to pass CI tests label Aug 26, 2020
@mpg mpg requested a review from gilles-peskine-arm August 26, 2020 10:12
@gilles-peskine-arm gilles-peskine-arm added needs-ci Needs to pass CI tests 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, needs-ci Needs to pass CI tests labels Aug 26, 2020
@gilles-peskine-arm gilles-peskine-arm merged commit de4bcf2 into Mbed-TLS:mbedtls-2.16 Aug 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports bug component-x509
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants