kube-apiserver: healthcheck via sidecar container #9069

justinsb · 2020-05-05T17:14:20Z

kube-apiserver doesn't expose the healthcheck via a dedicated
endpoint, instead relying on anonyomous-access being enabled. That
has previously forced us to enable the unauthenticated endpoint on
127.0.0.1:8080.

Instead we now run a small sidecar container, which
proxies /healthz and /readyz requests (only) adding appropriate
authentication using a client certificate.

This will also enable better load balancer checks in future, as these
have previously been hampered by the custom CA certificate.

k8s-ci-robot · 2020-05-05T17:14:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [justinsb]

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

johngmyers · 2020-05-05T18:22:03Z

Makefile

+	mkdir -p ${BAZELIMAGES}
+	DOCKER_REGISTRY="" DOCKER_IMAGE_PREFIX="kope/" KUBE_APISERVER_HEALTHCHECK_TAG=${KUBE_APISERVER_HEALTHCHECK_TAG} bazel build ${BAZEL_CONFIG} --platforms=@io_bazel_rules_go//go/toolchain:linux_amd64 //cmd/kube-apiserver-healthcheck:image-bundle.tar.gz //cmd/kube-apiserver-healthcheck:image-bundle.tar.gz.sha1 //cmd/kube-apiserver-healthcheck:image-bundle.tar.gz.sha256
+	cp -fp bazel-bin/cmd/kube-apiserver-healthcheck/image-bundle.tar.gz ${BAZELIMAGES}/kube-apiserver-healthcheck.tar.gz
+	cp -fp bazel-bin/cmd/kube-apiserver-healthcheck/image-bundle.tar.gz.sha1 ${BAZELIMAGES}/kube-apiserver-healthcheck.tar.gz.sha1


Why do we need to create and ship SHA-1 hashes?

I believe it's historical and we began migrating to sha256 about a year ago. I'm not sure on the specifics, we can probably talk about a deprecation at some point, however I don't think this PR is the right time to do it.

Good call - it's a new file, let's try removing the sha1, then we can find out if anyone is still using it (they shouldn't be!)

Oops - our responses crossed. The sha256 support (eca2ac6) should be in kops 1.15, so I am going ahead and removing the sha1. But if it gives us trouble, let's add it back ...

johngmyers · 2020-05-05T18:26:07Z

cmd/kube-apiserver-healthcheck/main.go

+	resp, err := httpClient.Do(req)
+	if err != nil {
+		klog.Infof("error from %s: %v", target, err)
+		http.Error(w, "internal error", http.StatusInternalServerError)


http.StatusBadGateway would be more appropriate.

johngmyers · 2020-05-05T18:33:00Z

cmd/kube-apiserver-healthcheck/main.go

+func (s *healthCheckServer) handler(w http.ResponseWriter, r *http.Request) {
+	path := r.URL.Path
+	path = strings.TrimPrefix(path, "/")
+	tokens := strings.Split(path, "/")


My druthers would be to just select(r.URL.Path) and leave out any parsing of the path. There's a risk of a discrepancy between the parsing here and the parsing in proxyRequest, leading to request injection vulnerabilities.

Good idea - generally reworked to avoid all parsing!

johngmyers · 2020-05-05T18:44:26Z

cmd/kube-apiserver-healthcheck/main.go

+func (s *healthCheckServer) httpClient() *http.Client {
+	transport := &http.Transport{
+		TLSClientConfig: s.tlsConfig,
+	}


It seems expensive to create a new Transport for each incoming query. Shouldn't this be one per healthCheckServer?

The fear is that they contaminate each other (e.g. cookies), but given our use case, you're right - simplifying.

johngmyers · 2020-05-05T18:56:23Z

nodeup/pkg/model/kube_apiserver_healthcheck.go

+		signerCertificate = cert
+	}
+
+	privateKey, err := NewPrivateKey(2048)


Please use pki.GeneratePrivateKey() and don't hardcode the key size.

I was thinking about creating simpler methods, but looking at pki, that's exactly what you've already done!

johngmyers · 2020-05-05T19:00:47Z

nodeup/pkg/model/kube_apiserver_healthcheck.go

+	}
+
+	klog.Infof("signing certificate for %q", id)
+	cert, err := NewSignedClientCert(id, privateKey, signerCertificate.Certificate, signer)


Use pki.SignNewCertificate()

johngmyers · 2020-05-05T20:49:22Z

pkg/assets/builder.go

@@ -164,6 +178,13 @@ func (a *AssetBuilder) RemapImage(image string) (string, error) {
 		}
 	}

+	if strings.HasPrefix(image, "kope/kube-apiserver-healthcheck:") {
+		override := os.Getenv("KUBE_APISERVER_HEALTHCHECK_IMAGE")


Add export KUBE_APISERVER_HEALTHCHECK_IMAGE= to hack/update-expected.sh?

Good catch!

johngmyers · 2020-05-05T21:14:11Z

nodeup/pkg/model/kube_apiserver_healthcheck.go

+		})
+	}
+
+	clientKey, clientCert, err := b.buildClientKeypair(id)


