search

zmap / zlint

X.509 Certificate Linter focused on Web PKI standards and requirements.
https://zmap.io
Apache License 2.0
349 stars 108 forks source link

Use help Method beforeoron instead of #717

Closed mtgag closed 3 months ago

mtgag commented 1 year ago

Two lints use the before method to the calculate the validity of certificates, while the BeforeOrOn help method should be used. Two certificates that test the edge case are also added.

mtgag commented 1 year ago

The implementation in the pull request and jzlint was motivated by the following discussions/pull requests:

https://github.com/zmap/zlint/issues/467 https://github.com/zmap/zlint/pull/469

Certificate eeServerCertValidOver398.pem (https://github.com/zmap/zlint/blob/master/v3/testdata/eeServerCertValidOver398.pem) from the testdata, as the name suggests, is valid over 398 days and the test lint_e_server_cert_valid_time_longer_than_398_days_test.go treats it as an error for the corresponding lint.

Accordingly, a certificate which has a notBefore "Jan 1 00:00:00 2023" and a notAfter "Feb 1 00:00:00 2023" has a validity of two months, which I believe is correct when the granularity of the duration is expressed in months.

CBonnell commented 1 year ago

The definition of Validity Period in the BRs was not aligned with the RFC 5280 definition until SC31. Changing the validity period calculation to align with 5280 for certificates issued prior to the SC31 effective date will yield false positive findings/errors.

Given this, I agree with @robplee that the changes proposed in this PR do not align with the BRs as written.

mtgag commented 1 year ago

Ok. Then, let us keep the certificates and change the assertion in the test to have an explicit test for this behaviour (or simply close this PR). I will commit this soon.

zakird commented 5 months ago

@CBonnell, are we in better shape after the latest change from @mtgag?

CBonnell commented 4 months ago

I think it's fine, as there appears to be no actual logic change introduced as part of this PR. It appears to merely add test cases of existing lints.