Skip to content

Conversation

@ikerexxe
Copy link
Contributor

@ikerexxe ikerexxe commented Oct 9, 2024

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.

@ikerexxe
Copy link
Contributor Author

ikerexxe commented Oct 9, 2024

@justin-stephenson and @sumit-bose here is the documentation that we agreed to create yesterday. Please take a look at it when you have some time.

@justin-stephenson
Copy link
Contributor

Thanks very much for working on this @ikerexxe

To understand more clearly I did some testing of our current handling of passkey user-verification, specifically with kerberos-enabled passkey auth with IPA.

When IPA require-user-verification == true and my yubikey device has NO PIN set: device auth will fail as we always try to send a PIN to the passkey_child. If 'enter' is pressed without typing any characters then sssd falls back from passkey to password auth, instead of sending an empty PIN string.

When IPA require-user-verification == false and my yubikey device has a PIN set: device auth will fail as no PIN is sent to SSSD and then to the passkey_child.

Is this the expected behavior when a device does not match the IPA server side policy to require user verification?

@ikerexxe
Copy link
Contributor Author

When IPA require-user-verification == true and my yubikey device has NO PIN set: device auth will fail as we always try to send a PIN to the passkey_child.

Yes, that's the expected behaviour and that's what I intended to convey with:

sssd.conf Device Result
True User verification is not configured Authentication fails

I know I mention sssd.conf instead of IPA, but I wanted to simplify the table and taking into account that sssd.conf takes precedence over IPA I thought this was the best to achieve it. If this is not understood, let me know and we will try to improve the text.

If 'enter' is pressed without typing any characters then sssd falls back from passkey to password auth, instead of sending an empty PIN string.

Should we mention this in the man page?

When IPA require-user-verification == false and my yubikey device has a PIN set: device auth will fail as no PIN is sent to SSSD and then to the passkey_child.

This doesn't align with the text I wrote. I based my understanding in the local case, where https://github.com/SSSD/sssd/blob/master/src/passkey_child/passkey_child_credentials.c#L133 is executed, and thus, the PIN is requested. Taking into account that the device won't allow the authentication without the PIN I think this is something that we need to improve in the online/kerberos workflow. As we'll run the preflight option in the preauthentication phase, we can use it to prepare for this use case and request the PIN. WDYT?

<row>
<entry>True</entry>
<entry>User verification is not configured</entry>
<entry>Authentication fails</entry>
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi,

I think might be more consistent to handle this case as if no (suitable) device is available at the pre-flight check. Then users have the chance to insert another device which requires a PIN before they enter the PIN.

bye,
Sumit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I wrote refers to what happens in the authentication phase. By the previous message you mean that I should also explain the preflight phase in the man pages?

By the way, the behaviour that you mention is what is currently proposed in #7631, as errors are ignored at the passkey_child.c level, thus leaving an option for the user to insert another FIDO2 device between the preauth and auth phases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi,

no, the pre-flight is is internal and should not be mentioned in the man page explicitly. What I meant is that "Authentication fails" give the impression that authentication fails immediately but I guess in reality what happens in the authentication phase is more a "User verification is requested but it is expected that authentication fails if no other device is inserted in between".

bye,
Sumit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you take a look at the updated text? I think I have reflected what you mention, but I would prefer to have a confirmation.

@justin-stephenson
Copy link
Contributor

justin-stephenson commented Oct 11, 2024

If 'enter' is pressed without typing any characters then sssd falls back from passkey to password auth, instead of sending an empty PIN string.

Should we mention this in the man page?

We can mention this.

When IPA require-user-verification == false and my yubikey device has a PIN set: device auth will fail as no PIN is sent to SSSD and then to the passkey_child.

This doesn't align with the text I wrote. I based my understanding in the local case, where https://github.com/SSSD/sssd/blob/master/src/passkey_child/passkey_child_credentials.c#L133 is executed, and thus, the PIN is requested. Taking into account that the device won't allow the authentication without the PIN I think this is something that we need to improve in the online/kerberos workflow. As we'll run the preflight option in the preauthentication phase, we can use it to prepare for this use case and request the PIN. WDYT?

Okay, yes I agree that this is a better approach. If UV is false (whether sssd.conf or IPA require-user-verification) and the device has a PIN set then SSSD will automatically determine this when it queries if the device has a PIN set and effectively re-enable UV for this auth(prompt for a PIN). This is also something to clearly state in the man page IMO.

@justin-stephenson
Copy link
Contributor

This doesn't align with the text I wrote. I based my understanding in the local case, where https://github.com/SSSD/sssd/blob/master/src/passkey_child/passkey_child_credentials.c#L133 is executed, and thus, the PIN is requested. Taking into account that the device won't allow the authentication without the PIN I think this is something that we need to improve in the online/kerberos workflow. As we'll run the preflight option in the preauthentication phase, we can use it to prepare for this use case and request the PIN. WDYT?

Okay, yes I agree that this is a better approach. If UV is false (whether sssd.conf or IPA require-user-verification) and the device has a PIN set then SSSD will automatically determine this when it queries if the device has a PIN set and effectively re-enable UV for this auth(prompt for a PIN). This is also something to clearly state in the man page IMO.

I suppose this will need to be fixed in the code as part of #7631 or my SSSD PR when it comes.

@ikerexxe
Copy link
Contributor Author

@justin-stephenson I updated the text to include the password fallback and the user verification from the device. Take a look at it when you have some time.

I suppose this will need to be fixed in the code as part of #7631 or my SSSD PR when it comes.

Yes, that would be the best approach.

@justin-stephenson
Copy link
Contributor

@justin-stephenson I updated the text to include the password fallback and the user verification from the device. Take a look at it when you have some time.

Looks good to me, thank you Iker.

I suppose this will need to be fixed in the code as part of #7631 or my SSSD PR when it comes.

Yes, that would be the best approach.

@alexey-tikhonov
Copy link
Member

@ikerexxe, please set backport labels.

@ikerexxe
Copy link
Contributor Author

@ikerexxe, please set backport labels.

I'm taking this PR as a forum to agree on the documentation. Once we all have the same understanding I'll close it and include the commit in #7631, as we'll need to adapt the behaviour a little bit to match the documentation.

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 <ipedrosa@redhat.com>
@ikerexxe
Copy link
Contributor Author

Hi @sumit-bose,

Are you ok with the current proposal? If so, I'd like to close this PR and move the changes to #7631 to have everything centralized in one place.

@sumit-bose
Copy link
Contributor

Hi @sumit-bose,

Are you ok with the current proposal? If so, I'd like to close this PR and move the changes to #7631 to have everything centralized in one place.

Hi,

thank you for the reminder, I just added a comment to one of the conversations, but feel free to ignore it, if you think the current sentence is clear enough. Otherwise I do not have further comments. Please go on and move the changes to #7631.

bye,
Sumit

@ikerexxe
Copy link
Contributor Author

thank you for the reminder, I just added a comment to one of the conversations, but feel free to ignore it, if you think the current sentence is clear enough. Otherwise I do not have further comments. Please go on and move the changes to #7631.

Updated, so I'm closing this PR and moving the changes to the other one. Thank you both for your reviews!

@ikerexxe ikerexxe closed this Oct 29, 2024
@ikerexxe ikerexxe deleted the man_passkey branch October 29, 2024 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants