🌱 adding TokenReview.auth.k8s.io/v1 webhook support #1440

christopherhein · 2021-03-19T15:40:54Z

This adds a new pkg/webhook/authentication/ package for setting up the ServeHTTP func for the webook server to be able to handle TokenReview.authentication.k8s.io/(v1,v1beta1) requests.

Closes #1436

Signed-off-by: Chris Hein me@chrishein.com

christopherhein · 2021-03-19T15:41:36Z

@rfranzke would you be interested in giving this a review and seeing if it aligns with what you implemented for Gardener?

christopherhein · 2021-03-19T15:43:56Z

/assign @alvaroaleman
/cc @coderanger

pkg/webhook/authentication/decoder.go

christopherhein · 2021-03-19T16:46:10Z

/test pull-controller-runtime-test-master

pkg/webhook/authentication/inject.go

pkg/webhook/alias.go

rfranzke · 2021-03-19T19:50:10Z

/assign

rfranzke

Thanks @christopherhein, I don't have too many comments because I'm not very familiar with the design decisions made for the admission webhooks, but overall I like it and it looks like it's going into the right direction! :)

Once this PR is merged, I will use it as reference to also contribute the authorization handling.

Generally, I see that many things are now "duplicated" and this might then happen again for the authz. Maybe we should try more to encapsulate shared functionality to make it reusable (not sure if this is always possible in all cases). Probably a maintainer can give better recommendations here.

examples/tokenreview/tokenreview.go

pkg/webhook/authentication/http.go

rfranzke · 2021-03-20T09:13:07Z

pkg/webhook/authentication/http.go

+	var body []byte
+	var err error
+	ctx := r.Context()
+	if wh.WithContextFunc != nil {
+		ctx = wh.WithContextFunc(ctx, r)
+	}
+
+	var reviewResponse Response
+	if r.Body != nil {
+		if body, err = ioutil.ReadAll(r.Body); err != nil {
+			wh.log.Error(err, "unable to read the body from the incoming request")
+			reviewResponse = Errored(err)
+			wh.writeResponse(w, reviewResponse)
+			return
+		}
+	} else {
+		err = errors.New("request body is empty")
+		reviewResponse = Errored(err)
+		wh.writeResponse(w, reviewResponse)
+		return
+	}
+
+	// verify the content type is accurate
+	contentType := r.Header.Get("Content-Type")
+	if contentType != "application/json" {
+		err = fmt.Errorf("contentType=%s, expected application/json", contentType)
+		wh.log.Error(err, "unable to process a request with an unknown content type", "content type", contentType)
+		reviewResponse = Errored(err)
+		wh.writeResponse(w, reviewResponse)
+		return
+	}


This is exactly the same like in

controller-runtime/pkg/webhook/admission/http.go

Lines 46 to 77 in b5065bd

var body []byte

var err error

ctx := r.Context()

if wh.WithContextFunc != nil {

ctx = wh.WithContextFunc(ctx, r)

}

var reviewResponse Response

if r.Body != nil {

if body, err = ioutil.ReadAll(r.Body); err != nil {

wh.log.Error(err, "unable to read the body from the incoming request")

reviewResponse = Errored(http.StatusBadRequest, err)

wh.writeResponse(w, reviewResponse)

return

}

} else {

err = errors.New("request body is empty")

wh.log.Error(err, "bad request")

reviewResponse = Errored(http.StatusBadRequest, err)

wh.writeResponse(w, reviewResponse)

return

}

// verify the content type is accurate

contentType := r.Header.Get("Content-Type")

if contentType != "application/json" {

err = fmt.Errorf("contentType=%s, expected application/json", contentType)

wh.log.Error(err, "unable to process a request with an unknown content type", "content type", contentType)

reviewResponse = Errored(http.StatusBadRequest, err)

wh.writeResponse(w, reviewResponse)

return

}

