From 26a857d133086267cdb94a7fc5e66233450e2fa1 Mon Sep 17 00:00:00 2001 From: Jusshersmith Date: Fri, 6 Dec 2019 15:17:16 +0000 Subject: [PATCH] sso_*:clean up some validation calls --- internal/pkg/options/email_group_validator.go | 2 +- internal/pkg/options/validators.go | 1 - internal/pkg/sessions/session_state.go | 8 ++ internal/proxy/oauthproxy.go | 85 ++++++++++++------ internal/proxy/providers/providers.go | 4 +- .../providers/singleflight_middleware.go | 14 +-- internal/proxy/providers/sso.go | 86 ++++--------------- 7 files changed, 93 insertions(+), 107 deletions(-) diff --git a/internal/pkg/options/email_group_validator.go b/internal/pkg/options/email_group_validator.go index 14d1c6dd..198cc8a4 100644 --- a/internal/pkg/options/email_group_validator.go +++ b/internal/pkg/options/email_group_validator.go @@ -42,7 +42,7 @@ func (v EmailGroupValidator) Validate(session *sessions.SessionState) error { func (v EmailGroupValidator) validate(session *sessions.SessionState) error { matchedGroups, valid, err := v.Provider.ValidateGroup(session.Email, v.AllowedGroups, session.AccessToken) if err != nil { - return ErrValidationError + return err } if valid { diff --git a/internal/pkg/options/validators.go b/internal/pkg/options/validators.go index 0b14e48f..535ca4c2 100644 --- a/internal/pkg/options/validators.go +++ b/internal/pkg/options/validators.go @@ -10,7 +10,6 @@ var ( // These error message should be formatted in such a way that is appropriate // for display to the end user. ErrInvalidEmailAddress = errors.New("Invalid Email Address In Session State") - ErrValidationError = errors.New("Error during validation") ) type Validator interface { diff --git a/internal/pkg/sessions/session_state.go b/internal/pkg/sessions/session_state.go index f6044d7e..adcd0182 100644 --- a/internal/pkg/sessions/session_state.go +++ b/internal/pkg/sessions/session_state.go @@ -45,6 +45,14 @@ func (s *SessionState) ValidationPeriodExpired() bool { return isExpired(s.ValidDeadline) } +// IsWithinGracePeriod returns true if the session is still within the grace period +func (s *SessionState) IsWithinGracePeriod(gracePeriodTTL time.Duration) bool { + if s.GracePeriodStart.IsZero() { + s.GracePeriodStart = time.Now() + } + return s.GracePeriodStart.Add(gracePeriodTTL).After(time.Now()) +} + func isExpired(t time.Time) bool { if t.Before(time.Now()) { return true diff --git a/internal/proxy/oauthproxy.go b/internal/proxy/oauthproxy.go index 0944c95e..7cfcc8ec 100644 --- a/internal/proxy/oauthproxy.go +++ b/internal/proxy/oauthproxy.go @@ -359,7 +359,7 @@ func (p *OAuthProxy) ErrorPage(rw http.ResponseWriter, req *http.Request, code i p.templates.ExecuteTemplate(rw, "error.html", t) } -// IsWhitelistedRequest cheks that proxy host exists and checks the SkipAuthRegex +// IsWhitelistedRequest checks that proxy host exists and checks the SkipAuthRegex func (p *OAuthProxy) IsWhitelistedRequest(req *http.Request) bool { if p.skipAuthPreflight && req.Method == "OPTIONS" { return true @@ -375,6 +375,28 @@ func (p *OAuthProxy) IsWhitelistedRequest(req *http.Request) bool { return false } +// runValidatorsWithGracePeriod runs all validators and upon finding errors, checks to see if the +// auth provider is explicity denying authentication or if it's merely unavailable. If it's unavailable, +// we check whether the session is within the grace period or not to determine whether to allow the request +// at this point. +func (p *OAuthProxy) runValidatorsWithGracePeriod(session *sessions.SessionState) (err error) { + logger := log.NewLogEntry() + errors := options.RunValidators(p.Validators, session) + if len(errors) == len(p.Validators) { + for _, err := range errors { + // Check to see if the auth provider is explicity denying authentication, or if it is merely unavailable. + if err == providers.ErrAuthProviderUnavailable && session.IsWithinGracePeriod(p.provider.Data().GracePeriodTTL) { + return err + } + } + allowedGroups := p.upstreamConfig.AllowedGroups + logger.WithUser(session.Email).WithAllowedGroups(allowedGroups).Info( + "no longer authorized after validation period: %q", errors) + return ErrUserNotAuthorized + } + return nil +} + func (p *OAuthProxy) isXHR(req *http.Request) bool { return req.Header.Get("X-Requested-With") == "XMLHttpRequest" } @@ -693,8 +715,6 @@ func (p *OAuthProxy) Authenticate(rw http.ResponseWriter, req *http.Request) (er remoteAddr := getRemoteAddr(req) tags := []string{"action:authenticate"} - allowedGroups := p.upstreamConfig.AllowedGroups - // Clear the session cookie if anything goes wrong. defer func() { if err != nil { @@ -705,7 +725,7 @@ func (p *OAuthProxy) Authenticate(rw http.ResponseWriter, req *http.Request) (er session, err := p.sessionStore.LoadSession(req) if err != nil { // We loaded a cookie but it wasn't valid, clear it, and reject the request - logger.Error(err, "error authenticating user") + logger.Error(err, "invalid session loaded") return err } @@ -728,7 +748,23 @@ func (p *OAuthProxy) Authenticate(rw http.ResponseWriter, req *http.Request) (er } else if session.RefreshPeriodExpired() { // Refresh period is the period in which the access token is valid. This is ultimately // controlled by the upstream provider and tends to be around 1 hour. - ok, err := p.provider.RefreshSession(session, allowedGroups) + // If it has expired we: + // - run email domain, email address, and email group validations against the session (if defined). + // - attempt to refresh the session + + err := p.runValidatorsWithGracePeriod(session) + if err != nil { + switch err { + case providers.ErrAuthProviderUnavailable: + tags = append(tags, "action:refresh_session", "error:validation_failed") + p.StatsdClient.Incr("provider_error_fallback", tags, 1.0) + session.RefreshDeadline = sessions.ExtendDeadline(p.provider.Data().SessionValidTTL) + default: + return ErrUserNotAuthorized + } + } + + ok, err := p.provider.RefreshSession(session) // We failed to refresh the session successfully // clear the cookie and reject the request if err != nil { @@ -757,9 +793,23 @@ func (p *OAuthProxy) Authenticate(rw http.ResponseWriter, req *http.Request) (er } else if session.ValidationPeriodExpired() { // Validation period has expired, this is the shortest interval we use to // check for valid requests. This should be set to something like a minute. - // This calls up the provider chain to validate this user is still active - // and hasn't been de-authorized. - ok := p.provider.ValidateSessionState(session, allowedGroups) + // In this case we: + // - run any defined email domain, email address, and email group validators against the session + // - call up the provider chain to validate this user is still active and hasn't been de-authorized. + + err := p.runValidatorsWithGracePeriod(session) + if err != nil { + switch err { + case providers.ErrAuthProviderUnavailable: + tags = append(tags, "action:refresh_session", "error:validation_failed") + p.StatsdClient.Incr("provider_error_fallback", tags, 1.0) + session.RefreshDeadline = sessions.ExtendDeadline(p.provider.Data().SessionValidTTL) + default: + return ErrUserNotAuthorized + } + } + + ok := p.provider.ValidateSessionToken(session) if !ok { // This user is now no longer authorized, or we failed to // validate the user. @@ -781,25 +831,6 @@ func (p *OAuthProxy) Authenticate(rw http.ResponseWriter, req *http.Request) (er } } - // We revalidate group membership whenever the session is refreshed or revalidated - // just above in the call to ValidateSessionState and RefreshSession. - // To reduce strain on upstream identity providers we only revalidate email domains and - // addresses on each request here. - for _, v := range p.Validators { - _, EmailGroupValidator := v.(options.EmailGroupValidator) - - if !EmailGroupValidator { - err := v.Validate(session) - if err != nil { - tags = append(tags, "error:validation_failed") - p.StatsdClient.Incr("application_error", tags, 1.0) - logger.WithRemoteAddress(remoteAddr).WithUser(session.Email).Info( - fmt.Sprintf("permission denied: unauthorized: %q", err)) - return ErrUserNotAuthorized - } - } - } - logger.WithRemoteAddress(remoteAddr).WithUser(session.Email).Info( fmt.Sprintf("authentication: user validated")) diff --git a/internal/proxy/providers/providers.go b/internal/proxy/providers/providers.go index 226dc16f..26b3e504 100644 --- a/internal/proxy/providers/providers.go +++ b/internal/proxy/providers/providers.go @@ -13,10 +13,10 @@ type Provider interface { Redeem(string, string) (*sessions.SessionState, error) ValidateGroup(string, []string, string) ([]string, bool, error) UserGroups(string, []string, string) ([]string, error) - ValidateSessionState(*sessions.SessionState, []string) bool + ValidateSessionToken(*sessions.SessionState) bool GetSignInURL(redirectURL *url.URL, finalRedirect string) *url.URL GetSignOutURL(redirectURL *url.URL) *url.URL - RefreshSession(*sessions.SessionState, []string) (bool, error) + RefreshSession(*sessions.SessionState) (bool, error) } // New returns a new sso Provider diff --git a/internal/proxy/providers/singleflight_middleware.go b/internal/proxy/providers/singleflight_middleware.go index 40e6fcf7..d0409b54 100644 --- a/internal/proxy/providers/singleflight_middleware.go +++ b/internal/proxy/providers/singleflight_middleware.go @@ -94,10 +94,10 @@ func (p *SingleFlightProvider) UserGroups(email string, groups []string, accessT return groups, nil } -// ValidateSessionState calls the provider's ValidateSessionState function and returns the response -func (p *SingleFlightProvider) ValidateSessionState(s *sessions.SessionState, allowedGroups []string) bool { - response, err := p.do("ValidateSessionState", s.AccessToken, func() (interface{}, error) { - valid := p.provider.ValidateSessionState(s, allowedGroups) +// ValidateSessionToken calls the provider's ValidateSessionToken function and returns the response +func (p *SingleFlightProvider) ValidateSessionToken(s *sessions.SessionState) bool { + response, err := p.do("ValidateSessionToken", s.AccessToken, func() (interface{}, error) { + valid := p.provider.ValidateSessionToken(s) return valid, nil }) if err != nil { @@ -112,11 +112,11 @@ func (p *SingleFlightProvider) ValidateSessionState(s *sessions.SessionState, al return valid } -// RefreshSession takes in a SessionState and allowedGroups and +// RefreshSession takes in a SessionState and // returns false if the session is not refreshed and true if it is. -func (p *SingleFlightProvider) RefreshSession(s *sessions.SessionState, allowedGroups []string) (bool, error) { +func (p *SingleFlightProvider) RefreshSession(s *sessions.SessionState) (bool, error) { response, err := p.do("RefreshSession", s.RefreshToken, func() (interface{}, error) { - return p.provider.RefreshSession(s, allowedGroups) + return p.provider.RefreshSession(s) }) if err != nil { return false, err diff --git a/internal/proxy/providers/sso.go b/internal/proxy/providers/sso.go index 1aca089c..f4dcdd91 100644 --- a/internal/proxy/providers/sso.go +++ b/internal/proxy/providers/sso.go @@ -98,17 +98,6 @@ func isProviderUnavailable(statusCode int) bool { return statusCode == http.StatusTooManyRequests || statusCode == http.StatusServiceUnavailable } -func extendDeadline(ttl time.Duration) time.Time { - return time.Now().Add(ttl).Truncate(time.Second) -} - -func (p *SSOProvider) withinGracePeriod(s *sessions.SessionState) bool { - if s.GracePeriodStart.IsZero() { - s.GracePeriodStart = time.Now() - } - return s.GracePeriodStart.Add(p.GracePeriodTTL).After(time.Now()) -} - // Redeem takes a redirectURL and code and redeems the SessionState func (p *SSOProvider) Redeem(redirectURL, code string) (*sessions.SessionState, error) { if code == "" { @@ -165,9 +154,9 @@ func (p *SSOProvider) Redeem(redirectURL, code string) (*sessions.SessionState, AccessToken: jsonResponse.AccessToken, RefreshToken: jsonResponse.RefreshToken, - RefreshDeadline: extendDeadline(time.Duration(jsonResponse.ExpiresIn) * time.Second), - LifetimeDeadline: extendDeadline(p.SessionLifetimeTTL), - ValidDeadline: extendDeadline(p.SessionValidTTL), + RefreshDeadline: sessions.ExtendDeadline(time.Duration(jsonResponse.ExpiresIn) * time.Second), + LifetimeDeadline: sessions.ExtendDeadline(p.SessionLifetimeTTL), + ValidDeadline: sessions.ExtendDeadline(p.SessionValidTTL), Email: jsonResponse.Email, User: user, @@ -245,9 +234,9 @@ func (p *SSOProvider) UserGroups(email string, groups []string, accessToken stri return jsonResponse.Groups, nil } -// RefreshSession takes a SessionState and allowedGroups and refreshes the session access token, +// RefreshSession takes a SessionState and refreshes the session access token, // returns `true` on success, and `false` on error -func (p *SSOProvider) RefreshSession(s *sessions.SessionState, allowedGroups []string) (bool, error) { +func (p *SSOProvider) RefreshSession(s *sessions.SessionState) (bool, error) { logger := log.NewLogEntry() if s.RefreshToken == "" { @@ -259,35 +248,17 @@ func (p *SSOProvider) RefreshSession(s *sessions.SessionState, allowedGroups []s // When we detect that the auth provider is not explicitly denying // authentication, and is merely unavailable, we refresh and continue // as normal during the "grace period" - if err == ErrAuthProviderUnavailable && p.withinGracePeriod(s) { + if err == ErrAuthProviderUnavailable && s.IsWithinGracePeriod(p.GracePeriodTTL) { tags := []string{"action:refresh_session", "error:redeem_token_failed"} p.StatsdClient.Incr("provider_error_fallback", tags, 1.0) - s.RefreshDeadline = extendDeadline(p.SessionValidTTL) + s.RefreshDeadline = sessions.ExtendDeadline(p.SessionValidTTL) return true, nil } return false, err } - inGroups, validGroup, err := p.ValidateGroup(s.Email, allowedGroups, newToken) - if err != nil { - // When we detect that the auth provider is not explicitly denying - // authentication, and is merely unavailable, we refresh and continue - // as normal during the "grace period" - if err == ErrAuthProviderUnavailable && p.withinGracePeriod(s) { - tags := []string{"action:refresh_session", "error:user_groups_failed"} - p.StatsdClient.Incr("provider_error_fallback", tags, 1.0) - s.RefreshDeadline = extendDeadline(p.SessionValidTTL) - return true, nil - } - return false, err - } - if !validGroup { - return false, errors.New("Group membership revoked") - } - s.Groups = inGroups - s.AccessToken = newToken - s.RefreshDeadline = extendDeadline(duration) + s.RefreshDeadline = sessions.ExtendDeadline(duration) s.GracePeriodStart = time.Time{} logger.WithUser(s.Email).WithRefreshDeadline(s.RefreshDeadline).Info("refreshed session access token") return true, nil @@ -340,8 +311,8 @@ func (p *SSOProvider) redeemRefreshToken(refreshToken string) (token string, exp return } -// ValidateSessionState takes a sessionState and allowedGroups and validates the session state -func (p *SSOProvider) ValidateSessionState(s *sessions.SessionState, allowedGroups []string) bool { +// ValidateSessionToken takes a sessionState and validates the session token +func (p *SSOProvider) ValidateSessionToken(s *sessions.SessionState) bool { logger := log.NewLogEntry() // we validate the user's access token is valid @@ -349,7 +320,7 @@ func (p *SSOProvider) ValidateSessionState(s *sessions.SessionState, allowedGrou params.Add("client_id", p.ClientID) req, err := p.newRequest("GET", fmt.Sprintf("%s?%s", p.ValidateURL.String(), params.Encode()), nil) if err != nil { - logger.WithUser(s.Email).Error(err, "error validating session state") + logger.WithUser(s.Email).Error(err, "error validating session access token") return false } @@ -358,7 +329,7 @@ func (p *SSOProvider) ValidateSessionState(s *sessions.SessionState, allowedGrou resp, err := httpClient.Do(req) if err != nil { - logger.WithUser(s.Email).Error("error making request to validate access token") + logger.WithUser(s.Email).Error("error making request to validate session access token") return false } @@ -366,44 +337,21 @@ func (p *SSOProvider) ValidateSessionState(s *sessions.SessionState, allowedGrou // When we detect that the auth provider is not explicitly denying // authentication, and is merely unavailable, we validate and continue // as normal during the "grace period" - if isProviderUnavailable(resp.StatusCode) && p.withinGracePeriod(s) { + if isProviderUnavailable(resp.StatusCode) && s.IsWithinGracePeriod(p.GracePeriodTTL) { tags := []string{"action:validate_session", "error:validation_failed"} p.StatsdClient.Incr("provider_error_fallback", tags, 1.0) - s.ValidDeadline = extendDeadline(p.SessionValidTTL) + s.ValidDeadline = sessions.ExtendDeadline(p.SessionValidTTL) return true } logger.WithUser(s.Email).WithHTTPStatus(resp.StatusCode).Info( - "could not validate user access token") - return false - } - - // check the user is in the proper group(s) - inGroups, validGroup, err := p.ValidateGroup(s.Email, allowedGroups, s.AccessToken) - if err != nil { - // When we detect that the auth provider is not explicitly denying - // authentication, and is merely unavailable, we validate and continue - // as normal during the "grace period" - if err == ErrAuthProviderUnavailable && p.withinGracePeriod(s) { - tags := []string{"action:validate_session", "error:user_groups_failed"} - p.StatsdClient.Incr("provider_error_fallback", tags, 1.0) - s.ValidDeadline = extendDeadline(p.SessionValidTTL) - return true - } - logger.WithUser(s.Email).Error(err, "error fetching group memberships") - return false - } - - if !validGroup { - logger.WithUser(s.Email).WithAllowedGroups(allowedGroups).Info( - "user is no longer in valid groups") + "could not validate session access token") return false } - s.Groups = inGroups - s.ValidDeadline = extendDeadline(p.SessionValidTTL) + s.ValidDeadline = sessions.ExtendDeadline(p.SessionValidTTL) s.GracePeriodStart = time.Time{} - logger.WithUser(s.Email).WithSessionValid(s.ValidDeadline).Info("validated session") + logger.WithUser(s.Email).WithSessionValid(s.ValidDeadline).Info("validated session access token") return true }