From a63d55201f7f1d62695938a7cb4aa3c4f5dbda7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Michaud?= Date: Thu, 6 Feb 2020 03:24:25 +0100 Subject: [PATCH] [MISC] Improve documentation around headers used by verify endpoint. (#620) * Explicit document missing X-Forwarded-Proto and X-Fowarded-Host headers. * Add the name of the authorization header in error messages. * Add error and debug logs about X-Original-URL header. * Add error log when not able to parse target URL in verify endpoint. * Fix unit tests. --- internal/handlers/const.go | 2 +- internal/handlers/errors.go | 3 +- internal/handlers/handler_verify.go | 33 ++++++++++--------- internal/handlers/handler_verify_test.go | 42 +++++++++++++++++++++--- 4 files changed, 59 insertions(+), 21 deletions(-) diff --git a/internal/handlers/const.go b/internal/handlers/const.go index 844f56ad..5d57ff1d 100644 --- a/internal/handlers/const.go +++ b/internal/handlers/const.go @@ -11,7 +11,7 @@ const ResetPasswordAction = "ResetPassword" const authPrefix = "Basic " -const authorizationHeader = "Proxy-Authorization" +const AuthorizationHeader = "Proxy-Authorization" const remoteUserHeader = "Remote-User" const remoteGroupsHeader = "Remote-Groups" diff --git a/internal/handlers/errors.go b/internal/handlers/errors.go index f9ebdb35..ea4b7f71 100644 --- a/internal/handlers/errors.go +++ b/internal/handlers/errors.go @@ -9,4 +9,5 @@ const InternalError = "Internal error." // UnauthorizedError is the error message sent when the user is not authorized. const UnauthorizedError = "You're not authorized." -var errMissingHeadersForTargetURL = errors.New("Missing headers for detecting target URL") +var errMissingXForwardedHost = errors.New("Missing header X-Fowarded-Host used to detect target URL") +var errMissingXForwardedProto = errors.New("Missing header X-Fowarded-Proto used to detect target URL") diff --git a/internal/handlers/handler_verify.go b/internal/handlers/handler_verify.go index 1e20b5aa..991191d6 100644 --- a/internal/handlers/handler_verify.go +++ b/internal/handlers/handler_verify.go @@ -20,31 +20,34 @@ func getOriginalURL(ctx *middlewares.AutheliaCtx) (*url.URL, error) { if originalURL != nil { url, err := url.ParseRequestURI(string(originalURL)) if err != nil { - return nil, err + return nil, fmt.Errorf("Unable to parse URL extracted from X-Original-URL header: %v", err) } return url, nil + } else { + ctx.Logger.Debug("No X-Original-URL header detected, fallback to combination of " + + "X-Fowarded-Proto, X-Forwarded-Host and X-Forwarded-URI headers") } forwardedProto := ctx.XForwardedProto() forwardedHost := ctx.XForwardedHost() forwardedURI := ctx.XForwardedURI() - if forwardedProto == nil || forwardedHost == nil { - return nil, errMissingHeadersForTargetURL + if forwardedProto == nil { + return nil, errMissingXForwardedProto + } + + if forwardedHost == nil { + return nil, errMissingXForwardedHost } var requestURI string scheme := append(forwardedProto, protoHostSeparator...) - if forwardedURI == nil { - requestURI = string(append(scheme, forwardedHost...)) - } - requestURI = string(append(scheme, append(forwardedHost, forwardedURI...)...)) - url, err := url.ParseRequestURI(string(requestURI)) + url, err := url.ParseRequestURI(requestURI) if err != nil { - return nil, err + return nil, fmt.Errorf("Unable to parse URL %s: %v", requestURI, err) } return url, nil } @@ -53,7 +56,7 @@ func getOriginalURL(ctx *middlewares.AutheliaCtx) (*url.URL, error) { // "Basic QWxhZGRpbjpvcGVuIHNlc2FtZQ==" returns ("Aladdin", "open sesame", true). func parseBasicAuth(auth string) (username, password string, err error) { if !strings.HasPrefix(auth, authPrefix) { - return "", "", fmt.Errorf("%s prefix not found in authorization header", strings.Trim(authPrefix, " ")) + return "", "", fmt.Errorf("%s prefix not found in %s header", strings.Trim(authPrefix, " "), AuthorizationHeader) } c, err := base64.StdEncoding.DecodeString(auth[len(authPrefix):]) if err != nil { @@ -62,7 +65,7 @@ func parseBasicAuth(auth string) (username, password string, err error) { cs := string(c) s := strings.IndexByte(cs, ':') if s < 0 { - return "", "", fmt.Errorf("Format for basic auth must be user:password") + return "", "", fmt.Errorf("Format of %s header must be user:password", AuthorizationHeader) } return cs[:s], cs[s+1:], nil } @@ -105,13 +108,13 @@ func verifyBasicAuth(auth []byte, targetURL url.URL, ctx *middlewares.AutheliaCt username, password, err := parseBasicAuth(string(auth)) if err != nil { - return "", nil, authentication.NotAuthenticated, fmt.Errorf("Unable to parse basic auth: %s", err) + return "", nil, authentication.NotAuthenticated, fmt.Errorf("Unable to parse content of %s header: %s", AuthorizationHeader, err) } authenticated, err := ctx.Providers.UserProvider.CheckUserPassword(username, password) if err != nil { - return "", nil, authentication.NotAuthenticated, fmt.Errorf("Unable to check password in basic auth mode: %s", err) + return "", nil, authentication.NotAuthenticated, fmt.Errorf("Unable to check credentials extracted from %s header: %s", AuthorizationHeader, err) } // If the user is not correctly authenticated, send a 401. @@ -123,7 +126,7 @@ func verifyBasicAuth(auth []byte, targetURL url.URL, ctx *middlewares.AutheliaCt details, err := ctx.Providers.UserProvider.GetDetails(username) if err != nil { - return "", nil, authentication.NotAuthenticated, fmt.Errorf("Unable to retrieve user details in basic auth mode: %s", err) + return "", nil, authentication.NotAuthenticated, fmt.Errorf("Unable to retrieve details of user %s: %s", username, err) } return username, details.Groups, authentication.OneFactor, nil @@ -200,7 +203,7 @@ func VerifyGet(ctx *middlewares.AutheliaCtx) { var groups []string var authLevel authentication.Level - proxyAuthorization := ctx.Request.Header.Peek(authorizationHeader) + proxyAuthorization := ctx.Request.Header.Peek(AuthorizationHeader) hasBasicAuth := proxyAuthorization != nil if hasBasicAuth { diff --git a/internal/handlers/handler_verify_test.go b/internal/handlers/handler_verify_test.go index 035dbfe8..9273ddd4 100644 --- a/internal/handlers/handler_verify_test.go +++ b/internal/handlers/handler_verify_test.go @@ -13,6 +13,7 @@ import ( "github.com/authelia/authelia/internal/mocks" "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" ) @@ -49,7 +50,7 @@ func TestShouldGetOriginalURLFromForwardedHeadersWithURI(t *testing.T) { mock.Ctx.Request.Header.Set("X-Original-URL", "htt-ps//home?-.example.com") _, err := getOriginalURL(mock.Ctx) assert.Error(t, err) - assert.Equal(t, "parse htt-ps//home?-.example.com: invalid URI for request", err.Error()) + assert.Equal(t, "Unable to parse URL extracted from X-Original-URL header: parse htt-ps//home?-.example.com: invalid URI for request", err.Error()) } func TestShouldRaiseWhenTargetUrlIsMalformed(t *testing.T) { @@ -71,14 +72,47 @@ func TestShouldRaiseWhenNoHeaderProvidedToDetectTargetURL(t *testing.T) { defer mock.Close() _, err := getOriginalURL(mock.Ctx) assert.Error(t, err) - assert.Equal(t, "Missing headers for detecting target URL", err.Error()) + assert.Equal(t, "Missing header X-Fowarded-Proto used to detect target URL", err.Error()) +} + +func TestShouldRaiseWhenNoXForwardedHostHeaderProvidedToDetectTargetURL(t *testing.T) { + mock := mocks.NewMockAutheliaCtx(t) + defer mock.Close() + + mock.Ctx.Request.Header.Set("X-Forwarded-Proto", "https") + _, err := getOriginalURL(mock.Ctx) + assert.Error(t, err) + assert.Equal(t, "Missing header X-Fowarded-Host used to detect target URL", err.Error()) +} + +func TestShouldRaiseWhenXForwardedProtoIsNotParseable(t *testing.T) { + mock := mocks.NewMockAutheliaCtx(t) + defer mock.Close() + + mock.Ctx.Request.Header.Set("X-Forwarded-Proto", "!:;;:,") + mock.Ctx.Request.Header.Set("X-Forwarded-Host", "myhost.local") + _, err := getOriginalURL(mock.Ctx) + assert.Error(t, err) + assert.Equal(t, "Unable to parse URL !:;;:,://myhost.local: parse !:;;:,://myhost.local: invalid URI for request", err.Error()) +} + +func TestShouldRaiseWhenXForwardedURIIsNotParseable(t *testing.T) { + mock := mocks.NewMockAutheliaCtx(t) + defer mock.Close() + + mock.Ctx.Request.Header.Set("X-Forwarded-Proto", "https") + mock.Ctx.Request.Header.Set("X-Forwarded-Host", "myhost.local") + mock.Ctx.Request.Header.Set("X-Forwarded-URI", "!:;;:,") + _, err := getOriginalURL(mock.Ctx) + require.Error(t, err) + assert.Equal(t, "Unable to parse URL https://myhost.local!:;;:,: parse https://myhost.local!:;;:,: invalid port \":,\" after host", err.Error()) } // Test parseBasicAuth func TestShouldRaiseWhenHeaderDoesNotContainBasicPrefix(t *testing.T) { _, _, err := parseBasicAuth("alzefzlfzemjfej==") assert.Error(t, err) - assert.Equal(t, "Basic prefix not found in authorization header", err.Error()) + assert.Equal(t, "Basic prefix not found in Proxy-Authorization header", err.Error()) } func TestShouldRaiseWhenCredentialsAreNotInBase64(t *testing.T) { @@ -91,7 +125,7 @@ func TestShouldRaiseWhenCredentialsAreNotInCorrectForm(t *testing.T) { // the decoded format should be user:password. _, _, err := parseBasicAuth("Basic am9obiBwYXNzd29yZA==") assert.Error(t, err) - assert.Equal(t, "Format for basic auth must be user:password", err.Error()) + assert.Equal(t, "Format of Proxy-Authorization header must be user:password", err.Error()) } func TestShouldReturnUsernameAndPassword(t *testing.T) {