Fix: Redis write actions not discarded when a conflict occurs due to optimistic locking

CHANGELOG.md

@@ -11,6 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 - Use golang version 1.23
+- Don't wait for at least one Redis replica anymore when using Redis Sentinel mode
+
+### Fixed
+- Redis write actions not discarded when a conflict occurs due to optimistic locking
 
 ### Security
 - Update go toolchain to 1.23.5

README.md

@@ -111,7 +111,9 @@ You can then start `irma` with the store-type flag set to Redis and the [default
 irma server -vv --store-type redis --redis-addr "localhost:6379" --redis-allow-empty-password --redis-no-tls
 ```
 
-If you use Redis in Sentinel mode for high availability, you need to consider whether you accept the risk of losing session state in case of a failover. Redis does not guarantee [strong consistency](https://redis.io/docs/management/scaling/#redis-cluster-consistency-guarantees) in these setups. We mitigated this by waiting for a write to have reached the master node and at least one replica. This means that at least two replicas should be configured for every master node to achieve high availability. Even then, there is a small chance of losing session state when a replica fails at the same time as the master node. For example, this might be problematic if you want to guarantee that a credential is not issued twice or if you need a session QR to have a long lifetime but you do want the session to be finished soon after the QR is scanned. If you require IRMA sessions to be highly consistent, you should use the default in-memory store or Redis in standalone mode. If you accept this risk, then you can enable Sentinel mode support by setting the `--redis-accept-inconsistency-risk` flag.
+If you use Redis in Sentinel mode for high availability, you need to consider whether you accept the risk of losing session state in case of a failover. Redis does not guarantee [strong consistency](https://redis.io/docs/management/scaling/#redis-cluster-consistency-guarantees) in these setups. This might be problematic if you want to guarantee that a credential is not issued twice or if you need a session QR to have a long lifetime but you do want the session to be finished soon after the QR is scanned. If you require IRMA sessions to be highly consistent, you should use the default in-memory store or Redis in standalone mode. If you accept this risk, then you can enable Sentinel mode support by setting the `--redis-accept-inconsistency-risk` flag.
+
+If you use a managed Redis service from a cloud provider, please be aware that if you enable high availability or cluster mode, they use Redis Cluster or Redis Sentinel under water. The `irma server` does not automatically detect this, so it does not require you to set the `--redis-accept-inconsistency-risk` flag. However, the same inconsistency risks hold here too.
 
 Besides the `irma server`, Redis can also be configured for the `irma keyshare server` and the `irma keyshare myirmaserver` in the same way as described above. Note that the `irma keyshare server` does not become stateless when using Redis, because it stores the keyshare commitments and authentication challenges in memory. These cannot be stored in Redis, because we require this data to be strongly consistent. Instead, you can use sticky sessions to make sure that the same user is always routed to the same keyshare server instance. The stored commitments and challenges are only relevant for a few seconds, so the risk of losing this data is low. The `irma keyshare myirmaserver` does become stateless when using Redis.
 

schemes.go

@@ -79,7 +79,7 @@ type (
 	SchemeFileHash []byte
 
 	// SchemeManagerIndex is a (signed) list of files under a scheme
-	// along with their SHA266 hash
+	// along with their SHA256 hash
 	SchemeManagerIndex map[string]SchemeFileHash
 
 	SchemeManagerStatus string

server/conf.go

@@ -104,8 +104,7 @@ type Configuration struct {
 
 type RedisClient struct {
 	*redis.Client
-	FailoverMode bool
-	KeyPrefix    string
+	KeyPrefix string
 }
 
 type RedisSettings struct {
@@ -498,9 +497,8 @@ func (conf *Configuration) RedisClient() (*RedisClient, error) {
 		keyPrefix = conf.RedisSettings.Username + ":"
 	}
 	conf.redisClient = &RedisClient{
-		Client:       cl,
-		FailoverMode: failoverMode,
-		KeyPrefix:    keyPrefix,
+		Client:    cl,
+		KeyPrefix: keyPrefix,
 	}
 	return conf.redisClient, nil
 }

server/irmaserver/sessions.go

@@ -253,30 +253,21 @@ func (s *redisSessionStore) add(ctx context.Context, session *sessionData) error
 	if ttl <= 0 {
 		return &RedisError{errors.New("session ttl is in the past")}
 	}
-	if err := s.client.Watch(ctx, func(tx *redis.Tx) error {
-		if err := tx.Set(
+	if _, err := s.client.TxPipelined(ctx, func(p redis.Pipeliner) error {
+		if err := p.Set(
 			ctx,
 			s.client.KeyPrefix+requestorTokenLookupPrefix+string(session.RequestorToken),
 			string(session.ClientToken),
 			ttl,
 		).Err(); err != nil {
 			return err
 		}
-		if err := tx.Set(
+		return p.Set(
 			ctx,
 			s.client.KeyPrefix+clientTokenLookupPrefix+string(session.ClientToken),
 			sessionJSON,
 			ttl,
-		).Err(); err != nil {
-			return err
-		}
-
-		if s.client.FailoverMode {
-			if err := s.client.Wait(ctx, 1, time.Second).Err(); err != nil {
-				return err
-			}
-		}
-		return nil
+		).Err()
 	}); err != nil {
 		return &RedisError{err}
 	}
@@ -342,18 +333,13 @@ func (s *redisSessionStore) clientTransaction(ctx context.Context, t irma.Client
 			return errors.New("session ttl is in the past")
 		}
 
-		if err := tx.Set(ctx, s.client.KeyPrefix+clientTokenLookupPrefix+string(t), sessionJSON, ttl).Err(); err != nil {
-			return err
-		}
-		if err := tx.Expire(ctx, s.client.KeyPrefix+requestorTokenLookupPrefix+string(session.RequestorToken), ttl).Err(); err != nil {
-			return err
-		}
-		if s.client.FailoverMode {
-			if err := tx.Wait(ctx, 1, time.Second).Err(); err != nil {
+		_, err = tx.TxPipelined(ctx, func(p redis.Pipeliner) error {
+			if err := p.Set(ctx, s.client.KeyPrefix+clientTokenLookupPrefix+string(t), sessionJSON, ttl).Err(); err != nil {
 				return err
 			}
-		}
-		return nil
+			return p.Expire(ctx, s.client.KeyPrefix+requestorTokenLookupPrefix+string(session.RequestorToken), ttl).Err()
+		})
+		return err
 	})
 	if _, ok := err.(*UnknownSessionError); ok {
 		return err

server/keyshare/myirmaserver/session.go

@@ -94,22 +94,12 @@ func (s *redisSessionStore) add(ctx context.Context, ses session) error {
 	if ttl <= 0 {
 		return errors.New("session expiry time is in the past")
 	}
-	if err := s.client.Watch(ctx, func(tx *redis.Tx) error {
-		if err := tx.Set(
-			ctx,
-			s.client.KeyPrefix+sessionLookupPrefix+ses.Token,
-			string(bytes),
-			ttl,
-		).Err(); err != nil {
-			return err
-		}
-		if s.client.FailoverMode {
-			if err := tx.Wait(ctx, 1, time.Second).Err(); err != nil {
-				return err
-			}
-		}
-		return nil
-	}); err != nil {
+	if err := s.client.Set(
+		ctx,
+		s.client.KeyPrefix+sessionLookupPrefix+ses.Token,
+		string(bytes),
+		ttl,
+	).Err(); err != nil {
 		s.logger.WithError(err).Error("failed to add session")
 		return errRedis
 	}
@@ -145,16 +135,10 @@ func (s *redisSessionStore) update(ctx context.Context, token string, handler fu
 		if ttl <= 0 {
 			return errors.New("session expiry time is in the past")
 		}
-		if err := tx.Set(ctx, key, string(updatedBytes), ttl).Err(); err != nil {
-			return err
-		}
-
-		if s.client.FailoverMode {
-			if err := tx.Wait(ctx, 1, time.Second).Err(); err != nil {
-				return err
-			}
-		}
-		return nil
+		_, err = tx.TxPipelined(ctx, func(p redis.Pipeliner) error {
+			return p.Set(ctx, key, string(updatedBytes), ttl).Err()
+		})
+		return err
 	})
 	if err == errUnknownSession {
 		return err