Do not require IdentityProviders for HTTP Authentication mechanisms without credential types and fail if the required provider is missing

[michalvavrik]

closes: #38508

[sberyozkin]

Hey @michalvavrik
Thanks for the quick fix. I'm sorry but I think we should simplify it, specifically, I don't think the builtin check is necessary. The whole HttpAuthenticationMechanism and IdentityProvider split is a best approach design decision recommendation which we should recommend, especially when more than one IdentityProvider can meaningfully process the current authentication credentials, but it should not be enforced. The decision whether to delegate to IdentityProviders or not should be entirely up to a given HttpAuthenticationMechanism, for example, delegating it to the identity provider gives an option to use a blocking context etc. But if all one wants to do is to check some custom headers against some local key or similar, it should work, without users having to register an identity provider, etc

IMHO there should only be 2 small code changes: 1. Remove the check which blocks HttpAuthenticationMechanism call if no IdentityProvider is registered, and 2. Throw a server error if IdentityProviderManager is called if no IdentityProvider is registered. May be also add a debug message if no IdentityProvider is registered, advising that in this case HttpAuthenticationMechanism must be able to resolve SecurityIdentity itself.

Also if you don't mind we should add a small section to the top of the Custom HttpAuthenticationMechanism docs that we recommend to use IdentityProvider because it allows for more options with verifying the credentials, as well as running blocking operations, but simple mechanisms requiring no blocking operations are not required to do it

[michalvavrik]

Thank you for detailed comment. I was afraid that what you suggest has potential for breaking change as till now, if someone added an extension with HttpAuthenticationMechanism and didn't provide his own IdentityProvider, nothing would happen. But when I read your comment I realized that if such auth mechanism is enabled and added as bean, there is no point having it without IdentityProvider. I inspected all our builtin implementations and I can't see how it could break anything. Your suggestion is better.

2. Throw a server error if IdentityProviderManager is called if no IdentityProvider is registered.

That is already happening.

May be also add a debug message if no IdentityProvider is registered, advising that in this case HttpAuthenticationMechanism must be able to resolve SecurityIdentity itself.

That can't be done inside IdentityProviderManager as it is not aware of the HttpAuthenticationMechanisms. To do that inside Vert.x HTTP it would require to do what we do right now just to write debug message. Honestly I think it is clear from thrown error message No IdentityProviders were registered to handle AuthenticationRequest xyz. So I'd rather not do it.

I'll try to add docs comment as well and I'm sure we will iterate on that :-).

[github-actions]

🙈 The PR is closed and the preview is expired.

[michalvavrik]

NoConfiguredRealmsTestCase.testSecureAccessFailure failed because the BasicAuthenticationMechanism is added when not explicitly disabled and neither form or mTLS is enabled etc. Previously it was just ignored if there was no identity provider supporting it's credentials. I find this somehow confusing considering that documentation says it is enabled by default if no auth mechanisms are configured:

quarkus/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/AuthConfig.java

Line 17 in 1fd7006

* If no authentication mechanisms are configured basic auth is the default.

1. It wasn't really true because we enabled it depending on mTLS and form, but there is plenty of other mechanisms ignored.
2. It wasn't added if there wasn't necessary identity provider.

Furthermore, for such a use case as "default" mechanism it really makes sense to only introduce it when there is required IdentityProvider, otherwise authentication can't ever succeed. It is a special case, therefore I added special handling to keep original behavior. I still believe it's good idea to make this change.

[sberyozkin]

Thanks @michalvavrik, about to propose a couple of tiny changes only

[michalvavrik]

Thanks @michalvavrik, about to propose a couple of tiny changes only

Appreciated, but I'll also push changes related to basic auth as there is too many tests failing. You will need to review again, sorry.

[sberyozkin]

I really think there needs to be an exception for basic auth mechanism.

Well, lets not fail then if the match does not exist if it is so tricky to untangle it for the basic mechanism, have a look though if it might be a test specific issue

[michalvavrik]

Well, lets not fail then if the match does not exist if it is so tricky to untangle it for the basic mechanism, have a look though if it might be a test specific issue

I think almost all test failed in Vert.x HTTP module so I'm pretty sure it is not test specific. But TBH I stopped testing after dozen of them.

The root cause is that basic authentication mechanism is added almost always unless you use @Alternative or you use form or mTLS. But I think that should be handled and discussed separately as it is breaking change. Changes in the PR now should be fine and not break things.

[sberyozkin]

@michalvavrik I propose to do a warning for all cases just so that we indeed avoid breaking something, appreciate your concern, rather than have mechanism specific exclusions. Warning should be fine because if the basic authentication is enabled by default but no matching identity provider exists it is still not working. Or how about debug level message for Basic if the warning would be too noisy but do warn for every other mechanism ?

[michalvavrik]

This was a tough one, I think I found right solution but please fight back if you disagree.

1. It is absolutely fine and user friendly to fail with an exception if mechanism is enabled explicitly or added by user but required identity provider is not there. Therefore I thrown an exception there.
2. Basic auth mechanism is added very often, we might need to refactor that in the future, but as long as it is added by default we shouldn't fail as it would be super breaking change. And user didn't explicitly require that mechanism. Therefore it is ignored with debug logging.
3. If basic auth mechanism is explicitly enabled but no provider was found, we should fail a build as it is good information for user.

I only run tests in Vert.x HTTP and Elytron properties modules, so let's see what CI thinks about that as well.

[quarkus-bot]

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 65f47c0.

✅ 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.

⚠️ There are other workflow runs running, you probably need to wait for their status before merging.

[sberyozkin]

Looks good, lets have it settled on main, IMHO this is effectively a hardening fix which will help users find problems with their custom mechanisms

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 65f47c0.

✅ 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.