Correct lint attribution for dnsname_etc lints and limit scope to just DNS SAN entries #609
Conversation
| @@ -303,7 +303,7 @@ | |||
| }, | |||
| "e_dnsname_contains_bare_iana_suffix": {}, | |||
| "e_dnsname_empty_label": { | |||
| "ErrCount": 11 | |||
| "ErrCount": 10 | |||
There was a problem hiding this comment.
The certificate in question here is https://crt.sh/?q=c5e0cc80ea898c2a9e2d09b0960df895535017eac46050c02458d16c6f6841d5 which failed because it's CommonName of MONICOMP Kereskedelmi és Szolgáltató Zrt. would get split on ., resulting in an "empty label" on the right hand side of the ..
|
@christopher-henderson Yeah, this is a bit complex. Checking the CN can be argued intentional (for serverAuth certs), because the CN will be used if SAN is absent. This is, effectively, the issue https://datatracker.ietf.org/doc/html/draft-ietf-uta-use-san is trying to solve. Rather than remove the CN check, did you give any consideration to checking it, but only for serverAuth? That is, taking the logic that was in |
|
@sleevi would it make sense to restrict the fallback even further and only check the CN if the cert is serverAuth and the SAN is empty? |
Maybe? Given that we're still seeing incidents where CAs omit CN from the SAN (e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=1716123 filed a few hours ago), I think it might make sense to check both. The context for "CN containing a domain name" mostly derives from RFC 2818; RFC 6125 clarified some further, but both left it ambiguous whether CN was a domain name if SAN was present (but, implied, it heavily is, because of backwards compat with non-SAN supporting systems). If it was a CA/B Forum lint, unconditionally checking CN is 100% the right thing to do - because CABF unambiguously says CN is DNS. However, even though it's not directly stated in 2818, it's heavily suggested as such, so I'd be game to continue to check both. If you did want to only check DNS here, while moving the lints to |
A bit unfortunate in the sense of code duplication, but I'm amenable to leaving the CABF lints where they are an introducing some new less, DNSName only, RFC lints (and have done so in the PR) @robplee just a heads up that I have fired off this PR to try to iterate on this a bit faster and get it in. |
|
This is going to need to get migrated to constructors. |
|
@sleevi just so that I recall what we were hemming-and-hawing about here - we were thinking about in-place moving these lints from CABF into RFC. However, we identified that CABF still would like do its own, similar checks, so this change got...changed...so simply copying the bulk of the extant CABF lints into RFC. So I think this one is GTG if that is the case. |
|
@sleevi gentle tap in that I think this PR is good to go and can be in an RC2 release, but I didn't want to pull the trigger without at least one green check mark. |
|
This change is append only and appears to conform to the consensus of the conversation, so in the interest of forward movement and getting this into RC2 I am going to force merge this. |
There was a problem hiding this comment.
Hey @christopher-henderson not sure how I missed this! Sorry for the radio silence on the pings, I need to figure out what's up with my mail filters and GitHub. Definitely feel free to drop a note directly if you're looking for feedback.
This looks good to me, but I wasn't sure with the SLD/TRD split, or the avoidance of the ABNF-defined restrictions. This is not a big deal, though, so happy to see this has landed :)
| func init() { | ||
| lint.RegisterLint(&lint.Lint{ | ||
| Name: "e_rfc_dnsname_hyphen_in_sld", | ||
| Description: "DNSName should not have a hyphen beginning or ending the SLD", |
There was a problem hiding this comment.
To confirm - the reason for limiting this to the second-level domain is the assumption that domain owners don't pay attention to the relevant RFCs and stick all sorts of stuff here (e.g. underscores, leading/trailing -), and since name resolution libraries don't fast-fail, ZLint doesn't bother? :)
(A very valid reason, but just confirming if that was the case)
| return &lint.LintResult{Status: lint.NA} | ||
| } | ||
| if strings.HasPrefix(parsedSANDNSNames[i].ParsedDomain.SLD, "-") || | ||
| strings.HasSuffix(parsedSANDNSNames[i].ParsedDomain.SLD, "-") { |
There was a problem hiding this comment.
It could make sense to document more of the citation why, but that's neither here nor there.
I believe the relevant language here is from RFC 1034 and relaxed by RFC 1123, namely, that the ABNF for a host in the "preferred name syntax" is
<domain> ::= <subdomain> | " "
<subdomain> ::= <label> | <subdomain> "." <label>
<label> ::= <letter> [ [ <ldh-str> ] <let-dig> ]
<ldh-str> ::= <let-dig-hyp> | <let-dig-hyp> <ldh-str>
<let-dig-hyp> ::= <let-dig> | "-"
<let-dig> ::= <letter> | <digit>
which in 1034 terms, indicates a <label> begins with <letter> and ends with let-dig. RFC 1123, Section 2.1, relaxes this, to become
<label> ::= <let-dig> [ [ <ldh-str> ] <let-dig> ]
by virtue of the following text:
The syntax of a legal Internet host name was specified in RFC-952
[DNS:4]. One aspect of host name syntax is hereby changed: the
restriction on the first character is relaxed to allow either a
letter or a digit. Host software MUST support this more liberal
syntax.
Note that 1123 references 952, which contained the following compatible-with-1034 ABNF
<hname> ::= <name>*["."<name>]
<name> ::= <let>[*[<let-or-digit-or-hyphen>]<let-or-digit>]
This is essentially a fork of #587 which resolves #538
There were a large number of discrepancies with the outcome of the corpus test caused by changing the lint source, which suddenly subjected many more certificates to linting since the following conditional was no longer applying
zlint/v3/lint/base.go
Line 100 in 7e75dc3
However, these CABF lints were also checking the CommonName of certificates to see if they followed rules that RFC 5280 4.1.2.6 lays out for SANs only. I believe that the correct thing to do here is to simply not check the CommonName in these lints.