Forbid server_info-based labels in deny rules

api/types/constants.go

@@ -836,6 +836,10 @@ const (
 	// See also TeleportNamespace and TeleportInternalLabelPrefix.
 	TeleportHiddenLabelPrefix = "teleport.hidden/"
 
+	// TeleportDynamicLabelPrefix is the prefix used by labels that can change
+	// over time and should not be used as part of a role's deny rules.
+	TeleportDynamicLabelPrefix = "dynamic/"
+
 	// DiscoveredNameLabel is a resource metadata label name used to identify
 	// the discovered name of a resource, i.e. the name of a resource before a
 	// uniquely distinguishing suffix is added by the discovery service.

api/types/server_info.go

@@ -18,6 +18,7 @@ package types
 
 import (
 	"fmt"
+	"strings"
 	"time"
 
 	"github.com/gravitational/trace"
@@ -161,6 +162,36 @@ func (s *ServerInfoV1) GetNewLabels() map[string]string {
 // SetNewLabels sets the labels to apply to matched Nodes.
 func (s *ServerInfoV1) SetNewLabels(labels map[string]string) {
 	s.Spec.NewLabels = labels
+	s.fixLabels()
+}
+
+// fixLabels sets the namespace of this ServerInfo's labels to match the
+// matching scheme indicated by the name.
+func (s *ServerInfoV1) fixLabels() {
+	// Determine which prefix the labels need, if any.
+	namePrefix, _, found := strings.Cut(s.GetName(), "-")
+	if !found {
+		return
+	}
+	var labelPrefix string
+	switch namePrefix {
+	case "aws":
+		labelPrefix = "aws/"
+	case "si":
+		labelPrefix = TeleportDynamicLabelPrefix
+	default:
+		return
+	}
+
+	// Replace the prefix on existing labels.
+	for k, v := range s.Spec.NewLabels {
+		prefix, name, _ := strings.Cut(k, "/")
+		if name == "" {
+			name = prefix
+		}
+		delete(s.Spec.NewLabels, k)
+		s.Spec.NewLabels[labelPrefix+name] = v
+	}
 }
 
 func (s *ServerInfoV1) setStaticFields() {
@@ -173,6 +204,7 @@ func (s *ServerInfoV1) setStaticFields() {
 // default values.
 func (s *ServerInfoV1) CheckAndSetDefaults() error {
 	s.setStaticFields()
+	s.fixLabels()
 	return trace.Wrap(s.Metadata.CheckAndSetDefaults())
 }
 

api/types/server_info_test.go

@@ -0,0 +1,74 @@
+/*
+Copyright 2023 Gravitational, Inc.
+
+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 types
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestServerInfoSetLabels(t *testing.T) {
+	t.Parallel()
+	labels := map[string]string{
+		"a":         "1",
+		"dynamic/b": "2",
+		"aws/c":     "3",
+	}
+
+	tests := []struct {
+		name           string
+		serverInfoName string
+		expectedLabels map[string]string
+	}{
+		{
+			name:           "fix manual labels",
+			serverInfoName: "si-test",
+			expectedLabels: map[string]string{
+				"dynamic/a": "1",
+				"dynamic/b": "2",
+				"dynamic/c": "3",
+			},
+		},
+		{
+			name:           "fix aws labels",
+			serverInfoName: "aws-test",
+			expectedLabels: map[string]string{
+				"aws/a": "1",
+				"aws/b": "2",
+				"aws/c": "3",
+			},
+		},
+		{
+			name:           "leave other labels alone",
+			serverInfoName: "test",
+			expectedLabels: labels,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			si, err := NewServerInfo(Metadata{
+				Name: tc.serverInfoName,
+			}, ServerInfoSpecV1{
+				NewLabels: labels,
+			})
+			require.NoError(t, err)
+			require.Equal(t, tc.expectedLabels, si.GetNewLabels())
+		})
+	}
+}

lib/auth/auth.go

@@ -142,6 +142,14 @@ const (
 	OSSDesktopsLimit    = 5
 )
 
+const (
+	dynamicLabelCheckPeriod  = time.Hour
+	dynamicLabelAlertID      = "dynamic-labels-in-deny-rules"
+	dynamicLabelAlertMessage = "One or more roles has deny rules that include dynamic/ labels. " +
+		"This is not recommended due to the volatitily of dynamic/ labels and is not allowed for new roles. " +
+		"(hint: use 'tctl get roles' to find roles that need updating)"
+)
+
 var ErrRequiresEnterprise = services.ErrRequiresEnterprise
 
 // ServerOption allows setting options as functional arguments to Server
@@ -1099,6 +1107,13 @@ func (a *Server) runPeriodicOperations() {
 		log.Warnf("Can't delete OSS non-AD desktops limit alert: %v", err)
 	}
 
+	dynamicLabelsCheck := interval.New(interval.Config{
+		Duration:      dynamicLabelCheckPeriod,
+		FirstDuration: utils.HalfJitter(time.Second * 10),
+		Jitter:        retryutils.NewSeventhJitter(),
+	})
+	defer dynamicLabelsCheck.Stop()
+
 	// isolate the schedule of potentially long-running refreshRemoteClusters() from other tasks
 	go func() {
 		// reasonably small interval to ensure that users observe clusters as online within 1 minute of adding them.
@@ -1162,6 +1177,8 @@ func (a *Server) runPeriodicOperations() {
 			go a.doInstancePeriodics(ctx)
 		case <-ossDesktopsCheck:
 			a.syncDesktopsLimitAlert(ctx)
+		case <-dynamicLabelsCheck.Next():
+			a.syncDynamicLabelsAlert(ctx)
 		}
 	}
 }
@@ -4966,6 +4983,40 @@ func (a *Server) desktopsLimitExceeded(ctx context.Context) (bool, error) {
 	return false, trace.Wrap(desktops.Done())
 }
 
+func (a *Server) syncDynamicLabelsAlert(ctx context.Context) {
+	roles, err := a.GetRoles(ctx)
+	if err != nil {
+		log.Warnf("Can't get roles: %v", err)
+	}
+	var rolesWithDynamicDenyLabels bool
+	for _, role := range roles {
+		err := services.CheckDynamicLabelsInDenyRules(role)
+		if trace.IsBadParameter(err) {
+			rolesWithDynamicDenyLabels = true
+			break
+		}
+		if err != nil {
+			log.Warnf("Error checking labels in role %s: %v", role.GetName(), err)
+			continue
+		}
+	}
+	if !rolesWithDynamicDenyLabels {
+		return
+	}
+	alert, err := types.NewClusterAlert(
+		dynamicLabelAlertID,
+		dynamicLabelAlertMessage,
+		types.WithAlertSeverity(types.AlertSeverity_MEDIUM),
+		types.WithAlertLabel(types.AlertVerbPermit, fmt.Sprintf("%s:%s", types.KindRole, types.VerbRead)),
+	)
+	if err != nil {
+		log.Warnf("Failed to build %s alert: %v (this is a bug)", dynamicLabelAlertID, err)
+	}
+	if err := a.UpsertClusterAlert(ctx, alert); err != nil {
+		log.Warnf("Failed to set %s alert: %v", dynamicLabelAlertID, err)
+	}
+}
+
 // GenerateCertAuthorityCRL generates an empty CRL for the local CA of a given type.
 func (a *Server) GenerateCertAuthorityCRL(ctx context.Context, caType types.CertAuthType) ([]byte, error) {
 	// Generate a CRL for the current cluster CA.

lib/auth/server_info_test.go

@@ -116,5 +116,5 @@ func TestReconcileLabels(t *testing.T) {
 	// Wait until the reconciler finishes processing the serverinfo.
 	clock.BlockUntil(1)
 	// Check that labels were received downstream.
-	require.Equal(t, map[string]string{"a": "1", "b": "3", "c": "4"}, upstream.updatedLabels)
+	require.Equal(t, map[string]string{"aws/a": "1", "aws/b": "2", "dynamic/b": "3", "dynamic/c": "4"}, upstream.updatedLabels)
 }

lib/services/access.go

@@ -20,6 +20,10 @@ package services
 
 import (
 	"context"
+	"fmt"
+	"strings"
+
+	"github.com/gravitational/trace"
 
 	"github.com/gravitational/teleport/api/types"
 )
@@ -59,3 +63,36 @@ type Access interface {
 	// ReplaceRemoteLocks replaces the set of locks associated with a remote cluster.
 	ReplaceRemoteLocks(ctx context.Context, clusterName string, locks []types.Lock) error
 }
+
+var dynamicLabelsErrorMessage = fmt.Sprintf("labels with %q prefix are not allowed in deny rules", types.TeleportDynamicLabelPrefix)
+
+// CheckDynamicLabelsInDenyRules checks if any deny rules in the given role use
+// labels prefixed with "dynamic/".
+func CheckDynamicLabelsInDenyRules(r types.Role) error {
+	for _, kind := range types.LabelMatcherKinds {
+		labelMatchers, err := r.GetLabelMatchers(types.Deny, kind)
+		if err != nil {
+			return trace.Wrap(err)
+		}
+		for label := range labelMatchers.Labels {
+			if strings.HasPrefix(label, types.TeleportDynamicLabelPrefix) {
+				return trace.BadParameter(dynamicLabelsErrorMessage)
+			}
+		}
+		const expressionMatch = `"` + types.TeleportDynamicLabelPrefix
+		if strings.Contains(labelMatchers.Expression, expressionMatch) {
+			return trace.BadParameter(dynamicLabelsErrorMessage)
+		}
+	}
+
+	for _, where := range []string{
+		r.GetAccessReviewConditions(types.Deny).Where,
+		r.GetImpersonateConditions(types.Deny).Where,
+	} {
+		if strings.Contains(where, types.TeleportDynamicLabelPrefix) {
+			return trace.BadParameter(dynamicLabelsErrorMessage)
+		}
+	}
+
+	return nil
+}

lib/services/access_test.go

@@ -0,0 +1,104 @@
+/*
+ * Teleport
+ * Copyright (C) 2023  Gravitational, Inc.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+package services
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/gravitational/teleport/api/types"
+)
+
+func TestCheckDynamicLabelsInDenyRules(t *testing.T) {
+	t.Parallel()
+	newRole := func(t *testing.T, spec types.RoleSpecV6) types.Role {
+		role, err := types.NewRole("test-role", spec)
+		require.NoError(t, err)
+		return role
+	}
+
+	tests := []struct {
+		name   string
+		role   types.Role
+		assert require.ErrorAssertionFunc
+	}{
+		{
+			name: "ok role",
+			role: newRole(t, types.RoleSpecV6{
+				Deny: types.RoleConditions{
+					NodeLabels: types.Labels{
+						"a": {"1"},
+						"b": {"2"},
+						"c": {"3"},
+					},
+				},
+			}),
+			assert: require.NoError,
+		},
+		{
+			name: "bad labels",
+			role: newRole(t, types.RoleSpecV6{
+				Deny: types.RoleConditions{
+					NodeLabels: types.Labels{
+						"a":         {"1"},
+						"dynamic/b": {"2"},
+						"c":         {"3"},
+					},
+				},
+			}),
+			assert: require.Error,
+		},
+		{
+			name: "bad labels in where clause",
+			role: newRole(t, types.RoleSpecV6{
+				Deny: types.RoleConditions{
+					NodeLabels: types.Labels{
+						"a": {"1"},
+						"b": {"2"},
+						"c": {"3"},
+					},
+					ReviewRequests: &types.AccessReviewConditions{
+						Where: `contains(user.spec.traits["allow-env"], labels["dynamic/env"])`,
+					},
+				},
+			}),
+			assert: require.Error,
+		},
+		{
+			name: "bad labels in label expression",
+			role: newRole(t, types.RoleSpecV6{
+				Deny: types.RoleConditions{
+					NodeLabels: types.Labels{
+						"a": {"1"},
+						"b": {"2"},
+						"c": {"3"},
+					},
+					NodeLabelsExpression: `contains(user.spec.traits["allow-env"], labels["dynamic/env"])`,
+				},
+			}),
+			assert: require.Error,
+		},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			tc.assert(t, CheckDynamicLabelsInDenyRules(tc.role))
+		})
+	}
+}