Support SSH keys on desktop 2024.12 #5187

dani-garcia · 2024-11-12T18:38:18Z

Added support for the new SSH item key type in desktop 2024.12+

To test it, you need a desktop build from bitwarden/clients:main, and to enable the feature flags on server.

BlackDex

I have not tested it. But looks good. Just one remark.

src/auth.rs

src/config.rs

larena1 · 2024-11-12T21:17:57Z

Nice, so Vaultwarden is only incompatible right now with recent web versions but all other clients fully work?

BlackDex · 2024-11-12T22:49:16Z

Nice, so Vaultwarden is only incompatible right now with recent web versions but all other clients fully work?

Once merged and released yes.

quexten · 2024-11-12T23:36:24Z

Note: this might cause problems for key rotation. If the web client does not support parsing and serializing key ciphers, then key rotation might either fail or even end up with undecryptable items (if skipped), so rotation should be blocked.

I'm not sure about what vaultwarden does for validation here, but upstream has rotation validators, that block rotation entirely for cases like this: https://github.com/bitwarden/server/blob/dfbc400520f4c474b28e1279a86b2fae1da052a9/src/Api/Vault/Validators/CipherRotationValidator.cs#L10

BlackDex · 2024-11-12T23:52:47Z

Note: this might cause problems for key rotation. If the web client does not support parsing and serializing key ciphers, then key rotation might either fail or even end up with undecryptable items (if skipped), so rotation should be blocked.

I'm not sure about what vaultwarden does for validation here, but upstream has rotation validators, that block rotation entirely for cases like this: https://github.com/bitwarden/server/blob/dfbc400520f4c474b28e1279a86b2fae1da052a9/src/Api/Vault/Validators/CipherRotationValidator.cs#L10

It probably will and i think it will cause issues.
We currently have some checks, but not the same specific check Bitwarden uses. Which is not that hard to add and would make it more safer.

BlackDex · 2024-11-13T11:08:12Z

We could check if there are any SSH type ciphers, if that is the case, cancel/abort the rekey process directly.
But the cipher count would the probably help there too, since if we do not return the SSH ciphers, it will be aborted too.
But a better descriptive message in those cases would be better.

BlackDex

As mentioned in the comments already. Before we add this feature, we should add the same kind of checks for the key-rotation to prevent missing keys in the rotation, and make them unavailable anymore, and thus make all SSH Key's unusable.

It might be nice to add a more descriptive warning in case someone is using a newer client then 2024.6.2 to run the key-rotation if there are SSH Ciphers. But a more general error would also be a good start for verifying all ciphers are inculded.

And i think we should do the same for all other rotated items like emergency request, folders etc...

dani-garcia · 2024-11-13T19:35:01Z

Nice, good call about the key rotation validation. I've improved the validation to match what Bitwarden is doing. We now check that we have all the ciphers, folders, sends, etc before doing the rotation

BlackDex

While i was not able to get a working Desktop v2024.12.0 version this looks to work great.

I tested it with a v2024.11.1 version which was build via Actions. But it was unable to either generate a key or import one from the clipboard.

Besides that, it did saved the ssh-cipher as an item and key-rotation failed with that item in the database as it should to prevent corruption. Without that cipher key-rotation worked fine.

quexten · 2024-11-15T17:44:20Z

I tested it with a v2024.11.1 version which was build via Actions. But it was unable to either generate a key or import one from the clipboard.

@BlackDex any errors? / what happened when you generated them? Also which build specifically (windows/mac/linux[what package])? If this is still a bug in upstream, I'd like to be able to reproduce it.

During development I've only tested against official server so far, but I don't think the server can make generation/import fail. The most similar bug encountered so far is: bitwarden/clients#11985

While i was not able to get a working Desktop v2024.12.0 version this looks to work great.

Needs to be a local build at the moment, you can change the version in the package.json file for desktop and it should work.

dani-garcia · 2024-11-15T18:04:42Z

Yeah, I've tested the client against this implementation and at least generation and saving worked fine. I didn't try to use them, but that is indendendent of the server.

If you're having problems building the desktop clients with the updated version, you can also comment out or reduce the version check on the server:

vaultwarden/src/api/core/ciphers.rs

Line 121 in 2393c3f

let ver_match = semver::VersionReq::parse(">=2024.12.0").unwrap();

And then you can use any recent builds from here:
https://github.com/bitwarden/clients/actions/workflows/build-desktop.yml?query=branch%3Amain

BlackDex · 2024-11-15T18:09:26Z

