Adding support for server to authenticate agent by dberkov · Pull Request #51 · kubernetes-sigs/apiserver-network-proxy

dberkov · 2020-01-22T06:49:04Z

The PR allows to proxy-server authenticate proxy-agent.

Agent:

Agent sends in GRPC metadata token associated to the pod by kubernetes system.

Server

Server as part of the connection opening step, reads the token sent by agent, invokes kubernetes.TokenReviews API, checks the token is valid and belongs to agent's pod by validating namespace + service account of the token's owner .

General

All examples/kubernetes/* templates and READ.me procedure has been updated for supporting fully working e2e test of this feature in kubernetes.

dberkov · 2020-01-22T07:05:39Z

/test pull-apiserver-network-proxy-test

dberkov · 2020-01-22T07:06:58Z

/assign @caesarxuchao @Jefftree

cmd/agent/main.go

cmd/client/main.go

cmd/proxy/main.go

pkg/agent/agentclient/stream.go

cmd/agent/main.go

caesarxuchao · 2020-01-22T20:38:55Z

cmd/agent/main.go

 		return fmt.Errorf("proxy server port %d must be greater than 0", o.proxyServerPort)
 	}
+	if o.saToken != "" {
+		if _, err := os.Stat(o.saToken); os.IsNotExist(err) {


We shouldn't ignore other types of error.

We do exactly same validation for all other files (agentCert, agentKey, etc..). I would prefer to keep it as is and we can refactor entire project's validation methods in separate PR

cmd/proxy/main.go

examples/kubernetes/README.md

caesarxuchao · 2020-01-22T21:06:05Z

pkg/agent/agentserver/server.go

+	}
+
+	if !r.Status.Authenticated {
+		return fmt.Errorf("lookup failed: service account jwt not valid")


Is this a jwt?

Yep sa are jwt

proto/agent/agent.proto

pkg/util/auth.go

proto/agent/agent.pb.go

caesarxuchao · 2020-01-22T21:35:31Z

examples/kubernetes/README.md

 CLUSTER_KEY=/etc/srv/kubernetes/pki/apiserver.key
 ```

+# Register SERVER_TOKEN in [static-token-file](https://kubernetes.io/docs/reference/access-authn-authz/authentication/#static-token-file)


Is "static-token" the standard way to authenticate a process running in the master node?

I think we can run the proxy server as a static pod, and then use a service account to authenticate it.

Yes, this pattern used across all other static pods. Ex: https://github.com/kubernetes/kubernetes/blob/c14106ad1234742da80eb8f12ddcbf19dba61284/cluster/gce/gci/configure-helper.sh#L613-L615

cmd/agent/main.go

cmd/proxy/main.go

examples/kubernetes/konnectivity-agent.yaml

pkg/agent/agentclient/client.go

pkg/util/auth.go

caesarxuchao

A few more nits.

@Jefftree @dberkov have you manually tested it in GCE/GKE?

@dberkov I understand that it's difficult to write a complete test because we don't have the test framework that runs a k8s cluster. Can you add an integration test to tests/ to verify that the proxy-server denies the Connect request if the agent doesn't send a bearer token at all? That doesn't require a k8s cluster running.

cmd/agent/main.go

cmd/proxy/main.go

pkg/agent/agentserver/server.go

Jefftree · 2020-01-24T02:01:19Z

@dberkov @caesarxuchao: Since network proxy promises MTLS, can we enforce that either a cert or token must be sent by the proxy agent? If no token is sent, the konnectivity-server will reject the request and unregister the connection, but the konnectivity-agent pod will be in a infinite crash loop state, and keep trying to connect to the server with no token info supplied.

examples/kubernetes/konnectivity-agent.yaml

pkg/agent/agentserver/server.go

proto/header/header.go

pkg/agent/agentserver/server.go

dberkov · 2020-01-24T22:56:42Z

A few more nits.

@Jefftree @dberkov have you manually tested it in GCE/GKE?

@dberkov I understand that it's difficult to write a complete test because we don't have the test framework that runs a k8s cluster. Can you add an integration test to tests/ to verify that the proxy-server denies the Connect request if the agent doesn't send a bearer token at all? That doesn't require a k8s cluster running.

I have created pkg/agent/agentserver/server_test.go with full test coverage on new behavior on server side

Jefftree

Thanks for adding the tests! I've verified this on GKE and seems to be working fine per the instructions. One minor nit, but otherwise lgtm from me.

Will defer to Chao and Walter for approval.

/assign @cheftako

Makefile

mocks/agent_mock.go

dberkov · 2020-01-27T17:13:13Z

/test pull-apiserver-network-proxy-test

dberkov · 2020-01-27T17:14:39Z

/test pull-apiserver-network-proxy-test

dberkov · 2020-01-27T17:18:50Z

/test pull-apiserver-network-proxy-test

cheftako · 2020-01-27T18:02:54Z

cmd/agent/main.go

+	}
+
+	if o.agentCert == "" && o.agentKey == "" && o.serviceAccountTokenPath == "" {
+		return fmt.Errorf("agent must enable certificate based or token based authentication")


I believe this should be enforced from the server side and not the client side. There are legitimate non production use cases for turning off client authentication.

In addition if we allow this through I think we don't need the mock agent as the real agent can do what is necessary.

We may want to add an enum setting to the proxy/main.go at this point for requiredAgentAuth? That way we can detect if the agent is meeting the minimum authentication requirements.

I brought this up because an incorrectly configured konnectivity-agent would send a large number of denied requests to the konnectivity-server. (agent retries + multiple threads hitting LB for regional clusters + CrashLoop retries).

Since there are use cases for turning off client authentication, removing this check is fine, but we should still think of ways to limit the outgoing requests of an incorrectly configured agent.

I removed this validation

cheftako · 2020-01-27T18:28:40Z

/assign @mikedanese
I would like to make sure Mike takes a look and approves prior to merge.

k8s-ci-robot · 2020-01-27T18:28:42Z

@cheftako: GitHub didn't allow me to assign the following users: mikedanese.

Note that only kubernetes-sigs members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

Details

In response to this:

/assign @mikedanese
I would like to make sure Mike takes a look and approves prior to merge.

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.

caesarxuchao

The gomock is cool. I wish I knew it earlier.

Makefile

caesarxuchao · 2020-02-03T23:35:57Z

cmd/proxy/main.go

+		}
+		if o.authenticationAudience == "" {
+			return fmt.Errorf("authenticationAudience cannot be empty when agent authentication is enabled")
+		}


Also check if o.kubeconfigPath==nil?

It cannot be nil, since we have newProxyRunOptions().

examples/kubernetes/README.md

caesarxuchao · 2020-02-03T23:58:13Z

examples/kubernetes/konnectivity-agent.yaml

+      sources:
+      - serviceAccountToken:
+          path: konnectivity-agent-token
+          audience: system:konnectivity-server


I guess this "audience" value gets encoded into the token?

KubernetesClient.AuthenticationV1().TokenReviews() gets the audience is the parameters, so k8s API validates that token is issued with this audience

But konnectivity-server calls TokenReviews, and this is the yaml for the konnectivity-agent.

My guess is the token data mounted by the agent will contain the "audience", and apiserver will be able to extract the "audience" out from the token.

pkg/agent/agentclient/stream.go

pkg/agent/agentserver/server_test.go

mocks/agent_mock.go

caesarxuchao · 2020-02-04T18:43:06Z

/lgtm

cheftako · 2020-02-05T01:59:01Z

cmd/proxy/main.go

 	serverCount uint
+	// Agent pod's namespace for token-based agent authentication
+	agentNamespace string
+	// Agent pod's service account for token-based agent authentication


Will we ever want different service accounts for different agents? Eg. agent service account per failure domain?

cheftako · 2020-02-05T02:02:38Z

cmd/proxy/main.go

+	// all 4 parametes must be empty or must have value (except kubeconfigPath that might be empty)
+	if o.agentNamespace != "" || o.agentServiceAccount != "" || o.authenticationAudience != "" || o.kubeconfigPath != "" {
+		if o.agentNamespace == "" {
+			return fmt.Errorf("agentNamespace cannot be empty when agent authentication is enabled")


For future we should consider accumulating these errors. Sort of annoying to be given and error I need a service account on run1 and then be given an error I need an audience on run2.

examples/kubernetes/README.md

cheftako

Please fix the GKE reference.

cheftako

Looks good

cheftako · 2020-02-06T01:19:26Z

/lgtm

cheftako · 2020-02-06T01:27:11Z

/approve

k8s-ci-robot · 2020-02-06T01:27:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cheftako, dberkov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [cheftako]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…odules/github.com/stretchr/testify-1.8.0 Build(deps): bump github.com/stretchr/testify from 1.7.5 to 1.8.0

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 22, 2020

k8s-ci-robot requested review from Sh4d1 and caesarxuchao January 22, 2020 06:49

dberkov force-pushed the token branch 3 times, most recently from 26cf0c1 to 5762da5 Compare January 22, 2020 07:05

k8s-ci-robot assigned caesarxuchao and Jefftree Jan 22, 2020

Sh4d1 suggested changes Jan 22, 2020

View reviewed changes

caesarxuchao reviewed Jan 22, 2020

View reviewed changes

caesarxuchao reviewed Jan 23, 2020

View reviewed changes

cmd/agent/main.go Outdated Show resolved Hide resolved

cmd/proxy/main.go Outdated Show resolved Hide resolved

cmd/proxy/main.go Outdated Show resolved Hide resolved

pkg/agent/agentserver/server.go Outdated Show resolved Hide resolved