From e2ebdb7e41711e5c17966e35fff799ec5a27153b Mon Sep 17 00:00:00 2001 From: Amir Zarrinkafsh Date: Tue, 10 Aug 2021 10:31:08 +1000 Subject: [PATCH] fix: oidc issuer path and strip path middleware (#2272) * fix: oidc issuer path and strip path middleware This ensures the server.path requests append the base_url to the oidc well-known issuer information and adjusts server.path configuration to only strip the configured path instead of the first level entirely regardless of its content. * fix: only log the token error and general refactoring * refactor: factorize base_url functions * refactor(server): include all paths in startup logging * refactor: factorize * refactor: GetExternalRootURL -> ExternalRootURL Co-authored-by: James Elliott --- internal/handlers/const.go | 2 + ...orize.go => handler_oidc_authorization.go} | 20 +++------- ...spect.go => handler_oidc_introspection.go} | 2 +- ...c_revoke.go => handler_oidc_revocation.go} | 2 +- internal/handlers/handler_oidc_token.go | 8 ++-- internal/handlers/handler_oidc_wellknown.go | 7 ++-- internal/handlers/oidc_register.go | 8 ++-- internal/handlers/response.go | 2 +- internal/middlewares/authelia_context.go | 38 ++++++++++++++----- internal/middlewares/errors.go | 7 ---- internal/middlewares/identity_verification.go | 4 +- internal/middlewares/strip_path.go | 15 ++++---- internal/server/server.go | 21 +++++++--- internal/server/template.go | 7 +++- 14 files changed, 82 insertions(+), 61 deletions(-) rename internal/handlers/{handler_oidc_authorize.go => handler_oidc_authorization.go} (90%) rename internal/handlers/{handler_oidc_introspect.go => handler_oidc_introspection.go} (82%) rename internal/handlers/{handler_oidc_revoke.go => handler_oidc_revocation.go} (71%) diff --git a/internal/handlers/const.go b/internal/handlers/const.go index 576327ebe..5c4609b7c 100644 --- a/internal/handlers/const.go +++ b/internal/handlers/const.go @@ -60,6 +60,8 @@ const ( // OIDC constants. const ( + pathOpenIDConnectWellKnown = "/.well-known/openid-configuration" + pathOpenIDConnectJWKs = "/api/oidc/jwks" pathOpenIDConnectAuthorization = "/api/oidc/authorize" pathOpenIDConnectToken = "/api/oidc/token" //nolint:gosec // This is not a hard coded credential, it's a path. diff --git a/internal/handlers/handler_oidc_authorize.go b/internal/handlers/handler_oidc_authorization.go similarity index 90% rename from internal/handlers/handler_oidc_authorize.go rename to internal/handlers/handler_oidc_authorization.go index 83fa8dbbb..d03051b33 100644 --- a/internal/handlers/handler_oidc_authorize.go +++ b/internal/handlers/handler_oidc_authorization.go @@ -17,7 +17,7 @@ import ( "github.com/authelia/authelia/internal/utils" ) -func oidcAuthorize(ctx *middlewares.AutheliaCtx, rw http.ResponseWriter, r *http.Request) { +func oidcAuthorization(ctx *middlewares.AutheliaCtx, rw http.ResponseWriter, r *http.Request) { ar, err := ctx.Providers.OpenIDConnect.Fosite.NewAuthorizeRequest(ctx, r) if err != nil { logging.Logger().Errorf("Error occurred in NewAuthorizeRequest: %+v", err) @@ -62,7 +62,7 @@ func oidcAuthorize(ctx *middlewares.AutheliaCtx, rw http.ResponseWriter, r *http return } - issuer, err := ctx.ForwardedProtoHost() + issuer, err := ctx.ExternalRootURL() if err != nil { ctx.Logger.Errorf("Error occurred obtaining issuer: %+v", err) ctx.Providers.OpenIDConnect.Fosite.WriteAuthorizeError(rw, ar, err) @@ -145,7 +145,7 @@ func oidcAuthorizeHandleAuthorizationOrConsentInsufficient( ctx *middlewares.AutheliaCtx, userSession session.UserSession, client *oidc.InternalClient, isAuthInsufficient bool, rw http.ResponseWriter, r *http.Request, ar fosite.AuthorizeRequester) { - forwardedProtoHost, err := ctx.ForwardedProtoHost() + issuer, err := ctx.ExternalRootURL() if err != nil { ctx.Logger.Errorf("%v", err) http.Error(rw, err.Error(), http.StatusBadRequest) @@ -153,7 +153,7 @@ func oidcAuthorizeHandleAuthorizationOrConsentInsufficient( return } - redirectURL := fmt.Sprintf("%s%s", forwardedProtoHost, string(ctx.Request.RequestURI())) + redirectURL := fmt.Sprintf("%s%s", issuer, string(ctx.Request.RequestURI())) ctx.Logger.Debugf("User %s must consent with scopes %s", userSession.Username, strings.Join(ar.GetRequestedScopes(), ", ")) @@ -175,17 +175,9 @@ func oidcAuthorizeHandleAuthorizationOrConsentInsufficient( return } - uri, err := ctx.ForwardedProtoHost() - if err != nil { - ctx.Logger.Errorf("%v", err) - http.Error(rw, err.Error(), http.StatusBadRequest) - - return - } - if isAuthInsufficient { - http.Redirect(rw, r, uri, http.StatusFound) + http.Redirect(rw, r, issuer, http.StatusFound) } else { - http.Redirect(rw, r, fmt.Sprintf("%s/consent", uri), http.StatusFound) + http.Redirect(rw, r, fmt.Sprintf("%s/consent", issuer), http.StatusFound) } } diff --git a/internal/handlers/handler_oidc_introspect.go b/internal/handlers/handler_oidc_introspection.go similarity index 82% rename from internal/handlers/handler_oidc_introspect.go rename to internal/handlers/handler_oidc_introspection.go index 0a46cdb11..0df4a0afe 100644 --- a/internal/handlers/handler_oidc_introspect.go +++ b/internal/handlers/handler_oidc_introspection.go @@ -6,7 +6,7 @@ import ( "github.com/authelia/authelia/internal/middlewares" ) -func oidcIntrospect(ctx *middlewares.AutheliaCtx, rw http.ResponseWriter, req *http.Request) { +func oidcIntrospection(ctx *middlewares.AutheliaCtx, rw http.ResponseWriter, req *http.Request) { oidcSession := newOpenIDSession("") ir, err := ctx.Providers.OpenIDConnect.Fosite.NewIntrospectionRequest(ctx, req, oidcSession) diff --git a/internal/handlers/handler_oidc_revoke.go b/internal/handlers/handler_oidc_revocation.go similarity index 71% rename from internal/handlers/handler_oidc_revoke.go rename to internal/handlers/handler_oidc_revocation.go index ac6f93191..b50e6989e 100644 --- a/internal/handlers/handler_oidc_revoke.go +++ b/internal/handlers/handler_oidc_revocation.go @@ -6,7 +6,7 @@ import ( "github.com/authelia/authelia/internal/middlewares" ) -func oidcRevoke(ctx *middlewares.AutheliaCtx, rw http.ResponseWriter, req *http.Request) { +func oidcRevocation(ctx *middlewares.AutheliaCtx, rw http.ResponseWriter, req *http.Request) { err := ctx.Providers.OpenIDConnect.Fosite.NewRevocationRequest(ctx, req) ctx.Providers.OpenIDConnect.Fosite.WriteRevocationResponse(rw, err) diff --git a/internal/handlers/handler_oidc_token.go b/internal/handlers/handler_oidc_token.go index 03f7925bf..d33d80aeb 100644 --- a/internal/handlers/handler_oidc_token.go +++ b/internal/handlers/handler_oidc_token.go @@ -11,10 +11,10 @@ import ( func oidcToken(ctx *middlewares.AutheliaCtx, rw http.ResponseWriter, req *http.Request) { oidcSession := newOpenIDSession("") - accessRequest, accessReqErr := ctx.Providers.OpenIDConnect.Fosite.NewAccessRequest(ctx, req, oidcSession) - if accessReqErr != nil { - ctx.Logger.Errorf("Error occurred in NewAccessRequest: %+v", accessRequest) - ctx.Providers.OpenIDConnect.Fosite.WriteAccessError(rw, accessRequest, accessReqErr) + accessRequest, err := ctx.Providers.OpenIDConnect.Fosite.NewAccessRequest(ctx, req, oidcSession) + if err != nil { + ctx.Logger.Errorf("Error occurred in NewAccessRequest: %+v", err) + ctx.Providers.OpenIDConnect.Fosite.WriteAccessError(rw, accessRequest, err) return } diff --git a/internal/handlers/handler_oidc_wellknown.go b/internal/handlers/handler_oidc_wellknown.go index 977baa37e..153c60418 100644 --- a/internal/handlers/handler_oidc_wellknown.go +++ b/internal/handlers/handler_oidc_wellknown.go @@ -11,10 +11,9 @@ import ( ) func oidcWellKnown(ctx *middlewares.AutheliaCtx) { - // TODO (james-d-elliott): append the server.path here for path based installs. Also check other instances in OIDC. - issuer, err := ctx.ForwardedProtoHost() + issuer, err := ctx.ExternalRootURL() if err != nil { - ctx.Logger.Errorf("Error occurred in ForwardedProtoHost: %+v", err) + ctx.Logger.Errorf("Error occurred determining OpenID Connect issuer details: %+v", err) ctx.Response.SetStatusCode(fasthttp.StatusBadRequest) return @@ -84,7 +83,7 @@ func oidcWellKnown(ctx *middlewares.AutheliaCtx) { ctx.SetContentType("application/json") if err := json.NewEncoder(ctx).Encode(wellKnown); err != nil { - ctx.Logger.Errorf("Error occurred in json Encode: %+v", err) + ctx.Logger.Errorf("Error occurred in JSON encode: %+v", err) // TODO: Determine if this is the appropriate error code here. ctx.Response.SetStatusCode(fasthttp.StatusInternalServerError) diff --git a/internal/handlers/oidc_register.go b/internal/handlers/oidc_register.go index bb1a51fec..a7096bc44 100644 --- a/internal/handlers/oidc_register.go +++ b/internal/handlers/oidc_register.go @@ -9,7 +9,7 @@ import ( // RegisterOIDC registers the handlers with the fasthttp *router.Router. TODO: Add paths for UserInfo, Flush, Logout. func RegisterOIDC(router *router.Router, middleware middlewares.RequestHandlerBridge) { // TODO: Add OPTIONS handler. - router.GET("/.well-known/openid-configuration", middleware(oidcWellKnown)) + router.GET(pathOpenIDConnectWellKnown, middleware(oidcWellKnown)) router.GET(pathOpenIDConnectConsent, middleware(oidcConsent)) @@ -17,16 +17,16 @@ func RegisterOIDC(router *router.Router, middleware middlewares.RequestHandlerBr router.GET(pathOpenIDConnectJWKs, middleware(oidcJWKs)) - router.GET(pathOpenIDConnectAuthorization, middleware(middlewares.NewHTTPToAutheliaHandlerAdaptor(oidcAuthorize))) + router.GET(pathOpenIDConnectAuthorization, middleware(middlewares.NewHTTPToAutheliaHandlerAdaptor(oidcAuthorization))) // TODO: Add OPTIONS handler. router.POST(pathOpenIDConnectToken, middleware(middlewares.NewHTTPToAutheliaHandlerAdaptor(oidcToken))) - router.POST(pathOpenIDConnectIntrospection, middleware(middlewares.NewHTTPToAutheliaHandlerAdaptor(oidcIntrospect))) + router.POST(pathOpenIDConnectIntrospection, middleware(middlewares.NewHTTPToAutheliaHandlerAdaptor(oidcIntrospection))) router.GET(pathOpenIDConnectUserinfo, middleware(middlewares.NewHTTPToAutheliaHandlerAdaptor(oidcUserinfo))) router.POST(pathOpenIDConnectUserinfo, middleware(middlewares.NewHTTPToAutheliaHandlerAdaptor(oidcUserinfo))) // TODO: Add OPTIONS handler. - router.POST(pathOpenIDConnectRevocation, middleware(middlewares.NewHTTPToAutheliaHandlerAdaptor(oidcRevoke))) + router.POST(pathOpenIDConnectRevocation, middleware(middlewares.NewHTTPToAutheliaHandlerAdaptor(oidcRevocation))) } diff --git a/internal/handlers/response.go b/internal/handlers/response.go index c4b94058e..9eaeec605 100644 --- a/internal/handlers/response.go +++ b/internal/handlers/response.go @@ -22,7 +22,7 @@ func handleOIDCWorkflowResponse(ctx *middlewares.AutheliaCtx) { return } - uri, err := ctx.ForwardedProtoHost() + uri, err := ctx.ExternalRootURL() if err != nil { ctx.Logger.Errorf("%v", err) handleAuthenticationUnauthorized(ctx, fmt.Errorf("Unable to get forward facing URI"), messageAuthenticationFailed) diff --git a/internal/middlewares/authelia_context.go b/internal/middlewares/authelia_context.go index e638886bb..5444f9b77 100644 --- a/internal/middlewares/authelia_context.go +++ b/internal/middlewares/authelia_context.go @@ -5,6 +5,7 @@ import ( "fmt" "net" "net/url" + "path" "strings" "github.com/asaskevich/govalidator" @@ -113,22 +114,41 @@ func (c *AutheliaCtx) XForwardedURI() []byte { return c.RequestCtx.Request.Header.Peek(headerXForwardedURI) } -// ForwardedProtoHost gets the X-Forwarded-Proto and X-Forwarded-Host headers and forms them into a URL. -func (c AutheliaCtx) ForwardedProtoHost() (string, error) { - XForwardedProto := c.XForwardedProto() +// BasePath returns the base_url as per the path visited by the client. +func (c *AutheliaCtx) BasePath() (base string) { + if baseURL := c.UserValue("base_url"); baseURL != nil { + return baseURL.(string) + } - if XForwardedProto == nil { + return base +} + +// ExternalRootURL gets the X-Forwarded-Proto, X-Forwarded-Host headers and the BasePath and forms them into a URL. +func (c *AutheliaCtx) ExternalRootURL() (string, error) { + protocol := c.XForwardedProto() + if protocol == nil { return "", errMissingXForwardedProto } - XForwardedHost := c.XForwardedHost() - - if XForwardedHost == nil { + host := c.XForwardedHost() + if host == nil { return "", errMissingXForwardedHost } - return fmt.Sprintf("%s://%s", XForwardedProto, - XForwardedHost), nil + externalRootURL := fmt.Sprintf("%s://%s", protocol, host) + + if base := c.BasePath(); base != "" { + externalBaseURL, err := url.Parse(externalRootURL) + if err != nil { + return "", err + } + + externalBaseURL.Path = path.Join(externalBaseURL.Path, base) + + return externalBaseURL.String(), nil + } + + return externalRootURL, nil } // XOriginalURL return the content of the X-Original-URL header. diff --git a/internal/middlewares/errors.go b/internal/middlewares/errors.go index 79789b888..d1d892b80 100644 --- a/internal/middlewares/errors.go +++ b/internal/middlewares/errors.go @@ -2,12 +2,5 @@ package middlewares import "errors" -// InternalError is the error message sent when there was an internal error but it should -// be hidden to the end user. In that case the error should be in the server logs. -const InternalError = "Internal error." - -// UnauthorizedError is the error message sent when the user is not authorized. -const UnauthorizedError = "You're not authorized." - var errMissingXForwardedHost = errors.New("Missing header X-Forwarded-Host") var errMissingXForwardedProto = errors.New("Missing header X-Forwarded-Proto") diff --git a/internal/middlewares/identity_verification.go b/internal/middlewares/identity_verification.go index d5f24ff29..d52babca1 100644 --- a/internal/middlewares/identity_verification.go +++ b/internal/middlewares/identity_verification.go @@ -51,13 +51,13 @@ func IdentityVerificationStart(args IdentityVerificationStartArgs) RequestHandle return } - uri, err := ctx.ForwardedProtoHost() + uri, err := ctx.ExternalRootURL() if err != nil { ctx.Error(err, messageOperationFailed) return } - link := fmt.Sprintf("%s%s%s?token=%s", uri, ctx.Configuration.Server.Path, args.TargetEndpoint, ss) + link := fmt.Sprintf("%s%s?token=%s", uri, args.TargetEndpoint, ss) bufHTML := new(bytes.Buffer) diff --git a/internal/middlewares/strip_path.go b/internal/middlewares/strip_path.go index 7d82ed271..2bddcb55f 100644 --- a/internal/middlewares/strip_path.go +++ b/internal/middlewares/strip_path.go @@ -1,20 +1,21 @@ package middlewares import ( - "bytes" + "strings" "github.com/valyala/fasthttp" ) // StripPathMiddleware strips the first level of a path. -func StripPathMiddleware(next fasthttp.RequestHandler) fasthttp.RequestHandler { +func StripPathMiddleware(path string, next fasthttp.RequestHandler) fasthttp.RequestHandler { return func(ctx *fasthttp.RequestCtx) { - uri := ctx.Request.RequestURI() - n := bytes.IndexByte(uri[1:], '/') + uri := ctx.RequestURI() - if n >= 0 { - uri = uri[n+1:] - ctx.Request.SetRequestURI(string(uri)) + if strings.HasPrefix(string(uri), path) { + ctx.SetUserValue("base_url", path) + + newURI := strings.TrimPrefix(string(uri), path) + ctx.Request.SetRequestURI(newURI) } next(ctx) diff --git a/internal/server/server.go b/internal/server/server.go index a12eb850e..d40905791 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -37,9 +37,9 @@ func registerRoutes(configuration schema.Configuration, providers middlewares.Pr embeddedFS := fasthttpadaptor.NewFastHTTPHandler(http.FileServer(http.FS(embeddedPath))) rootFiles := []string{"favicon.ico", "manifest.json", "robots.txt"} - serveIndexHandler := ServeTemplatedFile(embeddedAssets, indexFile, configuration.Server.Path, rememberMe, resetPassword, configuration.Session.Name, configuration.Theme) - serveSwaggerHandler := ServeTemplatedFile(swaggerAssets, indexFile, configuration.Server.Path, rememberMe, resetPassword, configuration.Session.Name, configuration.Theme) - serveSwaggerAPIHandler := ServeTemplatedFile(swaggerAssets, apiFile, configuration.Server.Path, rememberMe, resetPassword, configuration.Session.Name, configuration.Theme) + serveIndexHandler := ServeTemplatedFile(embeddedAssets, indexFile, rememberMe, resetPassword, configuration.Session.Name, configuration.Theme) + serveSwaggerHandler := ServeTemplatedFile(swaggerAssets, indexFile, rememberMe, resetPassword, configuration.Session.Name, configuration.Theme) + serveSwaggerAPIHandler := ServeTemplatedFile(swaggerAssets, apiFile, rememberMe, resetPassword, configuration.Session.Name, configuration.Theme) r := router.New() r.GET("/", serveIndexHandler) @@ -143,7 +143,7 @@ func registerRoutes(configuration schema.Configuration, providers middlewares.Pr handler := middlewares.LogRequestMiddleware(r.Handler) if configuration.Server.Path != "" { - handler = middlewares.StripPathMiddleware(handler) + handler = middlewares.StripPathMiddleware(configuration.Server.Path, handler) } if providers.OpenIDConnect.Fosite != nil { @@ -196,14 +196,23 @@ func Start(configuration schema.Configuration, providers middlewares.Providers) logger.Fatalf("Could not configure healthcheck: %v", err) } - logger.Infof("Listening for TLS connections on %s%s", addrPattern, configuration.Server.Path) + if configuration.Server.Path == "" { + logger.Infof("Listening for TLS connections on '%s' path '/'", addrPattern) + } else { + logger.Infof("Listening for TLS connections on '%s' paths '/' and '%s'", addrPattern, configuration.Server.Path) + } + logger.Fatal(server.ServeTLS(listener, configuration.Server.TLS.Certificate, configuration.Server.TLS.Key)) } else { if err = writeHealthCheckEnv(configuration.Server.DisableHealthcheck, "http", configuration.Server.Host, configuration.Server.Path, configuration.Server.Port); err != nil { logger.Fatalf("Could not configure healthcheck: %v", err) } - logger.Infof("Listening for non-TLS connections on %s%s", addrPattern, configuration.Server.Path) + if configuration.Server.Path == "" { + logger.Infof("Listening for non-TLS connections on '%s' path '/'", addrPattern) + } else { + logger.Infof("Listening for non-TLS connections on '%s' paths '/' and '%s'", addrPattern, configuration.Server.Path) + } logger.Fatal(server.Serve(listener)) } } diff --git a/internal/server/template.go b/internal/server/template.go index 70c327fd0..b94f4fcc7 100644 --- a/internal/server/template.go +++ b/internal/server/template.go @@ -18,7 +18,7 @@ var alphaNumericRunes = []rune("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUV // ServeTemplatedFile serves a templated version of a specified file, // this is utilised to pass information between the backend and frontend // and generate a nonce to support a restrictive CSP while using material-ui. -func ServeTemplatedFile(publicDir, file, base, rememberMe, resetPassword, session, theme string) fasthttp.RequestHandler { +func ServeTemplatedFile(publicDir, file, rememberMe, resetPassword, session, theme string) fasthttp.RequestHandler { logger := logging.Logger() f, err := assets.Open(publicDir + file) @@ -37,6 +37,11 @@ func ServeTemplatedFile(publicDir, file, base, rememberMe, resetPassword, sessio } return func(ctx *fasthttp.RequestCtx) { + base := "" + if baseURL := ctx.UserValue("base_url"); baseURL != nil { + base = baseURL.(string) + } + nonce := utils.RandomString(32, alphaNumericRunes) switch extension := filepath.Ext(file); extension {