Skip to content

Reject unsupported clients by default #41239

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rosstimothy merged 1 commit into from
May 13, 2024
Merged

Reject unsupported clients by default #41239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 0 additions & 69 deletions lib/auth/grpcserver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ import (
apidefaults "github.com/gravitational/teleport/api/defaults"
clusterconfigpb "github.com/gravitational/teleport/api/gen/proto/go/teleport/clusterconfig/v1"
"github.com/gravitational/teleport/api/internalutils/stream"
"github.com/gravitational/teleport/api/metadata"
"github.com/gravitational/teleport/api/mfa"
"github.com/gravitational/teleport/api/observability/tracing"
"github.com/gravitational/teleport/api/types"
Expand Down Expand Up @@ -4198,74 +4197,6 @@ func TestGRPCServer_GetInstallers(t *testing.T) {
}
}

// DELETE IN 16.0
// TestPing_VersionCheck_AccessMonitoringFlag tests that client versions <=14.2.0
// sets the IGS flag. Old clients will expect IGS == access monitoring.
func TestPing_VersionCheck_AccessMonitoringFlag(t *testing.T) {
modules.SetTestModules(t, &modules.TestModules{
TestFeatures: modules.Features{
AccessMonitoring: modules.AccessMonitoringFeature{
Enabled: true,
},
},
})

srv := newTestTLSServer(t)
srv.Auth().accessMonitoringEnabled = true

user, _, err := CreateUserAndRoleWithoutRoles(srv.Auth(), "user", nil)
require.NoError(t, err)

client, err := srv.NewClient(TestUser(user.GetName()))
require.NoError(t, err)

// Test with latest major version that supports access monitoring field.
// Should NOT enable IGS.
ctx := context.Background()
ctx = metadata.AddMetadataToContext(ctx, map[string]string{
metadata.VersionKey: "15.0.0",
})
ping, err := client.Ping(ctx)
require.NoError(t, err)
require.False(t, ping.ServerFeatures.IdentityGovernance, "expected field IdentityGovernance to be false")
require.True(t, ping.ServerFeatures.AccessMonitoring.Enabled, "expected field AccessMonitoring.Enabled to be true")

// Test with minimum major/minor/patch version that supports access monitoring field.
// Should NOT enable IGS.
ctx2 := context.Background()
ctx2 = metadata.AddMetadataToContext(ctx2, map[string]string{
metadata.VersionKey: "14.2.1",
})
ping, err = client.Ping(ctx2)
require.NoError(t, err)
require.False(t, ping.ServerFeatures.IdentityGovernance, "expected field IdentityGovernance to be false")
require.True(t, ping.ServerFeatures.AccessMonitoring.Enabled, "expected field AccessMonitoring.Enabled to be true")

// Test with version that does NOT support access monitoring field.
// Should return with IGS enabled to mean access monitoring was enabled
// (because that's what older clients expect)
ctx3 := context.Background()
ctx3 = metadata.AddMetadataToContext(ctx3, map[string]string{
metadata.VersionKey: "14.2.0",
})
ping, err = client.Ping(ctx3)
require.NoError(t, err)
require.True(t, ping.ServerFeatures.IdentityGovernance, "expected field IdentityGovernance to be true")
require.True(t, ping.ServerFeatures.AccessMonitoring.Enabled, "expected field AccessMonitoring.Enabled to be true")

// Test with an older version.
// Should return with IGS enabled to mean access monitoring was enabled
// (because that's what older clients expect)
ctx4 := context.Background()
ctx4 = metadata.AddMetadataToContext(ctx4, map[string]string{
metadata.VersionKey: "13.3.0",
})
ping, err = client.Ping(ctx4)
require.NoError(t, err)
require.True(t, ping.ServerFeatures.IdentityGovernance, "expected field IdentityGovernance to be true")
require.True(t, ping.ServerFeatures.AccessMonitoring.Enabled, "expected field AccessMonitoring.Enabled to be true")
}

