diff --git a/lib/sshca/identity.go b/lib/sshca/identity.go index 492fa116a69..2a1a7e6e86f 100644 --- a/lib/sshca/identity.go +++ b/lib/sshca/identity.go @@ -531,12 +531,19 @@ func DecodeIdentity(cert *ssh.Certificate) (*Identity, error) { allowedResourceAccessIDs = resourceAccessIDs } if len(allowedResourceAccessIDs) > 0 { - // Prefer new extension when present, old extension is redundant - // (exists for backward-compat with older agents/proxies). + // Prefer new extension when present. ident.AllowedResourceAccessIDs = allowedResourceAccessIDs + // Populate AllowedResourceIDs with any present unconstrained resources, + // so any path re-encoding this identity persists the resourceIDs + // to the legacy extension instead of adding a sentinel. + // + // TODO(kiosion): DELETE in 20.0.0 + ident.AllowedResourceIDs, _ = types.UnwrapResourceAccessIDs(allowedResourceAccessIDs) } else if len(allowedResourceIDs) > 0 { - // Fallback for certs from older auth servers that don't write the new extension. + // Fallback for certs from older Auths that don't write the new extension. ident.AllowedResourceAccessIDs = types.CombineAsResourceAccessIDs(allowedResourceIDs, nil) + //nolint:staticcheck // TODO(kiosion): deprecated, to be removed in v20 + ident.AllowedResourceIDs = allowedResourceIDs } ident.ConnectionDiagnosticID = takeValue(teleport.CertExtensionConnectionDiagnosticID) diff --git a/lib/sshca/identity_test.go b/lib/sshca/identity_test.go index d5c80a6acd6..4990d0db703 100644 --- a/lib/sshca/identity_test.go +++ b/lib/sshca/identity_test.go @@ -24,6 +24,7 @@ import ( "time" "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/crypto/ssh" "google.golang.org/protobuf/testing/protocmp" @@ -159,3 +160,94 @@ func TestIdentityConversion(t *testing.T) { require.Empty(t, cmp.Diff(ident, ident2, protocmp.Transform())) } + +// TestAllowedResources_SSHEncodeDecode verifies that AllowedResourceIDs and +// AllowedResourceAccessIDs are populated correctly after an SSH cert's +// encode-decode cycle across all resource mix permutations. +func TestAllowedResources_SSHEncodeDecode(t *testing.T) { + plainNode := types.ResourceID{ClusterName: "cluster", Kind: types.KindNode, Name: "prod-node"} + plainDB := types.ResourceID{ClusterName: "cluster", Kind: types.KindDatabase, Name: "prod-db"} + constrainedApp := types.ResourceAccessID{ + Id: types.ResourceID{ClusterName: "cluster", Kind: types.KindApp, Name: "aws-console"}, + Constraints: &types.ResourceConstraints{ + Version: types.V1, + Details: &types.ResourceConstraints_AwsConsole{ + AwsConsole: &types.AWSConsoleResourceConstraints{ + RoleArns: []string{"arn:aws:iam::123456789012:role/DevOps"}, + }, + }, + }, + } + + tcs := []struct { + name string + allowedResourceIDs []types.ResourceID + allowedResourceAccessIDs []types.ResourceAccessID + wantAllowedResourceIDs []types.ResourceID + wantAllowedResourceAccessIDs []types.ResourceAccessID + }{ + { + name: "plain resources only (new auth cert)", + allowedResourceIDs: []types.ResourceID{plainNode, plainDB}, + allowedResourceAccessIDs: types.ResourceIDsToResourceAccessIDs([]types.ResourceID{plainNode, plainDB}), + wantAllowedResourceIDs: []types.ResourceID{plainNode, plainDB}, + wantAllowedResourceAccessIDs: types.ResourceIDsToResourceAccessIDs([]types.ResourceID{plainNode, plainDB}), + }, + { + name: "constrained resources only (new auth cert)", + allowedResourceIDs: nil, + allowedResourceAccessIDs: []types.ResourceAccessID{constrainedApp}, + wantAllowedResourceIDs: nil, + wantAllowedResourceAccessIDs: []types.ResourceAccessID{constrainedApp}, + }, + { + name: "mixed plain and constrained (new auth cert)", + allowedResourceIDs: []types.ResourceID{plainNode}, + allowedResourceAccessIDs: append(types.ResourceIDsToResourceAccessIDs([]types.ResourceID{plainNode}), constrainedApp), + wantAllowedResourceIDs: []types.ResourceID{plainNode}, + wantAllowedResourceAccessIDs: append( + types.ResourceIDsToResourceAccessIDs([]types.ResourceID{plainNode}), + constrainedApp, + ), + }, + { + name: "old auth cert (only old extension)", + allowedResourceIDs: []types.ResourceID{plainNode, plainDB}, + allowedResourceAccessIDs: nil, + wantAllowedResourceIDs: []types.ResourceID{plainNode, plainDB}, + wantAllowedResourceAccessIDs: types.ResourceIDsToResourceAccessIDs([]types.ResourceID{plainNode, plainDB}), + }, + } + + for _, tt := range tcs { + t.Run(tt.name, func(t *testing.T) { + ident := &Identity{ + ValidBefore: uint64(time.Now().Add(time.Hour).Unix()), + CertType: ssh.UserCert, + Username: "test-user", + Roles: []string{"access"}, + //nolint:staticcheck // testing deprecated field + AllowedResourceIDs: tt.allowedResourceIDs, + AllowedResourceAccessIDs: tt.allowedResourceAccessIDs, + } + + cert, err := ident.Encode(constants.CertificateFormatStandard) + require.NoError(t, err) + + decoded, err := DecodeIdentity(cert) + require.NoError(t, err) + + assert.ElementsMatch(t, tt.wantAllowedResourceAccessIDs, decoded.AllowedResourceAccessIDs, + "AllowedResourceAccessIDs mismatch") + //nolint:staticcheck // testing deprecated field + assert.ElementsMatch(t, tt.wantAllowedResourceIDs, decoded.AllowedResourceIDs, + "AllowedResourceIDs mismatch") + + //nolint:staticcheck // testing deprecated field + for _, rid := range decoded.AllowedResourceIDs { + assert.False(t, types.IsSentinelResourceID(rid), + "sentinel value not expected in decoded AllowedResourceIDs") + } + }) + } +} diff --git a/lib/tlsca/ca.go b/lib/tlsca/ca.go index 878b69ae3db..874af2e76f9 100644 --- a/lib/tlsca/ca.go +++ b/lib/tlsca/ca.go @@ -1476,12 +1476,20 @@ func FromSubject(subject pkix.Name, expires time.Time) (*Identity, error) { } if len(allowedResourceAccessIDs) > 0 { - // Prefer new extension when present, old extension is redundant - // (exists for backward-compat with older agents/proxies). + // Prefer new extension when present. id.AllowedResourceAccessIDs = allowedResourceAccessIDs + // Populate AllowedResourceIDs with any present unconstrained resources, + // so any path re-encoding this identity (e.g., database proxy CSR signing) + // persists the resourceIDs to the legacy extension, instead of adding a + // sentinel. + // + // TODO(kiosion): DELETE in 20.0.0 + id.AllowedResourceIDs, _ = types.UnwrapResourceAccessIDs(allowedResourceAccessIDs) } else if len(allowedResourceIDs) > 0 { - // Fallback for certs from older auth servers that don't write the new extension. + // Fallback for certs from older Auths that don't write the new extension. id.AllowedResourceAccessIDs = types.CombineAsResourceAccessIDs(allowedResourceIDs, nil) + //nolint:staticcheck // TODO(kiosion): deprecated, to be removed in v20 + id.AllowedResourceIDs = allowedResourceIDs } if err := id.CheckAndSetDefaults(); err != nil { diff --git a/lib/tlsca/ca_test.go b/lib/tlsca/ca_test.go index b708bd51cd8..9bbc76bb940 100644 --- a/lib/tlsca/ca_test.go +++ b/lib/tlsca/ca_test.go @@ -494,6 +494,130 @@ func TestIdentity_ToFromSubject(t *testing.T) { } } +// TestAllowedResources_EncodeDecodeCycle verifies that AllowedResourceIDs and +// AllowedResourceAccessIDs survive encode-decode-re-encode cycles correctly +// across all resource mix permutations. The re-encode step simulates the +// database proxy CSR signing path. +func TestAllowedResources_EncodeDecodeCycle(t *testing.T) { + plainNode := types.ResourceID{ClusterName: "cluster", Kind: types.KindNode, Name: "prod-node"} + plainDB := types.ResourceID{ClusterName: "cluster", Kind: types.KindDatabase, Name: "prod-db"} + constrainedApp := types.ResourceAccessID{ + Id: types.ResourceID{ClusterName: "cluster", Kind: types.KindApp, Name: "aws-console"}, + Constraints: &types.ResourceConstraints{ + Version: types.V1, + Details: &types.ResourceConstraints_AwsConsole{ + AwsConsole: &types.AWSConsoleResourceConstraints{ + RoleArns: []string{"arn:aws:iam::123456789012:role/DevOps"}, + }, + }, + }, + } + sentinel := types.CreateSentinelResourceID() + + tests := []struct { + name string + // identity fields set before first encode + allowedResourceIDs []types.ResourceID + allowedResourceAccessIDs []types.ResourceAccessID + // expected state after decode + wantAllowedResourceIDs []types.ResourceID + wantAllowedResourceAccessIDs []types.ResourceAccessID + // expected contents of the legacy extension after re-encode + wantLegacyExtension []types.ResourceID + }{ + { + name: "plain resources only (new auth cert)", + allowedResourceIDs: []types.ResourceID{plainNode, plainDB}, + allowedResourceAccessIDs: types.ResourceIDsToResourceAccessIDs([]types.ResourceID{plainNode, plainDB}), + wantAllowedResourceIDs: []types.ResourceID{plainNode, plainDB}, + wantAllowedResourceAccessIDs: types.ResourceIDsToResourceAccessIDs([]types.ResourceID{plainNode, plainDB}), + wantLegacyExtension: []types.ResourceID{plainNode, plainDB}, + }, + { + name: "constrained resources only (new auth cert)", + allowedResourceIDs: nil, + allowedResourceAccessIDs: []types.ResourceAccessID{constrainedApp}, + wantAllowedResourceIDs: nil, + wantAllowedResourceAccessIDs: []types.ResourceAccessID{constrainedApp}, + wantLegacyExtension: []types.ResourceID{sentinel}, + }, + { + name: "mixed plain and constrained (new auth cert)", + allowedResourceIDs: []types.ResourceID{plainNode}, + allowedResourceAccessIDs: append(types.ResourceIDsToResourceAccessIDs([]types.ResourceID{plainNode}), constrainedApp), + wantAllowedResourceIDs: []types.ResourceID{plainNode}, + wantAllowedResourceAccessIDs: append( + types.ResourceIDsToResourceAccessIDs([]types.ResourceID{plainNode}), + constrainedApp, + ), + wantLegacyExtension: []types.ResourceID{plainNode}, + }, + { + name: "old auth cert (only old extension, no new extension)", + allowedResourceIDs: []types.ResourceID{plainNode, plainDB}, + allowedResourceAccessIDs: nil, + wantAllowedResourceIDs: []types.ResourceID{plainNode, plainDB}, + wantAllowedResourceAccessIDs: types.ResourceIDsToResourceAccessIDs([]types.ResourceID{plainNode, plainDB}), + wantLegacyExtension: []types.ResourceID{plainNode, plainDB}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + identity := &Identity{ + Username: "test-user", + Groups: []string{"access"}, + //nolint:staticcheck // testing deprecated field + AllowedResourceIDs: tt.allowedResourceIDs, + AllowedResourceAccessIDs: tt.allowedResourceAccessIDs, + } + + // Encode then decode + subj, err := identity.Subject() + require.NoError(t, err) + subj.Names = append(subj.Names, subj.ExtraNames...) + subj.ExtraNames = nil + + decoded, err := FromSubject(subj, time.Time{}) + require.NoError(t, err) + + assert.ElementsMatch(t, tt.wantAllowedResourceAccessIDs, decoded.AllowedResourceAccessIDs) + //nolint:staticcheck // testing deprecated field + assert.ElementsMatch(t, tt.wantAllowedResourceIDs, decoded.AllowedResourceIDs) + + // Re-encode, verify legacy extension, decode again + subj2, err := decoded.Subject() + require.NoError(t, err) + assert.ElementsMatch(t, tt.wantLegacyExtension, legacyExtensionIDs(t, subj2)) + + subj2.Names = append(subj2.Names, subj2.ExtraNames...) + subj2.ExtraNames = nil + + roundtripped, err := FromSubject(subj2, time.Time{}) + require.NoError(t, err) + + assert.ElementsMatch(t, decoded.AllowedResourceAccessIDs, roundtripped.AllowedResourceAccessIDs) + //nolint:staticcheck // testing deprecated field + assert.ElementsMatch(t, decoded.AllowedResourceIDs, roundtripped.AllowedResourceIDs) + }) + } +} + +// legacyExtensionIDs extracts ResourceIDs from the legacy AllowedResources +// extension (OID 1.3.9999.2.10) in a pkix.Name subject. This is what an +// old agent would parse from the cert. +func legacyExtensionIDs(t *testing.T, subj pkix.Name) []types.ResourceID { + t.Helper() + for _, name := range subj.ExtraNames { + if name.Type.Equal(AllowedResourcesASN1ExtensionOID) { + ids, err := types.ResourceIDsFromString(name.Value.(string)) + require.NoError(t, err) + return ids + } + } + return nil +} + func TestGCPExtensions(t *testing.T) { clock := clockwork.NewFakeClock() ca, err := FromKeys([]byte(fixtures.TLSCACertPEM), []byte(fixtures.TLSCAKeyPEM))