diff --git a/AGENTS.md b/AGENTS.md index 71d2fe2..99006f7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -24,7 +24,10 @@ Planned. Each v1 issue adds its part. telegram, store, sshauth, client parts). Packages do not import `cmd/`. - `internal/api/` - the protocol between the clients and the broker: - the paths, the request and answer types, and their caps. + the paths, the headers, the request and answer types, and their caps. + - `internal/version/` - versions of nodkey: the rule of a version that + the other side sends, release versions, and how two compare (DESIGN.md + section 10). - `internal/broker/` - the broker's service: it answers each request in the order of DESIGN.md sections 5 and 6. `nodkey broker` makes its parts and runs it. diff --git a/DESIGN.md b/DESIGN.md index 7d55b11..737abc4 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -170,7 +170,8 @@ nodkey run --secret GH_TOKEN=github-token \ `nodkey: waiting for approval (request 7qd2)` to stderr, so a person at the terminal can match it with the message on the phone. It takes the code only from that answer, and prints it once, only when it has the - form of a short code. It never prints another header. + form of a short code. It prints no other header, except the broker's + version in one warning, when the two versions differ (section 10). 6. On approval, the broker first writes the `request` entry and the `decision` (or `grant_used`) entry to the audit log; each one is synced to disk. Only then do the grants of an "Approve N min" tap start, and @@ -200,11 +201,12 @@ nodkey run --secret GH_TOKEN=github-token \ 8. On deny, timeout or error, the client prints one line to stderr, without any value, and exits: with 77 when the broker denies the request, and with 69 when it cannot reach the broker or gets another answer - (section 15). It never prints an answer body. If the person stops - `nodkey run` with Ctrl-C (`SIGINT`) or with `SIGTERM` while it waits, - the client closes the connection, so the broker cancels the request. - Then it prints its last line and ends itself with the same signal - (section 15). + (section 15). Right before that line, it may print one warning about + the version of the broker (section 10). It never prints an answer + body. If the person stops `nodkey run` with Ctrl-C (`SIGINT`) or with + `SIGTERM` while it waits, the client closes the connection, so the + broker cancels the request. Then it prints its last line and ends + itself with the same signal (section 15). There is no command that prints a secret. This is on purpose: the agent should never see the value in its own output. @@ -561,6 +563,23 @@ allowed by a "never ask" rule, without a tap command: restic backup /srv ``` +A notice about the version of a client (section 10) has no buttons +either. It goes to the notice chat without a sound, once for each client +in each start of the broker, rarely more than once (see "Telegram +details" below), in one of these forms: + +``` +ℹ️ nodkey: The client laptop says that it runs nodkey 0.2.0, which is older than nodkey 0.3.0 on the broker. To update it, run there: sudo nodkey update +ℹ️ nodkey: The client laptop says that it runs nodkey 0.4.0, which is newer than nodkey 0.3.0 on the broker. To update the broker, run as root on the broker host: nodkey update +ℹ️ nodkey: The client laptop sent no version of nodkey, so it may run an older nodkey. The broker runs nodkey 0.3.0. To update laptop, follow section 13 of docs/setup.md. +``` + +The name is the name of the `[client.*]` section, and the versions are +release versions: digits and dots only. "says that it runs" marks the +client's version as a claim. A client that sends no version runs nodkey +0.1, which has no `nodkey update`, so that notice points to the update +steps of [docs/setup.md](docs/setup.md#a-new-version-of-nodkey) instead. + - The host key shows only `SHA256:` and its first 4 characters. The broker has checked the key against its own `known_hosts`, and the host name is what the person reads. @@ -999,11 +1018,14 @@ Not masked, for example: request that waits for the person is denied at once, and so is every new request that needs a message. A message or a notice that is still being sent stops too, and no new notice goes out, so a "never ask" use that - needs a notice is denied as well. The approval core would still let - grants and "never ask" rules with `notify = false` allow requests, but - the broker denies every request from then on (503), and `nodkey broker` - stops (section 15). On a polling conflict, the broker first writes the - `error` entry with code `poll_conflict`. + needs a notice is denied as well. A notice about the version of a + client (below) is different: no new try of it starts, but a try that is + still being sent goes on until the service stops (section 15). The + approval core would still let grants and "never ask" rules with + `notify = false` allow requests, but the broker denies every request + from then on (503), and `nodkey broker` stops (section 15). On a + polling conflict, the broker first writes the `error` entry with code + `poll_conflict`. - Messages about "never ask" uses can go to a second chat (`notice_chat_id`), so the approval chat stays for approvals only. They are silent (`disable_notification`). At most 20 notices go out in @@ -1017,6 +1039,32 @@ Not masked, for example: `approver_failed`), and the use still counts, because the core cannot tell if the notice went out. That holds for Telegram's own code 429 after the last try too: an earlier try may have sent the notice. +- A notice about the version of a client (section 10) goes to the same + chat, also silent and with link previews off. It does not count toward + the 20 notices above, nor toward the 6 of a client in section 8, so a + "never ask" use right after it still gets its notice. It has its own + limit: at most 3 in any 60 seconds, for all clients together, so it + takes little of what Telegram allows in the chat. Telegram counts all + the messages of the chat, though. With up to 3 notices about versions + in a minute, such as right after a start, about 18 notices of "never + ask" uses already pass the about 20 that Telegram allows. Then + Telegram can refuse a notice of a "never ask" use with its code 429. + If the tries of that notice do not get through in time, the use is + denied, as for any other failure of its notice. This needs about 18 + such uses in one minute, so it is rare. +- The broker sends a notice about a version in the background, so it + never delays or denies a request. A client has at most one try at a + time, and each try is one call. A try that fails, also one that its + limit of 3 stops, is logged. Then the next request of that client that + comes at least 60 seconds after the failure makes a new try; there is + no timer. Once a notice went out, that client gets no more tries in + this start. A try can fail after Telegram took the message, such as on + a time-out; then the next try sends it again. Such a repeated notice + is rare and does no harm. A try that is still being sent when the + service stops is cancelled (section 15, step 2), and a try that fails + while the broker stops says that no new try comes. So each client gets + one such notice in each start, rarely more than one, and at most one + try a minute. ## 8. Rules and grants @@ -1098,7 +1146,8 @@ name exactly one client. The config check refuses all of these (section way: cause `limit`, and it does not count for `max_per_day`. A notice that the Telegram code refused, or could not send, still counts toward the 6 of its client, so a client cannot try again and again within the - minute. + minute. A notice about the version of a client (section 7) counts + toward neither of these limits. ## 9. Audit log @@ -1547,6 +1596,64 @@ a path, a query, `OPTIONS *` or `PRI *` gets 404. The types are in for a moment. Another request waits for a place, at most 10 seconds, and then gets `429`. +**Versions.** The JSON is strict on both sides, so a client and a broker +of different versions can fail with only "the answer of the broker is not +valid", or a `400`. So each side tells the other its version, and says +when they differ. + +- Each request has the header `Nodkey-Version` with the client's version, + as `nodkey version` prints it, such as `0.2.0` or `dev`. Each answer of + the broker has it with the broker's version: with every status, also + errors, `404`, `405` and the `102`. It is a header, not a JSON field, + so versions with other fields still read it. Two kinds of answers come + before the broker's code, and have no such header: the answers that + net/http writes itself to a request that is not valid HTTP, such as + headers over the limit, and the `403` for a request without a stored + client name (above). The client never causes them. +- A side takes the other side's value only when it is one header that + matches `^[A-Za-z0-9][A-Za-z0-9.+_-]{0,63}$`, the rule of the `version` + of the `broker_started` entry (section 9). Any other value counts as no + version, and is never printed or logged. A side whose own version does + not match sends none. +- Only release versions are compared, as numbers: `X.Y.Z`, with numbers + from 0 to 999999 and no leading zeros. Any other version, such as + `dev`, a commit or `1.2.0-rc.1`, is a build from the source: when either + side is one, neither side says anything. +- The client's version is a claim: anyone with the client's key can send + any value (THREAT-MODEL.md T33). It changes no answer, no check and no + audit entry, and the request never waits for what the broker does with + it. +- The client compares after each final answer, whatever its status. When + the two releases differ, or the client is a release and the broker + sends no valid version, it prints one warning on stderr, before its + last line or before the command starts. The line says which side is + older, and what to do there, for example: + + ``` + nodkey: warning: the broker runs nodkey 0.2.0, which is older than nodkey 0.3.0 on this machine; to update the broker, run "nodkey update" as root on the broker host + nodkey: warning: this machine runs nodkey 0.2.0, which is older than nodkey 0.3.0 on the broker; to update this machine, run "sudo nodkey update" + nodkey: warning: the broker runs an older nodkey that does not send its version, and this machine runs nodkey 0.3.0; to update the broker, follow section 13 of docs/setup.md + ``` + + Only a side that sends its version has `nodkey update`. A broker that + sends none runs nodkey 0.1, so the last line points to the update steps + of [docs/setup.md](docs/setup.md#a-new-version-of-nodkey) instead. + The line holds only fixed text and release versions. It goes only to + stderr, never into the environment or the output of the command. + `nodkey run` and `nodkey ssh-grant` send one request, so they print it + at most once. `nodkey ssh-agent` prints it once for each version that + the broker sends, so an update of the broker gives one more line, and + it stops after 16 lines. +- The broker compares when the TLS key has given the client's name. When + the client's release is not the broker's, or the client sends no valid + version while the broker is a release, the broker writes one line in + the journal, once for each client in each start of the broker, however + many versions the client claims. The line names the client, the + client's release when it sent one, and the broker's version. It also + sends one notice (section 7). A try of the notice that fails is tried + again with a later request of the client, at least 60 seconds later, + until one goes out. The request waits for neither. + **The broker's log.** Any peer can open a connection, and the log goes to the journal next to the audit anchors (section 9). So a log line about a connection never holds text that the peer chose, such as its ALPN names, @@ -1569,7 +1676,9 @@ a field of its certificate, or a line break: in its next line, or at the end of the minute when no line comes. - The broker's own lines come from slog's text handler: each line starts with `time=`, and a value with a line break is quoted and escaped. The - broker never logs claimed text, and cuts each value to 1 KiB. So no line + broker never logs claimed text, except the version that a client claims + when it is a release version, which is only digits and dots (see + "Versions" above), and cuts each value to 1 KiB. So no line can start with `audit anchor ` (section 9), and none comes near the 48 KiB at which journald splits a line. It logs at most 20 requests in a minute that a check refused, SSH logins (section 6) and variables on @@ -2104,7 +2213,8 @@ order: request that waits for the person is denied. 2. The service: new requests get `503`, and the requests in progress end and get `503` too (section 10). The broker waits for them, and writes - the `foreign_tap` entries that still wait. + the `foreign_tap` entries that still wait. A notice about the version + of a client that is still being sent is cancelled (section 7). 3. The server: it closes the `listen` address and every connection. 4. The alerts of the start: each one gets its first try, also when the stop began right after the start, but no try again. The tries run at @@ -2192,7 +2302,8 @@ The command itself may also exit with one of these codes. To tell them apart, look at the last line on stderr. Only when nodkey itself exits, its last line is `nodkey: (exit )`, for example `nodkey: denied (exit 77)`. When the command runs, nodkey prints nothing -after it. +after it. Before its last line, or before the command starts, nodkey may +print one warning about the version of the broker (section 10). `nodkey ssh-grant` exits with: @@ -2220,6 +2331,8 @@ nothing: the grants are made, and the last line says so. `-h` and 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)`. +Before it, it may print one warning about the version of the broker +(section 10). `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 @@ -2269,8 +2382,10 @@ Unix socket until `SIGINT` or `SIGTERM` (section 6). It holds no SSH key. k3f9)`), and one line about each request that it refused or that failed, such as `nodkey: no signature (request k3f9): denied`, with the same texts as `nodkey run`: `denied` for `403`, and `denied by a limit - of the broker` for `429`. A line holds only fixed text, numbers, the - short code, the broker's address, the socket's path and errors of the + of the broker` for `429`. It also prints a warning about the version of + the broker, once for each version that the broker sends (section 10). + A line holds only fixed text, numbers, release versions, the short + code, the broker's address, the socket's path and errors of the operating system, never text that a process on the agent machine sent. - Any process of the user can cause both kinds of lines: the broker sends a short code for every request that starts to wait, also in its @@ -2582,8 +2697,10 @@ bws 2.1.0 and bw 2026.9.1 on 2026-10-14 the logins after it; a denied login, after which the grants of the key have ended. Each case also checks what the message shows, what the clients print, the entries in the audit log, and that no other - message goes out. The config has no "never ask" rule, and nothing in - the test makes an alert, so every message must have buttons. At the + message goes out. The config has no "never ask" rule, nothing in the + test makes an alert, and the client and the broker are the same build + from the source, so no notice about a version goes out either: every + message must have buttons. At the end, `nodkey audit verify` checks the whole audit log with every anchor of the broker's journal, and a copy of the log with one changed entry fails. diff --git a/THREAT-MODEL.md b/THREAT-MODEL.md index 3e58951..57d9881 100644 --- a/THREAT-MODEL.md +++ b/THREAT-MODEL.md @@ -281,8 +281,18 @@ clients fail. The Telegram code also sends at most 20 notices in any 60 seconds for all clients together. A use over either limit is denied with cause `limit`, and it does not count for `max_per_day`. So a burst of notices of other clients cannot use up the uses of a night; it only -denies uses while the limit is full. -Risk that remains: low. +denies uses while the limit is full. A notice about the version of a +client (T33) counts toward neither limit. It has its own: at most 3 in +any 60 seconds, at most one try a minute for each client, and one notice +for each client in each start of the broker, rarely more than one. +Risk that remains: low. Telegram counts all the messages of the notice +chat. With up to 3 notices about versions in a minute, such as right +after a start, about 18 notices of "never ask" uses already pass the +about 20 that Telegram allows. Then Telegram can refuse a notice of a +"never ask" use with its code 429. If the tries of that notice do not +get through in time, the use is denied, and it counts for `max_per_day` +(DESIGN.md section 7). This needs about 18 such uses in one minute, so +it is rare. **T10. Someone on the network reads or changes the traffic.** TLS 1.3, with both sides' keys pinned. The pin is checked on every @@ -346,9 +356,11 @@ output.** No print command. Values that nodkey fetched are never logged, never in errors, never in the audit log, never in Telegram. Store program output is never logged. `nodkey run` prints only its own fixed text, the short code -of the `102 Processing` answer, the HTTP status, and names from its -arguments and its config. It never prints an answer body or another -header, and nothing after the command started. A command line can hold +of the `102 Processing` answer, the HTTP status, names from its +arguments and its config, and in a warning the broker's version from +the header `Nodkey-Version`, only when it is a release version: digits +and dots (DESIGN.md section 10). It never prints an answer body or any +other header, and nothing after the command started. A command line can hold a secret the agent typed in (for example `mysql -pSECRET`), and it would go to Telegram and the audit log. So the broker masks values that look like secrets in every claimed field, and @@ -772,8 +784,8 @@ of `golang.org/x/crypto` writes the text of each error to Go's global logger, without escaping and without a limit. So `nodkey ssh-agent` serves the agent protocol itself, and sends Go's global logger nowhere. Its own lines hold fixed text, numbers, the short code, the broker's -address, the socket's path and errors of the operating system, never text -that a process sent. A process can also cause many lines with a short +release version, the broker's address, the socket's path and errors of +the operating system, never text that a process sent. A process can also cause many lines with a short code: the broker sends one for every request that starts to wait, also in its queue, and a request that hangs up frees its place. So the agent writes at most 60 lines with a short code, and 60 about refused and @@ -855,6 +867,35 @@ What to do: Risk that remains: real. nodkey cannot see a read in Bitwarden's own apps, and it cannot check where an account is logged in. +**T33. A client claims a false version.** +Each request has the client's version in a header, and each answer the +broker's (DESIGN.md section 10). Anyone with the client's key (T11) can +send any value there. So the broker takes it only as a claim: it changes +no answer, no check and no audit entry, and the request never waits for +it. A value that does not match the strict rule of a version counts as +none, and is never logged or shown. A false claim can cause one wrong +line in the journal and one wrong notice for that client in each start +of the broker, rarely more than one notice, however many values the +client sends. +A notice that did not go out is tried again, at most once a minute for +each client, with a journal line for each failed try. The notice is +silent, shows only the client's name from the config and release +versions, which are digits and dots, and has its own limit of 3 in a +minute. So it does not take from the limits of the notices of "never +ask" uses (T9), and it cannot fill the notice chat. The check is cheap, +so it adds nothing to T28. +The other way, the client takes the broker's version only from the +pinned connection (T10), and shows only release versions from it. So +every pinned client, also a hostile agent with the client's key (T11), +learns the exact version of the broker: whether it misses a security +fix of a newer release. A peer without a pinned key learns nothing: the +TLS handshake fails before any answer. After a security release, update +the broker first. +Risk that remains: low. A false claim can only send the person to update +a machine that is up to date (also the broker host), or hide that a +client is old. To know what runs on a machine, run `nodkey version` +there. + ## 7. Assumptions - The broker host is patched, has no agent on it, and only the owner has diff --git a/cmd/nodkey/broker.go b/cmd/nodkey/broker.go index fd6a68d..51101d6 100644 --- a/cmd/nodkey/broker.go +++ b/cmd/nodkey/broker.go @@ -309,7 +309,8 @@ func (b *brokerParts) start(ctx context.Context, path string) error { return err } - svc, err := broker.New(broker.Parts{Config: cfg, Audit: l, Stores: b.stores, Keys: keys, Logger: b.log}) + svc, err := broker.New(broker.Parts{Config: cfg, Audit: l, Stores: b.stores, Keys: keys, Logger: b.log, + Version: version}) if err != nil { return err } @@ -447,7 +448,7 @@ func (b *brokerParts) startRun(core *approval.Core) { "every request is denied, and the broker stops") } }() - b.runErr = b.svc.Run(ctx, core, b.tg.Alert) + b.runErr = b.svc.Run(ctx, core, b.tg.Alert, b.tg.Inform) }() b.log.Info("nodkey broker: the approver runs: it polls Telegram for the answers") } diff --git a/cmd/nodkey/broker_linux_test.go b/cmd/nodkey/broker_linux_test.go index 15d3da9..426cb6d 100644 --- a/cmd/nodkey/broker_linux_test.go +++ b/cmd/nodkey/broker_linux_test.go @@ -198,6 +198,51 @@ func TestBrokerStartsAndStops(t *testing.T) { } } +// TestBrokerVersionNotice: a release of nodkey broker sends its version on +// each answer. A client that sends no version, as nodkey 0.1.0 does, gets +// one line in the journal and one silent notice, through the Telegram +// approver, in the chat of the notices: here the approver's. A second +// request gets neither, and the stop is clean. +func TestBrokerVersionNotice(t *testing.T) { + t.Parallel() + e := startListening(t, brokerOptions{}, seamReleaseVersion) + e.stderr.wait(t, lineListening) + c := e.httpClient() + for range 2 { + a := e.post(c, api.PathKeys, api.KeysRequest{}) + mustStatus(t, a, http.StatusOK) + if got := a.header.Values(api.VersionHeader); !slices.Equal(got, []string{"0.2.0"}) { + t.Errorf("the answer has the version %q, want 0.2.0", got) + } + } + var notice struct { + ChatID int64 `json:"chat_id"` + Text string `json:"text"` + DisableNotification bool `json:"disable_notification"` + LinkPreviewOptions struct { + IsDisabled bool `json:"is_disabled"` + } `json:"link_preview_options"` + } + if err := json.Unmarshal([]byte(e.tg.waitAlert("sent no version of nodkey")), ¬ice); err != nil { + t.Fatal(err) + } + want := "ℹ️ nodkey: The client laptop sent no version of nodkey, so it may run an older nodkey. The broker " + + "runs nodkey 0.2.0. To update laptop, follow section 13 of docs/setup.md." + if notice.ChatID != approverID || !notice.DisableNotification || !notice.LinkPreviewOptions.IsDisabled || + notice.Text != want { + t.Errorf("the notice: %+v", notice) + } + e.stderr.wait(t, "broker: a client sent no version of nodkey") + e.signal(syscall.SIGTERM) + e.mustExit(exitOK) + if n := e.tg.alertCount("sent no version of nodkey"); n != 1 { + t.Errorf("%d notices, want 1", n) + } + if n := e.stderr.count("a client sent no version of nodkey"); n != 1 { + t.Errorf("%d lines in the journal, want 1", n) + } +} + // TestBrokerPollConflict: when Telegram reports a polling conflict, the // broker sends an alert, writes an error entry with poll_conflict, stops // in order, and exits with exitPollConflict, which the example unit lists diff --git a/cmd/nodkey/brokerenv_linux_test.go b/cmd/nodkey/brokerenv_linux_test.go index 4b34d04..354e86a 100644 --- a/cmd/nodkey/brokerenv_linux_test.go +++ b/cmd/nodkey/brokerenv_linux_test.go @@ -82,6 +82,9 @@ const ( // seamFastAlertTries makes the tries of the alert messages of the // start short: 1.5 seconds each, and 0.1 to 0.4 seconds between them. seamFastAlertTries = "fast-alert-tries" + // seamReleaseVersion makes the child the release 0.2.0, as a release + // build is with -ldflags "-X main.version=0.2.0". + seamReleaseVersion = "release-version" ) // telegramHost is the host of the Bot API. @@ -134,6 +137,8 @@ func TestBrokerChild(t *testing.T) { } case seamFastAlertTries: alertTimeout, alertFirstWait, alertMaxWait = 1500*time.Millisecond, 100*time.Millisecond, 400*time.Millisecond + case seamReleaseVersion: + version = "0.2.0" case seamNoCoreDumps: if err := syscall.Setrlimit(syscall.RLIMIT_CORE, &syscall.Rlimit{}); err != nil { fmt.Fprintln(os.Stderr, "child: cannot set the hard limit of core dumps to 0") @@ -737,6 +742,7 @@ func (e *brokerEnv) httpClient() *http.Client { // brokerAnswer is what a client got. type brokerAnswer struct { status int + header http.Header body []byte err error } @@ -761,7 +767,7 @@ func (e *brokerEnv) post(c *http.Client, path string, body any) brokerAnswer { } defer func() { _ = resp.Body.Close() }() b, err := io.ReadAll(io.LimitReader(resp.Body, api.MaxAnswer+1)) - return brokerAnswer{status: resp.StatusCode, body: b, err: err} + return brokerAnswer{status: resp.StatusCode, header: resp.Header, body: b, err: err} } // approve sends a request that needs a tap, taps "Approve once" on its diff --git a/cmd/nodkey/run.go b/cmd/nodkey/run.go index 7372d2e..5d60f0a 100644 --- a/cmd/nodkey/run.go +++ b/cmd/nodkey/run.go @@ -11,5 +11,5 @@ import ( // ignore SIGPIPE, as keygen does: an ignored signal stays ignored after // execve, so the command would run with it ignored. func runRun(args []string, stderr io.Writer) int { - return client.Run(args, stderr) + return client.Run(args, stderr, version) } diff --git a/cmd/nodkey/sshagent.go b/cmd/nodkey/sshagent.go index 860fb67..0dc213a 100644 --- a/cmd/nodkey/sshagent.go +++ b/cmd/nodkey/sshagent.go @@ -18,5 +18,5 @@ import ( // machine sent. func runSSHAgent(args []string, stderr io.Writer) int { log.SetOutput(io.Discard) - return client.SSHAgent(args, stderr) + return client.SSHAgent(args, stderr, version) } diff --git a/cmd/nodkey/sshgrant.go b/cmd/nodkey/sshgrant.go index 3488fc7..84bcc33 100644 --- a/cmd/nodkey/sshgrant.go +++ b/cmd/nodkey/sshgrant.go @@ -10,5 +10,5 @@ import ( // 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) + return client.SSHGrant(args, stderr, version) } diff --git a/docs/setup.md b/docs/setup.md index 231ce3d..c3f49d3 100644 --- a/docs/setup.md +++ b/docs/setup.md @@ -113,9 +113,10 @@ section 5, item 8). So: ## 2. Get nodkey Install the same version of nodkey on the broker host and on each agent -machine, as `/usr/local/bin/nodkey`, owned by root. **Do it as root on -each host, never as the user of the agent.** The agent can change what -its own user downloads, builds or copies +machine, as `/usr/local/bin/nodkey`, owned by root. When the versions +differ, nodkey says so ([section 13](#a-new-version-of-nodkey)). **Do it +as root on each host, never as the user of the agent.** The agent can +change what its own user downloads, builds or copies ([section 1](#where-you-do-the-setup)). Get the root shell in a way that does not go through the agent's account: @@ -1382,6 +1383,43 @@ poll of the old broker was still on its way program uses the token, start the broker again with `systemctl start nodkey`. +Until every machine runs the new version, nodkey tells you. Each client +sends its version to the broker, and the broker sends its own back. +nodkey 0.1 sends no version. + +- On the agent machine, `nodkey run`, `nodkey ssh-grant` and + `nodkey ssh-agent` print a warning on stderr. It says which side is + older, and what to do there: + + ```text + nodkey: warning: the broker runs nodkey 0.2.0, which is older than nodkey 0.3.0 on this machine; to update the broker, run "nodkey update" as root on the broker host + ``` + + With a broker of nodkey 0.1, the warning says that the broker runs an + older nodkey that does not send its version, and points to this + section: nodkey 0.1 has no `nodkey update`. +- The broker writes a line in its journal, once in each start for each + client that runs another version, or that sends none. It also sends + you one silent notice for such a client: + + ```text + ℹ️ nodkey: The client laptop says that it runs nodkey 0.2.0, which is older than nodkey 0.3.0 on the broker. To update it, run there: sudo nodkey update + ``` + + For a client of nodkey 0.1, the notice points to this section too. If + the notice cannot go out, such as while Telegram cannot be reached + after a power cut, the journal says so, and the next request of that + client a minute later or more tries again. Rarely, the same notice + comes more than once. + +The steps above make both go away. A machine that still runs another +version works as before, unless the two versions do not understand each +other's requests: then the request fails, for example with +`the answer of the broker is not valid`. The version of a client is only +what the client says: the broker allows and denies nothing because of +it. A build from the source, such as one with the version `dev` or a +commit, gets no warning and no notice. + ### New versions of bws and bw Install them in the same way as the first time @@ -1453,6 +1491,8 @@ error names the file, the section or the key, never a secret value. | The installer says that `group or others can write to` a directory or a file, or that a path `is owned by user`. | As root, the installer runs programs from these places, so only root may be able to change them, and each directory above them. Fix the owner or the mode, such as with `chmod go-w /usr/local/bin`, or choose another place with `--bin-dir`, `--gh` or `TMPDIR`. A Debian system that was set up long ago may still have `/usr/local/bin` writable by the group `staff`. | | systemd does not start the broker: `systemctl status nodkey` shows `status=243/CREDENTIALS`. | systemd cannot decrypt a credential, so the broker does not run, and nothing is handed out. Check that each file of a `LoadCredentialEncrypted=` line is there, and was made with that name. After a firmware update or a change of the Secure Boot settings, the TPM2 chip may refuse credentials that are bound to PCR 7 ([section 3](#encrypted-credentials)). If you did not update the firmware or change Secure Boot, find out why first: someone may have changed how the machine starts. Then make each credential again, as in [section 13](#13-upgrades-and-key-changes): a new TLS key (and its pin on each client), the bot token, a new `bws` token, the `bw` master password, and new SSH keys. | | The journal says that the bot may not send to this chat. | You did not tap **Start** in the chat with the bot, or you blocked it. | +| The client prints `nodkey: warning: ...` with the versions of this machine and the broker. | They run different versions of nodkey. Update the older side, as the line says ([section 13](#a-new-version-of-nodkey)). Requests can fail until then, for example with `the answer of the broker is not valid`. | +| A silent notice says that a client runs another version of nodkey, or sent no version. | Update nodkey on the older side, as the notice says ([section 13](#a-new-version-of-nodkey)). The broker sends one such notice for each client in each start, rarely more than one, and the journal has a line about it. | | `a store could not start` | The broker runs without that store, denies requests for its secrets, and tries again with each request. The error names the store and the step. For bw: check the login ([section 5](#bw-a-separate-bitwarden-account)). | | The broker refuses a file: the owner, the mode, or a directory that group or others can write to. | The broker checks every file that it reads, and every directory on the way ([DESIGN.md section 14](../DESIGN.md#14-configuration)). Fix the owner or the mode that the error names. | | `cannot listen on ...` | The `listen` address is not on the broker host yet, such as before the VPN is up. systemd tries again every 5 seconds. | diff --git a/internal/api/api.go b/internal/api/api.go index 1df1b9f..0552c37 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -8,6 +8,8 @@ // status with {"error": ""} and nothing more. While a request // waits for the approver, the broker first sends an informational answer, // 102 Processing, with the short code in the header ShortCodeHeader. +// Each request has the client's version in the header VersionHeader, and +// each answer the broker's. // // The decoding is strict, on both sides: every field must be there, no // field may be null, and no other field may be there. Keys must match @@ -21,6 +23,8 @@ import ( "regexp" "slices" "strings" + + "github.com/code6x/nodkey/internal/version" ) // The paths of the broker. Each takes POST only. @@ -49,6 +53,26 @@ const ContentType = "application/json" // (DESIGN.md section 5). Read it with ShortCode. const ShortCodeHeader = "Nodkey-Short-Code" +// VersionHeader holds the version of nodkey of the side that sends it, +// such as "1.2.0" or "dev": the client's in each request, and the +// broker's in each answer, with every status (DESIGN.md section 10). It +// is a header, not a field of the JSON body, so that versions with other +// fields can still read it. The client's version is a claim: the broker +// decides nothing with it. Read it with Version. +const VersionHeader = "Nodkey-Version" + +// Version returns the version in the header VersionHeader of a request or +// an answer. It returns "" when there is no such header, more than one, +// or a value that version.Valid refuses. Such a value counts as no +// version, and is never printed or logged. +func Version(h http.Header) string { + values := h.Values(VersionHeader) + if len(values) != 1 || !version.Valid(values[0]) { + return "" + } + return values[0] +} + // Caps on a request (DESIGN.md section 10). With them, every valid request // fits in the 2 MiB that the broker reads (transport.MaxRequestBody); a // test checks it. diff --git a/internal/api/api_test.go b/internal/api/api_test.go index 7a071bd..09cabec 100644 --- a/internal/api/api_test.go +++ b/internal/api/api_test.go @@ -599,6 +599,36 @@ func TestShortCode(t *testing.T) { } } +// TestVersion: the version header counts only as one header with a valid +// version. Anything else is no version, so it is never passed on. +func TestVersion(t *testing.T) { + h := func(values ...string) http.Header { + return http.Header{http.CanonicalHeaderKey(VersionHeader): values} + } + for _, v := range []string{"0.1.0", "dev", "1a2b3c4", "v1.2.0-rc.1"} { + if got := Version(h(v)); got != v { + t.Errorf("Version(%q) = %q", v, got) + } + } + r, err := http.NewRequestWithContext(t.Context(), http.MethodPost, "https://broker.example/v1/keys", nil) + if err != nil { + t.Fatal(err) + } + r.Header.Set("nodkey-version", "0.1.0") // any case of the name + if got := Version(r.Header); got != "0.1.0" { + t.Errorf("Version of a request = %q", got) + } + for _, hh := range []http.Header{ + {}, h(), h(""), h("0.1.0", "0.2.0"), h("0.1.0", "0.1.0"), h("0.1.0, 0.2.0"), h("0.1.0 "), + h("0.1.0"), h("0.1.0\r\nX-Note: 1"), h(strings.Repeat("1", 65)), + {"Nodkey_version": {"0.1.0"}}, + } { + if got := Version(hh); got != "" { + t.Errorf("Version(%q) = %q, want none", hh, got) + } + } +} + // TestSecretsAnswerEncode: Encode writes the same bytes as json.Marshal, // into a buffer of exactly that size, and ReadAnswer takes them. It // refuses a variable name that is not valid, as such a name could need diff --git a/internal/audit/event.go b/internal/audit/event.go index 4ae56bc..c9dc69e 100644 --- a/internal/audit/event.go +++ b/internal/audit/event.go @@ -7,6 +7,8 @@ import ( "fmt" "reflect" "regexp" + + "github.com/code6x/nodkey/internal/version" ) // Event is the content of one audit entry: what happened. The events are @@ -276,8 +278,6 @@ var ( remoteUserPattern = regexp.MustCompile(`^[A-Za-z0-9._][A-Za-z0-9._-]{0,31}$`) // hostKeyPattern is an OpenSSH SHA256 fingerprint. hostKeyPattern = regexp.MustCompile(`^SHA256:[A-Za-z0-9+/]{43}$`) - // versionPattern is a version such as "1.2.0", "v1.2.0-rc.1" or "dev". - versionPattern = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9.+_-]{0,63}$`) ) // maxTapData is the most bytes of button data that Telegram allows. @@ -339,7 +339,7 @@ func (s *Subject) validate(needRequest bool) error { } func (e BrokerStarted) validate() error { - if !versionPattern.MatchString(e.Version) { + if !version.Valid(e.Version) { return errors.New("version must be a version such as 1.2.0 or dev") } return nil diff --git a/internal/broker/broker.go b/internal/broker/broker.go index d709e59..5379e9d 100644 --- a/internal/broker/broker.go +++ b/internal/broker/broker.go @@ -9,7 +9,9 @@ // it does, in this order: // // 1. It takes the client name only from transport.ClientName, and -// refuses a request without one. +// refuses a request without one. Then it compares the version that +// the client claims with its own (version.go). The claim changes +// nothing about the request. // 2. It routes by the exact method and path: anything else gets 404 or // 405, also "OPTIONS *" and "PRI *". // 3. It refuses every request while the approver is not running, after @@ -38,6 +40,9 @@ // answer with values is written by hand into one buffer, which is // cleared too (api.SecretsAnswer.Encode). // +// Every answer, with every status, has the broker's version in the header +// api.VersionHeader. +// // A panic in a request denies it: the connection drops, and the audit log // records the request, also before the core gave it an ID. Requests that a // limit denies, or that come after the approver stopped, cost the client @@ -68,6 +73,7 @@ import ( "github.com/code6x/nodkey/internal/sshauth" "github.com/code6x/nodkey/internal/store" "github.com/code6x/nodkey/internal/telegram" + "github.com/code6x/nodkey/internal/version" ) // MaxRequestsPerClient is the most requests of one client that the broker @@ -138,6 +144,13 @@ type AuditLog interface { // deadline. type AlertFunc func(ctx context.Context, msg string) error +// NoticeFunc sends a silent notice of the broker itself to the notice +// chat, such as telegram.Approver.Inform. It must not take from the +// limits of the notices of "never ask" uses. It makes one try; the +// Service tries again later after an error (version.go). msg is the +// broker's own text. ctx has a deadline. +type NoticeFunc func(ctx context.Context, msg string) error + // Parts are the parts that a Service uses. nodkey broker makes them at // start: it checks the config files, loads each SSH key with // sshauth.NewKey and the hosts of its [key.*] section, opens the audit @@ -155,6 +168,12 @@ type Parts struct { // Logger gets the broker's log lines, such as from NewLogger. It may // be nil; then nothing is logged. Logger *slog.Logger + // Version is the version of this broker, as nodkey version prints it, + // such as "1.2.0" or "dev". The broker sends it in the header + // api.VersionHeader of every answer, and compares it with the version + // that each client sends (version.go). Without a valid version, it + // sends none and compares nothing. + Version string } // Service answers the requests of the clients. Make it with New, pass it @@ -168,6 +187,12 @@ type Service struct { keysOf map[string]api.KeysAnswer // the answer of PathKeys, by client log *slog.Logger random io.Reader // crypto/rand.Reader, for request IDs + // version is the broker's own version (Parts.Version), or "" when it + // is not valid. + version string + // now is time.Now, for the tries of the notices about versions; tests + // change it. Read it with mu held. + now func() time.Time // maxPerClient is MaxRequestsPerClient; tests change it. maxPerClient int @@ -190,8 +215,12 @@ type Service struct { mu sync.Mutex core *approval.Core // set by Run alert AlertFunc // set by Run + notice NoticeFunc // set by Run closed bool inflight map[string]int // requests in progress, by client + // versionNotes holds where the broker is with the notice about the + // version of each client of the config, in this start (version.go). + versionNotes map[string]*versionNote coreStopped atomic.Bool // the approval core's Run returned broken atomic.Bool // an audit write failed @@ -246,10 +275,16 @@ func New(p Parts) (*Service, error) { stopCtx: stopCtx, stop: stop, inflight: make(map[string]int), + versionNotes: make(map[string]*versionNote, len(cfg.Clients)), + now: time.Now, taps: make(chan audit.ForeignTap, tapQueue), tapsStop: make(chan struct{}), } + if version.Valid(p.Version) { + s.version = p.Version + } for name := range cfg.Clients { + s.versionNotes[name] = &versionNote{} s.clientBodies[name] = make(chan struct{}, maxBodiesPerClient) s.refusedEntries[name] = newRateLimit(refusedEntriesPerMinute, func(n int) { s.log.Warn("broker: did not write the audit entries of refused requests, to keep the log small", @@ -329,14 +364,15 @@ func checkKeys(cfg *config.Broker, keys map[string]sshauth.Key) (map[string]api. // passes the person's answers to the core, until ctx ends or the approver // stops. Requests that come before Run, and after Run returned, are // denied. alert sends an alert message, such as when the audit log -// breaks; it may be nil. +// breaks; it may be nil. notice sends a notice about a client that runs +// another version of nodkey (version.go); it may be nil. // // When the core's Run returns, every request is denied, also one that a // grant or a "never ask" rule would allow, and the broker must stop. Run // returns why the core stopped. If it was telegram.ErrPollConflict, // another program polls with the bot token, so Run first writes an error // entry with code poll_conflict. -func (s *Service) Run(ctx context.Context, core *approval.Core, alert AlertFunc) error { +func (s *Service) Run(ctx context.Context, core *approval.Core, alert AlertFunc, notice NoticeFunc) error { if core == nil { return errors.New("broker: Run needs the approval core") } @@ -349,7 +385,7 @@ func (s *Service) Run(ctx context.Context, core *approval.Core, alert AlertFunc) s.mu.Unlock() return errors.New("broker: Run can run only once") } - s.core, s.alert = core, alert + s.core, s.alert, s.notice = core, alert, notice s.mu.Unlock() // Deny every request once the core stops, also when the approver's diff --git a/internal/broker/handler.go b/internal/broker/handler.go index 0106b1a..1d626c2 100644 --- a/internal/broker/handler.go +++ b/internal/broker/handler.go @@ -49,13 +49,19 @@ func route(r *http.Request) (endpoint, int) { // ServeHTTP answers one request of a client (see the package comment for // the steps). Every answer goes through transport.WriteJSON, -// transport.WriteError or transport.WriteProcessing. +// transport.WriteError or transport.WriteProcessing, and has the broker's +// version in its header. func (s *Service) ServeHTTP(w http.ResponseWriter, r *http.Request) { + if s.version != "" { + // First, so that every answer has it, whatever its status. + w.Header().Set(api.VersionHeader, s.version) + } client, ok := transport.ClientName(r.Context()) if !ok { transport.WriteError(w, http.StatusForbidden) return } + s.checkVersion(client, r.Header) ep, status := route(r) if status != 0 { if status == http.StatusMethodNotAllowed { diff --git a/internal/broker/helpers_test.go b/internal/broker/helpers_test.go index c4d7f6e..1377310 100644 --- a/internal/broker/helpers_test.go +++ b/internal/broker/helpers_test.go @@ -386,6 +386,7 @@ type env struct { userKeys map[string]ssh.Signer hosts map[string]ssh.AlgorithmSigner // web-1, web-2, db-1 alerts *syncBuffer + notices *noticeLog // the notices about versions runErr chan error stopRun context.CancelFunc } @@ -398,6 +399,11 @@ type envOptions struct { noRun bool // wrap, if set, wraps the Service before it goes to the server. wrap func(http.Handler) http.Handler + // version is the broker's version (Parts.Version); "" for none. + version string + // clients are more clients, each with its own key, and no secret or + // key of their own. + clients []string } // newEnv starts a broker with this config: @@ -422,6 +428,7 @@ func newEnv(t *testing.T, opts envOptions) *env { logs: new(syncBuffer), srvLogs: new(syncBuffer), alerts: new(syncBuffer), + notices: ¬iceLog{}, broker: newTLSKey(t, ""), clients: map[string]tlsKey{"laptop": newTLSKey(t, ""), "ci": newTLSKey(t, "")}, userKeys: make(map[string]ssh.Signer), @@ -432,6 +439,9 @@ func newEnv(t *testing.T, opts envOptions) *env { }, runErr: make(chan error, 1), } + for _, name := range opts.clients { + e.clients[name] = newTLSKey(t, "") + } e.main.items[idGitHub] = e.values["github-token"] e.main.items[idNPM] = e.values["npm-token"] e.main.items[idBackup] = e.values["backup-token"] @@ -479,6 +489,9 @@ func newEnv(t *testing.T, opts envOptions) *env { }, KnownHosts: known, } + for _, name := range opts.clients { + e.cfg.Clients[name] = config.Client{Name: name, Pin: e.clients[name].pin} + } if opts.config != nil { opts.config(e.cfg) } @@ -505,11 +518,12 @@ func newEnv(t *testing.T, opts envOptions) *env { } e.core = core svc, err := New(Parts{ - Config: e.cfg, - Audit: e.audit, - Stores: map[string]store.Store{"main": e.main, "vault": e.vault}, - Keys: keys, - Logger: NewLogger(e.logs), + Config: e.cfg, + Audit: e.audit, + Stores: map[string]store.Store{"main": e.main, "vault": e.vault}, + Keys: keys, + Logger: NewLogger(e.logs), + Version: opts.version, }) if err != nil { t.Fatal(err) @@ -596,6 +610,13 @@ func requestContexts() (func(http.Handler) http.Handler, chan context.Context) { return wrap, ch } +// setNow changes the clock of the tries of the notices about versions. +func (s *Service) setNow(now func() time.Time) { + s.mu.Lock() + defer s.mu.Unlock() + s.now = now +} + // setMaxPerClient changes the cap of requests in progress per client. func (s *Service) setMaxPerClient(n int) { s.mu.Lock() @@ -609,7 +630,7 @@ func (e *env) startRun(ctx context.Context) { _, err := e.alerts.Write([]byte(msg + "\n")) return err } - go func() { e.runErr <- e.svc.Run(ctx, e.core, alert) }() + go func() { e.runErr <- e.svc.Run(ctx, e.core, alert, e.notices.send) }() if err := e.approver.WaitRunning(waitLimit); err != nil { e.t.Fatal(err) } @@ -896,3 +917,88 @@ func (e *env) rawRequest(name, text string) int { _ = resp.Body.Close() return resp.StatusCode } + +// noticeLog records the notices of the Service about versions (its +// NoticeFunc): each try, and the notices that went out. fail, if set, +// gives the result of each try; gate, if set, holds each try until it is +// closed, or until the context of the try ends. +type noticeLog struct { + mu sync.Mutex + tries []string + sent []string + fail func(msg string) error + gate chan struct{} + inflight int // the tries that run now + most int // the most tries that ran at the same time +} + +func (n *noticeLog) send(ctx context.Context, msg string) error { + n.mu.Lock() + n.tries = append(n.tries, msg) + n.inflight++ + n.most = max(n.most, n.inflight) + fail, gate := n.fail, n.gate + n.mu.Unlock() + defer func() { + n.mu.Lock() + n.inflight-- + n.mu.Unlock() + }() + if gate != nil { + select { + case <-gate: + case <-ctx.Done(): + return ctx.Err() + } + } + if fail != nil { + if err := fail(msg); err != nil { + return err + } + } + n.mu.Lock() + n.sent = append(n.sent, msg) + n.mu.Unlock() + return nil +} + +// set changes the notice log under its lock. +func (n *noticeLog) set(change func(*noticeLog)) { + n.mu.Lock() + defer n.mu.Unlock() + change(n) +} + +// wentOut returns the notices that went out so far. +func (n *noticeLog) wentOut() []string { + n.mu.Lock() + defer n.mu.Unlock() + return slices.Clone(n.sent) +} + +// tried returns every try so far, and the most tries that ran at the same +// time. +func (n *noticeLog) tried() (tries []string, most int) { + n.mu.Lock() + defer n.mu.Unlock() + return slices.Clone(n.tries), n.most +} + +// waitVersionTry waits until no try of the notice about the version of +// client runs. A request that starts a try marks it before its answer. +func (s *Service) waitVersionTry(t testing.TB, client string) { + t.Helper() + deadline := time.Now().Add(waitLimit) + for { + s.mu.Lock() + busy := s.versionNotes[client].sending + s.mu.Unlock() + if !busy { + return + } + if time.Now().After(deadline) { + t.Fatalf("a try of the notice about the version of %s still runs", client) + } + time.Sleep(time.Millisecond) + } +} diff --git a/internal/broker/panic_test.go b/internal/broker/panic_test.go index 7fa3fd6..c654849 100644 --- a/internal/broker/panic_test.go +++ b/internal/broker/panic_test.go @@ -59,7 +59,7 @@ func runWithPanicApprover(t *testing.T, e *env) (*panicApprover, chan any) { recovered := make(chan any, 1) go func() { defer func() { recovered <- recover() }() // as nodkey broker might - _ = e.svc.Run(ctx, core, nil) + _ = e.svc.Run(ctx, core, nil, nil) }() if err := e.approver.WaitRunning(waitLimit); err != nil { t.Fatal(err) diff --git a/internal/broker/service_test.go b/internal/broker/service_test.go index cccd035..0f5e03c 100644 --- a/internal/broker/service_test.go +++ b/internal/broker/service_test.go @@ -593,7 +593,7 @@ func TestNotReady(t *testing.T) { e.svc.Close() mustStatus(t, e.post(t.Context(), c, api.PathSecrets, secretsRequest("GH_TOKEN", "github-token")), http.StatusServiceUnavailable) - if err := e.svc.Run(t.Context(), e.core, nil); !errors.Is(err, errClosed) { + if err := e.svc.Run(t.Context(), e.core, nil, nil); !errors.Is(err, errClosed) { t.Errorf("Run after Close: %v", err) } if n := len(e.audit.all()); n != 0 { @@ -681,10 +681,10 @@ func TestRunTwice(t *testing.T) { e := newEnv(t, envOptions{}) ctx, cancel := context.WithCancel(t.Context()) defer cancel() - if err := e.svc.Run(ctx, e.core, nil); err == nil { + if err := e.svc.Run(ctx, e.core, nil, nil); err == nil { t.Error("a second Run: no error") } - if err := e.svc.Run(ctx, nil, nil); err == nil { + if err := e.svc.Run(ctx, nil, nil, nil); err == nil { t.Error("Run without a core: no error") } } diff --git a/internal/broker/version.go b/internal/broker/version.go new file mode 100644 index 0000000..e102f63 --- /dev/null +++ b/internal/broker/version.go @@ -0,0 +1,135 @@ +package broker + +import ( + "context" + "log/slog" + "net/http" + "time" + + "github.com/code6x/nodkey/internal/api" + "github.com/code6x/nodkey/internal/version" +) + +// versionRetry is the shortest time from a failed try of the notice about +// the version of a client to its next try. +const versionRetry = time.Minute + +// versionNote is where the broker is with the notice about the version of +// one client of the config, in this start. Service.mu guards it. +type versionNote struct { + logged bool // the log line about the version is written + sending bool // a try runs now + sent bool // a notice went out: no more tries in this start + failed time.Time // when the last try failed; zero for none +} + +// checkVersion compares the version that client claims in the header of +// its request (api.VersionHeader) with the broker's own version (DESIGN.md +// section 10). Anyone with the client's key can claim any version, so the +// claim decides nothing: it changes no answer, no check and no audit +// entry, and the request never waits for it. A broker that is a build +// from the source compares nothing, and a claim of a build from the +// source is not compared either. +// +// When the claim is missing, or names another release than the broker's, +// the broker writes one log line, once for each client in each start, and +// tries to send one notice (noteVersion). Only a notice that went out +// ends the tries for that client in this start, so a notice that a limit +// stopped, or that Telegram could not take, goes out later. A client has +// at most one try at a time, and the next try after a failed one comes +// with a request of that client at least versionRetry after the failure. +// There is no timer. However many versions a client claims, it gets one +// notice that went out (THREAT-MODEL.md T33), rarely more than one: a +// try can fail after Telegram took the message, and then the next try +// sends it again. The real bound is one try a minute for each client. +// +// A try needs the notice function of Run. So none starts before Run, +// after the approver stopped, or once Close has begun. +func (s *Service) checkVersion(client string, h http.Header) { + claimed := api.Version(h) + diff := version.Check(s.version, claimed) + if diff == version.NoDiff { + return + } + s.mu.Lock() + defer s.mu.Unlock() + n := s.versionNotes[client] + switch { + case n == nil || n.sent || n.sending || s.notice == nil || s.closed || s.coreStopped.Load(): + return + case !n.failed.IsZero() && s.now().Sub(n.failed) < versionRetry: + return + } + n.sending = true + first := !n.logged + n.logged = true + s.bg.Add(1) + go s.noteVersion(s.notice, n, client, claimed, diff, first) +} + +// noteVersion makes one try of the notice about the version that client +// claimed, with notice, and writes the log line about the version first +// when first is set. It runs in a goroutine of its own, so the request +// does not wait for it, and Close waits for it and cancels it. A try that +// fails is only logged, and is noted in n, for the next try. Once Close +// has begun or the approver stopped, no next try comes, and the log line +// says so. claimed is a release version, or "" for none. +func (s *Service) noteVersion(notice NoticeFunc, n *versionNote, client, claimed string, diff version.Diff, + first bool, +) { + defer s.bg.Done() + sent := false + defer func() { + s.mu.Lock() + defer s.mu.Unlock() + n.sending, n.sent = false, sent + if !sent { + n.failed = s.now() + } + }() + defer s.recoverGoroutine("sending a notice about a version") + line, text := versionTexts(client, claimed, s.version, diff) + if first { + attrs := []any{slog.String("client", client)} + if claimed != "" { + attrs = append(attrs, slog.String("client_version", claimed)) + } + attrs = append(attrs, slog.String("broker_version", s.version)) + s.log.Warn(line, attrs...) + } + ctx, cancel := context.WithTimeout(s.stopCtx, alertTimeout) + defer cancel() + if err := notice(ctx, text); err != nil { + next := "a request of the client a minute later or more tries again" + if s.stopping() || s.coreStopped.Load() { + next = "the broker is stopping, so no new try comes" + } + s.log.Warn("broker: the notice about the version of a client did not go out; "+next, + slog.String("client", client), slog.String("error", errorText(err))) + return + } + sent = true +} + +// versionTexts returns the log line and the text of the notice about the +// version that client claimed. claimed and own are release versions, so +// they hold only digits and dots; claimed is "" when the client sent no +// version. The client name comes from the config. A client that sends no +// version runs nodkey 0.1, which has no nodkey update: the text points to +// the steps of docs/setup.md instead. +func versionTexts(client, claimed, own string, diff version.Diff) (line, text string) { + switch diff { + case version.NotSent: + return "broker: a client sent no version of nodkey, so it may run an older nodkey; update nodkey there", + "The client " + client + " sent no version of nodkey, so it may run an older nodkey. The broker " + + "runs nodkey " + own + ". To update " + client + ", follow section 13 of docs/setup.md." + case version.OtherOlder: + return "broker: a client says that it runs an older nodkey than the broker; update nodkey there", + "The client " + client + " says that it runs nodkey " + claimed + ", which is older than nodkey " + + own + " on the broker. To update it, run there: sudo nodkey update" + default: // version.OtherNewer + return "broker: a client says that it runs a newer nodkey than the broker; update nodkey on the broker host", + "The client " + client + " says that it runs nodkey " + claimed + ", which is newer than nodkey " + + own + " on the broker. To update the broker, run as root on the broker host: nodkey update" + } +} diff --git a/internal/broker/version_test.go b/internal/broker/version_test.go new file mode 100644 index 0000000..b0c4efd --- /dev/null +++ b/internal/broker/version_test.go @@ -0,0 +1,694 @@ +package broker + +import ( + "bufio" + "bytes" + "context" + "crypto/tls" + "encoding/json" + "errors" + "io" + "net/http" + "net/textproto" + "slices" + "strconv" + "strings" + "sync" + "sync/atomic" + "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/transport" +) + +// answer is the final answer to a request of a version test. +type answer struct { + status int + header http.Header + body []byte +} + +// claim sends body as JSON to path with c, with each of claimed as a +// header api.VersionHeader, and returns the answer. +func (e *env) claim(c *http.Client, path string, body any, claimed ...string) answer { + e.t.Helper() + res, err := e.tryClaim(c, path, body, claimed...) + if err != nil { + e.t.Fatalf("the request failed: %v", err) + } + return res +} + +// tryClaim is claim that does not fail the test, so it may run in its own +// goroutine. +func (e *env) tryClaim(c *http.Client, path string, body any, claimed ...string) (answer, error) { + data, err := json.Marshal(body) + if err != nil { + return answer{}, err + } + req, err := http.NewRequestWithContext(e.t.Context(), http.MethodPost, "https://"+e.addr+path, bytes.NewReader(data)) + if err != nil { + return answer{}, err + } + req.Header.Set("Content-Type", api.ContentType) + for _, v := range claimed { + req.Header.Add(api.VersionHeader, v) + } + resp, err := c.Do(req) + if err != nil { + return answer{}, err + } + defer func() { _ = resp.Body.Close() }() + b, err := io.ReadAll(io.LimitReader(resp.Body, api.MaxAnswer+1)) + if err != nil { + return answer{}, err + } + return answer{status: resp.StatusCode, header: resp.Header, body: b}, nil +} + +// mustAnswer fails unless res has the status. +func mustAnswer(t testing.TB, res answer, status int) { + t.Helper() + if res.status != status { + t.Fatalf("status %d, want %d", res.status, status) + } +} + +// rawAnswers sends text on a new TLS connection as the client name, and +// returns the head of each answer that comes, up to the final one: the +// status line and the headers, as they are on the wire. +func (e *env) rawAnswers(name, text string) []string { + e.t.Helper() + d := tls.Dialer{Config: transport.ClientTLSConfig(e.clients[name].cert, e.broker.pin)} + conn, err := d.DialContext(e.t.Context(), "tcp", e.addr) + if err != nil { + e.t.Fatal(err) + } + defer func() { _ = conn.Close() }() + if _, err := io.WriteString(conn, text); err != nil { + e.t.Fatal(err) + } + _ = conn.SetReadDeadline(time.Now().Add(waitLimit)) + br := bufio.NewReader(conn) + var heads []string + for { + var head strings.Builder + for !strings.HasSuffix(head.String(), "\r\n\r\n") { + b, err := br.ReadByte() + if err != nil { + e.t.Fatalf("reading an answer: %v (so far %q)", err, head.String()) + } + head.WriteByte(b) + } + heads = append(heads, head.String()) + if !strings.HasPrefix(head.String(), "HTTP/1.1 1") { + return heads + } + } +} + +// headerOf parses the header of the head of an answer. +func headerOf(t testing.TB, head string) textproto.MIMEHeader { + t.Helper() + _, rest, _ := strings.Cut(head, "\r\n") + h, err := textproto.NewReader(bufio.NewReader(strings.NewReader(rest))).ReadMIMEHeader() + if err != nil { + t.Fatalf("the header of %q: %v", head, err) + } + return h +} + +// TestVersionOnEveryAnswer: the broker sends its version on every answer, +// whatever the status: 200, every error, 404 and 405, and the 102 +// Processing, which still has the short code. A broker without a valid +// version sends none. +func TestVersionOnEveryAnswer(t *testing.T) { + e := newEnv(t, envOptions{version: "0.2.0"}) + post := func(path, ctype string, body string) string { + return "POST " + path + " HTTP/1.1\r\nHost: broker\r\nContent-Type: " + ctype + "\r\nContent-Length: " + + strconv.Itoa(len(body)) + "\r\n\r\n" + body + } + notAllowed, err := json.Marshal(secretsRequest("B", "backup-token")) // a secret of ci + if err != nil { + t.Fatal(err) + } + for _, tt := range []struct { + name, text string + status string + }{ + {"200", post(api.PathKeys, api.ContentType, "{}"), "200 OK"}, + {"400", post(api.PathKeys, api.ContentType, `{"x":1}`), "400 Bad Request"}, + {"403", post(api.PathSecrets, api.ContentType, string(notAllowed)), "403 Forbidden"}, + {"404", post("/v1/nope", api.ContentType, "{}"), "404 Not Found"}, + {"405", "GET /v1/keys HTTP/1.1\r\nHost: broker\r\n\r\n", "405 Method Not Allowed"}, + {"413", "POST /v1/keys HTTP/1.1\r\nHost: broker\r\nContent-Type: application/json\r\nContent-Length: 3000000\r\n\r\n", + "413 Request Entity Too Large"}, + {"415", post(api.PathKeys, "text/plain", "{}"), "415 Unsupported Media Type"}, + } { + heads := e.rawAnswers("laptop", tt.text) + last := heads[len(heads)-1] + if !strings.HasPrefix(last, "HTTP/1.1 "+tt.status+"\r\n") { + t.Errorf("%s: the answer is %q", tt.name, last) + continue + } + if got := headerOf(t, last).Values(api.VersionHeader); !slices.Equal(got, []string{"0.2.0"}) { + t.Errorf("%s: the version header is %q, want 0.2.0", tt.name, got) + } + } + + // The 102 Processing has the version too, after the short code, and + // the client still finds the code. The final answer has it as well. + body, err := json.Marshal(secretsRequest("GH_TOKEN", "github-token")) + if err != nil { + t.Fatal(err) + } + e.approver.Auto(audit.AnswerDeny) + heads := e.rawAnswers("laptop", post(api.PathSecrets, api.ContentType, string(body))) + if len(heads) != 2 { + t.Fatalf("answers %q, want a 102 and a final one", heads) + } + if !strings.HasPrefix(heads[0], "HTTP/1.1 102 Processing\r\nNodkey-Short-Code: ") || + !strings.HasSuffix(heads[0], "\r\nNodkey-Version: 0.2.0\r\n\r\n") { + t.Errorf("the 102 answer is %q", heads[0]) + } + if code, ok := api.ShortCode(http.StatusProcessing, headerOf(t, heads[0])); !ok || + code != e.waitAsked(1)[0].Prompt.ShortCode { + t.Errorf("the short code of the 102 is %q, %v", code, ok) + } + if !strings.HasPrefix(heads[1], "HTTP/1.1 403 Forbidden\r\n") || + headerOf(t, heads[1]).Get(api.VersionHeader) != "0.2.0" { + t.Errorf("the final answer is %q", heads[1]) + } + + // 429: over the cap of requests in progress. + e.svc.setMaxPerClient(0) + res := e.claim(e.httpClient("laptop"), api.PathKeys, api.KeysRequest{}, "0.2.0") + if res.status != http.StatusTooManyRequests || res.header.Get(api.VersionHeader) != "0.2.0" { + t.Errorf("429: status %d, version %q", res.status, res.header.Values(api.VersionHeader)) + } + + // 503: before Run. + ne := newEnv(t, envOptions{version: "0.2.0", noRun: true}) + res = ne.claim(ne.httpClient("laptop"), api.PathKeys, api.KeysRequest{}) + if res.status != http.StatusServiceUnavailable || res.header.Get(api.VersionHeader) != "0.2.0" { + t.Errorf("503: status %d, version %q", res.status, res.header.Values(api.VersionHeader)) + } + + // A source build sends its version as it is; a broker without a valid + // version sends none. + for v, want := range map[string][]string{"dev": {"dev"}, "": nil, "0.2.0 beta": nil} { + ve := newEnv(t, envOptions{version: v}) + for _, path := range []string{api.PathKeys, "/v1/nope"} { + res := ve.claim(ve.httpClient("laptop"), path, api.KeysRequest{}) + if got := res.header.Values(api.VersionHeader); !slices.Equal(got, want) { + t.Errorf("a broker with the version %q, %s: the version header is %q, want %q", v, path, got, want) + } + } + } +} + +// waitNotices waits until the Service has stopped, so that every try of a +// notice about a version has ended, and returns the notices that went +// out. +func (e *env) waitNotices() []string { + e.t.Helper() + e.svc.Close() // it waits for the goroutines of the notices + return e.notices.wentOut() +} + +// versionLines returns the log lines about the versions of clients. +func (e *env) versionLines() []string { + var lines []string + for _, l := range strings.Split(e.logs.String(), "\n") { + if strings.Contains(l, "version") { + lines = append(lines, l) + } + } + return lines +} + +// TestVersionNoticeOncePerClient: a client that claims another release, +// or sends no valid version, gets one log line and one notice in each +// start of the broker, however many versions it claims. The text says +// which side is older and what to run. A value that is not a valid +// version is never logged or shown. +func TestVersionNoticeOncePerClient(t *testing.T) { + e := newEnv(t, envOptions{version: "0.2.0"}) + laptop, ci := e.httpClient("laptop"), e.httpClient("ci") + claims := [][]string{{"0.1.0"}, {"0.1.1"}, {"0.3.0"}, nil, {"evil"}, {"0.1.0", "0.3.0"}, {"dev"}, {"0.2.0"}} + for _, c := range claims { + mustAnswer(t, e.claim(laptop, api.PathKeys, api.KeysRequest{}, c...), http.StatusOK) + } + for _, c := range [][]string{{"0.1.0 evil"}, {"0.1.0"}, {"0.4.0"}, nil} { + mustAnswer(t, e.claim(ci, api.PathKeys, api.KeysRequest{}, c...), http.StatusOK) + } + notices := e.waitNotices() + want := []string{ + "The client laptop says that it runs nodkey 0.1.0, which is older than nodkey 0.2.0 on the broker. " + + "To update it, run there: sudo nodkey update", + "The client ci sent no version of nodkey, so it may run an older nodkey. The broker runs nodkey 0.2.0. " + + "To update ci, follow section 13 of docs/setup.md.", + } + slices.Sort(notices) + slices.Sort(want) + if !slices.Equal(notices, want) { + t.Errorf("notices:\n%s\nwant:\n%s", strings.Join(notices, "\n"), strings.Join(want, "\n")) + } + lines := e.versionLines() + if len(lines) != 2 || + !slices.ContainsFunc(lines, func(l string) bool { + return strings.Contains(l, `msg="broker: a client says that it runs an older nodkey than the broker; `+ + `update nodkey there" client=laptop client_version=0.1.0 broker_version=0.2.0`) + }) || + !slices.ContainsFunc(lines, func(l string) bool { + return strings.Contains(l, `msg="broker: a client sent no version of nodkey, so it may run an older `+ + `nodkey; update nodkey there" client=ci broker_version=0.2.0`) + }) { + t.Errorf("the log lines about versions:\n%s", strings.Join(lines, "\n")) + } + if strings.Contains(e.logs.String(), "evil") || strings.Contains(strings.Join(notices, ""), "evil") { + t.Error("a value that is not a version is in the log or a notice") + } + if len(e.audit.all()) != 0 || len(e.approver.Asked()) != 0 || len(e.approver.Notices()) != 0 { + t.Error("a version made an audit entry, a message or a notice of the approver") + } +} + +// TestVersionNoticeNewerClient: a client that claims a newer release than +// the broker's gets a notice that says to update the broker. +func TestVersionNoticeNewerClient(t *testing.T) { + e := newEnv(t, envOptions{version: "0.2.0"}) + mustAnswer(t, e.claim(e.httpClient("laptop"), "/v1/nope", api.KeysRequest{}, "0.10.0"), http.StatusNotFound) + want := []string{"The client laptop says that it runs nodkey 0.10.0, which is newer than nodkey 0.2.0 on the " + + "broker. To update the broker, run as root on the broker host: nodkey update"} + if got := e.waitNotices(); !slices.Equal(got, want) { + t.Errorf("notices %q, want %q", got, want) + } + if lines := e.versionLines(); len(lines) != 1 || !strings.Contains(lines[0], + `a client says that it runs a newer nodkey than the broker; update nodkey on the broker host" `+ + `client=laptop client_version=0.10.0 broker_version=0.2.0`) { + t.Errorf("the log lines about versions:\n%s", strings.Join(lines, "\n")) + } +} + +// TestNoVersionNotice: no log line and no notice for the same release, +// for a client or a broker that is a build from the source, for a broker +// without a valid version, and for a client whose TLS key no [client.*] +// section pins. +func TestNoVersionNotice(t *testing.T) { + for _, tt := range []struct { + broker string + claimed [][]string + }{ + {"0.2.0", [][]string{{"0.2.0"}, {"dev"}, {"1a2b3c4"}, {"0.3.0-rc.1"}, {"v0.1.0"}}}, + {"dev", [][]string{{"0.1.0"}, nil, {"x"}, {"dev"}}}, + {"", [][]string{{"0.1.0"}, nil}}, + {"0.2.0 beta", [][]string{{"0.1.0"}, nil}}, + } { + e := newEnv(t, envOptions{version: tt.broker}) + for _, c := range tt.claimed { + for _, client := range []string{"laptop", "ci"} { + e.claim(e.httpClient(client), api.PathKeys, api.KeysRequest{}, c...) + } + } + if got := e.waitNotices(); len(got) != 0 { + t.Errorf("broker %q: notices %q, want none", tt.broker, got) + } + if lines := e.versionLines(); len(lines) != 0 { + t.Errorf("broker %q: log lines about versions:\n%s", tt.broker, strings.Join(lines, "\n")) + } + } + + // A key that no [client.*] section pins: the TLS layer refuses it, so + // the claim never reaches the broker. + e := newEnv(t, envOptions{version: "0.2.0"}) + stranger, err := transport.NewClient(e.addr, newTLSKey(t, "").cert, e.broker.pin) + if err != nil { + t.Fatal(err) + } + req, err := http.NewRequestWithContext(t.Context(), http.MethodPost, "https://"+e.addr+api.PathKeys, + strings.NewReader("{}")) + if err != nil { + t.Fatal(err) + } + req.Header.Set("Content-Type", api.ContentType) + req.Header.Set(api.VersionHeader, "0.1.0") + if resp, err := stranger.Do(req); err == nil { + _ = resp.Body.Close() + t.Errorf("a client with an unknown key got an answer: %d", resp.StatusCode) + } + if got := e.waitNotices(); len(got) != 0 { + t.Errorf("a client with an unknown key: notices %q, want none", got) + } + if lines := e.versionLines(); len(lines) != 0 { + t.Errorf("a client with an unknown key: log lines about versions:\n%s", strings.Join(lines, "\n")) + } +} + +// TestVersionClaimChangesNothing: the claimed version is a claim. With any +// value, or none, a request gets the same answer and the same audit +// entries: for the list of keys, for a secret that the client may not +// use, for a wrong path, and for a secret that the person approves. +func TestVersionClaimChangesNothing(t *testing.T) { + e := newEnv(t, envOptions{version: "0.2.0"}) + e.approver.Auto(audit.AnswerApproveOnce) + c := e.httpClient("laptop") + claims := [][]string{nil, {"0.2.0"}, {"0.1.0"}, {"0.3.0"}, {"dev"}, {"bad value"}, {"0.1.0", "0.2.0"}, + {strings.Repeat("9", 4000)}} + type outcome struct { + statuses []int + bodies [][]byte + entries string + } + var first *outcome + for i, claimed := range claims { + e.clock.Advance(time.Minute) // so the limit of 6 messages a minute does not hold a request + before := len(e.audit.all()) + var o outcome + for _, rq := range []struct { + path string + body any + }{ + {api.PathKeys, api.KeysRequest{}}, + {api.PathSecrets, secretsRequest("B", "backup-token")}, + {"/v1/nope", api.KeysRequest{}}, + {api.PathSecrets, secretsRequest("GH_TOKEN", "github-token")}, + } { + res := e.claim(c, rq.path, rq.body, claimed...) + o.statuses = append(o.statuses, res.status) + o.bodies = append(o.bodies, res.body) + } + events := e.audit.all()[before:] + names := eventNames(events) + // The request IDs differ; the entries must not. + o.entries = names + if first == nil { + first = &o + if !slices.Equal(o.statuses, []int{200, 403, 404, 200}) || + names != "request denied:not_allowed request decision:approve_once secret_sent" { + t.Fatalf("without a version: statuses %v, entries %s", o.statuses, names) + } + continue + } + if !slices.Equal(o.statuses, first.statuses) || o.entries != first.entries { + t.Errorf("claim %d: statuses %v and entries %s; without a version %v and %s", i, o.statuses, o.entries, + first.statuses, first.entries) + } + for j := range o.bodies { + if !bytes.Equal(o.bodies[j], first.bodies[j]) { + // The bodies are not printed: one holds a value. + t.Errorf("claim %d: answer %d differs (%d bytes, without a version %d)", i, j, len(o.bodies[j]), + len(first.bodies[j])) + } + } + } + e.main.checkCleared(t) +} + +// TestVersionNoticeKeepsNeverAskNotices: a notice about a version does not +// take from the limit of the notices of "never ask" uses: right after it, +// the client still gets all of its approval.MaxNoticesPerMinute uses with +// a notice, and then the limit denies, as without it. +func TestVersionNoticeKeepsNeverAskNotices(t *testing.T) { + e := newEnv(t, envOptions{version: "0.2.0"}) + ci := e.httpClient("ci") + for i := range approval.MaxNoticesPerMinute { + res := e.claim(ci, api.PathSecrets, secretsRequest("B", "backup-token"), "0.1."+strconv.Itoa(i)) + if res.status != http.StatusOK { + t.Fatalf("\"never ask\" use %d: status %d", i+1, res.status) + } + } + res := e.claim(ci, api.PathSecrets, secretsRequest("B", "backup-token"), "0.1.0") + if res.status != http.StatusTooManyRequests { + t.Errorf("one use over the limit: status %d, want 429", res.status) + } + if n := len(e.approver.Notices()); n != approval.MaxNoticesPerMinute { + t.Errorf("%d notices of \"never ask\" uses, want %d", n, approval.MaxNoticesPerMinute) + } + if got := e.waitNotices(); len(got) != 1 { + t.Errorf("%d notices about versions, want 1: %q", len(got), got) + } +} + +// failWith returns a fail function for the notice log that fails each +// try while failing is set. +func failWith(failing *atomic.Bool) func(string) error { + return func(string) error { + if failing.Load() { + return errors.New("telegram: sendMessage: Telegram asks to wait (429)") + } + return nil + } +} + +// TestVersionNoticeRetry: a try that fails changes nothing for the +// request: its answer comes at once, as without it. The failure is +// logged, once for each try. The next try comes only with a request of +// the client, and only at least a minute after the failure: an earlier +// request, or the time alone, starts none. Once a notice went out, no +// more tries come in this start. The log line about the version comes +// once. +func TestVersionNoticeRetry(t *testing.T) { + e := newEnv(t, envOptions{version: "0.2.0"}) + e.svc.setNow(e.clock.Now) + var failing atomic.Bool + failing.Store(true) + e.notices.set(func(n *noticeLog) { n.fail = failWith(&failing) }) + laptop := e.httpClient("laptop") + request := func() { + t.Helper() + mustAnswer(t, e.claim(laptop, api.PathKeys, api.KeysRequest{}, "0.1.0"), http.StatusOK) + e.svc.waitVersionTry(t, "laptop") + } + tries := func(want int) { + t.Helper() + if got, _ := e.notices.tried(); len(got) != want { + t.Fatalf("%d tries, want %d", len(got), want) + } + } + request() // the first try fails + tries(1) + request() + request() + e.clock.Advance(versionRetry - time.Second) + request() + tries(1) + e.clock.Advance(time.Second) // a minute after the failure: still no try without a request + tries(1) + request() // the second try, which fails too + tries(2) + e.clock.Advance(versionRetry) + failing.Store(false) + request() // the third try goes out + tries(3) + e.clock.Advance(10 * versionRetry) + request() + if got := e.waitNotices(); len(got) != 1 { + t.Errorf("%d notices went out, want 1", len(got)) + } + tries(3) + logs := e.logs.String() + if n := strings.Count(logs, "a client says that it runs an older nodkey"); n != 1 { + t.Errorf("%d log lines about the version, want 1:\n%s", n, logs) + } + if n := strings.Count(logs, `the notice about the version of a client did not go out; a request of the client `+ + `a minute later or more tries again" client=laptop error="telegram: sendMessage: Telegram asks to wait (429)"`); n != 2 || + strings.Contains(logs, "no new try") { + t.Errorf("%d log lines about failed tries, want 2:\n%s", n, logs) + } +} + +// TestVersionNoticeHangs: a try that hangs does not hold the requests, +// and Close ends it. Its log line says that no new try comes. +func TestVersionNoticeHangs(t *testing.T) { + e := newEnv(t, envOptions{version: "0.2.0"}) + e.notices.set(func(n *noticeLog) { n.gate = make(chan struct{}) }) // never opens + laptop := e.httpClient("laptop") + start := time.Now() + for range 3 { + mustAnswer(t, e.claim(laptop, api.PathKeys, api.KeysRequest{}, "0.1.0"), http.StatusOK) + } + if d := time.Since(start); d > waitLimit/2 { + t.Errorf("the requests took %s while the notice hung", d) + } + waitLog(t, e.logs, "a client says that it runs an older nodkey") + closed := make(chan struct{}) + go func() { + e.svc.Close() + close(closed) + }() + select { + case <-closed: + case <-time.After(waitLimit): + t.Fatal("Close waits for a notice that hangs") + } + waitLog(t, e.logs, "the notice about the version of a client did not go out; the broker is stopping, so no "+ + "new try comes") + if strings.Contains(e.logs.String(), "tries again") { + t.Errorf("the log promises a new try while the broker stops:\n%s", e.logs.String()) + } + if got, _ := e.notices.tried(); len(got) != 1 { + t.Errorf("%d tries of the notice, want 1", len(got)) + } +} + +// TestVersionNoticeFailsAfterTheApproverStopped: a try that fails after +// the approver stopped says that no new try comes, and none comes. +func TestVersionNoticeFailsAfterTheApproverStopped(t *testing.T) { + e := newEnv(t, envOptions{version: "0.2.0", noRun: true}) + e.svc.setNow(e.clock.Now) + gate := make(chan struct{}) + var failing atomic.Bool + failing.Store(true) + e.notices.set(func(n *noticeLog) { n.gate, n.fail = gate, failWith(&failing) }) + ctx, cancel := context.WithCancel(t.Context()) + e.startRun(ctx) + laptop := e.httpClient("laptop") + mustAnswer(t, e.claim(laptop, api.PathKeys, api.KeysRequest{}, "0.1.0"), http.StatusOK) + cancel() // the approver stops while the try waits at the gate + select { + case <-e.runErr: + case <-time.After(waitLimit): + t.Fatal("Run did not return") + } + close(gate) // the try fails + e.svc.waitVersionTry(t, "laptop") + logs := waitLog(t, e.logs, "the notice about the version of a client did not go out; the broker is stopping, "+ + "so no new try comes") + if strings.Contains(logs, "tries again") { + t.Errorf("the log promises a new try after the approver stopped:\n%s", logs) + } + e.clock.Advance(versionRetry) + e.claim(laptop, api.PathKeys, api.KeysRequest{}, "0.1.0") // 503 now + if got, _ := e.notices.tried(); len(got) != 1 { + t.Errorf("%d tries, want 1", len(got)) + } +} + +// TestVersionNoticeOneTryAtATime: many requests of one client at the same +// time, while a try runs, start no other try. +func TestVersionNoticeOneTryAtATime(t *testing.T) { + e := newEnv(t, envOptions{version: "0.2.0"}) + e.svc.setNow(e.clock.Now) + gate := make(chan struct{}) + var failing atomic.Bool + failing.Store(true) + e.notices.set(func(n *noticeLog) { n.gate, n.fail = gate, failWith(&failing) }) + laptop := e.httpClient("laptop") + burst := func() { + t.Helper() + var wg sync.WaitGroup + statuses := make([]int, 20) + for i := range statuses { + wg.Go(func() { + res, err := e.tryClaim(laptop, api.PathKeys, api.KeysRequest{}, "0.1.0") + if err == nil { + statuses[i] = res.status + } + }) + } + wg.Wait() + for i, status := range statuses { + if status != http.StatusOK { + t.Fatalf("request %d: status %d", i, status) + } + } + } + burst() // the first try waits at the gate + if tries, most := e.notices.tried(); len(tries) != 1 || most != 1 { + t.Fatalf("%d tries, %d at the same time; want 1 and 1", len(tries), most) + } + close(gate) // the try fails + e.svc.waitVersionTry(t, "laptop") + e.clock.Advance(versionRetry) + burst() + e.svc.waitVersionTry(t, "laptop") + if tries, most := e.notices.tried(); len(tries) != 2 || most != 1 { + t.Errorf("%d tries, %d at the same time; want 2 and 1", len(tries), most) + } +} + +// TestVersionNoticeForEveryClient: with more old clients in one minute +// than the limit of the notices lets through, each client still gets its +// notice in the end. The limit here stands for that of Inform: 3 tries in +// any 60 seconds. So 3 clients cannot use up the places and hide another +// old client. +func TestVersionNoticeForEveryClient(t *testing.T) { + more := []string{"agent-1", "agent-2", "agent-3"} + e := newEnv(t, envOptions{version: "0.2.0", clients: more}) + e.svc.setNow(e.clock.Now) + var mu sync.Mutex + var sentAt []time.Time + e.notices.set(func(n *noticeLog) { + n.fail = func(string) error { + mu.Lock() + defer mu.Unlock() + now := e.clock.Now() + sentAt = slices.DeleteFunc(sentAt, func(at time.Time) bool { return now.Sub(at) >= time.Minute }) + if len(sentAt) >= 3 { + return errors.New("telegram: too many notices of the broker in the last minute") + } + sentAt = append(sentAt, now) + return nil + } + }) + clients := append([]string{"laptop", "ci"}, more...) + round := func() { + t.Helper() + for _, name := range clients { + mustAnswer(t, e.claim(e.httpClient(name), api.PathKeys, api.KeysRequest{}, "0.1.0"), http.StatusOK) + e.svc.waitVersionTry(t, name) + } + } + round() // 3 notices go out, 2 meet the limit + if got := e.notices.wentOut(); len(got) != 3 { + t.Fatalf("%d notices went out in the first minute, want 3", len(got)) + } + round() // within the minute: no try + e.clock.Advance(versionRetry) + round() // the other 2 go out + got := e.waitNotices() + var want []string + for _, name := range clients { + want = append(want, "The client "+name+" says that it runs nodkey 0.1.0, which is older than nodkey 0.2.0 "+ + "on the broker. To update it, run there: sudo nodkey update") + } + slices.Sort(got) + slices.Sort(want) + if !slices.Equal(got, want) { + t.Errorf("notices:\n%s\nwant one for each client:\n%s", strings.Join(got, "\n"), strings.Join(want, "\n")) + } + if tries, _ := e.notices.tried(); len(tries) != len(clients)+2 { + t.Errorf("%d tries, want %d", len(tries), len(clients)+2) + } +} + +// TestVersionNoticeNeedsRun: before Run, and after the approver stopped, +// no notice goes out, and none is lost: a later request of the client, +// while Run runs, gets it. +func TestVersionNoticeNeedsRun(t *testing.T) { + e := newEnv(t, envOptions{version: "0.2.0", noRun: true}) + laptop := e.httpClient("laptop") + res := e.claim(laptop, api.PathKeys, api.KeysRequest{}, "0.1.0") + if res.status != http.StatusServiceUnavailable { + t.Fatalf("before Run: status %d", res.status) + } + ctx, cancel := context.WithCancel(t.Context()) + e.startRun(ctx) + mustAnswer(t, e.claim(laptop, api.PathKeys, api.KeysRequest{}, "0.1.0"), http.StatusOK) + waitLog(t, e.logs, "a client says that it runs an older nodkey") + cancel() + select { + case <-e.runErr: + case <-time.After(waitLimit): + t.Fatal("Run did not return") + } + // The approver stopped: no notice for ci. + e.claim(e.httpClient("ci"), api.PathKeys, api.KeysRequest{}, "0.1.0") + if got := e.waitNotices(); len(got) != 1 || !strings.Contains(got[0], "The client laptop ") { + t.Errorf("notices %q, want one for laptop", got) + } +} diff --git a/internal/client/client.go b/internal/client/client.go index 99155e8..200456a 100644 --- a/internal/client/client.go +++ b/internal/client/client.go @@ -10,7 +10,9 @@ // code; Post passes on only that code. Only a final 200 whose body passes // api.ReadAnswer counts. Every other answer, and every error, is an *Error // with a fixed text: it never holds an answer body, a header, a value, or -// other text that the other side of the connection chose. +// other text that the other side of the connection chose. Each request +// has the client's version in a header, and each answer the broker's; +// when they differ, the client prints one warning (version.go). package client import ( @@ -72,6 +74,9 @@ type Broker struct { addr string client *http.Client timeout time.Duration // RequestTimeout; tests make it short + // versions sends this client's version, and warns when the broker + // runs another (warnVersions). + versions versions } // Load reads the client config at path with config.LoadClient, and the @@ -220,6 +225,10 @@ func (tr *trace) isConnected() bool { // redirect; a 200 whose body api.ReadAnswer refuses; more than a few // informational answers; no answer within the deadline; and a failed // connection. +// +// The request has this client's version in its header, and Post compares +// the version in the header of the final answer with it, whatever its +// status, before it returns (warnVersions). func Post[T any, P interface { *T json.Unmarshaler @@ -243,12 +252,16 @@ func Post[T any, P interface { req.GetBody = nil req.Close = true req.Header.Set("Content-Type", api.ContentType) + b.versions.setVersion(req.Header) defer b.client.CloseIdleConnections() resp, err := b.client.Do(req) if err != nil { return nil, b.requestError(ctx, reqCtx, err, tr.isConnected()) } defer func() { _ = resp.Body.Close() }() + // Only the pinned broker can send this answer: the TLS handshake + // checked its key. + b.versions.check(resp.Header) if resp.StatusCode != http.StatusOK { // The body of an error answer is never read: it is not needed, and // it must never be shown. diff --git a/internal/client/files_linux_test.go b/internal/client/files_linux_test.go index 7385a0c..5ca8d81 100644 --- a/internal/client/files_linux_test.go +++ b/internal/client/files_linux_test.go @@ -66,7 +66,7 @@ func TestRunWithTheRealFiles(t *testing.T) { // not parallel: it sets HOME } }) var stderr strings.Builder - code := Run([]string{"--secret", "GH_TOKEN=github-token", "--", self(t)}, &stderr) + code := Run([]string{"--secret", "GH_TOKEN=github-token", "--", self(t)}, &stderr, "dev") if d := dumpable(); d != 0 { t.Errorf("after Run asked the broker, the process is dumpable (%d)", d) } @@ -81,7 +81,7 @@ func TestRunWithTheRealFiles(t *testing.T) { // not parallel: it sets HOME t.Fatal(err) } stderr.Reset() - code = Run([]string{"--secret", "GH_TOKEN=github-token", "--", self(t)}, &stderr) + code = Run([]string{"--secret", "GH_TOKEN=github-token", "--", self(t)}, &stderr, "dev") if last := lastLine(stderr.String()); code != exitUnavailable || !strings.Contains(last, "tls_key") || !strings.Contains(last, "group has access") { t.Errorf("with a key file that its group can read: exit code %d, stderr %q", code, safe(stderr.String())) diff --git a/internal/client/helpers_test.go b/internal/client/helpers_test.go index 01cecec..004e438 100644 --- a/internal/client/helpers_test.go +++ b/internal/client/helpers_test.go @@ -323,6 +323,13 @@ type testBroker struct { } func newTestBroker(t *testing.T) *testBroker { + t.Helper() + return newTestBrokerWith(t, "") +} + +// newTestBrokerWith is newTestBroker for a broker whose version is +// version ("" for none). +func newTestBrokerWith(t *testing.T, version string) *testBroker { t.Helper() tb := &testBroker{ key: newTLSKey(t), @@ -353,14 +360,14 @@ func newTestBroker(t *testing.T) *testBroker { t.Fatal(err) } svc, err := broker.New(broker.Parts{Config: cfg, Audit: tb.audit, - Stores: map[string]store.Store{"main": tb.store}}) + Stores: map[string]store.Store{"main": tb.store}, Version: version}) if err != nil { t.Fatal(err) } tb.addr = serve(t, tb.key, cfg.Clients, svc) runCtx, stopRun := context.WithCancel(context.Background()) runErr := make(chan error, 1) - go func() { runErr <- svc.Run(runCtx, core, nil) }() + go func() { runErr <- svc.Run(runCtx, core, nil, nil) }() t.Cleanup(func() { stopRun() select { diff --git a/internal/client/request_test.go b/internal/client/request_test.go index 475234c..dfb2368 100644 --- a/internal/client/request_test.go +++ b/internal/client/request_test.go @@ -333,7 +333,7 @@ func TestRunReadsTheClientConfig(t *testing.T) { // not parallel: it sets HOME t.Fatalf("ConfigPath = %q, %v", path, err) } var stderr strings.Builder - code := Run([]string{"--secret", "GH_TOKEN=github-token", "--", self(t)}, &stderr) + code := Run([]string{"--secret", "GH_TOKEN=github-token", "--", self(t)}, &stderr, "dev") if last := lastLine(stderr.String()); code != exitUnavailable || !strings.HasPrefix(last, "nodkey: config: ") || !strings.HasSuffix(last, "(exit 69)") { t.Errorf("without a config: exit code %d, stderr %q", code, safe(stderr.String())) diff --git a/internal/client/run.go b/internal/client/run.go index a0d51d8..a98ad49 100644 --- a/internal/client/run.go +++ b/internal/client/run.go @@ -62,10 +62,12 @@ const tooLong = "[hidden: too long]" const reasonVar = "NODKEY_REASON" // Run runs nodkey run with args, the arguments after "run", and writes its -// own messages to stderr. When the person approves, the command replaces -// nodkey (execve), with the values in its environment, and Run does not -// return. Otherwise Run returns the exit code of DESIGN.md section 15, and -// the last line on stderr is "nodkey: (exit )". +// own messages to stderr. version is the version of nodkey: it goes to the +// broker, and Run prints a warning when the broker runs another version. +// When the person approves, the command replaces nodkey (execve), with the +// values in its environment, and Run does not return. Otherwise Run +// returns the exit code of DESIGN.md section 15, and the last line on +// stderr is "nodkey: (exit )". // // Run never ignores SIGPIPE: an ignored signal stays ignored after execve, // so the command would run with it ignored. Run catches SIGINT and SIGTERM @@ -75,9 +77,9 @@ const reasonVar = "NODKEY_REASON" // shell sees it, as for a program that does not catch it. Right before the // command starts, Run stops catching them, so they end nodkey as they end // any Go program. -func Run(args []string, stderr io.Writer) int { +func Run(args []string, stderr io.Writer, version string) int { r := runner{load: Load, configPath: ConfigPath, environ: os.Environ, exec: syscall.Exec, - notDumpable: notDumpable} + notDumpable: notDumpable, version: version} return r.main(args, stderr) } @@ -97,6 +99,8 @@ type runner struct { // values: notDumpable. Tests in the test process replace it, so the // test process stays as it is. notDumpable func() error + // version is the version of nodkey (warnVersions); "" sends none. + version string } // main is Run with the parts of r. @@ -162,6 +166,7 @@ func (r runner) run(ctx context.Context, args []string, w io.Writer, beforeExec if err != nil { return fail(stderr, exitUnavailable, err.Error()) } + b.warnVersions(r.version, func(text string) { warnLine(stderr, text) }) // From here on, nodkey may hold values. So it writes no core dump, and // other processes of the user cannot read its memory. execve makes the // command dumpable again (DESIGN.md section 16). @@ -229,6 +234,12 @@ func fail(w io.Writer, code int, reason string) int { return code } +// warnLine writes "nodkey: " and text as one line, such as a warning about +// the version of the broker. It comes before the last line. +func warnLine(w io.Writer, text string) { + _, _ = io.WriteString(w, "nodkey: "+oneLine(text)+"\n") +} + // oneLine replaces control characters and line breaks in s with spaces. func oneLine(s string) string { return strings.Map(func(r rune) rune { diff --git a/internal/client/sshagent.go b/internal/client/sshagent.go index 114eecc..f4cea58 100644 --- a/internal/client/sshagent.go +++ b/internal/client/sshagent.go @@ -125,13 +125,15 @@ be made). ` // SSHAgent runs nodkey ssh-agent with args, the arguments after -// "ssh-agent", and writes its lines to stderr. It serves the agent until +// "ssh-agent", and writes its lines to stderr. version is the version of +// nodkey, as for Run; the agent prints one warning for each version of +// the broker that differs from it. It serves the agent until // SIGINT or SIGTERM. Then it removes the socket, writes its last line, // such as "nodkey: stopped by SIGTERM (exit 143)", and ends nodkey with // the same signal, as nodkey run does. Otherwise it returns the exit code // of DESIGN.md section 15, and its last line is "nodkey: (exit // )". -func SSHAgent(args []string, stderr io.Writer) int { +func SSHAgent(args []string, stderr io.Writer, version string) int { r := agentRunner{ load: Load, configPath: ConfigPath, @@ -139,6 +141,7 @@ func SSHAgent(args []string, stderr io.Writer) int { checkDirs: config.CheckParentDirs, proc: procFS{root: procDir}, peer: peerCredentials, + version: version, } return r.main(args, stderr) } @@ -163,6 +166,8 @@ type agentRunner struct { // tune, if set, changes the agent before it serves, such as its // limits. Tests use it. tune func(*sshAgent) + // version is the version of nodkey (warnVersions); "" sends none. + version string } // main is SSHAgent with the parts of r. @@ -213,6 +218,8 @@ func (r agentRunner) run(ctx context.Context, args []string, w io.Writer) int { if err != nil { return fail(log, exitUnavailable, err.Error()) } + // At most one line for each version of the broker, so no budget. + b.warnVersions(r.version, func(text string) { log.say("%s", text) }) self, err := currentUID() if err != nil { return fail(log, exitAgentFailed, err.Error()) diff --git a/internal/client/sshagent_helpers_test.go b/internal/client/sshagent_helpers_test.go index be24a71..04a3633 100644 --- a/internal/client/sshagent_helpers_test.go +++ b/internal/client/sshagent_helpers_test.go @@ -158,7 +158,7 @@ func newSSHBroker(t *testing.T, hosts map[string]ssh.PublicKey) *sshBroker { sb.addr = serve(t, sb.key, cfg.Clients, svc) runCtx, stopRun := context.WithCancel(context.Background()) runErr := make(chan error, 1) - go func() { runErr <- svc.Run(runCtx, core, nil) }() + go func() { runErr <- svc.Run(runCtx, core, nil, nil) }() t.Cleanup(func() { stopRun() select { diff --git a/internal/client/sshgrant.go b/internal/client/sshgrant.go index c84017d..12d3739 100644 --- a/internal/client/sshgrant.go +++ b/internal/client/sshgrant.go @@ -58,7 +58,8 @@ 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 +// 13 and 15). version is the version of nodkey, as for Run. 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. // @@ -66,8 +67,8 @@ const grantClock = "15:04" // 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} +func SSHGrant(args []string, stderr io.Writer, version string) int { + g := granter{load: Load, configPath: ConfigPath, environ: os.Environ, now: time.Now, version: version} return g.main(args, stderr) } @@ -82,6 +83,8 @@ type granter struct { // 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 + // version is the version of nodkey (warnVersions); "" sends none. + version string } // main is SSHGrant with the parts of g. @@ -131,6 +134,7 @@ func (g granter) run(ctx context.Context, args []string, w io.Writer) int { if err != nil { return fail(stderr, exitUnavailable, err.Error()) } + b.warnVersions(g.version, func(text string) { warnLine(stderr, text) }) // 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, diff --git a/internal/client/sshgrant_helpers_test.go b/internal/client/sshgrant_helpers_test.go index 46b2ac3..84fb622 100644 --- a/internal/client/sshgrant_helpers_test.go +++ b/internal/client/sshgrant_helpers_test.go @@ -154,7 +154,7 @@ func newGrantBrokerWith(t *testing.T, opts grantBrokerOptions) *grantBroker { 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) }() + go func() { runErr <- svc.Run(runCtx, core, nil, nil) }() t.Cleanup(func() { stopRun() select { diff --git a/internal/client/version.go b/internal/client/version.go new file mode 100644 index 0000000..fccdc89 --- /dev/null +++ b/internal/client/version.go @@ -0,0 +1,93 @@ +package client + +import ( + "net/http" + "sync" + + "github.com/code6x/nodkey/internal/api" + "github.com/code6x/nodkey/internal/version" +) + +// maxVersionWarnings is the most versions of the broker that one Broker +// warns about. The pinned broker sends its version, so a new one comes +// only with an update of the broker. More than this many means that it is +// broken, and the client stops warning. +const maxVersionWarnings = 16 + +// versions sends this client's version in each request, and warns when +// the broker runs another version (DESIGN.md section 10). Its zero value +// sends no version and warns about nothing. +type versions struct { + mu sync.Mutex + own string // this client's version, when version.Valid + warn func(string) // gets one line of text; nil warns about nothing + seen map[string]bool // the versions of the broker that warn got a line about; "" for none +} + +// warnVersions makes b send own, the version of this client, in the +// header api.VersionHeader of each request, when own is version.Valid. +// When own is a release, and the broker runs another release or sends no +// version, warn gets one line of text, without "nodkey: " and without a +// line break. It gets at most one line for each version that the broker +// sends, so after an update of the broker it can get one more. nodkey +// run and nodkey ssh-grant send one request, so they print at most one +// such line. +func (b *Broker) warnVersions(own string, warn func(text string)) { + b.versions.mu.Lock() + defer b.versions.mu.Unlock() + b.versions.own, b.versions.warn = "", warn + if version.Valid(own) { + b.versions.own = own + } +} + +// setVersion sets the version header of a request. +func (v *versions) setVersion(h http.Header) { + v.mu.Lock() + defer v.mu.Unlock() + if v.own != "" { + h.Set(api.VersionHeader, v.own) + } +} + +// check compares the version in h, the header of a final answer of the +// broker, with this client's version, and gives warn a line about a +// difference. The line holds only fixed text and release versions, which +// hold only digits and dots: never a value from the broker that is not a +// release version. +func (v *versions) check(h http.Header) { + broker := api.Version(h) + v.mu.Lock() + warn := v.warn + text := versionWarning(v.own, broker, version.Check(v.own, broker)) + if warn == nil || text == "" || v.seen[broker] || len(v.seen) >= maxVersionWarnings { + v.mu.Unlock() + return + } + if v.seen == nil { + v.seen = make(map[string]bool) + } + v.seen[broker] = true + v.mu.Unlock() + warn(text) +} + +// versionWarning returns the warning about the version of the broker, or +// "" for none. own and broker are the versions of the client and the +// broker, and diff is how they compare (version.Check). A broker that +// sends no version runs nodkey 0.1, which has no nodkey update: the +// warning points to the steps of docs/setup.md instead. +func versionWarning(own, broker string, diff version.Diff) string { + switch diff { + case version.NotSent: + return "warning: the broker runs an older nodkey that does not send its version, and this machine " + + "runs nodkey " + own + "; to update the broker, follow section 13 of docs/setup.md" + case version.OtherOlder: + return "warning: the broker runs nodkey " + broker + ", which is older than nodkey " + own + + ` on this machine; to update the broker, run "nodkey update" as root on the broker host` + case version.OtherNewer: + return "warning: this machine runs nodkey " + own + ", which is older than nodkey " + broker + + ` on the broker; to update this machine, run "sudo nodkey update"` + } + return "" +} diff --git a/internal/client/version_test.go b/internal/client/version_test.go new file mode 100644 index 0000000..045222f --- /dev/null +++ b/internal/client/version_test.go @@ -0,0 +1,304 @@ +package client + +import ( + "context" + "net/http" + "slices" + "strconv" + "strings" + "sync" + "syscall" + "testing" + "time" + + "github.com/code6x/nodkey/internal/api" + "github.com/code6x/nodkey/internal/audit" +) + +// The warnings of the tests below. +const ( + warnClientOlder = `nodkey: warning: this machine runs nodkey 0.1.0, which is older than nodkey 0.2.0 on the ` + + `broker; to update this machine, run "sudo nodkey update"` + warnBrokerOlder = `nodkey: warning: the broker runs nodkey 0.1.0, which is older than nodkey 0.2.0 on this ` + + `machine; to update the broker, run "nodkey update" as root on the broker host` + warnNotSent = `nodkey: warning: the broker runs an older nodkey that does not send its version, and this ` + + `machine runs nodkey 0.2.0; to update the broker, follow section 13 of docs/setup.md` +) + +// withVersion wraps handler so that each answer has these values in the +// header api.VersionHeader, and records the versions of the requests. +type withVersion struct { + mu sync.Mutex + answer []string // the values for the answers; nil for none + claimed [][]string // the values of each request +} + +func (v *withVersion) set(values ...string) { + v.mu.Lock() + defer v.mu.Unlock() + v.answer = values +} + +func (v *withVersion) requests() [][]string { + v.mu.Lock() + defer v.mu.Unlock() + return slices.Clone(v.claimed) +} + +func (v *withVersion) wrap(handler http.HandlerFunc) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + v.mu.Lock() + v.claimed = append(v.claimed, r.Header.Values(api.VersionHeader)) + for _, value := range v.answer { + w.Header().Add(api.VersionHeader, value) + } + v.mu.Unlock() + handler(w, r) + } +} + +// TestRunVersion: nodkey run sends its version, and prints one warning +// line when the broker runs another release, or sends no valid version. +// The line comes before the command runs, or before the last line. It +// says which side is older and what to run. It never shows a value from +// the broker that is not a release version, it does not go into the +// environment of the command, and builds from the source get none. +func TestRunVersion(t *testing.T) { + t.Parallel() + value := madeUp("value") + ok := secretsAnswer(t, "GH_TOKEN", value) + tests := []struct { + name string + own string // the client's version + broker []string // the values of the broker's header + status int // the status of the answer + want string // the warning line, or "" + }{ + {"the same version", "0.2.0", []string{"0.2.0"}, 200, ""}, + {"an older client", "0.1.0", []string{"0.2.0"}, 200, warnClientOlder}, + {"an older broker", "0.2.0", []string{"0.1.0"}, 200, warnBrokerOlder}, + {"a broker without the header", "0.2.0", nil, 200, warnNotSent}, + {"an older broker that denies", "0.2.0", []string{"0.1.0"}, 403, warnBrokerOlder}, + {"a broker without the header that fails", "0.2.0", nil, 400, warnNotSent}, + {"markup", "0.2.0", []string{"0.3.0BAD"}, 200, warnNotSent}, + {"a space", "0.2.0", []string{"0.3.0 BAD"}, 200, warnNotSent}, + {"too long", "0.2.0", []string{strings.Repeat("9", 65)}, 200, warnNotSent}, + {"two headers", "0.2.0", []string{"0.1.0", "0.3.0"}, 200, warnNotSent}, + {"an empty header", "0.2.0", []string{""}, 200, warnNotSent}, + {"a client from the source", "dev", []string{"0.2.0"}, 200, ""}, + {"a client from the source, a broker without the header", "dev", nil, 200, ""}, + {"a client without a version", "", nil, 200, ""}, + {"a client with a version that is not valid", "0.1.0 beta", []string{"0.2.0"}, 200, ""}, + {"a broker from the source", "0.2.0", []string{"dev"}, 200, ""}, + {"a broker at a commit", "0.2.0", []string{"1a2b3c4"}, 200, ""}, + {"a broker with a release candidate", "0.2.0", []string{"0.3.0-rc.1"}, 200, ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + v := &withVersion{answer: tt.broker} + body := ok + if tt.status != http.StatusOK { + body = `{"error":"` + http.StatusText(tt.status) + `"}` + } + fb := newFakeBroker(t, v.wrap(answer(tt.status, body))) + fx := &fakeExec{} + r := testRunner(fb.b, []string{"HOME=/home/dev"}, fx) + r.version = tt.own + code, stderr := runIn(t.Context(), r, "--secret", "GH_TOKEN=github-token", "--", self(t)) + lines := linesOf(stderr) + if tt.status == http.StatusOK { + if code != exitOK || len(fx.ran()) != 1 { + t.Fatalf("exit code %d, stderr %q; want the command to run", code, safe(stderr)) + } + if tt.want == "" && len(lines) != 0 || tt.want != "" && !slices.Equal(lines, []string{tt.want}) { + t.Errorf("stderr %q, want only %q", safe(stderr), tt.want) + } + if env := fx.ran()[0].envv; !slices.Equal(env, []string{"HOME=/home/dev", "GH_TOKEN=" + value}) { + t.Errorf("the environment of the command: %q", safe(strings.Join(env, " "))) + } + } else { + last := map[int]string{ + http.StatusForbidden: "nodkey: denied (exit 77)", + http.StatusBadRequest: "nodkey: the broker answered 400 Bad Request (exit 69)", + }[tt.status] + if !slices.Equal(lines, []string{tt.want, last}) || len(fx.ran()) != 0 { + t.Errorf("stderr %q, want the warning %q, then %q", safe(stderr), tt.want, last) + } + } + if strings.Contains(stderr, "BAD") || strings.Contains(stderr, "") || + strings.Contains(stderr, strings.Repeat("9", 65)) { + t.Errorf("stderr shows a value of the header that is not a version: %q", safe(stderr)) + } + mustNotLeak(t, value, stderr) + }) + } +} + +// TestPostSendsVersion: each request has the client's version in the +// header api.VersionHeader, once, when it is a valid version, also from a +// build from the source. A client without a valid version sends none, and +// its requests still work. +func TestPostSendsVersion(t *testing.T) { + t.Parallel() + for own, want := range map[string][]string{ + "0.2.0": {"0.2.0"}, + "dev": {"dev"}, + "1a2b3c4": {"1a2b3c4"}, + "": nil, + "0.2.0 beta": nil, + "0.2.0\nX": nil, + "0.2.0": nil, + } { + v := &withVersion{answer: []string{"0.2.0"}} + fb := newFakeBroker(t, v.wrap(answer(http.StatusOK, `{"keys":[]}`))) + fb.b.warnVersions(own, func(string) {}) + for range 2 { + if _, err := Post[api.KeysAnswer](t.Context(), fb.b, api.PathKeys, []byte("{}"), nil); err != nil { + t.Fatalf("version %q: %v", own, err) + } + } + if got := v.requests(); len(got) != 2 || !slices.Equal(got[0], want) || !slices.Equal(got[1], want) { + t.Errorf("version %q: the requests had %q, want %q each", own, got, want) + } + } + // A Broker that was not told its version sends none. + v := &withVersion{} + fb := newFakeBroker(t, v.wrap(answer(http.StatusOK, `{"keys":[]}`))) + if _, err := Post[api.KeysAnswer](t.Context(), fb.b, api.PathKeys, []byte("{}"), nil); err != nil { + t.Fatal(err) + } + if got := v.requests(); len(got) != 1 || got[0] != nil { + t.Errorf("the request had %q, want no version", got) + } +} + +// TestSSHGrantVersion: nodkey ssh-grant prints the warning before its +// last line, which stays the last line. +func TestSSHGrantVersion(t *testing.T) { + t.Parallel() + for _, tt := range []struct { + broker []string + answer http.HandlerFunc + code int + }{ + {[]string{"0.1.0"}, answer(http.StatusOK, `{"minutes":15}`), exitOK}, + {[]string{"0.1.0"}, answer(http.StatusForbidden, `{"error":"Forbidden"}`), exitDenied}, + } { + v := &withVersion{answer: tt.broker} + fb := newFakeBroker(t, v.wrap(tt.answer)) + g := testGranter(fb.b, nil) + g.version = "0.2.0" + code, stderr := grantIn(t.Context(), g, grantArgs()...) + lines := linesOf(stderr) + if code != tt.code || len(lines) != 2 || lines[0] != warnBrokerOlder || + !strings.HasSuffix(lines[1], "(exit "+strconv.Itoa(tt.code)+")") { + t.Errorf("exit code %d, stderr %q; want %d, the warning, and the last line", code, stderr, tt.code) + } + if got := v.requests(); len(got) != 1 || !slices.Equal(got[0], []string{"0.2.0"}) { + t.Errorf("the request had %q", got) + } + } +} + +// TestSSHAgentVersion: nodkey ssh-agent prints the warning once for each +// version that the broker sends, so an update of the broker gives one new +// line, and the same version again gives none. A value of the header that +// is not a version counts as none, and is never shown. +func TestSSHAgentVersion(t *testing.T) { + t.Parallel() + v := &withVersion{} + fb := newFakeBroker(t, v.wrap(answer(http.StatusOK, `{"keys":[]}`))) + xdg := shortTempDir(t) + r := testAgentRunner(fb.b, xdg) + r.version = "0.2.0" + ready := make(chan string, 1) + r.ready = func(p string) { ready <- p } + ctx, cancel := context.WithCancelCause(context.Background()) + defer cancel(nil) + var stderr syncBuffer + codes := make(chan int, 1) + go func() { codes <- r.run(ctx, nil, &stderr) }() + var path string + select { + case path = <-ready: + case <-time.After(waitLimit): + t.Fatal("the agent does not listen") + } + for _, values := range [][]string{ + // An older broker: one line for both answers. + {"0.1.0"}, {"0.1.0"}, + // A new version: one more line. + {"0.1.1"}, + // No version: one line for all of them. + nil, nil, {"BAD"}, {"0.3.0", "0.3.1"}, + // Nothing to say, and versions with a line before. + {"0.2.0"}, {"dev"}, {"1a2b3c4"}, {"0.1.0"}, {"0.1.1"}, + // A newer broker. + {"0.3.0"}, + } { + v.set(values...) + if _, err := listKeys(t, path); err != nil { + t.Fatalf("list with %q: %v", values, err) + } + } + cancel(interrupted{sig: syscall.SIGTERM}) + select { + case <-codes: + case <-time.After(waitLimit): + t.Fatal("the agent did not stop") + } + want := []string{ + "nodkey: the SSH agent listens on " + path + "; set SSH_AUTH_SOCK to this path for ssh", + warnBrokerOlder, + strings.Replace(warnBrokerOlder, "0.1.0", "0.1.1", 1), + warnNotSent, + `nodkey: warning: this machine runs nodkey 0.2.0, which is older than nodkey 0.3.0 on the broker; ` + + `to update this machine, run "sudo nodkey update"`, + "nodkey: stopped by SIGTERM (exit 143)", + } + if got := linesOf(stderr.String()); !slices.Equal(got, want) { + t.Errorf("stderr:\n%s\nwant:\n%s", strings.Join(got, "\n"), strings.Join(want, "\n")) + } + if strings.Contains(stderr.String(), "BAD") { + t.Error("the agent shows a value of the header that is not a version") + } + for i, got := range v.requests() { + if !slices.Equal(got, []string{"0.2.0"}) { + t.Errorf("request %d had the version %q, want 0.2.0", i, got) + } + } +} + +// TestVersionWarningsAreBounded: however many versions a broker sends, +// one Broker prints at most maxVersionWarnings lines. +func TestVersionWarningsAreBounded(t *testing.T) { + t.Parallel() + b := &Broker{} + var lines []string + b.warnVersions("9.0.0", func(text string) { lines = append(lines, text) }) + for i := range 3 * maxVersionWarnings { + b.versions.check(http.Header{api.VersionHeader: {"1.0." + strconv.Itoa(i)}}) + } + if len(lines) != maxVersionWarnings { + t.Errorf("%d lines, want %d", len(lines), maxVersionWarnings) + } +} + +// TestVersionWithTheRealBroker: the real broker service sends its version +// on its answers, and nodkey run warns about an older broker, before its +// last line. +func TestVersionWithTheRealBroker(t *testing.T) { + t.Parallel() + tb := newTestBrokerWith(t, "0.1.0") + fx := &fakeExec{} + r := testRunner(tb.client(t), nil, fx) + r.version = "0.2.0" + tb.approver.Auto(audit.AnswerDeny) + code, stderr := runIn(t.Context(), r, "--secret", "GH_TOKEN=github-token", "--", self(t)) + lines := linesOf(stderr) + if code != exitDenied || len(lines) != 3 || len(waitingCodes(stderr)) != 1 || lines[1] != warnBrokerOlder || + lines[2] != "nodkey: denied (exit 77)" { + t.Errorf("exit code %d, stderr %q", code, safe(stderr)) + } +} diff --git a/internal/telegram/canary_test.go b/internal/telegram/canary_test.go index 87b96cf..5405d00 100644 --- a/internal/telegram/canary_test.go +++ b/internal/telegram/canary_test.go @@ -66,6 +66,7 @@ func TestTokenNeverShown(t *testing.T) { addErr("End", a.End(ctx, shown.ID, secretsPrompt(), approval.Ending{Answer: audit.AnswerApproveOnce, At: noon})) addErr("Notify", a.Notify(ctx, noticeOf(secretsPrompt()))) addErr("Alert", a.Alert(ctx, "an alert")) + addErr("Inform", a.Inform(ctx, "a notice")) // Telegram refuses or fails. The description holds the token, as a // broken or hostile server might send it. @@ -95,6 +96,8 @@ func TestTokenNeverShown(t *testing.T) { } addErr("End with 503", a.End(ctx, shown.ID, secretsPrompt(), approval.Ending{Cause: audit.CauseExpired})) addErr("End with a bad ID", a.End(ctx, "x", secretsPrompt(), approval.Ending{})) + f.reply("sendMessage", 500, `{"ok":false,"error_code":500,"description":"Internal Server Error `+token+`"}`) + addErr("Inform with 500", a.Inform(ctx, "a notice")) // The network fails. release := f.hold("sendMessage") @@ -128,6 +131,8 @@ func TestTokenNeverShown(t *testing.T) { _, err := a.Ask(ctx, secretsPrompt()) addErr("Ask: "+where, err) addErr("Notify: "+where, a.Notify(ctx, noticeOf(secretsPrompt()))) + a.informs = nil // so that its limit does not stop it + addErr("Inform: "+where, a.Inform(ctx, "a notice")) } a.api.base = f.srv.URL diff --git a/internal/telegram/inform_test.go b/internal/telegram/inform_test.go new file mode 100644 index 0000000..3a94681 --- /dev/null +++ b/internal/telegram/inform_test.go @@ -0,0 +1,110 @@ +package telegram + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/code6x/nodkey/internal/approval" +) + +// TestInform: a notice of the broker goes to the notice chat, without a +// sound, without buttons and with link previews off. Its text is the +// broker's, escaped. +func TestInform(t *testing.T) { + e := newEnv(t) + if err := e.a.Inform(context.Background(), "The client laptop & says that it runs nodkey 0.1.0."); err != nil { + t.Fatal(err) + } + c := e.f.callsOf("sendMessage")[0] + want := "ℹ️ nodkey: The client laptop <b>&</b> says that it runs nodkey 0.1.0." + if c.num("chat_id") != noticeChatID || c.field("disable_notification") != "true" || c.has("reply_markup") || + c.field("link_preview_options") != `{"is_disabled":true}` || c.str("parse_mode") != "HTML" || + c.str("text") != want { + t.Errorf("notice %s", c.Raw) + } + ctx, cancel := context.WithCancel(context.Background()) + cancel() + if err := e.a.Inform(ctx, "a notice"); !errors.Is(err, context.Canceled) { + t.Errorf("Inform with a context that ended: %v, want context.Canceled", err) + } + if n := len(e.f.allCalls()); n != 1 { + t.Errorf("%d calls to Telegram, want 1", n) + } +} + +// TestInformTriesOnce: a notice that Telegram refuses, or that fails on +// the way, is not sent again, also after a 429 or a server error, which +// Notify and Alert try again. Inform does not wait either. +func TestInformTriesOnce(t *testing.T) { + for _, r := range []struct { + status int + body string + }{ + {429, `{"ok":false,"error_code":429,"description":"Too Many Requests","parameters":{"retry_after":5}}`}, + {500, `{"ok":false,"error_code":500,"description":"Internal Server Error"}`}, + {502, `bad gateway`}, + {400, `{"ok":false,"error_code":400,"description":"Bad Request: chat not found"}`}, + } { + e := newEnv(t) + e.f.reply("sendMessage", r.status, r.body) + if err := e.a.Inform(context.Background(), "a notice"); err == nil { + t.Errorf("%d: Inform did not fail", r.status) + } + if n := len(e.f.callsOf("sendMessage")); n != 1 { + t.Errorf("%d: %d tries, want 1", r.status, n) + } + if w := e.sleeps.all(); len(w) != 0 { + t.Errorf("%d: Inform waited %v", r.status, w) + } + } +} + +// TestInformLimit: Inform sends at most InformsPerMinute notices in any 60 +// seconds; a notice that failed counts too. Over the limit it makes no +// call. Its count is its own: notices of "never ask" uses do not take +// from it, and it does not take from theirs. So a "never ask" use right +// after notices of the broker still gets all of its notices. +func TestInformLimit(t *testing.T) { + e := newEnv(t) + now := time.Date(2026, 10, 17, 12, 0, 0, 0, time.UTC) + e.a.now = func() time.Time { return now } + e.f.reply("sendMessage", 400, `{"ok":false,"error_code":400,"description":"Bad Request: chat not found"}`) + if err := e.a.Inform(context.Background(), "a notice"); err == nil { + t.Fatal("the first notice did not fail") + } + for i := 1; i < InformsPerMinute; i++ { + now = now.Add(time.Second) + if err := e.a.Inform(context.Background(), "a notice"); err != nil { + t.Fatalf("notice %d: %v", i+1, err) + } + } + if err := e.a.Inform(context.Background(), "a notice"); !errors.Is(err, ErrInformLimit) { + t.Errorf("notice %d: %v, want ErrInformLimit", InformsPerMinute+1, err) + } + if n := len(e.f.callsOf("sendMessage")); n != InformsPerMinute { + t.Errorf("%d calls, want %d", n, InformsPerMinute) + } + p := noticeOf(secretsPrompt()) + for i := range NoticesPerMinute { + if err := e.a.Notify(context.Background(), p); err != nil { + t.Fatalf("notice of a \"never ask\" use %d after the notices of the broker: %v", i+1, err) + } + } + if err := e.a.Notify(context.Background(), p); !errors.Is(err, ErrNoticeLimit) { + t.Errorf("Notify over its limit: %v, want ErrNoticeLimit", err) + } + if errors.Is(ErrInformLimit, approval.ErrLimit) { + t.Error("ErrInformLimit is a limit for the approval core; Inform has nothing to do with the core") + } + // 60 seconds after the first notice of the broker, one more may go + // out, also while the notices of "never ask" uses are at their limit. + now = now.Add(time.Minute - time.Duration(InformsPerMinute-1)*time.Second) + if err := e.a.Inform(context.Background(), "a notice"); err != nil { + t.Errorf("after a minute: %v", err) + } + if err := e.a.Inform(context.Background(), "a notice"); !errors.Is(err, ErrInformLimit) { + t.Errorf("after a minute, the second: %v, want ErrInformLimit", err) + } +} diff --git a/internal/telegram/telegram.go b/internal/telegram/telegram.go index e32cf7d..c254b9d 100644 --- a/internal/telegram/telegram.go +++ b/internal/telegram/telegram.go @@ -47,6 +47,14 @@ import ( // for each client (approval.MaxNoticesPerMinute). const NoticesPerMinute = 20 +// InformsPerMinute is the most notices of the broker itself that Inform +// sends in any 60 seconds, such as a notice about a client that runs +// another version of nodkey. They have their own count, apart from +// NoticesPerMinute, so they never take the place of a notice of a "never +// ask" use. The number is small, so they take little of what Telegram +// allows in the notice chat. +const InformsPerMinute = 3 + // maxRemembered is the most messages whose state the approver keeps, for // taps that come after their request ended. const maxRemembered = 1024 @@ -88,6 +96,9 @@ var ( // nothing was sent: the core denies the use with cause limit, and the // use does not count for max_per_day. ErrNoticeLimit = fmt.Errorf("telegram: too many notices in the last minute: %w", approval.ErrLimit) + // ErrInformLimit: Inform sent InformsPerMinute notices in the last 60 + // seconds, so it sends no more. + ErrInformLimit = errors.New("telegram: too many notices of the broker in the last minute") // ErrTokenRefused: Telegram answered a call with 401 or 404: it does // not accept the bot token, or does not know the bot, such as after // the token was revoked or the bot deleted. Only a new token helps. @@ -119,8 +130,8 @@ type Options struct { } // Approver is the Telegram approver: an approval.Approver. Make it with -// New. Ask, End, Notify and Alert are safe for use by many goroutines. -// Run runs once at a time. +// New. Ask, End, Notify, Alert and Inform are safe for use by many +// goroutines. Run runs once at a time. type Approver struct { api *client userID int64 // the approver; the private chat with the approver has the same ID @@ -136,8 +147,9 @@ type Approver struct { // first; there are at most maxRemembered. sent map[approval.MessageID]*sentMessage order []approval.MessageID - // notices holds when the newest notices were sent. - notices []time.Time + // notices holds when the newest notices of Notify were sent, and + // informs those of Inform. + notices, informs []time.Time // reports counts the reports of refused taps of other users in the // current minute, and ownReports those of the approver's own taps. // Each is a budget of its own, so the taps of others cannot crowd out @@ -373,6 +385,39 @@ func (a *Approver) Alert(ctx context.Context, msg string) error { }, nil) } +// Inform sends msg to the notice chat, without a sound: a notice of the +// broker itself that needs no answer, such as a client that runs another +// version of nodkey. msg is the broker's own text, with names from the +// config and checked values only. It is HTML-escaped and cut to +// maxAlertUnits. +// +// Inform never takes the place of a notice of a "never ask" use: it has +// its own count. When it sent InformsPerMinute notices in the last 60 +// seconds, it sends nothing and returns ErrInformLimit. Inform makes one +// try, which takes at most callTimeout, and never tries again itself: +// the caller logs a notice that did not go out, and decides when to try +// again. +func (a *Approver) Inform(ctx context.Context, msg string) error { + if err := ctx.Err(); err != nil { + return err + } + if !a.take(&a.informs, InformsPerMinute) { + return ErrInformLimit + } + var t text + t.write("ℹ️ nodkey: " + cutUnits(strings.ToValidUTF8(msg, "�"), maxAlertUnits)) + if e := a.api.callOnce(ctx, "sendMessage", sendMessageParams{ + ChatID: a.noticeChat, + Text: t.html.String(), + ParseMode: "HTML", + LinkPreviewOptions: noPreview, + DisableNotification: true, + }, nil, a.api.timeout); e != nil { + return e + } + return nil +} + // edit replaces the text of a message with html, and removes its buttons. func (a *Approver) edit(ctx context.Context, chatID, msgID int64, html string) error { err := a.api.call(ctx, "editMessageText", editMessageTextParams{ @@ -473,15 +518,21 @@ func (a *Approver) takeChangedAlert() bool { return true } -// takeNotice reports whether a notice may go out now, and counts it. -func (a *Approver) takeNotice() bool { +// takeNotice reports whether a notice of Notify may go out now, and +// counts it. +func (a *Approver) takeNotice() bool { return a.take(&a.notices, NoticesPerMinute) } + +// take reports whether one more message may go out now, when sent holds +// when the newest ones went out and at most limit may go out in any 60 +// seconds. If so, it counts the message in sent. +func (a *Approver) take(sent *[]time.Time, limit int) bool { a.mu.Lock() defer a.mu.Unlock() now := a.now() - a.notices = slices.DeleteFunc(a.notices, func(t time.Time) bool { return now.Sub(t) >= time.Minute }) - if len(a.notices) >= NoticesPerMinute { + *sent = slices.DeleteFunc(*sent, func(t time.Time) bool { return now.Sub(t) >= time.Minute }) + if len(*sent) >= limit { return false } - a.notices = append(a.notices, now) + *sent = append(*sent, now) return true } diff --git a/internal/version/version.go b/internal/version/version.go new file mode 100644 index 0000000..0f5d929 --- /dev/null +++ b/internal/version/version.go @@ -0,0 +1,121 @@ +// Package version reads and compares versions of nodkey (DESIGN.md +// section 10). The client and the broker send each other their version +// in a header (api.VersionHeader). A value from the other side counts +// only if it has the form of a version (Valid); any other value counts as +// no version, and is never printed or logged. Only release versions +// (X.Y.Z) are compared, as numbers. A version of another form, such as +// "dev" or a commit, is a build from the source: Check reports nothing +// for it. +package version + +import ( + "cmp" + "regexp" + "strconv" + "strings" +) + +// maxLen is the most bytes of a version. +const maxLen = 64 + +// pattern is the form of a version, such as "1.2.0", "v1.2.0-rc.1" or +// "dev". The audit log uses it for the broker_started entry too. +var pattern = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9.+_-]{0,63}$`) + +// Valid reports whether s has the form of a version: 1 to 64 letters, +// digits, ".", "+", "_" or "-", with a letter or a digit first. Such a +// value holds no space, no control character and no markup, so it is +// safe to print and to log. A value from the other side that is not +// valid counts as no version, and must never be printed or logged. +func Valid(s string) bool { return len(s) <= maxLen && pattern.MatchString(s) } + +// maxDigits is the most digits of a number in a release version, so a +// number is at most 999999. +const maxDigits = 6 + +// Release is a release version: X.Y.Z. +type Release struct{ Major, Minor, Patch int } + +// ParseRelease reads a release version: three numbers from 0 to 999999, +// each without leading zeros, separated by ".", and nothing else, such as +// "1.2.0". ok is false for any other text, such as "dev", "1.2", +// "1.02.0", "1.2.0-rc.1", "v1.2.0" or "1.2.0 ". +func ParseRelease(s string) (r Release, ok bool) { + parts := strings.Split(s, ".") + if len(parts) != 3 { + return Release{}, false + } + var n [3]int + for i, p := range parts { + if p == "" || len(p) > maxDigits || len(p) > 1 && p[0] == '0' { + return Release{}, false + } + for j := range len(p) { + if p[j] < '0' || p[j] > '9' { + return Release{}, false + } + n[i] = 10*n[i] + int(p[j]-'0') + } + } + return Release{Major: n[0], Minor: n[1], Patch: n[2]}, true +} + +// Compare returns -1 when r is older than o, 0 when they are the same, +// and +1 when r is newer. It compares the numbers, not the text: 1.10.0 +// is newer than 1.9.0. +func (r Release) Compare(o Release) int { + for _, d := range [3]int{r.Major - o.Major, r.Minor - o.Minor, r.Patch - o.Patch} { + if d != 0 { + return cmp.Compare(d, 0) + } + } + return 0 +} + +// String returns the version as "X.Y.Z". +func (r Release) String() string { + return strconv.Itoa(r.Major) + "." + strconv.Itoa(r.Minor) + "." + strconv.Itoa(r.Patch) +} + +// Diff says how the version that the other side sent compares with the +// version of this side. +type Diff int + +const ( + // NoDiff: there is nothing to say. Both sides run the same release, + // or one of them is a build from the source. + NoDiff Diff = iota + // NotSent: this side runs a release, and the other side sent no + // version, or a value that is not Valid. It may run an older nodkey + // that does not send its version. + NotSent + // OtherOlder: the other side runs an older release than this side. + OtherOlder + // OtherNewer: the other side runs a newer release than this side. + OtherNewer +) + +// Check compares own, the version of this side, with other, the version +// that the other side sent ("" for none). When own is not a release, or +// other is a Valid version that is not a release, it returns NoDiff: +// builds from the source are not compared. +func Check(own, other string) Diff { + mine, ok := ParseRelease(own) + if !ok { + return NoDiff + } + if !Valid(other) { + return NotSent + } + theirs, ok := ParseRelease(other) + if !ok { + return NoDiff + } + switch theirs.Compare(mine) { + case -1: + return OtherOlder + case 1: + return OtherNewer + } + return NoDiff +} diff --git a/internal/version/version_test.go b/internal/version/version_test.go new file mode 100644 index 0000000..bef52d6 --- /dev/null +++ b/internal/version/version_test.go @@ -0,0 +1,124 @@ +package version + +import ( + "strings" + "testing" +) + +// TestValid: a version is 1 to 64 letters, digits, ".", "+", "_" or "-", +// with a letter or a digit first. Nothing else passes: no space, no +// control character, no markup, and nothing longer. +func TestValid(t *testing.T) { + for _, s := range []string{ + "0.1.0", "1.2.0", "v1.2.0-rc.1", "dev", "1a2b3c4", "1.2.0+build.5", "x_y", "A", + strings.Repeat("9", 64), + } { + if !Valid(s) { + t.Errorf("Valid(%q) = false", s) + } + } + for _, s := range []string{ + "", " ", "1.2.0 ", " 1.2.0", "1.2.0\n", "1.2.0\r\nX-Other: 1", "1.2.0\x00", "1.2.0", "1.2.0;rm", + "1.2.0,1.3.0", "-1.2.0", ".1.2.0", "_dev", "+dev", "1.2.0/x", "d\u00e9v", "1.2.0\u202e", strings.Repeat("9", 65), + strings.Repeat("a", 64<<10), + } { + if Valid(s) { + t.Errorf("Valid(%q) = true", s) + } + } +} + +// TestParseRelease: only X.Y.Z with numbers from 0 to 999999 and no +// leading zeros is a release. Everything else is not, so it is a build +// from the source, or not a version. +func TestParseRelease(t *testing.T) { + for _, tt := range []struct { + in string + want Release + }{ + {"0.0.0", Release{0, 0, 0}}, + {"0.1.0", Release{0, 1, 0}}, + {"1.2.3", Release{1, 2, 3}}, + {"10.20.30", Release{10, 20, 30}}, + {"999999.999999.999999", Release{999999, 999999, 999999}}, + } { + got, ok := ParseRelease(tt.in) + if !ok || got != tt.want { + t.Errorf("ParseRelease(%q) = %v, %v; want %v", tt.in, got, ok, tt.want) + } + if got.String() != tt.in { + t.Errorf("%v.String() = %q, want %q", got, got.String(), tt.in) + } + } + for _, s := range []string{ + "", "dev", "1", "1.2", "1.2.3.4", "1..3", ".1.2", "1.2.", "01.2.3", "1.02.3", "1.2.03", "00.1.2", + "1000000.0.0", "0.1000000.0", "0.0.1000000", "9999999999999999999999.0.0", "v1.2.3", "1.2.3-rc.1", + "1.2.3+build", " 1.2.3", "1.2.3 ", "1.2.3\n", "+1.2.3", "-1.2.3", "1.-2.3", "1.2.x", "1.2.3a", "\u0661.\u0662.\u0663", + "1.2.3\x00", + } { + if r, ok := ParseRelease(s); ok { + t.Errorf("ParseRelease(%q) = %v, want no release", s, r) + } + } +} + +// TestCompare: releases compare as numbers, not as text. +func TestCompare(t *testing.T) { + for _, tt := range []struct { + a, b string + want int + }{ + {"1.2.3", "1.2.3", 0}, + {"0.1.0", "0.2.0", -1}, + {"0.2.0", "0.1.0", 1}, + {"1.9.0", "1.10.0", -1}, + {"1.10.0", "1.9.0", 1}, + {"2.0.0", "1.999999.999999", 1}, + {"1.2.3", "1.2.4", -1}, + {"1.3.0", "1.2.9", 1}, + {"0.0.9", "0.0.10", -1}, + } { + a, okA := ParseRelease(tt.a) + b, okB := ParseRelease(tt.b) + if !okA || !okB { + t.Fatalf("%q or %q is not a release", tt.a, tt.b) + } + if got := a.Compare(b); got != tt.want { + t.Errorf("%s.Compare(%s) = %d, want %d", tt.a, tt.b, got, tt.want) + } + } +} + +// TestCheck: only two releases are compared. A side that is a build from +// the source gives nothing to say; a release on this side and no valid +// version from the other gives NotSent. +func TestCheck(t *testing.T) { + for _, tt := range []struct { + own, other string + want Diff + }{ + {"0.2.0", "0.2.0", NoDiff}, + {"0.2.0", "0.1.0", OtherOlder}, + {"0.2.0", "0.3.0", OtherNewer}, + {"1.9.0", "1.10.0", OtherNewer}, + {"0.2.0", "", NotSent}, + {"0.2.0", "0.1.0 x", NotSent}, + {"0.2.0", "0.1.0", NotSent}, + {"0.2.0", strings.Repeat("9", 65), NotSent}, + // Builds from the source, on either side. + {"0.2.0", "dev", NoDiff}, + {"0.2.0", "1a2b3c4", NoDiff}, + {"0.2.0", "0.3.0-rc.1", NoDiff}, + {"0.2.0", "v0.3.0", NoDiff}, + {"dev", "0.1.0", NoDiff}, + {"dev", "", NoDiff}, + {"dev", "dev", NoDiff}, + {"", "0.1.0", NoDiff}, + {"", "", NoDiff}, + {"0.2.0 x", "0.1.0", NoDiff}, + } { + if got := Check(tt.own, tt.other); got != tt.want { + t.Errorf("Check(%q, %q) = %d, want %d", tt.own, tt.other, got, tt.want) + } + } +} diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index 1eb10e2..7a6b02f 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -622,9 +622,11 @@ func firstOf(text string, parts []string) string { // nodkey: not the broker's journal, not nodkey ssh-agent, and not the // lines of nodkey run, nodkey ssh-grant, nodkey keygen and nodkey audit // verify. The config of the test has no "never ask" rule, so no notice -// goes out, and nothing in the test makes an alert: every message must -// have buttons. It runs after the broker has stopped, when its alerts in -// the background have gone out too. +// goes out, and nothing in the test makes an alert. The client and the +// broker are the same build from the source, so no notice about a version +// goes out either (DESIGN.md section 10): every message must have +// buttons. It runs after the broker has stopped, when its alerts in the +// background have gone out too. func (e *env) searchClaimed(t *testing.T) { for _, text := range e.tg.unasked(t) { t.Errorf("a message without buttons went out, an alert or a notice:\n%s", redact(text))