Skip to content

Commit

Permalink
va: Require RIR and Perspective and check for mismatches at runtime
Browse files Browse the repository at this point in the history
  • Loading branch information
beautifulentropy committed Nov 22, 2024
1 parent 2c1421f commit 852b3fb
Show file tree
Hide file tree
Showing 8 changed files with 75 additions and 20 deletions.
8 changes: 2 additions & 6 deletions cmd/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -457,9 +457,7 @@ type RemoteVAGRPCClientConfig struct {
// Requirement 2.7 ("Multi-Perspective Issuance Corroboration attempts
// from each Network Perspective"). It should uniquely identify a group
// of RVAs deployed in the same datacenter.
//
// TODO(#7615): Make mandatory.
Perspective string `validate:"omitempty"`
Perspective string `validate:"required"`

// 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 @@ -471,9 +469,7 @@ type RemoteVAGRPCClientConfig struct {
// - APNIC
// - LACNIC
// - AfriNIC
//
// TODO(#7615): Make mandatory.
RIR string `validate:"omitempty,oneof=ARIN RIPE APNIC LACNIC AfriNIC"`
RIR string `validate:"required,oneof=ARIN RIPE APNIC LACNIC AfriNIC"`
}

// GRPCServerConfig contains the information needed to start a gRPC server.
Expand Down
8 changes: 2 additions & 6 deletions cmd/remoteva/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,7 @@ type Config struct {
// Requirement 2.7 ("Multi-Perspective Issuance Corroboration attempts
// from each Network Perspective"). It should uniquely identify a group
// of RVAs deployed in the same datacenter.
//
// TODO(#7615): Make mandatory.
Perspective string `validate:"omitempty"`
Perspective string `validate:"required"`

// 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 @@ -39,9 +37,7 @@ type Config struct {
// - APNIC
// - LACNIC
// - AfriNIC
//
// TODO(#7615): Make mandatory.
RIR string `validate:"omitempty,oneof=ARIN RIPE APNIC LACNIC AfriNIC"`
RIR string `validate:"required,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
13 changes: 11 additions & 2 deletions va/va.go
Original file line number Diff line number Diff line change
Expand Up @@ -239,8 +239,7 @@ func NewValidationAuthorityImpl(

for i, va1 := range remoteVAs {
for j, va2 := range remoteVAs {
// TODO(#7819): Remove the != "" check once perspective is required.
if i != j && va1.Perspective == va2.Perspective && va1.Perspective != "" {
if i != j && va1.Perspective == va2.Perspective {
return nil, fmt.Errorf("duplicate remote VA perspective %q", va1.Perspective)
}
}
Expand Down Expand Up @@ -481,6 +480,16 @@ func (va *ValidationAuthorityImpl) performRemoteValidation(
for _, i := range rand.Perm(remoteVACount) {
go func(rva RemoteVA) {
res, err := rva.PerformValidation(subCtx, req)
if res == nil {
responses <- &response{rva.Address, rva.Perspective, rva.RIR, res, err}
return
}
if res.Perspective != rva.Perspective || res.Rir != rva.RIR {
err = fmt.Errorf(
"Remote VA %q.PerformValidation result included mismatched Perspective remote=[%q] local=[%q] and/or RIR remote=[%q] local=[%q]",
rva.Perspective, res.Perspective, rva.Perspective, res.Rir, rva.RIR,
)
}
responses <- &response{rva.Address, rva.Perspective, rva.RIR, res, err}
}(va.remoteVAs[i])
}
Expand Down
42 changes: 42 additions & 0 deletions va/va_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,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.Problems, "validation succeeded with mismatched remote VA perspectives")
test.AssertEquals(t, len(mockLog.GetAllMatching("result included mismatched")), 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.Problems, "validation succeeded with mismatched remote VA perspectives")
test.AssertEquals(t, len(mockLog.GetAllMatching("result included mismatched")), 2)
}

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

Expand Down

0 comments on commit 852b3fb

Please sign in to comment.