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

Don't require unlock to manage passkeys #317

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Conversation

emlun
Copy link
Member

@emlun emlun commented Aug 27, 2024

This was originally necessary because we re-encrypted the key store to a session key on login and didn't keep the main key unwrapped in the session, therefore we needed to re-authenticate the user in order to unwrap the main key so it can be re-wrapped under the new PRF key.

Now we do keep the main key unwrapped in the session, so we no longer need to re-authenticate the user.

emlun added 5 commits August 27, 2024 13:31
This is already enforced by the `OpenedContainer` type of the `privateData`
parameter.
This was originally necessary because we re-encrypted the key store to a session
key on login and didn't keep the main key unwrapped in the session, therefore we
needed to re-authenticate the user in order to unwrap the main key so it can be
re-wrapped under the new PRF key.

Now we do keep the main key unwrapped in the session, so we no longer need to
re-authenticate the user.
@emlun emlun requested a review from gkatrakazas August 27, 2024 11:33
@emlun
Copy link
Member Author

emlun commented Aug 28, 2024

Note that this also removes the requirement to "unlock" before allowing to delete the account. Is that a problem?

@gkatrakazas
Copy link
Member

That's a good question. It might be a good idea to keep this extra layer of security before allowing account deletion. Instead of having both an unlock/lock feature, we could simply require authentication (like unlocking) before proceeding to open the delete account popup.

@gkatrakazas
Copy link
Member

Regarding best practices, should users be required to unlock their manage passkey section when they want to edit them?

@emlun
Copy link
Member Author

emlun commented Aug 28, 2024

It might be a good idea to keep this extra layer of security before allowing account deletion. Instead of having both an unlock/lock feature, we could simply require authentication (like unlocking) before proceeding to open the delete account popup.

Ok, I'll update the PR with something for that.

Regarding best practices, should users be required to unlock their manage passkey section when they want to edit them?

I've never seen this on any other site, it was just due to a technical limitation in the v1 and v2 encryption architecture that is no longer present in the v4 architecture. We need the mainKey in order to re-encrypt the wallet to the new passkey PRF key, and we didn't have mainKey cached in the session so we needed to re-authenticate in order to re-derive the mainKey. The v4 architecture eliminated the sessionKey and simply caches the mainKey instead (because after v3, the mainKey can be easily replaced), so now there's no longer any need to re-authenticate.

The closest thing I've seen is the "sudo mode" step-up authentication done before entering account settings on sites like GitHub, but that's more to do with its sessions being much longer-lived than anything particular about passkeys.

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