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

Cache PIN in Linux keyring #168

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

Cache PIN in Linux keyring #168

wants to merge 2 commits into from

Conversation

Ma27
Copy link

@Ma27 Ma27 commented Feb 18, 2024

Unresolved questions / TODO:

  • Details of how the keyring works
    • Figure out how to safely write the timeout before placing sensitive material into the keyring.
    • Figure out behavioral differences in the keyring between distros/platforms (if any) and document them.
    • Session vs UserSession
  • Behavioral adjustments
    • Only use this code-path for PinPolicy::Once
    • Figure out a way to purge the material when unplugging the yubikey
    • Should the timeout be configurable? If so, how?
    • Perhaps we want to make this opt-in?
  • Code-wise
    • Check if it builds on non-Linux (builds fine on aarch64-darwin).
    • a more elegant way to handle the LInux-only code?

I'm happy to update the docs accordingly when we agreed on whether this is desirable.

cc @lheckemann

Co-authored-by: Linus Heckemann <git@sphalerite.org>
@Ma27
Copy link
Author

Ma27 commented Mar 23, 2024

Pushed another commit which sets the PIN while creating the key and sets the timeout after that. Unfortunately, key_update discards the timeout, so that's the only way to have expiring keys.

Btw cc @str4d, opinions? :)

In fact our little trick with writing garbage into the keyring and
setting the timeout afterwards doesn't seem to work. I noticed rather
late because I'm running this change with a higher timeout locally.

This is because the `key_update` in Linux resets `expiry` in this
case[1].

Originally I also tried to write into `@us` because that's shared
between `systemctl --user` and my Sway session, but it seems as if every
key created in there has the systemd user manager as possessor which
means that I can't do anything from the Sway session. This doesn't seem
to be changeable because newly created keys always have 0x3f010000 as
mask[2].

[1] https://github.com/torvalds/linux/blob/v6.8/security/keys/key.c#L1083
[2] https://github.com/torvalds/linux/blob/v6.8/security/keys/key.c#L905-L915
Copy link
Owner

@str4d str4d left a comment

Choose a reason for hiding this comment

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

I'm sympathetic to the UX issue, and I am not entirely against this change, but I would want to see documentation about how the Linux keyring behaves when used in this way (and if that behaviour is not consistent across all Linux OSs, an examination of those differences) before this could be merged.

PinPolicy::Once means slightly different things to different audiences:

  • To users, it means "once per time that the YubiKey is plugged in". The UX expectation is that the user plugs in their YubiKey, types in the PIN once, and then they don't need to do so again before they unplug their YubiKey. Unplugging and plugging in again requires another PIN entry.
  • To the YubiKey, it means "once per time that the PIV applet is opened on the YubiKey"; switching to the GPG or CTAP applets cause the PIV applet to be closed and the cached PIN dropped.

The discrepancy between these two meanings causes problems for the users' mental model, and I can see the benefit of using a temporary PIN cache to paper over the user's other apps accessing different YubiKey applets (e.g. for SSH authentication or GPG signing). However, we then need a way to ensure that the keyring cache is cleared when the YubiKey unplugs - and that can occur after age-plugin-yubikey has finished running! I don't know what kind of flexibility the Linux keyring offers, but if all we have access to is a timeout, then I don't think this PR is suitable as-is. We'd instead need some way for age-plugin-yubikey to spawn an independent child process that manages that keyring caching, and ensures eviction at the right time.

if let Ok(ring) = maybe_ring {
if let Ok(key) = ring.search(&description) {
if let Ok(cached_pin) = key.read_to_vec() {
if self.yubikey.verify_pin(&cached_pin).is_ok() {
Copy link
Owner

Choose a reason for hiding this comment

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

Any PIN caching MUST respect the configured policy for the slot. If the slot uses PinPolicy::Always, then we must request the PIN directly from the user, and never cache it. What that means in practice is that the only time this cache should be used is PinPolicy::Once.

ring.add_key(&description, &pin.expose_secret().as_bytes())
.map_err(|e| format!("add_key({description}, pin): {e}"))
.and_then(|key| {
key.set_timeout(300).map_err(|e| format!("set_timeout(300): {e}"))?;
Copy link
Owner

Choose a reason for hiding this comment

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

  • How reliable is this timeout?
  • Given that you are not setting a timeout at the point where the PIN is added to the keyring (I presume due to limitations with the linux-keyutils API), this means that if the keyring addition succeeds and then the timeout-setting fails, the PIN is now (IIUC) permanently stored in the keyring (which is not acceptable behaviour).

@Ma27
Copy link
Author

Ma27 commented Mar 30, 2024

Hi!

FIrst of all, thanks a lot for your input!

but I would want to see documentation about how the Linux keyring behaves when used in this way (and if that behaviour is not consistent across all Linux OSs, an examination of those differences) before this could be merged.

👍 will be added to the list above.

Not sure if you've seen the back-and-forth here, after the last iteration I'm not sure if UserSession or Session is what I want, so more research will be necessary anyways.

However, we then need a way to ensure that the keyring cache is cleared when the YubiKey unplugs - and that can occur after age-plugin-yubikey has finished running!

This is a good point. One thing I did so far was to invalidate the keyring when I lock my machine and I wanted to see if I can do it reasonably with udev as well.

Not sure about the capabilities of the keyring itself, but perhaps some kind of udev rule that will be installed with this package may be an option as well? I'm using a distro-provided package though, not sure how this behaves with cargo install yet.

Any PIN caching MUST respect the configured policy for the slot. If the slot uses PinPolicy::Always, then we must request the PIN directly from the user, and never cache it. What that means in practice is that the only time this cache should be used is PinPolicy::Once.

Agreed 👍

Given that you are not setting a timeout at the point where the PIN is added to the keyring (I presume due to limitations with the linux-keyutils API), this means that if the keyring addition succeeds and then the timeout-setting fails, the PIN is now (IIUC) permanently stored in the keyring (which is not acceptable behaviour).

@lheckemann heh, and I were discussing exactly that!

I reversed that however after finding out that you'll lose the timeout when keyctl updateing an entry (https://github.com/torvalds/linux/blob/v6.8/security/keys/key.c#L1083).
See also https://lore.kernel.org/keyrings/ygar0hbrm05.fsf@localhost/T/#m5a684396fdd57947f072809040976f33fff68f8a.
I'd like to understand first why it's implemented that way, but I don't see a reason so far to retain the timeout. We'll need to disable the feature then for kernels w/o this patch.

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.

None yet

2 participants