Skip to content

Conversation

Firstyear
Copy link
Member

Fixes #510

  • cargo test has been run and passes
  • documentation has been updated with relevant examples (if relevant)

@BlackDex
Copy link
Contributor

@Firstyear Would this PR mean my Voodoo checks in the commit dani-garcia/vaultwarden@df783fc would be no longer needed?

@tessus
Copy link
Contributor

tessus commented Aug 18, 2025

I also tried, but I also ran into the same issue that it doesn't compile.

@Firstyear
Copy link
Member Author

That will teach me for submitting a PR in a hurry on a busy day :)

@tessus
Copy link
Contributor

tessus commented Aug 19, 2025

We've all been there... I certainly more than once.

@Firstyear
Copy link
Member Author

@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:

  • cred.BE = true, auth_data.BE = true (valid)
  • cred.BE = true, auth_data.BE = false (currently this FAILS)
  • cred.BE = false, auth_data.BE = true (valid, covered by allow_backup_eligible_upgrade)
  • cred.BE = false, auth_data.BE = false (valid)

So I think that's the corner case I missed. I'll update the PR to accept this.

@yaleman
Copy link
Member

yaleman commented Aug 19, 2025

Rebase and fmt while you're there 😸

@Firstyear Firstyear force-pushed the 20250816-allow-backup-eligibility-to-change branch from f743fe7 to fe6e28c Compare August 19, 2025 02:49
@Firstyear
Copy link
Member Author

Done

@Firstyear
Copy link
Member Author

@Firstyear Would this PR mean my Voodoo checks in the commit dani-garcia/vaultwarden@df783fc would be no longer needed?

Hopefully yes :)

@Firstyear
Copy link
Member Author

@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.

@BlackDex
Copy link
Contributor

Sure, I'll see if i can do this today somewhere

@BlackDex
Copy link
Contributor

I quickly tried this today, but doesn't seem to work.

[2025-08-24 16:21:51.064][request][INFO] POST /identity/connect/token
[2025-08-24 16:21:52.180][webauthn_rs_core::core][DEBUG] Credential backup eligibility has changed! false -> true
[2025-08-24 16:21:52.180][webauthn_rs_core::core][ERROR] Credential indicates it is backed up, but has not declared valid backup eligibility
[2025-08-24 16:21:52.180][error][ERROR] Webauthn.
[CAUSE] CredentialMayNotBeHardwareBound
[2025-08-24 16:21:52.180][response][INFO] (login) POST /identity/connect/token => 400 Bad Request

@Firstyear
Copy link
Member Author

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.

@BlackDex
Copy link
Contributor

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.

@Firstyear
Copy link
Member Author

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.

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.

Migration from CredentialV3 to CredentialV5 seems to be impossible when the Passkey was backed up in the past

4 participants