Adding support for linting profiles #595

christopher-henderson · 2021-05-01T17:19:10Z

What is a Profile

A profile derives from the conversation in #583 wherein we would like to have curated lists of lints that, collectively, refer to a particular requirement from, say, the CABF.

The solution put forth in this PR is that a profile is essentially an alias for a comma separated list of lints submitted via -includeNames, however with some extra metadata attached (such as where did this profile come from).

Profile Implementation

A profile does not appear to have any behavior of it's own. Rather, it is simply a declaration of lints that are logically grouped together by an external requirement. As such, we can borrow infrastructure from our lints that is similar, however much simpler.

I believe that something as simple as the following should suffice.

package profiles

import "github.com/zmap/zlint/v3/lint"

func init() {
	lint.RegisterProfile(lint.Profile{
		Name:        "my_new_profile",
		Description: "In general, CAs should do all the things correctly and not do the wrong things",
		Citation:    "That one post I saw on Reddit said that this list was legit",
		Source:      lint.MyTrustWorthyUncle,
		LintNames: []string{
			"e_ca_common_name_missing",
			"e_ca_key_usage_missing",
		},
	})
}

I'm not sure if profiles should have a CheckApplies? One would think that is inherited from individual lints aggregated within the profile.

Running a Profile

Let's say that the_profile_in_question is comprised of the lints a, b, c, d.

./zlint -profile=the_profile_in_question

Is the equivalent to running

./zlint -includeNames="a,b,c,d"

A new CLI flag has been added that lists each profile as line-delimited JSON (much in the same way one prints out available lints).

$ ./zlint list-profiles
{"name":"whatever","lints":["e_ca_common_name_missing","e_ca_key_usage_missing"]}
{"name":"yooodaw","description":"Fill this in...","citation":"Fill this in...","source":"Unknown","lints":[]}
{"name":"yooodaws","description":"Fill this in...","citation":"Fill this in...","source":"Unknown","lints":[]}

Adding a New Profile

The convenience script newProfile.sh <profile_name> has been added that acts in much the same way as newLint.sh does.

Documentation has been added to CONTRIBUTING.md

