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

authtok: add support for decrypting an auth token #294

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

zhihengq
Copy link

Add support for storing an encrypted password in u2f_keys. During authentication, the password is decrypted and passed to PAM for other PAM modules.

Problem

When logging in with pam_u2f as a single factor, gnome-keyring cannot be auto-unlocked. This is because no password was provided during authentication, but gnome-keyring requires the user password to decrypt its database. Another user reported the same problem in #283.

Solution

This PR allows pamu2fcfg to optionally store a password for each authenticator. During authentication, pam_u2f retrieves the password and pass it to PAM as PAM_AUTHTOK. Subsequent PAM modules such as pam_gnome_keyring will use the password in PAM_AUTHTOK to auto-unlock the keyring.

Passwords are encrypted with AES-256-GCM and stored in the u2f_key file, along with an HMAC salt. The AES encryption key is derived using the FIDO2 HMAC-SECRET extension.

New arguments added:

  • pam_u2f:
    • allowauthtok: an optional argument that enables this feature.
  • pamu2fcfg:
    • -a, --authtok: an optional flag to prompt the user for a password to be stored.
    • -p, --pam-arguments=STRING: an optional argument that specifies the arguments passed to pam_u2f. This is used to ensure users are authenticated in the same way when getting the HMAC secret.

Next steps

Update the documentation for this feature.

Add support for storing an encrypted auth token in u2f_keys.
The auth token is passed to PAM and may be used by other PAM modules.
The encryption key is derived using the FIDO2 hmac-secret extension.
@LDVG
Copy link
Contributor

LDVG commented May 8, 2023

Hi,

First of all, thank you for the contribution!

I have given this a brief look and some thinking and wanted to write those down. As pointed out in the linked issue, there's a set of complexities here I'm worried about:

  • On a password change, the user would have to encrypt the new secret for each of their registered authenticators.
  • If the UV requirements changes in the service configuration or authfile, the same problem applies.

The first source of problems could potentially be partially alleviated by implementing pam_sm_chauthtok(), but would bring in a lot of new code. There's no hooks for us to detect the second problem and it could be worth considering to e.g. always require some form of UV for this feature as a workaround.

In any case, I think we'll have to carefully consider these consequences, how to deal with them, and whether the added complexity is worthwhile before being able to move forward. Any thoughts or suggestions?

@zhihengq
Copy link
Author

I don't think we should implement pam_sm_chauthtok() because:

  1. For pam_sm_chauthtok() to work, we need the authenticator to be present to encrypt the new password. And I don't feel that requiring the authenticator is reasonable when the user simply wants to change their password.
  2. The auth token that the user wants to set might not be the account password. One such case is when using full-disk encryption, a user might want to set the gnome-keyring password to be the same as the disk encryption password to achieve auto-unlocking. In this case, changing the account password shouldn't overwrite the auth token saved in the authfile.

Since we also can't verify if the current auth token still works with gnome-keyring (or any other service that the auth token is for), there is not much we can do here.

For UV requirement change, we can only detect this case during the next authentication by failing to validate the AEAD tag. At this point the encrypted auth token is already lost. The best we could do is to skip the auth token and report this error to the user.

In both of the cases, the user has to manually set up the auth token again with pamu2fcfg. The solution is definitely not ideal, but I can't come up with anything better. We could potentially change pamu2fcfg to support updating existing auth tokens, but I'm not sure if the benefit is worth the added complexity. That being said, I still think this feature would be helpful to some users, since entering password for gnome-keyring defeats the purpose of using YubiKeys in a passwordless setup.

@LDVG
Copy link
Contributor

LDVG commented May 16, 2023

Thank you for the feedback.

  1. For pam_sm_chauthtok() to work, we need the authenticator to be present to encrypt the new password. And I don't feel that requiring the authenticator is reasonable when the user simply wants to change their password.

Marking the module as optional could help for this, I suppose.

  1. The auth token that the user wants to set might not be the account password. One such case is when using full-disk encryption, a user might want to set the gnome-keyring password to be the same as the disk encryption password to achieve auto-unlocking. In this case, changing the account password shouldn't overwrite the auth token saved in the authfile.

Since we also can't verify if the current auth token still works with gnome-keyring (or any other service that the auth token is for), there is not much we can do here.

Good point. For some reason, this use-case escaped me and, as you say, is probably sufficient reason to not do something like that -- at least not without somehow knowing that the user wants to wrap their login credentials.

For UV requirement change, we can only detect this case during the next authentication by failing to validate the AEAD tag. At this point the encrypted auth token is already lost. The best we could do is to skip the auth token and report this error to the user.

