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 11 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
10 changes: 3 additions & 7 deletions cmd/boulder-va/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,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 @@ -43,10 +41,8 @@ type RemoteVAGRPCClientConfig struct {
// - RIPE
// - APNIC
// - LACNIC
// - AfriNIC
//
// TODO(#7615): Make mandatory.
RIR string `validate:"omitempty,oneof=ARIN RIPE APNIC LACNIC AfriNIC"`
// - AFRINIC
RIR string `validate:"required,oneof=ARIN RIPE APNIC LACNIC AFRINIC"`
}

type Config struct {
Expand Down
10 changes: 3 additions & 7 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 @@ -38,10 +36,8 @@ type Config struct {
// - RIPE
// - APNIC
// - LACNIC
// - AfriNIC
//
// TODO(#7615): Make mandatory.
RIR string `validate:"omitempty,oneof=ARIN RIPE APNIC LACNIC AfriNIC"`
// - 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
14 changes: 12 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,17 @@ func (va *ValidationAuthorityImpl) performRemoteValidation(
for _, i := range rand.Perm(remoteVACount) {
go func(rva RemoteVA) {
res, err := rva.PerformValidation(subCtx, req)
if err != nil {
responses <- &response{rva.Address, rva.Perspective, rva.RIR, res, err}
return
}
if res.Perspective != rva.Perspective || res.Rir != rva.RIR {
err = fmt.Errorf(
"Expected perspective %q (%q) but got reply from %q (%q) - misconfiguration likely", rva.Perspective, rva.RIR, res.Perspective, res.Rir,
)
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
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("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.Problems, "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