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

Support for Configurable Lints #648

Merged
merged 30 commits into from
Jul 23, 2022
Merged

Support for Configurable Lints #648

merged 30 commits into from
Jul 23, 2022

Conversation

christopher-henderson
Copy link
Member

@christopher-henderson christopher-henderson commented Nov 28, 2021

Howdy folks,

The following is an implementation of the notion of configurable lints that we have been working on.

First of all, there two fortunate attributes to this code review...

  1. All of the documentation written up in RFC for Configurable Lints #624 still apply, so if there are any questions on how this behaves or is used, then that document is still accurate.
  2. The wide majority of this PR are tests and docs (over 75%).

However, it still may be conducive to provide a walkthrough (or, at least, give an ordering of the file that should be read carefully). I'll endeavor to at least provide said ordering.

  1. v3/lint/configuration.go - This is the most involved file in the PR. Most others are plumbing.
  2. v3/lint/global_configurations.go - This is a list of the pre-defined configuration sections that ZLint will ship with. Of course they are all empty at the moment, but I think it still bares taking a look at.
  3. CONTRIBUTING.md - Docs on how to work with this system from a lint are important.

The rest are miscellaneous plumbing required to actually hookup the feature end-to-end as well as enabling configurations from within unit tests where desired.

This PR resolves #624

@@ -47,6 +47,8 @@ var ( // flags
includeSources string
excludeSources string
printVersion bool
config string
Copy link
Member Author

@christopher-henderson christopher-henderson Jan 8, 2022

Choose a reason for hiding this comment

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

The changes in main are to:

  1. Add the new flags that either point to a configuration file are prints an example default one to the terminal.
  2. If printing the default example is selected, then that is done and the problem exists. Any other flag is ignored. This behavior is consistent with printing a list of all available lints as well as printing ZLint's version.
  3. Loading the (optional) configuration file before selecting what lints to run.

return c.resolveHigherScopedReferences(target)
}

// resolveHigherScopeReferences takes in an interface{} value and attempts to
Copy link
Member Author

@christopher-henderson christopher-henderson Jan 9, 2022

Choose a reason for hiding this comment

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

Indeed, this method is what accomplishedsthe somewhat fancy scoping feature by reflecting into the struct provided by the lint and trying to find and data member that is one of the higher scoped configuration fields.

@@ -0,0 +1,913 @@
package lint
Copy link
Member Author

Choose a reason for hiding this comment

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

These tests require custom structs for each case that we would like to exercise. That, coupled with the fact the types are not values in Go, means that table driven testing cannot reasonably be done (at least, I don't immediate see a way - please speak up if you can think of one).

@christopher-henderson
Copy link
Member Author

Howdy folks,

Between the log4j shenanigans and the holidays I myself took December largely off from the repo, however I'm hoping to push this feature over the finish line because I think it can really unlock a good number of doors for us with regard to ZLint's power and accuracy.

I do appreciate that 1.7k additions is a bit daunting, but I hope that knowing that more than 50% of that is just unit tests that are forced to be a bit redundant can help ease the apprehension.

Please do not hesitate if you would like more from me before attempting to tackle this review.

@cpu @zakird @sleevi

@zakird
Copy link
Member

zakird commented Jan 14, 2022

I'm just beyond underwater this month, but can take a pass on February 5th.

@cpu
Copy link
Member

cpu commented Jan 14, 2022

I'm taking a break from volunteer open source work for the short term and won't be available to review this PR.

@cpu cpu removed their request for review January 14, 2022 15:42
README.md Outdated Show resolved Hide resolved
v3/cmd/zlint/main.go Outdated Show resolved Hide resolved
v3/lint/base.go Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Co-authored-by: Zakir Durumeric <zakird@gmail.com>
Co-authored-by: Zakir Durumeric <zakird@gmail.com>
Copy link
Contributor

@aarongable aarongable left a comment

Choose a reason for hiding this comment

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

Just dropping by to say that I'm very excited for this.

The use-case I have in mind right now is writing a lint which checks that the SCTs embedded in a cert abide by the new Chrome "two different operators" CT Policy. Doing that lint properly requires having a list of which logs are run by which operators, which means loading https://www.gstatic.com/ct/log_list/v3/log_list.json from disk, which means having a config which points to that file on disk.

README.md Outdated Show resolved Hide resolved
v3/lint/configuration.go Outdated Show resolved Hide resolved
v3/lint/configuration.go Show resolved Hide resolved
v3/lint/global_configurations.go Show resolved Hide resolved
v3/lint/registration.go Show resolved Hide resolved
v3/lint/registration.go Outdated Show resolved Hide resolved
v3/lint/registration.go Outdated Show resolved Hide resolved
v3/test/configuration_test_framework_test.go Outdated Show resolved Hide resolved
v3/test/helpers.go Outdated Show resolved Hide resolved
@christopher-henderson
Copy link
Member Author

Thank you @zakird and @aarongable for your patience - other low hanging fruit kept presenting itself and it was just too tempting to go for the small wins too often.

@aarongable thank you so much for hopping in here without even being asked to! It is very much appreciated to receive community interest and support, and I have been very interested in receiving feedback from consumers on the usability if this framework as well as candidate usecases.

@zakird I looked into how this would all work from the perspective of a library usage and thankfully not much had to be done beyond a bug fix, some extra tests, and the following documentation.

From the README

Library Usage

...
...
...

To lint a certificate in the presence of a particular configuration file, you must first construct the configuration and then make a call to SetConfiguration in the Registry interface.

A Configuration may be constructed using any of the following functions:

  • lint.NewConfig(r io.Reader) (Configuration, error)
  • lint.NewConfigFromFile(config string) (Configuration, error)
  • lint.NewConfigFromString(config string) (Configuration, error)

The contents of the input to all three constructors must be a valid TOML document.

import (
	"github.com/zmap/zcrypto/x509"
	"github.com/zmap/zlint/v3"
)

var certDER []byte = ...
parsed, err := x509.ParseCertificate(certDER)
if err != nil {
	// If x509.ParseCertificate fails, the certificate is too broken to lint.
	// This should be treated as ZLint rejecting the certificate
	log.Fatal("unable to parse certificate:", err)
}
configuration, err := lint.NewConfigFromString(`
        [some_configurable_lint]
        IsWebPki = true
        NumIterations = 42
        
        [some_configurable_lint.AnySubMapping]
        something = "else"
        anything = "at all"
`)
if err != nil {
	log.Fatal("unable to parse configuration:", err)
}
lint.GlobalRegistry().SetConfigutration(configuration)
zlintResultSet := zlint.LintCertificate(parsed)

Copy link
Contributor

@aarongable aarongable left a comment

Choose a reason for hiding this comment

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

All the core logic code looks good to me, just a few nits around the documentation and naming!

v3/cmd/zlint/main.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
v3/cmd/zlint/main.go Outdated Show resolved Hide resolved
v3/test/helpers.go Outdated Show resolved Hide resolved
@christopher-henderson christopher-henderson removed the request for review from sleevi July 17, 2022 22:08
@christopher-henderson
Copy link
Member Author

Howdy @aarongable, could you take one more perfunctory glance at this after I addresses some of the nits (I honestly have no idea why GH won't let me cycle your review status...). I think we're just about ready to get this change in in preparation for a minor version bump 😄

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.

RFC for Configurable Lints
4 participants