Bad design of jwks cache

[scrat98]

Describe the problem you'd like to have solved

1. Users may affect each other calling the server, that under the hood uses JwkProvider to validate the tokens. See the code below:

fun main() {
  val jwksUrl = "https://login.microsoftonline.com/common/discovery/keys".let {
    URI(it).normalize().toURL()
  }

  val foundKey = UrlJwkProvider(jwksUrl).all.first().id
  val notFoundKey = UUID.randomUUID().toString()

  val jwkProvider =
    JwkProviderBuilder(jwksUrl)
      .cached(100, 10, TimeUnit.MINUTES)
      .rateLimited(10, 1, TimeUnit.MINUTES)
      .build()

  /*
    someone requesting server with JWT, but which has invalid kid
   */
  repeat(11) {
    assertThrows<SigningKeyNotFoundException> {
      jwkProvider.get(notFoundKey)
    }
  }

  /*
    someone would like to request the server with valid JWT and will get rate limit error
   */
  jwkProvider.get(foundKey)
}

2. If oauth2 server has several jwks (with different kid and algorithms) then library will call jwks endpoint as many times as there are keys. https://github.com/auth0/jwks-rsa-java/blob/master/src/main/java/com/auth0/jwk/UrlJwkProvider.java#L163

Describe the ideal solution

1. Rate limit should not be there at all - it's responsibility of upper layer
2. It's more preferable to cache all jwks at once, not one by one

I think the strategy should be the following: cache all jwks response from server and return found jwk by kid. Cache should be update by time.

Alternatives and current work-arounds

class CachedJwkProvider(
  private val delegate: UrlJwkProvider,
  private val expiration: Duration
) : JwkProvider, Closeable {

  private var cache = mapOf<String, Jwk>()

  private val cacheUpdaterJob = timer(
    name = "jwks-cache-updater",
    daemon = true,
    period = expiration.toMillis()
  ) {
    val actual = delegate.all.associateBy { it.id }
    cache = actual
  }

  override fun get(keyId: String): Jwk {
    return cache[keyId] ?: throw SigningKeyNotFoundException("No key found with kid $keyId", null)
  }

  override fun close() {
    cacheUpdaterJob.cancel()
  }
}

fun main() {
  val jwksUrl = "https://login.microsoftonline.com/common/discovery/keys".let {
    URI(it).normalize().toURL()
  }
  val urlJwkProvider = UrlJwkProvider(jwksUrl)
  val foundKey = urlJwkProvider.all.first().id
  val notFoundKey = UUID.randomUUID().toString()

  val jwkProvider = CachedJwkProvider(urlJwkProvider, Duration.ofMinutes(10))

  /*
    waiting cache to load. It's up to implementation make the "get()" call blocking or not. But I
    prefer do not wait any 3party system, therefore we need to wait here just for test
   */
  while (runCatching { jwkProvider.get(foundKey) }.isFailure)

  /*
    someone requesting server with JWT, but which has invalid kid
   */
  repeat(1000) {
    assertThrows<SigningKeyNotFoundException> {
      jwkProvider.get(notFoundKey)
    }
  }

  /*
    someone would like to request the server with valid JWT and will NOT get rate limit error
   */
  jwkProvider.get(foundKey)
}

[jimmyjames]

Thanks @scrat98 for the feedback and proposed alternatives! Regarding the rate-limiting, you can choose to not enable rate limiting by not configuring it, correct? Regarding the caching behavior, that's something we should look into - it sounds familiar, I'll look into if this is something we looked into in the past and if there was any findings regarding the cache behaving that way.