From efe3578c48a54ae33556f7eb614d4ffeb01497fc Mon Sep 17 00:00:00 2001 From: Filip Hert Date: Thu, 3 Sep 2026 19:27:18 +0200 Subject: [PATCH 1/3] feat(secrets): add Bitwarden support via a provider abstraction Connection passwords can now be stored as bw:// references and are resolved through the Bitwarden CLI when a connection is launched. This works with bitwarden.com, self-hosted Bitwarden and Vaultwarden. Bitwarden has no library interface, so the provider runs "bw serve" as a hidden child process bound to 127.0.0.1 on a random free port and talks to its local REST API. The server starts lazily on first use and is stopped when the application exits; on Windows it is placed in a kill-on-close job object and on Linux it gets a parent death signal, so a crash cannot leave an orphaned server holding an unlocked vault. The child inherits BW_SESSION from the environment, so the vault is unlocked once in the user's shell. MremoteGO never asks for, sees or stores the master password. To avoid wiring a second password manager directly into the config manager, launcher and GUI, secret handling now goes through a small Provider interface and a process-wide Registry. 1Password keeps working unchanged; the crypto package uses the registry to decide that references must not be encrypted at rest. The connection dialogs gain a "Bitwarden..." button that lists and searches vault login items and writes back a reference, and a "Store password in Bitwarden" option that creates a login item from a typed password. The startup authentication warning is now generic across providers, only asks about providers the config actually uses, and runs off the UI goroutine because starting the CLI takes a moment. Covered by unit tests that exercise the client against a fake bw serve, so neither the CLI nor a GUI is required to run them. Co-Authored-By: Claude Opus 5 --- .github/workflows/build.yml | 7 +- CHANGELOG.md | 29 ++ README.md | 32 +- cmd/mremotego-gui/main.go | 6 + cmd/mremotego/cmd/root.go | 9 +- docs/BITWARDEN-SETUP.md | 162 ++++++ docs/ENCRYPTION.md | 2 +- docs/PASSWORD-MANAGEMENT.md | 30 ++ docs/README.md | 4 + go.mod | 4 +- go.sum | 6 +- internal/config/manager.go | 43 +- internal/crypto/encryption.go | 9 +- internal/crypto/encryption_test.go | 60 +++ internal/gui/dialogs.go | 130 ++--- internal/gui/mainwindow.go | 72 +-- internal/gui/secretstore.go | 264 ++++++++++ internal/launcher/launcher.go | 24 +- internal/secrets/bitwarden.go | 301 +++++++++++ internal/secrets/bitwarden_client.go | 256 ++++++++++ internal/secrets/bitwarden_process.go | 216 ++++++++ internal/secrets/bitwarden_process_linux.go | 17 + internal/secrets/bitwarden_process_other.go | 9 + internal/secrets/bitwarden_process_unix.go | 28 + internal/secrets/bitwarden_process_windows.go | 81 +++ internal/secrets/bitwarden_reference.go | 62 +++ internal/secrets/bitwarden_reference_test.go | 61 +++ internal/secrets/bitwarden_test.go | 477 ++++++++++++++++++ internal/secrets/onepassword.go | 17 +- internal/secrets/provider.go | 76 +++ internal/secrets/registry.go | 111 ++++ internal/secrets/registry_test.go | 154 ++++++ 32 files changed, 2608 insertions(+), 151 deletions(-) create mode 100644 docs/BITWARDEN-SETUP.md create mode 100644 internal/crypto/encryption_test.go create mode 100644 internal/gui/secretstore.go create mode 100644 internal/secrets/bitwarden.go create mode 100644 internal/secrets/bitwarden_client.go create mode 100644 internal/secrets/bitwarden_process.go create mode 100644 internal/secrets/bitwarden_process_linux.go create mode 100644 internal/secrets/bitwarden_process_other.go create mode 100644 internal/secrets/bitwarden_process_unix.go create mode 100644 internal/secrets/bitwarden_process_windows.go create mode 100644 internal/secrets/bitwarden_reference.go create mode 100644 internal/secrets/bitwarden_reference_test.go create mode 100644 internal/secrets/bitwarden_test.go create mode 100644 internal/secrets/provider.go create mode 100644 internal/secrets/registry.go create mode 100644 internal/secrets/registry_test.go diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 5255437..d57c3d3 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -45,7 +45,7 @@ jobs: - name: Set up Go uses: actions/setup-go@v5 with: - go-version: '1.23' + go-version: '1.24' - name: Install Linux dependencies if: matrix.os == 'ubuntu-latest' || matrix.os == 'ubuntu-24.04-arm' @@ -53,6 +53,11 @@ jobs: sudo apt-get update sudo apt-get install -y gcc libgl1-mesa-dev xorg-dev + - name: Test + env: + CGO_ENABLED: 1 + run: go test ./... + - name: Build (Windows) if: matrix.os == 'windows-latest' env: diff --git a/CHANGELOG.md b/CHANGELOG.md index 4ef1ee8..8800f8a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,35 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Added +- **Bitwarden integration**: connection passwords can be stored as `bw://` + references and are resolved through the Bitwarden CLI at connect time. Works + with bitwarden.com, self-hosted Bitwarden and Vaultwarden. + See [docs/BITWARDEN-SETUP.md](docs/BITWARDEN-SETUP.md). +- Optional field selector on references: `bw:///username`, `/totp`, + `/notes`; the password is the default. +- **Bitwarden item picker** in the add and edit connection dialogs, with search + and a vault sync button, plus a "Store password in Bitwarden" option that + creates a login item and replaces the password with its reference. +- Secret provider abstraction (`secrets.Provider`, `secrets.Registry`), so the + configuration manager, launcher and GUI no longer depend on a single password + manager. +- Unit tests for the secret providers, reference parsing and the encryption + helper, plus a `go test ./...` step in CI. + +### Changed +- The 1Password authentication warning at start-up is now a generic secret + provider check, runs off the UI goroutine, and only asks about providers that + the configuration actually references. + +### Security +- The `bw serve` helper process is bound to loopback on a random port, started + only when a Bitwarden reference is used, and terminated on exit. It is also + placed in a Windows job object, and given a parent death signal on Linux, so + it does not survive a crash. + ## [1.0.4] - 2026-01-28 ### Fixed diff --git a/README.md b/README.md index c56db0a..c288b36 100644 --- a/README.md +++ b/README.md @@ -1,8 +1,8 @@ # MremoteGO -> A modern, cross-platform remote connection manager with git-friendly YAML configs and 1Password integration. +> A modern, cross-platform remote connection manager with git-friendly YAML configs and 1Password and Bitwarden integration. -[![Go Version](https://img.shields.io/badge/Go-1.23+-00ADD8?style=flat&logo=go)](https://go.dev/) +[![Go Version](https://img.shields.io/badge/Go-1.24+-00ADD8?style=flat&logo=go)](https://go.dev/) [![License](https://img.shields.io/badge/License-MIT-blue.svg)](LICENSE) [![Platform](https://img.shields.io/badge/Platform-Windows%20%7C%20Linux%20%7C%20macOS-lightgrey)](https://github.com/jaydenthorup/mremotego) @@ -10,13 +10,14 @@ **The Problem**: mRemoteNG uses XML configs that are painful to diff, merge, and share with teams. Passwords are awkwardly encrypted per-machine. -**The Solution**: MremoteGO uses clean YAML configs that work beautifully with git, plus optional 1Password integration for secure team password sharing. +**The Solution**: MremoteGO uses clean YAML configs that work beautifully with git, plus optional 1Password and Bitwarden integration for secure team password sharing. ## ✨ Features - 🎨 **Modern GUI** - Clean interface with connection tree, search, and quick actions - 🔐 **Password Encryption** - AES-256-GCM encryption at rest with master password - 🔑 **1Password Integration** - Store passwords securely using `op://` references +- 🛡️ **Bitwarden Integration** - Store passwords in Bitwarden or Vaultwarden using `bw://` references - 📝 **Git-Friendly** - YAML configs are easy to diff, merge, and review - 🖥️ **Cross-Platform** - Windows, Linux, macOS (AMD64 & ARM64) - ⚡ **Fast** - Native GUI with instant connections @@ -69,7 +70,7 @@ Simply run the executable without arguments: 1. Click **[+]** or press `Ctrl+N` 2. Fill in connection details (name, protocol, host, credentials) -3. Optionally push password to 1Password +3. Optionally pick a password from Bitwarden, or push it to 1Password or Bitwarden 4. Click **Save** **Connecting:** @@ -145,13 +146,14 @@ connections: host: dev.example.com port: 3389 username: developer + password: bw://8f3c1d9a-4e2b-4c77-9f10-1a2b3c4d5e6f # Bitwarden reference ``` ## 🔐 Security ### Password Storage Options -MremoteGO supports three password storage methods: +MremoteGO supports four password storage methods: 1. **1Password Integration** (Recommended for teams): - Store passwords securely in 1Password vaults @@ -160,20 +162,27 @@ MremoteGO supports three password storage methods: - Supports biometric unlock - See [1Password Setup Guide](docs/1PASSWORD-SETUP.md) -2. **Encrypted** (Recommended for local use): +2. **Bitwarden Integration** (Recommended for teams): + - Store passwords in Bitwarden, self-hosted Bitwarden or Vaultwarden + - Use `bw://item-id` references in your config + - Safe to commit configs to git + - Pick items from the vault directly in the connection dialog + - See [Bitwarden Setup Guide](docs/BITWARDEN-SETUP.md) + +3. **Encrypted** (Recommended for local use): - AES-256-GCM encryption with PBKDF2 key derivation (100,000 iterations) - Master password required on startup - Passwords stored as `enc:base64(salt+nonce+ciphertext)` - See [Encryption Guide](docs/ENCRYPTION.md) -3. **Plain Text** (Not recommended): +4. **Plain Text** (Not recommended): - For testing or when other methods aren't suitable - Should not be committed to git - Use `.gitignore` to exclude `connections.yaml` and `config.yaml` ### Best Practices -- ✅ Use 1Password for team environments +- ✅ Use 1Password or Bitwarden for team environments - ✅ Use encryption for personal configs - ✅ Add `config.yaml` and `connections.yaml` to `.gitignore` - ✅ Use separate configs for different environments @@ -186,13 +195,14 @@ MremoteGO supports three password storage methods: - **[GUI Guide](docs/GUI-GUIDE.md)** - Complete GUI reference - **[Encryption Guide](docs/ENCRYPTION.md)** - Password encryption details - **[1Password Setup](docs/1PASSWORD-SETUP.md)** - Secure password management +- **[Bitwarden Setup](docs/BITWARDEN-SETUP.md)** - Bitwarden and Vaultwarden integration - **[Password Management](docs/PASSWORD-MANAGEMENT.md)** - Security best practices ## 🛠️ Development ### Prerequisites -- Go 1.23 or later +- Go 1.24 or later - For Linux: `gcc`, `libgl1-mesa-dev`, `xorg-dev` - For GUI builds: Fyne dependencies @@ -224,7 +234,7 @@ mremotego/ │ ├── crypto/ # Encryption/decryption │ ├── gui/ # Fyne GUI components │ ├── launcher/ # Protocol launchers (SSH, RDP, etc.) -│ └── secrets/ # 1Password integration +│ └── secrets/ # Password manager providers (1Password, Bitwarden) ├── pkg/ │ └── models/ # Data models └── docs/ # Documentation @@ -299,7 +309,7 @@ This project is licensed under the MIT License - see the [LICENSE](LICENSE) file ### 📋 Planned Features #### Password Managers -- [ ] Bitwarden CLI integration (`bw://` references) +- [x] Bitwarden CLI integration (`bw://` references) - [ ] LastPass CLI integration (`lpass://` references) - [ ] HashiCorp Vault integration - [ ] Pass (password-store) integration for Linux diff --git a/cmd/mremotego-gui/main.go b/cmd/mremotego-gui/main.go index 6842fbf..23f5e0e 100644 --- a/cmd/mremotego-gui/main.go +++ b/cmd/mremotego-gui/main.go @@ -13,6 +13,7 @@ import ( "github.com/jaydenthorup/mremotego/cmd/mremotego/cmd" "github.com/jaydenthorup/mremotego/internal/config" "github.com/jaydenthorup/mremotego/internal/gui" + "github.com/jaydenthorup/mremotego/internal/secrets" ) func main() { @@ -37,6 +38,11 @@ func runGUI() { myApp := app.NewWithID("com.mremotego.app") myApp.Settings().SetTheme(&customTheme{}) + // Secret providers may run helper processes; make sure they are stopped + // however the application exits. + myApp.Lifecycle().SetOnStopped(secrets.Shutdown) + defer secrets.Shutdown() + // Set application icon (ignore errors - icon is optional) if icon := gui.GetAppIcon(); icon != nil { myApp.SetIcon(icon) diff --git a/cmd/mremotego/cmd/root.go b/cmd/mremotego/cmd/root.go index b699e3e..0a9fb41 100644 --- a/cmd/mremotego/cmd/root.go +++ b/cmd/mremotego/cmd/root.go @@ -6,6 +6,7 @@ import ( "github.com/spf13/cobra" "github.com/jaydenthorup/mremotego/internal/config" + "github.com/jaydenthorup/mremotego/internal/secrets" ) var ( @@ -21,7 +22,13 @@ in a human-readable YAML format that works great with version control.`, // Execute runs the root command func Execute() { - if err := rootCmd.Execute(); err != nil { + err := rootCmd.Execute() + + // Stop helper processes started by secret providers. This cannot be a + // defer because the error path calls os.Exit. + secrets.Shutdown() + + if err != nil { fmt.Fprintln(os.Stderr, err) os.Exit(1) } diff --git a/docs/BITWARDEN-SETUP.md b/docs/BITWARDEN-SETUP.md new file mode 100644 index 0000000..c653f62 --- /dev/null +++ b/docs/BITWARDEN-SETUP.md @@ -0,0 +1,162 @@ +# Bitwarden Setup Guide + +MremoteGO can read connection passwords from Bitwarden, so that: + +- Passwords never appear in your config files +- Configs stay safe to commit and share via git +- Credentials can be rotated in one place for the whole team + +This works with bitwarden.com, self-hosted Bitwarden and Vaultwarden. + +## How it works + +Bitwarden has no library interface for other applications, so MremoteGO uses +the official Bitwarden CLI. On first use it starts `bw serve` as a hidden child +process bound to `127.0.0.1` on a random free port, reads what it needs over +that local API, and stops the process when MremoteGO exits. + +MremoteGO never asks for, sees or stores your master password. The child +process inherits the `BW_SESSION` variable, so you unlock the vault once in +your terminal and every lookup after that is authorised by that session. + +## Quick Setup + +### 1. Install the Bitwarden CLI + +```powershell +winget install Bitwarden.CLI +``` + +```bash +# macOS +brew install bitwarden-cli + +# Linux +npm install -g @bitwarden/cli +``` + +Verify with `bw --version`. + +### 2. Point the CLI at your server + +Only needed for self-hosted Bitwarden or Vaultwarden: + +```powershell +bw config server https://vault.example.com +``` + +### 3. Log in and unlock + +```powershell +bw login +$env:BW_SESSION = bw unlock --raw +bw status # should report "unlocked" +``` + +```bash +bw login +export BW_SESSION="$(bw unlock --raw)" +bw status +``` + +### 4. Start MremoteGO from that same terminal + +```powershell +.\mremotego.exe +``` + +The session key is inherited by MremoteGO and by the `bw serve` process it +starts. Launching MremoteGO from a desktop shortcut instead will leave the +vault locked, and it will show you these instructions. + +## Using Bitwarden References + +### In your config + +Instead of a plain text password, store a reference to the vault item: + +```yaml +connections: + - name: "Production Server" + protocol: ssh + host: prod.example.com + username: admin + password: bw://8f3c1d9a-4e2b-4c77-9f10-1a2b3c4d5e6f +``` + +Reference format: + +| Reference | Resolves to | +|-----------|-------------| +| `bw://` | the item's password | +| `bw:///password` | the item's password | +| `bw:///username` | the item's username | +| `bw:///totp` | the current TOTP code | +| `bw:///notes` | the item's notes | + +References use the item id rather than its name, so renaming an item in +Bitwarden does not break your config. + +### Finding an item id + +**In the GUI:** click **Bitwarden...** next to the password field when adding +or editing a connection. Search your vault, pick an item, and the reference is +filled in for you along with the username. + +**On the command line:** + +```powershell +bw list items --search "production" | ConvertFrom-Json | Select-Object id, name +``` + +### Creating items from MremoteGO + +1. Type the password into the password field as usual +2. Tick **Store password in Bitwarden** +3. Save + +MremoteGO creates a login item named after the connection, with the username +and a URI of `://`, then replaces the password in your config +with the new `bw://` reference. + +## Security notes + +- **`bw serve` has no authentication of its own.** Anything running as your + user on your machine could talk to it while it is up. MremoteGO limits the + exposure by binding it to loopback on a random port, starting it only when a + Bitwarden reference is actually used, and terminating it on exit. On Windows + the process is placed in a job object and on Linux it gets a parent death + signal, so it also dies if MremoteGO crashes. +- **`BW_SESSION` unlocks your whole vault.** Treat it like a password: do not + put it in a script that others can read, and close the terminal when done. +- **References are not encrypted at rest**, on purpose. They contain no secret + material, and leaving them readable is what keeps configs diffable in git. + This matches how `op://` references are handled. + +## Troubleshooting + +**"Bitwarden CLI (bw) is not installed or not in PATH"** +Install the CLI and make sure `bw --version` works in the same terminal you +start MremoteGO from. + +**"bitwarden vault is locked"** +The session key is missing or expired. Run `bw unlock --raw` again, set +`BW_SESSION`, and restart MremoteGO from that terminal. + +**"not logged in to bitwarden"** +Run `bw login`. For a self-hosted server, run `bw config server ` first. + +**An item you just created elsewhere is not in the picker** +The CLI serves items from a local cache. Click **Sync vault** in the picker, or +run `bw sync`. + +**RDP connects but prompts for the password** +RDP is deliberately allowed to continue when a reference cannot be resolved, so +you still get a login prompt instead of an error. Check the vault state as +above. + +## See also + +- [Password Management](PASSWORD-MANAGEMENT.md) - all password options +- [1Password Setup](1PASSWORD-SETUP.md) - the other supported password manager +- [Bitwarden CLI documentation](https://bitwarden.com/help/cli/) diff --git a/docs/ENCRYPTION.md b/docs/ENCRYPTION.md index 5b119ab..4ff690b 100644 --- a/docs/ENCRYPTION.md +++ b/docs/ENCRYPTION.md @@ -14,7 +14,7 @@ MremoteGO supports encrypting passwords at rest in the configuration file using - **AES-256-GCM**: Industry-standard authenticated encryption - **Unique Salt**: Each password gets a random 16-byte salt - **Random Nonce**: Each encryption uses a unique nonce -- **1Password Integration**: 1Password references (`op://...`) are NOT encrypted (no need) +- **Password Manager Integration**: references (`op://...`, `bw://...`) are NOT encrypted (no need) ## Using Encryption in the GUI diff --git a/docs/PASSWORD-MANAGEMENT.md b/docs/PASSWORD-MANAGEMENT.md index 9833c71..06319fc 100644 --- a/docs/PASSWORD-MANAGEMENT.md +++ b/docs/PASSWORD-MANAGEMENT.md @@ -7,6 +7,7 @@ MremoteGO provides flexible and secure password management with multiple options | Method | Security | Team Sharing | Auto-Login | Best For | |--------|----------|--------------|------------|----------| | 1Password | ✅ High | ✅ Yes | ✅ Yes | **Recommended** - Teams | +| Bitwarden | ✅ High | ✅ Yes | ✅ Yes | **Recommended** - Teams, self-hosting | | Plain Text | ⚠️ Low | ❌ No | ✅ Yes | Personal/testing only | | No Password | ✅ Manual | N/A | ❌ No | SSH keys, certificates | @@ -34,6 +35,35 @@ connections: - ✅ Automatic password rotation support - ✅ Audit logs +## Bitwarden Integration (Recommended) + +Store passwords in Bitwarden, bitwarden.com or self-hosted, and reference them +in config files. + +### Setup + +See [BITWARDEN-SETUP.md](BITWARDEN-SETUP.md) for complete setup instructions. + +### Usage + +```yaml +connections: + - name: "My Server" + password: bw://8f3c1d9a-4e2b-4c77-9f10-1a2b3c4d5e6f # Secure reference +``` + +References use the item id, so renaming an item does not break the config. A +field can be selected explicitly with `bw:///username`, `/totp` or +`/notes`. + +### Benefits +- ✅ Passwords never stored in config files +- ✅ Safe to commit configs to git +- ✅ Team password sharing +- ✅ Works with self-hosted Bitwarden and Vaultwarden +- ✅ Item picker built into the connection dialog +- ✅ Master password never handled by MremoteGO + ## Plain Text Passwords For personal use or testing environments: diff --git a/docs/README.md b/docs/README.md index 63c4010..7ebb45d 100644 --- a/docs/README.md +++ b/docs/README.md @@ -8,6 +8,7 @@ ## Security & Passwords - **[1Password Setup](1PASSWORD-SETUP.md)** - Secure password management with 1Password +- **[Bitwarden Setup](BITWARDEN-SETUP.md)** - Secure password management with Bitwarden or Vaultwarden - **[Password Management](PASSWORD-MANAGEMENT.md)** - All password options and security details ## Main Documentation @@ -21,6 +22,7 @@ - [Add your first connection](QUICKSTART.md#first-connection) - [Set up 1Password](1PASSWORD-SETUP.md#quick-setup) +- [Set up Bitwarden](BITWARDEN-SETUP.md#quick-setup) - [Organize connections](GUI-GUIDE.md#connection-tree) - [Share configs with team](1PASSWORD-SETUP.md#using-1password-references) @@ -28,6 +30,7 @@ - [RDP password issues](PASSWORD-MANAGEMENT.md#rdp-auto-login) - [1Password not working](1PASSWORD-SETUP.md#troubleshooting) +- [Bitwarden not working](BITWARDEN-SETUP.md#troubleshooting) - [SSH connections failing](QUICKSTART.md#troubleshooting) - [GUI issues](GUI-GUIDE.md#troubleshooting) @@ -38,6 +41,7 @@ | `QUICKSTART.md` | 5-minute getting started guide | | `GUI-GUIDE.md` | Complete GUI interface documentation | | `1PASSWORD-SETUP.md` | Setting up 1Password integration | +| `BITWARDEN-SETUP.md` | Setting up Bitwarden integration | | `PASSWORD-MANAGEMENT.md` | Security and password options | ## Contributing diff --git a/go.mod b/go.mod index acfcd70..ae704eb 100644 --- a/go.mod +++ b/go.mod @@ -4,9 +4,10 @@ go 1.24.0 require ( fyne.io/fyne/v2 v2.7.2 + github.com/danieljoos/wincred v1.2.3 github.com/spf13/cobra v1.8.0 golang.org/x/crypto v0.47.0 - golang.org/x/term v0.39.0 + golang.org/x/sys v0.40.0 gopkg.in/yaml.v3 v3.0.1 ) @@ -42,6 +43,5 @@ require ( github.com/yuin/goldmark v1.7.8 // indirect golang.org/x/image v0.24.0 // indirect golang.org/x/net v0.48.0 // indirect - golang.org/x/sys v0.40.0 // indirect golang.org/x/text v0.33.0 // indirect ) diff --git a/go.sum b/go.sum index 5f4159b..d8488e6 100644 --- a/go.sum +++ b/go.sum @@ -5,6 +5,8 @@ fyne.io/systray v1.12.0/go.mod h1:RVwqP9nYMo7h5zViCBHri2FgjXF7H2cub7MAq4NSoLs= github.com/BurntSushi/toml v1.5.0 h1:W5quZX/G/csjUnuI8SUYlsHs9M38FC7znL0lIO+DvMg= github.com/BurntSushi/toml v1.5.0/go.mod h1:ukJfTF/6rtPPRCnwkur4qwRxa8vTRFBF0uk2lLoLwho= github.com/cpuguy83/go-md2man/v2 v2.0.3/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46tRHOmNcaadrF8o= +github.com/danieljoos/wincred v1.2.3 h1:v7dZC2x32Ut3nEfRH+vhoZGvN72+dQ/snVXo/vMFLdQ= +github.com/danieljoos/wincred v1.2.3/go.mod h1:6qqX0WNrS4RzPZ1tnroDzq9kY3fu1KwE7MRLQK4X0bs= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/felixge/fgprof v0.9.3 h1:VvyZxILNuCiUCSXtPtYmmtGvb65nqXh2QFWc0Wpf2/g= @@ -69,6 +71,8 @@ github.com/srwiley/oksvg v0.0.0-20221011165216-be6e8873101c h1:km8GpoQut05eY3GiY github.com/srwiley/oksvg v0.0.0-20221011165216-be6e8873101c/go.mod h1:cNQ3dwVJtS5Hmnjxy6AgTPd0Inb3pW05ftPSX7NZO7Q= github.com/srwiley/rasterx v0.0.0-20220730225603-2ab79fcdd4ef h1:Ch6Q+AZUxDBCVqdkI8FSpFyZDtCVBc2VmejdNrm5rRQ= github.com/srwiley/rasterx v0.0.0-20220730225603-2ab79fcdd4ef/go.mod h1:nXTWP6+gD5+LUJ8krVhhoeHjvHTutPxMYl5SvkcnJNE= +github.com/stretchr/objx v0.5.2 h1:xuMeJ0Sdp5ZMRXx/aWO6RZxdr3beISkG5/G/aIRr3pY= +github.com/stretchr/objx v0.5.2/go.mod h1:FRsXN1f5AsAjCGJKqEizvkpNtU+EGNCLh3NxZ/8L+MA= github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= github.com/yuin/goldmark v1.7.8 h1:iERMLn0/QJeHFhxSt3p6PeN9mGnvIKSpG9YYorDMnic= @@ -81,8 +85,6 @@ golang.org/x/net v0.48.0 h1:zyQRTTrjc33Lhh0fBgT/H3oZq9WuvRR5gPC70xpDiQU= golang.org/x/net v0.48.0/go.mod h1:+ndRgGjkh8FGtu1w1FGbEC31if4VrNVMuKTgcAAnQRY= golang.org/x/sys v0.40.0 h1:DBZZqJ2Rkml6QMQsZywtnjnnGvHza6BTfYFWY9kjEWQ= golang.org/x/sys v0.40.0/go.mod h1:OgkHotnGiDImocRcuBABYBEXf8A9a87e/uXjp9XT3ks= -golang.org/x/term v0.39.0 h1:RclSuaJf32jOqZz74CkPA9qFuVTX7vhLlpfj/IGWlqY= -golang.org/x/term v0.39.0/go.mod h1:yxzUCTP/U+FzoxfdKmLaA0RV1WgE0VY7hXBwKtY/4ww= golang.org/x/text v0.33.0 h1:B3njUFyqtHDUI5jMn1YIr5B0IE2U0qck04r6d4KPAxE= golang.org/x/text v0.33.0/go.mod h1:LuMebE6+rBincTi9+xWTY8TztLzKHc/9C1uBCG27+q8= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= diff --git a/internal/config/manager.go b/internal/config/manager.go index 90888da..da3b9ce 100644 --- a/internal/config/manager.go +++ b/internal/config/manager.go @@ -14,18 +14,18 @@ import ( // Manager handles configuration file operations type Manager struct { - configPath string - config *models.Config - onePasswordProvider *secrets.OnePasswordProvider - encryptionProvider *crypto.EncryptionProvider + configPath string + config *models.Config + secrets *secrets.Registry + encryptionProvider *crypto.EncryptionProvider } // NewManager creates a new configuration manager func NewManager(configPath string) *Manager { return &Manager{ - configPath: configPath, - onePasswordProvider: secrets.NewOnePasswordProvider(), - encryptionProvider: nil, // Will be set when master password is provided + configPath: configPath, + secrets: secrets.Default(), + encryptionProvider: nil, // Will be set when master password is provided } } @@ -382,14 +382,31 @@ func (m *Manager) UpdateConnection(name string, updates *models.Connection) erro return nil } -// IsOnePasswordReference checks if a password is a 1Password reference -func (m *Manager) IsOnePasswordReference(password string) bool { - return m.onePasswordProvider.IsReference(password) +// Secrets returns the secret provider registry used by this manager. +func (m *Manager) Secrets() *secrets.Registry { + return m.secrets } -// CreateOnePasswordItem creates a new 1Password item and returns the reference -func (m *Manager) CreateOnePasswordItem(vault, title, username, password string) (string, error) { - return m.onePasswordProvider.CreateItem(vault, title, username, password) +// IsSecretReference reports whether a password is a reference handled by one of +// the configured secret providers, for example "op://" or "bw://". +func (m *Manager) IsSecretReference(password string) bool { + return m.secrets.IsReference(password) +} + +// CreateSecretItem stores a login item in the provider identified by scheme and +// returns the reference that should be written to the configuration. +func (m *Manager) CreateSecretItem(scheme string, req secrets.CreateItemRequest) (string, error) { + provider, ok := m.secrets.ByScheme(scheme) + if !ok { + return "", fmt.Errorf("unknown secret provider: %s", scheme) + } + + creator, ok := provider.(secrets.ItemCreator) + if !ok { + return "", fmt.Errorf("%s cannot create items", provider.Name()) + } + + return creator.CreateItem(req) } // saveRecentFile saves the current config path as the most recently used file diff --git a/internal/crypto/encryption.go b/internal/crypto/encryption.go index b66ab3a..9cfcfa0 100644 --- a/internal/crypto/encryption.go +++ b/internal/crypto/encryption.go @@ -10,6 +10,7 @@ import ( "io" "strings" + "github.com/jaydenthorup/mremotego/internal/secrets" "golang.org/x/crypto/pbkdf2" ) @@ -172,7 +173,8 @@ func (p *EncryptionProvider) DecryptIfNeeded(value string) (string, error) { } // ShouldEncrypt checks if a value should be encrypted -// Returns false for empty strings, 1Password references, or already encrypted values +// Returns false for empty strings, secret manager references, or already +// encrypted values func (p *EncryptionProvider) ShouldEncrypt(value string) bool { if !p.enabled || value == "" { return false @@ -183,8 +185,9 @@ func (p *EncryptionProvider) ShouldEncrypt(value string) bool { return false } - // Don't encrypt 1Password references - if strings.HasPrefix(value, "op://") { + // Don't encrypt references to an external secret manager; they contain no + // secret material and stay readable so configs remain diffable in git. + if secrets.IsKnownReference(value) { return false } diff --git a/internal/crypto/encryption_test.go b/internal/crypto/encryption_test.go new file mode 100644 index 0000000..79ca3a3 --- /dev/null +++ b/internal/crypto/encryption_test.go @@ -0,0 +1,60 @@ +package crypto + +import "testing" + +func TestShouldEncrypt(t *testing.T) { + provider := NewEncryptionProvider("master") + + tests := map[string]bool{ + "hunter2": true, + "": false, + "op://Private/Server/password": false, + "bw://8f3c-item-id": false, + "enc:AAAA": false, + } + + for value, want := range tests { + if got := provider.ShouldEncrypt(value); got != want { + t.Errorf("ShouldEncrypt(%q) = %v, want %v", value, got, want) + } + } +} + +func TestShouldEncryptDisabledWithoutMasterPassword(t *testing.T) { + provider := NewEncryptionProvider("") + + if provider.ShouldEncrypt("hunter2") { + t.Error("ShouldEncrypt should be false when no master password is set") + } +} + +func TestEncryptDecryptRoundTrip(t *testing.T) { + provider := NewEncryptionProvider("master") + + encrypted, err := provider.Encrypt("hunter2") + if err != nil { + t.Fatalf("Encrypt returned error: %v", err) + } + if !provider.IsEncrypted(encrypted) { + t.Fatalf("Encrypt produced %q which is not recognised as encrypted", encrypted) + } + + decrypted, err := provider.Decrypt(encrypted) + if err != nil { + t.Fatalf("Decrypt returned error: %v", err) + } + if decrypted != "hunter2" { + t.Errorf("Decrypt = %q, want %q", decrypted, "hunter2") + } +} + +func TestDecryptWithWrongPasswordFails(t *testing.T) { + encrypted, err := NewEncryptionProvider("master").Encrypt("hunter2") + if err != nil { + t.Fatalf("Encrypt returned error: %v", err) + } + + if _, err := NewEncryptionProvider("other").Decrypt(encrypted); err == nil { + t.Error("expected decryption with the wrong master password to fail") + } +} diff --git a/internal/gui/dialogs.go b/internal/gui/dialogs.go index 93e3140..e8245ec 100644 --- a/internal/gui/dialogs.go +++ b/internal/gui/dialogs.go @@ -1,7 +1,6 @@ package gui import ( - "fmt" "strconv" "strings" "time" @@ -110,7 +109,7 @@ func (w *MainWindow) showAddConnectionDialog() { usernameEntry.SetPlaceHolder("username") passwordEntry := widget.NewEntry() - passwordEntry.SetPlaceHolder("password or op://vault/item/field") + passwordEntry.SetPlaceHolder(passwordPlaceholder) domainEntry := widget.NewEntry() domainEntry.SetPlaceHolder("domain (for RDP)") @@ -127,34 +126,24 @@ func (w *MainWindow) showAddConnectionDialog() { folderSelect := widget.NewSelect(folderNames, nil) folderSelect.SetSelected("(Root)") - // 1Password integration - storeTo1PasswordCheck := widget.NewCheck("Store password in 1Password", nil) - vaultSelect := widget.NewSelect([]string{"DevOps", "Private", "Employee"}, nil) - vaultSelect.SetSelected("DevOps") - vaultSelect.Hide() - - storeTo1PasswordCheck.OnChanged = func(checked bool) { - if checked { - vaultSelect.Show() - } else { - vaultSelect.Hide() - } + // Password manager integration + secretStore := w.newSecretStoreControls(usernameEntry, passwordEntry) + + items := []*widget.FormItem{ + {Text: "Name", Widget: nameEntry}, + {Text: "Protocol", Widget: protocolSelect}, + {Text: "Host", Widget: hostEntry}, + {Text: "Port", Widget: portEntry}, + {Text: "Username", Widget: usernameEntry}, + {Text: "Password", Widget: secretStore.passwordWidget(passwordEntry)}, + {Text: "Domain", Widget: domainEntry}, + {Text: "Description", Widget: descriptionEntry}, + {Text: "Folder", Widget: folderSelect}, } + items = append(items, secretStore.formItems()...) form := &widget.Form{ - Items: []*widget.FormItem{ - {Text: "Name", Widget: nameEntry}, - {Text: "Protocol", Widget: protocolSelect}, - {Text: "Host", Widget: hostEntry}, - {Text: "Port", Widget: portEntry}, - {Text: "Username", Widget: usernameEntry}, - {Text: "Password", Widget: passwordEntry}, - {Text: "Domain", Widget: domainEntry}, - {Text: "Description", Widget: descriptionEntry}, - {Text: "Folder", Widget: folderSelect}, - {Text: "", Widget: storeTo1PasswordCheck}, - {Text: "Vault", Widget: vaultSelect}, - }, + Items: items, OnSubmit: func() { conn := models.NewConnection(nameEntry.Text, models.Protocol(protocolSelect.Selected)) conn.Host = hostEntry.Text @@ -173,17 +162,17 @@ func (w *MainWindow) showAddConnectionDialog() { conn.Port = conn.Protocol.GetDefaultPort() } - // If user wants to store in 1Password, create the item - if storeTo1PasswordCheck.Checked && conn.Password != "" && !w.manager.IsOnePasswordReference(conn.Password) { - vault := vaultSelect.Selected - reference, err := w.manager.CreateOnePasswordItem(vault, conn.Name, conn.Username, conn.Password) - if err != nil { - dialog.ShowError(fmt.Errorf("Failed to create 1Password item: %w", err), w.window) - return - } - // Replace password with 1Password reference - conn.Password = reference - dialog.ShowInformation("Success", fmt.Sprintf("Password stored in 1Password vault '%s'", vault), w.window) + // If requested, push the password to a password manager and keep + // only the reference in the configuration. + value, providerName, err := secretStore.storeIfRequested( + conn.Name, conn.Username, conn.Password, string(conn.Protocol), conn.Host) + if err != nil { + dialog.ShowError(err, w.window) + return + } + conn.Password = value + if providerName != "" { + dialog.ShowInformation("Success", "Password stored in "+providerName, w.window) } // Add to selected folder or root @@ -303,48 +292,37 @@ func (w *MainWindow) showEditConnectionDialog(conn *models.Connection) { folderSelect := widget.NewSelect(folderNames, nil) folderSelect.SetSelected(currentFolder) - // 1Password integration for edit - storeTo1PasswordCheck := widget.NewCheck("Push password to 1Password", nil) - vaultSelect := widget.NewSelect([]string{"DevOps", "Private", "Employee"}, nil) - vaultSelect.SetSelected("DevOps") - vaultSelect.Hide() - - storeTo1PasswordCheck.OnChanged = func(checked bool) { - if checked { - vaultSelect.Show() - } else { - vaultSelect.Hide() - } + // Password manager integration + secretStore := w.newSecretStoreControls(usernameEntry, passwordEntry) + + items := []*widget.FormItem{ + {Text: "Name", Widget: nameEntry}, + {Text: "Protocol", Widget: protocolSelect}, + {Text: "Host", Widget: hostEntry}, + {Text: "Port", Widget: portEntry}, + {Text: "Username", Widget: usernameEntry}, + {Text: "Password", Widget: secretStore.passwordWidget(passwordEntry)}, + {Text: "Domain", Widget: domainEntry}, + {Text: "Description", Widget: descriptionEntry}, + {Text: "Folder", Widget: folderSelect}, } + items = append(items, secretStore.formItems()...) form := &widget.Form{ - Items: []*widget.FormItem{ - {Text: "Name", Widget: nameEntry}, - {Text: "Protocol", Widget: protocolSelect}, - {Text: "Host", Widget: hostEntry}, - {Text: "Port", Widget: portEntry}, - {Text: "Username", Widget: usernameEntry}, - {Text: "Password", Widget: passwordEntry}, - {Text: "Domain", Widget: domainEntry}, - {Text: "Description", Widget: descriptionEntry}, - {Text: "Folder", Widget: folderSelect}, - {Text: "", Widget: storeTo1PasswordCheck}, - {Text: "Vault", Widget: vaultSelect}, - }, + Items: items, OnSubmit: func() { - // If user wants to push password to 1Password - if storeTo1PasswordCheck.Checked && passwordEntry.Text != "" && !w.manager.IsOnePasswordReference(passwordEntry.Text) { - vault := vaultSelect.Selected - reference, err := w.manager.CreateOnePasswordItem(vault, nameEntry.Text, usernameEntry.Text, passwordEntry.Text) - if err != nil { - dialog.ShowError(fmt.Errorf("Failed to create 1Password item: %w", err), w.window) - return - } - // Replace password with 1Password reference - conn.Password = reference - dialog.ShowInformation("Success", fmt.Sprintf("Password stored in 1Password vault '%s'", vault), w.window) - } else { - conn.Password = passwordEntry.Text + // If requested, push the password to a password manager and keep + // only the reference in the configuration. + value, providerName, err := secretStore.storeIfRequested( + nameEntry.Text, usernameEntry.Text, passwordEntry.Text, + protocolSelect.Selected, hostEntry.Text) + if err != nil { + dialog.ShowError(err, w.window) + return + } + conn.Password = value + if providerName != "" { + dialog.ShowInformation("Success", "Password stored in "+providerName, w.window) } conn.Name = nameEntry.Text diff --git a/internal/gui/mainwindow.go b/internal/gui/mainwindow.go index 0b707cc..6b1dd63 100644 --- a/internal/gui/mainwindow.go +++ b/internal/gui/mainwindow.go @@ -12,6 +12,7 @@ import ( "fyne.io/fyne/v2/widget" "github.com/jaydenthorup/mremotego/internal/config" "github.com/jaydenthorup/mremotego/internal/launcher" + "github.com/jaydenthorup/mremotego/internal/secrets" "github.com/jaydenthorup/mremotego/pkg/models" ) @@ -557,50 +558,63 @@ func (w *MainWindow) showAbout() { func (w *MainWindow) Show() { w.window.Show() - // Check 1Password CLI authentication status after window is shown - w.check1PasswordAuth() + // Check secret provider authentication status after window is shown + w.checkSecretProviderAuth() } -// check1PasswordAuth checks if 1Password CLI needs authentication -func (w *MainWindow) check1PasswordAuth() { - // Check if there are any 1Password references in use - hasOpReferences := false +// checkSecretProviderAuth warns about secret providers that are referenced by a +// connection but cannot currently deliver secrets. +func (w *MainWindow) checkSecretProviderAuth() { + registry := w.launcher.Secrets() + + // Collect the providers actually referenced by the configuration; there is + // no point asking the user to sign in to a provider they do not use. + var used []secrets.Provider + seen := make(map[string]bool) var checkConnections func([]*models.Connection) checkConnections = func(conns []*models.Connection) { for _, conn := range conns { if conn.IsFolder() { checkConnections(conn.Children) - } else if strings.HasPrefix(conn.Password, "op://") { - hasOpReferences = true - return + continue + } + if provider, ok := registry.ProviderFor(conn.Password); ok && !seen[provider.Scheme()] { + seen[provider.Scheme()] = true + used = append(used, provider) } } } checkConnections(w.manager.GetConfig().Connections) - // Only check if there are actually 1Password references in use - if !hasOpReferences { + if len(used) == 0 { return } - // Check if 1Password CLI is authenticated - opProvider := w.launcher.GetOnePasswordProvider() - if opProvider.IsEnabled() && !opProvider.IsAuthenticated() { - // Show a helpful dialog - content := widget.NewLabel(opProvider.GetAuthenticationInstructions()) - content.Wrapping = fyne.TextWrapWord - - scrollContainer := container.NewVScroll(content) - scrollContainer.SetMinSize(fyne.NewSize(600, 400)) - - dialog.ShowCustom( - "1Password CLI Not Authenticated", - "OK", - scrollContainer, - w.window, - ) - } -} // Reload refreshes the window with the loaded config + // Checking authentication can start a helper process and talk to it, so it + // must not run on the UI goroutine. + go func() { + for _, provider := range used { + if !provider.IsEnabled() || provider.IsAuthenticated() { + continue + } + + title := provider.Name() + " Not Authenticated" + instructions := provider.GetAuthenticationInstructions() + + fyne.Do(func() { + content := widget.NewLabel(instructions) + content.Wrapping = fyne.TextWrapWord + + scrollContainer := container.NewVScroll(content) + scrollContainer.SetMinSize(fyne.NewSize(600, 400)) + + dialog.ShowCustom(title, "OK", scrollContainer, w.window) + }) + } + }() +} + +// Reload refreshes the window with the loaded config func (w *MainWindow) Reload() { w.buildConnectionMap() w.tree.Refresh() diff --git a/internal/gui/secretstore.go b/internal/gui/secretstore.go new file mode 100644 index 0000000..cc62281 --- /dev/null +++ b/internal/gui/secretstore.go @@ -0,0 +1,264 @@ +package gui + +import ( + "fmt" + "strings" + + "fyne.io/fyne/v2" + "fyne.io/fyne/v2/container" + "fyne.io/fyne/v2/dialog" + "fyne.io/fyne/v2/layout" + "fyne.io/fyne/v2/widget" + "github.com/jaydenthorup/mremotego/internal/secrets" +) + +// passwordPlaceholder documents every accepted form of the password field. +const passwordPlaceholder = "password, op://vault/item/field or bw://item-id" + +// secretStoreControls are the shared widgets that let a connection dialog pick +// an existing secret or push a new one to a password manager. Both the add and +// the edit dialog use them, so the two stay in step. +type secretStoreControls struct { + window *MainWindow + + opCheck *widget.Check + bwCheck *widget.Check + vaultSelect *widget.Select + pickButton *widget.Button +} + +// newSecretStoreControls builds the controls for one dialog. usernameEntry and +// passwordEntry are filled in when the user picks an existing item. +func (w *MainWindow) newSecretStoreControls(usernameEntry, passwordEntry *widget.Entry) *secretStoreControls { + c := &secretStoreControls{window: w} + + c.vaultSelect = widget.NewSelect([]string{"DevOps", "Private", "Employee"}, nil) + c.vaultSelect.SetSelected("DevOps") + c.vaultSelect.Hide() + + c.opCheck = widget.NewCheck("Store password in 1Password", nil) + c.bwCheck = widget.NewCheck("Store password in Bitwarden", nil) + + // Storing the same password in two vaults would leave one of them stale, so + // the two options exclude each other. + c.opCheck.OnChanged = func(checked bool) { + if checked { + c.bwCheck.SetChecked(false) + c.vaultSelect.Show() + } else { + c.vaultSelect.Hide() + } + } + + bitwarden := w.bitwardenProvider() + + c.bwCheck.OnChanged = func(checked bool) { + if !checked { + return + } + c.opCheck.SetChecked(false) + // Starting the helper process takes a moment; do it now so that saving + // the form does not block. + if bitwarden != nil { + bitwarden.Warmup() + } + } + + c.pickButton = widget.NewButton("Bitwarden...", func() { + w.showBitwardenPicker(func(item secrets.BitwardenItem) { + passwordEntry.SetText("bw://" + item.ID) + if usernameEntry.Text == "" { + usernameEntry.SetText(item.Username) + } + }) + }) + + if bitwarden == nil || !bitwarden.IsEnabled() { + c.bwCheck.SetText("Store password in Bitwarden (bw CLI not installed)") + c.bwCheck.Disable() + c.pickButton.Hide() + } + + return c +} + +// passwordWidget wraps the password entry so the picker button sits next to it. +func (c *secretStoreControls) passwordWidget(passwordEntry *widget.Entry) fyne.CanvasObject { + return container.NewBorder(nil, nil, nil, c.pickButton, passwordEntry) +} + +// formItems returns the rows to append to a connection form. +func (c *secretStoreControls) formItems() []*widget.FormItem { + return []*widget.FormItem{ + {Text: "", Widget: c.opCheck}, + {Text: "", Widget: c.bwCheck}, + {Text: "Vault", Widget: c.vaultSelect}, + } +} + +// storeIfRequested pushes the password to the selected password manager and +// returns the value to write to the configuration: either a reference to the +// newly created item, or the password unchanged. providerName names the manager +// that was used, or is empty when nothing was stored. +func (c *secretStoreControls) storeIfRequested(title, username, password, protocol, host string) (value string, providerName string, err error) { + // Nothing to store, or the value already is a reference. + if password == "" || c.window.manager.IsSecretReference(password) { + return password, "", nil + } + + req := secrets.CreateItemRequest{ + Title: title, + Username: username, + Password: password, + } + + var scheme string + switch { + case c.opCheck.Checked: + scheme, providerName = secrets.SchemeOnePassword, "1Password" + req.Vault = c.vaultSelect.Selected + case c.bwCheck.Checked: + scheme, providerName = secrets.SchemeBitwarden, "Bitwarden" + // Recording the target makes the item useful in the Bitwarden clients + // as well, not just here. + if protocol != "" && host != "" { + req.URI = protocol + "://" + host + } + default: + return password, "", nil + } + + reference, err := c.window.manager.CreateSecretItem(scheme, req) + if err != nil { + return "", providerName, fmt.Errorf("failed to create %s item: %w", providerName, err) + } + + return reference, providerName, nil +} + +// bitwardenProvider returns the Bitwarden provider, or nil when it is not +// registered. +func (w *MainWindow) bitwardenProvider() *secrets.BitwardenProvider { + provider, ok := w.launcher.Secrets().ByScheme(secrets.SchemeBitwarden) + if !ok { + return nil + } + + bitwarden, _ := provider.(*secrets.BitwardenProvider) + return bitwarden +} + +// showBitwardenPicker lets the user choose a login item from their vault. The +// chosen item is reported as a reference, so the password itself never reaches +// the configuration file. +func (w *MainWindow) showBitwardenPicker(onPick func(secrets.BitwardenItem)) { + bitwarden := w.bitwardenProvider() + if bitwarden == nil || !bitwarden.IsEnabled() { + dialog.ShowError(fmt.Errorf("Bitwarden CLI (bw) is not installed or not in PATH"), w.window) + return + } + + var all []secrets.BitwardenItem + var filtered []secrets.BitwardenItem + var selected *secrets.BitwardenItem + + statusLabel := widget.NewLabel("Loading vault items...") + + list := widget.NewList( + func() int { return len(filtered) }, + func() fyne.CanvasObject { return widget.NewLabel("") }, + func(id widget.ListItemID, obj fyne.CanvasObject) { + if id >= len(filtered) { + return + } + item := filtered[id] + label := item.Name + if item.Username != "" { + label += " - " + item.Username + } + obj.(*widget.Label).SetText(label) + }, + ) + list.OnSelected = func(id widget.ListItemID) { + if id < len(filtered) { + item := filtered[id] + selected = &item + } + } + + searchEntry := widget.NewEntry() + searchEntry.SetPlaceHolder("Search by name, username or URL") + + // The whole vault is fetched once and filtered locally: asking the CLI on + // every keystroke would mean a round trip per character. + applyFilter := func(query string) { + query = strings.ToLower(strings.TrimSpace(query)) + + filtered = filtered[:0] + for _, item := range all { + if query == "" || + strings.Contains(strings.ToLower(item.Name), query) || + strings.Contains(strings.ToLower(item.Username), query) || + strings.Contains(strings.ToLower(item.URI), query) { + filtered = append(filtered, item) + } + } + + selected = nil + list.UnselectAll() + list.Refresh() + statusLabel.SetText(fmt.Sprintf("%d of %d items", len(filtered), len(all))) + } + + searchEntry.OnChanged = applyFilter + + // load fetches the vault in the background; every UI update goes through + // fyne.Do because Fyne widgets may only be touched from the UI goroutine. + load := func(sync bool) { + statusLabel.SetText("Loading vault items...") + + go func() { + if sync { + if err := bitwarden.Sync(); err != nil { + fyne.Do(func() { + statusLabel.SetText("Sync failed: " + err.Error()) + }) + return + } + } + + items, err := bitwarden.ListLoginItems("") + fyne.Do(func() { + if err != nil { + statusLabel.SetText("Failed to load items") + dialog.ShowError(err, w.window) + return + } + + all = items + applyFilter(searchEntry.Text) + }) + }() + } + + syncButton := widget.NewButton("Sync vault", func() { load(true) }) + + content := container.NewBorder( + searchEntry, + container.NewHBox(statusLabel, layout.NewSpacer(), syncButton), + nil, + nil, + list, + ) + + picker := dialog.NewCustomConfirm("Pick from Bitwarden", "Use", "Cancel", content, + func(confirmed bool) { + if confirmed && selected != nil { + onPick(*selected) + } + }, w.window) + picker.Resize(fyne.NewSize(600, 500)) + picker.Show() + + load(false) +} diff --git a/internal/launcher/launcher.go b/internal/launcher/launcher.go index 3b7212e..ec6a48c 100644 --- a/internal/launcher/launcher.go +++ b/internal/launcher/launcher.go @@ -15,19 +15,20 @@ import ( // Launcher handles launching connections type Launcher struct { - onePasswordProvider *secrets.OnePasswordProvider + secrets *secrets.Registry } // NewLauncher creates a new launcher func NewLauncher() *Launcher { return &Launcher{ - onePasswordProvider: secrets.NewOnePasswordProvider(), + secrets: secrets.Default(), } } -// GetOnePasswordProvider returns the 1Password provider for checking authentication status -func (l *Launcher) GetOnePasswordProvider() *secrets.OnePasswordProvider { - return l.onePasswordProvider +// Secrets returns the secret provider registry, for example to check +// authentication status before launching a connection. +func (l *Launcher) Secrets() *secrets.Registry { + return l.secrets } // Launch launches a connection based on its protocol @@ -36,18 +37,19 @@ func (l *Launcher) Launch(conn *models.Connection) error { return fmt.Errorf("cannot launch a folder") } - // Resolve 1Password reference if needed (make a copy to avoid modifying the original) + // Resolve a secret manager reference if needed (make a copy to avoid + // modifying the original) resolvedConn := *conn - if l.onePasswordProvider.IsReference(conn.Password) { - resolved, err := l.onePasswordProvider.ResolveSecret(conn.Password) + if provider, ok := l.secrets.ProviderFor(conn.Password); ok { + resolved, err := provider.ResolveSecret(conn.Password) if err != nil { // For RDP, we can continue without a password (will prompt) // For other protocols that require a password, return the error if conn.Protocol != models.ProtocolRDP { - return fmt.Errorf("failed to resolve password from 1Password: %w", err) + return fmt.Errorf("failed to resolve password from %s: %w", provider.Name(), err) } - // RDP: Clear the password so it doesn't try to use the op:// reference - fmt.Printf("Warning: Failed to resolve password from 1Password: %v (RDP will prompt for credentials)\n", err) + // RDP: Clear the password so it doesn't try to use the reference + fmt.Printf("Warning: Failed to resolve password from %s: %v (RDP will prompt for credentials)\n", provider.Name(), err) resolvedConn.Password = "" } else { resolvedConn.Password = resolved diff --git a/internal/secrets/bitwarden.go b/internal/secrets/bitwarden.go new file mode 100644 index 0000000..26f58d5 --- /dev/null +++ b/internal/secrets/bitwarden.go @@ -0,0 +1,301 @@ +package secrets + +import ( + "context" + "fmt" + "os/exec" + "strings" + "sync" +) + +// BitwardenProvider resolves "bw://" references through the Bitwarden CLI. +// +// The CLI has no library interface, so the provider runs "bw serve" as a child +// process and talks to its REST API over loopback. The server is started lazily +// on first use, bound to 127.0.0.1 on a random free port, and terminated when +// Close is called. +// +// The child inherits BW_SESSION from the environment. Unlocking therefore +// happens once, in the shell the application was started from, and no master +// password is ever handled by MremoteGO itself. +type BitwardenProvider struct { + bwPath string + enabled bool + + mu sync.Mutex + server *bwServer + client *bwClient + + // newClient creates the transport. Tests replace it to talk to an httptest + // server instead of spawning a real CLI. + newClient func() (*bwClient, *bwServer, error) +} + +var ( + _ Provider = (*BitwardenProvider)(nil) + _ ItemCreator = (*BitwardenProvider)(nil) +) + +// BitwardenItem is a login item as shown in the item picker. +type BitwardenItem struct { + ID string + Name string + Username string + URI string +} + +// NewBitwardenProvider creates a provider backed by the Bitwarden CLI. It only +// looks the binary up; no process is started until a secret is needed. +func NewBitwardenProvider() *BitwardenProvider { + path, err := exec.LookPath("bw") + provider := &BitwardenProvider{ + bwPath: path, + enabled: err == nil, + } + provider.newClient = provider.startServer + return provider +} + +// newBitwardenProviderWithClient builds a provider around an existing client. +// It exists for tests, which must not spawn the real CLI. +func newBitwardenProviderWithClient(client *bwClient) *BitwardenProvider { + provider := &BitwardenProvider{ + bwPath: "bw", + enabled: true, + } + provider.newClient = func() (*bwClient, *bwServer, error) { + return client, nil, nil + } + return provider +} + +// Name returns the human readable provider name. +func (p *BitwardenProvider) Name() string { return "Bitwarden" } + +// Scheme returns the reference scheme handled by this provider. +func (p *BitwardenProvider) Scheme() string { return SchemeBitwarden } + +// IsEnabled reports whether the Bitwarden CLI is installed. +func (p *BitwardenProvider) IsEnabled() bool { return p.enabled } + +// IsReference checks whether a value is a Bitwarden reference. +func (p *BitwardenProvider) IsReference(value string) bool { + return strings.HasPrefix(value, bitwardenPrefix) +} + +// Status returns the vault state: unlocked, locked or unauthenticated. +func (p *BitwardenProvider) Status() (string, error) { + client, err := p.ensureClient() + if err != nil { + return "", err + } + + status, err := client.status(context.Background()) + if err != nil { + return "", err + } + return status.Status, nil +} + +// IsAuthenticated reports whether the vault is unlocked and secrets can be +// read. Note that this may start the helper process, so it should not be called +// on a UI thread. +func (p *BitwardenProvider) IsAuthenticated() bool { + status, err := p.Status() + return err == nil && status == bwStatusUnlocked +} + +// GetAuthenticationInstructions explains how to make the vault available. +func (p *BitwardenProvider) GetAuthenticationInstructions() string { + return `Bitwarden CLI is installed but the vault is not available. + +MremoteGO reads secrets through the Bitwarden CLI, which needs an unlocked +vault. Unlock it once in a terminal and start MremoteGO from that same +terminal, so it inherits the session key. + +PowerShell: + bw config server https://your-server (self-hosted or Vaultwarden only) + bw login (once) + $env:BW_SESSION = bw unlock --raw + .\mremotego.exe + +bash / zsh: + bw config server https://your-server (self-hosted or Vaultwarden only) + bw login (once) + export BW_SESSION="$(bw unlock --raw)" + ./mremotego + +Run "bw status" to confirm the vault reports "unlocked". + +MremoteGO never asks for or stores your master password. It starts +"bw serve" on 127.0.0.1 with a random port and stops it on exit.` +} + +// ResolveSecret reads the field a reference points at. +func (p *BitwardenProvider) ResolveSecret(reference string) (string, error) { + id, field, err := parseBitwardenReference(reference) + if err != nil { + return "", err + } + + client, err := p.ensureClient() + if err != nil { + return "", err + } + + value, err := client.getField(context.Background(), field, id) + if err != nil { + return "", err + } + + if value == "" { + return "", fmt.Errorf("item %s has no %s", id, field) + } + + return value, nil +} + +// CreateItem stores a new login item and returns its "bw://" reference. +func (p *BitwardenProvider) CreateItem(req CreateItemRequest) (string, error) { + if req.Title == "" { + return "", fmt.Errorf("a title is required") + } + + client, err := p.ensureClient() + if err != nil { + return "", err + } + + login := &bwLogin{ + Username: req.Username, + Password: req.Password, + } + if req.URI != "" { + login.URIs = []bwURI{{URI: req.URI}} + } + + created, err := client.createItem(context.Background(), bwItem{ + Type: bwTypeLogin, + Name: req.Title, + Login: login, + }) + if err != nil { + return "", err + } + + return bitwardenReference(created.ID), nil +} + +// ListLoginItems returns the login items of the vault. Cards, identities and +// secure notes are skipped because they carry no connection credentials. +func (p *BitwardenProvider) ListLoginItems(search string) ([]BitwardenItem, error) { + client, err := p.ensureClient() + if err != nil { + return nil, err + } + + items, err := client.listItems(context.Background(), search) + if err != nil { + return nil, err + } + + var logins []BitwardenItem + for _, item := range items { + if item.Type != bwTypeLogin { + continue + } + + entry := BitwardenItem{ID: item.ID, Name: item.Name} + if item.Login != nil { + entry.Username = item.Login.Username + if len(item.Login.URIs) > 0 { + entry.URI = item.Login.URIs[0].URI + } + } + logins = append(logins, entry) + } + + return logins, nil +} + +// Sync pulls the latest vault contents from the server. The CLI serves items +// from a local cache, so items created elsewhere only appear after a sync. +func (p *BitwardenProvider) Sync() error { + client, err := p.ensureClient() + if err != nil { + return err + } + return client.sync(context.Background()) +} + +// Warmup starts the helper process in the background so that a later call from +// the UI thread does not have to wait for the CLI to boot. +func (p *BitwardenProvider) Warmup() { + if !p.enabled { + return + } + go func() { + _, _ = p.ensureClient() + }() +} + +// Close stops the helper process. +func (p *BitwardenProvider) Close() error { + p.mu.Lock() + server, client := p.server, p.client + p.server, p.client = nil, nil + p.mu.Unlock() + + if server != nil { + server.Stop() + } + _ = client + + return nil +} + +// ensureClient returns a client for a running server, starting one if needed. +func (p *BitwardenProvider) ensureClient() (*bwClient, error) { + if !p.enabled { + return nil, fmt.Errorf("Bitwarden CLI (bw) is not installed or not in PATH") + } + + p.mu.Lock() + defer p.mu.Unlock() + + // Restart if a previously started server has died. + if p.client != nil && (p.server == nil || !p.server.Exited()) { + return p.client, nil + } + if p.server != nil { + p.server.Stop() + p.server, p.client = nil, nil + } + + client, server, err := p.newClient() + if err != nil { + return nil, err + } + + p.client, p.server = client, server + return client, nil +} + +// startServer spawns "bw serve" and waits until it answers requests. +func (p *BitwardenProvider) startServer() (*bwClient, *bwServer, error) { + server, err := startBwServe(p.bwPath) + if err != nil { + return nil, nil, err + } + + client := newBwClient(server.baseURL) + if err := waitForReady(context.Background(), client, server.exited); err != nil { + server.Stop() + if tail := server.StderrTail(); tail != "" { + return nil, nil, fmt.Errorf("%w: %s", err, tail) + } + return nil, nil, err + } + + return client, server, nil +} diff --git a/internal/secrets/bitwarden_client.go b/internal/secrets/bitwarden_client.go new file mode 100644 index 0000000..75b9fc3 --- /dev/null +++ b/internal/secrets/bitwarden_client.go @@ -0,0 +1,256 @@ +package secrets + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "net/http" + "net/url" + "strings" + "time" +) + +// Errors returned by the Bitwarden client that callers may want to react to. +var ( + // ErrVaultLocked is returned when the vault is locked, so no secret can be + // read until the user unlocks it. + ErrVaultLocked = errors.New("bitwarden vault is locked") + // ErrNotAuthenticated is returned when no user is logged in. + ErrNotAuthenticated = errors.New("not logged in to bitwarden") + // ErrNotFound is returned when the requested item does not exist. + ErrNotFound = errors.New("item not found") +) + +// Vault states reported by the Bitwarden CLI. +const ( + bwStatusUnlocked = "unlocked" + bwStatusLocked = "locked" + bwStatusUnauthenticated = "unauthenticated" +) + +// Bitwarden item types; only logins carry credentials. +const bwTypeLogin = 1 + +const ( + bwRequestTimeout = 15 * time.Second + bwSyncTimeout = 60 * time.Second +) + +// bwClient talks to the local REST API exposed by "bw serve". +type bwClient struct { + baseURL string + http *http.Client +} + +func newBwClient(baseURL string) *bwClient { + return &bwClient{ + baseURL: strings.TrimSuffix(baseURL, "/"), + http: &http.Client{Timeout: bwSyncTimeout}, + } +} + +// bwEnvelope is the response wrapper used by every "bw serve" endpoint. +type bwEnvelope struct { + Success bool `json:"success"` + Data json.RawMessage `json:"data"` + Message string `json:"message"` +} + +type bwStatus struct { + ServerURL string `json:"serverUrl"` + UserEmail string `json:"userEmail"` + Status string `json:"status"` +} + +type bwURI struct { + URI string `json:"uri"` + Match *int `json:"match,omitempty"` +} + +type bwLogin struct { + Username string `json:"username,omitempty"` + Password string `json:"password,omitempty"` + Totp string `json:"totp,omitempty"` + URIs []bwURI `json:"uris,omitempty"` +} + +type bwItem struct { + ID string `json:"id,omitempty"` + Type int `json:"type"` + Name string `json:"name"` + Notes string `json:"notes,omitempty"` + Login *bwLogin `json:"login,omitempty"` + FolderID string `json:"folderId,omitempty"` +} + +// bwAPIError carries the message returned by the CLI so that it can be shown +// to the user unchanged. +type bwAPIError struct { + Message string +} + +func (e *bwAPIError) Error() string { return e.Message } + +// do performs a request and unwraps the response envelope. The Origin header is +// deliberately never set: "bw serve" rejects any request that carries one. +func (c *bwClient) do(ctx context.Context, method, path string, body any) (json.RawMessage, error) { + var reader *bytes.Reader + if body != nil { + encoded, err := json.Marshal(body) + if err != nil { + return nil, fmt.Errorf("failed to encode request: %w", err) + } + reader = bytes.NewReader(encoded) + } else { + reader = bytes.NewReader(nil) + } + + req, err := http.NewRequestWithContext(ctx, method, c.baseURL+path, reader) + if err != nil { + return nil, err + } + req.Header.Set("Accept", "application/json") + if body != nil { + req.Header.Set("Content-Type", "application/json") + } + + resp, err := c.http.Do(req) + if err != nil { + return nil, err + } + defer resp.Body.Close() + + var envelope bwEnvelope + if err := json.NewDecoder(resp.Body).Decode(&envelope); err != nil { + return nil, fmt.Errorf("unexpected response from bw serve (HTTP %d): %w", resp.StatusCode, err) + } + + if !envelope.Success { + return nil, classifyBwError(envelope.Message, resp.StatusCode) + } + + return envelope.Data, nil +} + +// classifyBwError maps CLI messages onto the sentinel errors above. +func classifyBwError(message string, statusCode int) error { + lower := strings.ToLower(message) + + switch { + case strings.Contains(lower, "vault is locked"): + return fmt.Errorf("%w", ErrVaultLocked) + case strings.Contains(lower, "not logged in"), strings.Contains(lower, "you are not logged in"): + return fmt.Errorf("%w", ErrNotAuthenticated) + case strings.Contains(lower, "not found"), statusCode == http.StatusNotFound: + if message == "" { + return ErrNotFound + } + return fmt.Errorf("%w: %s", ErrNotFound, message) + case message == "": + return fmt.Errorf("bw serve returned HTTP %d", statusCode) + } + + return &bwAPIError{Message: message} +} + +// status reports whether the vault is unlocked, locked or unauthenticated. +func (c *bwClient) status(ctx context.Context) (*bwStatus, error) { + ctx, cancel := context.WithTimeout(ctx, bwRequestTimeout) + defer cancel() + + data, err := c.do(ctx, http.MethodGet, "/status", nil) + if err != nil { + return nil, err + } + + // The payload is wrapped in a template object: + // {"object":"template","template":{"status":"unlocked",...}} + var wrapper struct { + Template bwStatus `json:"template"` + } + if err := json.Unmarshal(data, &wrapper); err != nil { + return nil, fmt.Errorf("failed to parse status: %w", err) + } + + return &wrapper.Template, nil +} + +// getField reads a single field of an item, e.g. its password. +func (c *bwClient) getField(ctx context.Context, field, id string) (string, error) { + ctx, cancel := context.WithTimeout(ctx, bwRequestTimeout) + defer cancel() + + path := "/object/" + url.PathEscape(field) + "/" + url.PathEscape(id) + data, err := c.do(ctx, http.MethodGet, path, nil) + if err != nil { + return "", err + } + + // Scalar fields are returned as {"object":"string","data":""}. + var wrapper struct { + Data string `json:"data"` + } + if err := json.Unmarshal(data, &wrapper); err != nil { + return "", fmt.Errorf("failed to parse %s: %w", field, err) + } + + return wrapper.Data, nil +} + +// listItems returns the vault items, optionally narrowed by a search term. +func (c *bwClient) listItems(ctx context.Context, search string) ([]bwItem, error) { + ctx, cancel := context.WithTimeout(ctx, bwRequestTimeout) + defer cancel() + + path := "/list/object/items" + if search != "" { + path += "?search=" + url.QueryEscape(search) + } + + data, err := c.do(ctx, http.MethodGet, path, nil) + if err != nil { + return nil, err + } + + var wrapper struct { + Data []bwItem `json:"data"` + } + if err := json.Unmarshal(data, &wrapper); err != nil { + return nil, fmt.Errorf("failed to parse item list: %w", err) + } + + return wrapper.Data, nil +} + +// createItem stores a new item and returns it including the assigned id. +func (c *bwClient) createItem(ctx context.Context, item bwItem) (*bwItem, error) { + ctx, cancel := context.WithTimeout(ctx, bwRequestTimeout) + defer cancel() + + data, err := c.do(ctx, http.MethodPost, "/object/item", item) + if err != nil { + return nil, err + } + + var created bwItem + if err := json.Unmarshal(data, &created); err != nil { + return nil, fmt.Errorf("failed to parse created item: %w", err) + } + + if created.ID == "" { + return nil, fmt.Errorf("bitwarden did not return an item id") + } + + return &created, nil +} + +// sync pulls the latest vault state from the server. +func (c *bwClient) sync(ctx context.Context) error { + ctx, cancel := context.WithTimeout(ctx, bwSyncTimeout) + defer cancel() + + _, err := c.do(ctx, http.MethodPost, "/sync", nil) + return err +} diff --git a/internal/secrets/bitwarden_process.go b/internal/secrets/bitwarden_process.go new file mode 100644 index 0000000..7e2baba --- /dev/null +++ b/internal/secrets/bitwarden_process.go @@ -0,0 +1,216 @@ +package secrets + +import ( + "context" + "errors" + "fmt" + "io" + "net" + "os" + "os/exec" + "strconv" + "strings" + "sync" + "time" +) + +const ( + // bwReadyTimeout bounds how long we wait for "bw serve" to accept requests. + // The CLI is a Node application and can take a few seconds to boot. + bwReadyTimeout = 20 * time.Second + // bwReadyPollInterval is how often readiness is probed. + bwReadyPollInterval = 150 * time.Millisecond + // bwStopGrace is how long Stop waits for the process to disappear. + bwStopGrace = 3 * time.Second + // bwStderrLimit caps the amount of stderr kept for diagnostics. + bwStderrLimit = 4096 +) + +// bwServer is a running "bw serve" child process. +type bwServer struct { + cmd *exec.Cmd + port int + baseURL string + stderr *boundedBuffer + exited chan struct{} + + stopOnce sync.Once +} + +// boundedBuffer keeps only the last n bytes written to it. The CLI can be +// chatty and only the tail is needed to explain a failed start. +type boundedBuffer struct { + mu sync.Mutex + limit int + data []byte +} + +func newBoundedBuffer(limit int) *boundedBuffer { + return &boundedBuffer{limit: limit} +} + +func (b *boundedBuffer) Write(p []byte) (int, error) { + b.mu.Lock() + defer b.mu.Unlock() + + b.data = append(b.data, p...) + if len(b.data) > b.limit { + b.data = b.data[len(b.data)-b.limit:] + } + return len(p), nil +} + +func (b *boundedBuffer) String() string { + b.mu.Lock() + defer b.mu.Unlock() + return strings.TrimSpace(string(b.data)) +} + +// findFreePort asks the operating system for an unused loopback port. +func findFreePort() (int, error) { + listener, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + return 0, err + } + defer listener.Close() + + addr, ok := listener.Addr().(*net.TCPAddr) + if !ok { + return 0, fmt.Errorf("unexpected listener address type %T", listener.Addr()) + } + return addr.Port, nil +} + +// startBwServe launches "bw serve" bound to loopback on a free port. +// +// The child inherits the environment, which is how BW_SESSION reaches it: the +// user unlocks the vault once in their shell and every request made through +// this server is then authorised. +func startBwServe(bwPath string) (*bwServer, error) { + port, err := findFreePort() + if err != nil { + return nil, fmt.Errorf("failed to reserve a local port: %w", err) + } + + args := []string{"serve", "--hostname", "127.0.0.1", "--port", strconv.Itoa(port)} + + var cmd *exec.Cmd + if isBatchFile(bwPath) { + // The npm distribution installs bw as a .cmd shim, which cannot be + // executed directly. + cmd = exec.Command("cmd", append([]string{"/c", bwPath}, args...)...) + } else { + cmd = exec.Command(bwPath, args...) + } + + cmd.Env = os.Environ() + cmd.Stdout = io.Discard + stderr := newBoundedBuffer(bwStderrLimit) + cmd.Stderr = stderr + hideConsoleWindow(cmd) + configureChildProcess(cmd) + + if err := cmd.Start(); err != nil { + return nil, fmt.Errorf("failed to start bw serve: %w", err) + } + + server := &bwServer{ + cmd: cmd, + port: port, + baseURL: fmt.Sprintf("http://127.0.0.1:%d", port), + stderr: stderr, + exited: make(chan struct{}), + } + + // Reap the child and let readiness polling notice an early exit. + go func() { + _ = cmd.Wait() + close(server.exited) + }() + + adoptChildProcess(cmd) + + return server, nil +} + +func isBatchFile(path string) bool { + lower := strings.ToLower(path) + return strings.HasSuffix(lower, ".cmd") || strings.HasSuffix(lower, ".bat") +} + +// Stop terminates the server. It is safe to call more than once. +func (s *bwServer) Stop() { + if s == nil { + return + } + + s.stopOnce.Do(func() { + if s.cmd.Process == nil { + return + } + + stopProcess(s.cmd) + + select { + case <-s.exited: + case <-time.After(bwStopGrace): + _ = s.cmd.Process.Kill() + } + }) +} + +// Exited reports whether the server process has already terminated. +func (s *bwServer) Exited() bool { + select { + case <-s.exited: + return true + default: + return false + } +} + +// StderrTail returns the tail of the stderr produced by the server, so that a +// failed start can be explained to the user. +func (s *bwServer) StderrTail() string { + if s == nil || s.stderr == nil { + return "" + } + return s.stderr.String() +} + +// waitForReady polls the status endpoint until the server answers, the process +// exits or the deadline passes. A locked or unauthenticated vault still counts +// as ready: the server is up, it simply has nothing to hand out yet. +func waitForReady(ctx context.Context, client *bwClient, exited <-chan struct{}) error { + ctx, cancel := context.WithTimeout(ctx, bwReadyTimeout) + defer cancel() + + ticker := time.NewTicker(bwReadyPollInterval) + defer ticker.Stop() + + var lastErr error + for { + _, err := client.status(ctx) + if err == nil || isVaultStateError(err) { + return nil + } + lastErr = err + + select { + case <-exited: + return fmt.Errorf("bw serve exited before it became ready") + case <-ctx.Done(): + if lastErr != nil { + return fmt.Errorf("bw serve did not become ready: %w", lastErr) + } + return fmt.Errorf("bw serve did not become ready within %s", bwReadyTimeout) + case <-ticker.C: + } + } +} + +// isVaultStateError reports whether err means the server works but the vault is +// not usable, as opposed to the server not being up yet. +func isVaultStateError(err error) bool { + return errors.Is(err, ErrVaultLocked) || errors.Is(err, ErrNotAuthenticated) +} diff --git a/internal/secrets/bitwarden_process_linux.go b/internal/secrets/bitwarden_process_linux.go new file mode 100644 index 0000000..c930712 --- /dev/null +++ b/internal/secrets/bitwarden_process_linux.go @@ -0,0 +1,17 @@ +//go:build linux + +package secrets + +import ( + "os/exec" + "syscall" +) + +// setParentDeathSignal makes the kernel send SIGTERM to the child when this +// process exits for any reason. +func setParentDeathSignal(cmd *exec.Cmd) { + if cmd.SysProcAttr == nil { + cmd.SysProcAttr = &syscall.SysProcAttr{} + } + cmd.SysProcAttr.Pdeathsig = syscall.SIGTERM +} diff --git a/internal/secrets/bitwarden_process_other.go b/internal/secrets/bitwarden_process_other.go new file mode 100644 index 0000000..c4802bf --- /dev/null +++ b/internal/secrets/bitwarden_process_other.go @@ -0,0 +1,9 @@ +//go:build !windows && !linux + +package secrets + +import "os/exec" + +// setParentDeathSignal is not available on this platform. The server is still +// stopped on a clean exit; only a hard crash can leave it running. +func setParentDeathSignal(cmd *exec.Cmd) {} diff --git a/internal/secrets/bitwarden_process_unix.go b/internal/secrets/bitwarden_process_unix.go new file mode 100644 index 0000000..51e80f4 --- /dev/null +++ b/internal/secrets/bitwarden_process_unix.go @@ -0,0 +1,28 @@ +//go:build !windows + +package secrets + +import ( + "os/exec" + "syscall" +) + +// configureChildProcess asks the kernel to signal the child when this process +// dies, so a crash cannot leave an orphaned "bw serve" running. +func configureChildProcess(cmd *exec.Cmd) { + setParentDeathSignal(cmd) +} + +// adoptChildProcess has nothing to do on Unix; the death signal is configured +// before the process starts. +func adoptChildProcess(cmd *exec.Cmd) {} + +// stopProcess asks the child to terminate. +func stopProcess(cmd *exec.Cmd) { + if cmd.Process == nil { + return + } + if err := cmd.Process.Signal(syscall.SIGTERM); err != nil { + _ = cmd.Process.Kill() + } +} diff --git a/internal/secrets/bitwarden_process_windows.go b/internal/secrets/bitwarden_process_windows.go new file mode 100644 index 0000000..a85d420 --- /dev/null +++ b/internal/secrets/bitwarden_process_windows.go @@ -0,0 +1,81 @@ +//go:build windows + +package secrets + +import ( + "os/exec" + "sync" + "unsafe" + + "golang.org/x/sys/windows" +) + +var ( + jobOnce sync.Once + jobHandle windows.Handle +) + +// killOnCloseJob returns a job object configured to kill its members once the +// last handle to it is closed, which happens when this process dies. That way a +// crash cannot leave an orphaned "bw serve" listening with an unlocked vault. +func killOnCloseJob() windows.Handle { + jobOnce.Do(func() { + handle, err := windows.CreateJobObject(nil, nil) + if err != nil { + return + } + + info := windows.JOBOBJECT_EXTENDED_LIMIT_INFORMATION{ + BasicLimitInformation: windows.JOBOBJECT_BASIC_LIMIT_INFORMATION{ + LimitFlags: windows.JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, + }, + } + if _, err := windows.SetInformationJobObject( + handle, + windows.JobObjectExtendedLimitInformation, + uintptr(unsafe.Pointer(&info)), + uint32(unsafe.Sizeof(info)), + ); err != nil { + _ = windows.CloseHandle(handle) + return + } + + jobHandle = handle + }) + + return jobHandle +} + +// configureChildProcess is a no-op on Windows; the console window is already +// hidden by hideConsoleWindow. +func configureChildProcess(cmd *exec.Cmd) {} + +// adoptChildProcess assigns the started child to the kill-on-close job object. +// Failures are ignored: losing this safety net is not a reason to refuse the +// connection, and Stop still terminates the process on a clean exit. +func adoptChildProcess(cmd *exec.Cmd) { + job := killOnCloseJob() + if job == 0 || cmd.Process == nil { + return + } + + handle, err := windows.OpenProcess( + windows.PROCESS_SET_QUOTA|windows.PROCESS_TERMINATE, + false, + uint32(cmd.Process.Pid), + ) + if err != nil { + return + } + defer windows.CloseHandle(handle) + + _ = windows.AssignProcessToJobObject(job, handle) +} + +// stopProcess terminates the child. Windows offers no graceful signal for a +// console-less child process, so it is killed outright. +func stopProcess(cmd *exec.Cmd) { + if cmd.Process != nil { + _ = cmd.Process.Kill() + } +} diff --git a/internal/secrets/bitwarden_reference.go b/internal/secrets/bitwarden_reference.go new file mode 100644 index 0000000..f2bdf09 --- /dev/null +++ b/internal/secrets/bitwarden_reference.go @@ -0,0 +1,62 @@ +package secrets + +import ( + "fmt" + "strings" +) + +const bitwardenPrefix = SchemeBitwarden + "://" + +// Fields of a Bitwarden login item that a reference may point at. +const ( + bitwardenFieldPassword = "password" + bitwardenFieldUsername = "username" + bitwardenFieldTotp = "totp" + bitwardenFieldNotes = "notes" +) + +var bitwardenFields = map[string]bool{ + bitwardenFieldPassword: true, + bitwardenFieldUsername: true, + bitwardenFieldTotp: true, + bitwardenFieldNotes: true, +} + +// parseBitwardenReference splits a reference into the item id and the field to +// read. Accepted formats: +// +// bw:// -> the item's password +// bw:/// -> password, username, totp or notes +func parseBitwardenReference(reference string) (id string, field string, err error) { + if !strings.HasPrefix(reference, bitwardenPrefix) { + return "", "", fmt.Errorf("reference must start with %s", bitwardenPrefix) + } + + rest := strings.TrimPrefix(reference, bitwardenPrefix) + parts := strings.Split(rest, "/") + + switch len(parts) { + case 1: + id, field = parts[0], bitwardenFieldPassword + case 2: + id, field = parts[0], parts[1] + default: + return "", "", fmt.Errorf("reference must be in format %s[/]", bitwardenPrefix) + } + + if id == "" { + return "", "", fmt.Errorf("reference is missing an item id") + } + + if !bitwardenFields[field] { + return "", "", fmt.Errorf("unsupported field %q, expected one of password, username, totp, notes", field) + } + + return id, field, nil +} + +// bitwardenReference builds the reference stored in the configuration for an +// item id. +func bitwardenReference(id string) string { + return bitwardenPrefix + id +} diff --git a/internal/secrets/bitwarden_reference_test.go b/internal/secrets/bitwarden_reference_test.go new file mode 100644 index 0000000..768cded --- /dev/null +++ b/internal/secrets/bitwarden_reference_test.go @@ -0,0 +1,61 @@ +package secrets + +import "testing" + +func TestParseBitwardenReference(t *testing.T) { + tests := []struct { + reference string + wantID string + wantField string + wantErr bool + }{ + {reference: "bw://abc-123", wantID: "abc-123", wantField: "password"}, + {reference: "bw://abc-123/password", wantID: "abc-123", wantField: "password"}, + {reference: "bw://abc-123/username", wantID: "abc-123", wantField: "username"}, + {reference: "bw://abc-123/totp", wantID: "abc-123", wantField: "totp"}, + {reference: "bw://abc-123/notes", wantID: "abc-123", wantField: "notes"}, + {reference: "bw://abc-123/secret", wantErr: true}, + {reference: "bw://abc-123/", wantErr: true}, + {reference: "bw://abc-123/password/extra", wantErr: true}, + {reference: "bw://", wantErr: true}, + {reference: "op://Private/Server/password", wantErr: true}, + {reference: "hunter2", wantErr: true}, + {reference: "", wantErr: true}, + } + + for _, tt := range tests { + id, field, err := parseBitwardenReference(tt.reference) + + if tt.wantErr { + if err == nil { + t.Errorf("parseBitwardenReference(%q) = (%q, %q), want an error", tt.reference, id, field) + } + continue + } + + if err != nil { + t.Errorf("parseBitwardenReference(%q) returned error: %v", tt.reference, err) + continue + } + if id != tt.wantID || field != tt.wantField { + t.Errorf("parseBitwardenReference(%q) = (%q, %q), want (%q, %q)", + tt.reference, id, field, tt.wantID, tt.wantField) + } + } +} + +func TestBitwardenReference(t *testing.T) { + if got := bitwardenReference("abc-123"); got != "bw://abc-123" { + t.Errorf("bitwardenReference = %q, want %q", got, "bw://abc-123") + } +} + +func TestBitwardenReferenceRoundTrip(t *testing.T) { + id, field, err := parseBitwardenReference(bitwardenReference("abc-123")) + if err != nil { + t.Fatalf("round trip returned error: %v", err) + } + if id != "abc-123" || field != bitwardenFieldPassword { + t.Errorf("round trip = (%q, %q), want (abc-123, password)", id, field) + } +} diff --git a/internal/secrets/bitwarden_test.go b/internal/secrets/bitwarden_test.go new file mode 100644 index 0000000..b607b85 --- /dev/null +++ b/internal/secrets/bitwarden_test.go @@ -0,0 +1,477 @@ +package secrets + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" +) + +// fakeBw is a stand-in for "bw serve" so the tests never touch the real CLI. +type fakeBw struct { + server *httptest.Server + + state string + items map[string]bwItem + lastCreated bwItem + sawOrigin bool + lastSearch string + syncCalls int +} + +func newFakeBw(t *testing.T) *fakeBw { + t.Helper() + + f := &fakeBw{ + state: bwStatusUnlocked, + items: make(map[string]bwItem), + } + + mux := http.NewServeMux() + + mux.HandleFunc("/status", func(w http.ResponseWriter, r *http.Request) { + f.note(r) + writeEnvelope(w, map[string]any{ + "object": "template", + "template": map[string]string{ + "serverUrl": "https://vault.example.com", + "userEmail": "user@example.com", + "status": f.state, + }, + }) + }) + + mux.HandleFunc("/sync", func(w http.ResponseWriter, r *http.Request) { + f.note(r) + if f.locked(w) { + return + } + f.syncCalls++ + writeEnvelope(w, map[string]string{"object": "message", "title": "Syncing complete."}) + }) + + mux.HandleFunc("/list/object/items", func(w http.ResponseWriter, r *http.Request) { + f.note(r) + if f.locked(w) { + return + } + f.lastSearch = r.URL.Query().Get("search") + + list := make([]bwItem, 0, len(f.items)) + for _, item := range f.items { + list = append(list, item) + } + writeEnvelope(w, map[string]any{"object": "list", "data": list}) + }) + + mux.HandleFunc("/object/", func(w http.ResponseWriter, r *http.Request) { + f.note(r) + if f.locked(w) { + return + } + + if r.Method == http.MethodPost { + var item bwItem + if err := json.NewDecoder(r.Body).Decode(&item); err != nil { + writeError(w, http.StatusBadRequest, "bad request body") + return + } + item.ID = "created-id" + f.lastCreated = item + f.items[item.ID] = item + writeEnvelope(w, item) + return + } + + // /object// + parts := strings.Split(strings.Trim(r.URL.Path, "/"), "/") + if len(parts) != 3 { + writeError(w, http.StatusNotFound, "Not found.") + return + } + kind, id := parts[1], parts[2] + + item, ok := f.items[id] + if !ok { + writeError(w, http.StatusNotFound, "Not found.") + return + } + + if kind == "item" { + writeEnvelope(w, item) + return + } + + var value string + if item.Login != nil { + switch kind { + case bitwardenFieldPassword: + value = item.Login.Password + case bitwardenFieldUsername: + value = item.Login.Username + case bitwardenFieldTotp: + value = item.Login.Totp + } + } + if kind == bitwardenFieldNotes { + value = item.Notes + } + + writeEnvelope(w, map[string]string{"object": "string", "data": value}) + }) + + f.server = httptest.NewServer(mux) + t.Cleanup(f.server.Close) + return f +} + +func (f *fakeBw) note(r *http.Request) { + if r.Header.Get("Origin") != "" { + f.sawOrigin = true + } +} + +// locked mimics the CLI, which rejects vault access while locked. +func (f *fakeBw) locked(w http.ResponseWriter) bool { + switch f.state { + case bwStatusLocked: + writeError(w, http.StatusOK, "Vault is locked.") + return true + case bwStatusUnauthenticated: + writeError(w, http.StatusOK, "You are not logged in.") + return true + } + return false +} + +func (f *fakeBw) provider() *BitwardenProvider { + return newBitwardenProviderWithClient(newBwClient(f.server.URL)) +} + +func (f *fakeBw) addLogin(id, name, username, password string) { + f.items[id] = bwItem{ + ID: id, + Type: bwTypeLogin, + Name: name, + Login: &bwLogin{Username: username, Password: password}, + } +} + +func writeEnvelope(w http.ResponseWriter, data any) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{"success": true, "data": data}) +} + +func writeError(w http.ResponseWriter, status int, message string) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + _ = json.NewEncoder(w).Encode(map[string]any{"success": false, "message": message}) +} + +func TestBitwardenResolvePassword(t *testing.T) { + fake := newFakeBw(t) + fake.addLogin("abc-123", "Web Server", "admin", "hunter2") + + got, err := fake.provider().ResolveSecret("bw://abc-123") + if err != nil { + t.Fatalf("ResolveSecret returned error: %v", err) + } + if got != "hunter2" { + t.Errorf("ResolveSecret = %q, want %q", got, "hunter2") + } +} + +func TestBitwardenResolveExplicitField(t *testing.T) { + fake := newFakeBw(t) + fake.addLogin("abc-123", "Web Server", "admin", "hunter2") + + got, err := fake.provider().ResolveSecret("bw://abc-123/username") + if err != nil { + t.Fatalf("ResolveSecret returned error: %v", err) + } + if got != "admin" { + t.Errorf("ResolveSecret = %q, want %q", got, "admin") + } +} + +func TestBitwardenResolveLockedVault(t *testing.T) { + fake := newFakeBw(t) + fake.addLogin("abc-123", "Web Server", "admin", "hunter2") + fake.state = bwStatusLocked + + _, err := fake.provider().ResolveSecret("bw://abc-123") + if !errors.Is(err, ErrVaultLocked) { + t.Fatalf("ResolveSecret error = %v, want ErrVaultLocked", err) + } +} + +func TestBitwardenResolveUnauthenticated(t *testing.T) { + fake := newFakeBw(t) + fake.state = bwStatusUnauthenticated + + _, err := fake.provider().ResolveSecret("bw://abc-123") + if !errors.Is(err, ErrNotAuthenticated) { + t.Fatalf("ResolveSecret error = %v, want ErrNotAuthenticated", err) + } +} + +func TestBitwardenResolveUnknownItem(t *testing.T) { + fake := newFakeBw(t) + + _, err := fake.provider().ResolveSecret("bw://missing") + if !errors.Is(err, ErrNotFound) { + t.Fatalf("ResolveSecret error = %v, want ErrNotFound", err) + } +} + +func TestBitwardenIsAuthenticated(t *testing.T) { + tests := map[string]bool{ + bwStatusUnlocked: true, + bwStatusLocked: false, + bwStatusUnauthenticated: false, + } + + for state, want := range tests { + fake := newFakeBw(t) + fake.state = state + + if got := fake.provider().IsAuthenticated(); got != want { + t.Errorf("IsAuthenticated with status %q = %v, want %v", state, got, want) + } + } +} + +func TestBitwardenCreateItem(t *testing.T) { + fake := newFakeBw(t) + + reference, err := fake.provider().CreateItem(CreateItemRequest{ + Title: "Web Server", + Username: "admin", + Password: "hunter2", + URI: "ssh://web.example.com", + }) + if err != nil { + t.Fatalf("CreateItem returned error: %v", err) + } + if reference != "bw://created-id" { + t.Errorf("CreateItem = %q, want %q", reference, "bw://created-id") + } + + created := fake.lastCreated + if created.Type != bwTypeLogin { + t.Errorf("created item type = %d, want %d", created.Type, bwTypeLogin) + } + if created.Name != "Web Server" { + t.Errorf("created item name = %q, want %q", created.Name, "Web Server") + } + if created.Login == nil || created.Login.Password != "hunter2" || created.Login.Username != "admin" { + t.Fatalf("created item login = %+v, want admin/hunter2", created.Login) + } + if len(created.Login.URIs) != 1 || created.Login.URIs[0].URI != "ssh://web.example.com" { + t.Errorf("created item URIs = %+v, want one ssh:// entry", created.Login.URIs) + } +} + +func TestBitwardenCreateItemRequiresTitle(t *testing.T) { + fake := newFakeBw(t) + + if _, err := fake.provider().CreateItem(CreateItemRequest{Password: "hunter2"}); err == nil { + t.Error("expected CreateItem to reject an empty title") + } +} + +func TestBitwardenListLoginItemsSkipsOtherTypes(t *testing.T) { + fake := newFakeBw(t) + fake.addLogin("login-1", "Web Server", "admin", "hunter2") + fake.items["note-1"] = bwItem{ID: "note-1", Type: 2, Name: "Secure Note"} + + items, err := fake.provider().ListLoginItems("") + if err != nil { + t.Fatalf("ListLoginItems returned error: %v", err) + } + if len(items) != 1 { + t.Fatalf("ListLoginItems returned %d items, want 1", len(items)) + } + if items[0].ID != "login-1" || items[0].Username != "admin" { + t.Errorf("ListLoginItems = %+v, want the login item", items[0]) + } +} + +func TestBitwardenListLoginItemsPassesSearch(t *testing.T) { + fake := newFakeBw(t) + + if _, err := fake.provider().ListLoginItems("web server"); err != nil { + t.Fatalf("ListLoginItems returned error: %v", err) + } + if fake.lastSearch != "web server" { + t.Errorf("search term = %q, want %q", fake.lastSearch, "web server") + } +} + +func TestBitwardenSync(t *testing.T) { + fake := newFakeBw(t) + + if err := fake.provider().Sync(); err != nil { + t.Fatalf("Sync returned error: %v", err) + } + if fake.syncCalls != 1 { + t.Errorf("sync called %d times, want 1", fake.syncCalls) + } +} + +// bw serve rejects any request carrying an Origin header, so the client must +// never send one. +func TestBitwardenClientSendsNoOriginHeader(t *testing.T) { + fake := newFakeBw(t) + fake.addLogin("abc-123", "Web Server", "admin", "hunter2") + + provider := fake.provider() + if _, err := provider.ResolveSecret("bw://abc-123"); err != nil { + t.Fatalf("ResolveSecret returned error: %v", err) + } + if _, err := provider.ListLoginItems(""); err != nil { + t.Fatalf("ListLoginItems returned error: %v", err) + } + + if fake.sawOrigin { + t.Error("client sent an Origin header, which bw serve would reject") + } +} + +func TestBitwardenDisabledProviderReportsMissingCLI(t *testing.T) { + provider := &BitwardenProvider{enabled: false} + + _, err := provider.ResolveSecret("bw://abc-123") + if err == nil { + t.Fatal("expected an error when the CLI is missing") + } + if !strings.Contains(err.Error(), "not installed") { + t.Errorf("error = %q, want it to mention the missing CLI", err) + } + + if provider.IsAuthenticated() { + t.Error("a disabled provider must not report itself as authenticated") + } +} + +func TestBitwardenIsReference(t *testing.T) { + provider := &BitwardenProvider{} + + tests := map[string]bool{ + "bw://abc": true, + "bw://abc/username": true, + "op://Private/Server/password": false, + "hunter2": false, + "": false, + } + + for value, want := range tests { + if got := provider.IsReference(value); got != want { + t.Errorf("IsReference(%q) = %v, want %v", value, got, want) + } + } +} + +func TestBitwardenResolveRejectsForeignReference(t *testing.T) { + fake := newFakeBw(t) + + if _, err := fake.provider().ResolveSecret("op://Private/Server/password"); err == nil { + t.Error("expected a 1Password reference to be rejected") + } +} + +func TestBitwardenStatusReported(t *testing.T) { + fake := newFakeBw(t) + fake.state = bwStatusLocked + + status, err := fake.provider().Status() + if err != nil { + t.Fatalf("Status returned error: %v", err) + } + if status != bwStatusLocked { + t.Errorf("Status = %q, want %q", status, bwStatusLocked) + } +} + +func TestWaitForReadyAcceptsLockedVault(t *testing.T) { + fake := newFakeBw(t) + fake.state = bwStatusLocked + + // A locked vault still means the server is up and usable once unlocked. + if err := waitForReady(context.Background(), newBwClient(fake.server.URL), make(chan struct{})); err != nil { + t.Errorf("waitForReady returned error for a locked vault: %v", err) + } +} + +func TestWaitForReadyDetectsExit(t *testing.T) { + // A server that never answers, plus an already exited process. + dead := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Error(w, "boom", http.StatusInternalServerError) + })) + defer dead.Close() + + exited := make(chan struct{}) + close(exited) + + err := waitForReady(context.Background(), newBwClient(dead.URL), exited) + if err == nil { + t.Fatal("expected an error when the process has exited") + } + if !strings.Contains(err.Error(), "exited") { + t.Errorf("error = %q, want it to mention the exit", err) + } +} + +func TestWaitForReadyTimesOut(t *testing.T) { + dead := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Error(w, "boom", http.StatusInternalServerError) + })) + defer dead.Close() + + ctx, cancel := context.WithTimeout(context.Background(), 400*time.Millisecond) + defer cancel() + + if err := waitForReady(ctx, newBwClient(dead.URL), make(chan struct{})); err == nil { + t.Fatal("expected waitForReady to give up") + } +} + +func TestFindFreePort(t *testing.T) { + port, err := findFreePort() + if err != nil { + t.Fatalf("findFreePort returned error: %v", err) + } + if port <= 0 || port > 65535 { + t.Errorf("findFreePort = %d, want a valid TCP port", port) + } +} + +func TestBoundedBufferKeepsTail(t *testing.T) { + buf := newBoundedBuffer(8) + if _, err := buf.Write([]byte("abcdefghijkl")); err != nil { + t.Fatalf("Write returned error: %v", err) + } + if got := buf.String(); got != "efghijkl" { + t.Errorf("String = %q, want %q", got, "efghijkl") + } +} + +func TestIsBatchFile(t *testing.T) { + tests := map[string]bool{ + `C:\npm\bw.cmd`: true, + `C:\npm\bw.CMD`: true, + `C:\tools\bw.bat`: true, + `C:\tools\bw.exe`: false, + "/usr/bin/bw": false, + } + + for path, want := range tests { + if got := isBatchFile(path); got != want { + t.Errorf("isBatchFile(%q) = %v, want %v", path, got, want) + } + } +} diff --git a/internal/secrets/onepassword.go b/internal/secrets/onepassword.go index 8b1ee06..10af8e4 100644 --- a/internal/secrets/onepassword.go +++ b/internal/secrets/onepassword.go @@ -12,6 +12,17 @@ type OnePasswordProvider struct { enabled bool } +var ( + _ Provider = (*OnePasswordProvider)(nil) + _ ItemCreator = (*OnePasswordProvider)(nil) +) + +// Name returns the human readable provider name. +func (p *OnePasswordProvider) Name() string { return "1Password" } + +// Scheme returns the reference scheme handled by this provider. +func (p *OnePasswordProvider) Scheme() string { return SchemeOnePassword } + // NewOnePasswordProvider creates a new 1Password provider func NewOnePasswordProvider() *OnePasswordProvider { return &OnePasswordProvider{ @@ -193,7 +204,11 @@ func (p *OnePasswordProvider) CheckItemExists(vault, title string) (string, bool // CreateItem creates a new Login item in 1Password // Returns the 1Password reference (op://vault/title/password) -func (p *OnePasswordProvider) CreateItem(vault, title, username, password string) (string, error) { +// The URI field of the request is ignored; 1Password items created here are +// plain login items. +func (p *OnePasswordProvider) CreateItem(req CreateItemRequest) (string, error) { + vault, title, username, password := req.Vault, req.Title, req.Username, req.Password + if !p.enabled { return "", fmt.Errorf("1Password CLI is not available") } diff --git a/internal/secrets/provider.go b/internal/secrets/provider.go new file mode 100644 index 0000000..8c611f4 --- /dev/null +++ b/internal/secrets/provider.go @@ -0,0 +1,76 @@ +package secrets + +import "strings" + +// Scheme identifiers for the supported secret providers. A connection password +// that starts with "://" is a reference and is resolved at launch time +// instead of being stored in the configuration file. +const ( + SchemeOnePassword = "op" + SchemeBitwarden = "bw" +) + +// knownSchemes lists every scheme understood by this package. It is kept as a +// package level variable (rather than derived from a Registry) so that callers +// which must not instantiate providers - such as the crypto package deciding +// whether a value needs encrypting - can still recognise a reference. +var knownSchemes = []string{SchemeOnePassword, SchemeBitwarden} + +// Provider resolves secret references belonging to a single scheme. +type Provider interface { + // Name returns a human readable provider name, e.g. "1Password". + Name() string + + // Scheme returns the URL scheme handled by this provider, e.g. "op". + Scheme() string + + // IsEnabled reports whether the backing tool is available on this machine. + IsEnabled() bool + + // IsAuthenticated reports whether secrets can currently be retrieved. + IsAuthenticated() bool + + // GetAuthenticationInstructions returns user facing text explaining how to + // authenticate when IsAuthenticated returns false. + GetAuthenticationInstructions() string + + // IsReference reports whether value is a reference for this provider. + IsReference(value string) bool + + // ResolveSecret retrieves the secret a reference points at. + ResolveSecret(reference string) (string, error) +} + +// CreateItemRequest is the provider agnostic input for creating a login item. +// Fields that a provider does not support are ignored by that provider. +type CreateItemRequest struct { + Vault string // 1Password vault name; ignored by Bitwarden + Title string + Username string + Password string + URI string // e.g. "ssh://host"; ignored by 1Password +} + +// ItemCreator is implemented by providers able to store new login items. +type ItemCreator interface { + // CreateItem stores a login item and returns a reference to it. + CreateItem(req CreateItemRequest) (reference string, err error) +} + +// KnownSchemes returns the schemes understood by this package. +func KnownSchemes() []string { + out := make([]string, len(knownSchemes)) + copy(out, knownSchemes) + return out +} + +// IsKnownReference reports whether value uses any known scheme prefix. It does +// not require a provider to be installed or authenticated. +func IsKnownReference(value string) bool { + for _, scheme := range knownSchemes { + if strings.HasPrefix(value, scheme+"://") { + return true + } + } + return false +} diff --git a/internal/secrets/registry.go b/internal/secrets/registry.go new file mode 100644 index 0000000..9f09e9f --- /dev/null +++ b/internal/secrets/registry.go @@ -0,0 +1,111 @@ +package secrets + +import ( + "fmt" + "io" + "sync" +) + +// Registry holds the configured secret providers and routes references to the +// provider that understands them. +type Registry struct { + providers []Provider +} + +// NewRegistry creates a registry from the given providers. +func NewRegistry(providers ...Provider) *Registry { + return &Registry{providers: providers} +} + +// Providers returns every registered provider. +func (r *Registry) Providers() []Provider { + return r.providers +} + +// ByScheme returns the provider registered for a scheme such as "op" or "bw". +func (r *Registry) ByScheme(scheme string) (Provider, bool) { + for _, p := range r.providers { + if p.Scheme() == scheme { + return p, true + } + } + return nil, false +} + +// ProviderFor returns the provider that owns the given reference. +func (r *Registry) ProviderFor(value string) (Provider, bool) { + for _, p := range r.providers { + if p.IsReference(value) { + return p, true + } + } + return nil, false +} + +// IsReference reports whether value is a reference for any registered provider. +func (r *Registry) IsReference(value string) bool { + _, ok := r.ProviderFor(value) + return ok +} + +// Resolve resolves a reference. Values that are not references are returned +// unchanged, which lets callers pass plain passwords through. +func (r *Registry) Resolve(value string) (string, error) { + provider, ok := r.ProviderFor(value) + if !ok { + return value, nil + } + + secret, err := provider.ResolveSecret(value) + if err != nil { + return "", fmt.Errorf("%s: %w", provider.Name(), err) + } + return secret, nil +} + +// Close releases resources held by providers, such as helper processes. +func (r *Registry) Close() { + for _, p := range r.providers { + if closer, ok := p.(io.Closer); ok { + _ = closer.Close() + } + } +} + +var ( + defaultOnce sync.Once + defaultMu sync.Mutex + defaultRegistry *Registry +) + +// Default returns the process wide registry. Providers are shared because some +// of them own a helper process (see BitwardenProvider) that must exist only +// once per application run. +func Default() *Registry { + defaultOnce.Do(func() { + registry := NewRegistry( + NewOnePasswordProvider(), + NewBitwardenProvider(), + ) + defaultMu.Lock() + defaultRegistry = registry + defaultMu.Unlock() + }) + + defaultMu.Lock() + defer defaultMu.Unlock() + return defaultRegistry +} + +// Shutdown closes the default registry if it was ever created. It is safe to +// call multiple times and must be called before the process exits so that +// helper processes are terminated. +func Shutdown() { + defaultMu.Lock() + registry := defaultRegistry + defaultMu.Unlock() + + if registry != nil { + registry.Close() + } +} diff --git a/internal/secrets/registry_test.go b/internal/secrets/registry_test.go new file mode 100644 index 0000000..99aa2c8 --- /dev/null +++ b/internal/secrets/registry_test.go @@ -0,0 +1,154 @@ +package secrets + +import ( + "errors" + "strings" + "testing" +) + +// fakeProvider is a Provider that resolves references from a static map. +type fakeProvider struct { + name string + scheme string + enabled bool + authed bool + values map[string]string + resolveErr error +} + +func (f *fakeProvider) Name() string { return f.name } +func (f *fakeProvider) Scheme() string { return f.scheme } +func (f *fakeProvider) IsEnabled() bool { + return f.enabled +} +func (f *fakeProvider) IsAuthenticated() bool { return f.authed } +func (f *fakeProvider) GetAuthenticationInstructions() string { return "sign in to " + f.name } +func (f *fakeProvider) IsReference(value string) bool { + return strings.HasPrefix(value, f.scheme+"://") +} +func (f *fakeProvider) ResolveSecret(reference string) (string, error) { + if f.resolveErr != nil { + return "", f.resolveErr + } + value, ok := f.values[reference] + if !ok { + return "", errors.New("not found") + } + return value, nil +} + +func newTestRegistry() (*Registry, *fakeProvider, *fakeProvider) { + op := &fakeProvider{ + name: "1Password", scheme: SchemeOnePassword, enabled: true, authed: true, + values: map[string]string{"op://Private/Server/password": "op-secret"}, + } + bw := &fakeProvider{ + name: "Bitwarden", scheme: SchemeBitwarden, enabled: true, authed: true, + values: map[string]string{"bw://item-id": "bw-secret"}, + } + return NewRegistry(op, bw), op, bw +} + +func TestRegistryProviderFor(t *testing.T) { + registry, op, bw := newTestRegistry() + + tests := []struct { + value string + want Provider + }{ + {"op://Private/Server/password", op}, + {"bw://item-id", bw}, + {"plain-password", nil}, + {"", nil}, + {"enc:AAAA", nil}, + } + + for _, tt := range tests { + got, ok := registry.ProviderFor(tt.value) + if tt.want == nil { + if ok { + t.Errorf("ProviderFor(%q) = %s, want no provider", tt.value, got.Name()) + } + continue + } + if !ok || got != tt.want { + t.Errorf("ProviderFor(%q) = %v, want %s", tt.value, got, tt.want.Name()) + } + } +} + +func TestRegistryResolve(t *testing.T) { + registry, _, _ := newTestRegistry() + + got, err := registry.Resolve("bw://item-id") + if err != nil { + t.Fatalf("Resolve returned error: %v", err) + } + if got != "bw-secret" { + t.Errorf("Resolve = %q, want %q", got, "bw-secret") + } + + // Plain values pass through untouched. + got, err = registry.Resolve("literal") + if err != nil { + t.Fatalf("Resolve returned error: %v", err) + } + if got != "literal" { + t.Errorf("Resolve = %q, want %q", got, "literal") + } +} + +func TestRegistryResolveErrorNamesProvider(t *testing.T) { + op := &fakeProvider{name: "1Password", scheme: SchemeOnePassword, resolveErr: errors.New("boom")} + registry := NewRegistry(op) + + _, err := registry.Resolve("op://a/b/c") + if err == nil { + t.Fatal("expected an error") + } + if !strings.Contains(err.Error(), "1Password") { + t.Errorf("error %q does not name the provider", err) + } +} + +func TestRegistryByScheme(t *testing.T) { + registry, _, bw := newTestRegistry() + + got, ok := registry.ByScheme(SchemeBitwarden) + if !ok || got != bw { + t.Errorf("ByScheme(%q) = %v, want Bitwarden provider", SchemeBitwarden, got) + } + + if _, ok := registry.ByScheme("nope"); ok { + t.Error("ByScheme returned a provider for an unknown scheme") + } +} + +func TestIsKnownReference(t *testing.T) { + tests := map[string]bool{ + "op://Private/Server/password": true, + "bw://8f3c": true, + "enc:AAAA": false, + "hunter2": false, + "": false, + "opsomething": false, + } + + for value, want := range tests { + if got := IsKnownReference(value); got != want { + t.Errorf("IsKnownReference(%q) = %v, want %v", value, got, want) + } + } +} + +func TestKnownSchemesIsACopy(t *testing.T) { + schemes := KnownSchemes() + if len(schemes) == 0 { + t.Fatal("expected at least one known scheme") + } + schemes[0] = "mutated" + + if KnownSchemes()[0] == "mutated" { + t.Error("KnownSchemes returned the underlying slice") + } +} From 15f3f74de9cf38ea7bbdb743ee8d3d1ac7a3c8cb Mon Sep 17 00:00:00 2001 From: Filip Hert Date: Thu, 3 Sep 2026 19:28:20 +0200 Subject: [PATCH 2/3] fix(security): stop passing passwords on the command line On every platform the connection password was visible in the process list for the lifetime of the client process, which means any local user could read it: putty.exe -ssh ... -pw host sshpass -p ssh ... cmdkey /generic:TERMSRV/host /user:u /pass: PuTTY itself documents -pw as insecure and recommends -pwfile, so the password now goes into a private temporary file that PuTTY reads while parsing its arguments; the file is removed as soon as the process is up, or after a short grace period. Files left behind by a killed run are cleaned up at start-up. sshpass cannot take the password from the environment directly here, because the terminal emulator hop (gnome-terminal, Terminal.app) does not forward environment variables or file descriptors. The generated shell snippet therefore reads the file into SSHPASS and deletes it before ssh is executed, so the password is neither in argv nor on disk for longer than necessary. RDP credentials are written through the Windows Credential Manager API instead of shelling out to cmdkey. The blob is encoded as UTF-16LE, which is what mstsc expects; UTF-8 would store a credential that looks valid but silently fails to log in. launchInTerminal now takes the prepared shell snippet plus an equivalent argument vector, used only on systems with no terminal emulator, and the host name is passed explicitly rather than guessed from the arguments. Every value interpolated into a snippet is single-quote escaped, as before. Known limitation: xfreerdp on Linux is still invoked with /p:. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 12 ++ docs/PASSWORD-MANAGEMENT.md | 39 +++--- internal/launcher/credential_other.go | 10 ++ internal/launcher/credential_windows.go | 56 ++++++++ internal/launcher/launcher.go | 171 ++++++++++++++++-------- internal/launcher/password_file.go | 104 ++++++++++++++ internal/launcher/password_file_test.go | 138 +++++++++++++++++++ 7 files changed, 457 insertions(+), 73 deletions(-) create mode 100644 internal/launcher/credential_other.go create mode 100644 internal/launcher/credential_windows.go create mode 100644 internal/launcher/password_file.go create mode 100644 internal/launcher/password_file_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 8800f8a..d10e128 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 only when a Bitwarden reference is used, and terminated on exit. It is also placed in a Windows job object, and given a parent death signal on Linux, so it does not survive a crash. +- Passwords are no longer passed on a command line, where any local process + could read them out of the process list: + - SSH on Windows uses PuTTY `-pwfile` with a private temporary file that is + deleted as soon as PuTTY has started, instead of `-pw`. + - SSH on Linux and macOS passes the password to `sshpass` through the + `SSHPASS` environment variable instead of `-p`; the temporary file is + removed by the generated snippet before `ssh` starts. + - RDP credentials are written to the Windows Credential Manager through the + API instead of `cmdkey /pass:`. + +### Known limitations +- On Linux, `xfreerdp` is still invoked with `/p:`. ## [1.0.4] - 2026-01-28 diff --git a/docs/PASSWORD-MANAGEMENT.md b/docs/PASSWORD-MANAGEMENT.md index 06319fc..3b36fe1 100644 --- a/docs/PASSWORD-MANAGEMENT.md +++ b/docs/PASSWORD-MANAGEMENT.md @@ -91,10 +91,10 @@ MremoteGO uses **Windows Credential Manager** for seamless RDP connections. ### How It Works -1. **First Connection**: Password (from 1Password or plain text) is stored in Windows Credential Manager - ``` - cmdkey /generic:TERMSRV/hostname /user:username /pass:password - ``` +1. **First Connection**: Password (from a password manager or plain text) is + stored in Windows Credential Manager as a generic `TERMSRV/hostname` + credential, written through the Credential Manager API rather than the + `cmdkey` command, so the password never appears on a command line. 2. **Subsequent Connections**: Windows automatically retrieves credentials ``` @@ -107,7 +107,7 @@ MremoteGO uses **Windows Credential Manager** for seamless RDP connections. - ✅ Native Windows integration - ✅ Persistent across sessions - ✅ User-specific security -- ✅ Works with 1Password references +- ✅ Works with password manager references - ✅ No passwords in temporary files ### Managing Stored Credentials @@ -133,17 +133,24 @@ cmdkey /list | Select-String "TERMSRV" | ForEach-Object { ## SSH Password Handling ### Windows (PuTTY) -MremoteGO uses PuTTY with password auto-fill: +MremoteGO uses PuTTY with password auto-fill. The password is written to a +private temporary file and passed with `-pwfile`, which PuTTY reads at start-up: ``` -putty.exe -ssh -P 22 -l username -pw password hostname +putty.exe -ssh -P 22 -l username -pwfile hostname ``` +The file is deleted as soon as PuTTY is running. PuTTY's `-pw` option is not +used because it puts the password on the command line, where any local process +can read it. ### Linux/Mac (Native SSH) Uses native ssh client: ``` ssh username@hostname -p 22 ``` -Note: Password is passed via environment or expected to use SSH keys. +When a password is set and `sshpass` is installed, the password is read from a +private temporary file into the `SSHPASS` environment variable and the file is +removed before `ssh` starts, so the password appears neither in the process +list nor on disk for longer than necessary. ### SSH Key Authentication (Recommended) For better security, use SSH keys instead of passwords: @@ -194,21 +201,21 @@ password: mypassword123 ## Best Practices ### For Teams -1. ✅ Use 1Password for all passwords -2. ✅ Store configs in git with `op://` references +1. ✅ Use 1Password or Bitwarden for all passwords +2. ✅ Store configs in git with `op://` or `bw://` references 3. ✅ Use shared vaults for team credentials 4. ✅ Enable biometric unlock 5. ✅ Regular access audits ### For Personal Use -1. ✅ Use 1Password if you have it +1. ✅ Use 1Password or Bitwarden if you have one 2. ⚠️ Plain text is acceptable for local dev 3. ✅ Use SSH keys where possible 4. ✅ Keep config file permissions restricted 5. ✅ Don't commit passwords to public repos ### For Production -1. ✅ Use 1Password or enterprise password manager +1. ✅ Use 1Password, Bitwarden or another enterprise password manager 2. ✅ Certificate-based authentication where possible 3. ✅ SSH keys instead of passwords 4. ✅ Regular credential rotation @@ -235,7 +242,7 @@ password: mypassword123 - Consider using SSH keys instead ### Password visible in process list -- This is normal for command-line tools -- Use 1Password to minimize exposure -- Passwords are only visible briefly during connection -- Windows Credential Manager used for RDP (not in process list) +- MremoteGO no longer passes passwords on a command line +- SSH uses PuTTY `-pwfile` on Windows and `SSHPASS` on Linux/Mac +- RDP uses the Windows Credential Manager API +- Known limitation: on Linux, `xfreerdp` is still invoked with `/p:` diff --git a/internal/launcher/credential_other.go b/internal/launcher/credential_other.go new file mode 100644 index 0000000..3b1b12b --- /dev/null +++ b/internal/launcher/credential_other.go @@ -0,0 +1,10 @@ +//go:build !windows + +package launcher + +// The Windows Credential Manager has no counterpart on other platforms; RDP +// clients there take credentials by other means. + +func writeGenericCredential(target, username, password string) error { return nil } + +func deleteGenericCredential(target string) error { return nil } diff --git a/internal/launcher/credential_windows.go b/internal/launcher/credential_windows.go new file mode 100644 index 0000000..d430193 --- /dev/null +++ b/internal/launcher/credential_windows.go @@ -0,0 +1,56 @@ +//go:build windows + +package launcher + +import ( + "encoding/binary" + "errors" + "unicode/utf16" + + "github.com/danieljoos/wincred" + "golang.org/x/sys/windows" +) + +// writeGenericCredential stores a credential in the Windows Credential Manager. +// +// This replaces shelling out to cmdkey, which takes the password on its command +// line where every local process can read it. +// +// The blob must be UTF-16LE: that is what cmdkey writes and what mstsc expects. +// Storing plain UTF-8 makes the credential look valid while silently failing to +// log in. Persistence is enterprise, matching "cmdkey /generic". +func writeGenericCredential(target, username, password string) error { + cred := wincred.NewGenericCredential(target) + cred.UserName = username + cred.CredentialBlob = utf16LE(password) + cred.Persist = wincred.PersistEnterprise + + return cred.Write() +} + +// deleteGenericCredential removes a credential. A credential that is already +// gone is not an error. +func deleteGenericCredential(target string) error { + cred, err := wincred.GetGenericCredential(target) + if err != nil { + if errors.Is(err, windows.ERROR_NOT_FOUND) { + return nil + } + return err + } + + return cred.Delete() +} + +// utf16LE encodes a string as UTF-16 little endian without a byte order mark +// and without a terminating NUL, which is how Windows stores credential blobs. +func utf16LE(s string) []byte { + encoded := utf16.Encode([]rune(s)) + + out := make([]byte, len(encoded)*2) + for i, r := range encoded { + binary.LittleEndian.PutUint16(out[i*2:], r) + } + + return out +} diff --git a/internal/launcher/launcher.go b/internal/launcher/launcher.go index ec6a48c..2a5b6c2 100644 --- a/internal/launcher/launcher.go +++ b/internal/launcher/launcher.go @@ -8,6 +8,7 @@ import ( "runtime" "strconv" "strings" + "time" "github.com/jaydenthorup/mremotego/internal/secrets" "github.com/jaydenthorup/mremotego/pkg/models" @@ -20,6 +21,9 @@ type Launcher struct { // NewLauncher creates a new launcher func NewLauncher() *Launcher { + // Remove password files left behind by a run that was killed. + cleanupStalePasswordFiles() + return &Launcher{ secrets: secrets.Default(), } @@ -94,9 +98,17 @@ func (l *Launcher) launchSSH(conn *models.Connection) error { args = append(args, "-l", conn.Username) } - // Add password if provided (for auto-login) + // Add password if provided (for auto-login). PuTTY documents -pw as + // insecure because the command line is visible to every local process, + // so the password is handed over in a private file instead. + cleanup := func() {} if conn.Password != "" { - args = append(args, "-pw", conn.Password) + passwordFile, remove, err := writePasswordFile(conn.Password) + if err != nil { + return fmt.Errorf("failed to prepare password for PuTTY: %w", err) + } + cleanup = remove + args = append(args, "-pwfile", passwordFile) } // Add extra args if provided @@ -111,9 +123,16 @@ func (l *Launcher) launchSSH(conn *models.Connection) error { cmd := exec.Command("putty.exe", args...) // Don't hide putty - it's a GUI application we want to see if err := cmd.Start(); err != nil { + cleanup() // Fall back to ssh command return l.launchSSHFallback(conn) } + + // PuTTY reads the password file while parsing its arguments, so it can + // be removed as soon as the process is up. Waiting for the process also + // covers the case where it fails immediately. + go removeAfterStart(cmd, cleanup) + return nil } @@ -151,64 +170,101 @@ func (l *Launcher) launchSSHFallback(conn *models.Connection) error { } var cmd *exec.Cmd + cleanup := func() {} // If password is provided, try to use sshpass (if available) if conn.Password != "" { // Check if sshpass is available if _, err := exec.LookPath("sshpass"); err == nil { - // Use sshpass to provide password - sshpassArgs := []string{"-p", conn.Password, "ssh"} - sshpassArgs = append(sshpassArgs, args...) + passwordFile, remove, err := writePasswordFile(conn.Password) + if err != nil { + return fmt.Errorf("failed to prepare password for sshpass: %w", err) + } + cleanup = remove - // Launch in a terminal emulator - cmd = l.launchInTerminal("sshpass", sshpassArgs...) + // sshpass -p would expose the password in the process list. The + // password cannot be passed through the environment either, + // because terminal emulators do not forward it, so the generated + // shell snippet reads the file and deletes it before ssh starts. + cmd = l.launchInTerminal(sshpassCommand(passwordFile, args), nil, conn.Host) } else { // sshpass not available, just use ssh (will prompt for password) - cmd = l.launchInTerminal("ssh", args...) + cmd = l.launchInTerminal(shellCommand("ssh", args...), append([]string{"ssh"}, args...), conn.Host) } } else { - cmd = l.launchInTerminal("ssh", args...) + cmd = l.launchInTerminal(shellCommand("ssh", args...), append([]string{"ssh"}, args...), conn.Host) } if cmd == nil { + cleanup() return fmt.Errorf("failed to create terminal command") } - return cmd.Start() + if err := cmd.Start(); err != nil { + cleanup() + return err + } + + // The shell snippet deletes the file itself; this only covers a terminal + // that never ran it. + time.AfterFunc(staleCleanupDelay, cleanup) + + return nil +} + +// removeAfterStart runs cleanup once the process has exited or after a short +// grace period, whichever comes first. +func removeAfterStart(cmd *exec.Cmd, cleanup func()) { + done := make(chan struct{}) + go func() { + _ = cmd.Wait() + close(done) + }() + + select { + case <-done: + case <-time.After(passwordFileGrace): + } + + cleanup() } -// launchInTerminal launches a command in a terminal emulator -func (l *Launcher) launchInTerminal(command string, args ...string) *exec.Cmd { - // Build the command with proper shell quoting - cmdParts := []string{command} - cmdParts = append(cmdParts, args...) +// shellCommand quotes a command and its arguments for execution by a shell. +func shellCommand(command string, args ...string) string { + parts := append([]string{command}, args...) - // Join command parts, using shell quoting for safety - var quotedParts []string - for _, part := range cmdParts { - // Use single quotes and escape any single quotes in the string - quotedParts = append(quotedParts, fmt.Sprintf("'%s'", strings.ReplaceAll(part, "'", "'\\''"))) + quoted := make([]string, 0, len(parts)) + for _, part := range parts { + quoted = append(quoted, "'"+strings.ReplaceAll(part, "'", `'\''`)+"'") } - fullCmd := strings.Join(quotedParts, " ") + return strings.Join(quoted, " ") +} + +// sshpassCommand builds a shell snippet that reads the password from a file +// into the environment and removes the file before ssh is executed, so the +// secret appears neither in the process list nor on disk for longer than +// necessary. +func sshpassCommand(passwordFile string, sshArgs []string) string { + quotedFile := "'" + strings.ReplaceAll(passwordFile, "'", `'\''`) + "'" + + return fmt.Sprintf( + "SSHPASS=\"$(head -n 1 %s)\"; rm -f %s; export SSHPASS; %s", + quotedFile, + quotedFile, + shellCommand("sshpass", append([]string{"-e", "ssh"}, sshArgs...)...), + ) +} + +// launchInTerminal launches a shell snippet in a terminal emulator. +// +// fullCmd is the snippet to run. argv is the equivalent argument vector and is +// only used on systems where no terminal emulator is available. hostname names +// the host being connected to and is used for the host key hint on Linux. +func (l *Launcher) launchInTerminal(fullCmd string, argv []string, hostname string) *exec.Cmd { // For SSH commands on Linux, wrap with error detection for host key changes var wrappedCmd string - if runtime.GOOS == "linux" && (command == "ssh" || command == "sshpass") { - // Extract hostname from args for ssh-keygen -R - hostname := "" - for i, arg := range args { - if !strings.HasPrefix(arg, "-") && i > 0 { - // This is likely the hostname or user@hostname - parts := strings.Split(arg, "@") - if len(parts) > 1 { - hostname = parts[1] - } else { - hostname = arg - } - break - } - } - + if runtime.GOOS == "linux" && hostname != "" { // Create wrapper that detects host key mismatch and shows helpful message // Use a temp file to capture stderr for error detection wrappedCmd = fmt.Sprintf(` @@ -282,7 +338,19 @@ func (l *Launcher) launchInTerminal(command string, args ...string) *exec.Cmd { // Fallback: try to run without terminal (will need stdin/stdout) fmt.Println("Fallback: Running without terminal") - cmd := exec.Command(command, args...) + + var cmd *exec.Cmd + if runtime.GOOS == "windows" { + // Windows has no POSIX shell to interpret the snippet, and sshpass does + // not exist there, so the argument vector is always safe to run. + if len(argv) == 0 { + return nil + } + cmd = exec.Command(argv[0], argv[1:]...) + } else { + cmd = exec.Command("sh", "-c", wrappedCmd) + } + cmd.Stdin = os.Stdin cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr @@ -603,14 +671,11 @@ func (l *Launcher) storeWindowsCredential(conn *models.Connection) error { username = fmt.Sprintf("%s\\%s", conn.Domain, conn.Username) } - // Use cmdkey to store the credential - // cmdkey /generic:TERMSRV/hostname /user:username /pass:password - cmd := exec.Command("cmdkey", "/generic:TERMSRV/"+target, "/user:"+username, "/pass:"+conn.Password) - hideConsoleWindow(cmd) - - output, err := cmd.CombinedOutput() - if err != nil { - return fmt.Errorf("cmdkey failed: %w, output: %s", err, string(output)) + // Write the credential through the Credential Manager API. Using cmdkey + // here would put the password on a command line, where any local process + // could read it. + if err := writeGenericCredential("TERMSRV/"+target, username, conn.Password); err != nil { + return fmt.Errorf("failed to store credential for %s: %w", target, err) } return nil @@ -632,17 +697,9 @@ func (l *Launcher) RemoveWindowsCredential(conn *models.Connection) error { target = fmt.Sprintf("%s:%d", conn.Host, port) } - // Use cmdkey to delete the credential - // cmdkey /delete:TERMSRV/hostname - cmd := exec.Command("cmdkey", "/delete:TERMSRV/"+target) - hideConsoleWindow(cmd) - - output, err := cmd.CombinedOutput() - if err != nil { - // Don't return error if credential doesn't exist - if !strings.Contains(string(output), "not found") { - return fmt.Errorf("cmdkey delete failed: %w, output: %s", err, string(output)) - } + // Delete through the Credential Manager API; a missing credential is fine. + if err := deleteGenericCredential("TERMSRV/" + target); err != nil { + return fmt.Errorf("failed to remove credential for %s: %w", target, err) } return nil diff --git a/internal/launcher/password_file.go b/internal/launcher/password_file.go new file mode 100644 index 0000000..e24ccf4 --- /dev/null +++ b/internal/launcher/password_file.go @@ -0,0 +1,104 @@ +package launcher + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "sync" + "time" +) + +const ( + // passwordFilePrefix marks the temporary files holding a password. + passwordFilePrefix = "pw-" + // passwordFileMaxAge is how long a leftover file may survive before it is + // considered stale and removed at start-up. + passwordFileMaxAge = 2 * time.Minute + // passwordFileGrace bounds how long the file may exist after the helper + // program was started, in case that program never exits. + passwordFileGrace = 5 * time.Second + // staleCleanupDelay is the safety net for terminal emulators that never ran + // the generated snippet, which would otherwise leave the file behind. + staleCleanupDelay = 60 * time.Second +) + +// tempDir is the directory used for the short lived files handed to helper +// programs. It is the same directory the generated .rdp files already use. +func tempDir() string { + return filepath.Join(os.TempDir(), "mremotego") +} + +// writePasswordFile stores a password in a private temporary file and returns +// its path together with a cleanup function. +// +// Passing a password through a file rather than the command line keeps it out +// of the process list, where any local user could read it. +// +// os.CreateTemp creates the file with mode 0600, and on Windows the per-user +// temporary directory is already restricted to the user, SYSTEM and +// administrators - the same protection the generated .rdp files rely on. The +// window in which the file exists is short: the helper program reads it while +// starting up and the caller removes it right after. +func writePasswordFile(password string) (path string, cleanup func(), err error) { + dir := tempDir() + if err := os.MkdirAll(dir, 0700); err != nil { + return "", func() {}, fmt.Errorf("failed to create temp directory: %w", err) + } + + file, err := os.CreateTemp(dir, passwordFilePrefix+"*.txt") + if err != nil { + return "", func() {}, fmt.Errorf("failed to create password file: %w", err) + } + path = file.Name() + + // PuTTY and the sshpass wrapper both read the first line of the file. + if _, err := file.WriteString(password + "\n"); err != nil { + file.Close() + os.Remove(path) + return "", func() {}, fmt.Errorf("failed to write password file: %w", err) + } + + if err := file.Close(); err != nil { + os.Remove(path) + return "", func() {}, fmt.Errorf("failed to close password file: %w", err) + } + + var once sync.Once + cleanup = func() { + once.Do(func() { + // Overwrite before unlinking so the content does not linger in + // slack space if the delete fails. + if f, err := os.OpenFile(path, os.O_WRONLY, 0600); err == nil { + _, _ = f.Write(make([]byte, len(password)+1)) + f.Close() + } + os.Remove(path) + }) + } + + return path, cleanup, nil +} + +// cleanupStalePasswordFiles removes password files left behind by an earlier +// run that was killed before it could clean up. +func cleanupStalePasswordFiles() { + entries, err := os.ReadDir(tempDir()) + if err != nil { + return + } + + cutoff := time.Now().Add(-passwordFileMaxAge) + for _, entry := range entries { + if entry.IsDir() || !strings.HasPrefix(entry.Name(), passwordFilePrefix) { + continue + } + + info, err := entry.Info() + if err != nil || info.ModTime().After(cutoff) { + continue + } + + os.Remove(filepath.Join(tempDir(), entry.Name())) + } +} diff --git a/internal/launcher/password_file_test.go b/internal/launcher/password_file_test.go new file mode 100644 index 0000000..61c5a0a --- /dev/null +++ b/internal/launcher/password_file_test.go @@ -0,0 +1,138 @@ +package launcher + +import ( + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +func TestWritePasswordFile(t *testing.T) { + path, cleanup, err := writePasswordFile("hunter2") + if err != nil { + t.Fatalf("writePasswordFile returned error: %v", err) + } + defer cleanup() + + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("failed to read password file: %v", err) + } + + // PuTTY and the sshpass wrapper read the first line of the file. + if string(data) != "hunter2\n" { + t.Errorf("password file contains %q, want %q", string(data), "hunter2\n") + } +} + +func TestWritePasswordFileCleanupRemovesFile(t *testing.T) { + path, cleanup, err := writePasswordFile("hunter2") + if err != nil { + t.Fatalf("writePasswordFile returned error: %v", err) + } + + cleanup() + + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Errorf("password file still exists after cleanup (stat error: %v)", err) + } +} + +func TestWritePasswordFileCleanupIsIdempotent(t *testing.T) { + _, cleanup, err := writePasswordFile("hunter2") + if err != nil { + t.Fatalf("writePasswordFile returned error: %v", err) + } + + cleanup() + cleanup() // must not panic or fail +} + +func TestWritePasswordFileIsUnique(t *testing.T) { + first, cleanupFirst, err := writePasswordFile("a") + if err != nil { + t.Fatalf("writePasswordFile returned error: %v", err) + } + defer cleanupFirst() + + second, cleanupSecond, err := writePasswordFile("b") + if err != nil { + t.Fatalf("writePasswordFile returned error: %v", err) + } + defer cleanupSecond() + + if first == second { + t.Error("two password files got the same path") + } +} + +func TestCleanupStalePasswordFiles(t *testing.T) { + stale := filepath.Join(tempDir(), passwordFilePrefix+"stale-test.txt") + if err := os.MkdirAll(tempDir(), 0700); err != nil { + t.Fatalf("failed to create temp dir: %v", err) + } + if err := os.WriteFile(stale, []byte("old\n"), 0600); err != nil { + t.Fatalf("failed to create stale file: %v", err) + } + + old := time.Now().Add(-2 * passwordFileMaxAge) + if err := os.Chtimes(stale, old, old); err != nil { + t.Fatalf("failed to age the stale file: %v", err) + } + + fresh, cleanupFresh, err := writePasswordFile("keep me") + if err != nil { + t.Fatalf("writePasswordFile returned error: %v", err) + } + defer cleanupFresh() + + cleanupStalePasswordFiles() + + if _, err := os.Stat(stale); !os.IsNotExist(err) { + os.Remove(stale) + t.Errorf("stale password file survived cleanup (stat error: %v)", err) + } + if _, err := os.Stat(fresh); err != nil { + t.Errorf("recent password file was removed: %v", err) + } +} + +func TestShellCommandQuotesArguments(t *testing.T) { + got := shellCommand("ssh", "-p", "22", "user@host") + want := "'ssh' '-p' '22' 'user@host'" + + if got != want { + t.Errorf("shellCommand = %q, want %q", got, want) + } +} + +func TestShellCommandEscapesSingleQuotes(t *testing.T) { + got := shellCommand("ssh", "o'brien@host") + + if strings.Contains(got, "o'brien@host") { + t.Errorf("shellCommand left an unescaped quote: %q", got) + } + if !strings.Contains(got, `'\''`) { + t.Errorf("shellCommand = %q, want the single quote shell-escaped", got) + } +} + +// The password must reach sshpass through the environment, never through argv, +// where every local user could read it. +func TestSshpassCommandKeepsPasswordOutOfArgv(t *testing.T) { + got := sshpassCommand("/tmp/mremotego/pw-1.txt", []string{"-p", "22", "user@host"}) + + if !strings.Contains(got, "SSHPASS=") { + t.Errorf("snippet does not set SSHPASS: %q", got) + } + if !strings.Contains(got, "'sshpass' '-e' 'ssh'") { + t.Errorf("snippet does not run sshpass -e ssh: %q", got) + } + if strings.Contains(got, "-p '") && strings.Contains(got, "sshpass' '-p") { + t.Errorf("snippet passes the password on the command line: %q", got) + } + if !strings.Contains(got, "rm -f") { + t.Errorf("snippet does not remove the password file: %q", got) + } +} From f2109fa389869d119a527d622b2e81e9e600dc29 Mon Sep 17 00:00:00 2001 From: Filip Hert Date: Thu, 3 Sep 2026 19:32:39 +0200 Subject: [PATCH 3/3] fix(bitwarden): report a missing login as a vault state The Bitwarden CLI refuses to start "bw serve" at all when no user is logged in: it prints "You are not logged in." on stderr and exits. That surfaced as "bw serve exited before it became ready", which tells the user nothing about what to do. The start-up failure output is now classified, so a missing login reports ErrNotAuthenticated and a locked vault reports ErrVaultLocked, the same errors the running server would produce. Connecting with an unusable vault now says "not logged in to bitwarden" and the start-up dialog offers the sign-in instructions. Also adds two tests that run against the real system when it is available and skip otherwise: - the Bitwarden CLI, to check that the helper process starts, answers and is stopped, treating an unauthenticated CLI as a valid outcome; - cmdkey, to check that the credential written through the Credential Manager API is byte for byte identical to what cmdkey stores. This pins the UTF-16LE blob encoding and enterprise persistence that mstsc relies on. Co-Authored-By: Claude Opus 5 --- .../credential_compat_windows_test.go | 88 +++++++++++++++++++ internal/secrets/bitwarden.go | 26 +++++- internal/secrets/bitwarden_cli_test.go | 77 ++++++++++++++++ 3 files changed, 190 insertions(+), 1 deletion(-) create mode 100644 internal/launcher/credential_compat_windows_test.go create mode 100644 internal/secrets/bitwarden_cli_test.go diff --git a/internal/launcher/credential_compat_windows_test.go b/internal/launcher/credential_compat_windows_test.go new file mode 100644 index 0000000..b002154 --- /dev/null +++ b/internal/launcher/credential_compat_windows_test.go @@ -0,0 +1,88 @@ +//go:build windows + +package launcher + +import ( + "bytes" + "os/exec" + "testing" + + "github.com/danieljoos/wincred" +) + +// TestCredentialMatchesCmdkey verifies that the credential written through the +// Credential Manager API is byte for byte what "cmdkey /generic" would have +// written. mstsc reads the blob as UTF-16LE, so storing plain UTF-8 would +// produce a credential that looks valid but silently fails to log in. +func TestCredentialMatchesCmdkey(t *testing.T) { + if _, err := exec.LookPath("cmdkey"); err != nil { + t.Skip("cmdkey not available") + } + + const ( + user = "tester" + password = "passw0rd123" + refTgt = "TERMSRV/mremotego-selftest-cmdkey" + apiTgt = "TERMSRV/mremotego-selftest-api" + ) + + // Start from a clean slate: a leftover credential from an earlier run would + // make the comparison meaningless. + _ = deleteGenericCredential(refTgt) + _ = deleteGenericCredential(apiTgt) + + out, err := exec.Command("cmdkey", "/generic:"+refTgt, "/user:"+user, "/pass:"+password).CombinedOutput() + if err != nil { + t.Skipf("cmdkey refused to store the reference credential: %v (%s)", err, out) + } + defer deleteGenericCredential(refTgt) + + reference, err := wincred.GetGenericCredential(refTgt) + if err != nil { + t.Skipf("cmdkey credential could not be read back: %v", err) + } + + if err := writeGenericCredential(apiTgt, user, password); err != nil { + t.Fatalf("writeGenericCredential returned error: %v", err) + } + defer deleteGenericCredential(apiTgt) + + written, err := wincred.GetGenericCredential(apiTgt) + if err != nil { + t.Fatalf("failed to read back the credential we wrote: %v", err) + } + + if !bytes.Equal(reference.CredentialBlob, written.CredentialBlob) { + t.Errorf("blob mismatch:\n cmdkey % x\n api % x", + reference.CredentialBlob, written.CredentialBlob) + } + if written.UserName != reference.UserName { + t.Errorf("user name = %q, want %q", written.UserName, reference.UserName) + } + if written.Persist != reference.Persist { + t.Errorf("persist = %v, want %v", written.Persist, reference.Persist) + } +} + +// TestUTF16LE pins the encoding independently of whether cmdkey is available. +func TestUTF16LE(t *testing.T) { + got := utf16LE("ab") + want := []byte{'a', 0x00, 'b', 0x00} + + if !bytes.Equal(got, want) { + t.Errorf("utf16LE(\"ab\") = % x, want % x", got, want) + } + + // No byte order mark and no terminating NUL, matching what cmdkey stores. + if len(utf16LE("")) != 0 { + t.Errorf("utf16LE(\"\") = % x, want empty", utf16LE("")) + } +} + +// TestDeleteMissingCredentialIsNotAnError guards the cleanup path, which runs +// for every RDP connection that is removed. +func TestDeleteMissingCredentialIsNotAnError(t *testing.T) { + if err := deleteGenericCredential("TERMSRV/mremotego-selftest-absent"); err != nil { + t.Errorf("deleting a missing credential returned: %v", err) + } +} diff --git a/internal/secrets/bitwarden.go b/internal/secrets/bitwarden.go index 26f58d5..51a9f15 100644 --- a/internal/secrets/bitwarden.go +++ b/internal/secrets/bitwarden.go @@ -290,8 +290,17 @@ func (p *BitwardenProvider) startServer() (*bwClient, *bwServer, error) { client := newBwClient(server.baseURL) if err := waitForReady(context.Background(), client, server.exited); err != nil { + tail := server.StderrTail() server.Stop() - if tail := server.StderrTail(); tail != "" { + + // The CLI refuses to start the server at all when no user is logged in, + // and prints the reason on stderr. Report that as the vault state + // rather than as a start-up failure, so the caller can offer the right + // advice. + if stateErr := classifyStartupFailure(tail); stateErr != nil { + return nil, nil, stateErr + } + if tail != "" { return nil, nil, fmt.Errorf("%w: %s", err, tail) } return nil, nil, err @@ -299,3 +308,18 @@ func (p *BitwardenProvider) startServer() (*bwClient, *bwServer, error) { return client, server, nil } + +// classifyStartupFailure maps the CLI output of a server that refused to start +// onto a vault state error, or returns nil when the output does not name one. +func classifyStartupFailure(output string) error { + lower := strings.ToLower(output) + + switch { + case strings.Contains(lower, "not logged in"): + return ErrNotAuthenticated + case strings.Contains(lower, "vault is locked"), strings.Contains(lower, "vault is locked."): + return ErrVaultLocked + } + + return nil +} diff --git a/internal/secrets/bitwarden_cli_test.go b/internal/secrets/bitwarden_cli_test.go new file mode 100644 index 0000000..d16a140 --- /dev/null +++ b/internal/secrets/bitwarden_cli_test.go @@ -0,0 +1,77 @@ +package secrets + +import ( + "errors" + "os/exec" + "testing" +) + +func TestClassifyStartupFailure(t *testing.T) { + tests := []struct { + output string + want error + }{ + {output: "You are not logged in.", want: ErrNotAuthenticated}, + {output: "Vault is locked.", want: ErrVaultLocked}, + {output: "EADDRINUSE: address already in use", want: nil}, + {output: "", want: nil}, + } + + for _, tt := range tests { + got := classifyStartupFailure(tt.output) + if tt.want == nil { + if got != nil { + t.Errorf("classifyStartupFailure(%q) = %v, want nil", tt.output, got) + } + continue + } + if !errors.Is(got, tt.want) { + t.Errorf("classifyStartupFailure(%q) = %v, want %v", tt.output, got, tt.want) + } + } +} + +// TestRealBwServeLifecycle exercises the actual Bitwarden CLI when it is +// installed. It does not need a logged-in vault: an unauthenticated CLI is a +// valid outcome and is checked for the right error. +func TestRealBwServeLifecycle(t *testing.T) { + if _, err := exec.LookPath("bw"); err != nil { + t.Skip("bw CLI not in PATH") + } + + provider := NewBitwardenProvider() + defer provider.Close() + + if !provider.IsEnabled() { + t.Fatal("provider reports disabled although bw is in PATH") + } + + status, err := provider.Status() + switch { + case errors.Is(err, ErrNotAuthenticated): + // The CLI refuses to serve without a login, which is the expected + // result on a machine that has never run "bw login". + t.Log("bw is not logged in; start-up failure correctly reported") + return + case err != nil: + t.Fatalf("Status returned error: %v", err) + } + + t.Logf("vault status: %q", status) + + provider.mu.Lock() + server := provider.server + provider.mu.Unlock() + + if server == nil { + t.Fatal("no bw serve process was started") + } + t.Logf("bw serve pid=%d port=%d", server.cmd.Process.Pid, server.port) + + if err := provider.Close(); err != nil { + t.Fatalf("Close returned error: %v", err) + } + if !server.Exited() { + t.Error("bw serve is still running after Close") + } +}