Authentication fixes

[stuartwdouglas]

• Use synthetic beans to configure form and basic auth
• Allow multiple authentication mechanisms
• Better default behaviour based on what is configured

Also includes synthetic bean fixes from @mkouba

Fixes #7768
Fixes #5284
Fixes #8138

[sberyozkin]

Hi @stuartwdouglas What is going to happen now when quarkus-oidc and quarkus-security-oauth2 and quarkus-smallrye-jwt are all included ? For example, what happens when quarkus-oidc detects an invalid token and fails the authentication ? Iterating to the next mechanism which may support the same credential type does not seem right in this case.

[stuartwdouglas]

authenticate explicitly differentiates between invalid and missing. If you return null the next one is tried, if it fails then no further action is taken.

In general you should only mix mechanisms that use different credentials, e.g. form and basic.

[sberyozkin]

@stuartwdouglas Sorry, I glanced and assumed the iteration would continue even if one of the Bearer authentication mechanism fails.

[gsmet]

@sberyozkin what's the status of this one? I can't infer it from your last comment.

[sberyozkin]

@gsmet Sorry, I initially assumed that multiple mechanisms will be iterated over even if the current one returns a fault, so Stuart clarified it was not the case, which is good.
I have though another question for Stuart, in the other thread above, it appears to me there could be a risk of one mechanism picking the identity provider from the other one.

[gsmet]

@stuartwdouglas I let you check @sberyozkin 's comment and give me the status of this one?

If some of the changes are too risky, maybe it warrants extracting the safe fixes so that I can only backport them.

[sberyozkin]

@gsmet @stuartwdouglas I've been thinking that in a way we can't prevent the users adding quarkus-oidc, quarkus-smallrye-jwt and quarkus-elytron-security-oauth2 at the same time and perhaps I'm over dramatizing there with my comment :-), as the same kind of scenario is possible even with the current code if this mix happens.
IMHO it would indeed be better though to have a dedicated issue for supporting multiple authentication mechanisms at runtime, I'd rather let the users compose the mechanisms easily depending on their requirements (may be with form+basic being the only exception) as opposed to trying all the matching HttpAuthenticationMechanisms - as it does not give a guarantee this is what a user intended :-)

[sberyozkin]

Or just mark quarkus-oidc, quarkus-smallrye-jwt and quarkus-elytron-security-oauth2 mechanisms as mutually exclusive with some annotation ? @stuartwdouglas - that would avoid all the ambiguities with respect to those 3 extensions ? It feels some more thinking is needed around it :-)

[stuartwdouglas]

They are currently mutually exclusive, in that they will fail if all of them are added. I think this is something we can improve later.

One option would be to have some way to express a credential transport type, and check compatibility that way, it's kinda hard to express in a generic way though. Stacking auth mechanisms is already an advanced use case, and I think it should be fairly obvious that you can't mix two that work the same way.

[sberyozkin]

Hi Stuart, @stuartwdouglas, #8138 has just been opened, I haven't looked into it yet, but I think it is related

[sberyozkin]

@stuartwdouglas My understanding is that a user expects an exception when more than one mechanism is present. It makes me think if the user-driven composition is the way to go - otherwise some users will be happy (form+basic case), and some - not

[sberyozkin]

@stuartwdouglas Or just let the users set a property
quarkus.security.support-multiple-mechanisms=true/false, false (default - that should be false I guess because it supports the principle of the least surprise)
If someone wants to have more than one, they just set that property to true.
That would be enough ? So your PR will go as is, except that it will check that property before going into the iteration over multiple mechanisms...

[gsmet]

Guys, I will need a conclusion for this one if you expect it to be part of 1.3.1.

[sberyozkin]

IMHO it would be better to extract the authentication related updates and merge the rest, as it is obvious now different users have different expectations about what should happen when more than one mechanism is available. Though having a property for the users to choose if multiple mechanisms are OK or not seems like an easy way to resolve the ambiguity :-). Not sure if Stuart will agree

[KeesKoffeman]

Well I stumbled upon #8138 while debugging why multiple mechanisms aren’t supported (my use case is multiple). Now it’s very blackbox if you add multiple and it just doesn’t work without any feedback. I’d be very happy if the next release includes this PR!

[sberyozkin]

@KeesKoffeman OK thanks, I thought you did not want that...
@stuartwdouglas What is your opinion about the property (qualified as needed) I suggested ? It would not change much in your PR except make it a bit more predictable. Example, if the property is disabled, then in case of #8138, a user would see "Please enable this property for the multiple authentication mechanisms to work" and once the user does it, all starts working.
If you are not convinced, then lets go ahead as this PR has other fixes. @gsmet Guillaume, please approve/merge once Stuart confirms. (support for combining the mechanisms is useful in general for sure)

[stuartwdouglas]

@sberyozkin if that property is false the app just won't work at all. I don't see any reason to have a config flag that serves no purpose other than to stop the users application from working.

Basically there are two cases:

• There is only one mechanism, things work the same as they do today
• There are multiple mechanisms, today this will just break (well for form+basic it won't work properly, for any other combination the app just won't work). If you add this flag then this situation will continue (i.e. the app won't work), so I don't really see the point of a flag thats only purpose is to stop the app from booting.

[stuartwdouglas]

I have added a credential transport notion to make sure that incompatible mechanisms are not enabled together.

[gsmet]

@sberyozkin can you check that PR in priority?

[sberyozkin]

Hi Stuart @stuartwdouglas
Thanks for the earlier comment,

so I don't really see the point of a flag thats only purpose is to stop the app from booting

I was just hoping to avoid some unpredictability. At the moment multiple mechanisms don't work, and now they will start working, and it may be worth to give an option to the users and prevent opening up the authentication space by accident.
Thanks for introducing this incompatibility notion, IMHO it would be better to start in a more restrictive mode and if it proves too restrictive we can just drop that altogether :-)

[sberyozkin]

That is fine, we can support bearer,code-flow in the future when a given mechanism supports different types

[sberyozkin]

@stuartwdouglas Thanks a lot, and thanks for the patience. The authentication part looks perfect now :-) The rest looks good for sure too

[sberyozkin]

@gsmet HI Guillaume sorry for a delay. Hope everyone is happy with the latest update from Stuart, I certainly am :-).