Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add lints checking for underscores in labels pre-and-post CABF 1.6.2 #656

Closed
wants to merge 1 commit into from

Conversation

christopher-henderson
Copy link
Member

@christopher-henderson christopher-henderson commented Jan 30, 2022

In the interest of moving #646 over the finish line (@CBonnell because I don't think I can add you as a reviewer), I believe that it would be prudent to add two lints regarding the presence of underscores in DNS lables.


Ballot SC12 sunsetted the permissibility of underscores in DNS names on April 1st, 2019 via CABF 1.6.2.

The language is as follows:

Prior to April 1, 2019, certificates containing underscore characters (“_”) in domain labels in dNSName entries MAY be issued as follows:

  • dNSName entries MAY include underscore characters such that replacing all underscore characters with hyphen characters (“-“) would result in a valid domain label, and;
  • Underscore characters MUST NOT be placed in the left most domain label, and;
  • Such certificates MUST NOT be valid for longer than 30 days.

This change adds two lints:

  1. One which allows for underscores, albeit with a WARN and only if the above clauses apply. Else the underscore is an error.
    1.a. This lint is ineffective after April 1st, 2019.
  2. One which is disallows underscores altogether.
    2.a: This lint is effectivve after April 1st, 2019.

Integration Test Failures

There are a large number of integration test failures (over 300) and a smoke check is showing that they are likely valid in that these certificates are valid for longer than the required thirty days. I will do a deep dive on these test failure, however.

@christopher-henderson christopher-henderson changed the title adding lint for underscores Add lints checking for underscores in labels pre-and-post CABF 1.6.2 Jan 30, 2022
Comment on lines +52 to +58
fqdnPortion := util.RemovePrependedWildcard(dns)
labels := strings.Split(fqdnPortion, ".")
for _, label := range labels {
if strings.Contains(label, "_") {
return &lint.LintResult{Status: lint.Error, Details: fmt.Sprintf("dNSName ('%s') MUST NOT contain an underscore character ('_')", dns)}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need to loop over labels here, or even remove the wildcard?

Since your error message reflects the full FQDN (dns), couldn't this just be

if strings.Contains(dns, "_") {
  return ...
}

"* Such certificates MUST NOT be valid for longer than 30 days.",
Citation: "BR 7.1.4.2.1",
Source: lint.CABFBaselineRequirements,
EffectiveDate: util.ZeroDate,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is correct?

That's what the discussion on the other issue was trying to capture:

  • From Zero Date to present, the IETF specifications forbid underscores
  • From Zero Date to 2018-12-10 (aka SC12), the IETF/RFC 5280 rules applied
  • From 2018-12-10 until 2019-04-01 T00:00:00Z, underscores followed BRs only where the LDH rule allowed them. Specifically:
    • no leading underscores, because of the [A-Za-z] rule in 1034 and the [A-Za-Z0-9] modifications in 1123
    • no underscores in the third-and-fourth character because with hyphens they would be reserved
    • no trailing underscore because that'd be a trailing hyphen, which would violate the <let-dig> termination of <label> from 1034

So basically, the IETF rules always apply, but if we're applying BR rules, then we've got three separate lint periods.

for _, label := range labels {
containsUnderscore := strings.Contains(label, "_")
wouldBeValidIfHyphenInstead := util.IsLDHLabel(strings.ReplaceAll(label, "_", "-"))
if containsUnderscore && wouldBeValidIfHyphenInstead && !validLongerThanThirtyDays {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this linting separate things? I thought we encouraged these as separate lints.

Namely, you're testing both the 30 day rule and the LDH-valid rule, but I think we'd have these separate?

Broken out, I think this is four BR lints plus one IETF lint:

  • BR lint
    • No underscore: Effective date is Zero date, Ineffective date is 2018-12-10 (which is the real 1.6.2 date)
    • 30-days-or-less when underscore present: Effective date is 2018-12-10, Ineffective date is 2019-04-01 (underscore sunset date)
    • Valid LDH-when-replaced: Effective date is 2018-12-10. Ineffective date is 2019-04-01.
    • No underscore. Effective date is 2019-04-01. No ineffective date
  • IETF lint
    • No underscore. Effective date is Zero date.

WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, especially since I appear to have gotten some core aspects incorrect attempting to bundle these together.

I have fired up the following in the same order as you proposed:

#659 (No underscores are allowed in DNSNames before BR 1.6.2's permissibility period)
#660 (Permit underscores in DNSNames if-and-only-if those certificates are valid for less than 30 days and during BR 1.6.2's permissibility period)
#661 (Permit underscores in DNSNames if-and-only-if replacing all underscores results in valid LDH labels during BR 1.6.2's permissibility period)
#662 (No underscores are allowed in DNSNames after BR 1.6.2's permissibility period)
#663 (RFC lint to disallow all underscores in DNSNames)

I'll keep this PR open for the moment for tracking sake, but I plan to close it as it is superseded by the above three.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good deal. I realize some of the feedback propagates through these changes, thanks for dealing with it :)

@dadrian
Copy link
Member

dadrian commented Sep 27, 2023

#646 is now closed.

@dadrian dadrian closed this Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants