Pass user authentication to Tiller #1932

tamalsaha · 2017-02-09T16:32:42Z

First step for #1918

k8s-ci-robot · 2017-02-09T16:32:44Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

Once you've signed, please reply here (e.g. "I signed it!") and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

tamalsaha · 2017-02-09T16:33:55Z

I signed it!

tamalsaha · 2017-02-09T16:52:01Z

@technosophos , here are few issues:

I don't see how to pass the updated context for StreamInterceptor.
KubeConfig has some extra options, like impersonate, insecureSkipVerify. https://github.com/kubernetes/client-go/blob/master/tools/clientcmd/api/v1/types.go#L84 Does those need to be supported?

technosophos · 2017-02-09T22:53:08Z

I think we should probably disallow both impersonate and insecureSkipVerify.

technosophos · 2017-02-09T22:53:52Z

I put this onto 2.3.0 and marked it WIP so it doesn't actually get merged into 2.2.0

saumanbiswas · 2017-02-10T05:09:41Z

I signed it!

tamalsaha · 2017-03-02T16:32:29Z

@technosophos do you mind taking a first pass at this pr. If the general structure looks ok, we can write unit tests.

We have a follow up pr to add authZ check. appscode#10

technosophos · 2017-03-07T19:20:58Z

This will definitely need some documentation, but things are looking good. I am going to do some manual testing today.

michelleN · 2017-03-09T16:28:36Z

Just curious. How come the "Released By" field doesn't get populated by default with "admin" on a cluster with no users/roles (RBAC not set up)?

tamalsaha · 2017-03-09T16:41:46Z

@michelleN , what are you seeing in such a cluster? What type of of auth are you using (basic/bearer/client cert)?

michelleN · 2017-03-09T17:28:35Z

@tamalsaha I'm using client cert

tamalsaha · 2017-03-14T17:53:19Z

Hi, any comments on the code?

adamreese · 2017-03-14T22:15:36Z

pkg/tiller/server.go

+	caCert, _ := getCertificateAuthority(md)
+
+	// ref: k8s.io/helm/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/util#NewFactory()
+	flags := pflag.NewFlagSet("", pflag.ContinueOnError)


can you remove all calls to pflags

adamreese · 2017-03-14T22:46:34Z

pkg/tiller/server.go

+	md, _ := metadata.FromContext(ctx)
+	token := md[string(helm.Authorization)][0][len("Bearer "):]
+
+	apiServer, err := getServerURL(md)


why would there be a different server url for the user?

adamreese · 2017-03-14T22:48:59Z

pkg/tiller/server.go

+	flagNames.ClusterOverrideFlags.APIServer.ShortName = "s"
+
+	clientcmd.BindOverrideFlags(overrides, flags, flagNames)
+	tokenConfig, err := clientcmd.NewInteractiveDeferredLoadingClientConfig(loadingRules, overrides, os.Stdin).ClientConfig()


you don't want to use an interactive loader in the server

adamreese · 2017-03-14T23:31:45Z

pkg/helm/option.go

