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

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

Closed
atoy3731 opened this issue Apr 14, 2021 · 12 comments · Fixed by #223
Closed

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

atoy3731 opened this issue Apr 14, 2021 · 12 comments · Fixed by #223
Labels
enhancement New feature or request

Comments

@atoy3731
Copy link

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
Copy link

incfly commented Aug 13, 2021

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
Copy link
Author

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 incfly added the enhancement New feature or request label Aug 19, 2021
@incfly
Copy link

incfly commented Aug 19, 2021

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
Copy link
Author

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
Copy link

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
Copy link

incfly commented Aug 26, 2021

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
Copy link
Collaborator

@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
Copy link

incfly commented Oct 19, 2021

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

@bburky
Copy link

bburky commented Oct 26, 2021

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.

// If the id_token header already exists,
// then let request continue.
// (It is up to the downstream system to validate the header is valid.)
if (headers.contains(idp_config_.id_token().header())) {
spdlog::info(
"{}: ID Token header already present. Allowing request to proceed "
"without adding any additional headers.",
__func__);
return google::rpc::Code::OK;
}

@Shikugawa
Copy link
Collaborator

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
Copy link

incfly commented Jun 15, 2022

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
Copy link

incfly commented Jul 27, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants