migration to fido2 #31

piotrbartman · 2023-05-06T22:07:48Z

Adds support for the fido2 tokens (such as PIN)
Adds CI tests/QA
Updates docs

marmarek · 2023-05-06T23:30:18Z

Is there any way to keep compatibility with the old u2f.Authenticate service (via some kind of wrapper or such)? Specifically, to keep it working without manual changes for users that have qrexec policies set that allow specific arguments for the u2f.Authenticate call (as documented at https://www.qubes-os.org/doc/u2f-proxy/#advanced-usage-per-qube-key-access). Alternatively, is it possible to automatically migrate policy (especially with specific arguments) to the new services? I guess that could be tricky if not impossible, but I'd like to confirm anyway.

And another, related question: are keys registered to websites via u2f.Register going to still work with the new service (assuming policy appropriately updated)? This I hope the answer is yes, right?

PS I updated codecov token in CI, next run should have it.

piotrbartman · 2023-05-07T19:06:01Z

The only thing that needs to be done is to change the name from u2f.Authenticate* to ctap.GetAssertion* (and for register respectively). The key generated using old U2F protocol should still work in FIDO2 (when I tested it, I didn't see any problems). I considered leaving the old names (then nothing would have to be changed in the policies), but that would be inconsistent. Currently, it seems that the best way for backward compatibility is (i) to apply a patch that renames the policy during the update of package, or (ii) to check if a given argument exists in the policy as ctap.GetAssertion+<argument> or u2f.Authenticate+<argument> every time we do authentication.

marmarek · 2023-05-07T19:20:13Z

(i) to apply a patch that renames the policy during the update of package, or

While technically possible, this will require the user to update both dom0 and relevant templates at the same time. We don't have any method to enforce it, so it the user updates one but not the other, the U2F will be broken for that time.

But, if we change the package name (as you suggested before), then the update will no longer be automatically installed (no surprise breakage on standard update), and we can document the migration procedure (update both the dom0 and templates at the same time). And then, we can also include this in the R4.1->R4.2 migration tool (QubesOS/qubes-issues#7832). I guess this is the way to go then.

codecov-commenter · 2023-05-08T22:51:49Z

Codecov Report

❗ No coverage uploaded for pull request base (main@5c20cf7). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #31   +/-   ##
=======================================
  Coverage        ?   72.81%           
=======================================
  Files           ?       20           
  Lines           ?     1501           
  Branches        ?        0           
=======================================
  Hits            ?     1093           
  Misses          ?      408           
  Partials        ?        0

marmarek · 2023-05-20T22:08:56Z

qubesu2f/sys_usb/qctap_get_assertion.py

-    and args.key_handle_hash != apdu.qrexec_arg):
+    allow_list = list(request.qrexec_args)
+    if (args.credential_id_hash is not None
+            and args.credential_id_hash not in allow_list):


Why in? When request.qrexec_args is a list of more than one element? Wouldn't it allow using other keys if a policy allows just one of them?

You're right. I added a function that removes the other keys from the request. According to the protocol, there should be no difference in which key is used. However, in our case, we want to have control over it.

marmarek · 2023-05-21T10:53:41Z

How to test CTAP2? My attempt with Firefox didn't produce any ctap.GetInfo calls.

piotrbartman · 2023-05-21T11:18:07Z

How to test CTAP2? My attempt with Firefox didn't produce any ctap.GetInfo calls.

From what I know, Firefox + MacOS/Linux support for FIDO2 is still broken. You need to try with a different browser.

marmarek · 2023-05-21T21:58:04Z

With Chromium indeed it tries ctap.GetInfo, which fails:

May 21 17:56:19 sys-usb qctap-get-info[32616]: asyncio Using selector: EpollSelector
May 21 17:56:19 sys-usb qctap-get-info[32616]: fido2.hid SEND: ffffffff860008ecf6c06f0a47f13b
May 21 17:56:19 sys-usb qctap-get-info[32616]: fido2.hid RECV: ffffffff860011ecf6c06f0a47f13b00040015020403070100000000000000000000000000000000000000000000000000000000000000000000000000000000
May 21 17:56:19 sys-usb qctap-get-info[32616]: mux pending={<Future pending cb=[_chain_future.<locals>._call_check_cancel() at /usr/lib64/python3.11/asyncio/futures.py:387]>}
May 21 17:56:19 sys-usb qctap-get-info[32616]: ctap.request Use CTAP 2 protocol.
May 21 17:56:19 sys-usb qctap-get-info[32616]: ctap.request return CborRequestWrapper
May 21 17:56:19 sys-usb qctap-get-info[32616]: mux.device request: b'\x04'
May 21 17:56:19 sys-usb qctap-get-info[32616]: ctap Execute CTAP1 request
May 21 17:56:19 sys-usb qctap-get-info[32616]: ctap Unexpected response error: 'Ctap1' object has no attribute 'send_cbor'
May 21 17:56:19 sys-usb qctap-get-info[32616]: mux.device response b'i\x85'
May 21 17:56:19 sys-usb qctap-get-info[32616]: root pending=set() done={<Future finished result=<qubesctap.pr...x77f63d96c990>>}

marmarek · 2023-05-21T23:25:44Z

In fact, it seems the new ctap.GetAssertion and ctap.MakeCredential work just fine with the old qu2f-proxy. I guess they are compatible the other way around too (new ctapproxy with old u2f.* as long as CTAP1 is used)?
In that case, can we make the migration a little bit less painful and:

include symlinks for old u2f.* services to their new equivalents, and
add a fallback, if calling ctap.* fails with code 126 (policy deny), retry with u2f.* call?

The second part surely will need some opt-out switch (maybe via qvm-service?).

marmarek · 2023-05-21T23:29:49Z

Or go even step further, and keep the old names for those two, accepting a little inaccuracy. TBH "Authenticate" and "Register" are easier to understand for the user than "GetAssertion" and "MakeCredential". The documentation can still clarify what actual operations they do.

piotrbartman · 2023-05-22T06:51:56Z

With Chromium indeed it tries ctap.GetInfo, which fails:

It seems that your device doesn't support CTAP2? I have added a message that is less misleading.

piotrbartman · 2023-05-22T06:56:23Z

Or go even step further, and keep the old names for those two, accepting a little inaccuracy. TBH "Authenticate" and "Register" are easier to understand for the user than "GetAssertion" and "MakeCredential". The documentation can still clarify what actual operations they do.

So we want:

ctap.GetInfo
ctap.ClientPin
u2f.Register
u2f.Authneticate

right?

marmarek · 2023-05-22T09:10:03Z

So we want:

ctap.GetInfo
ctap.ClientPin
u2f.Register
u2f.Authenticate

right?

Yes (with typo fixed), if I'm indeed correct about their compatibility for CTAP1 - do you confirm?

marmarek · 2023-05-22T09:13:06Z

It seems that your device doesn't support CTAP2? I have added a message that is less misleading.

Could be. See also my log contains Unexpected response error: 'Ctap1' object has no attribute 'send_cbor' - that looks like a bug your latest commit doesn't touch.

piotrbartman · 2023-05-22T09:41:39Z

So we want:
ctap.GetInfo
ctap.ClientPin
u2f.Register
u2f.Authenticate
right?
Yes (with typo fixed), if I'm indeed correct about their compatibility for CTAP1 - do you confirm?

Sorry for typo ;) Yes it is fully compatible

marmarek · 2023-05-22T10:27:04Z

It seems that your device doesn't support CTAP2? I have added a message that is less misleading.

FWIW, with device supporting CTAP2 it works correctly (and according to logs it indeed used CTAP2).

piotrbartman · 2023-05-22T10:47:58Z

Now it simply throws an exception if the device does not support CTAP2, but without the message "Unexpected error" because this error is expected. Most clients should then fallback to CTAP1 in such cases.

marmarek · 2023-05-22T10:57:02Z

and as for compatibility, qubes-ctapproxy service should also start if the old qvm-service is enabled; you can use "triggering" condition - ConditionPathExists=|/run/... - then either of them existing will be enough.

marmarek · 2023-07-06T00:42:37Z

I have re-tested new version and seems all combinations work with pre-existing keys and u2f.Authenticate service:

old backend, new frontend
new backend, old frontend
new backend, new frontend

For those tests, I had ctap.GetInfo method denied in policy.

But when I enabled ctap.GetInfo (and having both new backend and frontend), I couldn't use U2F token (that doesn't support FIDO2). I see ctap.GetInfo call in the log, that fails with ctap Device do not support CTAP2, trying execute CTAP1 request, but nothing else happens and the browser waits at demo.yubico.com wants to authenticate ....

