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

Misc. cleanups, unit test for finding leftover template bits. #340

Merged
merged 13 commits into from
Jan 3, 2020

Conversation

cpu
Copy link
Member

@cpu cpu commented Jan 3, 2020

As promised here are some misc cleanups and a new unit test inspired by my review of #337. I don't think any of these are especially controversial changes. I did my best to break them up by commit, messages for each included here:

tests: remove gofmt_test.go

The golangci-lint pass run in CI includes an equivalent test. If folks want to test for unformatted code locally install the linter and run golangci-lint run in the root directory. This will flag findings above and beyond gofmt problems ahead of CI failing.

lints: remove commented out code.

In three cases, remove a comment ahead of a return that added no useful context.

In lints/community/lint_rsa_exp_negative_test.go remove a commented out test case for a negative RSA exponent. The test code doesn't build as-is and the referenced test cert (rsaExpNegative.pem) doesn't exist in-tree. A TODO is left to indicate there's missing test coverage for later follow-up.

lints: fix "certtificate" comment typo.

Does what it says on the tin.

lints: fix tabs in ref text for lint_sub_cert_or_sub_ca_using_sha1.

Some BR text was copied into a comment with \t characters between every word making it hard to read.

lints: fix field name ref. in lint Descriptions.

The lints/community/lint_ian_wildcard_not_first.go and lints/rfc/lint_ian_bare_wildcard.go lints mistakenly said in their Description that they only check the DNSNames field of the certificate when in fact they only check the IANDNSNames field. There are two corresponding lints (lints/community/lint_san_wildcard_not_first.go and lints/community/lint_san_bare_wildcard.go) that check DNSNames.

lints: add slice of known LintSources, test for templating leftovers.

There should never be finished lint source code that contains template text intended to be replaced by the programmer. A new TestLeftoverTemplates unit test is added to make sure we enforce this during CI to lessen the burden on code reviewers to catch this problem.

lints: fix TestLeftoverTemplates findings

Prior to these fixes all of the modified files had templating leftovers:

=== RUN   TestLeftoverTemplates
--- FAIL: TestLeftoverTemplates (0.01s)
    template_test.go:49: Lint "cabf_br/lint_root_ca_extended_key_usage_present.go" contains template leftover "// Add actual lint here"
    template_test.go:49: Lint "cabf_br/lint_root_ca_key_usage_present.go" contains template leftover "// Add actual lint here"
    template_test.go:49: Lint "cabf_br/lint_sub_cert_cert_policy_empty.go" contains template leftover "// Add actual lint here"
    template_test.go:49: Lint "cabf_br/lint_sub_cert_certificate_policies_missing.go" contains template leftover "// Add actual lint here"
    template_test.go:49: Lint "cabf_br/lint_sub_cert_crl_distribution_points_does_not_contain_url.go" contains template leftover "// Add actual lint here"
    template_test.go:49: Lint "cabf_br/lint_sub_cert_eku_extra_values.go" contains template leftover "// Add actual lint here"
    template_test.go:49: Lint "cabf_br/lint_sub_cert_eku_missing.go" contains template leftover "// Add actual lint here"
    template_test.go:49: Lint "cabf_br/lint_sub_cert_eku_server_auth_client_auth_missing.go" contains template leftover "// Add actual lint here"
    template_test.go:49: Lint "cabf_br/lint_sub_cert_key_usage_cert_sign_bit_set.go" contains template leftover "// Add actual lint here"
    template_test.go:49: Lint "cabf_br/lint_sub_cert_key_usage_crl_sign_bit_set.go" contains template leftover "// Add actual lint here"
    template_test.go:49: Lint "rfc/lint_basic_constraints_not_critical.go" contains template leftover "// Add actual lint here"
    template_test.go:49: Lint "rfc/lint_ext_key_usage_not_critical.go" contains template leftover "// Add actual lint here"
    template_test.go:49: Lint "rfc/lint_basic_constraints_not_critical.go" contains template leftover "// Add actual lint here"
    template_test.go:49: Lint "rfc/lint_ext_key_usage_not_critical.go" contains template leftover "// Add actual lint here"
    template_test.go:49: Lint "rfc/lint_basic_constraints_not_critical.go" contains template leftover "// Add actual lint here"
    template_test.go:49: Lint "rfc/lint_ext_key_usage_not_critical.go" contains template leftover "// Add actual lint here"
FAIL
FAIL	command-line-arguments	0.017s
FAIL

lints: update template test with another string, fix occurrences.

Adding another template string to the TestLeftoverTemplates shook out some more lints to clean up:

=== RUN   TestLeftoverTemplates
--- FAIL: TestLeftoverTemplates (0.01s)
    template_test.go:50: Lint "cabf_br/lint_sub_ca_name_constraints_not_critical.go" contains template leftover "Change this to match source TEXT"
    template_test.go:50: Lint "community/lint_validity_time_not_positive.go" contains template leftover "Change this to match source TEXT"
    template_test.go:50: Lint "community/lint_validity_time_not_positive.go" contains template leftover "Change this to match source TEXT"
FAIL
FAIL	command-line-arguments	0.017s
FAIL

lints: move lint_ian_bare_wildcard.go from RFC to community.

It cites RFC 5280 but that RFC doesn't prescribe any semantics to the use of wildcards in DNSNames or elsewhere. I suspect this lint actually came from AWSLabs, similar to lint_ian_wildcard_not_first.go and lint_san_bare_wildcard.go, both of which are already in lints/community/.

lints: fix off-by-one's in RFC max length lint Descs.

The upper bounds being enforced against in the changed lints are inclusive. The lint tests were doing the right thing but the descriptions incorrectly described the boundary as if it were exclusive.

