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: SecurityPolicy #1950

Merged
merged 3 commits into from
Oct 25, 2023
Merged

Conversation

arkodg
Copy link
Contributor

@arkodg arkodg commented Oct 11, 2023

Relates to #1845

Relates to envoyproxy#1845

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
@arkodg arkodg requested a review from a team as a code owner October 11, 2023 01:06
@codecov
Copy link

codecov bot commented Oct 11, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@a00d289). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1950   +/-   ##
=======================================
  Coverage        ?   65.11%           
=======================================
  Files           ?       99           
  Lines           ?    14410           
  Branches        ?        0           
=======================================
  Hits            ?     9383           
  Misses          ?     4443           
  Partials        ?      584           

qicz
qicz previously approved these changes Oct 11, 2023
* This API will only support a single `targetRef` and can bind to a `Gateway` resource or a `HTTPRoute` or `GRPCRoute`.
* This API resource MUST be part of same namespace as the targetRef resource
* There can be only be ONE policy resource attached to a specific targetRef e.g. a `Listener` (section) within a `Gateway`
* If the policy targets a resource but cannot attach to it, this information should be reflected
Copy link
Member

@zhaohuabing zhaohuabing Oct 11, 2023

Choose a reason for hiding this comment

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

Is this the solution for OIDC before per-route configuration is supported?
If a policy contains OIDC targets an HTTPRoute, it fails with the Conflicted=True condition in the status.

Per-route OIDC issue: envoyproxy/envoy#29641

Copy link
Contributor Author

@arkodg arkodg Oct 11, 2023

Choose a reason for hiding this comment

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

we cannot add OIDC into this API until per route in envoy is supported

Copy link
Member

Choose a reason for hiding this comment

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

Adding Per-route configuration for OIDC is a big change, and it won't be available soon. How about we start by providing OIDC at the Gateway and Listener levels and then add per-route support when it's ready? Listener-level OIDC should cover most of what EG users need.

Per-route OIDC issue: envoyproxy/envoy#29641

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ptal @envoyproxy/gateway-maintainers

Copy link
Member

Choose a reason for hiding this comment

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

I verified that per-route configuration can be accomplished with "route level filter disabled support", so this is no longer blocking us. https://github.com/zhaohuabing/playground/blob/main/envoy/per-route-oauth2-oidc/envoy.yaml

* Policy A will be applied/attached to the specific Listener defined in the `targetRef.SectionName`
* Policy B will be applied to the remaining Listeners within the Gateway. Policy B will have an additional
status condition `Overridden=True`.
* A Policy targeting a xRoute (`HTTPRoute` or `GRPCRoute`) overrides a Policy targeting a Listener that is
Copy link
Member

@zhaohuabing zhaohuabing Oct 11, 2023

Choose a reason for hiding this comment

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

In Envoy, multiple routes share the same global HCM filter-level configuration, so a policy targeting an xRoute may conflict with a policy targeting its owner Gateway, or other xRoutes bound to the same Gateway.

For example, the following two policies contain ext auth configurations with different grcp_service configured, but only one grpc_service can be set at the HCM filter level.

Policy 1

apiVersion: gateway.envoyproxy.io/v1alpha1
kind: SecurityPolicy
metadata:
  name: ext-auth-policy-gateway
  namespace: default
spec:
  extAuth:
    grpcService:
      envoyGrpc:
          clusterName: ext-authz-service1
      ......     
  targetRef:
    group: gateway.networking.k8s.io
    kind: Gateway
    name: eg
    namespace: default

Policy 2

apiVersion: gateway.envoyproxy.io/v1alpha1
kind: SecurityPolicy
metadata:
  name: ext-auth-policy-gateway
  namespace: default
spec:
  extAuth:
    grpcService:
      envoyGrpc:
          clusterName: ext-authz-service2
      ......
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: foo
    namespace: default   

Envoy configuration:

http_filters:
  - name: envoy.filters.http.ext_authz
    typed_config:
      "@type": type.googleapis.com/envoy.extensions.filters.http.ext_authz.v3.ExtAuthz
      grpc_service:  // only one grpc service is allowed here
        envoy_grpc:
          cluster_name: ext-authz-service1
      ......

We need to define how to handle this kind of conflict in the design docs. I can think of two rules:

  • When multiple policies are at the same level, the oldest one wins.
  • When multiple policies are at different level, The one with the largest scope wins: Gateway > Listener > xRoute

Copy link
Contributor Author

@arkodg arkodg Oct 11, 2023

Choose a reason for hiding this comment

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

in the example you shared Policy 2 overrides Policy 1 and its entire spec wins since a xRoute target has high precedence over Gateway or Gateway section target (listener), so only Policy2's spec will be applied and translated

Copy link
Member

@zhaohuabing zhaohuabing Oct 12, 2023

Choose a reason for hiding this comment

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

There may be other HTTPRoutes. So Policy2 will be applied to HTTPRoute foo, and Policy1 should be applied to the owner listeners of foo as the default config for other HTTPRoutes. The conflict still exists.

A simpler example of a conflict:

HTTPRoute foo and bar target the same Gateway

apiVersion: gateway.networking.k8s.io/v1beta1
kind: HTTPRoute
metadata:
  name: foo
  namespace: default
spec:
  parentRefs:
    - name: eg
  hostnames:
    - "www.example.com"
  rules:
    - backendRefs:
        - group: ""
          kind: Service
          name: backend-foo
          port: 3000
          weight: 1
      matches:
        - path:
            type: PathPrefix
            value: /foo
---

apiVersion: gateway.networking.k8s.io/v1beta1
kind: HTTPRoute
metadata:
  name: bar
  namespace: default
spec:
  parentRefs:
    - name: eg
  hostnames:
    - "www.example.com"
  rules:
    - backendRefs:
        - group: ""
          kind: Service
          name: backend-bar
          port: 3000
          weight: 1
      matches:
        - path:
            type: PathPrefix
            value: /bar

Policy foo targets HTTPRoute foo, Policy bar targets HTTPRoute bar, and they have different extAuth configuration.

apiVersion: gateway.envoyproxy.io/v1alpha1
kind: SecurityPolicy
metadata:
  name: ext-auth-policy-foo
  namespace: default
spec:
  extAuth:
    grpcService:
      envoyGrpc:
          clusterName: ext-authz-service2
      ......
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: foo
    namespace: default 
---

apiVersion: gateway.envoyproxy.io/v1alpha1
kind: SecurityPolicy
metadata:
  name: ext-auth-policy-bar
  namespace: default
spec:
  extAuth:
    grpcService:
      envoyGrpc:
          clusterName: ext-authz-service2
      ......
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: bar
    namespace: default   

According to the design, these two policies should co-exist because they apply to different HTTPRoutes, however, after translating to xDS, they have to share a single grpc_service configuration at the HCM filter level.

In the Envoy configuration, only one grpc_service is allowed.

http_filters:
  - name: envoy.filters.http.ext_authz
    typed_config:
      "@type": type.googleapis.com/envoy.extensions.filters.http.ext_authz.v3.ExtAuthz
      grpc_service:  // only one grpc service is allowed at the HCM filter level
        envoy_grpc:
          cluster_name: ext-authz-service1
      ......

JWT filter doesn't have this problem because the JWT filter configuration allows multiple providers configured. EG can combine the providers of all the HTTPRoutes of a listener when translating.

providers:
  provider1:
    issuer: https://provider1.com
    local_jwks:
      inline_string: PUBLIC-KEY
  provider2:
    issuer: https://provider2.com
    local_jwks:
      inline_string: PUBLIC-KEY

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Copy link
Member

@zhaohuabing zhaohuabing left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for putting up the design.

@arkodg arkodg requested review from a team and Alice-Lilith and removed request for a team October 12, 2023 16:07
Signed-off-by: zirain <zirain2009@gmail.com>
@arkodg arkodg merged commit 1365090 into envoyproxy:main Oct 25, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants