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

Add authn policy design with JWT only #529

Merged
merged 18 commits into from
Dec 7, 2022

Conversation

lizan
Copy link
Member

@lizan lizan commented Oct 9, 2022

Fixes #336

@lizan lizan requested a review from a team as a code owner October 9, 2022 08:33
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
@lizan lizan force-pushed the authn_policy_doc branch from a591307 to 9a6ed41 Compare October 9, 2022 08:34
name: productpage
spec:
type: jwt
jwt:
Copy link
Contributor

Choose a reason for hiding this comment

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

if multiple issuers exist, does the user create multiple authn policies or will this policy allow the user to specify a list of issuers and associated jwksUri ?

Copy link
Contributor

Choose a reason for hiding this comment

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

#733 provides details. Multiple jwtProviders can be specified in an Authentication. The JWT auth passes if any one of the providers responds with a success. If an HTTPRoute references multiple Authentications, then at least one jwtProviders for each Authentication must pass for JWT auth to succeed.

@danehans
Copy link
Contributor

@lizan as we discussed during today's Envoy Gateway community meeting, this design should be reviewed with the Gateway API GAMMA team. It would be beneficial for the extension to be supported by both projects.

@danehans danehans added the documentation Improvements or additions to documentation label Oct 12, 2022
@danehans danehans modified the milestones: 0.2.0, 0.3.0-rc.1 Oct 12, 2022
Copy link
Member

@sunjayBhatia sunjayBhatia left a comment

Choose a reason for hiding this comment

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

Great to see some Gateway API extension CRDs being worked on, we're doing the same over at Contour, still in early staged trying to get settled how we will approach the extension mechanisms generally

Left a few comments and questions

If you like we can talk more broadly about how to approach the extension mechanisms, I've started some notes for Contour specifically here: projectcontour/contour#4749 but there is some background info that is more widely applicable as well

Also got some example spikes on how one might implement local rate limiting with Filters vs. Policies here:


In the first phase, Envoy Gateway will implement JWT Bearer Token authentication.

