[java-idp-plugin-webauthn] branch main updated: Add test: one user can not register credential for another user
Phil Smart
philip.smart at jisc.ac.uk
Mon Aug 12 14:19:25 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=15e2fefa7d64155b9dd3a6ebb8f36540193bdc65
The following commit(s) were added to refs/heads/main by this push:
new 15e2fef Add test: one user can not register credential for another user
15e2fef is described below
commit 15e2fefa7d64155b9dd3a6ebb8f36540193bdc65
Author: Phil Smart <philip.smart at jisc.ac.uk>
AuthorDate: Mon Aug 12 15:19:22 2024 +0100
Add test: one user can not register credential for another user
---
.../admin/impl/StorePublicKeyCredential.java | 3 +-
.../ValidateAuthenticatorAttestationResponse.java | 2 +-
.../webauthn/flow/AbstractWebAuthnFlowTest.java | 41 +++++++--
.../authn/webauthn/flow/TestRegistrationFlow.java | 97 ++++++++++++++++++++++
.../webauthn/storage/impl/MockAuthenticator.java | 4 +-
5 files changed, 134 insertions(+), 13 deletions(-)
diff --git a/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/admin/impl/StorePublicKeyCredential.java b/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/admin/impl/StorePublicKeyCredential.java
index 9625516..10080a4 100644
--- a/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/admin/impl/StorePublicKeyCredential.java
+++ b/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/admin/impl/StorePublicKeyCredential.java
@@ -56,7 +56,8 @@ import net.shibboleth.shared.primitive.LoggerFactory;
*
* <p>Importantly, the registration is stored against the user in the context (the authenticated user). This way, even
* if the user changed the webauthn create request in the browser (e.g. to a different userId), it will still be
- * registered against the authenticated user.</p>
+ * registered against the authenticated user. This prevents a different user from registering a credential against
+ * somebody else's account.</p>
*
* @event {WebAuthnRegistrationEventIds#INVALID_REGISTRATION}
* @pre <pre>ProfileRequestContext.getSubcontext(WebAuthnRegistrationContext.class) != null</pre>
diff --git a/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/admin/impl/ValidateAuthenticatorAttestationResponse.java b/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/admin/impl/ValidateAuthenticatorAttestationResponse.java
index b5e45d3..a5dcc9e 100644
--- a/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/admin/impl/ValidateAuthenticatorAttestationResponse.java
+++ b/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/admin/impl/ValidateAuthenticatorAttestationResponse.java
@@ -99,7 +99,7 @@ public class ValidateAuthenticatorAttestationResponse extends AbstractWebAuthnAc
protected void doExecute(@Nonnull final ProfileRequestContext profileRequestContext,
@Nonnull final WebAuthnRegistrationContext context) {
- try {
+ try {
final RegistrationResult credentialPublicKey =
getWebAuthnClient().validateAuthenticatorAttestationResponse(pkCredCreationOptions, attestation);
diff --git a/webauthn-impl/src/test/java/net/shibboleth/idp/plugin/authn/webauthn/flow/AbstractWebAuthnFlowTest.java b/webauthn-impl/src/test/java/net/shibboleth/idp/plugin/authn/webauthn/flow/AbstractWebAuthnFlowTest.java
index 51b43b0..a2ccfbc 100644
--- a/webauthn-impl/src/test/java/net/shibboleth/idp/plugin/authn/webauthn/flow/AbstractWebAuthnFlowTest.java
+++ b/webauthn-impl/src/test/java/net/shibboleth/idp/plugin/authn/webauthn/flow/AbstractWebAuthnFlowTest.java
@@ -70,6 +70,7 @@ import com.yubico.webauthn.data.ByteArray;
import com.yubico.webauthn.data.ClientAssertionExtensionOutputs;
import com.yubico.webauthn.data.ClientRegistrationExtensionOutputs;
import com.yubico.webauthn.data.PublicKeyCredential;
+import com.yubico.webauthn.data.RelyingPartyIdentity;
import com.yubico.webauthn.data.UserIdentity;
import com.yubico.webauthn.data.UserVerificationRequirement;
@@ -82,6 +83,7 @@ import net.shibboleth.idp.plugin.authn.webauthn.context.WebAuthnRegistrationCont
import net.shibboleth.idp.plugin.authn.webauthn.principal.WebAuthnUserIdPrinicpal;
import net.shibboleth.idp.plugin.authn.webauthn.storage.CredentialRegistration;
import net.shibboleth.idp.plugin.authn.webauthn.storage.impl.IdPStorageServiceCredentialRespository;
+import net.shibboleth.idp.plugin.authn.webauthn.storage.impl.InMemoryRegistrationStorage;
import net.shibboleth.idp.plugin.authn.webauthn.storage.impl.MockAuthenticator;
import net.shibboleth.idp.session.IdPSession;
import net.shibboleth.idp.session.context.SessionContext;
@@ -159,6 +161,16 @@ public class AbstractWebAuthnFlowTest extends AbstractFlowTest {
.addModule(new JavaTimeModule())
.build();
+ rp = RelyingParty.builder().identity(
+ RelyingPartyIdentity
+ .builder()
+ .id("idp.example.com")
+ .name("Demo IdP as a WebAuthn RP")
+ .build()).credentialRepository(new InMemoryRegistrationStorage())
+ .allowOriginPort(true)
+ .allowOriginSubdomain(true)
+ .build();
+
try {
// Create a new one for each test, else it holds state about created keys
mockAuthenticator = new MockAuthenticator(RPID);
@@ -374,21 +386,16 @@ public class AbstractWebAuthnFlowTest extends AbstractFlowTest {
*/
protected PublicKeyCredential<AuthenticatorAttestationResponse, ClientRegistrationExtensionOutputs>
createAttestationReponse() throws Exception {
-
-
- final Map<String, String> clientDataCreate = createClientData("webauthn.create", ORIGIN, CHALLENGE_B64);
-
- final PublicKeyCredential<AuthenticatorAttestationResponse, ClientRegistrationExtensionOutputs> attestation =
- mockAuthenticator.createAuthenticatorAttestationResponse(CHALLENGE_B64, clientDataCreate,
- Base64Support.decode(USER_HANDLE_B64));
- return attestation;
+ return createAttestationReponse(Base64Support.decode(CHALLENGE_B64), USER_HANDLE_B64);
}
/**
* Create a credential registration attestation response from the mock authenticator.
*
+ * @param challenge the challenge
+ *
* @return the credential registration
*
* @throws Exception on error
@@ -396,12 +403,28 @@ public class AbstractWebAuthnFlowTest extends AbstractFlowTest {
protected PublicKeyCredential<AuthenticatorAttestationResponse, ClientRegistrationExtensionOutputs>
createAttestationReponse(final byte[] challenge) throws Exception {
+ return createAttestationReponse(challenge, USER_HANDLE_B64);
+ }
+
+ /**
+ * Create a credential registration attestation response from the mock authenticator.
+ *
+ * @param challenge the challenge
+ * @param userHandleB64 the userHandle to use. Note this is just stashed in the authenticator, the attestation
+ * response does not contain a userhandle.
+ * @return the credential registration
+ *
+ * @throws Exception on error
+ */
+ protected PublicKeyCredential<AuthenticatorAttestationResponse, ClientRegistrationExtensionOutputs>
+ createAttestationReponse(final byte[] challenge, final String userHandleB64) throws Exception {
+
final String challengeB64 = Base64Support.encodeURLSafe(challenge);
final Map<String, String> clientDataCreate = createClientData("webauthn.create", ORIGIN, challengeB64);
final PublicKeyCredential<AuthenticatorAttestationResponse, ClientRegistrationExtensionOutputs> attestation =
mockAuthenticator.createAuthenticatorAttestationResponse(challengeB64, clientDataCreate,
- Base64Support.decode(USER_HANDLE_B64));
+ Base64Support.decode(userHandleB64));
return attestation;
}
diff --git a/webauthn-impl/src/test/java/net/shibboleth/idp/plugin/authn/webauthn/flow/TestRegistrationFlow.java b/webauthn-impl/src/test/java/net/shibboleth/idp/plugin/authn/webauthn/flow/TestRegistrationFlow.java
index 0d8a498..9fadeb5 100644
--- a/webauthn-impl/src/test/java/net/shibboleth/idp/plugin/authn/webauthn/flow/TestRegistrationFlow.java
+++ b/webauthn-impl/src/test/java/net/shibboleth/idp/plugin/authn/webauthn/flow/TestRegistrationFlow.java
@@ -17,6 +17,7 @@ package net.shibboleth.idp.plugin.authn.webauthn.flow;
import static org.testng.Assert.assertEquals;
import java.util.Map;
+import java.util.Optional;
import javax.annotation.Nonnull;
@@ -29,9 +30,13 @@ import org.springframework.webflow.executor.FlowExecutionResult;
import org.testng.annotations.Test;
import com.yubico.webauthn.data.AuthenticatorAssertionResponse;
+import com.yubico.webauthn.data.AuthenticatorSelectionCriteria;
import com.yubico.webauthn.data.ByteArray;
import com.yubico.webauthn.data.ClientAssertionExtensionOutputs;
import com.yubico.webauthn.data.PublicKeyCredential;
+import com.yubico.webauthn.data.PublicKeyCredentialCreationOptions;
+import com.yubico.webauthn.data.RegistrationExtensionInputs;
+import com.yubico.webauthn.data.UserIdentity;
import net.shibboleth.idp.plugin.authn.webauthn.admin.impl.ExtractKeyRemovalInformationFromFormRequest;
import net.shibboleth.idp.plugin.authn.webauthn.admin.impl.ExtractPublicKeyCredentialAttestationFromFormRequest;
@@ -197,6 +202,98 @@ public class TestRegistrationFlow extends AbstractWebAuthnFlowTest{
}
+ /* Make sure we can not add a registration for another user .*/
+ @SuppressWarnings("null")
+ @Test
+ public void testRegistrationFlow_AddCredentialForAnotherUser() throws Exception {
+
+ //Register a credential for another user
+ final var anotherUserUsername = "another-user";
+ final var anotherUserDisplayName = "another user";
+ final var anotherUserUserIdB64 = "2fixsNecxhORZpf4LMU9nA==";
+ final CredentialRegistration registration =
+ createCredentialRegistration(anotherUserUsername, anotherUserDisplayName, anotherUserUserIdB64);
+ credentialRepo.addRegistrationByUsername(anotherUserUsername, registration);
+
+ assertEquals(credentialRepo.getCredentialIdsForUsername(anotherUserUsername).size(),1);
+
+ //Register a credential for the actual user to authenticate with
+ final CredentialRegistration registrationCorrectUser =
+ createCredentialRegistration(USERNAME, DISPLAY_NAME, USER_HANDLE_B64);
+ credentialRepo.addRegistrationByUsername(USERNAME, registrationCorrectUser);
+
+ assertEquals(credentialRepo.getCredentialIdsForUsername(USERNAME).size(),1);
+
+ final Pair<FlowExecutionResult, FlowExecutionImpl> result = launchExecution(FLOW_ID, null, externalContext,
+ new LocalAttributeMap<>());
+
+ doAuthenticationDance(result, registrationCorrectUser);
+ assertCurrentStateEquals("DisplayWebAuthnRegistrationView", result.getSecond());
+
+ // Re-set external context to holder
+ ExternalContextHolder.setExternalContext(externalContext);
+
+ final ProfileRequestContext prc = getProfileRequestContextFromConversation(result.getSecond());
+
+ // Get the challenge that was set into the PublicKeyCredentialRequestOptions.
+ // otherwise we will end up signing a different one, which will provide its own test.
+ final WebAuthnRegistrationContext authnContext = getWebAuthnRegistrationContext(prc);
+ final var publicKeyCreationOptions = authnContext.getPublicKeyCredentialCreationOptions();
+ final ByteArray challenge = publicKeyCreationOptions.getChallenge();
+
+ // Now modify the creation options so it contains a different user than the one that authenticated e.g.
+ // that of the other user.
+ final var fakedUser = UserIdentity.builder()
+ .name(anotherUserUsername)
+ .displayName(anotherUserDisplayName)
+ .id(ByteArray.fromBase64Url(anotherUserUserIdB64)).build();
+
+ final var authSelectionCriteria = AuthenticatorSelectionCriteria.builder()
+ .userVerification(authnContext.getUserVerificationRequirement())
+ .residentKey(authnContext.getResidentKeyRequirement())
+ .authenticatorAttachment(authnContext.getAuthenticatorAttachmentRequirement())
+ .build();
+
+ final PublicKeyCredentialCreationOptions creation = PublicKeyCredentialCreationOptions.builder()
+ .rp(rp.getIdentity())
+ .user(fakedUser)
+ .challenge(publicKeyCreationOptions.getChallenge())
+ .pubKeyCredParams(publicKeyCreationOptions.getPubKeyCredParams())
+ .excludeCredentials(publicKeyCreationOptions.getExcludeCredentials())
+ .attestation(publicKeyCreationOptions.getAttestation())
+ .authenticatorSelection(authSelectionCriteria)
+ .extensions(RegistrationExtensionInputs.builder().credProps().build())
+ .timeout(Optional.empty()).build();
+
+ authnContext.setPublicKeyCredentialCreationOptions(creation);
+
+
+ // Create a credential for the other users UserId/UserHandle. This should be stored against the actual user
+ // We should not be able to trick the system into one user registering a credential for another user.
+ final var attestationResponse = createAttestationReponse(challenge.getBytes());
+ final String attestationResponseJson = jsonMapper.writeValueAsString(attestationResponse);
+ setHttpFormRequest("POST", Map.of(ExtractPublicKeyCredentialAttestationFromFormRequest.DEFAULT_PARAMETER_NAME,
+ attestationResponseJson,
+ ExtractPublicKeyCredentialAttestationFromFormRequest.DEFAULT_NICKNAME_FIELD_NAME, "new-cred"));
+ externalContext.setEventId("addKey");
+ result.getSecond().setCurrentState("DisplayWebAuthnRegistrationView");
+ result.getSecond().resume(externalContext);
+
+ assertCurrentStateEquals("DisplayWebAuthnRegistrationView", result.getSecond());
+ // The credential should be added to the authenticated user, not the manipulated user in the CreationOptions
+ // request.
+ assertEquals(credentialRepo.getCredentialIdsForUsername(USERNAME).size(),2);
+ assertEquals(credentialRepo.getCredentialIdsForUsername(anotherUserUsername).size(),1);
+
+ // Now end flow
+ ExternalContextHolder.setExternalContext(externalContext);
+ externalContext.setEventId("finish");
+ result.getSecond().setCurrentState("DisplayWebAuthnRegistrationView");
+ result.getSecond().resume(externalContext);
+ assertFlowExecutionInActive(result.getSecond());
+
+ }
+
@SuppressWarnings("null")
private void doAuthenticationDance(final Pair<FlowExecutionResult, FlowExecutionImpl> result,
final CredentialRegistration registration) throws Exception {
diff --git a/webauthn-impl/src/test/java/net/shibboleth/idp/plugin/authn/webauthn/storage/impl/MockAuthenticator.java b/webauthn-impl/src/test/java/net/shibboleth/idp/plugin/authn/webauthn/storage/impl/MockAuthenticator.java
index 1c40f07..3d5d460 100644
--- a/webauthn-impl/src/test/java/net/shibboleth/idp/plugin/authn/webauthn/storage/impl/MockAuthenticator.java
+++ b/webauthn-impl/src/test/java/net/shibboleth/idp/plugin/authn/webauthn/storage/impl/MockAuthenticator.java
@@ -163,7 +163,7 @@ public class MockAuthenticator {
*
* @throws Exception on error.
*/
- //TODO do we need challenge here, it goes in the clientData?
+ //TODO do we need challenge here, it goes in the clientData? And userHandle is only for quick reference
public com.yubico.webauthn.data.PublicKeyCredential<AuthenticatorAttestationResponse, ClientRegistrationExtensionOutputs>
createAuthenticatorAttestationResponse(@Nonnull @NotEmpty final String challenge,
final Map<String, String> clientData, final byte[] userHandle) throws Exception {
@@ -186,7 +186,7 @@ public class MockAuthenticator {
//convert from the test type to the correct type for the rest of the system
final var attestationAsJson = jsonMapper.writeValueAsString(publicKeyCredential);
final var pkCred =com.yubico.webauthn.data.PublicKeyCredential.parseRegistrationResponseJson(attestationAsJson);
-
+ log.debug("Created attestation response '{}'", attestationAsJson);
createdCredentialsMaps.put(Base64Support.encodeURLSafe(credentialId), publicKeyCredential);
return pkCred;
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.
More information about the commits
mailing list