From ea97e8c8447382e2a7d0c67bf8c3e460f6bc3284 Mon Sep 17 00:00:00 2001
From: Sergi Castro <sergikstro@gmail.com>
Date: Wed, 13 Apr 2022 11:46:06 +0200
Subject: [PATCH 1/2] Fix race in `jwk.AutoRefresh` (#686)

* Add test that shows up the race condition

This happens when AutoRefresh.Configure and
AutoRefresh.doRefreshRequests happens at same time, trying to read/write
the same target

* Fix the race in the AutoRefresh

Splicitly protect read and write to targets in the method
AutoRefresh.doRefreshRequest
---
 jwk/refresh.go      |  7 ++++++-
 jwk/refresh_test.go | 48 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/jwk/refresh.go b/jwk/refresh.go
index 051263259..0a8f75452 100644
--- a/jwk/refresh.go
+++ b/jwk/refresh.go
@@ -489,9 +489,9 @@ func (af *AutoRefresh) refreshLoop(ctx context.Context) {
 func (af *AutoRefresh) doRefreshRequest(ctx context.Context, url string, enableBackoff bool) error {
 	af.muRegistry.RLock()
 	t, ok := af.registry[url]
-	af.muRegistry.RUnlock()
 
 	if !ok {
+		af.muRegistry.RUnlock()
 		return errors.Errorf(`url "%s" is not registered`, url)
 	}
 
@@ -505,6 +505,7 @@ func (af *AutoRefresh) doRefreshRequest(ctx context.Context, url string, enableB
 	if t.wl != nil {
 		fetchOptions = append(fetchOptions, WithFetchWhitelist(t.wl))
 	}
+	af.muRegistry.RUnlock()
 
 	res, err := fetch(ctx, url, fetchOptions...)
 	if err == nil {
@@ -520,7 +521,9 @@ func (af *AutoRefresh) doRefreshRequest(ctx context.Context, url string, enableB
 				af.muCache.Lock()
 				af.cache[url] = keyset
 				af.muCache.Unlock()
+				af.muRegistry.RLock()
 				nextInterval := calculateRefreshDuration(res, t.refreshInterval, t.minRefreshInterval)
+				af.muRegistry.RUnlock()
 				rtr := &resetTimerReq{
 					t: t,
 					d: nextInterval,
@@ -532,8 +535,10 @@ func (af *AutoRefresh) doRefreshRequest(ctx context.Context, url string, enableB
 				}
 
 				now := time.Now()
+				af.muRegistry.Lock()
 				t.lastRefresh = now.Local()
 				t.nextRefresh = now.Add(nextInterval).Local()
+				af.muRegistry.Unlock()
 				return nil
 			}
 			err = parseErr
diff --git a/jwk/refresh_test.go b/jwk/refresh_test.go
index 723079086..4faa3a904 100644
--- a/jwk/refresh_test.go
+++ b/jwk/refresh_test.go
@@ -16,6 +16,7 @@ import (
 	"github.com/lestrrat-go/jwx/internal/jwxtest"
 	"github.com/lestrrat-go/jwx/jwk"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 //nolint:revive,golint
@@ -384,3 +385,50 @@ func TestErrorSink(t *testing.T) {
 		})
 	}
 }
+
+func TestAutoRefreshRace(t *testing.T) {
+	k, err := jwxtest.GenerateRsaJwk()
+	if !assert.NoError(t, err, `jwxtest.GenerateRsaJwk should succeed`) {
+		return
+	}
+	set := jwk.NewSet()
+	set.Add(k)
+
+	// set up a server that always success since we need to update the registered target
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusOK)
+		json.NewEncoder(w).Encode(k)
+	}))
+	defer srv.Close()
+
+	// configure a unique auto-refresh
+	ctx, cancel := context.WithTimeout(context.Background(), time.Minute)
+	defer cancel()
+	ar := jwk.NewAutoRefresh(ctx)
+	ch := make(chan jwk.AutoRefreshError, 256) // big buffer
+	ar.ErrorSink(ch)
+
+	wg := sync.WaitGroup{}
+	routineErr := make(chan error, 20)
+
+	// execute a bunch of parallel refresh forcing the requests to the server
+	// need to simulate configure happening also in the goroutine since this is
+	// the cause of races when refresh is updating the registered targets
+	for i := 0; i < 5000; i++ {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			ctx := context.Background()
+
+			ar.Configure(srv.URL, jwk.WithRefreshInterval(500*time.Millisecond))
+			_, err := ar.Refresh(ctx, srv.URL)
+
+			if err != nil {
+				routineErr <- err
+			}
+		}()
+	}
+	wg.Wait()
+
+	require.Len(t, routineErr, 0)
+}

From 8ff6c75e3523cea4138ef3c8913001ce543d82a4 Mon Sep 17 00:00:00 2001
From: Daisuke Maki <lestrrat+github@gmail.com>
Date: Wed, 13 Apr 2022 18:52:00 +0900
Subject: [PATCH 2/2] Update Changes

---
 Changes | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/Changes b/Changes
index fdabb455f..c1c6db298 100644
--- a/Changes
+++ b/Changes
@@ -1,6 +1,24 @@
 Changes
 =======
 
+v1.2.23 13 Apr 2022
+[Bug fixes]
+  * [jwk] jwk.AutoRefresh had a race condition when `Configure()` was
+    called concurrently (#686)
+    (It has been patched correctly, but we may come back to revisit
+     the design choices in the near future)
+
+v1.2.22 08 Apr 2022
+[Bug fixes]
+  * [jws] jws.Verify was ignoring the `b64` header when it was present
+    in the protected headers (#681). Now the following should work:
+
+      jws.Sign(..., jws.WithDetachedPayload(payload))
+      // previously payload had to be base64 encoded
+      jws.Verify(..., jws.WithDetachedPayload(payload))
+
+    (note: v2 branch was not affected)
+
 v1.2.21 30 Mar 2022
 [Bug fixes]
   * [jwk] RSA keys without p and q can now be parsed.