Propagate external traits to leaf clusters #6540
Conversation
lib/auth/permissions.go
Outdated
| // behavior, filter out any "internal" traits when applying traits from | ||
| // user identity. | ||
| for k, v := range u.Identity.Traits { | ||
| if !teleport.IsInternalTrait(k) { |
There was a problem hiding this comment.
optional: you could check whether k is already in traits instead of using a helper function
that way you have 1 fewer places to maintain "internal" traits
integration/integration_test.go
Outdated
| // roles in root and leaf clusters. | ||
| func TestTraitsPropagation(t *testing.T) { | ||
| log := testlog.FailureOnly(t) | ||
| startPort := utils.PortStartingNumber + (5 * AllocatePortsNum) + 1 |
There was a problem hiding this comment.
It was needed to avoid conflicts with ports allocated by other tests. It was pretty unfortunate, we would just do it by doing 3*AllocatePortsNum, 4*AllocatePortsNum etc. in different tests until now.
I have refactored this to allocate ports for all integration tests once when they start so individual tests/suites don't need to do that anymore. Hopefully we don't run out of ports, I made it 5000 but I don't think our integration tests actually need that many.
lib/auth/permissions.go
Outdated
| // Prior to Teleport 6.2 no user traits were passed to remote clusters | ||
| // except for those specified above. To preserve backwards compatible | ||
| // behavior, filter out any "internal" traits when applying traits from | ||
| // user identity. | ||
| for k, v := range u.Identity.Traits { | ||
| if _, ok := traits[k]; !ok { | ||
| traits[k] = v | ||
| } | ||
| } |
There was a problem hiding this comment.
Two questions:
- I'm not seeing the filtering out of internal traits mentioned in the comment?
- When can the second condition be met? Looks like you're just copying the map right? Shouldn't get duplicate keys ever?
There was a problem hiding this comment.
@russjones The filtering is the if _, ok := traits[k]; !ok { check - we're basically filtering out any traits which are already present in the traits above which contains only internal ones. In my original implementation I had an explicit function IsInternalTrait that would check if the trait is one of the "logins", "kube groups/users" or "db names/users", but then Andrew pointed out that I can check if the key is already in that traits map, which I did.
When can the second condition be met?
Not sure which condition you mean? We're copying traits from the certificate essentially into the traits map and skipping internal ones to make sure to preserve backwards compatible behavior.
| // extractRolesFromCert extracts roles from certificate metadata extensions. | ||
| func extractRolesFromCert(cert *ssh.Certificate) ([]string, error) { | ||
| data, ok := cert.Extensions[teleport.CertExtensionTeleportRoles] | ||
| if !ok { | ||
| // it's ok to not have any roles in the metadata | ||
| return nil, nil | ||
| } | ||
| return services.UnmarshalCertRoles(data) | ||
| } |
There was a problem hiding this comment.
I think this was for old style SSH certificates, do you know if we've dropped support for them?
There was a problem hiding this comment.
@russjones This logic is still being used - this was just a duplicate function. I've exported extractRolesFromCert and extractTraitsFromCert to use here instead (accounting for the possibility that they may return NotFound in case there are no roles/traits in old-style certs).
r0mant
force-pushed
the
roman/traits
branch
2 times, most recently
from
8164ca7 to
71589ef
Compare
This pull request makes sure that external user traits (usually populated from OIDC/SAML claims/attributes) are propagated to leaf clusters and can be used in leaf role templating. I have extracted this part from the larger AAP headers passthrough work since it felt like this could be a somewhat independent change.
This allows users to have, for example, identical roles in both clusters that grant access based on the user's traits, which is the behavior some customers expect. Example role node labels:
Fixes #6389.