Have buildClientKeypair() return a pki.Certificate and then use its AsBytes(). Use the pattern in KubeletBuilder

Thanks - done!

mikesplain

Most of this looks good to me. Holding on the lgtm since John had some comments, however I think some of those can be cleaned up over time as needed.

mikesplain · 2020-05-06T13:24:32Z

Makefile

+	mkdir -p ${BAZELIMAGES}
+	DOCKER_REGISTRY="" DOCKER_IMAGE_PREFIX="kope/" KUBE_APISERVER_HEALTHCHECK_TAG=${KUBE_APISERVER_HEALTHCHECK_TAG} bazel build ${BAZEL_CONFIG} --platforms=@io_bazel_rules_go//go/toolchain:linux_amd64 //cmd/kube-apiserver-healthcheck:image-bundle.tar.gz //cmd/kube-apiserver-healthcheck:image-bundle.tar.gz.sha1 //cmd/kube-apiserver-healthcheck:image-bundle.tar.gz.sha256
+	cp -fp bazel-bin/cmd/kube-apiserver-healthcheck/image-bundle.tar.gz ${BAZELIMAGES}/kube-apiserver-healthcheck.tar.gz
+	cp -fp bazel-bin/cmd/kube-apiserver-healthcheck/image-bundle.tar.gz.sha1 ${BAZELIMAGES}/kube-apiserver-healthcheck.tar.gz.sha1


I believe it's historical and we began migrating to sha256 about a year ago. I'm not sure on the specifics, we can probably talk about a deprecation at some point, however I don't think this PR is the right time to do it.

Makefile

johngmyers · 2020-05-06T15:51:30Z

cmd/kube-apiserver-healthcheck/main.go

+		Scheme:   "https",
+		Host:     "127.0.0.1",
+		Path:     r.URL.Path,
+		RawQuery: r.URL.RawQuery,


Do you really want to pass through an unvetted RawQuery? It seems risky to me.

So it's not trivial.. https://github.com/kubernetes/kubernetes/blob/08e1fd3bb947faf465e8a67d5c7106dbd10840c0/cluster/gce/manifests/kube-apiserver.manifest#L39

I don't know what validation we could / should do here. We could whitelist some queries I guess but I think that'll confuse people more. I imagine two things are coming:

exposing this for use as a load balancer health-check (i.e. publicly)

using the richer readyz / livez queries as in the upstream manifest

I think we need to parse the query and only pass through parameters we recognize as safe.

Looking at apiserver code, I see at least exclude and verbose, but that http.Request gets passed around all over the place.

So it's a risk assessment as to how likely it is that apiserver would have or later add an important authorization check on a parameter to those paths.

How about we parse and pass through exclude? I worry about information leakage from verbose.

Great idea - I implemented that. I also made it more testable and added a test!

johngmyers · 2020-05-06T15:56:18Z

nodeup/pkg/model/kube_apiserver_healthcheck.go

+	// KeyEncipherment allows the cert/key pair to be used to encrypt
+	// keys, including the symmetric keys negotiated during TLS setup
+	// and used for data transfer.
+	template.KeyUsage = template.KeyUsage | x509.KeyUsageKeyEncipherment


By the way, x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment is the default for pki.SignNewCertificate.

Ah - cool. I suggest leaving it for now? I think this function and the similar one to generate a kubeconfig end up being utils.

