Prevent repeated Quarkus Security exception handling that lead to duplicate headers during OIDC redirect

[michalvavrik]

fixes: #30011

When proactive auth is enabled both default auth failure handler and builtin exception mappers were setting headers, which lead to duplicate headers that some browsers could not handle (Safari). Now repeated Quarkus Security exception handling is prevented as we only handle them (iff proactive = true) when custom exception mappers has been defined. There is no point running built in ex. mappers as they do exactly same as default auth failure handler had done.

I added tests that are independent on auth mechanism, but I checked fix with OIDC in both classic and reactive.

[quarkus-bot]

Failing Jobs - Building 101796c

Status	Name	Step	Failures	Logs	Raw logs
✔️	JVM Tests - JDK 11
✔️	JVM Tests - JDK 17
✖	JVM Tests - JDK 17 MacOS M1	Build	Failures	Logs	Raw logs
✔️	JVM Tests - JDK 18

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 17 MacOS M1 #

- Failing: extensions/micrometer/deployment 
! Skipped: extensions/micrometer-registry-prometheus/deployment extensions/quartz/deployment extensions/scheduler/deployment and 21 more

📦 extensions/micrometer/deployment

✖ io.quarkus.micrometer.deployment.binder.RedisClientMetricsTest. - More details - Source on GitHub

java.lang.RuntimeException: java.lang.RuntimeException: Failed to start quarkus
	at io.quarkus.test.QuarkusUnitTest.beforeAll(QuarkusUnitTest.java:689)
	at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.lambda$invokeBeforeAllCallbacks$12(ClassBasedTestDescriptor.java:395)

[michalvavrik]

RedisClientMetricsTest failing on MacOS is unrelated.

[sberyozkin]

@michalvavrik Thanks for the fix, it looks good. What I'm somewhat uncertain about, is why do we have to do these essentially workarounds for the proactive auth case.
So, right now, the default handler is always run and then the exception mappers are checked and therefore, for these exceptions, we should now check if the default handler has already done its job. I see it works but I wonder if we are on the right path here.
Should the exception mappers be checked first and only if no matching mapper has been found, then if it is an instance of AuthenticationException, do try the default handler ?

[sberyozkin]

Or run the default handler only if no authentication exceptions have been detected at the build time and just skip a non-default handler altogether if the default handler has set a RouringContext property that it has handled the exception ? May be it is a better option

[michalvavrik]

@michalvavrik Thanks for the fix, it looks good. What I'm somewhat uncertain about, is why do we have to do these essentially workarounds for the proactive auth case. So, right now, the default handler is always run and then the exception mappers are checked and therefore, for these exceptions, we should now check if the default handler has already done its job. I see it works but I wonder if we are on the right path here. Should the exception mappers be checked first and only if no matching mapper has been found, then if it is an instance of AuthenticationException, do try the default handler ?

Hi Sergey @sberyozkin , in short I don't see this as workaround, this PR adds extra condition to RESTEasy Reactive and Classic failure handlers that says "only run exception mappers when proactive=true if user defined custom exception mapper", but we can discuss it. Below are my answers.

default handler is always run

nope, it's only run when proactive auth is enabled, both RESTEasy Reactive and RESTEasy Classic removes it (that's not my change, it was there already) as they wanted exception mappers to handle exception

we should now check if the default handler has already done its job

that's exactly what this change is doing; I can come with different mechanism to determine that default handler did its job (event attribute with default handler signature or headers check), but I like this one as header check is prone to changes and I'm not sure default auth handler should add attribute we don't need, it doesn't provide any extra information. If it is there, we know it run as proactive auth is enabled.

Should the exception mappers be checked first and only if no matching mapper has been found, then if it is an instance of AuthenticationException, do try the default handler

that would mean to add default auth failure handler as actual Vert.x failure handler, but it is not. The auth failure handler is called here when proactive=true and then event is failed. Yes, we could make your suggestion work, but it takes heavy refactoring and I strongly believe this is one of last changes needed for exception mappers/failure handlers of Quarkus Security exceptions to work. It was quite difficult to set up this flow as we need to think of every eventuality. We can only run exception mappers when RESTEasy Reactive/Classic begun processing, but that is done here through failure handler.

Or run the default handler only if no authentication exceptions have been detected at the build time and just skip a non-default handler altogether if the default handler has set a RouringContext property that it has handled the exception ? May be it is a better option

