From 19a46d6ca40d5b1b4cbf43fe90d1c18309b97052 Mon Sep 17 00:00:00 2001 From: Derek Menteer <105233703+hashi-derek@users.noreply.github.com> Date: Fri, 13 Jan 2023 14:20:28 -0600 Subject: [PATCH] Enforce lowercase peer names. (#15697) Enforce lowercase peer names. Prior to this change peer names could be mixed case. This can cause issues, as peer names are used as DNS labels in various locations. It also caused issues with envoy configuration. --- .changelog/15697.txt | 3 +++ agent/rpc/peering/service.go | 5 ++--- agent/rpc/peering/service_test.go | 24 ++++++++++++------------ agent/rpc/peering/validate.go | 18 ++++++++++++++++++ 4 files changed, 35 insertions(+), 15 deletions(-) create mode 100644 .changelog/15697.txt diff --git a/.changelog/15697.txt b/.changelog/15697.txt new file mode 100644 index 0000000000..bf2bfed840 --- /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 37a316410e..9a6b1adaa5 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 207709ac1f..833614559d 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 0f00978bc9..945a6a0dc2 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