Skip to content

Commit

Permalink
Use free functions for Validate
Browse files Browse the repository at this point in the history
  • Loading branch information
espadolini committed Jan 27, 2025
1 parent dffc17e commit cfce7b6
Show file tree
Hide file tree
Showing 7 changed files with 161 additions and 136 deletions.
37 changes: 0 additions & 37 deletions api/types/authentication.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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())
Expand Down
94 changes: 0 additions & 94 deletions api/types/authentication_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
})
}
})
}
2 changes: 1 addition & 1 deletion lib/auth/auth_with_roles.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down
7 changes: 4 additions & 3 deletions lib/auth/clusterconfig/clusterconfigv1/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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)
}

Expand Down
2 changes: 1 addition & 1 deletion lib/auth/stableunixusers/stableunixusers.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down
36 changes: 36 additions & 0 deletions lib/services/configuration.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}
119 changes: 119 additions & 0 deletions lib/services/configuration_test.go
Original file line number Diff line number Diff line change
@@ -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 <http://www.gnu.org/licenses/>.

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

0 comments on commit cfce7b6

Please sign in to comment.