Authorization HTTPFilter #532

jmprusi · 2021-01-19T23:09:36Z

What type of PR is this?
/kind design

What this PR does / why we need it:
This PR shows a possible new Filter design, called RequestAuthorization, that would be used to authorize incoming HTTP requests with an external authorization component.

Design doc: Pluggable Access Control for HTTPRoute

Which issue(s) this PR fixes:
Fixes #114

Does this PR introduce a user-facing change?:

Adds a new HTTPFilter: "RequestAuthorization", that allows users to authorize incoming HTTP request with an external authorization service.

k8s-ci-robot · 2021-01-19T23:09:44Z

Welcome @jmprusi!

It looks like this is your first PR to kubernetes-sigs/service-apis 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/service-apis has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-01-19T23:09:44Z

Hi @jmprusi. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

apis/v1alpha1/httproute_types.go

howardjohn · 2021-01-19T23:18:25Z

apis/v1alpha1/httproute_types.go

+	//   It is expected for the authorization server to implement the Envoy ExtAuthz HTTP protocol:
+	//   https://www.envoyproxy.io/docs/envoy/latest/api-v3/service/auth/v3/external_auth.proto
+	//
+	URL string `json:"url"`


How does configuration of how to reach this URl work? For example, if we need authentication, TLS, etc.

if it were a k8s service it would be "easy" since existing APIs defined here like BackendPolicy would apply. For a generic URL, I am not sure its easy.

Yes, that's a good point. Thinking out loud, perhaps splitting the URL into a service reference and a type or protocol field to define the type of authorization (grpc/HTTP) could work?

That could potentially work, I think some sort of reference is probably more consistent with the rest of the API as well. FWIW we recently went through lots of debate with https://docs.google.com/document/d/1V4mCQCw7mlGp0zSQQXYoBdbKMDnkPOjeyUb85U07iSI/ about this exact topic in Istio.

Thanks, @howardjohn! I will review that document and try to find a better API design.

