This version includes a fix that seems relevant to us:
> The HTTP/1 client did not fully validate the contents of the Host
header. A maliciously crafted Host header could inject additional
headers or entire requests. The HTTP/1 client now refuses to send
requests containing an invalid Request.Host or Request.URL.Host value.
>
> Thanks to Bartek Nowotarski for reporting this issue.
>
> Includes security fixes for CVE-2023-29406 and Go issue
https://go.dev/issue/60374
Previously, we had three chained calls initializing a database:
- InitWrappedDb calls NewDbMap
- NewDbMap calls NewDbMapFromConfig
Since all three are exporetd, this left me wondering when to call one vs
the others.
It turns out that NewDbMap is only called from tests, so I renamed it to
DBMapForTest to make that clear.
NewDbMapFromConfig is only called internally to the SA, so I made it
unexported it as newDbMapFromMysqlConfig.
Also, I copied the ParseDSN call into InitWrappedDb, so it doesn't need
to call DBMapForTest. Now InitWrappedDb and DBMapForTest both
independently call newDbMapFromMysqlConfig.
I also noticed that InitDBMetrics was only called internally so I
unexported it.
As a follow-up to #6780, add the same style of implementation test to
all of our other gRPC services. This was not included in that PR just to
keep it small and single-purpose.
* Perform Akamai OCSP cache purges on already revoked certificates in
`ra.RevokeCertByKey` and `ra.AdministrativelyRevokeCertificate` for
subsequent client calls to `/acme/revoke-cert`.
* Add `addToBlockedKeys` helper function used by
`ra.AdministrativelyRevokeCertificate` and `ra.RevokeCertByKey` to
de-duplicate code adding subject public key info hash to the
`blockedKeys` table.
* Add a duplicate revocation test to
`ra.TestAdministrativelyRevokeCertificate`
* Fix some grammar in an RA comment.
* Fixes https://github.com/letsencrypt/boulder/issues/4862
In particular, document that it does not purge the Akamai cache.
Also, in the RA, avoid creating a "fake" certificate object containing
only the serial. Instead, use req.Serial directly in most places. This
uncovered some incorrect logic. Fix that logic by gating the operations
that actually need a full *x509.Certificate: revoking by key, and
purging the Akamai cache.
Also, make `req.Serial` mandatory for AdministrativelyRevokeCertificate.
This is a reopen of #6693, which accidentally got merged into a
different feature branch.
Removes all code related to the `ReuseValidAuthz` feature flag. The
Boulder default is to now always reuse valid authorizations.
Fixes a panic in `test.AssertErrorIs` when `err` is unexpectedly `nil`
that was found this while reworking the
`TestPerformValidationAlreadyValid` test. The go stdlib `func Is`[1]
does not check for this.
1. https://go.dev/src/errors/wrap.go
Part 2/2, fixes https://github.com/letsencrypt/boulder/issues/2734
We rely on the ratelimit/ package in CI to validate our ratelimit
configurations. However, because that package relies on cmd/ just for
cmd.ConfigDuration, many additional dependencies get pulled in.
This refactors just that struct to a separate config package. This was
done using Goland's automatic refactoring tooling, which also organized
a few imports while it was touching them, keeping standard library,
internal and external dependencies grouped.
Add the "AsyncFinalize" feature flag. When enabled, this causes the RA
to return almost immediately from FinalizeOrder requests, with the
actual hard work of issuing the precertificate, getting SCTs, issuing
the final certificate, and updating the database accordingly all
occuring in a background goroutine while the client polls the GetOrder
endpoint waiting for the result.
This is implemented by factoring out the majority of the finalization
work into a new `issueCertificateOuter` helper function, and simply
using the new flag to determine whether we call that helper in a
goroutine or not. This makes removing the feature flag in the future
trivially easy.
Also add a new prometheus metric named `inflight_finalizes` which can be
used to count the number of simultaneous goroutines which are performing
finalization work. This metric is exported regardless of the state of
the AsyncFinalize flag, so that we can observe any changes to this
metric when the flag is flipped.
Fixes#6575
Remove the GenerateOCSP and GenerateCRL methods from the
CertificateAuthority gRPC service. These methods are no longer called by
any clients; all clients use their respective OCSPGenerator and
CRLGenerator gRPC services instead.
In addition, remove the CRLGeneratorServer field from the caImpl, as it
no longer needs it to serve as a backing implementation for the
GenerateCRL pass-through method. Unfortunately, we can't remove the
OCSPGeneratorServer field until after ROCSPStage7 is complete, and the
CA is no longer generating an OCSP response during initial certificate
issuance.
Part of #6448
Add a new time.Duration field, LagFactor, to both the SA's config struct
and the read-only SA's implementation struct. In the GetRegistration,
GetOrder, and GetAuthorization2 methods, if the database select returned
a NoRows error and a lagFactor duration is configured, then sleep for
lagFactor seconds and retry the select.
This allows us to compensate for the replication lag between our primary
write database and our read-only replica databases. Sometimes clients
will fire requests in rapid succession (such as creating a new order,
then immediately querying the authorizations associated with that
order), and the subsequent requests will fail because they are directed
to read replicas which are lagging behind the primary. Adding this
simple sleep-and-retry will let us mitigate many of these failures,
without adding too much complexity.
Fixes#6593
- Log counts of Authzs where CAA was rechecked/reused.
- Move the CAA recheck duration to a single variable in the RA.
- Add new method `InfoObject` to our logger.
Fixes#6560
Part of #6623
`ReuseValidAuthz` was introduced
here [1] and enabled in staging and production configs on 2016-07-13.
There was a brief stint during the TLS-SNI-01 challenge type removal where
SRE disabled it. However, time has finally come to remove this configuration
option. Issue #6623 will determine the feasibility of shorter authz
lifetimes and potentially the removal of authz reuse.
This change is broken up into two parts to allow SRE to safely remove
the flag from staging and production configs. We'll merge this PR, SRE
will deploy boulder and the config change, then we'll finish removing
`ReuseValidAuthz` configuration from the codebase.
[1] boulder commit 9abc212448
Part 1 of 2 for fixing #2734.
Simplify the control flow of the FinalizeOrder handler to make it easier
to read and reason about:
- Move all validation to before we set the order to Processing, and put
it all in a single helper funcion.
- Move almost all logEvent/trace handling directly into FinalizeOrder so
it cannot be missed.
- Flatten issueCertificate and issueCertificateInner into a single
helper function, now that they're no longer being called from both
ACMEv1 and v2 entry points.
- Other minor cleanups, such as making SolvedBy not return a pointer and
making matchesCSR private.
This paves the way for making both issueCertificateInner and failOrder
asynchronous, which we plan to do in the near future.
Part of #6575
The core.CertificateRequest wrapper type only had two fields: an
x509.CertificateRequest, and a slice of bytes representing the unparsed
version of the same request. However, x509.CertificateRequest carries
around its own field .Raw, which serves the same purpose. Also, we
weren't even referencing the .Bytes field anyway. Therefore, get rid of
this redundant wrapper type.
This also makes it clearer just how far the original CSR makes it
through our system. I'd like a future change to remove the CSR from the
request to the CA service, replacing it with a structured object that
only exposes exactly the fields of the CSR we care about, such as names,
public key, and whether or not the must-staple extension should be set.
Delete the NewOrder and NewAuthorizations2 methods from the SA's gRPC
interface. These methods have been replaced by the unified
NewOrderAndAuthzs method, which performs both sets of insertions in a
single transaction.
Also update the SA and RA unittests to not rely on these methods for
setting up test data that other functions-under-test rely on. In most
cases, replace calls to NewOrder with calls to NewOrderAndAuthzs. In the
SA tests specifically, replace calls to NewAuthorizations2 with a
streamlined helper function that simply does the single necessary
database insert.
Fixes#6510Fixes#5816
Deprecate these feature flags, which are consistently set in both prod
and staging and which we do not expect to change the value of ever
again:
- AllowReRevocation
- AllowV1Registration
- CheckFailedAuthorizationsFirst
- FasterNewOrdersRateLimit
- GetAuthzReadOnly
- GetAuthzUseIndex
- MozRevocationReasons
- RejectDuplicateCSRExtensions
- RestrictRSAKeySizes
- SHA1CSRs
Move each feature flag to the "deprecated" section of features.go.
Remove all references to these feature flags from Boulder application
code, and make the code they were guarding the only path. Deduplicate
tests which were testing both the feature-enabled and feature-disabled
code paths. Remove the flags from all config-next JSON configs (but
leave them in config ones until they're fully deleted, not just
deprecated). Finally, replace a few testdata CSRs used in CA tests,
because they had SHA1WithRSAEncryption signatures that are now rejected.
Fixes#5171Fixes#6476
Part of #5997
Create a new gRPC service named StorageAuthorityReadOnly which only
exposes a read-only subset of the existing StorageAuthority service's
methods.
Implement this by splitting the existing SA in half, and having the
read-write half embed and wrap an instance of the read-only half.
Unfortunately, many of our tests use exported read-write methods as part
of their test setup, so the tests are all being performed against the
read-write struct, but they are exercising the same code as the
read-only implementation exposes.
Expose this new service at the SA on the same port as the existing
service, but with (in config-next) different sets of allowed clients. In
the future, read-only clients will be removed from the read-write
service's set of allowed clients.
Part of #6454
Have the database query return timestamps for when certificates
were issued for certain names. Use that information to compute
when the next time that name will be eligible for issuance again.
Include that timestamp in the error message and a Retry-After
HTTP header.
Fixes#6465
When the RA is generating OCSP (as part of new issuance, revocation,
or when its own GenerateOCSP method is called by the ocsp-responder)
have it use the CA's dedicated OCSPGenerator service, rather than
calling the method exposed by the CA's catch-all CertificateAuthority
service. To facilitate this, add a new GRPCClientConfig stanza to the
RA.
This change will allow us to remove the GenerateOCSP and GenerateCRL
methods from the catch-all CertificateAuthority service, allowing us to
independently control which kinds of objects the CA is willing to sign
by turning off individual service interfaces. The RA's new config stanza
will need to be populated in prod before further changes are possible.
Fixes#6451
- Move incidents tables from `boulder_sa` to `incidents_sa` (added in #6344)
- Grant read perms for all tables in `incidents_sa`
- Modify unit tests to account for new schema and grants
- Add database cleaning func for `boulder_sa`
- Adjust cleanup funcs to omit `sql-migrate` tables instead of `goose`
Resolves#6328
Right now, Boulder expects to be able to connect to syslog, and panics
if it's not available. We'd like to be able to log to stdout/stderr as a
replacement for syslog.
- Add a detailed timestamp (down to microseconds, same as we collect in
prod via syslog).
- Remove the escape codes for colorizing output.
- Report the severity level numerically rather than with a letter prefix.
Add locking for stdout/stderr and syslog logs. Neither the [syslog] package
nor the [os] package document concurrency-safety, and the Go rule is: if
it's not documented to be concurrent-safe, it's not. Notably the [log.Logger]
package is documented to be concurrent-safe, and a look at its implementation
shows it uses a Mutex internally.
Remove places that use the singleton `blog.Get()`, and instead pass through
a logger from main in all the places that need it.
[syslog]: https://pkg.go.dev/log/syslog
[os]: https://pkg.go.dev/os
[log.Logger]: https://pkg.go.dev/log#Logger
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
The iotuil package has been deprecated since go1.16; the various
functions it provided now exist in the os and io packages. Replace all
instances of ioutil with either io or os, as appropriate.
The Fragment field of a parsed URL is only non-empty if there is
text following the octothorpe character. Check for the case that
the mailto: address ends in an octothorpe with no trailing value.
Fixes#6231
- Modify `ra.checkCertificatesPerFQDNSetLimit()` to use a leaky bucket algorithm
- Return issuance timestamps from `sa.FQDNSetTimestampsForWindow()` in descending order
Resolves#6154
Add a new `GenerateOCSP` gRPC method to the RA, which
passes the request through to the CA and returns the resulting
OCSP response bytes. This method does not store the new
response anywhere, it is up to the client (in this case intended to
be the new ocsp-responder's live-signing code path) to store it
in any/all appropriate locations.
Fixes#6189
Add a new code path to the ctpolicy package which enforces Chrome's new
CT Policy, which requires that SCTs come from logs run by two different
operators, rather than one Google and one non-Google log. To achieve
this, invert the "race" logic: rather than assuming we always have two
groups, and racing the logs within each group against each other, we now
race the various groups against each other, and pick just one arbitrary
log from each group to attempt submission to.
Ensure that the new code path does the right thing by adding a new zlint
which checks that the two SCTs embedded in a certificate come from logs
run by different operators. To support this lint, which needs to have a
canonical mapping from logs to their operators, import the Chrome CT Log
List JSON Schema and autogenerate Go structs from it so that we can
parse a real CT Log List. Also add flags to all services which run these
lints (the CA and cert-checker) to let them load a CT Log List from disk
and provide it to the lint.
Finally, since we now have the ability to load a CT Log List file
anyway, use this capability to simplify configuration of the RA. Rather
than listing all of the details for each log we're willing to submit to,
simply list the names (technically, Descriptions) of each log, and look
up the rest of the details from the log list file.
To support this change, SRE will need to deploy log list files (the real
Chrome log list for prod, and a custom log list for staging) and then
update the configuration of the RA, CA, and cert-checker. Once that
transition is complete, the deletion TODOs left behind by this change
will be able to be completed, removing the old RA configuration and old
ctpolicy race logic.
Part of #5938
These new linters are almost all part of golangci-lint's collection
of default linters, that would all be running if we weren't setting
`disable-all: true`. By adding them, we now have parity with the
default configuration, as well as the additional linters we like.
Adds the following linters:
* unconvert
* deadcode
* structcheck
* typecheck
* varcheck
* wastedassign
Simplify the WFE `RevokeCertificate` API method in three ways:
- Remove most of the logic checking if the requester is authorized to
revoke the certificate in question (based on who is making the
request, what authorizations they have, and what reason they're
requesting). That checking is now done by the RA. Instead, simply
verify that the JWS is authenticated.
- Remove the hard-to-read `authorizedToRevoke` callbacks, and make the
`revokeCertBySubscriberKey` (nee `revokeCertByKeyID`) and
`revokeCertByCertKey` (nee `revokeCertByJWK`) helpers much more
straight-line in their execution logic.
- Call the RA's new `RevokeCertByApplicant` and `RevokeCertByKey` gRPC
methods, rather than the deprecated `RevokeCertificateWithReg`.
This change, without any flag flips, should be invisible to the
end-user. It will slightly change some of our log message formats.
However, by now relying on the new RA gRPC revocation methods, this
change allows us to change our revocation policies by enabling the
`AllowDoubleRevocation` and `MozRevocationReasons` feature flags, which
affect the behavior of those new helpers.
Fixes#5936
Adds a rocsp redis client to the sa if cluster information is provided in the
sa config. If a redis cluster is configured, all new certificate OCSP
responses added with sa.AddPrecertificate will attempt to be written to
the redis cluster, but will not block or fail on errors.
Fixes: #5871
Add two new gRPC methods to the SA:
- `RevokeCertByKey` will be used when the API request was signed by the
certificate's keypair, rather than a Subscriber keypair. If the
request is for reason `keyCompromise`, it will ensure that the key is
added to the blocked keys table, and will attempt to "re-revoke" a
certificate that was already revoked for some other reason.
- `RevokeCertByApplicant` supports both the path where the original
subscriber or another account which has proven control over all of the
identifier in the certificate requests revocation via the API. It does
not allow the requested reason to be `keyCompromise`, as these
requests do not represent a demonstration of key compromise.
In addition, add a new feature flag `MozRevocationReasons` which
controls the behavior of these new methods. If the flag is not set, they
behave like they have historically (see above). If the flag is set to true,
then the new methods enforce the upcoming Mozilla policies around
revocation reasons, namely:
- Only the original Subscriber can choose the revocation reason; other
clients will get a set reason code based on the method of requesting
revocation. When the original Subscriber requests reason
`keyCompromise`, this request will be honored, but the key will not be
blocked and other certificates with that key will not also be revoked.
- Revocations signed with the certificate key will always get reason
`keyCompromise`, because we do not know who is sending the request and
therefore must assume that the use of the key in this way represents
compromise. Because these requests will always be fore reason
`keyCompromise`, they will always be added to the blocked keys table
and they will always attempt "re-revocation".
- Revocations authorized via control of all names in the cert will
always get reason `cessationOfOperation`, which is to be used when the
original Subscriber does not control all names in the certificate
anymore.
Finally, update the existing `AdministrativelyRevokeCertificate` method
to use the new helper functions shared by the two new methods.
Part of #5936
We have decided that we don't like the if err := call(); err != nil
syntax, because it creates confusing scopes, but we have not cleaned up
all existing instances of that syntax. However, we have now found a
case where that syntax enables a bug: It caused readers to believe that
a later err = call() statement was assigning to an already-declared err
in the local scope, when in fact it was assigning to an
already-declared err in the parent scope of a closure. This caused our
ineffassign and staticcheck linters to be unable to analyze the
lifetime of the err variable, and so they did not complain when we
never checked the actual value of that error.
This change standardizes on the two-line error checking syntax
everywhere, so that we can more easily ensure that our linters are
correctly analyzing all error assignments.
Add `stylecheck` to our list of lints, since it got separated out from
`staticcheck`. Fix the way we configure both to be clearer and not
rely on regexes.
Additionally fix a number of easy-to-change `staticcheck` and
`stylecheck` violations, allowing us to reduce our number of ignored
checks.
Part of #5681
These gRPC methods were only used by the ACMEv1 code paths.
Now that boulder-wfe has been fully removed, we can be confident
that no clients ever call these methods, and can remove them from
the gRPC service interface.
Part of #5816
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
Empty the bodies of the WFE's and RA's `NewAuthorization` methods. These
were used exclusively by the ACMEv1 flow. Also remove any helper functions
which were used exclusively by this code, and any tests which were testing
exclusively this code and which have equivalent tests for the ACMEv2 flow.
Greatly simply `SA.GetAuthorizations2`, as it no longer has to contend with
there being two different kinds of authorizations in the database. Add a few
TODOs to consider removing a few other SA gRPC methods which no longer
have any callers.
Part of #5681
The NewCertificate codepath was the ACME v1 API's equivalent of
the modern Finalize endpoint. Remove the bodies of the WFE's and
the RA's `NewCertificate` functions. Remove the functions which were
called only from those functions. One of the removed functions is the
old `checkAuthorizations`, so update some tests which were calling
that directly to instead use different entry points.
Part of #5681
Phil Porada
Jacob Hoffman-Andrews
Matthew McPherrin
Aaron Gable
Samantha
Nina
Andrew Gabbitas