+	)
+}
+
+func extractKubeConfig() map[string]string {


pkg/helm should not parse kubeconfig, it should be passed into the context from cmd/helm

there is a function that reads the config at https://github.com/kubernetes/helm/blob/master/cmd/helm/helm.go#L238-L250

adamreese · 2017-03-14T23:36:03Z

pkg/helm/option.go

+	}
+
+	// Kube APIServer URL
+	if len(c.Host) != 0 {


I don't think host is required, tiller already has it

adamreese · 2017-03-14T23:37:15Z

pkg/helm/option.go

+		configData[string(K8sClientCertificate)] = base64.StdEncoding.EncodeToString(c.TLSClientConfig.CertData)
+	}
+
+	if len(c.TLSClientConfig.CAFile) != 0 {


tiller already has the CA file

adamreese · 2017-03-14T23:44:21Z

pkg/tiller/server.go

+	ctx = context.WithValue(ctx, helm.K8sUser, user)
+	ctx = context.WithValue(ctx, helm.K8sConfig, kubeConfig)
+
+	// TODO: Remove


i think this needs to be removed

adamreese · 2017-03-14T23:47:16Z

pkg/tiller/server.go

+
+func checkBearerAuth(ctx context.Context) (*authenticationapi.UserInfo, *rest.Config, error) {
+	md, _ := metadata.FromContext(ctx)
+	token := md[string(helm.Authorization)][0][len("Bearer "):]


this needs to be checked before accessing to avoid panics

technosophos

This PR seems to reproduce lots of stuff that's already in either Kubernetes or in Helm. Some of it appears to be copied verbatim from Kubernetes. It looks to me like it could be significantly simplified.

technosophos · 2017-03-14T23:40:56Z

pkg/helm/option.go

+func extractKubeConfig() map[string]string {
+	configData := make(map[string]string)
+	clientConfig := cmdutil.DefaultClientConfig(pflag.NewFlagSet("", pflag.ContinueOnError))
+	c, err := clientConfig.ClientConfig()


Why not just clean c of fields we don't want, serialize it to JSON, and then send that instead of creating a whole bunch of different headers? That would simplify both the marshal and unmarshal operations for this data.

gRPC will even base64 encode/decode this for you if you suffix the header -bin. https://godoc.org/google.golang.org/grpc/metadata#New

technosophos · 2017-03-14T23:41:56Z

pkg/helm/types.go

@@ -0,0 +1,32 @@
+/*


This file should be named auth-header.go, not types.go.

auth_header.go

technosophos · 2017-03-14T23:47:22Z

pkg/helm/types.go

+//AuthHeader is key type for context
+type AuthHeader string
+
+const (


If these are used by both the client and the server, they probably shouldn't be in the client package. That requires importing the client package for the server.

technosophos · 2017-03-14T23:57:55Z

pkg/tiller/server.go

+	md, ok := metadata.FromContext(ctx)
+	if !ok {
+		return nil, errors.New("Missing metadata in context.")
+	}


This seems really complicated for what you are trying to do. Why not construct a single client config object and just run a client.DiscoveryClient.ServerVersion()? Wouldn't that just let the client library do most of what you do in the multiple functions below? This would be even easier if you sent a sanitized client config instead of storing various bits of the client config in different gRPC headers.

tamalsaha · 2017-03-16T19:48:37Z

@technosophos @adamreese PTAL. All comments are addressed.

bacongobbler · 2017-03-16T19:59:33Z

cmd/helm/installer/uninstall.go

 	"k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset"
 	"k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/typed/core/internalversion"
-
-	"k8s.io/helm/pkg/kube"


this should be moved back. It's a local package.

Typical best practices for go imports are

stdlib

dependencies

local packages

go fmt would agree.

@bacongobbler , gofmt is ok with things as is.

@tamalsaha run goimports to group them correctly

@adamreese I just ran:

$ goimports -w cmd $ goimports -w ./cmd/.. $ goimports -w ./pkg/.. $ goimports -w pkg

Still no change. In fact that suggests changes some other unrelated files:

modified: pkg/chartutil/load.go modified: pkg/releaseutil/filter_test.go modified: pkg/releaseutil/sorter_test.go

So, I skipped those.

Indeed, it seems like they don't really care about import ordering. Still, the precedence for packages being sorted in this order has always been implied. See cmd/helm/init.go (and anywhere else in the source code) for an example. It's just best practices we've been following.

Never heard of this best practice. But if project follows unwritten best practices regarding style, I'm happy to oblige. Just thought I was done with these, when I switched to writing GO. :)

bacongobbler · 2017-03-16T20:02:28Z

pkg/tiller/server.go

+	"google.golang.org/grpc/peer"
+	"k8s.io/helm/pkg/kube"
 	"k8s.io/helm/pkg/version"
+	authenticationapi "k8s.io/kubernetes/pkg/apis/authentication"


same here RE: package imports

bacongobbler · 2017-03-16T22:48:50Z

cmd/helm/installer/uninstall.go

 	"k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/typed/core/internalversion"

 	"k8s.io/helm/pkg/kube"
+	"k8s.io/kubernetes/pkg/api"


this got accidentally shifted over (third-party package from kubernetes)

tamalsaha · 2017-04-10T16:33:24Z

@michelleN , it seems that GKE does not support TokenReview api even with alpha features enabled in 1.5.6 cluster.

tamalsaha · 2017-04-10T16:35:47Z

@adamreese / @technosophos , to test this manually, deploy this tiller. Then install a new chart using helm build from this pr. Then list releases. You should see the deployer name.

$ helm install stable/redis
$ helm ls

# ./helm install stable/redis
NAME:   factual-lionfish
LAST DEPLOYED: Mon Apr 10 16:31:03 2017
NAMESPACE: default
STATUS: DEPLOYED
RELEASED BY: qacode.system-admin

RESOURCES:
==> v1/Secret
NAME                    TYPE    DATA  AGE
factual-lionfish-redis  Opaque  1     1s

==> v1/PersistentVolumeClaim
NAME                    STATUS   VOLUME  CAPACITY  ACCESSMODES  AGE
factual-lionfish-redis  Pending  1s

==> v1/Service
NAME                    CLUSTER-IP   EXTERNAL-IP  PORT(S)   AGE
factual-lionfish-redis  10.0.194.85  <none>       6379/TCP  1s

==> extensions/v1beta1/Deployment
NAME                    DESIRED  CURRENT  UP-TO-DATE  AVAILABLE  AGE
factual-lionfish-redis  1        1        1           0          1s


NOTES:
Redis can be accessed via port 3306 on the following DNS name from within your cluster:
factual-lionfish-redis.default.svc.cluster.local

To get your password run:

    REDIS_PASSWORD=$(kubectl get secret --namespace default factual-lionfish-redis -o jsonpath="{.data.redis-password}" | base64 --decode)

To connect to your Redis server:

1. Run a Redis pod that you can use as a client:

   kubectl run factual-lionfish-redis-client --rm --tty -i --env REDIS_PASSWORD=$REDIS_PASSWORD --image bitnami/redis:3.2.8-r2 -- bash

2. Connect using the Redis CLI:

  redis-cli -h factual-lionfish-redis -a $REDIS_PASSWORD

root@qa-buildbox:~# ./helm ls
NAME               	REVISION	UPDATED                 	STATUS  	CHART      	NAMESPACE	RELEASED BY        
factual-lionfish   	1       	Mon Apr 10 16:31:03 2017	DEPLOYED	redis-0.5.1	default  	qacode.system-admin
fancy-chinchilla   	1       	Mon Apr 10 08:02:27 2017	DEPLOYED	redis-0.5.1	default  	                   
kissable-woodpecker	1       	Mon Apr 10 07:59:31 2017	DEPLOYED	mysql-0.2.5	default

tamalsaha · 2017-04-10T22:16:52Z

I am told that GKE does not enable TokenReview in sig-apps. So, once auth is added Tiller will not work in GKE?

tamalsaha · 2017-04-13T14:04:09Z

I have added checks to dynamically check if TokenReview api is not enabled. I don't get the error in GKE any more. Obviously, you will not see the username in GKE.

technosophos · 2017-04-28T23:34:13Z

This should be unblocked again at this point. We've merged in gRPC 1.2.

technosophos · 2017-05-01T17:20:35Z

Bumping to 2.5.0, since it still needs rebasing.

technosophos · 2017-05-25T16:48:47Z

Refactor is done. This issue is unblocked.

mattfarina · 2017-06-06T16:54:17Z

Any movement on this happening?

k8s-reviewable · 2017-06-09T22:52:49Z

This change is

tamalsaha · 2017-06-09T22:56:14Z

Rebased.

seh · 2017-06-20T12:34:23Z

Do you intend to take group membership into account as well? I see treatment of "user names" in the proposed changes here in file pkg/tiller/server.go we're omitting the groups that you can extract from the "Organization" AVAs in the certificate's subject.

seh · 2017-06-20T12:36:14Z

pkg/tiller/server.go

+	crt := tlsInfo.State.VerifiedChains[0][0]
+	user := authenticationapi.UserInfo{
+		Username: crt.Subject.CommonName,
+	}


What about using crt.Subject.Organization here to populate the UserInfo "Groups" field?

Group name needs to be preserved for TokenReview calls. But I believe this was not next working at Kube 1.5, when I originally wrote this pr.

@seh, do you mind pointing how Kubectl is handling Group names from crt these days? I thin we need to parse the same way.

The authentication documentation on X.509 client certificates is here.

As of Kubernetes 1.4, client certificates can also indicate a user’s group memberships using the certificate’s organization fields. To include multiple group memberships for a user, include multiple organization fields in the certificate.

In my clusters, we write most of our RBAC bindings against groups, not users, so honoring the group membership expressed in the certificates' "O" AVAs is important for us.

tamalsaha · 2017-06-25T12:26:47Z

@technosophos, it seems that I won't be able to find time to work through the review process. I am sad that it turned out this way. I don't want to hold off this feature from Helm users. So, I am closing this pr. Core team / interested parties are welcome to work through the review process based on this pr.

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Feb 9, 2017

technosophos added this to the 2.3.0 milestone Feb 9, 2017

technosophos added the wip label Feb 9, 2017

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Feb 10, 2017

saumanbiswas force-pushed the tiller-auth branch from f8b8cee to d1d3bc0 Compare February 15, 2017 10:09

technosophos removed the wip label Mar 2, 2017

technosophos requested review from technosophos and adamreese March 6, 2017 21:58

technosophos requested review from michelleN and removed request for technosophos March 8, 2017 18:12

adamreese requested changes Mar 14, 2017

View reviewed changes

technosophos requested changes Mar 15, 2017

View reviewed changes

saumanbiswas force-pushed the tiller-auth branch from bca5a06 to f2a19a8 Compare March 16, 2017 08:23

bacongobbler reviewed Mar 16, 2017

View reviewed changes

bacongobbler requested changes Mar 16, 2017

View reviewed changes

thomastaylor312 added the feature label Mar 17, 2017

tamalsaha force-pushed the tiller-auth branch 2 times, most recently from 010e821 to 8035389 Compare April 10, 2017 16:32

tamalsaha force-pushed the tiller-auth branch 5 times, most recently from 22b62ec to 3b43607 Compare April 13, 2017 13:57

tamalsaha force-pushed the tiller-auth branch from 3b43607 to 64c4059 Compare April 13, 2017 22:55

technosophos modified the milestones: 2.5.0 - Features, 2.4.0 - Features May 1, 2017

technosophos modified the milestones: 2.5.0 - Features, 2.6.0 - Features Jun 7, 2017

Pass user authentication to Tiller

586a496

tamalsaha force-pushed the tiller-auth branch from 64c4059 to 586a496 Compare June 9, 2017 22:52

seh reviewed Jun 20, 2017

View reviewed changes

tamalsaha closed this Jun 25, 2017

Pass user authentication to Tiller #1932

Pass user authentication to Tiller #1932

Uh oh!

Conversation

tamalsaha commented Feb 9, 2017

Uh oh!

k8s-ci-robot commented Feb 9, 2017

Uh oh!

tamalsaha commented Feb 9, 2017

Uh oh!

tamalsaha commented Feb 9, 2017

Uh oh!

technosophos commented Feb 9, 2017

Uh oh!

technosophos commented Feb 9, 2017

Uh oh!

saumanbiswas commented Feb 10, 2017

Uh oh!

tamalsaha commented Mar 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

technosophos commented Mar 7, 2017

Uh oh!

michelleN commented Mar 9, 2017

Uh oh!

tamalsaha commented Mar 9, 2017

Uh oh!

michelleN commented Mar 9, 2017

Uh oh!

tamalsaha commented Mar 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

technosophos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tamalsaha commented Mar 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tamalsaha Mar 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bacongobbler Mar 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

tamalsaha commented Mar 2, 2017 •

edited

Loading

tamalsaha commented Mar 16, 2017 •

edited

Loading

tamalsaha Mar 16, 2017 •

edited

Loading

bacongobbler Mar 16, 2017 •

edited

Loading

bacongobbler Mar 16, 2017 •

edited

Loading

tamalsaha commented Apr 10, 2017 •

edited

Loading

tamalsaha commented Apr 10, 2017 •

edited

Loading

tamalsaha commented Apr 13, 2017 •

edited

Loading

tamalsaha Jun 20, 2017 •

edited

Loading