marmarek · 2023-07-06T00:44:43Z

BTW, package metadata will need adjustments for better rename handling (https://wiki.debian.org/RenamingPackages, and also spec needs to Provide: qubes-u2f = %version, not Provide: qubes-ctap , and Obsoletes: qubes-u2f < 2.0.0 - with a version).
But I'll handle that after merging.

piotrbartman · 2023-07-06T09:40:34Z

But when I enabled ctap.GetInfo (and having both new backend and frontend), I couldn't use U2F token (that doesn't support FIDO2). I see ctap.GetInfo call in the log, that fails with ctap Device do not support CTAP2, trying execute CTAP1 request, but nothing else happens and the browser waits at demo.yubico.com wants to authenticate ....

Hm, can you send me the logs from "client vm" and sys-usb?
To do this add -vvv flag to qctap-proxy service and the logs are in /var/log/qubes/qctap.
In sys-usb you can touch /usr/local/etc/qubes/ctap-debug-enable to increase verbosity.

marmarek · 2023-07-06T14:12:27Z

Crash on the client side:

2023-07-06 10:10:39,063 CTAPHIDQrexecDevice.ctaphid handle_ctaphid_cbor(cid=0x3dec9272, data=...)
2023-07-06 10:10:39,063 ctap.request Use CTAP 2 protocol.
2023-07-06 10:10:39,063 ctap.request return CborRequestWrapper
2023-07-06 10:10:39,063 CTAPHIDQrexecDevice.ctaphid handle_ctaphid_cbor(cid=0x3dec9272) cbor=GetInfo()
2023-07-06 10:10:39,063 CTAPHIDQrexecDevice.ctap handle_fido2_get_info()
2023-07-06 10:10:39,063 CTAPHIDQrexecDevice.qrexec qrexec_transaction(capdu, rpcname='ctap.GetInfo')
2023-07-06 10:10:39,514 asyncio Task exception was never retrieved
future: <Task finished name='Task-4' coro=<CTAPHIDDevice.handle_ctaphid_cbor() done, defined at /usr/lib/python3.11/site-packages/qubesctap/client/hidemu.py:334> exception=CtapError('CTAP error: 0x6A - UNKNOWN')>
Traceback (most recent call last):
  File "/usr/lib/python3.11/site-packages/qubesctap/client/hidemu.py", line 336, in handle_ctaphid_cbor
    await self._handle_ctaphid_request(
  File "/usr/lib/python3.11/site-packages/qubesctap/client/hidemu.py", line 318, in _handle_ctaphid_request
    bytes(await handle(request)))
          ^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/site-packages/qubesctap/client/qctap_proxy.py", line 87, in handle_fido2_get_info
    return CborResponseWrapper.from_bytes(response, expected_type=Info)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/site-packages/qubesctap/protocol.py", line 378, in from_bytes
    raise CtapError(status)
fido2.ctap.CtapError: CTAP error: 0x6A - UNKNOWN

marmarek · 2023-07-12T01:33:03Z

The U2F-only devices handling seems to work now. But while testing it, I found another case - if no token is plugged in at all, services fail with NotImplementedError:

qctap-get-info[14116]: asyncio Using selector: EpollSelector
qctap-get-info[14116]: mux pending=set()
qctap-get-info[14116]: mux no device, sending fake CONDITIONS_NOT_SATISFIED
ctap.GetInfo+-f37test[14140]: Traceback (most recent call last):
ctap.GetInfo+-f37test[14140]:   File "/usr/bin/qctap-get-info", line 5, in <module>
ctap.GetInfo+-f37test[14140]:     sys.exit(main())
ctap.GetInfo+-f37test[14140]:              ^^^^^^
ctap.GetInfo+-f37test[14140]:   File "/usr/lib/python3.11/site-packages/qubesctap/sys_usb/qctap_get_info.py", line 35, in main
ctap.GetInfo+-f37test[14140]:     loop.run_until_complete(mux(sys.stdin.buffer.read()))
ctap.GetInfo+-f37test[14140]:   File "/usr/lib64/python3.11/asyncio/base_events.py", line 653, in run_until_complete
ctap.GetInfo+-f37test[14140]:     return future.result()
ctap.GetInfo+-f37test[14140]:            ^^^^^^^^^^^^^^^
ctap.GetInfo+-f37test[14140]:   File "/usr/lib/python3.11/site-packages/qubesctap/sys_usb/mux.py", line 65, in mux
ctap.GetInfo+-f37test[14140]:     stream.write(bytes(response))
ctap.GetInfo+-f37test[14140]:                  ^^^^^^^^^^^^^^^
ctap.GetInfo+-f37test[14140]:   File "/usr/lib/python3.11/site-packages/qubesctap/protocol.py", line 92, in __bytes__
ctap.GetInfo+-f37test[14140]:     return self.to_bytes()
ctap.GetInfo+-f37test[14140]:            ^^^^^^^^^^^^^^^
ctap.GetInfo+-f37test[14140]:   File "/usr/lib/python3.11/site-packages/qubesctap/protocol.py", line 98, in to_bytes
ctap.GetInfo+-f37test[14140]:     raise NotImplementedError()
ctap.GetInfo+-f37test[14140]: NotImplementedError
qctap-make-cred[14146]: asyncio Using selector: EpollSelector
qctap-make-cred[14146]: mux pending=set()
qctap-make-cred[14146]: mux no device, sending fake CONDITIONS_NOT_SATISFIED
u2f.Register+-f37test[14169]: Traceback (most recent call last):
u2f.Register+-f37test[14169]:   File "/usr/bin/qctap-make-credential", line 5, in <module>
u2f.Register+-f37test[14169]:     sys.exit(main())
u2f.Register+-f37test[14169]:              ^^^^^^
u2f.Register+-f37test[14169]:   File "/usr/lib/python3.11/site-packages/qubesctap/sys_usb/qctap_make_credential.py", line 39, in main
u2f.Register+-f37test[14169]:     response = loop.run_until_complete(mux(sys.stdin.buffer.read()))
u2f.Register+-f37test[14169]:                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
u2f.Register+-f37test[14169]:   File "/usr/lib64/python3.11/asyncio/base_events.py", line 653, in run_until_complete
u2f.Register+-f37test[14169]:     return future.result()
u2f.Register+-f37test[14169]:            ^^^^^^^^^^^^^^^
u2f.Register+-f37test[14169]:   File "/usr/lib/python3.11/site-packages/qubesctap/sys_usb/mux.py", line 65, in mux
u2f.Register+-f37test[14169]:     stream.write(bytes(response))
u2f.Register+-f37test[14169]:                  ^^^^^^^^^^^^^^^
u2f.Register+-f37test[14169]:   File "/usr/lib/python3.11/site-packages/qubesctap/protocol.py", line 92, in __bytes__
u2f.Register+-f37test[14169]:     return self.to_bytes()
u2f.Register+-f37test[14169]:            ^^^^^^^^^^^^^^^
u2f.Register+-f37test[14169]:   File "/usr/lib/python3.11/site-packages/qubesctap/protocol.py", line 98, in to_bytes
u2f.Register+-f37test[14169]:     raise NotImplementedError()
u2f.Register+-f37test[14169]: NotImplementedError

