If two revocation requests for the same cert arrive in rapid
succession, it is possible for one of them to complete in the
time between the other one's initial check (that the cert isn't
revoked already) and final database update. This causes
the db update to fail, and the request to end in a 500.
Other methods, such as new account registration, have a
three-step "check for duplicates, update db, if that failed
check for duplicates again" flow. This change updates the
cert revocation handlers to have a similar flow.
It returns the RFC 8555 `alreadyRevoked` from wfe2, to
match the error code returned by the initial duplicate
check. It returns the non-standardized 409 Conflict from
wfe, to match the code returned by that frontend's initial
duplicate check.
Fixes#5107
Previously this was a NotFound error, but since we now update the
certificateStatus table synchronously on issuance and revocation, we
expect to always get a successful response; if we get an error, that's
a ServerInternal error.
As part of #5050, I'm updating some of the code in grpc/pb-marshaling.go
to move from nil checks to zero checks. In the process I'm introducing some
new zero checks, on things like challenge type, status, and token. This is
shaking out some places where our mocks have taken shortcuts by not
creating a "full" object including all fields that are normally present.
This PR updates our mocks and tests to provide more realistic objects in
all the places that broke when introducing those zero checks.
One slightly surprising / interesting thing: Since core types like
Order and Registration are still proto2 and have pointer fields,
there are actually some places in this PR where I had to add
a `*` rather than delete an `&`, because I was taking a pointer
field from one of those core types and passing it as a field in
an SA RPC request.
Fixes#5037.
Updates the Registration Authority to use proto3 for its
RPC methods. This turns out to be a fairly minimal change,
as many of the RA's request and response messages are
defined in core.proto, and are therefore still proto2.
Fixes#4955
ACME Challenges are well-known strings ("http-01", "dns-01", and
"tlsalpn-01") identifying which kind of challenge should be used
to verify control of a domain. Because they are well-known and
only certain values are valid, it is better to represent them as
something more akin to an enum than as bare strings. This also
improves our ability to ensure that an AcmeChallenge is not
accidentally used as some other kind of string in a different
context. This change also brings them closer in line with the
existing core.AcmeResource and core.OCSPStatus string enums.
Fixes#5009
We'd like to issue certs with no CN eventually, but it's not
going to happen any time soon. In the mean time, the existing
code never gets exercised and is rather complex, so this
removes it.
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.
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.
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
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.
Normally we do this with a reverse proxy in front of Boulder, but it's
nice to have an additional layer of protection in case someone deploys
Boulder without a reverse proxy.
Fixes#4730.
Currently 99.99% of RSA keys we see in certificates at Let's Encrypt are
either 2048, 3072, or 4096 bits, but we support every 8 bit increment
between 2048 and 4096. Supporting these uncommon key sizes opens us up to
having to block much larger ranges of keys when dealing with something
like the Debian weak keys incident. Instead we should just reduce the
set of key sizes we support down to what people actually use.
Fixes#4835.
This required a refactoring: Move validateEmail from the RA to ValidEmail
in the `policy` package. I also moved `ValidDomain` from a method on
PolicyAuthority to a standalone function so that ValidEmail can call it.
notify-mailer will now log invalid addresses and skip them without
attempting to send mail. Since @example.com addresses are invalid,
I updated the notify-mailer test, which used a lot of such addresses.
Also, now when notify-mailer receives an unrecoverable error sending
mail, it logs the email address and what offset within the list it was.
When we originally added this package (4 years ago) x/crypto/ocsp didn't
have its own list of revocation reasons, so we added our own. Now it does
have its own list, so just use that list instead of duplicating code for
no real reason.
Also we build a list of the revocation reasons we support so that we can
tell users when they try to use an unsupported one. Instead of building
this string every time, just build it once it during package initialization.
Finally return the same error message in wfe that we use in wfe2 when a
user requests an unsupported reason.
In #3708, we added formatters for the the convenience methods in the
`probs` package.
However, in #4783, @alexzorin pointed out that we were incorrectly
passing an error message through fmt.Sprintf as the format parameter
rather than as a value parameter.
I proposed a fix in #4784, but during code review we concluded that the
underlying problem was the pattern of using format-style functions that
don't have some variant of printf in the name. That makes this wrong:
`probs.DNS(err.Error())`, and this right: `probs.DNS("%s", err)`. Since
that's an easy mistake to make and a hard one to spot during code review,
we're going to stop using this particular pattern and call `fmt.Sprintf`
directly.
This PR reverts #3708 and adds some `fmt.Sprintf` where needed.
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.
Prev. we weren't checking the domain portion of an email contact address
very strictly in the RA. This updates the PA to export a function that
can be used to validate the domain the same way we validate domain
portions of DNS type identifiers for issuance.
This also changes the RA to use the `invalidEmail` error type in more
places.
A new Go integration test is added that checks these errors end-to-end
for both account creation and account update.
Errors that were being returned in the checkAlgorithm methods of both wfe and
wfe2 didn't really match up to what was actually being checked. This change
attempts to bring the errors in line with what is actually being tested.
Fixes#4452.
Prev. the WFE2 would return a 500 error in the case where an
authorization ID was invalid. The WFE1 would return a 404. Returning
a malformed request problem reports the true cause of the error as
an invalid client request.
Previously we made v2 authz handling part of regular authz handling, and
just tried to strip the v2/ prefix. This led to a bug: #4327, where
authzv1s that had an id randomly start with "v2" would 404 because they
were treated as authzv2's.
This change moves authz2's and related challenges onto their own prefix,
and uses HandleFunc to dispatch them appropriately. To reduce code
duplication, I factored out the code that was common to v1 and v2 into
new Common functions.
I also changed the path names to contain "v3" instead of "v2" to avoid
potential confusion.
Basically a complete re-write/re-design of the forwarding concept introduced in
#4297 (sorry for the rapid churn here). Instead of nonce-services blindly
forwarding nonces around to each other in an attempt to find out who issued the
nonce we add an identifying prefix to each nonce generated by a service. The
WFEs then use this prefix to decide which nonce-service to ask to validate the
nonce.
This requires a slightly more complicated configuration at the WFE/2 end, but
overall I think ends up being a way cleaner, more understandable, easy to
reason about implementation. When configuring the WFE you need to provide two
forms of gRPC config:
* one gRPC config for retrieving nonces, this should be a DNS name that
resolves to all available nonce-services (or at least the ones you want to
retrieve nonces from locally, in a two DC setup you might only configure the
nonce-services that are in the same DC as the WFE instance). This allows
getting a nonce from any of the configured services and is load-balanced
transparently at the gRPC layer.
* a map of nonce prefixes to gRPC configs, this maps each individual
nonce-service to it's prefix and allows the WFE instances to figure out which
nonce-service to ask to validate a nonce it has received (in a two DC setup
you'd want to configure this with all the nonce-services across both DCs so
that you can validate a nonce that was generated by a nonce-service in another
DC).
This balancing is implemented in the integration tests.
Given the current remote nonce code hasn't been deployed anywhere yet this
makes a number of hard breaking changes to both the existing nonce-service
code, and the forwarding code.
Fixes#4303.
Also fixes a minor bug where `sa.UpdateRegistration` didn't properly check a
returned error. If a `errors.Duplicate` type error is returned in either `KeyRollover`/
`Newaccount` in wfe2 or `NewRegistration` in wfe during the update/insert step
the account info/pointer will be returned instead of an internal server error.
Fixes#3000.
Enables integration tests for authz2 and fixes a few bugs that were flagged up during the process. Disables expired-authorization-purger integration tests if config-next is being used as expired-authz-purger expects to purge some stuff but doesn't know about authz2 authorizations, a new test will be added with #4188.
Fixes#4079.
This will allow implementing sub-problems without creating a cyclic
dependency between `core` and `problems`.
The `identifier` package is somewhat small/single-purpose and in the
future we may want to move more "ACME" bits beyond the `identifier`
types into a dedicated package outside of `core`.
This PR implements new SA methods for handling authz2 style authorizations and updates existing SA methods to count and retrieve them where applicable when the `NewAuthorizationSchema` feature is enabled.
Fixes#4093Fixes#4082
Updates #4078
Updates #4077
Early ACME drafts supported a notion of "combinations" of challenges
that had to be completed together. This was removed from subsequent
drafts. Boulder has only ever supported "combinations" that exactly map
to the list of challenges, 1 for 1.
This removes all the plumbing for combinations, and adds a list of
combinations to the authz JSON right before marshaling it in WFE1.
Previously we relied on each instance of `features.Set` to have a
corresponding `defer features.Reset()`. If we forget that, we can wind
up with unexpected behavior where features set in one test case leak
into another test case. This led to the bug in
https://github.com/letsencrypt/boulder/issues/4118 going undetected.
Fix#4120
* Remove the challenge whitelist
* Reduce the signature for ChallengesFor and ChallengeTypeEnabled
* Some unit tests in the VA were changed from testing TLS-SNI to testing the same behavior
in TLS-ALPN, when that behavior wasn't already tested. For instance timeouts during connect
are now tested.
Fixes#4109
This changeset implements the logic required for the WFE to retrieve v2 authorizations and their associated challenges while still maintaining the logic to retrieve old authorizations/challenges. Challenge IDs for v2 authorizations are obfuscated using a pretty simply scheme in order to prevent hard coding of indexes. A `V2` field is added to the `core.Authorization` object and populated using the existing field of the same name from the protobuf for convenience. v2 authorizations and challenges use a `v2` prefix in all their URLs in order to easily differentiate between v1 and v2 URLs (e.g. `/acme/authz/v2/asd` and `/acme/challenge/v2/asd/123`), once v1 authorizations cease to exist this prefix can be safely removed. As v2 authorizations use int IDs this change switches from string IDs to int IDs, this mainly only effects tests.
Integration tests are put off for #4079 as they really need #4077 and #4078 to be properly effective.
Fixes#4041.
Aaron Gable