[full-ci] Rewrite of the authentication middleware

changelog/unreleased/rewrite-authentication.md

@@ -0,0 +1,7 @@
+Enhancement: Rewrite of the request authentication middleware
+
+There were some flaws in the authentication middleware which were resolved by this rewrite.
+This rewrite also introduced the need to manually mark certain paths as "unprotected" if
+requests to these paths must not be authenticated.
+
+https://github.com/owncloud/ocis/pull/4374

services/proxy/pkg/command/server.go

@@ -166,6 +166,43 @@ func loadMiddlewares(ctx context.Context, logger log.Logger, cfg *config.Config)
 		Timeout: time.Second * 10,
 	}
 
+	var authenticators []middleware.Authenticator
+	if cfg.EnableBasicAuth {
+		logger.Warn().Msg("basic auth enabled, use only for testing or development")
+		authenticators = append(authenticators, middleware.BasicAuthenticator{
+			Logger:       logger,
+			UserProvider: userProvider,
+		})
+	}
+	authenticators = append(authenticators, middleware.NewOIDCAuthenticator(
+		logger,
+		cfg.OIDC.UserinfoCache.TTL,
+		oidcHTTPClient,
+		cfg.OIDC.Issuer,
+		func() (middleware.OIDCProvider, error) {
+			// Initialize a provider by specifying the issuer URL.
+			// it will fetch the keys from the issuer using the .well-known
+			// endpoint
+			return oidc.NewProvider(
+				context.WithValue(ctx, oauth2.HTTPClient, oidcHTTPClient),
+				cfg.OIDC.Issuer,
+			)
+		},
+		cfg.OIDC.JWKS,
+		cfg.OIDC.AccessTokenVerifyMethod,
+	))
+	authenticators = append(authenticators, middleware.PublicShareAuthenticator{
+		Logger:            logger,
+		RevaGatewayClient: revaClient,
+	})
+
+	authenticators = append(authenticators, middleware.SignedURLAuthenticator{
+		Logger:             logger,
+		PreSignedURLConfig: cfg.PreSignedURL,
+		UserProvider:       userProvider,
+		Store:              storeClient,
+	})
+
 	return alice.New(
 		// first make sure we log all requests and redirect to https if necessary
 		pkgmiddleware.TraceContext,
@@ -179,38 +216,12 @@ func loadMiddlewares(ctx context.Context, logger log.Logger, cfg *config.Config)
 			oidcHTTPClient,
 		),
 
-		// now that we established the basics, on with authentication middleware
 		middleware.Authentication(
-			// OIDC Options
-			middleware.OIDCProviderFunc(func() (middleware.OIDCProvider, error) {
-				// Initialize a provider by specifying the issuer URL.
-				// it will fetch the keys from the issuer using the .well-known
-				// endpoint
-				return oidc.NewProvider(
-					context.WithValue(ctx, oauth2.HTTPClient, oidcHTTPClient),
-					cfg.OIDC.Issuer,
-				)
-			}),
-			middleware.HTTPClient(oidcHTTPClient),
-			middleware.TokenCacheSize(cfg.OIDC.UserinfoCache.Size),
-			middleware.TokenCacheTTL(time.Second*time.Duration(cfg.OIDC.UserinfoCache.TTL)),
-			middleware.AccessTokenVerifyMethod(cfg.OIDC.AccessTokenVerifyMethod),
-			middleware.JWKSOptions(cfg.OIDC.JWKS),
-
-			// basic Options
-			middleware.Logger(logger),
-			middleware.EnableBasicAuth(cfg.EnableBasicAuth),
-			middleware.UserProvider(userProvider),
-			middleware.OIDCIss(cfg.OIDC.Issuer),
-			middleware.UserOIDCClaim(cfg.UserOIDCClaim),
-			middleware.UserCS3Claim(cfg.UserCS3Claim),
+			authenticators,
 			middleware.CredentialsByUserAgent(cfg.AuthMiddleware.CredentialsByUserAgent),
-		),
-		middleware.SignedURLAuth(
 			middleware.Logger(logger),
-			middleware.PreSignedURLConfig(cfg.PreSignedURL),
-			middleware.UserProvider(userProvider),
-			middleware.Store(storeClient),
+			middleware.OIDCIss(cfg.OIDC.Issuer),
+			middleware.EnableBasicAuth(cfg.EnableBasicAuth),
 		),
 		middleware.AccountResolver(
 			middleware.Logger(logger),
@@ -233,9 +244,5 @@ func loadMiddlewares(ctx context.Context, logger log.Logger, cfg *config.Config)
 			middleware.TokenManagerConfig(*cfg.TokenManager),
 			middleware.RevaGatewayClient(revaClient),
 		),
