Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relax the use of Email Domains/Addresses to enforce authentication #236

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jphines
Copy link
Contributor

@jphines jphines commented Aug 1, 2019

Problem

We currently mandate the use of a global email domain to authenticate both on the proxy side as well as the authenticator side. This is limiting for organizations and upstreams that have more diverse requirements that require a more flexible authenticator model.

We originally implemented this requirement as a safety precaution when SSO was less mature. It no longer provides the same assurances now that group usage is more robust and SSO itself has matured.

Solution

We propose to move adjust this configuration in two ways:

  1. Move the configuration in the proxy to the upstream configuration block.
  2. Remove the configuration and mechanism on the authenticator side. Instead, we will rely on the identity providers to provide this authentication mechanism.

Notes

The way email domains, addresses, and groups interact with one another is becoming increasing confusing. We should think about ways to help simplify the model and make it more intuitive.

@jphines jphines added the enhancement New feature or request label Aug 1, 2019
@jphines jphines self-assigned this Aug 1, 2019
@codecov
Copy link

codecov bot commented Aug 1, 2019

Codecov Report

Merging #236 into master will decrease coverage by 0.13%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master     #236      +/-   ##
==========================================
- Coverage   62.25%   62.11%   -0.14%     
==========================================
  Files          50       50              
  Lines        4069     4036      -33     
==========================================
- Hits         2533     2507      -26     
- Misses       1349     1350       +1     
+ Partials      187      179       -8
Impacted Files Coverage Δ
internal/auth/options.go 77.5% <ø> (-1.08%) ⬇️
internal/auth/authenticator.go 86.04% <ø> (-0.35%) ⬇️
internal/auth/mux.go 78% <ø> (+3%) ⬆️
internal/proxy/options.go 83.33% <ø> (-0.27%) ⬇️
internal/auth/configuration.go 49.71% <ø> (-0.02%) ⬇️
internal/proxy/proxy.go 21.95% <0%> (-2.44%) ⬇️
internal/proxy/proxy_config.go 78.37% <100%> (+0.23%) ⬆️
internal/proxy/oauthproxy.go 50.73% <0%> (-0.25%) ⬇️

@Jusshersmith
Copy link
Contributor

Jusshersmith commented Aug 6, 2019

This is largely just thinking out loud at the moment (I have no strong feelings), but is it worth allowing these configs to have a default set for them as well for where upstream specific settings are not required, but the restriction is still wanted. (I suppose here:

defaultUpstreamOptionsConfig := &OptionsConfig{
).

I'm not entirely sure if that would be useful to many, or if it's just further solidifying something that isn't all that useful anymore. (especially thinking of your comments in the 'Notes' section about simplifying this stuff)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants