sso-auth: decouple setting provider with options validation

[loganmeetsworld]

What

Move out initializing providers from validating Options into to a separate function that can be taken in as an option function.

Why

Currently we set the provider as the google provider during validation of the Options struct which makes it difficult to add a new provider and to add new features to existing providers. Decoupling the logic from validating Options will make it simpler to support adding new providers.

[sporkmonger]

I've started working on this.

[shrayolacrayon]

Sounds great, what is the approach you are planning to take in refactoring and decoupling the logic?

[sporkmonger]

I think I'll probably start by flagging sections of code that I think will be affected and linking them here for discussion.

[sporkmonger]

In some cases providers will send only an ID token, not access/refresh tokens. The session state struct should be expanded to include an ID token, and we should be more cautious about assuming an access token will always be present.

sso/internal/pkg/sessions/session_state.go

Lines 19 to 31 in a946076

	type SessionState struct {
	AccessToken string `json:"access_token"`
	RefreshToken string `json:"refresh_token"`
	RefreshDeadline time.Time `json:"refresh_deadline"`
	LifetimeDeadline time.Time `json:"lifetime_deadline"`
	ValidDeadline time.Time `json:"valid_deadline"`
	GracePeriodStart time.Time `json:"grace_period_start"`
	Email string `json:"email"`
	User string `json:"user"`
	Groups []string `json:"groups"`
	}

Not having one probably shouldn't be an error if you have an ID token instead. In addition, as mentioned in #27, not all providers validate via an HTTP endpoint. Some providers use OIDC well-known locations to distribute keys which allows tokens to be validated without making extra round-trips.

I'd probably recommend renaming this function as the method tokens are validated by is potentially provider-specific. I think there's going to be two fairly common validation strategies (HTTP validation endpoint & JWT w/ well-known key location).

sso/internal/auth/authenticator.go

Lines 827 to 856 in a946076

	// ValidateToken validates the X-Access-Token from the header and returns an error response
	// if it's invalid
	func (p *Authenticator) ValidateToken(rw http.ResponseWriter, req *http.Request) {
	accessToken := req.Header.Get("X-Access-Token")
	tags := []string{
	"action:validate",
	}
	if accessToken == "" {
	tags = append(tags, "error:empty_access_token")
	p.StatsdClient.Incr("application_error", tags, 1.0)
	rw.WriteHeader(http.StatusBadRequest)
	return
	}
	ok := p.provider.ValidateSessionState(&sessions.SessionState{
	AccessToken: accessToken,
	})
	if !ok {
	tags = append(tags, "error:invalid_access_token")
	p.StatsdClient.Incr("provider_error", tags, 1.0)
	rw.WriteHeader(http.StatusUnauthorized)
	return
	}
	rw.WriteHeader(http.StatusOK)
	return
	}

Since providers already have a validate method that takes a session state, once it's been expanded to include ID tokens, it should be easy enough for providers with no ValidateURL to implement validation checks there. And the internal_util.go file could add a second validate method for OIDC ID tokens to make that straightforward.

sso/internal/auth/providers/providers.go

Lines 27 to 38 in a946076

	// Provider is an interface exposing functions necessary to authenticate with a given provider.
	type Provider interface {
	Data() *ProviderData
	Redeem(string, string) (*sessions.SessionState, error)
	ValidateSessionState(*sessions.SessionState) bool
	GetSignInURL(redirectURI, finalRedirect string) string
	RefreshSessionIfNeeded(*sessions.SessionState) (bool, error)
	ValidateGroupMembership(string, []string) ([]string, error)
	Revoke(*sessions.SessionState) error
	RefreshAccessToken(string) (string, time.Duration, error)
	Stop()
	}

The options stuff for Google I ran into immediately when I started on #27.
This bit's probably fine:

sso/internal/auth/options.go

Lines 203 to 210 in a946076

	if o.GoogleAdminEmail != "" || o.GoogleServiceAccountJSON != "" {
	if o.GoogleAdminEmail == "" {
	msgs = append(msgs, "missing setting: google-admin-email")
	}
	if o.GoogleServiceAccountJSON == "" {
	msgs = append(msgs, "missing setting: google-service-account-json")
	}
	}

But this probably needs addressing:

sso/internal/auth/options.go

Lines 243 to 253 in a946076

	if o.GoogleServiceAccountJSON != "" {
	_, err := os.Open(o.GoogleServiceAccountJSON)
	if err != nil {
	msgs = append(msgs, "invalid Google credentials file: "+o.GoogleServiceAccountJSON)
	}
	}
	googleProvider := providers.NewGoogleProvider(p, o.GoogleAdminEmail, o.GoogleServiceAccountJSON)
	cache := groups.NewFillCache(googleProvider.PopulateMembers, o.GroupsCacheRefreshTTL)
	googleProvider.GroupsCache = cache
	o.GroupsCacheStopFunc = cache.Stop
	singleFlightProvider := providers.NewSingleFlightProvider(googleProvider)

The solution from https://github.com/bitly/oauth2_proxy strikes me as probably not difficult to port over (and indeed, having already taken a quick crack at it, so far that seems to be the case).
https://github.com/bitly/oauth2_proxy/blob/faff555c553374859d01534e74f72c22a0e4417e/options.go#L260-L281

I think the switch might end up being structured slightly differently and the single flight stuff we maybe will want to wrap all providers in? I didn't dive into single flight too deeply, but in my initial tests it seemed to work fine with Azure AD at least.

[sporkmonger]

Potentially I think this could be two refactor PRs, one for options cleanup, one for validation & ID token prep work.