-		middleware.PublicShareAuth(
-			middleware.Logger(logger),
-			middleware.RevaGatewayClient(revaClient),
-		),
 	)
 }

services/proxy/pkg/middleware/authentication.go

@@ -5,55 +5,165 @@ import (
 	"net/http"
 	"regexp"
 	"strings"
+
+	"github.com/owncloud/ocis/v2/services/proxy/pkg/webdav"
 )
 
 var (
 	// SupportedAuthStrategies stores configured challenges.
 	SupportedAuthStrategies []string
 
 	// ProxyWwwAuthenticate is a list of endpoints that do not rely on reva underlying authentication, such as ocs.
-	// services that fallback to reva authentication are declared in the "frontend" command on oCIS. It is a list of strings
-	// to be regexp compiled.
-	ProxyWwwAuthenticate = []string{"/ocs/v[12].php/cloud/"}
+	// services that fallback to reva authentication are declared in the "frontend" command on oCIS. It is a list of
+	// regexp.Regexp which are safe to use concurrently.
+	ProxyWwwAuthenticate = []regexp.Regexp{*regexp.MustCompile("/ocs/v[12].php/cloud/")}
+
+	_publicPaths = [...]string{
+		"/dav/public-files/",
+		"/remote.php/dav/public-files/",
+		"/remote.php/ocs/apps/files_sharing/api/v1/tokeninfo/unprotected",
+		"/ocs/v1.php/cloud/capabilities",
+	}
+	// _unprotectedPaths contains paths which don't need to be authenticated.
+	_unprotectedPaths = map[string]struct{}{
+		"/":                    {},
+		"/login":               {},
+		"/app/list":            {},
+		"/config.json":         {},
+		"/manifest.json":       {},
+		"/oidc-callback.html":  {},
+		"/oidc-callback":       {},
+		"/settings.js":         {},
+		"/data":                {},
+		"/konnect/v1/userinfo": {},
+		"/status.php":          {},
+		"/favicon.ico":         {},
+		"/ocs/v1.php/config":   {},
+		"/ocs/v2.php/config":   {},
+	}
+	// _unprotectedPathPrefixes contains paths which don't need to be authenticated.
+	_unprotectedPathPrefixes = [...]string{
+		"/files",
+		"/data",
+		"/account",
+		"/s/",
+		"/external/spaces",
+		"/apps/openidconnect/redirect",
+		"/settings",
+		"/user-management",
+		"/.well-known",
+		"/js",
+		"/css",
+		"/icons",
+		"/themes",
+		"/signin",
+		"/konnect",
+	}
+)
 
-	// WWWAuthenticate captures the Www-Authenticate header string.
-	WWWAuthenticate = "Www-Authenticate"
+const (
+	// WwwAuthenticate captures the Www-Authenticate header string.
+	WwwAuthenticate = "Www-Authenticate"
 )
 