For comparison the following lints already did the right thing already and had the UB+1 in the desc:

lints/rfc/lint_subject_given_name_max_length.go
lints/rfc/lint_subject_postal_code_max_length.go
lints/rfc/lint_subject_street_address_max_length.go
lints/rfc/lint_subject_surname_max_length.go

Daniel added 13 commits January 3, 2020 09:40
The golangci-lint pass run in CI includes an equivalent test. If folks
want to test for unformatted code locally install the linter and run
`golangci-lint run` in the root directory. This will flag findings
above and beyond `gofmt` problems ahead of CI failing.
In three cases, remove a comment ahead of a return that added no
useful context.

In `lints/community/lint_rsa_exp_negative_test.go` remove a commented
out test case for a negative RSA exponent. The test code doesn't build
as-is and the referenced test cert (`rsaExpNegative.pem`) doesn't exist
in-tree. A TODO is left to indicate there's missing test coverage for
later follow-up.
These two lints mistakenly said in their `Description` that they only
check the `DNSNames` field of the certificate when in fact they only
check the `IANDNSNames` field. There are two corresponding lints
(`lints/community/lint_san_wildcard_not_first.go` and
`lints/community/lint_san_bare_wildcard.go`) that check `DNSNames`.
There should never be finished lint source code that contains template
text intended to be replaced by the programmer. A new
`TestLeftoverTemplates` unit test is added to make sure we enforce this
during CI to lessen the burden on code reviewers to catch this problem.
Prior to these fixes all of the modified files had templating leftovers:
```
=== RUN   TestLeftoverTemplates
--- FAIL: TestLeftoverTemplates (0.01s)
    template_test.go:49: Lint "cabf_br/lint_root_ca_extended_key_usage_present.go" contains template leftover "// Add actual lint here"
    template_test.go:49: Lint "cabf_br/lint_root_ca_key_usage_present.go" contains template leftover "// Add actual lint here"
    template_test.go:49: Lint "cabf_br/lint_sub_cert_cert_policy_empty.go" contains template leftover "// Add actual lint here"
    template_test.go:49: Lint "cabf_br/lint_sub_cert_certificate_policies_missing.go" contains template leftover "// Add actual lint here"
    template_test.go:49: Lint "cabf_br/lint_sub_cert_crl_distribution_points_does_not_contain_url.go" contains template leftover "// Add actual lint here"
    template_test.go:49: Lint "cabf_br/lint_sub_cert_eku_extra_values.go" contains template leftover "// Add actual lint here"
    template_test.go:49: Lint "cabf_br/lint_sub_cert_eku_missing.go" contains template leftover "// Add actual lint here"
    template_test.go:49: Lint "cabf_br/lint_sub_cert_eku_server_auth_client_auth_missing.go" contains template leftover "// Add actual lint here"
    template_test.go:49: Lint "cabf_br/lint_sub_cert_key_usage_cert_sign_bit_set.go" contains template leftover "// Add actual lint here"
    template_test.go:49: Lint "cabf_br/lint_sub_cert_key_usage_crl_sign_bit_set.go" contains template leftover "// Add actual lint here"
    template_test.go:49: Lint "rfc/lint_basic_constraints_not_critical.go" contains template leftover "// Add actual lint here"
    template_test.go:49: Lint "rfc/lint_ext_key_usage_not_critical.go" contains template leftover "// Add actual lint here"
    template_test.go:49: Lint "rfc/lint_basic_constraints_not_critical.go" contains template leftover "// Add actual lint here"
    template_test.go:49: Lint "rfc/lint_ext_key_usage_not_critical.go" contains template leftover "// Add actual lint here"
    template_test.go:49: Lint "rfc/lint_basic_constraints_not_critical.go" contains template leftover "// Add actual lint here"
    template_test.go:49: Lint "rfc/lint_ext_key_usage_not_critical.go" contains template leftover "// Add actual lint here"
FAIL
FAIL	command-line-arguments	0.017s
FAIL
```
```
=== RUN   TestLeftoverTemplates
--- FAIL: TestLeftoverTemplates (0.01s)
    template_test.go:50: Lint "cabf_br/lint_sub_ca_name_constraints_not_critical.go" contains template leftover "Change this to match source TEXT"
    template_test.go:50: Lint "community/lint_validity_time_not_positive.go" contains template leftover "Change this to match source TEXT"
    template_test.go:50: Lint "community/lint_validity_time_not_positive.go" contains template leftover "Change this to match source TEXT"
FAIL
FAIL	command-line-arguments	0.017s
FAIL
```
It cites RFC 5280 but that RFC doesn't prescribe any semantics to the
use of wildcards in DNSNames or elsewhere. I suspect this lint actually
came from AWSLabs, similar to `lint_ian_wildcard_not_first.go` and
`lint_san_bare_wildcard.go`, both of which are already in
`lints/community/`.
The upper bounds being enforced against in the changed lints are
inclusive. The lint tests were doing the right thing but the
descriptions incorrectly described the boundary as if it were exclusive.

For comparison the following lints already did the right thing already
and had the UB+1 in the desc:
```
lints/rfc/lint_subject_given_name_max_length.go
lints/rfc/lint_subject_postal_code_max_length.go
lints/rfc/lint_subject_street_address_max_length.go
lints/rfc/lint_subject_surname_max_length.go
```
@cpu cpu requested a review from zakird January 3, 2020 16:32
@cpu cpu self-assigned this Jan 3, 2020
@zakird zakird merged commit 86bcc67 into zmap:master Jan 3, 2020
@cpu cpu deleted the cpu-post-review-tidy branch January 3, 2020 18:09
@cardonator
Copy link
Contributor

Thanks for this work @cpu!

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