You can also see the beginnings of a thought I'm exploring here, which is putting less logic into nodeup. The only parameter here is really the path and the CN, we could imagine this just being in the NodeUpConfig (or similar), meaning we would have more insight into exactly what nodeup does (because it would do less)

Can leave for now. I'll look into extracting a function.

Thanks - I don't think it's urgent, I just figured that's one way we could go, so it might not matter too much if we only have two of these (kubeconfig & "raw")

justinsb · 2020-05-07T04:17:03Z

Cleaned up the request processing in the proxy & added some tests

johngmyers · 2020-05-07T04:27:53Z

cmd/kube-apiserver-healthcheck/proxy_test.go

+					t.Fatalf("unexpected method %q", out.Method)
+				}
+				actual = out.URL.String()
+			}


This test wouldn't catch a bug where mapToProxyRequest returns an Request containing a URL of the empty string when it should be returning nil.

cmd/kube-apiserver-healthcheck/proxy_test.go

kube-apiserver doesn't expose the healthcheck via a dedicated endpoint, instead relying on anonyomous-access being enabled. That has previously forced us to enable the unauthenticated endpoint on 127.0.0.1:8080. Instead we now run a small sidecar container, which proxies /healthz and /readyz requests (only) adding appropriate authentication using a client certificate. This will also enable better load balancer checks in future, as these have previously been hampered by the custom CA certificate. Co-authored-by: John Gardiner Myers <[email protected]>

rifelpet · 2020-05-07T13:20:49Z

/test pull-kops-e2e-kubernetes-aws

johngmyers · 2020-05-07T15:36:49Z

/lgtm

Cherry pick of #9069 onto release-1.17

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 5, 2020

k8s-ci-robot requested review from joshbranham and robinpercy May 5, 2020 17:14

k8s-ci-robot added area/api area/nodeup approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 5, 2020

justinsb force-pushed the healthcheck branch 2 times, most recently from 2ebe0c7 to e755771 Compare May 5, 2020 17:55

johngmyers reviewed May 5, 2020

View reviewed changes

mikesplain reviewed May 6, 2020

View reviewed changes

justinsb force-pushed the healthcheck branch 2 times, most recently from 97b3f82 to 8a99603 Compare May 6, 2020 14:32

k8s-ci-robot added area/documentation area/provider/aws Issues or PRs related to aws provider labels May 6, 2020

justinsb force-pushed the healthcheck branch from 8a99603 to ef04f3d Compare May 6, 2020 14:57

johngmyers requested changes May 6, 2020

View reviewed changes

justinsb force-pushed the healthcheck branch 2 times, most recently from 8a34253 to 6b11d8b Compare May 7, 2020 03:33

johngmyers requested changes May 7, 2020

View reviewed changes

johngmyers reviewed May 7, 2020

View reviewed changes

cmd/kube-apiserver-healthcheck/proxy_test.go Show resolved Hide resolved

justinsb force-pushed the healthcheck branch from 22f10de to 75fd939 Compare May 7, 2020 12:11

k8s-ci-robot assigned johngmyers May 7, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 7, 2020

k8s-ci-robot merged commit c736595 into kubernetes:master May 7, 2020

k8s-ci-robot added this to the v1.18 milestone May 7, 2020

justinsb mentioned this pull request May 8, 2020

Cherry pick of #9069 onto release-1.17 #9092

Merged

k8s-ci-robot added a commit that referenced this pull request May 8, 2020

Merge pull request #9092 from justinsb/cherrypick_9069_release-1.17

ccb700b

Cherry pick of #9069 onto release-1.17

johngmyers mentioned this pull request May 29, 2020

[CIS Security Benchmark] Unable to disable insecurePort by setting insecurePort:0 #9209

Closed

fred-vogt mentioned this pull request Jun 2, 2020

node-local-dns crash looping on masters '169.254.20.10:8080: bind: address already in use' #9245

Closed

mmerrill3 mentioned this pull request Sep 29, 2020

change api server livenessProbe toi livez from healthz #9977

Closed

kube-apiserver: healthcheck via sidecar container #9069

kube-apiserver: healthcheck via sidecar container #9069

Conversation

justinsb commented May 5, 2020

k8s-ci-robot commented May 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

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

mikesplain left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb commented May 7, 2020

Choose a reason for hiding this comment

rifelpet commented May 7, 2020

johngmyers commented May 7, 2020