zmap / zcrypto

Liberal Go TLS + X.509 Library for Research
Other
135 stars 83 forks source link

backporting asn1, pkix to allow permissive mode for certificate parsing #298

Closed dissoupov closed 3 years ago

dissoupov commented 3 years ago

This change is used by censys in production for several weeks and we feel it's ready to be merged to master.

Changes:

Why?

There are many certificates created by different libraries and tools that may not confirm to strict DER encoding rules, therefore x509.ParseCertificate may fail. Some applications need to parse and annotate certificates, even if they are not encoded with strict DER rules. Setting asn1.AllowPermissiveParsing = true will allow permissive parsing.

Regressions

func (r RDNSequence) String() string

The standard Go uses a reverse order to print the RDNSequence while the legacy ZCrypto uses range function. Applications that rely on strict order in RDNSequence can set pkix.LegacyNameString = true to preserve legacy behaviour.

Example:

Standard Go: "serialNumber=67890, CN=name, serialNumber=12345, C=US, C=RU, postalCode=48109, ST=Michigan, L=Ann Arbor, street=2260 Hayward St, O=University of Michigan, OU=0x21, CN=common",
Legacy ZCrypto:   "CN=common, OU=0x21, O=University of Michigan, street=2260 Hayward St, L=Ann Arbor, ST=Michigan, postalCode=48109, C=US, C=RU, serialNumber=12345, CN=name, serialNumber=67890",
zakird commented 3 years ago

This looks very reasonable in the parts that have easily viewable diffs, though it's hard to tell what the broader implications will be with the unseen diff in the remainder of the ASN.1 code. I do know though that there's been quite a bit of testing at Censys though and I'm good with trying it. Want to give @cpu a chance to dive in though.

cpu commented 3 years ago

Want to give @cpu a chance to dive in though.

@zakird Sorry, I have a backlog of ZLint review tasks and won't likely be able to look at this branch. It's a massive diff and I don't see any way to review it against upstream in the current form without a huge manual effort.

zakird commented 3 years ago

@cpu yeah. :/ I think this is one of those ones we're just going to have to merge. We could test upstream dependencies though, @dissoupov, and at least have a bit more confidence. At the very least, we could try to point ZLint at this and see if there's any change in the more extensive tests there. @dissoupov would you be willing to do that and see if anything breaks?

dissoupov commented 3 years ago

@zakird we are already using ZLint 1.3 with the feature/parse_certs from ZCrypto in censys for several weeks. Zlint has a similar branch feature/parse_certs https://github.com/zmap/zlint/tree/feature/parse_certs

Looks like both repos are field tested in Prod

Once we merge ZCrypto, we will need to merge the feature branch in ZLint as well

zakird commented 3 years ago

~Oh perfect, just to confirm, the ZLint test suite was run against this branch as part of that and there weren't changes in any of the results?~ Oh, I see they have in Github

zakird commented 3 years ago

OK, sounds good. I'm going to go ahead and merge.