Fix role mapping for trusted clusters #3253
Conversation
This commit fixes #3252 Security patches 4.2 introduced a regression - leaf clusters ignore role mapping and attempt to use role names coming from identity of the root cluster whenever GetNodes method was used. This commit reverts back the logic, however it ensures that the original fix is preserved - traits and groups are updated on the user object. Integration test has been extended to avoid the regression in the future.
| @@ -1401,7 +1400,7 @@ func isFormatOld(cert *ssh.Certificate) bool { | |||
| _, hasRoles := cert.Extensions[teleport.CertExtensionTeleportRoles] | |||
| _, hasTraits := cert.Extensions[teleport.CertExtensionTeleportTraits] | |||
|
|
|||
| if hasRoles && hasTraits { | |||
| if hasRoles || hasTraits { | |||
There was a problem hiding this comment.
Just making sure you intended to change the logic here, ignore if it's on purpose.
| if err != nil { | ||
| return nil, trace.Wrap(err) | ||
| } | ||
| if len(traits) > 0 { |
There was a problem hiding this comment.
Pretty sure this is always true since traits is going to marshall to {} or null when empty.
There was a problem hiding this comment.
What about this? Same for lib/auth/native.go.
if len(c.Traits) > 0 {
traits, err := wrappers.MarshalTraits(&c.Traits)
if err != nil {
return nil, trace.Wrap(err)
}
cert.Permissions.Extensions[teleport.CertExtensionTeleportTraits] = string(traits)
}There was a problem hiding this comment.
the traits were omitted in my tests
There was a problem hiding this comment.
Just wrote a manual test, and MarshalTraits always returned either {} or null depending on whether the *wrappers.Traits value was empty or nil... 😕
Did your tests verify that the cert extension was actually omitted, or just that no traits were present in the decoded identity?
|
retest this please |
4 similar comments
|
retest this please |
|
retest this please |
|
retest this please |
|
retest this please |
This commit fixes #3252
Security patches 4.2 introduced a regression - leaf clusters ignore role mapping
and attempt to use role names coming from identity of the root cluster
whenever GetNodes method was used.
This commit reverts back the logic, however it ensures that the original
fix is preserved - traits and groups are updated on the user object.
Integration test has been extended to avoid the regression in the future, also manual testing with docker compose was done to verify that issue has been fixed.