feat(ssh-grant): one tap for SSH logins to a list of hosts

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

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,25 +1584,82 @@ 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;
 - a host is not in the broker's `known_hosts`, or not in the key's `hosts`
   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: <reason> (exit <code>)`, 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: <result> (exit <code>)`, 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: <reason> (exit 1)`.

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:

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":

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)
+}

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())
+	}
+}

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
 }
 

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 {

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
 		}

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)
+	})
+}

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.