Passkeys improvements #10318

varjolintu · 2024-02-19T16:29:48Z

Refactors the Passkey implementation to include more checks and a structure that is more aligned with the official specification.
Notable changes:

BrowserService no longer does the checks by itself. A new class BrowserPasskeysClient constructs the relevant objects, acting as a client. BrowserService only acts as a bridge between the client and BrowserPasskeys (authenticator) and calls the relevant popups for user interaction.
A new helper class PasskeyUtils includes the actual checks and parses the objects.
BrowserPasskeys is pretty much intact, but some functions have been moved to PasskeyUtils.
Fixes Ed25519 encoding in BrowserCBOR.
Adds new error messages.
User confirmation for Passkey retrieval is also asked even if discouraged is used. This goes against the specification, but currently there's no other way to verify the user.
cross-platform is also accepted for compatibility. This could be removed if there's a potential issue with it.
Extension data is now handled correctly during Authentication.
Allowed and excluded credentials are now handled correctly.
KPEX_PASSKEY_GENERATED_USER_ID is renamed to KPEX_PASSKEY_CREDENTIAL_ID
Adds a new option "Allow localhost with Passkeys" to Browser Integration -> Advanced tab. By default it's not allowed to access HTTP sites, but http://localhost can be allowed for debugging and testing purposes for local servers.
Add tag passkey to a Passkey entry, or an entry with an imported Passkey.

Corresponding KeePassXC-Browser side PR: keepassxreboot/keepassxc-browser#2121

Fixes #10287.

Testing strategy

Automatic tests added. Sites tested manually. I couldn't get the Microsoft Passkeys creation work anymore, even with the old implementation. The site always triggers OS/Browser level dialog that cannot be prevented.

Type of change

✅ Refactor (significant modification to existing code)

src/core/UrlTools.cpp

Ortham

Sorry about the flood of comments, but on a positive note I found the changed code much easier to follow than before, and thanks for addressing the issues I'd raised. 👍

src/browser/BrowserAction.cpp

src/browser/PasskeyUtils.cpp

src/browser/BrowserService.cpp

src/browser/BrowserPasskeys.cpp

src/browser/BrowserAction.cpp

src/browser/PasskeyUtils.cpp

src/browser/BrowserCbor.cpp

src/browser/BrowserPasskeys.cpp

src/browser/BrowserPasskeysClient.cpp

src/browser/PasskeyUtils.cpp

src/browser/BrowserService.cpp

src/browser/PasskeyUtils.cpp

Ortham · 2024-03-03T19:49:37Z

src/browser/BrowserPasskeys.cpp

 {
-    const auto pubKeyCredParams = publicKey["pubKeyCredParams"].toArray();
+    const auto pubKeyCredParams = credentialCreationOptions["credTypesAndPubKeyAlgs"].toArray();
    if (!pubKeyCredParams.isEmpty()) {
        const auto alg = pubKeyCredParams.first()["alg"].toInt();


Copied from an earlier comment:

This function doesn't correctly handle the case where the first element of credTypesAndPubKeyAlgs is an unsupported algorithm but a later algorithm is supported. E.g. an RP may prefer PS256 but accept RS256 as its second choice, and in that case KeePassXC should choose RS256 instead of using ES256.

I think this still applies.

I think you mean we should iterate through the array until we find a supported algorithm, instead of just picking the first element and bailing out if that doesn't work. I agree.

Just to note, the array comes from here: https://github.com/keepassxreboot/keepassxc/pull/10318/files#diff-02fad338f30f066992b2231a0cf4f5a91592313a32a22defedc5574b4a0d6a55R141
So the first element is already from the supported list.

Refactors the Passkey implementation to include more checks and a structure that is more aligned with the official specification. Notable changes: - _BrowserService_ no longer does the checks by itself. A new class _BrowserPasskeysClient_ constructs the relevant objects, acting as a client. _BrowserService_ only acts as a bridge between the client and _BrowserPasskeys_ (authenticator) and calls the relevant popups for user interaction. - A new helper class _PasskeyUtils_ includes the actual checks and parses the objects. - _BrowserPasskeys_ is pretty much intact, but some functions have been moved to PasskeyUtils. - Fixes Ed25519 encoding in _BrowserCBOR_. - Adds new error messages. - User confirmation for Passkey retrieval is also asked even if `discouraged` is used. This goes against the specification, but currently there's no other way to verify the user. - `cross-platform` is also accepted for compatibility. This could be removed if there's a potential issue with it. - Extension data is now handled correctly during Authentication. - Allowed and excluded credentials are now handled correctly. - `KPEX_PASSKEY_GENERATED_USER_ID` is renamed to `KPEX_PASSKEY_CREDENTIAL_ID` - Adds a new option "Allow localhost with Passkeys" to Browser Integration -> Advanced tab. By default it's not allowed to access HTTP sites, but `http://localhost` can be allowed for debugging and testing purposes for local servers. - Add tag `Passkey` to a Passkey entry, or an entry with an imported Passkey. Fixes keepassxreboot#10287.

Refactors the Passkey implementation to include more checks and a structure that is more aligned with the official specification. Notable changes: - _BrowserService_ no longer does the checks by itself. A new class _BrowserPasskeysClient_ constructs the relevant objects, acting as a client. _BrowserService_ only acts as a bridge between the client and _BrowserPasskeys_ (authenticator) and calls the relevant popups for user interaction. - A new helper class _PasskeyUtils_ includes the actual checks and parses the objects. - _BrowserPasskeys_ is pretty much intact, but some functions have been moved to PasskeyUtils. - Fixes Ed25519 encoding in _BrowserCBOR_. - Adds new error messages. - User confirmation for Passkey retrieval is also asked even if `discouraged` is used. This goes against the specification, but currently there's no other way to verify the user. - `cross-platform` is also accepted for compatibility. This could be removed if there's a potential issue with it. - Extension data is now handled correctly during Authentication. - Allowed and excluded credentials are now handled correctly. - `KPEX_PASSKEY_GENERATED_USER_ID` is renamed to `KPEX_PASSKEY_CREDENTIAL_ID` - Adds a new option "Allow localhost with Passkeys" to Browser Integration -> Advanced tab. By default it's not allowed to access HTTP sites, but `http://localhost` can be allowed for debugging and testing purposes for local servers. - Add tag `Passkey` to a Passkey entry, or an entry with an imported Passkey. Fixes #10287.

Release 2.7.7 - Support USB Hotplug for Hardware Key interface [keepassxreboot#10092] - Support 1PUX and Bitwarden import [keepassxreboot#9815] - Browser: Add support for PassKeys [keepassxreboot#8825, keepassxreboot#9987, keepassxreboot#10318] - Build System: Move to vcpkg manifest mode [keepassxreboot#10088] - Fix multiple TOTP issues [keepassxreboot#9874] - Fix focus loss on save when the editor is not visible anymore [keepassxreboot#10075] - Fix visual when removing entry from history [keepassxreboot#9947] - Fix first entry is not selected when a search is performed [keepassxreboot#9868] - Prevent scrollbars on entry drag/drop [keepassxreboot#9747] - Prevent duplicate characters in "Also choose from" field of password generator [keepassxreboot#9803] - Security: Prevent byte-by-byte and attachment inference side channel attacks [keepassxreboot#10266] - Browser: Fix raising Update Entry messagebox [keepassxreboot#9853] - Browser: Fix bugs when returning credentials [keepassxreboot#9136] - Browser: Fix crash on database open from browser [keepassxreboot#9939] - Browser: Fix support for referenced URL fields [keepassxreboot#8788] - MacOS: Fix crash when changing highlight/accent color [keepassxreboot#10348] - MacOS: Fix TouchID appearing even though lid is closed [keepassxreboot#10092] - Windows: Fix terminating KeePassXC processes with MSI installer [keepassxreboot#9822] - FdoSecrets: Fix database merge crash when enabled [keepassxreboot#10136] # -----BEGIN PGP SIGNATURE----- # # iQEzBAABCAAdFiEENIkEDB8MPuq41ValRA/GXy4MbgEFAmXs7VsACgkQRA/GXy4M # bgHLpwf/brnyPPs3gJxZmD2pn8542D4CCsDh0fTceurOtqCe3J4Y+Fftc5euuoQu # 6rP4vJdd586l7JX5FnYIPXvGiU9op3MudJh+y+RN/PWwKcXNIXfUItMhpZEka49n # xnw+Wvbilg1QIHSSmZdIjBpohnEkA67qhWauc3bCacrRyEvIOzVMTxnqDTe4GUDy # CyauaRMMKezRTpLxSsk63TDAZZgDwK4ci5lC6ysHekc1Za6IbI3fMFjz1BGj+kPU # tMHMfDCWqK/5JZ27ZWcxy7m8tJY9m3rb+MoCyFRQz9ixaEe29yf5NqYdm9sn1Dlh # O7aFi7/EJtsBlXdguw5BcTPbsL7XEQ== # =Cots # -----END PGP SIGNATURE----- # gpg: directory '/home/runner/.gnupg' created # gpg: keybox '/home/runner/.gnupg/pubring.kbx' created # gpg: Signature made Sat Mar 9 23:14:35 2024 UTC # gpg: using RSA key 3489040C1F0C3EEAB8D556A5440FC65F2E0C6E01 # gpg: Can't check signature: No public key

varjolintu added pr: refactoring Pull request that refactors code feature: Passkeys labels Feb 19, 2024

varjolintu added this to the v2.7.7 milestone Feb 19, 2024

varjolintu requested review from phoerious and droidmonkey February 19, 2024 16:29

varjolintu mentioned this pull request Feb 19, 2024

Passkey improvements keepassxreboot/keepassxc-browser#2121

Merged

varjolintu force-pushed the fix/passkeys_improvements branch 2 times, most recently from 54de4da to 089f7f8 Compare February 19, 2024 16:37

phoerious reviewed Feb 21, 2024

View reviewed changes

src/core/UrlTools.cpp Outdated Show resolved Hide resolved

varjolintu mentioned this pull request Feb 22, 2024

Security, privacy and other issues with WebAuthn/passkeys support #10287

Closed

droidmonkey mentioned this pull request Feb 22, 2024

Passkeys/Webauthn support broken on Mac or if another authenticator device is present (?) keepassxreboot/keepassxc-browser#2124

Closed

Ortham suggested changes Feb 26, 2024

View reviewed changes

Ortham reviewed Feb 28, 2024

View reviewed changes

src/browser/BrowserAction.cpp Outdated Show resolved Hide resolved

src/browser/BrowserAction.cpp Outdated Show resolved Hide resolved

src/browser/PasskeyUtils.cpp Outdated Show resolved Hide resolved

Passkeys improvements

b670e4c

varjolintu force-pushed the fix/passkeys_improvements branch from 90154e1 to b670e4c Compare March 3, 2024 09:07

droidmonkey approved these changes Mar 3, 2024

View reviewed changes

Small fixes

7a90f30

droidmonkey requested changes Mar 3, 2024

View reviewed changes

src/browser/BrowserService.cpp Outdated Show resolved Hide resolved

Ortham reviewed Mar 3, 2024

View reviewed changes

src/browser/PasskeyUtils.cpp Outdated Show resolved Hide resolved

Few more fixes

88229b8

droidmonkey approved these changes Mar 3, 2024

View reviewed changes

Ortham reviewed Mar 3, 2024

View reviewed changes

varjolintu added 2 commits March 4, 2024 18:04

Add tag to entry when creating or importing a Passkey

7db9f6c

Translate Passkey

cf87a1c

droidmonkey merged commit ac2b445 into keepassxreboot:develop Mar 6, 2024
11 checks passed

varjolintu deleted the fix/passkeys_improvements branch March 6, 2024 13:22

droidmonkey added the pr: backported Pull request backported to previous release label Mar 8, 2024

droidmonkey mentioned this pull request Mar 9, 2024

Final backport PR for 2.7.7 #10364

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Passkeys improvements #10318

Passkeys improvements #10318

varjolintu commented Feb 19, 2024 •

edited

Loading

Ortham left a comment

Ortham Mar 3, 2024

droidmonkey Mar 3, 2024 •

edited

Loading

varjolintu Mar 4, 2024 •

edited

Loading

Passkeys improvements #10318

Passkeys improvements #10318

Conversation

varjolintu commented Feb 19, 2024 • edited Loading

Testing strategy

Type of change

Ortham left a comment

Choose a reason for hiding this comment

Ortham Mar 3, 2024

Choose a reason for hiding this comment

droidmonkey Mar 3, 2024 • edited Loading

Choose a reason for hiding this comment

varjolintu Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

varjolintu commented Feb 19, 2024 •

edited

Loading

droidmonkey Mar 3, 2024 •

edited

Loading

varjolintu Mar 4, 2024 •

edited

Loading