boulder icon indicating copy to clipboard operation
boulder copied to clipboard

Add pkilint to CI via custom zlint

Open aarongable opened this issue 1 year ago • 5 comments

Add a new "LintConfig" item to the CA's config, which can point to a zlint configuration toml file. This allows lints to be configured, e.g. to control the number of rounds of factorization performed by the Fermat factorization lint.

Leverage this new config to create a new custom zlint which calls out to a configured pkilint API endpoint. In config-next integration tests, configure the lint to point at a new pkilint docker container.

This approach has three nice forward-looking features: we now have the ability to configure any of our lints; it's easy to expand this mechanism to lint CRLs when the pkilint API has support for that; and it's easy to enable this new lint if we decide to stand up a pkilint container in our production environment.

No production configuration changes are necessary at this time.

Fixes https://github.com/letsencrypt/boulder/issues/7430

aarongable avatar Apr 18 '24 23:04 aarongable

This PR is currently a draft because, although it works, pkilint has several complaints about the certificates we issue during our integration tests:

{
  "results": [
    {
      "validator": "DvSubcriberAttributeAllowanceValidator",
      "node_path": "certificate.tbsCertificate.subject.rdnSequence",
      "finding_descriptions": [
        {
          "severity": "WARNING",
          "code": "cabf.serverauth.dv.common_name_attribute_present",
          "message": null
        }
      ]
    },
    {
      "validator": "SubjectKeyIdentifierValidator",
      "node_path": "certificate.tbsCertificate.extensions.3.extnValue.subjectKeyIdentifier",
      "finding_descriptions": [
        {
          "severity": "INFO",
          "code": "pkix.subject_key_identifier_rfc7093_method_1_identified",
          "message": null
        }
      ]
    },
    {
      "validator": "GeneralNameUriInternalDomainNameValidator",
      "node_path": "certificate.tbsCertificate.extensions.5.extnValue.authorityInfoAccessSyntax.0.accessLocation.uniformResourceIdentifier",
      "finding_descriptions": [
        {
          "severity": "ERROR",
          "code": "cabf.internal_domain_name",
          "message": "Internal domain name: \"127.0.0.1\""
        }
      ]
    },
    {
      "validator": "GeneralNameUriSyntaxValidator",
      "node_path": "certificate.tbsCertificate.extensions.5.extnValue.authorityInfoAccessSyntax.1.accessLocation.uniformResourceIdentifier",
      "finding_descriptions": [
        {
          "severity": "ERROR",
          "code": "pkix.invalid_uri_syntax",
          "message": "Invalid URI syntax: \"http://127.0.0.1:4502/int ecdsa a\""
        }
      ]
    },
    {
      "validator": "GeneralNameUriInternalDomainNameValidator",
      "node_path": "certificate.tbsCertificate.extensions.5.extnValue.authorityInfoAccessSyntax.1.accessLocation.uniformResourceIdentifier",
      "finding_descriptions": [
        {
          "severity": "ERROR",
          "code": "cabf.internal_domain_name",
          "message": "Internal domain name: \"127.0.0.1\""
        }
      ]
    },
    {
      "validator": "SubscriberExtensionAllowanceValidator",
      "node_path": "certificate.tbsCertificate.extensions",
      "finding_descriptions": [
        {
          "severity": "WARNING",
          "code": "cabf.serverauth.subscriber.subject_key_identifier_extension_present",
          "message": null
        }
      ]
    }
  ],
  "linter": {
    "name": "DV-PRE-CERTIFICATE"
  }
}

The INFO level result can be ignored, it's just saying which method we use to compute the Key Identifier. The two WARNING-level results can also be ignored; we include the Subject Key Identifier and Common Name in our end-entity certs on purpose, though maybe we should move away from doing so...

The three ERROR level results are because our integration test certificates have obviously-fake values for their AIA Issuer, AIA OCSP, and CRLDP URLs. I'll look into fixing those so that we don't need to disable those lints entirely.

aarongable avatar Apr 18 '24 23:04 aarongable

@aarongable, this PR appears to contain configuration and/or SQL schema changes. Please ensure that a corresponding deployment ticket has been filed with the new values.

github-actions[bot] avatar Apr 23 '24 23:04 github-actions[bot]

@aarongable, this PR appears to contain configuration and/or SQL schema changes. Please ensure that a corresponding deployment ticket has been filed with the new values.

github-actions[bot] avatar Apr 23 '24 23:04 github-actions[bot]

Thanks @aarongable, this is really great.

-Tim

timfromdigicert avatar Apr 25 '24 13:04 timfromdigicert