Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

design: Adds external-auth support #1014

Closed

Conversation

simonswine
Copy link

This document outlines a specification to allow Contour to make use of the external authentication http_filter of Envoy to implement authentication.

This is an early version of a design draft. I have created an ugly PoC for it and this represents my learnings and my suggested approach. Happy for any feedback!

The feature is described in #432

This document outlines a specification to allow Contour to make use of
the [external authentication] http_filter of Envoy to implement
authentication.

Signed-off-by: Christian Simon <simon@swine.de>
@alexbrand
Copy link
Contributor

Thanks for taking the time to write this up @simonswine!

I am very curious as to what others think, but in my mind, Contour does not seem like the right place to implement the authentication/authorization server. Contour is an ingress controller, and adding the auth concerns seems to muddy its responsibilities and security posture.

With that said, from a user's perspective, it might be painful to have to implement an external authN/Z server if I wanted to use this. I wonder if the Open Policy Agent would make sense here? It looks like there is an (improperly named) OPA adapter/plugin that implements the Envoy Ext Auth API: https://github.com/open-policy-agent/opa-istio-plugin

Something else that is a bit unclear to me is: what are we trying to solve? The proposal talks about Basic Auth and OAuth, but #432 seems to be about exposing the right knobs so that users can bring their own AuthN/Z server. I think it might be beneficial to discuss auth use cases to try to come up with a solution.

@glerchundi
Copy link
Contributor

glerchundi commented Apr 13, 2019 via email

Copy link
Contributor

@davecheney davecheney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@simonswine thank you for working on this.

I have to be honest up front that #432 is not on the schedule for 0.12 so my time to review and comment on this proposal will be limited.

Some initial feedback (sorry my time is limited, maybe Alex or Steve can drive this):

The way envoys authz filter works is worrying, there is a large potential for screwing up when it needs to be turned on globally then turned off everywhere but where it is needed. For https routes I think we can avoid this by applying the filter at the listener level. for http routes maybe we can make the http listener look more like the https one.

Please take basic auth out of the picture. It's not in scope and complicated by http listeners which we cannot allow it on.

If we push basic auth out of scope the Authentication CRD is a lot of work to carry two parameters, can those parameters be merged into the IngressRoute CRD?

Thanks

Dave

@stevesloka
Copy link
Member

I kind of like having the separation with the Authentication CRD, it allows us to specify how something could auth centrally, then apply that to one or more routes.

I'm wondering though if we should apply auth at a Root IngressRoute level, where all routes within a single fqdn should use the same auth or if that's too broad of a statement. By doing this we could have many Root IngressRoutes all reference the same Authentication OIDC provider. If that provider changed we'd update just a single CRD which would roll across any other applications.

@glerchundi
Copy link
Contributor

Why not directly allow configuring the CheckRequest API instead of creating an abstraction? Really easy to sketch on the current IngressRoute and less API surface to be supported.

Once you make this configurable through the IngressRoute this allows other kind of deployments (separation of concerns for the win):

  • Envoy/Contour requesting authorization to a external gRPC service.
  • Envoy/Contour + (New CRDs those Authentication ones + Envoy/Contour implementing the authorization API): In this case Envoy would ask to Contour (which implements the ext_authz API) if the request is authorized or not.

Also, it seems like managed providers as well as other open source projects are embracing this 'Universal' API, for example: Google with its Traffic Director which implements xDS v2.

WDYT?

@stevesloka
Copy link
Member

Yup that's what I was suggesting @glerchundi, I see a way to configure the external gRPC service. I did something similar with my work on RateLimiting if you look at that PR. I showed how to use the Lyft implementation, but users could still integrate their own if they wanted.

@glerchundi
Copy link
Contributor

glerchundi commented May 24, 2019 via email

@glerchundi
Copy link
Contributor

@simonswine do you plan to make changes to this PR in the short term? I'm interested in making some progress and I've some time in the next two months. So in case you feel I can take this one let me know.

@github-actions
Copy link

Marking this PR stale since there has been no activity for 14 days. It will be closed if there is no activity for another 90 days.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 30, 2020
@michmike
Copy link
Contributor

michmike commented Mar 2, 2020

this PR is now stale. removing from the board so that when we prioritize #432, we will consider all options (including this design)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants