[java-idp-plugin-webauthn] branch main updated: Improve isSecondFactor logic
Phil Smart
philip.smart at jisc.ac.uk
Fri Jan 19 12:39:21 UTC 2024
This is an automated email from the git hooks/post-receive script.
philsmart pushed a commit to branch main
in repository java-idp-plugin-webauthn.
View the commit online:
http://git.shibboleth.net/view/?p=java-idp-plugin-webauthn.git;a=commit;h=f7c25174b2e533f583bf09493e6aaa413c869d3a
The following commit(s) were added to refs/heads/main by this push:
new f7c2517 Improve isSecondFactor logic
f7c2517 is described below
commit f7c25174b2e533f583bf09493e6aaa413c869d3a
Author: Phil Smart <philip.smart at jisc.ac.uk>
AuthorDate: Fri Jan 19 12:39:18 2024 +0000
Improve isSecondFactor logic
---
webauthn-api/pom.xml | 5 +
.../logic/IsDiscoverableCredentialRequired.java | 4 +-
.../webauthn/context/logic/IsSecondFactor.java | 117 +++++++++++++++++++--
.../PopulateWebAuthnAuthenticationContext.java | 9 +-
.../webauthn/impl/ValidateWebAuthnAssertion.java | 8 +-
.../idp/flows/authn/WebAuthn/webauthn-beans.xml | 6 +-
.../idp/flows/authn/WebAuthn/webauthn-flow.xml | 3 +-
.../authn/webauthn/conf/authn/webauthn.properties | 12 ++-
8 files changed, 136 insertions(+), 28 deletions(-)
diff --git a/webauthn-api/pom.xml b/webauthn-api/pom.xml
index 9287e55..a4d0850 100644
--- a/webauthn-api/pom.xml
+++ b/webauthn-api/pom.xml
@@ -60,6 +60,11 @@
<artifactId>idp-admin-api</artifactId>
<scope>provided</scope>
</dependency>
+ <dependency>
+ <groupId>${idp.groupId}</groupId>
+ <artifactId>idp-session-api</artifactId>
+ <scope>provided</scope>
+ </dependency>
<dependency>
<groupId>${idp.groupId}</groupId>
<artifactId>idp-profile-api</artifactId>
diff --git a/webauthn-api/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/context/logic/IsDiscoverableCredentialRequired.java b/webauthn-api/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/context/logic/IsDiscoverableCredentialRequired.java
index d8b3800..f9c121a 100644
--- a/webauthn-api/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/context/logic/IsDiscoverableCredentialRequired.java
+++ b/webauthn-api/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/context/logic/IsDiscoverableCredentialRequired.java
@@ -54,8 +54,8 @@ public class IsDiscoverableCredentialRequired implements Predicate<ProfileReques
return false;
}
final boolean discoverableCredentialRequired = webauthnContext.getUsername() == null;
- log.debug("{}", discoverableCredentialRequired ? "Usernameless authentication required" :
- "Passwordless authentication required for '"+webauthnContext.getUsername()+"'");
+ log.debug("{}", discoverableCredentialRequired ? "Usernameless (discoverable/passkey) authentication required" :
+ "Passwordless authentication required (username supplied) for '"+webauthnContext.getUsername()+"'");
return discoverableCredentialRequired;
}
}
diff --git a/webauthn-api/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/context/logic/IsSecondFactor.java b/webauthn-api/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/context/logic/IsSecondFactor.java
index a030d1d..6cad0c1 100644
--- a/webauthn-api/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/context/logic/IsSecondFactor.java
+++ b/webauthn-api/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/context/logic/IsSecondFactor.java
@@ -14,6 +14,10 @@
package net.shibboleth.idp.plugin.authn.webauthn.context.logic;
+import java.util.Collection;
+import java.util.Optional;
+import java.util.Set;
+import java.util.function.Function;
import java.util.function.Predicate;
import javax.annotation.Nonnull;
@@ -23,26 +27,83 @@ import org.opensaml.profile.context.ProfileRequestContext;
import org.slf4j.Logger;
import net.shibboleth.idp.authn.context.AuthenticationContext;
+import net.shibboleth.idp.authn.context.MultiFactorAuthenticationContext;
+import net.shibboleth.idp.session.context.navigate.CanonicalUsernameLookupStrategy;
+import net.shibboleth.shared.annotation.constraint.NonnullElements;
+import net.shibboleth.shared.collection.CollectionSupport;
import net.shibboleth.shared.component.AbstractInitializableComponent;
import net.shibboleth.shared.logic.Constraint;
import net.shibboleth.shared.logic.PredicateSupport;
import net.shibboleth.shared.primitive.LoggerFactory;
+import net.shibboleth.shared.primitive.StringSupport;
/**
* A predicate that determines if the authentication flow is being used as a second factor of authentication, and not
* a first (and possibly only) factor. Returns true if second factor use, or false if passwordless/first factor.
*/
+//FIXME this class is not clear, but I think I need something like it
+//FIXME enabled and forced2fa is confusing
public class IsSecondFactor extends AbstractInitializableComponent implements Predicate<ProfileRequestContext> {
/** Class logger. */
@Nonnull private final Logger log = LoggerFactory.getLogger(IsSecondFactor.class);
- /** If true, assume we are operating as a second factor irrespective of the conventional logic.*/
+ /** If true, set isSecondFactor is true irrespective of the conventional logic.*/
@Nonnull private Predicate<ProfileRequestContext> secondFactorOverride;
+ /**
+ * Determines if 2FA support should be enabled (true) or disabled (false). If enabled, the request is checked for
+ * 2FA suitability as normal.
+ * */
+ @Nonnull private Predicate<ProfileRequestContext> enabled;
+
+ /** Lookup strategy to find if a username has already been collected. */
+ @Nonnull private Function<ProfileRequestContext, String> usernameLookupStrategy;
+
+ /** Which previous factors are acceptable to allow a second factor only WebAuthn flow.*/
+ @Nonnull @NonnullElements private Set<String> allowedPreviousFactors;
+
+
/** Constructor.*/
public IsSecondFactor() {
secondFactorOverride = PredicateSupport.alwaysFalse();
+ enabled = PredicateSupport.alwaysFalse();
+ usernameLookupStrategy = new CanonicalUsernameLookupStrategy();
+ allowedPreviousFactors = CollectionSupport.emptySet();
+ }
+
+ /**
+ * Set the lookup strategy to check if a username has already been collected.
+ *
+ * @param strategy lookup strategy
+ */
+ public void setUsernameLookupStrategy(
+ @Nonnull final Function<ProfileRequestContext, String> strategy) {
+ checkSetterPreconditions();
+
+ usernameLookupStrategy = Constraint.isNotNull(strategy, "Username lookup strategy cannot be null");
+ }
+
+ /**
+ * Set a predicate that determines if 2FA support should be enabled (true) or disabled (false). If enabled,
+ * the predicate is allowed to check 2FA suitability.
+ *
+ * @param predicate the predicate
+ */
+ public void setEnabled(@Nonnull final Predicate<ProfileRequestContext> predicate) {
+ checkSetterPreconditions();
+ enabled = Constraint.isNotNull(predicate, "Enabled predicate can not be null");
+ }
+
+ /**
+ * Set a flag that determines if 2FA support should be enabled (true) or disabled (false). If enabled,
+ * the predicate is allowed to check 2FA suitability.
+ *
+ * @param flag the flag to set
+ */
+ public void setEnabled(final boolean flag) {
+ checkSetterPreconditions();
+ enabled = flag ? PredicateSupport.alwaysTrue() : PredicateSupport.alwaysFalse();
}
/**
@@ -53,7 +114,7 @@ public class IsSecondFactor extends AbstractInitializableComponent implements Pr
*/
public void setSecondFactorOverride(@Nonnull final Predicate<ProfileRequestContext> override) {
checkSetterPreconditions();
- secondFactorOverride = Constraint.isNotNull(override, "SecondFactorOverride can not be null");
+ secondFactorOverride = Constraint.isNotNull(override, "SecondFactorOverride predicate can not be null");
}
/**
@@ -66,33 +127,67 @@ public class IsSecondFactor extends AbstractInitializableComponent implements Pr
checkSetterPreconditions();
secondFactorOverride = flag ? PredicateSupport.alwaysTrue() : PredicateSupport.alwaysFalse();
}
+
+ /**
+ * Set the allowable previous factors.
+ *
+ * @param factors the factors to allow
+ */
+ public synchronized void setAllowedPreviousFactors(@Nullable @NonnullElements final Collection<String> factors) {
+ checkSetterPreconditions();
+ if (factors != null) {
+ allowedPreviousFactors = CollectionSupport.copyToSet(StringSupport.normalizeStringCollection(factors));
+ }
+ }
@Override
public boolean test(@Nullable final ProfileRequestContext input) {
checkComponentActive();
if (input == null) {
- log.trace("Profile context was null, assuming first factor");
+ log.trace("Profile context was null, assuming first factor usage");
+ return false;
+ }
+
+ if (!enabled.test(input)) {
+ log.debug("Use as a second factor authentication flow disabled, assuming first factor usage");
return false;
}
if (secondFactorOverride.test(input)){
- log.trace("Second factor authentication flow forced by configuration");
+ log.debug("Second factor authentication flow forced by configuration");
return true;
}
final AuthenticationContext authnContext = input.getSubcontext(AuthenticationContext.class);
if (authnContext == null) {
- log.trace("Authentication context was null, assuming first factor");
+ log.debug("Authentication context was null, assuming first factor usage");
+ return false;
+ }
+ final MultiFactorAuthenticationContext mfaContext =
+ authnContext.getSubcontext(MultiFactorAuthenticationContext.class);
+ if (mfaContext == null) {
+ log.debug("No MFA context available, assuming first factor usage");
return false;
}
- //TODO no decision here yet.
- final boolean secondFactor = false;
- if (secondFactor) {
- log.debug("Request contained a previous factor, assuming second factor");
+ // Must have a previous username
+ final String username = StringSupport.trimOrNull(usernameLookupStrategy.apply(input));
+ log.trace("{}", username != null ? "Found principal name '" + username + "'"
+ : "No previous principal name found");
+
+ // Must have had any previous authentication factor from the allowable list
+ final Optional<String> foundFactor =
+ mfaContext.getActiveResults().keySet().stream().filter(allowedPreviousFactors::contains).findFirst();
+ foundFactor.ifPresent(factor -> log.trace("Found acceptable previous factor '{}'", factor));
+
+ if (username != null && foundFactor.isPresent()) {
+ log.debug("Principal name '{}' found, and previous factor '{}' accepted, assuming second factor usage", username,
+ foundFactor.get());
return true;
+ } else {
+ log.debug("Request did not contain an previous factor, assuming first factor usage");
+ return false;
}
- log.debug("Request did not contain a previous factor, assuming first factor");
- return false;
+
}
}
diff --git a/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/impl/PopulateWebAuthnAuthenticationContext.java b/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/impl/PopulateWebAuthnAuthenticationContext.java
index 608c47e..2089ede 100644
--- a/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/impl/PopulateWebAuthnAuthenticationContext.java
+++ b/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/impl/PopulateWebAuthnAuthenticationContext.java
@@ -53,7 +53,7 @@ public class PopulateWebAuthnAuthenticationContext extends AbstractAuthenticatio
@Nonnull
private final Function<ProfileRequestContext,WebAuthnAuthenticationContext> webauthnAuthContextCreationStrategy;
- /** Lookup strategy for username to match against Duo identity. */
+ /** Lookup strategy for username to extract. */
@Nonnull private Function<ProfileRequestContext, String> usernameLookupStrategy;
/** Is the username required?*/
@@ -112,8 +112,9 @@ public class PopulateWebAuthnAuthenticationContext extends AbstractAuthenticatio
return;
}
+ //TODO this should not go in this action? Unless we always assume username input step before webauthn
final String username = usernameLookupStrategy.apply(profileRequestContext);
- // username = "philsmart";
+ //username = "philsmart";
if (username == null && usernameRequiredPredicate.test(profileRequestContext)) {
log.error("{} Error creating WebauthnAuthenticationContext, no username found", getLogPrefix());
ActionSupport.buildEvent(profileRequestContext, EventIds.INVALID_PROFILE_CTX);
@@ -121,10 +122,10 @@ public class PopulateWebAuthnAuthenticationContext extends AbstractAuthenticatio
}
if (username != null) {
context.setUsername(username);
- log.debug("Created Webauthn authentication context for user '{}'", context.getUsername());
+ log.debug("Created Webauthn authentication context for user '{}'", username);
return;
}
- log.debug("Created Webauthn authentication context for a discoverable credential (no username)'");
+ log.debug("Created Webauthn authentication context, no previous username provided'");
}
diff --git a/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/impl/ValidateWebAuthnAssertion.java b/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/impl/ValidateWebAuthnAssertion.java
index 5410859..f859c07 100644
--- a/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/impl/ValidateWebAuthnAssertion.java
+++ b/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/impl/ValidateWebAuthnAssertion.java
@@ -108,11 +108,7 @@ public class ValidateWebAuthnAssertion extends AbstractValidationAction {
final PublicKeyCredential<AuthenticatorAssertionResponse, ClientAssertionExtensionOutputs> assertion =
context.getAuthenticatorAssertionResponse();
-
- // TODO these need to be set from lookup earlier in the context in certain types of flows e.g. non-discoverable
- // Null username and userhandle describes a request for a discoverable credential
- context.setUsername(null);
-
+
if (assertion == null) {
log.warn("{} No authenticator assertion found, {} can not authenticate ",
@@ -133,7 +129,7 @@ public class ValidateWebAuthnAssertion extends AbstractValidationAction {
} catch (final AssertionFailureException e) {
log.warn("{} Error validating authenticator assertion for '{}'",
- getLogPrefix(),context.getUsername(), e);
+ getLogPrefix(),context.getUsername() != null ? context.getUsername() : "unknown username", e);
handleError(profileRequestContext, authenticationContext, "InvalidResponseType",
AuthnEventIds.INVALID_CREDENTIALS);
recordFailure(profileRequestContext);
diff --git a/webauthn-impl/src/main/resources/META-INF/net/shibboleth/idp/flows/authn/WebAuthn/webauthn-beans.xml b/webauthn-impl/src/main/resources/META-INF/net/shibboleth/idp/flows/authn/WebAuthn/webauthn-beans.xml
index 52cedc5..d755480 100644
--- a/webauthn-impl/src/main/resources/META-INF/net/shibboleth/idp/flows/authn/WebAuthn/webauthn-beans.xml
+++ b/webauthn-impl/src/main/resources/META-INF/net/shibboleth/idp/flows/authn/WebAuthn/webauthn-beans.xml
@@ -23,7 +23,9 @@
<bean id="IsSecondFactor" scope="prototype"
class="net.shibboleth.idp.plugin.authn.webauthn.context.logic.IsSecondFactor"
p:secondFactorOverride="#{getObject('shibboleth.authn.webauthn.SecondFactorOverride') != null ?
- getObject('shibboleth.authn.webauthn.SecondFactorOverride') : %{idp.authn.webauthn.forceSecondFactorFlow:false}}"/>
+ getObject('shibboleth.authn.webauthn.SecondFactorOverride') : %{idp.authn.webauthn.2fa.forceSecondFactorFlow:false}}"
+ p:allowedPreviousFactors="%{idp.authn.webauthn.2fa.allowedPreviousFactors}"
+ p:denySecondFactor="%{idp.authn.webauthn.2fa.denySecondFactor:false}"/>
<bean id="IsDiscoverableCredentialRequired" scope="prototype"
class="net.shibboleth.idp.plugin.authn.webauthn.context.logic.IsDiscoverableCredentialRequired" />
@@ -64,7 +66,7 @@
<bean id="ValidateWebAuthnAssertion" scope="prototype"
class="net.shibboleth.idp.plugin.authn.webauthn.impl.ValidateWebAuthnAssertion"
- p:webAuthnClient="#{getObject('shibboleth.authn.webauthn.DefaultWebauthnAuthenticationClientFactory')}" />
+ p:webAuthnClient="#{getObject('shibboleth.authn.webauthn.DefaultWebAuthnAuthenticationClientFactory')}" />
</beans>
diff --git a/webauthn-impl/src/main/resources/META-INF/net/shibboleth/idp/flows/authn/WebAuthn/webauthn-flow.xml b/webauthn-impl/src/main/resources/META-INF/net/shibboleth/idp/flows/authn/WebAuthn/webauthn-flow.xml
index 27aca88..7f8cf74 100644
--- a/webauthn-impl/src/main/resources/META-INF/net/shibboleth/idp/flows/authn/WebAuthn/webauthn-flow.xml
+++ b/webauthn-impl/src/main/resources/META-INF/net/shibboleth/idp/flows/authn/WebAuthn/webauthn-flow.xml
@@ -38,8 +38,9 @@
<transition on="proceed" to="GenerateAuthenticationCeremonyOptions" />
</action-state>
- <!-- If we are running after a first factor, perform 2FA only -->
+ <!-- If we are running after a first factor, perform 2FA only. Needs existing username -->
<action-state id="SecondFactorLogin">
+ <evaluate expression="LookupRegisteredCredentials"/>
<evaluate expression="AddUserVerificationNotRequired"/>
<evaluate expression="'proceed'" />
<transition on="proceed" to="GenerateAuthenticationCeremonyOptions" />
diff --git a/webauthn-impl/src/main/resources/net/shibboleth/idp/plugin/authn/webauthn/conf/authn/webauthn.properties b/webauthn-impl/src/main/resources/net/shibboleth/idp/plugin/authn/webauthn/conf/authn/webauthn.properties
index 2e629de..a1c1ce6 100644
--- a/webauthn-impl/src/main/resources/net/shibboleth/idp/plugin/authn/webauthn/conf/authn/webauthn.properties
+++ b/webauthn-impl/src/main/resources/net/shibboleth/idp/plugin/authn/webauthn/conf/authn/webauthn.properties
@@ -9,11 +9,19 @@ idp.authn.webauthn.allowOriginSubdomain = false
## Display debug information about the registration and authentication ceremony on their respective views?
#idp.authn.webauthn.ui.debug = false
-## Registration properties
+## Registration properties.
+
+### Require a residentKey to be created when registering a credential. One-of 'discouraged', 'preferred', 'required'
# idp.authn.webauthn.registration.residentKey = preferred
### The authenticatorAttachment requirement. One-of 'any', 'cross-platform', or 'platform'.
# idp.authn.webauthn.registration.authenticatorAttachment = any
### Require User Verification
# idp.authn.webauthn.registration.userVerification = discouraged
-idp.authn.webauthn.forceSecondFactorFlow = false
\ No newline at end of file
+## Settings for allowing 2FA usage of the WebAuthn flow.
+idp.authn.webauthn.2fa.allowedPreviousFactors = authn/Password
+### Force second factor even if no acceptable previous factor
+#idp.authn.webauthn.2fa.forceSecondFactorFlow = false
+### Deny second factor irrespective of the value of forceSecondFactorFlow and if an acceptable previous factor is found
+### Effectively turning off its ability to act as a second factor only
+#idp.authn.webauthn.2fa.denySecondFactor = false
\ No newline at end of file
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.
More information about the commits
mailing list