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

sso_*: prevent copying of session between upstreams #299

Merged
merged 1 commit into from
Jul 28, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions internal/pkg/logging/logging.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,11 @@ func (l *LogEntry) WithEndpoint(endpoint string) *LogEntry {
return l.withField("endpoint", endpoint)
}

// WithAuthorizedUpstream appends an `authorized_upstream` tag to a LogEntry.
func (l *LogEntry) WithAuthorizedUpstream(upstream string) *LogEntry {
return l.withField("authorized_upstream", upstream)
}

// WithError appends an `error` tag to a LogEntry. Useful for annotating non-Error log
// entries (e.g. Fatal messages) with an `error` object.
func (l *LogEntry) WithError(err error) *LogEntry {
Expand Down
7 changes: 4 additions & 3 deletions internal/pkg/sessions/session_state.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,10 @@ type SessionState struct {
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"`
Email string `json:"email"`
User string `json:"user"`
Groups []string `json:"groups"`
AuthorizedUpstream string `json:"authorized_upstream"`
}

// LifetimePeriodExpired returns true if the lifetime has expired
Expand Down
29 changes: 26 additions & 3 deletions internal/proxy/oauthproxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,10 @@ var SignatureHeaders = []string{

// Errors
var (
ErrLifetimeExpired = errors.New("user lifetime expired")
ErrUserNotAuthorized = errors.New("user not authorized")
ErrWrongIdentityProvider = errors.New("user authenticated with wrong identity provider")
ErrLifetimeExpired = errors.New("user lifetime expired")
ErrUserNotAuthorized = errors.New("user not authorized")
ErrWrongIdentityProvider = errors.New("user authenticated with wrong identity provider")
ErrUnauthorizedUpstreamRequested = errors.New("user session authorized with different upstream")
)

type ErrOAuthProxyMisconfigured struct {
Expand Down Expand Up @@ -591,6 +592,13 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) {
logger.WithRemoteAddress(remoteAddr).WithUser(session.Email).WithInGroups(session.Groups).Info(
fmt.Sprintf("oauth callback: user validated "))

// We add the request host into the session to allow us to validate that each request has
// been authorized for the upstream it's requesting.
// e.g. if a request is authenticated while trying to reach 'foo' upstream, it should not
// automatically be seen as authorized with 'bar' upstream. Each upstream may set different
// validators, so the request should be reauthenticated.
session.AuthorizedUpstream = req.Host

// We store the session in a cookie and redirect the user back to the application
err = p.sessionStore.SaveSession(rw, req, session)
if err != nil {
Expand Down Expand Up @@ -655,6 +663,12 @@ func (p *OAuthProxy) Proxy(rw http.ResponseWriter, req *http.Request) {
// the user has a stale sesssion.
p.OAuthStart(rw, req, tags)
return
case ErrUnauthorizedUpstreamRequested:
// The users session has been authorised for use with a different upstream than the one
// that is being requested, so we trigger the start of the oauth flow.
// This exists primarily to implement some form of grace period while this additional session
// check is being introduced.
p.OAuthStart(rw, req, tags)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: will this invalidate the current validated session for 'foo' upstream?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't invalidate the session on the original upstream - and because it restarts the oauth flow for the new request/upstream the UX should be pretty seamless when this is triggered.

case sessions.ErrInvalidSession:
// The user session is invalid and we can't decode it.
// This can happen for a variety of reasons but the most common non-malicious
Expand Down Expand Up @@ -720,6 +734,15 @@ func (p *OAuthProxy) Authenticate(rw http.ResponseWriter, req *http.Request) (er
return ErrWrongIdentityProvider
}

// check that the user has been authorized against the requested upstream
// this is primarily to combat against a user authorizing with one upstream and attempting to use
// the session cookie for a different upstream.
if req.Host != session.AuthorizedUpstream {
logger.WithProxyHost(req.Host).WithAuthorizedUpstream(session.AuthorizedUpstream).WithUser(session.Email).Warn(
"session authorized against different upstream; restarting authentication")
return ErrUnauthorizedUpstreamRequested
}

// Lifetime period is the entire duration in which the session is valid.
// This should be set to something like 14 to 30 days.
if session.LifetimePeriodExpired() {
Expand Down
Loading