diff --git a/.changelog/15697.txt b/.changelog/15697.txt new file mode 100644 index 000000000..bf2bfed84 --- /dev/null +++ b/.changelog/15697.txt @@ -0,0 +1,3 @@ +```release-note:breaking-change +peering: Newly created peering connections must use only lowercase characters in the `name` field. Existing peerings with uppercase characters will not be modified, but they may encounter issues in various circumstances. To maintain forward compatibility and avoid issues, it is recommended to destroy and re-create any invalid peering connections so that they do not have a name containing uppercase characters. +``` diff --git a/agent/rpc/peering/service.go b/agent/rpc/peering/service.go index 37a316410..9a6b1adaa 100644 --- a/agent/rpc/peering/service.go +++ b/agent/rpc/peering/service.go @@ -24,7 +24,6 @@ import ( "github.com/hashicorp/consul/acl/resolver" "github.com/hashicorp/consul/agent/consul/state" "github.com/hashicorp/consul/agent/consul/stream" - "github.com/hashicorp/consul/agent/dns" external "github.com/hashicorp/consul/agent/grpc-external" "github.com/hashicorp/consul/agent/grpc-external/services/peerstream" "github.com/hashicorp/consul/agent/structs" @@ -203,7 +202,7 @@ func (s *Server) GenerateToken( return nil, grpcstatus.Error(codes.InvalidArgument, err.Error()) } // validate prior to forwarding to the leader, this saves a network hop - if err := dns.ValidateLabel(req.PeerName); err != nil { + if err := validatePeerName(req.PeerName); err != nil { return nil, fmt.Errorf("%s is not a valid peer name: %w", req.PeerName, err) } @@ -351,7 +350,7 @@ func (s *Server) Establish( } // validate prior to forwarding to the leader, this saves a network hop - if err := dns.ValidateLabel(req.PeerName); err != nil { + if err := validatePeerName(req.PeerName); err != nil { return nil, fmt.Errorf("%s is not a valid peer name: %w", req.PeerName, err) } tok, err := s.Backend.DecodeToken([]byte(req.PeeringToken)) diff --git a/agent/rpc/peering/service_test.go b/agent/rpc/peering/service_test.go index 207709ac1..833614559 100644 --- a/agent/rpc/peering/service_test.go +++ b/agent/rpc/peering/service_test.go @@ -85,7 +85,7 @@ func TestPeeringService_GenerateToken(t *testing.T) { // TODO(peering): for more failure cases, consider using a table test // check meta tags - reqE := pbpeering.GenerateTokenRequest{PeerName: "peerB", Meta: generateTooManyMetaKeys()} + reqE := pbpeering.GenerateTokenRequest{PeerName: "peer-b", Meta: generateTooManyMetaKeys()} _, errE := client.GenerateToken(ctx, &reqE) require.EqualError(t, errE, "rpc error: code = Unknown desc = meta tags failed validation: Node metadata cannot contain more than 64 key/value pairs") @@ -95,7 +95,7 @@ func TestPeeringService_GenerateToken(t *testing.T) { ) testutil.RunStep(t, "peering token is generated with data", func(t *testing.T) { req := pbpeering.GenerateTokenRequest{ - PeerName: "peerB", + PeerName: "peer-b", Meta: map[string]string{"foo": "bar"}, } resp, err := client.GenerateToken(ctx, &req) @@ -135,7 +135,7 @@ func TestPeeringService_GenerateToken(t *testing.T) { peers[0].CreateIndex = 0 expect := &pbpeering.Peering{ - Name: "peerB", + Name: "peer-b", Partition: acl.DefaultPartitionName, ID: peerID, State: pbpeering.PeeringState_PENDING, @@ -153,7 +153,7 @@ func TestPeeringService_GenerateToken(t *testing.T) { }) testutil.RunStep(t, "re-generating a peering token re-generates the secret", func(t *testing.T) { - req := pbpeering.GenerateTokenRequest{PeerName: "peerB", Meta: map[string]string{"foo": "bar"}} + req := pbpeering.GenerateTokenRequest{PeerName: "peer-b", Meta: map[string]string{"foo": "bar"}} resp, err := client.GenerateToken(ctx, &req) require.NoError(t, err) @@ -197,7 +197,7 @@ func TestPeeringService_GenerateTokenExternalAddress(t *testing.T) { externalAddresses := []string{"32.1.2.3:8502"} // happy path - req := pbpeering.GenerateTokenRequest{PeerName: "peerB", Meta: map[string]string{"foo": "bar"}, ServerExternalAddresses: externalAddresses} + req := pbpeering.GenerateTokenRequest{PeerName: "peer-b", Meta: map[string]string{"foo": "bar"}, ServerExternalAddresses: externalAddresses} resp, err := client.GenerateToken(ctx, &req) require.NoError(t, err) @@ -396,7 +396,7 @@ func TestPeeringService_Establish_serverNameConflict(t *testing.T) { base64Token := base64.StdEncoding.EncodeToString(jsonToken) establishReq := &pbpeering.EstablishRequest{ - PeerName: "peerTwo", + PeerName: "peer-two", PeeringToken: base64Token, } @@ -1304,7 +1304,7 @@ func TestPeeringService_validatePeer(t *testing.T) { t.Cleanup(cancel) testutil.RunStep(t, "generate a token", func(t *testing.T) { - req := pbpeering.GenerateTokenRequest{PeerName: "peerB"} + req := pbpeering.GenerateTokenRequest{PeerName: "peer-b"} resp, err := client1.GenerateToken(ctx, &req) require.NoError(t, err) require.NotEmpty(t, resp) @@ -1325,7 +1325,7 @@ func TestPeeringService_validatePeer(t *testing.T) { testutil.RunStep(t, "send an establish request for a different peer name", func(t *testing.T) { resp, err := client1.Establish(ctx, &pbpeering.EstablishRequest{ - PeerName: "peerC", + PeerName: "peer-c", PeeringToken: s2Token, }) require.NoError(t, err) @@ -1333,24 +1333,24 @@ func TestPeeringService_validatePeer(t *testing.T) { }) testutil.RunStep(t, "attempt to generate token with the same name used as dialer", func(t *testing.T) { - req := pbpeering.GenerateTokenRequest{PeerName: "peerC"} + req := pbpeering.GenerateTokenRequest{PeerName: "peer-c"} resp, err := client1.GenerateToken(ctx, &req) require.Error(t, err) require.Contains(t, err.Error(), - "cannot create peering with name: \"peerC\"; there is already an established peering") + "cannot create peering with name: \"peer-c\"; there is already an established peering") require.Nil(t, resp) }) testutil.RunStep(t, "attempt to establish the with the same name used as acceptor", func(t *testing.T) { resp, err := client1.Establish(ctx, &pbpeering.EstablishRequest{ - PeerName: "peerB", + PeerName: "peer-b", PeeringToken: s2Token, }) require.Error(t, err) require.Contains(t, err.Error(), - "cannot create peering with name: \"peerB\"; there is an existing peering expecting to be dialed") + "cannot create peering with name: \"peer-b\"; there is an existing peering expecting to be dialed") require.Nil(t, resp) }) } diff --git a/agent/rpc/peering/validate.go b/agent/rpc/peering/validate.go index 0f00978bc..945a6a0dc 100644 --- a/agent/rpc/peering/validate.go +++ b/agent/rpc/peering/validate.go @@ -1,14 +1,32 @@ package peering import ( + "errors" "fmt" "net" + "regexp" "strconv" "github.com/hashicorp/consul/agent/connect" "github.com/hashicorp/consul/agent/structs" ) +// matches valid DNS labels according to RFC 1123, should be at most 63 +// characters according to the RFC. This does not allow uppercase letters, unlike +// node / service validation. All lowercase is enforced to reduce potential issues +// relating to case-mismatch throughout the codebase (state-store lookups, +// envoy listeners, etc). +var validPeeringName = regexp.MustCompile(`^[a-z0-9]([a-z0-9\-]{0,61}[a-z0-9])?$`) + +// validatePeerName returns an error if the peer name does not match +// the expected format. Returns nil on valid names. +func validatePeerName(name string) error { + if !validPeeringName.MatchString(name) { + return errors.New("a valid peering name must consist of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character") + } + return nil +} + // validatePeeringToken ensures that the token has valid values. func validatePeeringToken(tok *structs.PeeringToken) error { // the CA values here should be valid x509 certs