update database and kube name validation (#28841)

* update database name validation

* move name validation into DatabaseV3 CheckAndSetDefaults
* replace use of DNS1305 name validation
  * remove 63 char length restriction
  * remove lowercase restriction

* fix failing tests

* update more tests

* update CHANGELOG.md
* explain breaking change to database discovery name validation

CHANGELOG.md

@@ -24,6 +24,19 @@ Prior versions of Teleport would return an error from `tsh db` commands if those
 flags were empty (the only exception is Redis, where it would choose the Redis
 username "default" for `--db-user`).
 
+#### Auto-Discovered Database Names
+
+In Teleport 14, database discovery via `db_service` config will enforce the same
+name validation that we enforce for databases created via `tctl`, static config,
+and `discovery_service`.
+
+The regex used to constrain database names is: `^[a-zA-Z]([-a-zA-Z0-9]*[a-zA-Z0-9])?$`
+
+So databases in AWS and Azure must:
+1. start with a letter (uppercase and lowercase are both allowed).
+2. contain only letters, digits, and hyphens.
+3. end with a letter or digit (no trailing hyphens).
+
 ## 13.0.1 (05/xx/23)
 
 * Helm Charts

api/types/database.go

@@ -19,6 +19,7 @@ package types
 import (
 	"encoding/json"
 	"fmt"
+	"regexp"
 	"strings"
 	"time"
 
@@ -565,6 +566,21 @@ 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.
+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()
@@ -572,6 +588,10 @@ func (d *DatabaseV3) CheckAndSetDefaults() error {
 		return trace.Wrap(err)
 	}
 
+	if err := ValidateDatabaseName(d.GetName()); err != nil {
+		return trace.Wrap(err, "invalid database name")
+	}
+
 	for key := range d.Spec.DynamicLabels {
 		if !IsValidLabelKey(key) {
 			return trace.BadParameter("database %q invalid label key: %q", d.GetName(), key)

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)
+		})
+	}
+}

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
+}

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) {
+	if err := ValidateKubeClusterName(k.Metadata.Name); err != nil {
-		return trace.BadParameter("invalid kubernetes cluster name: %q", k.Metadata.Name)
+		return trace.Wrap(err, "invalid kubernetes cluster name")
 	}
 
 	if err := k.Spec.Azure.CheckAndSetDefaults(); err != nil && k.IsAzure() {

api/types/resource.go

@@ -17,6 +17,7 @@ limitations under the License.
 package types
 
 import (
+	"regexp"
 	"strings"
 	"time"
 
@@ -531,3 +532,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(),
+	)
+}

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)

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) {

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"),

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,13 +146,6 @@ 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)
-	}
-
 	if !slices.Contains(defaults.DatabaseProtocols, db.GetProtocol()) {
 		return trace.BadParameter("unsupported database %q protocol %q, supported are: %v", db.GetName(), db.GetProtocol(), defaults.DatabaseProtocols)
 	}

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{

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)
 

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"),
 		},

tool/tsh/common/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{