sso-auth: Azure AD provider

[sporkmonger]

This pull request implements support for Azure AD as an identity provider. It supports groups passed to the upstream via the X-Forwarded-Groups header.

Notes

Requires "Read All Groups" / "Read Directory Data" app-level permissions, as well as "Sign in and read user profile" delegated permissions. The former requires either full Azure admin role or Cloud app admin role in order to approve. The following environment variables must be set:

PROVIDER: 'azure_v2'
AZURE_TENANT: '<your-tenant-uuid-here>'
APPROVAL_PROMPT: 'consent'

I haven't fiddled with timeouts as part of this PR, but they probably need to be extended because first-time sign-in can take much longer due to requiring a large number of API calls. These are done concurrently, but even so, there's a significant risk of timeout waiting on the API calls to finish on the first go-around.

This PR also adds the OIDC provider, though I don't personally have a good generic OIDC provider to actually QA it against.

Fixes #98
See also #27

[katzdm]

Hey Bob - Thanks for this sizable contribution! We really appreciate the work.

This PR also adds the OIDC provider, though I don't personally have a good generic OIDC provider to actually QA it against.

I've only taken a brief look so far, but if the OIDC support is orthogonal to using Azure AD as an identity provider, then we might think about splitting OIDC into a separate PR - This PR is already fairly large, and splitting off the OIDC work will make things easier to digest and review.

Happy to discuss - Thoughts?

[sporkmonger]

It's not orthogonal. The Azure AD provider uses OIDC under the hood, but it additionally introduces groups support that's not part of OIDC.

[katzdm]

Mostly stylistic and comment nitpicks - Looking good!

[sylr]

Just leaving a message of encouragement because I'm really looking forward this 👍

[sylr]

@sporkmonger If you move forward with @katzdm reviews I'll will be happy to test it in my organization.

Cheers.

[sporkmonger]

Hey, sorry about delay on this. Was fully occupied w/ KubeCon stuff for a bit. I expect I should be able to tackle the review comments within the next week or so.

[katzdm]

Hey, sorry about delay on this. Was fully occupied w/ KubeCon stuff for a bit. I expect I should be able to tackle the review comments within the next week or so.

No problem - Thanks for pushing forward with this; we're really excited to merge this in! 🎉I'll take another look at this in the next few days.

[sporkmonger]

Alrighty, good news, I finished up the stuff on my plate that was blocking me from getting back to this. Starting in on the requested changes now. Sorry it's been taking so long.

[sporkmonger]

I've addressed all the minor issues. There's a small number of outstanding issues remaining that I expect I should be able to address in the next few days.

[sylr]

Wonderful!

[sporkmonger]

All changes have been made, this should be ready for re-review @katzdm.

[eyalzek]

@sporkmonger although you implemented the OIDC provider, there's still no way to pass it as a valid provider right? When setting PROVIDER=oidc I got: "error":"unimplemented provider: \"oidc\"" and looking at https://github.com/buzzfeed/sso/blob/e67f1a6c633fb6a3aabed3debda346c83a8fe0ca/internal/auth/options.go seems like it really isn't an option ATM.

[sporkmonger]

@eyalzek Yeah, that's true, it's currently closer to a functional stub than anything. Part of the problem is that Azure AD doesn't quite follow the OIDC spec to the letter and that's what I've got to test with. I was kinda hoping someone with a more compliant OIDC provider available to them might be able to take a look and get that part over the finish line, so that's why I didn't fully enable it.

[sporkmonger]

I could certainly turn it on if you'd be willing to test it and work with me on making sure it works in practice, not just in the test suite.

[sporkmonger]

My expectation would be that groups are unlikely to work for most OIDC providers that don't have provider-specific implementations though (like this Azure AD PR) since groups have to be passed through in the token and many providers do that in very implementation-specific ways (e.g. Azure AD only does it if certain flags and scopes are set and even then only returns group UUIDs, not group names).

[sporkmonger]

After all the changes, looks like it's running into cookie size issues again.

[sporkmonger]

I'm going to wait for #137 to land before attempting to resolve.

[eyalzek]

I'll be glad to help with testing. We're currently using dex in front of gitlab as the OIDC provider, but after looking at sso for a bit I'm wondering if dex is even necessary or can it be replaced with the sso-auth component.

[sporkmonger]

I'm working right now on trying to close gaps between sso-auth and dex. There's still some things dex does that sso-auth doesn't, but really depends on your use-case.

[sporkmonger]

Just an FYI, to simplify testing you can look at the "production" branch in our fork: https://github.com/Remitly/sso/tree/production

That contains a merged copy of all our outstanding PRs in one place. Since this PR doesn't really work terribly well without #150, it may be easier to test from the merged branch instead.

[sylr]

