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

New Features #32

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

New Features #32

wants to merge 20 commits into from

Conversation

hasechris
Copy link

Hi @joaojacome,

up to now i used KeepassXC, which got synced via nextcloud to all my devices. Now i moved to a "real" selfhosted passwordsafe solution (vaultwarden) and wanted to have the same flexibility as with my KeepassXC setup regarding the ssh key management.

Searching the internet i found your github repo in the bitwarden forum. I tested it and was initially happy, but i missed a bit of the convenience of the KeepassXC sshkey handling. So i added the features i was missing.

New features from me:

  • specify entryname instead of iterating over all entries from folder
  • remove an entry from ssh-agent
  • allow usage of password field as key password
  • assume flag which tells the script to use the entryname as the keyname (name of attachment in bitwarden), so no customfield is needed.

Also i merged the solution from #29 from @Weidows before all my development into my new fork, because i needed to specify the session token as the env variable was not always working in my case. Hope this is not a problem regarding merge conflicts.

Additionally i want to extend usage documentation regarding how i use shell aliases so the user is only specifying the parameters he absolutly has to. I could not find the right spot for that part, so it isn't part of this PR yet.

IMPORTANT: I would be relieved if someone could check my python programming. Im literally new to python in general (this is my second python script on which im working in my entire life).

Best regards
hasechris

@joaojacome
Copy link
Owner

Hi @hasechris,

Thanks for the contribution.

I'll take a look at your changes and I'll get back to you.

import subprocess
from typing import Any, Callable, Dict, List, Optional
from cryptography.hazmat.primitives.serialization import load_ssh_private_key
Copy link
Owner

Choose a reason for hiding this comment

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

Currently, we don't have any external dependencies (besides bitwarden-cli and ssh-agent, of course). Adding an external library as dependency would make this tool harder to install.

We might need to rethink if that's the direction we'd like to go, but for now I'd say we better avoid requiring extra libs.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @joaojacome,

Basically I agree with you, a new library is of course always a problem.
But I still decided to go this way, because at the moment the user has to manually remove the just added keys from the ssh-agent. From the user's point of view, I find this very inconvenient, so I decided to add the "Key Removal" function. Therefore the library is "required".

Additionally, due to your concerns, I tried to read up a bit about this library. Basically this library seems to be relatively well known, has thousands of downloads on pypi.org and a clean release management. I also checked the default package lists of Debian and Manjaro (desktop OS of my choice). In both operating systems this package (python-cryptography) is included in the default lists. So the coverage should be relatively wide.

Maybe you could also start a poll among users of the tool?

But it's your choice, of course. Feel free to reject the PR as is.

Greetings
hasechris :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be a compromise to leverage the ssh-agent timeout, to have simple way to restrict the maximum time a key is active?

We could simply pass a -t argument to the subprocess call. That's straight forward, needs no additional libraries, but would prevent keys from staying in memory indefinitely.

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.

4 participants