[java-idp-plugin-webauthn] branch main updated: Add signature counter update to the credential repository interface
Phil Smart
philip.smart at jisc.ac.uk
Fri Apr 12 11:35:04 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=6607e7ba7322e19cc692d01c6054153c6114f12c
The following commit(s) were added to refs/heads/main by this push:
new 6607e7b Add signature counter update to the credential repository interface
6607e7b is described below
commit 6607e7ba7322e19cc692d01c6054153c6114f12c
Author: Phil Smart <philip.smart at jisc.ac.uk>
AuthorDate: Fri Apr 12 12:35:01 2024 +0100
Add signature counter update to the credential repository interface
- Make this an explicit operation for the credential repository adaptor
- Add the new method to the validate WebAuthn assertion action
- Minor cleanup
---
.../authn/webauthn/authn/AssertionResult.java | 6 +-
.../StorageServiceCredentialRepository.java | 15 ++++-
.../webauthn/impl/AbstractWebAuthnBaseAction.java | 4 +-
.../webauthn/impl/ValidateWebAuthnAssertion.java | 54 ++++++++++++++-
.../metadata/FidoMetadataServiceFactory.java | 8 ++-
.../IdPStorageServiceCredentialRespository.java | 52 ++++++++++++++-
.../idp/flows/authn/WebAuthn/webauthn-beans.xml | 3 +-
...IdPStorageServiceCredentialRespositoryTest.java | 78 ++++++++++++++++++++++
8 files changed, 203 insertions(+), 17 deletions(-)
diff --git a/webauthn-api/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/authn/AssertionResult.java b/webauthn-api/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/authn/AssertionResult.java
index 7bbed4e..8870655 100644
--- a/webauthn-api/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/authn/AssertionResult.java
+++ b/webauthn-api/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/authn/AssertionResult.java
@@ -46,7 +46,7 @@ public class AssertionResult {
/**
* @return Returns the username.
*/
- public final String getUsername() {
+ @Nonnull public final String getUsername() {
return username;
}
@@ -64,12 +64,10 @@ public class AssertionResult {
this.username = builder.username;
this.signatureCounterValid = builder.signatureCounterValid;
}
-
public static Builder builder() {
return new Builder();
}
-
public static final class Builder {
private boolean success;
@@ -84,7 +82,7 @@ public class AssertionResult {
return this;
}
- public Builder withUsername(final String username) {
+ public Builder withUsername(@Nonnull final String username) {
this.username = username;
return this;
}
diff --git a/webauthn-api/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/storage/StorageServiceCredentialRepository.java b/webauthn-api/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/storage/StorageServiceCredentialRepository.java
index a1f6e88..16cbb1b 100644
--- a/webauthn-api/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/storage/StorageServiceCredentialRepository.java
+++ b/webauthn-api/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/storage/StorageServiceCredentialRepository.java
@@ -51,6 +51,19 @@ public interface StorageServiceCredentialRepository extends CredentialRepository
* @return true iff the registration was added. False otherwise.
*/
boolean addRegistrationByUsername(@Nonnull final String username, @Nonnull final CredentialRegistration credential);
+
+ /**
+ * Update the signature counter of the credential that belongs to the user.
+ *
+ * @param username the username of the user to update the signature counter for
+ * @param credentialId the identifier of the credential to update the signature counter for
+ * @param newSignatureCount the new signature counter value
+ *
+ * @return true iff the registration was updated, false otherwise.
+ */
+ boolean updateSignatureCounter(
+ @Nonnull final String username, @Nonnull ByteArray credentialId, long newSignatureCount);
+
/**
* Get the credential belonging to the user by its credential identifier.
@@ -61,7 +74,7 @@ public interface StorageServiceCredentialRepository extends CredentialRepository
* @return the credential if found, otherwise an empty {@link Optional}.
*/
@Nonnull Optional<CredentialRegistration> getRegistrationByUsernameAndCredentialId(@Nonnull final String username,
- @Nonnull final ByteArray id);
+ @Nonnull final ByteArray credentialId);
/**
* Remove the given registration for the give user.
diff --git a/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/impl/AbstractWebAuthnBaseAction.java b/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/impl/AbstractWebAuthnBaseAction.java
index 44bbe39..482c410 100644
--- a/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/impl/AbstractWebAuthnBaseAction.java
+++ b/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/impl/AbstractWebAuthnBaseAction.java
@@ -69,7 +69,6 @@ public abstract class AbstractWebAuthnBaseAction extends AbstractProfileAction {
@NonnullBeforeExec private WebAuthnAuthenticationClient webAuthnClient;
/** The credential repository to store valid credentials in.*/
- // TODO replace with an adaptor to the storage service?
@NonnullAfterInit private StorageServiceCredentialRepository credentialRepository;
@@ -113,8 +112,7 @@ public abstract class AbstractWebAuthnBaseAction extends AbstractProfileAction {
webAuthnBaseContextLookupStrategy =
Constraint.isNotNull(strategy, "WebauthnContextLookuplookup strategy cannot be null");
- }
-
+ }
/**
* Set the credential repository used to store WebAuthn credentials.
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 11e6e55..a70568c 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
@@ -11,6 +11,7 @@ import org.opensaml.profile.context.ProfileRequestContext;
import org.slf4j.Logger;
import com.yubico.webauthn.data.AuthenticatorAssertionResponse;
+import com.yubico.webauthn.data.ByteArray;
import com.yubico.webauthn.data.ClientAssertionExtensionOutputs;
import com.yubico.webauthn.data.PublicKeyCredential;
import com.yubico.webauthn.data.PublicKeyCredentialRequestOptions;
@@ -23,6 +24,7 @@ import net.shibboleth.idp.plugin.authn.webauthn.authn.AssertionResult;
import net.shibboleth.idp.plugin.authn.webauthn.client.WebAuthnAuthenticationClient;
import net.shibboleth.idp.plugin.authn.webauthn.context.WebAuthnAuthenticationContext;
import net.shibboleth.idp.plugin.authn.webauthn.exception.AssertionFailureException;
+import net.shibboleth.idp.plugin.authn.webauthn.storage.StorageServiceCredentialRepository;
import net.shibboleth.shared.annotation.constraint.NonnullAfterInit;
import net.shibboleth.shared.annotation.constraint.NonnullBeforeExec;
import net.shibboleth.shared.component.ComponentInitializationException;
@@ -50,7 +52,10 @@ public class ValidateWebAuthnAssertion extends AbstractValidationAction {
@NonnullBeforeExec private WebAuthnAuthenticationContext context;
/** The WebAuthn client to use.*/
- @NonnullAfterInit private WebAuthnAuthenticationClient webAuthnClient;
+ @NonnullAfterInit private WebAuthnAuthenticationClient webAuthnClient;
+
+ /** The credential repository to store valid credentials in.*/
+ @NonnullAfterInit private StorageServiceCredentialRepository credentialRepository;
/** The options used to create the authentication request.*/
@NonnullBeforeExec private PublicKeyCredentialRequestOptions publicKeyCredentialRequestOptions;
@@ -68,9 +73,21 @@ public class ValidateWebAuthnAssertion extends AbstractValidationAction {
if (webAuthnClient == null) {
throw new ComponentInitializationException("WebAuthn client can not be null. Configuration error.");
}
+ if (credentialRepository == null) {
+ throw new ComponentInitializationException("CredentialRepository can not be null");
+ }
super.doInitialize();
}
+ /**
+ * Set the credential repository used to store WebAuthn credentials.
+ *
+ * @param repository The respository to set.
+ */
+ public void setCredentialRepository(@Nonnull final StorageServiceCredentialRepository repository) {
+ checkSetterPreconditions();
+ credentialRepository = Constraint.isNotNull(repository, "Credential respository can not be null");
+ }
/**
* Set the WebAuthn client used to handle registration and authentication ceremonies.
@@ -111,9 +128,9 @@ public class ValidateWebAuthnAssertion extends AbstractValidationAction {
final PublicKeyCredential<AuthenticatorAssertionResponse, ClientAssertionExtensionOutputs> assertion =
context.getAuthenticatorAssertionResponse();
-
+ // TODO username can be null in a usernameless flow
if (assertion == null) {
- log.warn("{} No authenticator assertion found, {} can not authenticate ",
+ log.warn("{} No authenticator assertion found, {} can not authenticate",
getLogPrefix(),context.getUsername());
handleError(profileRequestContext, authenticationContext, "InvalidResponseType",
AuthnEventIds.INVALID_CREDENTIALS);
@@ -129,6 +146,12 @@ public class ValidateWebAuthnAssertion extends AbstractValidationAction {
throw new AssertionFailureException("Assestion was not valid");
}
+ if (!result.isSignatureCounterValid()) {
+ throw new AssertionFailureException("Assestion was not valid, signature count is invalid");
+ }
+ // Update the signature count with that from the assertion. It has already been validated at this point
+ updateSignatureCount(result.getUsername(), assertion);
+
log.info("{} WebAuthn authentication succeeded for '{}'",getLogPrefix(),result.getUsername());
context.setUsername(result.getUsername());
buildAuthenticationResult(profileRequestContext, authenticationContext);
@@ -144,6 +167,31 @@ public class ValidateWebAuthnAssertion extends AbstractValidationAction {
}
+ /**
+ * Take the new signature count from the authenticator assertion response and update the stored credential with that
+ * value. It is assumed the assertion and signature counter is valid by the time this method is called.
+ *
+ * @param username the username of the user to update signature count for
+ * @param assertion the assertion with the credential Id to update, and the new signature count
+ * @throws AssertionFailureException
+ */
+ private void updateSignatureCount(@Nonnull final String username, @Nonnull
+ final PublicKeyCredential<AuthenticatorAssertionResponse, ClientAssertionExtensionOutputs> assertion)
+ throws AssertionFailureException {
+
+ final ByteArray credentialId = assertion.getId();
+ if (credentialId == null) {
+ throw new AssertionFailureException("Can not update signature count for user '"+username+"' "
+ + "and credential '"+assertion.getId()+"'. Assertion goes not contain the credential Id.");
+ }
+ final long newSignatureCount = assertion.getResponse().getParsedAuthenticatorData().getSignatureCounter();
+
+ if (!credentialRepository.updateSignatureCounter(username, credentialId, newSignatureCount)) {
+ throw new AssertionFailureException("Failed to update signature counter");
+ }
+
+ }
+
/** {@inheritDoc} */
@Override protected void buildAuthenticationResult(@Nonnull final ProfileRequestContext profileRequestContext,
@Nonnull final AuthenticationContext authenticationContext) {
diff --git a/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/metadata/FidoMetadataServiceFactory.java b/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/metadata/FidoMetadataServiceFactory.java
index 1019a64..0ea2ff9 100644
--- a/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/metadata/FidoMetadataServiceFactory.java
+++ b/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/metadata/FidoMetadataServiceFactory.java
@@ -278,9 +278,13 @@ public class FidoMetadataServiceFactory extends AbstractIdentifiableInitializabl
*
* @param revocationLists The crls to set.
*/
- public synchronized void setCrls(@Nonnull final List<Resource> revocationLists) {
+ public synchronized void setCrls(@Nullable final List<Resource> revocationLists) {
checkSetterPreconditions();
- crls = Constraint.isNotNull(revocationLists, "CRL list can not be null");
+ if (revocationLists != null) {
+ crls = CollectionSupport.copyToList(revocationLists);
+ } else {
+ crls = CollectionSupport.emptyList();
+ }
}
/**
diff --git a/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/storage/impl/IdPStorageServiceCredentialRespository.java b/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/storage/impl/IdPStorageServiceCredentialRespository.java
index d424a0d..26f8d4f 100644
--- a/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/storage/impl/IdPStorageServiceCredentialRespository.java
+++ b/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/storage/impl/IdPStorageServiceCredentialRespository.java
@@ -22,7 +22,6 @@ import java.util.LinkedHashSet;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.locks.Lock;
-import java.util.concurrent.locks.ReadWriteLock;
import java.util.concurrent.locks.ReentrantReadWriteLock;
import java.util.stream.Collectors;
@@ -80,8 +79,11 @@ public class IdPStorageServiceCredentialRespository extends AbstractIdentifiable
/** The composed storage Service.*/
@NonnullAfterInit private EnumeratableStorageService storageService;
- /** A shared lock to synchronize access to read and write operations. */
- @NonnullAfterInit private ReadWriteLock lock;
+ /**
+ * A shared lock to synchronize access to read and write operations. Needs to be a reentrant type, so a read
+ * can occur within a write lock.
+ */
+ @NonnullAfterInit private ReentrantReadWriteLock lock;
/**
* Set the storage service to store registered credentials.
@@ -354,5 +356,49 @@ public class IdPStorageServiceCredentialRespository extends AbstractIdentifiable
}
}
+ /** {@inheritDoc} */
+ @Override
+ public boolean updateSignatureCounter(@Nonnull final String username, @Nonnull final ByteArray credentialId,
+ final long newSignatureCount) {
+ final Lock writeLock = lock.writeLock();
+ try {
+ writeLock.lock();
+ final Optional<CredentialRegistration> credential =
+ getRegistrationByUsernameAndCredentialId(username, credentialId);
+ if (credential.isEmpty()) {
+ log.warn("Can not update signature count for user '{}' and credential '{}'. "
+ + "No existing credential found.", username, credentialId.getBase64());
+ return false;
+ }
+ // Only update the signature counter, keep other fields the same as those already registered
+ final RegisteredCredential updatedCredential = credential.get().getCredential().toBuilder()
+ .signatureCount(newSignatureCount)
+ .build();
+ assert updatedCredential != null;
+ // Create a new credential from the existing credential, keeping the all the fields the same other than
+ // the registered credential (which itself should only have the signature counter updated).
+ final CredentialRegistration updatedRegistration = credential.get().withCredential(updatedCredential);
+
+ // Remove the old
+ final CredentialRegistration existingCredential = credential.get();
+ assert existingCredential != null && updatedRegistration != null;
+
+ if (!removeRegistrationByUsername(username, existingCredential)) {
+ log.warn("Can not update signature count for user '{}' and credential '{}'. "
+ + "Can not remove existing signature count.", username, credentialId.getBase64());
+ return false;
+ }
+ // Add the new
+ if (!addRegistrationByUsername(username, updatedRegistration)) {
+ log.warn("Can not update signature count for user '{}' and credential '{}'. "
+ + "Can not add new signature count.", username, credentialId.getBase64());
+ return false;
+ }
+ return true;
+ } finally {
+ writeLock.unlock();
+ }
+ }
+
}
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 d2625ae..9413bc9 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
@@ -93,7 +93,8 @@
<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')}"
+ p:credentialRepository-ref="shibboleth.authn.webauthn.DefaultCredentialRepository"/>
</beans>
diff --git a/webauthn-impl/src/test/java/net/shibboleth/idp/plugin/authn/webauthn/storage/impl/IdPStorageServiceCredentialRespositoryTest.java b/webauthn-impl/src/test/java/net/shibboleth/idp/plugin/authn/webauthn/storage/impl/IdPStorageServiceCredentialRespositoryTest.java
index 34746ed..6c2c89c 100644
--- a/webauthn-impl/src/test/java/net/shibboleth/idp/plugin/authn/webauthn/storage/impl/IdPStorageServiceCredentialRespositoryTest.java
+++ b/webauthn-impl/src/test/java/net/shibboleth/idp/plugin/authn/webauthn/storage/impl/IdPStorageServiceCredentialRespositoryTest.java
@@ -176,6 +176,54 @@ public class IdPStorageServiceCredentialRespositoryTest extends AbstractWebAuthn
assertEquals(registrations.size(), 0);
}
+ @SuppressWarnings("null")
+ @Test
+ public void testUpdateSignatureCounter() throws Exception {
+
+ final CredentialRegistration registration = createRegistration("jdoe", "John Doe", "user-handle".getBytes());
+ repo.addRegistrationByUsername("jdoe", registration);
+
+ var registrations = repo.getRegistrationsByUsername("jdoe");
+ assertNotNull(registrations);
+ assertEquals(registrations.size(), 1);
+ var iterator = registrations.iterator();
+ var credReg = iterator.next();
+ assertEquals(credReg.getUsername(),"jdoe");
+ assertEquals(credReg.getCredential().getCredentialId(),
+ registration.getCredential().getCredentialId());
+
+ repo.updateSignatureCounter("jdoe", credReg.getCredential().getCredentialId(), 10);
+ registrations = repo.getRegistrationsByUsername("jdoe");
+ assertNotNull(registrations);
+ assertEquals(registrations.size(), 1);
+ iterator = registrations.iterator();
+ credReg = iterator.next();
+ assertEquals(credReg.getCredential().getSignatureCount(),10);
+
+
+ }
+
+ @SuppressWarnings("null")
+ @Test
+ public void testUpdateSignatureCounter_NoCredential() throws Exception {
+
+ final CredentialRegistration registration = createRegistration("jdoe", "John Doe", "user-handle".getBytes());
+ repo.addRegistrationByUsername("jdoe", registration);
+
+ final var registrations = repo.getRegistrationsByUsername("jdoe");
+ assertNotNull(registrations);
+ assertEquals(registrations.size(), 1);
+ final var iterator = registrations.iterator();
+ final var credReg = iterator.next();
+ assertEquals(credReg.getUsername(),"jdoe");
+ assertEquals(credReg.getCredential().getCredentialId(),
+ registration.getCredential().getCredentialId());
+
+ final boolean updated = repo.updateSignatureCounter("jdoe", new ByteArray("000".getBytes()), 10);
+ assertFalse(updated);
+
+ }
+
@Test
public void testRemoveOneRegistrationFromTwoByUsername() throws Exception {
@@ -394,5 +442,35 @@ public class IdPStorageServiceCredentialRespositoryTest extends AbstractWebAuthn
}
+ /* Failure here would be non-deterministic if it happened.*/
+ @Test
+ public final void testThreadSafetyUpdateSignatureCount() throws Exception {
+
+ final CredentialRegistration registration = createRegistration("jdoe", "John Doe", "user-handle".getBytes());
+ repo.addRegistrationByUsername("jdoe", registration);
+
+ final ExecutorService service = Executors.newFixedThreadPool(3);
+ final Collection<Future<Boolean>> futures = new ArrayList<>(3);
+
+ futures.add(service.submit(()->repo.updateSignatureCounter(
+ "jdoe", registration.getCredential().getCredentialId(), 2)));
+ futures.add(service.submit(()->{
+ final var cred = repo.getRegistrationByUsernameAndCredentialId(
+ "jdoe", registration.getCredential().getCredentialId());
+ // Important, this should not be able to read a state where the update has just removed the old credential
+ // But not yet added back to the updated credential
+ if (cred.isEmpty()) return false;
+ // Accept this read either happens before the update or after.
+ return cred.get().getCredential().getSignatureCount() == 2 ||
+ cred.get().getCredential().getSignatureCount() == 0;
+ }));
+
+ for (final Future<Boolean> f : futures) {
+ final Boolean success = f.get();
+ assertTrue(success);
+ }
+
+ }
+
}
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.
More information about the commits
mailing list