diff --git a/DESIGN.md b/DESIGN.md index 5901dcd..fe6a64d 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -131,7 +131,9 @@ nodkey run --secret GH_TOKEN=github-token \ the terminal can match it with the message on the phone. 6. On approval, the broker first writes the decision to the audit log and syncs it to disk. Then it gets each value from its store and sends the - values back over the same TLS connection. + values back over the same TLS connection. To get a value, it runs the + store's program (section 16). If one value cannot be fetched, the + whole request is denied, and no value is sent. 7. The client adds `GH_TOKEN=` to its own environment and replaces itself with the command (`execve`). The command's exit code is the exit code of `nodkey run`. @@ -142,7 +144,10 @@ There is no command that prints a secret. This is on purpose: the agent should never see the value in its own output. The broker fetches a value only after approval, and fetches it again on -every request. Version 1 keeps no cache of values. +every request. Version 1 keeps no cache of values. For `bw`, it first +runs `bw sync --force`, which downloads the whole vault, so it never hands +out a value from an old copy of the vault. A sync without `--force` would +depend on the broker's clock (section 16). ## 6. Flow: an SSH login @@ -1137,11 +1142,34 @@ unlocked with the account's master password. An API key alone is not enough. To restart on its own, the broker must keep that password, so anyone who breaks into the broker host gets the whole vault. So nodkey supports `bw` only with a separate Bitwarden account that holds nothing but -the items nodkey may hand out. A person logs in once on the broker host -with `bw login --apikey`; after that the broker unlocks on its own at each -start. If the CLI is ever logged out, every `bw` request is denied until a -person logs in again. Bitwarden Secrets Manager (`bws`) does not have these -problems: its machine-account token can see only the projects you give it. +the items nodkey may hand out. Bitwarden Secrets Manager (`bws`) does not +have these problems: its machine-account token can see only the projects +you give it. + +How the broker uses `bw`: + +- A person logs in once on the broker host, as the broker user, with the + store's `program` and with the store's home directory (section 16) as + `HOME`, for example: + `sudo -u nodkey env -i HOME=/var/lib/nodkey/stores/vault PATH=/usr/local/bin:/usr/bin:/bin /usr/local/bin/bw login --apikey`. + For Bitwarden's EU cloud or your own server, run `bw config server URL` + in the same way first. +- At start, the broker unlocks with + `bw unlock --passwordfile FILE --raw --nointeraction`. bw reads the + password file itself, so the password never passes through the broker, + and it is never in an argument or a variable. Right before each unlock, + the broker checks the file again as at start (section 14). +- bw prints a session key. The broker keeps it only in memory, and gives + it to each later run of bw in the variable `BW_SESSION`. +- If bw is logged out, every request for that store is denied until a + person logs in again. Each request tries to unlock again, so no restart + is needed after the new login. +- If the session key stops working, for example because someone ran + `bw lock` or `bw unlock` in the store's home directory, the request is + denied. The broker forgets the key, and the next request unlocks again. +- Only one bw program runs at a time for one store. bw keeps the state of + the account in one file, and each unlock ends the session key before it. + A second request waits for its turn, or until it ends. **B. Keys inside hardware, through an upstream SSH agent.** The broker sends signing to a local agent socket on the broker host, for @@ -1355,9 +1383,14 @@ More rules: - Names of clients, stores, secrets, keys and known hosts are 1 to 64 letters, digits, `.`, `_` or `-`, and start with a letter or digit. - Paths must be absolute. Only `${CREDENTIALS_DIRECTORY}/` and `~/` at the - start of a path are expanded; any other `$` is an error. + start of a path are expanded; any other `$` is an error. A store's + `program`, `token_file` and `password_file` must also be clean: no `//`, + and no `.` or `..` parts. - A secret's `id` is the item's UUID in the store, so `bw` never searches - items by name. + items by name. For `bws`, nodkey hands out the secret's value. For `bw`, + the item must be a login item, and nodkey hands out its password. An + item in the trash or in the archive is refused, because `bw` still + returns such an item by its ID. - `clients` and `hosts` list each name once. `notice_chat_id` is not 0. A `window` is 1 minute to 8 hours. `timeout` is 15 s to 10 min. `ssh_timeout` is 15 s to 110 s, below @@ -1407,6 +1440,26 @@ any problem: - A store's `program` gets a token from the broker, so it must be owned by the broker user or root, and group and others must not be able to write to it. It may be a symbolic link. +- The `bws` token file is read once, at start; the broker keeps the token + in memory. The `bw` password file is checked at start and again before + each unlock, also that it holds 1 byte to 1 MiB; bw reads it itself + (section 11). +- For a `bw` store, each directory in bw's `PATH` (section 16) must pass + the same checks as the directories on the way to a key file, so that + nobody else can put a program, such as `node`, there. A directory that + is not there passes if the way to it passes. A `node` in one of those + directories must pass the same checks as a store's `program`, also + after its links, and so must every directory on the way to it: a bw + script starts it, and it reads the master password and gets the session + key. A `node` link whose target is missing is refused too. There must be + no `bw-data` next to the store's `program`, next to the file that it + links to, or next to `node` in one of those directories, also not after + its links: bw would keep its state there, not in its home directory. +- Each store's program gets its own home directory, `NAME` in the store + directory (section 16). The broker makes it with mode `0700` if it is + missing. If it is there, it must be a directory, not a symbolic link, + owned by the broker user, with mode `0700`. The directories on the way + must pass the same checks as those on the way to a key file. - The audit log may be missing; then the broker makes it. If it is there, it must be a regular file, not a symbolic link, owned by the broker user. Its mode may give only read and write to the owner, such as `0600`: an @@ -1508,16 +1561,30 @@ text of a line. It exits with: - The broker runs store programs by absolute path, with no shell, with only the environment they need (tokens and the `bw` session go in environment variables, never in arguments), with a timeout and a cap on output size. - Their output is never logged. -- `bw` keeps a local copy of the vault. The broker runs `bw sync` before each - fetch, so a changed or removed item is not handed out from an old copy. -- `bws` and `bw` write state files in their home directory. The systemd unit - gives the broker a private, writable home under its state directory. + Their output is never logged. The details are below. +- `bw` keeps a local copy of the vault. The broker runs `bw sync --force` + before each fetch, so a changed or removed item is not handed out from an + old copy. +- `bws` and `bw` write state files in their home directory. Each store gets + its own home directory, `NAME` in the store directory, such as + `/var/lib/nodkey/stores/vault`. The store directory is under the + broker's state directory, and only the broker user can write to it; the + example systemd unit makes it. - Limits on every input: request size (2 MiB, see section 10), number of secrets per request, field lengths. JSON decoding rejects unknown and repeated keys. - The broker turns off core dumps and marks itself not dumpable - (`PR_SET_DUMPABLE`). + (`PR_SET_DUMPABLE`). For core dumps, it sets its own soft `RLIMIT_CORE` + to 1 at start, and the example systemd unit sets `LimitCORE=1`. The store + programs inherit this limit; Go's `os/exec` cannot set a limit for one + child. If a store program crashes, a core handler such as + systemd-coredump would write the program's environment, with the token + or the session key, into the journal. With a limit of exactly 1 and a + pipe (`|`) or file `core_pattern`, such as the default of systemd, the + kernel calls no core handler and writes no core file. A limit of 0 does + not stop a core handler, and a socket `core_pattern` (`@`) does not look + at the limit at all. So keep a pipe or file `core_pattern` on the broker + host. - Time limits use Go's monotonic clock, so setting the wall clock does not move them. The monotonic clock stops while the host sleeps (suspend), so a request and a grant also end when the wall clock reaches their end: @@ -1525,7 +1592,100 @@ text of a line. It exits with: minutes ends at 12:10 on the wall clock, also if the host slept from 12:00 to 20:00. A request whose end the wall clock has passed takes no answer. -- A systemd unit with sandbox settings ships as an example. +- A systemd unit with sandbox settings ships as an example. Its sandbox + must let `bw` run: Node.js needs memory that is both writable and + executable, so the unit must not set `MemoryDenyWriteExecute=yes` when + it has a `bw` store. + +### Store programs + +The broker runs `bws` and `bw` like this (`internal/store`): + +- By the absolute path from the config, with no shell. stdin is + `/dev/null`, and the working directory is the store's home directory. +- With only these environment variables: `HOME` (the store's own home + directory), and `BWS_ACCESS_TOKEN` for `bws`, or `PATH` and + `BW_SESSION` for `bw`. `PATH` is `/usr/local/bin:/usr/bin:/bin`, so that + a bw from npm, a script that starts with `#!/usr/bin/env node`, finds + `node`. The broker checks these directories and the `node` in them at + start (section 14). bws + gets no `PATH`: it starts no other program. Nothing comes from the + broker's own environment. So a variable such as `NODE_OPTIONS`, `LD_PRELOAD` or + `BW_CLEANEXIT` (which makes bw exit with 0 after an error) cannot change + what the program does. Neither can a proxy setting (`HTTPS_PROXY`) or + extra CA certificates for bw (`NODE_EXTRA_CA_CERTS`): version 1 does not + support them. +- The arguments are fixed words, the item's UUID and the path of the + password file, never a token, a password or a session key. The broker + checks again that the ID is a UUID, so it never starts with `-`. +- In a process group of its own. Each run has a time limit of 30 seconds. + When the time is up, when the request ends (for example, the client + hangs up), or when the program writes more than 1 MiB to stdout or + 64 KiB to stderr, the broker kills the whole process group, and the run + fails. After each run, the broker kills the group again, in case the + program left a process behind. If such a process keeps the output open + for 2 seconds after the program ended, the run fails too. If the broker + ends during a run, for example in a crash, the kernel kills the program + (`Pdeathsig`). +- At most 4 runs of `bws` at the same time for one store, and one run of + `bw` (section 11). +- The broker reads stdout, never stderr, and logs neither. An error names + the store and the step, and gives a fixed reason and the exit code, such + as `store "vault": bw sync: the program failed (exit code 1)`. Any error + means deny. +- The broker clears the output, the token and the session key in its + memory when it no longer needs them. This is best effort: Go strings, + such as the environment variable for a run, and buffers of the operating + system are not cleared. +- bws keeps its login state in `.config/bws/state/` and its config in + `.config/bws/config` in its home directory. For Bitwarden's EU cloud or + your own server, run `bws config server-base URL` with that home, as for + bw in section 11. bw keeps its copy of the vault and its login in + `.config/Bitwarden CLI/data.json`. But if a directory named `bw-data` + is next to bw's executable, bw keeps its state there and ignores + `HOME`; so the broker refuses to start then (section 14). + +What the broker runs, and what it reads. We checked this in the source of +bws 2.1.0 and bw 2026.9.1 on 2026-10-14 +([bws](https://github.com/bitwarden/sdk-sm/tree/bws-v2.1.0/crates/bws), +[bw](https://github.com/bitwarden/clients/tree/cli-v2026.9.1/apps/cli)): + +| when | command | what the broker reads | +|---|---|---| +| a `bws` fetch | `bws secret get ID --output json --color no` | stdout: one JSON object with `id`, `organizationId`, `projectId`, `key`, `value`, `note`, `creationDate` and `revisionDate`. The broker reads only `id` and `value`. | +| a `bw` unlock | `bw unlock --passwordfile FILE --raw --nointeraction` | stdout: only the session key, in base64. | +| a `bw` fetch, first | `bw sync --force --nointeraction` | only the exit code. | +| a `bw` fetch, then | `bw get item ID --nointeraction` | stdout: one JSON object, the item. The broker reads only `id`, `login.password`, `deletedDate` and `archivedDate`. | +| after a `bw` step failed | `bw status --nointeraction` | stdout: one JSON object. The broker reads only `status`: `unauthenticated` (logged out), `locked` (the session key does not work) or `unlocked`. | + +- Both programs write errors to stderr and exit with 1; `bws` exits with + 2 on wrong usage. bw writes no newline at the end when stdout is not a + terminal. `--nointeraction` makes bw fail instead of asking for the + password when the vault is locked. +- An answer must be exactly one JSON object in valid UTF-8, with only + whitespace around it. A key may be in an object only once, and keys + must match exactly: `Value` is not `value`. A lone surrogate escape is + refused. Other fields are skipped, so a newer version with more fields + still works. Any other answer is refused, and the request is denied. +- `id` must be the asked ID, in upper or lower case. An answer without + it, or for another item, is refused. +- A value that is missing, `null` or empty is refused. So is a value with + a NUL byte: the environment of a command cannot hold it. So is an item + that is not a login item, an item in the trash (`deletedDate` is not + `null`) and an archived item (`archivedDate` is not `null`). bw 2026.9.1 + leaves both dates out of a normal item. +- bws exits with 0 and prints nothing when the token belongs to no + organization. That is refused too. +- Other fields can hold secrets too, such as `note` for bws, and `notes`, + `fields`, `passwordHistory` and `login.totp` for bw. The broker skips + them without keeping them, and clears the whole answer after reading it. +- Without `--force`, bw downloads the vault only when the server's + revision date of the account is later than bw's last sync, and bw takes + the time of each sync from the broker's clock. With that clock ahead, bw + would not download again, and a changed, trashed or archived item would + still be handed out. `--force` downloads the whole vault each time. That + is more traffic, but the account holds only the items that nodkey may + hand out (section 11). ## 17. Testing @@ -1544,7 +1704,10 @@ text of a line. It exits with: - **Fake approver**: a test approver that says yes, no, nothing, or answers late, on command. It and a fake clock are in `internal/approval/approvaltest`, for the tests of the broker too. -- **Fake store**: a small test program that acts like `bws`. +- **Fake stores**: the test binary of `internal/store` acts like `bws` or + `bw` when it runs under that name. It records its arguments and its + environment, so the tests can check that no token, password or session + key is ever an argument. - **End-to-end test**: a throwaway key, `sshd` in a container, the broker and the client on the test machine, and the fake approver. It checks that `ssh` and `git` over SSH work, that a deny blocks the login, that an diff --git a/THREAT-MODEL.md b/THREAT-MODEL.md index 2416932..6882bfe 100644 --- a/THREAT-MODEL.md +++ b/THREAT-MODEL.md @@ -19,7 +19,7 @@ access to another machine and asks a person before each use. | asset | where it lives | |---|---| | SSH private keys | broker memory; encrypted credentials on the broker host's disk | -| secret-store access (`bws` token, `bw` master password) | broker host | +| secret-store access (`bws` token, `bw` master password and session key) | broker host | | secret values | the store; for a moment the broker; after approval, the one command on the agent machine | | the approval decision | Telegram, the broker | | the audit log | broker host, and hash copies in the broker's journal | @@ -324,9 +324,25 @@ Risk that remains: **T18. The store programs leak a token.** Tokens and the `bw` session go in environment variables, never on the -command line, so other users cannot see them in the process list. The -broker passes only the variables the program needs. -Risk that remains: low. +command line, so other users cannot see them in the process list. The `bw` +master password is not in a variable either: bw reads the password file +itself. The broker passes only `HOME`, the one variable with the token +or the session key, and for bw a fixed `PATH`, and nothing from its own +environment, so a variable such as `NODE_OPTIONS` or `LD_PRELOAD` cannot +change what the program does. The broker never logs the programs' +output, and its errors hold only a fixed text and the exit code. Tests +check all of this with made-up secrets and fake programs that record what +they get. If a store program crashes, a core handler such as +systemd-coredump would write its environment, with the token or the +session key, into the journal. So the broker sets its soft `RLIMIT_CORE` +to 1, which the programs inherit, and the example unit sets +`LimitCORE=1`: with exactly 1 and a pipe (`|`) or file `core_pattern` +(the systemd default), the kernel calls no core handler. A limit of 0 +does not stop one, and a socket `core_pattern` (`@`) does not look at the +limit (DESIGN.md section 16). +Risk that remains: low. The broker clears the token and the session key +in its memory, but only as best effort: Go strings and the copies that the +system call makes are not cleared. **T19. The bot token leaks in an error message.** Go's HTTP errors include the URL, and the URL holds the token. So no @@ -427,8 +443,18 @@ read. The client checks its own TLS key in the same way. `nodkey keygen` writes a new key file with mode `0600`, never over a file or a link that is there. It checks the directories and the new, empty file in the same way before it writes the key. +Each store program keeps its state, such as bw's copy of the vault, in +its own home directory, with mode `0700`, owned by the broker user, below +directories that pass the same checks. The broker refuses a `bw-data` +directory next to bw's executable, where bw would keep its state instead. +bw reads its password file by its path; the broker checks the file right +before each unlock. The directories in bw's `PATH` pass the same checks, +and the `node` in them passes the checks of a store's program, also after +its links, so nobody else can put or change the `node` that a bw script +starts. Risk that remains: root and the broker user can still change everything -(T16). Programs and libraries that a store program loads are not checked. +(T16); for example, they can swap the bw password file between the check +and bw's read. The libraries that a store program loads are not checked. On overlayfs, nodkey sees only the mode bits of the top copy of a file; a lower copy of a key can be readable by others who can reach the lower layer (DESIGN.md section 14). Keep keys and tokens out of lower layers and @@ -491,6 +517,28 @@ service, not a way to get a secret or an approval. The answer is a firewall that lets only the agent machines reach the broker port (DESIGN.md section 10). +**T29. A store program hangs, floods, or answers wrong or old data.** +The broker gives each run of a store program 30 seconds and caps its +output: 1 MiB on stdout and 64 KiB on stderr. Over a limit, or when the +request ends, it kills the program's whole process group, and the request +is denied. If the broker itself ends during a run, the kernel kills the +program. The broker reads only one strict JSON object, and denies +anything else: an answer without the asked ID or for another item, and a +missing or empty value, or one with a NUL byte. bw keeps a copy of the +vault, so the broker runs `bw sync --force` before each fetch, and denies +the request if the sync fails. Without `--force`, bw would compare the +server's time with the broker's clock, and with that clock ahead it would +never download again. An item in the trash or in the archive is refused, +because `bw get item` still returns such an item by its ID. If bw is +logged out, every request for that store is denied until a person logs in +again (DESIGN.md sections 11 and 16). With a pipe or file +`core_pattern`, a crashed program writes no core dump and starts no core +handler (T18). +Risk that remains: a process that leaves the program's process group (for +example with `setsid`) is not killed; systemd stops it when the service +stops. Such a process keeps the token or the session key in its +environment. + ## 7. Assumptions - The broker host is patched, has no agent on it, and only the owner has diff --git a/internal/config/files.go b/internal/config/files.go index 8f6d1fd..becbe37 100644 --- a/internal/config/files.go +++ b/internal/config/files.go @@ -322,6 +322,18 @@ func checkAuditLogFile(fi fs.FileInfo, self uint32) error { return nil } +// CheckProgram makes the checks of a store's program (see CheckFiles) on +// the program at path. It follows symbolic links. Every directory on the +// way, also each one that holds a link, must pass the checks of +// ReadPrivateFile. The program must be a regular file owned by root or by +// the user that runs nodkey, and group and others must not be able to +// write to it. The store package checks with it the other programs that +// get a secret, such as the node that a bw script starts. +func CheckProgram(path string) error { + _, err := checkProgram(path) + return err +} + // checkProgram checks a program that the broker runs and gives a token to. // It follows symbolic links. Every directory on the way must pass the // checks of ReadPrivateFile. The program must be a regular file owned by diff --git a/internal/config/files_linux_test.go b/internal/config/files_linux_test.go index fa73695..b168f0d 100644 --- a/internal/config/files_linux_test.go +++ b/internal/config/files_linux_test.go @@ -13,6 +13,43 @@ import ( "time" ) +// TestCheckProgram checks CheckProgram, the check of a store's program, +// which the store package also uses for the node that a bw script starts. +// CheckFiles tests the same check in more cases. +func TestCheckProgram(t *testing.T) { + dir := privateTempDir(t) + bin := mkdir(t, dir, "bin", 0o755) + open := mkdir(t, dir, "open", 0o777) + good := writeFile(t, bin, "node", "", 0o755) + for _, path := range []string{good, symlink(t, good, filepath.Join(bin, "node-link"))} { + if err := CheckProgram(path); err != nil { + t.Errorf("CheckProgram(%s): %v", path, err) + } + } + writable := writeFile(t, bin, "node-0777", "", 0o777) + inOpen := writeFile(t, open, "node", "", 0o755) + hop := symlink(t, good, filepath.Join(open, "hop")) + tests := []struct{ name, path, want string }{ + {"others can write it", writable, writable + ": group or others can write to it (mode 0777)"}, + {"a link to a file that others can write", symlink(t, writable, filepath.Join(bin, "link-0777")), + writable + ": group or others can write to it (mode 0777)"}, + {"a link into a directory that others can write", symlink(t, inOpen, filepath.Join(bin, "link-open")), + open + ": group or others can write to this directory (mode 0777)"}, + {"a link held in a directory that others can write", symlink(t, hop, filepath.Join(bin, "link-hop")), + open + ": group or others can write to this directory (mode 0777)"}, + {"a directory", mkdir(t, bin, "node-dir", 0o755), "is not a regular file"}, + {"missing", filepath.Join(bin, "missing"), "no such file or directory"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + checkError(t, CheckProgram(tc.path), tc.want, "") + }) + } + if err := CheckProgram(filepath.Join(bin, "missing")); !errors.Is(err, os.ErrNotExist) { + t.Errorf("CheckProgram(missing) = %v, want an error that is fs.ErrNotExist", err) + } +} + // TestCheckPrivateFileWithACL sets a real POSIX ACL on a file, like the one // systemd puts on a credential, and checks that the group bits it shows in // the mode do not count as group access. diff --git a/internal/store/answer.go b/internal/store/answer.go new file mode 100644 index 0000000..7c51366 --- /dev/null +++ b/internal/store/answer.go @@ -0,0 +1,246 @@ +package store + +import ( + "bytes" + "fmt" +) + +// The answers that nodkey reads from bws and bw. DESIGN.md section 16 +// lists the fields. + +// Limits of the keys that a store keeps in memory. +const ( + // maxTokenLen is the longest bws access token. A token has about 100 + // characters. + maxTokenLen = 4096 + // maxSessionLen is the longest bw session key. A session key is 64 + // random bytes in base64: 88 characters. + maxSessionLen = 1024 + // maxStatusLen is the longest status in the answer of bw status. + maxStatusLen = 64 + // maxIDLen is the longest "id" in an answer that nodkey compares with + // the asked ID. A UUID has 36 characters. + maxIDLen = 64 +) + +// The states of bw that bw status reports. +const ( + bwUnauthenticated = "unauthenticated" // logged out + bwLocked = "locked" // logged in, but the session key does not work + bwUnlocked = "unlocked" // logged in, and the session key works +) + +// errOtherItem is the error of an answer without an "id", or with the ID +// of another item than the one asked for. +var errOtherItem = fmt.Errorf("%w: it has no id, or the id of another item", ErrBadAnswer) + +// parseBWSSecret reads the answer of "bws secret get ID --output json": +// one JSON object. It reads only the fields "id" and "value". "id" must be +// the asked ID id, in upper or lower case. A value that is missing, null or +// empty is ErrNoValue, and one with a NUL byte is ErrBadValue (see +// keepValue). The other fields, such as "key" and "note", are skipped; +// "note" can hold secrets too. +func parseBWSSecret(data []byte, id string) (Value, error) { + var value []byte + sameID := false + err := readAnswer(data, func(r *jsonReader, key string) error { + switch key { + case "id": + return readID(r, id, &sameID) + case "value": + switch r.peek() { + case 'n': + return r.null() + case '"': + b, err := r.secret() + value = b + return err + } + return ErrBadAnswer + } + return r.skip() + }) + if err == nil && !sameID { + err = errOtherItem + } + return keepValue(value, err) +} + +// parseBWItem reads the answer of "bw get item ID": one JSON object. It +// reads only the fields "id", "deletedDate", "archivedDate" and +// "login.password". "id" must be the asked ID id, in upper or lower case. +// An item in the trash (a deletedDate that is not null) is ErrTrashed, and +// an archived item (an archivedDate that is not null) is ErrArchived: bw +// still returns such items by their ID, but its own search skips them. A +// normal item has these two fields as null, or not at all. A password +// with a NUL byte is ErrBadValue (see keepValue). A password that is +// missing, null or empty is ErrNoValue; so is an item that is not +// a login, which has no login field. The other fields are skipped; +// "notes", "fields", "passwordHistory" and "login.totp" can hold secrets +// too. +func parseBWItem(data []byte, id string) (Value, error) { + var password []byte + sameID, trashed, archived := false, false, false + err := readAnswer(data, func(r *jsonReader, key string) error { + switch key { + case "id": + return readID(r, id, &sameID) + case "deletedDate": + return readDate(r, &trashed) + case "archivedDate": + return readDate(r, &archived) + case "login": + if r.peek() == 'n' { + return r.null() + } + return r.object(func(r *jsonReader, key string) error { + if key != "password" { + return r.skip() + } + switch r.peek() { + case 'n': + return r.null() + case '"': + b, err := r.secret() + password = b + return err + } + return ErrBadAnswer + }) + } + return r.skip() + }) + switch { + case err != nil: + case !sameID: + err = errOtherItem + case trashed: + err = ErrTrashed + case archived: + err = ErrArchived + } + return keepValue(password, err) +} + +// readID reads the field "id" of an answer. It sets *same when the ID is +// want, in upper or lower case. +func readID(r *jsonReader, want string, same *bool) error { + got, err := r.text(maxIDLen) + if err != nil { + return err + } + *same = sameUUID(got, want) + return nil +} + +// sameUUID reports whether a and b are the same text, where only the +// ASCII letters may differ in case. strings.EqualFold also takes other +// letters, such as the Kelvin sign for "k". +func sameUUID(a, b string) bool { + if len(a) != len(b) { + return false + } + for i := 0; i < len(a); i++ { + if lower(a[i]) != lower(b[i]) { + return false + } + } + return true +} + +// lower returns c in lower case, if it is an ASCII letter. +func lower(c byte) byte { + if 'A' <= c && c <= 'Z' { + return c + 'a' - 'A' + } + return c +} + +// readDate reads a date of a bw item, such as "deletedDate": null, or a +// string, which sets *set. +func readDate(r *jsonReader, set *bool) error { + switch r.peek() { + case 'n': + return r.null() + case '"': + *set = true + return r.skip() + } + return ErrBadAnswer +} + +// keepValue returns b as a Value when err is nil and b is a value that +// nodkey can hand out, and clears b otherwise. An empty value is +// ErrNoValue. A value with a NUL byte is ErrBadValue: a command's +// environment cannot hold it, because execve ends each variable at a NUL. +func keepValue(b []byte, err error) (Value, error) { + switch { + case err != nil: + case len(b) == 0: + err = ErrNoValue + case bytes.IndexByte(b, 0) >= 0: + err = ErrBadValue + } + if err != nil { + clear(b) + return Value{}, err + } + return newValue(b), nil +} + +// parseBWStatus reads the answer of "bw status": one JSON object. It +// reads only the field "status": unauthenticated, locked or unlocked. The +// other fields, such as "userEmail", are skipped. +func parseBWStatus(data []byte) (string, error) { + status := "" + err := readAnswer(data, func(r *jsonReader, key string) error { + if key != "status" { + return r.skip() + } + s, err := r.text(maxStatusLen) + status = s + return err + }) + if err != nil { + return "", err + } + switch status { + case bwUnauthenticated, bwLocked, bwUnlocked: + return status, nil + } + return "", ErrBadAnswer +} + +// parseKey reads a bws access token from its file, or a bw session key +// from the answer of "bw unlock --raw". Whitespace at the start and the +// end is dropped, such as the newline at the end of a file. What is left +// must be 1 to limit printable ASCII characters without spaces; a token +// and a session key have no others. It returns a new Value; the caller +// clears data. An error never shows the key. +func parseKey(data []byte, limit int) (Value, bool) { + start, end := 0, len(data) + for start < end && isSpace(data[start]) { + start++ + } + for end > start && isSpace(data[end-1]) { + end-- + } + if end == start || end-start > limit { + return Value{}, false + } + for _, c := range data[start:end] { + if c <= ' ' || c > '~' { + return Value{}, false + } + } + return newValue(append([]byte(nil), data[start:end]...)), true +} + +// isSpace reports whether c is ASCII whitespace. +func isSpace(c byte) bool { + switch c { + case ' ', '\t', '\n', '\r', '\v', '\f': + return true + } + return false +} diff --git a/internal/store/attack_test.go b/internal/store/attack_test.go new file mode 100644 index 0000000..088852b --- /dev/null +++ b/internal/store/attack_test.go @@ -0,0 +1,363 @@ +package store + +// Attack tests from the security review of issue #8. Each test runs a small +// /bin/sh script as the store program. +// +// - TestSetsidChild...: what happens for a risk that the threat model +// accepts (T29). If the behavior changes, update THREAT-MODEL.md. +// - The others: what the store must do, also against a program that +// misbehaves. +// +// None of them is parallel: a script file that another test's fork still +// holds open fails exec with ETXTBSY. + +import ( + "bytes" + "context" + "os" + "path/filepath" + "strconv" + "strings" + "syscall" + "testing" + "time" + + "github.com/code6x/nodkey/internal/config" +) + +// scriptStore makes a store of type typ whose program is the /bin/sh +// script body. It returns the store, its home directory and its config. +func scriptStore(t *testing.T, typ config.StoreType, body string, d deps) (Store, string, config.Store) { + t.Helper() + base := t.TempDir() + prog := filepath.Join(base, string(typ)) + if err := os.WriteFile(prog, []byte("#!/bin/sh\n"+body), 0o700); err != nil { //nolint:gosec // a test script. + t.Fatal(err) + } + dir := filepath.Join(base, "state") + if err := os.Mkdir(dir, 0o700); err != nil { + t.Fatal(err) + } + cfg := config.Store{Name: "main", Type: typ, Program: prog} + switch typ { + case config.StoreBWS: + cfg.TokenFile = filepath.Join(base, "token") + writeFile(t, cfg.TokenFile, madeUp("token")+"\n") + case config.StoreBW: + cfg.PasswordFile = filepath.Join(base, "password") + writeFile(t, cfg.PasswordFile, madeUp("password")+"\n") + } + s, err := newStore(cfg, dir, d) + if err != nil { + t.Fatalf("newStore: %v", err) + } + t.Cleanup(s.Close) + return s, filepath.Join(dir, cfg.Name), cfg +} + +// readPID reads a process ID that a script wrote to name in home. +func readPID(t *testing.T, home, name string) int { + t.Helper() + deadline := time.Now().Add(5 * time.Second) + for { + data, err := os.ReadFile(filepath.Join(home, name)) //nolint:gosec // test file. + if err == nil { + if pid, err := strconv.Atoi(strings.TrimSpace(string(data))); err == nil && pid > 1 { + return pid + } + } + if time.Now().After(deadline) { + t.Fatalf("the script wrote no process ID to %s", name) + } + time.Sleep(10 * time.Millisecond) + } +} + +// killLater kills pid when the test ends. +func killLater(t *testing.T, pid int) { + t.Cleanup(func() { _ = syscall.Kill(pid, syscall.SIGKILL) }) +} + +// A child that leaves the process group with setsid, and keeps stdout open. +// The run fails after waitDelay, but the child survives the group kill, with +// the token in its environment. THREAT-MODEL T29 accepts this. +func TestSetsidChildThatHoldsTheOutput(t *testing.T) { + d := testDeps() + d.waitDelay = 300 * time.Millisecond + s, home, _ := scriptStore(t, config.StoreBWS, `setsid sh -c 'echo $$ > "$HOME/escaped.pid"; exec sleep 300' & +sleep 0.3 +printf '{"value":"x"}' +`, d) + start := time.Now() + v, err := s.Fetch(t.Context(), testItemID) + checkErr(t, err, ErrLeftRunning) + if v.Bytes() != nil { + t.Error("a value with an error") + } + if took := time.Since(start); took > 5*time.Second { + t.Errorf("Fetch took %v", took) + } + pid := readPID(t, home, "escaped.pid") + killLater(t, pid) + if !alive(pid) { + t.Fatal("the escaped child is gone; T29 says it survives (update the threat model)") + } + env, err := os.ReadFile("/proc/" + strconv.Itoa(pid) + "/environ") + if err != nil { + t.Fatal(err) + } + if !bytes.Contains(env, []byte("BWS_ACCESS_TOKEN=")) { + t.Error("the escaped child has no token in its environment") + } +} + +// The same, but the child does not hold the output: the fetch works, and +// the child keeps running with the token. THREAT-MODEL T29 accepts this. +func TestSetsidChildThatLetsGoOfTheOutput(t *testing.T) { + s, home, _ := scriptStore(t, config.StoreBWS, `setsid sh -c 'echo $$ > "$HOME/escaped.pid"; exec sleep 300' >/dev/null 2>&1 & +sleep 0.3 +printf '{"id":"`+testItemID+`","value":"x"}' +`, testDeps()) + v, err := s.Fetch(t.Context(), testItemID) + if err != nil { + t.Fatalf("Fetch: %v", err) + } + v.Clear() + pid := readPID(t, home, "escaped.pid") + killLater(t, pid) + if !alive(pid) { + t.Fatal("the escaped child is gone; T29 says it survives (update the threat model)") + } +} + +// A child that stays in the group, and does not hold the output: the fetch +// works, and the group kill after the run stops the child. +func TestGroupChildKilledAfterSuccess(t *testing.T) { + s, home, _ := scriptStore(t, config.StoreBWS, `sleep 300 >/dev/null 2>&1 & +echo $! > "$HOME/child.pid" +printf '{"id":"`+testItemID+`","value":"x"}' +`, testDeps()) + v, err := s.Fetch(t.Context(), testItemID) + if err != nil { + t.Fatalf("Fetch: %v", err) + } + v.Clear() + pid := readPID(t, home, "child.pid") + killLater(t, pid) + waitGone(t, []int{pid}) +} + +// A program that stops itself (SIGSTOP) is still killed at the time limit. +func TestStoppedProgramTimesOut(t *testing.T) { + d := testDeps() + d.timeout = 2 * time.Second + s, home, _ := scriptStore(t, config.StoreBWS, `echo $$ > "$HOME/self.pid" +kill -STOP $$ +printf '{"value":"x"}' +`, d) + start := time.Now() + _, err := s.Fetch(t.Context(), testItemID) + checkErr(t, err, ErrTimeout) + if took := time.Since(start); took > 6*time.Second { + t.Errorf("Fetch took %v", took) + } + pid := readPID(t, home, "self.pid") + killLater(t, pid) + waitGone(t, []int{pid}) +} + +// A program that closes stdout and stderr and keeps running is killed at +// the time limit. +func TestProgramThatClosesItsOutputTimesOut(t *testing.T) { + d := testDeps() + d.timeout = 2 * time.Second + s, home, _ := scriptStore(t, config.StoreBWS, `echo $$ > "$HOME/self.pid" +exec >&- 2>&- +exec sleep 300 +`, d) + start := time.Now() + _, err := s.Fetch(t.Context(), testItemID) + checkErr(t, err, ErrTimeout) + if took := time.Since(start); took > 6*time.Second { + t.Errorf("Fetch took %v", took) + } + pid := readPID(t, home, "self.pid") + killLater(t, pid) + waitGone(t, []int{pid}) +} + +// /proc/PID/environ of a store program, which holds the token, is readable +// only by its own user (and root). cmdline has no token. +func TestEnvironmentIsPrivate(t *testing.T) { + s, home, _ := scriptStore(t, config.StoreBWS, `echo $$ > "$HOME/self.pid" +sleep 1 +printf '{"id":"`+testItemID+`","value":"x"}' +`, testDeps()) + done := make(chan error, 1) + go func() { + v, err := s.Fetch(context.Background(), testItemID) + v.Clear() + done <- err + }() + pid := readPID(t, home, "self.pid") + fi, err := os.Stat("/proc/" + strconv.Itoa(pid) + "/environ") + if err != nil { + t.Fatal(err) + } + if perm := fi.Mode().Perm(); perm != 0o400 { + t.Errorf("environ has mode %04o, want 0400", perm) + } + if st, ok := fi.Sys().(*syscall.Stat_t); !ok || int(st.Uid) != os.Geteuid() { + t.Errorf("environ is not owned by the broker user") + } + cmdline, err := os.ReadFile("/proc/" + strconv.Itoa(pid) + "/cmdline") + if err != nil { + t.Fatal(err) + } + if bytes.Contains(cmdline, []byte("token-")) { + t.Error("the token is in the command line") + } + if err := <-done; err != nil { + t.Fatalf("Fetch: %v", err) + } +} + +// bw: Close waits until a running Fetch ends, and that Fetch still hands +// out its value. The broker must cancel requests before it closes a store. +func TestBWCloseWaitsForAFetch(t *testing.T) { + s, _, _ := scriptStore(t, config.StoreBW, `case "$1" in +unlock) printf 'c2Vzc2lvbi1rZXk=' ;; +sync) sleep 2; printf 'Syncing complete.' ;; +get) printf '{"id":"`+testItemID+`","login":{"password":"x"}}' ;; +status) printf '{"status":"unlocked"}' ;; +esac +`, testDeps()) + type result struct { + v Value + err error + at time.Time + } + done := make(chan result, 1) + go func() { + v, err := s.Fetch(t.Context(), testItemID) + done <- result{v, err, time.Now()} + }() + time.Sleep(500 * time.Millisecond) + s.Close() + closed := time.Now() + r := <-done + if r.err != nil { + t.Fatalf("Fetch: %v", r.err) + } + r.v.Clear() + if closed.Before(r.at) { + t.Error("Close returned before the Fetch ended") + } +} + +// newStore refuses a relative password_file. CheckPrivateFile would check +// it relative to the broker's working directory, but bw runs in the +// store's home and reads it relative to that. The config package refuses +// relative paths, so this needs a config.Store that did not come from +// LoadBroker; newStore checks program for the same reason. +func TestNewRefusesRelativeFiles(t *testing.T) { + d := testDeps() + d.checkPrivateFile = func(string) error { return nil } // a check that passed for another file + base := t.TempDir() + dir := filepath.Join(base, "state") + if err := os.Mkdir(dir, 0o700); err != nil { + t.Fatal(err) + } + for _, pw := range []string{"password", "-password", "../password"} { + cfg := config.Store{Name: "vault", Type: config.StoreBW, Program: "/bin/false", PasswordFile: pw} + s, err := newStore(cfg, dir, d) + if err == nil { + s.Close() + t.Errorf("newStore took password_file %q, a relative path", pw) + } else if !strings.Contains(err.Error(), "password_file: must be an absolute path") { + t.Errorf("newStore with password_file %q: %v", pw, err) + } + } + cfg := config.Store{Name: "main", Type: config.StoreBWS, Program: "/bin/false", TokenFile: "token"} + d.readPrivateFile = func(string) ([]byte, error) { return []byte(madeUp("token")), nil } + if s, err := newStore(cfg, dir, d); err == nil { + s.Close() + t.Error("newStore took a relative token_file") + } else if !strings.Contains(err.Error(), "token_file: must be an absolute path") { + t.Errorf("newStore with a relative token_file: %v", err) + } +} + +// A value with a NUL byte can never go into an environment variable +// (execve ends each one at a NUL), so the broker denies it before the +// value leaves the broker. +func TestValueWithNULIsRefused(t *testing.T) { + s, _, _ := scriptStore(t, config.StoreBWS, `printf '{"id":"`+testItemID+`","value":"a\\u0000b"}' +`, testDeps()) + v, err := s.Fetch(t.Context(), testItemID) + if err == nil { + t.Errorf("Fetch handed out a value with a NUL byte (%d bytes)", len(v.Bytes())) + v.Clear() + } + checkErr(t, err, ErrBadValue) +} + +// An answer for another item is refused. Checking "id" against the asked +// ID costs nothing, and turns a CLI bug or a future fallback to a search +// into a deny. +func TestAnswerForAnotherItemIsRefused(t *testing.T) { + const other = "00000000-0000-4000-8000-0000000000aa" + for _, typ := range []config.StoreType{config.StoreBWS, config.StoreBW} { + s, _, _ := scriptStore(t, typ, `case "$1" in +secret) printf '{"id":"`+other+`","value":"x"}' ;; +unlock) printf 'c2Vzc2lvbi1rZXk=' ;; +sync) printf 'Syncing complete.' ;; +get) printf '{"object":"item","id":"`+other+`","type":1,"login":{"password":"x"}}' ;; +status) printf '{"status":"unlocked"}' ;; +esac +`, testDeps()) + v, err := s.Fetch(t.Context(), testItemID) + if err == nil { + t.Errorf("%s: Fetch handed out the value of item %s for item %s", typ, other, testItemID) + v.Clear() + } + checkErr(t, err, ErrBadAnswer) + } +} + +// bw get item by ID returns archived items too, as it does items in the +// trash; bw's own search skips both. A person who archives an item to +// retire it expects nodkey to stop handing it out. +func TestBWArchivedItemIsRefused(t *testing.T) { + s, _, _ := scriptStore(t, config.StoreBW, `case "$1" in +unlock) printf 'c2Vzc2lvbi1rZXk=' ;; +sync) printf 'Syncing complete.' ;; +get) printf '{"object":"item","id":"`+testItemID+`","type":1,"archivedDate":"2026-10-13T12:00:00.000Z","login":{"password":"x"}}' ;; +status) printf '{"status":"unlocked"}' ;; +esac +`, testDeps()) + v, err := s.Fetch(t.Context(), testItemID) + if err == nil { + t.Error("Fetch handed out an archived item") + v.Clear() + } + checkErr(t, err, ErrArchived) +} + +// Real bw 2026.9.1 leaves "deletedDate" out of a normal item (CipherView +// has it undefined, and JSON.stringify drops it). Such an answer must work. +func TestBWItemWithoutDeletedDate(t *testing.T) { + s, _, _ := scriptStore(t, config.StoreBW, `case "$1" in +unlock) printf 'c2Vzc2lvbi1rZXk=' ;; +sync) printf 'Syncing complete.' ;; +get) printf '{"object":"item","id":"`+testItemID+`","type":1,"name":"n","login":{"username":"u","password":"x"}}' ;; +status) printf '{"status":"unlocked"}' ;; +esac +`, testDeps()) + v, err := s.Fetch(t.Context(), testItemID) + if err != nil { + t.Fatalf("Fetch: %v", err) + } + checkValue(t, v, "x") + v.Clear() +} diff --git a/internal/store/bw.go b/internal/store/bw.go new file mode 100644 index 0000000..f241872 --- /dev/null +++ b/internal/store/bw.go @@ -0,0 +1,354 @@ +package store + +import ( + "context" + "errors" + "fmt" + "io" + "io/fs" + "os" + "path/filepath" + + "github.com/code6x/nodkey/internal/config" +) + +// bwPath is the PATH of bw. A bw that npm installed is a script that +// starts with "#!/usr/bin/env node", and env finds node through PATH. Only +// directories that root manages on most systems are in it, and New checks +// each of them, and the node in it (checkPathDirs). +const bwPath = "/usr/local/bin:/usr/bin:/bin" + +// maxPasswordFileSize is the largest bw password file: 1 MiB, as for every +// key, token and password file (DESIGN.md section 14). +const maxPasswordFileSize = 1 << 20 + +// bwDataDir is the name of a directory that bw uses for its state instead +// of its home directory, when the directory is next to bw's executable. +const bwDataDir = "bw-data" + +// The steps of a bw store, for errors. +const ( + stepUnlock = "bw unlock" + stepSync = "bw sync" + stepGetItem = "bw get item" + stepStatus = "bw status" +) + +// bwStore gets values from the Bitwarden Password Manager with bw. +// +// bw keeps the state of its account in one file in its home directory, +// and each "bw unlock" makes a new session key that ends the old one. So +// only one bw program runs at a time for one store: a run holds lock, and +// lock also guards session and closed. A second request waits for its +// turn, or until its context ends. +type bwStore struct { + run runner + path string // the PATH of bw + passwordFile string // the master password; bw reads it itself + checkPrivateFile func(string) error // checks passwordFile before each unlock + + lock chan struct{} // one place: held during every run of bw + session Value // the session key; zero when the vault must be unlocked + closed bool +} + +// newBW checks the directories in bw's PATH, that bw would keep its state +// in its home directory, and the master password file of a bw store, and +// returns the store. It does not run bw; Start unlocks. +func newBW(cfg config.Store, r runner, d deps) (*bwStore, error) { + if err := checkPathDirs(d.path, d.checkParentDirs, d.checkProgram); err != nil { + return nil, fmt.Errorf("store %q: %w", cfg.Name, err) // the error names the directory + } + if err := checkNoBWData(cfg.Program, d.path); err != nil { + return nil, fmt.Errorf("store %q: %w", cfg.Name, err) // the error names the directory + } + if err := checkPasswordFile(cfg.PasswordFile, d.checkPrivateFile); err != nil { + return nil, fmt.Errorf("store %q: password_file: %w", cfg.Name, err) // the error names the path + } + return &bwStore{ + run: r, + path: d.path, + passwordFile: cfg.PasswordFile, + checkPrivateFile: d.checkPrivateFile, + lock: make(chan struct{}, 1), + }, nil +} + +// checkPathDirs checks the directories in path, the PATH of bw, and the +// node in each of them. The node that a bw script starts through PATH +// reads the master password file and gets the session key, and so does +// any program that someone puts in such a directory. +// +// - Each directory must pass checkDirs (config.CheckParentDirs), like +// the directories on the way to a store's program: owned by root or by +// the user that runs nodkey, and not writable by group or others. A +// directory that is not there passes when the directories on the way +// to it pass: then only root or that user can make it. +// - A node in a directory, also a link, must pass checkProgram +// (config.CheckProgram), as a store's program does: after its links, +// it is a regular file owned by root or by that user, not writable by +// group or others, and every directory on the way to it passes the +// checks too. A link whose target is missing fails: someone could put +// a file there later. +func checkPathDirs(path string, checkDirs, checkProgram func(string) error) error { + for _, dir := range filepath.SplitList(path) { + if !cleanAbs(dir) { + return fmt.Errorf("PATH directory %q: %s", dir, cleanAbsRule) + } + node := filepath.Join(dir, "node") + err := checkDirs(node) // checks dir and the way to it + if errors.Is(err, fs.ErrNotExist) { + err = checkDirs(dir) // checks only the way to dir + } + if err != nil { + return fmt.Errorf("PATH directory %s: %w", dir, err) + } + switch _, err := os.Lstat(node); { + case errors.Is(err, fs.ErrNotExist): + case err != nil: + return fmt.Errorf("PATH directory %s: %w", dir, err) // a *fs.PathError names the path + default: + if err := checkProgram(node); err != nil { + return fmt.Errorf("PATH directory %s: node: %w", dir, err) + } + } + } + return nil +} + +// checkNoBWData refuses a directory named bw-data next to bw's +// executable. bw keeps its state there when it is there, and ignores HOME +// (see bw's service-container.ts): then the state is not in the store's +// private home, and two stores could share it. bw's executable is the +// program, after its links, for the standalone bw; for a bw from npm, it +// is node, which env finds through PATH. So checkNoBWData looks next to +// the program, next to the file that it links to, and next to node in +// each directory in path, before and after its links. +func checkNoBWData(program, path string) error { + target, err := filepath.EvalSymlinks(program) + if err != nil { + return fmt.Errorf("program: %w", err) // a *fs.PathError names the path + } + dirs := []string{filepath.Dir(program), filepath.Dir(target)} + for _, dir := range filepath.SplitList(path) { + node, err := filepath.EvalSymlinks(filepath.Join(dir, "node")) + switch { + case err == nil: + dirs = append(dirs, dir, filepath.Dir(node)) + case !errors.Is(err, fs.ErrNotExist): + return fmt.Errorf("PATH directory %s: %w", dir, err) + } + } + for _, dir := range dirs { + p := filepath.Join(dir, bwDataDir) + switch _, err := os.Lstat(p); { + case err == nil: + return fmt.Errorf("%s: bw would keep its state there, not in the store's home directory; "+ + "move it away", p) + case !errors.Is(err, fs.ErrNotExist): + return err // a *fs.PathError names the path + } + } + return nil +} + +// checkPasswordFile checks the bw password file with check +// (config.CheckPrivateFile), then that it is a regular file of 1 byte to +// 1 MiB. bw reads it by its path, so nodkey checks only the file there, +// without reading it. +func checkPasswordFile(path string, check func(string) error) error { + if err := check(path); err != nil { + return err // the error names the path + } + fi, err := os.Lstat(path) + switch { + case err != nil: + return err // a *fs.PathError names the path + case !fi.Mode().IsRegular(): + return fmt.Errorf("%s: is not a regular file", path) + case fi.Size() == 0: + return fmt.Errorf("%s: is empty; it must hold the master password", path) + case fi.Size() > maxPasswordFileSize: + return fmt.Errorf("%s: is larger than 1 MiB", path) + } + return nil +} + +// Start unlocks the vault, unless the store has a session key already. +func (s *bwStore) Start(ctx context.Context) error { + if err := acquire(ctx, s.lock); err != nil { + return stepError(s.run.store, stepUnlock, err) + } + defer s.release() + if s.closed { + return stepError(s.run.store, stepUnlock, ErrClosed) + } + if s.session.Bytes() != nil { + return nil + } + return s.unlock(ctx) +} + +// Fetch unlocks the vault if the store has no session key, runs +// "bw sync --force", then runs "bw get item ID" and returns the item's +// password. The session key goes in BW_SESSION. If any step fails, Fetch +// fails: it never hands out a value from bw's old copy of the vault. +// +// The sync is forced. Without --force, bw downloads the vault only when +// the server's revision date of the account is later than bw's last sync, +// and bw takes the time of each sync from the broker's clock. With that +// clock ahead, bw would never download again, and an item that a person +// changed, moved to the trash or archived would still be handed out. +func (s *bwStore) Fetch(ctx context.Context, id string) (Value, error) { + if !isUUID(id) { + return Value{}, stepError(s.run.store, stepGetItem, ErrBadID) + } + if err := acquire(ctx, s.lock); err != nil { + return Value{}, stepError(s.run.store, stepGetItem, err) + } + defer s.release() + if s.closed { + return Value{}, stepError(s.run.store, stepGetItem, ErrClosed) + } + if s.session.Bytes() == nil { + if err := s.unlock(ctx); err != nil { + return Value{}, err + } + } + out, err := s.runWithSession(ctx, stepSync, "sync", "--force") + if err != nil { + return Value{}, err + } + clear(out) // "Syncing complete." + out, err = s.runWithSession(ctx, stepGetItem, "get", "item", id) + if err != nil { + return Value{}, err + } + defer clear(out) + v, err := parseBWItem(out, id) + if err != nil { + return Value{}, stepError(s.run.store, stepGetItem, err) + } + return v, nil +} + +// unlock runs "bw unlock --passwordfile FILE --raw" and keeps the session +// key that it prints. bw reads the password file itself, so the password +// never passes through nodkey. The file is checked right before, as at +// start. The caller holds lock. +func (s *bwStore) unlock(ctx context.Context) error { + if err := checkPasswordFile(s.passwordFile, s.checkPrivateFile); err != nil { + return stepError(s.run.store, stepUnlock, fmt.Errorf("password_file: %w", err)) // names the path + } + out, err := s.run.run(ctx, stepUnlock, []string{"unlock", "--passwordfile", s.passwordFile, "--raw", "--nointeraction"}, + []string{"PATH=" + s.path}) + if err != nil { + return s.failed(ctx, stepUnlock, err) + } + defer clear(out) + session, ok := parseKey(out, maxSessionLen) + if !ok { + return stepError(s.run.store, stepUnlock, ErrBadAnswer) + } + s.session = session + return nil +} + +// runWithSession runs bw with args and the session key in BW_SESSION. The +// caller holds lock and has a session key. +func (s *bwStore) runWithSession(ctx context.Context, step string, args ...string) ([]byte, error) { + out, err := s.run.run(ctx, step, append(args, "--nointeraction"), s.sessionEnv()) + if err != nil { + return nil, s.failed(ctx, step, err) + } + return out, nil +} + +// failed handles a run of bw that failed. When bw exited with an error, +// failed asks bw status why: +// +// - unauthenticated: bw is logged out. The error is ErrLoggedOut, and +// every request fails until a person logs in again. +// - locked: the session key does not work any more, for example after +// someone ran "bw lock" or "bw unlock" in the store's home directory. +// The error is ErrLocked (for unlock itself, its own error), and the +// next request unlocks again. +// - unlocked: the step failed for another reason, such as a network +// error or an unknown item. The error is the step's own. +// +// The store forgets the session key in the first two cases, and when bw +// status fails too. The caller holds lock. +func (s *bwStore) failed(ctx context.Context, step string, err error) error { + if !errors.Is(err, ErrFailed) { + return err // a timeout, too much output, or the end of ctx + } + state, serr := s.status(ctx) + switch { + case serr != nil: + s.forget() + return err + case state == bwUnauthenticated: + s.forget() + return stepError(s.run.store, step, ErrLoggedOut) + case state == bwLocked: + s.forget() + if step == stepUnlock { + return err + } + return stepError(s.run.store, step, ErrLocked) + } + return err +} + +// status runs "bw status" and returns the state of bw, with the session +// key if the store has one. The caller holds lock. +func (s *bwStore) status(ctx context.Context) (string, error) { + out, err := s.run.run(ctx, stepStatus, []string{"status", "--nointeraction"}, s.sessionEnv()) + if err != nil { + return "", err + } + defer clear(out) // it holds the account's email address + state, err := parseBWStatus(out) + if err != nil { + return "", stepError(s.run.store, stepStatus, err) + } + return state, nil +} + +// sessionEnv returns the environment of a run of bw: PATH, and the +// session key when the store has one. The string with the key is a copy +// that cannot be cleared; so is the copy that os/exec makes for the system +// call. +func (s *bwStore) sessionEnv() []string { + env := []string{"PATH=" + s.path} + if s.session.Bytes() == nil { + return env + } + return append(env, "BW_SESSION="+string(s.session.Bytes())) +} + +// forget clears the session key, so the next request unlocks again. +func (s *bwStore) forget() { + s.session.Clear() + s.session = Value{} +} + +// release gives lock back. +func (s *bwStore) release() { <-s.lock } + +// Close clears the session key. It waits for a run of bw to end. +func (s *bwStore) Close() { + s.lock <- struct{}{} + defer s.release() + s.forget() + s.closed = true +} + +// String names the store, without its session key. +func (s *bwStore) String() string { return fmt.Sprintf("bw store %q", s.run.store) } + +// GoString names the store, without its session key. +func (s *bwStore) GoString() string { return s.String() } + +// Format writes String for every verb. +func (s *bwStore) Format(f fmt.State, _ rune) { _, _ = io.WriteString(f, s.String()) } diff --git a/internal/store/bw_test.go b/internal/store/bw_test.go new file mode 100644 index 0000000..29c7bbc --- /dev/null +++ b/internal/store/bw_test.go @@ -0,0 +1,495 @@ +package store + +import ( + "context" + "errors" + "slices" + "strings" + "sync" + "testing" + "time" + + "github.com/code6x/nodkey/internal/config" +) + +// bwScenario returns a scenario where bw knows one login item with the +// password value. Its other fields hold the made-up secret other. +func bwScenario(password, session, value, other string) fakeScenario { + return fakeScenario{ + Password: password, + Session: session, + Items: map[string]string{testItemID: bwItemJSON(testItemID, value, nil, other)}, + } +} + +func TestBWStartAndFetch(t *testing.T) { + t.Parallel() + password, session, value := madeUp("password"), madeUp("session"), madeUp("value") + f := newTestFake(t, config.StoreBW, bwScenario(password, session, value, madeUp("other")), testDeps()) + if err := f.store.Start(t.Context()); err != nil { + t.Fatalf("Start: %v", err) + } + if got := f.steps(); !slices.Equal(got, []string{"unlock"}) { + t.Errorf("Start ran %q, want unlock", got) + } + // A second Start keeps the session key. + if err := f.store.Start(t.Context()); err != nil { + t.Fatalf("Start: %v", err) + } + for range 2 { + v, err := f.store.Fetch(t.Context(), testItemID) + if err != nil { + t.Fatalf("Fetch: %v", err) + } + checkValue(t, v, value) + v.Clear() + } + if got, want := f.steps(), []string{"unlock", "sync", "get item", "sync", "get item"}; !slices.Equal(got, want) { + t.Errorf("steps = %q, want %q", got, want) + } + calls := f.calls() + unlock := calls[0] + if want := []string{"unlock", "--passwordfile", f.cfg.PasswordFile, "--raw", "--nointeraction"}; !slices.Equal(unlock.Args, want) { + t.Errorf("unlock arguments = %q, want %q", unlock.Args, want) + } + if got, want := envNames(unlock.Env), []string{"HOME", "PATH"}; !slices.Equal(got, want) { + t.Errorf("unlock environment names = %q, want %q", got, want) + } + // The sync is forced: bw would compare the server's time with the + // broker's clock otherwise. + if want := []string{"sync", "--force", "--nointeraction"}; !slices.Equal(calls[1].Args, want) { + t.Errorf("sync arguments = %q, want %q", calls[1].Args, want) + } + if want := []string{"get", "item", testItemID, "--nointeraction"}; !slices.Equal(calls[2].Args, want) { + t.Errorf("get item arguments = %q, want %q", calls[2].Args, want) + } + for _, c := range calls { + if c.Dir != f.home || envValue(c.Env, "HOME") != f.home || envValue(c.Env, "PATH") != bwPath { + t.Errorf("%q: working directory %q, HOME %q, PATH %q", c.Args, c.Dir, envValue(c.Env, "HOME"), + envValue(c.Env, "PATH")) + } + if !c.Leader { + t.Errorf("%q: bw does not lead its own process group", c.Args) + } + if c.Running != 1 { + t.Errorf("%q: %d runs of bw at once, want 1", c.Args, c.Running) + } + if c.Args[0] == "unlock" { + continue + } + if got, want := envNames(c.Env), []string{"BW_SESSION", "HOME", "PATH"}; !slices.Equal(got, want) { + t.Errorf("%q: environment names = %q, want %q", c.Args, got, want) + } + if envValue(c.Env, "BW_SESSION") != session { + t.Errorf("%q: BW_SESSION is not the session key from bw unlock", c.Args) + } + } +} + +func TestBWFetchUnlocksWithoutStart(t *testing.T) { + t.Parallel() + value := madeUp("value") + f := newTestFake(t, config.StoreBW, bwScenario(madeUp("password"), "", value, madeUp("other")), testDeps()) + v, err := f.store.Fetch(t.Context(), testItemID) + if err != nil { + t.Fatalf("Fetch: %v", err) + } + checkValue(t, v, value) + if got, want := f.steps(), []string{"unlock", "sync", "get item"}; !slices.Equal(got, want) { + t.Errorf("steps = %q, want %q", got, want) + } +} + +func TestBWLoggedOut(t *testing.T) { + t.Parallel() + value := madeUp("value") + f := newTestFake(t, config.StoreBW, bwScenario(madeUp("password"), "", value, madeUp("other")), testDeps()) + f.setBWState(fakeBWState{LoggedOut: true}) + checkErr(t, f.store.Start(t.Context()), ErrLoggedOut) + _, err := f.store.Fetch(t.Context(), testItemID) + checkErr(t, err, ErrLoggedOut) + if err != nil && !strings.HasPrefix(err.Error(), `store "vault": bw unlock: bw is logged out`) { + t.Errorf("error = %v", err) + } + if got, want := f.steps(), []string{"unlock", "status", "unlock", "status"}; !slices.Equal(got, want) { + t.Errorf("steps = %q, want %q", got, want) + } + // A person logs in again: the next request works. + f.setBWState(fakeBWState{}) + v, err := f.store.Fetch(t.Context(), testItemID) + if err != nil { + t.Fatalf("Fetch after a new login: %v", err) + } + checkValue(t, v, value) +} + +func TestBWLoggedOutLater(t *testing.T) { + t.Parallel() + f := newTestFake(t, config.StoreBW, bwScenario(madeUp("password"), "", madeUp("value"), madeUp("other")), testDeps()) + if err := f.store.Start(t.Context()); err != nil { + t.Fatalf("Start: %v", err) + } + st := f.bwState() + st.LoggedOut = true + f.setBWState(st) + _, err := f.store.Fetch(t.Context(), testItemID) + checkErr(t, err, ErrLoggedOut) + if err != nil && !strings.HasPrefix(err.Error(), `store "vault": bw sync: bw is logged out`) { + t.Errorf("error = %v", err) + } + // After a failed sync, bw get item never runs. + if got, want := f.steps(), []string{"unlock", "sync", "status"}; !slices.Equal(got, want) { + t.Errorf("steps = %q, want %q", got, want) + } + if f.store.(*bwStore).session.Bytes() != nil { + t.Error("the store kept the session key") + } +} + +func TestBWLockedUnlocksAgain(t *testing.T) { + t.Parallel() + value := madeUp("value") + f := newTestFake(t, config.StoreBW, bwScenario(madeUp("password"), "", value, madeUp("other")), testDeps()) + if err := f.store.Start(t.Context()); err != nil { + t.Fatalf("Start: %v", err) + } + old := f.bwState().Session + // Someone ran bw unlock in the store's home: the old key is no good. + f.setBWState(fakeBWState{Session: madeUp("someone-else")}) + _, err := f.store.Fetch(t.Context(), testItemID) + checkErr(t, err, ErrLocked) + if err != nil && !strings.HasPrefix(err.Error(), `store "vault": bw get item: the bw vault is locked`) { + t.Errorf("error = %v", err) + } + // The next request unlocks again, with a new session key. + v, err := f.store.Fetch(t.Context(), testItemID) + if err != nil { + t.Fatalf("Fetch after a lock: %v", err) + } + checkValue(t, v, value) + if f.bwState().Session == old { + t.Error("the store did not unlock again") + } + want := []string{"unlock", "sync", "get item", "status", "unlock", "sync", "get item"} + if got := f.steps(); !slices.Equal(got, want) { + t.Errorf("steps = %q, want %q", got, want) + } +} + +func TestBWSyncFails(t *testing.T) { + t.Parallel() + value := madeUp("value") + sc := bwScenario(madeUp("password"), "", value, madeUp("other")) + sc.Steps = map[string]fakeStep{"sync": {Stderr: "Syncing failed: fetch failed", Exit: 1}} + f := newTestFake(t, config.StoreBW, sc, testDeps()) + v, err := f.store.Fetch(t.Context(), testItemID) + checkErr(t, err, ErrFailed) + if err != nil && err.Error() != `store "vault": bw sync: the program failed (exit code 1)` { + t.Errorf("error = %v", err) + } + if v.Bytes() != nil { + t.Error("Fetch returned a value with its error") + } + // The value is never read from bw's old copy of the vault. + if got, want := f.steps(), []string{"unlock", "sync", "status"}; !slices.Equal(got, want) { + t.Errorf("steps = %q, want %q", got, want) + } + // bw is still unlocked, so the store keeps the session key. + if f.store.(*bwStore).session.Bytes() == nil { + t.Error("the store dropped a session key that works") + } +} + +func TestBWGetItemFails(t *testing.T) { + t.Parallel() + f := newTestFake(t, config.StoreBW, bwScenario(madeUp("password"), "", madeUp("value"), madeUp("other")), testDeps()) + _, err := f.store.Fetch(t.Context(), "00000000-0000-4000-8000-0000000000ff") // not found + checkErr(t, err, ErrFailed) + if err != nil && err.Error() != `store "vault": bw get item: the program failed (exit code 1)` { + t.Errorf("error = %v", err) + } + if f.store.(*bwStore).session.Bytes() == nil { + t.Error("the store dropped a session key that works") + } +} + +func TestBWWrongPassword(t *testing.T) { + t.Parallel() + password := madeUp("password") + f := newTestFake(t, config.StoreBW, bwScenario(password, "", madeUp("value"), madeUp("other")), testDeps()) + writeFile(t, f.cfg.PasswordFile, madeUp("wrong")+"\n") + err := f.store.Start(t.Context()) + checkErr(t, err, ErrFailed) + if err != nil && err.Error() != `store "vault": bw unlock: the program failed (exit code 1)` { + t.Errorf("error = %v", err) + } + if errors.Is(err, ErrLocked) { + t.Error("a failed unlock says that the vault is locked") + } + _, err = f.store.Fetch(t.Context(), testItemID) + checkErr(t, err, ErrFailed) + if got, want := f.steps(), []string{"unlock", "status", "unlock", "status"}; !slices.Equal(got, want) { + t.Errorf("steps = %q, want %q", got, want) + } +} + +func TestBWUnlockBadAnswer(t *testing.T) { + t.Parallel() + session := madeUp("session") + for name, stdout := range map[string]string{ + // bw unlock with an empty password file ends without output. + "nothing": "", + "a message": "Your vault is now unlocked!\n\n$ export BW_SESSION=\"" + session + "\"\n", + "two words": session + " " + session, + "color": "\x1b[32m" + session, + "too long": strings.Repeat("s", maxSessionLen+1), + "only spaces": " \n ", + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + sc := bwScenario(madeUp("password"), "", madeUp("value"), madeUp("other")) + sc.Steps = map[string]fakeStep{"unlock": {Stdout: stdout}} + f := newTestFake(t, config.StoreBW, sc, testDeps()) + err := f.store.Start(t.Context()) + checkErr(t, err, ErrBadAnswer) + if err != nil { + noCanary(t, "the error", err.Error(), session) + } + if f.store.(*bwStore).session.Bytes() != nil { + t.Error("the store kept a session key from a bad answer") + } + }) + } +} + +func TestBWStatusFails(t *testing.T) { + t.Parallel() + sc := bwScenario(madeUp("password"), "", madeUp("value"), madeUp("other")) + sc.Steps = map[string]fakeStep{"status": {Stdout: "not JSON"}} + f := newTestFake(t, config.StoreBW, sc, testDeps()) + if err := f.store.Start(t.Context()); err != nil { + t.Fatalf("Start: %v", err) + } + _, err := f.store.Fetch(t.Context(), "00000000-0000-4000-8000-0000000000ff") // not found + checkErr(t, err, ErrFailed) + // The store cannot tell why bw failed, so it unlocks again next time. + if f.store.(*bwStore).session.Bytes() != nil { + t.Error("the store kept the session key") + } +} + +func TestBWTimeoutKillsTheProcessGroup(t *testing.T) { + t.Parallel() + for _, step := range []string{"unlock", "sync", "get item"} { + t.Run(step, func(t *testing.T) { + t.Parallel() + value := madeUp("value") + sc := bwScenario(madeUp("password"), "", value, madeUp("other")) + sc.Steps = map[string]fakeStep{step: {Hang: true, Stdout: value}} + d := testDeps() + d.timeout = 5 * time.Second + f := newTestFake(t, config.StoreBW, sc, d) + v, err := f.store.Fetch(t.Context(), testItemID) + checkErr(t, err, ErrTimeout) + if v.Bytes() != nil { + t.Error("Fetch returned a value with its error") + } + if err != nil && !strings.HasPrefix(err.Error(), `store "vault": bw `+step+": the program took too long") { + t.Errorf("error = %v", err) + } + waitGone(t, f.pids()) + // A timeout says nothing about bw's state: no bw status. + if got := f.steps(); slices.Contains(got, "status") { + t.Errorf("steps = %q, want no status", got) + } + }) + } +} + +func TestBWTooMuchOutput(t *testing.T) { + t.Parallel() + for _, step := range []string{"unlock", "sync", "get item", "status"} { + t.Run(step, func(t *testing.T) { + t.Parallel() + sc := bwScenario(madeUp("password"), "", madeUp("value"), madeUp("other")) + sc.Steps = map[string]fakeStep{step: {Flood: "stdout"}} + if step == "status" { + // bw status runs only after a failed step. + sc.Steps["sync"] = fakeStep{Exit: 1} + } + f := newTestFake(t, config.StoreBW, sc, testDeps()) + _, err := f.store.Fetch(t.Context(), testItemID) + want := ErrTooMuchOutput + if step == "status" { + want = ErrFailed // the error of bw sync + } + checkErr(t, err, want) + }) + } +} + +func TestBWItemAnswers(t *testing.T) { + t.Parallel() + value, other := madeUp("value"), madeUp("other") + tests := map[string]struct { + item string + want error + }{ + "in the trash": {bwItemJSON(testItemID, value, "2026-10-13T12:00:00.000Z", other), ErrTrashed}, + "null password": {bwItemJSON(testItemID, nil, nil, other), ErrNoValue}, + "empty password": {bwItemJSON(testItemID, "", nil, other), ErrNoValue}, + "no login": {`{"object":"item","id":"` + testItemID + `","type":2,"notes":"` + other + `","deletedDate":null}`, ErrNoValue}, + "null login": {`{"id":"` + testItemID + `","login":null}`, ErrNoValue}, + "password number": {`{"login":{"password":7}}`, ErrBadAnswer}, + "deletedDate true": {`{"login":{"password":"` + value + `"},"deletedDate":true}`, ErrBadAnswer}, + "two passwords": {`{"login":{"password":"` + value + `","password":"` + value + `"}}`, ErrBadAnswer}, + "two logins": {`{"login":{"password":"` + value + `"},"login":{"password":"` + value + `"}}`, ErrBadAnswer}, + "Password": {`{"id":"` + testItemID + `","login":{"Password":"` + value + `"}}`, ErrNoValue}, + "cut short": {`{"login":{"password":"` + value + `"}`, ErrBadAnswer}, + "a list": {`[` + bwItemJSON(testItemID, value, nil, other) + `]`, ErrBadAnswer}, + } + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + sc := bwScenario(madeUp("password"), "", value, other) + sc.Items[testItemID] = tc.item + f := newTestFake(t, config.StoreBW, sc, testDeps()) + v, err := f.store.Fetch(t.Context(), testItemID) + checkErr(t, err, tc.want) + if v.Bytes() != nil { + t.Error("Fetch returned a value with its error") + } + if err != nil { + noCanary(t, "the error", err.Error(), value) + noCanary(t, "the error", err.Error(), other) + } + }) + } +} + +func TestBWConcurrentFetches(t *testing.T) { + t.Parallel() + value := madeUp("value") + sc := bwScenario(madeUp("password"), "", value, madeUp("other")) + sc.Pause = 30 * time.Millisecond + f := newTestFake(t, config.StoreBW, sc, testDeps()) + const n = 6 + var wg sync.WaitGroup + errs := make([]error, n) + for i := range n { + wg.Go(func() { + v, err := f.store.Fetch(t.Context(), testItemID) + if err == nil && string(v.Bytes()) != value { + err = errors.New("wrong value") + } + v.Clear() + errs[i] = err + }) + } + wg.Wait() + for _, err := range errs { + if err != nil { + t.Errorf("Fetch: %v", err) + } + } + // One unlock, then a sync and a get for each request, one bw at a + // time. + steps := f.steps() + if len(steps) != 1+2*n || steps[0] != "unlock" { + t.Errorf("steps = %q, want one unlock and %d of sync and get item", steps, n) + } + for _, c := range f.calls() { + if c.Running != 1 { + t.Errorf("%d runs of bw at once, want 1", c.Running) + } + } +} + +func TestBWFetchWaitsForItsTurn(t *testing.T) { + t.Parallel() + f := newTestFake(t, config.StoreBW, bwScenario(madeUp("password"), "", madeUp("value"), madeUp("other")), testDeps()) + s := f.store.(*bwStore) + s.lock <- struct{}{} // another request runs bw + ctx, cancel := context.WithTimeout(t.Context(), 100*time.Millisecond) + defer cancel() + _, err := s.Fetch(ctx, testItemID) + checkErr(t, err, context.DeadlineExceeded) + checkErr(t, s.Start(ctx), context.DeadlineExceeded) + s.release() + if calls := f.calls(); len(calls) != 0 { + t.Errorf("bw ran %d times, want 0", len(calls)) + } +} + +func TestBWChecksThePasswordFileBeforeEachUnlock(t *testing.T) { + t.Parallel() + f := setupFake(t, config.StoreBW, bwScenario(madeUp("password"), "", madeUp("value"), madeUp("other"))) + d := testDeps() + bad := errors.New("made-up: others can read it") + var mu sync.Mutex + checks, fail := 0, false + d.checkPrivateFile = func(path string) error { + mu.Lock() + defer mu.Unlock() + if path != f.cfg.PasswordFile { + t.Errorf("checked %q, want the password file", path) + } + checks++ + if fail { + return bad + } + return nil + } + s, err := newStore(f.cfg, f.dir, d) + if err != nil { + t.Fatalf("newStore: %v", err) + } + defer s.Close() + mu.Lock() + fail = true + mu.Unlock() + err = s.Start(t.Context()) + checkErr(t, err, bad) + if err != nil && !strings.HasPrefix(err.Error(), `store "vault": bw unlock: password_file: `) { + t.Errorf("error = %v", err) + } + if calls := f.calls(); len(calls) != 0 { + t.Errorf("bw ran %d times, want 0", len(calls)) + } + mu.Lock() + fail = false + mu.Unlock() + if err := s.Start(t.Context()); err != nil { + t.Fatalf("Start: %v", err) + } + mu.Lock() + if checks != 3 { // newStore, and each unlock + t.Errorf("the password file was checked %d times, want 3", checks) + } + // newStore refuses a password file that fails the check. + fail = true + mu.Unlock() + if _, err := newStore(f.cfg, f.dir, d); !errors.Is(err, bad) { + t.Errorf("newStore with a bad password file: %v", err) + } +} + +func TestBWClose(t *testing.T) { + t.Parallel() + f := newTestFake(t, config.StoreBW, bwScenario(madeUp("password"), "", madeUp("value"), madeUp("other")), testDeps()) + if err := f.store.Start(t.Context()); err != nil { + t.Fatalf("Start: %v", err) + } + session := f.store.(*bwStore).session + f.store.Close() + if session.Bytes() != nil { + t.Error("Close left the session key") + } + _, err := f.store.Fetch(t.Context(), testItemID) + checkErr(t, err, ErrClosed) + checkErr(t, f.store.Start(t.Context()), ErrClosed) + if got := f.steps(); !slices.Equal(got, []string{"unlock"}) { + t.Errorf("steps = %q, want only the first unlock", got) + } + f.store.Close() +} diff --git a/internal/store/bws.go b/internal/store/bws.go new file mode 100644 index 0000000..1bbc0cd --- /dev/null +++ b/internal/store/bws.go @@ -0,0 +1,108 @@ +package store + +import ( + "context" + "fmt" + "io" + "sync" + + "github.com/code6x/nodkey/internal/config" +) + +// bwsParallel is the most runs of bws at the same time for one store. +// bws keeps no state that two runs could break; the limit only caps the +// processes that many requests can start. +const bwsParallel = 4 + +// bwsStore gets values from Bitwarden Secrets Manager with bws. +type bwsStore struct { + run runner + slots chan struct{} // one place for each run of bws + + mu sync.Mutex + token Value // the machine account's access token + closed bool +} + +// newBWS reads the token file of a bws store and returns the store. +func newBWS(cfg config.Store, r runner, d deps) (*bwsStore, error) { + data, err := d.readPrivateFile(cfg.TokenFile) + if err != nil { + return nil, fmt.Errorf("store %q: token_file: %w", cfg.Name, err) // the error names the path, never the content + } + defer clear(data) + token, ok := parseKey(data, maxTokenLen) + if !ok { + return nil, fmt.Errorf("store %q: token_file %s: it holds no access token, or a character that "+ + "a token never has (the content is not shown here)", cfg.Name, cfg.TokenFile) + } + return &bwsStore{run: r, slots: make(chan struct{}, bwsParallel), token: token}, nil +} + +// Start does nothing for bws: the token is read, and bws logs in on each +// run. +func (s *bwsStore) Start(context.Context) error { + s.mu.Lock() + defer s.mu.Unlock() + if s.closed { + return stepError(s.run.store, "start", ErrClosed) + } + return nil +} + +// Fetch runs "bws secret get ID --output json --color no" with the token +// in BWS_ACCESS_TOKEN, and returns the field "value" of the answer. bws +// gets no PATH: it starts no other program. +func (s *bwsStore) Fetch(ctx context.Context, id string) (Value, error) { + const step = "bws secret get" + if !isUUID(id) { + return Value{}, stepError(s.run.store, step, ErrBadID) + } + if err := acquire(ctx, s.slots); err != nil { + return Value{}, stepError(s.run.store, step, err) + } + defer func() { <-s.slots }() + env, err := s.tokenEnv() + if err != nil { + return Value{}, stepError(s.run.store, step, err) + } + out, err := s.run.run(ctx, step, []string{"secret", "get", id, "--output", "json", "--color", "no"}, env) + if err != nil { + return Value{}, err + } + defer clear(out) + v, err := parseBWSSecret(out, id) + if err != nil { + return Value{}, stepError(s.run.store, step, err) + } + return v, nil +} + +// tokenEnv returns the environment variable with the token. The string +// is a copy that cannot be cleared; so is the copy that os/exec makes for +// the system call. +func (s *bwsStore) tokenEnv() ([]string, error) { + s.mu.Lock() + defer s.mu.Unlock() + if s.closed { + return nil, ErrClosed + } + return []string{"BWS_ACCESS_TOKEN=" + string(s.token.Bytes())}, nil +} + +// Close clears the token. +func (s *bwsStore) Close() { + s.mu.Lock() + defer s.mu.Unlock() + s.token.Clear() + s.closed = true +} + +// String names the store, without its token. +func (s *bwsStore) String() string { return fmt.Sprintf("bws store %q", s.run.store) } + +// GoString names the store, without its token. +func (s *bwsStore) GoString() string { return s.String() } + +// Format writes String for every verb. +func (s *bwsStore) Format(f fmt.State, _ rune) { _, _ = io.WriteString(f, s.String()) } diff --git a/internal/store/bws_test.go b/internal/store/bws_test.go new file mode 100644 index 0000000..1d030f7 --- /dev/null +++ b/internal/store/bws_test.go @@ -0,0 +1,424 @@ +package store + +import ( + "context" + "errors" + "os" + "path/filepath" + "slices" + "strconv" + "strings" + "sync" + "testing" + "time" + + "github.com/code6x/nodkey/internal/config" +) + +// bwsScenario returns a scenario where bws knows one item with value. +func bwsScenario(token, value string) fakeScenario { + return fakeScenario{Token: token, Items: map[string]string{testItemID: bwsSecretJSON(testItemID, value)}} +} + +func TestBWSFetch(t *testing.T) { + t.Parallel() + token, value := madeUp("token"), madeUp("value") + f := newTestFake(t, config.StoreBWS, bwsScenario(token, value), testDeps()) + if err := f.store.Start(t.Context()); err != nil { + t.Fatalf("Start: %v", err) + } + if calls := f.calls(); len(calls) != 0 { + t.Errorf("Start ran bws %d times, want 0", len(calls)) + } + // An ID in upper case works too; bws takes both. + for _, id := range []string{testItemID, strings.ToUpper(testItemID)} { + v, err := f.store.Fetch(t.Context(), id) + if err != nil { + t.Fatalf("Fetch(%s): %v", id, err) + } + checkValue(t, v, value) + v.Clear() + if v.Bytes() != nil { + t.Error("Clear left the value") + } + } + calls := f.calls() + if len(calls) != 2 { + t.Fatalf("bws ran %d times, want 2", len(calls)) + } + c := calls[0] + if want := []string{"secret", "get", testItemID, "--output", "json", "--color", "no"}; !slices.Equal(c.Args, want) { + t.Errorf("arguments = %q, want %q", c.Args, want) + } + // bws starts no other program, so it gets no PATH. + if got, want := envNames(c.Env), []string{"BWS_ACCESS_TOKEN", "HOME"}; !slices.Equal(got, want) { + t.Errorf("environment names = %q, want %q", got, want) + } + if envValue(c.Env, "BWS_ACCESS_TOKEN") != token { + t.Error("BWS_ACCESS_TOKEN is not the token from the token file") + } + if got := envValue(c.Env, "HOME"); got != f.home { + t.Errorf("HOME = %q, want %q", got, f.home) + } + if c.Dir != f.home { + t.Errorf("working directory = %q, want %q", c.Dir, f.home) + } + if c.Stdin != os.DevNull { + t.Errorf("stdin = %q, want %q", c.Stdin, os.DevNull) + } + if !c.Leader { + t.Error("bws does not lead its own process group") + } +} + +func TestBWSErrorExit(t *testing.T) { + t.Parallel() + value := madeUp("value") + for _, code := range []int{1, 2, 77} { + sc := bwsScenario(madeUp("token"), value) + sc.Steps = map[string]fakeStep{"secret get": { + Stdout: bwsSecretJSON(testItemID, value), Stderr: "Error: " + value, Exit: code, + }} + f := newTestFake(t, config.StoreBWS, sc, testDeps()) + v, err := f.store.Fetch(t.Context(), testItemID) + checkErr(t, err, ErrFailed) + if v.Bytes() != nil { + t.Error("Fetch returned a value with its error") + } + want := `store "main": bws secret get: the program failed (exit code ` + strconv.Itoa(code) + ")" + if err == nil || err.Error() != want { + t.Errorf("error = %v, want %q", err, want) + } + noCanary(t, "the error", err.Error(), value) + } +} + +func TestBWSWrongToken(t *testing.T) { + t.Parallel() + value := madeUp("value") + sc := bwsScenario(madeUp("token"), value) + f := newTestFake(t, config.StoreBWS, sc, testDeps()) + sc.Token = madeUp("other-token") // bws now refuses the token in the file + f.setScenario(sc) + _, err := f.store.Fetch(t.Context(), testItemID) + checkErr(t, err, ErrFailed) +} + +func TestBWSUnknownItem(t *testing.T) { + t.Parallel() + f := newTestFake(t, config.StoreBWS, bwsScenario(madeUp("token"), madeUp("value")), testDeps()) + _, err := f.store.Fetch(t.Context(), "00000000-0000-4000-8000-0000000000ff") + checkErr(t, err, ErrFailed) +} + +func TestBWSTimeoutKillsTheProcessGroup(t *testing.T) { + t.Parallel() + value := madeUp("value") + sc := bwsScenario(madeUp("token"), value) + sc.Steps = map[string]fakeStep{"secret get": {Hang: true, Stdout: bwsSecretJSON(testItemID, value)}} + d := testDeps() + d.timeout = 5 * time.Second + f := newTestFake(t, config.StoreBWS, sc, d) + start := time.Now() + _, err := f.store.Fetch(t.Context(), testItemID) + checkErr(t, err, ErrTimeout) + if err != nil && !strings.Contains(err.Error(), "(after 5s)") { + t.Errorf("error = %v, want the time limit in it", err) + } + if took := time.Since(start); took > 15*time.Second { + t.Errorf("Fetch took %v", took) + } + // The fake and the child it started are both gone. + waitGone(t, f.pids()) +} + +func TestCancelKillsTheProcessGroup(t *testing.T) { + t.Parallel() + sc := bwsScenario(madeUp("token"), madeUp("value")) + sc.Steps = map[string]fakeStep{"secret get": {Hang: true}} + f := newTestFake(t, config.StoreBWS, sc, testDeps()) + ctx, cancel := context.WithCancel(t.Context()) + done := make(chan error, 1) + go func() { + _, err := f.store.Fetch(ctx, testItemID) + done <- err + }() + // Wait until the fake has started its child. + deadline := time.Now().Add(10 * time.Second) + for { + if _, err := os.Stat(filepath.Join(f.home, fakePIDsFile)); err == nil { + break + } + if time.Now().After(deadline) { + t.Fatal("the fake did not start") + } + time.Sleep(10 * time.Millisecond) + } + cancel() + err := <-done + checkErr(t, err, context.Canceled) + waitGone(t, f.pids()) +} + +func TestBWSTooMuchOutput(t *testing.T) { + t.Parallel() + value := madeUp("value") + for _, stream := range []string{"stdout", "stderr"} { + t.Run(stream, func(t *testing.T) { + t.Parallel() + sc := bwsScenario(madeUp("token"), value) + sc.Steps = map[string]fakeStep{"secret get": {Flood: stream, Stdout: bwsSecretJSON(testItemID, value)}} + f := newTestFake(t, config.StoreBWS, sc, testDeps()) + start := time.Now() + v, err := f.store.Fetch(t.Context(), testItemID) + checkErr(t, err, ErrTooMuchOutput) + if v.Bytes() != nil { + t.Error("Fetch returned a value with its error") + } + // The cap stops the program, long before the time limit. + if took := time.Since(start); took > 10*time.Second { + t.Errorf("Fetch took %v", took) + } + if err != nil { + noCanary(t, "the error", err.Error(), value) + } + }) + } +} + +func TestBWSLeftRunning(t *testing.T) { + t.Parallel() + value := madeUp("value") + sc := bwsScenario(madeUp("token"), value) + // bws prints a good answer and exits with 0, but leaves a child that + // holds its stdout open. + sc.Steps = map[string]fakeStep{"secret get": {Leave: true, Stdout: bwsSecretJSON(testItemID, value)}} + d := testDeps() + d.waitDelay = 300 * time.Millisecond + f := newTestFake(t, config.StoreBWS, sc, d) + v, err := f.store.Fetch(t.Context(), testItemID) + checkErr(t, err, ErrLeftRunning) + if v.Bytes() != nil { + t.Error("Fetch returned a value with its error") + } + waitGone(t, f.pids()) +} + +func TestBWSMissingValue(t *testing.T) { + t.Parallel() + tests := map[string]struct { + stdout string + want error + }{ + "empty value": {bwsSecretJSON(testItemID, ""), ErrNoValue}, + "null value": {`{"id":"` + testItemID + `","value":null}`, ErrNoValue}, + "no value": {`{"id":"` + testItemID + `","key":"GITHUB_TOKEN","note":""}`, ErrNoValue}, + "empty object": {"{}\n", ErrBadAnswer}, // no id + // bws with a token of no organization logs an error and exits + // with 0, with nothing on stdout. + "no output": {"", ErrBadAnswer}, + // encoding/json would match these keys to "value". + "Value": {`{"id":"` + testItemID + `","Value":"x"}`, ErrNoValue}, + "VALUE": {`{"id":"` + testItemID + `","VALUE":"x"}`, ErrNoValue}, + "value number": {`{"value":7}`, ErrBadAnswer}, + "value object": {`{"value":{"x":"y"}}`, ErrBadAnswer}, + } + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + sc := bwsScenario(madeUp("token"), "") + sc.Steps = map[string]fakeStep{"secret get": {Stdout: tc.stdout}} + f := newTestFake(t, config.StoreBWS, sc, testDeps()) + v, err := f.store.Fetch(t.Context(), testItemID) + checkErr(t, err, tc.want) + if v.Bytes() != nil { + t.Error("Fetch returned a value with its error") + } + }) + } +} + +func TestBWSBadJSON(t *testing.T) { + t.Parallel() + value := madeUp("value") + good := bwsSecretJSON(testItemID, value) + tests := map[string]string{ + "two answers": good + good, + "text after": good + "done\n", + "cut short": good[:len(good)/2], + "an array": "[" + good + "]", + "a string": `"` + value + `"`, + "two values": `{"value":"` + value + `","value":"` + value + `"}`, + "color": "\x1b[38;5;231m" + good, + "bad escape": `{"value":"` + value + `\x"}`, + "lone surrogate": `{"value":"` + value + esc("d800") + `"}`, + "invalid UTF-8": `{"value":"` + value + "\xff\"}", + "control": "{\"value\":\"" + value + "\x01\"}", + "single quotes": `{'value':'` + value + `'}`, + "trailing comma": `{"value":"` + value + `",}`, + } + for name, stdout := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + sc := bwsScenario(madeUp("token"), value) + sc.Steps = map[string]fakeStep{"secret get": {Stdout: stdout}} + f := newTestFake(t, config.StoreBWS, sc, testDeps()) + v, err := f.store.Fetch(t.Context(), testItemID) + checkErr(t, err, ErrBadAnswer) + if v.Bytes() != nil { + t.Error("Fetch returned a value with its error") + } + if err != nil { + noCanary(t, "the error", err.Error(), value) + } + }) + } +} + +func TestFetchRefusesBadIDs(t *testing.T) { + t.Parallel() + for _, typ := range []config.StoreType{config.StoreBWS, config.StoreBW} { + f := newTestFake(t, typ, fakeScenario{Token: madeUp("token"), Password: madeUp("password")}, testDeps()) + for _, id := range []string{ + "", "-h", "--help", "--session", "github-token", testItemID + " ", " " + testItemID, + testItemID[:35], testItemID + "0", strings.ReplaceAll(testItemID, "-", "_"), + "6f1e2d3c-4b5a-4987-8a6b-5c4d3e2f1a0g", "-f1e2d3c-4b5a-4987-8a6b-5c4d3e2f1a0b", + } { + _, err := f.store.Fetch(t.Context(), id) + checkErr(t, err, ErrBadID) + } + if calls := f.calls(); len(calls) != 0 { + t.Errorf("%s ran %d times, want 0", typ, len(calls)) + } + } +} + +func TestBWSConcurrentFetches(t *testing.T) { + t.Parallel() + value := madeUp("value") + sc := bwsScenario(madeUp("token"), value) + sc.Pause = 100 * time.Millisecond + f := newTestFake(t, config.StoreBWS, sc, testDeps()) + const n = 12 + var wg sync.WaitGroup + errs := make([]error, n) + for i := range n { + wg.Go(func() { + v, err := f.store.Fetch(t.Context(), testItemID) + if err == nil && string(v.Bytes()) != value { + err = errors.New("wrong value") + } + v.Clear() + errs[i] = err + }) + } + wg.Wait() + for _, err := range errs { + if err != nil { + t.Errorf("Fetch: %v", err) + } + } + calls := f.calls() + if len(calls) != n { + t.Errorf("bws ran %d times, want %d", len(calls), n) + } + for _, c := range calls { + if c.Running > bwsParallel { + t.Errorf("%d runs of bws at once, want at most %d", c.Running, bwsParallel) + } + } +} + +func TestBWSTokenFile(t *testing.T) { + t.Parallel() + token := madeUp("token") + for name, content := range map[string]string{ + "newline": token + "\n", + "CRLF": token + "\r\n", + "spaces": " " + token + " \n", + "no newline": token, + "blank lines": "\n" + token + "\n\n", + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + sc := bwsScenario(token, madeUp("value")) + f := setupFake(t, config.StoreBWS, sc) + writeFile(t, f.cfg.TokenFile, content) + s, err := newStore(f.cfg, f.dir, testDeps()) + if err != nil { + t.Fatalf("newStore: %v", err) + } + defer s.Close() + if _, err := s.Fetch(t.Context(), testItemID); err != nil { + t.Errorf("Fetch: %v", err) + } + }) + } + for name, content := range map[string]string{ + "empty": "", + "only a space": " \n", + "two lines": token + "\n" + token + "\n", + "a space in it": token + " " + token, + "a control": token + "\x00", + "not ASCII": token + "รฉ", + "too long": strings.Repeat("t", maxTokenLen+1), + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + f := setupFake(t, config.StoreBWS, bwsScenario(token, madeUp("value"))) + writeFile(t, f.cfg.TokenFile, content) + _, err := newStore(f.cfg, f.dir, testDeps()) + if err == nil { + t.Fatal("newStore took a bad token file") + } + if !strings.Contains(err.Error(), "it holds no access token") { + t.Errorf("error = %v", err) + } + noCanary(t, "the error", err.Error(), token) + }) + } + // A token file that cannot be read. + f := setupFake(t, config.StoreBWS, bwsScenario(token, madeUp("value"))) + f.cfg.TokenFile = filepath.Join(t.TempDir(), "missing") + if _, err := newStore(f.cfg, f.dir, testDeps()); err == nil || !strings.Contains(err.Error(), "token_file") { + t.Errorf("newStore with a missing token file: %v", err) + } +} + +func TestBWSClose(t *testing.T) { + t.Parallel() + f := newTestFake(t, config.StoreBWS, bwsScenario(madeUp("token"), madeUp("value")), testDeps()) + s := f.store.(*bwsStore) + token := s.token + f.store.Close() + if token.Bytes() != nil { + t.Error("Close left the token") + } + _, err := f.store.Fetch(t.Context(), testItemID) + checkErr(t, err, ErrClosed) + checkErr(t, f.store.Start(t.Context()), ErrClosed) + if calls := f.calls(); len(calls) != 0 { + t.Errorf("bws ran %d times after Close, want 0", len(calls)) + } + f.store.Close() // a second Close does nothing +} + +func TestBWSFetchWaitsForAPlace(t *testing.T) { + t.Parallel() + sc := bwsScenario(madeUp("token"), madeUp("value")) + f := newTestFake(t, config.StoreBWS, sc, testDeps()) + s := f.store.(*bwsStore) + for range bwsParallel { + s.slots <- struct{}{} // every place is taken + } + ctx, cancel := context.WithTimeout(t.Context(), 100*time.Millisecond) + defer cancel() + _, err := s.Fetch(ctx, testItemID) + checkErr(t, err, context.DeadlineExceeded) + for range bwsParallel { + <-s.slots + } + if calls := f.calls(); len(calls) != 0 { + t.Errorf("bws ran %d times, want 0", len(calls)) + } +} diff --git a/internal/store/canary_test.go b/internal/store/canary_test.go new file mode 100644 index 0000000..6e44fb2 --- /dev/null +++ b/internal/store/canary_test.go @@ -0,0 +1,190 @@ +package store + +import ( + "bytes" + "encoding/json" + "fmt" + "log/slog" + "strings" + "testing" + "time" + + "github.com/code6x/nodkey/internal/config" +) + +// canaries are the made-up secrets of one canary test: they must never +// appear in an error, a log line, a printed store or value, or a program's +// arguments. +type canaries struct { + token, password, session, value, other string +} + +func newCanaries() canaries { + return canaries{ + token: madeUp("token"), + password: madeUp("password"), + session: madeUp("session"), + value: madeUp("value"), + other: madeUp("other"), + } +} + +func (c canaries) all() []string { + return []string{c.token, c.password, c.session, c.value, c.other} +} + +// check fails the test if text holds any canary. +func (c canaries) check(t *testing.T, where, text string) { + t.Helper() + for _, s := range c.all() { + noCanary(t, where, text, s) + } +} + +// checkCalls checks what the fake programs got: no canary in an +// argument; the token only in BWS_ACCESS_TOKEN and the session key only in +// BW_SESSION; the password, the value and the other secrets in no +// variable. +func (c canaries) checkCalls(t *testing.T, f *testFake) { + t.Helper() + calls := f.calls() + if len(calls) == 0 { + t.Fatal("no program ran") + } + for _, call := range calls { + for _, arg := range call.Args { + c.check(t, "an argument", arg) + } + for _, kv := range call.Env { + name, val, _ := strings.Cut(kv, "=") + noCanary(t, "the name of a variable", name, c.token) + for _, s := range []string{c.password, c.value, c.other} { + noCanary(t, "variable "+name, val, s) + } + if name != "BWS_ACCESS_TOKEN" { + noCanary(t, "variable "+name, val, c.token) + } + if name != "BW_SESSION" { + noCanary(t, "variable "+name, val, c.session) + } + } + } +} + +// TestCanaries runs both stores through success and many failures with +// made-up secrets: the bws token, the bw master password and session key, +// the value, and other secrets in the same item. In every case, no error, +// log line, printed value or printed store holds any of them, and the +// programs get them only where they must. +func TestCanaries(t *testing.T) { + t.Parallel() + type result struct { + where string + value Value + err error + } + type check struct { + name string + typ config.StoreType + setup func(c canaries, sc *fakeScenario, f *testFake) + want error // nil: Fetch must work + } + bwsAnswer := func(c canaries) string { + out, _ := json.MarshalIndent(map[string]any{"id": testItemID, "key": c.other, "value": c.value, "note": c.other}, "", " ") + return string(out) + "\n" + } + checks := []check{ + {name: "bws works", typ: config.StoreBWS}, + {"bws exits with 1", config.StoreBWS, func(c canaries, sc *fakeScenario, _ *testFake) { + sc.Steps = map[string]fakeStep{"secret get": {Stdout: bwsAnswer(c), Stderr: "Error: " + c.token + c.value, Exit: 1}} + }, ErrFailed}, + {"bws answers twice", config.StoreBWS, func(c canaries, sc *fakeScenario, _ *testFake) { + sc.Steps = map[string]fakeStep{"secret get": {Stdout: bwsAnswer(c) + bwsAnswer(c)}} + }, ErrBadAnswer}, + {"bws repeats value", config.StoreBWS, func(c canaries, sc *fakeScenario, _ *testFake) { + sc.Steps = map[string]fakeStep{"secret get": {Stdout: `{"value":"` + c.value + `","value":"` + c.other + `"}`}} + }, ErrBadAnswer}, + {"bws floods", config.StoreBWS, func(c canaries, sc *fakeScenario, _ *testFake) { + sc.Steps = map[string]fakeStep{"secret get": {Stdout: bwsAnswer(c), Flood: "stdout"}} + }, ErrTooMuchOutput}, + {"bws hangs", config.StoreBWS, func(c canaries, sc *fakeScenario, _ *testFake) { + sc.Steps = map[string]fakeStep{"secret get": {Stdout: bwsAnswer(c), Hang: true}} + }, ErrTimeout}, + {name: "bw works", typ: config.StoreBW}, + {"bw get item exits with 1", config.StoreBW, func(c canaries, sc *fakeScenario, _ *testFake) { + sc.Steps = map[string]fakeStep{"get item": { + Stdout: bwItemJSON(testItemID, c.value, nil, c.other), Stderr: c.session + c.password, Exit: 1, + }} + }, ErrFailed}, + {"bw item in the trash", config.StoreBW, func(c canaries, sc *fakeScenario, _ *testFake) { + sc.Items[testItemID] = bwItemJSON(testItemID, c.value, "2026-10-13T12:00:00.000Z", c.other) + }, ErrTrashed}, + {"bw unlock talks", config.StoreBW, func(c canaries, sc *fakeScenario, _ *testFake) { + sc.Steps = map[string]fakeStep{"unlock": {Stdout: "Your vault is now unlocked!\n$ export BW_SESSION=\"" + c.session + "\"\n"}} + }, ErrBadAnswer}, + {"bw is locked", config.StoreBW, func(c canaries, _ *fakeScenario, f *testFake) { + if err := f.store.Start(t.Context()); err != nil { + t.Fatalf("Start: %v", err) + } + f.setBWState(fakeBWState{Session: c.other}) + }, ErrLocked}, + {"bw is logged out", config.StoreBW, func(_ canaries, _ *fakeScenario, f *testFake) { + f.setBWState(fakeBWState{LoggedOut: true}) + }, ErrLoggedOut}, + {"bw status floods", config.StoreBW, func(c canaries, sc *fakeScenario, _ *testFake) { + sc.Steps = map[string]fakeStep{ + "sync": {Stderr: c.session, Exit: 1}, + "status": {Stdout: c.session + c.password, Flood: "stdout"}, + } + }, ErrFailed}, + } + for _, tc := range checks { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + c := newCanaries() + sc := bwsScenario(c.token, c.value) + sc.Items[testItemID] = bwsAnswer(c) + if tc.typ == config.StoreBW { + sc = bwScenario(c.password, c.session, c.value, c.other) + } + d := testDeps() + d.timeout = 5 * time.Second + f := newTestFake(t, tc.typ, sc, d) + if tc.setup != nil { + tc.setup(c, &sc, f) + f.setScenario(sc) + } + var logs bytes.Buffer + logger := slog.New(slog.NewTextHandler(&logs, nil)) + jsonLogger := slog.New(slog.NewJSONHandler(&logs, nil)) + startErr := f.store.Start(t.Context()) + v, err := f.store.Fetch(t.Context(), testItemID) + results := []result{{"Start", Value{}, startErr}, {"Fetch", v, err}} + if tc.want == nil { + if err != nil { + t.Fatalf("Fetch: %v", err) + } + checkValue(t, v, c.value) + } else { + checkErr(t, err, tc.want) + } + for _, r := range results { + logger.Error(r.where, "err", r.err, "value", r.value) + jsonLogger.Error(r.where, "err", r.err, "value", r.value) + logger.Info(r.where, slog.Any("value", r.value), slog.Any("store", f.store)) + var texts []string + if r.err != nil { + texts = append(texts, r.err.Error(), fmt.Sprintf("%v %+v %#v", r.err, r.err, r.err)) + } + texts = append(texts, fmt.Sprintf("%v %+v %#v %s %x", r.value, r.value, r.value, r.value, r.value)) + for _, text := range texts { + c.check(t, r.where, text) + } + } + c.check(t, "the logs", logs.String()) + c.check(t, "the printed store", fmt.Sprintf("%v %+v %#v %s", f.store, f.store, f.store, f.store)) + c.checkCalls(t, f) + v.Clear() + }) + } +} diff --git a/internal/store/checks_test.go b/internal/store/checks_test.go new file mode 100644 index 0000000..54d26b1 --- /dev/null +++ b/internal/store/checks_test.go @@ -0,0 +1,362 @@ +package store + +import ( + "errors" + "io/fs" + "os" + "os/exec" + "path/filepath" + "slices" + "strings" + "syscall" + "testing" + "time" + + "github.com/code6x/nodkey/internal/config" +) + +// TestBWChecksItsPathDirectories shows that New checks each directory in +// bw's PATH like the directories on the way to a program, and that a +// directory that is not there passes when the way to it passes. +func TestBWChecksItsPathDirectories(t *testing.T) { + t.Parallel() + f := setupFake(t, config.StoreBW, fakeScenario{Password: madeUp("password")}) + missing := filepath.Join(t.TempDir(), "missing") + d := testDeps() + d.path = "/usr/local/bin:" + missing + var checked []string + bad := errors.New("made-up: group or others can write to this directory") + d.checkParentDirs = func(path string) error { + checked = append(checked, path) + switch path { + case filepath.Join(missing, "node"): + return &fs.PathError{Op: "lstat", Path: missing, Err: fs.ErrNotExist} + case "/usr/local/bin/node": + if len(checked) > 4 { // the second store below + return bad + } + } + return nil + } + s, err := newStore(f.cfg, f.dir, d) + if err != nil { + t.Fatalf("newStore: %v", err) + } + s.Close() + want := []string{f.home, "/usr/local/bin/node", filepath.Join(missing, "node"), missing} + if !slices.Equal(checked, want) { + t.Errorf("checked %q, want %q", checked, want) + } + // A PATH directory that fails the check stops New. + _, err = newStore(f.cfg, f.dir, d) + checkErr(t, err, bad) + if err != nil && !strings.Contains(err.Error(), `store "vault": PATH directory /usr/local/bin: `) { + t.Errorf("error = %v", err) + } + // bws gets no PATH, so its PATH is not checked. + checked = nil + b := setupFake(t, config.StoreBWS, fakeScenario{Token: madeUp("token")}) + s, err = newStore(b.cfg, b.dir, d) + if err != nil { + t.Fatalf("newStore of bws: %v", err) + } + s.Close() + if !slices.Equal(checked, []string{b.home}) { + t.Errorf("bws checked %q, want only its home", checked) + } + // A PATH directory must be a clean absolute path. + for _, path := range []string{"bin", "/usr/bin:", "/usr//bin", "/usr/bin/../bin"} { + none := func(string) error { return nil } + if err := checkPathDirs(path, none, none); err == nil { + t.Errorf("checkPathDirs(%q) took it", path) + } + } +} + +// TestPathDirectoriesRealCheck runs checkPathDirs with the real check of +// the config package on a directory that everyone can write to. +func TestPathDirectoriesRealCheck(t *testing.T) { + t.Parallel() + open := filepath.Join(t.TempDir(), "open") + if err := os.Mkdir(open, 0o700); err != nil { + t.Fatal(err) + } + if err := os.Chmod(open, 0o777); err != nil { //nolint:gosec // G302: the test needs a directory that others can write to. + t.Fatal(err) + } + for _, path := range []string{open, filepath.Join(open, "missing")} { + err := checkPathDirs(path, config.CheckParentDirs, config.CheckProgram) + if err == nil || !strings.Contains(err.Error(), "group or others can write to this directory") { + t.Errorf("checkPathDirs(%s): %v", path, err) + } + } +} + +// TestBWPathNodeIsCheckedLikeTheProgram comes from the second security +// review of issue #8. The node that bw's PATH gives a bw script reads the +// master password file and gets the session key, like the program itself. +// So it must pass the checks of a program: owned by root or the broker +// user, not writable by group or others, also after its links, and the +// way to its target too. config.TestCheckProgram tests the check itself. +func TestBWPathNodeIsCheckedLikeTheProgram(t *testing.T) { + t.Parallel() + for name, setup := range map[string]func(t *testing.T, dir string){ + "a node that others can write": func(t *testing.T, dir string) { + node := filepath.Join(dir, "node") + writeFile(t, node, "") + if err := os.Chmod(node, 0o777); err != nil { //nolint:gosec // G302: the test needs a file that others can write to. + t.Fatal(err) + } + }, + "a node that links to a file that others can write": func(t *testing.T, dir string) { + target := filepath.Join(t.TempDir(), "node") + writeFile(t, target, "") + if err := os.Chmod(target, 0o777); err != nil { //nolint:gosec // G302: the test needs a file that others can write to. + t.Fatal(err) + } + if err := os.Symlink(target, filepath.Join(dir, "node")); err != nil { + t.Fatal(err) + } + }, + "a node that links to a missing file": func(t *testing.T, dir string) { + if err := os.Symlink(filepath.Join(t.TempDir(), "node"), filepath.Join(dir, "node")); err != nil { + t.Fatal(err) + } + }, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + f := setupFake(t, config.StoreBW, fakeScenario{Password: madeUp("password")}) + dir := t.TempDir() + setup(t, dir) + d := testDeps() + d.path = dir + d.checkProgram = config.CheckProgram // the real check + s, err := newStore(f.cfg, f.dir, d) + if err == nil { + s.Close() + t.Fatal("newStore took it") + } + if !strings.Contains(err.Error(), `store "vault": PATH directory `+dir+": node: ") { + t.Errorf("error = %v", err) + } + }) + } + // Each node is checked, by its path in PATH: the check follows the + // links and checks the way to the target. A directory without node + // has nothing to check. + f := setupFake(t, config.StoreBW, fakeScenario{Password: madeUp("password")}) + dir, empty, target := t.TempDir(), t.TempDir(), filepath.Join(t.TempDir(), "node") + writeFile(t, target, "") + if err := os.Symlink(target, filepath.Join(dir, "node")); err != nil { + t.Fatal(err) + } + d := testDeps() + d.path = empty + ":" + dir + var checked []string + d.checkProgram = func(p string) error { checked = append(checked, p); return nil } + s, err := newStore(f.cfg, f.dir, d) + if err != nil { + t.Fatalf("newStore: %v", err) + } + s.Close() + if want := []string{filepath.Join(dir, "node")}; !slices.Equal(checked, want) { + t.Errorf("checked %q, want %q", checked, want) + } + // A failed check stops New. + bad := errors.New("made-up: group or others can write to it") + d.checkProgram = func(string) error { return bad } + if _, err := newStore(f.cfg, f.dir, d); !errors.Is(err, bad) { + t.Errorf("newStore with a bad node: %v", err) + } +} + +// TestBWDataIsRefused shows that New refuses a bw-data directory next to +// bw's executable, where bw would keep its state instead of in its home. +func TestBWDataIsRefused(t *testing.T) { + t.Parallel() + makeDir := func(t *testing.T, path string) { + t.Helper() + if err := os.MkdirAll(path, 0o700); err != nil { + t.Fatal(err) + } + } + tests := map[string]func(t *testing.T, f *testFake, d *deps){ + "next to the program": func(t *testing.T, f *testFake, _ *deps) { + makeDir(t, filepath.Join(filepath.Dir(f.cfg.Program), bwDataDir)) + }, + "a file next to the program": func(t *testing.T, f *testFake, _ *deps) { + writeFile(t, filepath.Join(filepath.Dir(f.cfg.Program), bwDataDir), "") + }, + "next to the file that the program links to": func(t *testing.T, f *testFake, _ *deps) { + real := filepath.Join(t.TempDir(), "real", "bw") + makeDir(t, filepath.Dir(real)) + writeFile(t, real, "") + makeDir(t, filepath.Join(filepath.Dir(real), bwDataDir)) + if err := os.Remove(f.cfg.Program); err != nil { + t.Fatal(err) + } + if err := os.Symlink(real, f.cfg.Program); err != nil { + t.Fatal(err) + } + }, + "next to node in PATH": func(t *testing.T, _ *testFake, d *deps) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "node"), "") + makeDir(t, filepath.Join(dir, bwDataDir)) + d.path = "/usr/local/bin:" + dir + }, + "next to the file that node links to": func(t *testing.T, _ *testFake, d *deps) { + dir, real := t.TempDir(), t.TempDir() + writeFile(t, filepath.Join(real, "node"), "") + makeDir(t, filepath.Join(real, bwDataDir)) + if err := os.Symlink(filepath.Join(real, "node"), filepath.Join(dir, "node")); err != nil { + t.Fatal(err) + } + d.path = dir + }, + } + for name, setup := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + f := setupFake(t, config.StoreBW, fakeScenario{Password: madeUp("password")}) + d := testDeps() + setup(t, f, &d) + _, err := newStore(f.cfg, f.dir, d) + if err == nil || !strings.Contains(err.Error(), "/bw-data: bw would keep its state there") { + t.Errorf("newStore: %v", err) + } + }) + } + // A program that is not there cannot be checked. + f := setupFake(t, config.StoreBW, fakeScenario{Password: madeUp("password")}) + if err := os.Remove(f.cfg.Program); err != nil { + t.Fatal(err) + } + if _, err := newStore(f.cfg, f.dir, testDeps()); !errors.Is(err, fs.ErrNotExist) { + t.Errorf("newStore without the program: %v", err) + } + // bws does not use bw-data. + b := setupFake(t, config.StoreBWS, fakeScenario{Token: madeUp("token")}) + makeDir(t, filepath.Join(filepath.Dir(b.cfg.Program), bwDataDir)) + s, err := newStore(b.cfg, b.dir, testDeps()) + if err != nil { + t.Fatalf("newStore of bws: %v", err) + } + s.Close() +} + +// TestBWPasswordFileSize shows that the password file must hold 1 byte to +// 1 MiB, at New and before each unlock. +func TestBWPasswordFileSize(t *testing.T) { + t.Parallel() + password := madeUp("password") + for name, tc := range map[string]struct { + content string + want string + }{ + "empty": {"", "is empty"}, + "too big": {password + "\n" + strings.Repeat("x", maxPasswordFileSize), "is larger than 1 MiB"}, + "a folder": {"", "is not a regular file"}, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + f := setupFake(t, config.StoreBW, bwScenario(password, "", madeUp("value"), madeUp("other"))) + setFile := func() { + if name == "a folder" { + if err := os.RemoveAll(f.cfg.PasswordFile); err != nil { + t.Fatal(err) + } + if err := os.Mkdir(f.cfg.PasswordFile, 0o700); err != nil { + t.Fatal(err) + } + return + } + writeFile(t, f.cfg.PasswordFile, tc.content) + } + setFile() + _, err := newStore(f.cfg, f.dir, testDeps()) + if err == nil || !strings.Contains(err.Error(), tc.want) { + t.Errorf("newStore: %v, want an error with %q", err, tc.want) + } + // The file was good at New, and then changed. + if err := os.RemoveAll(f.cfg.PasswordFile); err != nil { + t.Fatal(err) + } + writeFile(t, f.cfg.PasswordFile, password+"\n") + s, err := newStore(f.cfg, f.dir, testDeps()) + if err != nil { + t.Fatalf("newStore: %v", err) + } + defer s.Close() + setFile() + err = s.Start(t.Context()) + if err == nil || !strings.Contains(err.Error(), `bw unlock: password_file: `) || !strings.Contains(err.Error(), tc.want) { + t.Errorf("Start: %v, want an error with %q", err, tc.want) + } + if calls := f.calls(); len(calls) != 0 { + t.Errorf("bw ran %d times, want 0", len(calls)) + } + }) + } +} + +// brokerHomeVar names the home directory of a store, in a child process +// of TestProgramDiesWithTheBroker that acts as the broker. +const brokerHomeVar = "NODKEY_TEST_BROKER_STORE" + +// TestProgramDiesWithTheBroker starts a child process that acts as the +// broker and runs a store program that hangs. Then it kills the child with +// SIGKILL, so the broker cannot clean up. The program must die too +// (Pdeathsig). +func TestProgramDiesWithTheBroker(t *testing.T) { + if dir := os.Getenv(brokerHomeVar); dir != "" { + // The child: a bws store whose program hangs. Fetch never returns. + cfg := config.Store{ + Name: "main", Type: config.StoreBWS, Program: filepath.Join(dir, "bin", "bws"), + TokenFile: filepath.Join(dir, "bws-token"), + } + s, err := newStore(cfg, filepath.Join(dir, "state"), testDeps()) + if err != nil { + t.Fatalf("newStore: %v", err) + } + _, err = s.Fetch(t.Context(), testItemID) + t.Fatalf("Fetch returned: %v", err) + } + t.Parallel() + sc := bwsScenario(madeUp("token"), madeUp("value")) + sc.Steps = map[string]fakeStep{"secret get": {Hang: true}} + f := setupFake(t, config.StoreBWS, sc) + broker := exec.CommandContext(t.Context(), os.Args[0], "-test.run=^TestProgramDiesWithTheBroker$") //nolint:gosec // G204: the test runs its own binary again. + broker.Env = append(os.Environ(), brokerHomeVar+"="+filepath.Dir(f.dir)) + if err := broker.Start(); err != nil { + t.Fatal(err) + } + done := make(chan error, 1) + go func() { done <- broker.Wait() }() + // Wait until the program runs: it writes its process IDs. + deadline := time.Now().Add(20 * time.Second) + for { + if _, err := os.Stat(filepath.Join(f.home, fakePIDsFile)); err == nil { + break + } + select { + case err := <-done: + t.Fatalf("the broker ended early: %v", err) + default: + } + if time.Now().After(deadline) { + t.Fatal("the program did not start") + } + time.Sleep(10 * time.Millisecond) + } + pids := f.pids() + // The program's own child is in its process group, which only the + // broker kills. Kill it when the test ends. + t.Cleanup(func() { _ = syscall.Kill(pids[1], syscall.SIGKILL) }) + if err := broker.Process.Signal(syscall.SIGKILL); err != nil { + t.Fatal(err) + } + <-done + waitGone(t, pids[:1]) +} diff --git a/internal/store/fake_test.go b/internal/store/fake_test.go new file mode 100644 index 0000000..38cddab --- /dev/null +++ b/internal/store/fake_test.go @@ -0,0 +1,382 @@ +package store + +import ( + "bufio" + "bytes" + "crypto/rand" + "encoding/base64" + "encoding/gob" + "encoding/json" + "errors" + "io/fs" + "os" + "os/exec" + "path/filepath" + "slices" + "strconv" + "strings" + "syscall" + "testing" + "time" +) + +// The fake store programs. The tests make a symbolic link named bws or bw +// to the test binary, and give its path to the store as the program. When +// the store runs it, TestMain sees the name and acts as that program. +// +// The fake gets only what the store gives a real program: its arguments, +// HOME, PATH and the token or session variable. So it reads what to do +// from fakeScenarioFile in HOME, and appends a fakeCall for each run to +// fakeCallsFile there. The fake bw keeps its state in fakeBWStateFile. + +// The files of the fake programs in the store's home directory. +const ( + fakeScenarioFile = "fake-scenario.gob" + fakeCallsFile = "fake-calls.jsonl" + fakeBWStateFile = "fake-bw-state.json" + fakePIDsFile = "fake-pids" + fakeRunningDir = "fake-running" +) + +// The fake programs write their answers and errors to stdout and stderr, +// as bws and bw do. Only the fakes use these; the tests never print. +var ( + fakeStdout = os.Stdout //nolint:forbidigo // the fake program answers on stdout, as bws and bw do. + fakeStderr = os.Stderr //nolint:forbidigo // the fake program writes errors to stderr, as bws and bw do. +) + +// fakeSleeper is the name under which the test binary only sleeps. The +// fake programs start it as a child, to test that a store stops the whole +// process group. +const fakeSleeper = "fake-sleeper" + +// TestMain runs the fake programs, or else the tests. +// +// A fake ends with syscall.Exit, not os.Exit: with the race detector, a +// program that exits with 0 through os.Exit first sleeps for a second +// (GORACE atexit_sleep_ms), and the fake gets no GORACE to turn it off. +// The fake writes to stdout and stderr without a buffer, so nothing is +// lost. +func TestMain(m *testing.M) { + switch filepath.Base(os.Args[0]) { + case "bws": + syscall.Exit(runFake("bws")) + case "bw": + syscall.Exit(runFake("bw")) + case fakeSleeper: + time.Sleep(time.Hour) + syscall.Exit(0) + } + os.Exit(m.Run()) +} + +// fakeScenario says what a fake program does. +type fakeScenario struct { + Token string `json:"token"` // bws: the access token that works + Password string `json:"password"` // bw: the master password, the first line of the password file + Session string `json:"session"` // bw: the session key that unlock makes; random if empty + Items map[string]string `json:"items"` // the answer for each item ID, in lower case + Steps map[string]fakeStep `json:"steps"` // what a step does instead of its usual work + Pause time.Duration `json:"pause"` // each run waits this long first +} + +// fakeStep is what a step does instead of its usual work. The steps are +// "secret get" for bws, and "unlock", "sync", "get item" and "status" for +// bw. +type fakeStep struct { + Stdout string `json:"stdout"` + Stderr string `json:"stderr"` + Exit int `json:"exit"` + // Hang starts a child that sleeps, writes both process IDs to + // fakePIDsFile, writes Stdout, and sleeps. + Hang bool `json:"hang"` + // Flood writes Stdout, then writes to "stdout" or "stderr" without end. + Flood string `json:"flood"` + // Leave starts a child that sleeps with the same stdout, writes both + // process IDs to fakePIDsFile, writes Stdout, and exits with Exit. + Leave bool `json:"leave"` +} + +// fakeBWState is the state of the fake bw: its account and its session. +type fakeBWState struct { + LoggedOut bool `json:"loggedOut"` + Session string `json:"session"` // the session key that works; empty when the vault is locked +} + +// fakeCall is what a fake program got in one run. +type fakeCall struct { + Program string `json:"program"` + Args []string `json:"args"` + Env []string `json:"env"` + Dir string `json:"dir"` + Stdin string `json:"stdin"` // what stdin is, such as /dev/null + Leader bool `json:"leader"` // the run leads its own process group + Running int `json:"running"` // runs of fakes in this home at that moment, this one too +} + +// runFake is the main function of a fake program. It returns the exit +// code. +func runFake(program string) int { + home := os.Getenv("HOME") + if home == "" { + _, _ = fakeStderr.WriteString("fake: no HOME\n") + return 99 + } + var sc fakeScenario + if err := readGob(filepath.Join(home, fakeScenarioFile), &sc); err != nil { + _, _ = fakeStderr.WriteString("fake: no scenario\n") + return 99 + } + running := filepath.Join(home, fakeRunningDir, strconv.Itoa(os.Getpid())) + if err := os.MkdirAll(filepath.Dir(running), 0o700); err != nil { //nolint:gosec // G703: the fake writes only in the home directory that its test made. + return 99 + } + if err := os.WriteFile(running, nil, 0o600); err != nil { //nolint:gosec // G703: the fake writes only in the home directory that its test made. + return 99 + } + entries, _ := os.ReadDir(filepath.Dir(running)) + stdin, _ := os.Readlink("/proc/self/fd/0") + dir, _ := os.Getwd() + call := fakeCall{ + Program: program, + Args: os.Args[1:], + Env: os.Environ(), + Dir: dir, + Stdin: stdin, + Leader: syscall.Getpgrp() == os.Getpid(), + Running: len(entries), + } + if err := appendJSONLine(filepath.Join(home, fakeCallsFile), call); err != nil { + return 99 + } + time.Sleep(sc.Pause) + var code int + if program == "bws" { + code = fakeBWS(home, &sc) + } else { + code = fakeBW(home, &sc) + } + _ = os.Remove(running) //nolint:gosec // G703: the fake removes only its own file in the home directory that its test made. + return code +} + +// fakeBWS acts as bws. +func fakeBWS(home string, sc *fakeScenario) int { + args := os.Args[1:] + if len(args) < 3 || args[0] != "secret" || args[1] != "get" { + return fakeFail("error: unexpected arguments", 2) + } + if step, ok := sc.Steps["secret get"]; ok { + return fakeOverride(home, step) + } + if os.Getenv("BWS_ACCESS_TOKEN") != sc.Token || sc.Token == "" { + return fakeFail("Error: \n 0: Access token is not in a valid format", 1) + } + item, ok := sc.Items[strings.ToLower(args[2])] + if !ok { + return fakeFail("Error: \n 0: Received error message from server: [404 Not Found]", 1) + } + _, _ = fakeStdout.WriteString(item) + return 0 +} + +// fakeBW acts as bw. +func fakeBW(home string, sc *fakeScenario) int { + args := os.Args[1:] + step := "" + switch { + case len(args) > 0 && slices.Contains([]string{"unlock", "sync", "status"}, args[0]): + step = args[0] + case len(args) > 2 && args[0] == "get" && args[1] == "item": + step = "get item" + default: + return fakeFail("Invalid command: "+strings.Join(args, " "), 1) + } + if o, ok := sc.Steps[step]; ok { + return fakeOverride(home, o) + } + var st fakeBWState + if err := readJSON(filepath.Join(home, fakeBWStateFile), &st); err != nil && !errors.Is(err, fs.ErrNotExist) { + return 99 + } + session := os.Getenv("BW_SESSION") + unlocked := st.Session != "" && session == st.Session + switch step { + case "status": + status := "locked" + switch { + case st.LoggedOut: + status = "unauthenticated" + case unlocked: + status = "unlocked" + } + out, _ := json.Marshal(map[string]any{ + "serverUrl": "https://bitwarden.example.com", "lastSync": "2026-10-14T12:00:00.000Z", + "userEmail": "nodkey@example.com", "userId": "00000000-0000-0000-0000-000000000000", "status": status, + }) + _, _ = fakeStdout.Write(out) + return 0 + case "unlock": + if st.LoggedOut { + return fakeFail("You are not logged in.", 1) + } + i := slices.Index(args, "--passwordfile") + if i < 0 || i+1 >= len(args) { + return fakeFail("Master password is required.", 1) + } + password, err := firstLine(args[i+1]) + if err != nil || password != sc.Password { + return fakeFail("Invalid master password.", 1) + } + st.Session = sc.Session + if st.Session == "" { + key := make([]byte, 64) + _, _ = rand.Read(key) + st.Session = base64.StdEncoding.EncodeToString(key) + } + if err := writeJSON(filepath.Join(home, fakeBWStateFile), st); err != nil { + return 99 + } + if !slices.Contains(args, "--raw") { + _, _ = fakeStdout.WriteString("Your vault is now unlocked!\n\n$ export BW_SESSION=\"" + st.Session + "\"\n") + return 0 + } + _, _ = fakeStdout.WriteString(st.Session) + return 0 + case "sync": + if st.LoggedOut { + return fakeFail("You are not logged in.", 1) + } + _, _ = fakeStdout.WriteString("Syncing complete.") + return 0 + } + // get item + switch { + case st.LoggedOut: + return fakeFail("You are not logged in.", 1) + case !unlocked: + return fakeFail("Vault is locked.", 1) + } + item, ok := sc.Items[strings.ToLower(args[2])] + if !ok { + return fakeFail("Not found.", 1) + } + _, _ = fakeStdout.WriteString(item) + return 0 +} + +// fakeOverride does what step says. +func fakeOverride(home string, step fakeStep) int { + if step.Hang || step.Leave { + exe, err := os.Executable() + if err != nil { + return 99 + } + child := exec.Cmd{Path: exe, Args: []string{fakeSleeper}} + if step.Leave { + child.Stdout = fakeStdout + } + if err := child.Start(); err != nil { + return 99 + } + pids := strconv.Itoa(os.Getpid()) + " " + strconv.Itoa(child.Process.Pid) + "\n" + if err := os.WriteFile(filepath.Join(home, fakePIDsFile), []byte(pids), 0o600); err != nil { //nolint:gosec // G703: the fake writes only in the home directory that its test made. + return 99 + } + } + _, _ = fakeStdout.WriteString(step.Stdout) + _, _ = fakeStderr.WriteString(step.Stderr) + switch { + case step.Hang: + time.Sleep(time.Hour) + case step.Flood != "": + w := fakeStdout + if step.Flood == "stderr" { + w = fakeStderr + } + chunk := []byte(strings.Repeat("flood ", 1000)) + for { + if _, err := w.Write(chunk); err != nil { + return 98 + } + } + } + return step.Exit +} + +// fakeFail writes msg to stderr, as bws and bw write their errors, and +// returns code. +func fakeFail(msg string, code int) int { + _, _ = fakeStderr.WriteString(msg) + return code +} + +// firstLine returns the first line of a file, as bw reads its password +// file. +func firstLine(path string) (string, error) { + f, err := os.Open(path) //nolint:gosec // G304: the fake reads the password file that its test made. + if err != nil { + return "", err + } + defer func() { _ = f.Close() }() + s := bufio.NewScanner(f) + if !s.Scan() { + return "", errors.New("empty") + } + return strings.TrimSuffix(s.Text(), "\r"), nil +} + +// readGob reads a gob file into v. The scenario is a gob, not JSON: +// encoding/json would change invalid UTF-8 in an answer that a test wants +// the fake to print as it is. +func readGob(path string, v any) error { + data, err := os.ReadFile(path) //nolint:gosec // G304: tests read only files in their own temporary directory. + if err != nil { + return err + } + return gob.NewDecoder(bytes.NewReader(data)).Decode(v) //nolint:gosec // G709: the fake reads the scenario that its own test wrote. +} + +// writeGob writes v to a gob file. +func writeGob(path string, v any) error { + var buf bytes.Buffer + if err := gob.NewEncoder(&buf).Encode(v); err != nil { + return err + } + return os.WriteFile(path, buf.Bytes(), 0o600) +} + +// readJSON reads a JSON file into v. +func readJSON(path string, v any) error { + data, err := os.ReadFile(path) //nolint:gosec // G304: tests read only files in their own temporary directory. + if err != nil { + return err + } + return json.Unmarshal(data, v) +} + +// writeJSON writes v to a JSON file. +func writeJSON(path string, v any) error { + data, err := json.Marshal(v) + if err != nil { + return err + } + return os.WriteFile(path, data, 0o600) //nolint:gosec // G703: tests and fakes write only in their own temporary directory. +} + +// appendJSONLine appends v to a file as one line, with one write. +func appendJSONLine(path string, v any) error { + data, err := json.Marshal(v) + if err != nil { + return err + } + f, err := os.OpenFile(path, os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0o600) //nolint:gosec // G304: tests write only files in their own temporary directory. + if err != nil { + return err + } + _, err = f.Write(append(data, '\n')) + if cerr := f.Close(); err == nil { + err = cerr + } + return err +} diff --git a/internal/store/fuzz_test.go b/internal/store/fuzz_test.go new file mode 100644 index 0000000..05fcd3a --- /dev/null +++ b/internal/store/fuzz_test.go @@ -0,0 +1,111 @@ +package store + +import ( + "encoding/json" + "strings" + "testing" +) + +// The fuzz tests compare nodkey's JSON reader with encoding/json. nodkey's +// reader is stricter, so the check goes one way: an answer that nodkey +// takes must be valid for encoding/json too, and both must read the same +// value from it. + +// fuzzSeeds are answers for every fuzz test. +var fuzzSeeds = []string{ + `{}`, `{"value":"x"}`, `{"value":null}`, `{"value":""}`, `{"value":"a","value":"b"}`, + `{"id":"` + testItemID + `","value":"x"}`, `{"value":"x","id":"` + strings.ToUpper(testItemID) + `"}`, + `{"id":"` + testItemID + `","login":{"password":"p"},"deletedDate":null,"archivedDate":null}`, + `{"id":"` + testItemID + `","login":{"password":"p` + esc("0000") + `"}}`, + `{"id":"` + testItemID + `","archivedDate":"2026-10-14","login":{"password":"p"}}`, + `{"login":{"password":"p"},"deletedDate":null}`, `{"login":null}`, `{"deletedDate":"2026-10-14"}`, + `{"status":"locked"}`, `{"status":"unlocked","userEmail":"nodkey@example.com"}`, + `{"value":"` + esc("d83d", "de00") + `"}`, `{"value":"` + esc("d800") + `"}`, "{\"value\":\"\xff\"}", + `{"a":[1,-0.5e+3,true,false,null,{"b":[]}],"value":"x"}`, ` {"value":"x"} `, `{"value":"x"}{}`, + `{"` + esc("0076") + `alue":"x"}`, `{"Value":"x"}`, +} + +func FuzzBWSSecret(f *testing.F) { + for _, s := range append(fuzzSeeds, realBWSAnswer, bwsSecretJSON(testItemID, "made-up")) { + f.Add([]byte(s)) + } + f.Fuzz(func(t *testing.T, data []byte) { + v, err := parseBWSSecret(data, testItemID) + if err != nil { + if v.Bytes() != nil { + t.Fatal("a value with an error") + } + return + } + top := fuzzTop(t, data) + var s string + if err := json.Unmarshal(top["value"], &s); err != nil || s != string(v.Bytes()) || s == "" || strings.ContainsRune(s, 0) { + t.Fatalf("encoding/json reads another value: %d bytes, %v; nodkey: %d bytes", len(s), err, len(v.Bytes())) + } + }) +} + +func FuzzBWItem(f *testing.F) { + for _, s := range append(fuzzSeeds, realBWItem, bwItemJSON(testItemID, "made-up", nil, "other")) { + f.Add([]byte(s)) + } + f.Fuzz(func(t *testing.T, data []byte) { + v, err := parseBWItem(data, testItemID) + if err != nil { + if v.Bytes() != nil { + t.Fatal("a value with an error") + } + return + } + top := fuzzTop(t, data) + for _, date := range []string{"deletedDate", "archivedDate"} { + if d, ok := top[date]; ok && string(d) != "null" { + t.Fatalf("nodkey took an item with %s %s", date, d) + } + } + var login map[string]json.RawMessage + if err := json.Unmarshal(top["login"], &login); err != nil { + t.Fatalf("encoding/json reads no login: %v", err) + } + var s string + if err := json.Unmarshal(login["password"], &s); err != nil || s != string(v.Bytes()) || s == "" || strings.ContainsRune(s, 0) { + t.Fatalf("encoding/json reads another password: %d bytes, %v; nodkey: %d bytes", len(s), err, len(v.Bytes())) + } + }) +} + +// fuzzTop reads an answer that nodkey took with encoding/json, and checks +// what both readers must agree on: the answer is valid, its "id" is the +// asked ID, and the value has no NUL byte. +func fuzzTop(t *testing.T, data []byte) map[string]json.RawMessage { + t.Helper() + var top map[string]json.RawMessage + if err := json.Unmarshal(data, &top); err != nil { + t.Fatalf("nodkey took an answer that encoding/json refuses: %v", err) + } + var id string + if err := json.Unmarshal(top["id"], &id); err != nil || !strings.EqualFold(id, testItemID) || len(id) != len(testItemID) { + t.Fatalf("nodkey took an answer for another item: %q, %v", id, err) + } + return top +} + +func FuzzBWStatus(f *testing.F) { + for _, s := range fuzzSeeds { + f.Add([]byte(s)) + } + f.Fuzz(func(t *testing.T, data []byte) { + status, err := parseBWStatus(data) + if err != nil { + return + } + var top map[string]json.RawMessage + if err := json.Unmarshal(data, &top); err != nil { + t.Fatalf("nodkey took an answer that encoding/json refuses: %v", err) + } + var s string + if err := json.Unmarshal(top["status"], &s); err != nil || s != status { + t.Fatalf("encoding/json reads another status: %q, %v; nodkey: %q", s, err, status) + } + }) +} diff --git a/internal/store/helpers_test.go b/internal/store/helpers_test.go new file mode 100644 index 0000000..461d53d --- /dev/null +++ b/internal/store/helpers_test.go @@ -0,0 +1,363 @@ +package store + +import ( + "bufio" + "bytes" + "crypto/rand" + "encoding/hex" + "encoding/json" + "errors" + "fmt" + "os" + "path/filepath" + "slices" + "strconv" + "strings" + "syscall" + "testing" + "time" + + "github.com/code6x/nodkey/internal/config" +) + +// testItemID is the item of most tests. +const testItemID = "6f1e2d3c-4b5a-4987-8a6b-5c4d3e2f1a0b" + +// testDeps returns deps for tests. The checks of the config package refuse +// every path below the system temporary directory, which everyone can +// write to; TestNewMakesTheRealChecks shows that New makes them. The check +// of node does nothing either, so that a node on the test machine does +// not change the tests; TestBWPathNodeIsCheckedLikeTheProgram runs it. +func testDeps() deps { + return deps{ + readPrivateFile: func(path string) ([]byte, error) { + return os.ReadFile(path) //nolint:gosec // G304: tests read only files in their own temporary directory. + }, + checkPrivateFile: func(path string) error { + _, err := os.Stat(path) //nolint:gosec // G703: tests check only files in their own temporary directory. + return err + }, + checkParentDirs: func(string) error { return nil }, + checkProgram: func(string) error { return nil }, + path: bwPath, + timeout: 20 * time.Second, + waitDelay: 2 * time.Second, + } +} + +// madeUp returns a new made-up secret with the label at its start, such +// as "token-0f1e...". It is made when the test runs, so the source holds no +// text that looks like a real token. It has no spaces, so it can be a +// token or a session key too. +func madeUp(label string) string { + b := make([]byte, 16) + _, _ = rand.Read(b) + return label + "-" + hex.EncodeToString(b) +} + +// testFake is a store under test, with its fake program. +type testFake struct { + t *testing.T + store Store + cfg config.Store + dir string // the state directory + home string // the store's home directory: dir/NAME +} + +// newTestFake makes a store of type typ whose program is the fake. The +// token or password file holds sc.Token or sc.Password and a newline. +func newTestFake(t *testing.T, typ config.StoreType, sc fakeScenario, d deps) *testFake { + t.Helper() + f := setupFake(t, typ, sc) + s, err := newStore(f.cfg, f.dir, d) + if err != nil { + t.Fatalf("newStore: %v", err) + } + f.store = s + t.Cleanup(s.Close) + return f +} + +// setupFake makes the files of a store with a fake program: the link to +// the fake, the token or password file, and the home directory with the +// scenario. It does not make the store. +func setupFake(t *testing.T, typ config.StoreType, sc fakeScenario) *testFake { + t.Helper() + base := t.TempDir() + exe, err := os.Executable() + if err != nil { + t.Fatal(err) + } + bin := filepath.Join(base, "bin") + if err := os.Mkdir(bin, 0o755); err != nil { //nolint:gosec // G301: a test directory for the link to the fake program. + t.Fatal(err) + } + f := &testFake{t: t, dir: filepath.Join(base, "state")} + f.cfg = config.Store{Name: "main", Type: typ, Program: filepath.Join(bin, string(typ))} + if typ == config.StoreBW { + f.cfg.Name = "vault" + } + if err := os.Symlink(exe, f.cfg.Program); err != nil { + t.Fatal(err) + } + switch typ { + case config.StoreBWS: + f.cfg.TokenFile = filepath.Join(base, "bws-token") + writeFile(t, f.cfg.TokenFile, sc.Token+"\n") + case config.StoreBW: + f.cfg.PasswordFile = filepath.Join(base, "bw-password") + writeFile(t, f.cfg.PasswordFile, sc.Password+"\n") + } + f.home = filepath.Join(f.dir, f.cfg.Name) + if err := os.MkdirAll(f.home, 0o700); err != nil { + t.Fatal(err) + } + f.setScenario(sc) + return f +} + +// writeFile writes a test file with mode 0600. +func writeFile(t *testing.T, path, content string) { + t.Helper() + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatal(err) + } +} + +// setScenario tells the fake what to do from now on. +func (f *testFake) setScenario(sc fakeScenario) { + f.t.Helper() + if err := writeGob(filepath.Join(f.home, fakeScenarioFile), sc); err != nil { + f.t.Fatal(err) + } +} + +// bwState returns the state of the fake bw. +func (f *testFake) bwState() fakeBWState { + f.t.Helper() + var st fakeBWState + if err := readJSON(filepath.Join(f.home, fakeBWStateFile), &st); err != nil { + f.t.Fatal(err) + } + return st +} + +// setBWState sets the state of the fake bw. +func (f *testFake) setBWState(st fakeBWState) { + f.t.Helper() + if err := writeJSON(filepath.Join(f.home, fakeBWStateFile), st); err != nil { + f.t.Fatal(err) + } +} + +// calls returns the runs of the fake so far. +func (f *testFake) calls() []fakeCall { + f.t.Helper() + file, err := os.Open(filepath.Join(f.home, fakeCallsFile)) + if errors.Is(err, os.ErrNotExist) { + return nil + } + if err != nil { + f.t.Fatal(err) + } + defer func() { _ = file.Close() }() + var calls []fakeCall + s := bufio.NewScanner(file) + s.Buffer(nil, 1<<20) + for s.Scan() { + var c fakeCall + if err := json.Unmarshal(s.Bytes(), &c); err != nil { + f.t.Fatal(err) + } + calls = append(calls, c) + } + if err := s.Err(); err != nil { + f.t.Fatal(err) + } + return calls +} + +// steps returns the step of each run of the fake so far, such as +// "unlock" or "get item". +func (f *testFake) steps() []string { + f.t.Helper() + var steps []string + for _, c := range f.calls() { + switch { + case len(c.Args) >= 2 && (c.Args[0] == "secret" || c.Args[0] == "get"): + steps = append(steps, c.Args[0]+" "+c.Args[1]) + case len(c.Args) >= 1: + steps = append(steps, c.Args[0]) + default: + steps = append(steps, "") + } + } + return steps +} + +// pids returns the process IDs that a hanging or leaving fake wrote. +func (f *testFake) pids() []int { + f.t.Helper() + data, err := os.ReadFile(filepath.Join(f.home, fakePIDsFile)) + if err != nil { + f.t.Fatalf("the fake wrote no process IDs: %v", err) + } + var pids []int + for _, w := range strings.Fields(string(data)) { + pid, err := strconv.Atoi(w) + if err != nil || pid <= 1 { + f.t.Fatalf("bad process ID %q", w) + } + pids = append(pids, pid) + } + if len(pids) != 2 { + f.t.Fatalf("got %d process IDs, want 2", len(pids)) + } + return pids +} + +// waitGone waits until each process is gone or a zombie, and fails the +// test if one still runs after 10 seconds. +func waitGone(t *testing.T, pids []int) { + t.Helper() + deadline := time.Now().Add(10 * time.Second) + for _, pid := range pids { + for alive(pid) { + if time.Now().After(deadline) { + _ = syscall.Kill(pid, syscall.SIGKILL) + t.Fatalf("process %d still runs", pid) + } + time.Sleep(10 * time.Millisecond) + } + } +} + +// alive reports whether a process runs: it exists and is not a zombie. +func alive(pid int) bool { + data, err := os.ReadFile("/proc/" + strconv.Itoa(pid) + "/stat") + if err != nil { + return false + } + // The state is the first field after the ")" that ends the name. + i := bytes.LastIndexByte(data, ')') + if i < 0 || i+2 >= len(data) { + return false + } + return data[i+2] != 'Z' && data[i+2] != 'X' +} + +// envNames returns the sorted names of the variables in env. +func envNames(env []string) []string { + var names []string + for _, kv := range env { + name, _, _ := strings.Cut(kv, "=") + names = append(names, name) + } + slices.Sort(names) + return names +} + +// envValue returns the value of the variable name in env. +func envValue(env []string, name string) string { + for _, kv := range env { + if v, ok := strings.CutPrefix(kv, name+"="); ok { + return v + } + } + return "" +} + +// noCanary fails the test if text holds the canary. The message never +// shows the text, which may hold a secret. +func noCanary(t *testing.T, where, text, canary string) { + t.Helper() + if canary == "" { + t.Fatal("empty canary") + } + if strings.Contains(text, canary) { + t.Errorf("%s holds the made-up secret (%d bytes of text, not shown)", where, len(text)) + } +} + +// checkValue fails the test if v is not want. The message shows only the +// lengths. +func checkValue(t *testing.T, v Value, want string) { + t.Helper() + if !bytes.Equal(v.Bytes(), []byte(want)) { + t.Errorf("the value is wrong: got %d bytes, want %d bytes (the values are not shown)", len(v.Bytes()), len(want)) + } +} + +// checkErr fails the test unless err wraps want. +func checkErr(t *testing.T, err, want error) { + t.Helper() + if !errors.Is(err, want) { + t.Errorf("error = %v, want %v", err, want) + } +} + +// bwsSecretJSON returns an answer of "bws secret get ID --output json" in +// the form of bws 2.1.0: serde_json's pretty print and a newline. +func bwsSecretJSON(id, value string) string { + out, err := json.MarshalIndent(map[string]any{ + "id": id, + "organizationId": "0a1b2c3d-0000-4000-8000-000000000001", + "projectId": "0a1b2c3d-0000-4000-8000-000000000002", + "key": "GITHUB_TOKEN", + "value": value, + "note": "", + "creationDate": "2026-10-14T12:00:00.000000Z", + "revisionDate": "2026-10-14T12:00:00.000000Z", + }, "", " ") + if err != nil { + panic(err) + } + return string(out) + "\n" +} + +// bwItemJSON returns an answer of "bw get item ID" for a login item, in +// the form of bw 2026.9.1: compact JSON without a newline. A normal item +// has no "deletedDate" and no "archivedDate" at all: bw leaves them +// undefined, and JSON.stringify drops them. deletedDate, if not nil, is +// the date the item went to the trash. secrets fills other fields that +// hold secrets, which nodkey must not read. +func bwItemJSON(id string, password any, deletedDate any, secrets string) string { + item := map[string]any{ + "passwordHistory": []any{map[string]any{"lastUsedDate": "2026-10-01T12:00:00.000Z", "password": secrets}}, + "revisionDate": "2026-10-14T12:00:00.000Z", + "creationDate": "2026-10-01T12:00:00.000Z", + "object": "item", + "id": id, + "organizationId": nil, + "folderId": nil, + "type": 1, + "reprompt": 0, + "name": "deploy token", + "notes": "notes: " + secrets, + "favorite": false, + "fields": []any{map[string]any{"name": "api", "value": secrets, "type": 1, "linkedId": nil}}, + "login": map[string]any{ + "uris": []any{map[string]any{"match": nil, "uri": "https://example.com"}}, + "username": "deploy", + "password": password, + "totp": secrets, + "passwordRevisionDate": nil, + "fido2Credentials": []any{}, + }, + "collectionIds": []any{}, + "attachments": nil, + } + if deletedDate != nil { + item["deletedDate"] = deletedDate + } + out, err := json.Marshal(item) + if err != nil { + panic(err) + } + return string(out) +} + +// String makes test failures show the scenario's steps, not its secrets. +func (sc fakeScenario) String() string { + return fmt.Sprintf("scenario with %d items and %d steps", len(sc.Items), len(sc.Steps)) +} diff --git a/internal/store/home.go b/internal/store/home.go new file mode 100644 index 0000000..737e423 --- /dev/null +++ b/internal/store/home.go @@ -0,0 +1,70 @@ +package store + +import ( + "errors" + "fmt" + "io/fs" + "math" + "os" +) + +// homePerm is the mode of a store's home directory: only its owner may +// read, write or enter it. +const homePerm fs.FileMode = 0o700 + +// prepareHome makes sure that home is a private directory for the +// program's state files. bws keeps its login state there, and bw keeps a +// copy of the vault and its login. +// +// - Every directory on the way to home must pass checkParentDirs +// (config.CheckParentDirs): owned by root or by the user that runs +// nodkey, and not writable by group or others. So nobody else can +// swap home. +// - If home is missing, it is made with mode 0700. Its parent must +// exist. +// - home must be a directory, not a symbolic link, owned by the user +// that runs nodkey, with mode 0700. +// +// An error can name the path. +func prepareHome(home string, checkParentDirs func(string) error) error { + if err := checkParentDirs(home); err != nil { + return err // the error names the path + } + made := false + switch err := os.Mkdir(home, homePerm); { //nolint:gosec // G703: home is the broker's state directory and a checked store name; checkParentDirs checked the way to it. + case err == nil: + made = true + case !errors.Is(err, fs.ErrExist): + return err // a *fs.PathError names the path + } + if made { + // The umask may have taken bits away; set the mode exactly. Only + // root and this user can change the directories on the way. + if err := os.Chmod(home, homePerm); err != nil { //nolint:gosec // G703: the directory that Mkdir just made. + return err + } + } + fi, err := os.Lstat(home) //nolint:gosec // G703: Lstat only looks at home; the checks below refuse anything but a private directory. + if err != nil { + return err + } + if fi.Mode()&fs.ModeSymlink != 0 { + return fmt.Errorf("%s: is a symbolic link; use the path of the directory itself", home) + } + if !fi.IsDir() { + return fmt.Errorf("%s: is not a directory", home) + } + owner, ok := fileOwner(fi) + if !ok { + return fmt.Errorf("%s: cannot check the owner on this system", home) + } + euid := os.Geteuid() + if euid < 0 || uint64(euid) > math.MaxUint32 || owner != uint32(euid) { + return fmt.Errorf("%s: is owned by user ID %d; it must be owned by the user that runs nodkey (ID %d)", + home, owner, euid) + } + if perm := fi.Mode().Perm(); perm != homePerm { + return fmt.Errorf("%s: has mode %04o; it must be private to its owner, with mode 0700", home, perm) + } + return nil +} diff --git a/internal/store/json.go b/internal/store/json.go new file mode 100644 index 0000000..e90d5d0 --- /dev/null +++ b/internal/store/json.go @@ -0,0 +1,387 @@ +package store + +import ( + "bytes" + "unicode/utf8" +) + +// The answers of bws and bw are JSON. nodkey reads them with its own small +// reader, not with encoding/json, for three reasons: +// +// - It is strict. The answer must be one JSON object and nothing more, +// in valid UTF-8 (RFC 8259). A key may appear only once in an object, +// and keys match only exactly. encoding/json takes the last of two +// equal keys, matches keys without regard to case, and puts U+FFFD in +// place of invalid UTF-8 or a lone surrogate (an escape from D800 to +// DFFF without its pair); that would hand out a changed value. +// - It decodes a secret into one buffer of the exact size, which the +// caller can clear. encoding/json makes Go strings, which cannot be +// cleared, and keeps copies in its own buffers. +// - It reads only the fields that nodkey needs, and skips the others +// without keeping them. + +// maxDepth is the deepest nesting of objects and arrays in an answer. The +// answers of bws and bw nest at most 4 levels deep. +const maxDepth = 32 + +// jsonReader reads one JSON text from data. +type jsonReader struct { + data []byte + pos int + depth int // the objects and arrays around pos +} + +// readAnswer reads data as one JSON object, with only whitespace around +// it. It calls member for each key of the object, with the reader at the +// key's value: member must read the value, for example with skip. +func readAnswer(data []byte, member func(r *jsonReader, key string) error) error { + r := &jsonReader{data: data} + r.space() + if err := r.object(member); err != nil { + return err + } + r.space() + if r.pos != len(r.data) { + return ErrBadAnswer + } + return nil +} + +// space skips whitespace. +func (r *jsonReader) space() { + for r.pos < len(r.data) { + switch r.data[r.pos] { + case ' ', '\t', '\n', '\r': + r.pos++ + default: + return + } + } +} + +// peek returns the next byte, or 0 at the end. +func (r *jsonReader) peek() byte { + if r.pos < len(r.data) { + return r.data[r.pos] + } + return 0 +} + +// enter counts one more level of nesting, and refuses too many. +func (r *jsonReader) enter() error { + if r.depth++; r.depth > maxDepth { + return ErrBadAnswer + } + r.pos++ // the "{" or "[" + r.space() + return nil +} + +// object reads an object, and calls member for each key, as readAnswer +// does. A key that appears twice is an error. +func (r *jsonReader) object(member func(r *jsonReader, key string) error) error { + if r.peek() != '{' { + return ErrBadAnswer + } + if err := r.enter(); err != nil { + return err + } + if r.peek() == '}' { + r.pos++ + r.depth-- + return nil + } + seen := make(map[string]bool) + var kb []byte + for { + kb = kb[:0] + if _, err := r.str(&kb); err != nil { + return err + } + key := string(kb) + if seen[key] { + return ErrBadAnswer + } + seen[key] = true + r.space() + if r.peek() != ':' { + return ErrBadAnswer + } + r.pos++ + r.space() + if err := member(r, key); err != nil { + return err + } + r.space() + switch r.peek() { + case ',': + r.pos++ + r.space() + case '}': + r.pos++ + r.depth-- + return nil + default: + return ErrBadAnswer + } + } +} + +// array reads an array and skips its values. +func (r *jsonReader) array() error { + if r.peek() != '[' { + return ErrBadAnswer + } + if err := r.enter(); err != nil { + return err + } + if r.peek() == ']' { + r.pos++ + r.depth-- + return nil + } + for { + if err := r.skip(); err != nil { + return err + } + r.space() + switch r.peek() { + case ',': + r.pos++ + r.space() + case ']': + r.pos++ + r.depth-- + return nil + default: + return ErrBadAnswer + } + } +} + +// skip reads one value and drops it. It checks the value as strictly as +// the values that nodkey keeps. +func (r *jsonReader) skip() error { + switch c := r.peek(); { + case c == '{': + return r.object(func(r *jsonReader, _ string) error { return r.skip() }) + case c == '[': + return r.array() + case c == '"': + _, err := r.str(nil) + return err + case c == 't': + return r.literal("true") + case c == 'f': + return r.literal("false") + case c == 'n': + return r.literal("null") + case c == '-' || '0' <= c && c <= '9': + return r.number() + } + return ErrBadAnswer +} + +// literal reads the word lit: true, false or null. +func (r *jsonReader) literal(lit string) error { + if !bytes.HasPrefix(r.data[r.pos:], []byte(lit)) { + return ErrBadAnswer + } + r.pos += len(lit) + return nil +} + +// null reads null. +func (r *jsonReader) null() error { + return r.literal("null") +} + +// number reads a number: -?(0|[1-9][0-9]*)(\.[0-9]+)?([eE][+-]?[0-9]+)? +func (r *jsonReader) number() error { + if r.peek() == '-' { + r.pos++ + } + switch c := r.peek(); { + case c == '0': + r.pos++ + case '1' <= c && c <= '9': + r.digits() + default: + return ErrBadAnswer + } + if r.peek() == '.' { + r.pos++ + if r.digits() == 0 { + return ErrBadAnswer + } + } + if c := r.peek(); c == 'e' || c == 'E' { + r.pos++ + if c := r.peek(); c == '+' || c == '-' { + r.pos++ + } + if r.digits() == 0 { + return ErrBadAnswer + } + } + return nil +} + +// digits reads digits, and returns how many. +func (r *jsonReader) digits() int { + start := r.pos + for r.pos < len(r.data) && '0' <= r.data[r.pos] && r.data[r.pos] <= '9' { + r.pos++ + } + return r.pos - start +} + +// text reads a string that is not secret, such as a key or a status, of +// at most limit bytes after decoding. +func (r *jsonReader) text(limit int) (string, error) { + var b []byte + n, err := r.str(&b) + if err != nil { + return "", err + } + if n > limit { + return "", ErrBadAnswer + } + return string(b), nil +} + +// secret reads a string that holds a secret. It decodes it into a new +// buffer of the exact size, so the buffer never grows and leaves a copy +// behind. The caller must clear the result. +func (r *jsonReader) secret() ([]byte, error) { + start := r.pos + n, err := r.str(nil) // check it and measure it + if err != nil { + return nil, err + } + r.pos = start + b := make([]byte, 0, n) + if _, err := r.str(&b); err != nil { + clear(b[:cap(b)]) + return nil, err + } + return b, nil +} + +// str reads a string. When out is not nil, it appends the decoded bytes +// to *out. It returns the number of decoded bytes. Control characters, a +// bad escape, invalid UTF-8 and a lone surrogate are errors. +func (r *jsonReader) str(out *[]byte) (int, error) { + if r.peek() != '"' { + return 0, ErrBadAnswer + } + r.pos++ + n := 0 + put := func(b ...byte) { + n += len(b) + if out != nil { + *out = append(*out, b...) + } + } + var enc [utf8.UTFMax]byte // an escaped character, encoded + defer clear(enc[:]) + for r.pos < len(r.data) { + c := r.data[r.pos] + switch { + case c == '"': + r.pos++ + return n, nil + case c == '\\': + rn, err := r.escape() + if err != nil { + return 0, err + } + put(enc[:utf8.EncodeRune(enc[:], rn)]...) + case c < 0x20: + return 0, ErrBadAnswer + case c < utf8.RuneSelf: + put(c) + r.pos++ + default: + rn, size := utf8.DecodeRune(r.data[r.pos:]) + if rn == utf8.RuneError && size == 1 { + return 0, ErrBadAnswer // invalid UTF-8, also an encoded surrogate + } + put(r.data[r.pos : r.pos+size]...) + r.pos += size + } + } + return 0, ErrBadAnswer // no closing quote +} + +// escape reads one escape after a backslash, and returns its character. +// A surrogate pair, a high surrogate (D800 to DBFF) and then a low one +// (DC00 to DFFF), gives one character. A lone surrogate is an error. +func (r *jsonReader) escape() (rune, error) { + r.pos++ // the backslash + if r.pos >= len(r.data) { + return 0, ErrBadAnswer + } + c := r.data[r.pos] + r.pos++ + switch c { + case '"', '\\', '/': + return rune(c), nil + case 'b': + return '\b', nil + case 'f': + return '\f', nil + case 'n': + return '\n', nil + case 'r': + return '\r', nil + case 't': + return '\t', nil + case 'u': + default: + return 0, ErrBadAnswer + } + r1, ok := r.hex4() + switch { + case !ok: + return 0, ErrBadAnswer + case 0xDC00 <= r1 && r1 < 0xE000: // a low surrogate first + return 0, ErrBadAnswer + case r1 < 0xD800 || r1 >= 0xE000: + return r1, nil + } + // A high surrogate: a low one must follow. + if !bytes.HasPrefix(r.data[r.pos:], []byte(`\u`)) { + return 0, ErrBadAnswer + } + r.pos += 2 + r2, ok := r.hex4() + if !ok || r2 < 0xDC00 || r2 >= 0xE000 { + return 0, ErrBadAnswer + } + return 0x10000 + (r1-0xD800)<<10 + (r2 - 0xDC00), nil +} + +// hex4 reads 4 hex digits. +func (r *jsonReader) hex4() (rune, bool) { + if len(r.data)-r.pos < 4 { + return 0, false + } + var v rune + for _, c := range r.data[r.pos : r.pos+4] { + var d byte + switch { + case '0' <= c && c <= '9': + d = c - '0' + case 'a' <= c && c <= 'f': + d = c - 'a' + 10 + case 'A' <= c && c <= 'F': + d = c - 'A' + 10 + default: + return 0, false + } + v = v<<4 | rune(d) + } + r.pos += 4 + return v, true +} diff --git a/internal/store/json_test.go b/internal/store/json_test.go new file mode 100644 index 0000000..48d049c --- /dev/null +++ b/internal/store/json_test.go @@ -0,0 +1,275 @@ +package store + +import ( + "errors" + "strings" + "testing" +) + +// esc returns JSON escapes: a backslash, "u" and each code, such as +// esc("00e9") for the escape of "รฉ". The tests build escapes with it, so +// that no tool that handles this file can turn them into characters. +func esc(codes ...string) string { + var b strings.Builder + for _, c := range codes { + b.WriteString("\\" + "u" + c) + } + return b.String() +} + +// realBWSAnswer is an answer of "bws secret get" in the form of bws 2.1.0 +// (serde_json's pretty print), with made-up IDs and text. +var realBWSAnswer = `{ + "id": "6f1e2d3c-4b5a-4987-8a6b-5c4d3e2f1a0b", + "organizationId": "0a1b2c3d-0000-4000-8000-000000000001", + "projectId": null, + "key": "DEPLOY_TOKEN", + "value": "made-up \"value\" รฉ โœ“ \\ / ` + esc("0001") + ` ๐Ÿ˜€ ` + esc("d83d", "de00") + `", + "note": "line 1\nline 2", + "creationDate": "2026-10-14T12:00:00.123456Z", + "revisionDate": "2026-10-14T12:00:00.123456Z" +} +` + +// realBWItem is an answer of "bw get item" in the form of bw 2026.9.1 +// (JSON.stringify), with made-up IDs and text. +var realBWItem = `{"passwordHistory":[{"lastUsedDate":"2026-10-01T12:00:00.000Z","password":"old"}],` + + `"revisionDate":"2026-10-14T12:00:00.000Z","creationDate":"2026-10-01T12:00:00.000Z","deletedDate":null,` + + `"archivedDate":null,"object":"item","id":"6f1e2d3c-4b5a-4987-8a6b-5c4d3e2f1a0b","organizationId":null,` + + `"folderId":null,"type":1,"reprompt":0,"name":"deploy","notes":null,"favorite":false,` + + `"fields":[{"name":"api","value":"other","type":1,"linkedId":null}],"login":{"fido2Credentials":[],` + + `"uris":[{"match":null,"uri":"https://example.com"}],"username":"deploy","password":"p` + esc("00e4") + `ss\"word",` + + `"totp":null,"passwordRevisionDate":null},"collectionIds":[],"attachments":[{"size":"12","sizeName":"12 B"}],` + + `"key":null,"n":-1.5e+10}` + +func TestParseRealAnswers(t *testing.T) { + t.Parallel() + v, err := parseBWSSecret([]byte(realBWSAnswer), testItemID) + if err != nil { + t.Fatalf("parseBWSSecret: %v", err) + } + if got, want := string(v.Bytes()), "made-up \"value\" รฉ โœ“ \\ / \x01 ๐Ÿ˜€ ๐Ÿ˜€"; got != want { + t.Errorf("value = %q, want %q", got, want) + } + v, err = parseBWItem([]byte(realBWItem), strings.ToUpper(testItemID)) + if err != nil { + t.Fatalf("parseBWItem: %v", err) + } + if got, want := string(v.Bytes()), "pรคss\"word"; got != want { + t.Errorf("password = %q, want %q", got, want) + } + status, err := parseBWStatus([]byte(`{"serverUrl":null,"lastSync":null,"userEmail":"nodkey@example.com",` + + `"userId":"00000000-0000-0000-0000-000000000000","status":"locked"}`)) + if err != nil || status != bwLocked { + t.Errorf("parseBWStatus = %q, %v", status, err) + } + for _, bad := range []string{`{"status":"Locked"}`, `{"status":"unknown"}`, `{}`, `{"status":null}`, + `{"status":"locked","status":"unlocked"}`, `{"status":"` + strings.Repeat("x", maxStatusLen+1) + `"}`} { + if _, err := parseBWStatus([]byte(bad)); !errors.Is(err, ErrBadAnswer) { + t.Errorf("parseBWStatus(%q) = %v, want %v", bad, err, ErrBadAnswer) + } + } +} + +func TestJSONStrings(t *testing.T) { + t.Parallel() + good := []struct{ in, want string }{ + {`"a"`, "a"}, + {`""`, ""}, + {`"\"\\\/\b\f\n\r\t"`, "\"\\/\b\f\n\r\t"}, + {`"` + esc("0041", "00e9", "20ac") + `"`, "Aรฉโ‚ฌ"}, + {`"` + esc("d83d", "de00") + `"`, "๐Ÿ˜€"}, + {`"` + esc("D83D", "DE00") + `"`, "๐Ÿ˜€"}, + {`"` + esc("0000") + `"`, "\x00"}, + {"\"\x7f\"", "\x7f"}, + {"\"\xef\xbf\xbd\"", string(rune(0xfffd))}, // U+FFFD itself is valid + {"\"\xf4\x8f\xbf\xbf\"", string(rune(0x10ffff))}, + {`"รฉโœ“๐Ÿ˜€"`, "รฉโœ“๐Ÿ˜€"}, + {`"` + esc("002f", "005C", "0022") + `"`, `/\"`}, + {`"` + esc("d7ff", "e000", "ffff") + `"`, string([]rune{0xd7ff, 0xe000, 0xffff})}, + {`"a` + esc("0062") + `c"`, "abc"}, + {`"` + string([]rune{0x2028, 0x2029}) + `"`, string([]rune{0x2028, 0x2029})}, + } + for _, tc := range good { + r := &jsonReader{data: []byte(tc.in)} + b, err := r.secret() + if err != nil || string(b) != tc.want || r.pos != len(tc.in) { + t.Errorf("secret(%q) = %q, %v at %d, want %q", tc.in, b, err, r.pos, tc.want) + } + if err == nil && cap(b) != len(b) { + t.Errorf("secret(%q): capacity %d for %d bytes", tc.in, cap(b), len(b)) + } + } + bad := []string{ + ``, `"`, `"abc`, `abc"`, `'a'`, `"\"`, `"\x"`, `"` + "\\" + `u12"`, `"` + "\\" + `u12g4"`, `"` + "\\" + `U0041"`, + `"` + esc("d800") + `"`, `"` + esc("dc00") + `"`, `"` + esc("d800", "0041") + `"`, `"` + esc("d800") + `x"`, + `"` + esc("de00", "d83d") + `"`, `"` + esc("d83d", "d83d") + `"`, `"` + esc("d83d") + "\\" + `n"`, + "\"\x00\"", "\"\x1f\"", "\"\n\"", "\"\t\"", "\"\xff\"", "\"\xc3\"", "\"\xed\xa0\x80\"", + "\"\xc0\xaf\"", "\"\xf8\x88\x80\x80\x80\"", "\"\xf4\x90\x80\x80\"", + } + for _, in := range bad { + r := &jsonReader{data: []byte(in)} + if b, err := r.secret(); !errors.Is(err, ErrBadAnswer) || b != nil { + t.Errorf("secret(%q) = %q, %v, want %v", in, b, err, ErrBadAnswer) + } + } +} + +// TestJSONAnswers checks the reader on whole answers. It reads every +// value and keeps none. +func TestJSONAnswers(t *testing.T) { + t.Parallel() + value := func(in string) error { + return readAnswer([]byte(in), func(r *jsonReader, _ string) error { return r.skip() }) + } + good := []string{ + `{"value":"x"}`, " \t\r\n{\"value\":\"x\"} \t\r\n", `{"value":"x","a":[1,-0,0.5,1e5,1E-5,-1.25e+3,true,false,null,{},[]]}`, + `{"a":{"b":{"c":[[[{"value":"no"}]]]}},"value":"x"}`, `{"value" : "x" , "b" : { } }`, + `{"value":"x","":"empty key","` + esc("0076") + `alue2":1}`, + `{"value":"x","a":` + strings.Repeat("[", maxDepth-1) + strings.Repeat("]", maxDepth-1) + `}`, + } + for _, in := range good { + if err := value(in); err != nil { + t.Errorf("readAnswer(%q) = %v", in, err) + } + } + bad := []string{ + ``, ` `, `null`, `"value"`, `[]`, `{`, `}`, `{"value":"x"`, `{"value":"x"}}`, `{"value":"x"},`, + `{"value":"x"} {}`, `{"value":"x"}x`, "\xef\xbb\xbf{\"value\":\"x\"}", `{"value":"x",}`, `{,"value":"x"}`, + `{"value" "x"}`, `{"value":"x" "a":1}`, `{value:"x"}`, `{"value":"x","a":}`, `{"value":"x","a":01}`, + `{"value":"x","a":1.}`, `{"value":"x","a":.5}`, `{"value":"x","a":+1}`, `{"value":"x","a":1e}`, + `{"value":"x","a":-}`, `{"value":"x","a":tru}`, `{"value":"x","a":nul}`, `{"value":"x","a":True}`, + `{"value":"x","a":NaN}`, `{"value":"x","a":[1,]}`, `{"value":"x","a":[,1]}`, `{"value":"x","a":[1 2]}`, + `{"value":"x","a":{"b":1,"b":2}}`, `{"value":"x","` + esc("0076") + `alue":"y"}`, + "{\"value\":\"x\",\"a\":\"\xff\"}", "{\"value\":\"x\",\"a\x01\":1}", + `{"value":"x","a":` + strings.Repeat("[", maxDepth) + strings.Repeat("]", maxDepth) + `}`, + "{\"value\":\"x\"}\x00", "{\"value\":\"x\"}\v", `{"value":"x"}/* */`, `{"value":"x",/**/"a":1}`, + } + for _, in := range bad { + if err := value(in); !errors.Is(err, ErrBadAnswer) { + t.Errorf("readAnswer(%q) = %v, want %v", in, err, ErrBadAnswer) + } + } +} + +// TestAnswersMustBeForTheAskedItem checks the "id" of both answers: it +// must be there, and be the asked ID, in upper or lower case. +func TestAnswersMustBeForTheAskedItem(t *testing.T) { + t.Parallel() + upper := strings.ToUpper(testItemID) + parsers := map[string]func(data []byte, id string) (Value, error){ + "bws": parseBWSSecret, + "bw": parseBWItem, + } + answer := map[string]func(id string) string{ + "bws": func(id string) string { return `{"id":"` + id + `","value":"v"}` }, + "bw": func(id string) string { return `{"id":"` + id + `","login":{"password":"v"}}` }, + } + for name, parse := range parsers { + for _, tc := range []struct{ answerID, askedID string }{ + {testItemID, testItemID}, {upper, testItemID}, {testItemID, upper}, + } { + v, err := parse([]byte(answer[name](tc.answerID)), tc.askedID) + if err != nil || string(v.Bytes()) != "v" { + t.Errorf("%s: answer for %s, asked %s: %v", name, tc.answerID, tc.askedID, err) + } + } + // The Kelvin sign folds to "k" in strings.EqualFold. A UUID never + // has a "k", so this only shows that the comparison is ASCII. + kelvin := string(rune(0x212a)) + for _, bad := range []string{ + answer[name]("00000000-0000-4000-8000-0000000000aa"), // another item + answer[name](testItemID + " "), + answer[name](testItemID[:35]), + answer[name](kelvin), + strings.Replace(answer[name](testItemID), `"id":"`+testItemID+`",`, "", 1), // no id + strings.Replace(answer[name](testItemID), `"id":"`+testItemID+`"`, `"id":null`, 1), + strings.Replace(answer[name](testItemID), `"id":"`+testItemID+`"`, `"id":7`, 1), + strings.Replace(answer[name](testItemID), `"id"`, `"ID"`, 1), + `{"id":"` + testItemID + `","id":"` + testItemID + `"}`, + } { + if v, err := parse([]byte(bad), testItemID); !errors.Is(err, ErrBadAnswer) || v.Bytes() != nil { + t.Errorf("%s: %q = %v, want %v", name, bad, err, ErrBadAnswer) + } + } + } + if sameUUID(kelvinUUID(), strings.ReplaceAll(kelvinUUID(), string(rune(0x212a)), "k")) { + t.Error("sameUUID folds the Kelvin sign") + } +} + +// kelvinUUID returns a text with the Kelvin sign in it. +func kelvinUUID() string { return "6f1e2d3c-" + string(rune(0x212a)) + "b5a-4987-8a6b-5c4d3e2f1a0b" } + +func TestValuesWithNULAreRefused(t *testing.T) { + t.Parallel() + nul := esc("0000") + for name, tc := range map[string]struct { + answer string + parse func(data []byte, id string) (Value, error) + }{ + "bws": {`{"id":"` + testItemID + `","value":"a` + nul + `b"}`, parseBWSSecret}, + "bws start": {`{"id":"` + testItemID + `","value":"` + nul + `"}`, parseBWSSecret}, + "bw": {`{"id":"` + testItemID + `","login":{"password":"a` + nul + `"}}`, parseBWItem}, + } { + v, err := tc.parse([]byte(tc.answer), testItemID) + if !errors.Is(err, ErrBadValue) || v.Bytes() != nil { + t.Errorf("%s: %v, want %v", name, err, ErrBadValue) + } + } + // Other control characters can be in a value. + v, err := parseBWSSecret([]byte(`{"id":"`+testItemID+`","value":"a`+esc("0001")+`\n\t"}`), testItemID) + if err != nil || string(v.Bytes()) != "a\x01\n\t" { + t.Errorf("a value with control characters: %q, %v", v.Bytes(), err) + } +} + +func TestBWItemDates(t *testing.T) { + t.Parallel() + item := func(fields string) string { + return `{"id":"` + testItemID + `",` + fields + `"login":{"password":"v"}}` + } + for fields, want := range map[string]error{ + ``: nil, // a normal item of bw 2026.9.1 + `"deletedDate":null,`: nil, + `"archivedDate":null,`: nil, + `"deletedDate":null,"archivedDate":null,`: nil, + `"deletedDate":"2026-10-13T12:00:00.000Z",`: ErrTrashed, + `"archivedDate":"2026-10-13T12:00:00.000Z",`: ErrArchived, + `"deletedDate":"2026-10-13T12:00:00.000Z","archivedDate":"2026-10-13T12:00:00.000Z",`: ErrTrashed, + `"deletedDate":"",`: ErrTrashed, + `"archivedDate":"",`: ErrArchived, + `"deletedDate":0,`: ErrBadAnswer, + `"archivedDate":true,`: ErrBadAnswer, + `"archivedDate":{},`: ErrBadAnswer, + } { + v, err := parseBWItem([]byte(item(fields)), testItemID) + if !errors.Is(err, want) || (want == nil) != (v.Bytes() != nil) { + t.Errorf("%s: %v, want %v", fields, err, want) + } + } +} + +func TestParseKey(t *testing.T) { + t.Parallel() + for in, want := range map[string]string{ + "abc": "abc", "abc\n": "abc", " abc \r\n": "abc", "\t\v\fabc": "abc", "a+/=:.-_~!": "a+/=:.-_~!", + } { + v, ok := parseKey([]byte(in), 16) + if !ok || string(v.Bytes()) != want { + t.Errorf("parseKey(%q) = %q, %v", in, v.Bytes(), ok) + } + } + for _, in := range []string{"", " ", "\n\n", "a b", "a\nb", "a\x00", "a\x7f", "รฉ", strings.Repeat("a", 17)} { + if _, ok := parseKey([]byte(in), 16); ok { + t.Errorf("parseKey(%q) worked", in) + } + } + // The result is a copy: clearing the input leaves it. + in := []byte("token\n") + v, _ := parseKey(in, 16) + clear(in) + if string(v.Bytes()) != "token" { + t.Error("parseKey did not copy the key") + } +} diff --git a/internal/store/run.go b/internal/store/run.go new file mode 100644 index 0000000..683761c --- /dev/null +++ b/internal/store/run.go @@ -0,0 +1,230 @@ +package store + +import ( + "context" + "errors" + "fmt" + "io" + "os/exec" + "runtime" + "syscall" + "time" +) + +// Limits of the output of one run of a store program. +const ( + // maxStdout is the most that a program may write to stdout: 1 MiB. + // The answers are much smaller: a bws secret, or a bw item with its + // notes, custom fields and password history. + maxStdout = 1 << 20 + // maxStderr is the most that a program may write to stderr: 64 KiB. + // stderr is only counted, never read or kept. + maxStderr = 64 << 10 + // firstBufferSize is the size of the first buffer for stdout. Most + // answers fit in it. + firstBufferSize = 4 << 10 +) + +var ( + // errTimedOut is the cause of a run's context when its time is up. + errTimedOut = errors.New("store: the time limit passed") + // errOverflow is the cause of a run's context when the program wrote + // too much, and the error that output returns then. + errOverflow = errors.New("store: too much output") +) + +// runner runs the program of one store. +type runner struct { + store string // the store's name, for errors + program string // the program's absolute path + home string // the program's private home and working directory + timeout time.Duration // the time limit of one run + waitDelay time.Duration // see deps.waitDelay +} + +// run runs the program once with args, and returns what it wrote to +// stdout. The caller must clear the result. env holds the variables that +// the program gets on top of HOME; nothing comes from the broker's own +// environment. step names the run in errors, such as "bw sync". +// +// stdin is /dev/null. The program runs in a process group of its own, so +// that a timeout, too much output or the end of ctx kills the program and +// every process it started in that group. After the run, the group is +// killed too, in case the program left a process behind. If the broker +// ends during the run, the program gets SIGKILL (see setProcessGroup). +// +// Errors give a fixed text, and the exit code or the signal, never the +// output. A run fails when the program exits with a code that is not 0, +// when it takes longer than the time limit, when it writes more than +// maxStdout or maxStderr, when it leaves a process that holds its output +// open, and when ctx ends, even after the program has finished. +func (r *runner) run(ctx context.Context, step string, args, env []string) ([]byte, error) { + if !supported { + return nil, fmt.Errorf("store %q: %s: %w: %w", r.store, step, ErrStart, errors.ErrUnsupported) + } + runCtx, cancel := context.WithCancelCause(ctx) + defer cancel(nil) + runCtx, stop := context.WithTimeoutCause(runCtx, r.timeout, errTimedOut) + defer stop() + + stdout := &output{keep: true, max: maxStdout, full: func() { cancel(errOverflow) }} + stderr := &output{max: maxStderr, full: func() { cancel(errOverflow) }} + defer stderr.clear() + + cmd := exec.CommandContext(runCtx, r.program, args...) //nolint:gosec // G204: the program comes from the broker config, which CheckFiles checked; the arguments are fixed words and a checked UUID or path. + cmd.Env = append([]string{"HOME=" + r.home}, env...) + cmd.Dir = r.home + cmd.Stdin = nil // /dev/null + cmd.Stdout = stdout + cmd.Stderr = stderr + cmd.WaitDelay = r.waitDelay + setProcessGroup(cmd) + // Start sets cmd.Process before it watches runCtx, so Cancel can read + // it. + cmd.Cancel = func() error { return killProcessGroup(cmd.Process) } + + // The program gets SIGKILL when the thread that started it ends + // (Pdeathsig). Go can end a thread while the broker goes on, so the + // run keeps its thread until the program has been waited for. Then + // only the end of the broker kills the program. + err := func() error { + runtime.LockOSThread() + defer runtime.UnlockOSThread() + return cmd.Run() + }() + if cmd.Process != nil { + // Whatever the program left in its group. The program has been + // waited for; while a process is left in the group, its ID stays + // in use, so the signal reaches only that group. + _ = killProcessGroup(cmd.Process) + } + if err == nil && !stdout.over && !stderr.over && ctx.Err() == nil { + return stdout.buf, nil + } + stdout.clear() + switch { + case stdout.over || stderr.over: + return nil, stepError(r.store, step, ErrTooMuchOutput) + case ctx.Err() != nil: + return nil, stepError(r.store, step, ctx.Err()) + case errors.Is(context.Cause(runCtx), errTimedOut): + return nil, fmt.Errorf("store %q: %s: %w (after %s)", r.store, step, ErrTimeout, r.timeout) + case errors.Is(err, exec.ErrWaitDelay): + return nil, stepError(r.store, step, ErrLeftRunning) + } + var exit *exec.ExitError + switch { + case errors.As(err, &exit): + return nil, fmt.Errorf("store %q: %s: %w (%s)", r.store, step, ErrFailed, exitDetail(exit)) + case cmd.Process != nil: + // It ran, but its output could not be read to the end. + return nil, fmt.Errorf("store %q: %s: %w (its output could not be read)", r.store, step, ErrFailed) + } + var errno syscall.Errno + if errors.As(err, &errno) { + return nil, fmt.Errorf("store %q: %s: %w: %w", r.store, step, ErrStart, errno) + } + return nil, stepError(r.store, step, ErrStart) +} + +// output takes what a program writes to stdout or stderr. It keeps the +// bytes of stdout, and only counts those of stderr. When a program writes +// more than max bytes, output calls full once and refuses the rest. +// +// os/exec copies the program's output with io.Copy, which calls ReadFrom. +// So the bytes go straight into output's own memory, and no other buffer +// of nodkey holds a copy. clear overwrites that memory. +type output struct { + keep bool // keep the bytes (stdout), or only count them (stderr) + max int // the most bytes the program may write + full func() // called once when the program writes more + buf []byte // the bytes kept + n int // the number of bytes written + over bool // more than max bytes were written + scratch []byte // stderr is read into it, then dropped +} + +// Write takes p, as io.Writer. +func (o *output) Write(p []byte) (int, error) { + if o.over || len(p) > o.max-o.n { + o.overflow() + return 0, errOverflow + } + if o.keep { + o.reserve(len(p)) + o.buf = append(o.buf, p...) + } + o.n += len(p) + return len(p), nil +} + +// ReadFrom reads from rd until its end, as io.ReaderFrom. +func (o *output) ReadFrom(rd io.Reader) (int64, error) { + var total int64 + for { + if o.over { + return total, errOverflow + } + // Read at most one byte more than allowed, to see an overflow. + room := o.max + 1 - o.n + var p []byte + if o.keep { + o.reserve(1) + p = o.buf[len(o.buf):min(cap(o.buf), len(o.buf)+room)] + } else { + if o.scratch == nil { + o.scratch = make([]byte, firstBufferSize) + } + p = o.scratch[:min(len(o.scratch), room)] + } + n, err := rd.Read(p) + total += int64(n) + o.n += n + if o.keep { + o.buf = o.buf[:len(o.buf)+n] + } + if o.n > o.max { + o.overflow() + return total, errOverflow + } + if errors.Is(err, io.EOF) { + return total, nil + } + if err != nil { + return total, err + } + } +} + +// reserve makes room for at least n more bytes in buf. A new buffer gets +// the old bytes, and the old buffer is cleared. buf never grows past +// max+1 bytes. +func (o *output) reserve(n int) { + if cap(o.buf)-len(o.buf) >= n { + return + } + size := max(2*cap(o.buf), firstBufferSize, len(o.buf)+n) + size = min(size, o.max+1) + b := make([]byte, len(o.buf), size) + copy(b, o.buf) + clear(o.buf[:cap(o.buf)]) + o.buf = b +} + +// overflow marks o as over its limit, and calls full the first time. +func (o *output) overflow() { + if o.over { + return + } + o.over = true + if o.full != nil { + o.full() + } +} + +// clear overwrites the memory of o. +func (o *output) clear() { + clear(o.buf[:cap(o.buf)]) + o.buf = nil + clear(o.scratch) +} diff --git a/internal/store/run_linux.go b/internal/store/run_linux.go new file mode 100644 index 0000000..8336171 --- /dev/null +++ b/internal/store/run_linux.go @@ -0,0 +1,54 @@ +package store + +import ( + "errors" + "fmt" + "io/fs" + "os" + "os/exec" + "syscall" +) + +// supported says that store programs can run on this system. +const supported = true + +// setProcessGroup makes the program the leader of a new process group, so +// that killProcessGroup reaches every process it starts in that group. The +// program also gets SIGKILL when the broker ends (Pdeathsig), so it never +// outlives a broker that crashed during a run. +func setProcessGroup(cmd *exec.Cmd) { + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true, Pdeathsig: syscall.SIGKILL} +} + +// killProcessGroup sends SIGKILL to the process group of p. When the +// group is gone, it returns os.ErrProcessDone. +func killProcessGroup(p *os.Process) error { + // The group ID is the leader's process ID. Never send to -0 or -1: + // those reach nodkey's own group and every process it may signal. + if p == nil || p.Pid <= 1 { + return errors.New("store: no process group to stop") + } + err := syscall.Kill(-p.Pid, syscall.SIGKILL) + if errors.Is(err, syscall.ESRCH) { + return os.ErrProcessDone + } + return err +} + +// exitDetail says how a program ended: its exit code, or the signal that +// stopped it. +func exitDetail(exit *exec.ExitError) string { + if ws, ok := exit.Sys().(syscall.WaitStatus); ok && ws.Signaled() { + return fmt.Sprintf("stopped by signal %d", int(ws.Signal())) + } + return fmt.Sprintf("exit code %d", exit.ExitCode()) +} + +// fileOwner returns the user ID that owns the file. +func fileOwner(fi fs.FileInfo) (uint32, bool) { + st, ok := fi.Sys().(*syscall.Stat_t) + if !ok { + return 0, false + } + return st.Uid, true +} diff --git a/internal/store/run_other.go b/internal/store/run_other.go new file mode 100644 index 0000000..02070e6 --- /dev/null +++ b/internal/store/run_other.go @@ -0,0 +1,24 @@ +//go:build !linux + +package store + +import ( + "errors" + "fmt" + "io/fs" + "os" + "os/exec" +) + +// Version 1 runs on Linux only (DESIGN.md section 3). Elsewhere a store +// fails closed: no program runs, and the home directory check fails. + +const supported = false + +func setProcessGroup(*exec.Cmd) {} + +func killProcessGroup(*os.Process) error { return errors.ErrUnsupported } + +func exitDetail(exit *exec.ExitError) string { return fmt.Sprintf("exit code %d", exit.ExitCode()) } + +func fileOwner(fs.FileInfo) (uint32, bool) { return 0, false } diff --git a/internal/store/run_test.go b/internal/store/run_test.go new file mode 100644 index 0000000..dcd2c3d --- /dev/null +++ b/internal/store/run_test.go @@ -0,0 +1,110 @@ +package store + +import ( + "bytes" + "errors" + "io" + "strings" + "testing" + "testing/iotest" +) + +func TestOutputKeepsUpToTheCap(t *testing.T) { + t.Parallel() + for _, n := range []int{0, 1, firstBufferSize - 1, firstBufferSize, firstBufferSize + 1, 3 * firstBufferSize, 10000} { + data := bytes.Repeat([]byte("x"), n) + for name, rd := range map[string]io.Reader{ + "one read": bytes.NewReader(data), + "byte by byte": iotest.OneByteReader(bytes.NewReader(data)), + "data and EOF": iotest.DataErrReader(bytes.NewReader(data)), + } { + full := 0 + o := &output{keep: true, max: 20000, full: func() { full++ }} + got, err := o.ReadFrom(rd) + if err != nil || got != int64(n) || !bytes.Equal(o.buf, data) || o.over || full != 0 { + t.Errorf("%d bytes, %s: ReadFrom = %d, %v; kept %d bytes; over %v; full called %d times", + n, name, got, err, len(o.buf), o.over, full) + } + if cap(o.buf) > o.max+1 { + t.Errorf("%d bytes, %s: the buffer grew to %d", n, name, cap(o.buf)) + } + } + } +} + +func TestOutputStopsAtTheCap(t *testing.T) { + t.Parallel() + for _, keep := range []bool{true, false} { + full := 0 + o := &output{keep: keep, max: 100, full: func() { full++ }} + _, err := o.ReadFrom(strings.NewReader(strings.Repeat("x", 101))) + if !errors.Is(err, errOverflow) || !o.over || full != 1 { + t.Errorf("keep %v: ReadFrom over the cap = %v; over %v; full called %d times", keep, err, o.over, full) + } + if keep && len(o.buf) > o.max+1 { + t.Errorf("kept %d bytes", len(o.buf)) + } + // Later writes fail too, and full is not called again. + if _, err := o.Write([]byte("x")); !errors.Is(err, errOverflow) || full != 1 { + t.Errorf("keep %v: Write after the cap = %v; full called %d times", keep, err, full) + } + if _, err := o.ReadFrom(strings.NewReader("x")); !errors.Is(err, errOverflow) { + t.Errorf("keep %v: ReadFrom after the cap = %v", keep, err) + } + } + // Write stops at the cap too, and counts what it took. + o := &output{keep: true, max: 10} + if n, err := o.Write([]byte("0123456789")); n != 10 || err != nil { + t.Errorf("Write up to the cap = %d, %v", n, err) + } + if _, err := o.Write([]byte("x")); !errors.Is(err, errOverflow) || !o.over { + t.Errorf("Write over the cap = %v", err) + } +} + +func TestOutputClearsItsMemory(t *testing.T) { + t.Parallel() + secret := madeUp("value") + o := &output{keep: true, max: 1 << 20} + if _, err := o.ReadFrom(iotest.HalfReader(strings.NewReader(strings.Repeat(secret, 1000)))); err != nil { + t.Fatal(err) + } + // Each bigger buffer clears the one before it. Keep the last one to + // look at it after clear. + last := o.buf[:cap(o.buf)] + o.clear() + if !bytes.Equal(last, make([]byte, len(last))) { + t.Error("clear left bytes in the buffer") + } + // stderr: the bytes are only counted, in a buffer that is cleared. + e := &output{max: 1 << 20} + if _, err := e.ReadFrom(strings.NewReader(strings.Repeat(secret, 10))); err != nil { + t.Fatal(err) + } + if e.buf != nil || e.n != 10*len(secret) { + t.Errorf("stderr kept %d bytes, counted %d", len(e.buf), e.n) + } + scratch := e.scratch + e.clear() + if !bytes.Equal(scratch, make([]byte, len(scratch))) { + t.Error("clear left bytes in the scratch buffer") + } +} + +func TestOutputReserveClearsTheOldBuffer(t *testing.T) { + t.Parallel() + o := &output{keep: true, max: 1 << 20} + if _, err := o.Write(bytes.Repeat([]byte("s"), firstBufferSize)); err != nil { + t.Fatal(err) + } + old := o.buf[:cap(o.buf)] + if _, err := o.Write([]byte("more")); err != nil { + t.Fatal(err) + } + if !bytes.Equal(old, make([]byte, len(old))) { + t.Error("the old buffer was not cleared when the buffer grew") + } + if len(o.buf) != firstBufferSize+4 { + t.Errorf("kept %d bytes", len(o.buf)) + } +} diff --git a/internal/store/store.go b/internal/store/store.go new file mode 100644 index 0000000..4ee162d --- /dev/null +++ b/internal/store/store.go @@ -0,0 +1,236 @@ +// Package store gets secret values from the secret stores: Bitwarden +// Secrets Manager (bws) and the Bitwarden Password Manager (bw). See +// DESIGN.md sections 5, 11 and 16, and THREAT-MODEL.md T18 and T29. +// +// nodkey does not link the Bitwarden SDK. A store runs the store's own +// program instead: +// +// - by the absolute path from the config, without a shell; +// - with stdin from /dev/null, in its own process group, and with its +// home directory as the working directory; +// - with only the environment it needs: HOME (a private directory for +// this store alone), the bws token or the bw session key in a +// variable, never in an argument, and for bw a fixed PATH of system +// directories, which New checks; +// - with a time limit and a cap on its output. On a timeout, too much +// output, or a cancelled request, the whole process group is killed. +// If the broker ends during a run, the kernel kills the program. +// +// The program's output is never logged. An error gives the store's name, +// the step (such as "bw sync"), a fixed text and the exit code, never the +// program's output. +package store + +import ( + "context" + "errors" + "fmt" + "path/filepath" + "strings" + "time" + + "github.com/code6x/nodkey/internal/config" +) + +// Store gets secret values from one [store.*] section of the broker +// config. The methods are safe for many goroutines. +type Store interface { + // Start prepares the store when the broker starts. For bw it unlocks + // the vault with the master password file and keeps the session key + // in memory. An error means that the store cannot hand out values + // now; Fetch tries again. For bws it does nothing. + Start(ctx context.Context) error + // Fetch gets the value of the item with the ID id, the id of a + // [secret.*] section. The caller must Clear the value when it is sent. + // Any error means deny. + Fetch(ctx context.Context, id string) (Value, error) + // Close clears the token or the session key that the store holds. + // Fetch and Start fail after Close. + Close() +} + +// The reasons of the errors of a store. An error of Start or Fetch wraps +// one of them, a context error, or the error of a file check. Test them +// with errors.Is. +var ( + ErrClosed = errors.New("the store is closed") + ErrBadID = errors.New("the item ID is not a UUID") + ErrStart = errors.New("cannot start the program") + ErrFailed = errors.New("the program failed") + ErrTimeout = errors.New("the program took too long and was stopped") + ErrTooMuchOutput = errors.New("the program wrote too much output and was stopped") + ErrLeftRunning = errors.New("the program left a process running, which was stopped") + ErrBadAnswer = errors.New("the program's answer is not in the expected form") + ErrNoValue = errors.New("the item has no value to hand out") + ErrBadValue = errors.New("the value holds a NUL byte, which a command's environment cannot hold") + ErrTrashed = errors.New("the item is in the trash") + ErrArchived = errors.New("the item is archived") + ErrLoggedOut = errors.New("bw is logged out; a person must log in again in the store's home directory") + ErrLocked = errors.New("the bw vault is locked; the next request unlocks it again") +) + +// DefaultTimeout is the time limit of one run of a store program. +const DefaultTimeout = 30 * time.Second + +// deps are the parts of a store that tests replace. +type deps struct { + // readPrivateFile reads the bws token file: config.ReadPrivateFile. + readPrivateFile func(path string) ([]byte, error) + // checkPrivateFile checks the bw password file before bw reads it: + // config.CheckPrivateFile. + checkPrivateFile func(path string) error + // checkParentDirs checks the directories on the way to a store's home + // directory, and the directories in path: config.CheckParentDirs. + checkParentDirs func(path string) error + // checkProgram checks the node in each directory in path, as a + // store's program is checked: config.CheckProgram. + checkProgram func(path string) error + // path is the PATH of bw: bwPath. + path string + // timeout limits each run of the program. + timeout time.Duration + // waitDelay is how long a run waits for the program's output after + // the program ended. A process that the program left behind can hold + // the output open; after this time the run fails. + waitDelay time.Duration +} + +// realDeps are the deps that New uses. +func realDeps() deps { + return deps{ + readPrivateFile: config.ReadPrivateFile, + checkPrivateFile: config.CheckPrivateFile, + checkParentDirs: config.CheckParentDirs, + checkProgram: config.CheckProgram, + path: bwPath, + timeout: DefaultTimeout, + waitDelay: 2 * time.Second, + } +} + +// New makes the store of one [store.*] section. dir is the directory for +// the programs' state; it must exist. The store's program gets its own +// home directory in it, dir/NAME, where NAME is the name of the section. +// So two stores never share one: bw keeps the state of one account there. +// +// The program, the token file and the password file must be clean +// absolute paths. The config package already makes them absolute, but New +// does not rely on it: bw reads the password file from its home directory, +// and a relative path would name another file there than the one that +// nodkey checked. +// +// New checks the home directory, and makes it if it is missing (see +// prepareHome). For bws, it reads the token file with +// config.ReadPrivateFile. For bw, it checks the master password file with +// config.CheckPrivateFile and its size, the directories in bw's PATH and +// the node in them (checkPathDirs), and that bw would not keep its state +// in a bw-data directory (checkNoBWData); bw reads the password file +// itself when it unlocks. New does not run the program; call Start. +func New(cfg config.Store, dir string) (Store, error) { + return newStore(cfg, dir, realDeps()) +} + +// newStore is New with deps. +func newStore(cfg config.Store, dir string, d deps) (Store, error) { + if !validName(cfg.Name) { + return nil, errors.New("store: the name of the store cannot be the name of a directory") + } + name := cfg.Name + files := []struct{ key, path string }{{"program", cfg.Program}} + switch cfg.Type { + case config.StoreBWS: + files = append(files, struct{ key, path string }{"token_file", cfg.TokenFile}) + case config.StoreBW: + files = append(files, struct{ key, path string }{"password_file", cfg.PasswordFile}) + default: + return nil, fmt.Errorf("store %q: unknown store type", name) + } + for _, f := range files { + if !cleanAbs(f.path) { + return nil, fmt.Errorf("store %q: %s: %s", name, f.key, cleanAbsRule) + } + } + if !cleanAbs(dir) { + return nil, fmt.Errorf("store %q: the state directory: %s", name, cleanAbsRule) + } + home := filepath.Join(dir, name) + if err := prepareHome(home, d.checkParentDirs); err != nil { + return nil, fmt.Errorf("store %q: home directory: %w", name, err) + } + r := runner{ + store: name, + program: cfg.Program, + home: home, + timeout: d.timeout, + waitDelay: d.waitDelay, + } + if cfg.Type == config.StoreBWS { + return newBWS(cfg, r, d) + } + return newBW(cfg, r, d) +} + +// cleanAbsRule says what cleanAbs allows, for errors. +const cleanAbsRule = `must be an absolute path without "//", "." or ".." parts` + +// cleanAbs reports whether p is an absolute path that filepath.Clean +// leaves as it is. Clean would change what ".." means after a symbolic +// link, so a path that is not clean is refused, not cleaned. +func cleanAbs(p string) bool { + return filepath.IsAbs(p) && filepath.Clean(p) == p +} + +// validName reports whether name can be one directory in the state +// directory. The config allows only such names (DESIGN.md section 14); +// this checks again, because the name becomes part of a path. +func validName(name string) bool { + return name != "" && name != "." && name != ".." && !strings.ContainsAny(name, "/\x00") && len(name) <= 64 +} + +// isUUID reports whether id is a UUID in the form +// 00000000-0000-0000-0000-000000000000, in upper or lower case. An ID +// goes into the program's arguments, so it must never start with "-", +// and bw must never search items by name. +func isUUID(id string) bool { + if len(id) != 36 { + return false + } + for i := 0; i < len(id); i++ { + c := id[i] + switch i { + case 8, 13, 18, 23: + if c != '-' { + return false + } + default: + if !isHex(c) { + return false + } + } + } + return true +} + +// isHex reports whether c is a hex digit, in upper or lower case. +func isHex(c byte) bool { + return '0' <= c && c <= '9' || 'a' <= c && c <= 'f' || 'A' <= c && c <= 'F' +} + +// stepError returns an error of one step of a store, such as +// `store "vault": bw sync: the program failed`. +func stepError(store, step string, err error) error { + return fmt.Errorf("store %q: %s: %w", store, step, err) +} + +// acquire takes a place in slots, or returns an error when ctx ends first. +func acquire(ctx context.Context, slots chan struct{}) error { + if err := ctx.Err(); err != nil { + return err + } + select { + case slots <- struct{}{}: + return nil + case <-ctx.Done(): + return ctx.Err() + } +} diff --git a/internal/store/store_test.go b/internal/store/store_test.go new file mode 100644 index 0000000..504ea23 --- /dev/null +++ b/internal/store/store_test.go @@ -0,0 +1,250 @@ +package store + +import ( + "errors" + "io/fs" + "os" + "path/filepath" + "slices" + "strings" + "syscall" + "testing" + + "github.com/code6x/nodkey/internal/config" +) + +// TestNewMakesTheRealChecks shows that New uses the checks of the config +// package. The other tests replace them (testDeps), because those checks +// refuse every path below the system temporary directory. +func TestNewMakesTheRealChecks(t *testing.T) { + t.Parallel() + for _, typ := range []config.StoreType{config.StoreBWS, config.StoreBW} { + f := setupFake(t, typ, fakeScenario{Token: madeUp("token"), Password: madeUp("password")}) + // A directory that everyone can write to, on the way to the home. + open := filepath.Join(t.TempDir(), "open") + if err := os.Mkdir(open, 0o700); err != nil { + t.Fatal(err) + } + if err := os.Chmod(open, 0o777); err != nil { //nolint:gosec // G302: the test needs a directory that others can write to. + t.Fatal(err) + } + dir := filepath.Join(open, "state") + if err := os.Mkdir(dir, 0o700); err != nil { + t.Fatal(err) + } + _, err := New(f.cfg, dir) + if err == nil || !strings.Contains(err.Error(), "group or others can write to this directory") { + t.Errorf("%s: New below a directory that others can write to: %v", typ, err) + } + // The token or password file is checked too. + key := filepath.Join(open, "key") + writeFile(t, key, madeUp("key")+"\n") + f.cfg.TokenFile, f.cfg.PasswordFile = "", "" + if typ == config.StoreBWS { + f.cfg.TokenFile = key + } else { + f.cfg.PasswordFile = key + } + _, err = newStore(f.cfg, f.dir, deps{ + readPrivateFile: config.ReadPrivateFile, + checkPrivateFile: config.CheckPrivateFile, + checkParentDirs: func(string) error { return nil }, + }) + if err == nil { + t.Errorf("%s: newStore took a key file below a directory that others can write to", typ) + } + if calls := f.calls(); len(calls) != 0 { + t.Errorf("%s ran %d times, want 0", typ, len(calls)) + } + } +} + +func TestNewRefusesBadConfig(t *testing.T) { + t.Parallel() + f := setupFake(t, config.StoreBWS, fakeScenario{Token: madeUp("token")}) + tests := map[string]struct { + change func(cfg *config.Store, dir *string) + want string + }{ + "relative program": {func(c *config.Store, _ *string) { c.Program = "bws" }, "program: must be an absolute path"}, + "program with ..": {func(c *config.Store, _ *string) { c.Program = "/usr/local/../local/bin/bws" }, "program: must be an absolute path without"}, + "program with //": {func(c *config.Store, _ *string) { c.Program = "/usr/local//bin/bws" }, "program: must be an absolute path without"}, + "relative token file": {func(c *config.Store, _ *string) { c.TokenFile = "bws-token" }, "token_file: must be an absolute path"}, + "token file with .": {func(c *config.Store, _ *string) { c.TokenFile = "/etc/nodkey/./bws-token" }, "token_file: must be an absolute path without"}, + "no token file": {func(c *config.Store, _ *string) { c.TokenFile = "" }, "token_file: must be an absolute path"}, + "bw, relative password file": {func(c *config.Store, _ *string) { + c.Type, c.PasswordFile = config.StoreBW, "-password" + }, "password_file: must be an absolute path"}, + "bw, password file with ..": {func(c *config.Store, _ *string) { + c.Type, c.PasswordFile = config.StoreBW, "/etc/nodkey/../nodkey/password" + }, "password_file: must be an absolute path without"}, + "bw, no password file": {func(c *config.Store, _ *string) { c.Type = config.StoreBW }, "password_file: must be an absolute path"}, + "unknown type": {func(c *config.Store, _ *string) { c.Type = "sops" }, "unknown store type"}, + "no type": {func(c *config.Store, _ *string) { c.Type = "" }, "unknown store type"}, + "name ..": {func(c *config.Store, _ *string) { c.Name = ".." }, "cannot be the name of a directory"}, + "name .": {func(c *config.Store, _ *string) { c.Name = "." }, "cannot be the name of a directory"}, + "name with /": {func(c *config.Store, _ *string) { c.Name = "a/b" }, "cannot be the name of a directory"}, + "empty name": {func(c *config.Store, _ *string) { c.Name = "" }, "cannot be the name of a directory"}, + "relative dir": {func(_ *config.Store, d *string) { *d = "state" }, "state directory: must be an absolute path"}, + "dir with ..": {func(_ *config.Store, d *string) { *d += "/../state" }, "state directory: must be an absolute path without"}, + "missing dir": {func(_ *config.Store, d *string) { *d += "/missing" }, "no such file or directory"}, + } + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + cfg, dir := f.cfg, f.dir + tc.change(&cfg, &dir) + _, err := newStore(cfg, dir, testDeps()) + if err == nil || !strings.Contains(err.Error(), tc.want) { + t.Errorf("newStore: %v, want an error with %q", err, tc.want) + } + }) + } +} + +// TestNewPreparesTheHome is not parallel: it changes the umask of the +// whole process. +func TestNewPreparesTheHome(t *testing.T) { + f := setupFake(t, config.StoreBWS, fakeScenario{Token: madeUp("token")}) + if err := os.RemoveAll(f.home); err != nil { + t.Fatal(err) + } + // A missing home is made with mode 0700, whatever the umask. + old := syscall.Umask(0o277) + s, err := newStore(f.cfg, f.dir, testDeps()) + syscall.Umask(old) + if err != nil { + t.Fatalf("newStore: %v", err) + } + s.Close() + fi, err := os.Lstat(f.home) + if err != nil { + t.Fatal(err) + } + if !fi.IsDir() || fi.Mode().Perm() != 0o700 { + t.Errorf("home mode = %v, want a directory with 0700", fi.Mode()) + } + // The directories on the way are checked. + d := testDeps() + bad := errors.New("made-up: a directory that others can write to") + var checked []string + d.checkParentDirs = func(path string) error { + checked = append(checked, path) + return bad + } + if _, err := newStore(f.cfg, f.dir, d); !errors.Is(err, bad) { + t.Errorf("newStore with a bad directory on the way: %v", err) + } + if !slices.Equal(checked, []string{f.home}) { + t.Errorf("checked %q, want the home %q", checked, f.home) + } +} + +func TestNewRefusesABadHome(t *testing.T) { + t.Parallel() + tests := map[string]struct { + make func(t *testing.T, home string) + want string + }{ + "group can read": {func(t *testing.T, home string) { chmod(t, home, 0o750) }, "has mode 0750"}, + "others can read": {func(t *testing.T, home string) { chmod(t, home, 0o705) }, "has mode 0705"}, + "owner cannot write": {func(t *testing.T, home string) { chmod(t, home, 0o500) }, "has mode 0500"}, + "a file": {func(t *testing.T, home string) { + if err := os.RemoveAll(home); err != nil { + t.Fatal(err) + } + writeFile(t, home, "") + }, "is not a directory"}, + "a link": {func(t *testing.T, home string) { + if err := os.Rename(home, home+"-real"); err != nil { + t.Fatal(err) + } + if err := os.Symlink(home+"-real", home); err != nil { + t.Fatal(err) + } + }, "is a symbolic link"}, + } + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + f := setupFake(t, config.StoreBWS, fakeScenario{Token: madeUp("token")}) + tc.make(t, f.home) + t.Cleanup(func() { chmod(t, f.home, 0o700) }) // so t.TempDir can remove it + _, err := newStore(f.cfg, f.dir, testDeps()) + if err == nil || !strings.Contains(err.Error(), tc.want) { + t.Errorf("newStore: %v, want an error with %q", err, tc.want) + } + }) + } +} + +// chmod sets the mode of a test file, if it is not a link. +func chmod(t *testing.T, path string, mode fs.FileMode) { + t.Helper() + fi, err := os.Lstat(path) + if err != nil || fi.Mode()&fs.ModeSymlink != 0 { + return + } + if err := os.Chmod(path, mode); err != nil { + t.Fatal(err) + } +} + +// TestNoInheritedEnvironment shows that a program gets nothing from the +// broker's own environment. Variables such as NODE_OPTIONS, LD_PRELOAD or +// BW_CLEANEXIT (which makes bw exit with 0 on errors) would change what +// the program does. +func TestNoInheritedEnvironment(t *testing.T) { + t.Setenv("NODE_OPTIONS", "--require=/made-up/module.js") + t.Setenv("BW_CLEANEXIT", "true") + t.Setenv("BWS_SERVER_URL", "https://made-up.example.com") + t.Setenv("NODKEY_TEST_MADE_UP", "x") + for _, typ := range []config.StoreType{config.StoreBWS, config.StoreBW} { + value := madeUp("value") + sc := bwsScenario(madeUp("token"), value) + if typ == config.StoreBW { + sc = bwScenario(madeUp("password"), "", value, madeUp("other")) + } + f := newTestFake(t, typ, sc, testDeps()) + v, err := f.store.Fetch(t.Context(), testItemID) + if err != nil { + t.Fatalf("%s: Fetch: %v", typ, err) + } + checkValue(t, v, value) + for _, c := range f.calls() { + for _, name := range envNames(c.Env) { + if !slices.Contains([]string{"HOME", "PATH", "BWS_ACCESS_TOKEN", "BW_SESSION"}, name) { + t.Errorf("%s %q got the variable %s", typ, c.Args, name) + } + } + } + } +} + +func TestMissingProgram(t *testing.T) { + t.Parallel() + f := newTestFake(t, config.StoreBWS, bwsScenario(madeUp("token"), madeUp("value")), testDeps()) + if err := os.Remove(f.cfg.Program); err != nil { + t.Fatal(err) + } + _, err := f.store.Fetch(t.Context(), testItemID) + checkErr(t, err, ErrStart) + checkErr(t, err, syscall.ENOENT) + if err != nil && err.Error() != `store "main": bws secret get: cannot start the program: no such file or directory` { + t.Errorf("error = %v", err) + } +} + +func TestIsUUID(t *testing.T) { + t.Parallel() + for _, id := range []string{testItemID, strings.ToUpper(testItemID), "00000000-0000-0000-0000-000000000000"} { + if !isUUID(id) { + t.Errorf("isUUID(%q) = false", id) + } + } + for _, id := range []string{"", "-", testItemID + "\n", "{" + testItemID + "}", "6f1e2d3c4b5a49878a6b5c4d3e2f1a0b"} { + if isUUID(id) { + t.Errorf("isUUID(%q) = true", id) + } + } +} diff --git a/internal/store/value.go b/internal/store/value.go new file mode 100644 index 0000000..e08a3c1 --- /dev/null +++ b/internal/store/value.go @@ -0,0 +1,88 @@ +package store + +import ( + "errors" + "fmt" + "io" + "log/slog" +) + +// hidden is what fmt, slog and String show in place of a secret. +const hidden = "[hidden]" + +// errNotEncoded is the error of MarshalJSON and MarshalText. +var errNotEncoded = errors.New("store: a secret value is never encoded as JSON or text; read it with Bytes") + +// Value is a secret: a value from a store, or the bws token or the bw +// session key that a store keeps. fmt, slog, encoding/json and +// encoding/xml never show it: String, GoString, Format and LogValue give +// "[hidden]", and MarshalJSON and MarshalText fail. Read it only with +// Bytes, and Clear it when it is no longer needed. +// +// A Value points to its bytes, and copies of a Value share them: Clear on +// one copy clears them for all. fmt prints an unexported struct field +// without calling its methods; for a Value it then shows only a pointer, +// never the bytes. +type Value struct { + p *valueBytes +} + +// valueBytes holds the bytes of a Value. It hides them from fmt too. +type valueBytes struct { + b []byte +} + +// newValue returns a Value that owns b. +func newValue(b []byte) Value { + return Value{p: &valueBytes{b: b}} +} + +// Bytes returns the value. It is the Value's own memory, not a copy: do +// not change it, and do not keep it after Clear. Never log or print it. +// The zero Value and a cleared Value give nil. +func (v Value) Bytes() []byte { + if v.p == nil { + return nil + } + return v.p.b +} + +// Clear overwrites the value with zeros and forgets it. Clearing is best +// effort: a copy that a caller made, a Go string, or a buffer of the +// operating system is not cleared. Clear must not run at the same time as +// another method of the same Value. +func (v Value) Clear() { + if v.p == nil { + return + } + clear(v.p.b) + v.p.b = nil +} + +// String returns "[hidden]". +func (Value) String() string { return hidden } + +// GoString returns a text without the value, for %#v. +func (Value) GoString() string { return "store.Value{" + hidden + "}" } + +// Format writes "[hidden]" for every verb, such as %v, %s, %q and %x. +func (Value) Format(f fmt.State, _ rune) { _, _ = io.WriteString(f, hidden) } + +// LogValue returns "[hidden]", so slog never logs the value. +func (Value) LogValue() slog.Value { return slog.StringValue(hidden) } + +// MarshalJSON fails, so a value cannot be sent or logged as JSON by +// mistake. Code that sends a value must read it with Bytes. +func (Value) MarshalJSON() ([]byte, error) { return nil, errNotEncoded } + +// MarshalText fails, as MarshalJSON does. +func (Value) MarshalText() ([]byte, error) { return nil, errNotEncoded } + +// String returns "[hidden]". +func (valueBytes) String() string { return hidden } + +// GoString returns a text without the value. +func (valueBytes) GoString() string { return "store.valueBytes{" + hidden + "}" } + +// Format writes "[hidden]" for every verb. +func (valueBytes) Format(f fmt.State, _ rune) { _, _ = io.WriteString(f, hidden) } diff --git a/internal/store/value_test.go b/internal/store/value_test.go new file mode 100644 index 0000000..503200b --- /dev/null +++ b/internal/store/value_test.go @@ -0,0 +1,124 @@ +package store + +import ( + "bytes" + "encoding/gob" + "encoding/json" + "encoding/xml" + "fmt" + "log/slog" + "testing" +) + +// TestValueHidesItself prints, logs and encodes a Value in every way we +// know, also inside other values, and checks that the secret never shows. +func TestValueHidesItself(t *testing.T) { + t.Parallel() + secret := madeUp("value") + v := newValue([]byte(secret)) + hexSecret := fmt.Sprintf("%x", secret) + + type exported struct{ V Value } + type unexported struct{ v Value } + type pointer struct{ v *Value } + var texts []string + for _, x := range []any{ + v, &v, exported{v}, &exported{v}, unexported{v}, &unexported{v}, pointer{&v}, + []Value{v}, map[string]Value{"k": v}, []any{v}, v.p, *v.p, + } { + for _, verb := range []string{"%v", "%+v", "%#v", "%s", "%q", "%x", "%X", "%d", "%T"} { + texts = append(texts, fmt.Sprintf(verb, x)) + } + texts = append(texts, fmt.Sprint(x), fmt.Sprintln(x)) + } + texts = append(texts, v.String(), v.GoString()) + + var logs bytes.Buffer + for _, h := range []slog.Handler{slog.NewTextHandler(&logs, nil), slog.NewJSONHandler(&logs, nil)} { + l := slog.New(h) + l.Info("fetched", "value", v, "pointer", &v, "struct", exported{v}, "inner", unexported{v}) + l.Info("fetched", slog.Any("value", v), slog.Group("g", "value", v)) + } + texts = append(texts, logs.String()) + + for _, x := range []any{v, exported{v}, map[string]Value{"k": v}} { + out, err := json.Marshal(x) + if err == nil { + t.Errorf("json.Marshal(%T) worked", x) + } else { + texts = append(texts, err.Error()) + } + texts = append(texts, string(out)) + out, err = xml.Marshal(x) + if err != nil { + texts = append(texts, err.Error()) + } + texts = append(texts, string(out)) + } + var buf bytes.Buffer + if err := gob.NewEncoder(&buf).Encode(v); err != nil { + texts = append(texts, err.Error()) + } + texts = append(texts, buf.String()) + + for i, text := range texts { + noCanary(t, fmt.Sprintf("text %d", i), text, secret) + noCanary(t, fmt.Sprintf("text %d", i), text, hexSecret) + } + if got := fmt.Sprintf("%v|%s|%+v", v, v, exported{v}); got != "[hidden]|[hidden]|{V:[hidden]}" { + t.Errorf("fmt = %q", got) + } +} + +func TestValueClear(t *testing.T) { + t.Parallel() + b := []byte(madeUp("value")) + v := newValue(b) + c := v // a copy shares the bytes + c.Clear() + if v.Bytes() != nil || c.Bytes() != nil { + t.Error("Clear left the value in a copy") + } + if !bytes.Equal(b, make([]byte, len(b))) { + t.Error("Clear did not overwrite the bytes") + } + v.Clear() // again: nothing happens + var zero Value + zero.Clear() + if zero.Bytes() != nil { + t.Error("the zero Value has bytes") + } + if got := fmt.Sprint(zero); got != "[hidden]" { + t.Errorf("fmt of the zero Value = %q", got) + } +} + +// TestStoresHideTheirSecrets prints the stores in every way we know. +func TestStoresHideTheirSecrets(t *testing.T) { + t.Parallel() + token, password, session := madeUp("token"), madeUp("password"), madeUp("session") + bws := newTestFake(t, "bws", fakeScenario{Token: token}, testDeps()) + bw := newTestFake(t, "bw", fakeScenario{Password: password, Session: session}, testDeps()) + if err := bw.store.Start(t.Context()); err != nil { + t.Fatalf("Start: %v", err) + } + type inner struct{ s Store } + for _, s := range []Store{bws.store, bw.store} { + var logs bytes.Buffer + slog.New(slog.NewTextHandler(&logs, nil)).Info("store", "s", s) + slog.New(slog.NewJSONHandler(&logs, nil)).Info("store", "s", s) + for _, text := range []string{ + fmt.Sprintf("%v %+v %#v %s %q %x %d", s, s, s, s, s, s, s), + fmt.Sprintf("%v %+v %#v", inner{s}, inner{s}, []Store{s}), + logs.String(), + } { + for _, secret := range []string{token, password, session} { + noCanary(t, "a printed store", text, secret) + noCanary(t, "a printed store", text, fmt.Sprintf("%x", secret)) + } + } + } + if got := fmt.Sprintf("%v", bw.store); got != `bw store "vault"` { + t.Errorf("fmt = %q", got) + } +}