Skip to content

Commit

Permalink
always make PortForwarding backwards compatible instead of it being c…
Browse files Browse the repository at this point in the history 
…onditional
  • Loading branch information
@eriktate
eriktate committed Dec 11, 2024
1 parent 3c7a221 commit 7045356
Show file tree
Hide file tree
Showing 2 changed files with 118 additions and 169 deletions.
44 changes: 5 additions & 39 deletions lib/auth/grpcserver.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1978,55 +1978,21 @@ func (g *GRPCServer) DeleteAllKubernetesServers(ctx context.Context, req *authpb
// version for some features of the role returns a shallow copy of the given
// role downgraded for compatibility with the older version.
func maybeDowngradeRole(ctx context.Context, role *types.RoleV6) (*types.RoleV6, error) {
clientVersionString, ok := metadata.ClientVersionFromContext(ctx)
if !ok {
// This client is not reporting its version via gRPC metadata. Teleport
// clients have been reporting their version for long enough that older
// clients won't even support v6 roles at all, so this is likely a
// third-party client, and we shouldn't assume that downgrading the role
// will do more good than harm.
return role, nil
}

clientVersion, err := semver.NewVersion(clientVersionString)
if err != nil {
return nil, trace.BadParameter("unrecognized client version: %s is not a valid semver", clientVersionString)
}

role = maybeDowngradeRoleSSHPortForwarding(role, clientVersion)
return role, nil
return withBackwardsCompatiblePortForwarding(role), nil
}

var minSupportedSSHPortForwardingVersions = map[int64]semver.Version{
15: {Major: 15, Minor: 4, Patch: 23},
16: {Major: 16, Minor: 4, Patch: 9},
17: {Major: 17, Minor: 0, Patch: 0},
}

func maybeDowngradeRoleSSHPortForwarding(role *types.RoleV6, clientVersion *semver.Version) *types.RoleV6 {
if role.GetOptions().SSHPortForwarding == nil {
return role
}

minSupportedVersion, ok := minSupportedSSHPortForwardingVersions[clientVersion.Major]
if ok && !clientVersion.LessThan(minSupportedVersion) {
func withBackwardsCompatiblePortForwarding(role *types.RoleV6) *types.RoleV6 {
sshPortForwarding := role.GetOptions().SSHPortForwarding
if sshPortForwarding == nil || (sshPortForwarding.Remote == nil && sshPortForwarding.Local == nil) {
return role
}

mode := services.RoleSet{role}.SSHPortForwardMode()
role = apiutils.CloneProtoMsg(role)
options := role.GetOptions()

//nolint:staticcheck // this field is preserved for backwards compatability
options.PortForwarding = types.NewBoolOption(mode.Legacy())
options.PortForwarding = types.NewBoolOption(services.RoleSet{role}.CanPortForward())
role.SetOptions(options)
reason := fmt.Sprintf(`Client version %q does not support granular SSH port forwarding. Role %q will be downgraded `+
`to simple port forwarding rules instead. In order to support granular SSH port forwarding, all clients must be `+
`updated to version %q or higher.`, clientVersion, role.GetName(), minSupportedVersion)
if role.Metadata.Labels == nil {
role.Metadata.Labels = make(map[string]string, 1)
}
role.Metadata.Labels[types.TeleportDowngradedLabel] = reason
return role
}

Expand Down
243 changes: 113 additions & 130 deletions lib/auth/grpcserver_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ import (
clusterconfigpb "github.com/gravitational/teleport/api/gen/proto/go/teleport/clusterconfig/v1"
mfav1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/mfa/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 @@ -4605,7 +4604,7 @@ func TestGRPCServer_GetInstallers(t *testing.T) {
}
}

func TestRoleVersions(t *testing.T) {
func TestRolePortForwarding(t *testing.T) {
t.Parallel()
srv := newTestTLSServer(t)

Expand All @@ -4629,7 +4628,7 @@ func TestRoleVersions(t *testing.T) {
Enabled: types.NewBoolOption(true),
},
Local: &types.SSHLocalPortForwarding{
Enabled: types.NewBoolOption(false),
Enabled: types.NewBoolOption(true),
},
},
},
Expand All @@ -4653,42 +4652,39 @@ func TestRoleVersions(t *testing.T) {
},
})

user, err := CreateUser(context.Background(), srv.Auth(), "user", enabledRole, disabledRole)
implicitRole := newRole("test_role_implicit", types.V7, types.RoleSpecV6{
Allow: types.RoleConditions{
Rules: []types.Rule{
types.NewRule(types.KindRole, services.RW()),
},
},
Options: types.RoleOptions{},
})

legacyRole := newRole("test_role_legacy", types.V7, types.RoleSpecV6{
Allow: types.RoleConditions{
Rules: []types.Rule{
types.NewRule(types.KindRole, services.RW()),
},
},
Options: types.RoleOptions{
PortForwarding: types.NewBoolOption(false),
},
})

user, err := CreateUser(context.Background(), srv.Auth(), "user", enabledRole, disabledRole, implicitRole, legacyRole)
require.NoError(t, err)

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

for _, tc := range []struct {
desc string
clientVersions []string
expectError bool
inputRole types.Role
expectedRole types.Role
expectDowngraded bool
desc string
inputRole types.Role
expectedRole types.Role
}{
{
desc: "up to date - enabled",
clientVersions: []string{
// "16.4.9", api.Version, "",
"17.1.0",
},
inputRole: enabledRole,
expectedRole: enabledRole,
},
{
desc: "up to date - disabled",
clientVersions: []string{
"16.4.9", "17.0.1", "",
},
inputRole: disabledRole,
expectedRole: disabledRole,
},
{
desc: "downgrade SSH access control granularity - enabled",
clientVersions: []string{
"16.4.8",
},
desc: "enabled role",
inputRole: enabledRole,
expectedRole: newRole(enabledRole.GetName(), types.V7, types.RoleSpecV6{
Allow: types.RoleConditions{
Expand All @@ -4701,13 +4697,9 @@ func TestRoleVersions(t *testing.T) {
SSHPortForwarding: enabledRole.GetOptions().SSHPortForwarding,
},
}),
expectDowngraded: true,
},
{
desc: "downgrade SSH access control granularity - disabled",
clientVersions: []string{
"16.4.8",
},
desc: "disabled role",
inputRole: disabledRole,
expectedRole: newRole(disabledRole.GetName(), types.V7, types.RoleSpecV6{
Allow: types.RoleConditions{
Expand All @@ -4720,112 +4712,103 @@ func TestRoleVersions(t *testing.T) {
SSHPortForwarding: disabledRole.GetOptions().SSHPortForwarding,
},
}),
expectDowngraded: true,
},
{
desc: "implicit role",
inputRole: implicitRole,
expectedRole: newRole(implicitRole.GetName(), types.V7, types.RoleSpecV6{
Allow: types.RoleConditions{
Rules: []types.Rule{
types.NewRule(types.KindRole, services.RW()),
},
},
Options: types.RoleOptions{
PortForwarding: nil,
SSHPortForwarding: implicitRole.GetOptions().SSHPortForwarding,
},
}),
},
{
desc: "legacy role",
inputRole: legacyRole,
expectedRole: newRole(legacyRole.GetName(), types.V7, types.RoleSpecV6{
Allow: types.RoleConditions{
Rules: []types.Rule{
types.NewRule(types.KindRole, services.RW()),
},
},
Options: types.RoleOptions{
PortForwarding: legacyRole.GetOptions().PortForwarding,
SSHPortForwarding: legacyRole.GetOptions().SSHPortForwarding,
},
}),
},
} {
t.Run(tc.desc, func(t *testing.T) {
for _, clientVersion := range tc.clientVersions {
t.Run(clientVersion, func(t *testing.T) {
// setup client metadata
ctx := context.Background()
if clientVersion == "" {
ctx = context.WithValue(ctx, metadata.DisableInterceptors{}, struct{}{})
} else {
ctx = metadata.AddMetadataToContext(ctx, map[string]string{
metadata.VersionKey: clientVersion,
})
}
// setup client metadata
ctx := context.Background()
checkRole := func(gotRole types.Role) {
t.Helper()
require.Empty(t, cmp.Diff(tc.expectedRole, gotRole,
cmpopts.IgnoreFields(types.Metadata{}, "Revision", "Labels")))
}

checkRole := func(gotRole types.Role) {
t.Helper()
if tc.expectError {
return
}
require.Empty(t, cmp.Diff(tc.expectedRole, gotRole,
cmpopts.IgnoreFields(types.Metadata{}, "Revision", "Labels")))
// The downgraded label value won't match exactly because it
// includes the client version, so just check it's not empty
// and ignore it in the role diff.
if tc.expectDowngraded {
require.NotEmpty(t, gotRole.GetMetadata().Labels[types.TeleportDowngradedLabel])
}
}
checkErr := func(err error) {
t.Helper()
if tc.expectError {
require.Error(t, err)
} else {
require.NoError(t, err)
}
}
checkErr := func(err error) {
t.Helper()
require.NoError(t, err)
}

// Test GetRole
gotRole, err := client.GetRole(ctx, tc.inputRole.GetName())
checkErr(err)
checkRole(gotRole)

// Test GetRoles
gotRoles, err := client.GetRoles(ctx)
checkErr(err)
if !tc.expectError {
foundTestRole := false
for _, gotRole := range gotRoles {
if gotRole.GetName() != tc.inputRole.GetName() {
continue
}
checkRole(gotRole)
foundTestRole = true
break
}
require.True(t, foundTestRole, "GetRoles result does not include expected role")
}
// Test GetRole
gotRole, err := client.GetRole(ctx, tc.inputRole.GetName())
checkErr(err)
checkRole(gotRole)

// Test GetRoles
gotRoles, err := client.GetRoles(ctx)
checkErr(err)
foundTestRole := false
for _, gotRole := range gotRoles {
if gotRole.GetName() != tc.inputRole.GetName() {
continue
}
checkRole(gotRole)
foundTestRole = true
break
}
require.True(t, foundTestRole, "GetRoles result does not include expected role")

ctx, cancel := context.WithTimeout(ctx, 5*time.Second)
defer cancel()
ctx, cancel := context.WithTimeout(ctx, 5*time.Second)
defer cancel()

// Test WatchEvents
watcher, err := client.NewWatcher(ctx, types.Watch{Name: "roles", Kinds: []types.WatchKind{{Kind: types.KindRole}}})
require.NoError(t, err)
defer watcher.Close()
// Test WatchEvents
watcher, err := client.NewWatcher(ctx, types.Watch{Name: "roles", Kinds: []types.WatchKind{{Kind: types.KindRole}}})
require.NoError(t, err)
defer watcher.Close()

// Swallow the init event
e := <-watcher.Events()
require.Equal(t, types.OpInit, e.Type)
// Swallow the init event
e := <-watcher.Events()
require.Equal(t, types.OpInit, e.Type)

// Re-upsert the role so that the watcher sees it, do this
// on the auth server directly to avoid the
// TeleportDowngradedLabel check in ServerWithRoles
tc.inputRole, err = srv.Auth().UpsertRole(ctx, tc.inputRole)
require.NoError(t, err)
// Re-upsert the role so that the watcher sees it, do this
// on the auth server directly to avoid the
// TeleportDowngradedLabel check in ServerWithRoles
tc.inputRole, err = srv.Auth().UpsertRole(ctx, tc.inputRole)
require.NoError(t, err)

gotRole, err = func() (types.Role, error) {
for {
select {
case <-watcher.Done():
return nil, watcher.Error()
case e := <-watcher.Events():
if gotRole, ok := e.Resource.(types.Role); ok && gotRole.GetName() == tc.inputRole.GetName() {
return gotRole, nil
}
}
}
}()
checkErr(err)
checkRole(gotRole)

if !tc.expectError {
// Try to re-upsert the role we got. If it was
// downgraded, it should be rejected due to the
// TeleportDowngradedLabel
_, err = client.UpsertRole(ctx, gotRole)
if tc.expectDowngraded {
require.Error(t, err)
} else {
require.NoError(t, err)
gotRole, err = func() (types.Role, error) {
for {
select {
case <-watcher.Done():
return nil, watcher.Error()
case e := <-watcher.Events():
if gotRole, ok := e.Resource.(types.Role); ok && gotRole.GetName() == tc.inputRole.GetName() {
return gotRole, nil
}
}
})
}
}
}()
checkErr(err)
checkRole(gotRole)
})
}
}
Expand Down

0 comments on commit 7045356

Please sign in to comment.