and I think it will be exactly the ~same for authorization (https://github.com/gardener/gardener/blob/master/pkg/admissioncontroller/webhooks/auth/seed/handler.go#L68-L95).

Probably it should be made re-useable and moved into a function?

christopherhein · 2021-03-22T22:12:04Z

~~Maybe I should take a step back and first move some of this logic out of the admission package in separate PRs then come back to this and use those packages.~~

@vincepri curious what you think, you'll see we called out a bunch of duplication. I can move some of those packages around/make a little more generic upfront in separate PRs then adjust this PR to have the moved packages/helpers, or we could move forward with some duplication and clean up after…

Rethinking this implementation a bit I was able to cut down on a bunch of the duplication, @rfranzke about the checks, got ideas on how we could pass all the different implementations of Response around? Since that and write response maps to specific writers for those k8s types.

christopherhein · 2021-03-24T20:56:48Z

@alvaroaleman any chance you'd have some time to review this? 🙏

pkg/webhook/alias.go

DirectXMan12 · 2021-03-24T21:14:07Z

pkg/webhook/authentication/http.go

+	// unregistered type to the v1 GVK, the decoder will coerce a v1beta1 TokenReview to v1.
+	// The actual TokenReview GVK will be used to write a typed response in case the
+	// webhook config permits multiple versions, otherwise this response will fail.
+	req := Request{}


this is maybe more dangerous that it might seem? What if a new field is added to v1beta1 that's actually different than v1 (they're not guarnateed to be the same, just round-trippable). We could end up making a decision on bad data, right?

I suppose for new named fields this is the same as operating on old types against a new apiserver.

If v1 and v1beta1 both introduced a field called foo with different types though, this would just break entirely.

Is v1beta1 deprecated? May be worth commenting, cause that makes the chances of that happening much less.

v1beta1 is deprecated in 1.19 as per - https://github.com/kubernetes/api/blob/master/authentication/v1beta1/types.go#L25-L31 currently it looks like v1 and v1beta1 are in parity. Given it's deprecated I assume that means we can support this with the expectation that v1beta1 shouldn't chnage, right?

If so I can add a note to the comment above. A lot of this was carried over from the Admissions webhook, even this comment tbh.

For now I've added a comment, mentioning that v1beta1 is deprecated as of 1.19 and will be removed in 1.22 per the v1.22 deprecation guide - https://kubernetes.io/docs/reference/using-api/deprecation-guide/#tokenreview-v122

alvaroaleman

Sorry for the delay in reviewing, I have a lot on my plate right now and this is a pretty big PR :) I am also not very familiar with the existing webhook code, so apologies if I am criticizing something you got from there.

alvaroaleman · 2021-03-26T13:27:37Z

pkg/webhook/authentication/http.go

+
+	var reviewResponse Response
+	if r.Body != nil {
+		if body, err = ioutil.ReadAll(r.Body); err != nil {


We need to close the body if its non-nil

#1440 (comment)

alvaroaleman · 2021-03-26T13:29:33Z

pkg/webhook/authentication/http.go

+			return
+		}
+	} else {
+		err = errors.New("request body is empty")


From the godocs of net/http.Request:

For server requests, the Request Body is always non-nil // but will return EOF immediately when no body is present.

So this doesn't need to be an else but simply at the top level after the r.Body != nil and check for the length of var body instead

This is another carryover from the admission package. https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/webhook/admission/http.go#L53-L67

That being said since we're not actually checking the HTTP Method, I believe this is what this block is meant to confirm that this isn't a GET, but I could have also understood the goals of it.

Do you think I should remove this?

I've switched around the way this is handled, the first check r.Body == nil now checks if it's a request that doesn't support a body, like a Get and fails out then we go on to reading the body.

alvaroaleman · 2021-03-26T13:30:43Z

pkg/webhook/authentication/http.go

+	}
+
+	// verify the content type is accurate
+	contentType := r.Header.Get("Content-Type")


Is this check needed? Deserialization will fail if its not json or not?

