Adds support for kube_users, extend interpolation #3404

klizhentas · 2020-03-04T17:24:23Z

This commit fixes #3369, refs #3374

Corresponding teleport.e commit:

https://github.com/gravitational/teleport.e/pull/128

It adds support for kubernetes_users section in roles,
allowing Teleport proxy to impersonate user identities.

It also extends variable interpolation syntax by adding
suffix and prefix to variables and function email.local:

Example:

kind: role
version: v3
metadata:
  name: admin
spec:
  allow:
    # extract email local part from the email claim
    logins: ['{{email.local(external.email)}}']

    # impersonate a kubernetes user with IAM prefix
    kubernetes_users: ['IAM#{{external.email}}']

  # the deny section uses the identical format as the 'allow' section.
  # the deny rules always override allow rules.
  deny: {}

Some notes on email.local behavior:

This is the only function supported in the template variables for now
In case if the email.local will encounter invalid email address,
it will interpolate to empty value, will be removed from resulting
output.

Changes in impersonation behavior:

If kubernetes_users part of the resulting role set will have one entry,
forwarding proxy will add this user to impersonate headers of the request, because
there is no ambiguity.
If kubernetes_users part of the resulting role set will have multiple entries,
forwarding proxy will not add any impersonating user headers by default, users
will have to set impersonate user headers.
Previous versions of the forwarding proxy were denying all external
impersonation headers, this commit allows 'Impesrsonate-User' and
'Impersonate-Group' header values that are allowed by role set.
Previous versions of the forwarding proxy ignored 'Deny' section of the roles
when applied to impersonation, this commit fixes that - roles with deny
kubernetes_users and kubernetes_groups section will not allow
impersonation of those users and groups.

benarent · 2020-03-04T18:04:33Z

👍 Looks like this solves a couple of customer issues. Have you tested it with Trusted Cluster? We've had a couple related bugs come up recently in regards to mapping. #3402 & #3376 It'll be good to fix this issue as many customer will deploy K8s as Trusted Clusters.

klizhentas · 2020-03-04T18:32:32Z

@benarent not sure I follow, are you asking if I can add two more issue fixes to this PR?

klizhentas · 2020-03-04T18:38:59Z

retest this please

fspmarshall · 2020-03-04T18:15:34Z

lib/kube/proxy/forwarder.go

