From cfce7b69d5d0b1040adb9bbf105342cacf8679c7 Mon Sep 17 00:00:00 2001 From: Edoardo Spadolini Date: Mon, 27 Jan 2025 19:31:37 +0100 Subject: [PATCH] Use free functions for Validate --- api/types/authentication.go | 37 ------ api/types/authentication_test.go | 94 -------------- lib/auth/auth_with_roles.go | 2 +- .../clusterconfig/clusterconfigv1/service.go | 7 +- lib/auth/stableunixusers/stableunixusers.go | 2 +- lib/services/configuration.go | 36 ++++++ lib/services/configuration_test.go | 119 ++++++++++++++++++ 7 files changed, 161 insertions(+), 136 deletions(-) create mode 100644 lib/services/configuration_test.go diff --git a/api/types/authentication.go b/api/types/authentication.go index 1a47d24da09ce..b5872f4651b51 100644 --- a/api/types/authentication.go +++ b/api/types/authentication.go @@ -202,11 +202,6 @@ type AuthPreference interface { // Clone makes a deep copy of the AuthPreference. Clone() AuthPreference - - // Validate performs checks that should happen before persisting a new - // version of the resource, typically only as part of Auth service - // operations. - Validate() error } // NewAuthPreference is a convenience method to to create AuthPreferenceV2. @@ -874,38 +869,6 @@ func (c *AuthPreferenceV2) CheckAndSetDefaults() error { return nil } -// Validate implements [AuthPreference]. -func (c *AuthPreferenceV2) Validate() error { - if c == nil { - return nil - } - - if err := c.Spec.StableUnixUserConfig.Validate(); err != nil { - return trace.Wrap(err) - } - - return nil -} - -// Validate checks if the configuration is suitable for storage and use. -func (c *StableUNIXUserConfig) Validate() error { - if c == nil || !c.Enabled { - return nil - } - - if c.FirstUid > c.LastUid { - return trace.BadParameter("stable UNIX user is enabled but UID range is empty") - } - - // see https://github.com/systemd/systemd/blob/cc7300fc5868f6d47f3f47076100b574bf54e58d/docs/UIDS-GIDS.md - const firstUserUID = 1000 - if c.FirstUid < firstUserUID { - return trace.BadParameter("stable UNIX user UID range includes negative or system UIDs; the configured range should be contained between 1000 and 2147483647") - } - - return nil -} - // String represents a human readable version of authentication settings. func (c *AuthPreferenceV2) String() string { return fmt.Sprintf("AuthPreference(Type=%q,SecondFactors=%q)", c.Spec.Type, c.GetSecondFactors()) diff --git a/api/types/authentication_test.go b/api/types/authentication_test.go index a0ea2ec59665c..7d6a76e9ae1ae 100644 --- a/api/types/authentication_test.go +++ b/api/types/authentication_test.go @@ -239,97 +239,3 @@ func TestNewAuthPreference_secondFactors(t *testing.T) { }) } } - -func TestAuthPreferenceValidate(t *testing.T) { - t.Parallel() - - t.Run("default", func(t *testing.T) { - t.Parallel() - - require.NoError(t, DefaultAuthPreference().Validate()) - }) - - t.Run("stable_unix_users", func(t *testing.T) { - t.Parallel() - - type testCase struct { - name string - config *StableUNIXUserConfig - check require.ErrorAssertionFunc - } - - testCases := []testCase{ - { - name: "missing", - config: nil, - check: require.NoError, - }, - { - name: "disabled", - config: &StableUNIXUserConfig{ - Enabled: false, - }, - check: require.NoError, - }, - { - name: "enabled", - config: &StableUNIXUserConfig{ - Enabled: true, - FirstUid: 30000, - LastUid: 40000, - }, - check: require.NoError, - }, - { - name: "empty_range", - config: &StableUNIXUserConfig{ - Enabled: true, - FirstUid: 30000, - LastUid: 29000, - }, - check: require.Error, - }, - { - name: "empty_range_disabled", - config: &StableUNIXUserConfig{ - Enabled: false, - FirstUid: 30000, - LastUid: 29000, - }, - check: require.NoError, - }, - { - name: "system_range", - config: &StableUNIXUserConfig{ - Enabled: true, - FirstUid: 100, - LastUid: 40000, - }, - check: require.Error, - }, - { - name: "negative_range", - config: &StableUNIXUserConfig{ - Enabled: true, - FirstUid: -100, - LastUid: 40000, - }, - check: require.Error, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - - authPref := &AuthPreferenceV2{ - Spec: AuthPreferenceSpecV2{ - StableUnixUserConfig: tc.config, - }, - } - tc.check(t, authPref.Validate()) - tc.check(t, authPref.Spec.StableUnixUserConfig.Validate()) - }) - } - }) -} diff --git a/lib/auth/auth_with_roles.go b/lib/auth/auth_with_roles.go index 17b1a5db2efd7..4e3d63dd68978 100644 --- a/lib/auth/auth_with_roles.go +++ b/lib/auth/auth_with_roles.go @@ -4731,7 +4731,7 @@ func (a *ServerWithRoles) SetAuthPreference(ctx context.Context, newAuthPref typ return trace.Wrap(err) } - if err := newAuthPref.Validate(); err != nil { + if err := services.ValidateAuthPreference(newAuthPref); err != nil { return trace.Wrap(err) } diff --git a/lib/auth/clusterconfig/clusterconfigv1/service.go b/lib/auth/clusterconfig/clusterconfigv1/service.go index 3c473f2f57c31..5795500515b6c 100644 --- a/lib/auth/clusterconfig/clusterconfigv1/service.go +++ b/lib/auth/clusterconfig/clusterconfigv1/service.go @@ -31,6 +31,7 @@ import ( dtconfig "github.com/gravitational/teleport/lib/devicetrust/config" "github.com/gravitational/teleport/lib/events" "github.com/gravitational/teleport/lib/modules" + "github.com/gravitational/teleport/lib/services" "github.com/gravitational/teleport/lib/services/readonly" ) @@ -179,7 +180,7 @@ func (s *Service) CreateAuthPreference(ctx context.Context, p types.AuthPreferen return nil, trace.AccessDenied("this request can be only executed by an auth server") } - if err := p.Validate(); err != nil { + if err := services.ValidateAuthPreference(p); err != nil { return nil, trace.Wrap(err) } @@ -238,7 +239,7 @@ func (s *Service) UpdateAuthPreference(ctx context.Context, req *clusterconfigpb return nil, trace.Wrap(err) } - if err := req.GetAuthPreference().Validate(); err != nil { + if err := services.ValidateAuthPreference(req.GetAuthPreference()); err != nil { return nil, trace.Wrap(err) } @@ -306,7 +307,7 @@ func (s *Service) UpsertAuthPreference(ctx context.Context, req *clusterconfigpb return nil, trace.Wrap(err) } - if err := req.GetAuthPreference().Validate(); err != nil { + if err := services.ValidateAuthPreference(req.GetAuthPreference()); err != nil { return nil, trace.Wrap(err) } diff --git a/lib/auth/stableunixusers/stableunixusers.go b/lib/auth/stableunixusers/stableunixusers.go index e13b3575fe05c..3885a30c7302b 100644 --- a/lib/auth/stableunixusers/stableunixusers.go +++ b/lib/auth/stableunixusers/stableunixusers.go @@ -256,7 +256,7 @@ func (s *server) createNewStableUNIXUser(ctx context.Context, username string, a // auth server, so just in case we don't consider it valid we will reject it // here (which will fail the API call, but it's better than potentially // trashing the storage) - if err := cfg.Validate(); err != nil { + if err := services.ValidateStableUNIXUserConfig(cfg); err != nil { return 0, true, trace.Wrap(err) } diff --git a/lib/services/configuration.go b/lib/services/configuration.go index 8dfbf063a780a..6d8ea6eb9add7 100644 --- a/lib/services/configuration.go +++ b/lib/services/configuration.go @@ -21,6 +21,8 @@ package services import ( "context" + "github.com/gravitational/trace" + clusterconfigpb "github.com/gravitational/teleport/api/gen/proto/go/teleport/clusterconfig/v1" "github.com/gravitational/teleport/api/types" "github.com/gravitational/teleport/lib/backend" @@ -143,3 +145,37 @@ type ClusterConfigurationInternal interface { // ClusterConfigurationInternal. AppendCheckAuthPreferenceActions(actions []backend.ConditionalAction, revision string) ([]backend.ConditionalAction, error) } + +// ValidateAuthPreference performs checks that should happen before persisting a +// new version of the preference resource, typically only as part of Auth +// service operations. +func ValidateAuthPreference(ap types.AuthPreference) error { + // TODO(espadolini): the checks that are duplicated in + // {Set,Create,Update,Upsert}AuthPreference should be moved here + + if err := ValidateStableUNIXUserConfig(ap.GetStableUNIXUserConfig()); err != nil { + return trace.Wrap(err) + } + + return nil +} + +// ValidateStableUNIXUserConfig checks if the configuration is suitable for +// storage and use. +func ValidateStableUNIXUserConfig(c *types.StableUNIXUserConfig) error { + if c == nil || !c.Enabled { + return nil + } + + if c.FirstUid > c.LastUid { + return trace.BadParameter("stable UNIX user is enabled but UID range is empty") + } + + // see https://github.com/systemd/systemd/blob/cc7300fc5868f6d47f3f47076100b574bf54e58d/docs/UIDS-GIDS.md + const firstUserUID = 1000 + if c.FirstUid < firstUserUID { + return trace.BadParameter("stable UNIX user UID range includes negative or system UIDs; the configured range should be contained between 1000 and 2147483647") + } + + return nil +} diff --git a/lib/services/configuration_test.go b/lib/services/configuration_test.go new file mode 100644 index 0000000000000..b5f1baf90330a --- /dev/null +++ b/lib/services/configuration_test.go @@ -0,0 +1,119 @@ +// Teleport +// Copyright (C) 2025 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 TestAuthPreferenceValidate(t *testing.T) { + t.Parallel() + + t.Run("default", func(t *testing.T) { + t.Parallel() + + require.NoError(t, ValidateAuthPreference(types.DefaultAuthPreference())) + }) + + t.Run("stable_unix_users", func(t *testing.T) { + t.Parallel() + + type testCase struct { + name string + config *types.StableUNIXUserConfig + check require.ErrorAssertionFunc + } + + testCases := []testCase{ + { + name: "missing", + config: nil, + check: require.NoError, + }, + { + name: "disabled", + config: &types.StableUNIXUserConfig{ + Enabled: false, + }, + check: require.NoError, + }, + { + name: "enabled", + config: &types.StableUNIXUserConfig{ + Enabled: true, + FirstUid: 30000, + LastUid: 40000, + }, + check: require.NoError, + }, + { + name: "empty_range", + config: &types.StableUNIXUserConfig{ + Enabled: true, + FirstUid: 30000, + LastUid: 29000, + }, + check: require.Error, + }, + { + name: "empty_range_disabled", + config: &types.StableUNIXUserConfig{ + Enabled: false, + FirstUid: 30000, + LastUid: 29000, + }, + check: require.NoError, + }, + { + name: "system_range", + config: &types.StableUNIXUserConfig{ + Enabled: true, + FirstUid: 100, + LastUid: 40000, + }, + check: require.Error, + }, + { + name: "negative_range", + config: &types.StableUNIXUserConfig{ + Enabled: true, + FirstUid: -100, + LastUid: 40000, + }, + check: require.Error, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + authPref := &types.AuthPreferenceV2{ + Spec: types.AuthPreferenceSpecV2{ + StableUnixUserConfig: tc.config, + }, + } + tc.check(t, ValidateAuthPreference(authPref)) + tc.check(t, ValidateStableUNIXUserConfig(authPref.Spec.StableUnixUserConfig)) + }) + } + }) +}