A radiator is a PC connected to a Big flat screen which displays dashboards.

[katzdm]

Mind if I ask for a status update on this change? AFAICT, the main blocker for here is the Cookie Overflow fix - Is that an accurate reading?

[sporkmonger]

Yes, I think that's accurate.

[eyalzek]

I tried testing this with gitlab as an oidc provider. I took this branch and rebased it on the production branch (to include both the cookie fix and the oidc provider code). sso-auth component fails to start with the following errors:

file not found
{"error":"Get /.well-known/openid-configuration: unsupported protocol scheme \"\"","level":"error","msg":"","service":"sso-authenticator","time":"2019-03-01 12:34:46.3112"}
{"error":"Get /.well-known/openid-configuration: unsupported protocol scheme \"\"","level":"error","msg":"error creating new Authenticator","service":"sso-authenticator","time":"2019-03-01 12:34:46.3112"}

REDIRECT_URL is set to https://sso-auth.FOO.BAR (FOO.BAR is just a redaction, the url is valid.), so I'm not really sure where it's getting \ as a protocol scheme. Couldn't find hints in the code... Any ideas?

[adamjacobmuller]

I have this mostly working but the final stage where sso-proxy calls back to sso-auth to redeem the token with sso-auth causes a panic inside sso-auth

2019/03/04 00:40:24 http: panic serving 127.0.0.1:48266: runtime error: invalid memory address or nil pointer dereference
goroutine 58 [running]:
net/http.(*conn).serve.func1(0xc00006fae0)
	/usr/local/Cellar/go/1.11.5/libexec/src/net/http/server.go:1746 +0xd0
panic(0x8a74c0, 0xcee5b0)
	/usr/local/Cellar/go/1.11.5/libexec/src/runtime/panic.go:513 +0x1b9
net/http.(*timeoutHandler).ServeHTTP(0xc000055000, 0x9b9ae0, 0xc000240a40, 0xc0000ed200)
	/usr/local/Cellar/go/1.11.5/libexec/src/net/http/server.go:3160 +0x84a
github.com/buzzfeed/sso/internal/auth.loggingHandler.ServeHTTP(0x9b6960, 0xc00000c020, 0xc000122000, 0x9b6880, 0xc000055000, 0x1, 0x9b9ea0, 0xc0002c89a0, 0xc0000ed200)
	/Users/adam/Scripts/go/src/github.com/buzzfeed/sso/internal/auth/logging_handler.go:101 +0x147
net/http.serverHandler.ServeHTTP(0xc000065ba0, 0x9b9ea0, 0xc0002c89a0, 0xc0000ed200)
	/usr/local/Cellar/go/1.11.5/libexec/src/net/http/server.go:2741 +0xab
net/http.(*conn).serve(0xc00006fae0, 0x9ba2e0, 0xc000240840)
	/usr/local/Cellar/go/1.11.5/libexec/src/net/http/server.go:1847 +0x646
created by net/http.(*Server).Serve
	/usr/local/Cellar/go/1.11.5/libexec/src/net/http/server.go:2851 +0x2f5

I am running the latest version of https://github.com/Remitly/sso/tree/production

[adamjacobmuller]

I have not been able to identify why, but, I rolled my own version which combined azure-ad-provider, compress-cookie and cookie-overflow-fix and it resolved my issue.

Posting here if anyone wants to compare.

I have another issue which I'm not sure how to solve. It seems that the SSO reply includes my primary proxyAddress, while the graph call requires the userPrincipalName and thus the graph call fails. I commented out that bit of code to make things work for now.

[sporkmonger]

@eyalzek Ah, sorry, production is what we're running internally and we introduced some other unrelated changes that you definitely don't want. Forgot I'd mentioned the branch over here.

[sporkmonger]

@adamjacobmuller Could you post a redacted version of the API responses you're getting?

[adamjacobmuller]

@sporkmonger