This was carried over from the admission package. https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/webhook/admission/http.go#L69-L77 I'd guess this is to give a bit more visibility into what actually went wrong in the request. Should I remove it?

So I guess that isn't what happens, the decoder doesn't seem to check what the actual headers are. If you don't have this check and the body is valid the request could be successful or could fail other places.

you get better errors this way too -- if you try to deserialize proto as json, for instance, you just end up with a really confusing error, whereas if you explicitly check content-type, you can provide a better error (plus, it's more correct, and like maybe theoretically avoids weird behavior where some blob of non-json looks enough like json to be deserialized, but is subtly wrong).

pkg/webhook/authentication/http.go

christopherhein · 2021-04-07T18:03:09Z

/test pull-controller-runtime-test-master

Signed-off-by: Chris Hein <me@chrishein.com>

christopherhein · 2021-05-06T02:46:18Z

@alvaroaleman this should be all updated.

DirectXMan12 · 2021-05-14T20:24:22Z

pkg/webhook/authentication/response_test.go

+			))
+
+			By("checking that a message is populated for 'Unauthenticated' responses")
+			Expect(ReviewResponse(false, authenticationv1.UserInfo{}, "UNACCEPTABLE!")).To(Equal(


so unacceptable 😝

DirectXMan12

/lgtm
/approve

k8s-ci-robot · 2021-05-14T20:28:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christopherhein, DirectXMan12

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~~ [DirectXMan12]

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

christopherhein · 2021-05-14T20:39:39Z

/test pull-controller-runtime-test-master

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 19, 2021

k8s-ci-robot requested review from mengqiy and shawn-hurley March 19, 2021 15:41

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 19, 2021

christopherhein changed the title ~~[WIP] 🌱 adding TokenReview.auth.k8s.io/v1 webhook support~~ 🌱 adding TokenReview.auth.k8s.io/v1 webhook support Mar 19, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 19, 2021

k8s-ci-robot assigned alvaroaleman Mar 19, 2021

k8s-ci-robot requested a review from coderanger March 19, 2021 15:43

christopherhein force-pushed the tokenreview-webhook-support branch from 4d07c03 to a9ebca3 Compare March 19, 2021 16:38

christopherhein commented Mar 19, 2021

View reviewed changes

pkg/webhook/authentication/decoder.go Outdated Show resolved Hide resolved

christopherhein commented Mar 19, 2021

View reviewed changes

pkg/webhook/authentication/inject.go Outdated Show resolved Hide resolved

christopherhein commented Mar 19, 2021

View reviewed changes

pkg/webhook/alias.go Outdated Show resolved Hide resolved

k8s-ci-robot assigned rfranzke Mar 19, 2021

rfranzke reviewed Mar 20, 2021

View reviewed changes

christopherhein force-pushed the tokenreview-webhook-support branch 5 times, most recently from 057ee0d to fe774d6 Compare March 23, 2021 04:11

DirectXMan12 reviewed Mar 24, 2021

View reviewed changes

christopherhein force-pushed the tokenreview-webhook-support branch 3 times, most recently from a0dcd8d to a0e1d83 Compare March 24, 2021 23:10

alvaroaleman reviewed Mar 26, 2021

View reviewed changes

christopherhein force-pushed the tokenreview-webhook-support branch from a0e1d83 to 3c8b36e Compare March 29, 2021 17:39

christopherhein force-pushed the tokenreview-webhook-support branch from 3c8b36e to 53d0278 Compare April 6, 2021 23:43

christopherhein force-pushed the tokenreview-webhook-support branch from 53d0278 to 9e86edf Compare May 6, 2021 02:14

adding TokenReview.auth.k8s.io/v1 webhook support

6ae3ee1

Signed-off-by: Chris Hein <me@chrishein.com>

christopherhein force-pushed the tokenreview-webhook-support branch from 9e86edf to 6ae3ee1 Compare May 6, 2021 02:32

DirectXMan12 reviewed May 14, 2021

View reviewed changes

DirectXMan12 approved these changes May 14, 2021

View reviewed changes

k8s-ci-robot assigned DirectXMan12 May 14, 2021

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 14, 2021

k8s-ci-robot merged commit ae50d4c into kubernetes-sigs:master May 14, 2021

k8s-ci-robot added this to the v0.9.x milestone May 14, 2021

christopherhein deleted the tokenreview-webhook-support branch May 14, 2021 20:46

rfranzke mentioned this pull request May 20, 2021

🌱 adding SubjectAccessReview.authorization.k8s.io/v1 webhook support #1535

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 adding TokenReview.auth.k8s.io/v1 webhook support #1440

🌱 adding TokenReview.auth.k8s.io/v1 webhook support #1440

christopherhein commented Mar 19, 2021

christopherhein commented Mar 19, 2021

christopherhein commented Mar 19, 2021

christopherhein commented Mar 19, 2021

rfranzke commented Mar 19, 2021

rfranzke left a comment

rfranzke Mar 20, 2021

christopherhein commented Mar 22, 2021 •

edited

christopherhein commented Mar 24, 2021

DirectXMan12 Mar 24, 2021

christopherhein Mar 24, 2021

christopherhein Mar 24, 2021 •

edited

alvaroaleman left a comment

alvaroaleman Mar 26, 2021

christopherhein Mar 29, 2021

alvaroaleman Mar 26, 2021

christopherhein Mar 29, 2021

christopherhein Mar 29, 2021

christopherhein May 6, 2021

alvaroaleman Mar 26, 2021

christopherhein Mar 29, 2021

christopherhein May 6, 2021

DirectXMan12 May 14, 2021

christopherhein commented Apr 7, 2021

christopherhein commented May 6, 2021

DirectXMan12 May 14, 2021 •

edited

christopherhein May 14, 2021

DirectXMan12 left a comment

k8s-ci-robot commented May 14, 2021

christopherhein commented May 14, 2021

	var body []byte
	var err error
	ctx := r.Context()
	if wh.WithContextFunc != nil {
	ctx = wh.WithContextFunc(ctx, r)
	}

	var reviewResponse Response
	if r.Body != nil {
	if body, err = ioutil.ReadAll(r.Body); err != nil {
	wh.log.Error(err, "unable to read the body from the incoming request")
	reviewResponse = Errored(http.StatusBadRequest, err)
	wh.writeResponse(w, reviewResponse)
	return
	}
	} else {
	err = errors.New("request body is empty")
	wh.log.Error(err, "bad request")
	reviewResponse = Errored(http.StatusBadRequest, err)
	wh.writeResponse(w, reviewResponse)
	return
	}

	// verify the content type is accurate
	contentType := r.Header.Get("Content-Type")
	if contentType != "application/json" {
	err = fmt.Errorf("contentType=%s, expected application/json", contentType)
	wh.log.Error(err, "unable to process a request with an unknown content type", "content type", contentType)
	reviewResponse = Errored(http.StatusBadRequest, err)
	wh.writeResponse(w, reviewResponse)
	return
	}

🌱 adding TokenReview.auth.k8s.io/v1 webhook support #1440

🌱 adding TokenReview.auth.k8s.io/v1 webhook support #1440

Conversation

christopherhein commented Mar 19, 2021

christopherhein commented Mar 19, 2021

christopherhein commented Mar 19, 2021

christopherhein commented Mar 19, 2021

rfranzke commented Mar 19, 2021

rfranzke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christopherhein commented Mar 22, 2021 • edited

christopherhein commented Mar 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christopherhein Mar 24, 2021 • edited

Choose a reason for hiding this comment

alvaroaleman 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

christopherhein commented Apr 7, 2021

christopherhein commented May 6, 2021

DirectXMan12 May 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DirectXMan12 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 14, 2021

christopherhein commented May 14, 2021

christopherhein commented Mar 22, 2021 •

edited

christopherhein Mar 24, 2021 •

edited

DirectXMan12 May 14, 2021 •

edited