[java-idp-plugin-webauthn] branch main updated: Improve signature counter update credential repository function
Phil Smart
philip.smart at jisc.ac.uk
Wed Feb 19 17:35:41 UTC 2025
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=928f69127d8402f22dd0a6123bd057ad46c85b93
The following commit(s) were added to refs/heads/main by this push:
new 928f691 Improve signature counter update credential repository function
928f691 is described below
commit 928f69127d8402f22dd0a6123bd057ad46c85b93
Author: Phil Smart <philip.smart at jisc.ac.uk>
AuthorDate: Wed Feb 19 17:35:37 2025 +0000
Improve signature counter update credential repository function
- Improve reliability
- Improve efficiency and concurrency (less calls to the storage
service)
---
.../storage/WebAuthnCredentialRepository.java | 5 +-
.../webauthn/impl/ValidateWebAuthnAssertion.java | 22 +--
.../IdPStorageServiceCredentialRespository.java | 179 ++++++++++++++++-----
...IdPStorageServiceCredentialRespositoryTest.java | 45 ++++++
4 files changed, 198 insertions(+), 53 deletions(-)
diff --git a/webauthn-api/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/storage/WebAuthnCredentialRepository.java b/webauthn-api/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/storage/WebAuthnCredentialRepository.java
index 15d5f9f..ee0ec74 100644
--- a/webauthn-api/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/storage/WebAuthnCredentialRepository.java
+++ b/webauthn-api/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/storage/WebAuthnCredentialRepository.java
@@ -132,5 +132,8 @@ public interface WebAuthnCredentialRepository extends CredentialRepository {
*
* @return true iff the credential was remove, false otherwise.
*/
- boolean removeRegistrationByUsernameAndCredentialId(@Nonnull String username, @Nonnull ByteArray credentialId);
+ boolean removeRegistrationByUsernameAndCredentialId(final @Nonnull String username,
+ final @Nonnull ByteArray credentialId);
+
+
}
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 a7e80ab..250df3a 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
@@ -41,6 +41,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.exception.CredentialRepositoryException;
import net.shibboleth.idp.plugin.authn.webauthn.principal.WebAuthnUserIdPrinicpal;
import net.shibboleth.idp.plugin.authn.webauthn.storage.WebAuthnCredentialRepository;
import net.shibboleth.shared.annotation.constraint.NonnullAfterInit;
@@ -109,7 +110,7 @@ public class ValidateWebAuthnAssertion extends AbstractAuditingValidationAction
/**
* Set the credential repository used to store WebAuthn credentials.
*
- * @param repository The repository to set.
+ * @param repository the repository to set.
*/
public void setCredentialRepository(@Nonnull final WebAuthnCredentialRepository repository) {
checkSetterPreconditions();
@@ -119,7 +120,7 @@ public class ValidateWebAuthnAssertion extends AbstractAuditingValidationAction
/**
* Set the WebAuthn client used to handle validation of the authentication ceremony.
*
- * @param client The webauthnClient to set.
+ * @param client the webauthnClient to set.
*/
public void setWebAuthnClient(@Nonnull final WebAuthnAuthenticationClient client) {
checkSetterPreconditions();
@@ -130,7 +131,7 @@ public class ValidateWebAuthnAssertion extends AbstractAuditingValidationAction
* Set the flag to determine if we should update the signature count on the credential in the repository
* after successful validation?
*
- * @param flag The flag to set.
+ * @param flag the flag to set.
*/
public void setUpdateSignatureCount(final boolean flag) {
checkSetterPreconditions();
@@ -141,13 +142,12 @@ public class ValidateWebAuthnAssertion extends AbstractAuditingValidationAction
* Set the predicate to determine if we should update the signature count on the credential in the repository
* after successful validation?
*
- * @param predicate The predicate to set.
+ * @param predicate the predicate to set.
*/
public void setUpdateSignatureCountPredicate(@Nonnull final Predicate<ProfileRequestContext> predicate) {
checkSetterPreconditions();
updateSignatureCount = Constraint.isNotNull(predicate, "updateSignatureCount predicate can not be null");
- }
-
+ }
@Override
protected boolean doPreExecute(@Nonnull final ProfileRequestContext profileRequestContext,
@@ -223,7 +223,7 @@ public class ValidateWebAuthnAssertion extends AbstractAuditingValidationAction
}
}
-
+
/**
* 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.
@@ -244,8 +244,12 @@ public class ValidateWebAuthnAssertion extends AbstractAuditingValidationAction
}
final long newSignatureCount = assertion.getResponse().getParsedAuthenticatorData().getSignatureCounter();
- if (!credentialRepository.updateSignatureCounter(username, credentialId, newSignatureCount)) {
- throw new AssertionFailureException("Failed to update signature counter");
+ try {
+ if (!credentialRepository.updateSignatureCounter(username, credentialId, newSignatureCount)) {
+ throw new AssertionFailureException("Failed to update signature counter");
+ }
+ } catch (final CredentialRepositoryException e) {
+ throw new AssertionFailureException(e);
}
}
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 e03852e..6175bf7 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
@@ -35,6 +35,7 @@ import org.opensaml.storage.StorageCapabilities;
import org.opensaml.storage.StorageRecord;
import org.opensaml.storage.StorageSerializer;
import org.opensaml.storage.StorageService;
+import org.opensaml.storage.VersionMismatchException;
import org.slf4j.Logger;
import com.yubico.webauthn.CredentialRepository;
@@ -87,7 +88,7 @@ public class IdPStorageServiceCredentialRespository extends AbstractIdentifiable
/**
* 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.
+ * can occur within a write lock (by the same thread).
*/
@NonnullAfterInit private ReentrantReadWriteLock lock;
@@ -289,7 +290,34 @@ public class IdPStorageServiceCredentialRespository extends AbstractIdentifiable
} finally {
readLock.unlock();
}
- }
+ }
+
+ /**
+ * Get credential registrations by username and return the storage record version.
+ *
+ * @param username the username to find registrations for
+ *
+ * @return the set of registered credentials associated to the user and the version of the storage record
+ */
+ @Nonnull
+ private VersionedCredentialSet getRegistrationsByUsernameWithVersion(final String username) {
+ checkComponentActive();
+ final Lock readLock = lock.readLock();
+ try {
+ readLock.lock();
+ final StorageRecord<Set<CredentialRecord>> registration =
+ storageService.read(STORAGE_CONTEXT, username);
+ if (registration != null) {
+ final Set<CredentialRecord> credentials = registration.getValue(serializer, STORAGE_CONTEXT, username);
+ return new VersionedCredentialSet(registration.getVersion(), credentials);
+ }
+ return new VersionedCredentialSet(0, CollectionSupport.emptySet());
+ } catch (final IOException e) {
+ throw new CredentialRepositoryException(e);
+ } finally {
+ readLock.unlock();
+ }
+ }
/** {@inheritDoc} */
@Override
@@ -338,17 +366,19 @@ public class IdPStorageServiceCredentialRespository extends AbstractIdentifiable
try {
writeLock.lock();
- final Set<CredentialRecord> existingRegistrations = getRegistrationsByUsername(username);
- if (!existingRegistrations.isEmpty()) {
- final Set<CredentialRecord> updateSet = new LinkedHashSet<>(existingRegistrations);
+ final VersionedCredentialSet existingRegistrations = getRegistrationsByUsernameWithVersion(username);
+ if (!existingRegistrations.getCredentials().isEmpty()) {
+ final Set<CredentialRecord> updateSet = new LinkedHashSet<>(existingRegistrations.getCredentials());
updateSet.add(reg);
- return storageService.update(STORAGE_CONTEXT, username, updateSet, serializer, null);
+ final Long updatedVersion = storageService.updateWithVersion(
+ existingRegistrations.getVersion(), STORAGE_CONTEXT, username, updateSet, serializer, null);
+ return updatedVersion != null;
} else {
final Set<CredentialRecord> addSet = new LinkedHashSet<>(1);
addSet.add(reg);
return storageService.create(STORAGE_CONTEXT, username, addSet, serializer, null);
}
- } catch (final IOException e) {
+ } catch (final IOException | VersionMismatchException e) {
throw new CredentialRepositoryException(e);
} finally {
writeLock.unlock();
@@ -363,23 +393,26 @@ public class IdPStorageServiceCredentialRespository extends AbstractIdentifiable
final Lock writeLock = lock.writeLock();
try {
writeLock.lock();
- final Set<CredentialRecord> existingRegistrations = getRegistrationsByUsername(username);
- if (!existingRegistrations.isEmpty()) {
- final Set<CredentialRecord> updateSet = new LinkedHashSet<>(existingRegistrations);
+ final VersionedCredentialSet existingRegistrations = getRegistrationsByUsernameWithVersion(username);
+ if (!existingRegistrations.getCredentials().isEmpty()) {
+ final Set<CredentialRecord> updateSet = new LinkedHashSet<>(existingRegistrations.getCredentials());
updateSet.remove(credentialRegistration);
if (updateSet.isEmpty()) {
//remove the entire storage record
- return storageService.delete(STORAGE_CONTEXT, username);
+ return storageService.deleteWithVersion(
+ existingRegistrations.getVersion(), STORAGE_CONTEXT, username);
} else {
//else, add back what remains
assert serializer != null;
- return storageService.update(STORAGE_CONTEXT, username, updateSet, serializer, null);
+ final Long updatedVersion = storageService.updateWithVersion(
+ existingRegistrations.getVersion(), STORAGE_CONTEXT, username, updateSet, serializer, null);
+ return updatedVersion != null;
}
}
// Nothing to do if the registration does not exist
return false;
- } catch (final IOException e) {
+ } catch (final IOException | VersionMismatchException e) {
throw new CredentialRepositoryException(e);
} finally {
writeLock.unlock();
@@ -399,16 +432,16 @@ public class IdPStorageServiceCredentialRespository extends AbstractIdentifiable
final String usernameKey = i.next();
assert usernameKey != null;
- final Set<CredentialRecord> existingRegistrations = getRegistrationsByUsername(usernameKey);
+ final VersionedCredentialSet existingRegistrations = getRegistrationsByUsernameWithVersion(usernameKey);
- if (existingRegistrations.isEmpty()) {
+ if (existingRegistrations.getCredentials().isEmpty()) {
log.trace("No existing registrations, nothing to remove");
// Nothing to do
continue;
}
// Find an matching credential from the existing registration
- final List<CredentialRecord> matchingRegistrations = existingRegistrations.stream()
+ final List<CredentialRecord> matchingRegistrations = existingRegistrations.getCredentials().stream()
.filter(reg -> reg.getCredential().getCredentialId().equals(credentialId))
.collect(CollectionSupport.nonnullCollector(Collectors.toList())).get();
@@ -418,17 +451,19 @@ public class IdPStorageServiceCredentialRespository extends AbstractIdentifiable
}
final CredentialRecord registrationToRemove = matchingRegistrations.get(0);
- final Set<CredentialRecord> updateSet = new LinkedHashSet<>(existingRegistrations);
+ final Set<CredentialRecord> updateSet = new LinkedHashSet<>(existingRegistrations.getCredentials());
updateSet.remove(registrationToRemove);
if (updateSet.isEmpty()) {
//remove the entire storage record
- if (storageService.delete(STORAGE_CONTEXT, usernameKey)) {
+ if (storageService.deleteWithVersion(
+ existingRegistrations.getVersion(), STORAGE_CONTEXT, usernameKey)) {
removalCount++;
}
} else {
//else, add back what remains
assert serializer != null;
- if (storageService.update(STORAGE_CONTEXT, usernameKey, updateSet, serializer, null)) {
+ if (storageService.updateWithVersion(existingRegistrations.getVersion(),
+ STORAGE_CONTEXT, usernameKey, updateSet, serializer, null)!=null) {
removalCount ++;
}
}
@@ -436,7 +471,7 @@ public class IdPStorageServiceCredentialRespository extends AbstractIdentifiable
// Nothing to do if the registration does not exist
return removalCount;
- } catch (final IOException e) {
+ } catch (final IOException | VersionMismatchException e) {
throw new CredentialRepositoryException(e);
} finally {
writeLock.unlock();
@@ -450,12 +485,12 @@ public class IdPStorageServiceCredentialRespository extends AbstractIdentifiable
final Lock writeLock = lock.writeLock();
try {
writeLock.lock();
- final Set<CredentialRecord> existingRegistrations = getRegistrationsByUsername(username);
- if (!existingRegistrations.isEmpty()) {
- final Set<CredentialRecord> updateSet = new LinkedHashSet<>(existingRegistrations);
+ final VersionedCredentialSet existingRegistrations = getRegistrationsByUsernameWithVersion(username);
+ if (!existingRegistrations.getCredentials().isEmpty()) {
+ final Set<CredentialRecord> updateSet = new LinkedHashSet<>(existingRegistrations.getCredentials());
// Find an matching credential from the existing registration
- final List<CredentialRecord> matchingRegistrations = existingRegistrations.stream()
+ final List<CredentialRecord> matchingRegistrations = existingRegistrations.getCredentials().stream()
.filter(reg -> reg.getCredential().getCredentialId().equals(credentialId))
.collect(CollectionSupport.nonnullCollector(Collectors.toList())).get();
@@ -467,17 +502,20 @@ public class IdPStorageServiceCredentialRespository extends AbstractIdentifiable
updateSet.remove(matchingRegistrations.iterator().next());
if (updateSet.isEmpty()) {
//remove the entire storage record
- return storageService.delete(STORAGE_CONTEXT, username);
+ return storageService.deleteWithVersion(
+ existingRegistrations.getVersion(), STORAGE_CONTEXT, username);
} else {
//else, add back what remains
assert serializer != null;
- return storageService.update(STORAGE_CONTEXT, username, updateSet, serializer, null);
+ final Long updatedVersion = storageService.updateWithVersion(
+ existingRegistrations.getVersion(), STORAGE_CONTEXT, username, updateSet, serializer, null);
+ return updatedVersion != null;
}
}
// Nothing to do if the registration does not exist
return false;
- } catch (final IOException e) {
+ } catch (final IOException | VersionMismatchException e) {
throw new CredentialRepositoryException(e);
} finally {
writeLock.unlock();
@@ -491,8 +529,12 @@ public class IdPStorageServiceCredentialRespository extends AbstractIdentifiable
final Lock writeLock = lock.writeLock();
try {
writeLock.lock();
- final Optional<CredentialRecord> credential =
- getRegistrationByUsernameAndCredentialId(username, credentialId);
+
+ final VersionedCredentialSet existingRegistrations = getRegistrationsByUsernameWithVersion(username);
+ final Optional<CredentialRecord> credential = existingRegistrations.getCredentials().stream()
+ .filter(credReg -> credentialId.equals(credReg.getCredential().getCredentialId()))
+ .findFirst();
+
if (credential.isEmpty()) {
log.warn("Can not update signature count for user '{}' and credential '{}'. "
+ "No existing credential found.", username, credentialId.getBase64());
@@ -505,24 +547,31 @@ public class IdPStorageServiceCredentialRespository extends AbstractIdentifiable
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 CredentialRecord updatedRegistration = credential.get().withCredential(updatedCredential);
+ final CredentialRecord updatedRegistration = credential.get().toBuilder()
+ .withCredential(updatedCredential)
+ .build();
- // Remove the old
- final CredentialRecord existingCredential = credential.get();
- assert existingCredential != null && updatedRegistration != null;
+ final Set<CredentialRecord> updateCredentialSet =
+ new LinkedHashSet<>(existingRegistrations.getCredentials());
+ updateCredentialSet.remove(credential.get());
+ updateCredentialSet.add(updatedRegistration);
- if (!removeRegistrationByUsername(username, existingCredential)) {
- log.warn("Can not update signature count for user '{}' and credential '{}'. "
- + "Can not remove existing signature count.", username, credentialId.getBase64());
+ if (updateCredentialSet.isEmpty()) {
+ // We are updating, so this should not be possible
+ log.debug("Can not update signature count for user '{}' and credential '{}'. "
+ + "Update set is empty.", 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;
+
+ try {
+ final Long updatedVersion = storageService.updateWithVersion(
+ existingRegistrations.getVersion(), STORAGE_CONTEXT, username, updateCredentialSet,
+ serializer, null);
+ return updatedVersion != null;
+ } catch (IOException | VersionMismatchException e) {
+ throw new CredentialRepositoryException(e);
+ }
+
} finally {
writeLock.unlock();
}
@@ -552,6 +601,50 @@ public class IdPStorageServiceCredentialRespository extends AbstractIdentifiable
readLock.unlock();
}
}
+
+ /**
+ * A class to wrap a set of deserialized credentials alongside the storage record version
+ * reported by the storage service.
+ */
+ private class VersionedCredentialSet {
+
+ /** The storage record version.*/
+ private final long version;
+
+ /** The wrapped set of credentials.*/
+ @Nonnull @Unmodifiable private final Set<CredentialRecord> credentials;
+
+ /**
+ * Constructor.
+ *
+ * @param version the storage record version
+ * @param credentials the wrapped set of credentials
+ */
+ public VersionedCredentialSet(final long ver, @Nonnull final Set<CredentialRecord> creds) {
+ credentials = Constraint.isNotNull(creds, "credential records can not be null");
+ version = ver;
+ }
+
+ /**
+ * Get the storage record version.
+ *
+ * @return the version.
+ */
+ public long getVersion() {
+ return version;
+ }
+
+ /**
+ * Get the wrapped set of credentials.
+ *
+ * @return the credentials.
+ */
+ @Nonnull @Unmodifiable @NotLive public Set<CredentialRecord> getCredentials() {
+ return CollectionSupport.copyToSet(credentials);
+ }
+
+
+ }
}
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 bc0a1e5..e1f08a8 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
@@ -468,6 +468,39 @@ public class IdPStorageServiceCredentialRespositoryTest extends AbstractWebAuthn
}
+ @SuppressWarnings("null")
+ @Test
+ public void testUpdateSignatureCounter_TwoCredentials() throws Exception {
+
+ final CredentialRecord registration = createRegistration("jdoe", "John Doe", "user-handle".getBytes());
+ repo.addRegistrationByUsername("jdoe", registration);
+ final CredentialRecord registrationTwo = createRegistration("jdoe", "John Doe", "user-handle".getBytes());
+ repo.addRegistrationByUsername("jdoe", registrationTwo);
+
+ var registrations = repo.getRegistrationsByUsername("jdoe");
+ assertNotNull(registrations);
+ assertEquals(registrations.size(), 2);
+
+ repo.updateSignatureCounter("jdoe", registrationTwo.getCredential().getCredentialId(), 10);
+ registrations = repo.getRegistrationsByUsername("jdoe");
+ assertNotNull(registrations);
+ assertEquals(registrations.size(), 2);
+
+ final Optional<CredentialRecord> registrationOneFound = registrations.stream()
+ .filter(cred -> cred.getCredential().getCredentialId()
+ .equals(registration.getCredential().getCredentialId())).findFirst();
+
+ final Optional<CredentialRecord> registrationTwoFound = registrations.stream()
+ .filter(cred -> cred.getCredential().getCredentialId()
+ .equals(registrationTwo.getCredential().getCredentialId())).findFirst();
+
+ assertTrue(registrationOneFound.isPresent());
+ assertTrue(registrationTwoFound.isPresent());
+ assertEquals(registrationOneFound.get().getCredential().getSignatureCount(),0);
+ assertEquals(registrationTwoFound.get().getCredential().getSignatureCount(),10);
+
+ }
+
@SuppressWarnings("null")
@Test
public void testUpdateSignatureCounter_NoCredential() throws Exception {
@@ -739,6 +772,18 @@ public class IdPStorageServiceCredentialRespositoryTest extends AbstractWebAuthn
}
+ @SuppressWarnings("null")
+ @Test
+ public final void testVersioning() throws Exception {
+
+ final CredentialRecord registration = createRegistration("jdoe", "John Doe", "user-handle".getBytes());
+ repo.addRegistrationByUsername("jdoe", registration);
+ repo.removeRegistrationByUsername("jdoe", registration);
+ registration.toBuilder().withLastUsedTime(Instant.now());
+ repo.addRegistrationByUsername("jdoe", registration);
+
+ }
+
/* Failure here would be non-deterministic if it happened.*/
@Test
public final void testThreadSafetyUpdateSignatureCount() throws Exception {
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.
More information about the commits
mailing list