Skip to content
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

Support SSH keys on desktop 2024.12 #5187

Merged
merged 5 commits into from
Nov 15, 2024
Merged

Support SSH keys on desktop 2024.12 #5187

merged 5 commits into from
Nov 15, 2024

Conversation

dani-garcia
Copy link
Owner

@dani-garcia dani-garcia commented Nov 12, 2024

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.

@dani-garcia dani-garcia requested a review from BlackDex November 12, 2024 18:45
Copy link
Collaborator

@BlackDex BlackDex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/auth.rs Show resolved Hide resolved
@larena1
Copy link

larena1 commented Nov 12, 2024

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

@BlackDex
Copy link
Collaborator

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

Once merged and released yes.

@quexten
Copy link
Contributor

quexten commented Nov 12, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator

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.

Copy link
Collaborator

@BlackDex BlackDex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Owner Author

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

@dani-garcia dani-garcia requested a review from BlackDex November 13, 2024 19:42
Copy link
Collaborator

@BlackDex BlackDex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dani-garcia dani-garcia merged commit 2393c3f into main Nov 15, 2024
8 checks passed
@dani-garcia dani-garcia deleted the ssh_keys branch November 15, 2024 17:38
@quexten
Copy link
Contributor

quexten commented Nov 15, 2024

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
Copy link
Owner Author

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:

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
Copy link
Collaborator

@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
Copy link

larena1 commented Nov 15, 2024

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

-----BEGIN OPENSSH PRIVATE KEY-----
b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAABFwAAAAdz
c2gtcnNhAAAAAwEAAQAAAQEA33DvUVqVptWy7A51kjLdCFaUa+qt4halVVISuK57
NSQLI2CFiGNqYTluyV5JCeo8turI4Ne9b5KouVLuZfO0CIgBS9EUuL5Z2QHOMUlz
Wx1o2YWdzst0mr5SvdhDYnt8i1Z1lSQa4DR8ZyHzZpl1R913ytL6jpgwrP0Oaatj
FdbJ3AVVUOne9emDXONfM/iW2wbCMjrfj76gyaSKn2Va7iMwSGG/kBy6YA3xDAB1
oQWfg4gHfnicBCpN6zzT7ESMVqBdfdE+shy6bJrwcbaj+RJoUkRBwSFPKoEdtkEE
bRxT4Ls9zE/5MwMgcKj/ctRmVjSPHVLohy8aOv81KcgzeQAAA9ANBNLCDQTSwgAA
AAdzc2gtcnNhAAABAQDfcO9RWpWm1bLsDnWSMt0IVpRr6q3iFqVVUhK4rns1JAsj
YIWIY2phOW7JXkkJ6jy26sjg171vkqi5Uu5l87QIiAFL0RS4vlnZAc4xSXNbHWjZ
hZ3Oy3SavlK92ENie3yLVnWVJBrgNHxnIfNmmXVH3XfK0vqOmDCs/Q5pq2MV1snc
BVVQ6d716YNc418z+JbbBsIyOt+PvqDJpIqfZVruIzBIYb+QHLpgDfEMAHWhBZ+D
iAd+eJwEKk3rPNPsRIxWoF190T6yHLpsmvBxtqP5EmhSREHBIU8qgR22QQRtHFPg
uz3MT/kzAyBwqP9y1GZWNI8dUuiHLxo6/zUpyDN5AAAAAwEAAQAAAQBvnrg+2NS3
ojueht6e6T/X4YCFpJe2wP9Y7wYhMjCkbFwQETDD4H4NEabRe4NbK6Om8QTmpX+h
1A7rfY1Qavz94gtbt5f1bknuCWPa5Ul2M+vj9kbOPn8Cqp8k7XtEIFIoPUnB9mZi
qHWZA7HXCEQ5YV5teRXn1AlE8amYiiCWkGYSjUur9pCi+rGrEWF7y7iWdoPGUaWL
DJrUU2HuRx79ECf/Eyn1Ieyo5hzNWTpdmuGJU8x4RR9o19YKVXK1X5iJjGWpoR4K
g3snYO8C2Wkm2+PMf3NpFATaoLc6SHKn61ssOwONHThajc88zOr9mrNSzcHm5B7X
YOm0/KcLV0yxAAAAgHBzQqencR46u0AHaZ36tUo/FdL6zuDaFLiAKgEOLHPeff6/
r68wgcVvYyFton7DaB/Dfe+hkB2zqpPCPay+WL6VOSBL8BNVIHjC7xz7XPSqpvq+
MDQy+CWfbRsJm+Xjc67SqWs5ESrhnnWRB7lDhMkhPJna4m09BCc1Hu/pq1DtAAAA
gQDw5Pmdl/zKMhRoZEf/BF/FQDsoQAqQq0bWxj4RginST3lmaxqTURFLLlI7viaG
EABNOgQK6HhkA7AIWGk01Sn0N+8yBjf4TOh994KikW9nnKcPVyMxfT4F/zb91VBD
fQc3HRzEemdN5oaqaNWG5KTD+pN5JqB8oK9XFhsjDlDWhQAAAIEA7XPJPjbhJNxk
Ag1v/0FIap8YXt0ishWMnPlVxNEQLENe0MJgwfpgHMtq+5zbxo4QKHWfTy6TmgjB
p/rIMmrW9Plw3HmmLfpo/xN4Asey0rvy6GDXm85x0Zjn0eVGYnXxan4OVxnmb9t4
h46TsIXSW16JlTo7rYhe7Yix9rFEnWUAAAAQcnNhLWtleS0yMDI0MTExNQECAwQF
BgcICQoL
-----END OPENSSH PRIVATE KEY-----