That could be done, but it was one of iterations we had and you basically said we should not allow extensions to stop default auth failure from running (at least that's how I understood it) - please don't make me to search for that comment, there was too many of them :-D. ATM that can't be done as:

1. RESTEasy Reactive can't prevent default auth failure handler when proactive=true, therefore it is always going to run
2. Vertx HTTP extension doesn't have knowledge of RESTEasy Reactive

[michalvavrik]

Btw. I think your comments are justifiable (makes sense) - when Stuart wrote the default auth failure handler, it always ended event, therefore following situations could not happen. I turned how it is used little bit, but trying to keeping changes small I left default auth failure handler intact (except it does not end event).

[sberyozkin]

Sure, sounds like this is as best as we can have it, but what do you think about this one,

RESTEasy Reactive can't prevent default auth failure handler when proactive=true, therefore it is always going to run

OK, I see what you mean based on the earlier comments

Vertx HTTP extension doesn't have knowledge of RESTEasy Reactive

Sure, what I meant was not that Vertx HTTP extension would start checking Resteasy classic or reactive mappers, etc.
But something like this:

• resteasy classic/reactive build time processors check if authentication mappers are present, I think you might already be having some of such checks coded.
• if the proactive=true then a non-default handler is not even invoked after the default handler is run in case of the authentication exceptions, meaning no need to do some explicit protection against the duplicate handling ?

Can that work/be easy enough to support ?

[michalvavrik]

• resteasy classic/reactive build time processors check if authentication mappers are present, I think you might already be having some of such checks coded.

True, we already have them, we could propagate them to Vert.x HTTP extension.

• if the proactive=true then a non-default handler is not even invoked after the default handler is run in case of the authentication exceptions

Can that work/be easy enough to support ?

It works like this (I know you know this, but I need to provide context):

1. default auth failure handler (not Vert.x failure handler, just piece of code) fails event
2. RESTEasy Reactive Vert.x failure handler is called as it was added to the route and:

• we always have to add this failure handler as it also handles ForbiddenException was thrown by permission checked
• maybe (not sure without longer inspection TBH) it is possible some other handler throws auth exception. I believe saw situation when authentication didn't fail as route wasn't secured, but permission checker thrown auth failure exception. I think it was related to JWT validation, sorry to be vague here, maybe I got this one wrong.
• it is possible to that there is custom AuthenticationFailedException so we need this failure handler to pass exception to exception mappers, but also AuthenticationRedicerException is throw and we don't want to invoke mappers for it. What then, should we have 4 failure handlers?
• we could tell default auth failure handler do not fail one exception e.g. completion ex, but fail others as custom ex. mappers need to handle them. that is more complexity than this PR IMHO

[sberyozkin]

@michalvavrik OK, thanks, we need to fix this duplication problem asap anyway, and the fix is OK, so I'm going to merge.
But please think in the next while, if we can avoid Resteasy Reactive/Classic having to have dedicated protection against the duplicated authentication exception handling, it feels it can eventually go, I agree investigating this option can be done at some other time.
Thanks

[sberyozkin]

@acoburn Hi Aaron, did you mean that you saw this problem with 2.14.3.Final as well or only in the 2.15 stream ?

[michalvavrik]

@michalvavrik OK, thanks, we need to fix this duplication problem asap anyway, and the fix is OK, so I'm going to merge. But please think in the next while, if we can avoid Resteasy Reactive/Classic having to have dedicated protection against the duplicated authentication exception handling, it feels it can eventually go, I agree investigating this option can be done at some other time. Thanks

Sure, I'm happy to do that. There have been many changes regarding exception mappers lately, let's keep it as is for a while.

[michalvavrik]

@acoburn Hi Aaron, did you mean that you saw this problem with 2.14.3.Final as well or only in the 2.15 stream ?

+1 to question, fyi Aaron - I tested your reproducer with 2.14.3.Final and could not reproduce it.

[sberyozkin]

Thanks, I was curious if a backport to 2.14 was needed, lets just have a backport (to 2.15) label for now only

[acoburn]

I definitely saw this problem in the 2.15 release. In our codebase where this issue appeared, I also saw it in 2.14.3, so it is curious that the reproducer on 2.14.3 doesn't surface the problem. In any case, fixing this for 2.15 would be great.

[michalvavrik]

Okay, thank you for letting us know @acoburn , but @sberyozkin this fix can't be backported to 2.14 anyway, please don't do that, thanks.