Fire OIDC security event when OIDC server not available

[michalvavrik]

closes: #35920

[sberyozkin]

@michalvavrik Thanks, great follow up. I'm no sure right now how many times it will be sent, for example, if the URL is not even set or mistyped then the server will not start - I believe this event should not be sent in these cases, please check.

Also, if a connection error happens for statically configured OIDCs, then a retry will be made at the request time. I wonder if sending this event at the first failed attempt time, will confuse the consumers, as opposed to sending it after the retry time failure ? They may fire some red alarm ex in RH Insights, but then 1 min later OIDC will be up...

May be we should support an OIDC_AVAILABLE event ?

[michalvavrik]

@michalvavrik Thanks, great follow up. I'm no sure right now how many times it will be sent,

• it is fired every time Quarkus tries to connect to the OIDC server and OIDC endpoint is not available
• you can see it in the logs as OIDC recorder logs information in the very same situation (equal): OIDC server is not available at the '...' URL.

for example, if the URL is not even set

I've tried following steps:

1. quarkus create app org.acme:security-openid-connect-multi-tenancy-quickstart --extension='oidc,resteasy-reactive-jackson' --no-code
2. cd security-openid-connect-multi-tenancy-quickstart/
3. mvn clean package
4. java -jar target/quarkus-app/quarkus-run.jar

with result 'quarkus.oidc.auth-server-url' property must be configured which is fired here

quarkus/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcRecorder.java

Line 211 in e1c4c96

throw new ConfigurationException("'quarkus.oidc.auth-server-url' property must be configured");

therefore I don't think that could be a case.

or mistyped

Validation happens here

quarkus/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcRecorder.java

Line 213 in e1c4c96

OidcCommonUtils.verifyEndpointUrl(oidcConfig.getAuthServerUrl().get());

while the long flow of unis that at certain point when no other uni failed starts here (which is later):

quarkus/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcRecorder.java

Line 313 in e1c4c96

return createOidcProvider(oidcConfig, tlsConfig, vertx)

then the server will not start - I believe this event should not be sent in these cases, please check.

It is not. If you need me to write tests, I didn't test when this "should not happen" because I suppose list could be long (potentially endless), however I'll add any test you require.

Also, if a connection error happens for statically configured OIDCs, then a retry will be made at the request time. I wonder if sending this event at the first failed attempt time, will confuse the consumers, as opposed to sending it after the retry time failure ? They may fire some red alarm ex in RH Insights, but then 1 min later OIDC will be up...

We are talking about Uni, which should mean there is item or failure once. Therefore idea that retry will lead to multiple exceptions -> multiple events is not correct. I checked here https://smallrye.io/smallrye-mutiny/2.5.3/tutorials/retrying/ and I think this sentence should give us certainty: If despite multiple attempts, it still fails, the failure is propagated downstream.

For that reason, I believe that retry like this one

quarkus/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcRecorder.java

Line 423 in e1c4c96

.retry()

that later leads to reporting event

quarkus/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcRecorder.java

Line 430 in e1c4c96

return toOidcException(t, oidcConfig.authServerUrl.get());

is fired when OIDC server is not available.

They may fire some red alarm ex in RH Insights, but then 1 min later OIDC will be up...

1 min in OpenShift where Deployment may scale in a seconds and downscale again based on load and POD health is eternity. Why would you like to disclose information from users? I believe your worries needs to be documented, rather than not fire event.

It sounds to me like you believe there is nothing reasonable users can do with this security event when 1 min later OIDC might be available. But based on that event, they can re-check health of OIDC server from whole different place than this Quarkus app.

May be we should support an OIDC_AVAILABLE event ?

That is sort of thing users can check easily with call to OIDC server from Quarkus app when they know there is a problem. I can add it if you want, but it wasn't part of #35920 so I didn't add it here. Your call.

Thanks @sberyozkin , I'll wait for feedback before I do any changes, as right now it can be anywhere between drop constructor and add SERVER URL and rewrite whole PR because your arguments are not sound enough.

[michalvavrik]

I've decided to address the comment I agree with after all. Done.

[sberyozkin]

Hi @michalvavrik Sorry for a delay.

As far as a retry is concerned: I don't refer to that Uni retry, if the connection fails at the start up, then it is pending a retry when the first client request arrives, so at that point a new attempt to get that Uni based connection (with its own retries) is done.

Does it clarify it ?

[michalvavrik]

Hi @michalvavrik Sorry for a delay.

No hurry, I sense this PR will take a while.

As far as a retry is concerned: I don't refer to that Uni retry, if the connection fails at the start up, then it is pending a retry when the first client request arrives, so at that point a new attempt to get that Uni based connection (with its own retries) is done.

I'm well aware, I tried to answer this here #37760 (comment). The idea that it is alright that we do not fire event because it might later be alright means users is loosing opportunity to react ASAP. When it fails on first request, it is worse situation than when it fails on startup. What matters are user requests.

Does it clarify it ?

Yes.

[sberyozkin]

@michalvavrik The PR is perfect, I'm just thinking about that retry thing, the existing oidc-wiremock test on main demonstrates it, it fails to connect at the start up, but then Wiremock server is started and the connection is established when the test makes a request. I believe there should be an option to observe that OIDC server is available (meaning the previous alarm can be dropped)

[michalvavrik]

@michalvavrik The PR is perfect, I'm just thinking about that retry thing, the existing oidc-wiremock test on main demonstrates it, it fails to connect at the start up, but then Wiremock server is started and the connection is established when the test makes a request. I believe there should be an option to observe that OIDC server is available (meaning the previous alarm can be dropped)

Sure, I'll add the OIDC_AVAILABLE event. I'm not sure about name, I'll use OIDC_SERVER_AVAILABLE but you can always request changes later. Thanks

[michalvavrik]

Done, I became worried that we will keep sending events for every tenant even though there were no issues every time there is an attempt to connect to the OIDC endpoint, therefore the event reporting that OIDC server is available is only reported once for the tenant that had previous connection failure.

[github-actions]

🙈 The PR is closed and the preview is expired.

[quarkus-bot]

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

[sberyozkin]

@michalvavrik This is the last one I think, that should probably remove the tenant from the map, I guess, otherwise, when a dynamic TenantConfigResolver is used, it might keep growing even if only when connection failures occur

[michalvavrik]

@sberyozkin I'll need little more to understand Sergey. Can you give me hint what you mean, please? Please note that tenant is removed when OIDC available event is reported. I don't want to remove it before that happens, because then I wouldn't have certainty that event were reported.