Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

va: Check for RIR and Perspective mismatches at runtime when they're provided #7841

Merged
merged 16 commits into from
Dec 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cmd/boulder-va/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,10 @@ type RemoteVAGRPCClientConfig struct {
// - RIPE
// - APNIC
// - LACNIC
// - AfriNIC
// - AFRINIC
//
// TODO(#7615): Make mandatory.
RIR string `validate:"omitempty,oneof=ARIN RIPE APNIC LACNIC AfriNIC"`
RIR string `validate:"omitempty,oneof=ARIN RIPE APNIC LACNIC AFRINIC"`
}

type Config struct {
Expand Down
6 changes: 3 additions & 3 deletions cmd/remoteva/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ type Config struct {
// of RVAs deployed in the same datacenter.
//
// TODO(#7615): Make mandatory.
Perspective string `validate:"omitempty"`
Perspective string `omitempty:"omitempty"`

// RIR indicates the Regional Internet Registry where this RVA is
// located. This field is used to identify the RIR region from which a
Expand All @@ -38,10 +38,10 @@ type Config struct {
// - RIPE
// - APNIC
// - LACNIC
// - AfriNIC
// - AFRINIC
//
// TODO(#7615): Make mandatory.
RIR string `validate:"omitempty,oneof=ARIN RIPE APNIC LACNIC AfriNIC"`
RIR string `validate:"omitempty,oneof=ARIN RIPE APNIC LACNIC AFRINIC"`

// SkipGRPCClientCertVerification, when disabled as it should typically
// be, will cause the remoteva server (which receives gRPCs from a
Expand Down
4 changes: 3 additions & 1 deletion test/config/remoteva-a.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,9 @@
"accountURIPrefixes": [
"http://boulder.service.consul:4000/acme/reg/",
"http://boulder.service.consul:4001/acme/acct/"
]
],
"perspective": "dadaist",
"rir": "ARIN"
},
"syslog": {
"stdoutlevel": 4,
Expand Down
4 changes: 3 additions & 1 deletion test/config/remoteva-b.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,9 @@
"accountURIPrefixes": [
"http://boulder.service.consul:4000/acme/reg/",
"http://boulder.service.consul:4001/acme/acct/"
]
],
"perspective": "surrealist",
"rir": "RIPE"
},
"syslog": {
"stdoutlevel": 4,
Expand Down
4 changes: 3 additions & 1 deletion test/config/remoteva-c.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,9 @@
"accountURIPrefixes": [
"http://boulder.service.consul:4000/acme/reg/",
"http://boulder.service.consul:4001/acme/acct/"
]
],
"perspective": "cubist",
"rir": "ARIN"
},
"syslog": {
"stdoutlevel": 4,
Expand Down
12 changes: 9 additions & 3 deletions test/config/va.json
Original file line number Diff line number Diff line change
Expand Up @@ -43,17 +43,23 @@
{
"serverAddress": "rva1.service.consul:9397",
"timeout": "15s",
"hostOverride": "rva1.boulder"
"hostOverride": "rva1.boulder",
"perspective": "dadaist",
"rir": "ARIN"
},
{
"serverAddress": "rva1.service.consul:9498",
"timeout": "15s",
"hostOverride": "rva1.boulder"
"hostOverride": "rva1.boulder",
"perspective": "surrealist",
"rir": "RIPE"
},
{
"serverAddress": "rva1.service.consul:9499",
"timeout": "15s",
"hostOverride": "rva1.boulder"
"hostOverride": "rva1.boulder",
"perspective": "cubist",
"rir": "ARIN"
}
],
"maxRemoteValidationFailures": 1,
Expand Down
17 changes: 12 additions & 5 deletions va/caa.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,12 +112,19 @@ func (va *ValidationAuthorityImpl) IsCAAValid(ctx context.Context, req *vapb.IsC
// It will also later be serialized in JSON, which defaults to UTF-8. Make
// sure it is UTF-8 clean now.
prob = filterProblemDetails(prob)
return &vapb.IsCAAValidResponse{Problem: &corepb.ProblemDetails{
ProblemType: string(prob.Type),
Detail: replaceInvalidUTF8([]byte(prob.Detail)),
}}, nil
return &vapb.IsCAAValidResponse{
Problem: &corepb.ProblemDetails{
ProblemType: string(prob.Type),
Detail: replaceInvalidUTF8([]byte(prob.Detail)),
},
Perspective: va.perspective,
Rir: va.rir,
}, nil
} else {
return &vapb.IsCAAValidResponse{}, nil
return &vapb.IsCAAValidResponse{
Perspective: va.perspective,
Rir: va.rir,
}, nil
}
}

