From 9a298efd551d787c1f9f102c8d8aebe83438e98d Mon Sep 17 00:00:00 2001 From: Alan Parra Date: Fri, 27 Sep 2024 12:29:22 -0300 Subject: [PATCH] [v15] fix: Enforce device trust on OSS processes (#46946) * fix: Enforce device trust on OSS processes * Fix TestSessionController_AcquireSessionContext --- lib/devicetrust/authz/authz.go | 20 +-------- lib/devicetrust/authz/authz_test.go | 6 +-- lib/devicetrust/config/config.go | 12 ++++++ lib/devicetrust/config/config_test.go | 58 +++++++++++++++++++++++++++ lib/srv/session_control_test.go | 19 +++++---- 5 files changed, 84 insertions(+), 31 deletions(-) diff --git a/lib/devicetrust/authz/authz.go b/lib/devicetrust/authz/authz.go index 5b3e9cdb8498..4e4a3d3c08eb 100644 --- a/lib/devicetrust/authz/authz.go +++ b/lib/devicetrust/authz/authz.go @@ -19,8 +19,6 @@ package authz import ( - "sync" - "github.com/gravitational/trace" log "github.com/sirupsen/logrus" "golang.org/x/crypto/ssh" @@ -28,7 +26,7 @@ import ( "github.com/gravitational/teleport" "github.com/gravitational/teleport/api/constants" "github.com/gravitational/teleport/api/types" - "github.com/gravitational/teleport/lib/devicetrust/config" + dtconfig "github.com/gravitational/teleport/lib/devicetrust/config" "github.com/gravitational/teleport/lib/tlsca" ) @@ -73,9 +71,7 @@ func VerifySSHUser(dt *types.DeviceTrust, cert *ssh.Certificate) error { } func verifyDeviceExtensions(dt *types.DeviceTrust, username string, verified bool) error { - mode := config.GetEffectiveMode(dt) - maybeLogModeMismatch(mode, dt) - + mode := dtconfig.GetEnforcementMode(dt) switch { case mode != constants.DeviceTrustModeRequired: return nil // OK, extensions not enforced. @@ -88,15 +84,3 @@ func verifyDeviceExtensions(dt *types.DeviceTrust, username string, verified boo return nil } } - -var logModeOnce sync.Once - -func maybeLogModeMismatch(effective string, dt *types.DeviceTrust) { - if dt == nil || dt.Mode == "" || effective == dt.Mode { - return - } - - logModeOnce.Do(func() { - log.Warnf("Device Trust: mode %q requires Teleport Enterprise. Using effective mode %q.", dt.Mode, effective) - }) -} diff --git a/lib/devicetrust/authz/authz_test.go b/lib/devicetrust/authz/authz_test.go index 66dc436d3530..313871bf34e5 100644 --- a/lib/devicetrust/authz/authz_test.go +++ b/lib/devicetrust/authz/authz_test.go @@ -174,13 +174,13 @@ func runVerifyUserTest(t *testing.T, method string, verify func(dt *types.Device assertErr: assertNoErr, }, { - name: "OSS mode never enforced", + name: "OSS mode=required (Enterprise Auth)", buildType: modules.BuildOSS, dt: &types.DeviceTrust{ - Mode: constants.DeviceTrustModeRequired, // Invalid for OSS, treated as "off". + Mode: constants.DeviceTrustModeRequired, }, ext: userWithoutExtensions, - assertErr: assertNoErr, + assertErr: assertDeniedErr, }, { name: "Enterprise mode=off", diff --git a/lib/devicetrust/config/config.go b/lib/devicetrust/config/config.go index 91449b7327a0..8b0f4b8e4951 100644 --- a/lib/devicetrust/config/config.go +++ b/lib/devicetrust/config/config.go @@ -42,6 +42,18 @@ func GetEffectiveMode(dt *types.DeviceTrust) string { return dt.Mode } +// GetEnforcementMode returns the configured device trust mode, disregarding the +// provenance of the binary if the mode is set. +// Used for device enforcement checks. Guarantees that OSS binaries paired with +// an Enterprise Auth will correctly enforce device trust. +func GetEnforcementMode(dt *types.DeviceTrust) string { + // If absent use the defaults from GetEffectiveMode. + if dt == nil || dt.Mode == "" { + return GetEffectiveMode(dt) + } + return dt.Mode +} + // ValidateConfigAgainstModules verifies the device trust configuration against // the current modules. // This method exists to provide feedback to users about invalid configurations, diff --git a/lib/devicetrust/config/config_test.go b/lib/devicetrust/config/config_test.go index 24d5222fcdfe..14a31af2fafb 100644 --- a/lib/devicetrust/config/config_test.go +++ b/lib/devicetrust/config/config_test.go @@ -32,6 +32,8 @@ import ( ) func TestValidateConfigAgainstModules(t *testing.T) { + // Don't t.Parallel, depends on modules.SetTestModules. + type testCase struct { name string buildType string @@ -110,3 +112,59 @@ func TestValidateConfigAgainstModules(t *testing.T) { }) } } + +func TestGetEnforcementMode(t *testing.T) { + // Don't t.Parallel, depends on modules.SetTestModules. + + tests := []struct { + name string + buildType string + dt *types.DeviceTrust + want string + }{ + { + name: "OSS default", + buildType: modules.BuildOSS, + want: constants.DeviceTrustModeOff, + }, + { + name: "Enterprise default", + buildType: modules.BuildEnterprise, + want: constants.DeviceTrustModeOptional, + }, + { + name: "dt.Mode empty", + buildType: modules.BuildEnterprise, + dt: &types.DeviceTrust{ + Mode: "", + }, + want: constants.DeviceTrustModeOptional, + }, + { + name: "dt.Mode set", + buildType: modules.BuildEnterprise, + dt: &types.DeviceTrust{ + Mode: constants.DeviceTrustModeRequired, + }, + want: constants.DeviceTrustModeRequired, + }, + { + name: "OSS node with Ent Auth", + buildType: modules.BuildOSS, + dt: &types.DeviceTrust{ + Mode: constants.DeviceTrustModeRequired, + }, + want: constants.DeviceTrustModeRequired, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + modules.SetTestModules(t, &modules.TestModules{ + TestBuildType: test.buildType, + }) + + got := dtconfig.GetEnforcementMode(test.dt) + assert.Equal(t, test.want, got, "dtconfig.GetEnforcementMode mismatch") + }) + } +} diff --git a/lib/srv/session_control_test.go b/lib/srv/session_control_test.go index 63cb7e75a92f..925cbac82b04 100644 --- a/lib/srv/session_control_test.go +++ b/lib/srv/session_control_test.go @@ -163,6 +163,10 @@ func TestSessionController_AcquireSessionContext(t *testing.T) { } return idCtx } + assertTrustedDeviceRequired := func(t *testing.T, _ context.Context, err error, _ *eventstest.MockRecorderEmitter) { + assert.ErrorContains(t, err, "device", "AcquireSessionContext returned an unexpected error") + assert.True(t, trace.IsAccessDenied(err), "AcquireSessionContext returned an error other than trace.AccessDeniedError: %T", err) + } cases := []struct { name string @@ -451,22 +455,17 @@ func TestSessionController_AcquireSessionContext(t *testing.T) { }, }, { - name: "device extensions not enforced for OSS", - cfg: cfgWithDeviceMode(constants.DeviceTrustModeRequired), - identity: minimalIdentity, - assertion: func(t *testing.T, _ context.Context, err error, _ *eventstest.MockRecorderEmitter) { - assert.NoError(t, err, "AcquireSessionContext returned an unexpected error") - }, + name: "device extensions enforced for OSS", + cfg: cfgWithDeviceMode(constants.DeviceTrustModeRequired), + identity: minimalIdentity, + assertion: assertTrustedDeviceRequired, }, { name: "device extensions enforced for Enterprise", buildType: modules.BuildEnterprise, cfg: cfgWithDeviceMode(constants.DeviceTrustModeRequired), identity: minimalIdentity, - assertion: func(t *testing.T, _ context.Context, err error, _ *eventstest.MockRecorderEmitter) { - assert.ErrorContains(t, err, "device", "AcquireSessionContext returned an unexpected error") - assert.True(t, trace.IsAccessDenied(err), "AcquireSessionContext returned an error other than trace.AccessDeniedError: %T", err) - }, + assertion: assertTrustedDeviceRequired, }, { name: "device extensions valid for Enterprise",