From a6e90f803f3aa782f31d5c0cb781ffa49bc25f4e Mon Sep 17 00:00:00 2001 From: Geno Roupsky <185580+groupsky@users.noreply.github.com> Date: Thu, 15 Oct 2026 08:06:45 +0300 Subject: [PATCH] feat(ssh-grant): one tap for SSH logins to a list of hosts nodkey ssh-grant asks once for logins as one user to listed hosts for N minutes; grants end at the second the message shows. Closes #14 --- AGENTS.md | 4 +- DESIGN.md | 111 ++- THREAT-MODEL.md | 22 +- cmd/nodkey/main.go | 3 + cmd/nodkey/sshgrant.go | 14 + cmd/nodkey/sshgrant_test.go | 40 ++ internal/approval/check.go | 4 +- internal/approval/clock.go | 8 + internal/approval/decide.go | 5 +- internal/approval/grantkeys_test.go | 46 ++ internal/approval/request.go | 7 +- internal/approval/sshgrant_attack_test.go | 65 ++ internal/broker/concurrent_test.go | 42 +- internal/broker/ssh.go | 7 + internal/client/child_linux_test.go | 4 + internal/client/sshgrant.go | 320 +++++++++ internal/client/sshgrant_attack_test.go | 217 ++++++ internal/client/sshgrant_helpers_test.go | 424 +++++++++++ internal/client/sshgrant_linux_test.go | 136 ++++ internal/client/sshgrant_test.go | 822 ++++++++++++++++++++++ internal/telegram/sshgrant_text_test.go | 112 +++ 21 files changed, 2392 insertions(+), 21 deletions(-) create mode 100644 cmd/nodkey/sshgrant.go create mode 100644 cmd/nodkey/sshgrant_test.go create mode 100644 internal/approval/grantkeys_test.go create mode 100644 internal/approval/sshgrant_attack_test.go create mode 100644 internal/client/sshgrant.go create mode 100644 internal/client/sshgrant_attack_test.go create mode 100644 internal/client/sshgrant_helpers_test.go create mode 100644 internal/client/sshgrant_linux_test.go create mode 100644 internal/client/sshgrant_test.go create mode 100644 internal/telegram/sshgrant_text_test.go diff --git a/AGENTS.md b/AGENTS.md index 805b751..b82522a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -24,8 +24,8 @@ Planned. Each v1 issue adds its part. parts and runs it. - `internal/client/` - the client side: the requests to the broker (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). + `nodkey run` (DESIGN.md sections 5 and 15), `nodkey ssh-agent` + (sections 6 and 15) and `nodkey ssh-grant` (sections 13 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 2fe06fc..94ba583 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -902,9 +902,12 @@ name exactly one client. The config check refuses all of these (section `window` rule. N is then the shortest window among them, in whole minutes, rounded down, so a grant is never longer than a window. The tap makes one grant for each secret. -- A grant ends N minutes after the tap. It starts only once the `decision` - entry of the tap is on disk (section 9). If that entry cannot be written, - the grant never starts. +- A grant ends N minutes after the tap. A message shows the time of the + tap to the second, and cuts the part of a second, so the end is cut to + the start of its second too, on both clocks: a tap at 14:02:41.7 gives + a grant until 14:17:41.0. It starts only once the `decision` entry of the + tap is on disk (section 9). If that entry cannot be written, the grant + never starts. - A Deny ends the grants of that client for each secret or key of the denied request, at once. For a key, that is every grant of the client for the key: at every host key and for every remote user, as wide as the @@ -1045,7 +1048,9 @@ later step fails after the approval, such as a store or the signature, an `error` entry and a `denied` entry with cause `error` follow instead. If the client hangs up after the approval, before anything was handed out, a `denied` entry with cause `cancelled` follows. An approved SSH grant hands -out nothing, so it ends with its `decision`. When grants and a "never ask" +out nothing, so it ends with its `decision`, unless its client hung up +before the grants started: then a `denied` entry with cause `cancelled` +follows, and no grant starts (section 13). When grants and a "never ask" rule allow one request together, it gets two `grant_used` entries, one for each, and each lists the secrets it covered. @@ -1579,18 +1584,21 @@ ansible-playbook site.yml `ssh-grant` sends one request. The message lists the key, the remote user, every host by its name in the broker's `known_hosts`, and the time the grant ends. That time is fixed when the request comes: its time plus -`--for`, in whole minutes, rounded down. A late tap does not move it. The -message has only "Approve" (until that time) and "Deny". It waits for an -answer as long as a request for secrets (`timeout`), because it is not an -SSH login, but never past the time the grants end: after that, an -approval would grant nothing. One tap makes a grant for each host key of -each listed host (a host can have one host key of each key type), exactly -as in B. +`--for`, in whole minutes, rounded down. The message shows it to the +second, so it is cut to the start of that second, on both clocks +(section 8): a request at 14:05:12.7 for 15 minutes gives grants until +14:20:12.0. A late tap does not move it. The message has only "Approve" +(until that time) and "Deny". It waits for an answer as long as a request +for secrets (`timeout`), because it is not an SSH login, but never past +the time the grants end: after that, an approval would grant nothing. One +tap makes a grant for each host key of each listed host (a host can have +one host key of each key type), exactly as in B. Then the playbook runs without more taps. A host that is not in the list still asks. A host that is listed twice counts once (but the list may have at most 20 names, section 10). The client sends `--for` in seconds; the broker rounds down to whole minutes. The broker refuses an `ssh-grant` before any message when: +- the key is not in the broker config, or this client may not use it; - the remote user name does not match the pattern in section 6; - the key's rule is not `window`; - `--for` is longer than the key's window, or shorter than one minute; @@ -1598,6 +1606,60 @@ before any message when: list when it has one; - it lists no host, or more than 20 hosts. +Each login under these grants still passes every check of section 6, and +gets its own audit entries (`request`, `grant_used`, `signed`). A Deny for +the key ends the grants at once (section 8). + +The grants are all that an approval of `ssh-grant` gives. So when the +client hangs up after the tap, such as after `SIGINT`, the broker starts +no grant. It looks once more, after the `decision` entry is on disk and +just before it starts the grants. Then a `denied` entry with cause +`cancelled` follows, and the client says `cancelled`. One tiny window +remains: when the client stops so late that the broker does not see the +hang-up at that look, the grants start, but the client still says +`cancelled`. The audit log shows which case it was: a `decision` entry +without a `denied` entry after it means that the grants may have +started (a Deny for the key in between also leaves no `denied` entry, +and then no grant starts). The +message on the phone shows the approval in both cases. A Deny for the key +ends such grants. + +The client checks its arguments first, to give a clear error, and exits +with 64 without a request: the key name and each host name follow the +name rule of section 14, the remote user name matches the pattern of +section 6, there are 1 to 20 hosts, each given once with its own +`--host`, and `--for` is a time in whole minutes, such as `15m` or `1h`, +from 1 minute to 8 hours, the longest window. The client cannot see the +key's rule, its window or the broker's `known_hosts`. The broker does not +rely on the client's checks: it makes its own checks, listed above. Two +checks of the client are no refusals on the broker: there, a host that is +listed twice counts once, and a time that is not whole minutes is rounded +down. The reason comes from `--reason`, or else from `NODKEY_REASON`, as +for `nodkey run`. The message shows it as claimed text. The client sends +no command line. + +The client takes the answer as `nodkey run` does (sections 5 and 10). It +sends the request once, with a deadline of 15 minutes, and prints the +short code of the `102 Processing` answer. Only a final `200` that passes +the strict decoding counts. Its `minutes` is how long the grants last, +from the time of the request. The client takes it only from 1 to the +minutes of `--for`. Any other number is an error (69), and the client +does not print it. After an approval, the last line names the remote +user, the number of hosts, and the end of the grants on the clock of the +agent machine. It shows the end, not the minutes: after a late tap, less +time is left. For example: + +``` +nodkey: waiting for approval (request m4k2) +nodkey: SSH logins as deploy to 2 hosts allowed until 14:20 (exit 0) +``` + +That end is the time when the client sent the request, plus the minutes, +without the seconds. So, when the two clocks agree, it is not later than +the end that the message shows. Only the broker's clocks decide when the +grants end (section 16). The line also does not see a later Deny that +ends the grants. + **Why B and D:** the host is the most useful thing a person checks, and both keep it in every grant. C is too wide: approving `web-1` would silently allow `db-1`. D turns the six taps of B into one, without making the scope wider. @@ -1968,6 +2030,33 @@ last line is `nodkey: (exit )`, for example `nodkey: denied (exit 77)`. When the command runs, nodkey prints nothing after it. +`nodkey ssh-grant` exits with: + +| code | meaning | +|---|---| +| 0 | the person approved: the grants are made | +| 64 | wrong usage | +| 69 | the broker cannot be reached | +| 77 | denied, expired or not allowed | + +It reads the broker's answers as `nodkey run` does: 77 for `403` and +`429`, and 69 for every other answer that is not `200`, and when the +config, the TLS key or the deadline fails. It also exits with 69 for a +`200` that is not a valid answer, or whose `minutes` is not 1 to the +minutes of `--for` (section 13): the person approved, and the broker is +at fault. It never tries again. `SIGINT` and `SIGTERM` while it waits +cancel the request, as for `nodkey run`: the last line is `nodkey: +cancelled by SIGINT (exit 130)`, or `SIGTERM` and 143, and then nodkey +ends itself with the same signal. Also after the tap, the broker then +starts no grant, except in a tiny window: when the signal comes just as +the broker starts the grants, they start anyway (section 13). The audit +log shows it. A signal that comes after the answer was read changes +nothing: the grants are made, and the last line says so. `-h` and +`--help` work as for `nodkey run`. It prints nothing +on stdout. Its last line on stderr is always +`nodkey: (exit )`, also after an approval, for example +`nodkey: SSH logins as deploy to 2 hosts allowed until 14:20 (exit 0)`. + `nodkey keygen` prints the pin on stdout, and nothing else. It exits with 0, with 64 on wrong usage, and with 1 on any other error. Then its last line on stderr is `nodkey: (exit 1)`. diff --git a/THREAT-MODEL.md b/THREAT-MODEL.md index d89de81..1985272 100644 --- a/THREAT-MODEL.md +++ b/THREAT-MODEL.md @@ -100,7 +100,18 @@ remains. **T1. The agent asks for a secret it should not have.** Only names in the broker config exist, and each lists the clients that may use it. Unknown names are denied without a message. -Risk that remains: none beyond the rules the approver wrote. +The answer tells the names apart, though. The broker checks the names +before the limits of DESIGN.md section 8. So when a limit denies a +request, such as in the pause of 60 seconds after a Deny, a request with +valid names gets `429`, and one with a name that does not exist, or that +the client may not use, gets `403`, both without a message. So an agent +can learn, without a message, which secret names it may use, and with +`nodkey ssh-grant` which hosts are in the broker's `known_hosts` and in +the `hosts` of a key, and the rule and window of a key. We accept this small leak: these are names, not +secret values. (The client may list its key names anyway, with +`/v1/keys`.) +Risk that remains: the agent can learn the names of the secrets and hosts +that it may use; nothing beyond the rules the approver wrote. **T2. The agent lies about the command or the reason.** These fields are marked as claimed, below the verified ones. For @@ -233,8 +244,13 @@ Risk that remains: real. nodkey cannot see which device pressed a button. A passkey page is a possible later step. **T8. Approval fatigue.** The person taps Approve without reading. -Grants for N minutes and `ssh-grant` cut the number of taps. Limits stop -floods. The message is short and puts the verified lines first. Notices +Grants for N minutes and `ssh-grant` cut the number of taps. One tap on +an `ssh-grant` message allows exactly what its verified lines show: logins +by that client, with that key, as that remote user, to each listed host +(by its host keys in the broker's `known_hosts`), until the end shown. +The broker checks all of it itself; the checks of the client only give +quick errors (DESIGN.md section 13). Limits stop floods. The message is +short and puts the verified lines first. Notices about "never ask" uses can go to a separate chat, so the approval chat stays for real decisions. When the person sees something strange and taps Deny, the client's grants for that secret or key end at once (for a key: diff --git a/cmd/nodkey/main.go b/cmd/nodkey/main.go index 386d49d..1c4a678 100644 --- a/cmd/nodkey/main.go +++ b/cmd/nodkey/main.go @@ -36,6 +36,7 @@ Commands: 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 + ssh-grant ask once for SSH logins to a list of hosts, for some minutes audit verify FILE [--anchor SEQ:HASH]... [--anchors FILE|-] check the hash chain of an audit log, and its anchors version print the version @@ -73,6 +74,8 @@ func run(args []string, stdin io.Reader, stdout, stderr io.Writer) int { return runRun(rest, stderr) case "ssh-agent": return runSSHAgent(rest, stderr) + case "ssh-grant": + return runSSHGrant(rest, stderr) case "version": return runVersion(rest, stdout, stderr) case "help": diff --git a/cmd/nodkey/sshgrant.go b/cmd/nodkey/sshgrant.go new file mode 100644 index 0000000..3488fc7 --- /dev/null +++ b/cmd/nodkey/sshgrant.go @@ -0,0 +1,14 @@ +package main + +import ( + "io" + + "github.com/code6x/nodkey/internal/client" +) + +// runSSHGrant runs nodkey ssh-grant (package client): it asks once for SSH +// logins as one user to a list of hosts, for some minutes (DESIGN.md +// section 13). +func runSSHGrant(args []string, stderr io.Writer) int { + return client.SSHGrant(args, stderr) +} diff --git a/cmd/nodkey/sshgrant_test.go b/cmd/nodkey/sshgrant_test.go new file mode 100644 index 0000000..3836889 --- /dev/null +++ b/cmd/nodkey/sshgrant_test.go @@ -0,0 +1,40 @@ +package main + +import ( + "bytes" + "strings" + "testing" +) + +// TestSSHGrantCommand: nodkey ssh-grant is a command of nodkey. Wrong usage +// prints its usage, and the last line says why, with exit code 64. --help +// alone prints the usage and exits with 0. help lists the command. It +// never writes to stdout. +func TestSSHGrantCommand(t *testing.T) { + var stdout, stderr bytes.Buffer + if code := run([]string{"ssh-grant"}, strings.NewReader(""), &stdout, &stderr); code != exitUsage { + t.Errorf("nodkey ssh-grant: exit code %d, want %d", code, exitUsage) + } + if !strings.HasPrefix(stderr.String(), "Usage: nodkey ssh-grant ") || + !strings.HasSuffix(stderr.String(), "give the key with --key NAME (exit 64)\n") { + t.Errorf("stderr = %q", stderr.String()) + } + stderr.Reset() + args := []string{"ssh-grant", "--key", "deploy", "--user", "deploy@web-1", "--host", "web-1", "--for", "15m"} + if code := run(args, strings.NewReader(""), &stdout, &stderr); code != exitUsage || + !strings.HasSuffix(stderr.String(), "must match [A-Za-z0-9._][A-Za-z0-9._-]{0,31} (exit 64)\n") { + t.Errorf("a bad user name: exit code %d, stderr %q; want 64", code, stderr.String()) + } + stderr.Reset() + if code := run([]string{"ssh-grant", "--help"}, strings.NewReader(""), &stdout, &stderr); code != exitOK || + !strings.HasPrefix(stderr.String(), "Usage: nodkey ssh-grant ") { + t.Errorf("nodkey ssh-grant --help: exit code %d, stderr %q; want 0 and the usage", code, stderr.String()) + } + if stdout.Len() != 0 { + t.Errorf("stdout = %q, want it empty", stdout.String()) + } + if run([]string{"help"}, strings.NewReader(""), &stdout, &bytes.Buffer{}); !strings.Contains(stdout.String(), + "\n ssh-grant ") { + t.Errorf("help does not list ssh-grant: %q", stdout.String()) + } +} diff --git a/internal/approval/check.go b/internal/approval/check.go index 86a24d9..a0608fb 100644 --- a/internal/approval/check.go +++ b/internal/approval/check.go @@ -190,7 +190,9 @@ func (c *Core) checkGrant(r *Request, p *pending) bool { return false } p.windowMinutes = minutes - p.grantEnds = p.start.add(time.Duration(minutes) * time.Minute) + // The message shows the end to the second. The grants end at the + // start of that second, not later. + p.grantEnds = p.start.add(time.Duration(minutes) * time.Minute).wholeSecond() return true } diff --git a/internal/approval/clock.go b/internal/approval/clock.go index 0a5472b..2ffdf11 100644 --- a/internal/approval/clock.go +++ b/internal/approval/clock.go @@ -70,6 +70,14 @@ func (m moment) add(d time.Duration) moment { return moment{mono: m.mono.Add(d), wall: m.wall.Add(d)} } +// wholeSecond returns m moved back to the start of its second on the wall +// clock, on both clocks. A message shows a time to the second, and cuts +// the part of a second. So an end made with wholeSecond never comes after +// the second that the person reads. +func (m moment) wholeSecond() moment { + return m.add(-m.wall.Sub(m.wall.Truncate(time.Second))) +} + // before reports whether m comes before end on both clocks. So an end has // come as soon as either clock reaches it. func (m moment) before(end moment) bool { diff --git a/internal/approval/decide.go b/internal/approval/decide.go index 72473e4..d135618 100644 --- a/internal/approval/decide.go +++ b/internal/approval/decide.go @@ -494,7 +494,10 @@ func (c *Core) Answer(a Answer) error { res.Allowed = true res.WindowMinutes = p.windowMinutes res.grants = p.newGrants() - res.grantEnd = now.add(time.Duration(p.windowMinutes) * time.Minute) + // The message shows the time of the tap to the second, and the + // minutes. The grants end at the start of the second that they + // give, not later. + res.grantEnd = now.add(time.Duration(p.windowMinutes) * time.Minute).wholeSecond() if p.kind == audit.KindSSHGrant { res.grantEnd = p.grantEnds // the end shown in the message } diff --git a/internal/approval/grantkeys_test.go b/internal/approval/grantkeys_test.go new file mode 100644 index 0000000..658a35b --- /dev/null +++ b/internal/approval/grantkeys_test.go @@ -0,0 +1,46 @@ +package approval_test + +import ( + "bytes" + "crypto/ed25519" + "testing" + "testing/synctest" + "time" + + "golang.org/x/crypto/ssh" +) + +// TestSSHGrantKeepsHostKeyBytes: KeysFor of the broker's known_hosts +// returns the keys that it holds, not copies of them, so a caller could +// change them (from the review of issue #9). The core keeps each host key +// of an SSH grant as its SSH wire bytes, made when the request comes. So +// a later change to such a key changes no grant: the grants cover the +// host keys that the person approved. A listed host is covered, and a +// host that is not in the list still asks. +func TestSSHGrantKeepsHostKeyBytes(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + e := newEnv(t, nil) + defer e.stop() + h := e.hosts + e.approveWindow(grantReq("laptop", "deploy", "deploy", 15*time.Minute, "web-1"), 1) + + // Change the Ed25519 host key of web-1 in place, as a careless + // caller of KeysFor could. + changed := false + for _, k := range e.cfg.KnownHosts.KeysFor("web-1") { + if pub, ok := k.(ssh.CryptoPublicKey).CryptoPublicKey().(ed25519.PublicKey); ok { + pub[0] ^= 0xff + changed = true + } + } + if !changed { + t.Fatal("web-1 has no Ed25519 host key to change") + } + if bytes.Equal(e.cfg.KnownHosts.KeysFor("web-1")[0].Marshal(), h.web1.Marshal()) { + t.Fatal("the change did not reach the known_hosts; the test does not test") + } + checkAllowed(t, e.now(loginReq("laptop", "deploy", h.web1, "deploy"))) + checkAllowed(t, e.now(loginReq("laptop", "deploy", h.web1ecdsa, "deploy"))) + e.needsTap(loginReq("laptop", "deploy", h.web2, "deploy"), 2) + }) +} diff --git a/internal/approval/request.go b/internal/approval/request.go index e648d2e..983f27e 100644 --- a/internal/approval/request.go +++ b/internal/approval/request.go @@ -153,8 +153,11 @@ func (r *Result) Events() []audit.Event { // then never start. So a grant never exists without its record. // // The grants end when the answer said: N minutes after the tap, or at the -// end shown in the message of an SSH grant. Calling StartGrants later -// does not make them last longer. A grant for a secret or key that the +// end shown in the message of an SSH grant, each cut to the start of its +// second, as a message shows a time. Calling StartGrants later does not +// make them last longer. For an SSH grant, the broker does not call it +// when the client hung up after the tap: the grants are all it gives. A +// grant for a secret or key that the // person denied after this tap does not start: a Deny ends the client's // grants for it, also those that have not started yet. For any other // result, and on a second call, StartGrants does nothing. diff --git a/internal/approval/sshgrant_attack_test.go b/internal/approval/sshgrant_attack_test.go new file mode 100644 index 0000000..211f04e --- /dev/null +++ b/internal/approval/sshgrant_attack_test.go @@ -0,0 +1,65 @@ +package approval_test + +import ( + "testing" + "testing/synctest" + "time" + + "github.com/code6x/nodkey/internal/approval/approvaltest" + "github.com/code6x/nodkey/internal/audit" +) + +// The format of a time of day in a message (as in package telegram): the +// part of a second is cut, not rounded. +const shownClock = "15:04:05" + +// TestSSHGrantEndsAtTheShownSecond: a request for an SSH grant comes at +// 12:00:00.7, with --for 15m. The message shows that the grants end at +// 12:15:00, so they end then, and not at 12:15:00.7. (From the security +// review of issue #14: a login at 12:15:00.5 was still signed.) +func TestSSHGrantEndsAtTheShownSecond(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + cfg, h := testConfig(t) + e := newEnvWith(t, cfg, h, approvaltest.NewClock(noon.Add(700*time.Millisecond))) + defer e.stop() + ch := e.decide(grantReq("laptop", "deploy", "deploy", 15*time.Minute, "web-1")) + mustWait(t, ch) + p := e.asked(1)[0].Prompt + ends := noon.Add(15 * time.Minute) + if !p.GrantEnds.Equal(ends) || p.GrantEnds.Format(shownClock) != "12:15:00" { + t.Fatalf("the grants end at %s; want exactly %s, the second that the message shows", + p.GrantEnds.Format("15:04:05.000"), ends.Format("15:04:05.000")) + } + e.tap(1, audit.AnswerApproveWindow) + r := mustResult(t, ch) + checkAllowed(t, r) + r.StartGrants() + + e.clock.Advance(ends.Add(-100 * time.Millisecond).Sub(e.clock.Now())) // 12:14:59.9 + checkAllowed(t, e.now(loginReq("laptop", "deploy", h.web1, "deploy"))) + e.clock.Advance(100 * time.Millisecond) // 12:15:00.0: the end shown + e.needsTap(loginReq("laptop", "deploy", h.web1, "deploy"), 2) + }) +} + +// TestWindowGrantEndsAtAWholeSecond: the same for "Approve 15 min". The +// person taps at 12:00:00.7, and the message then shows "approved for 15 +// min by you at 12:00:00". So the grant ends at 12:15:00, not later. +func TestWindowGrantEndsAtAWholeSecond(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + cfg, h := testConfig(t) + e := newEnvWith(t, cfg, h, approvaltest.NewClock(noon.Add(700*time.Millisecond))) + defer e.stop() + e.approveWindow(secretsReq("laptop", "github-token"), 1) + ended := e.fake.Ended() + if len(ended) != 1 || ended[0].Ending.At.Format(shownClock) != "12:00:00" || + ended[0].Ending.WindowMinutes != 15 { + t.Fatalf("the message after the tap shows %+v; want 15 min at 12:00:00", ended) + } + ends := noon.Add(15 * time.Minute) + e.clock.Advance(ends.Add(-100 * time.Millisecond).Sub(e.clock.Now())) // 12:14:59.9 + checkAllowed(t, e.now(secretsReq("laptop", "github-token"))) + e.clock.Advance(100 * time.Millisecond) // 12:15:00.0 + e.needsTap(secretsReq("laptop", "github-token"), 2) + }) +} diff --git a/internal/broker/concurrent_test.go b/internal/broker/concurrent_test.go index 64ce3f0..e2ab357 100644 --- a/internal/broker/concurrent_test.go +++ b/internal/broker/concurrent_test.go @@ -143,6 +143,40 @@ func TestHangUpBeforeSign(t *testing.T) { mustEvents(t, requestEvents(e.audit.waitFor(t, 3), id), "request decision:approve_once denied:cancelled") } +// TestHangUpBeforeGrants: the same for an SSH grant. The person approves, +// and the client hangs up while the decision entry is written, as after +// SIGINT. Its grants are all that the approval gives, so none starts: a +// login to a listed host still asks. +func TestHangUpBeforeGrants(t *testing.T) { + wrap, contexts := requestContexts() + e := newEnv(t, envOptions{wrap: wrap}) + gate := make(chan struct{}) + e.audit.setFail(func(ev audit.Event) error { + if _, ok := ev.(audit.Decision); ok { + <-gate + } + return nil + }) + c := e.httpClient("laptop") + p := e.start(c, api.PathSSHGrant, api.GrantRequest{ + Key: "deploy", User: "deploy", Hosts: []string{"web-1"}, Seconds: 15 * 60, Reason: "r", + }) + ctx := <-contexts + id := e.waitAsked(1)[0].Prompt.RequestID + e.tap(id, audit.AnswerApproveWindow) + time.Sleep(100 * time.Millisecond) + p.cancel() + p.wait(t) + waitDone(t, ctx) + close(gate) + mustEvents(t, requestEvents(e.audit.waitFor(t, 3), id), "request decision:approve_window denied:cancelled") + login := e.start(c, api.PathSign, e.signRequest(t, "deploy", "web-1", "deploy")) + <-contexts + e.waitAsked(2) + login.cancel() + login.wait(t) +} + // TestTwoLoginsAtOnce: two sign requests of one client wait at the // same time, for two remote users. The person approves the one for root // only. Its signature is over its own data, and the other gets 403. @@ -178,7 +212,8 @@ func TestTwoLoginsAtOnce(t *testing.T) { } } -// waitDone waits until ctx has ended. +// waitDone waits until ctx, the context of a request, has ended, and the +// handler's context with it. func waitDone(t testing.TB, ctx context.Context) { t.Helper() select { @@ -186,4 +221,9 @@ func waitDone(t testing.TB, ctx context.Context) { case <-time.After(waitLimit): t.Fatal("the server did not see the client hang up") } + // Package context closes the Done channel of ctx before it ends the + // contexts made from it, such as the handler's own, and it does all of + // that under the lock of ctx. context.Cause takes that lock: once it + // returns, the handler's context has ended too. + _ = context.Cause(ctx) } diff --git a/internal/broker/ssh.go b/internal/broker/ssh.go index 8621569..5dd0b4c 100644 --- a/internal/broker/ssh.go +++ b/internal/broker/ssh.go @@ -147,6 +147,13 @@ func (rq *request) grant(ctx context.Context, r *http.Request) { if !ok { return } + // The grants are all that an approval gives. When the client has hung + // up, such as after SIGINT, it tells the agent that the request was + // cancelled: then no grant starts. + if ctx.Err() != nil { + rq.cancelled() + return + } res.StartGrants() rq.ended = true rq.answerJSON(api.GrantAnswer{Minutes: res.WindowMinutes}) diff --git a/internal/client/child_linux_test.go b/internal/client/child_linux_test.go index f2a0a12..d28aa42 100644 --- a/internal/client/child_linux_test.go +++ b/internal/client/child_linux_test.go @@ -388,6 +388,10 @@ func TestRunInterrupted(t *testing.T) { case <-time.After(waitLimit): t.Fatal("no waiting line") } + // The broker sends the short code just before the message. A + // signal before the message would end the request without one, + // and then no message is updated. + tb.waitAsked(t, 1) if err := cmd.Process.Signal(sig); err != nil { t.Fatal(err) } diff --git a/internal/client/sshgrant.go b/internal/client/sshgrant.go new file mode 100644 index 0000000..53de10f --- /dev/null +++ b/internal/client/sshgrant.go @@ -0,0 +1,320 @@ +package client + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "os" + "regexp" + "strconv" + "strings" + "time" + + "github.com/code6x/nodkey/internal/api" + "github.com/code6x/nodkey/internal/config" + "github.com/code6x/nodkey/internal/sshauth" + "github.com/code6x/nodkey/internal/transport" +) + +// grantUsage is the help of nodkey ssh-grant. +const grantUsage = `Usage: nodkey ssh-grant --key NAME --user USER --host NAME... --for DURATION [--reason TEXT] + +Ask the broker once for SSH logins as USER with the key NAME to a list of +hosts, for DURATION, and wait until a person approves the request on the +phone. Until the grants end, ssh, git and Ansible through nodkey ssh-agent +log in to these hosts as USER without another tap. Other hosts, users and +keys still ask. + + --key NAME a key of the broker; its rule must be "window" + --user USER the remote user; it must match + [A-Za-z0-9._][A-Za-z0-9._-]{0,31} + --host NAME a host by its name in the broker's known_hosts; 1 to 20 + hosts, each with its own --host + --for DURATION how long the grants last, in whole minutes, such as 15m + or 1h; at most the window of the key + --reason TEXT why the logins are needed. Without it, the reason is the + environment variable NODKEY_REASON. + +The client config is ~/.config/nodkey/client.toml. nodkey ssh-grant exits +with 0 when the person approved, or with 64 (wrong usage), 69 (the broker +cannot be reached) or 77 (denied). SIGINT or SIGTERM while it waits +cancel the request. +` + +// grantNamePattern is the name rule of the broker config for keys and +// known hosts (DESIGN.md section 14). The client checks the names with it +// to give a quick error; the broker checks them again. +var grantNamePattern = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9._-]{0,63}$`) + +// grantNameRule says what grantNamePattern allows, for errors. +const grantNameRule = `1 to 64 letters, digits, ".", "_" or "-", starting with a letter or digit` + +// grantClock is the format of the end of the grants in the last line: the +// hour and the minute. The seconds are cut off, so the end that the line +// shows is never later than the end that it stands for. +const grantClock = "15:04" + +// SSHGrant runs nodkey ssh-grant with args, the arguments after +// "ssh-grant", and writes its own messages to stderr (DESIGN.md sections +// 13 and 15). It returns the exit code. Its last line on stderr is always +// "nodkey: (exit )". After an approval, the result names the +// remote user, the number of hosts, and when the grants end. +// +// As nodkey run, it catches SIGINT and SIGTERM while it waits. When one +// comes, it cancels the request: the connection closes, so the broker +// cancels the request too. Then it prints its last line and ends nodkey +// with the same signal, so that a shell sees it. +func SSHGrant(args []string, stderr io.Writer) int { + g := granter{load: Load, configPath: ConfigPath, environ: os.Environ, now: time.Now} + return g.main(args, stderr) +} + +// granter runs nodkey ssh-grant. Tests replace its parts. +type granter 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) + // environ returns the environment: os.Environ. + environ func() []string + // now returns the time of this machine's clock, for the end of the + // grants in the last line: time.Now. + now func() time.Time +} + +// main is SSHGrant with the parts of g. +func (g granter) main(args []string, stderr io.Writer) int { + ctx, cancel := context.WithCancelCause(context.Background()) + defer cancel(nil) + in := watchInterrupts(cancel) + defer in.stop() + code := g.run(ctx, args, stderr) + var sig interrupted + if errors.As(context.Cause(ctx), &sig) && code == sig.status() { + // run printed "cancelled by SIG... (exit 128+N)". A shell stops a + // loop or a script only when a command dies of SIGINT. + in.stop() + raise(sig.sig) + } + return code +} + +// run runs nodkey ssh-grant, and returns the exit code. When ctx ends while +// the request waits, run cancels the request, and returns the code of +// cancelled. +func (g granter) run(ctx context.Context, args []string, w io.Writer) int { + stderr := &lockedWriter{w: w} + if len(args) == 1 && isHelp(args[0]) { + _, _ = io.WriteString(stderr, grantUsage) + return exitOK + } + o, err := parseGrantArgs(args) + if err != nil { + _, _ = io.WriteString(stderr, grantUsage) + return fail(stderr, exitUsage, err.Error()) + } + reason := o.reason + if !o.reasonSet { + reason = lookupEnv(g.environ(), reasonVar) + } + body, err := encodeGrantRequest(newGrantRequest(o, reason)) + if err != nil { + return fail(stderr, exitUsage, err.Error()) + } + configPath, err := g.configPath() + if err != nil { + return fail(stderr, exitUnavailable, err.Error()) + } + b, err := g.load(configPath) + if err != nil { + return fail(stderr, exitUnavailable, err.Error()) + } + // The broker fixes the end of the grants when the request comes: its + // time plus the minutes. This time is taken before the request goes + // out, so the end in the last line is not later than the real end, + // when the two clocks agree. The broker's clocks decide. + sent := g.now() + ans, err := Post[api.GrantAnswer](ctx, b, api.PathSSHGrant, body, func(code string) { + _, _ = fmt.Fprintf(stderr, "nodkey: waiting for approval (request %s)\n", code) + }) + if err != nil { + return requestFailed(ctx, stderr, err) + } + minutes := int(o.duration / time.Minute) + if ans.Minutes < 1 || ans.Minutes > minutes { + // The person approved, and the broker sent a wrong answer. The + // error does not show the number from the answer. + return fail(stderr, exitUnavailable, + fmt.Sprintf("the answer of the broker is not valid: its time is not 1 to %d minutes", minutes)) + } + // The grants end at a time that the broker fixed when the request + // came, also after a late tap. So the line shows that end, not the + // minutes. A signal that comes now does not change the result: the + // grants are made. + until := sent.Add(time.Duration(ans.Minutes) * time.Minute) + _, _ = fmt.Fprintf(stderr, "nodkey: SSH logins as %s to %s allowed until %s (exit %d)\n", + o.user, grantHostCount(len(o.hosts)), until.Format(grantClock), exitOK) + return exitOK +} + +// grantHostCount returns "1 host" or "N hosts". +func grantHostCount(n int) string { + if n == 1 { + return "1 host" + } + return strconv.Itoa(n) + " hosts" +} + +// grantOptions are the arguments of nodkey ssh-grant. +type grantOptions struct { + key string + user string + hosts []string // in the order of the arguments, each once + duration time.Duration // --for: whole minutes, from 1 minute to config.MaxWindow + reason string + reasonSet bool +} + +// parseGrantArgs reads the arguments of nodkey ssh-grant. They are only +// options: --key NAME, --user USER, --host NAME, which may repeat, --for +// DURATION and --reason TEXT, each also with one dash and with "=" before +// the value, as the flag package reads them. +// +// It checks what the broker checks too, to give a quick error: the key +// name and each host name follow the name rule of the config, the remote +// user matches sshauth.ValidUser, there are 1 to api.MaxGrantHosts hosts, +// each only once, and --for is a time in whole minutes, from 1 minute to +// config.MaxWindow. The broker checks all of it again, and more: the rule +// and the window of the key, and that each host is in its known_hosts and +// in the hosts of the key. +func parseGrantArgs(args []string) (*grantOptions, error) { + o := &grantOptions{} + given := make(map[string]bool) // the options that may come only once + hosts := make(map[string]bool) + for rest := args; len(rest) > 0; { + arg := rest[0] + rest = rest[1:] + name, value, hasValue := splitOption(arg) + switch name { + case "key", "user", "host", "for", "reason": + case "h", "help": + // With other arguments, help would exit with 0 and ask + // nothing, which looks like a request that was approved. + return nil, errors.New("--help must be the only argument") + default: + if !strings.HasPrefix(arg, "-") { + return nil, fmt.Errorf("unexpected argument %.64q; put each host after its own --host", arg) + } + return nil, fmt.Errorf("unknown option %.64q", arg) + } + if !hasValue { + if len(rest) == 0 { + return nil, fmt.Errorf("--%s needs a value", name) + } + value, rest = rest[0], rest[1:] + } + if name != "host" { + if given[name] { + return nil, fmt.Errorf("--%s is given twice", name) + } + given[name] = true + } + var err error + switch name { + case "key": + if !grantNamePattern.MatchString(value) { + err = fmt.Errorf("--key: the key name %.64q must be %s", value, grantNameRule) + } + o.key = value + case "user": + if !sshauth.ValidUser(value) { + err = fmt.Errorf("--user: the remote user name %.64q must match [A-Za-z0-9._][A-Za-z0-9._-]{0,31}", + value) + } + o.user = value + case "host": + err = addGrantHost(o, hosts, value) + case "for": + o.duration, err = parseGrantDuration(value) + case "reason": + o.reason, o.reasonSet = value, true + } + if err != nil { + return nil, err + } + } + switch { + case !given["key"]: + return nil, errors.New("give the key with --key NAME") + case !given["user"]: + return nil, errors.New("give the remote user with --user USER") + case len(o.hosts) == 0: + return nil, errors.New("give at least one host with --host NAME") + case !given["for"]: + return nil, errors.New("give the time of the grants with --for DURATION, such as --for 15m") + } + return o, nil +} + +// addGrantHost adds the value of one --host to o. hosts holds the hosts +// that o has already. +func addGrantHost(o *grantOptions, hosts map[string]bool, host string) error { + switch { + case !grantNamePattern.MatchString(host): + return fmt.Errorf("--host: %.64q is not a host name: a name in the broker's known_hosts is %s; "+ + "give each host with its own --host", host, grantNameRule) + case hosts[host]: + return fmt.Errorf("--host: the host %s is given twice", host) + case len(o.hosts) == api.MaxGrantHosts: + return fmt.Errorf("at most %d --host options", api.MaxGrantHosts) + } + hosts[host] = true + o.hosts = append(o.hosts, host) + return nil +} + +// parseGrantDuration reads the value of --for: a time in the form of Go, +// such as 15m, 90m or 1h30m, in whole minutes, from 1 minute to +// config.MaxWindow, the longest window of a key. +func parseGrantDuration(value string) (time.Duration, error) { + d, err := time.ParseDuration(value) + switch { + case err != nil, d < time.Minute, d%time.Minute != 0: + return 0, fmt.Errorf("--for %.64q: give a time in whole minutes, such as 15m or 1h", value) + case d > config.MaxWindow: + return 0, fmt.Errorf("--for %.64q: the grants can last at most %d hours, the longest window of a key", + value, int(config.MaxWindow/time.Hour)) + } + return d, nil +} + +// newGrantRequest makes the request for SSH grants (DESIGN.md sections 10 +// and 13): the key, the remote user, the hosts, --for in seconds, and the +// claimed reason. A reason over api.MaxText bytes becomes "[hidden: too +// long]", as for nodkey run. +func newGrantRequest(o *grantOptions, reason string) api.GrantRequest { + return api.GrantRequest{ + Key: o.key, + User: o.user, + Hosts: o.hosts, + Seconds: int64(o.duration / time.Second), + Reason: claimedText(reason), + } +} + +// encodeGrantRequest 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 newGrantRequest with the +// options of parseGrantArgs passes: an error here is a bug. +func encodeGrantRequest(req api.GrantRequest) ([]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 +} diff --git a/internal/client/sshgrant_attack_test.go b/internal/client/sshgrant_attack_test.go new file mode 100644 index 0000000..3bfa0d7 --- /dev/null +++ b/internal/client/sshgrant_attack_test.go @@ -0,0 +1,217 @@ +package client + +import ( + "context" + "errors" + "net/http" + "slices" + "strings" + "sync" + "syscall" + "testing" + "time" + + "github.com/code6x/nodkey/internal/api" + "github.com/code6x/nodkey/internal/approval" + "github.com/code6x/nodkey/internal/audit" + "github.com/code6x/nodkey/internal/broker" +) + +// These tests come from the security review of issue #14. They act as a +// hostile agent, or as a person whose timing is bad. + +// TestSSHGrantHostSpellings: a hostile client sends host names that look +// like a name in known_hosts, but are not exactly it. The broker refuses +// each before any message, so a message never shows a name that is not in +// known_hosts, and no grant covers a host by a look-alike name. +func TestSSHGrantHostSpellings(t *testing.T) { + t.Parallel() + gb := newGrantBroker(t) + gb.approver.Auto(audit.AnswerApproveWindow) // a message would be approved at once + laptop := gb.client(t, "laptop") + for _, host := range []string{ + "web-1.", "web-1..", ".web-1", " web-1", "web-1 ", "web-1\x00", "web-1\n", "web-1\t", "Web-1", "WEB-1", + "w\u0435b-1", // a Cyrillic small ie in place of the e + "web\u20111", // a non-breaking hyphen + "web-1\u200b", // a zero width space + "\u202eweb-1", // a right-to-left override + "web-1,web-2", "web-1 web-2", "*", "web-?", "[web-1]:22", "deploy@web-1", "web-1:22", "|1|web-1", + "@revoked", "", + } { + req := api.GrantRequest{Key: "deploy", User: "deploy", Hosts: []string{host}, Seconds: 15 * 60, Reason: "r"} + _, err := Post[api.GrantAnswer](t.Context(), laptop, api.PathSSHGrant, mustJSON(t, req), nil) + var e *Error + if !errors.As(err, &e) || e.Status != http.StatusForbidden { + t.Errorf("host %q: %v; want 403", host, err) + } + } + if n := len(gb.approver.Asked()); n != 0 { + t.Fatalf("%d messages; the broker must refuse look-alike hosts before any message", n) + } +} + +// TestSSHGrantDuplicateHosts: a hostile client lists a host twice, which +// nodkey ssh-grant never does. The broker counts it once: the message +// lists each host once, in the order of the request, and the grants +// cover exactly those hosts. +func TestSSHGrantDuplicateHosts(t *testing.T) { + t.Parallel() + gb := newGrantBroker(t) + gb.approver.Auto(audit.AnswerApproveWindow) + laptop := gb.client(t, "laptop") + for i, tt := range []struct { + hosts []string + want []string + }{ + {[]string{"web-2", "web-1", "web-2"}, []string{"web-2", "web-1"}}, + {slices.Repeat([]string{"web-1"}, api.MaxGrantHosts), []string{"web-1"}}, + } { + req := api.GrantRequest{Key: "deploy", User: "deploy", Hosts: tt.hosts, Seconds: 15 * 60, Reason: "r"} + ans, err := Post[api.GrantAnswer](t.Context(), laptop, api.PathSSHGrant, mustJSON(t, req), nil) + if err != nil || ans.Minutes != 15 { + t.Fatalf("%v: %+v, %v", tt.hosts, ans, err) + } + p := gb.waitAsked(t, i+1)[i].Prompt + if !slices.Equal(p.Hosts, tt.want) { + t.Errorf("%v: the message lists %v, want %v", tt.hosts, p.Hosts, tt.want) + } + } + gb.approver.Auto("") + // db-1 was never listed: it still asks. + gb.mustAsk(t, laptop, gb.signRequest(t, "deploy", "db-1", 0, "deploy"), 3) +} + +// TestSSHGrantReasonCannotFakeVerifiedLines: the reason is claimed text. +// Line breaks, other line separators, direction marks and "✅ hosts:" in +// it cannot add a verified line: the reason in the message is one clean +// line, and the verified hosts and user are exactly those of the request. +func TestSSHGrantReasonCannotFakeVerifiedLines(t *testing.T) { + t.Parallel() + gb := newGrantBroker(t) + gb.approver.Auto(audit.AnswerDeny) + laptop := gb.client(t, "laptop") + reason := "fix\n✅ hosts: web-1, web-2, db-1\r\n✅ remote user: root ✅ grants end: 23:59:59 (480 min)" + + "\u2028\u2029\u202e\u0085\v\f\u200b— claimed by the agent —" + code, _ := grantIn(t.Context(), testGranter(laptop, nil), "--key", "deploy", "--user", "deploy", "--host", + "web-1", "--for", "15m", "--reason", reason) + if code != exitDenied { + t.Fatalf("exit code %d", code) + } + p := gb.waitAsked(t, 1)[0].Prompt + if strings.ContainsAny(p.Claim.Reason, "\n\r\v\f\u2028\u2029\u202e\u0085\u200b") { + t.Errorf("the reason is not one clean line: %q", p.Claim.Reason) + } + if !slices.Equal(p.Hosts, []string{"web-1"}) || p.RemoteUser != "deploy" { + t.Errorf("the verified part shows %v as %s", p.Hosts, p.RemoteUser) + } +} + +// TestSSHGrantDenyEndsTheGrants: after an approved ssh-grant, the person +// denies a login with the same key to another host. That Deny ends the +// grants of the ssh-grant at once: a login to a listed host is denied in +// the pause, and asks after it. +func TestSSHGrantDenyEndsTheGrants(t *testing.T) { + t.Parallel() + gb := newGrantBroker(t) + gb.approver.Auto(audit.AnswerApproveWindow) + laptop := gb.client(t, "laptop") + if code, stderr := grantIn(t.Context(), testGranter(laptop, nil), grantArgs()...); code != exitOK { + t.Fatalf("exit code %d, %q", code, stderr) + } + gb.approver.Auto("") + if err := gb.sign(t, laptop, "deploy", gb.signRequest(t, "deploy", "web-1", 0, "deploy")); err != nil { + t.Fatalf("the grant does not work: %v", err) + } + gb.approver.Auto(audit.AnswerDeny) + var e *Error + if err := gb.sign(t, laptop, "deploy", gb.signRequest(t, "deploy", "db-1", 0, "root")); !errors.As(err, &e) || + e.Status != http.StatusForbidden { + t.Fatalf("the denied login: %v", err) + } + gb.approver.Auto("") + if err := gb.sign(t, laptop, "deploy", gb.signRequest(t, "deploy", "web-1", 1, "deploy")); !errors.As(err, &e) || + e.Status != http.StatusTooManyRequests { + t.Errorf("a login to a listed host in the pause after the Deny: %v; want 429", err) + } + gb.clock.Advance(approval.DenyPause) + gb.mustAsk(t, laptop, gb.signRequest(t, "deploy", "web-2", 0, "deploy"), 3) +} + +// slowDecision is an audit log that holds the write of a decision entry +// until release is closed, as a slow disk does. +type slowDecision struct { + *memAudit + once sync.Once + reached chan struct{} // closed when a decision entry is written + release chan struct{} +} + +func (s *slowDecision) Append(ev audit.Event) (audit.Anchor, error) { + if _, ok := ev.(audit.Decision); ok { + s.once.Do(func() { close(s.reached) }) + <-s.release + } + return s.memAudit.Append(ev) +} + +// TestSSHGrantCancelAfterTheTap: the person taps Approve, and SIGINT comes +// while the broker writes the decision entry. nodkey ssh-grant says +// "cancelled by SIGINT (exit 130)", and so no grant may start: the broker +// sees the hang-up before it starts the grants, and writes a denied entry +// with cause cancelled. A login to a listed host still asks. +func TestSSHGrantCancelAfterTheTap(t *testing.T) { + t.Parallel() + slow := &slowDecision{reached: make(chan struct{}), release: make(chan struct{})} + contexts := make(chan context.Context, 8) + gb := newGrantBrokerWith(t, grantBrokerOptions{ + audit: func(m *memAudit) broker.AuditLog { slow.memAudit = m; return slow }, + contexts: contexts, + }) + var releaseOnce sync.Once + release := func() { releaseOnce.Do(func() { close(slow.release) }) } + defer release() // also when the test fails: the broker must not wait for ever + gb.approver.Auto(audit.AnswerApproveWindow) + laptop := gb.client(t, "laptop") + ctx, cancel := context.WithCancelCause(t.Context()) + defer cancel(nil) + type result struct { + code int + stderr string + } + done := make(chan result, 1) + go func() { + code, stderr := grantIn(ctx, testGranter(laptop, nil), grantArgs()...) + done <- result{code, stderr} + }() + var reqCtx context.Context + select { + case reqCtx = <-contexts: + case <-time.After(waitLimit): + t.Fatal("the request did not reach the broker") + } + select { + case <-slow.reached: + case <-time.After(waitLimit): + t.Fatal("the request did not reach its decision entry") + } + cancel(interrupted{sig: syscall.SIGINT}) + res := <-done + select { + case <-reqCtx.Done(): // the broker has seen the hang-up + case <-time.After(waitLimit): + t.Fatal("the broker did not see the client hang up") + } + // Package context closes the Done channel of reqCtx before it ends the + // contexts made from it, such as the handler's own, under the lock of + // reqCtx. context.Cause takes that lock: once it returns, the handler's + // context has ended too. + _ = context.Cause(reqCtx) + release() + if res.code != 130 || lastLine(res.stderr) != "nodkey: cancelled by SIGINT (exit 130)" { + t.Errorf("exit code %d, stderr %q; want 130", res.code, res.stderr) + } + p := gb.waitAsked(t, 1)[0].Prompt + gb.waitEvents(t, p.RequestID, "request decision:approve_window denied:cancelled") + gb.approver.Auto("") + gb.mustAsk(t, laptop, gb.signRequest(t, "deploy", "web-1", 0, "deploy"), 2) +} diff --git a/internal/client/sshgrant_helpers_test.go b/internal/client/sshgrant_helpers_test.go new file mode 100644 index 0000000..46b2ac3 --- /dev/null +++ b/internal/client/sshgrant_helpers_test.go @@ -0,0 +1,424 @@ +package client + +import ( + "bytes" + "context" + "crypto/ecdsa" + "crypto/ed25519" + "crypto/elliptic" + "crypto/rand" + "errors" + "fmt" + "net/http" + "slices" + "strings" + "testing" + "time" + + "golang.org/x/crypto/ssh" + + "github.com/code6x/nodkey/internal/api" + "github.com/code6x/nodkey/internal/approval" + "github.com/code6x/nodkey/internal/approval/approvaltest" + "github.com/code6x/nodkey/internal/audit" + "github.com/code6x/nodkey/internal/broker" + "github.com/code6x/nodkey/internal/config" + "github.com/code6x/nodkey/internal/sshauth" +) + +// The tests of nodkey ssh-grant run it against 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 the +// fake clock of package approvaltest, and an audit log in memory, as the +// tests of nodkey run do. After a grant, they ask the broker for +// signatures as nodkey ssh-agent does (POST /v1/sign with Post), with +// valid session-binds of the hosts in the broker's known_hosts. + +// grantBroker is a running broker for one test of nodkey ssh-grant. Its +// config has: +// +// - the clients laptop and ci; +// - the keys deploy (window 15m, laptop and ci, any host), web (window +// 15m, laptop, web-1 and web-2), admin (always, laptop, any host) and +// backup (never, ci, web-1); +// - the hosts web-1 (an Ed25519 and an ECDSA host key), web-2 and db-1 +// in known_hosts. +type grantBroker struct { + addr string + key tlsKey // the broker's TLS key + clients map[string]tlsKey // the TLS keys of laptop and ci + approver *approvaltest.Approver + clock *approvaltest.Clock + audit *memAudit + // userKeys holds the public key of each [key.*] section. + userKeys map[string]ssh.PublicKey + // hostKeys holds the host keys of each host, in the order of + // known_hosts. + hostKeys map[string][]ssh.AlgorithmSigner +} + +// grantWindow is the window of the keys deploy and web. +const grantWindow = 15 * time.Minute + +// grantBrokerOptions change a grantBroker before it starts. +type grantBrokerOptions struct { + // audit, if set, wraps the audit log of the broker, such as to hold a + // write as a slow disk does. + audit func(*memAudit) broker.AuditLog + // contexts, if set, gets the context of each request that reaches the + // broker, so that a test can wait until the broker has seen a client + // hang up. It needs room for every request of the test. + contexts chan<- context.Context +} + +// newGrantBroker starts a grantBroker. +func newGrantBroker(t *testing.T) *grantBroker { + t.Helper() + return newGrantBrokerWith(t, grantBrokerOptions{}) +} + +// newGrantBrokerWith starts a grantBroker with the changes of opts. +func newGrantBrokerWith(t *testing.T, opts grantBrokerOptions) *grantBroker { + t.Helper() + gb := &grantBroker{ + key: newTLSKey(t), + clients: map[string]tlsKey{"laptop": newTLSKey(t), "ci": newTLSKey(t)}, + approver: approvaltest.NewApprover(approverID), + clock: approvaltest.NewClock(noon), + audit: newMemAudit(), + userKeys: make(map[string]ssh.PublicKey), + hostKeys: make(map[string][]ssh.AlgorithmSigner), + } + var kh strings.Builder + for _, h := range []struct { + name string + ecdsa bool + }{{"web-1", false}, {"web-1", true}, {"web-2", false}, {"db-1", false}} { + s := newGrantHostKey(t, h.ecdsa) + gb.hostKeys[h.name] = append(gb.hostKeys[h.name], s) + kh.WriteString(h.name + " " + string(ssh.MarshalAuthorizedKey(s.PublicKey()))) + } + known, err := config.ParseKnownHosts([]byte(kh.String())) + if err != nil { + t.Fatal(err) + } + window := config.Rule{Ask: config.AskWindow, Window: grantWindow} + cfg := &config.Broker{ + Telegram: config.Telegram{UserID: approverID, NoticeChatID: approverID, Timeout: secretsTimeout, + SSHTimeout: 90 * time.Second}, + Clients: map[string]config.Client{ + "laptop": {Name: "laptop", Pin: gb.clients["laptop"].pin}, + "ci": {Name: "ci", Pin: gb.clients["ci"].pin}, + }, + Keys: map[string]config.Key{ + "deploy": {Name: "deploy", Clients: []string{"laptop", "ci"}, Rule: window}, + "web": {Name: "web", Clients: []string{"laptop"}, Rule: window, Hosts: []string{"web-1", "web-2"}}, + "admin": {Name: "admin", Clients: []string{"laptop"}, Rule: config.Rule{Ask: config.AskAlways}}, + "backup": {Name: "backup", Clients: []string{"ci"}, Rule: config.Rule{Ask: config.AskNever, Notify: true}, + Hosts: []string{"web-1"}}, + }, + KnownHosts: known, + } + keys := make(map[string]sshauth.Key) + for name, k := range cfg.Keys { + _, priv, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + t.Fatal(err) + } + key, err := sshauth.NewKey(name, priv, k.Hosts) + if err != nil { + t.Fatal(err) + } + keys[name] = key + gb.userKeys[name] = key.PublicKey() + } + core, err := approval.New(cfg, gb.approver, approval.Options{Clock: gb.clock}) + if err != nil { + t.Fatal(err) + } + var log broker.AuditLog = gb.audit + if opts.audit != nil { + log = opts.audit(gb.audit) + } + svc, err := broker.New(broker.Parts{Config: cfg, Audit: log, Keys: keys}) + if err != nil { + t.Fatal(err) + } + var handler http.Handler = svc + if opts.contexts != nil { + handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + opts.contexts <- r.Context() + svc.ServeHTTP(w, r) + }) + } + gb.addr = serve(t, gb.key, cfg.Clients, handler) + 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 := gb.approver.WaitRunning(waitLimit); err != nil { + t.Fatal(err) + } + return gb +} + +// newGrantHostKey makes an SSH host key: ECDSA on P-256, or Ed25519. +func newGrantHostKey(t testing.TB, isECDSA bool) ssh.AlgorithmSigner { + t.Helper() + var priv any + var err error + if isECDSA { + priv, err = ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + } else { + _, priv, err = ed25519.GenerateKey(rand.Reader) + } + if err != nil { + t.Fatal(err) + } + s, err := ssh.NewSignerFromKey(priv) + if err != nil { + t.Fatal(err) + } + as, ok := s.(ssh.AlgorithmSigner) + if !ok { + t.Fatal("the host key cannot choose a signature algorithm") + } + return as +} + +// client returns a Broker for the client name, as Load makes it. +func (gb *grantBroker) client(t testing.TB, name string) *Broker { + t.Helper() + k, ok := gb.clients[name] + if !ok { + t.Fatalf("no client %q", name) + } + b, err := newBroker(gb.addr, k.cert, gb.key.pin) + if err != nil { + t.Fatal(err) + } + return b +} + +// waitAsked waits until the approver was asked for n messages, and returns +// them. +func (gb *grantBroker) waitAsked(t testing.TB, n int) []approvaltest.Asked { + t.Helper() + asked, err := gb.approver.WaitAsked(n, waitLimit) + if err != nil { + t.Fatal(err) + } + return asked +} + +// signRequest returns a valid request to sign a login with the key to the +// host as user, in a new session, as nodkey ssh-agent forwards it from +// OpenSSH: a session-bind with the host key's signature over the session +// identifier, and the data to sign of RFC 4252. hostKey picks one of the +// host keys of the host, in the order of known_hosts. +func (gb *grantBroker) signRequest(t testing.TB, key, host string, hostKey int, user string) api.SignRequest { + t.Helper() + h := gb.hostKeys[host][hostKey] + sid := make([]byte, 32) + if _, err := rand.Read(sid); err != nil { + t.Fatal(err) + } + sig, err := h.SignWithAlgorithm(rand.Reader, sid, "") + if err != nil { + t.Fatal(err) + } + pub := gb.userKeys[key] + data := ssh.Marshal(struct { + SessionID []byte + Msg byte + User string + Service string + Method string + HasSig bool + Algorithm string + Key []byte + }{sid, 50, user, "ssh-connection", "publickey", true, pub.Type(), pub.Marshal()}) + bind := ssh.Marshal(struct { + HostKey []byte + SessionID []byte + Signature []byte + Forwarding bool + }{h.PublicKey().Marshal(), sid, ssh.Marshal(sig), false}) + return api.SignRequest{ + PublicKey: pub.Marshal(), Data: data, Binds: [][]byte{bind}, + Commands: [][]string{{"ssh", user + "@" + host}, {"ansible-playbook", "site.yml"}}, + Reason: "apply the nginx fix", + } +} + +// sign asks the broker for the signature of req as the client b, as nodkey +// ssh-agent does, and checks the signature with the public key of the key. +func (gb *grantBroker) sign(t testing.TB, b *Broker, key string, req api.SignRequest) error { + t.Helper() + ans, err := Post[api.SignAnswer](t.Context(), b, api.PathSign, mustJSON(t, req), nil) + if err != nil { + return err + } + if err := gb.userKeys[key].Verify(req.Data, &ssh.Signature{Format: ans.Format, Blob: ans.Blob}); err != nil { + t.Errorf("the signature of the broker is not valid: %v", err) + } + return nil +} + +// mustAsk sends req as the client b, checks that the broker shows it to +// the person as message n, cancels it, and returns the prompt of the +// message. A cancelled request starts no pause and ends no grant. +func (gb *grantBroker) mustAsk(t *testing.T, b *Broker, req api.SignRequest, n int) approval.Prompt { + t.Helper() + body := mustJSON(t, req) + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + done := make(chan error, 1) + go func() { + _, err := Post[api.SignAnswer](ctx, b, api.PathSign, body, nil) + done <- err + }() + p := gb.waitAsked(t, n)[n-1].Prompt + cancel() + select { + case err := <-done: + if err == nil { + t.Error("a request that waited for the person got a signature") + } + case <-time.After(waitLimit): + t.Fatal("the cancelled request did not end") + } + gb.waitEvents(t, p.RequestID, "request denied:cancelled") + return p +} + +// waitEvents waits until the audit log has exactly the entries want for +// the request id, such as "request denied:cancelled". +func (gb *grantBroker) waitEvents(t testing.TB, id, want string) { + t.Helper() + deadline := time.NewTimer(waitLimit) + defer deadline.Stop() + for { + gb.audit.mu.Lock() + events, changed := slices.Clone(gb.audit.events), gb.audit.changed + gb.audit.mu.Unlock() + got := grantEventNames(grantEventsOf(events, id)) + if got == want { + return + } + select { + case <-changed: + case <-deadline.C: + t.Fatalf("the audit entries of request %s are %q, want %q", id, got, want) + return + } + } +} + +// lastEvents returns the names of the entries of the newest request in +// the audit log. +func (gb *grantBroker) lastEvents() string { + events := gb.audit.all() + for i := len(events) - 1; i >= 0; i-- { + if id := grantSubjectOf(events[i]).RequestID; id != "" { + return grantEventNames(grantEventsOf(events, id)) + } + } + return "" +} + +// grantSubjectOf returns the subject of an entry about a request. +func grantSubjectOf(ev audit.Event) audit.Subject { + switch e := ev.(type) { + case audit.Request: + return e.Subject + case audit.Decision: + return e.Subject + case audit.GrantUsed: + return e.Subject + case audit.SecretSent: + return e.Subject + case audit.Signed: + return e.Subject + case audit.Denied: + return e.Subject + case audit.Failure: + return e.Subject + } + return audit.Subject{} +} + +// grantEventsOf returns the entries of the request id, in order. +func grantEventsOf(events []audit.Event, id string) []audit.Event { + var out []audit.Event + for _, ev := range events { + if grantSubjectOf(ev).RequestID == id { + out = append(out, ev) + } + } + return out +} + +// grantEventNames returns the names of the entries, separated by spaces, +// such as "request grant_used:grant signed". +func grantEventNames(events []audit.Event) string { + names := make([]string, len(events)) + for i, ev := range events { + switch e := ev.(type) { + case audit.Request: + names[i] = "request" + case audit.Decision: + names[i] = "decision:" + string(e.Answer) + case audit.GrantUsed: + names[i] = "grant_used:" + string(e.Via) + case audit.Signed: + names[i] = "signed" + case audit.Denied: + names[i] = "denied:" + string(e.Cause) + case audit.Failure: + names[i] = "error:" + string(e.Code) + default: + names[i] = fmt.Sprintf("%T", ev) + } + } + return strings.Join(names, " ") +} + +// testGranter returns a granter whose broker is b, and whose environment +// is env. It reads no config file. Its clock says noon in UTC. +func testGranter(b *Broker, env []string) granter { + return granter{ + 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 }, + environ: func() []string { return slices.Clone(env) }, + now: func() time.Time { return noon }, + } +} + +// grantIn runs nodkey ssh-grant in the test, with g and ctx, and returns the +// exit code and what it wrote to stderr. +func grantIn(ctx context.Context, g granter, args ...string) (int, string) { + var stderr bytes.Buffer + code := g.run(ctx, args, &stderr) + return code, stderr.String() +} + +// grantArgs are the arguments of a request for logins as deploy with the +// key deploy to web-1 and web-2, for 15 minutes, and then extra. +func grantArgs(extra ...string) []string { + return append([]string{"--key", "deploy", "--user", "deploy", "--host", "web-1", "--host", "web-2", + "--for", "15m"}, extra...) +} diff --git a/internal/client/sshgrant_linux_test.go b/internal/client/sshgrant_linux_test.go new file mode 100644 index 0000000..0e00e29 --- /dev/null +++ b/internal/client/sshgrant_linux_test.go @@ -0,0 +1,136 @@ +package client + +import ( + "bufio" + "bytes" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "syscall" + "testing" + "time" + + "github.com/code6x/nodkey/internal/audit" + "github.com/code6x/nodkey/internal/config" + "github.com/code6x/nodkey/internal/transport" +) + +// The variables of a child that runs as nodkey ssh-grant. The child is +// the test binary again, which runs only TestSSHGrantInterrupted: with +// these variables, that test is nodkey ssh-grant with grantArgs, with the +// broker of the parent test, the real signals and the real exit. +const ( + grantChildAddr = "NODKEY_TEST_GRANT_BROKER" // the broker's address + grantChildPin = "NODKEY_TEST_GRANT_BROKER_PIN" // the pin of the broker's key + grantChildKey = "NODKEY_TEST_GRANT_KEY" // the client's key file +) + +// grantChild runs nodkey ssh-grant as SSHGrant does, but with the key file +// and the broker of the parent test: Load would refuse the system +// temporary directory, which everyone can write to. +func grantChild() int { + load := func(string) (*Broker, error) { + pin, err := config.ParsePin(os.Getenv(grantChildPin)) + if err != nil { + return nil, err + } + data, err := os.ReadFile(os.Getenv(grantChildKey)) //nolint:gosec // G703: the parent 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(grantChildAddr), key, pin) + } + g := granter{load: load, configPath: func() (string, error) { return "test", nil }, environ: os.Environ, + now: time.Now} + return g.main(grantArgs(), os.Stderr) //nolint:forbidigo // the child is nodkey ssh-grant, which writes to stderr. +} + +// TestSSHGrantInterrupted: SIGINT or SIGTERM while nodkey ssh-grant waits. +// It closes the connection, so the broker cancels the request, prints its +// last line with the status that a shell shows (130 or 143), and then +// ends with the same signal, so that a shell stops a loop or a script +// too. No grant starts. +func TestSSHGrantInterrupted(t *testing.T) { + if os.Getenv(grantChildAddr) != "" { + os.Exit(grantChild()) + } + 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) + } + gb := newGrantBroker(t) + keyFile := filepath.Join(t.TempDir(), "client.key") + if err := os.WriteFile(keyFile, gb.clients["laptop"].file, 0o600); err != nil { + t.Fatal(err) + } + cmd := exec.CommandContext(t.Context(), self(t), "-test.run=^TestSSHGrantInterrupted$") //nolint:gosec // G204: the test runs its own binary again. + cmd.Env = withEnv(os.Environ(), []string{ + grantChildAddr + "=" + gb.addr, grantChildPin + "=" + gb.key.pin.String(), grantChildKey + "=" + keyFile, + }) + var stdout bytes.Buffer + cmd.Stdout = &stdout + pipe, err := cmd.StderrPipe() + if err != nil { + t.Fatal(err) + } + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + lines := make(chan string, 16) + go func() { + defer close(lines) + r := bufio.NewReader(pipe) + for { + line, err := r.ReadString('\n') + if line != "" { + lines <- line + } + if err != nil { + return + } + } + }() + var stderr strings.Builder + select { + case line := <-lines: + stderr.WriteString(line) + if len(waitingCodes(line)) != 1 { + t.Fatalf("the first line is %q, want the waiting line", line) + } + case <-time.After(waitLimit): + t.Fatal("no waiting line") + } + p := gb.waitAsked(t, 1)[0].Prompt + if err := cmd.Process.Signal(sig); err != nil { + t.Fatal(err) + } + for line := range lines { + stderr.WriteString(line) + } + err = cmd.Wait() + name := map[syscall.Signal]string{syscall.SIGINT: "SIGINT", syscall.SIGTERM: "SIGTERM"}[sig] + want := fmt.Sprintf("nodkey: cancelled by %s (exit %d)", name, 128+int(sig)) + if !diedOf(err, sig) || lastLine(stderr.String()) != want { + t.Errorf("%v; stderr %q; want an end by %s and the last line %q", err, stderr.String(), sig, want) + } + if stdout.Len() != 0 { + t.Errorf("stdout = %q, want it empty", stdout.String()) + } + ended, err := gb.approver.WaitEnded(1, waitLimit) + if err != nil || ended[0].Ending.Cause != audit.CauseCancelled { + t.Errorf("the broker did not cancel the request: %+v, %v", ended, err) + } + gb.waitEvents(t, p.RequestID, "request denied:cancelled") + gb.mustAsk(t, gb.client(t, "laptop"), gb.signRequest(t, "deploy", "web-1", 0, "deploy"), 2) + }) + } +} diff --git a/internal/client/sshgrant_test.go b/internal/client/sshgrant_test.go new file mode 100644 index 0000000..af60d97 --- /dev/null +++ b/internal/client/sshgrant_test.go @@ -0,0 +1,822 @@ +package client + +import ( + "context" + "errors" + "io" + "net" + "net/http" + "os" + "path/filepath" + "slices" + "strings" + "syscall" + "testing" + "time" + + "golang.org/x/crypto/ssh" + + "github.com/code6x/nodkey/internal/api" + "github.com/code6x/nodkey/internal/approval" + "github.com/code6x/nodkey/internal/audit" + "github.com/code6x/nodkey/internal/config" + "github.com/code6x/nodkey/internal/sshauth" + "github.com/code6x/nodkey/internal/transport" +) + +// grantApproved is the last line of nodkey ssh-grant after the approval +// of grantArgs, with the clock of testGranter. +const grantApproved = "nodkey: SSH logins as deploy to 2 hosts allowed until 12:15 (exit 0)" + +// TestSSHGrantApproves: the person approves. nodkey ssh-grant prints the +// short code of the message once, then its last line, and exits with 0. +// The message lists the key, the remote user, every host and the end of +// the grants, and only the reason as claimed text. Then the broker signs +// logins with that key as that user to each host key of each listed host +// without a tap, until the end shown in the message. A host that is not in +// the list, another user, another key and another client still ask. +func TestSSHGrantApproves(t *testing.T) { + t.Parallel() + gb := newGrantBroker(t) + gb.approver.Auto(audit.AnswerApproveWindow) + laptop := gb.client(t, "laptop") + code, stderr := grantIn(t.Context(), testGranter(laptop, []string{"NODKEY_REASON=from the environment"}), + grantArgs("--reason", "apply the nginx fix")...) + p := gb.waitAsked(t, 1)[0].Prompt + if want := "nodkey: waiting for approval (request " + p.ShortCode + ")\n" + grantApproved + "\n"; code != exitOK || + stderr != want { + t.Fatalf("exit code %d, stderr %q; want 0 and %q", code, stderr, want) + } + ends := noon.Add(grantWindow) + if p.Kind != audit.KindSSHGrant || p.Client != "laptop" || p.Key != "deploy" || + !slices.Equal(p.Hosts, []string{"web-1", "web-2"}) || p.RemoteUser != "deploy" || p.HostKey != "" || + len(p.Secrets) != 0 || !p.GrantEnds.Equal(ends) { + t.Errorf("the message shows %+v; want the key deploy, the user deploy, web-1 and web-2, until %v", p, ends) + } + if p.Once || p.WindowMinutes != 15 || p.TimeLeft != secretsTimeout { + t.Errorf("approve once %v, window %d, time left %v; want only Approve and Deny, and %v to answer", + p.Once, p.WindowMinutes, p.TimeLeft, secretsTimeout) + } + if p.Claim != (approval.Claim{Reason: "apply the nginx fix"}) { + t.Errorf("the claimed part is %+v; want only the reason of --reason", p.Claim) + } + if got := gb.lastEvents(); got != "request decision:approve_window" { + t.Errorf("audit entries: %s", got) + } + + gb.approver.Auto("") + // Each host key of each listed host, as deploy, with the key deploy. + for _, h := range []struct { + host string + key int + }{{"web-1", 0}, {"web-1", 1}, {"web-2", 0}} { + if err := gb.sign(t, laptop, "deploy", gb.signRequest(t, "deploy", h.host, h.key, "deploy")); err != nil { + t.Fatalf("a login to %s with its host key %d: %v", h.host, h.key, err) + } + if got := gb.lastEvents(); got != "request grant_used:grant signed" { + t.Errorf("a login to %s with its host key %d: audit entries %s", h.host, h.key, got) + } + } + if n := len(gb.approver.Asked()); n != 1 { + t.Errorf("%d messages, want 1: the grants cover these logins", n) + } + n := 1 + for _, tt := range []struct { + name string + client, key, host, user string + }{ + {"a host that is not in the list", "laptop", "deploy", "db-1", "deploy"}, + {"another remote user", "laptop", "deploy", "web-1", "root"}, + {"another key", "laptop", "web", "web-1", "deploy"}, + {"another client", "ci", "deploy", "web-1", "deploy"}, + } { + n++ + p := gb.mustAsk(t, gb.client(t, tt.client), gb.signRequest(t, tt.key, tt.host, 0, tt.user), n) + if p.Kind != audit.KindSSHLogin || p.Client != tt.client || p.Key != tt.key || + !slices.Equal(p.Hosts, []string{tt.host}) || p.RemoteUser != tt.user { + t.Errorf("%s: the message shows %+v", tt.name, p) + } + } + // The grants end at the time in the message. + gb.clock.Advance(grantWindow - time.Second) + if err := gb.sign(t, laptop, "deploy", gb.signRequest(t, "deploy", "web-2", 0, "deploy")); err != nil { + t.Fatalf("a login one second before the grants end: %v", err) + } + gb.clock.Advance(time.Second) + gb.mustAsk(t, laptop, gb.signRequest(t, "deploy", "web-2", 0, "deploy"), n+1) +} + +// TestSSHGrantDenied: the person taps Deny. nodkey ssh-grant exits with 77 +// after its waiting line, and no grant starts. In the pause after the +// Deny, a second ssh-grant gets 429 from the broker (77 too, without a +// waiting line), and a login with the key is denied without a message. +// After the pause, a login asks. +func TestSSHGrantDenied(t *testing.T) { + t.Parallel() + gb := newGrantBroker(t) + gb.approver.Auto(audit.AnswerDeny) + laptop := gb.client(t, "laptop") + g := testGranter(laptop, nil) + code, stderr := grantIn(t.Context(), g, grantArgs()...) + if code != exitDenied || lastLine(stderr) != "nodkey: denied (exit 77)" || len(waitingCodes(stderr)) != 1 { + t.Errorf("exit code %d, stderr %q; want 77, one waiting line, and the last line %q", code, stderr, + "nodkey: denied (exit 77)") + } + if got := gb.lastEvents(); got != "request decision:deny denied:approver" { + t.Errorf("audit entries: %s", got) + } + code, stderr = grantIn(t.Context(), g, grantArgs()...) + if want := "nodkey: denied by a limit of the broker (exit 77)\n"; code != exitDenied || stderr != want { + t.Errorf("in the pause after a Deny: exit code %d, stderr %q; want 77 and only %q", code, stderr, want) + } + gb.approver.Auto("") + var e *Error + if err := gb.sign(t, laptop, "deploy", gb.signRequest(t, "deploy", "web-1", 0, "deploy")); !errors.As(err, &e) || + e.Status != http.StatusTooManyRequests { + t.Errorf("a login in the pause after the Deny: %v; want 429", err) + } + gb.clock.Advance(approval.DenyPause) + gb.mustAsk(t, laptop, gb.signRequest(t, "deploy", "web-1", 0, "deploy"), 2) +} + +// TestSSHGrantExpired: nobody answers in time. nodkey ssh-grant exits with +// 77, and no grant starts. A request for grants waits as long as a request +// for secrets, but never past the end of its grants: with --for 1m, one +// minute. +func TestSSHGrantExpired(t *testing.T) { + t.Parallel() + for _, tt := range []struct { + forText string + wait time.Duration + }{ + {"15m", secretsTimeout}, + {"1m", time.Minute}, + } { + t.Run(tt.forText, func(t *testing.T) { + t.Parallel() + gb := newGrantBroker(t) + laptop := gb.client(t, "laptop") + type result struct { + code int + stderr string + } + done := make(chan result, 1) + go func() { + code, stderr := grantIn(t.Context(), testGranter(laptop, nil), + "--key", "deploy", "--user", "deploy", "--host", "web-1", "--for", tt.forText) + done <- result{code, stderr} + }() + p := gb.waitAsked(t, 1)[0].Prompt + if p.TimeLeft != tt.wait || !p.GrantEnds.Equal(noon.Add(mustDuration(t, tt.forText))) { + t.Errorf("time left %v, grants end %v; want %v and %s after noon", p.TimeLeft, p.GrantEnds, tt.wait, + tt.forText) + } + gb.clock.Advance(tt.wait) + var res result + select { + case res = <-done: + case <-time.After(waitLimit): + t.Fatal("nodkey ssh-grant did not end") + } + if res.code != exitDenied || lastLine(res.stderr) != "nodkey: denied (exit 77)" { + t.Errorf("exit code %d, stderr %q; want 77", res.code, res.stderr) + } + if got := gb.lastEvents(); got != "request denied:expired" { + t.Errorf("audit entries: %s", got) + } + gb.mustAsk(t, laptop, gb.signRequest(t, "deploy", "web-1", 0, "deploy"), 2) + }) + } +} + +// mustDuration parses a duration of a test. +func mustDuration(t testing.TB, s string) time.Duration { + t.Helper() + d, err := time.ParseDuration(s) + if err != nil { + t.Fatal(err) + } + return d +} + +// TestSSHGrantRefusedByTheBroker: the broker refuses, before any message, +// the SSH grants that the client cannot check itself (DESIGN.md section +// 13): a key whose rule is not "window", a time longer than the key's +// window, a host that is not in the broker's known_hosts or not in the +// key's hosts, an unknown key, and a key of another client. nodkey +// ssh-grant exits with 77, without a waiting line, and no grant starts. +func TestSSHGrantRefusedByTheBroker(t *testing.T) { + t.Parallel() + gb := newGrantBroker(t) + gb.approver.Auto(audit.AnswerApproveWindow) // a message would be approved at once + grant := func(key, forText string, hosts ...string) []string { + args := []string{"--key", key, "--user", "deploy", "--for", forText} + for _, h := range hosts { + args = append(args, "--host", h) + } + return args + } + for _, tt := range []struct { + name string + client string + args []string + }{ + {"a key whose rule is always", "laptop", grant("admin", "15m", "web-1")}, + {"a key whose rule is never", "ci", grant("backup", "15m", "web-1")}, + {"longer than the key's window", "laptop", grant("deploy", "16m", "web-1")}, + {"a host that is not in known_hosts", "laptop", grant("deploy", "15m", "web-1", "web-9")}, + {"a host that is not in the key's hosts", "laptop", grant("web", "15m", "web-1", "db-1")}, + {"an unknown key", "laptop", grant("nope", "15m", "web-1")}, + {"a key of another client", "ci", grant("web", "15m", "web-1")}, + } { + code, stderr := grantIn(t.Context(), testGranter(gb.client(t, tt.client), nil), tt.args...) + if code != exitDenied || stderr != "nodkey: denied (exit 77)\n" { + t.Errorf("%s: exit code %d, stderr %q; want 77 without a waiting line", tt.name, code, stderr) + } + if got := gb.lastEvents(); got != "request denied:not_allowed" { + t.Errorf("%s: audit entries %s", tt.name, got) + } + } + if n := len(gb.approver.Asked()); n != 0 { + t.Fatalf("%d messages; the broker must refuse these grants before any message", n) + } + gb.approver.Auto("") + gb.mustAsk(t, gb.client(t, "laptop"), gb.signRequest(t, "deploy", "web-1", 0, "deploy"), 1) +} + +// TestSSHGrantBrokerChecksItself: the broker does not rely on the checks of +// nodkey ssh-grant. A hostile client can send any request; these are ones +// that nodkey ssh-grant never sends. The broker refuses each before any +// message: with 403, or with 400 for more than 20 hosts, which is over a +// cap of the API. +func TestSSHGrantBrokerChecksItself(t *testing.T) { + t.Parallel() + gb := newGrantBroker(t) + gb.approver.Auto(audit.AnswerApproveWindow) // a message would be approved at once + laptop := gb.client(t, "laptop") + many := make([]string, api.MaxGrantHosts+1) + for i := range many { + many[i] = "web-1" + } + for _, tt := range []struct { + name string + edit func(*api.GrantRequest) + status int + }{ + {"a remote user with @", func(r *api.GrantRequest) { r.User = "deploy@web-1" }, http.StatusForbidden}, + {"a remote user that starts with -", func(r *api.GrantRequest) { r.User = "-oProxyCommand=x" }, + http.StatusForbidden}, + {"a remote user with 33 characters", func(r *api.GrantRequest) { r.User = "u" + strings.Repeat("x", 32) }, + http.StatusForbidden}, + {"a remote user with a line break", func(r *api.GrantRequest) { r.User = "deploy\nroot" }, + http.StatusForbidden}, + {"no remote user", func(r *api.GrantRequest) { r.User = "" }, http.StatusForbidden}, + {"no host", func(r *api.GrantRequest) { r.Hosts = nil }, http.StatusForbidden}, + {"more than 20 hosts", func(r *api.GrantRequest) { r.Hosts = many }, http.StatusBadRequest}, + {"a host with a port", func(r *api.GrantRequest) { r.Hosts = []string{"web-1:22"} }, http.StatusForbidden}, + {"a host with a wildcard", func(r *api.GrantRequest) { r.Hosts = []string{"web-*"} }, http.StatusForbidden}, + {"a host in another case", func(r *api.GrantRequest) { r.Hosts = []string{"WEB-1"} }, http.StatusForbidden}, + {"under one minute", func(r *api.GrantRequest) { r.Seconds = 59 }, http.StatusForbidden}, + {"no time", func(r *api.GrantRequest) { r.Seconds = 0 }, http.StatusForbidden}, + {"a time below 0", func(r *api.GrantRequest) { r.Seconds = -900 }, http.StatusForbidden}, + {"one second over the window", func(r *api.GrantRequest) { r.Seconds = 15*60 + 1 }, http.StatusForbidden}, + {"a huge time", func(r *api.GrantRequest) { r.Seconds = 1 << 62 }, http.StatusForbidden}, + } { + req := api.GrantRequest{Key: "deploy", User: "deploy", Hosts: []string{"web-1"}, Seconds: 15 * 60, Reason: "r"} + tt.edit(&req) + _, err := Post[api.GrantAnswer](t.Context(), laptop, api.PathSSHGrant, mustJSON(t, req), nil) + var e *Error + if !errors.As(err, &e) || e.Status != tt.status { + t.Errorf("%s: %v; want status %d", tt.name, err, tt.status) + } + if got := gb.lastEvents(); got != "request denied:not_allowed" { + t.Errorf("%s: audit entries %s", tt.name, got) + } + } + if n := len(gb.approver.Asked()); n != 0 { + t.Errorf("%d messages; the broker must refuse these grants before any message", n) + } +} + +// TestSSHGrantUsage: wrong usage exits with 64, prints the usage, and asks +// no one. The client checks what it can, to give a clear error: the key +// name, the remote user, 1 to 20 hosts that are plain names and each there +// once, and a time in whole minutes of at most 8 hours. +func TestSSHGrantUsage(t *testing.T) { + t.Parallel() + base := func(drop string, extra ...string) []string { + var args []string + for _, kv := range [][2]string{{"--key", "deploy"}, {"--user", "deploy"}, {"--host", "web-1"}, {"--for", "15m"}} { + if kv[0] != drop { + args = append(args, kv[0], kv[1]) + } + } + return append(args, extra...) + } + var many []string + for i := range api.MaxGrantHosts + 1 { + many = append(many, "--host", "web-"+itoa(i)) + } + tests := []struct { + name string + args []string + want string // a part of the last line + }{ + {"nothing", nil, "give the key with --key NAME"}, + {"no key", base("--key"), "give the key with --key NAME"}, + {"no user", base("--user"), "give the remote user with --user USER"}, + {"no host", base("--host"), "give at least one host with --host NAME"}, + {"no time", base("--for"), "give the time of the grants with --for DURATION"}, + {"no value", []string{"--key"}, "--key needs a value"}, + {"no value at the end", base("", "--host"), "--host needs a value"}, + {"an empty key", base("--key", "--key="), `--key: the key name "" must be 1 to 64 letters`}, + {"a key with a space", base("--key", "--key", "de ploy"), `the key name "de ploy" must be`}, + {"a key that starts with .", base("--key", "--key", ".deploy"), `the key name ".deploy" must be`}, + {"a key with 65 characters", base("--key", "--key", strings.Repeat("k", 65)), "must be 1 to 64 letters"}, + {"a user with @", base("--user", "--user", "deploy@web-1"), + `--user: the remote user name "deploy@web-1" must match [A-Za-z0-9._][A-Za-z0-9._-]{0,31}`}, + {"a user that starts with -", base("--user", "--user", "-oProxyCommand=x"), "must match"}, + {"an empty user", base("--user", "--user="), `the remote user name "" must match`}, + {"a user with 33 characters", base("--user", "--user", "u"+strings.Repeat("x", 32)), "must match"}, + {"a user with a line break", base("--user", "--user", "deploy\nroot"), `"deploy\nroot" must match`}, + {"a host with a user", base("", "--host", "deploy@web-2"), `--host: "deploy@web-2" is not a host name`}, + {"a host with a port", base("", "--host", "web-2:22"), `"web-2:22" is not a host name`}, + {"a host in brackets", base("", "--host", "[web-2]:22"), "is not a host name"}, + {"two hosts in one", base("", "--host", "web-2,web-3"), "give each host with its own --host"}, + {"a wildcard", base("", "--host", "*.example.com"), "is not a host name"}, + {"an empty host", base("", "--host="), `"" is not a host name`}, + {"a host that starts with -", base("", "--host", "-web-2"), `"-web-2" is not a host name`}, + {"a host with 65 characters", base("", "--host", strings.Repeat("h", 65)), "is not a host name"}, + {"a host twice", base("", "--host", "web-1"), "--host: the host web-1 is given twice"}, + {"21 hosts", append(base("--host"), many...), "at most 20 --host options"}, + {"0 minutes", base("--for", "--for", "0m"), `--for "0m": give a time in whole minutes, such as 15m or 1h`}, + {"a time below 0", base("--for", "--for", "-15m"), "give a time in whole minutes"}, + {"seconds", base("--for", "--for", "90s"), "give a time in whole minutes"}, + {"part of a minute", base("--for", "--for", "1m30s"), "give a time in whole minutes"}, + {"no unit", base("--for", "--for", "15"), "give a time in whole minutes"}, + {"not a time", base("--for", "--for", "soon"), "give a time in whole minutes"}, + {"too large to read", base("--for", "--for", "9999999999h"), "give a time in whole minutes"}, + {"over 8 hours", base("--for", "--for", "8h1m"), + `--for "8h1m": the grants can last at most 8 hours, the longest window of a key`}, + {"a key twice", base("", "--key", "web"), "--key is given twice"}, + {"a user twice", base("", "--user", "root"), "--user is given twice"}, + {"a time twice", base("", "--for=15m"), "--for is given twice"}, + {"a reason twice", base("", "--reason", "a", "-reason=b"), "--reason is given twice"}, + {"an unknown option", base("", "--bogus"), `unknown option "--bogus"`}, + {"one dash", base("", "-"), `unknown option "-"`}, + {"two dashes", base("", "--"), `unknown option "--"`}, + {"a host without --host", base("", "web-2"), `unexpected argument "web-2"; put each host after its own --host`}, + {"help with other arguments", base("", "--help"), "--help must be the only argument"}, + {"help with a value", []string{"-h=1"}, "--help must be the only argument"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + code, stderr := grantIn(t.Context(), testGranter(nil, nil), tt.args...) + if !strings.HasPrefix(stderr, grantUsage) { + t.Errorf("stderr = %q; want the usage first", stderr) + } + if last := lastLine(stderr); code != exitUsage || !strings.HasPrefix(last, "nodkey: ") || + !strings.Contains(last, tt.want) || !strings.HasSuffix(last, " (exit 64)") { + t.Errorf("exit code %d, last line %q; want 64 and %q", code, last, tt.want) + } + }) + } +} + +// TestSSHGrantHelp: -h and --help print the usage and exit with 0, but only +// as the only argument. +func TestSSHGrantHelp(t *testing.T) { + t.Parallel() + for _, arg := range []string{"-h", "--help", "-help", "--h"} { + code, stderr := grantIn(t.Context(), testGranter(nil, nil), arg) + if code != exitOK || stderr != grantUsage { + t.Errorf("nodkey ssh-grant %s: exit code %d, stderr %q; want 0 and only the usage", arg, code, stderr) + } + } +} + +// TestParseGrantArgs: the forms of the options, as the flag package reads +// them, and the good edges: 20 hosts, the longest names, and the times of +// 1 minute and 8 hours. +func TestParseGrantArgs(t *testing.T) { + t.Parallel() + o, err := parseGrantArgs([]string{"-key", "deploy", "--user=deploy", "-host=web-1", "--host", "web-2", + "-for=1h30m", "-reason=why"}) + if err != nil { + t.Fatal(err) + } + if o.key != "deploy" || o.user != "deploy" || !slices.Equal(o.hosts, []string{"web-1", "web-2"}) || + o.duration != 90*time.Minute || o.reason != "why" || !o.reasonSet { + t.Errorf("parseGrantArgs = %+v", o) + } + o, err = parseGrantArgs([]string{"--reason", "--", "--key", "a", "--user", "a", "--host", "a", "--for", "1m"}) + if err != nil || o.reason != "--" || !o.reasonSet || o.duration != time.Minute { + t.Errorf("a reason that is \"--\" and a time of 1 minute: %+v, %v", o, err) + } + if o, err := parseGrantArgs(grantArgs()); err != nil || o.reasonSet { + t.Errorf("without --reason: %+v, %v", o, err) + } + long := func(c string, n int) string { return strings.Repeat(c, n) } + args := []string{"--key", long("k", 64), "--user", "." + long("u", 31), "--for", "8h"} + var hosts []string + for i := range api.MaxGrantHosts { + h := "h" + itoa(i) + "-" + long("x", 64) + hosts = append(hosts, h[:64]) + args = append(args, "--host", h[:64]) + } + o, err = parseGrantArgs(args) + if err != nil { + t.Fatalf("20 hosts with the longest names: %v", err) + } + if !slices.Equal(o.hosts, hosts) || o.duration != config.MaxWindow { + t.Errorf("parseGrantArgs = %+v", o) + } + req := newGrantRequest(o, long("\x01", api.MaxText)) + if req.Seconds != 8*60*60 { + t.Errorf("--for 8h is %d seconds", req.Seconds) + } + body, err := encodeGrantRequest(req) + if err != nil || len(body) > transport.MaxRequestBody { + t.Errorf("the largest request: %d bytes, %v", len(body), err) + } + if _, err := encodeGrantRequest(api.GrantRequest{Hosts: append(hosts, "one-more")}); err == nil { + t.Errorf("encodeGrantRequest took %d hosts", api.MaxGrantHosts+1) + } + if one, twenty := grantHostCount(1), grantHostCount(20); one != "1 host" || twenty != "20 hosts" { + t.Errorf("grantHostCount: %q and %q", one, twenty) + } +} + +// TestGrantNameRule: a key name or a host name passes the client's check +// exactly when it follows the name rule of the broker config, as the +// broker's known_hosts reads it. +func TestGrantNameRule(t *testing.T) { + t.Parallel() + key := strings.TrimSpace(string(ssh.MarshalAuthorizedKey(newGrantHostKey(t, false).PublicKey()))) + for _, name := range []string{ + "web-1", "a", "Z", "0", "x.y_z-1", strings.Repeat("a", 64), strings.Repeat("a", 65), ".web", "_web", + "-web", "web 1", "web-1:22", "web-1,web-2", "*", "web?", "é", "web/1", "deploy@web-1", + } { + _, err := config.ParseKnownHosts([]byte(name + " " + key + "\n")) + if got, want := grantNamePattern.MatchString(name), err == nil; got != want { + t.Errorf("%q: the client says %v, known_hosts says %v (%v)", name, got, want, err) + } + } + if !sshauth.ValidUser("deploy") || sshauth.ValidUser("deploy@web-1") { + t.Error("sshauth.ValidUser does not check the remote user") + } +} + +// TestSSHGrantAnswers: only a final 200 whose body passes api.ReadAnswer, +// with 1 to 15 minutes for --for 15m, counts. 403 and 429 exit with 77; +// every other answer, also a redirect, a body that api.ReadAnswer refuses, +// and minutes out of range, with 69. nodkey ssh-grant sends the request +// once, and never prints an answer body, a header, or the number of an +// answer that it refused. +func TestSSHGrantAnswers(t *testing.T) { + t.Parallel() + value := madeUp("value") + const notValid = "the answer of the broker is not valid" + const outOfRange = "the answer of the broker is not valid: its time is not 1 to 15 minutes" + tests := []struct { + name string + handler http.HandlerFunc + code int + want string // the last line, without "nodkey: " and " (exit N)" + }{ + {"403 with text in the body and a header", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("X-Note", value) + _ = transport.WriteJSONBody(w, http.StatusForbidden, []byte(`{"error":"`+value+`"}`)) + }, exitDenied, "denied"}, + {"429", answer(http.StatusTooManyRequests, `{"error":"Too Many Requests"}`), exitDenied, + "denied by a limit of the broker"}, + {"400", answer(http.StatusBadRequest, `{"error":"Bad Request"}`), exitUnavailable, + "the broker answered 400 Bad Request"}, + {"404", answer(http.StatusNotFound, `{"error":"Not Found"}`), exitUnavailable, + "the broker answered 404 Not Found"}, + {"500 with minutes in the body", answer(http.StatusInternalServerError, `{"minutes":15}`), exitUnavailable, + "the broker answered 500 Internal Server Error"}, + {"503", answer(http.StatusServiceUnavailable, `{"error":"Service Unavailable"}`), exitUnavailable, + "the broker answered 503 Service Unavailable"}, + {"201 with a valid answer", answer(http.StatusCreated, `{"minutes":15}`), exitUnavailable, + "the broker answered 201 Created"}, + {"a redirect", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Location", "https://192.0.2.10:7743/v1/ssh-grant") + w.WriteHeader(http.StatusTemporaryRedirect) + }, exitUnavailable, "the broker answered 307 Temporary Redirect"}, + {"not JSON", answer(http.StatusOK, "not json "+value), exitUnavailable, notValid}, + {"null", answer(http.StatusOK, "null"), exitUnavailable, notValid}, + {"no minutes", answer(http.StatusOK, `{}`), exitUnavailable, notValid}, + {"minutes null", answer(http.StatusOK, `{"minutes":null}`), exitUnavailable, notValid}, + {"minutes as text", answer(http.StatusOK, `{"minutes":"15"}`), exitUnavailable, notValid}, + {"minutes with a fraction", answer(http.StatusOK, `{"minutes":14.5}`), exitUnavailable, notValid}, + {"minutes with an exponent", answer(http.StatusOK, `{"minutes":1e1}`), exitUnavailable, notValid}, + {"minutes twice", answer(http.StatusOK, `{"minutes":15,"minutes":15}`), exitUnavailable, notValid}, + {"an unknown field", answer(http.StatusOK, `{"minutes":15,"note":"`+value+`"}`), exitUnavailable, notValid}, + {"a field in another case", answer(http.StatusOK, `{"Minutes":15}`), exitUnavailable, notValid}, + {"data after the answer", answer(http.StatusOK, `{"minutes":15}{}`), exitUnavailable, notValid}, + {"larger than api.MaxAnswer", answer(http.StatusOK, `{"minutes":15}`+strings.Repeat(" ", api.MaxAnswer)), + exitUnavailable, notValid}, + {"0 minutes", answer(http.StatusOK, `{"minutes":0}`), exitUnavailable, outOfRange}, + {"minutes below 0", answer(http.StatusOK, `{"minutes":-15}`), exitUnavailable, outOfRange}, + {"more minutes than --for", answer(http.StatusOK, `{"minutes":16}`), exitUnavailable, outOfRange}, + {"a huge number of minutes", answer(http.StatusOK, `{"minutes":1099511627776}`), exitUnavailable, notValid}, + {"a body shorter than its length", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Length", "100") + w.WriteHeader(http.StatusOK) + _, _ = io.WriteString(w, `{"minutes":`) + _ = http.NewResponseController(w).Flush() + panic(http.ErrAbortHandler) // drop the connection + }, exitUnavailable, notValid}, + {"a dropped connection", func(http.ResponseWriter, *http.Request) { panic(http.ErrAbortHandler) }, + exitUnavailable, "broke before the answer"}, + {"too many informational answers", func(w http.ResponseWriter, _ *http.Request) { + for range maxInformational + 1 { + if err := transport.WriteProcessing(w, http.Header{api.ShortCodeHeader: {"7qd2"}}); err != nil { + return + } + } + _ = transport.WriteJSONBody(w, http.StatusOK, []byte(`{"minutes":15}`)) + }, exitUnavailable, "the broker sent more than 4 informational answers"}, + {"1 minute", answer(http.StatusOK, `{"minutes":1}`), exitOK, + "SSH logins as deploy to 2 hosts allowed until 12:01"}, + {"15 minutes", answer(http.StatusOK, `{"minutes":15}`), exitOK, + "SSH logins as deploy to 2 hosts allowed until 12:15"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + fb := newFakeBroker(t, tt.handler) + code, stderr := grantIn(t.Context(), testGranter(fb.b, nil), grantArgs()...) + last := lastLine(stderr) + if code != tt.code || !strings.HasPrefix(last, "nodkey: ") || !strings.Contains(last, tt.want) || + !strings.HasSuffix(last, " (exit "+itoa(tt.code)+")") { + t.Errorf("exit code %d, stderr %q; want %d and %q in the last line", code, safe(stderr), tt.code, tt.want) + } + if fb.requests.Load() != 1 { + t.Errorf("the broker got %d requests, want 1", fb.requests.Load()) + } + mustNotLeak(t, value, stderr) + if strings.Contains(stderr, "1099511627776") { + t.Errorf("stderr shows the number of the answer: %q", stderr) + } + }) + } +} + +// TestSSHGrantShortCode: nodkey ssh-grant prints the short code of the +// first 102 Processing once, and only a code that has the form of a short +// code. It never prints another header, or a code from another status. +func TestSSHGrantShortCode(t *testing.T) { + t.Parallel() + value := madeUp("value") + informational := func(status int, code string) func(http.ResponseWriter) { + return func(w http.ResponseWriter) { + w.Header().Set(api.ShortCodeHeader, code) + w.Header().Set("X-Note", value) + w.WriteHeader(status) + w.Header().Del(api.ShortCodeHeader) + w.Header().Del("X-Note") + } + } + tests := []struct { + name string + first []func(http.ResponseWriter) + final string // the short code header of the final answer + want string + }{ + {"one", []func(http.ResponseWriter){informational(http.StatusProcessing, "m4k2")}, "", "m4k2"}, + {"two", []func(http.ResponseWriter){ + informational(http.StatusProcessing, "m4k2"), informational(http.StatusProcessing, "k3f9"), + }, "", "m4k2"}, + {"not a short code first", []func(http.ResponseWriter){ + informational(http.StatusProcessing, "M4K2"), informational(http.StatusProcessing, "k3f9"), + }, "", "k3f9"}, + {"not a short code", []func(http.ResponseWriter){informational(http.StatusProcessing, value)}, "", ""}, + {"not 102", []func(http.ResponseWriter){informational(http.StatusEarlyHints, "m4k2")}, "", ""}, + {"in the final answer", nil, "m4k2", ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + fb := newFakeBroker(t, func(w http.ResponseWriter, _ *http.Request) { + for _, f := range tt.first { + f(w) + } + if tt.final != "" { + w.Header().Set(api.ShortCodeHeader, tt.final) + } + _ = transport.WriteJSONBody(w, http.StatusOK, []byte(`{"minutes":15}`)) + }) + code, stderr := grantIn(t.Context(), testGranter(fb.b, nil), grantArgs()...) + want := grantApproved + "\n" + if tt.want != "" { + want = "nodkey: waiting for approval (request " + tt.want + ")\n" + want + } + if code != exitOK || stderr != want { + t.Errorf("exit code %d, stderr %q; want 0 and %q", code, safe(stderr), want) + } + mustNotLeak(t, value, stderr) + }) + } +} + +// TestSSHGrantOutputAsInDesign: the output of nodkey ssh-grant after an +// approval is exactly the example of DESIGN.md section 13: the waiting +// line, and the last line with the end of the grants, the time when the +// request went out plus the minutes of the answer, without the seconds. +func TestSSHGrantOutputAsInDesign(t *testing.T) { + t.Parallel() + doc, err := os.ReadFile(filepath.Join("..", "..", "DESIGN.md")) + if err != nil { + t.Fatal(err) + } + fb := newFakeBroker(t, func(w http.ResponseWriter, _ *http.Request) { + if err := transport.WriteProcessing(w, http.Header{api.ShortCodeHeader: {"m4k2"}}); err != nil { + return + } + _ = transport.WriteJSONBody(w, http.StatusOK, []byte(`{"minutes":15}`)) + }) + g := testGranter(fb.b, nil) + g.now = func() time.Time { return time.Date(2026, 10, 14, 14, 5, 12, 0, time.UTC) } + code, stderr := grantIn(t.Context(), g, grantArgs("--reason", "apply the nginx fix")...) + if code != exitOK || !strings.Contains(string(doc), "```\n"+stderr+"```") { + t.Errorf("exit code %d; DESIGN.md section 13 does not show this output:\n%s", code, stderr) + } +} + +// TestSSHGrantRequest: the request is one POST of JSON to /v1/ssh-grant, +// which the broker can read (transport.ReadJSON), with the header +// "Connection: close", so no other request goes on its connection. It has +// the key, the remote user, the hosts in the order of the arguments, --for +// in seconds, and the reason: from --reason, else from NODKEY_REASON. A +// reason over 64 KiB becomes "[hidden: too long]". +func TestSSHGrantRequest(t *testing.T) { + t.Parallel() + full := strings.Repeat("r", api.MaxText) + tests := []struct { + name string + args []string + env []string + reason string + }{ + {"option", []string{"--reason", "from the option"}, []string{"NODKEY_REASON=from the environment"}, + "from the option"}, + {"environment", nil, []string{"A=1", "NODKEY_REASON=from the environment", "NODKEY_REASON=second"}, + "from the environment"}, + {"empty option", []string{"--reason="}, []string{"NODKEY_REASON=from the environment"}, ""}, + {"none", nil, nil, ""}, + {"at the limit", []string{"--reason", full}, nil, full}, + {"too long", []string{"--reason", full + "r"}, nil, tooLong}, + {"too long in the environment", nil, []string{"NODKEY_REASON=" + full + "r"}, tooLong}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + var got *api.GrantRequest + var header http.Header + var method string + fb := newFakeBroker(t, func(w http.ResponseWriter, r *http.Request) { + method, header = r.Method+" "+r.RequestURI, r.Header.Clone() + req, err := transport.ReadJSON[api.GrantRequest](w, r) + if err != nil { + return + } + got = req + transport.WriteError(w, http.StatusForbidden) + }) + args := append([]string{"--key", "deploy", "--user", "deploy", "--host", "web-2", "--host", "web-1", + "--for", "1h"}, tt.args...) + code, _ := grantIn(t.Context(), testGranter(fb.b, tt.env), args...) + if code != exitDenied || got == nil { + t.Fatalf("exit code %d; the broker could not read the request", code) + } + want := api.GrantRequest{Key: "deploy", User: "deploy", Hosts: []string{"web-2", "web-1"}, Seconds: 3600, + Reason: tt.reason} + if a, b := mustJSON(t, *got), mustJSON(t, want); string(a) != string(b) { + t.Errorf("the request is %.300s\nwant %.300s", a, b) + } + if method != "POST "+api.PathSSHGrant || header.Get("Content-Type") != api.ContentType || + header.Get("Connection") != "close" { + t.Errorf("the request is %s with the headers %v", method, header) + } + }) + } +} + +// TestSSHGrantCancel: the person stops nodkey ssh-grant while it waits +// (SIGINT or SIGTERM end ctx). The connection closes, the broker cancels +// the request, and run returns 130, the status of a command that SIGINT +// ended; main then ends nodkey with SIGINT (TestSSHGrantInterrupted). No +// grant starts. +func TestSSHGrantCancel(t *testing.T) { + t.Parallel() + gb := newGrantBroker(t) + laptop := gb.client(t, "laptop") + ctx, cancel := context.WithCancelCause(t.Context()) + defer cancel(nil) + done := make(chan int, 1) + var stderr string + go func() { + var code int + code, stderr = grantIn(ctx, testGranter(laptop, nil), grantArgs()...) + done <- code + }() + p := gb.waitAsked(t, 1)[0].Prompt + cancel(interrupted{sig: syscall.SIGINT}) + if code := <-done; code != 130 || lastLine(stderr) != "nodkey: cancelled by SIGINT (exit 130)" { + t.Errorf("exit code %d, stderr %q; want 130", code, stderr) + } + ended, err := gb.approver.WaitEnded(1, waitLimit) + if err != nil || ended[0].Ending.Cause != audit.CauseCancelled { + t.Errorf("the broker did not cancel the request: %+v, %v", ended, err) + } + gb.waitEvents(t, p.RequestID, "request denied:cancelled") + gb.mustAsk(t, laptop, gb.signRequest(t, "deploy", "web-1", 0, "deploy"), 2) +} + +// TestSSHGrantUnreachable: nodkey ssh-grant exits with 69 when it cannot +// read its config, cannot reach the broker, or when the other side is not +// the broker of broker_pin. It never asks again. +func TestSSHGrantUnreachable(t *testing.T) { + t.Parallel() + gb := newGrantBroker(t) + closed := func(t *testing.T) string { + ln, err := (&net.ListenConfig{}).Listen(t.Context(), "tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + addr := ln.Addr().String() + if err := ln.Close(); err != nil { + t.Fatal(err) + } + return addr + } + granterFor := func(t *testing.T, addr string, key tlsKey, pin config.Pin) granter { + b, err := newBroker(addr, key.cert, pin) + if err != nil { + t.Fatal(err) + } + return testGranter(b, nil) + } + tests := []struct { + name string + g func(t *testing.T) granter + want string // a part of the last line + }{ + {"no config", func(*testing.T) granter { + g := testGranter(nil, nil) + g.load = func(path string) (*Broker, error) { + return nil, errors.New("config " + path + ": made-up error") + } + return g + }, "config /nonexistent/client.toml: made-up error"}, + {"no home", func(*testing.T) granter { + g := testGranter(nil, nil) + g.configPath = func() (string, error) { return "", errors.New("the home directory is not known") } + return g + }, "the home directory is not known"}, + {"broker down", func(t *testing.T) granter { + return granterFor(t, closed(t), gb.clients["laptop"], gb.key.pin) + }, "cannot connect to the broker at 127.0.0.1:"}, + {"another broker key", func(t *testing.T) granter { + return granterFor(t, gb.addr, gb.clients["laptop"], newTLSKey(t).pin) + }, "the broker's TLS key is not the pinned key"}, + {"a client key the broker does not pin", func(t *testing.T) granter { + return granterFor(t, gb.addr, newTLSKey(t), gb.key.pin) + }, "refused the TLS connection"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + code, stderr := grantIn(t.Context(), tt.g(t), grantArgs()...) + if code != exitUnavailable || !strings.Contains(lastLine(stderr), tt.want) || + !strings.HasSuffix(stderr, "(exit 69)\n") { + t.Errorf("exit code %d, stderr %q; want 69 and %q", code, stderr, tt.want) + } + }) + } + if n := len(gb.audit.all()); n != 0 { + t.Errorf("the broker has %d audit entries; no request should reach it", n) + } +} + +// TestSSHGrantDeadline: a broker that does not answer in time. nodkey +// ssh-grant closes the connection at the deadline, and exits with 69. +func TestSSHGrantDeadline(t *testing.T) { + t.Parallel() + fb := newFakeBroker(t, func(_ http.ResponseWriter, r *http.Request) { + _, _ = io.Copy(io.Discard, r.Body) + <-r.Context().Done() + }) + fb.b.timeout = 500 * time.Millisecond + code, stderr := grantIn(t.Context(), testGranter(fb.b, nil), grantArgs()...) + if want := "nodkey: no answer from the broker within 500ms (exit 69)"; code != exitUnavailable || + lastLine(stderr) != want { + t.Errorf("exit code %d, stderr %q; want the last line %q", code, stderr, want) + } + if n := fb.requests.Load(); n > 1 { + t.Errorf("%d requests, want at most 1", n) + } +} diff --git a/internal/telegram/sshgrant_text_test.go b/internal/telegram/sshgrant_text_test.go new file mode 100644 index 0000000..d10bcf3 --- /dev/null +++ b/internal/telegram/sshgrant_text_test.go @@ -0,0 +1,112 @@ +package telegram + +import ( + "context" + "fmt" + "os" + "path/filepath" + "slices" + "strings" + "testing" + "time" + + "github.com/code6x/nodkey/internal/approval" + "github.com/code6x/nodkey/internal/audit" +) + +// designGrant is the example of an SSH grant in DESIGN.md section 7, as the +// person sees it. It is the message of +// +// nodkey ssh-grant --key deploy --user deploy --host web-1 --host web-2 \ +// --for 15m --reason "apply the nginx fix" +const designGrant = `🔐 SSH grant · request m4k2 +✅ machine: laptop +✅ key: deploy +✅ hosts: web-1, web-2 +✅ remote user: deploy +✅ grants end: 14:20:00 (15 min) +— claimed by the agent — +reason: apply the nginx fix +expires 14:07:00 (120 s) +[ Approve ] [ Deny ]` + +// sshGrantPrompt is the prompt that the core makes for that command: +// nodkey ssh-grant sends no command line, only the reason. +func sshGrantPrompt() approval.Prompt { + p := grantPrompt() + p.Claim = approval.Claim{Reason: "apply the nginx fix"} + return p +} + +// TestSSHGrantMessageAsInDesign: the text that the person sees for a +// request of nodkey ssh-grant is exactly the example of DESIGN.md section +// 7, and DESIGN.md still shows it. The verified lines list the key, every +// host, the remote user and the end of the grants. The message has only +// "Approve" and "Deny". +func TestSSHGrantMessageAsInDesign(t *testing.T) { + doc, err := os.ReadFile(filepath.Join("..", "..", "DESIGN.md")) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(doc), "```\n"+designGrant+"\n```") { + t.Errorf("DESIGN.md section 7 does not show this message any more:\n%s", designGrant) + } + e := newEnv(t) + shown, err := e.a.Ask(context.Background(), sshGrantPrompt()) + if err != nil { + t.Fatal(err) + } + if shown.Text != designGrant { + t.Errorf("text:\n%s\nwant:\n%s", shown.Text, designGrant) + } +} + +// TestSSHGrantShowsEveryHost: the message of an SSH grant lists every +// host, also 20 hosts with the longest names, with the longest key name +// and remote user, and a long reason. The reason is cut so that the +// message fits; no verified line is cut, also not after the request +// ended. +func TestSSHGrantShowsEveryHost(t *testing.T) { + p := sshGrantPrompt() + p.Key = strings.Repeat("k", 64) + p.RemoteUser = "u" + strings.Repeat("x", 31) + p.Hosts = nil + for i := range approval.MaxGrantHosts { + p.Hosts = append(p.Hosts, fmt.Sprintf("h%02d-", i)+strings.Repeat("x", 60)) + } + p.Claim.Reason = strings.Repeat("😀", 3000) + verified := []string{ + "✅ machine: laptop", "✅ key: " + p.Key, "✅ hosts: " + strings.Join(p.Hosts, ", "), + "✅ remote user: " + p.RemoteUser, "✅ grants end: 14:20:00 (15 min)", + } + asked, kb, err := promptMessage(p) + if err != nil { + t.Fatal(err) + } + if got := buttonsLine(kb); got != "[ Approve ] [ Deny ]" { + t.Errorf("the buttons are %s", got) + } + approved := endingText(p, approval.Ending{Answer: audit.AnswerApproveWindow, WindowMinutes: 15, + At: time.Date(2026, 10, 14, 14, 2, 41, 0, time.UTC)}) + for _, tc := range []struct { + name string + tx *text + last string + }{ + {"the message", asked, "expires 14:07:00 (120 s)"}, + {"the message after the approval", approved, "👍 approved by you at 14:02:41, until 14:20:00"}, + } { + plain := tc.tx.plain.String() + lines := strings.Split(plain, "\n") + if len(lines) < 9 || !slices.Equal(lines[1:6], verified) { + t.Errorf("%s: the verified lines are not all there:\n%s", tc.name, plain) + continue + } + if !strings.HasPrefix(lines[7], "reason: 😀") || !strings.HasSuffix(lines[7], "…") || lines[8] != tc.last { + t.Errorf("%s: the reason is not cut, or the last line is wrong:\n%.300s…%s", tc.name, plain, lines[8]) + } + if n := utf16Len(plain); n > maxTextUnits { + t.Errorf("%s: %d UTF-16 code units, more than %d", tc.name, n, maxTextUnits) + } + } +}