Skip to content

Conversation

ana-vinogradova-camunda
Copy link
Contributor

@ana-vinogradova-camunda ana-vinogradova-camunda commented Aug 9, 2025

Description

The bug:

  • application tries to obtain the OpenID configuration from the IdP at startup, and if it's not available (as in, the server is not available), it fails to launch.

Expected behaviour:

  • the application should still star successfully, but authentication will fail until the IdP is available.

Solution (please see commit messages for more details):
Two places are affected by this bug:

  • clientRegistrationRepository bean creation
  • jwtDecoder bean creation (as it needs the clientRegistrationRepository)

Lets introduce custom ClientRegistrationRepository and JwtDecoder implementations that:

  • will attempt to construct InMemoryClientRegistrationRepository and decoder the same way as it is done now (on application startup). Let's attempt it once. This would ensure that if there are no issues we are ready to authenticate after startup straight away.
  • if for some reason construction of the above beans fails:
    • we log a warning message (as it is recoverable error)
    • we will schedule async retries to create the repository and the decoder in separate threads. We will continue with Spring start up.
    • we will throw an exception telling that the configuration is not yet initialised until the retries succeed.
    • the exception will be propagated to the user
    • once retries succeed - we construct both repository and decoder and use them under the hood.
    • users are able to authenticate

Integration test with the below scenario was also added:

  • set up the application to point to a proxy instead of keycloak first
  • verify that application is up but client request fails with the expected message
  • point proxy to keycloak
  • verify that client request succeeds

❓ Open question:

  • Error handling. @Ben-Sheppard I'd appreciate your help here 🙏

Checklist

Related issues

closes #34189

@github-actions github-actions bot added the component/zeebe Related to the Zeebe component/team label Aug 9, 2025
@ana-vinogradova-camunda ana-vinogradova-camunda force-pushed the 34189-failure-to-start-if-OIDC-provider-is-unavailable branch from c3abe01 to 084f548 Compare August 9, 2025 10:16
The aim of the test is to test unauthorized behaviour
So in this case the setup is not really used anywhere in this test.
Adding this dependency as it going to be used in tests.
This class will be used in two places, so I've decided to make it generic:
- for `ClientRegistrationRepository` bean creation
- for `JwtDecoder` bean creation

The aim of this class is to ensure eventual creation of the object. What that means:
- we will try to create the object on a startup
- in case of failure it will keep retrying the creation in async way until it succeeds

Open question: what is the best value for default retry delay?
Tests two scenarios:
- initialisation on startup
- initialisation after retries
The main purpose of the wrapper is to ensure `ClientRegistrationRepository`
is created rather on a startup or later during retries:
- on a startup it will attempt to create a repository
- if this fails, it'll create `SafeInitProxy` (see previous commits) and pass 2 arguments:
 a supplier to create a `ClientRegistrationRepository` and en error listener (log entry will be present)
- startup will succeed even when IdP is not available
- created proxy will keep attempting to create the repository in the background
- at the same time, if client makes a call (any), `findByRegistrationId` will attempt to get a value
from the proxy (if successful that means the IdP is available). If the value is not present
(meaning IdP is still not available) it'll throw an exception.
Same as for the previous wrapper,the main purpose of this is to ensure `JwtDecoder`
is created rather on a startup or later during retries:

- on a startup it will attempt to create a decoder
- if this fails, it'll create `SafeInitProxy` instance (see previous commits) and pass 2 arguments:
 a supplier to create a `JwtDecoder` and en error listener (log entry will be present)
- this will schedule async retries of creation
- startup will succeed
- created proxy will keep attempting to create the decoder in the background
- at the same time, if client makes a call (any), `decode` will attempt to get a value
from the proxy (if successful that means the IdP is available). If the value is not present
(meaning IdP is still not available) it'll throw an exception that will be returned to the client.

Open question: what the error message should be here?
JwtDecoder relies on ClientRegistrationRepository so I guess the error here should
probably be the same?
…ecoder`

Also, add `.authenticationEntryPoint(authFailureHandler)` (that acts as error handler) to:
`getTokenValidator` and `oidcWebappSecurity`.

This is done so we could customize the error message for the client
and specify the details. This would return our standard error response instead
of a default Spring error response.
As this is a recoverable failure, log level should be "warn" for the repository.
As decoder error is following the repository error, it can be `debug`
This test:
- sets up the application to point to a proxy instead of keycloak first
- verifies that application is up but client request fails with the expected message
- points proxy to keycloak
- verifies that client request succeeds

please note, setting up keycloak takes time.
@ana-vinogradova-camunda ana-vinogradova-camunda force-pushed the 34189-failure-to-start-if-OIDC-provider-is-unavailable branch from 825c215 to c4848fb Compare August 11, 2025 12:20
@ana-vinogradova-camunda ana-vinogradova-camunda force-pushed the 34189-failure-to-start-if-OIDC-provider-is-unavailable branch from d92310a to bd5bc87 Compare August 11, 2025 13:44
@ana-vinogradova-camunda ana-vinogradova-camunda marked this pull request as ready for review August 12, 2025 06:27
@ana-vinogradova-camunda ana-vinogradova-camunda requested a review from a team as a code owner August 12, 2025 06:27
@ana-vinogradova-camunda ana-vinogradova-camunda requested review from bmudric and Ben-Sheppard and removed request for a team August 12, 2025 06:27
return proxy
.orElseThrow(
() ->
new RuntimeException(
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and in SafeInitJwtDecoder it would be useful to provide an Exception class that is more semantically appropriate.
I was thinking Spring's BeanInitializationException as we're building a Bean, but actually we throw here on access, not on initialization.
Perhaps then the Java built-in IllegalStateException makes most sense.
We could add our own Exception, but that is probably just noise if we're not handling it specifically elsewhere.
WDYT?

Copy link
Contributor Author

@ana-vinogradova-camunda ana-vinogradova-camunda Aug 12, 2025

Choose a reason for hiding this comment

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

I agree with IllegalStateException exception for SafeInitClientRegistrationRepository
However, for SafeInitJwtDecoder we have to have AuthenticationCredentialsNotFoundException in order for Spring to pass it to our error handler (unless I am missing something, but I think I am correct). There are other exceptions that could be used to achieve the same but the choice is limited as they have to extend AuthenticationException 🙂

UPD: I've just verified my assumption - yes, this is how it currently works.
For reference : please see AuthFailureHandler, AuthenticationEntryPoint and corresponding AuthenticationException.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could potentially introduce our own Exception what would extends AuthenticationException but it seems like an overkill to me (especially in the timeline that we currently have).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For clarity of what's happening you can also look at BearerTokenAuthenticationFilter line 160:

	catch (AuthenticationException failed) {
		this.securityContextHolderStrategy.clearContext();
		this.logger.trace("Failed to process authentication request", failed);
		this.authenticationFailureHandler.onAuthenticationFailure(request, response, failed);
	}

And line 83:

private AuthenticationFailureHandler authenticationFailureHandler = new AuthenticationEntryPointFailureHandler(
	(request, response, exception) -> this.authenticationEntryPoint.commence(request, response, exception));

So if we throw something that is not AuthenticationException, it is not gonna be handled properly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the investigation and explanation!
Then I'd propose:

  • let's change the RuntimeException to an IllegalStateException for clarity
  • add a comment on top of AuthenticationCredentialsNotFoundException explaining, in short, why the exception type is there as you just explained to me - it will be very useful for the next person not to scratch their head :)

Copy link
Contributor

@bmudric bmudric left a comment

Choose a reason for hiding this comment

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

Thank you for the nice implementation and addressing the comments!

@ana-vinogradova-camunda ana-vinogradova-camunda added this pull request to the merge queue Aug 13, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 13, 2025
@ana-vinogradova-camunda ana-vinogradova-camunda added this pull request to the merge queue Aug 14, 2025
Merged via the queue into main with commit e067234 Aug 14, 2025
165 of 167 checks passed
@ana-vinogradova-camunda ana-vinogradova-camunda deleted the 34189-failure-to-start-if-OIDC-provider-is-unavailable branch August 14, 2025 06:40
@bmudric bmudric restored the 34189-failure-to-start-if-OIDC-provider-is-unavailable branch August 14, 2025 14:06
github-merge-queue bot pushed a commit that referenced this pull request Aug 14, 2025
…6789)

Reverts #36639

This fix caused an issue that led Spring Security to fall back to the
default login endpoint due to missing `Iterable` Implementation in the
`SafeInitClientRegistrationRepository`.
The real solution needs further root cause discussion on how we deal
with startup in the multi-module setup.

See internal discussion:
https://camunda.slack.com/archives/C08MRKHJ0CD/p1755164727531609
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/zeebe Related to the Zeebe component/team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failure to start if OIDC provider is unavailable
2 participants