From 9fb2de7100357bac899673b067555879ba30a730 Mon Sep 17 00:00:00 2001 From: Geno Roupsky <185580+groupsky@users.noreply.github.com> Date: Thu, 15 Oct 2026 06:57:58 +0300 Subject: [PATCH] feat(ssh-agent): nodkey ssh-agent forwards SSH logins to the broker An SSH agent socket for plain ssh, git and Ansible; each signature request goes to the broker with its session-binds and the claimed context, and the answer is checked before ssh gets it. Closes #13 --- AGENTS.md | 5 +- DESIGN.md | 130 +- THREAT-MODEL.md | 49 + cmd/nodkey/main.go | 3 + cmd/nodkey/sshagent.go | 22 + cmd/nodkey/sshagent_test.go | 35 + internal/client/child_linux_test.go | 52 +- internal/client/client.go | 5 +- internal/client/helpers_test.go | 2 + internal/client/sshagent.go | 956 ++++++++++ internal/client/sshagent_child_linux_test.go | 362 ++++ internal/client/sshagent_command_test.go | 208 +++ internal/client/sshagent_fuzz_test.go | 219 +++ internal/client/sshagent_helpers_test.go | 590 ++++++ internal/client/sshagent_linux.go | 123 ++ .../client/sshagent_openssh_linux_test.go | 553 ++++++ internal/client/sshagent_other.go | 30 + internal/client/sshagent_proc.go | 267 +++ internal/client/sshagent_proc_linux_test.go | 139 ++ internal/client/sshagent_proc_test.go | 284 +++ internal/client/sshagent_socket.go | 298 +++ internal/client/sshagent_socket_linux_test.go | 419 +++++ internal/client/sshagent_test.go | 1630 +++++++++++++++++ internal/client/sshagent_wire.go | 213 +++ internal/config/broker.go | 12 +- 25 files changed, 6567 insertions(+), 39 deletions(-) create mode 100644 cmd/nodkey/sshagent.go create mode 100644 cmd/nodkey/sshagent_test.go create mode 100644 internal/client/sshagent.go create mode 100644 internal/client/sshagent_child_linux_test.go create mode 100644 internal/client/sshagent_command_test.go create mode 100644 internal/client/sshagent_fuzz_test.go create mode 100644 internal/client/sshagent_helpers_test.go create mode 100644 internal/client/sshagent_linux.go create mode 100644 internal/client/sshagent_openssh_linux_test.go create mode 100644 internal/client/sshagent_other.go create mode 100644 internal/client/sshagent_proc.go create mode 100644 internal/client/sshagent_proc_linux_test.go create mode 100644 internal/client/sshagent_proc_test.go create mode 100644 internal/client/sshagent_socket.go create mode 100644 internal/client/sshagent_socket_linux_test.go create mode 100644 internal/client/sshagent_test.go create mode 100644 internal/client/sshagent_wire.go diff --git a/AGENTS.md b/AGENTS.md index 2fb6042..805b751 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -23,8 +23,9 @@ Planned. Each v1 issue adds its part. the order of DESIGN.md sections 5 and 6. `nodkey broker` makes its parts and runs it. - `internal/client/` - the client side: the requests to the broker - (config, TLS key, deadline, short code, strict answer), and - `nodkey run` (DESIGN.md sections 5 and 15). + (config, TLS key, deadline, short code, strict answer), + `nodkey run` (DESIGN.md sections 5 and 15), and `nodkey ssh-agent` + (DESIGN.md sections 6 and 15). - `contrib/systemd/` - the example systemd unit of the broker, with encrypted credentials and a sandbox (DESIGN.md section 16). - `test/e2e/` - end-to-end tests with `sshd` in a container. Build tag diff --git a/DESIGN.md b/DESIGN.md index 3b31786..2fe06fc 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -223,18 +223,55 @@ NODKEY_REASON="apply the nginx fix" ansible-playbook site.yml 2. OpenSSH 8.9 and newer then sends the agent a `session-bind@openssh.com` message. It holds the server's host key, the session identifier, the server's signature over the session identifier, and an `is_forwarding` - flag. The client keeps these for that agent connection. + flag. The client keeps these for that agent connection, and answers + each with success, as OpenSSH's own agent does. It keeps at most 2, + each at most 8 KiB: the most that the broker reads. It refuses more. + Then that agent connection gets no signature, because the client could + not send all of its session-binds; it asks the broker nothing for it. 3. `ssh` asks the agent for its keys. The client asks the broker (`POST /v1/keys`, section 10). The broker returns the public keys this - client may use. + client may use. The client gives `ssh` each key with its name as the + comment, as `ssh-add -l` shows it. It asks the broker each time. 4. The server accepts one key. `ssh` asks the agent to sign the authentication request. 5. The client sends the broker (`POST /v1/sign`): the key, the data to - sign, the flags, the session-bind messages of this connection (at most - 2), and some claimed context: the command lines of the `ssh` process and - of at most 3 of its parent processes (from `/proc`), and - `NODKEY_REASON` from the environment of the `ssh` process, if set. - Section 10 limits their size. + sign and the flags, with exactly the bytes that `ssh` sent, the + session-bind messages of this connection (at most 2), and some claimed + context: the command lines of the `ssh` process and of at most 3 of its + parent processes (from `/proc`), and `NODKEY_REASON` from the + environment of the `ssh` process, if set. Section 10 limits their + size. Apart from these limits, the client checks nothing in the + request: the broker does all the checks. + - The `ssh` process is the process that connected to the agent's + socket. The kernel says which one (`SO_PEERCRED`). The client reads + `/proc/PID/cmdline` of it, and the parents from `/proc/PID/stat`. The + chain stops at a parent that cannot be read, has no command line + (such as a kernel thread), or is there twice. The client opens the + directory `/proc/PID` of a process once, and reads its files through + it, so that they all come from that one process. + - A read of `/proc` can block, such as on a stuck FUSE or NFS file + system. The client waits for it at most 2 seconds, and runs at most + 4 such reads at the same time. Otherwise the request goes without + the command lines and the reason, and a line says why. + - The reason is the first `NODKEY_REASON` in `/proc/PID/environ` of the + `ssh` process, as `getenv` finds it. The client keeps no other + variable of that environment, and does not use its own environment. + - All of this is claimed: the agent machine can change the command + line and the environment of its own processes, and a process ID can + be used again by a new process. A field that is too long becomes + `[hidden: too long]`, as for `nodkey run`. + - The request has a deadline of 115 seconds: the longest `ssh_timeout` + of the broker (110 seconds, section 14), and 5 seconds for the rest. + So a late tap still counts. It is below `sshd`'s `LoginGraceTime` + (120 seconds by default): after that, a signature cannot log in. As + for `nodkey run` (section 5), the client sends the request once, on + a connection of its own, and never again. + - When `ssh` closes its agent connection while the request waits, such + as after Ctrl-C, the client closes its connection to the broker, so + the broker cancels the request (section 7). It sees this also when + `ssh` sent more bytes before: it asks the kernel whether the other + side hung up (`poll` with `POLLRDHUP`), and does not read those + bytes. 6. The broker checks, and denies on any failure. It makes these checks before any message or place in a queue (section 8). A request that fails them gets no message, and the audit log gets it with the key name, if @@ -284,6 +321,16 @@ NODKEY_REASON="apply the nginx fix" ansible-playbook site.yml RSA, the hash that the flags ask for), and checks the new signature. Then it writes the `signed` entry, and only then returns the signature. If the client has hung up before the broker signs, it does not sign. +9. Only a final `200` whose body passes the strict decoding of section 10 + counts. The client takes the short code only from the + `102 Processing` answer, and prints it as in section 5. Before it gives + the signature to `ssh`, it checks it: the format must be the one that + the key and the flags ask for (for RSA, `rsa-sha2-256` or + `rsa-sha2-512`, as the flags say; for Ed25519 and ECDSA, the key type, + with no flags), and the signature must be valid for the data with that + key. Every other answer, and every error, gives `ssh` a failure + (`SSH_AGENT_FAILURE`), which says nothing about why. The client prints + one line that says why (section 15). The checks and the signing are in `internal/sshauth`. Signing takes the result of the checks, not raw bytes, so the broker cannot sign data that @@ -1925,9 +1972,72 @@ after it. 0, with 64 on wrong usage, and with 1 on any other error. Then its last line on stderr is `nodkey: (exit 1)`. -`nodkey ssh-agent` creates its socket in a `0700` directory with mode -`0600`. It refuses connections from other users. It refuses `ssh-add` -(adding, removing, locking keys). +`nodkey ssh-agent [--socket PATH]` serves the SSH agent protocol on a +Unix socket until `SIGINT` or `SIGTERM` (section 6). It holds no SSH key. + +- The default socket is `$XDG_RUNTIME_DIR/nodkey/agent.sock`. Without + `XDG_RUNTIME_DIR`, give `--socket`. The path must be absolute and + clean, and at most 107 bytes: the limit of a Unix socket. Point ssh to + it: `export SSH_AUTH_SOCK=$XDG_RUNTIME_DIR/nodkey/agent.sock`. +- The socket's directory must be a directory, not a symbolic link, owned + by the user, with mode `0700`. Any such directory passes, also + `$XDG_RUNTIME_DIR` itself. nodkey makes the directory with mode `0700` + if it is missing, such as `$XDG_RUNTIME_DIR/nodkey`, and never changes + a directory that is there. Every directory on the way must pass the + checks of the directories on the way to a key file (section 14), so + that no other user can swap the socket's directory. The socket has + mode `0600`, whatever the umask. +- While it runs, the agent holds a lock (`flock`) on the file + `PATH.lock` next to the socket. nodkey makes that file with mode + `0600`; it must be a regular file of the user, and it stays after the + agent ends. A second agent on the same path stops at once (exit 1), so + two agents never remove each other's socket. +- A socket that is already at the path is removed only when it is a + socket of the user that no program serves: an agent ended without + removing it. If a program serves it, or anything else is at the path, + it stays, and the agent does not start. +- It refuses connections of other users, also of root: the kernel says + who connected (`SO_PEERCRED`). Root can read the client's TLS key + anyway. +- It serves at most 128 connections at the same time, and closes more at + once. A message is at most 256 KiB, as for OpenSSH's own agent; its + buffer grows as its bytes come, so a length alone takes little memory. +- It refuses `ssh-add` (adding, removing, locking and unlocking keys), + every extension but `session-bind@openssh.com`, and every request that + it does not know, with `SSH_AGENT_FAILURE`. It does not parse the + private key that `ssh-add` sends, and clears that message from its + memory, as best effort. +- It serves the agent protocol itself. The agent server of + `golang.org/x/crypto` writes the text of each error to Go's global + logger, without escaping and without a limit, so nodkey does not use + it, and sends Go's global logger nowhere. +- On stderr, it prints where it listens, the short code of each request + that waits for the person (`nodkey: waiting for approval (request + k3f9)`), and one line about each request that it refused or that + failed, such as `nodkey: no signature (request k3f9): denied`, with the + same texts as `nodkey run`: `denied` for `403`, and `denied by a limit + of the broker` for `429`. A line holds only fixed text, numbers, the + short code, the broker's address, the socket's path and errors of the + operating system, never text that a process on the agent machine sent. +- Any process of the user can cause both kinds of lines: the broker + sends a short code for every request that starts to wait, also in its + queue. So each kind has its own budget: at most 60 lines in a minute. + Honest use has at most 15 requests that wait at the same time (section + 8). The lines over a budget are counted. The next line of the same kind + in a later minute says how many were left out, and so does the agent + when it stops. +- On `SIGINT` or `SIGTERM`, it closes the open connections, so the broker + cancels the requests that still wait, and it waits for the connections + at most 5 seconds. It removes its socket, if the socket is still the + one it made, prints `nodkey: stopped by SIGTERM (exit 143)`, and ends + itself with the same signal, as `nodkey run` does. So a shell sees 130 + or 143, and systemd sees a clean stop. A signal while the agent starts + ends it the same way. After the first signal, nodkey no longer catches + them: a second one ends it at once, and the socket stays until the next + start removes it. +- It exits with 64 on wrong usage, with 69 when the client config or the + TLS key cannot be read (as `nodkey run`), and with 1 when it cannot make + its socket. `nodkey audit verify FILE [--anchor SEQ:HASH]... [--anchors FILE|-]` takes `FILE` before or after the options. diff --git a/THREAT-MODEL.md b/THREAT-MODEL.md index de5799c..d89de81 100644 --- a/THREAT-MODEL.md +++ b/THREAT-MODEL.md @@ -115,6 +115,10 @@ request waits, its new target runs. Whoever can change the link, its directory or the file already controls that program (for `/usr/bin`, only root). This helps against honest mistakes and tricks, not against a hostile agent: the client runs on the agent machine. +For an SSH login, `nodkey ssh-agent` reads the command lines of `ssh` and +its parents, and the reason, from `/proc` (DESIGN.md section 6). A +process can change its own command line and environment, and a process ID +can be used again by a new process. So these are claims too. Risk that remains: the person can still be fooled. Deny any request that looks strange. @@ -291,6 +295,13 @@ result of the checks, not raw bytes, so the broker signs only data that it checked. The forwarding flag comes from the client, so refusing it stops honest mistakes, not a hostile agent; the host check above does not depend on that flag. +`nodkey ssh-agent` sends the broker every session-bind of the agent +connection, at most 2, with the exact bytes of the key, the data and the +flags; it refuses a third session-bind, and then asks nothing for that +connection. It gives `ssh` a signature only after it checked that the +format fits the key and the flags, and that the signature is valid for +the data. That is a check of the broker's answer, as for `nodkey run` +(T10); the checks that matter are the broker's. Risk that remains: low. **T13. The agent keeps access after an approved login.** See section 5, @@ -640,6 +651,44 @@ 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. +**T30. Another user on the agent machine uses the agent socket.** +With the socket, a user could ask for signatures in the name of this +client, and the message would show this machine. `nodkey ssh-agent` puts +its socket in a directory of the user with mode `0700`, below directories +that no other user can change, and gives the socket mode `0600`. It also +asks the kernel who connected (`SO_PEERCRED`), and closes the connection +of any other user, also of root, before it reads a request. It never +removes a file that is not a socket of the user, or a socket that a +program serves, and it holds a lock while it runs, so a second agent on +the same path does not start (DESIGN.md section 15). +Risk that remains: root on the agent machine can still act as the user, +and so use the socket. Root can read the client's TLS key anyway (T11), +and every request still needs the approver. Any process of the user can +keep the agent busy, but it serves at most 128 connections at the same +time, and a message takes memory only as its bytes come. That is a +denial of service, which a process of the user can cause anyway. + +**T31. A process on the agent machine writes into the agent's log.** +Any process of the user can send `nodkey ssh-agent` requests, and the +agent's stderr may go to a terminal or to the journal. The agent server +of `golang.org/x/crypto` writes the text of each error to Go's global +logger, without escaping and without a limit. So `nodkey ssh-agent` +serves the agent protocol itself, and sends Go's global logger nowhere. +Its own lines hold fixed text, numbers, the short code, the broker's +address, the socket's path and errors of the operating system, never text +that a process sent. A process can also cause many lines with a short +code: the broker sends one for every request that starts to wait, also in +its queue, and a request that hangs up frees its place. So the agent +writes at most 60 lines with a short code, and 60 about refused and +failed requests, in a minute, each kind with its own budget, and says +how many it left out. A test checks that a refused request gives one +fixed line, and nothing in Go's global logger. +Risk that remains: low. A process can still fill a budget. Then the +other lines of that kind are left out for the rest of that minute, also +the short code of a real request. The person can still match the message +on the phone by its verified lines: the host, the key and the remote +user. + ## 7. Assumptions - The broker host is patched, has no agent on it, and only the owner has diff --git a/cmd/nodkey/main.go b/cmd/nodkey/main.go index 25ec59d..386d49d 100644 --- a/cmd/nodkey/main.go +++ b/cmd/nodkey/main.go @@ -35,6 +35,7 @@ Commands: run the broker: the daemon that asks the approver on Telegram keygen make a TLS key for the broker or a client, and print its pin run run a command with secrets, once a person approves on the phone + ssh-agent serve an SSH agent for ssh; the keys stay on the broker audit verify FILE [--anchor SEQ:HASH]... [--anchors FILE|-] check the hash chain of an audit log, and its anchors version print the version @@ -70,6 +71,8 @@ func run(args []string, stdin io.Reader, stdout, stderr io.Writer) int { return runAudit(rest, stdin, stdout, stderr) case "run": return runRun(rest, stderr) + case "ssh-agent": + return runSSHAgent(rest, stderr) case "version": return runVersion(rest, stdout, stderr) case "help": diff --git a/cmd/nodkey/sshagent.go b/cmd/nodkey/sshagent.go new file mode 100644 index 0000000..860fb67 --- /dev/null +++ b/cmd/nodkey/sshagent.go @@ -0,0 +1,22 @@ +package main + +import ( + "io" + "log" + + "github.com/code6x/nodkey/internal/client" +) + +// runSSHAgent runs nodkey ssh-agent (package client) until SIGINT or +// SIGTERM. +// +// The agent serves the SSH agent protocol itself, and writes its own +// fixed lines. It does not use the agent server of golang.org/x/crypto, +// which writes the text of every error to Go's global logger, unescaped +// and without a limit. In case any library still writes to that logger, +// its lines go nowhere: they could hold text that a process on the agent +// machine sent. +func runSSHAgent(args []string, stderr io.Writer) int { + log.SetOutput(io.Discard) + return client.SSHAgent(args, stderr) +} diff --git a/cmd/nodkey/sshagent_test.go b/cmd/nodkey/sshagent_test.go new file mode 100644 index 0000000..5747421 --- /dev/null +++ b/cmd/nodkey/sshagent_test.go @@ -0,0 +1,35 @@ +package main + +import ( + "bytes" + "io" + "log" + "strings" + "testing" +) + +// TestSSHAgentCommand: nodkey ssh-agent is a command of nodkey. -h prints +// its usage; wrong usage prints its usage, and the last line says why, +// with exit code 64. It sends Go's global logger nowhere, so that no +// library writes text that a process on the agent machine sent. +func TestSSHAgentCommand(t *testing.T) { // not parallel: it changes the global logger + prev := log.Writer() + t.Cleanup(func() { log.SetOutput(prev) }) + var stdout, stderr bytes.Buffer + if code := run([]string{"ssh-agent", "-h"}, strings.NewReader(""), &stdout, &stderr); code != exitOK || + !strings.HasPrefix(stderr.String(), "Usage: nodkey ssh-agent [--socket PATH]\n") || stdout.Len() != 0 { + t.Errorf("nodkey ssh-agent -h: exit code %d, stdout %q, stderr %q", code, stdout.String(), stderr.String()) + } + if log.Writer() != io.Discard { + t.Error("the global logger still writes") + } + stderr.Reset() + if code := run([]string{"ssh-agent", "--bogus"}, strings.NewReader(""), &stdout, &stderr); code != exitUsage || + !strings.HasSuffix(stderr.String(), "(exit 64)\n") { + t.Errorf("nodkey ssh-agent --bogus: exit code %d, stderr %q", code, stderr.String()) + } + stdout.Reset() + if run([]string{"help"}, strings.NewReader(""), &stdout, &bytes.Buffer{}); !strings.Contains(stdout.String(), "\n ssh-agent ") { + t.Errorf("help does not list ssh-agent: %q", stdout.String()) + } +} diff --git a/internal/client/child_linux_test.go b/internal/client/child_linux_test.go index f9fd9a8..f2a0a12 100644 --- a/internal/client/child_linux_test.go +++ b/internal/client/child_linux_test.go @@ -25,9 +25,10 @@ import ( // These tests run nodkey run in a child process, with the real execve and // the real signals. The child is the test binary again: with the // variables childAddr, childPin and childKey it runs as nodkey run, with -// the broker of the test. The command that it starts is the test binary -// too, with the first argument helperArg (see helper), or a program such -// as cat. +// the broker of the test, and with childAgent too, as nodkey ssh-agent +// (childSSHAgent). The command that it starts is the test binary too, +// with the first argument helperArg (see helper), or a program such as +// cat. // The variables of a child that runs as nodkey run. const ( @@ -44,33 +45,42 @@ func TestMain(m *testing.M) { if len(os.Args) > 1 && os.Args[1] == helperArg { os.Exit(helper(os.Args[2:])) } + if len(os.Args) > 1 && os.Args[1] == agentClientArg { + os.Exit(agentClient(os.Args[2:])) + } + if os.Getenv(childAgent) != "" { + os.Exit(childSSHAgent(os.Args[1:])) + } if os.Getenv(childAddr) != "" { os.Exit(childRun(os.Args[1:])) } os.Exit(m.Run()) } +// childLoad takes the place of Load in a child: it reads the key file and +// the broker of the test. Load would refuse the system temporary +// directory, which everyone can write to. +func childLoad(string) (*Broker, error) { + pin, err := config.ParsePin(os.Getenv(childPin)) + if err != nil { + return nil, err + } + data, err := os.ReadFile(os.Getenv(childKey)) //nolint:gosec // G703: the test wrote this key file, and passes its path. + if err != nil { + return nil, err + } + key, err := transport.ParseKeyFile(data) + if err != nil { + return nil, err + } + return newBroker(os.Getenv(childAddr), key, pin) +} + // childRun runs nodkey run as Run does, with the real signals and the real -// execve, but with the key file and the broker of the test: Load would -// refuse the system temporary directory, which everyone can write to. +// execve, but with the key file and the broker of the test (childLoad). func childRun(args []string) int { - load := func(string) (*Broker, error) { - pin, err := config.ParsePin(os.Getenv(childPin)) - if err != nil { - return nil, err - } - data, err := os.ReadFile(os.Getenv(childKey)) //nolint:gosec // G703: the test wrote this key file, and passes its path. - if err != nil { - return nil, err - } - key, err := transport.ParseKeyFile(data) - if err != nil { - return nil, err - } - return newBroker(os.Getenv(childAddr), key, pin) - } r := runner{ - load: load, + load: childLoad, configPath: func() (string, error) { return "test", nil }, environ: os.Environ, exec: checkedExec, diff --git a/internal/client/client.go b/internal/client/client.go index 38ccbb1..9cf5351 100644 --- a/internal/client/client.go +++ b/internal/client/client.go @@ -1,5 +1,6 @@ -// Package client is the client side of nodkey (DESIGN.md sections 5 and -// 10): the requests of a client to the broker, and the command nodkey run. +// Package client is the client side of nodkey (DESIGN.md sections 5, 6 +// and 10): the requests of a client to the broker, and the commands +// nodkey run and nodkey ssh-agent. // // A client reads its config with config.LoadClient and its TLS key with // transport.LoadKeyFile, and talks to the broker over the pinned mutual diff --git a/internal/client/helpers_test.go b/internal/client/helpers_test.go index 27fd0c7..01cecec 100644 --- a/internal/client/helpers_test.go +++ b/internal/client/helpers_test.go @@ -230,6 +230,8 @@ func eventNames(events []audit.Event) string { names[i] = "decision:" + string(e.Answer) case audit.SecretSent: names[i] = "secret_sent" + case audit.Signed: + names[i] = "signed" case audit.Denied: names[i] = "denied:" + string(e.Cause) case audit.Failure: diff --git a/internal/client/sshagent.go b/internal/client/sshagent.go new file mode 100644 index 0000000..6756b10 --- /dev/null +++ b/internal/client/sshagent.go @@ -0,0 +1,956 @@ +package client + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "io" + "math" + "net" + "os" + "regexp" + "strings" + "sync" + "sync/atomic" + "time" + + "golang.org/x/crypto/ssh" + "golang.org/x/crypto/ssh/agent" + + "github.com/code6x/nodkey/internal/api" + "github.com/code6x/nodkey/internal/config" + "github.com/code6x/nodkey/internal/transport" +) + +// nodkey ssh-agent (DESIGN.md sections 6 and 15) serves the SSH agent +// protocol on a Unix socket, so that plain ssh, git and Ansible work. It +// holds no SSH key. For each list request it asks the broker for the keys +// of this client (api.PathKeys), and for each sign request it asks the +// broker for the signature (api.PathSign), with the session-bind messages +// of that agent connection and the claimed context from /proc. The broker +// checks the real target host and asks the person. The agent refuses to +// add, remove, lock or unlock keys, and every other extension. +// +// The requests to the broker follow the rules of nodkey run: one POST on +// a connection of its own, with a deadline, and never again (Post). Only a +// final 200 whose body passes api.ReadAnswer counts, and the short code +// comes only from the 102 Processing answer. Before the agent hands a +// signature to ssh, it checks that its format fits the key and the sign +// flags, and that it is a valid signature of the data with that key. Any +// other result is SSH_AGENT_FAILURE, which says nothing about why. +// +// The agent writes its lines to its stderr: where it listens, the short +// code of each request that waits for the person, and why a request was +// refused or failed. Each line is fixed text, with at most numbers, the +// short code, the broker's address from the client config, the socket +// path and errors of the operating system. No line holds text that ssh or +// another process sent. The lines with a short code, and the lines about +// refused and failed requests, each have a limit per minute +// (agentLogLines). + +// exitAgentFailed is the exit code of nodkey ssh-agent when it cannot make +// its socket, or the socket stops working. +const exitAgentFailed = 1 + +// signTimeout is the deadline of a request for a signature. The broker +// waits for the person at most its ssh_timeout, which is at most +// config.MaxSSHTimeout (110 s). sshd drops a login that does not finish +// within its LoginGraceTime, 120 seconds by default. The deadline lies +// between them: a late tap still counts, and the agent gives up before +// sshd does. +const signTimeout = config.MaxSSHTimeout + 5*time.Second + +// keysTimeout is the deadline of a request for the list of keys. The +// broker answers it at once, without asking anyone. +const keysTimeout = 30 * time.Second + +// agentLogLines is the most lines of one kind in a minute: lines with the +// short code of a request that waits for the person, and lines about +// refused and failed requests. Any process of the user can cause both +// kinds at will: the broker sends a short code for every request that +// starts to wait, in the queue too, and a request that hangs up frees its +// place. Any other user can cause the lines about refused connections. +// Honest use has at most 15 requests that wait at the same time (5 +// messages and 10 in the queue, DESIGN.md section 8). The lines over the +// limit are counted. The count goes out with the next line of the same +// kind in a later minute, or when the agent stops. +const ( + agentLogLines = 60 + agentLogWindow = time.Minute +) + +// maxAgentConns is the most connections that the agent serves at the same +// time. ssh uses one for each login, and Ansible makes 5 logins at the +// same time by default. A connection over it is closed at once. +const maxAgentConns = 128 + +// procTimeout is the longest wait for the claimed context from /proc. A +// read there can block, such as on a mapping of a stuck FUSE or NFS file +// system. Then the request goes without the command lines and the +// reason. +const procTimeout = 2 * time.Second + +// maxProcReads is the most reads of /proc at the same time. A read that +// blocks keeps its goroutine. When this many block, the agent starts no +// other read, and requests go without the claimed context. +const maxProcReads = 4 + +// stopWait is the longest wait for the connections when the agent stops. +// Every wait of a connection has its own limit, so this is only a last +// resort. +const stopWait = 5 * time.Second + +// agentUsage is the help of nodkey ssh-agent. +const agentUsage = `Usage: nodkey ssh-agent [--socket PATH] + +Serve an SSH agent on a Unix socket, for ssh, git and Ansible. The SSH keys +stay on the broker. For each login, the agent asks the broker for a +signature, and the broker asks a person on the phone. Point ssh to the +socket: + + export SSH_AUTH_SOCK=$XDG_RUNTIME_DIR/nodkey/agent.sock + + --socket PATH the socket. The default is $XDG_RUNTIME_DIR/nodkey/agent.sock. + Its directory must have mode 0700; nodkey makes it if it is + missing. + +The message shows NODKEY_REASON from the environment of ssh as the reason. +The agent cannot add, remove or lock keys. The client config is +~/.config/nodkey/client.toml. nodkey ssh-agent runs until SIGINT or +SIGTERM, and then removes the socket. It exits with 64 (wrong usage), 69 +(the client config or the TLS key cannot be read) or 1 (the socket cannot +be made). +` + +// SSHAgent runs nodkey ssh-agent with args, the arguments after +// "ssh-agent", and writes its lines to stderr. It serves the agent until +// SIGINT or SIGTERM. Then it removes the socket, writes its last line, +// such as "nodkey: stopped by SIGTERM (exit 143)", and ends nodkey with +// the same signal, as nodkey run does. Otherwise it returns the exit code +// of DESIGN.md section 15, and its last line is "nodkey: (exit +// )". +func SSHAgent(args []string, stderr io.Writer) int { + r := agentRunner{ + load: Load, + configPath: ConfigPath, + getenv: os.Getenv, + checkDirs: config.CheckParentDirs, + proc: procFS{root: procDir}, + peer: peerCredentials, + } + return r.main(args, stderr) +} + +// agentRunner runs nodkey ssh-agent. Tests replace its parts. +type agentRunner struct { + // load reads the client config and the TLS key: Load. + load func(path string) (*Broker, error) + // configPath returns the path of the client config: ConfigPath. + configPath func() (string, error) + // getenv reads the agent's own environment, for XDG_RUNTIME_DIR. + getenv func(string) string + // checkDirs checks the directories on the way to the socket: + // config.CheckParentDirs. + checkDirs func(path string) error + // proc reads the claimed context of a sign request. + proc procFS + // peer returns who connected to the socket: peerCredentials. + peer func(*net.UnixConn) (peer, error) + // ready, if set, gets the socket path once the agent serves it. + ready func(path string) + // tune, if set, changes the agent before it serves, such as its + // limits. Tests use it. + tune func(*sshAgent) +} + +// main is SSHAgent with the parts of r. +func (r agentRunner) main(args []string, stderr io.Writer) int { + ctx, cancel := context.WithCancelCause(context.Background()) + defer cancel(nil) + in := watchInterrupts(cancel) + defer in.stop() + // After the first SIGINT or SIGTERM, nodkey no longer catches them: a + // second one ends it at once, even when it still waits for its + // connections. + stopCatching := context.AfterFunc(ctx, in.stop) + defer stopCatching() + code := r.run(ctx, args, stderr) + var sig interrupted + if errors.As(context.Cause(ctx), &sig) && code == sig.status() { + // As for nodkey run: a shell sees the signal, and systemd counts + // SIGTERM as a clean stop. + in.stop() + raise(sig.sig) + } + return code +} + +// run runs nodkey ssh-agent until ctx ends, and returns its exit code. +func (r agentRunner) run(ctx context.Context, args []string, w io.Writer) int { + log := newAgentLog(w) + if len(args) == 1 && isHelp(args[0]) { + _, _ = io.WriteString(log, agentUsage) + return exitOK + } + path, err := parseAgentArgs(args) + if err == nil && path == "" { + path, err = defaultSocket(r.getenv) + } + if err == nil { + err = checkSocketPath(path) + } + if err != nil { + _, _ = io.WriteString(log, agentUsage) + return fail(log, exitUsage, err.Error()) + } + configPath, err := r.configPath() + if err != nil { + return fail(log, exitUnavailable, err.Error()) + } + b, err := r.load(configPath) + if err != nil { + return fail(log, exitUnavailable, err.Error()) + } + self, err := currentUID() + if err != nil { + return fail(log, exitAgentFailed, err.Error()) + } + sock, err := listenAgent(ctx, path, self, r.checkDirs) + if err != nil { + if code, ok := stoppedBySignal(ctx, log); ok { + return code // the signal came while the agent started + } + return fail(log, exitAgentFailed, err.Error()) + } + a := newSSHAgent(b, log, r.proc, r.peer, self) + if r.tune != nil { + r.tune(a) + } + log.say("the SSH agent listens on %s; set SSH_AUTH_SOCK to this path for ssh", path) + if r.ready != nil { + r.ready(path) + } + a.serve(ctx, sock.ln) + sock.close() + log.flush() + if code, ok := stoppedBySignal(ctx, log); ok { + return code + } + if ctx.Err() != nil { + return fail(log, exitOK, "stopped") + } + return fail(log, exitAgentFailed, "the socket stopped working") +} + +// stoppedBySignal writes the last line when SIGINT or SIGTERM ended ctx, +// and returns the status that a shell shows for the signal. +func stoppedBySignal(ctx context.Context, log io.Writer) (int, bool) { + var sig interrupted + if !errors.As(context.Cause(ctx), &sig) { + return 0, false + } + return fail(log, sig.status(), "stopped by "+sig.name()), true +} + +// parseAgentArgs reads the arguments of nodkey ssh-agent: at most one +// --socket PATH, also with one dash and with "=" before the value, as the +// flag package reads them. It returns "" when --socket is not given. +func parseAgentArgs(args []string) (string, error) { + socket, set := "", false + for rest := args; len(rest) > 0; { + arg := rest[0] + rest = rest[1:] + name, value, hasValue := splitOption(arg) + switch name { + case "socket": + case "h", "help": + return "", errors.New("--help must be the only argument") + default: + if !strings.HasPrefix(arg, "-") { + return "", fmt.Errorf("unexpected argument %.64q", arg) + } + return "", fmt.Errorf("unknown option %.64q", arg) + } + if !hasValue { + if len(rest) == 0 { + return "", errors.New("--socket needs a value") + } + value, rest = rest[0], rest[1:] + } + if set { + return "", errors.New("--socket is given twice") + } + if value == "" { + return "", errors.New("--socket needs a path") + } + socket, set = value, true + } + return socket, nil +} + +// currentUID returns the user ID that runs nodkey. +func currentUID() (uint32, error) { + euid := os.Geteuid() + if euid < 0 || uint64(euid) > math.MaxUint32 { + return 0, errors.New("cannot get the user ID on this system") + } + return uint32(euid), nil +} + +// peer is the process at the other end of an agent connection, as the +// kernel recorded it at connect(2). +type peer struct { + uid uint32 + pid int32 // 0 when it is not known +} + +// checkPeer allows only the user that runs nodkey ssh-agent (DESIGN.md +// section 15). Every other user is refused, also root: root can read the +// client's TLS key anyway, and ask the broker with it. +func checkPeer(p peer, self uint32) error { + if p.uid != self { + return fmt.Errorf("refused a connection of user ID %d: only user ID %d may use this agent", p.uid, self) + } + return nil +} + +// sshAgent serves the agent protocol to ssh, and asks the broker. Make it +// with newSSHAgent; tests change its limits. +type sshAgent struct { + broker *Broker + log *agentLog + proc procFS + peer func(*net.UnixConn) (peer, error) + self uint32 + // keysTimeout and signTimeout are the deadlines of the requests to the + // broker. + keysTimeout, signTimeout time.Duration + // maxConns is the most open connections: maxAgentConns. + maxConns int + // procTimeout is the longest wait for the claimed context: + // procTimeout. + procTimeout time.Duration + // stopWait is the longest wait for the connections at the end: + // stopWait. + stopWait time.Duration + + procReads atomic.Int32 // reads of /proc that run now + + mu sync.Mutex + closed bool // the agent stops: no new connection + conns map[*net.UnixConn]struct{} // the open connections + wg sync.WaitGroup // one for each connection +} + +// newSSHAgent returns an agent that asks the broker b, writes its lines to +// log, reads the claimed context with proc, checks each connection with +// peer, and serves only the user self. +func newSSHAgent(b *Broker, log *agentLog, proc procFS, peer func(*net.UnixConn) (peer, error), + self uint32) *sshAgent { + return &sshAgent{broker: b, log: log, proc: proc, peer: peer, self: self, + keysTimeout: keysTimeout, signTimeout: signTimeout, maxConns: maxAgentConns, procTimeout: procTimeout, + stopWait: stopWait} +} + +// serve serves the connections to ln until ctx ends or ln fails. Then it +// closes ln and every connection, and waits until each has ended. A +// request to the broker that still runs then ends too: the connection to +// the broker closes, so the broker cancels the request. +func (a *sshAgent) serve(ctx context.Context, ln *net.UnixListener) { + stop := context.AfterFunc(ctx, func() { _ = ln.Close() }) + defer stop() + var backoff time.Duration + for { + c, err := ln.AcceptUnix() + if err != nil { + if ctx.Err() != nil || errors.Is(err, net.ErrClosed) { + break + } + // Such as too many open files: wait a little, and try again. + backoff = min(max(2*backoff, 5*time.Millisecond), time.Second) + a.log.note("cannot take a connection: %s", systemError(err)) + sleep(ctx, backoff) + continue + } + backoff = 0 + switch a.track(c) { + case trackStopping: + _ = c.Close() + continue + case trackFull: + a.log.note("refused a connection: %d connections are open, the most that the agent serves", a.maxConns) + _ = c.Close() + continue + } + a.wg.Go(func() { + defer a.untrack(c) + a.serveConn(ctx, c) + }) + } + a.closeAll() + a.waitConns() +} + +// waitConns waits until every connection has ended, but at most +// a.stopWait: each wait of a connection has its own limit, so this is +// only a last resort. +func (a *sshAgent) waitConns() { + done := make(chan struct{}) + go func() { + a.wg.Wait() + close(done) + }() + t := time.NewTimer(a.stopWait) + defer t.Stop() + select { + case <-done: + case <-t.C: + a.mu.Lock() + n := len(a.conns) + a.mu.Unlock() + a.log.note("stopped without waiting longer for its connections (%d still open)", n) + } +} + +// sleep waits for d, or until ctx ends. +func sleep(ctx context.Context, d time.Duration) { + t := time.NewTimer(d) + defer t.Stop() + select { + case <-t.C: + case <-ctx.Done(): + } +} + +// The results of track. +const ( + trackOK = iota // c is open and tracked + trackStopping // the agent stops + trackFull // a.maxConns connections are open +) + +// track adds c to the open connections, unless the agent stops or +// a.maxConns connections are open. +func (a *sshAgent) track(c *net.UnixConn) int { + a.mu.Lock() + defer a.mu.Unlock() + switch { + case a.closed: + return trackStopping + case len(a.conns) >= a.maxConns: + return trackFull + } + if a.conns == nil { + a.conns = make(map[*net.UnixConn]struct{}) + } + a.conns[c] = struct{}{} + return trackOK +} + +// untrack removes c from the open connections. +func (a *sshAgent) untrack(c *net.UnixConn) { + a.mu.Lock() + defer a.mu.Unlock() + delete(a.conns, c) +} + +// closeAll closes every open connection, and takes no new one. +func (a *sshAgent) closeAll() { + a.mu.Lock() + defer a.mu.Unlock() + a.closed = true + for c := range a.conns { + _ = c.Close() + } +} + +// serveConn serves one connection: first it checks the user that +// connected (SO_PEERCRED), then it answers one request after the other, +// until the connection closes. A message that is empty or longer than +// maxAgentMessage closes the connection. +func (a *sshAgent) serveConn(ctx context.Context, c *net.UnixConn) { + defer func() { _ = c.Close() }() + defer func() { + // A request must never stop the agent. The panic value is not + // written: it can hold anything from the request. + if recover() != nil { + a.log.note("a request made the agent fail; its connection is closed") + } + }() + p, err := a.peer(c) + if err != nil { + a.log.note("refused a connection: its user cannot be checked") + return + } + if err := checkPeer(p, a.self); err != nil { + a.log.note("%s", err.Error()) + return + } + conn := &agentConn{a: a, conn: c, pid: p.pid} + for { + msg, err := readAgentMessage(c) + if err != nil { + if errors.Is(err, errMessageSize) { + a.log.note("closed a connection that sent a message that is empty or longer than %d bytes", + maxAgentMessage) + } + return + } + answer := conn.handle(ctx, msg) + // An add request holds a private key, and a lock request a + // passphrase. Best effort: the operating system keeps copies. + clear(msg) + if err := writeAgentMessage(c, answer); err != nil { + return + } + } +} + +// agentConn is one connection to the agent. Its methods run in the one +// goroutine of the connection. +type agentConn struct { + a *sshAgent + conn *net.UnixConn + pid int32 // the process that connected, from SO_PEERCRED + // binds are the contents of the session-bind@openssh.com messages of + // this connection, in order: at most api.MaxBinds. + binds [][]byte + // refusedBind is set when the agent refused a session-bind. Then this + // connection gets no signature: the agent could not send all of its + // session-binds. + refusedBind bool +} + +// handle answers one request. +func (c *agentConn) handle(ctx context.Context, msg []byte) []byte { + body := msg[1:] + switch msg[0] { + case agentListRequest: + if len(body) != 0 { + c.a.log.note("refused a key list request that is not valid") + return failureMessage + } + return c.a.list(ctx) + case agentSignRequest: + key, data, flags, ok := parseSignRequest(body) + if !ok { + c.a.log.note("refused a sign request that is not valid") + return failureMessage + } + return c.sign(ctx, key, data, flags) + case agentExtension: + name, contents, ok := parseExtension(body) + switch { + case !ok: + c.a.log.note("refused an extension request that is not valid") + return failureMessage + case name != sessionBindRequest: + c.a.log.note("refused an extension request: the agent knows only " + sessionBindRequest) + return failureMessage + } + return c.bind(contents) + case agentAddIdentity, agentAddConstrained, agentAddSmartcardKey, agentAddSmartcardConstrained, + agentRemoveIdentity, agentRemoveAll, agentRemoveSmartcardKey, agentLock, agentUnlock, + agentAddRSA, agentRemoveRSA, agentRemoveAllRSA, agentAddRSAConstrained: + c.a.log.note("refused a request to add, remove, lock or unlock keys: the keys stay on the broker") + return failureMessage + } + c.a.log.note("refused an agent request of the unknown type %d", msg[0]) + return failureMessage +} + +// bind keeps the contents of a session-bind@openssh.com message for this +// connection, and answers success, as OpenSSH's own agent does. The +// broker checks it. The agent keeps at most api.MaxBinds of them, each at +// most api.MaxBind bytes: the most that the broker reads. It refuses +// more, and then this connection gets no signature. +func (c *agentConn) bind(contents []byte) []byte { + switch { + case c.refusedBind: + c.a.log.note("refused a session-bind: the agent refused one before on this connection") + case len(contents) > api.MaxBind: + c.a.log.note("refused a session-bind longer than %d bytes; this connection gets no signature", api.MaxBind) + case len(c.binds) >= api.MaxBinds: + c.a.log.note("refused a session-bind after %d of them; this connection gets no signature", api.MaxBinds) + default: + c.binds = append(c.binds, bytes.Clone(contents)) + return []byte{agentSuccess} + } + c.refusedBind = true + return failureMessage +} + +// sign asks the broker for a signature of data with key (DESIGN.md +// section 6, step 5). It sends the exact bytes of the key, the data and +// the flags that ssh sent, every session-bind of this connection, and the +// claimed context of the process that connected. It checks the answer +// with checkSignAnswer before it gives the signature to ssh. When ssh +// closes the connection while the request waits, it ends the request. +func (c *agentConn) sign(ctx context.Context, key, data []byte, flags uint32) []byte { + a := c.a + switch { + case c.refusedBind: + a.log.note("no signature: the agent refused a session-bind on this connection") + return failureMessage + case len(key) > api.MaxKey, len(data) > api.MaxData: + a.log.note("no signature: the key or the data to sign is longer than the broker reads") + return failureMessage + } + commands, reason := a.claimed(ctx, c.pid) + body, err := encodeSignRequest(newSignRequest(key, data, flags, c.binds, commands, reason)) + if err != nil { + a.log.note("no signature: %s", err.Error()) + return failureMessage + } + code := &shortCode{} + reqCtx, cancel := context.WithTimeoutCause(ctx, a.signTimeout, deadline{a.signTimeout}) + defer cancel() + // When ssh ends while the request waits, such as after Ctrl-C, the + // connection to the broker closes, so the broker cancels the request + // (DESIGN.md section 7). + waitCtx, hangUp := context.WithCancelCause(reqCtx) + defer hangUp(nil) + stop := watchHangUp(c.conn, hangUp) + ans, err := Post[api.SignAnswer](waitCtx, a.broker, api.PathSign, body, func(s string) { + code.set(s) + a.log.waitingFor(s) + }) + stop() + if err != nil { + a.log.note("no signature%s: %s", code.label(), failureText(err)) + return failureMessage + } + sig, err := checkSignAnswer(key, data, flags, ans) + if err != nil { + a.log.note("no signature%s: %s", code.label(), err.Error()) + return failureMessage + } + return signAnswerMessage(sig) +} + +// claimed returns the claimed context of a sign request from process pid +// (procFS.claimedContext). It waits for the reads of /proc at most +// a.procTimeout, and at most maxProcReads such reads run at the same +// time: a read can block, such as on a stuck FUSE or NFS file system. +// Otherwise the request goes without the command lines and the reason, +// and a line says why. +func (a *sshAgent) claimed(ctx context.Context, pid int32) ([][]string, string) { + if a.procReads.Add(1) > maxProcReads { + a.procReads.Add(-1) + a.log.note("sent a sign request without the command lines and the reason: %d reads of /proc are stuck", + maxProcReads) + return [][]string{}, "" + } + type found struct { + commands [][]string + reason string + } + got := make(chan found, 1) + go func() { + defer a.procReads.Add(-1) + commands, reason := a.proc.claimedContext(pid) + got <- found{commands, reason} + }() + t := time.NewTimer(a.procTimeout) + defer t.Stop() + select { + case c := <-got: + return c.commands, c.reason + case <-t.C: + a.log.note("sent a sign request without the command lines and the reason: /proc did not answer within %s", + a.procTimeout) + case <-ctx.Done(): + } + return [][]string{}, "" +} + +// list asks the broker for the keys that this client may use, and +// answers ssh with them. The comment of each key is its name in the +// broker config. +func (a *sshAgent) list(ctx context.Context) []byte { + reqCtx, cancel := context.WithTimeoutCause(ctx, a.keysTimeout, deadline{a.keysTimeout}) + defer cancel() + ans, err := Post[api.KeysAnswer](reqCtx, a.broker, api.PathKeys, []byte("{}"), nil) + if err != nil { + a.log.note("no key list: %s", failureText(err)) + return failureMessage + } + if err := checkKeys(ans.Keys); err != nil { + a.log.note("no key list: %s", err.Error()) + return failureMessage + } + return listAnswerMessage(ans.Keys) +} + +// newSignRequest makes the request for a signature (DESIGN.md sections 6 +// and 10). It keeps at most api.MaxCommands command lines, and makes each +// claimed field as nodkey run does: a command line or a reason that is too +// long becomes "[hidden: too long]". So every request fits in the broker's +// cap on its size (transport.MaxRequestBody). +func newSignRequest(key, data []byte, flags uint32, binds [][]byte, commands [][]string, reason string) api.SignRequest { + shown := make([][]string, 0, min(len(commands), api.MaxCommands)) + for _, argv := range commands[:min(len(commands), api.MaxCommands)] { + shown = append(shown, claimedCommand(argv)) + } + return api.SignRequest{ + PublicKey: key, Data: data, Flags: flags, Binds: binds, Commands: shown, Reason: claimedText(reason), + } +} + +// encodeSignRequest returns req as JSON. It checks the caps of the request, +// and that the body is not larger than the broker reads +// (transport.MaxRequestBody). Every request of newSignRequest within the +// caps of the key, the data and the session-binds passes. +func encodeSignRequest(req api.SignRequest) ([]byte, error) { + if err := req.Check(); err != nil { + return nil, errors.New("the request is over a cap of the broker") + } + body, err := json.Marshal(req) + if err != nil || len(body) > transport.MaxRequestBody { + return nil, errors.New("the request is larger than the broker reads") + } + return body, nil +} + +// The errors of checkSignAnswer and checkKeys. They hold nothing from the +// answer or the request. +var ( + errSignatureKey = errors.New("the key of the request is not a valid SSH public key") + errSignatureFormat = errors.New("the signature of the broker does not fit the key and the sign flags") + errSignatureValid = errors.New("the signature of the broker is not a valid signature of the data") + errKeyList = errors.New("the answer of the broker has a key that is not valid") +) + +// checkSignAnswer checks the broker's signature before the agent gives it +// to ssh: its format must be the one that the key and the flags ask for +// (signatureFormat), and it must be a valid signature of data with key. +// It returns the signature for the sign answer. +func checkSignAnswer(key, data []byte, flags uint32, ans *api.SignAnswer) (*ssh.Signature, error) { + pub, err := ssh.ParsePublicKey(key) + if err != nil { + return nil, errSignatureKey + } + if want, ok := signatureFormat(pub.Type(), flags); !ok || ans.Format != want { + return nil, errSignatureFormat + } + sig := &ssh.Signature{Format: ans.Format, Blob: ans.Blob} + if err := pub.Verify(data, sig); err != nil { + return nil, errSignatureValid + } + return sig, nil +} + +// signatureFormat returns the format of a signature with a key of type +// keyType and these sign flags: the formats that the broker signs with +// (DESIGN.md section 6). Ed25519 and ECDSA keys take no flags. An RSA key +// needs the flag of SHA-256 or of SHA-512. ok is false for any other key +// type or flags. +func signatureFormat(keyType string, flags uint32) (string, bool) { + switch keyType { + case ssh.KeyAlgoED25519, ssh.KeyAlgoECDSA256, ssh.KeyAlgoECDSA384, ssh.KeyAlgoECDSA521: + return keyType, flags == 0 + case ssh.KeyAlgoRSA: + switch agent.SignatureFlags(flags) { + case agent.SignatureFlagRsaSha256: + return ssh.KeyAlgoRSASHA256, true + case agent.SignatureFlagRsaSha512: + return ssh.KeyAlgoRSASHA512, true + } + } + return "", false +} + +// keyNamePattern is the name rule of the broker config (DESIGN.md section +// 14). +var keyNamePattern = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9._-]{0,63}$`) + +// checkKeys checks the keys of the broker's answer: each has a name that +// follows the name rule of the config, and a plain Ed25519, ECDSA or RSA +// public key in its usual SSH wire form, as the broker signs only with +// such keys. +func checkKeys(keys []api.Key) error { + for _, k := range keys { + if !keyNamePattern.MatchString(k.Name) { + return errKeyList + } + pub, err := ssh.ParsePublicKey(k.PublicKey) + if err != nil || !bytes.Equal(pub.Marshal(), k.PublicKey) { + return errKeyList + } + switch pub.Type() { + case ssh.KeyAlgoED25519, ssh.KeyAlgoECDSA256, ssh.KeyAlgoECDSA384, ssh.KeyAlgoECDSA521, ssh.KeyAlgoRSA: + default: + return errKeyList + } + } + return nil +} + +// watchHangUp cancels a request with errHungUp when the process at the +// other end of c, ssh, closes the connection while the request waits for +// the broker. stop ends the watch; call it before the next read of c. +func watchHangUp(c *net.UnixConn, cancel context.CancelCauseFunc) (stop func()) { + done := make(chan struct{}) + go func() { + defer close(done) + if waitHangUp(c) { + cancel(errHungUp) + } + }() + return func() { + _ = c.SetReadDeadline(time.Now()) // ends waitHangUp + <-done + _ = c.SetReadDeadline(time.Time{}) + } +} + +// errHungUp is the cause of the end of a request's context when ssh closed +// its connection to the agent while the request waited. +var errHungUp = errors.New("client: ssh closed its connection to the agent") + +// deadline is the cause of the end of a request's context when its +// deadline passed. +type deadline struct{ d time.Duration } + +func (e deadline) Error() string { return "no answer from the broker within " + e.d.String() } + +// failureText describes a request to the broker that got no valid answer, +// for a line of the agent: the text of an *Error, which says "denied" for +// 403, or the deadline. It holds nothing from the answer. +func failureText(err error) string { + var e *Error + var d deadline + var sig interrupted + switch { + case errors.As(err, &e): + return e.Error() + case errors.As(err, &d): + return d.Error() + case errors.As(err, &sig): + return "the agent stops" + case errors.Is(err, errHungUp): + return "ssh closed its connection while the request waited" + } + return "the request to the broker failed" +} + +// shortCode is the short code of one request, once the broker sent it. +// Post calls onCode in a goroutine of net/http. +type shortCode struct { + mu sync.Mutex + code string +} + +func (s *shortCode) set(code string) { + s.mu.Lock() + defer s.mu.Unlock() + s.code = code +} + +// label returns " (request CODE)", or "" when no code came. +func (s *shortCode) label() string { + s.mu.Lock() + defer s.mu.Unlock() + if s.code == "" { + return "" + } + return " (request " + s.code + ")" +} + +// agentLog writes the lines of nodkey ssh-agent. Each line starts with +// "nodkey: ", is one line (oneLine), and goes out in one write, under a +// lock: lines come from many connections. +type agentLog struct { + mu sync.Mutex + w io.Writer + now func() time.Time + waiting lineBudget // the lines with the short code of a request that waits + refused lineBudget // the lines about refused and failed requests +} + +// lineBudget counts the lines of one kind in the current minute. +type lineBudget struct { + about string // what the lines are about, for the count of those left out + start time.Time // the start of the current minute + lines int // the lines written in that minute + dropped int // the lines left out in that minute +} + +func newAgentLog(w io.Writer) *agentLog { + return &agentLog{w: w, now: time.Now, + waiting: lineBudget{about: "requests that wait for approval"}, + refused: lineBudget{about: "refused or failed requests"}, + } +} + +// Write writes p as it is: the usage and the last line. +func (l *agentLog) Write(p []byte) (int, error) { + l.mu.Lock() + defer l.mu.Unlock() + return l.w.Write(p) +} + +// say writes a line without a limit: lines that come once, such as where +// the agent listens. +func (l *agentLog) say(format string, args ...any) { + l.mu.Lock() + defer l.mu.Unlock() + l.line(fmt.Sprintf(format, args...)) +} + +// waitingFor writes the line with the short code of a request that waits +// for the person: at most agentLogLines in a minute. +func (l *agentLog) waitingFor(code string) { + l.mu.Lock() + defer l.mu.Unlock() + l.limited(&l.waiting, "waiting for approval (request "+code+")") +} + +// note writes a line about a refused or failed request: at most +// agentLogLines in a minute. +func (l *agentLog) note(format string, args ...any) { + l.mu.Lock() + defer l.mu.Unlock() + l.limited(&l.refused, fmt.Sprintf(format, args...)) +} + +// limited writes text, unless b already let agentLogLines lines through in +// the current minute. Then it counts the line. The first line of a later +// minute says first how many lines were left out. l.mu must be held. +func (l *agentLog) limited(b *lineBudget, text string) { + // time.Now has a monotonic clock reading, so a change of the wall + // clock does not move the minute. + if now := l.now(); b.start.IsZero() || now.Sub(b.start) >= agentLogWindow { + l.flushLocked(b) + b.start, b.lines = now, 0 + } + if b.lines >= agentLogLines { + b.dropped++ + return + } + b.lines++ + l.line(text) +} + +// flush writes how many lines were left out, if any. The agent calls it +// when it stops. +func (l *agentLog) flush() { + l.mu.Lock() + defer l.mu.Unlock() + l.flushLocked(&l.waiting) + l.flushLocked(&l.refused) +} + +func (l *agentLog) flushLocked(b *lineBudget) { + if b.dropped > 0 { + l.line(fmt.Sprintf("left out %d lines about %s, to keep the log short", b.dropped, b.about)) + b.dropped = 0 + } +} + +// line writes "nodkey: " and text as one line. l.mu must be held. +func (l *agentLog) line(text string) { + _, _ = io.WriteString(l.w, "nodkey: "+oneLine(text)+"\n") +} diff --git a/internal/client/sshagent_child_linux_test.go b/internal/client/sshagent_child_linux_test.go new file mode 100644 index 0000000..ed82f99 --- /dev/null +++ b/internal/client/sshagent_child_linux_test.go @@ -0,0 +1,362 @@ +package client + +import ( + "context" + "encoding/binary" + "errors" + "fmt" + "io" + "io/fs" + "net" + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "syscall" + "testing" + "time" + + "golang.org/x/crypto/ssh" +) + +// childAgent makes the test binary run as nodkey ssh-agent, with the +// broker of childAddr, childPin and childKey. With the value "stuck", the +// agent's check of each connection writes stuckLine and never returns, +// and the agent waits for its connections a minute when it stops. +const childAgent = "NODKEY_TEST_CHILD_AGENT" + +// stuckLine is the line of a "stuck" child when its check of a connection +// starts: then the agent has taken that connection. +const stuckLine = "test: the check of a connection is stuck" + +// childSSHAgent runs nodkey ssh-agent as SSHAgent does, with the real +// signals and /proc, but with the key file and the broker of the test +// (childLoad), and without the checks of the directories on the way to +// the socket, which refuse the system temporary directory. +func childSSHAgent(args []string) int { + stderr := os.Stderr //nolint:forbidigo // the child is nodkey ssh-agent, which writes to stderr. + r := agentRunner{ + load: childLoad, + configPath: func() (string, error) { return "test", nil }, + getenv: os.Getenv, + checkDirs: noDirCheck, + proc: procFS{root: procDir}, + peer: peerCredentials, + } + if os.Getenv(childAgent) == "stuck" { + r.peer = func(*net.UnixConn) (peer, error) { + _, _ = fmt.Fprintln(stderr, stuckLine) + select {} + } + r.tune = func(a *sshAgent) { a.stopWait = time.Minute } + } + return r.main(args, stderr) +} + +// agentClientArg is the first argument of the test binary when it runs as +// a client of an agent socket (agentClient). +const agentClientArg = "nodkey-test-agent-client" + +// agentClient connects to the agent socket args[0], asks for the keys, +// and prints what came: "keys N", "closed" when the agent closed the +// connection without an answer, or "error: ..." when the client could +// not connect. +func agentClient(args []string) int { + stdout := os.Stdout //nolint:forbidigo // the helper reports on stdout. + if len(args) != 1 { + return 2 + } + c, err := (&net.Dialer{}).DialContext(context.Background(), "unix", args[0]) + if err != nil { + _, _ = fmt.Fprintln(stdout, "error: cannot connect:", err) + return 1 + } + defer func() { _ = c.Close() }() + _ = c.SetDeadline(time.Now().Add(waitLimit)) + if err := writeAgentMessage(c, []byte{agentListRequest}); err != nil { + _, _ = fmt.Fprintln(stdout, "closed") + return 0 + } + answer, err := readAgentMessage(c) + switch { + case errors.Is(err, io.EOF), errors.Is(err, syscall.ECONNRESET): + _, _ = fmt.Fprintln(stdout, "closed") + case err != nil: + _, _ = fmt.Fprintln(stdout, "error: no answer:", err) + case answer[0] == agentListAnswer && len(answer) >= 5: + _, _ = fmt.Fprintf(stdout, "keys %d\n", binary.BigEndian.Uint32(answer[1:5])) + default: + _, _ = fmt.Fprintf(stdout, "answer %d\n", answer[0]) + } + return 0 +} + +// agentChild returns the command that runs nodkey ssh-agent with args in +// a child process, with the broker sb as the client laptop. +func (sb *sshBroker) agentChild(t *testing.T, args ...string) *exec.Cmd { + t.Helper() + keyFile := filepath.Join(t.TempDir(), "client.key") + if err := os.WriteFile(keyFile, sb.laptop.file, 0o600); err != nil { + t.Fatal(err) + } + cmd := exec.CommandContext(t.Context(), self(t), args...) //nolint:gosec // G204: the test runs its own binary again. + cmd.Env = withEnv(os.Environ(), []string{childAgent + "=1", childAddr + "=" + sb.addr, + childPin + "=" + sb.key.pin.String(), childKey + "=" + keyFile}) + return cmd +} + +// TestSSHAgentInAChild runs nodkey ssh-agent in a child process, with the +// real signals. It serves ssh until SIGINT or SIGTERM. Then it removes its +// socket, prints its last line with the status that a shell shows, and +// ends with the same signal, as nodkey run does. +func TestSSHAgentInAChild(t *testing.T) { + t.Parallel() + for _, sig := range []syscall.Signal{syscall.SIGINT, syscall.SIGTERM} { + t.Run(sig.String(), func(t *testing.T) { + t.Parallel() + if sig == syscall.SIGINT { + needSIGINT(t) + } + host, _ := newSSHKey(t) + sb := newSSHBroker(t, map[string]ssh.PublicKey{"web-1": host.PublicKey()}) + path := filepath.Join(shortTempDir(t), "nodkey", socketName) + cmd := sb.agentChild(t, "--socket", path) + var stderr syncBuffer + cmd.Stderr = &stderr + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + waited := false + defer func() { + if !waited { + _ = cmd.Process.Kill() + _ = cmd.Wait() + } + }() + waitFor(t, "the agent to listen", func() bool { return strings.Contains(stderr.String(), "listens on") }) + keys, err := listKeys(t, path) + if err != nil || len(keys) != 1 || keys[0].Comment != "deploy" { + t.Fatalf("the agent lists %v, %v; stderr %q", keys, err, stderr.String()) + } + if err := cmd.Process.Signal(sig); err != nil { + t.Fatal(err) + } + done := make(chan error, 1) + go func() { done <- cmd.Wait() }() + select { + case err = <-done: + waited = true + case <-time.After(waitLimit): + t.Fatal("the agent did not stop") + } + if !diedOf(err, sig) { + t.Errorf("the agent ended with %v; want it to die of %v", err, sig) + } + want := "nodkey: stopped by " + map[syscall.Signal]string{syscall.SIGINT: "SIGINT", + syscall.SIGTERM: "SIGTERM"}[sig] + " (exit " + strconv.Itoa(128+int(sig)) + ")" + if last := lastLine(stderr.String()); last != want { + t.Errorf("the last line is %q, want %q", last, want) + } + if _, err := os.Lstat(path); !errors.Is(err, fs.ErrNotExist) { + t.Errorf("the socket is still there: %v", err) + } + }) + } +} + +// TestSSHAgentSecondSignal: after the first SIGTERM, nodkey ssh-agent no +// longer catches the signal. When it cannot stop, here because a +// connection is stuck and it waits for it, a second SIGTERM ends it at +// once. +func TestSSHAgentSecondSignal(t *testing.T) { + t.Parallel() + host, _ := newSSHKey(t) + sb := newSSHBroker(t, map[string]ssh.PublicKey{"web-1": host.PublicKey()}) + path := filepath.Join(shortTempDir(t), "nodkey", socketName) + cmd := sb.agentChild(t, "--socket", path) + cmd.Env = withEnv(cmd.Env, []string{childAgent + "=stuck"}) + var stderr syncBuffer + cmd.Stderr = &stderr + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + var waitErr error + exited := make(chan struct{}) + go func() { + waitErr = cmd.Wait() + close(exited) + }() + t.Cleanup(func() { + _ = cmd.Process.Kill() + <-exited + }) + waitFor(t, "the agent to listen", func() bool { return strings.Contains(stderr.String(), "listens on") }) + // A connection that the agent's check holds. A dial ends when the + // connection waits in the socket's queue; the agent may not have taken + // it yet, and a SIGTERM then would drop it. So wait until the agent's + // check of it has started. + c, err := (&net.Dialer{}).DialContext(t.Context(), "unix", path) + if err != nil { + t.Fatal(err) + } + defer func() { _ = c.Close() }() + waitFor(t, "the agent to take the connection", func() bool { return strings.Contains(stderr.String(), stuckLine) }) + start := time.Now() + if err := cmd.Process.Signal(syscall.SIGTERM); err != nil { + t.Fatal(err) + } + // The first SIGTERM closes the socket: new connections are refused. + waitFor(t, "the socket to close", func() bool { + c, err := (&net.Dialer{}).DialContext(t.Context(), "unix", path) + if err == nil { + _ = c.Close() + } + return err != nil + }) + // It waits for the stuck connection. + select { + case <-exited: + t.Fatalf("the first SIGTERM ended the agent at once (%v); stderr %q", waitErr, stderr.String()) + case <-time.After(300 * time.Millisecond): + } + ended := false + for !ended && time.Since(start) < waitLimit { + _ = cmd.Process.Signal(syscall.SIGTERM) + select { + case <-exited: + ended = true + case <-time.After(200 * time.Millisecond): + } + } + if !ended { + t.Fatalf("a second SIGTERM did not end the agent; stderr %q", stderr.String()) + } + if !diedOf(waitErr, syscall.SIGTERM) { + t.Errorf("the agent ended with %v; want it to die of SIGTERM", waitErr) + } +} + +// TestSSHAgentSignalWhileStarting: SIGTERM while the agent starts, also +// while it checks a socket that is already there, stops it as a signal +// does: the last line, and the status that a shell shows. (From the +// security review of issue #13: it exited with 1.) +func TestSSHAgentSignalWhileStarting(t *testing.T) { + t.Parallel() + for _, stale := range []bool{false, true} { + path := filepath.Join(shortTempDir(t), "nodkey", socketName) + if stale { + if err := os.Mkdir(filepath.Dir(path), 0o700); err != nil { + t.Fatal(err) + } + ln, err := (&net.ListenConfig{}).Listen(t.Context(), "unix", path) + if err != nil { + t.Fatal(err) + } + ul, ok := ln.(*net.UnixListener) + if !ok { + t.Fatal("not a Unix listener") + } + ul.SetUnlinkOnClose(false) + _ = ul.Close() + } + ctx, cancel := context.WithCancelCause(context.Background()) + cancel(interrupted{sig: syscall.SIGTERM}) + var stderr syncBuffer + fb := newFakeSSHBroker(t, nil, nil) + code := testAgentRunner(fb.b, "").run(ctx, []string{"--socket", path}, &stderr) + if last := lastLine(stderr.String()); code != 128+int(syscall.SIGTERM) || + last != "nodkey: stopped by SIGTERM (exit 143)" { + t.Errorf("a stale socket %v: exit code %d, stderr %q", stale, code, stderr.String()) + } + } +} + +// connectAs runs the test binary as a client of the agent socket +// (agentClient), as the user of cred, or as the user of the test when cred +// is nil, and returns what it printed. For another user, the binary is +// copied to a directory that everyone can read, and the socket and its +// directories are opened to everyone, so that only the agent's own check +// of the user (SO_PEERCRED) can refuse the connection. +func connectAs(t *testing.T, socket string, cred *syscall.Credential) string { + t.Helper() + bin := self(t) + if cred != nil { + dir, err := os.MkdirTemp("", "nodkey-bin-") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.RemoveAll(dir) }) + data, err := os.ReadFile(bin) //nolint:gosec // G304: the test binary itself, to copy it. + if err != nil { + t.Fatal(err) + } + bin = filepath.Join(dir, "client.test") + if err := os.WriteFile(bin, data, 0o755); err != nil { //nolint:gosec // G306: another user runs this copy of the test binary. + t.Fatal(err) + } + for _, p := range []struct { + path string + mode os.FileMode + }{{dir, 0o755}, {bin, 0o755}, {filepath.Dir(filepath.Dir(socket)), 0o711}, {filepath.Dir(socket), 0o711}, + {socket, 0o666}} { + if err := os.Chmod(p.path, p.mode); err != nil { + t.Fatal(err) + } + } + } + ctx, cancel := context.WithTimeout(t.Context(), waitLimit) + defer cancel() + cmd := exec.CommandContext(ctx, bin, agentClientArg, socket) //nolint:gosec // G204: the test runs its own binary again. + cmd.Env = []string{"PATH=" + os.Getenv("PATH")} + if cred != nil { + cmd.SysProcAttr = &syscall.SysProcAttr{Credential: cred} + } + out, err := cmd.Output() + if err != nil { + t.Fatalf("the client: %v; it printed %q", err, out) + } + return strings.TrimSpace(string(out)) +} + +// TestAgentServesAChildOfTheUser: a process of the same user, in a child +// process, connects and gets the keys. (The check of connectAs for +// TestAgentRefusesAnotherRealUser.) +func TestAgentServesAChildOfTheUser(t *testing.T) { + t.Parallel() + deploy, _ := newSSHKey(t) + fb := newFakeSSHBroker(t, map[string]ssh.PublicKey{"deploy": deploy.PublicKey()}, nil) + au := startAgent(t, fb.b, agentOptions{}) + if got := connectAs(t, au.socket, nil); got != "keys 1" { + t.Errorf("the child got %q", got) + } +} + +// TestAgentRefusesAnotherRealUser runs only as root: a child switches to +// another user and connects. The kernel tells the agent who connected +// (SO_PEERCRED), and the agent closes the connection without an answer. +// The socket is open to everyone here, so the check of the user is the +// only thing that refuses. Run it as root, such as in the end-to-end job +// (issue #15). +func TestAgentRefusesAnotherRealUser(t *testing.T) { + t.Parallel() + if os.Geteuid() != 0 { + t.Skip("needs root, to run a child as another user; the end-to-end test runs it") + } + deploy, _ := newSSHKey(t) + fb := newFakeSSHBroker(t, map[string]ssh.PublicKey{"deploy": deploy.PublicKey()}, nil) + au := startAgent(t, fb.b, agentOptions{}) + const nobody = 65534 + if got := connectAs(t, au.socket, &syscall.Credential{Uid: nobody, Gid: nobody}); got != "closed" { + t.Errorf("the other user got %q; want the connection closed", got) + } + want := fmt.Sprintf("nodkey: refused a connection of user ID %d: only user ID 0 may use this agent", nobody) + waitFor(t, "the agent's line", func() bool { return strings.Contains(au.log.String(), want) }) + if n := fb.requests.Load(); n != 0 { + t.Errorf("the broker got %d requests", n) + } + // Root itself is served. + if got := connectAs(t, au.socket, nil); got != "keys 1" { + t.Errorf("root got %q", got) + } +} diff --git a/internal/client/sshagent_command_test.go b/internal/client/sshagent_command_test.go new file mode 100644 index 0000000..5a274c7 --- /dev/null +++ b/internal/client/sshagent_command_test.go @@ -0,0 +1,208 @@ +package client + +import ( + "context" + "errors" + "io/fs" + "os" + "path/filepath" + "slices" + "strings" + "syscall" + "testing" + "time" + + "golang.org/x/crypto/ssh" + "golang.org/x/crypto/ssh/agent" +) + +// testAgentRunner returns a runner of nodkey ssh-agent whose broker is b. +// It reads no config file, and its environment has only xdg as +// XDG_RUNTIME_DIR. +func testAgentRunner(b *Broker, xdg string) agentRunner { + return agentRunner{ + load: func(string) (*Broker, error) { + if b == nil { + return nil, errors.New("this test must not connect to a broker") + } + return b, nil + }, + configPath: func() (string, error) { return "/nonexistent/client.toml", nil }, + getenv: func(name string) string { + if name == "XDG_RUNTIME_DIR" { + return xdg + } + return "" + }, + checkDirs: noDirCheck, + proc: procFS{root: "/nonexistent"}, + peer: peerCredentials, + } +} + +// TestSSHAgentUsage: wrong usage exits with 64, prints the usage, and +// connects to no broker; -h and --help alone print the usage and exit +// with 0. +func TestSSHAgentUsage(t *testing.T) { + t.Parallel() + tests := []struct { + name string + args []string + xdg string + want string // the last line, without "nodkey: " and " (exit 64)"; "" for help + }{ + {"-h", []string{"-h"}, "", ""}, + {"--help", []string{"--help"}, "", ""}, + {"help with more", []string{"--socket", "/run/a/agent.sock", "-h"}, "", "--help must be the only argument"}, + {"no value", []string{"--socket"}, "", "--socket needs a value"}, + {"an empty value", []string{"--socket="}, "", "--socket needs a path"}, + {"twice", []string{"--socket", "/run/a/b.sock", "-socket=/run/a/c.sock"}, "", "--socket is given twice"}, + {"an unknown option", []string{"--bogus"}, "", `unknown option "--bogus"`}, + {"an argument", []string{"extra"}, "", `unexpected argument "extra"`}, + {"a relative path", []string{"--socket", "agent.sock"}, "", "the socket path must be absolute"}, + {"a path with ..", []string{"--socket", "/run/a/../agent.sock"}, "", "the socket path must be clean"}, + {"a path that is too long", []string{"--socket", "/" + strings.Repeat("a", maxSocketPath) + "/s"}, "", + "the socket path is longer than 107 bytes"}, + {"no XDG_RUNTIME_DIR", nil, "", "XDG_RUNTIME_DIR is not set, or not an absolute path; give the socket with --socket PATH"}, + {"a relative XDG_RUNTIME_DIR", nil, "run/user/1000", "XDG_RUNTIME_DIR is not set"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + var stderr syncBuffer + code := testAgentRunner(nil, tt.xdg).run(t.Context(), tt.args, &stderr) + out := stderr.String() + if tt.want == "" { + if code != exitOK || out != agentUsage { + t.Errorf("exit code %d, stderr %q; want 0 and the usage", code, out) + } + return + } + if last := lastLine(out); code != exitUsage || !strings.HasPrefix(out, agentUsage) || + !strings.HasPrefix(last, "nodkey: "+tt.want) || !strings.HasSuffix(last, " (exit 64)") { + t.Errorf("exit code %d, stderr %q; want 64 and %q", code, out, tt.want) + } + }) + } +} + +// TestSSHAgentStartFails: the agent exits with 69 when the client config +// or the TLS key cannot be read, as nodkey run does, and with 1 when it +// cannot make its socket. +func TestSSHAgentStartFails(t *testing.T) { + t.Parallel() + path := filepath.Join(shortTempDir(t), "nodkey", socketName) + r := testAgentRunner(nil, "") + r.configPath = func() (string, error) { return "", errors.New("the home directory is not known") } + var stderr syncBuffer + if code := r.run(t.Context(), []string{"--socket", path}, &stderr); code != exitUnavailable || + stderr.String() != "nodkey: the home directory is not known (exit 69)\n" { + t.Errorf("without a config path: exit code %d, stderr %q", code, stderr.String()) + } + r = testAgentRunner(nil, "") + stderr = syncBuffer{} + if code := r.run(t.Context(), []string{"--socket", path}, &stderr); code != exitUnavailable || + stderr.String() != "nodkey: this test must not connect to a broker (exit 69)\n" { + t.Errorf("a config that cannot be read: exit code %d, stderr %q", code, stderr.String()) + } + fb := newFakeSSHBroker(t, nil, nil) + open := filepath.Join(shortTempDir(t), "open") + if err := os.Mkdir(open, 0o700); err != nil { + t.Fatal(err) + } + if err := os.Chmod(open, 0o755); err != nil { //nolint:gosec // G302: the test needs a directory that others can read. + t.Fatal(err) + } + stderr = syncBuffer{} + code := testAgentRunner(fb.b, "").run(t.Context(), []string{"--socket", filepath.Join(open, socketName)}, &stderr) + if last := lastLine(stderr.String()); code != exitAgentFailed || !strings.Contains(last, "must have mode 0700") || + !strings.HasSuffix(last, "(exit 1)") { + t.Errorf("a directory with mode 0755: exit code %d, stderr %q", code, stderr.String()) + } + if fb.requests.Load() != 0 { + t.Error("the agent asked the broker") + } +} + +// TestSSHAgentRun runs nodkey ssh-agent as SSHAgent does, on the default +// socket: it says where it listens, serves ssh, and on SIGTERM removes the +// socket and prints its last line with the status that a shell shows. +func TestSSHAgentRun(t *testing.T) { + t.Parallel() + host, _ := newSSHKey(t) + sb := newSSHBroker(t, map[string]ssh.PublicKey{"web-1": host.PublicKey()}) + xdg := shortTempDir(t) + path := filepath.Join(xdg, "nodkey", socketName) + r := testAgentRunner(sb.client(t), xdg) + ready := make(chan string, 1) + r.ready = func(p string) { ready <- p } + ctx, cancel := context.WithCancelCause(context.Background()) + defer cancel(nil) + var stderr syncBuffer + codes := make(chan int, 1) + go func() { codes <- r.run(ctx, nil, &stderr) }() + select { + case got := <-ready: + if got != path { + t.Fatalf("the agent listens on %s, want %s", got, path) + } + case <-time.After(waitLimit): + t.Fatal("the agent does not listen") + } + mustMode(t, filepath.Dir(path), fs.ModeDir, 0o700) + mustMode(t, path, fs.ModeSocket, 0o600) + au := &agentUnderTest{socket: path, log: &stderr} + keys, err := au.client(t).List() + if err != nil || len(keys) != 1 || keys[0].Comment != "deploy" || + !slices.Equal(keys[0].Blob, sb.user.PublicKey().Marshal()) { + t.Fatalf("the agent lists %v, %v", keys, err) + } + cancel(interrupted{sig: syscall.SIGTERM}) + select { + case code := <-codes: + if code != 128+int(syscall.SIGTERM) { + t.Errorf("exit code %d, want 143", code) + } + case <-time.After(waitLimit): + t.Fatal("the agent did not stop") + } + want := []string{ + "nodkey: the SSH agent listens on " + path + "; set SSH_AUTH_SOCK to this path for ssh", + "nodkey: stopped by SIGTERM (exit 143)", + } + if got := linesOf(stderr.String()); !slices.Equal(got, want) { + t.Errorf("stderr %q, want %q", got, want) + } + if _, err := os.Lstat(path); !errors.Is(err, fs.ErrNotExist) { + t.Errorf("the socket is still there: %v", err) + } + // A context that ends without a signal: exit code 0. (Another + // directory: a child that another test forks can hold the lock of the + // first agent for a moment.) + r = testAgentRunner(sb.client(t), shortTempDir(t)) + ctx2, cancel2 := context.WithCancel(context.Background()) + cancel2() + stderr = syncBuffer{} + if code := r.run(ctx2, nil, &stderr); code != exitOK || lastLine(stderr.String()) != "nodkey: stopped (exit 0)" { + t.Errorf("a context that ended: exit code %d, stderr %q", code, stderr.String()) + } +} + +// mustMode checks the type and the mode bits of path. +func mustMode(t *testing.T, path string, typ, perm fs.FileMode) { + t.Helper() + fi, err := os.Lstat(path) + if err != nil { + t.Fatal(err) + } + if fi.Mode().Type() != typ || fi.Mode().Perm() != perm { + t.Errorf("%s has mode %s; want %s with %04o", path, fi.Mode(), typ, perm) + } +} + +// listKeys lists the keys of the agent at socket, on a new connection. +func listKeys(t *testing.T, socket string) ([]*agent.Key, error) { + t.Helper() + au := &agentUnderTest{socket: socket} + return au.client(t).List() +} diff --git a/internal/client/sshagent_fuzz_test.go b/internal/client/sshagent_fuzz_test.go new file mode 100644 index 0000000..e8486a2 --- /dev/null +++ b/internal/client/sshagent_fuzz_test.go @@ -0,0 +1,219 @@ +package client + +// Fuzz tests of nodkey ssh-agent, from its security review (issue #13): +// the parser of the agent protocol, envValue, and handle with any +// message. + +import ( + "bytes" + "context" + "encoding/binary" + "io" + "net" + "os" + "strings" + "syscall" + "testing" + "testing/iotest" +) + +// FuzzParseSignRequest: a sign request that parses is exactly +// key, data and flags, with nothing more and nothing less. +func FuzzParseSignRequest(f *testing.F) { + f.Add(signRequestMessage([]byte("k"), []byte("d"), 0)[1:]) + f.Add([]byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}) + f.Add([]byte{0xff, 0xff, 0xff, 0xff, 0, 0, 0, 0}) + f.Add([]byte{0, 0, 0, 1, 'k', 0xff, 0xff, 0xff, 0xf0, 'd', 0, 0, 0, 0}) + f.Fuzz(func(t *testing.T, body []byte) { + key, data, flags, ok := parseSignRequest(body) + if !ok { + return + } + re := binary.BigEndian.AppendUint32(appendString(appendString(nil, key), data), flags) + if !bytes.Equal(re, body) { + t.Fatalf("parsed % x as key % x, data % x, flags %d", body, key, data, flags) + } + }) +} + +// FuzzParseExtension: an extension that parses is exactly its name +// and its contents. +func FuzzParseExtension(f *testing.F) { + f.Add(extensionMessage(sessionBindRequest, []byte("x"))[1:]) + f.Add([]byte{0, 0, 0, 0}) + f.Add([]byte{0x80, 0, 0, 0, 'x'}) + f.Fuzz(func(t *testing.T, body []byte) { + name, contents, ok := parseExtension(body) + if !ok { + return + } + if re := append(appendString(nil, []byte(name)), contents...); !bytes.Equal(re, body) { + t.Fatalf("parsed % x as %q and % x", body, name, contents) + } + }) +} + +// refEnvValue is a plain reading of an environment: the first entry that +// starts with name and "=". +func refEnvValue(env []byte, name string, limit int) (string, bool, bool) { + for e := range bytes.SplitSeq(env, []byte{0}) { + if v, ok := bytes.CutPrefix(e, []byte(name+"=")); ok { + if len(v) > limit { + return "", true, true + } + return string(v), true, false + } + } + return "", false, false +} + +// FuzzEnvValue: envValue agrees with a plain reading, in pieces of +// any size. +func FuzzEnvValue(f *testing.F) { + f.Add([]byte("A=1\x00NODKEY_REASON=x\x00"), uint8(1)) + f.Add([]byte("NODKEY_REASON\x00NODKEY_REASON=y"), uint8(3)) + f.Add([]byte("NODKEY_REASONNODKEY_REASON=z\x00"), uint8(7)) + f.Add([]byte("NODKEY_REASON=0123456789\x00"), uint8(2)) + f.Fuzz(func(t *testing.T, env []byte, piece uint8) { + const limit = 8 + want, wantFound, wantLong := refEnvValue(env, reasonVar, limit) + var r io.Reader = bytes.NewReader(env) + if piece%2 == 0 { + r = iotest.OneByteReader(r) + } else { + r = iotest.HalfReader(r) + } + got, found, long := envValue(r, reasonVar, limit) + if found != wantFound || long != wantLong || (!long && got != want) { + t.Fatalf("envValue(%q) = %q, %v, %v; want %q, %v, %v", env, got, found, long, want, wantFound, wantLong) + } + }) +} + +// FuzzHandle: handle never panics, answers only failure, success +// for a kept session-bind, or nothing else (the broker cannot be +// reached), keeps a copy of each session-bind, and writes only fixed +// lines. +func FuzzHandle(f *testing.F) { + f.Add([]byte{agentListRequest}) + f.Add(signRequestMessage([]byte("k"), []byte("d"), 7)) + f.Add(extensionMessage(sessionBindRequest, []byte("bind"))) + f.Add(extensionMessage("query", nil)) + f.Add([]byte{agentAddIdentity, 'x'}) + f.Add([]byte{agentExtension, 0xff, 0xff, 0xff, 0xff}) + key := newTLSKey(f) + pin := newTLSKey(f).pin + b, err := newBroker("127.0.0.1:1", key.cert, pin) // nothing listens there + if err != nil { + f.Fatal(err) + } + fds, err := syscall.Socketpair(syscall.AF_UNIX, syscall.SOCK_STREAM, 0) + if err != nil { + f.Fatal(err) + } + ours, theirs := os.NewFile(uintptr(fds[0]), "ours"), os.NewFile(uintptr(fds[1]), "theirs") + f.Cleanup(func() { _ = ours.Close(); _ = theirs.Close() }) + nc, err := net.FileConn(ours) + if err != nil { + f.Fatal(err) + } + uc, ok := nc.(*net.UnixConn) + if !ok { + f.Fatal("not a Unix connection") + } + f.Fuzz(func(t *testing.T, msg []byte) { + if len(msg) == 0 || len(msg) > maxAgentMessage { + return + } + var out syncBuffer + a := newSSHAgent(b, newAgentLog(&out), procFS{root: "/nonexistent"}, peerCredentials, 0) + c := &agentConn{a: a, conn: uc} + in := bytes.Clone(msg) + answer := c.handle(context.Background(), in) + kept := len(c.binds) + if bytes.Equal(answer, []byte{agentSuccess}) { + if kept != 1 || msg[0] != agentExtension { + t.Fatalf("success for % x", msg) + } + clear(in) // as serveConn does + _, contents, _ := parseExtension(msg[1:]) + if !bytes.Equal(c.binds[0], contents) { + t.Fatal("the kept session-bind shares memory with the message") + } + } else if !bytes.Equal(answer, failureMessage) { + t.Fatalf("answer % x to % x", answer, msg) + } + for _, l := range linesOf(out.String()) { + if !strings.HasPrefix(l, "nodkey: ") || strings.ContainsAny(l[8:], "\x00\r\n") { + t.Fatalf("line %q", l) + } + } + }) +} + +// chunkReader gives data in pieces whose sizes come from sizes, in turn, +// and records the largest buffer that it gets. +type chunkReader struct { + data []byte + sizes []byte + i int + most int // the largest buffer it got +} + +func (r *chunkReader) Read(p []byte) (int, error) { + r.most = max(r.most, len(p)) + if len(r.data) == 0 { + return 0, io.EOF + } + n := 1 + if len(r.sizes) > 0 { + n = int(r.sizes[r.i%len(r.sizes)])*97 + 1 + r.i++ + } + n = copy(p, r.data[:min(len(r.data), n)]) + r.data = r.data[n:] + return n, nil +} + +// FuzzReadMessage: readAgentMessage gives exactly the message of a plain +// reading, in pieces of any size, and an error for a short or bad +// message. Its buffer is never much larger than the bytes that came: at +// most firstPiece, or twice those bytes. (From the security review of +// issue #13.) +func FuzzReadMessage(f *testing.F) { + f.Add(binary.BigEndian.AppendUint32(nil, 5), []byte("hello"), []byte{1, 2, 3}) + f.Add(binary.BigEndian.AppendUint32(nil, 300000), []byte("x"), []byte{255}) + f.Add(binary.BigEndian.AppendUint32(nil, 0), []byte{}, []byte{}) + f.Add([]byte{0, 0}, []byte{}, []byte{0}) + f.Fuzz(func(t *testing.T, head, body, sizes []byte) { + stream := append(bytes.Clone(head), body...) + r := &chunkReader{data: bytes.Clone(stream), sizes: sizes} + msg, err := readAgentMessage(r) + // The plain reading. + var want []byte + wantErr := false + if len(stream) < 4 { + wantErr = true + } else { + // On 386, a length of 2 GiB or more is less than 0 here. + n := int(binary.BigEndian.Uint32(stream[:4])) + switch { + case n <= 0 || n > maxAgentMessage: + wantErr = true + case len(stream)-4 < n: + wantErr = true + default: + want = stream[4 : 4+n] + } + } + if wantErr != (err != nil) || (!wantErr && !bytes.Equal(msg, want)) { + t.Fatalf("got % x, %v; want % x (an error: %v)", msg, err, want, wantErr) + } + if len(stream) >= 4 { + n := int(binary.BigEndian.Uint32(stream[:4])) + if n > 0 && n <= maxAgentMessage && r.most > max(4, min(n, firstPiece), min(len(stream)-4, n)*2) { + t.Fatalf("a buffer of %d bytes for a message of %d bytes, of which %d came", r.most, n, len(stream)-4) + } + } + }) +} diff --git a/internal/client/sshagent_helpers_test.go b/internal/client/sshagent_helpers_test.go new file mode 100644 index 0000000..446e306 --- /dev/null +++ b/internal/client/sshagent_helpers_test.go @@ -0,0 +1,590 @@ +package client + +import ( + "bytes" + "context" + "crypto/ed25519" + "crypto/rand" + "encoding/binary" + "errors" + "io" + "net" + "net/http" + "os" + "path/filepath" + "strings" + "sync" + "testing" + "time" + + "golang.org/x/crypto/ssh" + "golang.org/x/crypto/ssh/agent" + + "github.com/code6x/nodkey/internal/api" + "github.com/code6x/nodkey/internal/approval" + "github.com/code6x/nodkey/internal/approval/approvaltest" + "github.com/code6x/nodkey/internal/broker" + "github.com/code6x/nodkey/internal/config" + "github.com/code6x/nodkey/internal/sshauth" + "github.com/code6x/nodkey/internal/store" + "github.com/code6x/nodkey/internal/transport" +) + +// The tests of nodkey ssh-agent run the agent in the test process, on a +// socket in a short temporary directory. Its requests go to the real +// broker service (package broker) behind the real transport server, with +// mutual TLS on the loopback address, the real approval core, the fake +// approver and clock, and the real SSH checks (package sshauth); or to a +// test handler (newFakeBroker), for answers that the broker never gives. +// The SSH clients are the agent client of golang.org/x/crypto, and the +// real OpenSSH ssh and ssh-add. The SSH servers are an in-process server +// of golang.org/x/crypto/ssh, and the real sshd. + +// sshTimeout is the broker's timeout of an SSH request. +const sshTimeout = 90 * time.Second + +// syncBuffer is a bytes.Buffer that goroutines can write while a test +// reads it. +type syncBuffer struct { + mu sync.Mutex + b bytes.Buffer +} + +func (s *syncBuffer) Write(p []byte) (int, error) { + s.mu.Lock() + defer s.mu.Unlock() + return s.b.Write(p) +} + +func (s *syncBuffer) String() string { + s.mu.Lock() + defer s.mu.Unlock() + return s.b.String() +} + +// linesOf returns the lines of s, without their line breaks. +func linesOf(s string) []string { + if s == "" { + return nil + } + return strings.Split(strings.TrimSuffix(s, "\n"), "\n") +} + +// waitFor waits until ok is true, and fails the test after waitLimit. +func waitFor(t testing.TB, what string, ok func() bool) { + t.Helper() + deadline := time.Now().Add(waitLimit) + for !ok() { + if time.Now().After(deadline) { + t.Fatalf("waited %s in vain for %s", waitLimit, what) + } + time.Sleep(time.Millisecond) + } +} + +// newSSHKey makes an Ed25519 SSH key. +func newSSHKey(t testing.TB) (ssh.AlgorithmSigner, ed25519.PrivateKey) { + t.Helper() + _, priv, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + t.Fatal(err) + } + s, err := ssh.NewSignerFromSigner(priv) + if err != nil { + t.Fatal(err) + } + as, ok := s.(ssh.AlgorithmSigner) + if !ok { + t.Fatal("the signer cannot choose an algorithm") + } + return as, priv +} + +// sshBroker is a running broker for one SSH test, with one client, +// laptop, and one key, deploy: an Ed25519 key with the rule "always", +// for any host in the broker's known_hosts. +type sshBroker struct { + addr string + key tlsKey // the broker's TLS key + laptop tlsKey // the TLS key of the client laptop + approver *approvaltest.Approver + clock *approvaltest.Clock + audit *memAudit + user ssh.Signer // the private key of deploy, for the test servers +} + +// newSSHBroker starts a broker whose known_hosts has hosts, by name. +func newSSHBroker(t *testing.T, hosts map[string]ssh.PublicKey) *sshBroker { + t.Helper() + sb := &sshBroker{ + key: newTLSKey(t), + laptop: newTLSKey(t), + approver: approvaltest.NewApprover(approverID), + clock: approvaltest.NewClock(noon), + audit: newMemAudit(), + } + var kh strings.Builder + for name, k := range hosts { + kh.WriteString(name + " " + string(ssh.MarshalAuthorizedKey(k))) + } + known, err := config.ParseKnownHosts([]byte(kh.String())) + if err != nil { + t.Fatal(err) + } + signer, priv := newSSHKey(t) + sb.user = signer + deploy, err := sshauth.NewKey("deploy", priv, nil) + if err != nil { + t.Fatal(err) + } + cfg := &config.Broker{ + Telegram: config.Telegram{UserID: approverID, NoticeChatID: approverID, Timeout: secretsTimeout, + SSHTimeout: sshTimeout}, + Clients: map[string]config.Client{"laptop": {Name: "laptop", Pin: sb.laptop.pin}}, + Keys: map[string]config.Key{ + "deploy": {Name: "deploy", Clients: []string{"laptop"}, Rule: config.Rule{Ask: config.AskAlways}}, + }, + KnownHosts: known, + } + core, err := approval.New(cfg, sb.approver, approval.Options{Clock: sb.clock}) + if err != nil { + t.Fatal(err) + } + svc, err := broker.New(broker.Parts{Config: cfg, Audit: sb.audit, Stores: map[string]store.Store{}, + Keys: map[string]sshauth.Key{"deploy": deploy}}) + if err != nil { + t.Fatal(err) + } + sb.addr = serve(t, sb.key, cfg.Clients, svc) + runCtx, stopRun := context.WithCancel(context.Background()) + runErr := make(chan error, 1) + go func() { runErr <- svc.Run(runCtx, core, nil) }() + t.Cleanup(func() { + stopRun() + select { + case <-runErr: + case <-time.After(waitLimit): + t.Error("Run did not return") + } + svc.Close() + }) + if err := sb.approver.WaitRunning(waitLimit); err != nil { + t.Fatal(err) + } + return sb +} + +// client returns a Broker for the client laptop, as Load makes it. +func (sb *sshBroker) client(t testing.TB) *Broker { + t.Helper() + b, err := newBroker(sb.addr, sb.laptop.cert, sb.key.pin) + if err != nil { + t.Fatal(err) + } + return b +} + +// waitAsked waits until the approver was asked for n messages, and +// returns them. +func (sb *sshBroker) waitAsked(t testing.TB, n int) []approvaltest.Asked { + t.Helper() + asked, err := sb.approver.WaitAsked(n, waitLimit) + if err != nil { + t.Fatal(err) + } + return asked +} + +// agentUnderTest is nodkey ssh-agent, serving in the test process. +type agentUnderTest struct { + socket string + log *syncBuffer // what the agent wrote to its stderr + alog *agentLog // the agent's lines, with their budgets + agent *sshAgent + stop func() // stops the agent, and waits until it has stopped +} + +// agentOptions change the agent of startAgent. +type agentOptions struct { + // proc reads the claimed context. The default is a tree without + // processes: no claimed context. + proc *procFS + // peer replaces peerCredentials. + peer func(*net.UnixConn) (peer, error) + // keysTimeout, signTimeout, procTimeout and stopWait replace the + // limits of the agent, and maxConns the most open connections. + keysTimeout, signTimeout, procTimeout, stopWait time.Duration + maxConns int + // now replaces the clock of the agent's lines. + now func() time.Time +} + +// noDirCheck takes the place of config.CheckParentDirs, which refuses +// the system temporary directory. +func noDirCheck(string) error { return nil } + +// startAgent starts the agent with the broker b, until the test ends. Its +// socket is in a new directory of a short temporary directory: the +// directory checks of the real agent refuse the system temporary +// directory, which everyone can write to. +func startAgent(t *testing.T, b *Broker, opts agentOptions) *agentUnderTest { + t.Helper() + path := filepath.Join(shortTempDir(t), "a", socketName) + self, err := currentUID() + if err != nil { + t.Fatal(err) + } + sock, err := listenAgent(t.Context(), path, self, noDirCheck) + if err != nil { + t.Fatal(err) + } + au := &agentUnderTest{socket: path, log: &syncBuffer{}} + log := newAgentLog(au.log) + if opts.now != nil { + log.now = opts.now + } + au.alog = log + a := newSSHAgent(b, log, procFS{root: t.TempDir()}, peerCredentials, self) + au.agent = a + if opts.proc != nil { + a.proc = *opts.proc + } + if opts.peer != nil { + a.peer = opts.peer + } + for _, o := range []struct { + to *time.Duration + from time.Duration + }{{&a.keysTimeout, opts.keysTimeout}, {&a.signTimeout, opts.signTimeout}, {&a.procTimeout, opts.procTimeout}, + {&a.stopWait, opts.stopWait}} { + if o.from > 0 { + *o.to = o.from + } + } + if opts.maxConns > 0 { + a.maxConns = opts.maxConns + } + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan struct{}) + go func() { + defer close(done) + a.serve(ctx, sock.ln) + sock.close() + log.flush() + }() + au.stop = func() { + cancel() + select { + case <-done: + case <-time.After(waitLimit): + t.Error("the agent did not stop") + } + } + t.Cleanup(au.stop) + return au +} + +// conn connects to the agent, for raw messages (rawCall). +func (au *agentUnderTest) conn(t *testing.T) net.Conn { + t.Helper() + c, err := (&net.Dialer{}).DialContext(t.Context(), "unix", au.socket) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = c.Close() }) + return c +} + +// client connects to the agent, and returns the agent client of +// golang.org/x/crypto on that connection. The client reads the answers +// itself, so do not use rawCall on its connection. +func (au *agentUnderTest) client(t *testing.T) agent.ExtendedAgent { + t.Helper() + return agent.NewClient(au.conn(t)) +} + +// lines returns the lines that the agent wrote so far. +func (au *agentUnderTest) lines() []string { return linesOf(au.log.String()) } + +// openConns returns how many connections the agent serves now. +func (au *agentUnderTest) openConns() int { + au.agent.mu.Lock() + defer au.agent.mu.Unlock() + return len(au.agent.conns) +} + +// shortCodes returns how many short codes the agent got so far: the lines +// with a short code that it wrote, and those it left out. +func (au *agentUnderTest) shortCodes() int { + au.alog.mu.Lock() + defer au.alog.mu.Unlock() + return au.alog.waiting.lines + au.alog.waiting.dropped +} + +// rawCall sends one message on c, and returns the answer. +func rawCall(t *testing.T, c net.Conn, msg []byte) []byte { + t.Helper() + if err := c.SetDeadline(time.Now().Add(waitLimit)); err != nil { + t.Fatal(err) + } + if err := writeAgentMessage(c, msg); err != nil { + t.Fatal(err) + } + answer, err := readAgentMessage(c) + if err != nil { + t.Fatalf("no answer: %v", err) + } + return answer +} + +// signRequestMessage returns a sign request for the key blob, the data and +// the flags. +func signRequestMessage(key, data []byte, flags uint32) []byte { + b := appendString([]byte{agentSignRequest}, key) + b = appendString(b, data) + return binary.BigEndian.AppendUint32(b, flags) +} + +// extensionMessage returns an extension request. +func extensionMessage(name string, contents []byte) []byte { + return append(appendString([]byte{agentExtension}, []byte(name)), contents...) +} + +// bindAgent sends a session-bind@openssh.com message with contents, and +// reports whether the agent answered success. +func bindAgent(t *testing.T, c net.Conn, contents []byte) bool { + t.Helper() + answer := rawCall(t, c, extensionMessage(sessionBindRequest, contents)) + switch { + case bytes.Equal(answer, []byte{agentSuccess}): + return true + case bytes.Equal(answer, failureMessage): + return false + } + t.Fatalf("the answer to a session-bind is % x", answer) + return false +} + +// signatureOf parses the answer to a sign request: the signature, or nil +// for SSH_AGENT_FAILURE. +func signatureOf(t *testing.T, answer []byte) *ssh.Signature { + t.Helper() + if bytes.Equal(answer, failureMessage) { + return nil + } + if len(answer) == 0 || answer[0] != agentSignAnswer { + t.Fatalf("the answer to a sign request is % x", answer) + } + r := newWireReader(answer[1:]) + inner := newWireReader(r.string()) + sig := &ssh.Signature{Format: string(inner.string()), Blob: inner.string()} + if !r.done() || !inner.done() { + t.Fatalf("the sign answer has another form: % x", answer) + } + return sig +} + +// sshServer is an in-process SSH server of golang.org/x/crypto/ssh, as in +// the tests of package sshauth. Only userKey may log in. It records its +// host key's signature over the session identifier of each key exchange: +// OpenSSH puts that into the session-bind that it sends to its agent. +type sshServer struct { + addr string + hostKey ssh.PublicKey + + mu sync.Mutex + kex [][]byte // the session-bind of each key exchange, in order + logins int // the logins that the server accepted +} + +// recordingSigner signs with a host key and records each signature. +type recordingSigner struct { + ssh.AlgorithmSigner + record func(data []byte, sig *ssh.Signature) +} + +func (r recordingSigner) Sign(rand io.Reader, data []byte) (*ssh.Signature, error) { + return r.SignWithAlgorithm(rand, data, "") +} + +func (r recordingSigner) SignWithAlgorithm(rand io.Reader, data []byte, algorithm string) (*ssh.Signature, error) { + sig, err := r.AlgorithmSigner.SignWithAlgorithm(rand, data, algorithm) + if err == nil { + r.record(data, sig) + } + return sig, err +} + +// marshalBind returns the contents of a session-bind@openssh.com message. +func marshalBind(hostKey ssh.PublicKey, sessionID []byte, sig *ssh.Signature, forwarding bool) []byte { + return ssh.Marshal(struct { + HostKey []byte + SessionID []byte + Signature []byte + Forwarding bool + }{hostKey.Marshal(), sessionID, ssh.Marshal(sig), forwarding}) +} + +// startSSHServer starts a server with the host key host on the loopback +// address. Only userKey may log in. +func startSSHServer(t *testing.T, host ssh.AlgorithmSigner, userKey ssh.PublicKey) *sshServer { + t.Helper() + s := &sshServer{hostKey: host.PublicKey()} + cfg := &ssh.ServerConfig{ + PublicKeyCallback: func(_ ssh.ConnMetadata, key ssh.PublicKey) (*ssh.Permissions, error) { + if !bytes.Equal(key.Marshal(), userKey.Marshal()) { + return nil, errors.New("not this key") + } + return &ssh.Permissions{}, nil + }, + } + cfg.AddHostKey(recordingSigner{host, func(data []byte, sig *ssh.Signature) { + s.mu.Lock() + defer s.mu.Unlock() + s.kex = append(s.kex, marshalBind(host.PublicKey(), bytes.Clone(data), sig, false)) + }}) + ln, err := (&net.ListenConfig{}).Listen(t.Context(), "tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + s.addr = ln.Addr().String() + var wg sync.WaitGroup + t.Cleanup(func() { + _ = ln.Close() + wg.Wait() + }) + wg.Go(func() { + for { + nc, err := ln.Accept() + if err != nil { + return + } + wg.Go(func() { s.serve(nc, cfg) }) + } + }) + return s +} + +// serve serves one connection: it runs each "exec" request as a command +// that exits with 0. +func (s *sshServer) serve(nc net.Conn, cfg *ssh.ServerConfig) { + defer func() { _ = nc.Close() }() + _ = nc.SetDeadline(time.Now().Add(2 * waitLimit)) + conn, chans, reqs, err := ssh.NewServerConn(nc, cfg) + if err != nil { + return + } + defer func() { _ = conn.Close() }() + s.mu.Lock() + s.logins++ + s.mu.Unlock() + go ssh.DiscardRequests(reqs) + for nch := range chans { + ch, chReqs, err := nch.Accept() + if err != nil { + return + } + go func() { + for req := range chReqs { + exec := req.Type == "exec" + _ = req.Reply(exec, nil) + if exec { + _, _ = ch.SendRequest("exit-status", false, ssh.Marshal(struct{ Status uint32 }{0})) + _ = ch.Close() + } + } + }() + } +} + +// binds returns the session-bind of each key exchange so far. +func (s *sshServer) binds() [][]byte { + s.mu.Lock() + defer s.mu.Unlock() + return append([][]byte(nil), s.kex...) +} + +// loginCount returns how many logins the server accepted. +func (s *sshServer) loginCount() int { + s.mu.Lock() + defer s.mu.Unlock() + return s.logins +} + +// fakeSSHBroker is a test handler in place of the broker: it lists keys, +// and answers each sign request with sign. It keeps every sign request. +type fakeSSHBroker struct { + *fakeBroker + mu sync.Mutex + signs []*api.SignRequest +} + +// newFakeSSHBroker starts a fake broker that lists keys, by name, and +// answers each sign request with sign. +func newFakeSSHBroker(t *testing.T, keys map[string]ssh.PublicKey, + sign func(w http.ResponseWriter, req *api.SignRequest)) *fakeSSHBroker { + t.Helper() + ans := api.KeysAnswer{Keys: []api.Key{}} + for name, k := range keys { + ans.Keys = append(ans.Keys, api.Key{Name: name, PublicKey: k.Marshal()}) + } + fb := &fakeSSHBroker{} + fb.fakeBroker = newFakeBroker(t, func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case api.PathKeys: + if _, err := transport.ReadJSON[api.KeysRequest](w, r); err != nil { + return + } + _ = transport.WriteJSON(w, http.StatusOK, ans) + case api.PathSign: + req, err := transport.ReadJSON[api.SignRequest](w, r) + if err != nil { + return + } + fb.mu.Lock() + fb.signs = append(fb.signs, req) + fb.mu.Unlock() + sign(w, req) + default: + transport.WriteError(w, http.StatusNotFound) + } + }) + return fb +} + +// signRequests returns the sign requests so far. +func (fb *fakeSSHBroker) signRequests() []*api.SignRequest { + fb.mu.Lock() + defer fb.mu.Unlock() + return append([]*api.SignRequest(nil), fb.signs...) +} + +// signWith returns a sign handler that signs the data of each request +// with signer, and says that the signature has the format format ("" for +// the format of the signature). +func signWith(signer ssh.AlgorithmSigner, algorithm, format string) func(http.ResponseWriter, *api.SignRequest) { + return func(w http.ResponseWriter, req *api.SignRequest) { + sig, err := signer.SignWithAlgorithm(rand.Reader, req.Data, algorithm) + if err != nil { + transport.WriteError(w, http.StatusInternalServerError) + return + } + f := format + if f == "" { + f = sig.Format + } + _ = transport.WriteJSON(w, http.StatusOK, api.SignAnswer{Format: f, Blob: sig.Blob}) + } +} + +// shortTempDir returns a new directory with a short path, because the +// path of a Unix socket must be shorter than 108 bytes. +func shortTempDir(t *testing.T) string { + t.Helper() + dir, err := os.MkdirTemp("", "nodkey-") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.RemoveAll(dir) }) + return dir +} diff --git a/internal/client/sshagent_linux.go b/internal/client/sshagent_linux.go new file mode 100644 index 0000000..fae572c --- /dev/null +++ b/internal/client/sshagent_linux.go @@ -0,0 +1,123 @@ +package client + +import ( + "errors" + "io/fs" + "net" + "os" + "syscall" + "unsafe" +) + +// peerCredentials returns the user ID and the process ID of the process +// that connected to c, as the kernel recorded them at connect(2) +// (SO_PEERCRED). The process cannot change them. The process ID is 0 when +// the process is in a PID namespace that nodkey cannot see. +func peerCredentials(c *net.UnixConn) (peer, error) { + raw, err := c.SyscallConn() + if err != nil { + return peer{}, err + } + var cred *syscall.Ucred + var credErr error + if err := raw.Control(func(fd uintptr) { + cred, credErr = syscall.GetsockoptUcred(int(fd), syscall.SOL_SOCKET, syscall.SO_PEERCRED) + }); err != nil { + return peer{}, err + } + if credErr != nil { + return peer{}, credErr + } + return peer{uid: cred.Uid, pid: cred.Pid}, nil +} + +// ownerOf returns the user ID that owns the file of fi. +func ownerOf(fi fs.FileInfo) (uint32, bool) { + st, ok := fi.Sys().(*syscall.Stat_t) + if !ok { + return 0, false + } + return st.Uid, true +} + +// lockFile takes an exclusive lock on f (flock), or fails at once with +// syscall.EWOULDBLOCK when another process holds it. The lock ends when f +// is closed, also when the process ends. +func lockFile(f *os.File) error { + raw, err := f.SyscallConn() + if err != nil { + return err + } + var lockErr error + if err := raw.Control(func(fd uintptr) { + lockErr = syscall.Flock(int(fd), syscall.LOCK_EX|syscall.LOCK_NB) + }); err != nil { + return err + } + return lockErr +} + +// The events of poll(2) that say that the other side of a connection is +// gone. POLLRDHUP is Linux's own: the other side closed the connection, or +// its half for writing. poll reports it also when bytes that nobody read +// wait in the socket. +const ( + pollERR = 0x8 + pollHUP = 0x10 + pollNVAL = 0x20 + pollRDHUP = 0x2000 +) + +// pollFd is struct pollfd of poll(2). +type pollFd struct { + fd int32 + events int16 + revents int16 +} + +// hungUp reports whether the other side of the connection fd has hung up, +// with poll(2) (as ppoll, which every Linux architecture has), without +// waiting. +func hungUp(fd uintptr) (bool, error) { + p := pollFd{fd: int32(fd), events: pollRDHUP} //nolint:gosec // G115: a file descriptor fits in an int32. + var noWait syscall.Timespec + for { + _, _, errno := syscall.Syscall6(syscall.SYS_PPOLL, uintptr(unsafe.Pointer(&p)), 1, //nolint:gosec // G103: struct pollfd for ppoll(2), as the C library passes it. + uintptr(unsafe.Pointer(&noWait)), 0, 0, 0) //nolint:gosec // G103: struct timespec for ppoll(2). + switch { + case errno == 0: + return p.revents&(pollRDHUP|pollHUP|pollERR|pollNVAL) != 0, nil + case errors.Is(errno, syscall.EINTR): + continue + default: + return false, errno + } + } +} + +// waitHangUp waits until the process at the other end of c closes the +// connection, as ssh does when it ends, and reports true then. It returns +// false when the read deadline of c passes first, or when poll fails. It +// reads no data, so bytes that the process sent before it hung up, such +// as its next request, do not end the watch, and stay for +// readAgentMessage. +// +// Go's poller wakes it when the socket can be read: when bytes come, and +// when the other side hangs up. Each time, hungUp asks the kernel whether +// the other side is gone. +func waitHangUp(c *net.UnixConn) bool { + raw, err := c.SyscallConn() + if err != nil { + return false + } + gone := false + err = raw.Read(func(fd uintptr) bool { + h, err := hungUp(fd) + if err != nil { + return true // the watch ends; the request still waits + } + gone = h + return h + }) + return err == nil && gone +} diff --git a/internal/client/sshagent_openssh_linux_test.go b/internal/client/sshagent_openssh_linux_test.go new file mode 100644 index 0000000..1a2d494 --- /dev/null +++ b/internal/client/sshagent_openssh_linux_test.go @@ -0,0 +1,553 @@ +package client + +import ( + "bytes" + "context" + "encoding/pem" + "net" + "net/http" + "os" + "os/exec" + "os/user" + "path/filepath" + "regexp" + "slices" + "strconv" + "strings" + "testing" + "time" + + "golang.org/x/crypto/ssh" + + "github.com/code6x/nodkey/internal/api" + "github.com/code6x/nodkey/internal/audit" + "github.com/code6x/nodkey/internal/transport" +) + +// These tests run the real OpenSSH ssh and ssh-add with nodkey ssh-agent, +// so the session-bind, the data to sign and the refused requests are what +// OpenSSH sends. Some also run the real sshd, without root. They skip +// when a program is missing, too old, or cannot run here. When the CI +// environment variable is set, they fail instead: CI installs OpenSSH, so +// there these tests must run. + +// skipOrFail skips the test, or fails it when the CI environment +// variable is set. +func skipOrFail(t *testing.T, format string, args ...any) { + t.Helper() + if os.Getenv("CI") != "" { + t.Fatalf("CI is set, so this test must run, but: "+format, args...) + } + t.Skipf(format, args...) +} + +// requireOpenSSH returns the paths of ssh and ssh-add. It skips the test +// (see skipOrFail) when there are none, or when ssh is older than 8.9, +// which sends no session-bind. +func requireOpenSSH(t *testing.T) (sshPath, addPath string) { + t.Helper() + sshPath, err := exec.LookPath("ssh") + if err != nil { + skipOrFail(t, "no ssh program") + } + addPath, err = exec.LookPath("ssh-add") + if err != nil { + skipOrFail(t, "no ssh-add program") + } + out, err := exec.CommandContext(t.Context(), sshPath, "-V").CombinedOutput() + if err != nil { + skipOrFail(t, "ssh -V: %v", err) + } + m := regexp.MustCompile(`OpenSSH_(\d+)\.(\d+)`).FindSubmatch(out) + if m == nil { + skipOrFail(t, "ssh is not OpenSSH: %q", out) + } + major, _ := strconv.Atoi(string(m[1])) + minor, _ := strconv.Atoi(string(m[2])) + if major < 8 || major == 8 && minor < 9 { + skipOrFail(t, "OpenSSH %d.%d sends no session-bind; it needs 8.9 or newer", major, minor) + } + return sshPath, addPath +} + +// sshArgs returns the arguments of ssh for a login to addr as user, with +// only the agent's keys, and "true" as the command. ssh knows the host as +// web-1 with hostKey, from a known_hosts file in dir. +func sshArgs(t *testing.T, addr, user string, hostKey ssh.PublicKey, dir string) []string { + t.Helper() + host, port, err := net.SplitHostPort(addr) + if err != nil { + t.Fatal(err) + } + knownHosts := filepath.Join(dir, "known_hosts") + if err := os.WriteFile(knownHosts, []byte("web-1 "+string(ssh.MarshalAuthorizedKey(hostKey))), 0o600); err != nil { + t.Fatal(err) + } + return []string{ + "-F", "/dev/null", // no config file + "-o", "IdentityFile=" + filepath.Join(dir, "no-such-key"), // no key files, only the agent + "-o", "UserKnownHostsFile=" + knownHosts, + "-o", "GlobalKnownHostsFile=/dev/null", + "-o", "StrictHostKeyChecking=yes", + "-o", "HostKeyAlias=web-1", + "-o", "CheckHostIP=no", + "-o", "BatchMode=yes", + "-o", "PreferredAuthentications=publickey", + "-o", "UpdateHostKeys=no", + "-o", "ControlMaster=no", + "-o", "ControlPath=none", + "-o", "ConnectTimeout=10", + "-p", port, "-l", user, host, "true", + } +} + +// sshEnv returns the environment of ssh: the agent's socket, and env. +func sshEnv(socket, home string, env ...string) []string { + return append([]string{"SSH_AUTH_SOCK=" + socket, "PATH=" + os.Getenv("PATH"), "HOME=" + home}, env...) +} + +// runSSH runs ssh with args and env, and returns what it printed. +func runSSH(t *testing.T, sshPath string, args, env []string) ([]byte, error) { + t.Helper() + ctx, cancel := context.WithTimeout(t.Context(), waitLimit) + defer cancel() + cmd := exec.CommandContext(ctx, sshPath, args...) //nolint:gosec // G204: the test runs ssh from PATH with its own arguments. + cmd.Env = env + return cmd.CombinedOutput() +} + +// TestOpenSSHLogin logs in with the real ssh through the agent and the +// real broker, to an in-process SSH server. Only a login that the person +// approves works. The message shows the reason from the environment of +// ssh, and the command lines of ssh and its parents. +func TestOpenSSHLogin(t *testing.T) { + t.Parallel() + sshPath, _ := requireOpenSSH(t) + for _, tt := range []struct { + name string + answer audit.Answer // "" lets the request expire + }{ + {"approved", audit.AnswerApproveOnce}, + {"denied", audit.AnswerDeny}, + {"expired", ""}, + } { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + host, _ := newSSHKey(t) + sb := newSSHBroker(t, map[string]ssh.PublicKey{"web-1": host.PublicKey()}) + srv := startSSHServer(t, host, sb.user.PublicKey()) + au := startAgent(t, sb.client(t), agentOptions{proc: &procFS{root: procDir}}) + sb.approver.Auto(tt.answer) + dir := shortTempDir(t) + type result struct { + out []byte + err error + } + done := make(chan result, 1) + args := sshArgs(t, srv.addr, "deploy", host.PublicKey(), dir) + go func() { + out, err := runSSH(t, sshPath, args, sshEnv(au.socket, dir, "NODKEY_REASON=apply the nginx fix")) + done <- result{out, err} + }() + asked := sb.waitAsked(t, 1)[0] + if tt.answer == "" { + sb.clock.Advance(sshTimeout) + } + res := <-done + if tt.answer == audit.AnswerApproveOnce { + if res.err != nil || srv.loginCount() != 1 { + t.Fatalf("ssh: %v; agent lines %q\n%s", res.err, au.lines(), res.out) + } + } else if res.err == nil || srv.loginCount() != 0 { + t.Fatalf("ssh logged in: %v\n%s", res.err, res.out) + } + p := asked.Prompt + if p.Key != "deploy" || !slices.Equal(p.Hosts, []string{"web-1"}) || p.RemoteUser != "deploy" { + t.Errorf("the message shows %+v", p) + } + if p.Claim.Reason != "apply the nginx fix" || !strings.HasPrefix(p.Claim.Command, sshPath+" ") || + !strings.Contains(p.Claim.Command, " ← ") { + t.Errorf("the claimed part is %+v", p.Claim) + } + lines := au.lines() + if len(lines) == 0 || lines[0] != "nodkey: waiting for approval (request "+p.ShortCode+")" { + t.Errorf("the agent wrote %q", lines) + } + if tt.answer != audit.AnswerApproveOnce && + (len(lines) != 2 || lines[1] != "nodkey: no signature (request "+p.ShortCode+"): denied") { + t.Errorf("the agent wrote %q", lines) + } + }) + } +} + +// TestOpenSSHSignRequest: what the agent sends the broker for a login of +// the real ssh, which a shell started: the exact key that the agent +// listed, the data and flags of ssh, the one session-bind of ssh, which +// holds the server's key exchange, the command lines of ssh and of 3 of +// its parents, and NODKEY_REASON from the environment of ssh. Nothing +// else of the environment. +func TestOpenSSHSignRequest(t *testing.T) { + t.Parallel() + sshPath, _ := requireOpenSSH(t) + sh := needProgram(t, "sh") + host, _ := newSSHKey(t) + userKey, _ := newSSHKey(t) + srv := startSSHServer(t, host, userKey.PublicKey()) + bodies := make(chan []byte, 1) + fb := newFakeBroker(t, func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case api.PathKeys: + ans := api.KeysAnswer{Keys: []api.Key{{Name: "deploy", PublicKey: userKey.PublicKey().Marshal()}}} + _ = transport.WriteJSON(w, http.StatusOK, ans) + case api.PathSign: + bodies <- readBody(t, r) + transport.WriteError(w, http.StatusForbidden) + } + }) + au := startAgent(t, fb.b, agentOptions{proc: &procFS{root: procDir}}) + dir := shortTempDir(t) + canary := madeUp("canary") + args := sshArgs(t, srv.addr, "deploy", host.PublicKey(), dir) + // The shell stays as the parent of ssh: it has more to do after ssh. + script := `NODKEY_REASON="from ssh" "$0" "$@"; :` + ctx, cancel := context.WithTimeout(t.Context(), waitLimit) + defer cancel() + cmd := exec.CommandContext(ctx, sh, append([]string{"-c", script, sshPath}, args...)...) //nolint:gosec // G204: the test runs ssh through sh. + cmd.Env = sshEnv(au.socket, dir, "NODKEY_REASON=from the shell", "SECRET_OF_THE_SHELL="+canary) + out, err := cmd.CombinedOutput() + if err != nil || srv.loginCount() != 0 { + t.Fatalf("sh: %v; %d logins\n%s", err, srv.loginCount(), out) + } + var body []byte + select { + case body = <-bodies: + default: + t.Fatalf("the broker got no sign request; agent lines %q\n%s", au.lines(), out) + } + if strings.Contains(string(body), canary) { + t.Error("the request holds a value from the environment") + } + var req api.SignRequest + if err := req.UnmarshalJSON(body); err != nil { + t.Fatal(err) + } + if !bytes.Equal(req.PublicKey, userKey.PublicKey().Marshal()) || req.Flags != 0 { + t.Errorf("the request has another key, or the flags %d", req.Flags) + } + binds := srv.binds() + if len(req.Binds) != 1 || len(binds) != 1 || !bytes.Equal(req.Binds[0], binds[0]) { + t.Errorf("the request has %d session-binds; want the one of the server's key exchange", len(req.Binds)) + } + // The data to sign starts with the session identifier of the bind. + var bind struct { + HostKey, SessionID, Signature []byte + Forwarding bool + } + if err := ssh.Unmarshal(binds[0], &bind); err != nil { + t.Fatal(err) + } + if !bytes.HasPrefix(req.Data, ssh.Marshal(struct{ ID []byte }{bind.SessionID})) || + !bytes.Contains(req.Data, []byte("deploy")) { + t.Error("the data to sign is not the login of ssh") + } + want := [][]string{append([]string{sshPath}, args...), cmd.Args, os.Args} + if parent := realCommandLine(t, os.Getppid()); parent != nil { + want = append(want, parent) + } + if !slices.EqualFunc(req.Commands, want, slices.Equal) { + t.Errorf("the command lines are\n%q\nwant\n%q", strings.Join(flat(req.Commands), " | "), + strings.Join(flat(want), " | ")) + } + if req.Reason != "from ssh" { + t.Errorf("the reason is %q, want the one of ssh", req.Reason) + } +} + +// TestOpenSSHAgentChecksTheSignature: a signature that the broker gets +// wrong never reaches ssh, which then cannot log in, and a line says why. +// The same for an answer that is not 200, and without a broker. +func TestOpenSSHAgentChecksTheSignature(t *testing.T) { + t.Parallel() + sshPath, _ := requireOpenSSH(t) + host, _ := newSSHKey(t) + userKey, _ := newSSHKey(t) + other, _ := newSSHKey(t) + tests := []struct { + name string + sign func(http.ResponseWriter, *api.SignRequest) + want string + }{ + {"valid", signWith(userKey, "", ""), ""}, + {"a signature of another key", signWith(other, "", ""), + "the signature of the broker is not a valid signature of the data"}, + {"another format", signWith(userKey, "", ssh.KeyAlgoECDSA256), + "the signature of the broker does not fit the key and the sign flags"}, + {"503", func(w http.ResponseWriter, _ *api.SignRequest) { + transport.WriteError(w, http.StatusServiceUnavailable) + }, "the broker answered 503 Service Unavailable"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + srv := startSSHServer(t, host, userKey.PublicKey()) + fb := newFakeSSHBroker(t, map[string]ssh.PublicKey{"deploy": userKey.PublicKey()}, tt.sign) + au := startAgent(t, fb.b, agentOptions{}) + dir := shortTempDir(t) + out, err := runSSH(t, sshPath, sshArgs(t, srv.addr, "deploy", host.PublicKey(), dir), sshEnv(au.socket, dir)) + if tt.want == "" { + if err != nil || srv.loginCount() != 1 { + t.Fatalf("ssh: %v; agent lines %q\n%s", err, au.lines(), out) + } + return + } + if err == nil || srv.loginCount() != 0 { + t.Fatalf("ssh logged in\n%s", out) + } + if got := au.lines(); len(got) != 1 || got[0] != "nodkey: no signature: "+tt.want { + t.Errorf("the agent wrote %q, want the line %q", got, tt.want) + } + }) + } + + t.Run("no broker", func(t *testing.T) { + t.Parallel() + srv := startSSHServer(t, host, userKey.PublicKey()) + ln, err := (&net.ListenConfig{}).Listen(t.Context(), "tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + addr := ln.Addr().String() + _ = ln.Close() + b, err := newBroker(addr, newTLSKey(t).cert, newTLSKey(t).pin) + if err != nil { + t.Fatal(err) + } + au := startAgent(t, b, agentOptions{}) + dir := shortTempDir(t) + out, err := runSSH(t, sshPath, sshArgs(t, srv.addr, "deploy", host.PublicKey(), dir), sshEnv(au.socket, dir)) + if err == nil || srv.loginCount() != 0 { + t.Fatalf("ssh logged in\n%s", out) + } + // Another test may take the free port at once: then the TLS + // handshake fails instead. + want := "nodkey: no key list: cannot connect to the broker at " + addr + ": " + if got := au.lines(); len(got) != 1 || !strings.HasPrefix(got[0], want) { + t.Errorf("the agent wrote %q, want a line that starts with %q", got, want) + } + }) +} + +// TestOpenSSHAdd: ssh-add lists the broker's keys, by their names. The +// agent refuses ssh-add when it adds or removes keys: the private key +// that ssh-add sends is not kept, and no line holds it. +func TestOpenSSHAdd(t *testing.T) { + t.Parallel() + _, addPath := requireOpenSSH(t) + host, _ := newSSHKey(t) + sb := newSSHBroker(t, map[string]ssh.PublicKey{"web-1": host.PublicKey()}) + au := startAgent(t, sb.client(t), agentOptions{}) + dir := shortTempDir(t) + add := func(args ...string) (string, error) { + ctx, cancel := context.WithTimeout(t.Context(), waitLimit) + defer cancel() + cmd := exec.CommandContext(ctx, addPath, args...) //nolint:gosec // G204: the test runs ssh-add from PATH. + cmd.Env = sshEnv(au.socket, dir) + cmd.Stdin = strings.NewReader("") + out, err := cmd.CombinedOutput() + return string(out), err + } + out, err := add("-L") + if want := strings.TrimSpace(string(ssh.MarshalAuthorizedKey(sb.user.PublicKey()))) + " deploy\n"; err != nil || + out != want { + t.Errorf("ssh-add -L: %v, %q; want %q", err, out, want) + } + if out, err := add("-l"); err != nil || !strings.Contains(out, ssh.FingerprintSHA256(sb.user.PublicKey())+" deploy (ED25519)") { + t.Errorf("ssh-add -l: %v, %q", err, out) + } + // A private key that ssh-add sends. + _, priv := newSSHKey(t) + block, err := ssh.MarshalPrivateKey(priv, "added key") + if err != nil { + t.Fatal(err) + } + keyFile := filepath.Join(dir, "id_ed25519") + keyPEM := pem.EncodeToMemory(block) + if err := os.WriteFile(keyFile, keyPEM, 0o600); err != nil { + t.Fatal(err) + } + pubFile := keyFile + ".pub" + if err := os.WriteFile(pubFile, ssh.MarshalAuthorizedKey(sb.user.PublicKey()), 0o600); err != nil { + t.Fatal(err) + } + const refused = "nodkey: refused a request to add, remove, lock or unlock keys: the keys stay on the broker" + for _, tt := range []struct { + args []string + requests int + }{ + {[]string{keyFile}, 1}, + {[]string{"-t", "60", keyFile}, 1}, + {[]string{"-d", pubFile}, 1}, + // ssh-add -D removes the keys of SSH protocol 2, then those of + // protocol 1. + {[]string{"-D"}, 2}, + } { + before := len(au.lines()) + out, err := add(tt.args...) + if err == nil { + t.Errorf("ssh-add %q worked: %q", tt.args, out) + } + if got := au.lines()[before:]; len(got) != tt.requests || slices.ContainsFunc(got, func(l string) bool { + return l != refused + }) { + t.Errorf("ssh-add %q: the agent wrote %q", tt.args, got) + } + } + if out, err := add("-L"); err != nil || strings.Count(out, "\n") != 1 { + t.Errorf("after the refused requests, ssh-add -L: %v, %q", err, out) + } + if strings.Contains(au.log.String(), "added key") { + t.Error("the agent wrote the comment of the added key") + } +} + +// requireSSHD returns the path of sshd. It skips the test (see +// skipOrFail) when there is none. +func requireSSHD(t *testing.T) string { + t.Helper() + if path, err := exec.LookPath("sshd"); err == nil { + return path + } + for _, path := range []string{"/usr/sbin/sshd", "/usr/local/sbin/sshd"} { + if _, err := os.Stat(path); err == nil { + return path + } + } + skipOrFail(t, "no sshd program") + return "" +} + +// sshUserName returns the name of the user who runs the test: sshd +// without root logs in only that user. It skips the test (see +// skipOrFail) when the name does not match the broker's rule for remote +// users. +func sshUserName(t *testing.T) string { + t.Helper() + u, err := user.Current() + if err != nil || !regexp.MustCompile(`^[A-Za-z0-9._][A-Za-z0-9._-]{0,31}$`).MatchString(u.Username) { + skipOrFail(t, "the user name of this test does not match the rule for remote users") + } + return u.Username +} + +// startSSHD starts the real sshd on 127.0.0.1, as the user who runs the +// test, with the host key hostKey, and userKey in its authorized_keys. It +// returns the address. It skips the test (see skipOrFail) when sshd +// cannot start. +func startSSHD(t *testing.T, sshdPath, dir string, hostKey any, userKey ssh.PublicKey) string { + t.Helper() + block, err := ssh.MarshalPrivateKey(hostKey, "") + if err != nil { + t.Fatal(err) + } + hostKeyFile := filepath.Join(dir, "host_key") + if err := os.WriteFile(hostKeyFile, pem.EncodeToMemory(block), 0o600); err != nil { + t.Fatal(err) + } + authorizedKeys := filepath.Join(dir, "authorized_keys") + if err := os.WriteFile(authorizedKeys, ssh.MarshalAuthorizedKey(userKey), 0o600); err != nil { + t.Fatal(err) + } + ln, err := (&net.ListenConfig{}).Listen(t.Context(), "tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + addr := ln.Addr().String() + _ = ln.Close() + _, port, _ := net.SplitHostPort(addr) + cfg := strings.Join([]string{ + "ListenAddress 127.0.0.1", + "Port " + port, + "HostKey " + hostKeyFile, + "AuthorizedKeysFile " + authorizedKeys, + "PidFile none", + "StrictModes no", + "UsePAM no", + "PasswordAuthentication no", + "KbdInteractiveAuthentication no", + "PubkeyAuthentication yes", + "LogLevel DEBUG1", + }, "\n") + "\n" + cfgFile := filepath.Join(dir, "sshd_config") + if err := os.WriteFile(cfgFile, []byte(cfg), 0o600); err != nil { + t.Fatal(err) + } + ctx, cancel := context.WithCancel(context.Background()) + cmd := exec.CommandContext(ctx, sshdPath, "-D", "-e", "-f", cfgFile) //nolint:gosec // G204: the test runs sshd with its own config. + log := &syncBuffer{} + cmd.Stdout, cmd.Stderr = log, log + if err := cmd.Start(); err != nil { + cancel() + skipOrFail(t, "sshd cannot start: %v", err) + } + done := make(chan struct{}) + go func() { + _ = cmd.Wait() + close(done) + }() + t.Cleanup(func() { + cancel() + <-done + if t.Failed() { + t.Logf("sshd log:\n%s", log.String()) + } + }) + deadline := time.Now().Add(10 * time.Second) + for time.Now().Before(deadline) { + select { + case <-done: + skipOrFail(t, "sshd stopped at once:\n%s", log.String()) + default: + } + c, err := (&net.Dialer{Timeout: time.Second}).DialContext(t.Context(), "tcp", addr) + if err == nil { + _ = c.Close() + return addr + } + time.Sleep(50 * time.Millisecond) + } + skipOrFail(t, "sshd does not listen after 10 seconds:\n%s", log.String()) + return "" +} + +// TestOpenSSHLoginToSSHD logs in with the real ssh to the real sshd +// through the agent and the real broker. sshd offers the hostbound +// method, so the data to sign holds the host key too, and sshd checks the +// signature of the broker. +func TestOpenSSHLoginToSSHD(t *testing.T) { + t.Parallel() + sshPath, _ := requireOpenSSH(t) + sshdPath, name := requireSSHD(t), sshUserName(t) + host, hostPriv := newSSHKey(t) + sb := newSSHBroker(t, map[string]ssh.PublicKey{"web-1": host.PublicKey()}) + dir := shortTempDir(t) + addr := startSSHD(t, sshdPath, dir, hostPriv, sb.user.PublicKey()) + au := startAgent(t, sb.client(t), agentOptions{proc: &procFS{root: procDir}}) + sb.approver.Auto(audit.AnswerApproveOnce) + out, err := runSSH(t, sshPath, sshArgs(t, addr, name, host.PublicKey(), dir), sshEnv(au.socket, dir)) + asked := sb.approver.Asked() + if len(asked) == 0 { + // sshd did not even accept the key, so the cause is not the agent: + // for example a locked account. + skipOrFail(t, "ssh made no sign request (%v):\n%s", err, out) + } + if err != nil { + t.Fatalf("ssh: %v; agent lines %q\n%s", err, au.lines(), out) + } + if p := asked[0].Prompt; p.RemoteUser != name || !slices.Equal(p.Hosts, []string{"web-1"}) { + t.Errorf("the message shows %+v", p) + } + if got := eventNames(sb.audit.waitFor(t, 3)); got != "request decision:approve_once signed" { + t.Errorf("audit entries %q", got) + } +} diff --git a/internal/client/sshagent_other.go b/internal/client/sshagent_other.go new file mode 100644 index 0000000..3b14526 --- /dev/null +++ b/internal/client/sshagent_other.go @@ -0,0 +1,30 @@ +//go:build !linux + +package client + +import ( + "errors" + "io/fs" + "net" + "os" +) + +// peerCredentials refuses every connection here: version 1 runs on Linux +// only (DESIGN.md section 3), and nodkey ssh-agent checks the user of +// each connection with SO_PEERCRED. +func peerCredentials(*net.UnixConn) (peer, error) { + return peer{}, errors.New("client: the user of a connection cannot be checked on this system") +} + +// ownerOf cannot tell the owner of a file here, so every check of an +// owner fails. +func ownerOf(fs.FileInfo) (uint32, bool) { return 0, false } + +// lockFile cannot lock a file here, so the agent does not start. +func lockFile(*os.File) error { + return errors.New("client: the socket cannot be locked on this system") +} + +// waitHangUp cannot watch a connection here: it reports false at once, so +// a request is not cancelled when ssh hangs up. +func waitHangUp(*net.UnixConn) bool { return false } diff --git a/internal/client/sshagent_proc.go b/internal/client/sshagent_proc.go new file mode 100644 index 0000000..881acf5 --- /dev/null +++ b/internal/client/sshagent_proc.go @@ -0,0 +1,267 @@ +package client + +import ( + "errors" + "io" + "os" + "path/filepath" + "strconv" + "strings" + + "github.com/code6x/nodkey/internal/api" +) + +// The claimed context of an SSH login (DESIGN.md section 6, step 5): the +// command lines of the ssh process and of at most 3 of its parents, and +// NODKEY_REASON from the environment of ssh. nodkey ssh-agent reads them +// from /proc, for the process that connected to the agent (SO_PEERCRED). +// They are claims: the agent machine can write anything there, and a +// process ID can be used again by a new process. The broker shows them +// under the "claimed by the agent" line, after it masked them. + +// procDir is where Linux shows its processes. +const procDir = "/proc" + +// Limits of what the agent reads from /proc. +const ( + // maxParents is the most parent processes whose command lines go with + // a sign request, after the one of ssh (DESIGN.md section 6). + maxParents = 3 + // maxCmdline is the most bytes of a command line in /proc that the + // agent sends: api.MaxText bytes of arguments, and a NUL byte after + // each of at most api.MaxArgs arguments. A longer one is sent as + // "[hidden: too long]". + maxCmdline = api.MaxText + api.MaxArgs + // maxStat is the most bytes of /proc/PID/stat that the agent reads. + // The line is far shorter. + maxStat = 4 << 10 + // maxEnviron is the most bytes of /proc/PID/environ that the agent + // reads. Linux allows at most 6 MiB of arguments and environment for + // a new program. + maxEnviron = 8 << 20 +) + +// procFS reads processes from a /proc tree. Its root is procDir; tests +// give it a tree of their own. +type procFS struct { + root string +} + +// claimedContext returns the claimed context of a sign request from the +// process pid: the command lines of pid and of at most maxParents of its +// parents, pid first, and NODKEY_REASON from the environment of pid. It +// returns nothing when pid is 0 (not known, such as a process in another +// PID namespace) or its command line cannot be read. The chain of parents +// stops at the first one that cannot be read, has no command line, such +// as a kernel thread, or is there twice. +// +// Each command line and the reason are as nodkey run sends them: a field +// that is too long becomes "[hidden: too long]" (claimedCommand, +// claimedText). Nothing else of the environment is kept or sent. +// +// A read of /proc can block, such as on a mapping of a stuck FUSE or NFS +// file system; sshAgent.claimed waits for claimedContext only for a short +// time. +func (p procFS) claimedContext(pid int32) (commands [][]string, reason string) { + commands = [][]string{} + pr, err := p.openProcess(int(pid)) + if err != nil { + return commands, "" + } + argv, ok := pr.commandLine() + if !ok { + pr.close() + return commands, "" + } + commands = append(commands, argv) + reason = pr.reason() + parent, hasParent := pr.parent() + pr.close() + seen := map[int]bool{int(pid): true} + for range maxParents { + if !hasParent || seen[parent] { + break + } + seen[parent] = true + pr, err := p.openProcess(parent) + if err != nil { + break + } + argv, ok := pr.commandLine() + parent, hasParent = pr.parent() + pr.close() + if !ok { + break + } + commands = append(commands, argv) + } + return commands, reason +} + +// process is the open directory of one process in /proc. Its files are +// read through it (openat), so that they all come from that one process: +// when the process ends and a new process gets its ID, a read fails +// instead of reading the new process. +type process struct { + dir *os.Root +} + +// openProcess opens the directory of process pid. +func (p procFS) openProcess(pid int) (*process, error) { + if pid <= 0 { + return nil, errors.New("client: no such process") + } + dir, err := os.OpenRoot(filepath.Join(p.root, strconv.Itoa(pid))) + if err != nil { + return nil, err + } + return &process{dir: dir}, nil +} + +// close closes the directory. +func (pr *process) close() { _ = pr.dir.Close() } + +// read reads the file name of the process, at most limit bytes. over is +// true, and data nil, when the file has more. +func (pr *process) read(name string, limit int) (data []byte, over bool, err error) { + f, err := pr.dir.Open(name) + if err != nil { + return nil, false, err + } + defer func() { _ = f.Close() }() + data, err = io.ReadAll(io.LimitReader(f, int64(limit)+1)) + if err != nil { + return nil, false, err + } + if len(data) > limit { + return nil, true, nil + } + return data, false, nil +} + +// commandLine returns the command line of the process, from +// /proc/PID/cmdline: the arguments, each followed by a NUL byte. ok is +// false when it cannot be read, or is empty, as for a kernel thread or a +// process that ended. +func (pr *process) commandLine() ([]string, bool) { + data, over, err := pr.read("cmdline", maxCmdline) + switch { + case err != nil: + return nil, false + case over: + return []string{tooLong}, true + case len(data) == 0: + return nil, false + } + // The NUL after the last argument ends it; it does not start another + // one. + s := strings.TrimSuffix(string(data), "\x00") + return claimedCommand(strings.Split(s, "\x00")), true +} + +// parent returns the parent of the process, from /proc/PID/stat: +// "PID (NAME) STATE PPID ...". NAME can hold any byte, also ")" and +// spaces, so the fields after it start after the last ")". ok is false +// when there is no parent, as for process 1, or it cannot be read. +func (pr *process) parent() (int, bool) { + data, over, err := pr.read("stat", maxStat) + if err != nil || over { + return 0, false + } + s := string(data) + i := strings.LastIndexByte(s, ')') + if i < 0 { + return 0, false + } + fields := strings.Fields(s[i+1:]) + if len(fields) < 2 { + return 0, false + } + ppid, err := strconv.Atoi(fields[1]) + if err != nil || ppid <= 0 { + return 0, false + } + return ppid, true +} + +// reason returns NODKEY_REASON from the environment of the process, from +// /proc/PID/environ, as claimedText makes it: "" when it is not set or +// cannot be read, and "[hidden: too long]" when it is longer than +// api.MaxText bytes. +func (pr *process) reason() string { + f, err := pr.dir.Open("environ") + if err != nil { + return "" + } + defer func() { _ = f.Close() }() + value, found, long := envValue(io.LimitReader(f, maxEnviron), reasonVar, api.MaxText) + switch { + case !found: + return "" + case long: + return tooLong + } + return value +} + +// envValue returns the value of the first variable called name in r, an +// environment as /proc/PID/environ holds it: entries "NAME=VALUE", each +// followed by a NUL byte. The first one counts, as for getenv(3) of the C +// library, which ssh uses. A value longer than limit bytes is not +// returned: long is true then. +// +// envValue keeps no other entry. It reads r in small pieces, looks at one +// byte at a time, and clears each piece after use. So no other variable of +// the environment, which may hold a secret, stays in memory. +func envValue(r io.Reader, name string, limit int) (value string, found, long bool) { + prefix := name + "=" + buf := make([]byte, 4<<10) + defer clear(buf) + var val []byte + defer func() { clear(val) }() + const ( + inName = iota // in the name of an entry; matched bytes of prefix so far + inValue // in the value of the variable + skipping // in another entry, up to its NUL + ) + state, matched := inName, 0 + for { + n, err := r.Read(buf) + for _, c := range buf[:n] { + switch state { + case inName: + switch c { + case 0: + matched = 0 // the entry ended; the next one starts + case prefix[matched]: + if matched++; matched == len(prefix) { + state = inValue + } + default: + state = skipping + } + case inValue: + if c == 0 { + return string(val), true, long + } + if len(val) < limit { + val = append(val, c) + } else { + long = true + } + case skipping: + if c == 0 { + state, matched = inName, 0 + } + } + } + if err != nil { + break + } + } + // The last entry may end without a NUL byte. + if state == inValue { + return string(val), true, long + } + return "", false, false +} diff --git a/internal/client/sshagent_proc_linux_test.go b/internal/client/sshagent_proc_linux_test.go new file mode 100644 index 0000000..7168074 --- /dev/null +++ b/internal/client/sshagent_proc_linux_test.go @@ -0,0 +1,139 @@ +package client + +import ( + "bufio" + "os" + "os/exec" + "slices" + "strconv" + "strings" + "syscall" + "testing" +) + +// TestClaimedContextRealProcess reads the claimed context of a real +// process from /proc: a sleep that a shell started, with NODKEY_REASON in +// its own environment. Its parents are the shell, the test, and the +// program that started the test. The reason of the shell and of the test +// does not count, and nothing else of the environment is returned. +func TestClaimedContextRealProcess(t *testing.T) { + t.Parallel() + sh, sleep := needProgram(t, "sh"), needProgram(t, "sleep") + canary := madeUp("canary") + script := `NODKEY_REASON="from the sleep" NODKEY_CANARY="$1" "$0" 60 & echo "$!"; wait` + cmd := exec.CommandContext(t.Context(), sh, "-c", script, sleep, canary) //nolint:gosec // G204: the test runs sleep through sh. + cmd.Env = append(withEnv(os.Environ(), nil), "NODKEY_REASON=from the shell", "OTHER="+canary) + out, err := cmd.StdoutPipe() + if err != nil { + t.Fatal(err) + } + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + line, err := bufio.NewReader(out).ReadString('\n') + if err != nil { + t.Fatal(err) + } + pid, err := strconv.Atoi(strings.TrimSpace(line)) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + _ = syscall.Kill(pid, syscall.SIGKILL) + _ = cmd.Wait() + }) + // The shell forks, and the child then runs sleep. + waitFor(t, "the sleep to start", func() bool { + argv := realCommandLine(t, pid) + return len(argv) > 0 && argv[0] == sleep + }) + proc := procFS{root: procDir} + commands, reason := proc.claimedContext(int32(pid)) //nolint:gosec // G115: a process ID fits in an int32. + want := [][]string{{sleep, "60"}, {sh, "-c", script, sleep, canary}, os.Args} + if parent := realCommandLine(t, os.Getppid()); parent != nil { + want = append(want, parent) + } + if !slices.EqualFunc(commands, want, slices.Equal) { + t.Errorf("the command lines are %q\nwant %q", safe(strings.Join(flat(commands), " | ")), + safe(strings.Join(flat(want), " | "))) + } + if reason != "from the sleep" { + t.Errorf("the reason is %q, want the one of the sleep", safe(reason)) + } +} + +// realCommandLine returns the command line of process pid, read here +// without procFS, or nil when there is none. +func realCommandLine(t *testing.T, pid int) []string { + t.Helper() + data, err := os.ReadFile("/proc/" + strconv.Itoa(pid) + "/cmdline") + if err != nil || len(data) == 0 { + return nil + } + return strings.Split(strings.TrimSuffix(string(data), "\x00"), "\x00") +} + +// flat joins each command line with spaces. +func flat(commands [][]string) []string { + out := make([]string, len(commands)) + for i, c := range commands { + out[i] = strings.Join(c, " ") + } + return out +} + +// TestRootPinsTheProcess: openProcess opens /proc/PID once, and the files +// come only from that process. The links of /proc/PID, such as cwd, exe +// and root, are never followed out of it. When the process ends, its +// files cannot be read any more, also not those of a new process with the +// same ID. (From the security review of issue #13.) +func TestRootPinsTheProcess(t *testing.T) { + t.Parallel() + sleep := needProgram(t, "sleep") + cmd := exec.CommandContext(t.Context(), sleep, "60") //nolint:gosec // G204: the test runs sleep. + cmd.Env = []string{"NODKEY_REASON=from the sleep"} + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + ended := false + t.Cleanup(func() { + if !ended { + _ = cmd.Process.Kill() + _ = cmd.Wait() + } + }) + pid := cmd.Process.Pid + waitFor(t, "the sleep to start", func() bool { + argv := realCommandLine(t, pid) + return len(argv) > 0 && argv[0] == sleep + }) + pr, err := procFS{root: procDir}.openProcess(pid) + if err != nil { + t.Fatal(err) + } + defer pr.close() + if argv, ok := pr.commandLine(); !ok || argv[0] != sleep { + t.Fatalf("the command line is %q, %v", argv, ok) + } + if r := pr.reason(); r != "from the sleep" { + t.Fatalf("the reason is %q", r) + } + // fd/0, fd/1 and map_files name files of the process itself; the + // others lead out of /proc/PID. + for _, name := range []string{"cwd", "exe", "root", "root/etc/hostname", "cwd/x", "../1/cmdline", + "/etc/hostname", "task/" + strconv.Itoa(pid) + "/root/etc/hostname"} { + if f, err := pr.dir.Open(name); err == nil { + _ = f.Close() + t.Errorf("%s: opened through the directory of the process", name) + } + } + _ = cmd.Process.Kill() + _ = cmd.Wait() + ended = true + argv, ok := pr.commandLine() + _, hasParent := pr.parent() + if reason := pr.reason(); ok || hasParent || reason != "" { + t.Errorf("after the process ended: the command line %q (%v), a parent %v, the reason %q", argv, ok, hasParent, + reason) + } +} diff --git a/internal/client/sshagent_proc_test.go b/internal/client/sshagent_proc_test.go new file mode 100644 index 0000000..990050c --- /dev/null +++ b/internal/client/sshagent_proc_test.go @@ -0,0 +1,284 @@ +package client + +import ( + "bytes" + "fmt" + "net" + "net/http" + "os" + "path/filepath" + "slices" + "strconv" + "strings" + "testing" + "testing/iotest" + + "github.com/code6x/nodkey/internal/api" + "github.com/code6x/nodkey/internal/transport" +) + +// fakeProc is a /proc tree of a test. +type fakeProc struct{ root string } + +func newFakeProc(t *testing.T) fakeProc { return fakeProc{root: t.TempDir()} } + +// fs returns the procFS that reads the tree. +func (f fakeProc) fs() *procFS { return &procFS{root: f.root} } + +// write writes the file name of process pid. +func (f fakeProc) write(t *testing.T, pid int, name string, data []byte) { + t.Helper() + dir := filepath.Join(f.root, strconv.Itoa(pid)) + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, name), data, 0o600); err != nil { + t.Fatal(err) + } +} + +// add adds process pid with its parent, its command line (a NUL byte +// after each argument, as Linux writes it) and its environment. +func (f fakeProc) add(t *testing.T, pid, ppid int, argv, env []string) { + t.Helper() + f.write(t, pid, "cmdline", nulList(argv)) + f.write(t, pid, "stat", fmt.Appendf(nil, "%d (%s) S %d %d %d 0 -1 4194304", pid, filepath.Base(argv[0]), ppid, + ppid, ppid)) + f.write(t, pid, "environ", nulList(env)) +} + +// commandLineOf reads the command line of process pid of the tree p. +func commandLineOf(p *procFS, pid int) ([]string, bool) { + pr, err := p.openProcess(pid) + if err != nil { + return nil, false + } + defer pr.close() + return pr.commandLine() +} + +// reasonOf reads NODKEY_REASON of process pid of the tree p. +func reasonOf(p *procFS, pid int) string { + pr, err := p.openProcess(pid) + if err != nil { + return "" + } + defer pr.close() + return pr.reason() +} + +// nulList returns the strings, each followed by a NUL byte. +func nulList(list []string) []byte { + var b []byte + for _, s := range list { + b = append(append(b, s...), 0) + } + return b +} + +// TestClaimedContextParents: the command lines of the process that +// connected and of at most 3 of its parents (DESIGN.md section 6), that +// process first, and NODKEY_REASON from the environment of that process +// only. +func TestClaimedContextParents(t *testing.T) { + t.Parallel() + p := newFakeProc(t) + p.add(t, 100, 99, []string{"ssh", "deploy@web-1"}, []string{"A=1", "NODKEY_REASON=apply the nginx fix", "B=2"}) + p.add(t, 99, 98, []string{"ansible-playbook", "site.yml"}, []string{"NODKEY_REASON=from the parent"}) + p.add(t, 98, 97, []string{"/bin/bash", "-c", "make deploy"}, nil) + p.add(t, 97, 96, []string{"node", "agent.js"}, nil) + p.add(t, 96, 1, []string{"sshd: dev@pts/0"}, nil) + p.add(t, 1, 0, []string{"/sbin/init"}, nil) + commands, reason := p.fs().claimedContext(100) + want := [][]string{{"ssh", "deploy@web-1"}, {"ansible-playbook", "site.yml"}, {"/bin/bash", "-c", "make deploy"}, + {"node", "agent.js"}} + if !slices.EqualFunc(commands, want, slices.Equal) || reason != "apply the nginx fix" { + t.Errorf("claimedContext = %q, %q; want %q, the reason of process 100", commands, reason, want) + } + if got, _ := p.fs().claimedContext(98); len(got) != 4 || got[3][0] != "/sbin/init" { + t.Errorf("from process 98: %q; want 98, 97, 96 and 1", got) + } + if got, reason := p.fs().claimedContext(1); len(got) != 1 || reason != "" { + t.Errorf("from process 1: %q, %q; it has no parent", got, reason) + } +} + +// TestClaimedContextStops: the chain of parents stops at a parent that +// cannot be read, has no command line (a kernel thread), or is there +// twice. Without the command line of the process that connected, there +// is no claimed context at all. +func TestClaimedContextStops(t *testing.T) { + t.Parallel() + p := newFakeProc(t) + p.add(t, 10, 11, []string{"ssh", "web-1"}, []string{"NODKEY_REASON=r"}) // 11 is not there + p.add(t, 20, 21, []string{"ssh", "web-1"}, nil) + p.add(t, 21, 2, []string{"x"}, nil) + p.write(t, 21, "cmdline", nil) // a kernel thread + p.add(t, 30, 31, []string{"ssh"}, nil) + p.add(t, 31, 30, []string{"loop"}, nil) + p.add(t, 40, 41, []string{"ssh"}, nil) + p.write(t, 40, "stat", []byte("40 (ssh) S")) + p.add(t, 50, 51, []string{"ssh"}, nil) + p.write(t, 50, "stat", []byte("50 (a) b (c) d) S 51 1 1")) + p.add(t, 51, 0, []string{"parent of a strange name"}, nil) + p.write(t, 60, "environ", nulList([]string{"NODKEY_REASON=only an environment"})) + tests := []struct { + pid int32 + want [][]string + reason string + }{ + {10, [][]string{{"ssh", "web-1"}}, "r"}, + {20, [][]string{{"ssh", "web-1"}}, ""}, + {30, [][]string{{"ssh"}, {"loop"}}, ""}, + {40, [][]string{{"ssh"}}, ""}, + {50, [][]string{{"ssh"}, {"parent of a strange name"}}, ""}, + {60, [][]string{}, ""}, + {70, [][]string{}, ""}, + {0, [][]string{}, ""}, + {-1, [][]string{}, ""}, + } + for _, tt := range tests { + commands, reason := p.fs().claimedContext(tt.pid) + if !slices.EqualFunc(commands, tt.want, slices.Equal) || commands == nil || reason != tt.reason { + t.Errorf("process %d: %q, %q; want %q, %q", tt.pid, commands, reason, tt.want, tt.reason) + } + } +} + +// TestCommandLine: a command line is as nodkey run sends one: more than +// api.MaxArgs arguments, or more than api.MaxText bytes in all, become +// "[hidden: too long]". +func TestCommandLine(t *testing.T) { + t.Parallel() + p := newFakeProc(t) + full := make([]string, api.MaxArgs) + for i := range full { + full[i] = strings.Repeat("a", api.MaxText/api.MaxArgs) + } + tests := []struct { + name string + data []byte + want []string + }{ + {"arguments", []byte("ssh\x00-p\x002222\x00web-1\x00"), []string{"ssh", "-p", "2222", "web-1"}}, + {"an empty argument", []byte("ssh\x00\x00"), []string{"ssh", ""}}, + {"no NUL at the end", []byte("ssh: web-1 [mux]"), []string{"ssh: web-1 [mux]"}}, + {"at the limits", nulList(full), full}, + {"too many arguments", nulList(append(slices.Clone(full[:2]), make([]string, api.MaxArgs)...)), + []string{tooLong}}, + {"too long", nulList(append(slices.Clone(full), "a")), []string{tooLong}}, + {"far too long", bytes.Repeat([]byte("a"), maxCmdline+1), []string{tooLong}}, + } + for i, tt := range tests { + p.write(t, 200+i, "cmdline", tt.data) + got, ok := commandLineOf(p.fs(), 200+i) + if !ok || !slices.Equal(got, tt.want) { + t.Errorf("%s: %d arguments, %v; want %d", tt.name, len(got), ok, len(tt.want)) + } + } + if _, ok := commandLineOf(p.fs(), 300); ok { + t.Error("a process that is not there has a command line") + } +} + +// TestEnvValue: the first NODKEY_REASON of an environment, exactly that +// name, also without a NUL byte at the end; a value over the limit is +// "too long". +func TestEnvValue(t *testing.T) { + t.Parallel() + long := strings.Repeat("r", api.MaxText) + tests := []struct { + name string + env string + value string + found bool + long bool + }{ + {"one", "A=1\x00NODKEY_REASON=apply the fix\x00B=2\x00", "apply the fix", true, false}, + {"first", "NODKEY_REASON=first\x00NODKEY_REASON=second\x00", "first", true, false}, + {"empty", "NODKEY_REASON=\x00", "", true, false}, + {"no NUL at the end", "A=1\x00NODKEY_REASON=last", "last", true, false}, + {"a longer name", "NODKEY_REASONS=x\x00NODKEY_REASON_2=y\x00", "", false, false}, + {"a name that ends with it", "X_NODKEY_REASON=x\x00", "", false, false}, + {"the name in a value", "A=NODKEY_REASON=x\x00", "", false, false}, + {"without =", "NODKEY_REASON\x00NODKEY_REASON=after\x00", "after", true, false}, + {"a short entry first", "NODKEY\x00NODKEY_REASON=after\x00", "after", true, false}, + {"a line break in the value", "NODKEY_REASON=a\nb\x00", "a\nb", true, false}, + {"at the limit", "NODKEY_REASON=" + long + "\x00", long, true, false}, + {"too long", "NODKEY_REASON=" + long + "r\x00", "", true, true}, + {"not set", "A=1\x00B=2\x00", "", false, false}, + {"empty environment", "", "", false, false}, + } + for _, tt := range tests { + // One byte at a time, so that every place where a piece ends is + // tried, and in pieces of 4 KiB. + value, found, long := envValue(iotest.OneByteReader(strings.NewReader(tt.env)), reasonVar, api.MaxText) + if found != tt.found || long != tt.long || (!long && value != tt.value) { + t.Errorf("%s: %d bytes, %v, %v; want %d bytes, %v, %v", tt.name, len(value), found, long, + len(tt.value), tt.found, tt.long) + } + value2, found2, long2 := envValue(strings.NewReader(tt.env), reasonVar, api.MaxText) + if value2 != value || found2 != found || long2 != long { + t.Errorf("%s: another result when read in larger pieces", tt.name) + } + } + p := newFakeProc(t) + p.write(t, 5, "environ", []byte("NODKEY_REASON="+long+"r\x00")) + if got := reasonOf(p.fs(), 5); got != tooLong { + t.Errorf("a reason over api.MaxText is %d bytes", len(got)) + } + if got := reasonOf(p.fs(), 6); got != "" { + t.Errorf("the reason of a process that is not there is %q", got) + } +} + +// TestAgentSendsClaimedContext: the sign request holds the command lines +// and the reason of the process that connected, from /proc, and nothing +// from the agent's own environment. +func TestAgentSendsClaimedContext(t *testing.T) { // not parallel: it sets NODKEY_REASON + t.Setenv(reasonVar, "the reason of the agent itself") + canary := madeUp("canary") + p := newFakeProc(t) + p.add(t, 4242, 4241, []string{"ssh", "-o", "Password=" + canary, "web-1"}, + []string{"SECRET=" + canary, "NODKEY_REASON=apply the nginx fix", "TOKEN=" + canary}) + p.add(t, 4241, 4240, []string{"ansible-playbook", "site.yml"}, []string{"NODKEY_REASON=from ansible"}) + self, err := currentUID() + if err != nil { + t.Fatal(err) + } + bodies := make(chan []byte, 1) + fb := newFakeBroker(t, func(w http.ResponseWriter, r *http.Request) { + bodies <- readBody(t, r) + transport.WriteError(w, http.StatusForbidden) + }) + au := startAgent(t, fb.b, agentOptions{proc: p.fs(), peer: func(*net.UnixConn) (peer, error) { + return peer{uid: self, pid: 4242}, nil + }}) + if signatureOf(t, rawCall(t, au.conn(t), signRequestMessage([]byte("key"), []byte("data"), 0))) != nil { + t.Fatal("the agent gave ssh a signature") + } + body := <-bodies + var req api.SignRequest + if err := req.UnmarshalJSON(body); err != nil { + t.Fatal(err) + } + want := [][]string{{"ssh", "-o", "Password=" + canary, "web-1"}, {"ansible-playbook", "site.yml"}} + if !slices.EqualFunc(req.Commands, want, slices.Equal) || req.Reason != "apply the nginx fix" { + t.Errorf("the request has %q and the reason %q", safe(fmt.Sprint(req.Commands)), req.Reason) + } + // The command line goes as it is: the broker masks it. Nothing else + // of the environment goes. + if n := strings.Count(string(body), canary); n != 1 { + t.Errorf("the request holds the canary %d times; want once, in the command line", n) + } +} + +// readBody reads the body of a request to a fake broker. +func readBody(t *testing.T, r *http.Request) []byte { + t.Helper() + var b bytes.Buffer + if _, err := b.ReadFrom(r.Body); err != nil { + t.Error(err) + } + return b.Bytes() +} diff --git a/internal/client/sshagent_socket.go b/internal/client/sshagent_socket.go new file mode 100644 index 0000000..b2aa930 --- /dev/null +++ b/internal/client/sshagent_socket.go @@ -0,0 +1,298 @@ +package client + +import ( + "context" + "errors" + "fmt" + "io/fs" + "net" + "os" + "path/filepath" + "strings" + "syscall" + "time" +) + +// The socket of nodkey ssh-agent (DESIGN.md section 15). It is in a +// directory with mode 0700, owned by the user that runs the agent, such +// as $XDG_RUNTIME_DIR/nodkey; nodkey makes the directory when it is +// missing. The socket has mode 0600. Every directory on the way passes +// config.CheckParentDirs, so no other user can swap the directory or the +// socket. The directory keeps other users away from the socket, and the +// agent checks the user of each connection too (SO_PEERCRED). While the +// agent runs, it holds a lock on the file PATH.lock next to the socket, +// so a second agent on the same path stops at once. + +// socketDirName is the directory of the default socket, in +// $XDG_RUNTIME_DIR. +const socketDirName = "nodkey" + +// socketName is the name of the default socket. +const socketName = "agent.sock" + +// maxSocketPath is the longest path of a Unix socket: the kernel's +// sun_path holds 108 bytes, with a NUL byte at the end. +const maxSocketPath = 107 + +// staleCheckTimeout bounds the connection to a socket that is already +// there, to learn whether a program serves it. +const staleCheckTimeout = 2 * time.Second + +// defaultSocket returns the default socket: $XDG_RUNTIME_DIR/nodkey/agent.sock. +// systemd makes XDG_RUNTIME_DIR for each user who logs in, with mode 0700. +func defaultSocket(getenv func(string) string) (string, error) { + dir := getenv("XDG_RUNTIME_DIR") + if dir == "" || !filepath.IsAbs(dir) { + return "", errors.New("XDG_RUNTIME_DIR is not set, or not an absolute path; give the socket with --socket PATH") + } + return filepath.Join(dir, socketDirName, socketName), nil +} + +// checkSocketPath checks the form of the socket's path: absolute, clean +// (no "//", "." or ".." parts, and no "/" at the end), not in "/" itself, +// and short enough for a Unix socket. +func checkSocketPath(path string) error { + switch { + case !filepath.IsAbs(path): + return errors.New("the socket path must be absolute, so that SSH_AUTH_SOCK works in every directory") + case filepath.Clean(path) != path, strings.ContainsRune(path, 0): + return errors.New(`the socket path must be clean: no "//", "." or ".." parts, and no "/" at the end`) + case filepath.Dir(path) == string(filepath.Separator): + return errors.New("the socket must not be in /: its directory must have mode 0700") + case len(path) > maxSocketPath: + return fmt.Errorf("the socket path is longer than %d bytes, the limit of a Unix socket", maxSocketPath) + } + return nil +} + +// agentSocket is the listening socket of nodkey ssh-agent. +type agentSocket struct { + path string + ln *net.UnixListener + fi fs.FileInfo // the socket file that Listen made + lock *os.File // PATH.lock, with the lock of the agent +} + +// listenAgent makes the socket at path, for the user self. checkDirs is +// config.CheckParentDirs; tests replace it, because the system temporary +// directory does not pass it. +// +// 1. The directories on the way to the socket's directory must pass +// checkDirs: owned by root or by self, and group and others cannot +// write to them. So no other user can swap the directory. +// 2. The socket's directory is made with mode 0700 if it is missing. If +// it is there, it must be a directory, not a symbolic link, owned by +// self, with mode 0700, such as $XDG_RUNTIME_DIR itself. nodkey never +// changes a directory that it did not make. +// 3. The agent takes the lock of the socket (lockSocket) and holds it +// until close. A second agent on the same path stops here, so two +// agents never remove each other's socket. +// 4. A socket that is already at path is removed only when it is owned +// by self and no program serves it: it is left from an agent that +// ended without removing it. Anything else at path is an error, and +// stays. +// 5. The new socket gets mode 0600, whatever the umask. +func listenAgent(ctx context.Context, path string, self uint32, checkDirs func(string) error) (*agentSocket, error) { + if err := checkSocketPath(path); err != nil { + return nil, err + } + dir := filepath.Dir(path) + if err := checkDirs(dir); err != nil { + return nil, err // the error names the directory + } + if err := makeSocketDir(dir, self); err != nil { + return nil, err + } + if err := checkDirs(path); err != nil { + return nil, err + } + lock, err := lockSocket(path, self) + if err != nil { + return nil, err + } + if err := removeStale(ctx, path, self); err != nil { + _ = lock.Close() + return nil, err + } + ln, err := (&net.ListenConfig{}).Listen(ctx, "unix", path) + if err != nil { + _ = lock.Close() + return nil, fmt.Errorf("cannot make the socket: %w", err) // a *net.OpError names the path + } + ul, ok := ln.(*net.UnixListener) + if !ok { + _ = ln.Close() + _ = lock.Close() + return nil, errors.New("cannot make the socket: it is not a Unix socket") + } + // close removes the socket only while it is the one that Listen made. + ul.SetUnlinkOnClose(false) + s := &agentSocket{path: path, ln: ul, lock: lock} + if s.fi, err = os.Lstat(path); err != nil { //nolint:gosec // G703: the socket path of the user; checkSocketPath and checkDirs checked it. + _ = ul.Close() + _ = lock.Close() + return nil, err + } + if err := s.setMode(self); err != nil { + s.close() + return nil, err + } + return s, nil +} + +// setMode gives the new socket mode 0600 and checks it. bind(2) makes it +// with the mode that the umask allows. +func (s *agentSocket) setMode(self uint32) error { + if err := os.Chmod(s.path, 0o600); err != nil { + return err + } + fi, err := os.Lstat(s.path) + if err != nil { + return err + } + owner, ok := ownerOf(fi) + if !os.SameFile(fi, s.fi) || fi.Mode().Type() != fs.ModeSocket || !ok || owner != self || + fi.Mode().Perm() != 0o600 { + return fmt.Errorf("%s: the new socket is not a socket of the user with mode 0600", s.path) + } + return nil +} + +// close stops listening, removes the socket if it is still the one that +// Listen made, and then gives up the lock. So an agent never removes the +// socket of another one. +func (s *agentSocket) close() { + _ = s.ln.Close() + // The path of the user; listenAgent checked it. Only the socket that + // Listen made is removed. + if fi, err := os.Lstat(s.path); err == nil && os.SameFile(fi, s.fi) { //nolint:gosec // G703: see above. + _ = os.Remove(s.path) //nolint:gosec // G703: see above. + } + _ = s.lock.Close() +} + +// lockSocket takes the lock of the socket at path: an flock on the file +// PATH.lock, in the socket's directory. nodkey makes that file with mode +// 0600 if it is missing. If it is there, it must be a regular file of +// self, not a symbolic link. If another process holds the lock, such as a +// second nodkey ssh-agent on the same path, lockSocket fails at once. The +// lock ends when the returned file is closed, also when the agent ends in +// a crash. The file stays: removing it would let two agents lock two +// different files. +func lockSocket(path string, self uint32) (*os.File, error) { + name := path + ".lock" + // O_EXCL makes it only when it is missing, so that the mode can be + // set after the umask. O_NOFOLLOW refuses a symbolic link, and + // O_NONBLOCK does not wait on a FIFO, which the check below refuses. + const flags = os.O_RDONLY | syscall.O_NOFOLLOW | syscall.O_NONBLOCK + f, err := os.OpenFile(name, flags|os.O_CREATE|os.O_EXCL, 0o600) //nolint:gosec // G304: next to the user's socket path, which listenAgent checked. + switch { + case err == nil: + if err := f.Chmod(0o600); err != nil { + _ = f.Close() + return nil, err + } + case errors.Is(err, fs.ErrExist): + f, err = os.OpenFile(name, flags, 0) //nolint:gosec // G304: see above. + if err != nil { + return nil, fmt.Errorf("cannot open the lock file of the socket: %w", err) // a *fs.PathError names it + } + default: + return nil, fmt.Errorf("cannot make the lock file of the socket: %w", err) + } + fi, err := f.Stat() + if err != nil { + _ = f.Close() + return nil, err + } + if owner, ok := ownerOf(fi); !fi.Mode().IsRegular() || !ok || owner != self { + _ = f.Close() + return nil, fmt.Errorf("%s: is not a regular file of the user; nodkey does not use it as a lock", name) + } + if err := lockFile(f); err != nil { + _ = f.Close() + if errors.Is(err, syscall.EWOULDBLOCK) { + return nil, fmt.Errorf("%s: another nodkey ssh-agent uses this socket; stop it, or give another --socket", + path) + } + return nil, fmt.Errorf("cannot lock the socket: %w", err) + } + return f, nil +} + +// makeSocketDir makes the socket's directory dir with mode 0700, or checks +// the one that is there: a directory, not a symbolic link, owned by self, +// with mode 0700. +func makeSocketDir(dir string, self uint32) error { + // dir is the directory of the user's socket path: checkSocketPath and + // checkDirs checked it. + err := os.Mkdir(dir, 0o700) //nolint:gosec // G703: see above. + switch { + case err == nil: + // The umask may have taken bits away. The directory is new and + // nodkey's own. + if err := os.Chmod(dir, 0o700); err != nil { //nolint:gosec // G302: a directory needs the execute bit; 0700 gives nothing to group and others. + return err + } + case !errors.Is(err, fs.ErrExist): + return err // a *fs.PathError names the directory + } + fi, err := os.Lstat(dir) //nolint:gosec // G703: see above. + if err != nil { + return err + } + switch { + case fi.Mode()&fs.ModeSymlink != 0: + return fmt.Errorf("%s: is a symbolic link; the directory of the socket must be a directory with mode 0700", dir) + case !fi.IsDir(): + return fmt.Errorf("%s: is not a directory", dir) + } + owner, ok := ownerOf(fi) + switch { + case !ok: + return fmt.Errorf("%s: cannot check the owner on this system", dir) + case owner != self: + return fmt.Errorf("%s: is owned by user ID %d; the directory of the socket must be owned by the user that "+ + "runs nodkey ssh-agent (ID %d)", dir, owner, self) + } + if perm := fi.Mode().Perm(); perm != 0o700 { + return fmt.Errorf("%s: has mode %04o; the directory of the socket must have mode 0700, so that no other "+ + "user can reach the socket", dir, perm) + } + return nil +} + +// removeStale removes a socket at path that no program serves. It must be +// a socket owned by self. If a program serves it, such as another nodkey +// ssh-agent, or anything else is at path, it stays, and the agent does not +// start. +func removeStale(ctx context.Context, path string, self uint32) error { + fi, err := os.Lstat(path) //nolint:gosec // G703: the user's socket path, which listenAgent checked. + switch { + case errors.Is(err, fs.ErrNotExist): + return nil + case err != nil: + return err + case fi.Mode().Type() != fs.ModeSocket: + return fmt.Errorf("%s: is there, and it is not a socket; nodkey does not remove it", path) + } + if owner, ok := ownerOf(fi); !ok || owner != self { + return fmt.Errorf("%s: is a socket of another user; nodkey does not remove it", path) + } + dialCtx, cancel := context.WithTimeout(ctx, staleCheckTimeout) + defer cancel() + c, err := (&net.Dialer{}).DialContext(dialCtx, "unix", path) + if err == nil { + _ = c.Close() + return fmt.Errorf("%s: another program serves this socket, such as another nodkey ssh-agent; stop it, "+ + "or give another --socket", path) + } + if !errors.Is(err, syscall.ECONNREFUSED) { + return fmt.Errorf("%s: cannot check the socket that is there: %w", path, err) + } + // Nobody serves it: an agent ended without removing it. + if err := os.Remove(path); err != nil && !errors.Is(err, fs.ErrNotExist) { //nolint:gosec // G703: a stale socket of the user, as checked above. + return err + } + return nil +} diff --git a/internal/client/sshagent_socket_linux_test.go b/internal/client/sshagent_socket_linux_test.go new file mode 100644 index 0000000..6b2b21a --- /dev/null +++ b/internal/client/sshagent_socket_linux_test.go @@ -0,0 +1,419 @@ +package client + +import ( + "errors" + "io/fs" + "net" + "os" + "path/filepath" + "strings" + "syscall" + "testing" + "time" + + "github.com/code6x/nodkey/internal/config" +) + +// uid returns the user ID of the test. +func uid(t *testing.T) uint32 { + t.Helper() + self, err := currentUID() + if err != nil { + t.Fatal(err) + } + return self +} + +// TestListenAgentModes: the agent makes the socket's directory with mode +// 0700 and the socket with mode 0600, whatever the umask. The socket +// works, and close removes it. +func TestListenAgentModes(t *testing.T) { // not parallel: it changes the umask + for _, mask := range []int{0, 0o022, 0o777} { + dir := filepath.Join(shortTempDir(t), "nodkey") + path := filepath.Join(dir, socketName) + self := uid(t) + old := syscall.Umask(mask) + sock, err := listenAgent(t.Context(), path, self, noDirCheck) + syscall.Umask(old) + if err != nil { + t.Fatalf("umask %04o: %v", mask, err) + } + mustMode(t, dir, fs.ModeDir, 0o700) + mustMode(t, path, fs.ModeSocket, 0o600) + c, err := (&net.Dialer{}).DialContext(t.Context(), "unix", path) + if err != nil { + t.Fatalf("umask %04o: the socket does not take a connection: %v", mask, err) + } + _ = c.Close() + sock.close() + if _, err := os.Lstat(path); !errors.Is(err, fs.ErrNotExist) { + t.Errorf("umask %04o: the socket is still there after close: %v", mask, err) + } + mustMode(t, dir, fs.ModeDir, 0o700) // the directory stays + } +} + +// TestListenAgentRefusesTheDirectory: a directory that is there must be a +// real directory of the user with mode 0700. nodkey does not change it. +func TestListenAgentRefusesTheDirectory(t *testing.T) { + t.Parallel() + base := shortTempDir(t) + open := filepath.Join(base, "open") + if err := os.Mkdir(open, 0o700); err != nil { + t.Fatal(err) + } + if err := os.Chmod(open, 0o755); err != nil { //nolint:gosec // G302: the test needs a directory that others can read. + t.Fatal(err) + } + private := filepath.Join(base, "private") + if err := os.Mkdir(private, 0o700); err != nil { + t.Fatal(err) + } + link := filepath.Join(base, "link") + if err := os.Symlink(private, link); err != nil { + t.Fatal(err) + } + file := filepath.Join(base, "file") + if err := os.WriteFile(file, nil, 0o600); err != nil { + t.Fatal(err) + } + tests := []struct { + dir string + want string + }{ + {open, "has mode 0755; the directory of the socket must have mode 0700"}, + {link, "is a symbolic link"}, + {file, "is not a directory"}, + {filepath.Join(base, "missing", "nodkey"), "no such file or directory"}, + } + for _, tt := range tests { + _, err := listenAgent(t.Context(), filepath.Join(tt.dir, socketName), uid(t), noDirCheck) + if err == nil || !strings.Contains(err.Error(), tt.want) { + t.Errorf("%s: %v; want %q", tt.dir, err, tt.want) + } + } + mustMode(t, open, fs.ModeDir, 0o755) + if _, err := os.Lstat(filepath.Join(private, socketName)); !errors.Is(err, fs.ErrNotExist) { + t.Error("a socket was made through the link") + } + // Another user would own the directory: the user ID differs. + if _, err := listenAgent(t.Context(), filepath.Join(private, socketName), uid(t)+1, noDirCheck); err == nil || + !strings.Contains(err.Error(), "must be owned by the user") { + t.Errorf("a directory of another user: %v", err) + } +} + +// TestListenAgentStaleSocket: a socket that no program serves is left +// from an agent that ended without removing it; the new agent removes +// it. A socket that a program serves, and anything that is not a socket, +// stay, and the agent does not start. +func TestListenAgentStaleSocket(t *testing.T) { + t.Parallel() + dir := filepath.Join(shortTempDir(t), "nodkey") + if err := os.Mkdir(dir, 0o700); err != nil { + t.Fatal(err) + } + path := filepath.Join(dir, socketName) + listen := func() *net.UnixListener { + ln, err := (&net.ListenConfig{}).Listen(t.Context(), "unix", path) + if err != nil { + t.Fatal(err) + } + ul, ok := ln.(*net.UnixListener) + if !ok { + t.Fatal("not a Unix listener") + } + ul.SetUnlinkOnClose(false) + return ul + } + + // A program serves the socket. + live := listen() + if _, err := listenAgent(t.Context(), path, uid(t), noDirCheck); err == nil || + !strings.Contains(err.Error(), "another program serves this socket") { + t.Errorf("a socket that a program serves: %v", err) + } + c, err := (&net.Dialer{}).DialContext(t.Context(), "unix", path) + if err != nil { + t.Fatalf("the socket of the other program was removed: %v", err) + } + _ = c.Close() + + // The program ended without removing it. A child that another test + // forks at this moment holds the socket until it runs its program, so + // wait until the socket refuses connections. + _ = live.Close() + waitFor(t, "the socket to refuse connections", func() bool { + c, err := (&net.Dialer{}).DialContext(t.Context(), "unix", path) + if err == nil { + _ = c.Close() + } + return errors.Is(err, syscall.ECONNREFUSED) + }) + sock, err := listenAgain(t, path) + if err != nil { + t.Fatalf("a stale socket: %v", err) + } + sock.close() + + // Anything else stays. + if err := os.WriteFile(path, []byte("x"), 0o600); err != nil { + t.Fatal(err) + } + if _, err := listenAgain(t, path); err == nil || + !strings.Contains(err.Error(), "is there, and it is not a socket") { + t.Errorf("a file at the socket's path: %v", err) + } + mustMode(t, path, 0, 0o600) + if err := os.Remove(path); err != nil { + t.Fatal(err) + } + if err := os.Symlink(filepath.Join(dir, "elsewhere"), path); err != nil { + t.Fatal(err) + } + if _, err := listenAgain(t, path); err == nil || + !strings.Contains(err.Error(), "is there, and it is not a socket") { + t.Errorf("a symbolic link at the socket's path: %v", err) + } + mustMode(t, path, fs.ModeSymlink, 0o777) +} + +// listenAgain is listenAgent on a path whose lock the test held before. +// A child that another test forks keeps a copy of each open file until it +// runs its program, and with it the lock: so listenAgain tries again for a +// while when the lock is still held. +func listenAgain(t *testing.T, path string) (*agentSocket, error) { + t.Helper() + deadline := time.Now().Add(waitLimit) + for { + s, err := listenAgent(t.Context(), path, uid(t), noDirCheck) + if err == nil || !strings.Contains(err.Error(), "another nodkey ssh-agent uses this socket") || + time.Now().After(deadline) { + return s, err + } + time.Sleep(time.Millisecond) + } +} + +// TestAgentSocketCloseKeepsAnotherSocket: close removes the socket only +// while it is the one that the agent made. A socket that another program +// put at the path later stays. +func TestAgentSocketCloseKeepsAnotherSocket(t *testing.T) { + t.Parallel() + path := filepath.Join(shortTempDir(t), "nodkey", socketName) + sock, err := listenAgent(t.Context(), path, uid(t), noDirCheck) + if err != nil { + t.Fatal(err) + } + if err := os.Remove(path); err != nil { + t.Fatal(err) + } + other, err := (&net.ListenConfig{}).Listen(t.Context(), "unix", path) + if err != nil { + t.Fatal(err) + } + defer func() { _ = other.Close() }() + sock.close() + if _, err := os.Lstat(path); err != nil { + t.Errorf("the other socket was removed: %v", err) + } +} + +// TestListenAgentLock: an agent holds the lock of its socket while it +// runs. A second agent on the same path stops at once, with a clear +// error, and leaves the first one's socket. Of two agents that start at +// the same time, one gets the socket. The lock file stays, with mode +// 0600, and the next agent can use it. +func TestListenAgentLock(t *testing.T) { + t.Parallel() + path := filepath.Join(shortTempDir(t), "nodkey", socketName) + first, err := listenAgent(t.Context(), path, uid(t), noDirCheck) + if err != nil { + t.Fatal(err) + } + _, err = listenAgent(t.Context(), path, uid(t), noDirCheck) + if want := path + ": another nodkey ssh-agent uses this socket; stop it, or give another --socket"; err == nil || + err.Error() != want { + t.Errorf("a second agent: %v; want %q", err, want) + } + c, err := (&net.Dialer{}).DialContext(t.Context(), "unix", path) + if err != nil { + t.Fatalf("the second agent removed the socket of the first: %v", err) + } + _ = c.Close() + first.close() + mustMode(t, path+".lock", 0, 0o600) + again, err := listenAgain(t, path) + if err != nil { + t.Fatalf("an agent after the first one ended: %v", err) + } + again.close() + + // Two agents at the same time, many times: each time one of them + // gets the socket, and it works. + for range 20 { + path := filepath.Join(shortTempDir(t), "nodkey", socketName) + results := make(chan *agentSocket, 2) + for range 2 { + go func() { + s, err := listenAgent(t.Context(), path, uid(t), noDirCheck) + if err != nil && !strings.Contains(err.Error(), "another nodkey ssh-agent uses this socket") { + t.Errorf("an agent that lost the race: %v", err) + } + results <- s + }() + } + var won []*agentSocket + for range 2 { + if s := <-results; s != nil { + won = append(won, s) + } + } + if len(won) != 1 { + t.Fatalf("%d agents got the socket; want 1", len(won)) + } + c, err := (&net.Dialer{}).DialContext(t.Context(), "unix", path) + if err != nil { + t.Fatalf("the socket of the agent that won does not work: %v", err) + } + _ = c.Close() + won[0].close() + } +} + +// TestLockSocketRefuses: the lock file must be a regular file of the +// user, not a symbolic link or a FIFO; nodkey does not wait on a FIFO. +func TestLockSocketRefuses(t *testing.T) { + t.Parallel() + dir := shortTempDir(t) + link := filepath.Join(dir, "link.sock") + if err := os.Symlink(filepath.Join(dir, "elsewhere"), link+".lock"); err != nil { + t.Fatal(err) + } + if _, err := lockSocket(link, uid(t)); err == nil { + t.Error("lockSocket took a symbolic link") + } + if _, err := os.Lstat(filepath.Join(dir, "elsewhere")); !errors.Is(err, fs.ErrNotExist) { + t.Error("lockSocket made the target of the link") + } + fifo := filepath.Join(dir, "fifo.sock") + if err := syscall.Mkfifo(fifo+".lock", 0o600); err != nil { + t.Fatal(err) + } + if _, err := lockSocket(fifo, uid(t)); err == nil || !strings.Contains(err.Error(), "is not a regular file") { + t.Errorf("a FIFO: %v", err) + } + if _, err := lockSocket(filepath.Join(dir, "other.sock"), uid(t)+1); err == nil { + t.Error("lockSocket took a lock file of another user") + } +} + +// agentCheckedDir returns a new directory with a short path that passes +// config.CheckParentDirs, such as one in $XDG_RUNTIME_DIR or in the home +// directory. The system temporary directory does not pass: everyone can +// write to it. The test is skipped when there is no such directory. +func agentCheckedDir(t *testing.T) string { + t.Helper() + for _, base := range []string{os.Getenv("XDG_RUNTIME_DIR"), os.Getenv("HOME")} { + if base == "" || !filepath.IsAbs(base) || len(base) > 40 { + continue + } + if err := config.CheckParentDirs(filepath.Join(base, "x")); err != nil { + continue + } + dir, err := os.MkdirTemp(base, "nodkey-test-") + if err != nil { + continue + } + t.Cleanup(func() { _ = os.RemoveAll(dir) }) //nolint:gosec // G703: dir is the new directory that MkdirTemp made. + if err := os.Chmod(dir, 0o700); err != nil { //nolint:gosec // G302: a directory needs the execute bit. + t.Fatal(err) + } + return dir + } + t.Skip("no short directory that passes config.CheckParentDirs") + return "" +} + +// TestListenAgentChecksTheDirectories: with config.CheckParentDirs, the +// directories on the way to the socket must be owned by the user or +// root, and others must not be able to write to them. So the system +// temporary directory is refused, and so is a directory on the way that +// others can write to. +func TestListenAgentChecksTheDirectories(t *testing.T) { + t.Parallel() + base := agentCheckedDir(t) + sock, err := listenAgent(t.Context(), filepath.Join(base, "nodkey", socketName), uid(t), config.CheckParentDirs) + if err != nil { + t.Fatalf("a directory that passes the checks: %v", err) + } + sock.close() + shared := filepath.Join(base, "shared") + if err := os.Mkdir(shared, 0o700); err != nil { + t.Fatal(err) + } + if err := os.Chmod(shared, 0o777); err != nil { //nolint:gosec // G302: the test needs a directory that others can write to. + t.Fatal(err) + } + paths := []string{filepath.Join(shared, "nodkey", socketName)} + if fi, err := os.Stat(os.TempDir()); err == nil && fi.Mode().Perm()&0o002 != 0 { + paths = append(paths, filepath.Join(shortTempDir(t), "nodkey", socketName)) + } + for _, path := range paths { + _, err := listenAgent(t.Context(), path, uid(t), config.CheckParentDirs) + if err == nil || !strings.Contains(err.Error(), "group or others can write to this directory") { + t.Errorf("%s: %v", path, err) + } + if _, err := os.Lstat(filepath.Dir(path)); !errors.Is(err, fs.ErrNotExist) { + t.Errorf("%s: the directory of the socket was made", path) + } + } +} + +// TestCheckSocketPath: the socket path is absolute, clean, not in /, and +// short enough for a Unix socket. +func TestCheckSocketPath(t *testing.T) { + t.Parallel() + for _, tt := range []struct { + path string + ok bool + }{ + {"/run/user/1000/nodkey/agent.sock", true}, + {"/" + strings.Repeat("a", maxSocketPath-3) + "/s", true}, + {"/" + strings.Repeat("a", maxSocketPath-2) + "/s", false}, + {"agent.sock", false}, + {"./agent.sock", false}, + {"/run/user/1000/../1001/agent.sock", false}, + {"/run/user/1000//agent.sock", false}, + {"/run/user/1000/nodkey/", false}, + {"/agent.sock", false}, + {"/run/a\x00b/agent.sock", false}, + } { + if err := checkSocketPath(tt.path); (err == nil) != tt.ok { + t.Errorf("%q: %v", tt.path, err) + } + } +} + +// TestDefaultSocket: the default socket is $XDG_RUNTIME_DIR/nodkey/agent.sock +// (DESIGN.md section 6). Without XDG_RUNTIME_DIR, the user must give +// --socket. +func TestDefaultSocket(t *testing.T) { + t.Parallel() + env := func(v string) func(string) string { + return func(name string) string { + if name == "XDG_RUNTIME_DIR" { + return v + } + return "" + } + } + if got, err := defaultSocket(env("/run/user/1000")); err != nil || got != "/run/user/1000/nodkey/agent.sock" { + t.Errorf("defaultSocket = %q, %v", got, err) + } + for _, v := range []string{"", "run/user/1000"} { + if _, err := defaultSocket(env(v)); err == nil || !strings.Contains(err.Error(), "--socket") { + t.Errorf("XDG_RUNTIME_DIR=%q: %v", v, err) + } + } +} diff --git a/internal/client/sshagent_test.go b/internal/client/sshagent_test.go new file mode 100644 index 0000000..79c88fc --- /dev/null +++ b/internal/client/sshagent_test.go @@ -0,0 +1,1630 @@ +package client + +import ( + "bytes" + "context" + "crypto/rand" + "crypto/rsa" + "encoding/binary" + "errors" + "fmt" + "io" + "log" + "math" + "net" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "runtime" + "slices" + "strings" + "syscall" + "testing" + "time" + + "golang.org/x/crypto/ssh" + "golang.org/x/crypto/ssh/agent" + + "github.com/code6x/nodkey/internal/api" + "github.com/code6x/nodkey/internal/audit" + "github.com/code6x/nodkey/internal/config" + "github.com/code6x/nodkey/internal/transport" +) + +// agentSigner logs in with a key of the agent, as OpenSSH does: right +// before the agent signs, it sends the session-bind of the key exchange +// to the agent, on the same connection. +type agentSigner struct { + ag agent.ExtendedAgent + pub ssh.PublicKey + bind func() []byte +} + +func (s *agentSigner) PublicKey() ssh.PublicKey { return s.pub } + +func (s *agentSigner) Sign(r io.Reader, data []byte) (*ssh.Signature, error) { + return s.SignWithAlgorithm(r, data, s.pub.Type()) +} + +func (s *agentSigner) SignWithAlgorithm(_ io.Reader, data []byte, algorithm string) (*ssh.Signature, error) { + if _, err := s.ag.Extension(sessionBindRequest, s.bind()); err != nil { + return nil, err + } + var flags agent.SignatureFlags + switch algorithm { + case ssh.KeyAlgoRSASHA256: + flags = agent.SignatureFlagRsaSha256 + case ssh.KeyAlgoRSASHA512: + flags = agent.SignatureFlagRsaSha512 + } + return s.ag.SignWithFlags(s.pub, data, flags) +} + +// login logs in to srv as user, with the first key that the agent ag +// lists, as OpenSSH does with an agent. Give it a new agent connection. +// It may run in a goroutine of its own: it only returns errors. +func login(ctx context.Context, ag agent.ExtendedAgent, srv *sshServer, user string) error { + keys, err := ag.List() + if err != nil { + return err + } + if len(keys) == 0 { + return errors.New("the agent lists no key") + } + pub, err := ssh.ParsePublicKey(keys[0].Blob) + if err != nil { + return err + } + signer := &agentSigner{ag: ag, pub: pub, bind: func() []byte { + b := srv.binds() + return b[len(b)-1] + }} + nc, err := (&net.Dialer{Timeout: waitLimit}).DialContext(ctx, "tcp", srv.addr) + if err != nil { + return err + } + defer func() { _ = nc.Close() }() + cfg := &ssh.ClientConfig{User: user, Auth: []ssh.AuthMethod{ssh.PublicKeys(signer)}, + HostKeyCallback: ssh.FixedHostKey(srv.hostKey)} + conn, chans, reqs, err := ssh.NewClientConn(nc, srv.addr, cfg) + if err != nil { + return err + } + return ssh.NewClient(conn, chans, reqs).Close() +} + +// TestAgentLogin logs in to an in-process SSH server through the agent +// and the real broker. Only a login that the person approves works. The +// agent prints the short code of the request, and why a request failed. +func TestAgentLogin(t *testing.T) { + t.Parallel() + tests := []struct { + name string + answer audit.Answer // "" lets the request expire + events string + }{ + {"approved", audit.AnswerApproveOnce, "request decision:approve_once signed"}, + {"denied", audit.AnswerDeny, "request decision:deny denied:approver"}, + {"expired", "", "request denied:expired"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + host, _ := newSSHKey(t) + sb := newSSHBroker(t, map[string]ssh.PublicKey{"web-1": host.PublicKey()}) + srv := startSSHServer(t, host, sb.user.PublicKey()) + au := startAgent(t, sb.client(t), agentOptions{}) + sb.approver.Auto(tt.answer) + ag := au.client(t) + done := make(chan error, 1) + go func() { done <- login(t.Context(), ag, srv, "deploy") }() + asked := sb.waitAsked(t, 1)[0] + if tt.answer == "" { + sb.clock.Advance(sshTimeout) + } + err := <-done + code := asked.Prompt.ShortCode + waiting := "nodkey: waiting for approval (request " + code + ")" + want := []string{waiting} + if tt.answer == audit.AnswerApproveOnce { + if err != nil || srv.loginCount() != 1 { + t.Fatalf("the login failed: %v; agent lines %q", err, au.lines()) + } + } else { + if err == nil || srv.loginCount() != 0 { + t.Fatalf("the login worked, %v", err) + } + want = append(want, "nodkey: no signature (request "+code+"): denied") + } + p := asked.Prompt + if p.Key != "deploy" || !slices.Equal(p.Hosts, []string{"web-1"}) || p.RemoteUser != "deploy" || + p.Client != "laptop" { + t.Errorf("the message shows %+v", p) + } + waitFor(t, "the agent's lines", func() bool { return len(au.lines()) >= len(want) }) + if got := au.lines(); !slices.Equal(got, want) { + t.Errorf("the agent wrote %q, want %q", got, want) + } + if got := eventNames(sb.audit.waitFor(t, len(strings.Fields(tt.events)))); got != tt.events { + t.Errorf("audit entries %q, want %q", got, tt.events) + } + }) + } +} + +// TestAgentLists: the agent lists the keys of the broker's answer, each +// with its exact public key and its name as the comment. Each list asks +// the broker once. +func TestAgentLists(t *testing.T) { + t.Parallel() + deploy, _ := newSSHKey(t) + backup, _ := newSSHKey(t) + fb := newFakeSSHBroker(t, map[string]ssh.PublicKey{"deploy": deploy.PublicKey(), "backup": backup.PublicKey()}, + nil) + au := startAgent(t, fb.b, agentOptions{}) + ag := au.client(t) + for i := range 2 { + keys, err := ag.List() + if err != nil { + t.Fatal(err) + } + got := map[string][]byte{} + for _, k := range keys { + got[k.Comment] = k.Blob + } + if len(keys) != 2 || !bytes.Equal(got["deploy"], deploy.PublicKey().Marshal()) || + !bytes.Equal(got["backup"], backup.PublicKey().Marshal()) { + t.Errorf("the agent lists %v", keys) + } + if n := fb.requests.Load(); n != int32(i+1) { + t.Errorf("the broker got %d requests after %d lists", n, i+1) + } + } + if lines := au.lines(); len(lines) != 0 { + t.Errorf("the agent wrote %q", lines) + } +} + +// TestAgentListFailures: a list request that the broker does not answer +// with a valid list gets SSH_AGENT_FAILURE, and one line says why. +func TestAgentListFailures(t *testing.T) { + t.Parallel() + pub, _ := newSSHKey(t) + cert := &ssh.Certificate{Key: pub.PublicKey(), CertType: ssh.UserCert, ValidBefore: ssh.CertTimeInfinity} + if err := cert.SignCert(rand.Reader, pub); err != nil { + t.Fatal(err) + } + keysJSON := func(name string, blob []byte) string { + return string(mustJSON(t, api.KeysAnswer{Keys: []api.Key{{Name: name, PublicKey: blob}}})) + } + tests := []struct { + name string + handler http.HandlerFunc + want string + }{ + {"403", answer(http.StatusForbidden, `{"error":"Forbidden"}`), "denied"}, + {"503", answer(http.StatusServiceUnavailable, `{"error":"Service Unavailable"}`), + "the broker answered 503 Service Unavailable"}, + {"not JSON", answer(http.StatusOK, "keys"), "the answer of the broker is not valid"}, + {"an unknown field", answer(http.StatusOK, `{"keys":[],"more":1}`), "the answer of the broker is not valid"}, + {"a certificate", answer(http.StatusOK, keysJSON("deploy", cert.Marshal())), + "the answer of the broker has a key that is not valid"}, + {"not a key", answer(http.StatusOK, keysJSON("deploy", []byte("not a key"))), + "the answer of the broker has a key that is not valid"}, + {"a name with a line break", answer(http.StatusOK, keysJSON("deploy\naudit anchor 1:x", pub.PublicKey().Marshal())), + "the answer of the broker has a key that is not valid"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + fb := newFakeBroker(t, tt.handler) + au := startAgent(t, fb.b, agentOptions{}) + ag := au.client(t) + if keys, err := ag.List(); err == nil { + t.Fatalf("the agent listed %v", keys) + } + want := []string{"nodkey: no key list: " + tt.want} + if got := au.lines(); !slices.Equal(got, want) { + t.Errorf("the agent wrote %q, want %q", got, want) + } + if fb.requests.Load() != 1 { + t.Errorf("the broker got %d requests, want 1", fb.requests.Load()) + } + }) + } +} + +// TestAgentBrokerDown: without a broker, ssh gets no key and no +// signature, and a line says why. +func TestAgentBrokerDown(t *testing.T) { + t.Parallel() + ln, err := (&net.ListenConfig{}).Listen(t.Context(), "tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + addr := ln.Addr().String() + _ = ln.Close() + b, err := newBroker(addr, newTLSKey(t).cert, newTLSKey(t).pin) + if err != nil { + t.Fatal(err) + } + au := startAgent(t, b, agentOptions{}) + ag, c := au.client(t), au.conn(t) + if _, err := ag.List(); err == nil { + t.Error("the agent listed keys") + } + pub, _ := newSSHKey(t) + if !bindAgent(t, c, []byte("bind")) { + t.Fatal("the session-bind was refused") + } + if sig := signatureOf(t, rawCall(t, c, signRequestMessage(pub.PublicKey().Marshal(), []byte("data"), 0))); sig != nil { + t.Error("the agent signed") + } + // Another test may take the free port at once: then the TLS handshake + // fails instead. + want := "cannot connect to the broker at " + addr + ": " + if got := au.lines(); len(got) != 2 || !strings.HasPrefix(got[0], "nodkey: no key list: "+want) || + !strings.HasPrefix(got[1], "nodkey: no signature: "+want) { + t.Errorf("the agent wrote %q", got) + } +} + +// signOnce sends one session-bind and one sign request for key on a new +// connection, and returns the signature, or nil for SSH_AGENT_FAILURE. +func signOnce(t *testing.T, au *agentUnderTest, key ssh.PublicKey, data []byte, flags uint32) *ssh.Signature { + t.Helper() + c := au.conn(t) + if !bindAgent(t, c, []byte("a session-bind")) { + t.Fatal("the session-bind was refused") + } + return signatureOf(t, rawCall(t, c, signRequestMessage(key.Marshal(), data, flags))) +} + +// TestAgentSignAnswers: only a final 200 whose body passes +// api.ReadAnswer, with a signature that fits the key and the flags and is +// valid for the data, goes to ssh. Every other answer is +// SSH_AGENT_FAILURE, and one line says why, as nodkey run says it: denied +// for 403 and 429, and the answer for the others. The agent sends each +// request once, and never writes an answer body or a header. +func TestAgentSignAnswers(t *testing.T) { + t.Parallel() + deploy, _ := newSSHKey(t) + other, _ := newSSHKey(t) + peerText := "PEER\nnodkey: approved (exit 0)" + sigOf := func(signer ssh.AlgorithmSigner, data []byte) *ssh.Signature { + sig, err := signer.SignWithAlgorithm(rand.Reader, data, "") + if err != nil { + t.Fatal(err) + } + return sig + } + write := func(status int, v any) func(http.ResponseWriter, *api.SignRequest) { + return func(w http.ResponseWriter, _ *api.SignRequest) { + w.Header().Set("X-Note", peerText) + _ = transport.WriteJSON(w, status, v) + } + } + tests := []struct { + name string + sign func(http.ResponseWriter, *api.SignRequest) + want string // the rest of the line after "nodkey: no signature: "; "" for a valid signature + }{ + {"valid", signWith(deploy, "", ""), ""}, + {"403", write(http.StatusForbidden, api.ErrorAnswer{Error: peerText}), "denied"}, + {"429", write(http.StatusTooManyRequests, api.ErrorAnswer{Error: "Too Many Requests"}), + "denied by a limit of the broker"}, + {"400", write(http.StatusBadRequest, api.ErrorAnswer{Error: "Bad Request"}), + "the broker answered 400 Bad Request"}, + {"503", write(http.StatusServiceUnavailable, api.ErrorAnswer{Error: "Service Unavailable"}), + "the broker answered 503 Service Unavailable"}, + {"201 with a valid signature", func(w http.ResponseWriter, req *api.SignRequest) { + sig := sigOf(deploy, req.Data) + _ = transport.WriteJSON(w, http.StatusCreated, api.SignAnswer{Format: sig.Format, Blob: sig.Blob}) + }, "the broker answered 201 Created"}, + {"a redirect", func(w http.ResponseWriter, _ *api.SignRequest) { + w.Header().Set("Location", "https://192.0.2.10:7743/v1/sign") + w.WriteHeader(http.StatusTemporaryRedirect) + }, "the broker answered 307 Temporary Redirect"}, + {"not JSON", func(w http.ResponseWriter, _ *api.SignRequest) { + _ = transport.WriteJSONBody(w, http.StatusOK, []byte(`"`+strings.ReplaceAll(peerText, "\n", " ")+`"`)) + }, "the answer of the broker is not valid"}, + {"an unknown field", func(w http.ResponseWriter, req *api.SignRequest) { + sig := sigOf(deploy, req.Data) + _ = transport.WriteJSON(w, http.StatusOK, map[string]any{"format": sig.Format, "blob": sig.Blob, "x": 1}) + }, "the answer of the broker is not valid"}, + {"no blob", func(w http.ResponseWriter, _ *api.SignRequest) { + _ = transport.WriteJSON(w, http.StatusOK, map[string]any{"format": ssh.KeyAlgoED25519}) + }, "the answer of the broker is not valid"}, + {"larger than api.MaxAnswer", func(w http.ResponseWriter, req *api.SignRequest) { + sig := sigOf(deploy, req.Data) + body := string(mustJSON(t, api.SignAnswer{Format: sig.Format, Blob: sig.Blob})) + + strings.Repeat(" ", api.MaxAnswer) + _ = transport.WriteJSONBody(w, http.StatusOK, []byte(body)) + }, "the answer of the broker is not valid"}, + {"a signature of another key", signWith(other, "", ""), + "the signature of the broker is not a valid signature of the data"}, + {"a signature of other data", func(w http.ResponseWriter, req *api.SignRequest) { + sig := sigOf(deploy, append(bytes.Clone(req.Data), 'x')) + _ = transport.WriteJSON(w, http.StatusOK, api.SignAnswer{Format: sig.Format, Blob: sig.Blob}) + }, "the signature of the broker is not a valid signature of the data"}, + {"an empty blob", write(http.StatusOK, api.SignAnswer{Format: ssh.KeyAlgoED25519, Blob: []byte{}}), + "the signature of the broker is not a valid signature of the data"}, + {"another format", signWith(deploy, "", ssh.KeyAlgoRSASHA256), + "the signature of the broker does not fit the key and the sign flags"}, + {"a format in another case", signWith(deploy, "", "SSH-ED25519"), + "the signature of the broker does not fit the key and the sign flags"}, + {"a dropped connection", func(http.ResponseWriter, *api.SignRequest) { panic(http.ErrAbortHandler) }, + "broke before the answer"}, + {"too many informational answers", func(w http.ResponseWriter, req *api.SignRequest) { + for range maxInformational + 1 { + if err := transport.WriteProcessing(w, http.Header{api.ShortCodeHeader: {"7qd2"}}); err != nil { + return + } + } + signWith(deploy, "", "")(w, req) + }, "(request 7qd2): the broker sent more than 4 informational answers"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + fb := newFakeSSHBroker(t, map[string]ssh.PublicKey{"deploy": deploy.PublicKey()}, tt.sign) + au := startAgent(t, fb.b, agentOptions{}) + data := []byte("the data to sign " + rand.Text()) + sig := signOnce(t, au, deploy.PublicKey(), data, 0) + lines := au.lines() + if tt.want == "" { + if sig == nil || deploy.PublicKey().Verify(data, sig) != nil || len(lines) != 0 { + t.Fatalf("no valid signature: %v; lines %q", sig, lines) + } + return + } + if sig != nil { + t.Fatalf("the agent gave ssh a signature") + } + if tt.name == "too many informational answers" { + if len(lines) != 2 || lines[0] != "nodkey: waiting for approval (request 7qd2)" || + lines[1] != "nodkey: no signature "+tt.want { + t.Errorf("the agent wrote %q", lines) + } + } else if len(lines) != 1 || !strings.HasPrefix(lines[0], "nodkey: no signature: ") || + !strings.Contains(lines[0], tt.want) { + t.Errorf("the agent wrote %q; want one line with %q", lines, tt.want) + } + if strings.Contains(au.log.String(), "PEER") { + t.Errorf("the agent wrote text of the answer: %q", lines) + } + if n := fb.requests.Load(); n != 1 { + t.Errorf("the broker got %d requests, want 1", n) + } + }) + } +} + +// TestAgentSignFormats: the format of the broker's signature must be the +// one that the key and the sign flags ask for. RSA needs the flag of +// SHA-256 or SHA-512, and SHA-1 never passes; Ed25519 takes no flags. +func TestAgentSignFormats(t *testing.T) { + t.Parallel() + rsaKey, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatal(err) + } + rs, err := ssh.NewSignerFromSigner(rsaKey) + if err != nil { + t.Fatal(err) + } + rsaSigner, ok := rs.(ssh.AlgorithmSigner) + if !ok { + t.Fatal("the RSA signer cannot choose an algorithm") + } + ed, _ := newSSHKey(t) + tests := []struct { + name string + signer ssh.AlgorithmSigner + algorithm string // that the broker signs with + flags uint32 + ok bool + }{ + {"rsa-sha2-256", rsaSigner, ssh.KeyAlgoRSASHA256, uint32(agent.SignatureFlagRsaSha256), true}, + {"rsa-sha2-512", rsaSigner, ssh.KeyAlgoRSASHA512, uint32(agent.SignatureFlagRsaSha512), true}, + {"rsa-sha2-512 for the flag of SHA-256", rsaSigner, ssh.KeyAlgoRSASHA512, + uint32(agent.SignatureFlagRsaSha256), false}, + {"ssh-rsa (SHA-1) without a flag", rsaSigner, ssh.KeyAlgoRSA, 0, false}, + {"RSA with both flags", rsaSigner, ssh.KeyAlgoRSASHA256, + uint32(agent.SignatureFlagRsaSha256 | agent.SignatureFlagRsaSha512), false}, + {"ssh-ed25519", ed, ssh.KeyAlgoED25519, 0, true}, + {"ssh-ed25519 with a flag", ed, ssh.KeyAlgoED25519, uint32(agent.SignatureFlagRsaSha256), false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + fb := newFakeSSHBroker(t, nil, signWith(tt.signer, tt.algorithm, "")) + au := startAgent(t, fb.b, agentOptions{}) + data := []byte("data " + rand.Text()) + sig := signOnce(t, au, tt.signer.PublicKey(), data, tt.flags) + if !tt.ok { + if sig != nil { + t.Fatalf("the agent gave ssh a signature in %s", sig.Format) + } + want := "nodkey: no signature: the signature of the broker does not fit the key and the sign flags" + if got := au.lines(); !slices.Equal(got, []string{want}) { + t.Errorf("the agent wrote %q", got) + } + return + } + if sig == nil || sig.Format != tt.algorithm || tt.signer.PublicKey().Verify(data, sig) != nil { + t.Fatalf("no valid signature in %s: %v; lines %q", tt.algorithm, sig, au.lines()) + } + }) + } +} + +// TestSignatureFormat: the formats that the agent takes. +func TestSignatureFormat(t *testing.T) { + t.Parallel() + sha256, sha512 := uint32(agent.SignatureFlagRsaSha256), uint32(agent.SignatureFlagRsaSha512) + tests := []struct { + keyType string + flags uint32 + want string // "" when no format fits + }{ + {ssh.KeyAlgoED25519, 0, ssh.KeyAlgoED25519}, + {ssh.KeyAlgoECDSA256, 0, ssh.KeyAlgoECDSA256}, + {ssh.KeyAlgoECDSA384, 0, ssh.KeyAlgoECDSA384}, + {ssh.KeyAlgoECDSA521, 0, ssh.KeyAlgoECDSA521}, + {ssh.KeyAlgoECDSA256, sha256, ""}, + {ssh.KeyAlgoRSA, sha256, ssh.KeyAlgoRSASHA256}, + {ssh.KeyAlgoRSA, sha512, ssh.KeyAlgoRSASHA512}, + {ssh.KeyAlgoRSA, 0, ""}, + {ssh.KeyAlgoRSA, sha256 | sha512, ""}, + {ssh.KeyAlgoRSA, 1, ""}, + {ssh.CertAlgoED25519v01, 0, ""}, + {ssh.KeyAlgoSKED25519, 0, ""}, + {"ssh-dss", 0, ""}, + } + for _, tt := range tests { + got, ok := signatureFormat(tt.keyType, tt.flags) + if got != tt.want && ok || ok != (tt.want != "") { + t.Errorf("signatureFormat(%s, %d) = %q, %v; want %q", tt.keyType, tt.flags, got, ok, tt.want) + } + } +} + +// TestAgentShortCode: the agent prints the short code of the first +// 102 Processing once, and only a code that has the form of a short code, +// as nodkey run does. A code in the final answer is not printed. +func TestAgentShortCode(t *testing.T) { + t.Parallel() + deploy, _ := newSSHKey(t) + informational := func(w http.ResponseWriter, status int, code string) { + w.Header().Set(api.ShortCodeHeader, code) + w.WriteHeader(status) + w.Header().Del(api.ShortCodeHeader) + } + tests := []struct { + name string + first func(http.ResponseWriter) + final string + want string + }{ + {"one", func(w http.ResponseWriter) { informational(w, http.StatusProcessing, "k3f9") }, "", "k3f9"}, + {"two", func(w http.ResponseWriter) { + informational(w, http.StatusProcessing, "k3f9") + informational(w, http.StatusProcessing, "7qd2") + }, "", "k3f9"}, + {"not a short code", func(w http.ResponseWriter) { + informational(w, http.StatusProcessing, "k3f9\nnodkey: approved") + }, "", ""}, + {"not 102", func(w http.ResponseWriter) { informational(w, http.StatusEarlyHints, "k3f9") }, "", ""}, + {"in the final answer", func(http.ResponseWriter) {}, "k3f9", ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + fb := newFakeSSHBroker(t, nil, func(w http.ResponseWriter, _ *api.SignRequest) { + tt.first(w) + if tt.final != "" { + w.Header().Set(api.ShortCodeHeader, tt.final) + } + transport.WriteError(w, http.StatusForbidden) + }) + au := startAgent(t, fb.b, agentOptions{}) + if sig := signOnce(t, au, deploy.PublicKey(), []byte("data"), 0); sig != nil { + t.Fatal("the agent gave ssh a signature") + } + want := []string{"nodkey: no signature: denied"} + if tt.want != "" { + want = []string{"nodkey: waiting for approval (request " + tt.want + ")", + "nodkey: no signature (request " + tt.want + "): denied"} + } + if got := au.lines(); !slices.Equal(got, want) { + t.Errorf("the agent wrote %q, want %q", got, want) + } + }) + } +} + +// TestAgentSignDeadline: a sign request waits at most its deadline. Then +// ssh gets SSH_AGENT_FAILURE, the connection to the broker closes, so the +// broker cancels the request, and the agent does not ask again. +func TestAgentSignDeadline(t *testing.T) { + t.Parallel() + deploy, _ := newSSHKey(t) + hungUp := make(chan struct{}) + fb := newFakeBroker(t, func(w http.ResponseWriter, r *http.Request) { + if _, err := transport.ReadJSON[api.SignRequest](w, r); err != nil { + return + } + if err := transport.WriteProcessing(w, http.Header{api.ShortCodeHeader: {"k3f9"}}); err != nil { + return + } + _ = http.NewResponseController(w).Flush() + <-r.Context().Done() + close(hungUp) + }) + const timeout = time.Second + au := startAgent(t, fb.b, agentOptions{signTimeout: timeout}) + start := time.Now() + if sig := signOnce(t, au, deploy.PublicKey(), []byte("data"), 0); sig != nil { + t.Fatal("the agent gave ssh a signature") + } + if d := time.Since(start); d < timeout || d > waitLimit { + t.Errorf("the request ended after %s; want its deadline, %s", d, timeout) + } + select { + case <-hungUp: + case <-time.After(waitLimit): + t.Fatal("the broker did not see the client hang up") + } + const last = "no answer from the broker within 1s" + switch got := au.lines(); { + case slices.Equal(got, []string{"nodkey: waiting for approval (request k3f9)", + "nodkey: no signature (request k3f9): " + last}): + case slices.Equal(got, []string{"nodkey: no signature: " + last}): + // A slow machine: the deadline came before the 102 Processing. + default: + t.Errorf("the agent wrote %q", got) + } + if n := fb.requests.Load(); n != 1 { + t.Errorf("the broker got %d requests, want 1", n) + } +} + +// TestSignTimeoutIsBelowLoginGraceTime: the deadline of a sign request is +// longer than the longest ssh_timeout of the broker, so that a late tap +// still counts, and shorter than the LoginGraceTime of sshd (120 seconds +// by default), after which a signature cannot log in. +func TestSignTimeoutIsBelowLoginGraceTime(t *testing.T) { + t.Parallel() + const loginGraceTime = 120 * time.Second + if signTimeout <= config.MaxSSHTimeout || signTimeout >= loginGraceTime { + t.Errorf("signTimeout is %s; want more than %s and less than %s", signTimeout, config.MaxSSHTimeout, + loginGraceTime) + } + if keysTimeout >= loginGraceTime { + t.Errorf("keysTimeout is %s", keysTimeout) + } +} + +// TestAgentSendsTheRequestAsItIs: the agent sends the exact bytes of the +// key, the data and the flags that ssh sent, and every session-bind of +// that connection, in order, without the extension name, and the binds of +// no other connection. It answers each session-bind with success. +func TestAgentSendsTheRequestAsItIs(t *testing.T) { + t.Parallel() + fb := newFakeSSHBroker(t, nil, func(w http.ResponseWriter, _ *api.SignRequest) { + transport.WriteError(w, http.StatusForbidden) + }) + au := startAgent(t, fb.b, agentOptions{}) + one := au.conn(t) + two := au.conn(t) + b1, b2, b3 := []byte("bind one "+rand.Text()), []byte("bind two "+rand.Text()), []byte("bind three") + if !bindAgent(t, one, b1) || !bindAgent(t, two, b3) || !bindAgent(t, one, b2) { + t.Fatal("the agent refused a session-bind") + } + key := []byte("any key bytes \x00\xff") + data := []byte("any data \x00\xff") + if signatureOf(t, rawCall(t, one, signRequestMessage(key, data, 0xfffffffe))) != nil || + signatureOf(t, rawCall(t, two, signRequestMessage(key[:3], data[:3], 0))) != nil { + t.Fatal("the agent gave ssh a signature") + } + reqs := fb.signRequests() + if len(reqs) != 2 { + t.Fatalf("the broker got %d sign requests", len(reqs)) + } + if r := reqs[0]; !bytes.Equal(r.PublicKey, key) || !bytes.Equal(r.Data, data) || r.Flags != 0xfffffffe || + len(r.Binds) != 2 || !bytes.Equal(r.Binds[0], b1) || !bytes.Equal(r.Binds[1], b2) { + t.Errorf("the first request is %+v", r) + } + if r := reqs[1]; !bytes.Equal(r.PublicKey, key[:3]) || !bytes.Equal(r.Data, data[:3]) || r.Flags != 0 || + len(r.Binds) != 1 || !bytes.Equal(r.Binds[0], b3) { + t.Errorf("the second request is %+v", r) + } + // Without a session-bind, the broker decides: it refuses the request. + three := au.conn(t) + if signatureOf(t, rawCall(t, three, signRequestMessage(key, data, 0))) != nil { + t.Fatal("the agent gave ssh a signature") + } + if reqs := fb.signRequests(); len(reqs) != 3 || len(reqs[2].Binds) != 0 || reqs[2].Binds == nil { + t.Errorf("the request without a session-bind has %v", reqs[len(reqs)-1].Binds) + } +} + +// TestAgentRefusesAThirdBind: the agent keeps at most 2 session-binds of a +// connection, each at most 8 KiB, the most that the broker reads. It +// refuses more. Then it cannot send every session-bind of the connection, +// so the connection gets no signature, and the broker gets no request. +func TestAgentRefusesAThirdBind(t *testing.T) { + t.Parallel() + fb := newFakeSSHBroker(t, nil, func(w http.ResponseWriter, _ *api.SignRequest) { + transport.WriteError(w, http.StatusForbidden) + }) + au := startAgent(t, fb.b, agentOptions{}) + c := au.conn(t) + if !bindAgent(t, c, []byte("one")) || !bindAgent(t, c, bytes.Repeat([]byte("2"), api.MaxBind)) { + t.Fatal("the agent refused one of the first two session-binds") + } + if bindAgent(t, c, []byte("three")) { + t.Fatal("the agent took a third session-bind") + } + if signatureOf(t, rawCall(t, c, signRequestMessage([]byte("key"), []byte("data"), 0))) != nil { + t.Fatal("the agent gave ssh a signature") + } + long := au.conn(t) + if bindAgent(t, long, bytes.Repeat([]byte("x"), api.MaxBind+1)) || bindAgent(t, long, []byte("short")) { + t.Fatal("the agent took a session-bind over 8 KiB, or one after it") + } + if signatureOf(t, rawCall(t, long, signRequestMessage([]byte("key"), []byte("data"), 0))) != nil { + t.Fatal("the agent gave ssh a signature") + } + if n := fb.requests.Load(); n != 0 { + t.Errorf("the broker got %d requests, want none", n) + } + want := []string{ + "nodkey: refused a session-bind after 2 of them; this connection gets no signature", + "nodkey: no signature: the agent refused a session-bind on this connection", + "nodkey: refused a session-bind longer than 8192 bytes; this connection gets no signature", + "nodkey: refused a session-bind: the agent refused one before on this connection", + "nodkey: no signature: the agent refused a session-bind on this connection", + } + if got := au.lines(); !slices.Equal(got, want) { + t.Errorf("the agent wrote %q, want %q", got, want) + } +} + +// TestAgentSignCaps: a key over api.MaxKey or data over api.MaxData is +// refused by the agent: the broker would not read the request. +func TestAgentSignCaps(t *testing.T) { + t.Parallel() + fb := newFakeSSHBroker(t, nil, func(w http.ResponseWriter, _ *api.SignRequest) { + transport.WriteError(w, http.StatusForbidden) + }) + au := startAgent(t, fb.b, agentOptions{}) + c := au.conn(t) + for _, m := range [][]byte{ + signRequestMessage(bytes.Repeat([]byte("k"), api.MaxKey+1), []byte("data"), 0), + signRequestMessage([]byte("key"), bytes.Repeat([]byte("d"), api.MaxData+1), 0), + } { + if signatureOf(t, rawCall(t, c, m)) != nil { + t.Fatal("the agent gave ssh a signature") + } + } + // At the caps, the request goes to the broker. + if signatureOf(t, rawCall(t, c, signRequestMessage(bytes.Repeat([]byte("k"), api.MaxKey), + bytes.Repeat([]byte("d"), api.MaxData), 0))) != nil { + t.Fatal("the agent gave ssh a signature") + } + if n := fb.requests.Load(); n != 1 { + t.Errorf("the broker got %d requests, want 1", n) + } + want := "nodkey: no signature: the key or the data to sign is longer than the broker reads" + if got := au.lines(); !slices.Equal(got, []string{want, want, "nodkey: no signature: denied"}) { + t.Errorf("the agent wrote %q", got) + } +} + +// TestAgentRefusesKeyChanges: the agent refuses to add, remove, lock and +// unlock keys, every extension but session-bind@openssh.com, and every +// request it does not know, with SSH_AGENT_FAILURE. Each refused request +// gives one fixed line, and none reaches the broker. +func TestAgentRefusesKeyChanges(t *testing.T) { + t.Parallel() + fb := newFakeSSHBroker(t, nil, nil) + au := startAgent(t, fb.b, agentOptions{}) + ag, c := au.client(t), au.conn(t) + pub, priv := newSSHKey(t) + const changes = "nodkey: refused a request to add, remove, lock or unlock keys: the keys stay on the broker" + calls := []struct { + name string + call func() error + line string + }{ + {"add", func() error { return ag.Add(agent.AddedKey{PrivateKey: priv, Comment: "added"}) }, changes}, + {"add with a constraint", func() error { + return ag.Add(agent.AddedKey{PrivateKey: priv, LifetimeSecs: 60, ConfirmBeforeUse: true}) + }, changes}, + {"remove", func() error { return ag.Remove(pub.PublicKey()) }, changes}, + {"remove all", ag.RemoveAll, changes}, + {"lock", func() error { return ag.Lock([]byte("passphrase")) }, changes}, + {"unlock", func() error { return ag.Unlock([]byte("passphrase")) }, changes}, + {"query", func() error { _, err := ag.Extension("query", nil); return err }, + "nodkey: refused an extension request: the agent knows only session-bind@openssh.com"}, + {"another extension", func() error { _, err := ag.Extension("x@example.com", []byte("x")); return err }, + "nodkey: refused an extension request: the agent knows only session-bind@openssh.com"}, + } + for _, cl := range calls { + before := len(au.lines()) + if err := cl.call(); err == nil { + t.Errorf("%s: the agent did it", cl.name) + } + if got := au.lines()[before:]; !slices.Equal(got, []string{cl.line}) { + t.Errorf("%s: the agent wrote %q, want %q", cl.name, got, cl.line) + } + } + raw := []struct { + name string + msg []byte + line string + }{ + {"add a smartcard key", []byte{agentAddSmartcardKey, 0, 0, 0, 1, 'x', 0, 0, 0, 0}, changes}, + {"remove a smartcard key", []byte{agentRemoveSmartcardKey, 0, 0, 0, 0, 0, 0, 0, 0}, changes}, + {"add a smartcard key with a constraint", []byte{agentAddSmartcardConstrained, 0, 0, 0, 0}, changes}, + {"SSH1 list", []byte{1}, "nodkey: refused an agent request of the unknown type 1"}, + {"SSH1 remove all", []byte{9}, changes}, + {"SSH1 add", []byte{7, 'x'}, changes}, + {"SSH1 sign", []byte{3, 'x'}, "nodkey: refused an agent request of the unknown type 3"}, + {"unknown", []byte{99, 'x'}, "nodkey: refused an agent request of the unknown type 99"}, + {"a list request with more bytes", []byte{agentListRequest, 0}, "nodkey: refused a key list request that is not valid"}, + {"a sign request with more bytes", append(signRequestMessage([]byte("k"), []byte("d"), 0), 0), + "nodkey: refused a sign request that is not valid"}, + {"a short sign request", []byte{agentSignRequest, 0, 0, 0, 9, 'k'}, + "nodkey: refused a sign request that is not valid"}, + {"an extension without a name", []byte{agentExtension, 0, 0}, + "nodkey: refused an extension request that is not valid"}, + } + for _, r := range raw { + before := len(au.lines()) + if answer := rawCall(t, c, r.msg); !bytes.Equal(answer, failureMessage) { + t.Errorf("%s: the answer is % x", r.name, answer) + } + if got := au.lines()[before:]; !slices.Equal(got, []string{r.line}) { + t.Errorf("%s: the agent wrote %q, want %q", r.name, got, r.line) + } + } + if n := fb.requests.Load(); n != 0 { + t.Errorf("the broker got %d requests", n) + } + if keys, err := ag.List(); err != nil || len(keys) != 0 { + t.Errorf("after the refused requests, the agent lists %v, %v", keys, err) + } +} + +// TestAgentRefusedRequestLogsOneFixedLine: the agent server of +// golang.org/x/crypto writes the text of every error to Go's global +// logger, unescaped and without a limit (issue #13). nodkey ssh-agent +// serves the protocol itself: a refused request gives one fixed line on +// its own stderr, with no text that the client sent, and nothing in the +// global logger. +func TestAgentRefusedRequestLogsOneFixedLine(t *testing.T) { // not parallel: it changes the global logger + var global syncBuffer + prevW, prevF := log.Writer(), log.Flags() + log.SetOutput(&global) //nolint:forbidigo // the test captures what a library writes to the global logger. + log.SetFlags(0) + t.Cleanup(func() { + log.SetOutput(prevW) //nolint:forbidigo // restores the global logger. + log.SetFlags(prevF) + }) + fb := newFakeSSHBroker(t, nil, nil) + au := startAgent(t, fb.b, agentOptions{}) + ag, c := au.client(t), au.conn(t) + canary := "line one\naudit anchor 1:" + strings.Repeat("A", 64) + _, priv := newSSHKey(t) + if err := ag.Add(agent.AddedKey{PrivateKey: priv, Comment: canary}); err == nil { + t.Fatal("the agent added a key") + } + want := []string{"nodkey: refused a request to add, remove, lock or unlock keys: the keys stay on the broker"} + if got := au.lines(); !slices.Equal(got, want) { + t.Errorf("the agent wrote %q, want %q", got, want) + } + // The same with requests that x/crypto's server does not even parse. + for _, msg := range [][]byte{ + append([]byte{agentAddIdentity}, []byte(canary)...), + extensionMessage(canary, []byte(canary)), + append([]byte{200}, []byte(canary)...), + } { + before := len(au.lines()) + rawCall(t, c, msg) + if got := au.lines()[before:]; len(got) != 1 { + t.Errorf("one refused request gave %d lines: %q", len(got), got) + } + } + if strings.Contains(au.log.String(), "audit anchor") || strings.Contains(au.log.String(), "line one") { + t.Errorf("the agent wrote text of a request: %q", au.lines()) + } + if global.String() != "" { + t.Errorf("the global logger got %q", global.String()) + } +} + +// TestAgentLogLimit: the lines with a short code, and the lines about +// refused and failed requests, are each at most agentLogLines in a +// minute, with a budget of their own, so neither kind crowds out the +// other. The lines over a budget are counted: the next line of the same +// kind in a later minute says how many were left out, and so does flush, +// when the agent stops. A line that comes once is not limited. +func TestAgentLogLimit(t *testing.T) { + t.Parallel() + var out syncBuffer + l := newAgentLog(&out) + now := noon + l.now = func() time.Time { return now } + for range agentLogLines + 10 { + l.note("refused a request") + l.waitingFor("k3f9") + } + l.say("the SSH agent listens on /run/a/agent.sock") + lines := linesOf(out.String()) + if n := len(lines); n != 2*agentLogLines+1 || countLines(out.String(), "refused a request") != agentLogLines || + countLines(out.String(), "waiting for approval (request k3f9)") != agentLogLines || + lines[n-1] != "nodkey: the SSH agent listens on /run/a/agent.sock" { + t.Fatalf("%d lines; the last is %q", len(lines), lines[len(lines)-1]) + } + now = now.Add(agentLogWindow) + l.waitingFor("7qd2") + l.note("refused a request") + lines = linesOf(out.String())[2*agentLogLines+1:] + want := []string{ + "nodkey: left out 10 lines about requests that wait for approval, to keep the log short", + "nodkey: waiting for approval (request 7qd2)", + "nodkey: left out 10 lines about refused or failed requests, to keep the log short", + "nodkey: refused a request", + } + if !slices.Equal(lines, want) { + t.Errorf("after a minute: %q, want %q", lines, want) + } + for range agentLogLines + 3 { + l.note("refused a request") + l.waitingFor("7qd2") + } + l.flush() + lines = linesOf(out.String()) + want = []string{ + "nodkey: left out 4 lines about requests that wait for approval, to keep the log short", + "nodkey: left out 4 lines about refused or failed requests, to keep the log short", + } + if got := lines[len(lines)-2:]; !slices.Equal(got, want) { + t.Errorf("flush wrote %q, want %q", got, want) + } + // A line is always one line. + l.say("a\nb\rc") + if last := lastLine(out.String()); last != "nodkey: a b c" { + t.Errorf("a line with line breaks became %q", last) + } +} + +// countLines counts the lines of out that contain s. +func countLines(out, s string) int { + n := 0 + for _, l := range linesOf(out) { + if strings.Contains(l, s) { + n++ + } + } + return n +} + +// TestCheckPeer: only the user that runs the agent may use it; another +// user is refused, also root. +func TestCheckPeer(t *testing.T) { + t.Parallel() + if err := checkPeer(peer{uid: 1000, pid: 7}, 1000); err != nil { + t.Errorf("the same user: %v", err) + } + for _, uid := range []uint32{1001, 0, 999, math.MaxUint32} { + err := checkPeer(peer{uid: uid, pid: 7}, 1000) + if want := fmt.Sprintf("refused a connection of user ID %d: only user ID 1000 may use this agent", uid); err == nil || + err.Error() != want { + t.Errorf("user %d: %v", uid, err) + } + } +} + +// TestAgentRefusesOtherUsers: a connection of another user is closed at +// once, without an answer, and one line says so. It reaches no broker. A +// connection whose user cannot be checked is closed too. +func TestAgentRefusesOtherUsers(t *testing.T) { + t.Parallel() + self, err := currentUID() + if err != nil { + t.Fatal(err) + } + for _, tt := range []struct { + name string + peer func(*net.UnixConn) (peer, error) + line string + }{ + {"another user", func(*net.UnixConn) (peer, error) { return peer{uid: self + 1, pid: 1}, nil }, + fmt.Sprintf("nodkey: refused a connection of user ID %d: only user ID %d may use this agent", self+1, self)}, + {"root", func(*net.UnixConn) (peer, error) { return peer{uid: 0, pid: 1}, nil }, + fmt.Sprintf("nodkey: refused a connection of user ID 0: only user ID %d may use this agent", self)}, + {"unknown", func(*net.UnixConn) (peer, error) { return peer{}, errors.New("no credentials") }, + "nodkey: refused a connection: its user cannot be checked"}, + } { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if self == 0 && tt.name == "root" { + t.Skip("the test runs as root") + } + fb := newFakeSSHBroker(t, nil, nil) + au := startAgent(t, fb.b, agentOptions{peer: tt.peer}) + ag := au.client(t) + if _, err := ag.List(); err == nil { + t.Fatal("the agent answered") + } + if got := au.lines(); !slices.Equal(got, []string{tt.line}) { + t.Errorf("the agent wrote %q, want %q", got, tt.line) + } + if fb.requests.Load() != 0 { + t.Error("a request reached the broker") + } + }) + } +} + +// TestPeerCredentials: SO_PEERCRED gives the user and the process that +// connected. +func TestPeerCredentials(t *testing.T) { + t.Parallel() + got := make(chan peer, 1) + self, err := currentUID() + if err != nil { + t.Fatal(err) + } + fb := newFakeSSHBroker(t, nil, nil) + au := startAgent(t, fb.b, agentOptions{peer: func(c *net.UnixConn) (peer, error) { + p, err := peerCredentials(c) + got <- p + return p, err + }}) + ag := au.client(t) + if _, err := ag.List(); err != nil { + t.Fatal(err) + } + if p := <-got; p.uid != self || int(p.pid) != os.Getpid() { + t.Errorf("SO_PEERCRED gives %+v; want user %d and process %d", p, self, os.Getpid()) + } +} + +// TestAgentClosesOnBadLengths: a message that is empty or longer than +// 256 KiB closes the connection. +func TestAgentClosesOnBadLengths(t *testing.T) { + t.Parallel() + fb := newFakeSSHBroker(t, nil, nil) + au := startAgent(t, fb.b, agentOptions{}) + for _, n := range []uint32{0, maxAgentMessage + 1, math.MaxUint32} { + c := au.conn(t) + if _, err := c.Write(binary.BigEndian.AppendUint32(nil, n)); err != nil { + t.Fatal(err) + } + _ = c.SetReadDeadline(time.Now().Add(waitLimit)) + if _, err := c.Read(make([]byte, 1)); !errors.Is(err, io.EOF) { + t.Errorf("length %d: the read gave %v, want EOF", n, err) + } + } + want := fmt.Sprintf("nodkey: closed a connection that sent a message that is empty or longer than %d bytes", + maxAgentMessage) + if got := au.lines(); !slices.Equal(got, []string{want, want, want}) { + t.Errorf("the agent wrote %q", got) + } + // A message of the most bytes is read, and refused. + c := au.conn(t) + msg := append([]byte{agentAddIdentity}, bytes.Repeat([]byte("k"), maxAgentMessage-1)...) + if answer := rawCall(t, c, msg); !bytes.Equal(answer, failureMessage) { + t.Errorf("the answer is % x", answer) + } +} + +// TestLargestSignRequestFits: the largest request that the agent sends +// fits in the broker's cap (transport.MaxRequestBody), and the broker +// reads it: a key of api.MaxKey bytes, data of api.MaxData bytes, two +// session-binds of api.MaxBind bytes, 4 command lines and a reason at +// their limits, of characters that take the most bytes in JSON. Longer +// claimed fields become "[hidden: too long]", and more command lines are +// not sent. +func TestLargestSignRequestFits(t *testing.T) { + t.Parallel() + worst := strings.Repeat("\x01", api.MaxText) + argv := make([]string, api.MaxArgs) + for i := range argv { + argv[i] = worst[:api.MaxText/api.MaxArgs] + } + binds := [][]byte{bytes.Repeat([]byte{0xff}, api.MaxBind), bytes.Repeat([]byte{0xfe}, api.MaxBind)} + key, data := bytes.Repeat([]byte{0xfd}, api.MaxKey), bytes.Repeat([]byte{0xfc}, api.MaxData) + commands := [][]string{argv, argv, argv, argv, argv} + req := newSignRequest(key, data, math.MaxUint32, binds, commands, worst) + if len(req.Commands) != api.MaxCommands || len(req.Commands[0]) != api.MaxArgs || req.Reason != worst { + t.Fatalf("the request has %d command lines", len(req.Commands)) + } + body, err := encodeSignRequest(req) + if err != nil { + t.Fatal(err) + } + if len(body) > transport.MaxRequestBody { + t.Errorf("the largest request has %d bytes, more than %d", len(body), transport.MaxRequestBody) + } + t.Logf("the largest sign request has %d bytes", len(body)) + // The broker reads it. + r := httptest.NewRequestWithContext(t.Context(), http.MethodPost, api.PathSign, bytes.NewReader(body)) + r.Header.Set("Content-Type", api.ContentType) + got, err := transport.ReadJSON[api.SignRequest](httptest.NewRecorder(), r) + if err != nil { + t.Fatalf("the broker cannot read the largest request: %v", err) + } + if !bytes.Equal(got.PublicKey, key) || len(got.Commands) != api.MaxCommands || got.Flags != math.MaxUint32 { + t.Error("the broker read another request") + } + // Longer claimed fields are hidden. + long := newSignRequest(key, data, 0, binds, [][]string{append(argv, "x"), {worst + "x"}}, worst+"x") + if !slices.Equal(long.Commands[0], []string{tooLong}) || !slices.Equal(long.Commands[1], []string{tooLong}) || + long.Reason != tooLong { + t.Errorf("the long fields are not hidden") + } + if maxParents+1 != api.MaxCommands { + t.Errorf("the agent sends %d command lines, the broker takes %d", maxParents+1, api.MaxCommands) + } + // A third session-bind is over the cap. + if _, err := encodeSignRequest(newSignRequest(key, data, 0, append(binds, nil), nil, "")); err == nil { + t.Error("encodeSignRequest took 3 session-binds") + } +} + +// TestCheckSignAnswer: the checks of a signature before ssh gets it. +func TestCheckSignAnswer(t *testing.T) { + t.Parallel() + signer, _ := newSSHKey(t) + data := []byte("data") + sig, err := signer.Sign(rand.Reader, data) + if err != nil { + t.Fatal(err) + } + key := signer.PublicKey().Marshal() + if got, err := checkSignAnswer(key, data, 0, &api.SignAnswer{Format: sig.Format, Blob: sig.Blob}); err != nil || + got.Format != sig.Format || !bytes.Equal(got.Blob, sig.Blob) { + t.Errorf("a valid signature: %v, %v", got, err) + } + for _, tt := range []struct { + name string + key []byte + ans api.SignAnswer + want error + }{ + {"not a key", []byte("key"), api.SignAnswer{Format: sig.Format, Blob: sig.Blob}, errSignatureKey}, + {"no format", key, api.SignAnswer{Blob: sig.Blob}, errSignatureFormat}, + {"a blob with more bytes", key, api.SignAnswer{Format: sig.Format, Blob: append(bytes.Clone(sig.Blob), 0)}, + errSignatureValid}, + } { + if _, err := checkSignAnswer(tt.key, data, 0, &tt.ans); !errors.Is(err, tt.want) { + t.Errorf("%s: %v, want %v", tt.name, err, tt.want) + } + } +} + +// TestAgentStopsCleanly: when the agent stops, it closes the open +// connections, also one whose request waits for the broker, and that +// request ends. +func TestAgentStopsCleanly(t *testing.T) { + t.Parallel() + deploy, _ := newSSHKey(t) + waiting := make(chan struct{}) + hungUp := make(chan struct{}) + fb := newFakeBroker(t, func(w http.ResponseWriter, r *http.Request) { + if _, err := transport.ReadJSON[api.SignRequest](w, r); err != nil { + return + } + close(waiting) + <-r.Context().Done() + close(hungUp) + }) + self, err := currentUID() + if err != nil { + t.Fatal(err) + } + path := shortTempDir(t) + "/a/" + socketName + sock, err := listenAgent(t.Context(), path, self, noDirCheck) + if err != nil { + t.Fatal(err) + } + var out syncBuffer + a := newSSHAgent(fb.b, newAgentLog(&out), procFS{root: t.TempDir()}, peerCredentials, self) + ctx, cancel := context.WithCancelCause(context.Background()) + done := make(chan struct{}) + go func() { + defer close(done) + a.serve(ctx, sock.ln) + sock.close() + }() + c, err := (&net.Dialer{}).DialContext(t.Context(), "unix", path) + if err != nil { + t.Fatal(err) + } + defer func() { _ = c.Close() }() + if err := writeAgentMessage(c, signRequestMessage(deploy.PublicKey().Marshal(), []byte("d"), 0)); err != nil { + t.Fatal(err) + } + <-waiting + cancel(interrupted{sig: 15}) + select { + case <-done: + case <-time.After(waitLimit): + t.Fatal("the agent did not stop") + } + select { + case <-hungUp: + case <-time.After(waitLimit): + t.Fatal("the broker did not see the agent hang up") + } + // The request may get its failure before the connection closes; it + // never gets a signature. + _ = c.SetReadDeadline(time.Now().Add(waitLimit)) + if answer, err := readAgentMessage(c); err == nil { + if !bytes.Equal(answer, failureMessage) { + t.Errorf("the request got % x", answer) + } + if _, err := readAgentMessage(c); err == nil { + t.Error("the connection is still open") + } + } + if got := linesOf(out.String()); !slices.Equal(got, []string{"nodkey: no signature: the agent stops"}) { + t.Errorf("the agent wrote %q", got) + } + if _, err := (&net.Dialer{}).DialContext(t.Context(), "unix", path); err == nil { + t.Error("the socket still takes connections") + } +} + +// TestAgentCancelsWhenSSHHangsUp: when ssh ends while its request waits +// for the person, such as after Ctrl-C, the agent closes its connection to +// the broker, so the broker cancels the request (DESIGN.md section 7). +func TestAgentCancelsWhenSSHHangsUp(t *testing.T) { + t.Parallel() + host, _ := newSSHKey(t) + sb := newSSHBroker(t, map[string]ssh.PublicKey{"web-1": host.PublicKey()}) + srv := startSSHServer(t, host, sb.user.PublicKey()) + au := startAgent(t, sb.client(t), agentOptions{}) + c := au.conn(t) + ag := agent.NewClient(c) + done := make(chan error, 1) + go func() { done <- login(t.Context(), ag, srv, "deploy") }() + code := sb.waitAsked(t, 1)[0].Prompt.ShortCode + _ = c.Close() + if err := <-done; err == nil { + t.Fatal("the login worked") + } + if got := eventNames(sb.audit.waitFor(t, 2)); got != "request denied:cancelled" { + t.Errorf("audit entries %q", got) + } + const last = "ssh closed its connection while the request waited" + waitFor(t, "the agent's last line", func() bool { return strings.Contains(au.log.String(), last) }) + switch got := au.lines(); { + case slices.Equal(got, []string{"nodkey: waiting for approval (request " + code + ")", + "nodkey: no signature (request " + code + "): " + last}): + case slices.Equal(got, []string{"nodkey: no signature: " + last}): + // ssh hung up before the agent read the 102 Processing. + default: + t.Errorf("the agent wrote %q", got) + } +} + +// TestAgentKeepsAPipelinedRequest: a client may send its next request +// before the answer. The watch for a hang-up does not take it, and does +// not cancel the request that waits. +func TestAgentKeepsAPipelinedRequest(t *testing.T) { + t.Parallel() + deploy, _ := newSSHKey(t) + waiting := make(chan struct{}) + release := make(chan struct{}) + stillThere := make(chan bool, 1) + keys := api.KeysAnswer{Keys: []api.Key{{Name: "deploy", PublicKey: deploy.PublicKey().Marshal()}}} + fb := newFakeBroker(t, func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == api.PathKeys { + _ = transport.WriteJSON(w, http.StatusOK, keys) + return + } + req, err := transport.ReadJSON[api.SignRequest](w, r) + if err != nil { + return + } + close(waiting) + <-release + stillThere <- r.Context().Err() == nil + signWith(deploy, "", "")(w, req) + }) + au := startAgent(t, fb.b, agentOptions{}) + c := au.conn(t) + if !bindAgent(t, c, []byte("bind")) { + t.Fatal("the session-bind was refused") + } + data := []byte("data") + if err := writeAgentMessage(c, signRequestMessage(deploy.PublicKey().Marshal(), data, 0)); err != nil { + t.Fatal(err) + } + <-waiting + if err := writeAgentMessage(c, []byte{agentListRequest}); err != nil { + t.Fatal(err) + } + // Time for a wrong watch to take the request, or to hang up. + time.Sleep(50 * time.Millisecond) + close(release) + if !<-stillThere { + t.Error("the agent hung up on the broker") + } + _ = c.SetReadDeadline(time.Now().Add(waitLimit)) + first, err := readAgentMessage(c) + if err != nil { + t.Fatal(err) + } + if sig := signatureOf(t, first); sig == nil || deploy.PublicKey().Verify(data, sig) != nil { + t.Fatalf("no valid signature; lines %q", au.lines()) + } + second, err := readAgentMessage(c) + if err != nil || len(second) == 0 || second[0] != agentListAnswer { + t.Fatalf("the list request got % x, %v", second, err) + } +} + +// TestAgentCancelsWhenSSHHangsUpAfterMoreBytes: ssh hangs up while its +// request waits, also after it sent more bytes, such as the start of a +// next request that nobody reads. The agent still sees the hang-up, and +// closes its connection to the broker, so the broker cancels the request. +// (From the security review of issue #13: the watch stopped at the first +// byte.) +func TestAgentCancelsWhenSSHHangsUpAfterMoreBytes(t *testing.T) { + t.Parallel() + for _, extra := range [][]byte{nil, {0}, {0, 0, 0, 9, agentListRequest}} { + t.Run(fmt.Sprintf("%d more bytes", len(extra)), func(t *testing.T) { + t.Parallel() + deploy, _ := newSSHKey(t) + waiting := make(chan struct{}) + cancelled := make(chan bool, 1) + fb := newFakeBroker(t, func(w http.ResponseWriter, r *http.Request) { + if _, err := transport.ReadJSON[api.SignRequest](w, r); err != nil { + return + } + close(waiting) + select { + case <-r.Context().Done(): + cancelled <- true + case <-time.After(waitLimit): + cancelled <- false + } + transport.WriteError(w, http.StatusForbidden) + }) + au := startAgent(t, fb.b, agentOptions{}) + c := au.conn(t) + if !bindAgent(t, c, []byte("bind")) { + t.Fatal("the session-bind was refused") + } + if err := writeAgentMessage(c, signRequestMessage(deploy.PublicKey().Marshal(), []byte("d"), 0)); err != nil { + t.Fatal(err) + } + <-waiting + if len(extra) > 0 { + if _, err := c.Write(extra); err != nil { + t.Fatal(err) + } + time.Sleep(50 * time.Millisecond) // time for the agent to see the bytes first + } + _ = c.Close() + if !<-cancelled { + t.Fatal("ssh hung up, and the request still waits at the broker") + } + const last = "nodkey: no signature: ssh closed its connection while the request waited" + waitFor(t, "the agent's line", func() bool { return slices.Contains(au.lines(), last) }) + }) + } +} + +// captureSigner records the data that the ssh client of golang.org/x/crypto +// asks it to sign, and fails the login. +type captureSigner struct { + pub ssh.PublicKey + data chan []byte +} + +func (s *captureSigner) PublicKey() ssh.PublicKey { return s.pub } + +func (s *captureSigner) Sign(_ io.Reader, data []byte) (*ssh.Signature, error) { + select { + case s.data <- bytes.Clone(data): + default: + } + return nil, errors.New("captured") +} + +// validLogin returns a session-bind and data to sign that pass every check +// of the real broker for the key deploy, from one real key exchange with +// srv. The broker does not refuse a second use of them, so a process can +// send them again and again. +func validLogin(t *testing.T, sb *sshBroker, srv *sshServer) (bind, data []byte) { + t.Helper() + cs := &captureSigner{pub: sb.user.PublicKey(), data: make(chan []byte, 1)} + nc, err := (&net.Dialer{Timeout: waitLimit}).DialContext(t.Context(), "tcp", srv.addr) + if err != nil { + t.Fatal(err) + } + defer func() { _ = nc.Close() }() + cfg := &ssh.ClientConfig{User: "deploy", Auth: []ssh.AuthMethod{ssh.PublicKeys(cs)}, + HostKeyCallback: ssh.FixedHostKey(srv.hostKey)} + _, _, _, _ = ssh.NewClientConn(nc, srv.addr, cfg) + select { + case data = <-cs.data: + default: + t.Fatal("no data to sign") + } + b := srv.binds() + return b[len(b)-1], data +} + +// TestAgentShortCodeLinesAreLimited: the broker sends a short code for +// every request that starts to wait, also in its queue, and a request +// that hangs up frees its place. So any process of the user can make the +// agent get many short codes. The agent writes at most agentLogLines of +// them in a minute, and then says how many it left out. (From the +// security review of issue #13: one process made 150 lines in 1.7 s.) +func TestAgentShortCodeLinesAreLimited(t *testing.T) { + t.Parallel() + host, _ := newSSHKey(t) + sb := newSSHBroker(t, map[string]ssh.PublicKey{"web-1": host.PublicKey()}) + srv := startSSHServer(t, host, sb.user.PublicKey()) + // A clock that does not move: the whole test is in one minute. + au := startAgent(t, sb.client(t), agentOptions{now: func() time.Time { return noon }}) + bind, data := validLogin(t, sb, srv) + msg := signRequestMessage(sb.user.PublicKey().Marshal(), data, 0) + // Fill the 5 places for messages; nobody answers them. + for range 5 { + c := au.conn(t) + if !bindAgent(t, c, bind) { + t.Fatal("the session-bind was refused") + } + if err := writeAgentMessage(c, msg); err != nil { + t.Fatal(err) + } + } + sb.waitAsked(t, 5) + waitFor(t, "5 short codes", func() bool { return au.shortCodes() == 5 }) + // Each new request goes into the queue, gets a short code, and hangs + // up, which frees its place. + const rounds = agentLogLines + 10 + for i := 0; i < rounds; { + before := au.shortCodes() + c, err := (&net.Dialer{}).DialContext(t.Context(), "unix", au.socket) + if err != nil { + t.Fatal(err) + } + if !bindAgent(t, c, bind) { + t.Fatal("the session-bind was refused") + } + if err := writeAgentMessage(c, msg); err != nil { + t.Fatal(err) + } + queued := false + deadline := time.Now().Add(waitLimit) + for !queued && time.Now().Before(deadline) { + if au.shortCodes() > before { + queued = true + break + } + _ = c.SetReadDeadline(time.Now().Add(time.Millisecond)) + if _, err := readAgentMessage(c); err == nil { + break // denied, such as by a full queue: try again + } + } + _ = c.Close() + if queued { + i++ + } + } + au.stop() + out := au.log.String() + if n := countLines(out, "waiting for approval"); n != agentLogLines { + t.Errorf("%d lines with a short code; want %d", n, agentLogLines) + } + want := fmt.Sprintf("nodkey: left out %d lines about requests that wait for approval, to keep the log short", + 5+rounds-agentLogLines) + if !slices.Contains(linesOf(out), want) { + t.Errorf("no line %q", want) + } +} + +// pieceReader gives data in pieces of at most 1000 bytes, and records the +// size of each buffer that it gets. +type pieceReader struct { + data []byte + sizes []int +} + +func (r *pieceReader) Read(p []byte) (int, error) { + r.sizes = append(r.sizes, len(p)) + if len(r.data) == 0 { + return 0, io.EOF + } + n := copy(p, r.data[:min(len(r.data), 1000)]) + r.data = r.data[n:] + return n, nil +} + +// TestReadAgentMessageGrows: the buffer of a message grows as its bytes +// come. A length alone, without the bytes, takes at most firstPiece +// bytes. (From the security review of issue #13: 200 connections that +// sent only a length held about 55 MiB.) +func TestReadAgentMessageGrows(t *testing.T) { + t.Parallel() + body := make([]byte, maxAgentMessage) + for i := range body { + body[i] = byte(i) + } + r := &pieceReader{data: append(binary.BigEndian.AppendUint32(nil, maxAgentMessage), body...)} + msg, err := readAgentMessage(r) + if err != nil || !bytes.Equal(msg, body) { + t.Fatalf("the message of %d bytes: %v", len(msg), err) + } + if len(r.sizes) < 2 || r.sizes[1] > firstPiece { + t.Errorf("the first buffer for the bytes has %v bytes; want at most %d", r.sizes[1:min(2, len(r.sizes))], firstPiece) + } + // Only a length and a few bytes: the buffer stays small, and the + // short message is an error. + r = &pieceReader{data: append(binary.BigEndian.AppendUint32(nil, maxAgentMessage), "short"...)} + if _, err := readAgentMessage(r); !errors.Is(err, io.ErrUnexpectedEOF) { + t.Errorf("a short message: %v", err) + } + if most := slices.Max(r.sizes); most > firstPiece { + t.Errorf("a short message got a buffer of %d bytes", most) + } +} + +// TestAgentCapsConnections: the agent serves at most maxConns connections +// at the same time, and closes a connection over that at once, with one +// line. When a connection ends, a new one is served again. +func TestAgentCapsConnections(t *testing.T) { + t.Parallel() + deploy, _ := newSSHKey(t) + fb := newFakeSSHBroker(t, map[string]ssh.PublicKey{"deploy": deploy.PublicKey()}, nil) + au := startAgent(t, fb.b, agentOptions{maxConns: 3}) + var open []net.Conn + for range 3 { + open = append(open, au.conn(t)) + } + over := au.conn(t) + _ = over.SetReadDeadline(time.Now().Add(waitLimit)) + if _, err := over.Read(make([]byte, 1)); !errors.Is(err, io.EOF) { + t.Fatalf("a connection over the cap: %v; want it closed", err) + } + want := "nodkey: refused a connection: 3 connections are open, the most that the agent serves" + if got := au.lines(); !slices.Equal(got, []string{want}) { + t.Errorf("the agent wrote %q", got) + } + _ = open[0].Close() + waitFor(t, "the closed connection to end", func() bool { return au.openConns() == 2 }) + if keys, err := au.client(t).List(); err != nil || len(keys) != 1 { + t.Errorf("after a connection ended: %v, %v", keys, err) + } +} + +// TestAgentIdleConnectionsHoldLittleMemory: many connections that send +// only the length of a long message hold little memory: the buffers grow +// as bytes come, and the agent serves at most maxAgentConns connections. +func TestAgentIdleConnectionsHoldLittleMemory(t *testing.T) { // not parallel: it measures the heap + fb := newFakeSSHBroker(t, nil, nil) + au := startAgent(t, fb.b, agentOptions{}) + runtime.GC() + var before, after runtime.MemStats + runtime.ReadMemStats(&before) + const n = 200 + for range n { + // The agent closes the connections over its cap at once, so a + // write to them may fail. + _, _ = au.conn(t).Write(binary.BigEndian.AppendUint32(nil, maxAgentMessage)) + } + waitFor(t, "the agent to take the connections", func() bool { return au.openConns() == maxAgentConns }) + time.Sleep(300 * time.Millisecond) // time to read the lengths + runtime.GC() + runtime.ReadMemStats(&after) + grew := int64(after.HeapAlloc) - int64(before.HeapAlloc) //nolint:gosec // G115: heap sizes fit in an int64. + t.Logf("%d connections that sent only a length: the heap grew by %d KiB", n, grew>>10) + if grew > 8<<20 { + t.Errorf("the heap grew by %d MiB", grew>>20) + } +} + +// TestAgentStopsWithStuckConnections: when the agent stops, it waits for +// its connections at most stopWait, also when one of them is stuck. +func TestAgentStopsWithStuckConnections(t *testing.T) { + t.Parallel() + release := make(chan struct{}) + defer close(release) + stuck := make(chan struct{}, 1) + fb := newFakeSSHBroker(t, nil, nil) + au := startAgent(t, fb.b, agentOptions{stopWait: 100 * time.Millisecond, + peer: func(*net.UnixConn) (peer, error) { + stuck <- struct{}{} + <-release + return peer{}, errors.New("released") + }}) + au.conn(t) + <-stuck + start := time.Now() + au.stop() + if d := time.Since(start); d > waitLimit/2 { + t.Errorf("the agent stopped after %s", d) + } + if got := au.lines(); !slices.Contains(got, "nodkey: stopped without waiting longer for its connections (1 still open)") { + t.Errorf("the agent wrote %q", got) + } +} + +// TestAgentProcTimeout: a read of /proc that blocks, such as on a stuck +// FUSE or NFS file system, holds a request at most procTimeout. The +// request then goes without the command lines and the reason, and a line +// says why. When maxProcReads reads are stuck, the agent starts no new +// one. (From the security review of issue #13.) +func TestAgentProcTimeout(t *testing.T) { + t.Parallel() + p := newFakeProc(t) + p.add(t, 4242, 1, []string{"ssh", "web-1"}, []string{"NODKEY_REASON=r"}) + fifo := filepath.Join(p.root, "4242", "cmdline") + if err := os.Remove(fifo); err != nil { + t.Fatal(err) + } + if err := syscall.Mkfifo(fifo, 0o600); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + // A writer ends the opens that wait; the reads then end. + if w, err := os.OpenFile(fifo, os.O_WRONLY|syscall.O_NONBLOCK, 0); err == nil { //nolint:gosec // G304: the FIFO that the test made. + _ = w.Close() + } + }) + self, err := currentUID() + if err != nil { + t.Fatal(err) + } + fb := newFakeSSHBroker(t, nil, func(w http.ResponseWriter, _ *api.SignRequest) { + transport.WriteError(w, http.StatusForbidden) + }) + au := startAgent(t, fb.b, agentOptions{proc: p.fs(), procTimeout: 100 * time.Millisecond, + peer: func(*net.UnixConn) (peer, error) { return peer{uid: self, pid: 4242}, nil }}) + for range maxProcReads + 1 { + start := time.Now() + if signOnce(t, au, deploy0(t), []byte("data"), 0) != nil { + t.Fatal("the agent gave ssh a signature") + } + if d := time.Since(start); d > waitLimit/2 { + t.Fatalf("the request took %s", d) + } + } + reqs := fb.signRequests() + if len(reqs) != maxProcReads+1 { + t.Fatalf("the broker got %d requests", len(reqs)) + } + for _, r := range reqs { + if len(r.Commands) != 0 || r.Reason != "" { + t.Errorf("a request has the claimed context %q, %q", r.Commands, r.Reason) + } + } + timeout := "nodkey: sent a sign request without the command lines and the reason: /proc did not answer within 100ms" + stuck := fmt.Sprintf("nodkey: sent a sign request without the command lines and the reason: %d reads of /proc "+ + "are stuck", maxProcReads) + if got := au.lines(); countLines(au.log.String(), timeout[len("nodkey: "):]) != maxProcReads || + got[len(got)-2] != stuck { + t.Errorf("the agent wrote %q", got) + } +} + +// deploy0 returns an Ed25519 public key for requests that a fake broker +// answers. +func deploy0(t *testing.T) ssh.PublicKey { + t.Helper() + k, _ := newSSHKey(t) + return k.PublicKey() +} diff --git a/internal/client/sshagent_wire.go b/internal/client/sshagent_wire.go new file mode 100644 index 0000000..b180935 --- /dev/null +++ b/internal/client/sshagent_wire.go @@ -0,0 +1,213 @@ +package client + +import ( + "encoding/binary" + "errors" + "io" + + "golang.org/x/crypto/ssh" + + "github.com/code6x/nodkey/internal/api" +) + +// nodkey ssh-agent serves the SSH agent protocol itself (the IETF draft +// draft-miller-ssh-agent, and OpenSSH's PROTOCOL.agent). It does not use +// agent.ServeAgent of golang.org/x/crypto: that server writes the text of +// every error to Go's global logger, unescaped and without a limit, and it +// parses the private key of an add request before the agent can refuse it. +// +// Each message is a uint32 length and that many bytes. The first byte is +// the type of the message. + +// The types of the messages that nodkey ssh-agent reads and writes. +const ( + agentFailure = 5 // SSH_AGENT_FAILURE + agentSuccess = 6 // SSH_AGENT_SUCCESS + agentListRequest = 11 // SSH_AGENTC_REQUEST_IDENTITIES + agentListAnswer = 12 // SSH_AGENT_IDENTITIES_ANSWER + agentSignRequest = 13 // SSH_AGENTC_SIGN_REQUEST + agentSignAnswer = 14 // SSH_AGENT_SIGN_RESPONSE + agentExtension = 27 // SSH_AGENTC_EXTENSION + sessionBindRequest = "session-bind@openssh.com" +) + +// The requests that change the keys of an agent, or lock it. nodkey +// ssh-agent refuses all of them: the keys stay on the broker. +const ( + agentAddIdentity = 17 // SSH_AGENTC_ADD_IDENTITY + agentRemoveIdentity = 18 // SSH_AGENTC_REMOVE_IDENTITY + agentRemoveAll = 19 // SSH_AGENTC_REMOVE_ALL_IDENTITIES + agentAddSmartcardKey = 20 // SSH_AGENTC_ADD_SMARTCARD_KEY + agentRemoveSmartcardKey = 21 // SSH_AGENTC_REMOVE_SMARTCARD_KEY + agentLock = 22 // SSH_AGENTC_LOCK + agentUnlock = 23 // SSH_AGENTC_UNLOCK + agentAddConstrained = 25 // SSH_AGENTC_ADD_ID_CONSTRAINED + agentAddSmartcardConstrained = 26 // SSH_AGENTC_ADD_SMARTCARD_KEY_CONSTRAINED + // The same for keys of SSH protocol 1, which OpenSSH no longer has. + // ssh-add -D still sends agentRemoveAllRSA. + agentAddRSA = 7 // SSH_AGENTC_ADD_RSA_IDENTITY + agentRemoveRSA = 8 // SSH_AGENTC_REMOVE_RSA_IDENTITY + agentRemoveAllRSA = 9 // SSH_AGENTC_REMOVE_ALL_RSA_IDENTITIES + agentAddRSAConstrained = 24 // SSH_AGENTC_ADD_RSA_ID_CONSTRAINED +) + +// maxAgentMessage is the longest message that the agent reads: 256 KiB, +// as OpenSSH's own agent (AGENT_MAX_LEN). A longer one closes the +// connection. A valid request of ssh is far shorter: the key, the data to +// sign and a session-bind are each at most a few KiB. +const maxAgentMessage = 256 << 10 + +// errMessageSize: a message is empty or longer than maxAgentMessage. +var errMessageSize = errors.New("client: an agent message is empty or longer than 256 KiB") + +// failureMessage is the answer to every request that the agent refuses or +// cannot do. It says nothing about why. +var failureMessage = []byte{agentFailure} + +// firstPiece is the first size of the buffer of a message. It grows as +// the bytes come. +const firstPiece = 4 << 10 + +// readAgentMessage reads one message. Its buffer grows as the bytes come, +// as in OpenSSH's own agent: a length alone, without the bytes, takes +// little memory. An old buffer is cleared when the buffer grows. The +// caller clears the message after use: an add request holds a private +// key, and a lock request a passphrase. +func readAgentMessage(r io.Reader) ([]byte, error) { + var head [4]byte + if _, err := io.ReadFull(r, head[:]); err != nil { + return nil, err + } + size := binary.BigEndian.Uint32(head[:]) + if size == 0 || size > maxAgentMessage { + return nil, errMessageSize + } + n := int(size) + msg := make([]byte, 0, min(n, firstPiece)) + for len(msg) < n { + if len(msg) == cap(msg) { + grown := make([]byte, len(msg), min(2*cap(msg), n)) + copy(grown, msg) + clear(msg) + msg = grown + } + k, err := r.Read(msg[len(msg):cap(msg)]) + msg = msg[:len(msg)+k] + if err != nil && len(msg) < n { + clear(msg) + if errors.Is(err, io.EOF) { + err = io.ErrUnexpectedEOF + } + return nil, err + } + } + return msg, nil +} + +// writeAgentMessage writes one message, with its length, in one write. +func writeAgentMessage(w io.Writer, msg []byte) error { + if len(msg) == 0 || len(msg) > maxAgentMessage { + return errMessageSize + } + b := binary.BigEndian.AppendUint32(make([]byte, 0, 4+len(msg)), uint32(len(msg))) //nolint:gosec // G115: the length is checked above. + _, err := w.Write(append(b, msg...)) + return err +} + +// wireReader reads SSH wire data (RFC 4251 section 5). After a read fails, +// every later read fails too, so a parser checks ok once at the end. +type wireReader struct { + b []byte + ok bool +} + +func newWireReader(b []byte) *wireReader { return &wireReader{b: b, ok: true} } + +// uint32 reads a uint32. +func (r *wireReader) uint32() uint32 { + if !r.ok || len(r.b) < 4 { + r.ok = false + return 0 + } + v := binary.BigEndian.Uint32(r.b) + r.b = r.b[4:] + return v +} + +// string reads a string: a uint32 length and that many bytes. The result +// shares memory with the data. +func (r *wireReader) string() []byte { + n := r.uint32() + if !r.ok || uint64(n) > uint64(len(r.b)) { + r.ok = false + return nil + } + s := r.b[:n:n] + r.b = r.b[n:] + return s +} + +// rest returns the bytes that are left. +func (r *wireReader) rest() []byte { + s := r.b + r.b = nil + return s +} + +// done reports whether every read worked and no byte is left. +func (r *wireReader) done() bool { return r.ok && len(r.b) == 0 } + +// parseSignRequest reads the body of a sign request, after its type: +// +// string key blob +// string data +// uint32 flags +// +// It refuses bytes after the flags. The results share memory with body. +func parseSignRequest(body []byte) (key, data []byte, flags uint32, ok bool) { + r := newWireReader(body) + key = r.string() + data = r.string() + flags = r.uint32() + if !r.done() { + return nil, nil, 0, false + } + return key, data, flags, true +} + +// parseExtension reads the body of an extension request, after its type: +// string name, then the contents, which are the rest of the message. The +// results share memory with body. +func parseExtension(body []byte) (name string, contents []byte, ok bool) { + r := newWireReader(body) + n := r.string() + if !r.ok { + return "", nil, false + } + return string(n), r.rest(), true +} + +// appendString appends s as an SSH string: its length and its bytes. +func appendString(b, s []byte) []byte { + b = binary.BigEndian.AppendUint32(b, uint32(len(s))) //nolint:gosec // G115: every string here is far shorter than 4 GiB: a key, a name or a signature. + return append(b, s...) +} + +// listAnswerMessage returns the answer to a list request: the number of +// keys, and for each its public key in SSH wire form and its comment, +// which is the key's name in the broker config. +func listAnswerMessage(keys []api.Key) []byte { + b := binary.BigEndian.AppendUint32([]byte{agentListAnswer}, uint32(len(keys))) //nolint:gosec // G115: api.KeysAnswer holds at most api.MaxKeys keys. + for _, k := range keys { + b = appendString(b, k.PublicKey) + b = appendString(b, []byte(k.Name)) + } + return b +} + +// signAnswerMessage returns the answer to a sign request: the signature, +// as an SSH string that holds its format and its blob. +func signAnswerMessage(sig *ssh.Signature) []byte { + inner := appendString(appendString(nil, []byte(sig.Format)), sig.Blob) + return appendString([]byte{agentSignAnswer}, inner) +} diff --git a/internal/config/broker.go b/internal/config/broker.go index 25f3067..acf93c5 100644 --- a/internal/config/broker.go +++ b/internal/config/broker.go @@ -21,11 +21,15 @@ const ( defaultTimeout = 120 * time.Second defaultSSHTimeout = 90 * time.Second minTimeout = 15 * time.Second - // sshd drops a login that does not finish within LoginGraceTime, - // 120 seconds by default (DESIGN.md section 6). - maxSSHTimeout = 110 * time.Second ) +// MaxSSHTimeout is the longest timeout of an SSH request that the broker +// config allows (ssh_timeout in [telegram]). sshd drops a login that does +// not finish within LoginGraceTime, 120 seconds by default (DESIGN.md +// section 6). nodkey ssh-agent does not know the broker's own setting, so +// it waits this long, and a short margin. +const MaxSSHTimeout = 110 * time.Second + // MaxTimeout is the longest timeout of a request for secrets that the // broker config allows (timeout in [telegram]). nodkey run does not know // the broker's own setting, so it waits this long, and a margin. @@ -316,7 +320,7 @@ func parseTelegram(s *telegramSection) (Telegram, error) { if t.Timeout, err = parseTimeout(sec, "timeout", s.Timeout, defaultTimeout, MaxTimeout); err != nil { return Telegram{}, err } - if t.SSHTimeout, err = parseTimeout(sec, "ssh_timeout", s.SSHTimeout, defaultSSHTimeout, maxSSHTimeout); err != nil { + if t.SSHTimeout, err = parseTimeout(sec, "ssh_timeout", s.SSHTimeout, defaultSSHTimeout, MaxSSHTimeout); err != nil { return Telegram{}, err } return t, nil