search

buzzfeed / sso

sso, aka S.S.Octopus, aka octoboi, is a single sign-on solution for securing internal services
MIT License
3.09k stars 186 forks source link

sso-proxy: reject request if any validation fails (fixes #125) #280

Closed tl-adrian-bridgett closed 4 years ago

tl-adrian-bridgett commented 4 years ago

Problem

Group validations were being ignored

Solution

Fail validation when any validation fails, ATM it looks to only fail when all fail.

Notes

I'm just a bit worried I'm missing something :-) (Not familiar with codebase)

tl-adrian-bridgett commented 4 years ago

ready for review

Jusshersmith commented 4 years ago

Hi @tl-adrian-bridgett!

Welcome, and thanks so much for contributing! This behaviour is expected -- we decided to move to this behaviour whereby requests would be allowed providing one of the defined validators succeed.

Over time, and as SSO matured this seemed a logical step forward and allowed for much easier flexibility, removing the requirement for some awkward workarounds that some people were having to adopt.

tl-adrian-bridgett commented 4 years ago

Hi, you're most welcome, thanks so much for this (essential!) project.

Accepting any validator seems to then mean that group-based validation is pointless/ignored. We have allowed-groups on the upstream-config, AUTHORIZE_EMAIL_DOMAINS set in the environment. Without this change, any user gets in - (it's not restricting to those in the groups).

Or is it that we shouldn't be setting AUTHORIZED_EMAIL_DOMAINS?

tl-adrian-bridgett commented 4 years ago

Done a bit more testing (I see there's a big note at the top of https://github.com/buzzfeed/sso/releases which I hadn't noticed!).

So by setting DEFAULT_ALLOWED_EMAIL_DOMAINS we've allowed that everywhere and the sso-upstream-configs.yaml isn't overriding it as I expected. Even setting allowed_ email_domains (as empty) there doesn't help.

So instead I'm setting DEFAULT_ALLOWED_EMAIL_DOMAINS to (blank) and then the group based auth works as I'm expecting.

Jusshersmith commented 4 years ago

Hey @tl-adrian-bridgett

I'm sorry for this confusing process. I think some of this stems from a bug which prevented the use of groups alone as a validator. I'll look at getting out a release in the next day or so which will include the fix, allowing you to use groups as the only form of validation within sso_proxy

As a side note, you can still use the email domain/address as a validator on the sso_auth side (which wouldn't be affected by any defined allowed groups in sso_proxy), however that is only effective if there isn't already a valid sso_auth session with that request.

tl-adrian-bridgett commented 4 years ago

No worries - we've got a workaround (thanks for your explanation - wouldn't have figured it out otherwise) so no need to hurry for us! Just saw quite a few others struggling. In any case looks like this PR should be close and not merged.