(resolves #595 )

christopher-henderson · 2021-05-01T17:50:49Z

README.md

@@ -97,6 +97,9 @@ Example ZLint CLI usage:
 	echo "Lint mycert.pem with all of the lints except for ETSI ESI sourced lints"
 	zlint -excludeSources=ETSI_ESI mycert.pem

+	echo "List available lint profiles. A profile is a pre-defined collection of lints."


I was apprehensive of adding an example of running a profile here because don't actually have any yet. That is, I didn't want to give an example that doesn't actually run.

christopher-henderson · 2021-05-01T17:52:24Z

v3/cmd/zlint/main.go

@@ -32,11 +32,14 @@ import (
 	"github.com/zmap/zlint/v3"
 	"github.com/zmap/zlint/v3/formattedoutput"
 	"github.com/zmap/zlint/v3/lint"
+
+	_ "github.com/zmap/zlint/v3/profiles"


We need to trigger the init functions in /profiles so that we can build our FilterOptions appropriately. If we try to add this import into lint then it causes an import cycle between lint and profiles 😕

christopher-henderson · 2021-05-01T17:54:25Z

v3/cmd/zlint/main.go

@@ -192,10 +207,17 @@ func trimmedList(raw string) []string {
 // use.
 func setLints() (lint.Registry, error) {
 	// If there's no filter options set, use the global registry as-is
-	if nameFilter == "" && includeNames == "" && excludeNames == "" && includeSources == "" && excludeSources == "" {


Just a style change because this boolean condition was getting long.

Nice, looks good :-)

christopher-henderson · 2021-05-01T17:56:08Z

v3/profiles/todo.go

@@ -0,0 +1,5 @@
+package profiles


Yeah, sorry, if you have a package with nothing in it but test files then golangci-lint vomits uncontrollably. We'll have to delete this once our first profile lands.

cpu

Thanks @christopher-henderson. This seems like a nice extension. I left a few small comments.

Do you have thoughts on the first profile to include? There are a couple points in the PR where you work around not having a profile (e.g. w/ todo.go, and not including a README example). I also had a question about whether we needed a couple model fields. I think maybe we could address both of these things by including a commit in this branch that introduces a profile if there's a good first candidate. What do you think?

v3/lint/profile.go

cpu · 2021-05-10T22:06:31Z

v3/lint/profile.go

+	// The source of the check, e.g. "BRs: 6.1.6" or "RFC 5280: 4.1.2.6".
+	Citation string `json:"citation,omitempty"`
+
+	// Programmatic source of the check, BRs, RFC5280, or ZLint
+	Source LintSource `json:"source,omitempty"`


I'm wondering whether it's acceptable to consider a Profile's citations/sources as the aggregate of the citations/sources of the lints indicated in LintNames. If so maybe we can simplify the model a little bit and drop these two fields.

If you think there is a use-case for having profile-level citations/sources I think the comments need to be adjusted slightly. "source of the check" reads like there's a singular "check" which (IMO) conflicts with the multiple LintNames.

(Sorry, this is likely an artifact that I hadn't bothered to aggregate the discussion in #583 into #594)

So these profiles aren't arbitrary groupings. The plan is that they do come a from source requirement (the first impetus of which is sleevi/cabforum-docs#36). So the idea is that a parent source lists a collection of requirements with an explanation as to why they are grouped.

CABF/BR x.y.z Section A: Due to whatever reason <requirement A>, <requirement B>, <requirement C> from RFRC5280 should...

{ Citation: "Due to whatever reason, these requirements from RFC5280 should...", Source: lint.CABFBR_X_Y_Y }

"source of the check" reads like there's a singular "check"

Effectively there is, in that if one sub-check fails then the whole check fails, and thus you have failed the profile from the cited source. A bit like how a boolean expression can comprised of multiple boolean expressions (and named).

// In Chris' humble opinion the following is what is considered // altogether a nice day. func isANiceDay(sunny, cool, butNotTooCool, lightbreeze, bool) { return sunny && cool && butNotTooCool && lightbreeze }

Each individual one may-or-may-not be desirable in their own context. However, grouped together they answer the single question, "By Chris' definition is this a nice day?"

....although that was my take on it. Since @sleevi is the first source of these, does it make sense for there to be a simple inheritance type scheme where we can just say that a profile's citations is the list of its lints' citations?

cpu · 2021-05-10T22:09:08Z

v3/lint/profile.go

+	return profile, ok
+}
+
+func AllProfiles() []Profile {


nit: can you add a godoc comment on this exported func too? 🙇

v3/newProfile.sh

Absolutely Co-authored-by: Daniel McCarney <daniel@binaryparadox.net>

christopher-henderson · 2021-05-15T17:43:04Z

@cpu

Do you have thoughts on the first profile to include?

The first candidates come from @sleevi's work at sleevi/cabforum-docs#36.

Keep this branch around until concrete profiles are published.
- It would be nice to have something ready on day zero, not entirely necessary from what I can see.
Merge empty and add profiles once they're published.
- Awkward, like you have pointed out. Plus the framework may need tweeks once we have something concrete to work with

I'm somewhat partial to #1 as I think it makes sense to have code ready to go, but not necessarily have dead code in master.

christopher-henderson added 2 commits May 1, 2021 10:18

adding support for linting profiles

3e11191

at least tests running

4c0bbb7

christopher-henderson commented May 1, 2021

View reviewed changes

christopher-henderson requested review from cpu, sleevi and zakird May 1, 2021 17:56

cpu self-assigned this May 10, 2021

cpu requested changes May 10, 2021

View reviewed changes

cpu mentioned this pull request May 14, 2021

ISSUE: Time Stamping certificates with false warning results #602

Open

christopher-henderson and others added 2 commits May 15, 2021 10:08

Update v3/lint/profile.go

316b30a

Absolutely Co-authored-by: Daniel McCarney <daniel@binaryparadox.net>

Update v3/newProfile.sh

b5de4eb

adding godoc to AllProfiles

76e78e0

christopher-henderson requested a review from cpu May 18, 2021 17:51

cpu unassigned cpu Feb 17, 2022

sandorszoke mentioned this pull request Apr 14, 2022

ISSUE: lint "e_key_usage_and_extended_key_usage_inconsistent" in technically constrained CA certificates #593

Open

web-flow and others added 7 commits October 6, 2022 19:22

util: gtld_map autopull updates for 2022-10-06T19:22:06 UTC

b67e984

Trigger GHA

a3a2cc0

Merge branch 'zlint-gtld-update' of github.com:zmap/zlint

f4fe968

Merge branch 'master' of github.com:zmap/zlint

8e996c0

Merge branch 'master' of github.com:zmap/zlint

d3cd218

Merge branch 'master' into profiles

f62997a

fixing linter

749e1b9

christopher-henderson merged commit 6292ca4 into master Oct 16, 2022

christopher-henderson deleted the profiles branch October 16, 2022 18:41

robplee mentioned this pull request Nov 9, 2023

Address inconsistencies between RFC5280 and CABF BRs/X.520 #762

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for linting profiles #595

Adding support for linting profiles #595

christopher-henderson commented May 1, 2021 •

edited

Loading

christopher-henderson May 1, 2021

christopher-henderson May 1, 2021

christopher-henderson May 1, 2021

cpu May 10, 2021

christopher-henderson May 1, 2021

cpu left a comment

cpu May 10, 2021

christopher-henderson May 15, 2021

christopher-henderson May 15, 2021

cpu May 10, 2021

christopher-henderson commented May 15, 2021

Adding support for linting profiles #595

Adding support for linting profiles #595

Conversation

christopher-henderson commented May 1, 2021 • edited Loading

What is a Profile

Profile Implementation

Running a Profile

Adding a New Profile

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christopher-henderson commented May 15, 2021

christopher-henderson commented May 1, 2021 •

edited

Loading