diff --git a/changelog/16.0/16.0.1/summary.md b/changelog/16.0/16.0.1/summary.md index e2a393310ba..05381d735bc 100644 --- a/changelog/16.0/16.0.1/summary.md +++ b/changelog/16.0/16.0.1/summary.md @@ -7,3 +7,9 @@ Below is a summary of this Go patch release. You can learn more [here](https://g > go1.20.2 (released 2023-03-07) includes a security fix to the crypto/elliptic package, as well as bug fixes to the compiler, the covdata command, the linker, the runtime, and the crypto/ecdh, crypto/rsa, crypto/x509, os, and syscall packages. +### Keyspace name validation in TopoServer + +Prior to v16.0.1, it was possible to create a keyspace with invalid characters, which would then be inaccessible to various cluster management operations. + + Keyspace names may no longer contain the forward slash ("/") character, and TopoServer's `GetKeyspace` and `CreateKeyspace` methods return an error if given such a name. + diff --git a/go/vt/topo/keyspace.go b/go/vt/topo/keyspace.go index aeacacb232f..3a0416c9032 100755 --- a/go/vt/topo/keyspace.go +++ b/go/vt/topo/keyspace.go @@ -18,6 +18,7 @@ package topo import ( "path" + "strings" "google.golang.org/protobuf/proto" @@ -54,6 +55,20 @@ func (ki *KeyspaceInfo) SetKeyspaceName(name string) { ki.keyspace = name } +var invalidKeyspaceNameChars = "/" + +// ValidateKeyspaceName checks if the provided name is a valid name for a +// keyspace. +// +// As of v16.0.1, "all invalid characters" is just the forward slash ("/"). +func ValidateKeyspaceName(name string) error { + if strings.ContainsAny(name, invalidKeyspaceNameChars) { + return vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "keyspace name %s contains invalid characters; may not contain any of the following: %+v", name, strings.Split(invalidKeyspaceNameChars, "")) + } + + return nil +} + // GetServedFrom returns a Keyspace_ServedFrom record if it exists. func (ki *KeyspaceInfo) GetServedFrom(tabletType topodatapb.TabletType) *topodatapb.Keyspace_ServedFrom { for _, ksf := range ki.ServedFroms { @@ -161,6 +176,10 @@ func (ki *KeyspaceInfo) ComputeCellServedFrom(cell string) []*topodatapb.SrvKeys // CreateKeyspace wraps the underlying Conn.Create // and dispatches the event. func (ts *Server) CreateKeyspace(ctx context.Context, keyspace string, value *topodatapb.Keyspace) error { + if err := ValidateKeyspaceName(keyspace); err != nil { + return vterrors.Wrapf(err, "CreateKeyspace: %s", err) + } + data, err := proto.Marshal(value) if err != nil { return err @@ -181,6 +200,10 @@ func (ts *Server) CreateKeyspace(ctx context.Context, keyspace string, value *to // GetKeyspace reads the given keyspace and returns it func (ts *Server) GetKeyspace(ctx context.Context, keyspace string) (*KeyspaceInfo, error) { + if err := ValidateKeyspaceName(keyspace); err != nil { + return nil, vterrors.Wrapf(err, "GetKeyspace: %s", err) + } + keyspacePath := path.Join(KeyspacesPath, keyspace, KeyspaceFile) data, version, err := ts.globalCell.Get(ctx, keyspacePath) if err != nil { diff --git a/go/vt/topo/topotests/keyspace_test.go b/go/vt/topo/topotests/keyspace_test.go new file mode 100644 index 00000000000..b0b35c1421c --- /dev/null +++ b/go/vt/topo/topotests/keyspace_test.go @@ -0,0 +1,72 @@ +/* +Copyright 2023 The Vitess Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package topotests + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "vitess.io/vitess/go/vt/topo/memorytopo" + "vitess.io/vitess/go/vt/vterrors" + + topodatapb "vitess.io/vitess/go/vt/proto/topodata" + "vitess.io/vitess/go/vt/proto/vtrpc" +) + +func TestCreateKeyspace(t *testing.T) { + ts := memorytopo.NewServer("zone1") + ctx := context.Background() + + t.Run("valid name", func(t *testing.T) { + err := ts.CreateKeyspace(ctx, "ks", &topodatapb.Keyspace{}) + require.NoError(t, err) + }) + t.Run("invalid name", func(t *testing.T) { + err := ts.CreateKeyspace(ctx, "no/slashes/allowed", &topodatapb.Keyspace{}) + assert.Error(t, err) + assert.Equal(t, vtrpc.Code_INVALID_ARGUMENT, vterrors.Code(err), "%+v", err) + }) +} + +func TestGetKeyspace(t *testing.T) { + ts := memorytopo.NewServer("zone1") + ctx := context.Background() + + t.Run("valid name", func(t *testing.T) { + // First, create the keyspace. + err := ts.CreateKeyspace(ctx, "ks", &topodatapb.Keyspace{}) + require.NoError(t, err) + + // Now, get it. + ks, err := ts.GetKeyspace(ctx, "ks") + require.NoError(t, err) + assert.NotNil(t, ks) + }) + + t.Run("invalid name", func(t *testing.T) { + // We can't create the keyspace (because we can't create a keyspace + // with an invalid name), so we'll validate the error we get is *not* + // NOT_FOUND. + ks, err := ts.GetKeyspace(ctx, "no/slashes/allowed") + assert.Error(t, err) + assert.Equal(t, vtrpc.Code_INVALID_ARGUMENT, vterrors.Code(err), "%+v", err) + assert.Nil(t, ks) + }) +} diff --git a/go/vt/vtorc/inst/keyspace_dao.go b/go/vt/vtorc/inst/keyspace_dao.go index 7e55471854d..fe8d37af575 100644 --- a/go/vt/vtorc/inst/keyspace_dao.go +++ b/go/vt/vtorc/inst/keyspace_dao.go @@ -30,6 +30,10 @@ var ErrKeyspaceNotFound = errors.New("keyspace not found") // ReadKeyspace reads the vitess keyspace record. func ReadKeyspace(keyspaceName string) (*topo.KeyspaceInfo, error) { + if err := topo.ValidateKeyspaceName(keyspaceName); err != nil { + return nil, err + } + query := ` select keyspace_type,