@BlackDex
Copy link
Collaborator

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

-----BEGIN OPENSSH PRIVATE KEY-----
b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAMwAAAAtzc2gtZW
QyNTUxOQAAACDrfOWZfRl2wtvp8X28eS15bXyBuhVbhY0gji5NieZoBQAAAJglDHxGJQx8
RgAAAAtzc2gtZWQyNTUxOQAAACDrfOWZfRl2wtvp8X28eS15bXyBuhVbhY0gji5NieZoBQ
AAAEBFYnkD2FmgYEOfoaV9hSEOuqwEAxQXGkXEb+My1Kz/J+t85Zl9GXbC2+nxfbx5LXlt
fIG6FVuFjSCOLk2J5mgFAAAAFG1hdGhpanNAbWF0aGlqcy1wMTZzAQ==
-----END OPENSSH PRIVATE KEY-----

With Password (test):

-----BEGIN OPENSSH PRIVATE KEY-----
b3BlbnNzaC1rZXktdjEAAAAACmFlczI1Ni1jdHIAAAAGYmNyeXB0AAAAGAAAABAQ7xfBfk
Unb1XQXa+OLWhUAAAAGAAAAAEAAAAzAAAAC3NzaC1lZDI1NTE5AAAAIGFz3ULvhFDr7jyP
3N2QDASsC8zv1QL+OF0sS7RYQL2wAAAAoP9ZjIo+uZChyM6nlehpf92OCLuilYKdYmOEsF
MLKL93I3IT9KPYRaKNuw/Xykq1KK1HocoETk0bPfQEw2rs+7t/2vnNxJe8ddc4U98Egpci
jBTpi7bcrRY8Znf9yrY8qbmM7ymLEUOUgvLih/lIlbg2kVXNjXgNyLntmqHgBUaYNgsW2m
in7ci9qOfiraS+jmM0UrbqGPqUMj3Ys3nPSHE=
-----END OPENSSH PRIVATE KEY-----

@larena1
Copy link

larena1 commented Nov 16, 2024

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
Copy link
Collaborator

BlackDex commented Nov 16, 2024

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
Copy link

larena1 commented Nov 16, 2024

@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
Copy link
Collaborator

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
Copy link
Contributor

quexten commented Nov 16, 2024

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
Copy link

larena1 commented Nov 16, 2024

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
Copy link
Collaborator

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

@quexten
Copy link
Contributor

quexten commented Nov 18, 2024

@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: RustCrypto/SSH#316

@larena1
Copy link

larena1 commented Nov 21, 2024

@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
Copy link
Contributor

quexten commented Nov 21, 2024

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
Copy link

larena1 commented Nov 21, 2024

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?

@Svenum
Copy link

Svenum commented Dec 11, 2024

I have enabled the EXPERIMENTAL_FLAGS, but I dont see the ssh type. What did I wrong?

@BlackDex
Copy link
Collaborator

Where are you checking this?
It is only visible in the Desktop client as far as i know.

@Svenum
Copy link

Svenum commented Dec 11, 2024

I checked if the options appear in the admon panel and installed the newest desktop client from flathub (stable)

@BlackDex
Copy link
Collaborator

The desktop version is still v2024.11.x as far as i can see. It's not yet v2024.12.x which is needed for SSH support.

@quexten
Copy link
Contributor

quexten commented Dec 11, 2024

You need 2024.12.0 which is not released on flathub yet.

Alternatively you can use the latest build from clients main branch: https://github.com/bitwarden/clients/actions/runs/12276234794 (there also is a flatpak attached).

That said, I'm not entirely sure if ssh agent works on flatpak yet, (same goes for snap).

@DireMunchkin
Copy link

Hello! I also tried out the SSH keys feature with my Vaultwarden. However I wasn't able to get SSH agent to work.

Here's what I did:

  1. enabled the feature flags at the server
  2. downloaded desktop client 12.0 (AppImage on Linux)
  3. made or imported a couple of new SSH key entries
  4. enabled SSH-agent setting in the client settings menu
  5. started SSH agent, restarted the client

If I understood it correctly I should now be able to see keys saved in Bitwarden using ssh-add -L in a separate terminal, and use it for sessions. But the agent doesn't have anything. Did I miss a step in setting it up or is it broken?

@quexten
Copy link
Contributor

quexten commented Dec 13, 2024

Yeah, there is a bug (fixed in main but did not make it to 2024.12.0). You can probably get it to work by running touch ~/.bitwarden-ssh-agent.sock, then restarting bitwarden desktop. Otherwise stay tuned for the January release, which will fix this bug.

@DireMunchkin
Copy link

OK cool, will wait for the fix upstream then. Thanks for clarifying!

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.

7 participants