Adds the chosen DNS resolver to the VAs `ValidationRecord` object so
that for each challenge type during a validation, boulder can audit log
the resolver(s) chosen to fulfill the request..
Fixes https://github.com/letsencrypt/boulder/issues/7140
If the resolver provides EDE (https://www.rfc-editor.org/rfc/rfc8914),
Boulder will automatically expose it in the error message. Note that
most error messages contain the error RCODE (NXDOMAIN, SERVFAIL, etc),
when there is EDE present we omit it in the interest of brevity. In
practice it will almost always be SERVFAIL, and the extended error
information is more informative anyhow.
This will have no effect in production until we configure Unbound to
enable EDE.
Fixes#6875.
---------
Co-authored-by: Matthew McPherrin <mattm@letsencrypt.org>
[Go style says](https://blog.golang.org/package-names):
> Avoid stutter. Since client code uses the package name as a prefix
> when referring to the package contents, the names for those contents
> need not repeat the package name. The HTTP server provided by the
> http package is called Server, not HTTPServer. Client code refers to
> this type as http.Server, so there is no ambiguity.
Rename DNSClient, DNSClientImpl, NewDNSClientImpl,
NewTestDNSClientImpl, DNSError, and MockDNSClient to follow those
guidelines.
Unexport DNSClientImpl and MockTimeoutError (was only used internally).
Make New and NewTest return the Client interface rather than a concrete
`impl` type.
When the VA encounters CAA records, it logs the contents of those
records. When those records were the result of following a chain of
CNAMEs, the CNAMEs are included as part of the response from our
recursive resolver. However, the current flow for logging the responses
logs only the CAA records, not the CNAMEs.
This change returns the complete dig-style RR response from bdns to the
va where the response of the authoritative CAA RR is string-quoted and
logged.
This dig-style RR response is quite verbose, however it is only ever
returned from bdns.LookupCAA when a CAA response is non-empty. If the CAA
response is empty only an empty string is returned.
Fixes#5082
We've found we need the context offered from logging the error closer to when it
happens in the `bdns` package rather than in the `va`. Adopting the function
requires adapting it slightly. Specifically in the new location we know it won't
be called with any timeout results, with a non-dns error, or with a nil
underlying error.
Having the logging done in `bdns` (and specifically from `exchangeOne`) also
lets us log the wire format of the query and response when we get a `dns.ErrId`
error indicating a query/response ID mismatch. A small unit test is included
that ensures the logging happens as expected.
In case it proves useful for matching against other metrics the DNS ID mismatch
error case also now increments a dedicated prometheus counter vector stat,
`dns_id_mismatch`. The stat is labelled by resolver and query type.
Resolves https://github.com/letsencrypt/boulder/issues/4532
When we get a DNS error that has an internal cause (like connection
refused), we return a generic message like "networking error" to the
user to avoid revealing details that would be confusing. However, when
debugging problems with our own services, it's useful to have the
underlying errors.
This adds a helper method in the VA and calls it from each place we use
DNS errors.
Before this change, we would just log "Correct value not found for DNS challenge"
when we got a TXT record that didn't match what we expected. This was different
from the error when no TXT records were found at all, but viewing the error out of
context doesn't make that clear. This change improves the error to specifically say
that we found a TXT record, but it was the wrong one.
Also in this change: if we found multiple TXT records, we mention the number;
and we trim the length of the echoed TXT record.
There were a bunch of test fixtures in bdns/mocks.go that were only used in va/caa_test.go. This moves them to be in the same file so we have less spooky action at a distance.
One side-effect: We can't construct bdns.DNSError with the internal fields we want, because those fields are unexported. So we switch a couple of mock cases to just return a generic error, and the corresponding test cases to expect that error.
This implements the pre-erratum 5065 version of CAA, behind a feature flag.
This involved refactoring DNSClient.LookupCAA to return a list of CNAMEs in addition to the CAA records, and adding an alternate lookuper that does tree-climbing on single-depth aliases.
Fixes#639.
This resolves something that has bugged me for two+ years, our DNSResolverImpl is not a DNS resolver, it is a DNS client. This change just makes that obvious.
This PR introduces a new feature flag "IPv6First".
When the "IPv6First" feature is enabled the VA's HTTP dialer and TLS SNI
(01 and 02) certificate fetch requests will attempt to automatically
retry when the initial connection was to IPv6 and there is an IPv4
address available to retry with.
This resolves https://github.com/letsencrypt/boulder/issues/2623
This patch removes all usages of the `core.XXXError` and almost all usages of `probs` outside of the WFE and VA and replaces them with a unified internal error type. Since the VA uses `probs.ProblemDetails` quite extensively in challenges, and currently stores them in the DB I've saved this change for another change (it'll also require a migration). Since `ProblemDetails` should only ever be exposed to end-users all of its related logic should be moved into the `WFE` but since it still needs to be exposed to the VA and SA I've left it in place for now.
The new internal `errors` package offers the same convenience functions as `probs` does as well as a new simpler type testing method. A few small changes have also been made to error messages, mainly adding the library and function name to internal server errors for easier debugging (i.e. where a number of functions return the exact same errors and there is no other way to distinguish which method threw the error).
Also adds proper encoding of internal errors transferred over gRPC (the current encoding scheme is kept for `core` and `probs` errors since it'll be ideally be removed after we deploy this and follow-up changes) using `grpc/metadata` instead of the gRPC status codes.
Fixes#2507. Updates #2254 and #2505.
Presently when the VA performs a DNS-01 challenge verification it
returns the same error for the case where the remote nameserver had the
**wrong** TXT value, and when the remote nameserver had an **empty**
response for the TXT query. It would aid debugging if the user was told
which of the two failure cases was responsible for the overall challenge
failure.
This commit adds a unique error message for the empty TXT records case,
and a unit test/mock to exercise the new the error message.
Resolves#2326
This PR reworks the validateEmail() function from the RA to allow timeouts during DNS validation of MX/A/AAAA records for an email to be non-fatal and match our intention to verify emails best-effort.
Notes:
bdns/problem.go - DNSError.Timeout() was changed to also include context cancellation and timeout as DNS timeouts. This matches what DNSError.Error() was doing to set the error message and supports external callers to Timeout not duplicating the work.
bdns/mocks.go - the LookupMX mock was changed to support always.error and always.timeout in a manner similar to the LookupHost mock. Otherwise the TestValidateEmail unit test for the RA would fail when the MX lookup completed before the Host lookup because the error wouldn't be correct (empty DNS records vs a timeout or network error).
test/config/ra.json, test/config-next/ra.json - the dnsTries and dnsTimeout values were updated such that dnsTries * dnsTimeout was <= the WFE->RA RPC timeout (currently 15s in the test configs). This allows the dns lookups to all timeout without the overall RPC timing out.
Resolves#2260.
When a CAA request to Unbound times out, fall back to checking CAA via Google Public DNS' HTTPS API, through multiple proxies so as to hit geographically distributed paths. All successful multipath responses must be identical in order to succeed, and at most one can fail.
Fixes#1618
* Split out CAA checking service (minus logging etc)
* Add example.yml config + follow general Boulder style
* Update protobuf package to correct version
* Add grpc client to va
* Add TLS authentication in both directions for CAA client/server
* Remove go lint check
* Add bcodes package listing custom codes for Boulder
* Add very basic (pull-only) gRPC metrics to VA + caa-service
Also, stop skipping CAA lookups for the root TLDs. The RFC is unclear on
the desired behavior here, but the ICANNTLD function is nonstandard and
the behavior is strictly more conservative than what we had before.
This unblocks the removal of the ICANNTLD function, which allows us to
stop forking upstream.
Closes#1522
The CAA response checking method has been refactored to have a
easier to follow straight-line control flow. Several bugs in it have
been fixed:
- Firstly, parameters for issue and issuewild directives were not
parsed, so any attempt to specify parameters would result in
a string mismatch with the CA CAA identity (e.g. "letsencrypt.org").
Moreover, the syntax as specified permits leading and trailing
whitespace, so a parameter-free record such as
" letsencrypt.org ; " would not be considered a match.
This has been fixed by stripping whitespace and parameters. The RFC
does not specify the criticality of parameters, so unknown
parameters (currently all parameters) are considered noncritical.
I justify this as follows:
If someone decides to nominate a CA in a CAA record, they can,
with trivial research, determine what parameters, if any, that
CA supports, and presumably in trusting them in the first place
is able to adequately trust that the CA will continue to support
those parameters. The risk from other CAs is zero because other CAs
do not process the parameters because the records in which they
appear they do not relate to them.
- Previously, all of the flag bits were considered to effectively mean
'critical'. However, the RFC specifies that all bits except for the
actual critical bit (decimal 128) should be ignored. In practice,
many people have misunderstood the RFC to mean that the critical bit
is decimal 1, so both bits are interpreted to mean 'critical', but
this change ignores all of the other bits. This ensures that the
remaining six bits are reasonably usable for future standards action
if any need should arise.
- Previously, existence of an "issue" directive but no "issuewild"
directive was essentially equivalent to an unsatisfiable "issuewild"
directive, meaning that no wildcard identifiers could pass the CAA
check. This is contrary to the RFC, which states that issuewild
should default to what is specified for "issue" if no issuewild
directives are specified. (This is somewhat moot since boulder
doesn't currently support wildcard issuance.)
- Conversely, existence of an "issuewild" directive but no "issue"
directive would cause CAA validation for a non-wildcard identifier
to fail, which was contrary to the RFC. This has been fixed.
- More generally, existence of any unknown non-critical directive, say
"foobar", would cause the CAA checking code to act as though an
unsatisfiable "issue" directive existed, preventing any issuance.
This has been fixed.
Test coverage for corner cases is enhanced and provides regression
testing for these bugs.
statsd statistics have been added for tracking the relative frequency
of occurrence of different CAA features and outcomes. I added these on
a whim suspecting that they may be of interest.
Fixes#1436.
Previously we would return a detailed errorString, which ProblemDetailsFromDNSError
would turn into a generic, uninformative "Server failure at resolver".
Now we return a new internal dnsError type, which ProblemDetailsFromDNSError can
turn into a more informative message to be shown to the user.
James Renken
Phil Porada
Jacob Hoffman-Andrews
Jacob Hoffman-Andrews
Samantha
Daniel McCarney
Roland Bracewell Shoemaker
Roland Bracewell Shoemaker
Daniel McCarney
Kane York
Hugo Landau
Romain Fliedel