Skip to content

Commit

Permalink
va: compute maxRemoteFailures based on MPIC (#7810)
Browse files Browse the repository at this point in the history 
Previously this was a configuration field.

Ports `maxAllowedFailures()` from `determineMaxAllowedFailures()` in
#7794.

Test updates:
 
Remove the `maxRemoteFailures` param from `setup` in all VA tests.

Some tests were depending on setting this param directly to provoke
failures.

For example, `TestMultiVAEarlyReturn` previously relied on "zero allowed
failures". Since the number of allowed failures is now 1 for the number
of remote VAs we were testing (2), the VA wasn't returning early with an
error; it was succeeding! To fix that, make sure there are two failures.
Since two failures from two RVAs wouldn't exercise the right situation,
add a third RVA, so we get two failures from three RVAs.

Similarly, TestMultiCAARechecking had several test cases that omitted
this field, effectively setting it to zero allowed failures. I updated
the "1 RVA failure" test case to expect overall success and added a "2
RVA failures" test case to expect overall failure (we previously
expected overall failure from a single RVA failing).

In TestMultiVA I had to change a test for `len(lines) != 1` to
`len(lines) == 0`, because with more backends we were now logging more
errors, and finding e.g. `len(lines)` to be 2.
  • Loading branch information
@jsha
jsha authored Nov 18, 2024
1 parent 20fdcbc commit a46c388
Show file tree
Hide file tree
Showing 11 changed files with 273 additions and 137 deletions.
6 changes: 3 additions & 3 deletions cmd/boulder-va/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,9 @@ import (
type Config struct {
VA struct {
vaConfig.Common
RemoteVAs []cmd.GRPCClientConfig `validate:"omitempty,dive"`
MaxRemoteValidationFailures int `validate:"omitempty,min=0,required_with=RemoteVAs"`
RemoteVAs []cmd.GRPCClientConfig `validate:"omitempty,dive"`
// Deprecated and ignored
MaxRemoteValidationFailures int `validate:"omitempty,min=0,required_with=RemoteVAs"`
Features features.Config
}

Expand Down Expand Up @@ -109,7 +110,6 @@ func main() {
vai, err := va.NewValidationAuthorityImpl(
resolver,
remotes,
c.VA.MaxRemoteValidationFailures,
c.VA.UserAgent,
c.VA.IssuerDomain,
scope,
Expand Down
1 change: 0 additions & 1 deletion cmd/remoteva/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,6 @@ func main() {
vai, err := va.NewValidationAuthorityImpl(
resolver,
nil, // Our RVAs will never have RVAs of their own.
0, // Only the VA is concerned with max validation failures
c.RVA.UserAgent,
c.RVA.IssuerDomain,
scope,
Expand Down
2 changes: 1 addition & 1 deletion docs/multi-va.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ and
as their config files.

We require that almost all remote validation requests succeed; the exact number
is controlled by the VA's `maxRemoteFailures` config variable. If the number of
is controlled by the VA based on the thresholds required by MPIC. If the number of
failing remote VAs exceeds that threshold, validation is terminated. If the
number of successful remote VAs is high enough that it would be impossible for
the outstanding remote VAs to exceed that threshold, validation immediately
Expand Down
4 changes: 2 additions & 2 deletions features/features.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@ type Config struct {

// MultiCAAFullResults will cause the main VA to block and wait for all of
// the remote VA CAA recheck results instead of returning early if the
// number of failures is greater than the configured
// maxRemoteValidationFailures. Only used when EnforceMultiCAA is true.
// number of failures is greater than the number allowed by MPIC.
// Only used when EnforceMultiCAA is true.
MultiCAAFullResults bool

// MultipleCertificateProfiles, when enabled, triggers the following
Expand Down
1 change: 0 additions & 1 deletion test/config-next/va.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@
"hostOverride": "rva1.boulder"
}
],
"maxRemoteValidationFailures": 1,
"accountURIPrefixes": [
"http://boulder.service.consul:4000/acme/reg/",
"http://boulder.service.consul:4001/acme/acct/"
Expand Down
76 changes: 53 additions & 23 deletions va/caa_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ func (mock caaMockDNS) LookupCAA(_ context.Context, domain string) ([]*dns.CAA,
}

func TestCAATimeout(t *testing.T) {
va, _ := setup(nil, 0, "", nil, caaMockDNS{})
va, _ := setup(nil, "", nil, caaMockDNS{})

params := &caaParams{
accountURIID: 12345,
Expand Down Expand Up @@ -408,7 +408,7 @@ func TestCAAChecking(t *testing.T) {
method := core.ChallengeTypeHTTP01
params := &caaParams{accountURIID: accountURIID, validationMethod: method}

va, _ := setup(nil, 0, "", nil, caaMockDNS{})
va, _ := setup(nil, "", nil, caaMockDNS{})
va.accountURIPrefixes = []string{"https://letsencrypt.org/acct/reg/"}

for _, caaTest := range testCases {
Expand All @@ -431,7 +431,7 @@ func TestCAAChecking(t *testing.T) {
}

func TestCAALogging(t *testing.T) {
va, _ := setup(nil, 0, "", nil, caaMockDNS{})
va, _ := setup(nil, "", nil, caaMockDNS{})

testCases := []struct {
Name string
Expand Down Expand Up @@ -521,7 +521,7 @@ func TestCAALogging(t *testing.T) {
// TestIsCAAValidErrMessage tests that an error result from `va.IsCAAValid`
// includes the domain name that was being checked in the failure detail.
func TestIsCAAValidErrMessage(t *testing.T) {
va, _ := setup(nil, 0, "", nil, caaMockDNS{})
va, _ := setup(nil, "", nil, caaMockDNS{})

// Call IsCAAValid with a domain we know fails with a generic error from the
// caaMockDNS.
Expand All @@ -546,7 +546,7 @@ func TestIsCAAValidErrMessage(t *testing.T) {
// which do not have the necessary parameters to do CAA Account and Method
// Binding checks.
func TestIsCAAValidParams(t *testing.T) {
va, _ := setup(nil, 0, "", nil, caaMockDNS{})
va, _ := setup(nil, "", nil, caaMockDNS{})

// Calling IsCAAValid without a ValidationMethod should fail.
_, err := va.IsCAAValid(ctx, &vapb.IsCAAValidRequest{
Expand Down Expand Up @@ -592,7 +592,7 @@ func (b caaBrokenDNS) LookupCAA(_ context.Context, domain string) ([]*dns.CAA, s
func TestDisabledMultiCAARechecking(t *testing.T) {
brokenRVA := setupRemote(nil, "broken", caaBrokenDNS{}, "", "")
remoteVAs := []RemoteVA{{brokenRVA, "broken"}}
va, _ := setup(nil, 0, "local", remoteVAs, nil)
va, _ := setup(nil, "local", remoteVAs, nil)

features.Set(features.Config{
EnforceMultiCAA: false,
Expand Down Expand Up @@ -671,7 +671,6 @@ func TestMultiCAARechecking(t *testing.T) {

testCases := []struct {
name string
maxLookupFailures int
domains string
remoteVAs []RemoteVA
expectedProbSubstring string
Expand Down Expand Up @@ -719,13 +718,31 @@ func TestMultiCAARechecking(t *testing.T) {
{
name: "functional localVA, 1 broken RVA, no CAA records",
domains: "present-dns-only.com",
localDNSClient: caaMockDNS{},
expectedDiffLogSubstring: `RemoteSuccesses":2,"RemoteFailures":[{"VAHostname":"broken","Problem":{"type":"dns","detail":"While processing CAA for`,
remoteVAs: []RemoteVA{
{brokenVA, brokenUA},
{remoteVA, remoteUA},
{remoteVA, remoteUA},
},
expectedLabels: prometheus.Labels{
"operation": opCAA,
"perspective": allPerspectives,
"challenge_type": string(core.ChallengeTypeDNS01),
"problem_type": "",
"result": pass,
},
},
{
name: "functional localVA, 2 broken RVAs, no CAA records",
domains: "present-dns-only.com",
expectedProbSubstring: "During secondary CAA checking: While processing CAA",
expectedProbType: probs.DNSProblem,
expectedDiffLogSubstring: `RemoteSuccesses":2,"RemoteFailures":[{"VAHostname":"broken","Problem":{"type":"dns","detail":"While processing CAA for`,
expectedDiffLogSubstring: `RemoteSuccesses":1,"RemoteFailures":[{"VAHostname":"broken","Problem":{"type":"dns","detail":"While processing CAA for`,
localDNSClient: caaMockDNS{},
remoteVAs: []RemoteVA{
{brokenVA, brokenUA},
{remoteVA, remoteUA},
{brokenVA, brokenUA},
{remoteVA, remoteUA},
},
expectedLabels: prometheus.Labels{
Expand Down Expand Up @@ -776,15 +793,33 @@ func TestMultiCAARechecking(t *testing.T) {
{
name: "functional localVA, 1 broken RVA, CAA issue type present",
domains: "present.com",
expectedProbSubstring: "During secondary CAA checking: While processing CAA",
expectedProbType: probs.DNSProblem,
expectedDiffLogSubstring: `RemoteSuccesses":2,"RemoteFailures":[{"VAHostname":"broken","Problem":{"type":"dns","detail":"While processing CAA for`,
localDNSClient: caaMockDNS{},
remoteVAs: []RemoteVA{
{brokenVA, brokenUA},
{remoteVA, remoteUA},
{remoteVA, remoteUA},
},
expectedLabels: prometheus.Labels{
"operation": opCAA,
"perspective": allPerspectives,
"challenge_type": string(core.ChallengeTypeDNS01),
"problem_type": "",
"result": pass,
},
},
{
name: "functional localVA, 2 broken RVA, CAA issue type present",
domains: "present.com",
expectedProbSubstring: "During secondary CAA checking: While processing CAA",
expectedProbType: probs.DNSProblem,
expectedDiffLogSubstring: `RemoteSuccesses":1,"RemoteFailures":[{"VAHostname":"broken","Problem":{"type":"dns","detail":"While processing CAA for`,
localDNSClient: caaMockDNS{},
remoteVAs: []RemoteVA{
{brokenVA, brokenUA},
{brokenVA, brokenUA},
{remoteVA, remoteUA},
},
expectedLabels: prometheus.Labels{
"operation": opCAA,
"perspective": allPerspectives,
Expand Down Expand Up @@ -831,8 +866,6 @@ func TestMultiCAARechecking(t *testing.T) {
{
name: "1 hijacked RVA, CAA issue type present",
domains: "present.com",
expectedProbSubstring: "CAA record for present.com prevents issuance",
expectedProbType: probs.CAAProblem,
expectedDiffLogSubstring: `RemoteSuccesses":2,"RemoteFailures":[{"VAHostname":"hijacked","Problem":{"type":"caa","detail":"While processing CAA for`,
localDNSClient: caaMockDNS{},
remoteVAs: []RemoteVA{
Expand Down Expand Up @@ -870,8 +903,6 @@ func TestMultiCAARechecking(t *testing.T) {
{
name: "1 hijacked RVA, CAA issuewild type present",
domains: "satisfiable-wildcard.com",
expectedProbSubstring: "During secondary CAA checking: While processing CAA",
expectedProbType: probs.CAAProblem,
expectedDiffLogSubstring: `RemoteSuccesses":2,"RemoteFailures":[{"VAHostname":"hijacked","Problem":{"type":"caa","detail":"While processing CAA for`,
localDNSClient: caaMockDNS{},
remoteVAs: []RemoteVA{
Expand Down Expand Up @@ -907,9 +938,8 @@ func TestMultiCAARechecking(t *testing.T) {
},
},
{
name: "1 hijacked RVA, CAA issuewild type present, 1 failure allowed",
name: "1 hijacked RVA, CAA issuewild type present",
domains: "satisfiable-wildcard.com",
maxLookupFailures: 1,
expectedDiffLogSubstring: `RemoteSuccesses":2,"RemoteFailures":[{"VAHostname":"hijacked","Problem":{"type":"caa","detail":"While processing CAA for`,
localDNSClient: caaMockDNS{},
remoteVAs: []RemoteVA{
Expand All @@ -919,9 +949,8 @@ func TestMultiCAARechecking(t *testing.T) {
},
},
{
name: "2 hijacked RVAs, CAA issuewild type present, 1 failure allowed",
name: "2 hijacked RVAs, CAA issuewild type present",
domains: "satisfiable-wildcard.com",
maxLookupFailures: 1,
expectedProbSubstring: "During secondary CAA checking: While processing CAA",
expectedProbType: probs.CAAProblem,
expectedDiffLogSubstring: `RemoteSuccesses":1,"RemoteFailures":[{"VAHostname":"hijacked","Problem":{"type":"caa","detail":"While processing CAA for`,
Expand All @@ -933,9 +962,8 @@ func TestMultiCAARechecking(t *testing.T) {
},
},
{
name: "3 hijacked RVAs, CAA issuewild type present, 1 failure allowed",
name: "3 hijacked RVAs, CAA issuewild type present",
domains: "satisfiable-wildcard.com",
maxLookupFailures: 1,
expectedProbSubstring: "During secondary CAA checking: While processing CAA",
expectedProbType: probs.CAAProblem,
expectedDiffLogSubstring: `RemoteSuccesses":0,"RemoteFailures":[{"VAHostname":"hijacked","Problem":{"type":"caa","detail":"While processing CAA for`,
Expand All @@ -950,7 +978,7 @@ func TestMultiCAARechecking(t *testing.T) {

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
va, mockLog := setup(nil, tc.maxLookupFailures, localUA, tc.remoteVAs, tc.localDNSClient)
va, mockLog := setup(nil, localUA, tc.remoteVAs, tc.localDNSClient)
defer mockLog.Clear()

// MultiCAAFullResults: false is inherently flaky because of the
Expand All @@ -971,12 +999,14 @@ func TestMultiCAARechecking(t *testing.T) {
test.AssertNotError(t, err, "Should not have errored, but did")

if tc.expectedProbSubstring != "" {
test.AssertNotNil(t, isValidRes.Problem, "IsCAAValidRequest returned nil problem, but should not have")
test.AssertContains(t, isValidRes.Problem.Detail, tc.expectedProbSubstring)
} else if isValidRes.Problem != nil {
test.AssertBoxedNil(t, isValidRes.Problem, "IsCAAValidRequest returned a problem, but should not have")
}

if tc.expectedProbType != "" {
test.AssertNotNil(t, isValidRes.Problem, "IsCAAValidRequest returned nil problem, but should not have")
test.AssertEquals(t, string(tc.expectedProbType), isValidRes.Problem.ProblemType)
}

Expand Down Expand Up @@ -1017,7 +1047,7 @@ func TestCAAFailure(t *testing.T) {
hs := httpSrv(t, expectedToken)
defer hs.Close()

va, _ := setup(hs, 0, "", nil, caaMockDNS{})
va, _ := setup(hs, "", nil, caaMockDNS{})

err := va.checkCAA(ctx, dnsi("reserved.com"), &caaParams{1, core.ChallengeTypeHTTP01})
if err == nil {
Expand Down
20 changes: 10 additions & 10 deletions va/dns_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import (
)

func TestDNSValidationEmpty(t *testing.T) {
va, _ := setup(nil, 0, "", nil, nil)
va, _ := setup(nil, "", nil, nil)

// This test calls PerformValidation directly, because that is where the
// metrics checked below are incremented.
Expand All @@ -38,7 +38,7 @@ func TestDNSValidationEmpty(t *testing.T) {
}

func TestDNSValidationWrong(t *testing.T) {
va, _ := setup(nil, 0, "", nil, nil)
va, _ := setup(nil, "", nil, nil)
_, err := va.validateDNS01(context.Background(), dnsi("wrong-dns01.com"), expectedKeyAuthorization)
if err == nil {
t.Fatalf("Successful DNS validation with wrong TXT record")
Expand All @@ -48,7 +48,7 @@ func TestDNSValidationWrong(t *testing.T) {
}

func TestDNSValidationWrongMany(t *testing.T) {
va, _ := setup(nil, 0, "", nil, nil)
va, _ := setup(nil, "", nil, nil)

_, err := va.validateDNS01(context.Background(), dnsi("wrong-many-dns01.com"), expectedKeyAuthorization)
if err == nil {
Expand All @@ -59,7 +59,7 @@ func TestDNSValidationWrongMany(t *testing.T) {
}

func TestDNSValidationWrongLong(t *testing.T) {
va, _ := setup(nil, 0, "", nil, nil)
va, _ := setup(nil, "", nil, nil)

_, err := va.validateDNS01(context.Background(), dnsi("long-dns01.com"), expectedKeyAuthorization)
if err == nil {
Expand All @@ -70,7 +70,7 @@ func TestDNSValidationWrongLong(t *testing.T) {
}

func TestDNSValidationFailure(t *testing.T) {
va, _ := setup(nil, 0, "", nil, nil)
va, _ := setup(nil, "", nil, nil)

_, err := va.validateDNS01(ctx, dnsi("localhost"), expectedKeyAuthorization)
prob := detailedError(err)
Expand All @@ -84,7 +84,7 @@ func TestDNSValidationInvalid(t *testing.T) {
Value: "790DB180-A274-47A4-855F-31C428CB1072",
}

va, _ := setup(nil, 0, "", nil, nil)
va, _ := setup(nil, "", nil, nil)

_, err := va.validateDNS01(ctx, notDNS, expectedKeyAuthorization)
prob := detailedError(err)
Expand All @@ -93,7 +93,7 @@ func TestDNSValidationInvalid(t *testing.T) {
}

func TestDNSValidationServFail(t *testing.T) {
va, _ := setup(nil, 0, "", nil, nil)
va, _ := setup(nil, "", nil, nil)

_, err := va.validateDNS01(ctx, dnsi("servfail.com"), expectedKeyAuthorization)

Expand All @@ -102,7 +102,7 @@ func TestDNSValidationServFail(t *testing.T) {
}

func TestDNSValidationNoServer(t *testing.T) {
va, log := setup(nil, 0, "", nil, nil)
va, log := setup(nil, "", nil, nil)
staticProvider, err := bdns.NewStaticProvider([]string{})
test.AssertNotError(t, err, "Couldn't make new static provider")

Expand All @@ -121,15 +121,15 @@ func TestDNSValidationNoServer(t *testing.T) {
}

func TestDNSValidationOK(t *testing.T) {
va, _ := setup(nil, 0, "", nil, nil)
va, _ := setup(nil, "", nil, nil)

_, prob := va.validateDNS01(ctx, dnsi("good-dns01.com"), expectedKeyAuthorization)

test.Assert(t, prob == nil, "Should be valid.")
}

func TestDNSValidationNoAuthorityOK(t *testing.T) {
va, _ := setup(nil, 0, "", nil, nil)
va, _ := setup(nil, "", nil, nil)

_, prob := va.validateDNS01(ctx, dnsi("no-authority-dns01.com"), expectedKeyAuthorization)

Expand Down
Loading

0 comments on commit a46c388

Please sign in to comment.