@quexten. I used the AppImage build for Linux.
I'm not behind my computer right now, but it generated a lot of errors in the log/stdout/stderr.

It did this both via the generate and import.

@dani-garcia, i downgraden the version check to v2024.11.0 or greater, so that worked at least.

Not certain which specific build i used, but it was one from that list, just a bit older then the latest there right now.

I also enabled the ssh-agent checkbox in the settings, restarted, but it didn't helped.

I'm using Arch Linux with Gnome 46.x.
I did not looked any further yet though because of time.

larena1 · 2024-11-15T21:28:25Z

@quexten I'm just getting key invalid on Windows. Can you import this key? It's auto generating a key fine though

BlackDex · 2024-11-16T14:34:09Z

@quexten the problem seems to be with password protected keys.
If i generate a non-password protected key, it works.

Also, using a build from here: https://github.com/bitwarden/clients/actions/runs/11860813999 seems to have solved the generate issue i had before.

I see the following log output (The reading 'split' is repeating a lot of times):

15:23:09.462 › Unhandled error in angular Error: Cannot read properties of null (reading 'split')
15:23:09.462 › Unhandled error in angular Error: Cannot read properties of null (reading 'split')
15:23:09.580 › Unhandled error in angular Error: Cannot read properties of null (reading 'split')
15:23:09.583 › Unhandled error in angular Error: Cannot read properties of null (reading 'split')
    at IpcRenderer.<anonymous> (<anonymous>:902:21)
    at IpcRenderer.emit (node:electron/js2c/sandbox_bundle:2:34768)
    at Object.onMessage (node:electron/js2c/sandbox_bundle:2:50819)
15:23:09.584 › Unhandled error in angular Error: Cannot read properties of null (reading 'split')
    at IpcRenderer.<anonymous> (<anonymous>:902:21)
    at IpcRenderer.emit (node:electron/js2c/sandbox_bundle:2:34768)
    at Object.onMessage (node:electron/js2c/sandbox_bundle:2:50819)
15:23:09.584 › Unhandled error in angular Error: Cannot read properties of null (reading 'split')
    at IpcRenderer.<anonymous> (<anonymous>:902:21)
    at IpcRenderer.emit (node:electron/js2c/sandbox_bundle:2:34768)
    at Object.onMessage (node:electron/js2c/sandbox_bundle:2:50819)
15:23:10.462 › Unhandled error in angular Error: Cannot read properties of null (reading 'split')
15:23:10.463 › Unhandled error in angular Error: Cannot read properties of null (reading 'split')
15:23:10.463 › Unhandled error in angular Error: Cannot read properties of null (reading 'split')
15:23:10.463 › Unhandled error in angular Error: Cannot read properties of null (reading 'split')
15:23:10.932 › Unhandled error in angular Error: Cannot read properties of null (reading 'split')

I tried the following (freshly) generated keys:
No Password:

With Password (test):

larena1 · 2024-11-16T14:41:56Z

The key I posted is an unencrypted RSA key. I also tried an ed22519 key both encrypted and unencrypted and always getting key invalid.

Importing a key that Bitwarden itself generated works however so there might be a bug in rust ssh-key as the keys are perfectly valid.

@BlackDex feel free to use this version https://github.com/larena1/bw-clients/actions/runs/11863465157

BlackDex · 2024-11-16T14:48:25Z

This has nothing to do with Rust. Those keys were generated with OpenSSH tools ssh-keygen which generates ed25519 by default. The password-less one worked just fine to try and import, but the one with a password failed.

And since the importing happens in the Desktop client it doesn't even save that, so the server isn't even called, and Vaultwarden just stores this as a blob of data, no parsing is done.

Your version has the exact same problem, but that is to be expected since it only seems to change the version number, which wasn't an issue for me since i changed the version check in Vaultwarden it self.

larena1 · 2024-11-16T14:51:14Z

@BlackDex I was only referring to my problem and not yours. Sorry for the confusion. I did not yet try to save a key as I'm stuck with not being able to import any of mine currently

BlackDex · 2024-11-16T14:54:10Z

My comments still apply. It has nothing to do with Rust and keys generated using official OpenSSH tools also fail.
So, it probably is a bug in the Desktop which doesn't understand password protected keys, or somehow doesn't show a popup where to provide a password to verify and extract the public-key from it.

I also find it a bit strange the Desktop client doesn't provide an option to generate a password protected ssh-key btw.

quexten · 2024-11-16T14:55:28Z

