OIDC named tenants fail to recover when unavailable at startup

segfault24 commented 2 months ago

Describe the bug

When using quarkus-oidc with named tenants and resolve-tenants-with-issuer, if an OIDC server is not available when the application starts, requests with credentials issued by that OIDC server will never succeed.

This behavior prevents an application from self-healing and requires a manual restart to return to normal operation.

Variations attempted:

single named tenant
multiple named tenants
discovery-enabled=false
connection-delay=PT2M
connection-retry-count=999

Expected behavior

Quarkus eventually reattempts to contact the OIDC server, and subsequent requests succeed with 200 Ok.

Actual behavior

Quarkus never reattempts to contact the OIDC server, and all requests fail with 401 Unauthorized forever.

How to Reproduce?

https://github.com/segfault24/quarkus-oidc-bug/tree/main

Start the application, and wait for it to become ready. It will produce warnings about the OIDC server not being available.

./mvnw quarkus:dev

2024-08-08 21:57:04,578 WARN  [io.qua.oid.run.OidcRecorder] (vert.x-eventloop-thread-1) Tenant 'keycloak-1': 'OIDC Server is not available'. OIDC server is not available yet, an attempt to connect will be made during the first request. Access to resources protected by this tenant may fail if OIDC server will not become available
...
2024-08-08 21:57:04,647 INFO  [io.quarkus] (Quarkus Main Thread) quarkus-oidc-bug 1.0.0-SNAPSHOT on JVM (powered by Quarkus 3.13.1) started in 2.053s. Listening on: http://localhost:8080

In a second terminal, start the Keycloak container, and wait for it to become ready.

docker-compose up

...
2024-08-08 21:57:37,093 INFO  [org.keycloak.services] (main) KC-SERVICES0032: Import finished successfully
2024-08-08 21:57:37,228 INFO  [org.keycloak.services] (main) KC-SERVICES0009: Added user 'admin' to realm 'master'
2024-08-08 21:57:37,295 INFO  [io.quarkus] (main) Keycloak 25.0.2 on JVM (powered by Quarkus 3.8.5) started in 7.546s. Listening on: http://0.0.0.0:8080. Management interface listening on http://0.0.0.0:9000.

In a third terminal, acquire an access token and make a request to the application. It will always respond with 401 Unauthorized.

ACCESS_TOKEN=$( \
    curl http://localhost:8081/realms/quarkus/protocol/openid-connect/token \
    -d "client_id=test-client" -d "username=alice" -d "password=alice" -d "grant_type=password" \
    | jq -r '.access_token')
curl -v http://localhost:8080/hello -H "Authorization: bearer ${ACCESS_TOKEN}"

Output of `uname -a` or `ver`

MINGW64_NT-10.0-22631 DESKTOP-REDACTED 3.4.10-87d57229.x86_64 2024-02-14 20:17 UTC x86_64 Msys

Output of `java -version`

openjdk version "21.0.1" 2023-10-17 LTS OpenJDK Runtime Environment Corretto-21.0.1.12.1 (build 21.0.1+12-LTS) OpenJDK 64-Bit Server VM Corretto-21.0.1.12.1 (build 21.0.1+12-LTS, mixed mode, sharing)

Quarkus version or git rev

3.13.1

Build tool (ie. output of `mvnw --version` or `gradlew --version`)

Apache Maven 3.9.7 (8b094c9513efc1b9ce2d952b3b9c8eaedaf8cbf0) Maven home: C:\Users\austin.m2\wrapper\dists\apache-maven-3.9.7-bin\33482774\apache-maven-3.9.7 Java version: 21.0.1, vendor: Amazon.com Inc., runtime: C:\Program Files\Amazon Corretto\jdk21.0.1_12 Default locale: en_US, platform encoding: UTF-8 OS name: "windows 11", version: "10.0", arch: "amd64", family: "windows"

Additional information

No response

quarkus-bot[bot] commented 2 months ago

/cc @pedroigor (oidc), @sberyozkin (oidc)

sberyozkin commented 2 months ago

Thanks @segfault24, this one will require some refactoring and deprecation due to the fact TenantResolver which deals with the issuer based matching returns String but it will have to return Uni<String> to allow the just in time resolution, thus impacting a lot of code around it. It may take a bit of time...

This issue is indeed specific to the issuer-based tenant resolution strategy

michalvavrik commented 1 month ago

Thanks @segfault24, this one will require some refactoring and deprecation due to the fact TenantResolver which deals with the issuer based matching returns String but it will have to return Uni<String> to allow the just in time resolution, thus impacting a lot of code around it. It may take a bit of time...

yes, it does :-) it's chicken egg problem (determine tenant before you know the issuer of the tenant)

This issue is indeed specific to the issuer-based tenant resolution strategy

yep; I am getting closer to the fix and I am worried about retries. It can get to multitudes of retries on the tenants that might never become available. that would slow down application. I have been thinking to introduce simple interface that users can implement, like:

public interface TenantInitializationStrategy {
    boolean tryToInitialize(String tenantId);
}

while default strategy would always return true; I am on the fence whether we should wait till someone ask for it.

michalvavrik commented 1 month ago

I'll go without any strategy for now, because if user doesn't expect the tenant to get initialized, the user can disable the tenant.

quarkusio / quarkus