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]

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]

@ploxiln you were right. We removed default provider and were defining provider for each option set in every test. New tests didn't have that. I removed back Google as a default provider

[ploxiln]

fwiw, I don't think the more strict provider option is a bad idea, just unrelated :)

(I would still have it default to google, but right now it accepts any unrecognized string as google, so that could be cleaned up - in a separate PR)

[johnarnold]

@talem @jediah This is really useful, any issues blocking merge?

[johnarnold]

"In order to generalize group support, the Google google-groups field was renamed to permit-groups."

consider leaving the google-groups param along with permit-groups,for backwards compatibility, and set permit-groups = google-groups if permit-groups is none, and google-groups is not none.

[iksaif]

Agreed with @johnarnold, having this merged would be great.

[thefatalist]

I'll update PR next week, but don't think anyone would merge it =(

[iksaif]

why is that ? @jehiah tagged the associated issue (#335) as enhancement, so I don't think authors would be against this.

[thefatalist]

Well, I hope you are right @iksaif

[combor]

The -pass-groups=true parameter will only work if -pass-access-token is set. The reason is that groups are saved in the session cookie which is created inside EncryptedString method that requires c *cookie.Cipher. This is only set when: https://github.com/bitly/oauth2_proxy/blob/731fa9f8e00b294ff1bf4a687e63d2ecdd9a4a50/oauthproxy.go#L166

I think this needs some explanation in docs or change of the condition so if pass-groups is set than the pass-access-token is a required flag.

[pasoroki]

@combor PassAccessToken is setup by default https://github.com/bitly/oauth2_proxy/blob/master/options.go#L111 I marked in README that its default value is false

[pasoroki]

Thank you @jan-schulz-k24 that is very helpful Now I need to figure why golang 1.9 is timing out

[pasoroki]

Hey guys, any updates so far? Anything else I can do?

[nduijvelshoff]

Hejj all, what us the current status of this? Can't wait to use it. 👍

[combor]

We've been using it for couple of months now from our fork and it works ok

[nduijvelshoff]

@combor When I clown the fork, the build fails on my opensuse server...

[nduijvelshoff]

Clone*

[pingles]

@nduijvelshoff could you give us the errors you're getting/build output and we'll see if we can debug the problem

[combor]

@nduijvelshoff You can also use this container: quay.io/uswitch/oauth2_proxy

[nduijvelshoff]

@combor This won't suite for me since I'm reusing the Kibana docker image. @pingles Hereby my error: ./oauthproxy.go:569: session.IDToken undefined (type *providers.SessionState has no field or method IDToken) ./oauthproxy.go:570: session.IDToken undefined (type *providers.SessionState has no field or method IDToken) ./oauthproxy.go:571: p.provider.GetGroups undefined (type providers.Provider has no field or method GetGroups) ./oauthproxy.go:576: session.Groups undefined (type *providers.SessionState has no field or method Groups) ./oauthproxy.go:585: cannot use session (type *providers.SessionState) as type string in argument to p.provider.ValidateGroup ./oauthproxy.go:702: session.Groups undefined (type *providers.SessionState has no field or method Groups) ./oauthproxy.go:703: session.Groups undefined (type *providers.SessionState has no field or method Groups) ./options.go:282: assignment count mismatch: 2 = 1 ./options.go:291: p.SetGroupRestriction undefined (type *providers.AzureProvider has no field or method SetGroupRestriction)

[pingles]

Wow, super strange. I've take a clean clone on a new laptop and can build with:

$ go get -v -t ./...
$ CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -o bin/proxy

I'd say maybe its a dependency versioning issue but then the options go assignment error suggests something else is wrong.

[nduijvelshoff]

@pingles going to check it out tonight.... can you share the compiled bin with me? 👼

[combor]

You can find it in our container.

[nduijvelshoff]

@pingles are you sure we are talking about the same repo? https://github.com/outlook/oauth2_proxy.git Tested it on Suse and Ubuntu (Go 1.10).