Fix auth routing logic (cs3org#4266)

* Fixes on the auth interceptors, WIP * bail out on grpc unprotected methods * Expose public endpoints for the gateway * allow only one trailing slash in skip logic * add changelog * fix comments * fix changelog typo --------- Co-authored-by: Hugo Gonzalez Labrador <[email protected]>
labkode · Oct 19, 2023 · fb60c27 · fb60c27
1 parent 99498d0
commit fb60c27
Show file tree

Hide file tree

Showing 5 changed files with 46 additions and 32 deletions.
diff --git a/changelog/unreleased/auth-fixes.md b/changelog/unreleased/auth-fixes.md
@@ -0,0 +1,6 @@
+Enhancement: improve authentication routing logic
+
+Provides a safer approach to route requests, both in HTTP and gRPC land
+when authentication is needed.
+
+https://github.com/cs3org/reva/pull/4266
diff --git a/internal/grpc/interceptors/auth/auth.go b/internal/grpc/interceptors/auth/auth.go
@@ -98,23 +98,10 @@ func NewUnary(m map[string]interface{}, unprotected []string) (grpc.UnaryServerI
 
 		if utils.Skip(info.FullMethod, unprotected) {
 			log.Debug().Str("method", info.FullMethod).Msg("skipping auth")
-
-			// If a token is present, set it anyway, as we might need the user info
-			// to decide the storage provider.
-			tkn, ok := appctx.ContextGetToken(ctx)
-			if ok {
-				u, scopes, err := dismantleToken(ctx, tkn, req, tokenManager, conf.GatewayAddr, true)
-				if err == nil {
-					if blockedUsers.IsBlocked(u.Username) {
-						return nil, status.Errorf(codes.PermissionDenied, "user %s blocked", u.Username)
-					}
-					ctx = appctx.ContextSetUser(ctx, u)
-					ctx = appctx.ContextSetScopes(ctx, scopes)
-				}
-			}
 			return handler(ctx, req)
 		}
 
+		// from this point on, the method must be authenticated
 		tkn, ok := appctx.ContextGetToken(ctx)
 
 		if !ok || tkn == "" {
@@ -137,6 +124,7 @@ func NewUnary(m map[string]interface{}, unprotected []string) (grpc.UnaryServerI
 		ctx = appctx.ContextSetScopes(ctx, scopes)
 		return handler(ctx, req)
 	}
+
 	return interceptor, nil
 }
 
@@ -171,19 +159,6 @@ func NewStream(m map[string]interface{}, unprotected []string) (grpc.StreamServe
 
 		if utils.Skip(info.FullMethod, unprotected) {
 			log.Debug().Str("method", info.FullMethod).Msg("skipping auth")
-
-			// If a token is present, set it anyway, as we might need the user info
-			// to decide the storage provider.
-			tkn, ok := appctx.ContextGetToken(ctx)
-			if ok {
-				u, scopes, err := dismantleToken(ctx, tkn, ss, tokenManager, conf.GatewayAddr, true)
-				if err == nil {
-					ctx = appctx.ContextSetUser(ctx, u)
-					ctx = appctx.ContextSetScopes(ctx, scopes)
-					ss = newWrappedServerStream(ctx, ss)
-				}
-			}
-
 			return handler(srv, ss)
 		}
 

diff --git a/internal/grpc/services/gateway/gateway.go b/internal/grpc/services/gateway/gateway.go
@@ -170,7 +170,42 @@ func (s *svc) Close() error {
 }
 
 func (s *svc) UnprotectedEndpoints() []string {
-	return []string{"/cs3.gateway.v1beta1.GatewayAPI"}
+	return []string{
+		"/cs3.gateway.v1beta1.GatewayAPI/ListShare",
+		"/cs3.gateway.v1beta1.GatewayAPI/GetAppPassword",
+		"/cs3.gateway.v1beta1.GatewayAPI/AddAppProvider",
+		"/cs3.gateway.v1beta1.GatewayAPI/ListSupportedMimeTypes",
+		"/cs3.gateway.v1beta1.GatewayAPI/Authenticate",
+		"/cs3.gateway.v1beta1.GatewayAPI/GetAuthProvider",
+		"/cs3.gateway.v1beta1.GatewayAPI/ListAuthProviders",
+		"/cs3.gateway.v1beta1.GatewayAPI/CreateOCMCoreShare",
+		"/cs3.gateway.v1beta1.GatewayAPI/AcceptInvite",
+		"/cs3.gateway.v1beta1.GatewayAPI/GetAcceptedUser",
+		"/cs3.gateway.v1beta1.GatewayAPI/IsProviderAllowed",
+		"/cs3.gateway.v1beta1.GatewayAPI/ListAllProviders",
+		"/cs3.gateway.v1beta1.GatewayAPI/GetOCMShareByToken",
+		"/cs3.gateway.v1beta1.GatewayAPI/GetPublicShareByToken",
+		"/cs3.gateway.v1beta1.GatewayAPI/GetUser",
+		"/cs3.gateway.v1beta1.GatewayAPI/GetUserByClaim",
+		"/cs3.gateway.v1beta1.GatewayAPI/GetUserGroups",
+
+		"/cs3.auth.applications.v1beta1.ApplicationsAPI/GetAppPassword",
+		"/cs3.app.registry.v1beta1.RegistryAPI/AddAppProvider",
+		"/cs3.app.registry.v1beta1.RegistryAPI/ListSupportedMimeTypes",
+		"/cs3.auth.provider.v1beta1.ProviderAPI/Authenticate",
+		"/cs3.auth.registry.v1beta1.RegistryAPI/GetAuthProvider",
+		"/cs3.auth.registry.v1beta1.RegistryAPI/ListAuthProviders",
+		"/cs3.ocm.core.v1beta1.OcmCoreAPI/CreateOCMCoreShare",
+		"/cs3.ocm.invite.v1beta1.InviteAPI/AcceptInvite",
+		"/cs3.ocm.invite.v1beta1.InviteAPI/GetAcceptedUser",
+		"/cs3.ocm.provider.v1beta1.ProviderAPI/IsProviderAllowed",
+		"/cs3.ocm.provider.v1beta1.ProviderAPI/ListAllProviders",
+		"/cs3.sharing.ocm.v1beta1.OcmAPI/GetOCMShareByToken",
+		"/cs3.sharing.link.v1beta1.LinkAPI/GetPublicShareByToken",
+		"/cs3.identity.user.v1beta1.UserAPI/GetUser",
+		"/cs3.identity.user.v1beta1.UserAPI/GetUserByClaim",
+		"/cs3.identity.user.v1beta1.UserAPI/GetUserGroups",
+	}
 }
 
 func getTokenManager(manager string, m map[string]map[string]interface{}) (token.Manager, error) {

diff --git a/internal/http/interceptors/auth/auth.go b/internal/http/interceptors/auth/auth.go
@@ -168,10 +168,8 @@ func New(m map[string]interface{}, unprotected []string) (global.Middleware, err
 
 			log := appctx.GetLogger(r.Context())
 
-			// For unprotected URLs, we try to authenticate the request in case some service needs it,
-			// but don't return any errors if it fails.
 			if utils.Skip(r.URL.Path, unprotected) {
-				log.Info().Msg("skipping auth check for: " + r.URL.Path)
+				log.Info().Interface("unprotected", unprotected).Msg("skipping auth check for: " + r.URL.Path)
 			} else {
 				ctx, err := authenticateUser(w, r, conf, tokenStrategyChain, tokenManager, tokenWriter, credChain, false)
 				if err != nil {

diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go
@@ -59,7 +59,7 @@ var (
 // i.e: /a/b/c/d/e contains prefix /a/b/c.
 func Skip(source string, prefixes []string) bool {
 	for i := range prefixes {
-		if strings.HasPrefix(source, prefixes[i]) {
+		if strings.HasPrefix(path.Join(source, "/"), path.Join(prefixes[i], "/")) {
 			return true
 		}
 	}