quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.53k stars 2.61k forks source link

quarkus-webauhn error: "AAGUID is not 00000000-0000-0000-0000-000000000000!" #38043

Open Doogiemuc opened 8 months ago

Doogiemuc commented 8 months ago

Describe the bug

I tried to create a REALLY minimal demo of quarkus-webauhn. No reactive. Even no database or real entities at all. Just mocks.

https://github.com/Doogiemuc/quarkus-webauthn-minimal-demo

But I am having some problems. When trying to register a new user on Mac Safari with Fingerprint I get the error:

HTTP Request to /q/webauthn/callback failed, error id: 9b447afd-1358-4e5c-9478-f9f674be4cae-1: io.vertx.ext.auth.webauthn.impl.attestation.AttestationException: AAGUID is not 00000000-0000-0000-0000-000000000000!

I see that the class NoneAttestation.java seem to check for this specific AAGUID. But where or how can I set it during register?

It seems like Apple Safari does send a AAGUID when authenticating via fingerprint. How to accept this AAGUID?

Expected behavior

There should be a way to make authentication via fingerprint or face-id work on apple safari (and safari on iOS).

Actual behavior

Any webauthn with an AAGUID other then zeros only is rejected by quarkus-webauthn.

How to Reproduce?

See this repo: https://github.com/Doogiemuc/quarkus-webauthn-minimal-demo See README.md

Output of uname -a or ver

Darwin James.fritz.box 23.2.0 Darwin Kernel Version 23.2.0: Wed Nov 15 21:53:18 PST 2023; root:xnu-10002.61.3~2/RELEASE_ARM64_T6000 arm64

Output of java -version

openjdk version "17.0.1" 2021-10-19

Quarkus version or git rev

io.quarkus.platform:3.6.4

Build tool (ie. output of mvnw --version or gradlew --version)

IntelliJ

Additional information

See source code in repo

quarkus-bot[bot] commented 8 months ago

/cc @pedroigor (bearer-token)

sberyozkin commented 8 months ago

Cc @FroMage

FroMage commented 8 months ago

I had to fix your code a little bit, because I got an NPE and then an error because the user was not registered, but then it worked just fine on Linux with Google Chrome and a Yubikey hardware token, which is what I always used for testing. But this has a 00000000-0000-0000-0000-000000000000 AAGUID. I tried to use my Android device as passkey, and register worked, but for login it failed to ask my phone, so not sure what went wrong there.

I wonder why you did not get the NPE and user not found error, though, this happens before any AAGUID check.

FroMage commented 8 months ago

This check depends on your fmt field in the attestationObject value send to the server during registration. This is a base64-encoded CBOR field. It's a bit annoying to decode, but you can first decode it to hex at https://cryptii.com/pipes/base64-to-hex (using the url variant) and then as CBOR at https://cbor.nemo157.com/ and that'll show you the fmt field.

In my case, it's none.

sberyozkin commented 7 months ago

I wonder if we should do more about CBOR in Quarkus, @FroMage, even in scope of OIDC, encoding the session token as CBOR/COSE, thanks for mentioning it here, makes sense to discuss it further IMHO

FroMage commented 7 months ago

Well, we could start by adding CBOR-decoding tools to the DEV UI, on top of JWT-decoding tools.

StephenOTT commented 7 months ago

Hit this issue today using the iCloud keychain for passkey login.

Can see the required aaguid here:

https://github.com/passkeydeveloper/passkey-authenticator-aaguids/blob/main/aaguid.json#L75

Even with quarkus.webauthn.attestation=direct, it will return none for the fmt value and NoneAttestation.class gets used.

Lots of articles about lack of attestation support on apple devices.

Seems like the underlying vertx library is the issue here, with how NoneAttestation was originally setup vs the aaguids that get used even when none attestation is used: https://github.com/passkeydeveloper/passkey-authenticator-aaguids/blob/main/aaguid.json

ynojima commented 7 months ago

Originally, In WebAuthn spec, it is defined that 'when "none" attestation is requested, client must replace the AAGUID with 16 zero bytes.'

Replace the aaguid in the attested credential data with 16 zero bytes. https://www.w3.org/TR/2019/REC-webauthn-1-20190304/#CreateCred-DetermineRpId

So, non-zero AAGUID must not be returned for "none" attestation. But recently, Apple start ingoring this rule and returns non-zero AAGUID for the passkey. This behavior conflicts vertx-auth's NoneAttestation#validate logic. https://github.com/eclipse-vertx/vertx-auth/blob/4.5.1/vertx-auth-webauthn/src/main/java/io/vertx/ext/auth/webauthn/impl/attestation/NoneAttestation.java#L45-L47

Since WebAuthn spec's None Attestaion Statement Format Verification procedure doesn't require to check AAGUID is zero, I think this AAGUID zero check can be removed to support Apple's passkey.

StephenOTT commented 7 months ago

just for fidelity, note that it is not only for apple: https://github.com/passkeydeveloper/passkey-authenticator-aaguids/blob/main/aaguid.json#L75. Hello Windows, Chrome, etc are doing similar behaviour as the apple example

ynojima commented 7 months ago

Thank you for the additional info. Yes, Google Password Manager, which manages passkeys too, also returns AAGUID. I confirmed with Chrome for Android. https://github.com/passkeydeveloper/passkey-authenticator-aaguids/blob/main/aaguid.json#L2

FroMage commented 7 months ago

Thanks. So, to be clear, we should remove the check, or replace it with a check for a AAGUID of all those we know about?

StephenOTT commented 7 months ago

If it is to be replaced with the known AAGUIDs, then i would suggest that there is a config(s) option to set additional AAGUIDs or just optionally disable the AAGUID check. "Tomorrow" if someone creates an additional AAGUID, then this lib breaks again.

ynojima commented 7 months ago

I think we should just remove the AAGUID check here: https://github.com/eclipse-vertx/vertx-auth/blob/4.5.1/vertx-auth-webauthn/src/main/java/io/vertx/ext/auth/webauthn/impl/attestation/NoneAttestation.java#L45-L47

I made a pull-request for this: https://github.com/eclipse-vertx/vertx-auth/pull/671

FroMage commented 7 months ago

Oh great, thanks!

terfex commented 3 months ago

Has this issue been confirmed to be resolved? As of today, I'm still able to reproduce this same error as well.

FroMage commented 3 months ago

On Vert.x main yes, I've asked which released version this went in: https://github.com/eclipse-vertx/vertx-auth/pull/671#issuecomment-2158362560

terfex commented 2 months ago

Thanks