Just getting to review this one now, Contour added an ExtensionService to cover the "service reference" part of this (see https://projectcontour.io/docs/v1.12.0/config/api/#projectcontour.io/v1alpha1.ExtensionService), and then an AuthorizationServer struct that holds the ExtensionService reference. (https://projectcontour.io/docs/v1.12.0/config/api/#projectcontour.io/v1.AuthorizationServer). Both the ExtensionService and AuthorizationServer have places to put config.

I'm not suggesting doing the same thing, but I think the idea of a service reference and a type or protocol field would be great.

@youngnick It already has a protocol field, but for the service reference, we can follow the example of the other filters and replace URL with:

ServiceName *string `json:"serviceName,omitempty"` BackendRef *LocalObjectReference `json:"backendRef,omitempty"` Port *PortNumber `json:"port,omitempty"`

Or add a new structure that allows us to cross the namespace boundary, with additional information on how to connect to the external service, like for example a TLS client certificate to use.

apis/v1alpha1/httproute_types.go

robscott · 2021-01-22T02:27:12Z

Thanks for the work on this @jmprusi! Based on our conversation yesterday at the community meeting, it seems like there's broad support for an approach like this if we can make it portable. Ideally this filter should be usable with more than Envoy ExtAuthz. It seems like external auth is generally configured with the same approximate concepts:

Protocol
URL
Timeout
FailureMode

That seems rather similar to what is already proposed here. I'm hesitant about specifying which underlying approach an implementation of this API should take, but if we can avoid that, I think this has the potential to be a valuable and portable filter.

jmprusi · 2021-02-10T18:25:54Z

Thanks for the work on this @jmprusi! Based on our conversation yesterday at the community meeting, it seems like there's broad support for an approach like this if we can make it portable. Ideally this filter should be usable with more than Envoy ExtAuthz. It seems like external auth is generally configured with the same approximate concepts:

Protocol

URL

Timeout

FailureMode

That seems rather similar to what is already proposed here. I'm hesitant about specifying which underlying approach an implementation of this API should take, but if we can avoid that, I think this has the potential to be a valuable and portable filter.

I've updated the PR, but I think we should discuss a couple of fields:

Metadata: This could be a useful field to add. It will allow the user to add "hardcoded" specific information either for the controller or the authorization service. This can be implemented in the form of a Header for nginx or as context metadata in envoy.
RequestBody: Send the original request body to the authorization service or not. This can be configured in Envoy or Nginx.

Another interesting "common" field could be:

ForwardHeaders: A list of headers that will be forwarded to the authorization endpoint from the original request, instead of the "default" behavior of sending all of them or a known subset.

k8s-ci-robot · 2021-02-16T16:30:47Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jmprusi
To complete the pull request process, please assign robscott after the PR has been reviewed.
You can assign the PR to them by writing /assign @robscott in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

bowei · 2021-03-10T18:31:09Z

/ok-to-test

jpeach · 2021-03-10T20:45:45Z

apis/v1alpha1/httproute_types.go

+	// Support: Extended
+	//
+	// +optional
+	Metadata map[string]string `json:"metadata,omitempty"`


AFAIK, only the envoy GRPC protocol can pass arbitrary key value pairs here. How do you indicate missing or partial support for this field?

I think this is for the implementor to decide; for example, in Nginx, this could be implemented as a custom HTTP header sent to the authorization service. Perhaps we should put some validation in place to make that easier, like a max length and allowed characters.

If the implementation doesn't support this, it can reject the route and set the proper status condition.

jpeach · 2021-03-10T20:49:08Z

apis/v1alpha1/httproute_types.go

+	// Support: Extended
+	//
+	// +optional
+	// +kubebuilder:default=http


Can we add enum validation for allowed values?

jpeach · 2021-03-10T20:51:34Z

apis/v1alpha1/httproute_types.go

+	//
+	// +optional
+	// +kubebuilder:default=http
+	Protocol string `json:"protocol,omitempty"`


For protocols over TLS, is there a way to tweak the server certificate validation?

Yes, that would great to have, but I don't see any example of this in the API. I looked at the HTTPRouteForwardTo struct, and it doesn't seem to support any TLS settings. All the other TLS settings seem to be focused on the "listener side," not starting a connection.

Any suggestion on how can we add this to the filter?

evankanderson · 2021-07-27T18:51:11Z

(Looks like this got rebased a few weeks ago)

k8s-ci-robot · 2021-08-19T14:12:15Z

@jmprusi: PR needs rebase.

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.

hbagdi · 2021-08-23T23:40:01Z

I think it is unlikely that we will merge this. A custom filter would be the path forward here.
@robscott @jpeach @youngnick Thoughts?

robscott · 2021-08-24T00:51:02Z

I tend to agree with that. If there's a way we can provide some form of portability for authorization here that would be awesome, but we've got so many things on the go right now I'm not sure how soon we'll be able to tackle that. It's likely easiest to start as a custom filter.

hbagdi · 2021-08-24T15:00:11Z

Let the community experiment for now. If there is convergence, we would love to get this into this API itself.
/close

k8s-ci-robot · 2021-08-24T15:00:21Z

@hbagdi: Closed this PR.

In response to this:

Let the community experiment for now. If there is convergence, we would love to get this into this API itself.
/close

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.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/design Categorizes issue or PR as related to design. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 19, 2021

k8s-ci-robot requested review from robscott and thockin January 19, 2021 23:09

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 19, 2021

howardjohn reviewed Jan 19, 2021

View reviewed changes

jpeach reviewed Jan 20, 2021

View reviewed changes

apis/v1alpha1/httproute_types.go Outdated Show resolved Hide resolved

danehans reviewed Jan 20, 2021

View reviewed changes

apis/v1alpha1/httproute_types.go Outdated Show resolved Hide resolved

jmprusi force-pushed the auth_filter branch 2 times, most recently from c356917 to 8bcfc88 Compare February 10, 2021 18:18

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 15, 2021

jmprusi force-pushed the auth_filter branch from ef788db to 976992d Compare February 17, 2021 12:37

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 17, 2021

jmprusi changed the title ~~WIP - Authorization HTTPFilter~~ Authorization HTTPFilter Feb 17, 2021

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Feb 17, 2021

eloycoto mentioned this pull request Mar 2, 2021

Add Gateway API TLS Support projectcontour/contour#3404

Closed

3 tasks

jmprusi force-pushed the auth_filter branch from 976992d to 6e27dba Compare March 4, 2021 10:26

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 4, 2021

jmprusi requested a review from youngnick March 8, 2021 16:30

jmprusi requested review from danehans and jpeach March 8, 2021 16:30

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 10, 2021

jpeach reviewed Mar 10, 2021

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 28, 2021

jmprusi added 2 commits June 18, 2021 16:03

Adds new RequestAuthorization HTTPFilter

c9816f8

make generate

4ef30d9

jmprusi force-pushed the auth_filter branch from 6e27dba to 4ef30d9 Compare June 18, 2021 14:03

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 18, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 19, 2021

k8s-ci-robot closed this Aug 24, 2021

This was referenced Sep 22, 2023

GEP: Gateway/HTTPRoute level authentication #1494

Open

CFP: Support basic-auth/oauth2/mTLS for north-south traffic (GatewayAPI/Ingress) cilium/cilium#23797

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Authorization HTTPFilter #532

Authorization HTTPFilter #532

jmprusi commented Jan 19, 2021 •

edited

Loading

k8s-ci-robot commented Jan 19, 2021

k8s-ci-robot commented Jan 19, 2021

howardjohn Jan 19, 2021

jmprusi Jan 19, 2021

howardjohn Jan 19, 2021

jmprusi Jan 20, 2021

youngnick Feb 16, 2021

jmprusi Feb 17, 2021

robscott commented Jan 22, 2021

jmprusi commented Feb 10, 2021

k8s-ci-robot commented Feb 16, 2021

bowei commented Mar 10, 2021

jpeach Mar 10, 2021

jmprusi Mar 11, 2021

jpeach Mar 10, 2021

jpeach Mar 10, 2021

jmprusi Mar 11, 2021

evankanderson commented Jul 27, 2021

k8s-ci-robot commented Aug 19, 2021

hbagdi commented Aug 23, 2021

robscott commented Aug 24, 2021

hbagdi commented Aug 24, 2021

k8s-ci-robot commented Aug 24, 2021

Authorization HTTPFilter #532

Authorization HTTPFilter #532

Conversation

jmprusi commented Jan 19, 2021 • edited Loading

k8s-ci-robot commented Jan 19, 2021

k8s-ci-robot commented Jan 19, 2021

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

robscott commented Jan 22, 2021

jmprusi commented Feb 10, 2021

k8s-ci-robot commented Feb 16, 2021

bowei commented Mar 10, 2021

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

evankanderson commented Jul 27, 2021

k8s-ci-robot commented Aug 19, 2021

hbagdi commented Aug 23, 2021

robscott commented Aug 24, 2021

hbagdi commented Aug 24, 2021

k8s-ci-robot commented Aug 24, 2021

jmprusi commented Jan 19, 2021 •

edited

Loading