make extension parsing more permissive

[iJustErikk]

if allowpermissive, then tolerate errors in extension parsing

searched for returns in parsecertificate. continued in each case for an easy check that this is as nil safe as the code was before

here is a cert that we can now permissively parse:

MIICQDCCAamgAwIBAgIQa9ciWy1lvZtGRujXx93btjANBgkqhkiG9w0BAQUFADBAMRowGAYDVQQDExFEaWdpQ2VydCBzZWN1cml0eTEVMBMGA1UEChMMRGlnaUNlcnQgSW5jMQswCQYDVQQGEwJVUzAeFw0yMzA3MDQyMTE3MjRaFw0yNTA3MDQyMTE3MjRaMEAxGjAYBgNVBAMTEURpZ2lDZXJ0IHNlY3VyaXR5MRUwEwYDVQQKEwxEaWdpQ2VydCBJbmMxCzAJBgNVBAYTAlVTMIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQCoqY2vi90BoFCGapgvjh2uCtqKBOFjN+3kU2Lc2UuSlt/pbJIBsRK+pAFkgy1khodbkzthiuzzDzqb95Y9a3eJ+G3dx1aW872v8ub8qe/ZlXOVDnNh/AahIzKlBGE3cuCT7eHJnGHt2tUmAoCc5OodgXdso6vKYxdYrszLrDnYWQIDAQABozswOTALBgNVHQ4EBAMCA7gwCwYDVR0PBAQDAgO4MB0GA1UdJQQWMBQGCCsGAQUFBwMBBggrBgEFBQcDAjANBgkqhkiG9w0BAQUFAAOBgQCnChlYocxRYGl7QSq5luUpak2L5u0PcsEzCUd9yIUCB1eubqMVQTJfp7frMWM313aBpNeWQ3leLZvNn3hzPPBWy8ZH18plZRjkTxh9U7cEK3I/yltzLlZdUPNqgEJdeHMHYjKtO7LcZATm+DAhkk0HdX0KS4dN0G+zFKFWCEPhCg==

[zakird]

This broadly makes sense to me. Do we, however, want some way of indicating that the certificate has only partially been parsed? It seems bad to simply swallow the errors and not indicate that while we're giving back a result, it could be nonsensical with only a fraction of the (intended) encoded data?

[iJustErikk]

yeah I could just have an error that we'll conditionally toss permissive errors into and prefix it with permissive:. Does that sound good?

[zakird]

Yeah, I think that makes sense.

[codyprime]

yeah I could just have an error that we'll conditionally toss permissive errors into and prefix it with permissive: . Does that sound good?

Will this still work as the original PR intended? With this error appending addition to the PR, ParseCertificate will now return a non-nil error even with asn1.AllowPermissiveParsing.

[iJustErikk]

yeah I could just have an error that we'll conditionally toss permissive errors into and prefix it with permissive: . Does that sound good?

Will this still work as the original PR intended? With this error appending addition to the PR, ParseCertificate will now return a non-nil error even with asn1.AllowPermissiveParsing.

the user can check for "permissive" to know if the parsing only had permissive errors. additionally, we only append on the new checks, so users of the old "just ignore it" behavior won't be affected

[elliotcubit]

I missed the boat here, I think we should return without an error? That's the behavior everywhere else we do permissive parsing.

If we really want to start returning these errors, we should do so with an explicit go type, but I don't think we want that? Or at least, if we add such a feature, it shouldn't just be here.

[codyprime]

FWIW, I'm fine returning without an error, since it is explicitly requested with AllowPermissiveParsing to being with

[zakird]

Why not return and even capture the error? Like the caller can throw it away if they want and take whatever came back? There was a problem parsing and the caller should treat the result with caution?

[codyprime]

I was OK enough with it to give it my approval, but for my $0.02 I think the concerns I'd have are twofold:

1. It is somewhat ad-hoc and brittle to check error message strings, especially without a consistent project-wide scheme, and
2. The AllowPermissiveParsing flag already indicates the caller is OK with less than strict parsing, so if we are relaxing within that parameter I don't see it strictly necessary to return an err

[elliotcubit]

This line in the patch handling the error for this one case was the red flag for me -- this pattern becomes necessary everywhere that permissive parsing is desired.

if err != nil && (!asn1.AllowPermissiveParsing || !strings.HasPrefix(err.Error(), "permissive")) {

Though it isn't bad in theory, it breaks the API of AllowPermissiveParsing flag.

[zakird]

@codyprime I guess I'm thinking for something like Censys, we'd probably want to allow permissive parsing, always. But we would also want to know and mark in our data that it failed? Otherwise you'd have no idea if you're missing a field because it's not there or just because the record might be incomplete.

@elliotcubit Yeah, agree on checking permissive is gross and we probably want a different way to express it back to the caller.

[elliotcubit]

But we would also want to know and mark in our data that it failed

It's already the case that fields might be missing with no indication otherwise; when the permissive parsing flag allows a malformed cert, Censys doesn't mark it as incomplete, because zcrypto doesn't indicate that.

Even if an error were handed back here, Censys would just drop it.

[iJustErikk]

sooo do we want to break the api for AllowPermissiveParsing or do we want to give back errors?

only other choice I can think of is keeping ParseCertificate and having a separate ParseCertificateWithPermissiveErrors with the new behavior