No whitelist or domains configured -> allow all

[cyounkins]

vouch-proxy/handlers/handlers.go

Line 345 in a832330

ok = true

The above appears to allow all users when neither a whitelist or domains are configured. Given the purpose of Vouch, configurations like that are almost certainly unintentional. There is the allowAllUsers variable for that.

bnfinet if you agree this should change I'm happy to take a stab at it and send a pull request.

[bnfinet]

hmm, should it default open or default closed?

Although I do tend to agree with you I'm reluctant to change the behavior. I don't want to break existing setups.

There's general sense that the domain/whitelist/AllowAllUsers config options are confusing. I expect that it'll get rewritten for v1.0.0. Do you have any opinion on what that interface should look like?

In either case the current behavior should be documented, perhaps in the config.

[halkeye]

I personally don't care which state it is, but I agree, allowAllUsers exists to allow all users, so it does't make sense.

My vote is pick a state, make sure the config errors if both are true on startup, and bump the major version or something.

[cyounkins]

IMO Vouch should default to disallow all until properly configured with a whitelist OR allowAllUsers is set. This is a security product, and must be secure by default.

I don't want to break existing setups.

I understand. For the configurations where allowAllUsers is 'false' and no domains or whitelist are configured, the main question for me is whether administrators under that their configuration is equivalent to "allow all". If they do not realize this, "breaking" their setup would likely be a good thing.

In these configurations I think it is likely the admin does not realize Vouch is effectively in "allow all". Vouch pushes the client through the authentication flow, and it is reasonable to assume during testing that this authentication "did" something. People are much less likely to test authenticating with a disallowed user.

Do you have any opinion on what that interface should look like?

I agree that the configuration parameters are not clear. To be honest I still don't understand domains and what that is really used for. I wrote an oauth proxy like this for a previous employer, so I'm adding on my TODO list a task to review the whole app. At the same time I will get a much better understanding of the configuration parameters and will be able to make a proposal at that point. If I don't put something here or tag this issue, feel free to ping me.

[vulcan25]

IMO Vouch should default to disallow all until properly configured with a whitelist OR allowAllUsers is set. This is a security product, and must be secure by default.

I'll second this. The default fallback behavour you raised in this issue really makes me uneasy. Leave out some configuration options, and it defaults to wide open?! I would expect this to be fixed with urgency, even if it breaks existing deployments.

Similarly I was quite surprised that the default oauth scope was User for the github provider (Not sure about other providers). This means whoever administrates vouch is getting write access to my profile by default. It can be disabled by adding scopes as an empty list to the oauth block:

oauth:
  scopes:
   - ''

However this felt like a hack to achieve something which I feel should be default behaviour. If I'm protecting some of my endpoints with vouch, I'd expect to make the concious decision to enable a level of oauth which gives me any write access to my user's profiles.

[bnfinet]

thanks @vulcan25

Could you open a separate issue for the github scopes issue? I'm happy to work with you on exploring better default scopes.

[vulcan25]

@bnfinet Seems I spoke to soon regarding scopes; Looks like you changed this to read:user as per #63

[bnfinet]

@mmorsky suggested in #106 an update to the Debug message from this block

[bnfinet]

There has been a clear log.warn in for 9 months...

log.Warn("verifyUser: no domains, whitelist, teamWhitelist or AllowAllUsers configured, any successful auth to the IdP authorizes access")

https://github.com/vouch/vouch-proxy/blob/master/handlers/auth.go#L166

I feel like that's a good compromise. closing