RPJosh
/
authelia
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

[BUGFIX] Bad redirection behavior after inactivity and inactivity update events. (#911) 

* This affects primarily Authelia instances running behind Traefik or
nginx ingress controllers within Kubernetes because those proxies
require that Authelia returns 302 instead of 401 after the session
has been inactive for too long.
* fixes #909
* fixed activity timestamp not being updated when accessing forbidden resources.
* fix inactivity not updated when user was inactive for too long.
* cover inactivity timeout updates with unit tests.
pull/914/head
Clément Michaud 2020-04-25 01:29:36 +02:00 committed by GitHub
parent f92480b44b
commit 9116135401
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 127 additions and 36 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

80
internal/handlers/handler_verify.go
View File

@ -6,7 +6,6 @@ import (
"net"
"net/url"
"strings"
"time"
"github.com/valyala/fasthttp"
@ -195,12 +194,47 @@ func verifyFromSessionCookie(targetURL url.URL, ctx *middlewares.AutheliaCtx) (u
return "", nil, authentication.NotAuthenticated, fmt.Errorf("Unable to destroy user session after long inactivity: %s", err)
}
return "", nil, authentication.NotAuthenticated, fmt.Errorf("User %s has been inactive for too long", userSession.Username)
return userSession.Username, userSession.Groups, authentication.NotAuthenticated, fmt.Errorf("User %s has been inactive for too long", userSession.Username)
}
}
return userSession.Username, userSession.Groups, userSession.AuthenticationLevel, nil
}
func handleUnauthorized(ctx *middlewares.AutheliaCtx, targetURL fmt.Stringer, username string) {
// Kubernetes ingress controller and Traefik use the rd parameter of the verify
// endpoint to provide the URL of the login portal. The target URL of the user
// is computed from X-Fowarded-* headers or X-Original-URL
rd := string(ctx.QueryArgs().Peek("rd"))
if rd != "" {
redirectionURL := fmt.Sprintf("%s?rd=%s", rd, url.QueryEscape(targetURL.String()))
if strings.Contains(redirectionURL, "/%23/") {
ctx.Logger.Warn("Characters /%23/ have been detected in redirection URL. This is not needed anymore, please strip it")
}
ctx.Logger.Infof("Access to %s is not authorized to user %s, redirecting to %s", targetURL.String(), username, redirectionURL)
ctx.Redirect(redirectionURL, 302)
ctx.SetBodyString(fmt.Sprintf("Found. Redirecting to %s", redirectionURL))
} else {
ctx.Logger.Infof("Access to %s is not authorized to user %s, sending 401 response", targetURL.String(), username)
ctx.ReplyUnauthorized()
}
}
func updateActivityTimestamp(ctx *middlewares.AutheliaCtx, isBasicAuth bool, username string) error {
if isBasicAuth || username == "" {
return nil
}
userSession := ctx.GetSession()
// We don't need to update the activity timestamp when user checked keep me logged in.
if userSession.KeepMeLoggedIn {
return nil
}
// Mark current activity
userSession.LastActivity = ctx.Clock.Now().Unix()
return ctx.SaveSession(userSession)
}
// VerifyGet is the handler verifying if a request is allowed to go through
func VerifyGet(ctx *middlewares.AutheliaCtx) {
ctx.Logger.Tracef("Headers=%s", ctx.Request.Header.String())
@ -230,9 +264,9 @@ func VerifyGet(ctx *middlewares.AutheliaCtx) {
var authLevel authentication.Level
proxyAuthorization := ctx.Request.Header.Peek(AuthorizationHeader)
hasBasicAuth := proxyAuthorization != nil
isBasicAuth := proxyAuthorization != nil
if hasBasicAuth {
if isBasicAuth {
username, groups, authLevel, err = verifyBasicAuth(proxyAuthorization, *targetURL, ctx)
} else {
username, groups, authLevel, err = verifyFromSessionCookie(*targetURL, ctx)
@ -240,7 +274,11 @@ func VerifyGet(ctx *middlewares.AutheliaCtx) {
if err != nil {
ctx.Logger.Error(fmt.Sprintf("Error caught when verifying user authorization: %s", err))
ctx.ReplyUnauthorized()
if err := updateActivityTimestamp(ctx, isBasicAuth, username); err != nil {
ctx.Error(fmt.Errorf("Unable to update last activity: %s", err), operationFailedMessage)
return
}
handleUnauthorized(ctx, targetURL, username)
return
}
@ -248,39 +286,15 @@ func VerifyGet(ctx *middlewares.AutheliaCtx) {
groups, ctx.RemoteIP(), authLevel)
if authorization == Forbidden {
ctx.Logger.Infof("Access to %s is forbidden to user %s", targetURL.String(), username)
ctx.ReplyForbidden()
ctx.Logger.Errorf("Access to %s is forbidden to user %s", targetURL.String(), username)
return
} else if authorization == NotAuthorized {
// Kubernetes ingress controller and Traefik use the rd parameter of the verify
// endpoint to provide the URL of the login portal. The target URL of the user
// is computed from X-Fowarded-* headers or X-Original-URL
rd := string(ctx.QueryArgs().Peek("rd"))
if rd != "" {
redirectionURL := fmt.Sprintf("%s?rd=%s", rd, url.QueryEscape(targetURL.String()))
if strings.Contains(redirectionURL, "/%23/") {
ctx.Logger.Warn("Characters /%23/ have been detected in redirection URL. This is not needed anymore, please strip it")
}
ctx.Redirect(redirectionURL, 302)
ctx.SetBodyString(fmt.Sprintf("Found. Redirecting to %s", redirectionURL))
} else {
ctx.ReplyUnauthorized()
ctx.Logger.Errorf("Access to %s is not authorized to user %s", targetURL.String(), username)
}
handleUnauthorized(ctx, targetURL, username)
} else if authorization == Authorized {
setForwardedHeaders(&ctx.Response.Header, username, groups)
}
// We mark activity of the current user if he comes with a session cookie
if !hasBasicAuth && username != "" {
// Mark current activity
userSession := ctx.GetSession()
userSession.LastActivity = time.Now().Unix()
err = ctx.SaveSession(userSession)
if err != nil {
ctx.Error(fmt.Errorf("Unable to update last activity: %s", err), operationFailedMessage)
return
}
if err := updateActivityTimestamp(ctx, isBasicAuth, username); err != nil {
ctx.Error(fmt.Errorf("Unable to update last activity: %s", err), operationFailedMessage)
}
}

83
internal/handlers/handler_verify_test.go
View File

@ -471,6 +471,7 @@ func TestShouldDestroySessionWhenInactiveForTooLong(t *testing.T) {
clock := mocks.TestingClock{}
clock.Set(time.Now())
past := clock.Now().Add(-1 * time.Hour)
mock.Ctx.Configuration.Session.Inactivity = "10"
// Reload the session provider since the configuration is indirect
@ -480,7 +481,7 @@ func TestShouldDestroySessionWhenInactiveForTooLong(t *testing.T) {
userSession := mock.Ctx.GetSession()
userSession.Username = "john"
userSession.AuthenticationLevel = authentication.TwoFactor
userSession.LastActivity = clock.Now().Add(-1 * time.Hour).Unix()
userSession.LastActivity = past.Unix()
mock.Ctx.SaveSession(userSession) //nolint:errcheck // TODO: Legacy code, consider refactoring time permitting.
mock.Ctx.Request.Header.Set("X-Original-URL", "https://two-factor.example.com")
@ -491,6 +492,9 @@ func TestShouldDestroySessionWhenInactiveForTooLong(t *testing.T) {
newUserSession := mock.Ctx.GetSession()
assert.Equal(t, "", newUserSession.Username)
assert.Equal(t, authentication.NotAuthenticated, newUserSession.AuthenticationLevel)
// Check the inactivity timestamp has been updated to current time in the new session.
assert.Equal(t, clock.Now().Unix(), newUserSession.LastActivity)
}
func TestShouldDestroySessionWhenInactiveForTooLongUsingDurationNotation(t *testing.T) {
@ -533,7 +537,7 @@ func TestShouldKeepSessionWhenUserCheckedRememberMeAndIsInactiveForTooLong(t *te
userSession := mock.Ctx.GetSession()
userSession.Username = "john"
userSession.AuthenticationLevel = authentication.TwoFactor
userSession.LastActivity = clock.Now().Add(-1 * time.Hour).Unix()
userSession.LastActivity = 0
userSession.KeepMeLoggedIn = true
mock.Ctx.SaveSession(userSession) //nolint:errcheck // TODO: Legacy code, consider refactoring time permitting.
@ -545,6 +549,9 @@ func TestShouldKeepSessionWhenUserCheckedRememberMeAndIsInactiveForTooLong(t *te
newUserSession := mock.Ctx.GetSession()
assert.Equal(t, "john", newUserSession.Username)
assert.Equal(t, authentication.TwoFactor, newUserSession.AuthenticationLevel)
// Check the inactivity timestamp is set to 0 in case remember me is checked.
assert.Equal(t, int64(0), newUserSession.LastActivity)
}
func TestShouldKeepSessionWhenInactivityTimeoutHasNotBeenExceeded(t *testing.T) {
@ -556,10 +563,12 @@ func TestShouldKeepSessionWhenInactivityTimeoutHasNotBeenExceeded(t *testing.T)
mock.Ctx.Configuration.Session.Inactivity = "10"
past := clock.Now().Add(-1 * time.Hour)
userSession := mock.Ctx.GetSession()
userSession.Username = "john"
userSession.AuthenticationLevel = authentication.TwoFactor
userSession.LastActivity = clock.Now().Add(-1 * time.Second).Unix()
userSession.LastActivity = past.Unix()
mock.Ctx.SaveSession(userSession) //nolint:errcheck // TODO: Legacy code, consider refactoring time permitting.
mock.Ctx.Request.Header.Set("X-Original-URL", "https://two-factor.example.com")
@ -570,6 +579,74 @@ func TestShouldKeepSessionWhenInactivityTimeoutHasNotBeenExceeded(t *testing.T)
newUserSession := mock.Ctx.GetSession()
assert.Equal(t, "john", newUserSession.Username)
assert.Equal(t, authentication.TwoFactor, newUserSession.AuthenticationLevel)
// Check the inactivity timestamp has been updated to current time in the new session.
assert.Equal(t, clock.Now().Unix(), newUserSession.LastActivity)
}
// In the case of Traefik and Nginx ingress controller in Kube, the response to an inactive
// session is 302 instead of 401.
func TestShouldRedirectWhenSessionInactiveForTooLongAndRDParamProvided(t *testing.T) {
mock := mocks.NewMockAutheliaCtx(t)
defer mock.Close()
clock := mocks.TestingClock{}
clock.Set(time.Now())
mock.Ctx.Configuration.Session.Inactivity = "10"
// Reload the session provider since the configuration is indirect
mock.Ctx.Providers.SessionProvider = session.NewProvider(mock.Ctx.Configuration.Session)
assert.Equal(t, time.Second*10, mock.Ctx.Providers.SessionProvider.Inactivity)
past := clock.Now().Add(-1 * time.Hour)
userSession := mock.Ctx.GetSession()
userSession.Username = "john"
userSession.AuthenticationLevel = authentication.TwoFactor
userSession.LastActivity = past.Unix()
mock.Ctx.SaveSession(userSession) //nolint:errcheck // TODO: Legacy code, consider refactoring time permitting.
mock.Ctx.QueryArgs().Add("rd", "https://login.example.com")
mock.Ctx.Request.Header.Set("X-Original-URL", "https://two-factor.example.com")
VerifyGet(mock.Ctx)
assert.Equal(t, "Found. Redirecting to https://login.example.com?rd=https%3A%2F%2Ftwo-factor.example.com",
string(mock.Ctx.Response.Body()))
assert.Equal(t, 302, mock.Ctx.Response.StatusCode())
// Check the inactivity timestamp has been updated to current time in the new session.
newUserSession := mock.Ctx.GetSession()
assert.Equal(t, clock.Now().Unix(), newUserSession.LastActivity)
}
func TestShouldUpdateInactivityTimestampEvenWhenHittingForbiddenResources(t *testing.T) {
mock := mocks.NewMockAutheliaCtx(t)
defer mock.Close()
clock := mocks.TestingClock{}
clock.Set(time.Now())
mock.Ctx.Configuration.Session.Inactivity = "10"
past := clock.Now().Add(-1 * time.Hour)
userSession := mock.Ctx.GetSession()
userSession.Username = "john"
userSession.AuthenticationLevel = authentication.TwoFactor
userSession.LastActivity = past.Unix()
mock.Ctx.SaveSession(userSession) //nolint:errcheck // TODO: Legacy code, consider refactoring time permitting.
mock.Ctx.Request.Header.Set("X-Original-URL", "https://deny.example.com")
VerifyGet(mock.Ctx)
// The resource if forbidden
assert.Equal(t, 403, mock.Ctx.Response.StatusCode())
// Check the inactivity timestamp has been updated to current time in the new session.
newUserSession := mock.Ctx.GetSession()
assert.Equal(t, clock.Now().Unix(), newUserSession.LastActivity)
}
func TestShouldURLEncodeRedirectionURLParameter(t *testing.T) {