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

[Feature Request] Keyring-rs to store secrets #2

Closed
veeso opened this issue Jan 16, 2021 · 7 comments
Closed

[Feature Request] Keyring-rs to store secrets #2

veeso opened this issue Jan 16, 2021 · 7 comments
Assignees
Labels
new feature New feature or request
Milestone

Comments

@veeso
Copy link
Owner

veeso commented Jan 16, 2021

Premise

TermSCP uses an AES 256 byte key to encrypt secrets/passwords in bookmarks.
At the moment the AES key is stored in one of the following paths, based on the user's operating system:

  • Linux: /home/alice/.config/termscp/.ssh/
  • MacOS: /Users/Alice/Library/Application Support/termscp/.ssh/
  • Windows: C:\Users\Alice\AppData\Roaming\termscp\.ssh\

This method is quite safe, since anyway, keys are located in a path which is accessible only by the current user.

The Keyring solution

The optimal solution to store the secret would probably be to use keyring-rs which allows the user to store the termscp secret in the operating system vault.
The solution is actually quite simple, since just in a few lines of code, this crates provides a way to do so:

extern crate keyring;
extern crate whoami; // whoami is already in termscp, so only keyring must be added

use keyring::Keyring;

let service: String = String::from("termscp");
let secret: String = generate_key(); // Generates a 256 Bytes random key
let username: String = whoami::username();
let storage: Keyring = Keyring::new(service.as_str(), username.as_str());
// Save password into the storage
if let Err(err) = storage.set_password(secret.as_str()) {
    // Handle error...
}
// ...
// Retrieve password from the storage to decrypt
let secret: String = match storage.get_password() {
    Ok(s) => s,
    Err(e) => { /* Handle error */ },
};

Unfortunately, if on one side this makes safer to handle secrets, at the same time adds a lot of issues.

The BSD compatibility break

Keyring-rs just doesn't support BSD, which would break the compatibility with it. So it must be a feature for BSD

The D-BUS dilemma

On Linux keyring-rs uses D-BUS and requires a service which provides org.freedesktop.secrets. And there are three issues with that.

  1. Some DE doesn't provide any org.freedesktop.secrets service. As a Manjaro/KDE user, I wasn't able to find any service running which provides it, making me unable to use it. I haven't checked how many DE actually supports it, GNOME supports it for sure.
  2. the D-BUS low-level dependency: unfortunately, building keyring-rs on Linux, requires to have the libdbus installed, which is totally MEH. There is no problem if you use a real GNU/Linux operating systems, but what about for example WSL? My WSL doesn't have libdbus installed, since it is not supported, and I haven't any intention to install it. In addition personally, I'm not a huge fan of forcing users to install C libraries to make an application to work. Personally when I see a crate which requires a particular C library to be installed, I give up installing it, not because it's complicated, but just because it's annoying.
  3. The application name is bounded torust-keyring. I'm not 100% sure this means the only service you can use is actually rust-keyring, but if it so, for me it's totally a big no no.

Implementation

After the analysis of the issues introduced by the crate, I haven't given up in implementing this feature, indeed I've actually decided I will implement it in 0.3.1/0.3.2.

The implementation consists in adding a new trait Keystorage which provides these methods:

  • get_secret() -> &str
  • set_secret(secret: &str) -> Result
  • supported() -> bool (static)

and I will implement two providers: KeyringStorage which uses keyring-rs and FileStorage which uses the current implementation.

The keyring-rs crate will be available on Windows and MacOS only, while on Linux, at least until libdbus will be required, I won't be supported. I know it's a pity, considering that probably a big slice of the audience of termscp are Linux/WSL users, but this would introduce many compatibility issues and would remove the coverage for many users (wsl users in particular).

@veeso veeso added the new feature New feature or request label Jan 16, 2021
@veeso veeso added this to the 0.3.1 milestone Jan 16, 2021
@veeso veeso self-assigned this Jan 16, 2021
@veeso veeso mentioned this issue Jan 16, 2021
11 tasks
@veeso veeso closed this as completed Jan 16, 2021
@veeso veeso pinned this issue Feb 4, 2021
@djmattyg007
Copy link

the D-BUS low-level dependency: unfortunately, building keyring-rs on Linux, requires to have the libdbus installed, which is totally MEH. There is no problem if you use a real GNU/Linux operating systems, but what about for example WSL? My WSL doesn't have libdbus installed, since it is not supported, and I haven't any intention to install it. In addition personally, I'm not a huge fan of forcing users to install C libraries to make an application to work. Personally when I see a crate which requires a particular C library to be installed, I give up installing it, not because it's complicated, but just because it's annoying.