Same for u2f.Authenticate.

It seems to be relatively harmless (GetInfo would fail anyway, and Register/Authenticate is retried). Plugging the token in afterwards still works (although for FIDO2 token, I guess behavior is slightly different, as it will be treated as FIDO1/U2F token now; I don't think the proxy can do anything about that).

piotrbartman · 2023-07-12T13:21:38Z

although for FIDO2 token, I guess behavior is slightly different, as it will be treated as FIDO1/U2F token now; I don't think the proxy can do anything about that

After some investigation I think I can handle that but the question is that we want it now or in later PR?

marmarek · 2023-07-12T13:36:57Z

After some investigation I think I can handle that but the question is that we want it now or in later PR?

Can be a later PR, it isn't critical. BTW, how do you plan to do that? have you found a way to get GetInfo call retried?

ctr49 · 2023-09-03T16:57:57Z

it seems this change requires python3-fido2 >= 1.0.0 (only then AttestationResponse was introduced for ctap).
However, Debian ships with lower versions (Bullseye with 0.8.1, Bookworm with 0.9.1) so this will not work on a Debian-based sys-usb.

migration to fido2

703a6d2

piotrbartman force-pushed the prb/fido2 branch from 6f6fff5 to 703a6d2 Compare May 6, 2023 22:17

piotrbartman mentioned this pull request May 6, 2023

Change maximum key handle length to 255. #28

Closed

migration to fido2: CI

dae0c34

piotrbartman added 4 commits May 8, 2023 19:40

migration to fido2: simplyfy from_bytes

0ef2268

migration to fido2: fido2 in ci/requirements

847b3aa

migration to fido2: mypy

d1dd61f

migration to fido2: drops deprecated package 'distutils.version'

363fbee

piotrbartman added 2 commits May 20, 2023 16:44

migration to fido2: renaming

758bf75

migration to fido2: add docs

4c41fab

marmarek reviewed May 20, 2023

View reviewed changes

piotrbartman added 2 commits May 21, 2023 12:11

migration to fido2: trim allowlist

0b5750a

migration to fido2: update architecture

da97bed

This was referenced May 21, 2023

migration to fido2: update architecture scheme QubesOS/qubes-attachment#64

Merged

migration to fido2: update docs QubesOS/qubes-doc#1323

Merged

migration to fido2: pylint + changelog update

2658a58

piotrbartman mentioned this pull request May 21, 2023

migration to fido2: update index QubesOS/qubesos.github.io#224

Merged

migration to fido2: change the message to more informative

912ead0

migration to fido2: just raise error for CTAP2 request for CTAP1 device

46ee032

piotrbartman added 2 commits May 29, 2023 23:58

migration to fido2: backward compatible policies names

00603bc

migration to fido2: backward compatible service running

4735a61

piotrbartman force-pushed the prb/fido2 branch from c21a371 to 4735a61 Compare May 29, 2023 23:42

migration to fido2: fix compatibility for legacy devices

2eefd1e

migration to fido2: fix NotImplementedError when there is no device

62c7f5b

marmarek approved these changes Jul 12, 2023

View reviewed changes

piotrbartman added 2 commits July 13, 2023 12:45

migration to fido2: wait for the user to insert the key

b30819b

migration to fido2: better error message

2075ea3

marmarek approved these changes Jul 16, 2023

View reviewed changes

marmarek merged commit 2075ea3 into QubesOS:main Jul 17, 2023

ctr49 mentioned this pull request Sep 19, 2023

fido2 implementation breaks on Debian-based sys-usb QubesOS/qubes-issues#8529

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

migration to fido2 #31

migration to fido2 #31

piotrbartman commented May 6, 2023 •

edited

Loading

marmarek commented May 6, 2023

piotrbartman commented May 7, 2023

marmarek commented May 7, 2023

codecov-commenter commented May 8, 2023 •

edited

Loading

marmarek May 20, 2023

piotrbartman May 21, 2023

marmarek commented May 21, 2023

piotrbartman commented May 21, 2023

marmarek commented May 21, 2023 •

edited

Loading

marmarek commented May 21, 2023

marmarek commented May 21, 2023

piotrbartman commented May 22, 2023

piotrbartman commented May 22, 2023

marmarek commented May 22, 2023 •

edited

Loading

marmarek commented May 22, 2023

piotrbartman commented May 22, 2023

marmarek commented May 22, 2023 •

edited

Loading

piotrbartman commented May 22, 2023

marmarek commented May 22, 2023

marmarek commented Jul 6, 2023

marmarek commented Jul 6, 2023

piotrbartman commented Jul 6, 2023

marmarek commented Jul 6, 2023

marmarek commented Jul 12, 2023

piotrbartman commented Jul 12, 2023

marmarek commented Jul 12, 2023

ctr49 commented Sep 3, 2023 •

edited

Loading

migration to fido2 #31

migration to fido2 #31

Conversation

piotrbartman commented May 6, 2023 • edited Loading

marmarek commented May 6, 2023

piotrbartman commented May 7, 2023

marmarek commented May 7, 2023

codecov-commenter commented May 8, 2023 • edited Loading

Codecov Report

marmarek May 20, 2023

Choose a reason for hiding this comment

piotrbartman May 21, 2023

Choose a reason for hiding this comment

marmarek commented May 21, 2023

piotrbartman commented May 21, 2023

marmarek commented May 21, 2023 • edited Loading

marmarek commented May 21, 2023

marmarek commented May 21, 2023

piotrbartman commented May 22, 2023

piotrbartman commented May 22, 2023

marmarek commented May 22, 2023 • edited Loading

marmarek commented May 22, 2023

piotrbartman commented May 22, 2023

marmarek commented May 22, 2023 • edited Loading

piotrbartman commented May 22, 2023

marmarek commented May 22, 2023

marmarek commented Jul 6, 2023

marmarek commented Jul 6, 2023

piotrbartman commented Jul 6, 2023

marmarek commented Jul 6, 2023

marmarek commented Jul 12, 2023

piotrbartman commented Jul 12, 2023

marmarek commented Jul 12, 2023

ctr49 commented Sep 3, 2023 • edited Loading

piotrbartman commented May 6, 2023 •

edited

Loading

codecov-commenter commented May 8, 2023 •

edited

Loading

marmarek commented May 21, 2023 •

edited

Loading

marmarek commented May 22, 2023 •

edited

Loading

marmarek commented May 22, 2023 •

edited

Loading

ctr49 commented Sep 3, 2023 •

edited

Loading