Ack.

In both of the cases, the user has to manually set up the auth token again with pamu2fcfg. The solution is definitely not ideal, but I can't come up with anything better. We could potentially change pamu2fcfg to support updating existing auth tokens, but I'm not sure if the benefit is worth the added complexity.

Agreed. At least for a potential first stab at this, the preference would be to keep it as simple as possible and manually dealing with the problem using pamu2fcfg would probably be the way to go (as cumbersome as that may be).

I have not yet had time to take a closer look at your implementation or try it out for myself. I hope to be able to do so soon.

Copy link
Contributor

@LDVG LDVG left a comment

Choose a reason for hiding this comment

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

Apologies for the delay. I've played around with this for a bit and left a couple of mostly high-level comments.

There's a few things we need to take into consideration, I hope I managed to cover most of them in the in-line comments. (It's quite a large diff which might benefit being split into smaller commits/parts/multiple MR:s to ease the design/review process.)

Thanks again for having looked into this!

authtok.c Show resolved Hide resolved
authtok.c Outdated Show resolved Hide resolved
authtok.c Outdated Show resolved Hide resolved
authtok.c Show resolved Hide resolved
util.c Outdated Show resolved Hide resolved
util.c Outdated Show resolved Hide resolved
authtok.c Outdated Show resolved Hide resolved
authtok.c Outdated Show resolved Hide resolved
pamu2fcfg/pamu2fcfg.c Outdated Show resolved Hide resolved
pamu2fcfg/pamu2fcfg.c Outdated Show resolved Hide resolved
@akallabeth
Copy link

akallabeth commented May 7, 2024

hi, did test out this pr and it seems to address exactly the remaining issue I have (full debian login with fido2/u2f token)
what is preventing this pr from being merged? May I help in some way?

note: did successfully test this:

  1. install this patched version instead of system pam on my debian 12
  2. modify /etc/pam.d/common-auth to contain auth [success=ok new_authtok_reqd=ok ignore=ignore default=ignore] pam_u2f.so userpresence=1 userverification=0 pinverification=1 allowauthtok (do not use sufficient, it will prevent gnome-keyring from being unlocked)
  3. generate the yubikey file in ~/.config/Yubico/u2f_keys withoutput from pamu2fcfg -a (enter PIN, press button, enter login password, press button)
  4. reboot, login using GDM with PIN and yubikey only

@zhihengq
Copy link
Author

zhihengq commented May 8, 2024

@LDVG I think I addressed your previous concerns. Could you take another look? Thanks!

@akallabeth
Copy link

@zhihengq only thing I found a bit lacking is the debug output.
maybe add 2 more lines to tell if

  1. the authtoken will be used (not the config setting, but if it is actually tried to decrypt)
  2. if the authtoken was successfully decrypted

@akallabeth
Copy link

as an idea (most likely for a follow up pull) maybe it would also be possible to implement pam_sm_chauthtok to update the data in the mapping on password change?

@zhihengq
Copy link
Author

@zhihengq only thing I found a bit lacking is the debug output. maybe add 2 more lines to tell if

1. the `authtoken` will be used (not the config setting, but if it is actually tried to decrypt)
2. if the `authtoken` was successfully decrypted

That's a great idea! Could you help me put it in? Or I can do that when I have some time.

as an idea (most likely for a follow up pull) maybe it would also be possible to implement pam_sm_chauthtok to update the data in the mapping on password change?

I'm not exactly sure how to make it work. Changing the auth token requires presence of the fido2 device (and possibly human interaction) to encrypt the new token. What happens if the fido2 device is not present?

@akallabeth
Copy link

1. the `authtoken` will be used (not the config setting, but if it is actually tried to decrypt)
2. if the `authtoken` was successfully decrypted

That's a great idea! Could you help me put it in? Or I can do that when I have some time.

ok, will push to your branch.

as an idea (most likely for a follow up pull) maybe it would also be possible to implement pam_sm_chauthtok to update the data in the mapping on password change?

I'm not exactly sure how to make it work. Changing the auth token requires presence of the fido2 device (and possibly human interaction) to encrypt the new token. What happens if the fido2 device is not present?

well, same as other PAM modules I guess, depending on configuration?
e.g. if it is required changing fails, if it is optional changing succeeds even if the token did not?

@akallabeth
Copy link

ok, can´t push to your branch, so here is the patch:

patch.zip

Debug log whether authtok is enabled and when token descryption is
successful.
Yubico#294 (comment)
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.

None yet

3 participants