Expand Down
17 changes: 15 additions & 2 deletions va/va.go
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ func NewValidationAuthorityImpl(

for i, va1 := range remoteVAs {
for j, va2 := range remoteVAs {
// TODO(#7819): Remove the != "" check once perspective is required.
// TODO(#7615): Remove the != "" check once perspective is required.
if i != j && va1.Perspective == va2.Perspective && va1.Perspective != "" {
return nil, fmt.Errorf("duplicate remote VA perspective %q", va1.Perspective)
}
Expand Down Expand Up @@ -504,6 +504,19 @@ func (va *ValidationAuthorityImpl) performRemoteOperation(ctx context.Context, o
for _, i := range rand.Perm(remoteVACount) {
go func(rva RemoteVA) {
res, err := op(subCtx, rva, req)
if err != nil {
responses <- &response{rva.Address, rva.Perspective, rva.RIR, res, err}
return
}
// TODO(#7615): Remove the != "" checks once perspective and rir are required.
if (rva.Perspective != "" && res.GetPerspective() != "" && res.GetPerspective() != rva.Perspective) ||
(rva.RIR != "" && res.GetRir() != "" && res.GetRir() != rva.RIR) {
err = fmt.Errorf(
"Expected perspective %q (%q) but got reply from %q (%q) - misconfiguration likely", rva.Perspective, rva.RIR, res.GetPerspective(), res.GetRir(),
)
responses <- &response{rva.Address, rva.Perspective, rva.RIR, res, err}
return
}
responses <- &response{rva.Address, rva.Perspective, rva.RIR, res, err}
}(va.remoteVAs[i])
}
Expand Down Expand Up @@ -538,7 +551,7 @@ func (va *ValidationAuthorityImpl) performRemoteOperation(ctx context.Context, o
}
if isCAACheck {
// We're checking CAA, log the problem.
va.log.Errf("Operation on Remote VA (%s) returned a problem: %s", resp.addr, err)
va.log.Errf("Operation on Remote VA (%s) returned a problem: %s", resp.addr, currProb)
}
} else {
// The remote VA returned a successful result.
Expand Down
50 changes: 46 additions & 4 deletions va/va_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ func setupRemote(srv *httptest.Server, userAgent string, mockDNSClientOverride b
rva.perspective = perspective
rva.rir = rir

return RemoteClients{VAClient: &inMemVA{*rva}, CAAClient: &inMemVA{*rva}}
return RemoteClients{VAClient: &inMemVA{rva}, CAAClient: &inMemVA{rva}}
}

// RIRs
Expand Down Expand Up @@ -296,14 +296,14 @@ func (b brokenRemoteVA) IsCAAValid(_ context.Context, _ *vapb.IsCAAValidRequest,
// ValidationAuthorityImpl rather than over the network. This lets a local
// in-memory mock VA act like a remote VA.
type inMemVA struct {
rva ValidationAuthorityImpl
rva *ValidationAuthorityImpl
}

func (inmem inMemVA) PerformValidation(ctx context.Context, req *vapb.PerformValidationRequest, _ ...grpc.CallOption) (*vapb.ValidationResult, error) {
func (inmem *inMemVA) PerformValidation(ctx context.Context, req *vapb.PerformValidationRequest, _ ...grpc.CallOption) (*vapb.ValidationResult, error) {
return inmem.rva.PerformValidation(ctx, req)
}

func (inmem inMemVA) IsCAAValid(ctx context.Context, req *vapb.IsCAAValidRequest, _ ...grpc.CallOption) (*vapb.IsCAAValidResponse, error) {
func (inmem *inMemVA) IsCAAValid(ctx context.Context, req *vapb.IsCAAValidRequest, _ ...grpc.CallOption) (*vapb.IsCAAValidResponse, error) {
return inmem.rva.IsCAAValid(ctx, req)
}

Expand Down Expand Up @@ -333,6 +333,48 @@ func TestNewValidationAuthorityImplWithDuplicateRemotes(t *testing.T) {
test.AssertContains(t, err.Error(), "duplicate remote VA perspective \"dadaist\"")
}

func TestPerformValidationWithMismatchedRemoteVAPerspectives(t *testing.T) {
mismatched1 := RemoteVA{
RemoteClients: setupRemote(nil, "", nil, "dadaist", arin),
Perspective: "baroque",
RIR: arin,
}
mismatched2 := RemoteVA{
RemoteClients: setupRemote(nil, "", nil, "impressionist", ripe),
Perspective: "minimalist",
RIR: ripe,
}
remoteVAs := setupRemotes([]remoteConf{{rir: ripe}}, nil)
remoteVAs = append(remoteVAs, mismatched1, mismatched2)
va, mockLog := setup(nil, "", remoteVAs, nil)

req := createValidationRequest("good-dns01.com", core.ChallengeTypeDNS01)
res, _ := va.PerformValidation(context.Background(), req)
test.AssertNotNil(t, res.GetProblem(), "validation succeeded with mismatched remote VA perspectives")
test.AssertEquals(t, len(mockLog.GetAllMatching("Expected perspective")), 2)
}

func TestPerformValidationWithMismatchedRemoteVARIRs(t *testing.T) {
mismatched1 := RemoteVA{
RemoteClients: setupRemote(nil, "", nil, "dadaist", arin),
Perspective: "dadaist",
RIR: ripe,
}
mismatched2 := RemoteVA{
RemoteClients: setupRemote(nil, "", nil, "impressionist", ripe),
Perspective: "impressionist",
RIR: arin,
}
remoteVAs := setupRemotes([]remoteConf{{rir: ripe}}, nil)
remoteVAs = append(remoteVAs, mismatched1, mismatched2)
va, mockLog := setup(nil, "", remoteVAs, nil)

req := createValidationRequest("good-dns01.com", core.ChallengeTypeDNS01)
res, _ := va.PerformValidation(context.Background(), req)
test.AssertNotNil(t, res.GetProblem(), "validation succeeded with mismatched remote VA perspectives")
test.AssertEquals(t, len(mockLog.GetAllMatching("Expected perspective")), 2)
}

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

Expand Down
Loading