func TestUpsertApplicationServerOrigin(t *testing.T) {
t.Parallel()

Expand Down
62 changes: 59 additions & 3 deletions lib/auth/middleware.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,12 @@ import (
"crypto/tls"
"crypto/x509"
"encoding/json"
"fmt"
"net"
"net/http"
"os"
"slices"
"sync/atomic"
"time"

"github.com/coreos/go-semver/semver"
Expand Down Expand Up @@ -167,9 +169,9 @@ func NewTLSServer(ctx context.Context, cfg TLSServerConfig) (*TLSServer, error)
return nil, trace.Wrap(err)
}

var oldestSupportedVersion *semver.Version
if os.Getenv("TELEPORT_UNSTABLE_REJECT_OLD_CLIENTS") == "yes" {
oldestSupportedVersion = &teleport.MinClientSemVersion
oldestSupportedVersion := &teleport.MinClientSemVersion
if os.Getenv("TELEPORT_UNSTABLE_ALLOW_OLD_CLIENTS") == "yes" {
oldestSupportedVersion = nil
}

// authMiddleware authenticates request assuming TLS client authentication
Expand All @@ -181,6 +183,9 @@ func NewTLSServer(ctx context.Context, cfg TLSServerConfig) (*TLSServer, error)
Limiter: limiter,
GRPCMetrics: grpcMetrics,
OldestSupportedVersion: oldestSupportedVersion,
AlertCreator: func(ctx context.Context, a types.ClusterAlert) error {
return trace.Wrap(cfg.AuthServer.UpsertClusterAlert(ctx, a))
},
}

apiServer, err := NewAPIServer(&cfg.APIConfig)
Expand Down Expand Up @@ -336,6 +341,13 @@ type Middleware struct {
// originated from a client that is using an unsupported version. If not set, then no
// rejection occurs.
OldestSupportedVersion *semver.Version
// AlertCreator if provided is used to generate a cluster alert when any
// unsupported connections are rejected.
AlertCreator func(ctx context.Context, a types.ClusterAlert) error

// lastRejectedAlertTime is the timestamp at which the last alert
// was created in response to rejecting unsupported clients.
lastRejectedAlertTime atomic.Int64
}

// Wrap sets next handler in chain
Expand Down Expand Up @@ -391,23 +403,67 @@ func (a *Middleware) ValidateClientVersion(ctx context.Context, info IdentityInf
clientVersion, err := semver.NewVersion(clientVersionString)
if err != nil {
logger.WithError(err).Warn("Failed to determine client version")
a.displayRejectedClientAlert(ctx)
if err := info.Conn.Close(); err != nil {
logger.WithError(err).Warn("Failed to close client connection")
}

return trace.AccessDenied("client version is unsupported")
}

if clientVersion.LessThan(*a.OldestSupportedVersion) {
logger.Info("Terminating connection of client using unsupported version")
a.displayRejectedClientAlert(ctx)

if err := info.Conn.Close(); err != nil {
logger.WithError(err).Warn("Failed to close client connection")
}

return trace.AccessDenied("client version is unsupported")
}

return nil
}

// displayRejectedClientAlert creates an alert to notify admins that
// unsupported Teleport versions exist in the cluster and are explicitly
// being denied to prevent causing issues. Alerts are limited to being
// created once per day to reduce backend writes if there are a large
// number of unsupported clients constantly being rejected.
func (a *Middleware) displayRejectedClientAlert(ctx context.Context) {
if a.AlertCreator == nil {
return
}

now := time.Now()
lastAlert := a.lastRejectedAlertTime.Load()
then := time.Unix(0, lastAlert)
if lastAlert != 0 && now.Before(then.Add(24*time.Hour)) {
return
}

if !a.lastRejectedAlertTime.CompareAndSwap(lastAlert, now.UnixNano()) {
Comment thread
rosstimothy marked this conversation as resolved.
Outdated
return
}

alert, err := types.NewClusterAlert(
"rejected-unsupported-connection",
fmt.Sprintf("Connections were rejected from agents running unsupported Teleport versions(<%s), they will be inaccessible until upgraded.", a.OldestSupportedVersion),
types.WithAlertSeverity(types.AlertSeverity_MEDIUM),
types.WithAlertLabel(types.AlertOnLogin, "yes"),
types.WithAlertLabel(types.AlertVerbPermit, fmt.Sprintf("%s:%s", types.KindToken, types.VerbCreate)),
)
if err != nil {
log.WithError(err).Warn("failed to create rejected-unsupported-connection alert")
return
}

if err := a.AlertCreator(ctx, alert); err != nil {
log.WithError(err).Warn("failed to persist rejected-unsupported-connection alert")
return
}
}

// withAuthenticatedUser returns a new context with the ContextUser field set to
// the caller's user identity as authenticated by their client TLS certificate.
func (a *Middleware) withAuthenticatedUser(ctx context.Context) (context.Context, error) {
Expand Down
71 changes: 64 additions & 7 deletions lib/auth/middleware_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,15 @@ import (
"net"
"net/http"
"net/http/httptest"
"sync"
"testing"
"time"

"github.com/coreos/go-semver/semver"
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/gravitational/trace"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"google.golang.org/grpc/metadata"

Expand Down Expand Up @@ -671,50 +673,51 @@ func (f fakeConn) Close() error {
func TestValidateClientVersion(t *testing.T) {
cases := []struct {
name string
middleware Middleware
middleware *Middleware
clientVersion string
errAssertion func(t *testing.T, err error)
}{
{
name: "rejection disabled",
name: "rejection disabled",
middleware: &Middleware{},
errAssertion: func(t *testing.T, err error) {
require.NoError(t, err)
},
},
{
name: "rejection enabled and client version not specified",
middleware: Middleware{OldestSupportedVersion: &teleport.MinClientSemVersion},
middleware: &Middleware{OldestSupportedVersion: &teleport.MinClientSemVersion},
errAssertion: func(t *testing.T, err error) {
require.NoError(t, err)
},
},
{
name: "client rejected",
middleware: Middleware{OldestSupportedVersion: &teleport.MinClientSemVersion},
middleware: &Middleware{OldestSupportedVersion: &teleport.MinClientSemVersion},
clientVersion: semver.Version{Major: teleport.SemVersion.Major - 2}.String(),
errAssertion: func(t *testing.T, err error) {
require.True(t, trace.IsAccessDenied(err), "got %T, expected access denied error", err)
},
},
{
name: "valid client v-1",
middleware: Middleware{OldestSupportedVersion: &teleport.MinClientSemVersion},
middleware: &Middleware{OldestSupportedVersion: &teleport.MinClientSemVersion},
clientVersion: semver.Version{Major: teleport.SemVersion.Major - 1}.String(),
errAssertion: func(t *testing.T, err error) {
require.NoError(t, err)
},
},
{
name: "valid client v-0",
middleware: Middleware{OldestSupportedVersion: &teleport.MinClientSemVersion},
middleware: &Middleware{OldestSupportedVersion: &teleport.MinClientSemVersion},
clientVersion: semver.Version{Major: teleport.SemVersion.Major}.String(),
errAssertion: func(t *testing.T, err error) {
require.NoError(t, err)
},
},
{
name: "invalid client version",
middleware: Middleware{OldestSupportedVersion: &teleport.MinClientSemVersion},
middleware: &Middleware{OldestSupportedVersion: &teleport.MinClientSemVersion},
clientVersion: "abc123",
errAssertion: func(t *testing.T, err error) {
require.True(t, trace.IsAccessDenied(err), "got %T, expected access denied error", err)
Expand All @@ -733,3 +736,57 @@ func TestValidateClientVersion(t *testing.T) {
})
}
}

func TestRejectedClientClusterAlert(t *testing.T) {
var alerts []types.ClusterAlert
mw := Middleware{
OldestSupportedVersion: &teleport.MinClientSemVersion,
AlertCreator: func(ctx context.Context, a types.ClusterAlert) error {
alerts = append(alerts, a)
return nil
},
}

// Validate an unsupported client, which should trigger an alert
ctx := metadata.NewIncomingContext(context.Background(), metadata.New(map[string]string{
"version": semver.Version{Major: teleport.SemVersion.Major - 20}.String(),
}))
err := mw.ValidateClientVersion(ctx, IdentityInfo{Conn: fakeConn{}, IdentityGetter: TestBuiltin(types.RoleNode).I})
assert.Error(t, err)

// Validate a client with an unknown version, which should trigger an alert, however,
// due to rate limiting of 1 alert per 24h no alert should be created.
ctx = metadata.NewIncomingContext(context.Background(), metadata.New(map[string]string{
"version": "abcd",
}))
err = mw.ValidateClientVersion(ctx, IdentityInfo{Conn: fakeConn{}, IdentityGetter: TestBuiltin(types.RoleNode).I})
assert.Error(t, err)

// Assert that only a single alert was created based on the above rejections.
require.Len(t, alerts, 1)
require.Equal(t, "rejected-unsupported-connection", alerts[0].GetName())

// Reset the last alert time to a time beyond the rate limit, allowing the next
// rejection to trigger another alert.
mw.lastRejectedAlertTime.Store(time.Now().Add(-25 * time.Hour).UnixNano())

// Validate two unsupported clients in parallel to verify that concurrent attempts
// to create an alert are prevented.
var wg sync.WaitGroup
for i := 0; i < 2; i++ {
wg.Add(1)
go func() {
defer wg.Done()
err := mw.ValidateClientVersion(ctx, IdentityInfo{Conn: fakeConn{}, IdentityGetter: TestBuiltin(types.RoleNode).I})
assert.Error(t, err)
}()
}

wg.Wait()

// Assert that only a single additional alert was created.
require.Len(t, alerts, 2)
for _, alert := range alerts {
assert.Equal(t, "rejected-unsupported-connection", alert.GetName())
}
}