Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test item GetAssertionEmptyUserIdTest #90

Open
nuno0529 opened this issue Nov 20, 2020 · 15 comments
Open

Test item GetAssertionEmptyUserIdTest #90

nuno0529 opened this issue Nov 20, 2020 · 15 comments
Assignees
Labels
question Further information is requested

Comments

@nuno0529
Copy link

nuno0529 commented Nov 20, 2020

{
  "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
Copy link
Collaborator

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

@kaczmarczyck kaczmarczyck self-assigned this Nov 23, 2020
@kaczmarczyck kaczmarczyck added the question Further information is requested label Nov 23, 2020
@nuno0529
Copy link
Author

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
Copy link
Collaborator

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

@nuno0529
Copy link
Author

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
Copy link
Collaborator

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.
google/OpenSK#28

@kaczmarczyck
Copy link
Collaborator

WebAuthn adapted just now:
w3c/webauthn#1537

@nuno0529
Copy link
Author

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
Copy link
Collaborator

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

@nuno0529
Copy link
Author

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

@kaczmarczyck
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

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

@kaczmarczyck
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants