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

Pre-issuance certificate linting support. #1008

Closed
cpu opened this issue Jun 19, 2019 · 4 comments · Fixed by #1015
Closed

Pre-issuance certificate linting support. #1008

cpu opened this issue Jun 19, 2019 · 4 comments · Fixed by #1015

Comments

@cpu
Copy link
Contributor

cpu commented Jun 19, 2019

Hi folks,

At Let's Encrypt we would like to build optional pre-issuance certificate linting into CFSSL. Pre-issuance linting is repeatedly cited as a best-practice in incident post-mortems in the web PKI community. Adding support to CFSSL will help interested parties add more safe-guards to their issuance pipeline. I'd be happy to commit to doing the work provided the proposed design outlined below sounds good.

Boulder already uses zlint, a Go based X.509 Certificate Linter, as a library for post-issuance linting. I think it makes sense to integrate this project as a CFSSL dependency to accomplish pre-issuance linting.

High Level

At a high level we would like to:

  • Add a dependency on zlint
  • Update the config.SigningProfile struct to accept two new optional fields:
    1. lintErrLevel int
    2. ignoredLints []string
  • Update the signer.local.Signer struct to generate one throw-away private key at creation for linting.
  • Update the signer.local.Signer's sign(*x509.Certificate) function to:
    1. Sign the TBSCert template with the Signer's throw-away key.
    2. Pass the the resulting cert to zlint for linting.
    3. Handle any lint results above the lintErrLevel as errors (iff the lint is not in the ignoredLints list)

Adding a dep.

We think integrating zlint directly so it can work out-of-box with a config change rather than building in a callback for the same purpose and supplying the linting externally on a project-specific basis will help more folks benefit from the linting. zlint has a bias towards CA/B reqs and RFC 5280 but it is an extensible way to add additional lints and many of the existing lints vet best-practices that are broadly applicable.

If it helps you feel better about a new dependency I'm also a committer/maintainer of zlint so you would have a contact on that side of the fence from the get-go.

Config changes

The lintErrLevel will default to 0 for existing configs. We'll match this up with the zlint.LintResult.LintStatus in the local signer to know when to treat a result as an error. Any LintStatus > the lintErrLevel will be an error, except in the case where lintErrLevel is zero and linting is disabled.

  • 0 = zlint.lints.Reserved -> no linting will be done. no results considered. Existing CFSSL behaviour.
  • 1, 2 = zlint.lints.NA and zlint.lints.NE -> these don't really make sense in this context.
  • 3 = zlint.lints.Pass -> all lint results except pass will be considered errors.
  • 4 = zlint.lints.Notice -> all lint results except pass and notice will be considered errors.
  • 5 = zlint.lints.Warn -> all lint results except pass, notice and warning will be considered errors.
  • 6 = zlint.lints.Error -> all lint results except pass, notice, warning, and error will be considered errors (e.g. fatal errors only)
  • 7+ = zlint.lints.Fatal -> no errors will be treated as errors.

The ignoredLints map offers a more fine-grained way to ignore specific lints by name. E.g.:

ignoredLints: [
  "n_subject_common_name_included",
  "subject_contains_reserved_arpa_ip"
]

What do you folks think? CC @cbroglie

@cbroglie
Copy link
Contributor

Sounds like a great addition to me so long as the default behavior is unchanged. I have no issues with the zlint dependency. @lgarofalo, any objections on your end?

@lgarofalo
Copy link
Contributor

This is a good plan, no objections.

Thank you for writing this up @cpu

@cpu
Copy link
Contributor Author

cpu commented Jun 27, 2019

Great, thanks for the 👀 @cbroglie, @lgarofalo! I'll aim to have a pull request available for review in the next week or so.

@cpu
Copy link
Contributor Author

cpu commented Jul 1, 2019

I'll aim to have a pull request available for review in the next week or so.

👉 #1015

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 a pull request may close this issue.

3 participants