google / CTAP2-test-tool

Test tool for CTAP2 authenticators
Apache License 2.0
56 stars 26 forks source link

Test item GetAssertionEmptyUserIdTest #90

Open nuno0529 opened 4 years ago

nuno0529 commented 4 years ago
{
  "description": "Tests if empty user IDs are omitted in the response.",
  "error_message": "Cannot make credential with an empty user ID.",
  "id": "get_assertion_empty_user_id",
  "observations": [
    "A prompt was expected, but not performed. Sometimes it is just not recognized if performed too fast.",
    "The failing error code is `CTAP2_ERR_PROCESSING`."
  ],
  "result": "fail",
  "tags": []
},

https://github.com/fido-alliance/fido-2-specs/pull/963 Although fido2 spec says empty account identifier is valid but can't be returned in get(), this case makes authenticator need to save some useless data...

Supplement on Nov-25 I can't find the string to say but can't be returned in get() in spec.

kaczmarczyck commented 4 years ago

Thanks for opening the specification issue. We can follow up here after the discussion on the specification.

nuno0529 commented 4 years ago

Another question, shouldn't this test case (i.e. GetAssertionEmptyUserIdTest ) use rk=true in makeCred command or normal authenticator (w/o force to create rk) won't return any user data in getAssert response.

kaczmarczyck commented 4 years ago

Good point, rk=true would more likely trigger bad behaviour. Also, I'll follow up on the specification isse.

nuno0529 commented 3 years ago

For this test case after related information from ctap2/webauth, I think the exepected reuslts should be

  1. (unchanged) Authenticator MUST accept makCred command with empty user ID with rk.
  2. (changed) Authenticator MUST return resident credential with empty user ID in geAssert response.
kaczmarczyck commented 3 years ago

I can only change this test to the changed behaviour after confirming it doesn't break Windows support. At the moment, this tool is used to test firmware releases, so I have to test what actually works, even if it contradicts the specification. @nuno0529 Have you tested your proposed behaviour on Windows? We had problems with OpenSK, i.e. https://github.com/google/OpenSK/issues/28

kaczmarczyck commented 3 years ago

WebAuthn adapted just now: https://github.com/w3c/webauthn/pull/1537

nuno0529 commented 3 years ago

I didn't test so many fido2 test servers which are for test purpose only. At least I didn't encounter this issue on mainstream websites and main test server webauthntest.azurewebsites.net I used. So I'm still thinking if authenticators do need to apply this kind of workaround behavior for this issue (i.e. not return user info with empty user.id).

kaczmarczyck commented 3 years ago

May I ask you to test on https://webauthn.me/ on a Windows 10, with UV being turned on?

nuno0529 commented 3 years ago

I don't see any problem when testing on https://webauthn.me/ and this doesn't use empty user.id in create().

kaczmarczyck commented 3 years ago

I wanted to get an ATKey.pro to get to the bottom of this, but Amazon doesn't ship it to Europe it seems. I assume this is one of the device I could use to reproduce? Any other way to get one of them?

nuno0529 commented 3 years ago

We can send you one to Europe, please contact jenny.hsu@authentrend.com with your address and I have notify her this request from @kaczmarczyck.

But if the issue you want to reproduce is "Cannot make credential with an empty user ID." I can also reproduce this behavior with Yubikeys and the log here also show this error. https://github.com/google/CTAP2-test-tool/blob/master/results/Security%20Key%20by%20Yubico_.json

kaczmarczyck commented 3 years ago

Thanks, email is out! Just making sure, and for documentation porpuses: For reproducing on https://webauthn.me/, you have to be on Windows 10 and your authenticator needs to be set up with UV.

nuno0529 commented 3 years ago

For reproducing on https://webauthn.me/, you have to be on Windows 10 and your authenticator needs to be set up with UV.

I test on Windows 10 20H2(19042.685) with MS Edge 87.0.664.60, I use my authenticator with built-in UV and I don't see any problem or and special parts related to this issue. Do you mean under https://webauthn.me/debugger ? But the user.id here still can't be set to empty or any else.

kaczmarczyck commented 3 years ago

I'll debug locally (after Christmas) and come back to you. Thanks so far for the detailed information!

kaczmarczyck commented 3 years ago

Update: We still have the same behaviour for OpenSK on Windows for webauthn.me with UV. GetAssertion fails if we remove this check.

We can leave this issue open for tracking, but I think it currently a good compromise between specification and working in the real world.