Yubico / java-webauthn-server

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

Support for PublicKeyCredential.authenticatorAttachment #249

Closed luisgoncalves closed 1 year ago

luisgoncalves commented 1 year ago

Hi,

In the WebAuthn Level 3 draft, PublicKeyCredential contains a new authenticatorAttachment field (https://w3c.github.io/webauthn/#iface-pkcredential). I saw that you recently added some WebAuthn 3 bits to the library as experimental (e.g. BE flag), so I'm wondering if authenticatorAttachment could be added as well?

If so, I may be able to put up a PR for it. I guess the obvious change is to add the new field to com.yubico.webauthn.data.PublicKeyCredential. Some other questions:

Any other pointers are welcome.

emlun commented 1 year ago

Hi! Sure, we should do that. There's no specific processing defined for it, so it should just be copied through to RegistrationResult and AssertionResult - just as a convenience so you don't need to juggle as many objects.

Let me know if you'd like to give it a try, otherwise I'll do it. The tests should only need to verify that it is copied through correctly. That'll be one test in RelyingPartyRegistrationSpec and one in RelyingPartyAssertionSpec, both directly after the test case "exposes isBackedUp() with the BS flag value in authenticator data.". com.yubico.webauthn.data.Generators would need a small update to include the new field in generated PublicKeyCredential values, and the new tests could perhaps make use of the updated generators.

luisgoncalves commented 1 year ago

I'll give it a try based on your input and what was done for isBackedUp().

emlun commented 1 year ago

Thanks! Note that I'll go on leave for three weeks starting next week, so it might be a while before I'm available to review and integrate.

luisgoncalves commented 1 year ago

Hi again @emlun

I'm not familiar with Scala and the generators.. I've updated PublicKeyCredential and the arbitraryPublicKeyCredentialWithAttestation and arbitraryPublicKeyCredentialWithAssertion generators, but I don't know how to use them from the tests.

I want to be sure that all the possible values for authenticatorAttachment are tested, so I have:

        describe("exposes getAuthenticatorAttachment() with the value in the registration response when") {
          for { authenticatorAttachment <- AuthenticatorAttachment.values() } {
            it(s"attachment is ${authenticatorAttachment}") {
              val pkc = // PublicKeyCredential using authenticatorAttachment
              // ...
            }
          }
        }

How can I create a PublicKeyCredential based on the generators and specifying authenticatorAttachment? Do I need a method similar to the ones in com.yubico.webauthn.data.Generators.Extensions?

Is this the right path on using the test harness? Sorry if the answers are obvious and I missed it...

emlun commented 1 year ago

Hm, on a closer look I don't think it'll be very useful to use the PublicKeyCredential generators for this, because they currently generate values that are structurally well-formed but with invalid signatures and such. We want to test that the values make it through RelyingParty.finishRegistration and .finishAssertion, so they need to be not only structurally but also semantically valid. It would take quite a lot of work to parameterize the generators for that, and there is a better solution already: TestAuthenticator.createUnattestedCredential and .createAssertion, which are used in the isBackedUp() neighboring tests.

You can use the generator for AuthenticatorAttachment though. So you can write the tests like this:

        it(
          "exposes getAuthenticatorAttachment() with the authenticatorAttachment value from the PublicKeyCredential."
        ) {
          val (pkcTemplate, _, _) =
            TestAuthenticator.createUnattestedCredential(challenge =
              request.getChallenge
            )

          forAll { authenticatorAttachment: Option[AuthenticatorAttachment] =>
            val pkc = pkcTemplate.toBuilder
              .authenticatorAttachment(authenticatorAttachment.orNull)
              .build()

            val result = rp.finishRegistration(
              FinishRegistrationOptions
                .builder()
                .request(request)
                .response(pkc)
                .build()
            )

            result.getAuthenticatorAttachment should equal (pkc.getAuthenticatorAttachment)
          }
        }

and this:

          it(
            "exposes getAuthenticatorAttachment() with the authenticatorAttachment value from the PublicKeyCredential."
          ) {
            val pkcTemplate =
              TestAuthenticator.createAssertion(
                challenge =
                  request.getPublicKeyCredentialRequestOptions.getChallenge,
                credentialKey = credentialKeypair,
                credentialId = credential.getId,
              )

            forAll { authenticatorAttachment: Option[AuthenticatorAttachment] =>
              val pkc = pkcTemplate.toBuilder
                .authenticatorAttachment(authenticatorAttachment.orNull)
                .build()

              val result = rp.finishAssertion(
                FinishAssertionOptions
                  .builder()
                  .request(request)
                  .response(pkc)
                  .build()
              )

              result.getAuthenticatorAttachment should equal (pkc.getAuthenticatorAttachment)
            }
          }
emlun commented 1 year ago

Oh, and you can use ./gradlew spotlessApply before committing to prevent GitHub Actions from nagging you when you push. :slightly_smiling_face:

luisgoncalves commented 1 year ago

I see your point. Thanks for the inputs!

luisgoncalves commented 1 year ago

PR is up :)

emlun commented 1 year ago

This is now available in pre-release 2.3.0-RC1. We'll promote it to a proper release when I return in three weeks.

Thanks for contributing!

luisgoncalves commented 1 year ago

Great! It will be available on Maven central, right?

emlun commented 1 year ago

Yes, and the RC1 pre-release is also available on Maven Central now.

emlun commented 1 year ago

The full 2.3.0 release is also live on Maven Central now. Thanks again!