Support SSH keys on desktop 2024.12

dani-garcia commented 1 week ago

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.

larena1 commented 1 week ago

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

BlackDex commented 1 week ago

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

Once merged and released yes.

quexten commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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.

dani-garcia commented 1 week ago

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

quexten commented 6 days ago

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: https://github.com/bitwarden/clients/pull/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 commented 6 days ago

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: https://github.com/dani-garcia/vaultwarden/blob/2393c3f3c08f451a04643fd9fed5027f491dc12d/src/api/core/ciphers.rs#L121

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

BlackDex commented 6 days ago

@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 commented 6 days ago

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

BlackDex commented 6 days ago

@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 commented 6 days ago

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 commented 6 days ago

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 commented 6 days ago

@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 commented 6 days ago

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 commented 6 days ago

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 commented 6 days ago

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 commented 6 days ago

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

quexten commented 4 days ago

@larena1 Putty encodes openssh-formatted private keys that are encoded differently to openssh. I don't know whether this is a putty bug or an ssh-keys bug, but tracking here: https://github.com/RustCrypto/SSH/issues/316

larena1 commented 1 day ago

@quexten I decrypted the key using ssh-keygen and was able to import it then.

Can you also add pageant support for PuTTY on Windows additionally to OpenSSH agent? PuTTY unfortunately only supports the former.

quexten commented 1 day ago

Yeah, there is an internal ticket for pageant support. Unfortunately pageant is seemingly pretty nasty to implement in rust: https://github.com/Eugeny/russh/blob/main/pageant/src/pageant_impl.rs . Not sure what's required exactly there yet.

larena1 commented 1 day ago

https://github.com/ndbeals/winssh-pageant

There is also an implementation of the receiving side in Go.

Sounds like it might take a bit longer then until the feature is available?

dani-garcia / vaultwarden

Support SSH keys on desktop 2024.12 #5187