Skip to content

Comments

Feature Implementation: PIN#308

Open
antotocar34 wants to merge 8 commits intodoy:mainfrom
antotocar34:pin
Open

Feature Implementation: PIN#308
antotocar34 wants to merge 8 commits intodoy:mainfrom
antotocar34:pin

Conversation

@antotocar34
Copy link
Contributor

Related issues: #166 #297

This PR implements a pin feature to rbw. The design is described in issue #297. All additions are gated behind the "pin" feature flag. I've implemented two 'backends' for the local secret that enables the pin. One is using the age crate to allow the user to encrypt the vault decryption key using a hardware token backed age identity. The other just uses the operating systems keyring, using the keyring crate.

I'm happy to take feedback. I realise this is a big PR, so happy to answer any questions. I've been using this build for a few months and have had no issues with it so far.

DEMO

Here is a demo of how it looks like using an age-plugin-se identity.

pin_age_demo.mov

AI Usage Disclaimer:
AI was used in the following way to make this PR: AI was used when I was stuck on a bug, and to provide a second opinion on implementation decisions. I also used AI to reorder and clean the commit history before submitting this PR.

@doy
Copy link
Owner

doy commented Dec 27, 2025

thanks for this! this is a very large pull request with a lot of new cryptography, so it will probably take me a while to fully review it, but i did have some initial questions:

  • the potential backends are a bit varied in terms of their guarantees - tpm and secure enclave make the most obvious sense to me, but yubikeys can be moved between machines, and i don't know all that much about the properties of the macos keychain (can it be replicated to a new machine via icloud or whatever? does that matter?). this is maybe okay, but we should at least be more explicit about where the security boundaries are, and what the tradeoffs are between different backends. (also, the docs say that the windows and linux system keyrings are supported, but that doesn't appear to be the case given the features that are enabled for the keyring crate.)
  • given that the purpose of this is for the credentials to be device-bound, we can probably do better than hardcoded argon2 parameters here. could we instead look at the running system to get a sense of reasonable memory usage, and try a few different parameters to get to a configuration that takes a reasonable amount of time? hardcoded parameters are necessary in cases where a lot of different clients are going to be interacting with the same secret, but that shouldn't really be a restriction here.

@doy
Copy link
Owner

doy commented Dec 27, 2025

one other thing that i think would be useful would be an option to store the wrapped keys in runtime_dir() instead of cache_dir() - this would require unlocking with the master password once when you restart your computer but would mean that the less well protected key material is never saved to disk (on linux at least)

@doy
Copy link
Owner

doy commented Jan 1, 2026

one other thing to consider here - while using the device-local backends does prevent fully offline attacks, any amount of access to the machine is sufficient to allow for unlimited unlock attempts, which isn't great if there is a desire to allow for very short (or empty) pins. how possible would it be to move the entire operation into the secure environment, which would allow for things like limiting the number of pin entry attempts, etc? this would be more work to implement, but it'd be nice to at least keep the option open.

@antotocar34
Copy link
Contributor Author

antotocar34 commented Jan 8, 2026

Thanks for your feedback and questions :)

given that the purpose of this is for the credentials to be device-bound, we can probably do better than hardcoded argon2 parameters here. could we instead look at the running system to get a sense of reasonable memory usage, and try a few different parameters to get to a configuration that takes a reasonable amount of time

I considered this, but chose not to do so for simplicity. I can add it as a TODO. I chose to allow the user to set the KDF params in the config file argon2_iterations argon2_memory argon2_parallelism, although this is not documented and could probably benefit from better UX.

the potential backends are a bit varied in terms of their guarantees

good point, it raises the question whether we only want to allow certain mechanism with certain security properties, or document the options, and leave the responsibility on the user to decide if their chosen pin method is secure enough.

one other thing that i think would be useful would be an option to store the wrapped keys in runtime_dir() instead of cache_dir()

Yes having this as an option (maybe default?) sounds great, although I like it that my fingerprint secured pin persists across sessions; keeping both options is what I would suggest. I'll implement when I get the time.

while using the device-local backends does prevent fully offline attacks, any amount of access to the machine is sufficient to allow for unlimited unlock attempt. how possible would it be to move the entire operation into the secure environment?

This is not a problem for fingerprint based age-plugin-se and yubikey pin policies with age-plugin-yubikey, but I take your point.

I'm not an expert here, but I think implementing this would not be straightforward at all.

One possibility is to get the rbw agent to talk to the TPM directly, and encrypts the local_secret with the value of the NV counter as associated data at pin creation time. Whenever the user enters a pin, the rbw-agent increments the NV counter. If the rbw-agent sees that the NV counter exceeds the pin-creation-time value plus 3 (for example), the rbw-agent could purge the pin state.

I'm not certain the above is a good idea, but it seems to me that any solution would require talking to the tpm directly, which would require writing non-trivial code that calls the TPM2 API. There might exist a crate that would do this for us. I haven't found one but haven't looked very hard either.

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.

2 participants