Protect user_interactive reads and writes with locks (#2635)

* Protect user_interactive reads and writes with locks

* Ignore golangci-lint false positive

* fix lint

Co-authored-by: Tak Wai Wong <tak@hntlabs.com>

.github/workflows/dendrite.yml

@@ -19,10 +19,10 @@ jobs:
     runs-on: ubuntu-latest
     if: ${{ false }} # disable for now
     steps:
-      - uses: actions/checkout@v2
+      - uses: actions/checkout@v3
 
       - name: Install Go
-        uses: actions/setup-go@v2
+        uses: actions/setup-go@v3
         with:
           go-version: 1.18
 
@@ -66,6 +66,10 @@ jobs:
     runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@v3
+      - name: Install Go
+        uses: actions/setup-go@v3
+        with:
+          go-version: 1.18
       - name: golangci-lint
         uses: golangci/golangci-lint-action@v3
 
@@ -101,7 +105,7 @@ jobs:
     steps:
       - uses: actions/checkout@v3
       - name: Setup go
-        uses: actions/setup-go@v2
+        uses: actions/setup-go@v3
         with:
           go-version: ${{ matrix.go }}
       - uses: actions/cache@v3
@@ -133,7 +137,7 @@ jobs:
     steps:
       - uses: actions/checkout@v3
       - name: Setup go
-        uses: actions/setup-go@v2
+        uses: actions/setup-go@v3
         with:
           go-version: ${{ matrix.go }}
       - name: Install dependencies x86
@@ -167,7 +171,7 @@ jobs:
     steps:
       - uses: actions/checkout@v3
       - name: Setup Go ${{ matrix.go }}
-        uses: actions/setup-go@v2
+        uses: actions/setup-go@v3
         with:
           go-version: ${{ matrix.go }}
       - name: Install dependencies
@@ -208,7 +212,7 @@ jobs:
     steps:
       - uses: actions/checkout@v3
       - name: Setup go
-        uses: actions/setup-go@v2
+        uses: actions/setup-go@v3
         with:
           go-version: "1.18"
       - uses: actions/cache@v3
@@ -233,7 +237,7 @@ jobs:
     steps:
       - uses: actions/checkout@v3
       - name: Setup go
-        uses: actions/setup-go@v2
+        uses: actions/setup-go@v3
         with:
           go-version: "1.18"
       - uses: actions/cache@v3

clientapi/auth/user_interactive.go

@@ -18,6 +18,7 @@ import (
 	"context"
 	"encoding/json"
 	"net/http"
+	"sync"
 
 	"github.com/matrix-org/dendrite/clientapi/jsonerror"
 	"github.com/matrix-org/dendrite/setup/config"
@@ -102,6 +103,7 @@ type userInteractiveFlow struct {
 // the user already has a valid access token, but we want to double-check
 // that it isn't stolen by re-authenticating them.
 type UserInteractive struct {
+	sync.RWMutex
 	Flows []userInteractiveFlow
 	// Map of login type to implementation
 	Types map[string]Type
@@ -128,6 +130,8 @@ func NewUserInteractive(userAccountAPI api.UserLoginAPI, cfg *config.ClientAPI)
 }
 
 func (u *UserInteractive) IsSingleStageFlow(authType string) bool {
+	u.RLock()
+	defer u.RUnlock()
 	for _, f := range u.Flows {
 		if len(f.Stages) == 1 && f.Stages[0] == authType {
 			return true
@@ -137,8 +141,10 @@ func (u *UserInteractive) IsSingleStageFlow(authType string) bool {
 }
 
 func (u *UserInteractive) AddCompletedStage(sessionID, authType string) {
+	u.Lock()
 	// TODO: Handle multi-stage flows
 	delete(u.Sessions, sessionID)
+	u.Unlock()
 }
 
 type Challenge struct {
@@ -150,12 +156,17 @@ type Challenge struct {
 }
 
 // Challenge returns an HTTP 401 with the supported flows for authenticating
-func (u *UserInteractive) Challenge(sessionID string) *util.JSONResponse {
+func (u *UserInteractive) challenge(sessionID string) *util.JSONResponse {
+	u.RLock()
+	completed := u.Sessions[sessionID]
+	flows := u.Flows
+	u.RUnlock()
+
 	return &util.JSONResponse{
 		Code: 401,
 		JSON: Challenge{
-			Completed: u.Sessions[sessionID],
-			Flows:     u.Flows,
+			Completed: completed,
+			Flows:     flows,
 			Session:   sessionID,
 			Params:    make(map[string]interface{}),
 		},
@@ -170,8 +181,10 @@ func (u *UserInteractive) NewSession() *util.JSONResponse {
 		res := jsonerror.InternalServerError()
 		return &res
 	}
+	u.Lock()
 	u.Sessions[sessionID] = []string{}
-	return u.Challenge(sessionID)
+	u.Unlock()
+	return u.challenge(sessionID)
 }
 
 // ResponseWithChallenge mixes together a JSON body (e.g an error with errcode/message) with the
@@ -184,7 +197,7 @@ func (u *UserInteractive) ResponseWithChallenge(sessionID string, response inter
 		return &ise
 	}
 	_ = json.Unmarshal(b, &mixedObjects)
-	challenge := u.Challenge(sessionID)
+	challenge := u.challenge(sessionID)
 	b, err = json.Marshal(challenge.JSON)
 	if err != nil {
 		ise := jsonerror.InternalServerError()
@@ -213,7 +226,11 @@ func (u *UserInteractive) Verify(ctx context.Context, bodyBytes []byte, device *
 
 	// extract the type so we know which login type to use
 	authType := gjson.GetBytes(bodyBytes, "auth.type").Str
+
+	u.RLock()
 	loginType, ok := u.Types[authType]
+	u.RUnlock()
+
 	if !ok {
 		return nil, &util.JSONResponse{
 			Code: http.StatusBadRequest,
@@ -223,7 +240,12 @@ func (u *UserInteractive) Verify(ctx context.Context, bodyBytes []byte, device *
 
 	// retrieve the session
 	sessionID := gjson.GetBytes(bodyBytes, "auth.session").Str
-	if _, ok = u.Sessions[sessionID]; !ok {
+
+	u.RLock()
+	_, ok = u.Sessions[sessionID]
+	u.RUnlock()
+
+	if !ok {
 		// if the login type is part of a single stage flow then allow them to omit the session ID
 		if !u.IsSingleStageFlow(authType) {
 			return nil, &util.JSONResponse{

internal/caching/impl_ristretto.go

@@ -146,7 +146,7 @@ func (c *RistrettoCostedCachePartition[K, V]) Set(key K, value V) {
 }
 
 type RistrettoCachePartition[K keyable, V any] struct {
-	cache   *ristretto.Cache
+	cache   *ristretto.Cache //nolint:all,unused
 	Prefix  byte
 	Mutable bool
 	MaxAge  time.Duration