Repository navigation
Conditional SPNEGO Authenticator - #17064
slominskir wants to merge 1 commit into
Conversation
4333eea to
cff7dd0
Compare
Closes 8989
cff7dd0 to
9eecf9a
Compare
There was a problem hiding this comment.
@slominskir Thanks for this effort!
I've added few comments inline. Also I think that we may need to test this - possibly new test in AbstractKerberosSingleRealmTest? See for example BrowserFlowTest how to easily modify authentication flow
| * </p> | ||
| * @author <a href="mailto:Ryan.Slominski@gmail.com">Ryan Slominski</a> | ||
| */ | ||
| public class ConditionalSpnegoAuthenticator extends SpnegoAuthenticator { |
There was a problem hiding this comment.
It will be better to implement this as separate condition instead of extension of SpnegoAuthenticator.
I know we have already ConditionalFormAuthenticatorFactory, but that was introduced before we have conditions support for authentication flows. For the sake of better composing and ability to re-use the similar condition in other places too, the dedicated implementation of ConditionalAuthenticator is much better option.
I am not sure if it is easily possible to test with X-Forwarded header, but I think that we can at least test with protocol re-authentication (EG. check that with prompt=login the SPNEGO is not triggered, but without prompt=login it is triggered).
There was a problem hiding this comment.
Ok, I'll look into ConditionalAuthenticator. Just looking at the docs it's a little confusing as it says:
- If all executions evaluate as true, the Conditional sub-flow acts as Required.
- If all executions evaluate as false, the Conditional sub-flow acts as Disabled.
I think this might be trying to say:
- If all executions evaluate as true, the Conditional sub-flow acts as Required.
- If any execution evaluates as false, the Conditional sub-flow acts as Disabled.
Otherwise the case of some executions evaluate to true while others evaluate to false is left unexplained.
Also, a SPNEGO execution is usually configured as an ALTERNATIVE, meaning if it fails no problem just continue on to the next authenticator.
REQUIRED is defined in the docs as:
All Required elements in the flow must be successfully sequentially executed. The flow terminates if a required element fails.
Does that mean that if we use a ConditionalAuthenticator it is not possible to conditonally attempt SPNEGO, and if attempted, but fails, continue on to the next authenticator? If possible, then maybe I'm confusing the definition of a REQUIRED subflow vs a REQUIRED execution?
There was a problem hiding this comment.
Yes, you're right. I think the docs should be updated as you suggested (regarding if any ...)
Regarding conditional subflow: I think that what you want to achieve probably can be achieved with something like replacing default Kerberos authenticator with something like:
- kerberos-subflow1 ALTERNATIVE
-- nested-conditional-subflow CONDITIONAL
--- condition1-reauthentication REQUIRED
--- condition2-xforwarded-headers REQUIRED
--- Kerberos ALTERNATIVE
Note that I can be wrong as I did not test it by myself. This configuration is a bit cumbersome and we plan to improve authentication flows by adding "Authentication policies" in the future, which will allow to configure them in the more intuitive way.
There was a problem hiding this comment.
Wow, that's a mess. It looks like the requirement type of Conditional unnecessarily complicates and de-normalizes. The Conditional requirement should probably be deprecated and instead simply adding an execution type of Conditional subflow is enough to indicate you mean conditional subflow. Having a separate requirement type is redundant and actually prevents you from indicating that the subflow is ALTERNATIVE. Intuitively the only difference between a regular subflow and a conditional subflow would be that the regular one has ordered executions and a conditional subflow has ordered conditions and then one execution at the end that is only executed if the gauntlet of conditions are all passed. It's actually not immediately clear how this is supposed to work. It looks like this was defined in #7897
There was a problem hiding this comment.
Actually, it looks like the way conditional subflows work is if their condition fails then they can short-circuit the conditional subflow as if the entire subflow execution is DISABLED. If their condition passes then execution is allowed to continue and any executions in the conditional flow behave as if they're in a regular flow and not in a subflow at all. If that's true then the setup would look like:
- conditional-subflow CONDITIONAL
-- condition1-reauthentication
-- condition2-xforwarded-headers
-- Kerberos ALTERNATIVE
I don't think conditions have "requirements" - they just simply resolve to pass or fail. I haven't tested any of this either and it's worrying that it's not immediately clear how this is supposed to work.
There was a problem hiding this comment.
The idea of a set of generic reusable conditions is good, but an ordered list of conditions should probably be an attribute of an execution, not a special subflow.
There was a problem hiding this comment.
Sorry about the confusion with the requirement on the conditions...
I think that for your configuration, the conditional-subflow will become REQUIRED in case that conditions evaluate to true. Hence it likely won't work correctly if you want it to use at same level with other things and achieve something like: cookie OR conditional-kerberos OR identity-provider-redirector OR forms-subflow
Hence why I think another "parent" alternative subflow might be needed to wrap the conditional-subflow... You can try and see.
I definitely agree that this stuff may require some more love to become more usable and intuitive to configure.
There was a problem hiding this comment.
I created a discussion about this here. I'm not really sure how to proceed. I'm leaning towards sticking with the current approach of extending SpnegoAuthenticator for now until the new way is improved. It seems like the IP based Condition someone else is already working on may solve this partially.
| public void authenticate(AuthenticationFlowContext context) { | ||
| AuthenticationSessionModel session = context.getAuthenticationSession(); | ||
| Map<String, String> clientNotes = session.getClientNotes(); | ||
| if ("login".equals(clientNotes.get("prompt"))) { |
There was a problem hiding this comment.
Few points to this:
- Is it possible to replace this with something like:
LoginProtocol protocol = context.getSession().getProvider(LoginProtocol.class, authSession.getProtocol());
if (protocol.requireReauthentication(context.getUserSession(), context.getAuthenticationSession());
Will make it possible to use this with context of SAML as well. It is possible that this will need to add method getUserSession() to AuthenticationFlowContext (UserSession might have been attached before by CookieAuthenticator, which is usually before SPNEGO).
Also will likely require some checks for null userSession in the protocol implementations.
- Is it possible to add option to the condition like
Ignore when protocol requires re-authenticationwith the description similar toWhen this option is enabled, then condition is evaluated to false when protocol requires re-authentication. For example when parameter prompt=login for OIDC protocol is used or forceauthn for SAML protocol.
This will allow that condition can be evaluated for:
- only protocol authentication
- only x-forwarded header
- both
There was a problem hiding this comment.
It sounds like we may need two new separate ConditionalAuthenticators:
- XForwardedForPatternMatcherCondition
- PromptLoginCondition
There was a problem hiding this comment.
Ah, I see, yes supporting both SAML and OIDC would be best. So instead of PromptLoginCondition perhaps we call it ReauthenticationCondition.
There was a problem hiding this comment.
yes, 2 separate conditions can be better option. +1 for ReauthenticationCondition (will be good to add option to it which allows to evaluate it to true in case that protocol re-authentication is used but also for the opposite case when protocol re-authentication is not used. As I think that for various cases, both conditions can be useful).
There was a problem hiding this comment.
Looks like someone else is already working on XForwardedForCondition: #16453
There was a problem hiding this comment.
but also for the opposite case when protocol re-authentication is not used
This sounds like something the ConditionalAuthenticator framework should support generically - i.e. a config option to apply not (!) on the boolean result that a condition provides.
There was a problem hiding this comment.
This sounds like something the
ConditionalAuthenticatorframework should support generically - i.e. a config option to applynot(!) on the boolean result that a condition provides.
+1, it does not right now, but it is generally useful for every condition though.
| AuthenticationSessionModel session = context.getAuthenticationSession(); | ||
| Map<String, String> clientNotes = session.getClientNotes(); | ||
| if ("login".equals(clientNotes.get("prompt"))) { | ||
| logger.debug("Skip SPNEGO because of prompt=login"); |
There was a problem hiding this comment.
Minor: Will be nice to remove SPNEGO from the debug message (if we go with dedicated condition as I've proposed). Similar to other debug messages.
Closes 8989
See Also: #16981