From 71153e17aa4a13109a63458ceccac184578e45b0 Mon Sep 17 00:00:00 2001 From: Gavin Frazar Date: Wed, 12 Jul 2023 16:08:27 -0700 Subject: [PATCH] [v13] update database and kube name validation Backport #28841 to branch/v13 --- api/types/database.go | 19 +++++++ api/types/database_test.go | 38 +++++++++++++ api/types/databaseserver_test.go | 57 ++++++++++--------- api/types/kubernetes.go | 10 +++- api/types/resource.go | 11 ++++ api/types/resource_test.go | 34 +++++------ api/types/server_test.go | 2 +- .../awsoidc/listdatabases_test.go | 3 +- lib/services/database.go | 11 ++-- lib/services/database_test.go | 13 ----- lib/srv/db/sqlserver/connect_test.go | 10 ++-- lib/srv/db/watcher_test.go | 2 +- tool/tsh/tsh_test.go | 4 +- 13 files changed, 137 insertions(+), 77 deletions(-) diff --git a/api/types/database.go b/api/types/database.go index d86c0e4a12c78..1e9621a031ac3 100644 --- a/api/types/database.go +++ b/api/types/database.go @@ -19,6 +19,7 @@ package types import ( "encoding/json" "fmt" + "regexp" "strings" "time" @@ -565,6 +566,24 @@ func (d *DatabaseV3) setStaticFields() { d.Version = V3 } +// validDatabaseNameRegexp filters the allowed characters in database names. +// This is the (almost) the same regexp used to check for valid DNS 1035 labels, +// except we allow uppercase chars. +var validDatabaseNameRegexp = regexp.MustCompile(`^[a-zA-Z]([-a-zA-Z0-9]*[a-zA-Z0-9])?$`) + +// ValidateDatabaseName returns an error if a given string is not a valid +// Database name. +// Unlike application access proxy, database name doesn't necessarily +// need to be a valid subdomain but use the same validation logic for the +// simplicity and consistency, except two differences: don't restrict names to +// 63 chars in length and allow upper case chars. +// This was added in v14 and backported, except that it's intentionally called +// in lib/services:ValidateDatabase instead of within CheckAndSetDefaults below +// for backwards compatibility. +func ValidateDatabaseName(name string) error { + return ValidateResourceName(validDatabaseNameRegexp, name) +} + // CheckAndSetDefaults checks and sets default values for any missing fields. func (d *DatabaseV3) CheckAndSetDefaults() error { d.setStaticFields() diff --git a/api/types/database_test.go b/api/types/database_test.go index 769fd05bc9607..e7ab2114fb57c 100644 --- a/api/types/database_test.go +++ b/api/types/database_test.go @@ -17,6 +17,7 @@ limitations under the License. package types import ( + "strings" "testing" "github.com/google/go-cmp/cmp" @@ -890,3 +891,40 @@ func TestAWSIsEmpty(t *testing.T) { }) } } + +func TestValidateDatabaseName(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + dbName string + expectErrContains string + }{ + { + name: "valid long name and uppercase chars", + dbName: strings.Repeat("aA", 100), + }, + { + name: "invalid trailing hyphen", + dbName: "invalid-database-name-", + expectErrContains: `"invalid-database-name-" does not match regex`, + }, + { + name: "invalid first character", + dbName: "1-invalid-database-name", + expectErrContains: `"1-invalid-database-name" does not match regex`, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + err := ValidateDatabaseName(test.dbName) + if test.expectErrContains != "" { + require.Error(t, err) + require.ErrorContains(t, err, test.expectErrContains) + return + } + require.NoError(t, err) + }) + } +} diff --git a/api/types/databaseserver_test.go b/api/types/databaseserver_test.go index 1ef66ddaf2220..e0f9cb1914663 100644 --- a/api/types/databaseserver_test.go +++ b/api/types/databaseserver_test.go @@ -108,31 +108,6 @@ func TestDatabaseServerSorter(t *testing.T) { }, } - makeServers := func(testVals []string, testField string) []DatabaseServer { - servers := make([]DatabaseServer, len(testVals)) - for i := 0; i < len(testVals); i++ { - testVal := testVals[i] - dbSpec := dbSpecs[i%len(dbSpecs)] - var err error - - servers[i], err = NewDatabaseServerV3(Metadata{ - Name: "_", - }, DatabaseServerSpecV3{ - HostID: "_", - Hostname: "_", - Database: &DatabaseV3{ - Metadata: Metadata{ - Name: getTestVal(testField == ResourceMetadataName, testVal), - Description: getTestVal(testField == ResourceSpecDescription, testVal), - }, - Spec: dbSpec, - }, - }) - require.NoError(t, err) - } - return servers - } - cases := []struct { name string wantErr bool @@ -156,7 +131,7 @@ func TestDatabaseServerSorter(t *testing.T) { c := c t.Run(fmt.Sprintf("%s desc", c.name), func(t *testing.T) { sortBy := SortBy{Field: c.fieldName, IsDesc: true} - servers := DatabaseServers(makeServers(testValsUnordered, c.fieldName)) + servers := DatabaseServers(makeServers(t, testValsUnordered, dbSpecs, c.fieldName)) require.NoError(t, servers.SortByCustom(sortBy)) targetVals, err := servers.GetFieldVals(c.fieldName) require.NoError(t, err) @@ -165,7 +140,7 @@ func TestDatabaseServerSorter(t *testing.T) { t.Run(fmt.Sprintf("%s asc", c.name), func(t *testing.T) { sortBy := SortBy{Field: c.fieldName} - servers := DatabaseServers(makeServers(testValsUnordered, c.fieldName)) + servers := DatabaseServers(makeServers(t, testValsUnordered, dbSpecs, c.fieldName)) require.NoError(t, servers.SortByCustom(sortBy)) targetVals, err := servers.GetFieldVals(c.fieldName) require.NoError(t, err) @@ -175,6 +150,32 @@ func TestDatabaseServerSorter(t *testing.T) { // Test error. sortBy := SortBy{Field: "unsupported"} - servers := makeServers(testValsUnordered, "does-not-matter") + servers := makeServers(t, testValsUnordered, dbSpecs, "does-not-matter") require.True(t, trace.IsNotImplemented(DatabaseServers(servers).SortByCustom(sortBy))) } + +func makeServers(t *testing.T, testVals []string, dbSpecs []DatabaseSpecV3, testField string) []DatabaseServer { + t.Helper() + servers := make([]DatabaseServer, len(testVals)) + for i := 0; i < len(testVals); i++ { + testVal := testVals[i] + dbSpec := dbSpecs[i%len(dbSpecs)] + var err error + + servers[i], err = NewDatabaseServerV3(Metadata{ + Name: "foo", + }, DatabaseServerSpecV3{ + HostID: "_", + Hostname: "_", + Database: &DatabaseV3{ + Metadata: Metadata{ + Name: getTestVal(testField == ResourceMetadataName, testVal), + Description: getTestVal(testField == ResourceSpecDescription, testVal), + }, + Spec: dbSpec, + }, + }) + require.NoError(t, err) + } + return servers +} diff --git a/api/types/kubernetes.go b/api/types/kubernetes.go index 747423b27bf13..ec00f1c5e3955 100644 --- a/api/types/kubernetes.go +++ b/api/types/kubernetes.go @@ -334,6 +334,12 @@ func (k *KubernetesClusterV3) setStaticFields() { // sneaky cluster names being used for client directory traversal and exploits. var validKubeClusterName = regexp.MustCompile(`^[a-zA-Z0-9._-]+$`) +// ValidateKubeClusterName returns an error if a given string is not a valid +// KubeCluster name. +func ValidateKubeClusterName(name string) error { + return ValidateResourceName(validKubeClusterName, name) +} + // CheckAndSetDefaults checks and sets default values for any missing fields. func (k *KubernetesClusterV3) CheckAndSetDefaults() error { k.setStaticFields() @@ -346,8 +352,8 @@ func (k *KubernetesClusterV3) CheckAndSetDefaults() error { } } - if !validKubeClusterName.MatchString(k.Metadata.Name) { - return trace.BadParameter("invalid kubernetes cluster name: %q", k.Metadata.Name) + if err := ValidateKubeClusterName(k.Metadata.Name); err != nil { + return trace.Wrap(err, "invalid kubernetes cluster name") } if err := k.Spec.Azure.CheckAndSetDefaults(); err != nil && k.IsAzure() { diff --git a/api/types/resource.go b/api/types/resource.go index c2eb51e0622b7..1c73bcc439433 100644 --- a/api/types/resource.go +++ b/api/types/resource.go @@ -524,3 +524,14 @@ type ListResourcesResponse struct { // TotalCount is the total number of resources available as a whole. TotalCount int } + +// ValidateResourceName validates a resource name using a given regexp. +func ValidateResourceName(validationRegex *regexp.Regexp, name string) error { + if validationRegex.MatchString(name) { + return nil + } + return trace.BadParameter( + "%q does not match regex used for validation %q", + name, validationRegex.String(), + ) +} diff --git a/api/types/resource_test.go b/api/types/resource_test.go index 661ee1783a45e..0c8a7a9bc81d9 100644 --- a/api/types/resource_test.go +++ b/api/types/resource_test.go @@ -128,12 +128,12 @@ func TestMatchSearch_ResourceSpecific(t *testing.T) { // searchNotDefined refers to resources where the searcheable field values are not defined. searchNotDefined bool matchingSearchVals []string - newResource func() ResourceWithLabels + newResource func(*testing.T) ResourceWithLabels }{ { name: "node", matchingSearchVals: []string{"foo", "bar", "prod", "os"}, - newResource: func() ResourceWithLabels { + newResource: func(t *testing.T) ResourceWithLabels { server, err := NewServerWithLabels("_", KindNode, ServerSpecV2{ Hostname: "foo", Addr: "bar", @@ -146,7 +146,7 @@ func TestMatchSearch_ResourceSpecific(t *testing.T) { { name: "node using tunnel", matchingSearchVals: []string{"tunnel"}, - newResource: func() ResourceWithLabels { + newResource: func(t *testing.T) ResourceWithLabels { server, err := NewServer("_", KindNode, ServerSpecV2{ UseTunnel: true, }) @@ -158,7 +158,7 @@ func TestMatchSearch_ResourceSpecific(t *testing.T) { { name: "windows desktop", matchingSearchVals: []string{"foo", "bar", "env", "prod", "os"}, - newResource: func() ResourceWithLabels { + newResource: func(t *testing.T) ResourceWithLabels { desktop, err := NewWindowsDesktopV3("foo", labels, WindowsDesktopSpecV3{ Addr: "bar", }) @@ -170,7 +170,7 @@ func TestMatchSearch_ResourceSpecific(t *testing.T) { { name: "application", matchingSearchVals: []string{"foo", "bar", "baz", "mac"}, - newResource: func() ResourceWithLabels { + newResource: func(t *testing.T) ResourceWithLabels { app, err := NewAppV3(Metadata{ Name: "foo", Description: "bar", @@ -187,7 +187,7 @@ func TestMatchSearch_ResourceSpecific(t *testing.T) { { name: "kube cluster", matchingSearchVals: []string{"foo", "prod", "env"}, - newResource: func() ResourceWithLabels { + newResource: func(t *testing.T) ResourceWithLabels { kc, err := NewKubernetesClusterV3FromLegacyCluster("_", &KubernetesCluster{ Name: "foo", StaticLabels: labels, @@ -200,7 +200,7 @@ func TestMatchSearch_ResourceSpecific(t *testing.T) { { name: "database", matchingSearchVals: []string{"foo", "bar", "baz", "prod", DatabaseTypeRedshift}, - newResource: func() ResourceWithLabels { + newResource: func(t *testing.T) ResourceWithLabels { db, err := NewDatabaseV3(Metadata{ Name: "foo", Description: "bar", @@ -222,9 +222,9 @@ func TestMatchSearch_ResourceSpecific(t *testing.T) { { name: "database with gcp keywords", matchingSearchVals: []string{"cloud", "cloud sql"}, - newResource: func() ResourceWithLabels { + newResource: func(t *testing.T) ResourceWithLabels { db, err := NewDatabaseV3(Metadata{ - Name: "_", + Name: "foo", Labels: labels, }, DatabaseSpecV3{ Protocol: "_", @@ -242,9 +242,9 @@ func TestMatchSearch_ResourceSpecific(t *testing.T) { { name: "app server", searchNotDefined: true, - newResource: func() ResourceWithLabels { + newResource: func(t *testing.T) ResourceWithLabels { appServer, err := NewAppServerV3(Metadata{ - Name: "_", + Name: "foo", }, AppServerSpecV3{ HostID: "_", App: &AppV3{Metadata: Metadata{Name: "_"}, Spec: AppSpecV3{URI: "_"}}, @@ -257,9 +257,9 @@ func TestMatchSearch_ResourceSpecific(t *testing.T) { { name: "db server", searchNotDefined: true, - newResource: func() ResourceWithLabels { + newResource: func(t *testing.T) ResourceWithLabels { dbServer, err := NewDatabaseServerV3(Metadata{ - Name: "_", + Name: "foo", }, DatabaseServerSpecV3{ HostID: "_", Hostname: "_", @@ -272,10 +272,10 @@ func TestMatchSearch_ResourceSpecific(t *testing.T) { { name: "kube server", searchNotDefined: true, - newResource: func() ResourceWithLabels { + newResource: func(t *testing.T) ResourceWithLabels { kubeServer, err := NewKubernetesServerV3( Metadata{ - Name: "_", + Name: "foo", }, KubernetesServerSpecV3{ HostID: "_", Hostname: "_", @@ -293,7 +293,7 @@ func TestMatchSearch_ResourceSpecific(t *testing.T) { { name: "desktop service", searchNotDefined: true, - newResource: func() ResourceWithLabels { + newResource: func(t *testing.T) ResourceWithLabels { desktopService, err := NewWindowsDesktopServiceV3(Metadata{ Name: "foo", }, WindowsDesktopServiceSpecV3{ @@ -312,7 +312,7 @@ func TestMatchSearch_ResourceSpecific(t *testing.T) { t.Run(tc.name, func(t *testing.T) { t.Parallel() - resource := tc.newResource() + resource := tc.newResource(t) // Nil search values, should always return true match := resource.MatchSearch(nil) diff --git a/api/types/server_test.go b/api/types/server_test.go index e241fc915ad78..28782516248d0 100644 --- a/api/types/server_test.go +++ b/api/types/server_test.go @@ -31,7 +31,7 @@ func getTestVal(isTestField bool, testVal string) string { return testVal } - return "_" + return "foo" } func TestServerSorter(t *testing.T) { diff --git a/lib/integrations/awsoidc/listdatabases_test.go b/lib/integrations/awsoidc/listdatabases_test.go index 568606fb0b7fe..5438b66d8ad4f 100644 --- a/lib/integrations/awsoidc/listdatabases_test.go +++ b/lib/integrations/awsoidc/listdatabases_test.go @@ -24,7 +24,6 @@ import ( "github.com/aws/aws-sdk-go-v2/service/rds" rdsTypes "github.com/aws/aws-sdk-go-v2/service/rds/types" - "github.com/google/uuid" "github.com/gravitational/trace" "github.com/stretchr/testify/require" @@ -99,7 +98,7 @@ func TestListDatabases(t *testing.T) { for i := 0; i < totalDBs; i++ { allInstances = append(allInstances, rdsTypes.DBInstance{ DBInstanceStatus: stringPointer("available"), - DBInstanceIdentifier: stringPointer(uuid.NewString()), + DBInstanceIdentifier: stringPointer(fmt.Sprintf("db-%v", i)), DbiResourceId: stringPointer("db-123"), DBInstanceArn: stringPointer("arn:aws:iam::123456789012:role/MyARN"), Engine: stringPointer("postgres"), diff --git a/lib/services/database.go b/lib/services/database.go index 43a864ab1ecdc..cef7b755ac772 100644 --- a/lib/services/database.go +++ b/lib/services/database.go @@ -44,7 +44,6 @@ import ( "go.mongodb.org/mongo-driver/mongo/readpref" "go.mongodb.org/mongo-driver/x/mongo/driver/connstring" "golang.org/x/exp/slices" - "k8s.io/apimachinery/pkg/util/validation" "github.com/gravitational/teleport/api/types" apiawsutils "github.com/gravitational/teleport/api/utils/aws" @@ -147,11 +146,11 @@ func ValidateDatabase(db types.Database) error { return trace.Wrap(err) } - // Unlike application access proxy, database proxy name doesn't necessarily - // need to be a valid subdomain but use the same validation logic for the - // simplicity and consistency. - if errs := validation.IsDNS1035Label(db.GetName()); len(errs) > 0 { - return trace.BadParameter("invalid database %q name: %v", db.GetName(), errs) + // This was added in v14 and backported, except that it's intentionally + // called here instead of in CheckAndSetDefaults for backwards + // compatibility. + if err := types.ValidateDatabaseName(db.GetName()); err != nil { + return trace.Wrap(err, "invalid database name") } if !slices.Contains(defaults.DatabaseProtocols, db.GetProtocol()) { diff --git a/lib/services/database_test.go b/lib/services/database_test.go index 70eb49851da31..2aa9a8ad21983 100644 --- a/lib/services/database_test.go +++ b/lib/services/database_test.go @@ -122,19 +122,6 @@ func TestValidateDatabase(t *testing.T) { inputSpec types.DatabaseSpecV3 expectError bool }{ - { - // Captured error: - // a DNS-1035 label must consist of lower case alphanumeric - // characters or '-', start with an alphabetic character, and end - // with an alphanumeric character (e.g. 'my-name', or 'abc-123', - // regex used for validation is '[a-z]([-a-z0-9]*[a-z0-9])?') - inputName: "invalid-database-name-", - inputSpec: types.DatabaseSpecV3{ - Protocol: defaults.ProtocolPostgres, - URI: "localhost:5432", - }, - expectError: true, - }, { inputName: "invalid-database-protocol", inputSpec: types.DatabaseSpecV3{ diff --git a/lib/srv/db/sqlserver/connect_test.go b/lib/srv/db/sqlserver/connect_test.go index 7c77d53d5b5fe..9491ab7d21fee 100644 --- a/lib/srv/db/sqlserver/connect_test.go +++ b/lib/srv/db/sqlserver/connect_test.go @@ -18,13 +18,13 @@ import ( "context" _ "embed" "errors" + "fmt" "os" "os/exec" "path/filepath" "testing" "time" - "github.com/google/uuid" "github.com/stretchr/testify/require" "github.com/gravitational/teleport/api/client/proto" @@ -48,7 +48,7 @@ func TestConnectorSelection(t *testing.T) { connector := &connector{DBAuth: &mockDBAuth{}} - for _, tt := range []struct { + for i, tt := range []struct { desc string databaseSpec types.DatabaseSpecV3 errAssertion require.ErrorAssertionFunc @@ -118,7 +118,7 @@ func TestConnectorSelection(t *testing.T) { } { t.Run(tt.desc, func(t *testing.T) { database, err := types.NewDatabaseV3(types.Metadata{ - Name: uuid.NewString(), + Name: fmt.Sprintf("db-%v", i), }, tt.databaseSpec) require.NoError(t, err) @@ -240,7 +240,7 @@ func TestConnectorKInitClient(t *testing.T) { err := os.WriteFile(krbConfPath, []byte(krb5Conf), 0664) require.NoError(t, err) - for _, tt := range []struct { + for i, tt := range []struct { desc string databaseSpec types.DatabaseSpecV3 errAssertion require.ErrorAssertionFunc @@ -301,7 +301,7 @@ func TestConnectorKInitClient(t *testing.T) { } { t.Run(tt.desc, func(t *testing.T) { database, err := types.NewDatabaseV3(types.Metadata{ - Name: uuid.NewString(), + Name: fmt.Sprintf("db-%v", i), }, tt.databaseSpec) require.NoError(t, err) diff --git a/lib/srv/db/watcher_test.go b/lib/srv/db/watcher_test.go index 4bdde2818f9f0..93e09f63d117e 100644 --- a/lib/srv/db/watcher_test.go +++ b/lib/srv/db/watcher_test.go @@ -368,7 +368,7 @@ func makeAzureSQLServer(t *testing.T, name, group string) (*armsql.Server, types server := &armsql.Server{ ID: to.Ptr(fmt.Sprintf("/subscriptions/sub-id/resourceGroups/%v/providers/Microsoft.Sql/servers/%v", group, name)), - Name: to.Ptr(fmt.Sprintf("%s.database.windows.net", name)), + Name: to.Ptr(fmt.Sprintf("%s-database-windows-net", name)), Properties: &armsql.ServerProperties{ FullyQualifiedDomainName: to.Ptr("localhost"), }, diff --git a/tool/tsh/tsh_test.go b/tool/tsh/tsh_test.go index c82bb43a9441e..d2de83c406058 100644 --- a/tool/tsh/tsh_test.go +++ b/tool/tsh/tsh_test.go @@ -3512,7 +3512,7 @@ func TestSerializeDatabases(t *testing.T) { "kind": "db", "version": "v3", "metadata": { - "name": "my db", + "name": "my-db", "description": "this is the description", "labels": {"a": "1", "b": "2"} }, @@ -3566,7 +3566,7 @@ func TestSerializeDatabases(t *testing.T) { }] ` db, err := types.NewDatabaseV3(types.Metadata{ - Name: "my db", + Name: "my-db", Description: "this is the description", Labels: map[string]string{"a": "1", "b": "2"}, }, types.DatabaseSpecV3{