validity check changes

[bryanpitcher]

Hi, thanks for providing the community with your excellent linting tool - a lot of people have benefited from this.

Please consider merging this pull request. There are a couple commits I've made to improve validity checking.

1. The first is to correct a typo on a variable comparison that resulted in the validity checks always passing previously. Now the comparisons are for NotAfter > (NotBefore + offset)

2. The second is to take into account recent BR and EV guidelines on allowed validity duration. This was a little tricky to determine all the rules based on different issuance dates that reflected different CAB guidance over time. I've reviewed the guidelines and relevant ballot and I think the updated checks have it covered. Specifically:

• Ballot 193 was adopted into BR 1.4.4 with an effective date of 2018-03-01. The adopted guidance states: "Subscriber Certificates issued after 1 March 2018 MUST have a Validity Period no greater than 825 days. Subscriber Certificates issued after 1 July 2016 but prior to 1 March 2018 MUST have a Validity Period no greater than 39 months." Certs issued prior to 2016-07-01 are allowed to live up to 60 months.
• Ballot 193 was adopted into EV Guidelines 1.6.2 effective 2017-03-17. This allows EV SSL certs to live no longer than 825 days. Prior to this, EV SSL certs were allowed to live 27 months. I debated simplifying the logic to just allow 825 regardless of issuance date since 825 days is so close to 27 months. In the end I decided that the more precise checking depending on date of issuance might be valuable.

[bryanpitcher]

Whoops, it looks that the tests aren't passing due to a need to update a few of the golden files. I've added a commit to correct that, but the tests seem to still be using the old versions. When I run this locally on my machine it's passing for me now:
go test -race -coverprofile=coverage.txt -covermode=atomic

Any ideas?

[codecov]

Codecov Report

Merging #20 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #20   +/-   ##
=======================================
  Coverage   33.81%   33.81%           
=======================================
  Files           1        1           
  Lines         278      278           
=======================================
  Hits           94       94           
  Misses        169      169           
  Partials       15       15

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8aaa8c...d33269e. Read the comment docs.

[bryanpitcher]

I've had some more time to look into why travisci tests are failing by setting up and experimenting with my own travisci environment. The test cases are working now (See https://travis-ci.org/bryanpitcher/certlint/builds/469648573 ), here is what I had to do:

1. Change namespaces for local package imports - otherwise travisci script call to go get -t -v ./... fails with: checks/certificate/all/all.go:9:2: use of internal package not allowed

I was surprised to see that go doesn't seem to have imports relative to the local repo, which causes trouble for forked repos. However, I'm relatively new to go, and so maybe there is a better way to achieve this testing for forked repos without having to switch the full package name out. I suppose the other option would be to rename the internal test directory to something else so that it's not flagged as an internal package. Note: I've only switched the package name out on my 'travisci' branch so that my travisci tests would pass. I haven't switched it out on master.

I suspect this is also why the tests appear to be using the old golden files (it's using them from the globalsign repo vs my forked repo).

2. The go:1.8 build was failing because of this error during the go get -t -v ./...
   ../../google/certificate-transparency-go/client/logclient.go:60: syntax error: unexpected = in type declaration

This code uses a new-style typedef first introduced in go 1.9. I suspect it may be time to remove support for 1.8 given that dependencies are starting to do so. So, I've updated the .travis.yml to remove the 1.8 test and to add a newer go:1.11 test

3. The go:master build was failing because of what appears to be an error in go vet, or possibly an unintended use. When go vet ./... is run using go:master, the following error is emitted:
   checks/certificate/serialnumber/serialnumber.go:25:3: Err format %d has arg big.NewInt(1) of wrong type *math/big.Int

In the go source repo, the function matchArgTypeInternal in src/cmd/vendor/golang.org/x/tools/go/analysis/passes/printf/types.go attempts to determine if the input type is a formatter by calling the isFormatter function defined in cmd/vendor/golang.org/x/tools/go/analysis/passes/printf/printf.go. That function requires that the fmt package has been imported in the current context of the source file being vetted in order to return true. So, one workaround is to import fmt in checks/certificate/serialnumber/serialnumber.go. In order to prevent an error regarding an unused package, I've used it once in the source file to Sprintf a String.

[tadukurow]

Hi @bryanpitcher thanks very much for your contribution. All looks good to me.
I've had a look at the failing tests and it was caused by some caching on travis. Which also explains your clean build passing. I've fixed the issue in #22, so if you could update this PR, I'll merge it.

[bryanpitcher]

@tadukurow Thanks, I was starting to wonder if this project would be maintained going forward and I'm happy to see it is.

I've merged in your updates in #22 to this pull request. I re-wrote history on my forked repo to make the commit sequence a bit cleaner since we had both edited the travisci config, and it should be good for a merge at this point.

[tadukurow]

@bryanpitcher thanks for the updates.

Actually this repo will be archived shortly, but wanted to get your PR in before doing so.
This project is currently not used internally and there are no immediate plans to do so.
We do however encourage anyone who wants to take it forward to fork.