diff --git a/api/types/constants.go b/api/types/constants.go index 0c697d71db37f..2ef3dc51d8828 100644 --- a/api/types/constants.go +++ b/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. diff --git a/api/types/server_info.go b/api/types/server_info.go index 9fb2356ac0cb4..aab6e1c18c4b4 100644 --- a/api/types/server_info.go +++ b/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()) } diff --git a/api/types/server_info_test.go b/api/types/server_info_test.go new file mode 100644 index 0000000000000..5757bd4d2876b --- /dev/null +++ b/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()) + }) + } +} diff --git a/lib/auth/auth.go b/lib/auth/auth.go index 25ee1e7a09d8f..8f3f4ecda568c 100644 --- a/lib/auth/auth.go +++ b/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. diff --git a/lib/auth/server_info_test.go b/lib/auth/server_info_test.go index de88669587ea6..11b19753dc53c 100644 --- a/lib/auth/server_info_test.go +++ b/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) } diff --git a/lib/services/access.go b/lib/services/access.go index c321536bb0724..9802f6edb81c2 100644 --- a/lib/services/access.go +++ b/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 +} diff --git a/lib/services/access_test.go b/lib/services/access_test.go new file mode 100644 index 0000000000000..e490075b1ec82 --- /dev/null +++ b/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 . + */ + +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)) + }) + } +} diff --git a/tool/tctl/common/resource_command.go b/tool/tctl/common/resource_command.go index 276ccd21f12ba..69ad6e1d2daa7 100644 --- a/tool/tctl/common/resource_command.go +++ b/tool/tctl/common/resource_command.go @@ -438,6 +438,12 @@ func (rc *ResourceCommand) createRole(ctx context.Context, client auth.ClientI, // check for syntax errors in predicates return trace.Wrap(err) } + err = services.CheckDynamicLabelsInDenyRules(role) + if trace.IsBadParameter(err) { + return trace.BadParameter(dynamicLabelWarningMessage(role)) + } else if err != nil { + return trace.Wrap(err) + } warnAboutKubernetesResources(rc.config.Log, role) roleName := role.GetName() @@ -468,6 +474,7 @@ func (rc *ResourceCommand) updateRole(ctx context.Context, client auth.ClientI, } warnAboutKubernetesResources(rc.config.Log, role) + warnAboutDynamicLabelsInDenyRule(rc.config.Log, role) if _, err := client.UpdateRole(ctx, role); err != nil { return trace.Wrap(err) @@ -496,6 +503,24 @@ func warnAboutKubernetesResources(logger utils.Logger, r types.Role) { } } +func dynamicLabelWarningMessage(r types.Role) string { + return fmt.Sprintf("existing role %q has labels with the %q prefix in its deny rules. This is not recommended due to the volatitily of %q labels and is not allowed for new roles", + r.GetName(), types.TeleportDynamicLabelPrefix, types.TeleportDynamicLabelPrefix) +} + +// warnAboutDynamicLabelsInDenyRule warns about using dynamic/ labels in deny +// rules. Only applies to existing roles as adding dynamic/ labels to deny +// rules in a new role is not allowed. +func warnAboutDynamicLabelsInDenyRule(logger utils.Logger, r types.Role) { + if err := services.CheckDynamicLabelsInDenyRules(r); err == nil { + return + } else if trace.IsBadParameter(err) { + logger.Warningf(dynamicLabelWarningMessage(r)) + } else { + logger.WithError(err).Warningf("error checking deny rules labels") + } +} + // createUser implements `tctl create user.yaml` command. func (rc *ResourceCommand) createUser(ctx context.Context, client auth.ClientI, raw services.UnknownResource) error { user, err := services.UnmarshalUser(raw.Raw) @@ -1810,6 +1835,7 @@ func (rc *ResourceCommand) getCollection(ctx context.Context, client auth.Client if err != nil { return nil, trace.Wrap(err) } + warnAboutDynamicLabelsInDenyRule(rc.config.Log, role) return &roleCollection{roles: []types.Role{role}}, nil case types.KindNamespace: if rc.ref.Name == "" {