This enables the gosec linter. It also disables a number of
warnings which it emits on the current codebase. Some of these
(e.g. G104: Errors unhandled) we expect to leave disabled
permanently; others (e.g. G601: Implicit memory aliasing in for loop)
we expect to fix and then enable to prevent regressions.
Part of #4948
This mocks out the signer type rather than mocking out the pkcs11
object, making the test less dependent on the internals of our
pkcs11helpers package.
Part of refactorings related to #4918.
This is the only method on the ca which uses a non-proto
type as its request or response value. Changing this to
use a proto removes the last logic from the wrappers,
allowing them to be removed in a future CL. It also makes
the interface more uniform and easier to reason about.
Issue: #4940
Because these `wfe.sendError()` calls were not followed
by `return`s, the wfe was sending both them and the
next error encountered. In some cases, this could result
in the wrong HTTP response code being set, as that is
determined by the last error sent.
Previously we were relying on a "more" boolean returned from
FindObjects. But according to
https://pkg.go.dev/github.com/miekg/pkcs11?tab=doc#Ctx.FindObjects,
> The returned boolean value is deprecated and should be ignored.
Instead, we ask for more objects than we need and error if we get more
than 1.
Add a test, and in the process split up the relevant test into
multiple smaller test cases.
Introduces a new generic helper utility to check that
fields of proto messages are non-nil and non-zero.
Uses this helper to simplify the ca RPC wrapper
methods, moving their completeness checks into
the underlying method handler. Also annotates the
completeness checks to justify which fields are or
are not being checked for future readers. Finally,
removes the similar non-nil checks from the client
wrappers, where they provide no marginal value.
Follow-up changes will do the same for other RPC
services, migrate said services to proto3, and change
the IssueCertificateForPrecertificate method to return
a corepb.Certificate instead of a core.Certificate, like
the other methods on the ca service.
Issues: #4955
We previously used mixed case names for proto imports
(e.g. both `caPB` and `rapb`), sometimes in the same file.
This change standardizes on the all-lowercase spelling,
which was predominant throughout the codebase.
Previously, this limit was bucketed by hour, but that created too much
sudden traffic at the beginning of each hour as accounts' rate limits
expired. Chunking by the minute should make it possible to smooth out
traffic more.
This was necessary to work around a poor interaction between
Go 1.4.x and unpatched linux kernels. Although we are still using
the same version of Go, and the Linux project only released the
fix in kernel 5.4.2 and later, Ubuntu has backported the fix into
Focal Fossa 20.04's 5.4.0 kernel. Therefore this workaround is
no longer needed.
https://github.com/golang/go/issues/37436#issuecomment-657436406
This also removes one need for elevated permissions, making it
easier to use docker rootless for development.
Simplify database interactions
This change is a result of an audit of all places where
Go code directly constructs SQL queries and executes them
against a dbMap, with the goal of eliminating all instances
of constructing a well-known object type (such as a
core.CertificateStatus) from explicitly-listed database columns.
Instead, we should be relying on helper functions defined in the
sa itself to determine which columns are relevant for the
construction of any given object.
This audit did not find many places where this was occurring. It
did reveal a few simplifications, which are contained in this
change:
1) Greater use of existing SelectFoo methods provided by models.go
2) Streamlining of various SelectSingularFoo methods to always
select by serial string, rather than user-provided WHERE clause
3) One spot (in ocsp-responder) where using a well-known type seemed
better than using a more minimal custom type
Addresses #4899
This updates va.proto to use proto3 syntax, and updates
all clients of the autogenerated code to use the new types. In
particular, it removes indirection from built-in types (proto3
uses ints, rather than pointers to ints, for example).
Fixes#4956
We had some duplicated code related to opening the PKCS#11 session and
generating a signer, so I pulled it out into a separate function. This
function also takes an issuer so it can verify that the public key
matches what's expected.
This updates the ca.proto to use proto3 syntax, and updates
all clients of the autogenerated code to use the new types. In
particular, it removes indirection from built-in types (proto3
uses ints, rather than pointers to ints, for example).
It also updates a few instances where tests were being
conducted to see if various object fields were nil to instead
check for those fields' new zero-value.
Fixes#4940
Our PKCS11 config sections fall into two categories:
- Those used for generating keys, where the HSM is both an input and an
output, storing the resulting key in a specified slot.
- Those used for signing certs, where the HSM is an input.
This creates dedicated config structs for these two, reducing
duplication in defining overall config structs. This also makes
defining test cases involving these config structs much more concise.
Fixes#4915
When StoreIssuerInfo is enabled the CA loses its ability to verify that the certificate we are requesting an OCSP response for is real directly (previously we sent the cert DER and checked the signature on it). In order to prevent the ocsp-updater from sending a request for a serial that doesn't exist we added a check that the serial we were being asked to generate a response for did actually exist. This introduced a significant amount of database pressure as it requires a DB query for every single OCSP response we generate. It also provides a minimal level of security, we already trust the ocsp-updater and creating a response for a certificate that doesn't exist doesn't actually accomplish much (if the ocsp-updater was compromised the more realistic attack would be asking to generate a good response for a revoked certificate).
This change removes the check that the serial exists from the CA.
Fixes#4935.
The ocsp-updater no longer makes RPCs to the SA, but accesses the DB
directly (sometimes with help from SA functions to keep the DB
consistent).
This removes the SA connection config from config-next/. We'll remove
from config/ once the corresponding config has been removed from prod.
The `KeyPolicy.GoodKey` method is used to validate both public keys
used to sign JWK messages, and public keys contained inside CSR
messages.
According to RFC8555 section 6.7, validation failure in the former
case should result in `badPublicKey`, while validation failure in
the latter case should result in `badCSR`. In either case, a failure
due to reasons other than the key itself should result in
`serverInternal`.
However, the GoodKey method returns a variety of different errors
which are not all applicable depending on the context in which it is
called. In addition, the `csr.VerifyCSR` method passes these errors
through verbatim, resulting in ACME clients receiving confusing and
incorrect error message types.
This change causes the GoodKey method to always return either a
generic error or a KeyError. Calling methods should treat a `KeyError`
as either a `badPublicKey` or a `badCSR` depending on their context,
and may treat a generic error however they choose (though likely as a
serverInternal error).
Fixes#4930
The StoreKeyHashes feature flag controls whether rows are added to the
keyHashToSerial table. This feature is now enabled everywhere, so the
flag-protected code can be turned on unconditionally and the flag
removed from configs.
Related to #4895
Previously, if ocspchecker encountered a cert whose OCSP response
it didn't like, it would print the first reason it didn't like it and then
move on to the next cert provided on the command line. This
behavior both obscures the cert in question (by not printing
details of its OCSP response) and the error in question (by only
printing the first encountered error, instead of all errors).
This change causes ocspchecker to both print the details of the
OCSP response whether it likes the response or not, and to
accumulate all errors it encounters while validating that response.
This should increase interactive usability, as requested in #4901.
This changes the default behavior of the `-expectStatus` flag of
the ocspchecker and ocsp_forever binaries (as well as any other
consumer of the ocsp/helper.go library).
Previously, the flag had a default value of 0, meaning that all
OCSP responses were expected to have a status of "Good" (i.e. not
"Revoked"); having any other status is an error which would cause
the tool to print different information and have a non-zero exit code.
This flag now has a default value of -1, meaning that no enforcement
of the OCSP status should happen. All OCSP responses, whether Good,
Revoked, or otherwise, will simply be logged for informational
purposes and the tool will exit successfully.
This is a backwards-incompatible change, and users which rely on
the default behavior of this flag should update themselves.
The key-hash-backfill cmd was used to fill the keyHashToSerial table
when it was first created. Now that the table is up and running, the
backfill utility can be removed.
Fixes#4895
Previously this was logging a map of emails to unrevokedCertificates.
Since unrevokedCertificates includes DER, which is a []byte, this was
getting printed as a series of decimal numbers, one for each byte.
This adds a Stringer implementation for unrevokedCertificates that
omits the DER.
Fixes#4921.
This copies over a number of features flags and other settings from
test/config-next that have been applied in prod.
Also, remove the config-next gate on various tests.
Jacob Hoffman-Andrews
Aaron Gable
milgradesec
Phil Porada
Roland Bracewell Shoemaker
orangepizza