woodruffw / kbs2 Goto Github PK

This needs more thought before I or someone else goes and does it, but:

• The current master password approach is slightly ugly: it uses a not-very-well-specified POSIX API (named SHM objects) and requires additional hacks for different OSes.

• Most modern desktop environments already provide an integrated keychain, and some (like macOS) integrate that keychain tightly into hardened hardware. This makes it an ideal place to store a master password.

• There are a few Rust crates for managing system keychains/keyrings:

  • https://github.com/hwchen/keyring-rs
  • Some stuff here: https://crates.io/keywords/keychain

• We should still fall back on the current approach when we fail to detect a suitable keychain to interact with, or if the user specifically requests the current approach.

These should be documented in the README, at the very least.

Decryption error messages are currently leaked to the user with the rage backend (and probably age as well):

$ echo junk > ~/.local/share/kbs2/junk
$ kbs2 pass junk
Error: failed to fill whole buffer

[ Did rage not do what you expected? Could an error be more useful? ]
[ Tell us: https://str4d.xyz/rage/report                            ]
Fatal: EOF while parsing a value at line 1 column 0

These should be hidden and propagated more cleanly.

This section is empty in the README.

The kbs2 crate itself should also be documented.

The Backend trait should learn a rewrap_keypair API that takes a path to a wrapped key and does the following:

• Prompts the user for their current master password
• Prompts the user for a new master password
• Uses the current master password to unwrap/grab the current key from the agent
• Re-wrap the current key using the new master password
• Write the newly wrapped key to the given path

Then, the kbs2 CLI should learn kbs2 passwd/kbs2 rewrap or similar, which should first create a backup of the keyfile and then call rewrap_keypair with the keyfile.

This needs discussion before implementation, but: kbs2 should have support for some kind of password generation, enabling interactions like these:

# note: the user is not prompted for a password
$ kbs2 new login -g whatever
Username: whatever

# similarly for non-interactive record creation
$ kbs2 new login -g whatever <<< "whatever"

kbs2 currently uses one singular shared memory object for the unwrapped key: /__kbs2_unwrapped_key.

This causes problems when a user wants to use multiple different configuration directories, each with its own wrapped key.

For example, assuming that the default configuration directory's key has already been unwrapped, the following fails:

KBS2_CONFIG_DIR=/tmp/kbs2/ kbs2 dmenu-pass
Fatal: unable to decrypt (backend reports: NoMatchingKeys)

...as it tries to use the already unwrapped key on the wrong store.

To fix this, kbs2 should augment the shared memory object's name to depend on the (fully-qualified) keyfile's path. There are length limitations to consider (i.e. NAME_MAX=255), so maybe a reasonable checksum like SHA-2/256 instead of just concatenating the keyfile's canonical path.

cargo clippy should be run on every commit.

kbs2 currently has a primitive approach to error handling: it supplies kbs2::error::Error, a bunch of From<T> for Error traits, and turns (almost) every failure into a fatal condition with Fatal: {} printed on stderr.

This should be revisited and substantially refactored, with the following goals in mind:

• Simplicity: We want to minimize the number of From<T> for Error boilerplate impls required
• Context: We want to supply as much detail for an error to the user as possible (the filename associated with an I/O failure, &c)
• Readability: map_err et al. are visually distracting; we should take an approach that requires a minimal number of manual transformations between errors

anyhow seems like a good candidate for our use case; there are probably other good alternatives.

Most users don't know to look in this directory. We should just use ~/.config, imitating the default XDG behavior on Linux.

This will make it easier to generate secrets programmatically, and will obviate the need for generator support in terse input mode (which currently works, but is hacky).

Planned design:

$ kbs2 generate [generator]

with generator defaulting to default. For example:

$ kbs2 generate
Goh7gah5Sae7irok

$ kbs2 generate iphone-pin
5882300

Currently, kbs2 embeds the public half of the age key in public-key and points to the (usually wrapped) private half using keyfile. As a result, a fully configured kbs2 has two pieces of configured state: the main config file itself, and the keyfile.

We always armor the keyfile when generating (or wrapping) it, which means that there's nothing stopping us from just embedding it into the config file entirely.

Pros:

• Reduces the number of points of on-disk configuration state

• Allows users to create a single-file kbs2 configuration that can be easily moved around

• Avoids a separate I/O step/point of failure for loading the key (the key becomes an invariant of having a Config)

• Allows us to change -c/--config-dir into --config-file

Cons:

• Makes it harder to reuse a separate age key with kbs2 (maybe not a use case we're targeting anyways)

• Clutters up the config with a big ASCII-armored blob

• Complicates the agent design slightly (the agent currently receives paths to keyfiles)

Currently, kbs2 new stores an empty string when the user presses return on the password field.

This isn't unreasonable, but the default should be changed to generating a password with the default generator. This can then be disabled via a config setting (something like new.generate-on-empty = false).

An incorrect password currently causes the entire kbs2 command to fail, rather than giving the user an opportunity to retry. This should be configurable.

Something like:

pinentry-retry = true

...maybe. Or maybe it should be supported unconditionally, including for the rpassword fallback.

post-hook currently runs unconditionally, and doesn't supply the user with any to determine whether they should perform a set of actions (other than letting the user determine the state of the store for themselves).

It might be nice to pass an argument (or environment variable) indicating the "disposition" of main kbs2 action, e.g. whether it read from the store, wrote to it, both, &c.

Some notes:

1. Custom subcommands would probably have to be out of scope, i.e. not allowed to report disposition (other than KBS2_DISPOSITION=subcommand maybe?).
2. This requires us to return something meaningful from each kbs2::command::whatever function, which is annoying.

Users can currently add their own desktop notifications on some commands (and with the global pre/post hooks), but it might be best to just supply these directly.

Some ideas:

• A top-level notifications: bool field in the config, defaulting to true
• Per-command notifications: bool that overrides the top-level config for a particular command
• Use https://github.com/hoodie/notify-rust to provide notifications so that we don't have to do anything platform-specific ourselves

Subcommands should receive some additional environmental state, to help them:

• KBS2_STORE should contain the fully-qualified path to the kbs2 store
• KBS2_SUBCOMMAND should be 1

There are probably additional settings we should supply.

macOS apparently uses PSHMNAMLEN instead of MAX_PATH for SHM objects, and limits PSHMNAMLEN to 31. Thus, the current SHM name generation fails with ENAMETOOLONG. Sigh.

Source: https://github.com/apple/darwin-xnu/blob/master/bsd/sys/posix_shm.h#L53

Both of these external commands should have optional functionality to present a desktop notification that contains the copied record's username. Makes them a bit easier to use with services that require a username.

kbs2 currently uses the default behavior from rust-clipboard, which is to use the "Clipboard" selection on X11.

We should also support the "Primary" selection, enabling uses to use their middle mouse button to paste secrets from kbs2 pass -c.

The configuration is already in place for this, in the form of the X11Clipboard enum and the commands.pass.x11-clipboard setting.

kbs2 should learn a new config setting, agent-autostart: bool.

That setting should control whether RageLib creation also spawns a kbs2 agent process (if one isn't already running).

In terms of implementation: it probably makes sense to use Command::spawn with std::env::current_exe to avoid an unsafe fork.

This should be relatively simpler with cargo aur.

This should be unlinked with shm_unlink on decryption failure.

External kbs2 commands should additionally receive KBS2_MAJOR_VERSION, KBS2_MINOR_VERSION, and KBS2_PATCH_VERSION in their environments.

That would make it easier for them to check their compatibility with the running version of kbs2 (instead of parsing kbs2 --version, which isn't guaranteed to have a stable format) and would allow them to fail early instead of blowing up when they attempt to use missing functionality.

kbs2 dump already knows how to emit JSON, but it wouldn't hurt to make both of these easier to consume in traditional UNIX tool contexts.

KBSecret has a raw-edit command that opens the given record in $EDITOR:

$ kbsecret raw-edit whatever

We should add a kbs2 edit command that functions similarly.

Emitting a trailing newline makes sense for terminal consumers, but not for tools that would need to strip it off anyways.

kbs2 should be packaged for Homebrew, probably in an independent tap for now. I'll need to do this one myself, probably.

This part of the README needs to be filled in.

As mentioned in #61:

Currently, hooks are only guaranteed to receive one environment variable: KBS2_HOOK=1. They should also receive KBS2_CONFIG_DIR, to prevent the following buggy behavior:

post-hook = "~/.config/kbs2/hooks/whatever-hook"

whatever-hook:

logins=$(kbs2 list -k login)

When invoked via kbs2 -c /some/config/dir dump foobar, this will do the wrong thing: it will attempt to run kbs2 list -k login with the default configuration directory instead of /some/config/dir, returning either the wrong results or an error.

kbs2 currently supports two CLI backends: age and rage.

These are simple and straightforward to support, but are suboptimal in terms of performance (since each encrypt/decrypt operation requires an exec).

We should support an additional backend that uses the age Rust library directly.

Here's how records are currently defined:

#[derive(Debug, Deserialize, Serialize)]
pub struct Record {
    pub timestamp: u64,
    pub label: String,
    pub kind: RecordKind,
    pub fields: Vec<Field>,
}

This is non-ideal for a few reasons:

• kind and fields are disconnected, meaning that serde can't provide any structural guarantees during serialization and deserialization.
• fields is just a naive list of fields, each of which is boils down to a name, value pair. This is a little tedious to create and, like above, incorrectly decouples the fields from their intended type.

Here's a potentially better structure:

pub struct Record {
    pub timestamp: u64,
    pub label: String,
    pub body: RecordBody,
}

pub enum RecordBody {
    Login(LoginFields),
    Environment(EnvironmentFields),
    Unstructured(UnstructuredFields),
}

pub struct LoginFields {
    pub username: String,
    pub password: String,
}

...and so forth. This structure makes it a little harder to filter by kind, but we could fix that by using #[serde(tag = "kind", content = "fields")] for RecordBody.

kbs2 is sorely in need of unit tests.

Tracking by module:

• backend (#45)
• command
• config (#41)
• generator (#43)
• input
• record (#46)
• session (#48)
• util (#40)

We currently use pinentry-rs to interact with a pinentry binary. Specifically, we use with_default_binary to find a binary called pinentry somewhere in the $PATH.

We should allow users to override this somewhere in their configuration, since it's common for users to use pinentry-mac on macOS or pinentry-X where X is some WM/DE specific binary on Linux.

kbs2's threat model is currently narrow: the secret store and private key both live on disk, so any attacker with read access to the user's files might as well have fully access to their unencrypted secrets. In effect, using kbs2 is about the same as using pass with a PGP key that doesn't have a password.

We could improve this by password-protecting the private key. In effect, this would require us to:

• Prompt the user for a master password during kbs2 init, or generate one for them
• Establish a policy for decrypting the private key using the master password (on login, on first use, after periods of inactivity, &c)
• Maintain an agent-style program that persists the unencrypted private key in its memory for kbs2 sessions to consume

These backends currently don't support wrapped keys, returning errors on creation if a user attempts to use them with a wrapped = true in their config.

We should either support wrapped keypairs for these backends or remove them entirely. The latter might be preferable, given that RageLib performs better than both.

kbs2 currently generates tab completions via --completions=SHELL, enabling users to do something like this in their shell profile/initialization file:

eval "$(kbs2 --completions=bash)"

This is pleasant, but limited: the clap-generated completions don't support intelligent positional expansion, e.g.:

• All available record labels for <label>
• All available generators for <generator>
• Possible external subcommands (found by searching PATH for kbs2-*)

We should investigate to see whether clap can populate these correctly and, if not, how difficult it would be to implement (either in clap or on our own).

kbs2 new et al. currently fail with an unhelpful error if the secret store doesn't exist.

It should be unconditionally created, with a very early failure if creation fails.

We should use a GitHub Actions workflow to automatically publish kbs2 releases whenever a new tag is pushed, instead of doing in manually locally.

Hey,
Recently I added tests to the kbs2 package and tried to build it in a clean chroot. Unfortunately it fails due to test_find_default_config_dir function.

failures:

---- kbs2::config::tests::test_find_default_config_dir stdout ----
thread 'kbs2::config::tests::test_find_default_config_dir' panicked at 'assertion failed: parent.exists()', src/kbs2/config.rs:473:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    kbs2::config::tests::test_find_default_config_dir

test result: FAILED. 26 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

It's because home directory points to /root/ so /root/.config/ doesn't exist. Tests are passing when I remove the last 2 lines from the function:

    #[test]
    fn test_find_default_config_dir() {
        let dir = find_default_config_dir().unwrap();
        // NOTE: We can't check whether the main config dir exists since we create it if it
        // doesn't; instead, we just check that it isn't something weird like a regular file.
        assert!(!dir.is_file());

        let parent = dir.parent().unwrap();

        //assert!(parent.exists());
        //assert!(parent.is_dir());
    }

What can we do about this?

Similar to pre-hook and post-hook: it would be nice to allow users to register an error-hook, which gets called exactly once if kbs2 fails. This should include cases where an external command (run through kbs2 <whatever>) fails.

The error-hook should be limited in functionality to just receiving the error message that kbs2 intends to exit with.

Users can modify the config directory using -c/--config-dir and KBS2_CONFIG_DIR when running kbs2 init.

Users can then modify the default store in their newly initialized config by editing the config manually. This is tedious and indirect, so kbs2 init should learn a flag that supports it directly:

For example:

$ kbs2 -c /tmp/kbs2 init --store /tmp/kbs2-store

This should be relatively straightforward using cargo rpm.

Similar to #135, except for GPG.

GPG doesn't have anything exactly like ssh-add, so the closest equivalent is probably something like this:

kbs2 dump --json "${record}" | jq -r ".body.fields.contents" | gpg-preset-passphrase "${keygrip}"

Where record contains the GPG passphrase for the key identified by keygrip (which is inexplicably different from a key's keyid).

Users will have to supply the keygrip on their own, extracted from the output of gpg --list-keys --with-keygrip (I'm not even going to try to parse the colon format).

kbs2 pass should probably have some hooks:

• pre-hook: Run before fetching the password
• post-hook: Run after fetching the password and printing/copying it
• clear-hook: Run after the password has been cleared from the clipboard (if clearing is enabled)

pre-hook and post-hook should probably receive the label passed to kbs2 pass. This would enable some interesting use cases, like transforming the password before and after every use (would this even be useful?).

Setting the following:

[commands.pass]
clipboard-duration = 0

Should cause the clipboard contents to be persisted indefinitely.

It'd be nice to have a kbs2 ssh-add or similar that essentially pulls an SSH key from the given (unstructured) record and adds it to the current ssh-agent using ssh-add. Essentially just this:

kbs2 dump --json "${record}" | jq -r ".body.fields.contents" | base64 -d | ssh-add -

kbs2 currently hardcodes the delimiter as ASCII SOH:

This should be made configurable via both the CLI and the config file, with the current delimiter retained as the default.

This is an absolute must for a 1.0 release.

kbs2 should learn the rekey command, which should:

1. Take a new master passphrase from the user, and use it to generate a new wrapped keypair
2. Make a backup of the current store and keyfile
3. Iterate over every secret in the current store, decrypting and re-encrypting them with the new key
4. Swap the public-key and keyfile out for their new versions

It might make sense to do this by creating an entirely new store from scratch and copying each record into it one-by-one, then doing a directory swap at the very end.