sso-auth: add all providers from oauth2_proxy

[eforbus]

This project looks promising and it would be a sure winner if you add support for all the providers currently supported by bitlys oauth2_proxy project https://github.com/bitly/oauth2_proxy. Especially because that project is really being maintained

[sporkmonger]

I'm beginning to work on an Azure AD provider.

[sporkmonger]

Things diverged somewhat between this project and https://github.com/bitly/oauth2_proxy and in particular I think verification logic needs to be refactored. (e.g. Azure AD doesn't have a verification endpoint, it expects implementors to actually do full OIDC, which neither project really does.)

[dekimsey]

@sporkmonger Which version of Azure's oidc are you working on? v1 or v2? I created a provider* for oauth2_proxy that does mostly azure's v2 oidc. I haven't looked into porting it to buzzfeed/sso, but I'd like to. I dunno if that's helpful to your work. v2 supports roles, which our applications need, the original implementation in oauth2_proxy was only v1.

Getting oauth2_proxy running with merging several different outstanding pull requests was messy. I'd very much like to switch to a tool being better maintained!

*: Implementation was a bit clunky, since oauth2_proxy didn't use discovery url's or oauth2's v2 libraries which seem to have better support for OpenID Connect flows. Also, groups were pulled from graph and not from the uudi's provided in the jwt.

[sporkmonger]

I need v2, I was starting w/ v1 as a basis. Sounds like what you've got would probably be quite helpful, thanks!

[sporkmonger]

Also I need the groups to be in the JWT for token-pass-through on #45 as well.

[kevinoconnor7]

I think #6 needs to be resolved first as well since config validation is currently responsible for creating the Google auth provider. A refactor to decouple this will be necessary to be able to cleanly add more auth providers.

(For reference I start looking into implementing a generic oidc provider last week).

[shrayolacrayon]

Hi @kevinoconnor7, thank you for looking into adding more providers, we appreciate your contributions! For some context, we have prioritized #6 as well as creating a dummy provider for integration tests, so that it will be simpler to add new providers to sso-auth and validate changes.

[sporkmonger]

Definitely agree that refactor should happen first. Since it's blocking the work I'm trying to do, I'll probably pick at least some of that up unless someone else has already started on it.

[luisdavim]

hi, any updates on this?

[emilaasa]

@sporkmonger Are you working on this now?
We need some of this functionality at work pretty soon and I am starting a rough implementation off of the case statement in options#newProvider. Would be happy to help in any way to make it good enough for upstream merge.

[sporkmonger]

Yes, I'm working on this actively. I have a partially working Azure AD provider in progress, but ran into some hiccups along the way that I'm still working through. I have a working flow for initial auth, but once the token expires for the first time it drops 500s ("missing refresh token") and 401s ("not authorized after refreshing the session"). Still debugging that. Presumably need to disabuse it of the notion that it's always going to have a refresh token. Azure AD only sends an ID token.

[sporkmonger]

I should add that the Azure AD provider I'm working on is derived from a generic OIDC provider, and both will get packaged together in the same PR.

[sporkmonger]

@emilaasa Which provider are you looking for?

[sstarcher]

I'm looking for SAML support so I can use Okta.

[emilaasa]

@sporkmonger Ah I was not very clear. I'm working on getting Azure v2.0 to work as expected - having some issues with the mandatory https redirect url and enabling ssl in the project.

Regarding your issue, the v2 endpoint only return refresh_token if you include "offline_access" in the scope of your first request. To refresh, POST the refresh_token instead of the code as per:
https://docs.microsoft.com/en-us/azure/active-directory/develop/v2-oauth2-auth-code-flow#refresh-the-access-token

