Add lint to check that a CRL does not contain an empty revokedCertificates element

[defacto64]

RFC5280 at §5.1.2.6 (Revoked Certificates) requires that:

When there are no revoked certificates, the revoked certificates list MUST be absent

This lint checks that the above requirement is satisfied.

[pgporada]

If you're at all interested in using cryptobyte to parse the DER bytes of a given CRL rather of relying upon crypto/x509 and asn1.Unmarshal to correctly parse a nil value or zero-value, check out my alternative implementation over here.

[aarongable]

It doesn't appear to me that asn1.Unmarshal has a documented guarantee that a nil slice means "this field was missing" and a present-but-zero-length slice means "this field was present but empty". Therefore I don't think that this asn1-based approach is guaranteed to remain correct, and that using a cryptobyte-based approach would be both simpler (no need to replicate all of the structs) and more obviously correct.

[defacto64]

@aarongable , my lint is not based on the assumption that a nil slice means "this field was missing" as you say; on the contrary, it is based on the assumption that a non-nil slice means "this field was found" (although it may be empty). I am achieving this by unmarshalling the DER based on an ad hoc struct. In my opinion this approach is sufficiently sound, and very readable.

At any rate, the asn1.Unmarshal function is used dozens of times within the Zlint source code, overall; if it were affected by the indeterminacy that you suggest, it would be quite worrying because it would potentially impact many other pieces of Zlint, in addition to my lint, as well as other important softwares that make use of that function; so I'm somewhat perplexed by what you're saying, and I'd like to hear the opinion of the authors of the encoding/asn1 package except that I don't know who they are and how to contact them....

[defacto64]

I have added the the human readable text to the test data, but something is causing 3 out of 4 checks to fail... not sure what.

[aaomidi]

Error is: lints/rfc/lint_crl_revoked_certificates_field_empty.go:46:6: NewEmptyRevokedCertificates redeclared in this block lints/rfc/lint_crl_empty_revoked_certificates.go:70:6: other declaration of NewEmptyRevokedCertificates

[christopher-henderson]

The function name NewEmptyrevokedCertificates got taken last week in https://github.com/zmap/zlint/commit/6c7d024812dfb2f25143f31e4655240d66e4058a.

It looks like this was caused by them not using newLint.sh which would have generated for them a constructor called NewRevokedCertificates.

There is no hard requirement on the precise names of these structs/functions so you are free to rename yours-or-theirs to something else that makes in order to fix this clash.

[pgporada]

It looks like this was caused by them not using newLint.sh which would have generated for them a constructor called NewRevokedCertificates.

I apologize, sorry about that. I did not use newLint.sh.

[christopher-henderson]

No worries @pgporada! It's a helper, not a requirement :smile:

[defacto64]

I have changed slightly the name of my constructor, now all checks are passed.