Yubico / java-webauthn-server

Server-side Web Authentication library for Java https://www.w3.org/TR/webauthn/#rp-operations
Other
443 stars 139 forks source link

Using webauthn-server-demo with FidoMetadataService? #279

Closed mmoayyed closed 1 year ago

mmoayyed commented 1 year ago

Hello, I am using the latest version of the java-webauthn-server as of this writing, 2.4.1 and I am experimenting with the provided demo project to allow it to use the FidoMetadataService. One potential issue I see is that the WebAuthnServer.java declares this:

private final YubicoJsonMetadataService metadataService = new YubicoJsonMetadataService();

...which is then used here:

private CredentialRegistration addRegistration(
      UserIdentity userIdentity, Optional<String> nickname, RegistrationResult result) {
    return addRegistration(
        userIdentity,
        nickname,
        RegisteredCredential.builder()
            .credentialId(result.getKeyId().getId())
            .userHandle(userIdentity.getId())
            .publicKeyCose(result.getPublicKeyCose())
            .signatureCount(result.getSignatureCount())
            .build(),
        result.getKeyId().getTransports().orElseGet(TreeSet::new),
        result
            .getAttestationTrustPath()
            .flatMap(x5c -> x5c.stream().findFirst())
            .flatMap(metadataService::findMetadata)); // called here...
  }

Of course, the findMetadata() method is part of the AttestationMetadataSource hierarchy which does not make for an easy replacement of the metadataService with an instance of FidoMetadataService, etc.

Could the findMetadata() method be moved up the chain and is that a sensible thing to do, or are there better alternatives in adapting the demo to use the FidoMetadataService?

Thanks for your help!

emlun commented 1 year ago

Hi! Does PR #280 help do what you want?

We considered having some kind of findMetadata() method in AttestationTrustSource but decided against it, for a couple of reasons.

But none of that is to stop downstream users from adding their own abstractions. :slightly_smiling_face: For example PR #280 extends the demo app with a new MetadataService interface which does abstract the findMetadata() method, and a FidoMetadataServiceAdapter that wraps FidoMetadataService under the new interface. This allows the demo to combine both a FidoMetadataService and a YubicoJsonMetadataService into one composite metadata service (with Object as the metadata type, as noted above!).

Does any of that help?

mmoayyed commented 1 year ago

Yes, thank you. In fact, I was about to do something quite similar to how you manage composite sources, etc. This should do the job for me.

I'll also add that the new abstractions you added would be very useful to include in the "core" upstream project. I don't really see/treat them as demo quality (and I would, i.e. be somewhat forced to copy/paste those and adapt in very small ways for my own project). So it would be great if they were to become part of the core library, as that would be less code for me to manage and maintain, but of course, perhaps more work for you :)

Thanks again! If it's OK with you, I can close this issue out and then try out the changes in the PR when they get merged, etc.

emlun commented 1 year ago

Ok, glad I could help!

Unfortunately I don't think the demo abstractions are a good fit for promoting into the core library, or even the attestation module, again for the same reasons that they end up tightly coupled to a particular implementation of a metadata source. But we'll reconsider if something new comes up that would enable some kind of generic abstraction - other than degenerating to Object types and thus forfeiting most of the benefits of the static type system.