{"error":"could not get groups: api error: {\r\n  \"error\": {\r\n    \"code\": \"Request_ResourceNotFound\",\r\n    \"message\": \"Resource '[email protected]' does not exist or one of its queried reference-property objects are not present.\",\r\n    \"innerError\": {\r\n      \"request-id\": \"7ba87cce-741f-45f6-8a5b-7eb7ea56f342\",\r\n      \"date\": \"2019-03-16T18:32:15\"\r\n    }\r\n  }\r\n}"

[adamjacobmuller]

using the Graph Explorer, querying with my userPrinicpalName works fine, querying with my primary email fails with the same error.

[adamjacobmuller]

the id token response contains preferred_username which works.

patch is very trivial but I can make a PR if you like

[sylr]

Where are we about this ? I've been using it for a while now, it would be nice to see it merged.

[sporkmonger]

Cookie overflow issue is what this is gated on.

[kevinoconnor7]

@sporkmonger Would it be possible to maybe split this PR? I believe only Azure has the cookie overflow issue (and not other OIDC providers).

If we could separately get the general-OIDC provider in first that would likely help building out other providers.

[sporkmonger]

I'm in favor of that course of action, but don't currently have the time to dedicate to it splitting it out either.

In practice I believe Azure AD would benefit primarily from just moving to a Redis cookie store rather than dealing with cookie splitting. Frankly I think that is probably a better solution in general because even for identity providers with sane token sizes, there's not necessarily much of a limit to how many groups you can belong to, other than convention. So all session state is technically unbounded in size, and Azure AD just hits the limits when others don't because MS loves their cruft.

I can say that I've been doing cookie splitting in production for a couple months now and it has been nothing but headaches and I absolutely do not recommend it unless you have no choice.

Unfortunately, I've got a giant pile of other things on my plate this quarter and landing these PRs is super time-consuming.

[brianv0]

I have an implementation of a Redis cookie store I've implemented in a fork of pusher/oauth2_proxy that can probably be cherry picked, I've been using it with pretty good success, but I haven't finished cleaning up my branch and writing some unit tests for it. I'm hoping to get to that this week. I'm not sure that helps you out all that much, but I will report back when I can push my changes.

[sylr]

@brianv0 That would be brilliant! See #158.

[sporkmonger]

I haven't forgotten about this PR. I'm hoping to have some time to dedicate to this and #158 sometime this quarter.

[sporkmonger]

I'm actively working on #158 in support of this PR. We've been running #150 in production for awhile now and it's an endless source of difficult to diagnose problems and frustration.

[sylr]

I tried to merged this and #224 but when I'm trying to test it I get:

{"error":"3 error(s) decoding:\n\n* 'client[id]' expected a map, got 'string'\n* 'client[secret]' expected a map, got 'string'\n* 'provider' expected a map, got 'string'","level":"error","msg":"error loading in config from env vars","service":"sso-authenticator","time":"2019-07-23 11:35:22.810"}

Do you have a list of env vars needed to be set to make this work ?

[sporkmonger]

This should get you in the right ballpark.

sso-auth:

SERVER_SCHEME: 'https'
SERVER_HOST: $hostname
SESSION_KEY: $session_secret
SESSION_COOKIE_SECRET: $cookie_secret
SESSION_COOKIE_SECURE: true
SESSION_COOKIE_HTTPONLY: true
SESSION_REDIS_CONNECTION: 'redis://sso-sessions.xxxxxx.xx.0001.usw2.cache.amazonaws.com:6379'
PROVIDER_AZURE_TYPE: 'azure'
PROVIDER_AZURE_SLUG:: 'azure'
PROVIDER_AZURE_CLIENT_ID: $idp_client_id
PROVIDER_AZURE_CLIENT_SECRET: $idp_client_secret
PROVIDER_AZURE_AZURE_TENANT: '00000000-0000-0000000-00000000'
PROVIDER_AZURE_AZURE_PROMPT: 'consent',
AUTHORIZE_EMAIL_DOMAINS: 'example.com'
AUTHORIZE_PROXY_DOMAINS: 'int.example.com'
CLIENT_PROXY_ID: $sso_client_id
CLIENT_PROXY_SECRET: $sso_client_secret
METRICS_STATSD_HOST: '127.0.0.1'
METRICS_STATSD_PORT: '8125'

sso-proxy:

DEFAULT_PROVIDER_SLUG: 'azure'
EMAIL_DOMAIN: 'example.com'
UPSTREAM_CONFIGS: $upstream_configs
PROVIDER_URL: "https://${auth_hostname}/"
COOKIE_SECURE: true
COOKIE_HTTP_ONLY: false
REDIS_CONNECTION_URL: 'redis://sso-sessions.xxxxxx.xx.0001.usw2.cache.amazonaws.com:6379',
STATSD_HOST: '127.0.0.1'
STATSD_PORT: '8125'
VIRTUAL_HOST: '*.int.example.com,*.k8s.example.com,*.dev.example.com'

[sporkmonger]

We discovered a subtle bug in the implementation around nested groups but haven't tracked it down yet. Currently it's possible to list overlapping groups (i.e. both a parent and a child group) which should be granted access, and instead of the expected outcome, access is denied when the child group is listed, but granted when it is removed if the user belongs to a child group not on the list that is also a member of the parent group.

[dmitryzykov]

Hello. Any updates? Thank you.

[chris-scentregroup]

How can we work towards getting this merged? Let me know how I can help..

[sporkmonger]

This PR is effectively abandoned. We're no longer using Azure AD and we're unable to dedicate any resources to this PR. If anyone else wants to pick it up, by all means.