Adds impersonation as a first class citizen #6009

klizhentas · 2021-03-16T03:30:05Z

allow:
  impersonate:
    users: ['alice', 'bob']
    roles: ['*']
    where: 'contains(user.spec.traits["groups"], impersonate_role.traits)'

klizhentas · 2021-03-17T23:19:40Z

@r0mant @fspmarshall can you folks please review? I'm adding more tests, but largely done with the implementation

lib/auth/auth_with_roles.go

lib/services/impersonate.go

lib/services/role.go

fspmarshall

General thoughts:

It should be possible to differentiate regular and impersonation certs in some manner (and possibly at least recover the root identity's name for audit log purposes).
An impersonated identity probably should not be able to perform impersonation itself (i.e. impersonation should not be recursive).
The lifetime of the impersonated cert should be limited by min(root_cert_exp, now + impersonated_identity_session_ttl). If folks want impersonation that escapes the expiry of the root identity's cert, that should be opt-in behavior (and if we do allow an escape mechanism, then impersonation definitely can't be recursive, or else you could just "ping-pong" between identities and your certs would never go invalid).

Additionally, I think we should have a rough idea of how we want this to interact with the upcoming locking feature before this gets merged. IMO impersonated identities should be affected both by locks that affect the impersonated identity, and locks that affect the root identity. If we think there is a chance that we will permit locks based on roles (likely I think), then things get extra tricky. Consider this scenario:

kind: user
metadata:
  name: alice
spec:
  roles: ['patriot']
---
kind: role
metadata:
  name: patriot
spec:
  allow:
    request:
      roles: ['spy']
  options:
    lockable: true # hypothetical
---
kind: role
metadata:
  name: spy
spec:
  allow:
    impersonate:
      users: ['bob']
      roles: ['enemy']

If alice assumes role spy via the access request system, she will now be able to impersonate bob with role enemy (assuming thats a valid user+role combination). Now say that a lock is applied which affects all users with role spy. Since alice's ability to act as bob is dependent upon role spy, she should probably lose the ability to act as bob.
Since alice holding role spy isn't a static assignment, we can't just say "this lock applies to the root identity behind this cert". We need to be able to say "this lock applies to the exact identity used to issue this impersonation cert".

fspmarshall · 2021-03-18T02:03:00Z

lib/services/role.go

+// newMatcherFn returns a matcher function based
+// on incoming values - if there is a wildcard matcher
+// present, it returns a wildcard matcher
+func newMatcherFn(in []string) func(in string) bool {
+	var set map[string]struct{}
+	for _, v := range in {
+		// there is no need to build a set if there is a wildcard
+		if v == Wildcard {
+			return matchAny
+		}
+		if set == nil {
+			set = make(map[string]struct{}, len(in))
+		}
+		set[v] = struct{}{}
+	}
+	return func(in string) bool { _, ok := set[in]; return ok }
+}


Unless there is some specific reason to only support literal + full wildcard matches, its probably preferable to user parse.NewMatcher, which will allow impersonate.roles to maintain parity with request.roles.

fspmarshall · 2021-03-18T04:00:38Z

api/types/role.go

+		if r.Spec.Deny.Impersonate.Where != "" {
+			return trace.BadParameter("'where' is not supported in deny.impersonate conditions")
+		}


Why not? Seems like that would be useful.

klizhentas · 2021-03-18T04:21:09Z

It should be possible to differentiate regular and impersonation certs in some manner (and possibly at least recover the root >identity's name for audit log purposes).
An impersonated identity probably should not be able to perform impersonation itself (i.e. impersonation should not be recursive).

Good points. I will bake in the impersonator in the cert, will add to the audit event and prevent recursive impersonation.

The lifetime of the impersonated cert should be limited by min(root_cert_exp, now + impersonated_identity_session_ttl). If folks > want impersonation that escapes the expiry of the root identity's cert, that should be opt-in behavior (and if we do allow an >escape mechanism, then impersonation definitely can't be recursive, or else you could just "ping-pong" between identities and your certs would never go invalid).

Good point, I will restrict the TTL

klizhentas · 2021-03-18T04:23:03Z

Re: locks

If we create a lock per identity, it should apply to certs of the impersonator. In your example, if we lock Alice by identity, then Alice's certs, including impersonated certs will be locked out, because her identity will be baked in the cert as impersonator.

If we create a lock per role, Alice's cert impersonating role that is not locked will not be locked as well, which I think is OK.

lib/auth/auth_with_roles.go

klizhentas · 2021-03-18T21:23:54Z

@fspmarshall take another look, I have:

Prevented recursive impersonation
Added impersonator in client SSH and TLS certs
Limited TTL of impersonating users to the TTL of their cert
Added impersonator to events and updated almost all audit events

lib/auth/auth_with_roles.go

klizhentas · 2021-03-18T23:17:46Z

@fspmarshall applied your comments on renewal of impersonated users, made sure they are not escaping, but allowed them to renew their own certs

fspmarshall

@klizhentas LGMT.

