istio-ecosystem / authservice

Move OIDC token acquisition out of your app code and into the Istio mesh
Apache License 2.0
217 stars 63 forks source link

multiple idp support #51

Closed nickrmc83 closed 4 years ago

nickrmc83 commented 4 years ago

This PR adds the ability to support multiple IdPs based on a matching criteria for a request. A request is compared to a number of criteria and when a match is identified it is processed by a configured filter chain. A filter chain is a list of filters that sequentially processes a request.

nickrmc83 commented 4 years ago

@cfryanr @tylerschultz @peterhaochen47 this PR adds the capability to support multiple IdPs by introducing a request matching criteria. I'll do some more testing on it but please feel free to start reviewing the changes.

istio-testing commented 4 years ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nickrmc83 To complete the pull request process, please assign brenodemedeiros You can assign the PR to them by writing /assign @brenodemedeiros in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/istio-ecosystem/authservice/blob/master/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
cfryanr commented 4 years ago

Hi @nickrmc83,

If we're reading the code correctly, it seems like with this change the user must configure a matcher for each chain, which means they need to know the name of a header which they care to match against. Previously an OIDC filter would be used for all requests, regardless of headers. To keep that behavior for users who wish to have a simple configuration and/or do not have any special headers to read, it would be nice if the user could declare in a simple way that their filter chain should match all requests.

One simple way to implement this would be to make the Match object optional in the FilterChain definition in the proto file. Then, in FilterChainImpl::Matches(), always return true when the Match object was not configured for the chain.

This would allow the following configuration to mean that the OIDC filter shown below should be used on every request (note that it does not declare a matches section):

{
  "listen_address": "127.0.0.1",
  "listen_port": "10003",
  "log_level": "trace",
  "chains": [
    {
      "name": "my-chain",
      "filters": [
        {
          "oidc": {
             ...(the usual OIDC config goes here)
          }
        }
      ]
    }
  ]
}

Alternatively, maybe the user always declares a matches section, but we provide some nice way within the matches section to declare that all requests should match without needing to give a header name or an equality check.

What do you think?

Additionally, It would be nice if the new config options were documented in the README and the simplest version (where the chain always matches) demonstrated in the sample config file.

nickrmc83 commented 4 years ago

@c

nickrmc83 commented 4 years ago

Hi @nickrmc83,

If we're reading the code correctly, it seems like with this change the user must configure a matcher for each chain, which means they need to know the name of a header which they care to match against. Previously an OIDC filter would be used for all requests, regardless of headers. To keep that behavior for users who wish to have a simple configuration and/or do not have any special headers to read, it would be nice if the user could declare in a simple way that their filter chain should match all requests.

One simple way to implement this would be to make the Match object optional in the FilterChain definition in the proto file. Then, in FilterChainImpl::Matches(), always return true when the Match object was not configured for the chain.

This would allow the following configuration to mean that the OIDC filter shown below should be used on every request (note that it does not declare a matches section):

{
  "listen_address": "127.0.0.1",
  "listen_port": "10003",
  "log_level": "trace",
  "chains": [
    {
      "name": "my-chain",
      "filters": [
        {
          "oidc": {
             ...(the usual OIDC config goes here)
          }
        }
      ]
    }
  ]
}

Alternatively, maybe the user always declares a matches section, but we provide some nice way within the matches section to declare that all requests should match without needing to give a header name or an equality check.

What do you think?

Additionally, It would be nice if the new config options were documented in the README and the simplest version (where the chain always matches) demonstrated in the sample config file.

@cfryanr I've made the match message field optional. If no match is specified a filter chain will always match a request and therefore the operator does need to specify it if they only have one IdP to support or they wanted to specify a default IdP as part of the last chain in the list. I've updated the template to include the 1 IdP case as well as adding a unit test to cover this.

A more significant change is I've added the generation of configuration docs using a tool. Now all configuration documentation should be included in the proto files and a make all will update the README in ./docs.

cfryanr commented 4 years ago

/lgtm