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

ext_authz is ignored for paths matching a grpc_json_transcoder filter #9929

Closed
sgtsquiggs opened this issue Feb 4, 2020 · 6 comments
Closed
Labels
investigate Potential bug that needs verification stale stalebot believes this issue/PR has not been touched recently

Comments

@sgtsquiggs
Copy link

sgtsquiggs commented Feb 4, 2020

(as of v1.13.0)

Having trouble hiding a grpc_json transcoded server behind ext_authz. With the below config, any valid guinanpb/Guinan endpoint skips over ext_authz.

If I rearrange the filters (grpc_json before authz) the authz check occurs, but envoy always returns a 500 error code (my service redirects for SAML auth)

admin:
  access_log_path: /dev/null
  address:
    socket_address: { address: 127.0.0.1, port_value: 9901 }

static_resources:
  listeners:
    - name: listener1
      address:
        socket_address: { address: 0.0.0.0, port_value: 15000 }
      filter_chains:
        - filters:
            - name: envoy.http_connection_manager
              typed_config:
                "@type": type.googleapis.com/envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager
                stat_prefix: grpc_json
                codec_type: AUTO
                route_config:
                  name: local_route
                  virtual_hosts:
                    - name: local_service
                      domains: ["*"]
                      cors:
                        allow_credentials: true
                        allow_methods: GET, PUT, DELETE, POST, PATCH, OPTIONS
                        allow_headers: authorization,keep-alive,user-agent,cache-control,content-type,content-transfer-encoding,x-accept-content-transfer-encoding,x-accept-response-streaming,x-user-agent
                        expose_headers: grpc-status,grpc-message,x-envoy-upstream-service-time
                      routes:
                        - match: { path: "/_saml_callback" }
                          route: { cluster: samlauth }
                          typed_per_filter_config:
                            envoy.ext_authz:
                              "@type": type.googleapis.com/envoy.config.filter.http.ext_authz.v2.ExtAuthzPerRoute
                              disabled: true
                        - match: { prefix: "/guinanpb.Guinan" }
                          route: { cluster: guinan_grpc, timeout: { seconds: 60 } }
                http_filters:
                  - name: envoy.ext_authz
                    typed_config:
                      "@type": type.googleapis.com/envoy.config.filter.http.ext_authz.v2.ExtAuthz
                      grpc_service:
                        envoy_grpc:
                          cluster_name: ext_authz
                        timeout: 0.5s
                      failure_mode_allow: true
                  - name: envoy.grpc_json_transcoder
                    typed_config:
                      "@type": type.googleapis.com/envoy.config.filter.http.transcoder.v2.GrpcJsonTranscoder
                      proto_descriptor: "pb/guinan.pb"
                      services: ["guinanpb.Guinan"]
                      convert_grpc_status: true
                      print_options:
                        add_whitespace: true
                        always_print_primitive_fields: true
                        always_print_enums_as_ints: false
                        preserve_proto_field_names: true
                  - name: envoy.router
                    typed_config: {}
  clusters:
    - name: guinan_grpc
      type: static
      connect_timeout: 0.25s
      http2_protocol_options: {}
      load_assignment:
        cluster_name: guinan_grpc
        endpoints:
          - lb_endpoints:
              - endpoint:
                  address:
                    socket_address:
                      address: 127.0.0.1
                      port_value: 40013
    - name: ext_authz
      type: static
      connect_timeout: 0.25s
      http2_protocol_options: {}
      load_assignment:
        cluster_name: ext_authz
        endpoints:
          - lb_endpoints:
              - endpoint:
                  address:
                    socket_address:
                      address: 127.0.0.1
                      port_value: 40080
    - name: samlauth
      type: static
      connect_timeout: 0.25s
      load_assignment:
        cluster_name: samlauth
        endpoints:
          - lb_endpoints:
              - endpoint:
                  address:
                    socket_address:
                      address: 127.0.0.1
                      port_value: 30080
@sgtsquiggs
Copy link
Author

I found that if I add a third route:

                        - match: {prefix: "/" }
                          route: { cluster: samlauth }

then everything works properly. Smells like a bug!

@mattklein123 mattklein123 added the investigate Potential bug that needs verification label Feb 5, 2020
@mattklein123
Copy link
Member

cc @dio

@yuval-k
Copy link
Contributor

yuval-k commented Feb 5, 2020

your problem is this line:

if (route == nullptr || route->routeEntry() == nullptr) {

because the grpc filter changes the path header and clears the route cache, route selection will be done after the ext auth filter;
so when the ext auth filter is called it will have no route and won't engage.
reversing the order of the filters will also help in your case.

@sgtsquiggs
Copy link
Author

If you reverse the order of the filters, it kind-of works. My ext_authz service returns an empty body, a location header, and a 307 status code. When I reverse the order and omit the third route (mentioned here), I get the correct location header, a 500 status code, and a non-empty body (grpc error as json)

@stale
Copy link

stale bot commented Mar 6, 2020

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Mar 6, 2020
@stale
Copy link

stale bot commented Mar 13, 2020

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted". Thank you for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
investigate Potential bug that needs verification stale stalebot believes this issue/PR has not been touched recently
Projects
None yet
Development

No branches or pull requests

3 participants