passkey: implement preflight option #7631
Conversation
ikerexxe
added
Waiting for review
passkey
ikerexxe
force-pushed
the
passkey_pin
branch
2 times, most recently
from
ad8168c
to
fdb0520
Compare
|
Isn't this ("whether PIN is needed and number of PIN attempts left") security sensitive information? |
|
Yes, but there's nothing an attacker can do that they couldn't do before knowing that information. Moreover, this information can be obtained by other tools already present in the system. Generally speaking, if an attacker wants to block a given FIDO2 key, then they just need to enter the PIN at most 8 times incorrectly, as that's the maximum number. As for the PIN request, they can always try to authenticate once and enter an empty PIN to see whether that's accepted. |
| ret = preflight(&data, 1); | ||
| /* Errors are ignored, as in most cases they are due to the device not | ||
| * being connected to the system. If an error occurs, the default | ||
| * values are returned, and that is sufficient for the time being. |
There was a problem hiding this comment.
I don't understand this comment, why would we print default values if the device is not connected to the system? Shouldn't we print some error in obtaining preflight data?
There was a problem hiding this comment.
The preflight operation takes place before authentication, and should take as little time as possible, as we do not want to disturb the user. It is a mere check of the options, and whether we can deduce from the available information the exact workflow the user will follow. If the information is not available (i.e. the FIDO2 device is not present), then we will present the user with the complete workflow. This is a limitation of the SSSD design.
There was a problem hiding this comment.
Does it mean SSSD will need to fork/exec the passkey child an additional time with this --preflight operation prior to the --get-assert operation? I suppose it makes sense to hold off on reviewing this PR fully until the SSSD implementation is worked on. I'm still a bit unclear how this implementation will work with SSSD/GDM.
| @@ -93,6 +93,12 @@ int main(int argc, const char *argv[]) | |||
| ERROR("Verification error.\n"); | |||
| goto done; | |||
| } | |||
| } else if (data.action == ACTION_PREFLIGHT) { | |||
| ret = preflight(&data, 1); | |||
There was a problem hiding this comment.
1 second timeout seems low?
There was a problem hiding this comment.
Yes, the reason is explained in my previous comment
| && (data->domain == NULL || data->public_key_list == NULL | ||
| || data->key_handle_list == NULL)) { | ||
| DEBUG(SSSDBG_OP_FAILURE, | ||
| "Too few arguments for preflight action.\n"); |
There was a problem hiding this comment.
AFAIK there is no way to tell what arguments should be provided to passkey_child --preflight
/usr/libexec/sssd/passkey_child --preflight --help does not say and /usr/libexec/sssd/passkey_child only prints Invalid argument(s).
There was a problem hiding this comment.
Same as with other options. In this particular case this option is going to be executed by SSSD and not by the user, so I don't see a reason why we should indicate which options are missing.
There was a problem hiding this comment.
To me this seems like we are hiding how to actually use passkey_child inside SSSD codebase. At least for troubleshooting some users may end up wanting to execute passkey_child directly outside of SSSD. I don't insist about it, maybe we can create a separate ticket to improve this.
For comparison I noticed p11_child provides some hints about which arguments are missing.
justin@fedora:~$ /usr/libexec/sssd/p11_child --auth
Missing CA DB path: --ca_db must be specified.
Usage: p11_child [-?] [-?|--help] [--usage] [-d|--debug-level=INT] [--debug-timestamps=INT] [--debug-microseconds=INT] [--dumpable=INT] [--debug-fd=INT] [--logger=stderr|files|journald] [--auth] [--pre] [--wait_for_card]
[--verification] [--pin] [--keypad] [--verify=STRING] [--ca_db=STRING] [--module_name=STRING] [--token_name=STRING] [--key_id=STRING] [--label=STRING] [--certificate=STRING] [--uri=STRING] [--chain-id=LONG]
justin@fedora:~$ /usr/libexec/sssd/p11_child --ca_db test --auth
Missing PIN mode for authentication, either --pin or --keypad must be specified.
Usage: p11_child [-?] [-?|--help] [--usage] [-d|--debug-level=INT] [--debug-timestamps=INT] [--debug-microseconds=INT] [--dumpable=INT] [--debug-fd=INT] [--logger=stderr|files|journald] [--auth] [--pre] [--wait_for_card]
[--verification] [--pin] [--keypad] [--verify=STRING] [--ca_db=STRING] [--module_name=STRING] [--token_name=STRING] [--key_id=STRING] [--label=STRING] [--certificate=STRING] [--uri=STRING] [--chain-id=LONG]
| done: | ||
| if (ret != EOK) { | ||
| data->user_verification = FIDO_OPT_TRUE; | ||
| pin_retries = MAX_PIN_RETRIES; |
There was a problem hiding this comment.
Similar to my other comment about printing default values, why set these to some typical value if ret != EOK ? We should error out instead?
There was a problem hiding this comment.
This has been clarified in my first reply
There was a problem hiding this comment.
Added some comments/concerns. I'm not sure but perhaps a sssctl wrapper for the --preflight operation would be useful if we anticipate admins to request this information?
I also see some compiler warnings,:
../src/passkey_child/passkey_child_devices.c: In function ‘list_devices’:
../src/passkey_child/passkey_child_devices.c:54:12: warning: ‘ret’ may be used uninitialized [-Wmaybe-uninitialized]
passkey_child/passkey_child_devices.c: In function ‘get_device_pin_retries’:
../src/passkey_child/passkey_child_devices.c:267:12: warning: ‘ret’ may be used uninitialized [-Wmaybe-uninitialized]
I don't think this is specially useful for them. In fact, it is possible that at some point we will need to add options to look at these kinds of parameters, but not in this way. For now, let's be happy that there are already other tools that allow you to check these options.
Fixed, and thanks for reporting them. |
|
This is blocked by #7637 |
alexey-tikhonov
added
branch: sssd-2-9
branch: sssd-2-10
and removed
backport-to-stable
labels
Include a new man page for passkey to explain the behaviour of `user_verification` option in the different scenarios. It is a complex option, so it has been decided to add a table to simplify its understanding. Signed-off-by: Iker Pedrosa <[email protected]>
Timeout to search for a device should be imposed by the action that wants to be performed. Thus, refactor the high level functions and list_devices() to take it as an argument. Signed-off-by: Iker Pedrosa <[email protected]>
Expand the passkey_child to detect whether a FIDO2 device needs a PIN and the number of attempts left for the PIN. This is needed to improve the overall user experience by providing a way for SSSD to detect these features before the user is requested to interact with them. Expected input to run passkey_child: `--preflight`, `--domain`, `--key-handle` and `--public-key`. Output: whether PIN is needed (boolean) and number of PIN attempts left (integer). Example: 1 8 Signed-off-by: Iker Pedrosa <[email protected]>
|
Although #7637 was clarified Justin still has to implement his part to make this PR meaningful, so for the moment I am moving to draft. |
Expand the passkey_child to detect whether a FIDO2 device needs a PIN
and the number of attempts left for the PIN. This is needed to improve
the overall user experience by providing a way for SSSD to detect these
features before the user is requested to interact with them.
Expected input to run passkey_child:
--preflight,--domain,--key-handleand--public-key.Output: whether PIN is needed (boolean) and number of PIN attempts left
(integer). Example:
1
8