Enable the "unparam" linter, which checks for unused function
parameters, unused function return values, and parameters and
return values that always have the same value every time they
are used.
In addition, fix many instances where the unparam linter complains
about our existing codebase. Remove error return values from a
number of functions that never return an error, remove or use
context and test parameters that were previously unused, and
simplify a number of (mostly test-only) functions that always take the
same value for their parameter. Most notably, remove the ability to
customize the RSA Public Exponent from the ceremony tooling,
since it should always be 65537 anyway.
Fixes#6104
Update:
- golangci-lint from v1.42.1 to v1.46.2
- protoc from v3.15.6 to v3.20.1
- protoc-gen-go from v1.26.0 to v1.28.0
- protoc-gen-go-grpc from v1.1.0 to v1.2.0
- fpm from v1.14.0 to v1.14.2
Also remove a reference to go1.17.9 from one last place.
This does result in updating all of our generated .pb.go files, but only
to update the version number embedded in each file's header.
Fixes#6123
Running an older version (v0.0.1-2020.1.4) of `staticcheck` in
whole-program mode (`staticcheck --unused.whole-program=true -- ./...`)
finds various instances of unused code which don't normally show up
as CI issues. I've used this to find and remove a large chunk of the
unused code, to pave the way for additional large deletions accompanying
the WFE1 removal.
Part of #5681
Create script which finds every .proto file in the repo and correctly
invokes `protoc` for each. Create a single file with a `//go:generate`
directive to invoke the new script. Delete all of the other generate.go
files, so that our proto generation is unified in one place.
Fixes#5453
protoc now generates grpc code in a separate file from protobuf code.
Also, grpc servers are now required to embed an "unimplemented"
interface from the generated .pb.go file, which provides forward
compatibility.
Update the generate.go files since the invocation for protoc has changed
with the split into .pb.org and _grpc.pb.go.
Fixes#5368
Update all of our tests to use `AssertMetricWithLabelsEquals`
instead of combinations of the older `CountFoo` helpers with
simple asserts. This coalesces all of our prometheus inspection
logic into a single function, allowing the deletion of four separate
helper functions.
The old `config.Common.CT.IntermediateBundleFilename` format is no
longer used in any production configs, and can be removed safely.
Part of #5162
Part of #5242Fixes#5269
Delete the PublisherClientWrapper and PublisherServerWrapper. Update
various structs and functions to expect a pubpb.PublisherClient instead
of a core.Publisher; these two interfaces differ only in that the
auto-generated PublisherClient takes a variadic CallOptions parameter.
Update all mock publishers in tests to match the new interface. Finally,
delete the now-unused core.Publisher interface and some already-unused
mock-generating code.
This deletes a single sanity check (for a nil SCT even when there is a
nil error), but that check was redundant with an identical check in the
only extant client code in ctpolicy.go.
Fixes#5323
A new, more realistic, test certificate hierarchy was added in #5273.
Update publisher tests to use the test certificate hierarchy now present
at test/hierarchy.
Fixes#5279
Publisher currently loads a PEM formatted certificate bundle from file
using LoadCertBundle a utility function in the core package.
LoadCertBundle parses the PEM file to a slice of x509.Certificates and
returns them to boulder-publisher (without checking validity). Using
these x509 Certificates, boulder-publisher to construct an ASN1Cert
bundle. This bundle is passed to each new publisher instance. When
publisher receives a request it unconditionally appends this bundle to
each end-entity precertificate for submission to CT logs.
This change augments this process to add support for multiple issuers
using the IssuerNameID concept in the Issuance package. Config field
Common.CT.CertificateBundleFilename has been replaced with the Chains
field. LoadChain, a utility function added in PR #5271, loads and
validates the chain (which nets us some added deploy-time safety) before
returning it to boulder-publisher. Using these x509 Certificates,
boulder-publisher constructs a mapping of IssuerNameID to ASN1Cert
bundle and passes this to each new publisher instance. When publisher
receives a request it determines the IssuerNameID of the precertificate
to select and append the correct ASN1Cert bundle for a given Issuer.
A followup issue #5269 has been created to address removal of the Common
field from the publisher configuration and code has been commented with
TODOs where code will need to be removed or refactored.
Fixes#1669
Our proto files had a variety of indentation styles: 2 spaces,
4 spaces, 8 spaces, and tabs; sometimes mixed within the same
file. The proto3 style guide[1] says to use 2-space indents,
so this change standardizes on that.
[1] https://developers.google.com/protocol-buffers/docs/style
And make corresponding changes to call sites and wrappers.
Note that proto2 vs proto3 is distinction in the syntax of the .proto files
and doesn't change the wire format, so this meets the deployability
guidelines.
There are some changes to the code generated in the latest version, so
this modifies every .pb.go file.
Also, the way protoc-gen-go decides where to put files has changed, so
each generate.go gets the --go_opt=paths=source_relative flag to
tell protoc to continue placing output next to the input.
Remove staticcheck from build.sh; we get it via golangci-lint now.
Pass --no-document to gem install fpm; this is recommended in the fpm docs.
In a handful of places I've nuked old stats which are not used in any alerts or dashboards as they either duplicate other stats or don't provide much insight/have never actually been used. If we feel like we need them again in the future it's trivial to add them back.
There aren't many dashboards that rely on old statsd style metrics, but a few will need to be updated when this change is deployed. There are also a few cases where prometheus labels have been changed from camel to snake case, dashboards that use these will also need to be updated. As far as I can tell no alerts are impacted by this change.
Fixes#4591.
SCT signature verification already happens within the CT client, so we are
currently doing it twice for no reason. This change removes the redundant check
we perform.
In Boulder Issue 3821[0] we found that HTTP/2 support was causing hard
to diagnose intermittent freezes in CT submission. Disabling HTTP/2 with
an environment variable resolved the freezes but is not a stable fix.
Per the Go `http` package docs we can make this change persistent by
changing the `http.Transport` config:
Programs that must disable HTTP/2 can do so by setting
Transport.TLSNextProto (for clients) or Server.TLSNextProto (for
servers) to a non-nil, empty map"
[0]: https://github.com/letsencrypt/boulder/issues/3821
Precursor to #4116. Since some of our dependencies impose a minimum
version on these two packages higher than what we have in Godeps, we'll
have to bump them anyhow. Bumping them independently of the modules
update should keep things a little simpler.
In order to get protobuf tests to pass, I had to update protoc-gen-go in
boulder-tools. Now we download a prebuilt binary instead of using the
Ubuntu package, which is stuck on 3.0.0. This also meant I needed to
re-generate our pb.go files, since the new version generates somewhat
different output.
This happens to change the tag for pbutil, but it's not a substantive change - they just added a tagged version where there was none.
$ go test github.com/miekg/dns/...
ok github.com/miekg/dns 4.675s
ok github.com/miekg/dns/dnsutil 0.003s
ok github.com/golang/protobuf/descriptor (cached)
ok github.com/golang/protobuf/jsonpb (cached)
? github.com/golang/protobuf/jsonpb/jsonpb_test_proto [no test files]
ok github.com/golang/protobuf/proto (cached)
? github.com/golang/protobuf/proto/proto3_proto [no test files]
? github.com/golang/protobuf/proto/test_proto [no test files]
ok github.com/golang/protobuf/protoc-gen-go (cached)
? github.com/golang/protobuf/protoc-gen-go/descriptor [no test files]
ok github.com/golang/protobuf/protoc-gen-go/generator (cached)
ok github.com/golang/protobuf/protoc-gen-go/generator/internal/remap (cached)
? github.com/golang/protobuf/protoc-gen-go/grpc [no test files]
? github.com/golang/protobuf/protoc-gen-go/plugin [no test files]
ok github.com/golang/protobuf/ptypes (cached)
? github.com/golang/protobuf/ptypes/any [no test files]
? github.com/golang/protobuf/ptypes/duration [no test files]
? github.com/golang/protobuf/ptypes/empty [no test files]
? github.com/golang/protobuf/ptypes/struct [no test files]
? github.com/golang/protobuf/ptypes/timestamp [no test files]
? github.com/golang/protobuf/ptypes/wrappers [no test files]
Now that Pebble has a `pebble-challtestsrv` we can remove the `challtestrv`
package and associated command from Boulder. I switched CI to use
`pebble-challtestsrv`. Notably this means that we have to add our expected mock
data using the HTTP management interface. The Boulder-tools images are
regenerated to include the `pebble-challtestsrv` command.
Using this approach also allows separating the TLS-ALPN-01 and HTTPS HTTP-01
challenges by binding each challenge type in the `pebble-challtestsrv` to
different interfaces both using the same VA
HTTPS port. Mock DNS directs the VA to the correct interface.
The load-generator command that was previously using the `challtestsrv` package
from Boulder is updated to use a vendored copy of the new
`github.org/letsencrypt/challtestsrv` package.
Vendored dependencies change in two ways:
1) Gomock is updated to the latest release (matching what the Bouldertools image
provides)
2) A couple of new subpackages in `golang.org/x/net/` are added by way of
transitive dependency through the challtestsrv package.
Unit tests are confirmed to pass for `gomock`:
```
~/go/src/github.com/golang/mock/gomock$ git log --pretty=format:'%h' -n 1
51421b9
~/go/src/github.com/golang/mock/gomock$ go test ./...
ok github.com/golang/mock/gomock 0.002s
? github.com/golang/mock/gomock/internal/mock_matcher [no test files]
```
For `/x/net` all tests pass except two `/x/net/icmp` `TestDiag.go` test cases
that we have agreed are OK to ignore.
Resolves https://github.com/letsencrypt/boulder/issues/3962 and
https://github.com/letsencrypt/boulder/issues/3951
Even though we create a different `http.Client` for each log, all those
clients have their `Transport` pointed at `http.DefaultTransport`, which
means they share a connection pool and the relevant locks.
We occasionally see a bug where a single publisher instance will hang on
all log submissions, while other publisher instances continue normally.
One possibility is that `http.Client` is blocking on some internal state
related to connection pooling. To rule that out, we provide a separate
`Transport` for each log.
This updates Boulder's vendored dependency for `github.com/google/certificate-transparency-go` to c25855a, the tip of master at the time of writing.
Unit tests are confirmed to pass:
```
$ git log --pretty=format:'%h' -n 1
c25855a
$ go test ./...
ok github.com/google/certificate-transparency-go (cached)
ok github.com/google/certificate-transparency-go/asn1 (cached)
ok github.com/google/certificate-transparency-go/client 22.985s
? github.com/google/certificate-transparency-go/client/configpb [no test files]
? github.com/google/certificate-transparency-go/client/ctclient [no test files]
ok github.com/google/certificate-transparency-go/ctpolicy (cached)
ok github.com/google/certificate-transparency-go/ctutil (cached)
? github.com/google/certificate-transparency-go/ctutil/sctcheck [no test files]
? github.com/google/certificate-transparency-go/ctutil/sctscan [no test files]
ok github.com/google/certificate-transparency-go/dnsclient (cached)
ok github.com/google/certificate-transparency-go/fixchain 0.091s
? github.com/google/certificate-transparency-go/fixchain/chainfix [no test files]
ok github.com/google/certificate-transparency-go/fixchain/ratelimiter (cached)
ok github.com/google/certificate-transparency-go/gossip (cached)
? github.com/google/certificate-transparency-go/gossip/gossip_server [no test files]
ok github.com/google/certificate-transparency-go/gossip/minimal 0.028s
? github.com/google/certificate-transparency-go/gossip/minimal/configpb [no test files]
? github.com/google/certificate-transparency-go/gossip/minimal/goshawk [no test files]
? github.com/google/certificate-transparency-go/gossip/minimal/gosmin [no test files]
ok github.com/google/certificate-transparency-go/gossip/minimal/x509ext (cached)
ok github.com/google/certificate-transparency-go/ingestor/ranges (cached)
ok github.com/google/certificate-transparency-go/jsonclient 0.007s
ok github.com/google/certificate-transparency-go/logid (cached)
ok github.com/google/certificate-transparency-go/loglist (cached)
? github.com/google/certificate-transparency-go/loglist/findlog [no test files]
ok github.com/google/certificate-transparency-go/loglist2 (cached)
? github.com/google/certificate-transparency-go/preload [no test files]
? github.com/google/certificate-transparency-go/preload/dumpscts [no test files]
? github.com/google/certificate-transparency-go/preload/preloader [no test files]
ok github.com/google/certificate-transparency-go/scanner 0.009s
? github.com/google/certificate-transparency-go/scanner/scanlog [no test files]
ok github.com/google/certificate-transparency-go/tls (cached)
ok github.com/google/certificate-transparency-go/trillian/ctfe (cached)
? github.com/google/certificate-transparency-go/trillian/ctfe/configpb [no test files]
? github.com/google/certificate-transparency-go/trillian/ctfe/ct_server [no test files]
? github.com/google/certificate-transparency-go/trillian/ctfe/testonly [no test files]
ok github.com/google/certificate-transparency-go/trillian/integration 0.023s
? github.com/google/certificate-transparency-go/trillian/integration/ct_hammer [no test files]
? github.com/google/certificate-transparency-go/trillian/migrillian [no test files]
? github.com/google/certificate-transparency-go/trillian/migrillian/configpb [no test files]
ok github.com/google/certificate-transparency-go/trillian/migrillian/core (cached)
? github.com/google/certificate-transparency-go/trillian/mockclient [no test files]
ok github.com/google/certificate-transparency-go/trillian/util (cached)
ok github.com/google/certificate-transparency-go/x509 (cached)
? github.com/google/certificate-transparency-go/x509/pkix [no test files]
? github.com/google/certificate-transparency-go/x509util [no test files]
? github.com/google/certificate-transparency-go/x509util/certcheck [no test files]
? github.com/google/certificate-transparency-go/x509util/crlcheck [no test files]
```
Resolves https://github.com/letsencrypt/boulder/issues/3872
**Note to reviewers**: There's an outstanding bug that I've tracked down to the `--load` stage of the integration tests that results in one of the remote VA instances in the `test/config-next` configuration under Go 1.11.1 to fail to cleanly shut down. I'm working on finding the root cause but in the meantime I've disabled `--load` during CI so we can unblock moving forward with getting Go 1.11.1 in dev/CI. Tracking this in https://github.com/letsencrypt/boulder/issues/3889
Does a simpler probe than compared to using a `blackbox_exporter`, but directly collects the info we think will aid debugging publisher outages.
Updates #3821.
Things removed:
* features.EmbedSCTs (and all the associated RA/CA/ocsp-updater code etc)
* ca.enablePrecertificateFlow (and all the associated RA/CA code)
* sa.AddSCTReceipt and sa.GetSCTReceipt RPCs
* publisher.SubmitToCT and publisher.SubmitToSingleCT RPCs
Fixes#3755.
A very large number of the logger calls are of the form log.Function(fmt.Sprintf(...)).
Rather than sprinkling fmt.Sprintf at every logger call site, provide formatting versions
of the logger functions and call these directly with the format and arguments.
While here remove some unnecessary trailing newlines and calls to String/Error.
Submits final certificates to any configured CT logs. This doesn't introduce a feature flag as it is config gated, any log we want to submit final certificates to needs to have it's log description updated to include the `"submitFinalCerts": true` field.
Fixes#3605.
Also instead of repeating the same bucket definitions everywhere just use a single top level var in the metrics package in order to discourage copy/pasting.
Fixes#3607.
In publisher and in the integration test, check that SCTs are in a
reasonable range. Also, update CreateTestingSignedSCT (used by
ct-test-srv) to produce SCTs correctly with a timetamp in Unix epoch
milliseconds.
This change updates boulder-tools to use Go 1.10, and references a
newly-pushed image built using that new config.
Since boulder-tools pulls in the latest Certbot master at the time of
build, this also pulls in the latest changes to Certbot's acme module,
which now supports ACME v2. This means we no longer have to check out
the special acme-v2-integration branch in our integration tests.
This also updates chisel2.py to reflect some of the API changes that
landed in the acme module as it was merged to master.
Since we don't need additional checkouts to get the ACMEv2-compatible
version of the acme module, we can include it in the default RUN set for
local tests.
With the CT policy changes, we cancel any outstanding requests in a
group once we've gotten a successful response from any log in that
group. That means various function calls will return early with an error
code indicating cancellation. We want to avoid logging such error codes,
because they were not really errors, they were intentional.
This change introduces a small utility package called "canceled", which
checks for both context.Canceled and the gRPC return code indicating
cancelled.
shell_test.go and publisher_test.go had unnecessary references to
../test/test-ca.pem. This change makes them a little more self-contained.
Note: ca/ca_test.go still depends on test-ca.pem, but removing the dependency
turns out to be a little more complicated due to hardcoded expectations in some
of the test cases.
Updates the buckets for histograms in the publisher, va, and expiration-mailer which are used to measure the latency of operations that go over the internet and therefore are liable to take a lot longer than the default buckets can measure. Uses a standard set of buckets for all three instead of attempting to tune for each one.
Fixes#3217.
Makes a couple of changes:
* Change `SubmitToCT` to make submissions to each log in parallel instead of in serial, this prevents a single slow log from eating up the majority of the deadline and causing submissions to other logs to fail
* Remove the 'submissionTimeout' field on the publisher since it is actually bounded by the gRPC timeout as is misleading
* Add a timeout to the CT clients internal HTTP client so that when log servers hang indefinitely we actually do retries instead of just using the entire submission deadline. Currently set at 2.5 minutes
Fixes#3218.
Aaron Gable