Why couldn't it be an optional dependency configured at build-time with a feature flag?

@veeso
Copy link
Owner Author

veeso commented Apr 7, 2021

I've already though about this possibility, but I've decided not to opt for it.
The reason is quite simple: most of the termscp users don't install it through cargo install, but through packages (deb, rpm...).
I don't know the exact percentage, but I guess that people which installs termscp via cargo is below 25%, which makes that in the most optimistic case the 15% of my users installs termscp via cargo and are linux users.

How many of them will enable the keyring feature in the build? Considering that not all the linux distros support the service used by the d-bus, it's really not worth it to implement the keyring for linux.
In addition we must consider there's no security vulnerability atm in practice. The key is stored in a file which is accessible only by the current user, as happens for example for the ssh keys and nobody has ever complained for it. In addition to this, you're completely free to choose whether to store or not the passwords and I've adivesed the community not to store passwords for production servers (and I really hope that they opt to use rsa-key-based authentication for them).

@djmattyg007
Copy link

Considering that not all the linux distros support the service used by the d-bus, it's really not worth it to implement the keyring for linux.

This is a bit unfair. All of the major distributions (Debian, Ubuntu, Fedora, CentOS, RHEL, Arch, etc) use systemd, and systemd has a hard dependency on dbus. Gentoo users are used to compiling things themselves, so adding/removing compile-time flags is no big deal there. That's the majority of users covered, and I know for Arch at least, maintainers absolutely would enable the keyring feature at build-time.

Dismissing the addition of an optional feature because there exists at least one distribution that wouldn't use that feature is a disappointing attitude.

@veeso
Copy link
Owner Author

veeso commented Apr 8, 2021

This is a bit unfair. All of the major distributions (Debian, Ubuntu, Fedora, CentOS, RHEL, Arch, etc) use systemd, and systemd has a hard dependency on dbus. Gentoo users are used to compiling things themselves, so adding/removing compile-time flags is no big deal there. That's the majority of users covered, and I know for Arch at least, maintainers absolutely would enable the keyring feature at build-time.

The problem is not with D-Bus, but is that a service which provides org.freedesktop.secrets on D-Bus must be installed on your system, and as far as I know only GNOME has a service which provides it, making all the other DEs users unable to use this feature.

I will repeat myself: this feature is completely useless afterall. It doesn't add anything to the user experience, it's just an additional security layer, I've implemented it on MacOs and Windows because it was "free".
In addition I just want to keep this program mostly free from C-bindings, which is something I hate and without any optional feature. I'm not against features at all, but this is a CLI program and for what it does, is already stressful enough to maintain without optional features.

Dismissing the addition of an optional feature because there exists at least one distribution that wouldn't use that feature is a disappointing attitude.

I just have my point of view, and before defining someone's attitude as disappointing, remember that me, as any other open source developer, owe nothing to you and to the community.

@djmattyg007
Copy link

For what it's worth, KeepassXC also provides org.freedesktop.secrets:

https://archlinux.org/todo/gnome-keyring-dependency-replacement-with-orgfreedesktopsecrets/

@veeso
Copy link
Owner Author

veeso commented May 2, 2021

I'll make some tests with it when I have some spare time using keyring-rs to see if it works fine.

Once I'll be come back with a definitive analysis in implementing keyring storage for linux I will eventually re-open this issue to link a PR for termscp 0.6.0 (I guess, there's still a lot to do for 0.5.0).

Thanks

@veeso veeso reopened this Jun 10, 2021
@veeso veeso modified the milestones: 0.3.1, 0.6.0 Jun 10, 2021
veeso added a commit that referenced this issue Jun 13, 2021
# This is the 1st commit message:

Use containers to test file transfers

# This is the commit message #2:

Container setup

# This is the commit message #3:

Container setup

# This is the commit message #4:

tests with docker-compose

# This is the commit message #5:

these tests won't work with containers

# This is the commit message #6:

ftp tests with containers; removed crap servers; tests only lib

# This is the commit message #7:

hostname for github

# This is the commit message #8:

booooooh

# This is the commit message #9:

fixed recursive remove FTP
@veeso
Copy link
Owner Author

veeso commented Jun 14, 2021

Issue closed, for updates and linux implementation, please refer to issue #48

@veeso veeso closed this as completed Jun 14, 2021
@veeso veeso changed the title [Feature Request] Keyring-rs to store secret [Feature Request] Keyring-rs to store secrets Jun 14, 2021
@veeso veeso unpinned this issue Jul 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants