-
Notifications
You must be signed in to change notification settings - Fork 110
Allow backup eligibility to change #513
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
base: master
Are you sure you want to change the base?
Allow backup eligibility to change #513
Conversation
@Firstyear Would this PR mean my Voodoo checks in the commit dani-garcia/vaultwarden@df783fc would be no longer needed? |
I also tried, but I also ran into the same issue that it doesn't compile. |
That will teach me for submitting a PR in a hurry on a busy day :) |
We've all been there... I certainly more than once. |
@BlackDex Yeah, I think I need to just clean this up a bit: We have all your V3 -> V5 credentials moving to backup_eligible == true. This means during authentication you have:
So I think that's the corner case I missed. I'll update the PR to accept this. |
Rebase and fmt while you're there 😸 |
f743fe7
to
fe6e28c
Compare
Done |
Hopefully yes :) |
@BlackDex Can you confirm if this updated PR resolves your issue without the need for all your checks? If it does, I'll do a release so you can use the updated version. |
Sure, I'll see if i can do this today somewhere |
I quickly tried this today, but doesn't seem to work.
|
That sucks :( I see you have merged a separate PR so in this case, do you want me to just close this one since you have a work around? I'm so sorry this has been such a pain for you, it is a pretty big mistake on our part. |
Ow, no problem. It was fun to figure it out. And i see it more as our own fault for not having updated it more quickly. If this could help others or future implementations, then it might be useful to continue this PR. I know Proxmox had a similar issue as Vaultwarden. But there implementation is a bit different then ours. Also, it shouldn't lower or demote security of course. |
Correct, in this case it doesn't really affect your security stance. Realistically, given how the passkey ecosystem has moved, flags like backup eligibility and such dont matter, as the only way to truly guarantee properties about authenticators is to attest them. If you don't do attestation, the assumption is "anything goes" given how passkey providers operate. So I think there is even an argument that we remove this field and flag. |
Fixes #510