In general those policy translates into Envoy HTTP filters at HTTP connection manager level, and route specific settings will be applied for each route. These APIs are expressed in a Policy CRD and attached to Gateway API resources with [Policy Attachement](https://gateway-api.sigs.k8s.io/references/policy-attachment/).
Copy link
Member

Choose a reason for hiding this comment

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

re: route specific settings will be applied for each route

The Gateway API Policy targetRef field does not yet have a way to specify a sub-section of a resource, so applying a Policy to an individual route rule is not "directly" possible, unless you require users to separate that route rule into its own HTTPRoute resource (in this case you might have a Policy target a Gateway and another target an attached HTTPRoute that specifies some overrides just for it)

Am I understanding the intent here properly? Might be good to flesh out some more examples on how this might work

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for writing down that in depth analysis !
and surfacing some limitations with the PolicyAttachment that exist today.
Hoping those get incorporated soon, @youngnick has already acknowledged this !

Until then, since we plan on adding new features such as authentication in our next v0.3.0 release, imho we should peruse the existing spec for now

Copy link
Contributor

Choose a reason for hiding this comment

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

From the Gateway API Policy Attachment spec:

Although Policy attachment can be used to target an entire Route or Backend, it cannot currently be used to target specific Route rules or backend references.

Based on this information, it appears that an authentication API fits better as an implementation-specific filter since HTTPRoute rules may require different authentication specs. In the future, EG can add GWAPI Policy Attachment support for authentication defaulting and overrides.

Copy link
Member

Choose a reason for hiding this comment

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

HTTPRoute Filters themselves also have some drawbacks as they may not directly translate to Envoy HTTP Filters, given the scope they are configurable at.

HTTPRoute Filters can be configured per-Route Rule or per-Backend and Envoy HTTP Filters mostly have the bulk of their configuration as part of the Filter in a per-virtualhost HTTPConnectionManager FilterChain. I say "mostly" here because many filters enable per-Route enablement/disablement, but most of the configuration (e.g. JWT provider specification: https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/http/jwt_authn/v3/config.proto#envoy-v3-api-msg-extensions-filters-http-jwt-authn-v3-jwtauthentication) lives at the higher level Filter object.

We may be able to have per-route JWT providers we collect from HTTPRoute Filter objects and then configure on the Envoy JWT HTTP Filter, but I haven't actually tried it so not sure how complex that will get.

Contour implemented this recently in HTTPProxy here:

Curious to see your thoughts @lizan if you've gotten a chance to spike this out as a Policy vs. a Filter

Copy link
Contributor

Choose a reason for hiding this comment

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

HTTPRoute Filters can be configured per-Route Rule or per-Backend

#733 implements authN as an HTTPRoute filter.

We may be able to have per-route JWT providers we collect from HTTPRoute Filter objects and then configure on the Envoy JWT HTTP Filter, but I haven't actually tried it so not sure how complex that will get.

I am going to test this as an approach for implementing authN. @lizan please comment if you have any suggestions.

docs/design/authentication_policy.md Outdated Show resolved Hide resolved
@danehans danehans mentioned this pull request Nov 1, 2022
7 tasks
@arkodg
Copy link
Contributor

arkodg commented Nov 1, 2022

@lizan any new insights wrt this PR ?
this PR / Issue will be setting the standard for other advanced features scheduled in this release and should be the reference pattern for those features such as ratelimiting, cors, etc

The one thing I'd add is to consider limiting the Policy CRDs to 1 or 2 i.e.

  1. one generic policy CRD with features as top level fields
apiVersion: gateway.envoyproxy.io/v1alpha1
 kind: Policy
 .....
 spec:
   authentication:
   .....
   ratelimiting:
   ....
  1. one generic policy CRD with networking (shapes traffic) and security (authenticates & authorizes traffic) as top fields
apiVersion: gateway.envoyproxy.io/v1alpha1
 kind: Policy
 .....
 spec:
   security:
     authentication:
     ....
   networking:
     ratelimiting:
     .....
  1. 2 Policy CRDs - one for networking and the other for security
apiVersion: gateway.envoyproxy.io/v1alpha1
 kind: SecurityPolicy
 ....
 ....
 spec:
   authentication:
apiVersion: gateway.envoyproxy.io/v1alpha1
 kind: NetworkingPolicy
 ....
 spec:
   ratelimiting:
   ....

@danehans
Copy link
Contributor

danehans commented Nov 1, 2022

During today's EG community meeting, a consensus was achieved for using separate API types to express request authentication and other advanced Envoy features, e.g. #671, #670, etc.

@arkodg
Copy link
Contributor

arkodg commented Nov 2, 2022

During today's EG community meeting, a consensus was achieved for using separate API types to express request authentication and other advanced Envoy features, e.g. #671, #670, etc.

@danehans raised #677 so all our votes are recorded and maintainers who weren't able to attend can also vote.

lizan and others added 2 commits November 2, 2022 10:45
Co-authored-by: Arko Dasgupta <arkodg@users.noreply.github.com>
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
Co-authored-by: Arko Dasgupta <arkodg@users.noreply.github.com>
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
@arkodg arkodg added the priority/high Label used to express the "high" priority level label Nov 3, 2022
danehans and others added 2 commits November 8, 2022 12:19
Signed-off-by: danehans <daneyonhansen@gmail.com>
danehans and others added 5 commits November 11, 2022 13:38
Signed-off-by: danehans <daneyonhansen@gmail.com>
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
Co-authored-by: Arko Dasgupta <arkodg@users.noreply.github.com>
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
Co-authored-by: Arko Dasgupta <arkodg@users.noreply.github.com>
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
Signed-off-by: danehans <daneyonhansen@gmail.com>
@danehans
Copy link
Contributor

Based on #675 (comment), maintainers have agreed to use an HTTPRoute implementation-specific filter to implement request authentication. The design doc currently follows this approach.

Signed-off-by: danehans <daneyonhansen@gmail.com>
Signed-off-by: danehans <daneyonhansen@gmail.com>

### Security API Group

A new API group, `security.gateway.envoyproxy.io` is introduced to group security-related APIs. This will allow security
Copy link
Contributor

Choose a reason for hiding this comment

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

curious why a new group is needed ?
why not graduate individual extensions from v1alpha1 to v1beta1 similar to what Gateway API does https://github.com/kubernetes-sigs/gateway-api/tree/main/apis

Copy link
Contributor

Choose a reason for hiding this comment

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

A group is not needed per se. The reason for the group is documented. I don't have a strong opinion on the topic. We grouped the config APIs, so we've already established a pattern.

@codecov-commenter
Copy link

Codecov Report

Merging #529 (362eb96) into main (b8f234b) will increase coverage by 0.71%.
The diff coverage is 60.16%.

@@            Coverage Diff             @@
##             main     #529      +/-   ##
==========================================
+ Coverage   62.72%   63.43%   +0.71%     
==========================================
  Files          42       47       +5     
  Lines        4496     5927    +1431     
==========================================
+ Hits         2820     3760     +940     
- Misses       1532     1929     +397     
- Partials      144      238      +94     
Impacted Files Coverage Δ
internal/cmd/certgen.go 12.82% <ø> (ø)
internal/cmd/server.go 7.62% <0.00%> (-0.27%) ⬇️
internal/cmd/versions.go 50.00% <0.00%> (+21.42%) ⬆️
internal/cmd/xdstest.go 3.65% <ø> (ø)
internal/crypto/certgen.go 77.21% <ø> (ø)
internal/envoygateway/config/config.go 0.00% <ø> (ø)
internal/gatewayapi/sort.go 97.56% <ø> (+20.81%) ⬆️
internal/infrastructure/kubernetes/labels.go 100.00% <ø> (ø)
internal/ir/infra.go 67.41% <ø> (ø)
internal/ir/zz_generated.deepcopy.go 19.09% <0.00%> (+19.09%) ⬆️
... and 51 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

- Should local JWKS be implemented before remote JWKS?
- How should Envoy obtain the trusted CA for a remote JWKS?
- Should HTTPS be the only supported scheme for remote JWKS?
- Should OR'ing JWT providers be supported?
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm working through the implementation to support this.

- OIDC
- External authentication

## Outstanding Questions
Copy link
Contributor

Choose a reason for hiding this comment

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

I would appreciate any feedback to help resolve these outstanding questions.


## Outstanding Questions

- If Envoy Gateway owns the Authentication API, is an xDS IR equivalent needed?
Copy link
Contributor

Choose a reason for hiding this comment

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

auth will be a field within the xds IR route or listener struct

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct, and auth will be of type #733 as opposed to an internal-only API type. This is because we own this API as opposed to any of the core or extended Gateway APIs.

...
default_filter_chain:
filters:
- name: envoy.filters.network.http_connection_manager_1
Copy link
Member Author

Choose a reason for hiding this comment

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

This won't work, HCM is a terminal filter, so any filter after that won't be effect and be rejected.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lizan I have updated the JWT xDS configuration example, PTAL.

Signed-off-by: danehans <daneyonhansen@gmail.com>
Comment on lines +369 to +377
rules:
- match:
exact: /foo
requires:
provider_name: default-example1-example1
- match:
exact: /bar
requires:
provider_name: default-example2-example2
Copy link
Member Author

Choose a reason for hiding this comment

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

This works, though we might end up with per route config and requirement map when this grows.

kflynn
kflynn previously approved these changes Nov 29, 2022
Copy link
Contributor

@kflynn kflynn left a comment

Choose a reason for hiding this comment

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

Overall I think this looks really good. As discussed in the 29 November Envoy Gateway meeting, I think it's important that this stay at alpha until we have real-world usage to back it up.

## Outstanding Questions

- If Envoy Gateway owns the Authentication API, is an xDS IR equivalent needed?
- Should local JWKS be implemented before remote JWKS?
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the next two questions about remote JWKS, I'd definitely vote in favor of local first.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure why my previous comment is not shown anymore #529 (comment)

Remote JWKS first is strongly preferred as for most of public JWT issuer, keys are rotated quite frequently. (e.g. Google OAuth2 public key https://www.googleapis.com/oauth2/v3/certs rotates daily IIRC).

Rest of questions answered there as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on prioritizing remote jwks.

docs/latest/design/request-authentication.md Show resolved Hide resolved
@danehans
Copy link
Contributor

Definitely worth considering what status updates would make sense.

@kflynn I agree. I would like to add status as a follow-on after implementing the API and gaining an understanding of how it's used.

@danehans
Copy link
Contributor

Overall I think this looks really good. As discussed in the 29 November Envoy Gateway meeting, I think it's important that this stay at alpha until we have real-world usage to back it up.

@kflynn agreed and why I created #714.

Signed-off-by: danehans <daneyonhansen@gmail.com>
@danehans
Copy link
Contributor

commit 947359c removes the security API group and renames the API to AuthenticationFilter.

cc: @arkodg @youngnick

@danehans danehans merged commit 0456aa7 into envoyproxy:main Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation priority/high Label used to express the "high" priority level
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Design Authentication Policy as a GatewayAPI Extension
7 participants