Improve support for AAD Auth proxying w/ group authz support

[thenewwazoo]

This PR improves support for backing oauth2_proxy with Azure Active Directory, along with a few other changes. We've been using this internally for lightly-loaded services for a couple of weeks, and are ready to open contribute it back upstream. Thanks for the great base upon which to build!

BREAKING CHANGES:

• In order to generalize group support, the Google google-groups field was renamed to permit-groups.
• Raw cookie value construction was changed to delineate fields using the : character instead of the | character in order to adopt conventional construction of the X-Forwarded-Groups header (usually foo|bar|baz).
• The provider option is now mandatory.

Added functionality:

• Azure support now includes group support, including passing an X-Forwarded-Groups header, filtering groups (in order to limit cookie size), and restricting access based on group string matches.
• Added a Vagrantfile and simple HTTP request reflector for testing

[Esfera5]

ETA?

[thenewwazoo]

Sorry, I don't follow. We've been using this internally for a bit now. Does it need something to be ready for merge?

[bdwyertech]

FYI, I went live with this and it turns out that having prompt=consent as a query param for initial authorization does not work for non-admin users. I had to revert back to the mainline version here without the prompt query param. I'm not sure if its flaggable (it should be), but its easy enough to append &prompt=admin_consent to the url by hand for initial application provisioning, granting consent for all users.

[thenewwazoo]

@bdwyertech, thanks for the report. I'm unfortunately no longer in a position to test this, but did you have any success with different values for prompt? Thinking about it, it is probably advisable to remove it altogether, since I don't believe it's necessary to require the consent prompt every time.

I'm surprised to hear you had that result. I don't believe that everyone was an admin in my former team. Hopefully it's a simple fix. Do let @pasoroki know if you're able to do any testing, as he may be interested in updating this PR. :)

[pasoroki]

I will definitely update the PR.
We have a ticket for one person who didn't get 2FA request when he tried to authorize. That might be related. I'll try first to remove prompt at all from the request

[pasoroki]

This PR is updated:

• merged latest changes from the origin
• added empty prompt option by default
• fixed issue with HTTP 500 error if state option were not set
• we had extra testing files (vagrant and configs). Removed that to make it clean

The test.sh throw lots of errors, they are unrelated to our changes.

[ploxiln]

test.sh is currently stopping due "go fmt" not liking this line which you changed

[ploxiln]

The test panic is:

--- FAIL: TestAuthOnlyEndpointSetXAuthRequestHeaders (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
...
github.com/bitly/oauth2_proxy.NewOAuthProxy(0xc4201f9800, 0xc42021e070, 0xc42021e060)
	/home/travis/gopath/src/github.com/bitly/oauth2_proxy/oauthproxy.go:163 +0x590
github.com/bitly/oauth2_proxy.TestAuthOnlyEndpointSetXAuthRequestHeaders(0xc4201f66c0)
	/home/travis/gopath/src/github.com/bitly/oauth2_proxy/oauthproxy_test.go:628 +0x1fe

I think I that's on this line:

log.Printf("OAuthProxy configured for %s Client ID: %s", opts.provider.Data().ProviderName, opts.ClientID)

so opts.provider.Data().ProviderName is probably de-referencing a nil somewhere.

You are making a significant change to how providers work in this PR: making the provider no longer default to "google" but instead return nil (and an error) if no recognized provider is specified. So it is quite plausible that this provider-option-handling change is related to this test failure.

Command-line common-use-case compatibility-breaking changes should not be bundled with other changes like group handling enhancements, and is probably enough to prevent this from being merged. On the other hand, while this project/repo is not "dead", it is not clear what might be merged next, or in how many weeks or months. It looks like any of the changes you want to get in might be a bit too big to get into the next round, not really because they're "bad", but just because review capacity of the maintainers is low. (I'm not a maintainer.)

If this PR is just useful as a common resource for multiple users of this variant/functionality, that's quite fine, you can ignore me :)

[pasoroki]

Sorry @kerma I don't work on that

[kerma]

@pasoroki would you accept a PR with that functionality ?

[pasoroki]

@kerma sure, just don't forget to cover code with tests

[pasoroki]

@kerma sure, just don't forget to cover code with tests

[jcrowthe]

When might this be merged? Groups support is really the only path forward when it comes to Azure provider support for companies larger than ~10 individuals.

[lukasmrtvy]

Merge? We really need this feature. Thanks

[mkoch1]

This looks like exactly what we need - any idea when it can get merged (and released)?

[dekimsey]

I've played with this PR and haven't been able to get Azure Roles or Groups passed through. Does anyone have docs on the Azure set-up side of things?

[lukasmrtvy]

@dekimsey you will have to use v2.0 api endpoint and scope: profile I guess

[dekimsey]

@lukasmrtvy Thanks, that made me look into this deeper. I haven't been able to see/get roles but I did get groups working. It was a number of things some PEBKAC others permissions issues with the Graph API (user_list_memberof)[https://developer.microsoft.com/en-us/graph/docs/api-reference/v1.0/api/user_list_memberof].

WRT to the PR itself, I did find that a large group list will cause it to go into loop hitting the azure cookie /oauth2/callback repeatedly as the Cookie maximum size is exceeded and unable to save in the browser. To whom whoever decides to merge this or take this over, might be worth catching that situation rather than infinite looping. Also, the FilterGroups parameter cannot be specified multiple times which makes filtering tricky.

[mfyll]

This looks like exactly what we need - any idea when it can get merged?

[lukasmrtvy]

@mariusfylling this is dead project. Keycloak and Keycloak Gatekeeper can do the magic for You.
Keycloak Gatekeeper itself can replace this oauth2 proxy, but there is a small problem: https://issues.jboss.org/browse/KEYCLOAK-8728

[mfyll]

@mariusfylling this is dead project. Keycloak and Keycloak Gatekeeper can do the magic for You.
Keycloak Gatekeeper itself can replace this oauth2 proxy, but there is a small problem: https://issues.jboss.org/browse/KEYCLOAK-8728

Thank you, will look into this.

[martin-loetzsch]

Fyi: there is an active discussion about forking this project here: #628