add additional validation to logout token

services/proxy/pkg/command/server.go

@@ -10,6 +10,7 @@ import (
 
 	gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1"
 	"github.com/justinas/alice"
+
 	"github.com/opencloud-eu/opencloud/pkg/config/configlog"
 	"github.com/opencloud-eu/opencloud/pkg/generators"
 	"github.com/opencloud-eu/opencloud/pkg/log"
@@ -72,6 +73,7 @@ func Server(cfg *config.Config) *cobra.Command {
 				microstore.Nodes(cfg.PreSignedURL.SigningKeys.Nodes...),
 				microstore.Database("proxy"),
 				microstore.Table("signing-keys"),
+				store.DisablePersistence(cfg.PreSignedURL.SigningKeys.DisablePersistence),
 				store.Authentication(cfg.PreSignedURL.SigningKeys.AuthUsername, cfg.PreSignedURL.SigningKeys.AuthPassword),
 			)
 

services/proxy/pkg/config/defaults/defaultconfig.go

@@ -45,7 +45,7 @@ func DefaultConfig() *config.Config {
 			AccessTokenVerifyMethod: config.AccessTokenVerificationJWT,
 			SkipUserInfo:            false,
 			UserinfoCache: &config.Cache{
-				Store:    "memory",
+				Store:    "memory", // toDo: check if "memory" is the right default or if "nats-js-kv" is a better match
 				Nodes:    []string{"127.0.0.1:9233"},
 				Database: "cache-userinfo",
 				TTL:      time.Second * 10,

services/proxy/pkg/middleware/oidc_auth.go

@@ -9,13 +9,14 @@ import (
 	"time"
 
 	"github.com/golang-jwt/jwt/v5"
-	"github.com/opencloud-eu/opencloud/pkg/log"
-	"github.com/opencloud-eu/opencloud/pkg/oidc"
 	"github.com/pkg/errors"
 	"github.com/vmihailenco/msgpack/v5"
-	store "go-micro.dev/v4/store"
+	"go-micro.dev/v4/store"
 	"golang.org/x/crypto/sha3"
 	"golang.org/x/oauth2"
+
+	"github.com/opencloud-eu/opencloud/pkg/log"
+	"github.com/opencloud-eu/opencloud/pkg/oidc"
 )
 
 const (
@@ -102,29 +103,41 @@ func (m *OIDCAuthenticator) getClaims(token string, req *http.Request) (map[stri
 	// always set an exp claim
 	claims["exp"] = expiration.Unix()
 	go func() {
-		if d, err := msgpack.Marshal(claims); err != nil {
+		d, err := msgpack.Marshal(claims)
+		if err != nil {
 			m.Logger.Error().Err(err).Msg("failed to marshal claims for userinfo cache")
-		} else {
-			err = m.userInfoCache.Write(&store.Record{
-				Key:    encodedHash,
-				Value:  d,
-				Expiry: time.Until(expiration),
-			})
-			if err != nil {
-				m.Logger.Error().Err(err).Msg("failed to write to userinfo cache")
-			}
+			return
+		}
 
-			if sid := aClaims.SessionID; sid != "" {
-				// reuse user cache for session id lookup
-				err = m.userInfoCache.Write(&store.Record{
-					Key:    sid,
-					Value:  []byte(encodedHash),
-					Expiry: time.Until(expiration),
-				})
-				if err != nil {
-					m.Logger.Error().Err(err).Msg("failed to write session lookup cache")
-				}
-			}
+		if err := m.userInfoCache.Write(&store.Record{
+			Key:    encodedHash,
+			Value:  d,
+			Expiry: time.Until(expiration),
+		}); err != nil {
+			m.Logger.Error().Err(err).Msg("failed to write to userinfo cache")
+		}
+
+		subject, sessionId := aClaims.Subject, aClaims.SessionID
+
+		// toDo:
+		//  - is it possible that the token Subject is empty?
+		//  - is it possible that the token SessionID is empty?
+		var key string
+		switch {
+		case sessionId != "" && subject == "":
+			key = sessionId
+		case sessionId != "" && subject != "":
+			key = strings.Join([]string{subject, sessionId}, ".")
+		default:
+			return
+		}
+
+		if err := m.userInfoCache.Write(&store.Record{
+			Key:    key,
+			Value:  []byte(encodedHash),
+			Expiry: time.Until(expiration),
+		}); err != nil {
+			m.Logger.Error().Err(err).Msg("failed to write session lookup cache")
 		}
 	}()
 

services/proxy/pkg/staticroutes/backchannellogout.go

@@ -2,19 +2,23 @@ package staticroutes
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net/http"
+	"strings"
 
 	"github.com/go-chi/render"
-	"github.com/opencloud-eu/opencloud/pkg/oidc"
-	"github.com/opencloud-eu/reva/v2/pkg/events"
-	"github.com/opencloud-eu/reva/v2/pkg/utils"
-	"github.com/pkg/errors"
 	"github.com/vmihailenco/msgpack/v5"
 	microstore "go-micro.dev/v4/store"
+
+	"github.com/opencloud-eu/reva/v2/pkg/events"
+	"github.com/opencloud-eu/reva/v2/pkg/utils"
 )
 
 // handle backchannel logout requests as per https://openid.net/specs/openid-connect-backchannel-1_0.html#BCRequest
+//
+// toDo:
+//   - keyCloak "Sign out all active sessions" fails to log out, no incoming request
 func (s *StaticRouteHandler) backchannelLogout(w http.ResponseWriter, r *http.Request) {
 	// parse the application/x-www-form-urlencoded POST request
 	logger := s.Logger.SubloggerWithRequestID(r.Context())
@@ -25,6 +29,13 @@ func (s *StaticRouteHandler) backchannelLogout(w http.ResponseWriter, r *http.Re
 		return
 	}
 
+	if r.PostFormValue("logout_token") == "" {
+		logger.Warn().Msg("logout_token is missing")
+		render.Status(r, http.StatusBadRequest)
+		render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: "logout_token is missing"})
+		return
+	}
+
 	logoutToken, err := s.OidcClient.VerifyLogoutToken(r.Context(), r.PostFormValue("logout_token"))
 	if err != nil {
 		logger.Warn().Err(err).Msg("VerifyLogoutToken failed")
@@ -33,60 +44,119 @@ func (s *StaticRouteHandler) backchannelLogout(w http.ResponseWriter, r *http.Re
 		return
 	}
 
-	records, err := s.UserInfoCache.Read(logoutToken.SessionId)
-	if errors.Is(err, microstore.ErrNotFound) || len(records) == 0 {
-		render.Status(r, http.StatusOK)
-		render.JSON(w, r, nil)
+	tokenSessionId := strings.TrimSpace(logoutToken.SessionId)
+	tokenSubject := strings.TrimSpace(logoutToken.Subject)
+
+	// if there is no session id or no subject, something went horribly wrong with the request,
+	// exit and let the caller now.
+	if tokenSessionId == "" && tokenSubject == "" {
+		logger.Warn().Msg("no session id and subject")
+		render.Status(r, http.StatusBadRequest)
+		render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: "invalid logout token"})
 		return
 	}
-	if err != nil {
+
+	// the store has the following key structure:
+	//  - [sessionID].value == claim_key
+	//  - [subject.sessionID].value == claim_key
+	//  - [claim_key].value == claims (msgpack)
+	//
+	// finding the records by sessionID: *.sessionID (suffix)
+	// finding the records by sessionID: sessionID (suffix)
+	// finding the records by subject: subject.* (prefix)
+	//
+	// toDo:
+	//  - is using ReadSuffix and ReadPrefix risky (aka exact match) without having the separator:
+	//    DB/key + "." ... VS DB/key ... - prefix match
+	//    DB/... "." + key VS DB/... + key - suffix match
+	var key string
+	var opts []microstore.ReadOption
+	switch {
+	case tokenSessionId != "":
+		key = tokenSessionId
+		opts = append(opts, microstore.ReadSuffix())
+	case tokenSubject != "":
+		key = tokenSubject
+		opts = append(opts, microstore.ReadPrefix())
+	}
+
+	// search for the related session records
+	//
+	// toDo:
+	//  - memstore needs the ReadLimit... why?
+	sessionRecords, err := s.UserInfoCache.Read(key, append(opts, microstore.ReadLimit(1000))...)
+	switch {
+	case errors.Is(err, microstore.ErrNotFound): // toDo: is it ok to ignore notFound -> http.StatusOK
+		break
+	case err != nil:
 		logger.Error().Err(err).Msg("Error reading userinfo cache")
 		render.Status(r, http.StatusBadRequest)
 		render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: err.Error()})
 		return
 	}
 
-	for _, record := range records {
-		err := s.publishBackchannelLogoutEvent(r.Context(), record, logoutToken)
-		if err != nil {
-			s.Logger.Warn().Err(err).Msg("could not publish backchannel logout event")
+	for _, record := range sessionRecords {
+		var sessionId string
+		switch values := strings.Split(record.Key, "."); {
+		case len(values) == 1: // the record key just contains the session [session]
+			sessionId = values[0]
+		case len(values) == 2: // the record key contains the subject and session [subject.session]
+			sessionId = values[1]
+		default:
+			continue
 		}
-		err = s.UserInfoCache.Delete(string(record.Value))
-		if err != nil && !errors.Is(err, microstore.ErrNotFound) {
-			// Spec requires us to return a 400 BadRequest when the session could not be destroyed
-			logger.Err(err).Msg("could not delete user info from cache")
-			render.Status(r, http.StatusBadRequest)
-			render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: err.Error()})
-			return
+
+		// send backchannel logout event
+		claimId := string(record.Value)
+		if claimId != "" {
+			if err := s.publishBackchannelLogoutEvent(r.Context(), claimId, sessionId); err != nil {
+				s.Logger.Warn().Err(err).Msg("could not publish backchannel logout event")
+			}
 		}
-		logger.Debug().Msg("Deleted userinfo from cache")
-	}
 
-	// we can ignore errors when cleaning up the lookup table
-	err = s.UserInfoCache.Delete(logoutToken.SessionId)
-	if err != nil {
-		logger.Debug().Err(err).Msg("Failed to cleanup sessionid lookup entry")
+		// cleanup cache
+		for _, key := range []string{record.Key, claimId} {
+			err := s.UserInfoCache.Delete(key)
+			isClaimDeletionErr := key == claimId
+			isNotFoundErr := errors.Is(err, microstore.ErrNotFound)
+
+			switch {
+			case isNotFoundErr: // not found errs can be ignored
+				continue
+			case !isClaimDeletionErr: // non claim deletion errs can be ignored
+				continue
+			case err != nil:
+				// Spec requires us to return a 400 BadRequest when the session could not be destroyed
+				// toDo: add link to the spec
+				logger.Err(err).Msg(fmt.Errorf("could not delete session from cache (%s)", key).Error())
+				// We only return on requests that do only attempt to destroy a single session, not multiple
+				render.Status(r, http.StatusBadRequest)
+				render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: err.Error()})
+				return
+			}
+		}
 	}
 
 	render.Status(r, http.StatusOK)
 	render.JSON(w, r, nil)
 }
 
 // publishBackchannelLogoutEvent publishes a backchannel logout event when the callback revived from the identity provider
-func (s StaticRouteHandler) publishBackchannelLogoutEvent(ctx context.Context, record *microstore.Record, logoutToken *oidc.LogoutToken) error {
+func (s StaticRouteHandler) publishBackchannelLogoutEvent(ctx context.Context, claimId, sessionId string) error {
 	if s.EventsPublisher == nil {
 		return fmt.Errorf("the events publisher is not set")
 	}
-	urecords, err := s.UserInfoCache.Read(string(record.Value))
+
+	claimRecord, err := s.UserInfoCache.Read(claimId)
 	if err != nil {
 		return fmt.Errorf("reading userinfo cache: %w", err)
 	}
-	if len(urecords) == 0 {
+	if len(claimRecord) == 0 {
 		return fmt.Errorf("userinfo not found")
 	}
 
 	var claims map[string]interface{}
-	if err = msgpack.Unmarshal(urecords[0].Value, &claims); err != nil {
+	if err = msgpack.Unmarshal(claimRecord[0].Value, &claims); err != nil {
 		return fmt.Errorf("could not unmarshal userinfo: %w", err)
 	}
 
@@ -102,12 +172,13 @@ func (s StaticRouteHandler) publishBackchannelLogoutEvent(ctx context.Context, r
 
 	e := events.BackchannelLogout{
 		Executant: user.GetId(),
-		SessionId: logoutToken.SessionId,
+		SessionId: sessionId,
 		Timestamp: utils.TSNow(),
 	}
 
 	if err := events.Publish(ctx, s.EventsPublisher, e); err != nil {
 		return fmt.Errorf("could not publish user created event %w", err)
 	}
+
 	return nil
 }