mirror of
https://github.com/gravitational/teleport.git
synced 2026-09-24 16:17:11 +08:00
Always populate AllowedResourceIDs on decoded Identity (#66870)
* fix: Always populate AllowedResourceIDs on decoded Identity - TLS and SSH cert decoders populated AllowedResourceAccessIDs but left AllowedResourceIDs nil. Any path decoding an identity and re-encoding via Subject() would result in the encoder injecting a sentinel value into the legacy field, and storing all resources under the new AllowedResourceAccessIDs field only. This specifically broke database access in mixed-version clusters: Proxy would decode user cert, build a CSR (Subject()), send it to Auth for signing (FromSubject() then Subject() again). This dropped AllowedResourceIDs, storing all under the new extension, so older database agents received the cert with only the sentinel in the legacy extension, and unaware of the new extension, denied access. Fix by populating AllowedResourceIDs at decode time in both decoders via UnwrapResourceAccessIDs, ensuring and unconstrained resources persist across decode-re-encode cycles. * refac: tidy ca_test encode/decode test
This commit is contained in:
Maxim
+10
-3
@@ -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)
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
+11
-3
@@ -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 {
|
||||
|
||||
@@ -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))
|
||||
|
||||
Reference in New Issue
Block a user