+		if len(ctx.kubeUsers) == 1 {
+			for user := range ctx.kubeUsers {
+				impersonateUser = user
+				log.Debugf("Setting impersonate user to %v: defaulting single value from roles", user)
+				break
+			}
+		} else {


Why is holding a single user given special treatment? This seems like it could create confusing behavior.

E.G. alice has role dev. Alice impersonates herself by default. Bob adds admin user to the role dev. Now alice impersonates admin. Bob then adds testing user to dev. Alice is now back to impersonating herself. From alice's point of view, this is very confusing.

This is to allow a default: it does not matter how many roles Alice holds, as long as the resulting role set only specifies one user to impersonate, the system will default to it. In your example, if Bob adds admin user to the role dev, the system will not be able to choose a default, because there are two users in the set, and will ask Alice to pick one.

the system will not be able to choose a default, because there are two users in the set, and will ask Alice to pick one.

Seems that impersonateUser = ctx.User.GetName() always runs unless len(ctx.kubeUsers) == 1... maybe I'm misunderstanding, but that looks to me like alice will initially impersonate herself, start impersonating the first user added to any role she holds, then go back to impersonating herself as soon as any role adds an additional user.

yes, you are right, that would be the effect, I'm wondering what would be a better default then:

Alice would not be impersonating any user by default unless specified explicitly

Alice would be impersonating herself by default at all times, unless specified otherwise and impersonating herself explicitly will be allowed

Alice would be impersonating only users set in the kubernetes_users from now on (if there is only one user specified) and no other impersonation will be allowed

fspmarshall · 2020-03-04T18:17:15Z

lib/kube/proxy/forwarder.go

+			if _, ok := ctx.kubeUsers[impersonateUser]; !ok {
+				return trace.AccessDenied("%v, user header %q is not allowed in roles", ImpersonationRequestDeniedMessage, impersonateUser)
+			}


Will this reject a user attempting to impersonate their own name? If so, this is potentially confusing since users are allowed to impersonate themselves as a default behavior.

Their own name could mean something else in the context of the Kubernetes cluster, e.g. IAM role with some privileges. That's why impersonation is fully controlled by roles and there are no default allowed values.

That's why impersonation is fully controlled by roles and there are no default allowed values.

This line looks like it causes users to impersonate their own usernames by default, regardless of what the roles say.

Got it, thanks for pointing it out, that was a behavior before kuberenetes_users, so I tried to preserve the default behavior of the system, but now I see how it can be confusing. I wonder if we should always allow users to impersonate themselves then 🤔

I don't know the ramifications of kubernetes user impersonation well enough to comment on whether self-impersonation is a desirable default... I will say that I prefer it over breaking existing deployments (though I think it should either always or never be the default).

If we don't want to keep self-impersonation as the default, perhaps we could provide a migration which allowed existing deployments to keep their behavior? E.G.:

options: k8s_user_passthrough: yes

The above could resolve to no if no roles specify it, resolve to yes if one or more roles specify yes (which could be set by a migration), and resolve to no if any role explicitly specifies no.

@fspmarshall I think this will be reasonable but a bit confusing behavior to explain to users and admins:

By default, if no kubernetes_users is set (which will be a majority), user will impersonate themselves

As long as at least one kubernetes_users is set, it will start controlling the overall list of users allowed by the client to impersonate. If it does not include actual user name, well, will be rejected (otherwise there will be no way to exclude the user from the list)

If the kuberentes_users set includes only one user (many times that's the real intent), teleport will default to it, otherwise it will refuse to select. This will enable IAM#{{external.email}} use case that is the driver for this feature.

@klizhentas sounds good, that does feel easier to explain.

fspmarshall · 2020-03-04T18:32:09Z

lib/tlsca/ca.go

+	// DELETE IN (4.4.0)
+	// Groups are marshaled to both ASN1 extension
+	// and old Province section, for backwards-compatibility,
+	// however begin migration to ASN1 extensions in the future
+	// for this and other properties


If we're going to delete this on a minor version change, we should definitely add checks in tsh to warn about outdated cert format when loading identity files.

Maybe we should start explicitly versioning the teleport certificate format and producing readable errors on version incompatibility. Certificate incompatibility has the potential to fail silently in very confusing ways.

We can delete this in major, overall it should not have any impact if we just skip one version, because the new cert fields are not used on the tsh client, are only in memory in proxy and are not stored anywhere, so it will be safe to delete with no backwards compatibility problems.

Oops, this comment was more directed at the parsing side of things, see below.

klizhentas · 2020-03-04T22:52:56Z

retest this please

klizhentas · 2020-03-04T23:13:22Z

retest this please

fspmarshall · 2020-03-04T23:40:28Z

lib/tlsca/ca.go

+	// DELETE IN(4.5.0): This logic is using Province field
+	// from subject in case if Kubernetes groups were not populated
+	// from ASN1 extension
+	if len(id.KubernetesGroups) == 0 {
+		id.KubernetesGroups = subject.Province
+	}


We should either keep this around longer or make a non-empty Province become an error. Silently pruning kubernetes groups on older certs (such as those exported with tctl auth sign) would be very confusing.

I'm ok to keep this longer let's say to 5.0 release

klizhentas · 2020-03-05T00:34:54Z

retest this please

russjones · 2020-03-05T01:08:01Z

lib/utils/parse/parse.go


 	switch n := node.(type) {
+	case *ast.CallExpr:


// Parse call expression. At the moment only selector expressions are supported to allow // transformation of data. For example, email.local(foo@example.com) -> foo@example.com.

russjones · 2020-03-05T01:11:42Z

lib/utils/parse/parse.go

+		case *ast.Ident:
+			return nil, trace.BadParameter("function %v is not supported", call.Name)
+		case *ast.SelectorExpr:
+			namespace, ok := call.X.(*ast.Ident)


// Extract name of transformation function.

russjones · 2020-03-05T01:12:01Z

lib/utils/parse/parse.go

+				return nil, trace.BadParameter("expected 1 argument for email.local got %v", len(n.Args))
+			}
+			result.transform = EmailLocal
+			ret, err := walk(n.Args[0])


// Extract email address.

russjones · 2020-03-05T01:29:09Z

lib/tlsca/ca.go

 	subject.Province = append([]string{}, id.KubernetesGroups...)
 	subject.StreetAddress = []string{id.RouteToCluster}
 	subject.PostalCode = []string{string(rawTraits)}

+	for i := range id.KubernetesUsers {
+		kubeUser := id.KubernetesUsers[i]
+		subject.ExtraNames = append(subject.ExtraNames,


If we're moving data out of the subject, why not move this information to the extensions? That where the SAN, key usage, and other metadata now lives.

what's the benefit of doing that vs keeping it in subject?

with keeping it in subject, there is a benefit of locality as we plan on keeping other things like common name to be used for groups, unless we want to completely stop using the subject.

I thought that is what was being suggested in one of the comments, to get everything off of subject.{Organization, OrganizationUnit, Locality} in the future?

I don't have a technical argument for moving data out of the subject, but more for debugging. With our current approach of packing everything into the subject, when running a command like openssl x509 -in <cert> -text -noout I have to mentally map fields in the subject to Teleport metadata like OU=groups.

Ideally we'd see something like x-teleport-groups: admin, dev instead. It appears that's possible with extensions.

I'd suggest we keep the metadata in the subject for now, but instead use name extensions instead of common fields. The same command you have outlined works with subject as well, no problem

fspmarshall · 2020-03-06T01:29:53Z

lib/kube/proxy/forwarder.go

-		for _, group := range ctx.kubeGroups {
-			req.Header.Add("Impersonate-Group", group)
-			f.Debugf("Impersonate Group: %v", group)
+		headers.Add("Impersonate-User", impersonateUser)


It appears that impersonateUser may be the empty string ("") if len(ctx.kubeUsers) != 1. Is that intended behavior?

@fspmarshall thanks for outstanding detailed review. I think this is undefined behavior, however I restricted our approach to rejecting the empty impersonating header and in case of multiple choice we will refuse to make a selection, asking user to select a user, see my changes above.

klizhentas · 2020-03-06T21:32:33Z

retest this please

klizhentas · 2020-03-06T22:29:48Z

retest this please

This commit fixes #3369, refs #3374 It adds support for kuberenetes_users section in roles, allowing Teleport proxy to impersonate user identities. It also extends variable interpolation syntax by adding suffix and prefix to variables and function `email.local`: Example: ```yaml kind: role version: v3 metadata: name: admin spec: allow: # extract email local part from the email claim logins: ['{{email.local(external.email)}}'] # impersonate a kubernetes user with IAM prefix kubernetes_users: ['IAM#{{external.email}}'] # the deny section uses the identical format as the 'allow' section. # the deny rules always override allow rules. deny: {} ``` Some notes on email.local behavior: * This is the only function supported in the template variables for now * In case if the email.local will encounter invalid email address, it will interpolate to empty value, will be removed from resulting output. Changes in impersonation behavior: * By default, if no kubernetes_users is set, which is a majority of cases, user will impersonate themselves, which is the backwards-compatible behavior. * As long as at least one `kubernetes_users` is set, the forwarder will start limiting the list of users allowed by the client to impersonate. * If the users' role set does not include actual user name, it will be rejected, otherwise there will be no way to exclude the user from the list). * If the `kuberentes_users` role set includes only one user (quite frequently that's the real intent), teleport will default to it, otherwise it will refuse to select. This will enable the use case when `kubernetes_users` has just one field to link the user identity with the IAM role, for example `IAM#{{external.email}}` * Previous versions of the forwarding proxy were denying all external impersonation headers, this commit allows 'Impesrsonate-User' and 'Impersonate-Group' header values that are allowed by role set. * Previous versions of the forwarding proxy ignored 'Deny' section of the roles when applied to impersonation, this commit fixes that - roles with deny kubernetes_users and kubernetes_groups section will not allow impersonation of those users and groups.

klizhentas · 2020-03-07T00:35:03Z

retest this please

This commit fixes #3369, refs #3374 It adds support for kuberenetes_users section in roles, allowing Teleport proxy to impersonate user identities. It also extends variable interpolation syntax by adding suffix and prefix to variables and function `email.local`: Example: ```yaml kind: role version: v3 metadata: name: admin spec: allow: # extract email local part from the email claim logins: ['{{email.local(external.email)}}'] # impersonate a kubernetes user with IAM prefix kubernetes_users: ['IAM#{{external.email}}'] # the deny section uses the identical format as the 'allow' section. # the deny rules always override allow rules. deny: {} ``` Some notes on email.local behavior: * This is the only function supported in the template variables for now * In case if the email.local will encounter invalid email address, it will interpolate to empty value, will be removed from resulting output. Changes in impersonation behavior: * By default, if no kubernetes_users is set, which is a majority of cases, user will impersonate themselves, which is the backwards-compatible behavior. * As long as at least one `kubernetes_users` is set, the forwarder will start limiting the list of users allowed by the client to impersonate. * If the users' role set does not include actual user name, it will be rejected, otherwise there will be no way to exclude the user from the list). * If the `kuberentes_users` role set includes only one user (quite frequently that's the real intent), teleport will default to it, otherwise it will refuse to select. This will enable the use case when `kubernetes_users` has just one field to link the user identity with the IAM role, for example `IAM#{{external.email}}` * Previous versions of the forwarding proxy were denying all external impersonation headers, this commit allows 'Impesrsonate-User' and 'Impersonate-Group' header values that are allowed by role set. * Previous versions of the forwarding proxy ignored 'Deny' section of the roles when applied to impersonation, this commit fixes that - roles with deny kubernetes_users and kubernetes_groups section will not allow impersonation of those users and groups.

klizhentas requested review from benarent, fspmarshall and russjones March 4, 2020 17:24

klizhentas force-pushed the sasha/ku branch from 9b85151 to 4ae31f7 Compare March 4, 2020 17:30

fspmarshall reviewed Mar 4, 2020

View reviewed changes

benarent linked an issue Mar 4, 2020 that may be closed by this pull request

Teleport to access IBM Kubernetes Service using JWT auth scheme #3289

Closed

fspmarshall reviewed Mar 4, 2020

View reviewed changes

russjones reviewed Mar 5, 2020

View reviewed changes

klizhentas force-pushed the sasha/ku branch from 4ae31f7 to 6c8fd57 Compare March 5, 2020 20:16

klizhentas requested review from russjones and fspmarshall March 5, 2020 20:16

fspmarshall reviewed Mar 6, 2020

View reviewed changes

klizhentas force-pushed the sasha/ku branch from 6c8fd57 to 9cf596d Compare March 6, 2020 16:24

fspmarshall approved these changes Mar 6, 2020

View reviewed changes

russjones approved these changes Mar 6, 2020

View reviewed changes

klizhentas force-pushed the sasha/ku branch from 9cf596d to 6416a57 Compare March 6, 2020 22:50

klizhentas merged commit 31fd5b1 into branch/4.2 Mar 7, 2020

awly mentioned this pull request May 18, 2020

kubectl config Error from server (BadRequest) on leaf K8s #3716

Closed

klizhentas deleted the sasha/ku branch March 15, 2021 16:50

strideynet mentioned this pull request Nov 15, 2022

Clarify behaviour of kubernetes_users role field in docs #18495

Closed

Adds support for kube_users, extend interpolation #3404

Adds support for kube_users, extend interpolation #3404

Conversation

klizhentas commented Mar 4, 2020 • edited Loading

benarent commented Mar 4, 2020

klizhentas commented Mar 4, 2020

klizhentas commented Mar 4, 2020

Choose a reason for hiding this comment

klizhentas Mar 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fspmarshall Mar 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klizhentas commented Mar 4, 2020

klizhentas commented Mar 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klizhentas commented Mar 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klizhentas Mar 6, 2020 • edited Loading

Choose a reason for hiding this comment

klizhentas commented Mar 6, 2020

klizhentas commented Mar 6, 2020

klizhentas commented Mar 7, 2020

klizhentas commented Mar 4, 2020 •

edited

Loading

klizhentas Mar 4, 2020 •

edited

Loading

fspmarshall Mar 5, 2020 •

edited

Loading

klizhentas Mar 6, 2020 •

edited

Loading