(

[sporkmonger]

@emilaasa I'm aware of that, but see #95 for some concerns. There's room in the session for basically the ID token and not much else.

[luisdavim]

I'm looking for Azure AD support. It's awesome that yu're already working on it. Thanks @sporkmonger

[shrayolacrayon]

Thank you all for starting on implementing additional providers! I think the scope of this issue is broad and I'd like to link smaller per-provider issues to this issue.

@sporkmonger - I am going to create an issue for Azure AD and assign it to you if that sounds reasonable.

@sstarcher - we are interested in using Okta and will be working towards that this quarter. We'd be happy to keep you in the loop on progress with that.

If there are other providers that are being worked on feel free to open and reference this issue. @luisdavim @emilaasa

[bhack]

github?

[sporkmonger]

@emilaasa Yeah, if I enable offline_access, cookie lands at 3926 bytes, and apparently something else is setting some other cookie that takes it over 4096. That's even with cookie compression enabled.

[sporkmonger]

I'm currently debating maybe throwing away the ID token, but copying the relevant parts out after verification rather than trying to hold onto it. This potentially affects implementation details for #45, though I suspect that's more my problem than most other users.

[sporkmonger]

Oh, and the proxy cookie is similarly massive. The big danger w/ massive proxy cookies is that the application you're proxying to also needs to set cookies and some (most?) browsers enforce a maximum across all cookies, not just for a single cookie.

[luisdavim]

http://browsercookielimits.squawky.net for reference.

[luisdavim]

Can't the methods described here http://fgiasson.com/blog/index.php/2012/01/05/jquery-cookie-pluging-extended-with-html5-localstorage-and-chunked-cookies/ be used in this case to overcome the cookie size limitations?

[sporkmonger]

As long as the contents of the cookie are encrypted, it's probably not as much of a problem to use local storage, however unencrypted sensitive data in local storage is a serious security risk. The main hang-up is then that local storage and cookie semantics are quite different.

[sporkmonger]

http://browsercookielimits.squawky.net at least suggests that problems are likely to be limited provided you can be sure everyone's on Firefox or Chrome, which at least might be less of a concern in an internal corporate environment, though I don't think I'd discount it either.

[sporkmonger]

@shrayolacrayon What would your opinion be about maybe breaking it into multiple cookies? It seems like all the modern browsers allow ~4096ish bytes per cookie and don't enforce a total size of 4096 per domain. Splitting out encrypted access token and encrypted refresh token apart from the rest of the session data should resolve this pretty thoroughly without necessitating a persistent server-side state mechanism, something I'd really like to avoid.

[luisdavim]

Yeah, that's one of the approaches mentioned in the link I've shared before, "chucked cookies". The split you propose makes sense to me.

[shrayolacrayon]

@sporkmonger, this seems reasonable to me! I think ideally we'd want to have the whole session in one cookie with the option of separating them out into two cookies. Or having this be something that can be figured based on the provider. I'm going to think more about it, but I'd be interested in hearing any ideas you might have as well!

[emilaasa]

Poked around a bit and there seems to be some prior art, nginx and apache both uses chunked session cookies for what seems to be much the same reasons (big payloads from openidc providers).

@shrayolacrayon Implementing the logic in a way that does not chunk unless we're exceeding the limit should solve the problem without waste right? It would not chunk unless chunking was required and it will only be required when providers delivers large payloads.

[sporkmonger]

Chunking at a fixed size does seem reasonable to me, w/ a fatal error for N cookies, where N is probably 10 or something sane. We could take the approach that a zero value disables chunking for providers that don't need it.

[shrayolacrayon]

@emilaasa @sporkmonger I commented on the PR, but copying it here as well. I think chunking the session cookies seems fine to me and can be a separate SessionStore interface implementation, which can be configured to be used for providers that might need it.

I have a potential solution for supporting multiple cookies in a session to store the session state but it depends on #43 being completed. I have this prioritized to work on this week, so I'll try to push up a PR as soon as I can.
After both sso-proxy and sso-auth use a SessionStore for saving and loading sessions, we could create a new cookie store that would be a distributed sessions store that implements the SessionStore interface and can be configured to be used instead of CookieStore for providers that need it.

[mreiferson]

As @shrayolacrayon mentioned here this issue is too broad and we should open provider-specific requests going forward, so I'm gonna close this.

The Azure AD PR is in #118, let's move any pertinent discussion there.

Thanks ❤️