-// userAgentLocker aids in dependency injection for helper methods. The set of fields is arbitrary and the only relation
-// they share is to fulfill their duty and lock a User-Agent to its correct challenge if configured.
-type userAgentLocker struct {
-	w        http.ResponseWriter
-	r        *http.Request
-	locks    map[string]string // locks represents a reva user-agent:challenge mapping.
-	fallback string
+// Authenticator is the common interface implemented by all request authenticators.
+
+type Authenticator interface {
+	// Authenticate is used to authenticate incoming HTTP requests.
+	// The Authenticator may augment the request with user info or anything related to the
+	// authentication and return the augmented request.
+	Authenticate(*http.Request) (*http.Request, bool)
 }
 
 // Authentication is a higher order authentication middleware.
-func Authentication(opts ...Option) func(next http.Handler) http.Handler {
+func Authentication(auths []Authenticator, opts ...Option) func(next http.Handler) http.Handler {
 	options := newOptions(opts...)
-
 	configureSupportedChallenges(options)
-	oidc := newOIDCAuth(options)
-	basic := newBasicAuth(options)
 
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			if options.OIDCIss != "" && options.EnableBasicAuth {
-				oidc(basic(next)).ServeHTTP(w, r)
+			if isOIDCTokenAuth(r) || isUnprotectedPath(r) {
+				// The authentication for this request is handled by the IdP.
+				next.ServeHTTP(w, r)
+				return
 			}
 
-			if options.OIDCIss != "" && !options.EnableBasicAuth {
-				oidc(next).ServeHTTP(w, r)
+			for _, a := range auths {
+				if req, ok := a.Authenticate(r); ok {
+					next.ServeHTTP(w, req)
+					return
+				}
+			}
+			if !isPublicPath(r.URL.Path) {
+				// Failed basic authentication attempts receive the Www-Authenticate header in the response
+				var touch bool
+				for k, v := range options.CredentialsByUserAgent {
+					if strings.Contains(k, r.UserAgent()) {
+						removeSuperfluousAuthenticate(w)
+						w.Header().Add("Www-Authenticate", fmt.Sprintf("%v realm=\"%s\", charset=\"UTF-8\"", strings.Title(v), r.Host))
+						touch = true
+						break
+					}
+				}
+
+				// if the request is not bound to any user agent, write all available challenges
+				if !touch &&
+					// This is a temporary hack... Before the authentication middleware rewrite all
+					// unauthenticated requests were still handled. The reva http services then did add
+					// the supported authentication headers to the response. Since we are not allowing the
+					// requests to continue so far we have to do it here. But we shouldn't do it for the graph service.
+					// That's the reason for this hard check here.
+					!strings.HasPrefix(r.URL.Path, "/graph") {
+					writeSupportedAuthenticateHeader(w, r)
+				}
 			}
 
-			if options.OIDCIss == "" && options.EnableBasicAuth {
-				basic(next).ServeHTTP(w, r)
+			for _, s := range SupportedAuthStrategies {
+				userAgentAuthenticateLockIn(w, r, options.CredentialsByUserAgent, s)
+			}
+			w.WriteHeader(http.StatusUnauthorized)
+			// if the request is a PROPFIND return a WebDAV error code.
+			// TODO: The proxy has to be smart enough to detect when a request is directed towards a webdav server
+			// and react accordingly.
+			if webdav.IsWebdavRequest(r) {
+				b, err := webdav.Marshal(webdav.Exception{
+					Code:    webdav.SabredavPermissionDenied,
+					Message: "Authentication error",
+				})
+
+				webdav.HandleWebdavError(w, b, err)
 			}
 		})
 	}
 }
 
+// The token auth endpoint uses basic auth for clients, see https://openid.net/specs/openid-connect-basic-1_0.html#TokenRequest
+// > The Client MUST authenticate to the Token Endpoint using the HTTP Basic method, as described in 2.3.1 of OAuth 2.0.
+func isOIDCTokenAuth(req *http.Request) bool {
+	return req.URL.Path == "/konnect/v1/token"
+}
+
+func isUnprotectedPath(r *http.Request) bool {
+	if _, ok := _unprotectedPaths[r.URL.Path]; ok {
+		return true
+	}
+	for _, p := range _unprotectedPathPrefixes {
+		if strings.HasPrefix(r.URL.Path, p) {
+			return true
+		}
+	}
+	return false
+}
+
+func isPublicPath(p string) bool {
+	for _, pp := range _publicPaths {
+		if strings.HasPrefix(p, pp) {
+			return true
+		}
+	}
+	return false
+}
+
 // configureSupportedChallenges adds known authentication challenges to the current session.
 func configureSupportedChallenges(options Options) {
 	if options.OIDCIss != "" {
@@ -66,13 +176,22 @@ func configureSupportedChallenges(options Options) {
 }
 
 func writeSupportedAuthenticateHeader(w http.ResponseWriter, r *http.Request) {
-	for i := 0; i < len(SupportedAuthStrategies); i++ {
-		w.Header().Add(WWWAuthenticate, fmt.Sprintf("%v realm=\"%s\", charset=\"UTF-8\"", strings.Title(SupportedAuthStrategies[i]), r.Host))
+	for _, s := range SupportedAuthStrategies {
+		w.Header().Add(WwwAuthenticate, fmt.Sprintf("%v realm=\"%s\", charset=\"UTF-8\"", strings.Title(s), r.Host))
 	}
 }
 
 func removeSuperfluousAuthenticate(w http.ResponseWriter) {
-	w.Header().Del(WWWAuthenticate)
+	w.Header().Del(WwwAuthenticate)
+}
+
+// userAgentLocker aids in dependency injection for helper methods. The set of fields is arbitrary and the only relation
+// they share is to fulfill their duty and lock a User-Agent to its correct challenge if configured.
+type userAgentLocker struct {
+	w        http.ResponseWriter
+	r        *http.Request
+	locks    map[string]string // locks represents a reva user-agent:challenge mapping.
+	fallback string
 }
 
 // userAgentAuthenticateLockIn sets Www-Authenticate according to configured user agents. This is useful for the case of
@@ -86,49 +205,21 @@ func userAgentAuthenticateLockIn(w http.ResponseWriter, r *http.Request, locks m
 		fallback: fallback,
 	}
 
-	for i := 0; i < len(ProxyWwwAuthenticate); i++ {
-		evalRequestURI(&u, i)
+	for _, r := range ProxyWwwAuthenticate {
+		evalRequestURI(u, r)
 	}
 }
 
-func evalRequestURI(l *userAgentLocker, i int) {
-	r := regexp.MustCompile(ProxyWwwAuthenticate[i])
-	if r.Match([]byte(l.r.RequestURI)) {
-		for k, v := range l.locks {
-			if strings.Contains(k, l.r.UserAgent()) {
-				removeSuperfluousAuthenticate(l.w)
-				l.w.Header().Add(WWWAuthenticate, fmt.Sprintf("%v realm=\"%s\", charset=\"UTF-8\"", strings.Title(v), l.r.Host))
-				return
-			}
+func evalRequestURI(l userAgentLocker, r regexp.Regexp) {
+	if !r.MatchString(l.r.RequestURI) {
+		return
+	}
+	for k, v := range l.locks {
+		if strings.Contains(k, l.r.UserAgent()) {
+			removeSuperfluousAuthenticate(l.w)
+			l.w.Header().Add(WwwAuthenticate, fmt.Sprintf("%v realm=\"%s\", charset=\"UTF-8\"", strings.Title(v), l.r.Host))
+			return
 		}
-		l.w.Header().Add(WWWAuthenticate, fmt.Sprintf("%v realm=\"%s\", charset=\"UTF-8\"", strings.Title(l.fallback), l.r.Host))
 	}
-}
-
-// newOIDCAuth returns a configured oidc middleware
-func newOIDCAuth(options Options) func(http.Handler) http.Handler {
-	return OIDCAuth(
-		Logger(options.Logger),
-		OIDCProviderFunc(options.OIDCProviderFunc),
-		HTTPClient(options.HTTPClient),
-		OIDCIss(options.OIDCIss),
-		TokenCacheSize(options.UserinfoCacheSize),
-		TokenCacheTTL(options.UserinfoCacheTTL),
-		CredentialsByUserAgent(options.CredentialsByUserAgent),
-		AccessTokenVerifyMethod(options.AccessTokenVerifyMethod),
-		JWKSOptions(options.JWKS),
-	)
-}
-
-// newBasicAuth returns a configured basic middleware
-func newBasicAuth(options Options) func(http.Handler) http.Handler {
-	return BasicAuth(
-		UserProvider(options.UserProvider),
-		Logger(options.Logger),
-		EnableBasicAuth(options.EnableBasicAuth),
-		OIDCIss(options.OIDCIss),
-		UserOIDCClaim(options.UserOIDCClaim),
-		UserCS3Claim(options.UserCS3Claim),
-		CredentialsByUserAgent(options.CredentialsByUserAgent),
-	)
+	l.w.Header().Add(WwwAuthenticate, fmt.Sprintf("%v realm=\"%s\", charset=\"UTF-8\"", strings.Title(l.fallback), l.r.Host))
 }

services/proxy/pkg/middleware/authentication_test.go

@@ -0,0 +1,19 @@
+package middleware
+
+import (
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+var _ = Describe("authentication helpers", func() {
+	DescribeTable("isPublicPath should recognize public paths",
+		func(input string, expected bool) {
+			isPublic := isPublicPath(input)
+			Expect(isPublic).To(Equal(expected))
+		},
+		Entry("public files path", "/remote.php/dav/public-files/", true),
+		Entry("public files path without remote.php", "/remote.php/dav/public-files/", true),
+		Entry("token info path", "/remote.php/ocs/apps/files_sharing/api/v1/tokeninfo/unprotected", true),
+		Entry("capabilities", "/ocs/v1.php/cloud/capabilities", true),
+	)
+})