Authservice should implicitly deny requests that don't match config prefixes

[atoy3731]

Issue

Currently, a request to a workload that is authservice-enabled that does not match one of the prefixes in the authservice config will allow the request.

Recreation

1. Spin up authservice with ext_authz envoyfilter
2. Deploy Istio gateway/virtualservice for application that does not match chain in authservice config
3. Deploy workload matching envoyfilter selector using above virtualservice
4. Access the application using the VS domain/routes, traffic will be allowed even though it isn't specified in config.

Expected behavior

If a workload is authservice-enabled and it doesn't match a filter in the chain, it should be denied.

[incfly]

I am a bit confused on the problem statement. Could you elaborate a bit? Some sample configuration would help.

If a workload is authservice-enabled

This means envoyfilter created on the workload correct?

it doesn't match a filter in the chain

What does this mean? The virtual service/gateway contains the paths [a,b], but the client browser is trying to access /c and should be denied?

[atoy3731]

Using this example config:

{
  "listen_address": "0.0.0.0",
  "listen_port": "10003",
  "log_level": "trace",
  "default_oidc_config": {
    "authorization_uri": "REDACTED",
    "token_uri": "REDACTED",
    "jwks": "REDACTED",
    "client_id": "REDACTED",
    "client_secret": "REDACTED",
    "cookie_name_prefix": "REDACTED",
    "id_token": {
      "preamble": "Bearer",
      "header": "Authorization"
    },
    "access_token": {
      "header": "JWT"
    },
    "trusted_certificate_authority": "REDACTED",
    "logout": {
      "path": "/oauth/logout",
      "redirect_uri": "REDACTED"
    },
    "scopes": []
  },
  "threads": 8,
  "chains": [
    {
      "name": "test",
      "match": {
        "header": ":authority",
        "prefix": "test.example.com"
      },
      "filters": [
        {
          "oidc_override": {
            "redis_session_store_config": {
              "server_uri": "tcp://REDACTED:6379"
            },
            "callback_uri": "https://test.example.com/oauth/callback",
            "cookie_name_prefix": "test",
            "logout": {
              "redirect_uri": "https://REDACTED/logout"
            },
            "scopes": []
          }
        }
      ]
    }
  ]
}

Right now if a request comes into a sidecar for no-match.example.com, matches the envoyfilter and proxies to the authservice, because it does not match a prefix (the only one in this config being test.example.com), it will fail open and be allowed through.

It'd be ideal if a request is proxied to the authservice, and it does not match any chains, the request would be denied.

[incfly]

I see. It sounds like this use case want to ensure the application behind sidecar/gateway is always protected under OIDC. I think this is possible in updated config example. I can verify it a bit.

BTW, are you enabling authservice on application sidecar or the ingress gateway?

[atoy3731]

Currently we're doing this at the application sidecar, but plan to support both gateway and application implementations based off of the environment we're deploying to.

[tmitchell]

We get bitten by this routinely and I came here about to file a similar issue. A default-deny/fail-safe mode seems like a key feature for security relevant software.

[incfly]

I tested indeed if you configure "test.example.com" in the chain match, a not matched filter chain will make authservice chain allow request go through without oidc authenticaiton. It gives some log like below.

[2021-08-26 22:03:48.836] [console] [debug] Check: no matching filter chain for request to https://localhost:8443/static/bootstrap/js/bootstrap.min.js

For longer term, we won't expose this low level filter match logic as end user configuration. Moreover, as we are updating to use Istio 1.9 ext authz api, you can configure a proxy (sidecar or gateway), when to trigger the ext authz to the authservice. e.g. not trigger it if the path is "/public". but the authservice itself is always kicked off the OIDC flow.

I can update the documentation of how to selectively trigger ext authz to authservice with new API. Once we verified (need to compare the feature set a bit) selectively triggering condition are same, we can totally remove the match from the config option.

For the time being, if you remove the match filed at all, then all requets will be authenticated. will that work for you?

[Shikugawa]

@incfly I also think this feature If the request didn't match any chain, then it proceeds without any authentication is a bit confusing. Also, if we don't configure chain matcher, all of request will proceed without any authentication. In my consideration, we should DENY all requests without any header match, and optionally configure this behavior (But default ALLOW behavior should be used only debug environments from the perspective of security, AFAIK). So I think that to introduce the knob to switch ALLOW/DENY if no chain match. WDYT?

[incfly]

Add a field to control the behavior when the no match is found sounds good to me.

[bburky]

What are the security guarantees we are trying to provide with this? This change wouldn't quite let you say "any request that authservice allowed is trusted".

Currently Authservice allows requests through that already have an Authorization header present. So you can't assume that Authservice actually validated all credentials that flowed through it. The change in #183 would only lets you assert that all requests matched a filter, not that the JWT was validated against the JWKS.

https://github.com/istio-ecosystem/authservice/blob/39833fbfedb96b75fdb68154d8de4ab3415f941b/src/filters/oidc/oidc_filter.cc#L104-L113

[Shikugawa]

The change in #183 would only lets you assert that all requests matched a filter, not that the JWT was validated against the JWKS.

Yes. It makes sense. I also think authservice should provide verifying JWKs every time. It is worth doing in another PR. Thanks.

[incfly]

I tried with 0.5.1 image, which should include #183. However it does not have intended behavior: the request with unmatched :authority header still able to get through. when unmatched host is found, we returns earlier here.

However, I don't think we should go after fixing that. Instead I still think we should achieve this via Istio RequestAuthentication and AuthorizationPolicy.

People using this repo would always use Istio. Istio authn & authz policy has already supports such use case: we should configure a request authentication to validate the id token, and then in the authorization policy we can specify to.hosts, to require a requestPrincipals. For the unmatched ones, all requests will get 403 rbac denied.

Additionally istio authz allows much richer flexibility in terms of when to allow or deny requests (conditions, app label selector). Authservice in this case can be just viewed as a token acquisition component.

I have tested and here is some sample configuration for the bookinfo product page. I am using accounts.google.com as IdP. And access it via localhost:8443.

apiVersion: security.istio.io/v1beta1
kind: RequestAuthentication
metadata:
  name: "productpage-jwt"
spec:
  selector:
    matchLabels:
      app: productpage
  jwtRules:
  - issuer: "https://accounts.google.com"
    jwksUri: "https://www.googleapis.com/oauth2/v3/certs"
---
apiVersion: security.istio.io/v1beta1
kind: AuthorizationPolicy
metadata:
  name: "productpage-only-allow-authenticated-access"
spec:
  selector:
    matchLabels:
      app: productpage
  action: ALLOW
  rules:
  - to:
    - operation:
        hosts: ["localhost:8443"]
    from:
    - source:
        requestPrincipals: ["*"]

[incfly]

We talked with @atoy3731 and the reason is that while it's recommended and we should rely on istio API for such access control, this change is just to make less likely to shoot ourselves in the foot. e.g. someone less familiar with istio internals. having a double safety net is good.

That's why #223 is made.