Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: updates jwx library to use its thread-safe jwks cache #88

Merged
merged 3 commits into from
Oct 24, 2024

Conversation

ctran88
Copy link
Contributor

@ctran88 ctran88 commented Oct 17, 2024

What's New?

Problem:
Our jwks cache was a map, so it is not thread safe.

If the passage.App struct was being called via passage.New in multiple threads, i.e. it is being initialized per request thread in an API server, then it would throw a fatal "concurrent map writes" error.

Solution:
Use a thread-safe data structure to cache the jwks.

Our existing jwt/jwks library, https://github.com/lestrrat-go/jwx, has its own thread-safe (via RW mutex) caching implementation that we can make use of by upgrading from v1 to v2.

Tests:
Adding and running this test on main will fail because it detects a race condition (concurrent writes to the jwkCache map variable). It will pass on this branch

go test -race -run TestAppJWKSCacheWriteConcurrency

WARNING: DATA RACE
Write at 0x00c0000013a8 by goroutine 9:
  github.com/passageidentity/passage-go.(*App).fetchJWKS()
      /Users/christran/repos/passage-go/authentication.go:70 +0x100
  github.com/passageidentity/passage-go.New()
      /Users/christran/repos/passage-go/app.go:41 +0x208
  github.com/passageidentity/passage-go_test.TestAppJWKSCacheWriteConcurrency.func1()
      /Users/christran/repos/passage-go/app_test.go:54 +0x138

Previous write at 0x00c0000013a8 by goroutine 8:
  github.com/passageidentity/passage-go.(*App).fetchJWKS()
      /Users/christran/repos/passage-go/authentication.go:70 +0x100
  github.com/passageidentity/passage-go.New()
      /Users/christran/repos/passage-go/app.go:41 +0x208
  github.com/passageidentity/passage-go_test.TestAppJWKSCacheWriteConcurrency.func1()
      /Users/christran/repos/passage-go/app_test.go:54 +0x138

Goroutine 9 (running) created at:
  github.com/passageidentity/passage-go_test.TestAppJWKSCacheWriteConcurrency()
      /Users/christran/repos/passage-go/app_test.go:51 +0x5c
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1689 +0x180
  testing.(*T).Run.gowrap1()
      /usr/local/go/src/testing/testing.go:1742 +0x40

Goroutine 8 (finished) created at:
  github.com/passageidentity/passage-go_test.TestAppJWKSCacheWriteConcurrency()
      /Users/christran/repos/passage-go/app_test.go:51 +0x5c
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1689 +0x180
  testing.(*T).Run.gowrap1()
      /usr/local/go/src/testing/testing.go:1742 +0x40
==================
--- FAIL: TestAppJWKSCacheWriteConcurrency (0.20s)
    testing.go:1398: race detected during execution of test
FAIL
exit status 1
FAIL	github.com/passageidentity/passage-go	0.812s

passage-go/app_test.go

Lines 52 to 70 in 795b803

func TestAppJWKSCacheWriteConcurrency(t *testing.T) {
goRoutineCount := 2
var wg sync.WaitGroup
wg.Add(goRoutineCount)
for i := 0; i < goRoutineCount; i++ {
go func() {
defer wg.Done()
_, err := passage.New(PassageAppID, &passage.Config{
APIKey: PassageApiKey, // An API_KEY environment variable is required for testing.
})
require.Nil(t, err)
}()
}
wg.Wait()
}

References:

Screenshots (if appropriate):

Please attach any screenshots that would help illustrate the changes.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have manually tested my code thoroughly
  • I have added/updated inline documentation for public facing interfaces if relevant
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing integration and unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Additional context

Add any other context or screenshots about the pull request here.

@ctran88 ctran88 requested a review from a team October 17, 2024 23:54
@ctran88 ctran88 force-pushed the PSG-4620-fix-thread-unsafe-jwks-cache branch from 795b803 to 8cb184a Compare October 17, 2024 23:54
@ctran88 ctran88 marked this pull request as ready for review October 17, 2024 23:55
app.go Show resolved Hide resolved
@ctran88 ctran88 linked an issue Oct 22, 2024 that may be closed by this pull request
app.go Outdated
@@ -38,16 +42,17 @@ func New(appID string, config *Config) (*App, error) {
client: client,
}

app.JWKS, err = app.fetchJWKS()
if err != nil {
// cached set setup taken from https://github.com/lestrrat-go/jwx/blob/8d1d78351e9f259723f9f558889a78e327379683/examples/jwk_cache_example_test.go#L21-L36
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This link can perhaps be omitted, as the example is surfaced in the package's docs here.

But I'm also good with keeping it if it feels like a good point of reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed in 058e8d4

@ctran88 ctran88 force-pushed the PSG-4620-fix-thread-unsafe-jwks-cache branch from 287bc9e to 058e8d4 Compare October 24, 2024 17:03
Copy link

sonarcloud bot commented Oct 24, 2024

@ctran88 ctran88 merged commit b677920 into main Oct 24, 2024
2 checks passed
@ctran88 ctran88 mentioned this pull request Oct 24, 2024
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

jwkCache can't be updated concurrently
2 participants