passkey: implement preflight option #7631

ikerexxe · 2024-10-03T09:59:00Z

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

alexey-tikhonov · 2024-10-03T12:57:21Z

Isn't this ("whether PIN is needed and number of PIN attempts left") security sensitive information?

ikerexxe · 2024-10-03T13:54:58Z

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.

justin-stephenson · 2024-10-03T19:00:15Z

src/passkey_child/passkey_child.c

+        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.


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?

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.

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.

Yes, you are right

src/passkey_child/passkey_child.h

justin-stephenson · 2024-10-03T19:01:56Z

src/passkey_child/passkey_child.c

@@ -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);


1 second timeout seems low?

Yes, the reason is explained in my previous comment

justin-stephenson · 2024-10-03T19:05:14Z

src/passkey_child/passkey_child_common.c

+        && (data->domain == NULL || data->public_key_list == NULL
+        || data->key_handle_list == NULL)) {
+        DEBUG(SSSDBG_OP_FAILURE,
+              "Too few arguments for preflight action.\n");


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).

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.

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]

justin-stephenson · 2024-10-03T19:07:47Z

src/passkey_child/passkey_child_common.c

+done:
+    if (ret != EOK) {
+        data->user_verification = FIDO_OPT_TRUE;
+        pin_retries = MAX_PIN_RETRIES;


Similar to my other comment about printing default values, why set these to some typical value if ret != EOK ? We should error out instead?

This has been clarified in my first reply

justin-stephenson

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]

ikerexxe · 2024-10-04T09:52:58Z

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 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.

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]

Fixed, and thanks for reporting them.

ikerexxe · 2024-10-09T10:00:48Z

This is blocked by #7637

src/passkey_child/passkey_child_devices.c

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]>

ikerexxe · 2024-10-29T15:09:42Z

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.

thalman · 2024-11-26T12:39:41Z

src/passkey_child/passkey_child_devices.c

+
+done:
+    return ret;
+}


nitpicking - could you add new-line at the end of file?

ikerexxe added Waiting for review passkey Issues and PRs related to 'passkey' feature labels Oct 3, 2024

ikerexxe force-pushed the passkey_pin branch 2 times, most recently from ad8168c to fdb0520 Compare October 3, 2024 11:10

ikerexxe added the backport-to-stable label Oct 3, 2024

ikerexxe changed the title ~~Passkey pin~~ passkey: implement preflight option Oct 3, 2024

andreboscatto assigned thalman and justin-stephenson Oct 3, 2024

andreboscatto requested review from thalman and justin-stephenson October 3, 2024 12:50

justin-stephenson reviewed Oct 3, 2024

View reviewed changes

src/passkey_child/passkey_child.h Outdated Show resolved Hide resolved

justin-stephenson reviewed Oct 3, 2024

View reviewed changes

ikerexxe force-pushed the passkey_pin branch from fdb0520 to 987935a Compare October 4, 2024 09:50

ikerexxe added the Blocked label Oct 8, 2024

justin-stephenson reviewed Oct 9, 2024

View reviewed changes

src/passkey_child/passkey_child_devices.c Show resolved Hide resolved

ikerexxe mentioned this pull request Oct 11, 2024

man: add sssd-passkey.5 #7637

Closed

alexey-tikhonov added branch: sssd-2-9 branch: sssd-2-10 and removed backport-to-stable labels Oct 15, 2024

ikerexxe added 3 commits October 29, 2024 16:07

man: add sssd-passkey.5

e37b7fc

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]>

passkey: timeout argument refactor

afba3a4

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]>

ikerexxe force-pushed the passkey_pin branch from 987935a to 01363fe Compare October 29, 2024 15:08

ikerexxe marked this pull request as draft October 29, 2024 15:09

ikerexxe removed the Waiting for review label Oct 29, 2024

thalman requested changes Nov 26, 2024

View reviewed changes

src/passkey_child/passkey_child_devices.c

done:

return ret;

}

Copy link

Contributor

thalman Nov 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpicking - could you add new-line at the end of file?

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

passkey: implement preflight option #7631

passkey: implement preflight option #7631

ikerexxe commented Oct 3, 2024 •

edited

Loading

alexey-tikhonov commented Oct 3, 2024

ikerexxe commented Oct 3, 2024

justin-stephenson Oct 3, 2024

ikerexxe Oct 4, 2024

justin-stephenson Oct 4, 2024

ikerexxe Oct 7, 2024

justin-stephenson Oct 3, 2024

ikerexxe Oct 4, 2024

justin-stephenson Oct 3, 2024

ikerexxe Oct 4, 2024

justin-stephenson Oct 4, 2024

justin-stephenson Oct 3, 2024

ikerexxe Oct 4, 2024

justin-stephenson left a comment •

edited

Loading

ikerexxe commented Oct 4, 2024

ikerexxe commented Oct 9, 2024

ikerexxe commented Oct 29, 2024

thalman Nov 26, 2024

passkey: implement preflight option #7631

Are you sure you want to change the base?

passkey: implement preflight option #7631

Conversation

ikerexxe commented Oct 3, 2024 • edited Loading

alexey-tikhonov commented Oct 3, 2024

ikerexxe commented Oct 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justin-stephenson left a comment • edited Loading

Choose a reason for hiding this comment

ikerexxe commented Oct 4, 2024

ikerexxe commented Oct 9, 2024

ikerexxe commented Oct 29, 2024

Choose a reason for hiding this comment

ikerexxe commented Oct 3, 2024 •

edited

Loading

justin-stephenson left a comment •

edited

Loading