I'll debug these next week, thank you for the keys.

clients does not implement a password prompt UI yet, but the rust part supports bcrypt-pbkdf + aes256-ctr encrypted keys, so there should be an error message specifically stating that password protected keys are not supported yet. If they use another way of encrypting, then that is not yet supported in the ssh-key crate.

However, the unencrypted RSA key from @larena1 should work, but it also does not for me. @larena1 what tool were the keys generated with? (The ones used in the unit tests of the importer are generated with ssh-keygen).

larena1 · 2024-11-16T14:58:41Z

https://github.com/bitwarden/clients/blob/main/apps/desktop/desktop_native/core/src/ssh_agent/importer.rs#L236

There is code to handle encrypted openssh keys but maybe that's not fully working or not properly showing a password prompt yet.

Keys were generated using puttygen on Windows.

BlackDex · 2024-11-16T15:10:35Z

Ah, the rust part in the client. I didn't looked there :). That is also a bit confusing but clear now.

Support SSH keys on desktop 2024.12

b952c49

dani-garcia requested a review from BlackDex November 12, 2024 18:45

BlackDex requested changes Nov 12, 2024

View reviewed changes

src/auth.rs Show resolved Hide resolved

pquantin reviewed Nov 12, 2024

View reviewed changes

src/config.rs Show resolved Hide resolved

Document flags in .env.template

f0e3dcd

BlackDex requested changes Nov 13, 2024

View reviewed changes

Validate key rotation contents

6765b21

dani-garcia requested a review from BlackDex November 13, 2024 19:42

dani-garcia added 2 commits November 14, 2024 22:16

Merge branch 'main' into ssh_keys

f819e6c

Merge branch 'main' into ssh_keys

548db62

BlackDex approved these changes Nov 15, 2024

View reviewed changes

dani-garcia merged commit 2393c3f into main Nov 15, 2024
8 checks passed

dani-garcia deleted the ssh_keys branch November 15, 2024 17:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support SSH keys on desktop 2024.12 #5187

Support SSH keys on desktop 2024.12 #5187

dani-garcia commented Nov 12, 2024 •

edited

Loading

BlackDex left a comment

larena1 commented Nov 12, 2024

BlackDex commented Nov 12, 2024

quexten commented Nov 12, 2024

BlackDex commented Nov 12, 2024

BlackDex commented Nov 13, 2024

BlackDex left a comment

dani-garcia commented Nov 13, 2024

BlackDex left a comment •

edited

Loading

quexten commented Nov 15, 2024 •

edited

Loading

dani-garcia commented Nov 15, 2024

BlackDex commented Nov 15, 2024

larena1 commented Nov 15, 2024 •

edited

Loading

BlackDex commented Nov 16, 2024

larena1 commented Nov 16, 2024

BlackDex commented Nov 16, 2024 •

edited

Loading

larena1 commented Nov 16, 2024

BlackDex commented Nov 16, 2024

quexten commented Nov 16, 2024

larena1 commented Nov 16, 2024

BlackDex commented Nov 16, 2024

Support SSH keys on desktop 2024.12 #5187

Support SSH keys on desktop 2024.12 #5187

Conversation

dani-garcia commented Nov 12, 2024 • edited Loading

BlackDex left a comment

Choose a reason for hiding this comment

larena1 commented Nov 12, 2024

BlackDex commented Nov 12, 2024

quexten commented Nov 12, 2024

BlackDex commented Nov 12, 2024

BlackDex commented Nov 13, 2024

BlackDex left a comment

Choose a reason for hiding this comment

dani-garcia commented Nov 13, 2024

BlackDex left a comment • edited Loading

Choose a reason for hiding this comment

quexten commented Nov 15, 2024 • edited Loading

dani-garcia commented Nov 15, 2024

BlackDex commented Nov 15, 2024

larena1 commented Nov 15, 2024 • edited Loading

BlackDex commented Nov 16, 2024

larena1 commented Nov 16, 2024

BlackDex commented Nov 16, 2024 • edited Loading

larena1 commented Nov 16, 2024

BlackDex commented Nov 16, 2024

quexten commented Nov 16, 2024

larena1 commented Nov 16, 2024

BlackDex commented Nov 16, 2024

dani-garcia commented Nov 12, 2024 •

edited

Loading

BlackDex left a comment •

edited

Loading

quexten commented Nov 15, 2024 •

edited

Loading

larena1 commented Nov 15, 2024 •

edited

Loading

BlackDex commented Nov 16, 2024 •

edited

Loading