PAM: Add passkey preflight operation

[justin-stephenson]

PIN attempts message is only shown when <= 3

justin@fedora:~$ su - pkuser
Insert your passkey device, then press ENTER.
Enter PIN:
You have 3 PIN attempts remaining

In addition, refactored a lot of PAM passkey child related code

[ikerexxe]

I did not check the code, but went straight to testing. There is one thing that caught my attention. When the number of attempts remaining is less than 3 the user is supposed to get that number before asking for the PIN, so that they are aware that they are in a risky situation. Something like:

$ su - iker@ipa.test
Insert your passkey device, then press ENTER.
You have 3 PIN attempts remaining
Enter PIN:
su: Authentication failure

In general terms, passkey preflight should be run during preauth stage. I don't know how hard it would be to implement it this way but it would make more sense.

[justin-stephenson]

I did not check the code, but went straight to testing. There is one thing that caught my attention. When the number of attempts remaining is less than 3 the user is supposed to get that number before asking for the PIN, so that they are aware that they are in a risky situation. Something like:

$ su - iker@ipa.test
Insert your passkey device, then press ENTER.
You have 3 PIN attempts remaining
Enter PIN:
su: Authentication failure

In general terms, passkey preflight should be run during preauth stage. I don't know how hard it would be to implement it this way but it would make more sense.

It is run during preauth stage, I just had to move around the pam_sss code which sends the message to the console. Please try again.

[ikerexxe]

Ok, now the testing worked as expected:

su - iker@ipa.test
Insert your passkey device, then press ENTER.
You have 3 PIN attempts remaining

Enter PIN:
su: Authentication failure

I didn't check all the code, but there is something that caught my eye. See inline.

[justin-stephenson]

@ikerexxe Please confirm these are the appropriate backport labels to use also

[alexey-tikhonov]

@ikerexxe Please confirm these are the appropriate backport labels to use also

What RHEL versions do you target?

[ikerexxe]

This PR supersedes #7631

[ikerexxe]

Just some minor comments. By the way, I forgot to mention that I tested it and it works as expected

[justin-stephenson]

I've left a bunch of (pretty cosmetic) comments inline.

But I'm only reading code, I can't actually test this.

Thus I'll add @madhuriupadhye as additional reviewer (to actually test this and also fix passkey tests that are broken by this PR).

Thank you for the review. I addressed comments related to my code, and will let Iker address his commits when he returns from PTO.

[ikerexxe]

I've updated the code to address Alexey's concerns

[alexey-tikhonov]

@ikerexxe, thanks for updates.

Do I understand correctly that the only point of passkey: timeout argument refactor patch is to use 'timeout = 1' for preflight()?

I think it's not enough.

'sssd_pam' should provide timeout as a command line argument to 'passkey_child' and this (perhaps decreased by 1) should be used instead of TIMEOUT define (see 'p11_child' as example).

What about this ^^?

[ikerexxe]

Do I understand correctly that the only point of passkey: timeout argument refactor patch is to use 'timeout = 1' for preflight()?
I think it's not enough.
'sssd_pam' should provide timeout as a command line argument to 'passkey_child' and this (perhaps decreased by 1) should be used instead of TIMEOUT define (see 'p11_child' as example).

Unless you plan to provide a way for this to be configurable I don't see any difference between the actual implementation and your proposal. Thus, I'd prefer to keep it as it is.

[alexey-tikhonov]

Do I understand correctly that the only point of passkey: timeout argument refactor patch is to use 'timeout = 1' for preflight()?
I think it's not enough.
'sssd_pam' should provide timeout as a command line argument to 'passkey_child' and this (perhaps decreased by 1) should be used instead of TIMEOUT define (see 'p11_child' as example).

Unless you plan to provide a way for this to be configurable

It is already configurable via sssd.conf::passkey_child_timeout option.

I don't see any difference between the actual implementation and your proposal.

The issue is that 'passkey_child' doesn't use it.

[ikerexxe]

There are two distinct timeouts happening here: the timeout you're referring to controls how long the PAM responder will wait for the passkey_child process to complete. The other, separate timeout determines how much time the user has to physically connect their FIDO2 token.

[alexey-tikhonov]

There are two distinct timeouts happening here: the timeout you're referring to controls how long the PAM responder will wait for the passkey_child process to complete. The other, separate timeout determines how much time the user has to physically connect their FIDO2 token.

They should be related. At the very least 2nd can't be greater than 1st.
Take 'p11_child' as an example: all internal child timeouts should be less than governing sssd_pam timeout.

[ikerexxe]

Agreed to tackle the timeout topic on a different PR.

[thalman]

LGTM

[justin-stephenson]

@ikerexxe @madhuriupadhye @alexey-tikhonov Hi, can you approve the PR if you have no further comments/concerns?

[ikerexxe]

CI, and more specifically the system tests, are in the red, and I believe this is due to our changes. I have asked Madhuri to review them.

[alexey-tikhonov]

@ikerexxe @madhuriupadhye @alexey-tikhonov Hi, can you approve the PR if you have no further comments/concerns?

IIUC, we need tests to be updated alongside with this PR.
I don't care if it's part of this PR or a separate PR, but it should be ready.