Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

util: return PAM_CRED_UNAVAIL from do_authentication() if no suitable devices are connected #360

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SimonPilkington
Copy link

Allows the case from #322 to be handled with the PAM stack. Comments re: suitability of return code and any other pits I may have fallen in welcome.

@LDVG
Copy link
Contributor

LDVG commented Mar 28, 2025

Hi,

Thank you for the PR. Suitability of return code is indeed a good question, one of our aims moving forward is to be more consistent return codes so whatever we end up choosing will need good documentation (#320).

At a glance, this looks slightly more involved than the original request that only seemed concerned about the case where there was no authenticator plugged in to the system at all -- checking if there was no authenticator plugged in that recognized any of the enrolled credentials. Was that the intention?

If so, one pitfall is then that our current handling of discoverable credentials would not work with this implementation as we unconditionally try all plugged in authenticators in that case.

@SimonPilkington
Copy link
Author

Suitability of return code is indeed a good question, one of our aims moving forward is to be more consistent return codes so whatever we end up choosing will need good documentation (#320).

pam_types.h defines

#define PAM_CRED_UNAVAIL 15	/* Underlying authentication service */
				                    /* can not retrieve user credentials */

I think an enrolled device being plugged in counts as a necessary part of a user's credentials in this case so I chose that code. But like I said, I'm open to suggestions. Here's the whole list.

checking if there was no authenticator plugged in that recognized any of the enrolled credentials. Was that the intention?

Yes. I assumed a device would be somehow matched to the auth info from u2f_keys before asking for the PIN or presence. Is that not the case? If so then yeah, this is bogus as is.

@LDVG
Copy link
Contributor

LDVG commented Mar 28, 2025

Is that not the case?

When a discoverable / resident credential is used, all plugged in authenticators are tried unconditionally.

@SimonPilkington
Copy link
Author

SimonPilkington commented Mar 28, 2025

When a discoverable / resident credential is used, all plugged in authenticators are tried unconditionally.

I see. I wasn't completely clear on the definition of discoverable credentials but I looked up the relevant docs and I get it now. My bad.

If this is to be merged then the simplest option for now would be to only support it when there are per-user credential files configured and the user doesn't have any discoverable credentials enrolled, I suppose. I don't believe discoverable credentials make sense outside of a centralised authfile anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants