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

ci: remove vendor dir, Go 1.13.x -> 1.14.x, fix integration test data #432

Merged
merged 4 commits into from
May 13, 2020

Conversation

cpu
Copy link
Member

@cpu cpu commented May 13, 2020

chore: remove vendor dir.

Vendoring has lost favour compared to relying on the Go 1.13+ proxy/module checksum behaviour.

ci: go 1.13.x -> go1.14.x

Switch to the latest stable Go release stream. Also remove setting GO111MODULE and GOFLAGS in the makefile env. The former is already the default since Go 1.12.x and the latter isn't required because we removed the vendor dir.

ci: update expected integration test data for Go 1.14.

New lints without result cases in our integration test data (e.g. e_ext_nc_intersects_reserved_ip, e_mp_rsassa-pss_in_spki) are added with the expected set {}.

Four existing lints have their expected error result tallies updated:

  1. "e_sub_cert_locality_name_must_not_appear"
    old: fatals: 0 errs: 23 warns: 0 infos: 0
    new: fatals: 0 errs: 13 warns: 0 infos: 0

  2. "e_sub_cert_province_must_not_appear"
    old: fatals: 0 errs: 16 warns: 0 infos: 0
    new: fatals: 0 errs: 8 warns: 0 infos: 0

  3. "e_sub_cert_street_address_should_not_exist"
    old: fatals: 0 errs: 8 warns: 0 infos: 0
    new: fatals: 0 errs: 0 warns: 0 infos: 0

  4. "e_sub_cert_postal_code_must_not_appear"
    old: fatals: 0 errs: 8 warns: 0 infos: 0
    new: fatals: 0 errs: 0 warns: 0 infos: 0

These four lints previously returned an error result for certificates that had a Subject Organization/GivenName/Surname that were encoded as a BMPString. Go < 1.14.x's ASN.1 package did not support this encoding type (See golang/go#26690) and so the lints assumed the field was absent, resulting in a false positive. In Go 1.14.x+ the field is correctly decoded and the error result is no longer applicable.

As one concrete example consider the certificate with fingerprint 008082596e15c4a1092b4c1ca44fb7d600669116d15f8bf04730b77b5630f4e9 and the e_sub_cert_locality_name_must_not_appear lint. This certificate has a subject organization name encoded as a BMPSTRING, and all other subject fields encoded as PRINTABLESTRING.

With ZLint built with Go 1.13.x this results in the cert.Subject.Organization field being considered empty but the cert.Subject.Locality is not. The e_sub_cert_locality_name_must_not_appear lint thus finds a non-empty cert.Subject.Locality but no cert.Subject.Organization, cert.Subject.GivenName or cert.Subject.Surname and an error result result is (incorrectly) returned:

if len(c.Subject.Organization) == 0 && len(c.Subject.GivenName) == 0 && len(c.Subject.Surname) == 0 {
if len(c.Subject.Locality) > 0 {
return &lint.LintResult{Status: lint.Error}
}
}

With ZLint built with Go 1.14.x the cert.Subject.Organization field encoding is supported and so the decoded value is not empty. This in turn short-circuits the conditional in the e_sub_cert_locality_name_must_not_appear lint and a pass result is returned.

cpu added 3 commits May 13, 2020 16:09
Vendoring has lost favour compared to relying on the Go 1.13+
proxy/module checksum behaviour[0].

[0]: https://proxy.golang.org/
Also remove setting GO111MODULE and GOFLAGS. The former is already the
default since Go 1.12.x and the latter isn't required because we removed
the vendor dir.
New lints without result cases in our integration test data are added
with the expected set `{}`.

Four existing lints have their expected error result tallies updated:

1. "e_sub_cert_locality_name_must_not_appear"
  old: fatals: 0    errs: 23   warns: 0    infos: 0
  new: fatals: 0    errs: 13   warns: 0    infos: 0

2. "e_sub_cert_province_must_not_appear"
  old: fatals: 0    errs: 16   warns: 0    infos: 0
  new: fatals: 0    errs: 8    warns: 0    infos: 0

3. "e_sub_cert_street_address_should_not_exist"
  old: fatals: 0    errs: 8    warns: 0    infos: 0
  new: fatals: 0    errs: 0    warns: 0    infos: 0

4. "e_sub_cert_postal_code_must_not_appear"
  old: fatals: 0    errs: 8    warns: 0    infos: 0
  new: fatals: 0    errs: 0    warns: 0    infos: 0

These four lints previously returned an error result for certificates
that had a Subject Organization/GivenName/Surname that were encoded as
a BMPString. Go < 1.14.x's ASN.1 package did not support this encoding
type and so the lints assumed the field was absent, resulting in a false
positive. In Go 1.14.x+ the field is correctly decoded and the error
result is no longer applicable.
@cpu cpu self-assigned this May 13, 2020
@cpu cpu requested review from dadrian and zakird May 13, 2020 20:52
@cpu cpu merged commit a42b778 into zmap:master May 13, 2020
@cpu cpu deleted the cpu-go-1.14 branch May 13, 2020 21:01
@dadrian
Copy link
Member

dadrian commented May 13, 2020

In Go 1.14.x+ the field is correctly decoded and the error result is no longer applicable.

Always good to see results getting "more correct!". Do we have a lint that checks for BMPString encoding? I'm not sure what the BR's say about it, but it might be worth at least a notice.

@cpu
Copy link
Member Author

cpu commented May 13, 2020

Thanks for the quick 🔍's!

@cpu
Copy link
Member Author

cpu commented May 13, 2020

Do we have a lint that checks for BMPString encoding? I'm not sure what the BR's say about it, but it might be worth at least a notice.

I don't think there is one. Legacy BMPString encoding is mentioned as a SHOULD NOT in #368 within the context of DirectoryString encoding. I'm not sure off-hand if there are other contexts to lint for this string encoding.

@sleevi
Copy link
Contributor

sleevi commented May 13, 2020

@dadrian https://tools.ietf.org/html/rfc5280#section-4.1.2.6

When encoding attribute values of type DirectoryString, conforming
CAs MUST use PrintableString or UTF8String encoding, with the
following exceptions:

(a) applies to CA
(b) applies to CAs (CRL issuers)

  (c)  TeletexString, BMPString, and UniversalString are included
      for backward compatibility, and SHOULD NOT be used for
      certificates for new subjects.  However, these types MAY be
      used in certificates where the name was previously
      established, including cases in which a new certificate is
      being issued to an existing subject or a certificate is being
      issued to a new subject where the attributes being encoded
      have been previously established in certificates issued to
      other subjects.  Certificate users SHOULD be prepared to
      receive certificates with these types.

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.

4 participants