(linking #5441 here to remind me that these are gonna need some care when merging)

lib/auth/auth_with_roles.go

russjones · 2021-03-19T21:26:34Z

lib/tlsca/parsegen.go

@@ -218,6 +218,21 @@ func ParsePublicKeyDER(der []byte) (crypto.PublicKey, error) {
 	return generalKey, nil
 }

+// MarshalPrivateKeyPEM marshals RSA private key to PEM.
+// Current implementation returns error for any other type of private key.
+func MarshalPrivateKeyPEM(key interface{}) ([]byte, error) {


This is implemented in utils.MarshalPrivateKey.

russjones · 2021-03-19T21:37:56Z

lib/auth/auth_with_roles.go

+		req.Expires = a.authServer.GetClock().Now().Add(ttl)
+		if err != nil {
+			log.Warning(err)
+			err := trace.AccessDenied("user %q has requested to generate certs for %q.", a.context.User.GetName(), roles)


How will admins using Cloud debug this?

I don't have a good answer to that question at the moment either, but @alex-kovoy has mentioned that he has to fetch process logs to help customers debug SAML connectors. I imagine a similar situation will happen here.

Fixes #5352 ```yaml allow: impersonate: users: ['alice', 'bob'] roles: ['*'] where: 'contains(user.spec.traits["groups"], impersonate_role.traits)' ``` Adds "impersonator" to all X.509 and SSH client certs issued using impersonation and does best effort to track requests by impersonators in audit events. Limits certs TTL to the impersonator's max session TTL. Prevents impersonating users to recursively impersonate other users. Allows impersonating users to renew their own certificate, for example to set route to cluster. Adds missing token permission for editor role.

klizhentas force-pushed the sasha/impersonate branch 2 times, most recently from e86d27d to f7341c4 Compare March 17, 2021 19:36

klizhentas marked this pull request as ready for review March 17, 2021 19:44

klizhentas requested review from r0mant and russjones as code owners March 17, 2021 19:44

klizhentas force-pushed the sasha/impersonate branch 2 times, most recently from 0d26a83 to fb7d9dc Compare March 17, 2021 23:15

klizhentas requested a review from fspmarshall March 17, 2021 23:19

klizhentas force-pushed the sasha/impersonate branch 3 times, most recently from 9f3d37e to fed4970 Compare March 18, 2021 00:27

r0mant approved these changes Mar 18, 2021

View reviewed changes

fspmarshall reviewed Mar 18, 2021

View reviewed changes

klizhentas force-pushed the sasha/impersonate branch from fed4970 to d496435 Compare March 18, 2021 03:57

fspmarshall reviewed Mar 18, 2021

View reviewed changes

klizhentas force-pushed the sasha/impersonate branch from d496435 to ec09beb Compare March 18, 2021 04:17

fspmarshall reviewed Mar 18, 2021

View reviewed changes

lib/auth/auth_with_roles.go Outdated Show resolved Hide resolved

klizhentas force-pushed the sasha/impersonate branch 3 times, most recently from c6c5bb1 to 426e6ca Compare March 18, 2021 21:21

klizhentas changed the title ~~work in progress impersonation~~ Adds impersonation as a first class citizen Mar 18, 2021

klizhentas requested review from fspmarshall and r0mant March 18, 2021 21:22

klizhentas force-pushed the sasha/impersonate branch from 426e6ca to 0fe9ef4 Compare March 18, 2021 21:31

fspmarshall reviewed Mar 18, 2021

View reviewed changes

lib/auth/auth_with_roles.go Outdated Show resolved Hide resolved

lib/auth/auth_with_roles.go Show resolved Hide resolved

klizhentas force-pushed the sasha/impersonate branch from 0fe9ef4 to e500f9b Compare March 18, 2021 23:17

klizhentas force-pushed the sasha/impersonate branch 2 times, most recently from dbdf4cc to c6d957b Compare March 18, 2021 23:20

fspmarshall approved these changes Mar 18, 2021

View reviewed changes

lib/auth/auth_with_roles.go Outdated Show resolved Hide resolved

klizhentas force-pushed the sasha/impersonate branch 2 times, most recently from 5085237 to e53a97b Compare March 19, 2021 20:21

russjones reviewed Mar 19, 2021

View reviewed changes

russjones approved these changes Mar 19, 2021

View reviewed changes

klizhentas force-pushed the sasha/impersonate branch from e53a97b to cb6e697 Compare March 19, 2021 21:54

klizhentas merged commit 05f6294 into branch/v6 Mar 19, 2021

klizhentas deleted the sasha/impersonate branch March 19, 2021 22:21

webvictim mentioned this pull request May 5, 2021

ami auto branch 1620236164 #6745

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds impersonation as a first class citizen #6009

Adds impersonation as a first class citizen #6009

klizhentas commented Mar 16, 2021

klizhentas commented Mar 17, 2021

fspmarshall left a comment •

edited

Loading

fspmarshall Mar 18, 2021

fspmarshall Mar 18, 2021

klizhentas commented Mar 18, 2021

klizhentas commented Mar 18, 2021

klizhentas commented Mar 18, 2021

klizhentas commented Mar 18, 2021

fspmarshall left a comment

russjones Mar 19, 2021

russjones Mar 19, 2021

Adds impersonation as a first class citizen #6009

Adds impersonation as a first class citizen #6009

Conversation

klizhentas commented Mar 16, 2021

klizhentas commented Mar 17, 2021

fspmarshall left a comment • edited Loading

Choose a reason for hiding this comment

fspmarshall Mar 18, 2021

Choose a reason for hiding this comment

fspmarshall Mar 18, 2021

Choose a reason for hiding this comment

klizhentas commented Mar 18, 2021

klizhentas commented Mar 18, 2021

klizhentas commented Mar 18, 2021

klizhentas commented Mar 18, 2021

fspmarshall left a comment

Choose a reason for hiding this comment

russjones Mar 19, 2021

Choose a reason for hiding this comment

russjones Mar 19, 2021

Choose a reason for hiding this comment

fspmarshall left a comment •

edited

Loading