diff --git a/DESIGN.md b/DESIGN.md index dd53d06..fb77127 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -1449,7 +1449,9 @@ name exactly one client. The config check refuses all of these (section not started yet. Grants of other clients stay. - A window is 1 minute to 8 hours. A shorter one would give grants of 0 whole minutes, so the rule would quietly act like `always`. -- Grants live only in memory. A broker restart removes them all. +- Grants live only in memory. A broker restart removes them all. A reload + of the config keeps the grants that the new config still allows + (section 14, "Reload"). - A secret or key that is not in the broker config does not exist for the client. The client sends names, never store IDs. So the agent cannot ask for any other item in the store. @@ -1909,7 +1911,7 @@ and the alarm comes. Run the job as its own system user (see written and without its newline, in hex. The first entry has 64 zeros. - A line is at most 64 KiB. The broker refuses a longer entry, so the request is denied. -- Events: `broker_started`, `request`, `decision`, `grant_used`, +- Events: `broker_started`, `config_reloaded`, `request`, `decision`, `grant_used`, `grant_ended`, `grant_alarm`, `secret_sent`, `signed`, `denied`, `foreign_tap`, `error`, `log_recovered`, `broker_stopped`. Each event about a request holds the request ID, the client, the names, the verified host if any, and the @@ -1956,10 +1958,11 @@ The fields of each event, after `seq`, `time`, `event` and `prev`: |---|---| | `broker_started` | `version` | | `broker_stopped` | none | +| `config_reloaded` | `config_sha256`: the SHA-256 of the new config file, in hex; `grants_ended`: how many grants the reload ended, of every kind (section 14, "Reload") | | `request` | the request fields; `message`: the exact text the approver saw, if a message was sent | | `decision` | the request fields; `user_id`: the Telegram user that answered, or 0 with ntfy; `channel`: only with ntfy, `ntfy:` and the answer topic, such as `ntfy:nodkey-answers`; `answer`: `approve_once`, `approve_window`, `deny` or `renew`; `window_minutes` for `approve_window`; for `renew`, the fields of the new standing grant (a new `request_id` and `grant_id`, `not_before` the time of the tap) and `renew_of`, the ID of the grant it renews | | `grant_used` | the request fields; `via`: `grant`, `never_rule`, `scheduled` or `standing` (then `grant_id` is in the request fields); for `standing` also `new_token_hash` | -| `grant_ended` | the request fields of the `preapprove` request that made the grant, with its `grant_id`; `cause`: `used_up`, `ended`, `cancelled`, `denied`, `revoked`, `stopped`, `alarm` or `renewed`; `uses_left` | +| `grant_ended` | the request fields of the `preapprove` request that made the grant, with its `grant_id`; `cause`: `used_up`, `ended`, `cancelled`, `denied`, `revoked`, `stopped`, `alarm`, `renewed` or `config_changed`; `uses_left` | | `grant_alarm` | the request fields; `grant_id` (none for an unknown token); `cause`: `old_token`, `over_uses`, `outside_window`, `unknown_token` or `missed_window` | | `secret_sent`, `signed` | the request fields | | `denied` | the request fields; `cause`: `approver`, `expired`, `cancelled`, `not_allowed`, `limit` or `error` | @@ -3116,10 +3119,69 @@ write to, below directories that pass the same checks. It may be a symbolic link. `broker_pin` is the client's only trust in the broker, so other users must not be able to change it. +### Reload + +A change of the config needs no restart (issue 29). `SIGHUP` asks the +broker to load its config again; `nodkey broker reload` sends it and waits +for the result (section 15). A restart ends every grant; a reload ends only +the grants that the new config takes away. + +1. The broker loads the config file, `known_hosts` and the SSH keys with + the same code and the same checks as a start (`config.LoadBroker`, then + the keys). A config that fails any check is refused. +2. A reload may change only `[client.*]`, `[secret.*]`, `[key.*]`, the + `known_hosts` path and its hosts. Every other part must be equal, also + a part that a later version adds, until it is decided otherwise. A change of `listen`, `tls_key`, + `audit_log`, `[telegram]`, `[ntfy]`, `[updates]`, `[preapprove]`, + `[standing]` or any `[store.*]` is refused with "needs a restart". + These parts hold open sockets, tokens, running stores and timers; a + restart is the one safe way to change them. +3. A refused reload changes nothing: the broker goes on with the config it + has, logs why, and writes no audit entry. +4. A good reload makes the new config the config of the server (the client + pins) first, then of the approval core (which ends grants, rule 5), and + last of the service. A new connection is checked + against the new pins. An open connection of a client that is gone, or + that has a new pin, gets `403` at its next request and is closed. +5. Each grant stays only when the new config allows it in the same way: + - its client is still there, with the same pin; + - each of its secrets is still there, still lists the client, and has + the same store, item and rule (other clients in the list may change); + - its key is still there, still lists the client, has the same file, + the same public key in it and the same rule, and each host of the + grant is still in `known_hosts` and still allowed by `hosts`. + + A grant that stays keeps its uses left and its end. Every other grant + ends at once. A scheduled or standing grant that ends gets a + `grant_ended` entry with cause `config_changed`, and its message loses + the Revoke button. +6. A request that the broker checked with the old config and that waits + for a tap is still decided. But its approval starts no grant (an + `nodkey ssh-grant` gets a deny), and before the broker fetches a value + or signs, and again before it sends the answer, it checks the request + again. The pin of the client's key must be the pin of the client in the + config of now. The client, each secret, or the key, its public key and + the host must be the same (rule 5) from the config of the start of the + request, and from the config that the core decided with, to the config + of now. If not, the request is denied (cause `not_allowed`, `403`), + after its `decision` entry, and a fetched value is cleared. +7. Then the broker writes `config_reloaded`, with the SHA-256 of the new + config file and the number of grants that ended (section 9). +8. The broker writes the result to `reload-status.json`, in the directory + of `audit_log`: its process ID and start time, a count of reloads, the + result (`started`, `loaded` or `refused`), the reason of a refusal, and + the SHA-256 of the config file that it uses. It writes the file at the + start too. The file is written to a new name and then renamed, mode + 0600. + +A reload never changes the limits of grants that exist: `[preapprove]` +and `[standing]` need a restart. + ## 15. Commands and exit codes ``` nodkey broker [--config FILE] +nodkey broker reload [--config FILE] [--timeout DURATION] nodkey run [--secret VAR=NAME]... [--grant-token FILE] [--reason TEXT] [--mask | --no-mask] -- COMMAND [ARGS...] nodkey ssh-agent [--socket PATH] nodkey ssh-grant --key NAME --user USER --host NAME... --for DURATION [--reason TEXT] @@ -3229,6 +3291,27 @@ it started before. 11. Only then does it read the connections on the `listen` address and take requests. +SIGHUP never stops it, also not during the start: once it is ready, a +SIGHUP reloads the config (section 14, "Reload"). + +`nodkey broker reload` asks a running broker to reload, and waits for the +result. It runs as root or as the broker's user. It reads the config +file only for the path of `audit_log` and the SHA-256 of the file; it +checks nothing else, because the broker checks the config itself. It +reads `reload-status.json` next to the audit log: a regular file, not a +symbolic link, not owned by root. It sends SIGHUP only to the process in +the file, and only when that process runs as the owner of the file and +has the start time in the file (`/proc/PID/stat`), so a stale file never +signals another process. Then it reads the file until the count of +reloads goes up, at most `--timeout` (default 60s). It exits 0 only when +the broker loaded the config and the SHA-256 of the config in use is the +SHA-256 of the file when the command began. It exits 1 with the reason +when the broker refused the config, when the file changed during the +reload, when the broker restarted or did not answer in time, or when the +command cannot parse the config (then it sends no signal); 64 on wrong +usage. A tool that applies config changes, such as from git, runs it +after each change and undoes the change when it fails. + It stops on SIGTERM or SIGINT, also during the start, when the polling stops for good (section 7), when its server fails, and when a write to stderr fails (a log line or an audit anchor), such as after the reader diff --git a/THREAT-MODEL.md b/THREAT-MODEL.md index a9a67c6..50c1881 100644 --- a/THREAT-MODEL.md +++ b/THREAT-MODEL.md @@ -1444,6 +1444,47 @@ Risk that remains: real. nodkey cannot see a read in Infisical's own apps or with the client secret, and it cannot check where an account is logged in. A leaked client secret gets tokens until it ends or is revoked. +**T42. A reload of the config leaves access that the new config takes +away, or is used to send a signal to another process.** +A reload (DESIGN.md section 14, "Reload") must end every grant that the +new config takes away, at once, and a removed client must not go on with +a connection that it opened before. + +What nodkey does: +- The broker checks the new config with the same code as a start. Any + error refuses it, and the old config stays (fail closed). A change of a + part that holds sockets, tokens or running stores is refused: it needs + a restart. +- A grant stays only when its client has the same pin, and each secret + or key, with its store item, key file, public key and rule, is the same + and still lists the client, and each host of a key grant is still + allowed. Every other grant ends before the reload is done. A change + that nodkey does not know as harmless ends the grant. +- An open connection of a removed client, or of a client with a new pin, + gets `403` at its next request and is closed; a new handshake checks + the new pins. +- A request that waits for a tap during a reload starts no grant. Before + a value is fetched or a login signed, and again before the answer, the + broker checks the request against the config of now: the pin of the + client's key, and each secret or key, from the config of the start of + the request and from the config that the core decided with. +- Each reload that the broker took has a `config_reloaded` entry. A + refused one is in the journal. +- `nodkey broker reload` runs as root. It sends SIGHUP only to the process + in the status file, and only when that process runs as the owner of + the file (never root) and has the start time in the file. It does not + follow a symbolic link to the file. So a stale file, or one that the + broker's user wrote, cannot make root signal another user's process. + +Risk that remains: low. An answer that passed the last check just before +the reload ends can still go out. Whoever can +write the config can widen access with a reload, as with a restart; the +config file must stay writable by root only (section 14). The broker's +user can write the status file, so it can make `nodkey broker reload` +signal another process of the broker's user, or report a false result; +that user owns the broker already. The command cleans the text of the +file before it prints it. + ## 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 a064c4f..5ed710d 100644 --- a/cmd/nodkey/broker.go +++ b/cmd/nodkey/broker.go @@ -10,6 +10,7 @@ import ( "log/slog" "maps" "net" + "os" "os/signal" "path/filepath" "slices" @@ -104,6 +105,7 @@ var newStore = store.New var errPanicked = errors.New("it panicked; the panic value is not logged") const brokerUsage = `Usage: nodkey broker [--config FILE] + nodkey broker reload [--config FILE] [--timeout DURATION] Run the broker: the daemon that holds the SSH keys and the access to the secret stores. It asks the approver on Telegram before it hands out a @@ -113,7 +115,8 @@ unit such as contrib/systemd/nodkey.service. --config FILE the broker config (default /etc/nodkey/broker.toml) It logs to stderr, which systemd keeps in the journal, with the anchors -of the audit log. SIGTERM or SIGINT stops it. So does a failed write to +of the audit log. SIGTERM or SIGINT stops it. SIGHUP reloads the config +without a restart; see "nodkey broker reload --help". So does a failed write to stderr, such as after its reader is gone: it stops with exit code 1. Exit codes: 0 stopped by SIGTERM or SIGINT, 1 any other error, 64 wrong @@ -124,7 +127,10 @@ systemd must not start the broker again: only a person can fix them. // runBroker runs nodkey broker with args (after "broker") and returns the // exit code. -func runBroker(args []string, stderr io.Writer) int { +func runBroker(args []string, stdout, stderr io.Writer) int { + if len(args) > 0 && args[0] == "reload" { + return runBrokerReload(args[1:], stdout, stderr) + } // A broken stderr must not end the broker with SIGPIPE in the middle // of a line: the write fails instead, and the broker stops in its // order (issue #73). @@ -155,7 +161,12 @@ func runBroker(args []string, stderr io.Writer) int { } ctx, stop := signal.NotifyContext(context.Background(), syscall.SIGTERM, syscall.SIGINT) defer stop() - return serveBroker(ctx, *path, watch, log) + // SIGHUP asks for a reload (brokerreload.go). It must never stop the + // broker, which is what it does by default, also during the start. + hup := make(chan os.Signal, 1) + signal.Notify(hup, syscall.SIGHUP) + defer signal.Stop(hup) + return serveBroker(ctx, *path, watch, log, hup) } // stderrWatch passes each write on to w, and closes failed at the first @@ -192,7 +203,9 @@ func printBrokerUsage(w io.Writer) { // serveBroker starts the broker with the config at path, serves until ctx // ends (a signal) or a part stops, then stops the broker and returns the // exit code (DESIGN.md section 15). -func serveBroker(ctx context.Context, path string, stderr *stderrWatch, log *slog.Logger) (code int) { +func serveBroker(ctx context.Context, path string, stderr *stderrWatch, log *slog.Logger, + hup <-chan os.Signal, +) (code int) { defer func() { // The runtime would print the panic value, and no line of the // broker may hold text it did not choose. @@ -201,7 +214,8 @@ func serveBroker(ctx context.Context, path string, stderr *stderrWatch, log *slo code = exitFailure } }() - b := &brokerParts{log: log, stderr: stderr, stderrFailed: stderr.failed} + b := &brokerParts{log: log, stderr: stderr, stderrFailed: stderr.failed, hup: hup, path: path, + procStart: ownStart()} bySignal := false switch err := b.start(ctx, path); { case err != nil && errors.Is(err, context.Canceled) && ctx.Err() != nil: @@ -275,6 +289,14 @@ type brokerParts struct { // started is true once the broker_started entry is on disk. started bool + // hup gets SIGHUP: a reload of the config at path (brokerreload.go). + // reloads counts the reloads; procStart is the start time of this + // process, for the status file. + hup <-chan os.Signal + path string + reloads uint64 + procStart uint64 + // stopCheck ends the check for a newer release; checkDone is closed // when the check returned. Both are nil when the check is off. stopCheck context.CancelFunc @@ -410,6 +432,7 @@ func (b *brokerParts) start(ctx context.Context, path string) error { b.startCheck(ch.inform, ch.loud) b.serve(srv, ln) b.log.Info("nodkey broker: listening; the broker is ready", slog.String("address", ln.Addr().String())) + b.writeReloadStatus(reloadStatus{Result: reloadStarted, ConfigSHA256: cfg.Sum}) return nil } @@ -638,24 +661,28 @@ func (b *brokerParts) serve(srv *transport.Server, ln net.Listener) { } // wait waits until ctx ends, the approver stops, the server stops, or a -// write to stderr fails. It reports whether a signal asked to stop. +// write to stderr fails. On SIGHUP it reloads the config, and waits again. It reports whether a signal asked to stop. func (b *brokerParts) wait(ctx context.Context) bool { - select { - case <-ctx.Done(): - b.log.Info("nodkey broker: stopping, as a signal asked") - return true - case <-b.runDone: - // The service has logged why. - b.log.Error("nodkey broker: stopping, because the approver stopped: no answer can come any more") - return false - case <-b.serveDone: - b.log.Error("nodkey broker: stopping, because the server stopped", slog.String("error", errText(b.serveErr))) - return false - case <-b.stderrFailed: - // This line most likely fails too. - b.log.Error("nodkey broker: stopping, because a write to stderr failed: " + - "the log lines and the audit anchors have nowhere to go") - return false + for { + select { + case <-b.hup: + b.reload(b.path) + case <-ctx.Done(): + b.log.Info("nodkey broker: stopping, as a signal asked") + return true + case <-b.runDone: + // The service has logged why. + b.log.Error("nodkey broker: stopping, because the approver stopped: no answer can come any more") + return false + case <-b.serveDone: + b.log.Error("nodkey broker: stopping, because the server stopped", slog.String("error", errText(b.serveErr))) + return false + case <-b.stderrFailed: + // This line most likely fails too. + b.log.Error("nodkey broker: stopping, because a write to stderr failed: " + + "the log lines and the audit anchors have nowhere to go") + return false + } } } diff --git a/cmd/nodkey/broker_reload_linux_test.go b/cmd/nodkey/broker_reload_linux_test.go new file mode 100644 index 0000000..b0f671b --- /dev/null +++ b/cmd/nodkey/broker_reload_linux_test.go @@ -0,0 +1,243 @@ +package main + +import ( + "bytes" + "encoding/json" + "net/http" + "os" + "path/filepath" + "regexp" + "strings" + "syscall" + "testing" + + "github.com/code6x/nodkey/internal/api" + "github.com/code6x/nodkey/internal/transport" +) + +// runReload runs nodkey broker reload for the config of e. +func runReload(e *brokerEnv, extra ...string) (code int, stdout, stderr string) { + var out, errOut bytes.Buffer + args := append([]string{"broker", "reload", "--config", e.cfgPath, "--timeout", "30s"}, extra...) + code = run(args, strings.NewReader(""), &out, &errOut) + return code, out.String(), errOut.String() +} + +// listKeys asks the broker for the keys of the client, with c. +func listKeys(t *testing.T, e *brokerEnv, c *http.Client) []api.Key { + t.Helper() + a := e.post(c, api.PathKeys, api.KeysRequest{}) + mustStatus(t, a, http.StatusOK) + ans, err := api.ReadAnswer[api.KeysAnswer](bytes.NewReader(a.body)) + if err != nil { + t.Fatal(err) + } + return ans.Keys +} + +// TestBrokerReload: nodkey broker reload sends SIGHUP and exits 0 only +// when the broker loaded the config that is in the file. An invalid config +// and a change that needs a restart are refused: the command exits 1 with +// the reason, the broker logs why and goes on with the old config. A new +// config takes effect at once; a client with a new pin cannot use its open +// connection; each reload that the broker took has a config_reloaded entry. +func TestBrokerReload(t *testing.T) { + t.Parallel() + e := startListening(t, brokerOptions{}) + e.stderr.wait(t, lineListening) + c := e.httpClient() + orig, err := os.ReadFile(e.cfgPath) + if err != nil { + t.Fatal(err) + } + write := func(cfg string) { writeTestFile(t, e.cfgPath, []byte(cfg), 0o600) } + if n := len(listKeys(t, e, c)); n != 1 { + t.Fatalf("%d keys at the start, want 1", n) + } + + // The same config: loaded. + code, out, errOut := runReload(e) + if code != exitOK || !strings.Contains(out, "the broker loaded the new config") || errOut != "" { + t.Fatalf("reload: exit %d, stdout %q, stderr %q", code, out, errOut) + } + + // A config that does not parse: the command says why, and sends no + // signal. SIGHUP by hand: the broker refuses it, logs why, and goes on + // with the old config. + write(string(orig) + "\n[client.bad]\npin = \"not-a-pin\"\n") + code, out, errOut = runReload(e) + if code != exitFailure || out != "" || !strings.Contains(errOut, "[client.bad] pin") || + strings.Count(errOut, "\n") != 1 { + t.Fatalf("reload of an invalid config: exit %d, stdout %q, stderr %q", code, out, errOut) + } + e.signal(syscall.SIGHUP) + e.stderr.wait(t, "nodkey broker: refused the new config; the broker goes on with the config it has") + if !e.running() { + t.Fatal("SIGHUP stopped the broker") + } + if n := len(listKeys(t, e, c)); n != 1 { + t.Fatalf("%d keys after a refused reload, want 1", n) + } + + // A config that parses, but names a key file that is not there: the + // broker refuses it, and the command exits 1 with the reason. + write(strings.Replace(string(orig), "/ssh-deploy\"", "/ssh-missing\"", 1)) + code, out, errOut = runReload(e) + if code != exitFailure || out != "" || !strings.Contains(errOut, "the broker refused the new config") || + !strings.Contains(errOut, "[key.deploy]") || strings.Count(errOut, "\n") != 1 { + t.Fatalf("reload of a missing key file: exit %d, stdout %q, stderr %q", code, out, errOut) + } + if n := len(listKeys(t, e, c)); n != 1 { + t.Fatalf("%d keys after a refused reload, want 1", n) + } + + // A change that needs a restart: refused. + write(strings.Replace(string(orig), "listen = ", "listen = \"192.0.2.1:7000\"\n# ", 1)) + code, _, errOut = runReload(e) + if code != exitFailure || !strings.Contains(errOut, "listen: is changed, and a reload cannot change it") { + t.Fatalf("reload of a new listen address: exit %d, stderr %q", code, errOut) + } + + // The key is removed: the client sees it at once. + cut := strings.Index(string(orig), "[key.deploy]") + write(string(orig[:cut])) + if code, out, errOut = runReload(e); code != exitOK { + t.Fatalf("reload without the key: exit %d, stdout %q, stderr %q", code, out, errOut) + } + if n := len(listKeys(t, e, c)); n != 0 { + t.Fatalf("%d keys after the key was removed, want 0", n) + } + + // The client gets a new pin: its open connection is refused. + pin, err := transport.GenerateKeyFile(filepath.Join(e.dir, "client2.key")) + if err != nil { + t.Fatal(err) + } + re := regexp.MustCompile(`(?m)^(\[client\.laptop\]\npin = )"[^"]*"`) + write(re.ReplaceAllString(string(orig), `${1}"`+pin.String()+`"`)) + if code, out, errOut = runReload(e); code != exitOK { + t.Fatalf("reload with a new pin: exit %d, stdout %q, stderr %q", code, out, errOut) + } + if a := e.post(c, api.PathKeys, api.KeysRequest{}); a.err == nil && a.status != http.StatusForbidden { + t.Fatalf("the old key of the client got status %d after its pin changed", a.status) + } + if a := e.post(e.httpClient(), api.PathKeys, api.KeysRequest{}); a.err == nil { + t.Fatalf("a new connection with the old key got status %d", a.status) + } + + e.signal(syscall.SIGINT) + e.mustExit(exitOK) + got := eventNames(e.auditEntries()) + if n := countOf(got, "config_reloaded"); n != 3 { + t.Errorf("%d config_reloaded entries, want 3: %q", n, got) + } +} + +// countOf counts s in list. +func countOf(list []string, s string) int { + n := 0 + for _, x := range list { + if x == s { + n++ + } + } + return n +} + +// TestBrokerReloadNoBroker: without a running broker, nodkey broker reload +// sends no signal and exits 1 with the reason: when the status file is +// missing, and when the process in it is not the broker that wrote it. +func TestBrokerReloadNoBroker(t *testing.T) { + t.Parallel() + e := newBrokerEnv(t, brokerOptions{}) + code, out, errOut := runReload(e) + if code != exitFailure || out != "" || !strings.Contains(errOut, "is missing: the broker does not run") { + t.Fatalf("exit %d, stdout %q, stderr %q", code, out, errOut) + } + // A status file of this test process, with another start time: the + // signal would go to the wrong process. + start, err := procStart(os.Getpid()) + if err != nil { + t.Fatal(err) + } + data, err := json.Marshal(reloadStatus{PID: os.Getpid(), Start: start + 1, Result: reloadStarted, + ConfigSHA256: strings.Repeat("0", 64)}) + if err != nil { + t.Fatal(err) + } + status := filepath.Join(filepath.Dir(e.auditPath), reloadStatusName) + writeTestFile(t, status, data, 0o600) + code, _, errOut = runReload(e) + if code != exitFailure || !strings.Contains(errOut, "does not run any more") { + t.Fatalf("exit %d, stderr %q", code, errOut) + } + // A symbolic link is refused. + if err := os.Rename(status, status+".real"); err != nil { + t.Fatal(err) + } + if err := os.Symlink(status+".real", status); err != nil { + t.Fatal(err) + } + if code, _, errOut = runReload(e); code != exitFailure || !strings.Contains(errOut, "cannot read") { + t.Fatalf("a link: exit %d, stderr %q", code, errOut) + } +} + +// TestBrokerReloadUsage: wrong usage is one line and exit 64. +func TestBrokerReloadUsage(t *testing.T) { + for _, args := range [][]string{{"extra"}, {"--timeout", "0s"}, {"--timeout", "2h"}, {"--nope"}} { + var out, errOut bytes.Buffer + code := run(append([]string{"broker", "reload"}, args...), strings.NewReader(""), &out, &errOut) + if code != exitUsage || strings.Count(errOut.String(), "\n") != 1 || out.Len() != 0 { + t.Errorf("%q: exit %d, stderr %q", args, code, errOut.String()) + } + } + var out bytes.Buffer + if code := run([]string{"broker", "reload", "--help"}, strings.NewReader(""), &out, &bytes.Buffer{}); code != exitOK || + !strings.HasPrefix(out.String(), "Usage: nodkey broker reload") { + t.Errorf("--help: exit %d, %q", code, out.String()) + } +} + +// TestBrokerReloadDuringTap: a request that waits for a tap while a reload +// gives its secret another store item is denied after the tap: no value +// is fetched, and the audit log records the deny. +func TestBrokerReloadDuringTap(t *testing.T) { + t.Parallel() + e := startListening(t, brokerOptions{}) + e.stderr.wait(t, lineListening) + c := e.httpClient() + orig, err := os.ReadFile(e.cfgPath) + if err != nil { + t.Fatal(err) + } + res := make(chan brokerAnswer, 1) + go func() { res <- e.post(c, api.PathSecrets, secretsRequest("TOKEN", "github-token")) }() + msgID, requestID := e.tg.waitAsk(1) + other := "00000000-0000-4000-8000-000000000001" + writeTestFile(t, e.cfgPath, []byte(strings.Replace(string(orig), idGitHub, other, 1)), 0o600) + if code, out, errOut := runReload(e); code != exitOK { + t.Fatalf("reload: exit %d, stdout %q, stderr %q", code, out, errOut) + } + for try := 1; ; try++ { + answer := e.tg.waitTapAnswer(e.tg.tap(msgID, "o:"+requestID)) + if !strings.Contains(answer, "Not ready yet") { + break + } + if try == 100 { + t.Fatal("every tap came too early") + } + } + a := <-res + mustStatus(t, a, http.StatusForbidden) + e.stderr.wait(t, "broker: a reload of the config took away what an approved request uses; the request is denied") + e.signal(syscall.SIGINT) + e.mustExit(exitOK) + got := eventNames(e.auditEntries()) + // The request entry is written with its decision, after the tap. + want := []string{"broker_started", "config_reloaded", "request", "decision:approve_once", "denied:not_allowed", + "broker_stopped"} + if strings.Join(got, " ") != strings.Join(want, " ") { + t.Errorf("audit entries:\n%q\nwant:\n%q", got, want) + } +} diff --git a/cmd/nodkey/brokerreload.go b/cmd/nodkey/brokerreload.go new file mode 100644 index 0000000..9a16477 --- /dev/null +++ b/cmd/nodkey/brokerreload.go @@ -0,0 +1,273 @@ +package main + +import ( + "encoding/json" + "errors" + "flag" + "fmt" + "io" + "log/slog" + "os" + "path/filepath" + "time" + + "github.com/code6x/nodkey/internal/broker" + "github.com/code6x/nodkey/internal/claim" + "github.com/code6x/nodkey/internal/config" + "github.com/code6x/nodkey/internal/doctor" +) + +// The reload of the broker config without a restart (DESIGN.md section 14, +// "Reload"). SIGHUP asks the broker to load its config file again. The +// broker writes the result to the status file, next to the audit log. +// nodkey broker reload sends the signal and reads the result from there. + +// reloadStatusName is the name of the status file in the directory of the +// audit log. +const reloadStatusName = "reload-status.json" + +// maxReloadStatus is the largest status file that nodkey broker reload +// reads. +const maxReloadStatus = 64 << 10 + +// The results in the status file. +const ( + reloadStarted = "started" // the broker started; no reload yet + reloadLoaded = "loaded" // the reload took the new config + reloadRefused = "refused" // the reload refused the new config; the old one stays +) + +// reloadStatus is the content of the status file. +type reloadStatus struct { + // PID is the broker's process ID, and Start the start time of the + // process from /proc/PID/stat (field 22). With both, nodkey broker + // reload signals only the broker that wrote the file. + PID int `json:"pid"` + Start uint64 `json:"start"` + // Reloads counts the reloads since the start. It is 0 at the start. + Reloads uint64 `json:"reloads"` + // Result is reloadStarted, reloadLoaded or reloadRefused. + Result string `json:"result"` + // Error says why a reload was refused. It is the broker's own text + // about the config, never a value of a secret. + Error string `json:"error,omitempty"` + // ConfigSHA256 is the SHA-256 of the config file that the broker uses + // now, in hex. + ConfigSHA256 string `json:"config_sha256"` + // GrantsEnded is how many grants the last reload ended. + GrantsEnded int `json:"grants_ended"` + // At is when the broker wrote the file, in RFC 3339. + At string `json:"at"` +} + +// reloadStatusPath returns the path of the status file of cfg. +func reloadStatusPath(cfg *config.Broker) string { + return filepath.Join(filepath.Dir(cfg.AuditLog), reloadStatusName) +} + +// writeReloadStatus writes st to the status file of b: to a new file first, +// then it renames it, so a reader never sees half a file. An error is +// logged: the broker goes on, and nodkey broker reload waits in vain and +// says so. +func (b *brokerParts) writeReloadStatus(st reloadStatus) { + st.PID, st.Start = os.Getpid(), b.procStart + st.At = time.Now().UTC().Format(time.RFC3339) + data, err := json.Marshal(st) + if err == nil { + err = writeFileAtomic(reloadStatusPath(b.cfg), append(data, '\n')) + } + if err != nil { + b.log.Error("nodkey broker: cannot write the reload status file; nodkey broker reload cannot see the "+ + "result; check the directory of the audit log", slog.String("error", err.Error())) + } +} + +// writeFileAtomic writes data to path with mode 0600 through a new file in +// the same directory, then renames it to path. +func writeFileAtomic(path string, data []byte) error { + tmp := path + ".new" + _ = os.Remove(tmp) + f, err := os.OpenFile(tmp, os.O_WRONLY|os.O_CREATE|os.O_EXCL|noFollow, 0o600) //nolint:gosec // G304: the status file next to the audit log of the config. + if err != nil { + return err + } + _, werr := f.Write(data) + if cerr := f.Close(); werr == nil { + werr = cerr + } + if werr == nil { + werr = os.Rename(tmp, path) + } + if werr != nil { + _ = os.Remove(tmp) + } + return werr +} + +// reload loads the config file again and makes it the config of the +// broker, after the checks of a start and ReloadFixed. On any error it +// changes nothing, logs why, and the broker goes on with the config it +// has. It writes the result to the status file. +func (b *brokerParts) reload(path string) { + b.reloads++ + b.log.Info("nodkey broker: reloading the config, as SIGHUP asked", slog.String("config", path)) + cfg, ended, err := b.loadNew(path) + if err != nil { + b.log.Error("nodkey broker: refused the new config; the broker goes on with the config it has", + slog.String("error", err.Error())) + b.writeReloadStatus(reloadStatus{Reloads: b.reloads, Result: reloadRefused, Error: err.Error(), + ConfigSHA256: b.cfg.Sum}) + return + } + b.cfg = cfg + b.log.Info("nodkey broker: the new config is loaded; the grants that it still allows stay", + slog.String("config_sha256", cfg.Sum), slog.Int("grants_ended", ended)) + b.writeReloadStatus(reloadStatus{Reloads: b.reloads, Result: reloadLoaded, ConfigSHA256: cfg.Sum, + GrantsEnded: ended}) +} + +// loadNew loads the config at path and the SSH keys that it names, as a +// start does, and hands them to the server and the service. It returns the +// new config and how many grants ended. +func (b *brokerParts) loadNew(path string) (*config.Broker, int, error) { + if b.svc == nil || b.srv == nil { + return nil, 0, errors.New("the broker has not started") + } + cfg, err := config.LoadBroker(path) // it runs CheckFiles + if err != nil { + return nil, 0, err + } + if err := b.cfg.ReloadFixed(cfg); err != nil { + return nil, 0, fmt.Errorf("config %s: %w", path, err) + } + keys, err := loadSSHKeys(cfg, config.ReadPrivateFile) + if err != nil { + return nil, 0, err + } + // The server first: a removed key gets no new request from now on. A + // new client is refused by the service until it has the new config. + if err := b.srv.SetClients(cfg.Clients); err != nil { + return nil, 0, fmt.Errorf("config %s: %w", path, err) + } + ended, err := b.svc.Reload(cfg, keys, cfg.Sum) + if err != nil && !errors.Is(err, broker.ErrNotRecorded) { + // Nothing changed in the service: the server gets the old clients + // back. They passed this check at the start or the last reload. + if rerr := b.srv.SetClients(b.cfg.Clients); rerr != nil { + b.log.Error("nodkey broker: cannot give the server its old clients back", slog.String("error", rerr.Error())) + } + return nil, 0, err + } + if err != nil { + b.log.Error("nodkey broker: the new config is in use, but its audit entry is missing", + slog.String("error", err.Error())) + } + return cfg, ended, nil +} + +const brokerReloadUsage = `Usage: nodkey broker reload [--config FILE] [--timeout DURATION] + +Ask the running broker to load its config again, without a restart, and +wait for the result. Run it as root, or as the broker's user, after you +changed the config. The broker loads the config file and the files that +it names, and checks them as at a start. A reload can change the +clients, the secrets, the SSH keys and known_hosts. A change of any +other part needs a restart: the reload refuses it. + +When the new config is good, the broker uses it at once. Grants that the +new config still allows stay, with the same uses left and the same end. +Grants for a removed or changed client, secret or key end. When the new +config is not good, the broker goes on with the old one, and this +command prints why. + + --config FILE the broker config (default /etc/nodkey/broker.toml) + --timeout DURATION how long to wait for the result (default 60s) + +Exit codes: 0 the broker loaded the config that is in FILE now, 1 the +broker refused it, or did not answer in time, or the result cannot be +read, 64 wrong usage. +` + +// reloadPoll is how often nodkey broker reload reads the status file. +var reloadPoll = 200 * time.Millisecond + +// runBrokerReload runs nodkey broker reload. +func runBrokerReload(args []string, stdout, stderr io.Writer) int { + fs := flag.NewFlagSet("nodkey broker reload", flag.ContinueOnError) + fs.SetOutput(io.Discard) + path := fs.String("config", defaultBrokerConfig, "the broker config") + timeout := fs.Duration("timeout", 60*time.Second, "how long to wait") + if err := fs.Parse(args); err != nil { + if errors.Is(err, flag.ErrHelp) { + _, _ = io.WriteString(stdout, brokerReloadUsage) + return exitOK + } + return parseErrorCode(stderr, "broker reload", err) + } + if fs.NArg() > 0 { + return usageFail(stderr, "broker reload", + fmt.Sprintf("unexpected argument %.64q; it takes only --config FILE and --timeout DURATION", fs.Arg(0))) + } + if *timeout <= 0 || *timeout > time.Hour { + return usageFail(stderr, "broker reload", "--timeout must be more than 0 and at most 1h, such as 60s") + } + fail := func(msg string) int { + _, _ = fmt.Fprintf(stderr, "nodkey broker reload: %s\n", msg) + return exitFailure + } + // Only the path of the audit log and the SHA-256 are needed here: the + // broker checks the config itself. Root may not pass the checks of the + // files that are for the broker's user. + if os.Getenv("CREDENTIALS_DIRECTORY") == "" { + // Outside the unit it is not set. Only this process sees the + // value, and no file in it is read here (as in nodkey doctor + // broker). + _ = os.Setenv("CREDENTIALS_DIRECTORY", "/run/credentials/"+doctor.Unit) + } + cfg, err := config.ReadBroker(*path) + if err != nil { + return fail(err.Error() + "; fix the config, then run nodkey broker reload again") + } + statusPath := reloadStatusPath(cfg) + before, uid, err := readReloadStatus(statusPath) + if err != nil { + return fail(err.Error()) + } + if err := signalBroker(before, uid); err != nil { + return fail(err.Error()) + } + deadline := time.Now().Add(*timeout) + for { + time.Sleep(reloadPoll) + st, _, err := readReloadStatus(statusPath) + switch { + case err == nil && (st.PID != before.PID || st.Start != before.Start): + return fail("the broker restarted during the reload; check that it runs, then run nodkey broker " + + "reload again") + case err == nil && st.Reloads > before.Reloads: + return reloadResult(st, cfg.Sum, *path, stdout, fail) + case time.Now().After(deadline): + return fail(fmt.Sprintf("the broker did not write the result to %s within %s; read its journal "+ + "(journalctl -u nodkey)", statusPath, *timeout)) + } + } +} + +// reloadResult prints the result of a reload and returns the exit code. +// want is the SHA-256 of the config file at path when the command began. +func reloadResult(st reloadStatus, want, path string, stdout io.Writer, fail func(string) int) int { + switch { + case st.Result == reloadRefused: + return fail("the broker refused the new config and goes on with the old one: " + claim.Line(st.Error, 1000) + + "; fix the config, then run nodkey broker reload again") + case st.Result != reloadLoaded: + return fail("the status file of the broker has a result that is not known; read its journal " + + "(journalctl -u nodkey)") + case st.ConfigSHA256 != want: + return fail("the broker loaded a config that is not the one in " + path + " now: the file changed " + + "during the reload; run nodkey broker reload again") + } + _, _ = fmt.Fprintf(stdout, "nodkey broker reload: the broker loaded the new config (sha256 %s); %d grant(s) "+ + "ended, the others stay\n", st.ConfigSHA256, st.GrantsEnded) + return exitOK +} diff --git a/cmd/nodkey/brokerreload_linux.go b/cmd/nodkey/brokerreload_linux.go new file mode 100644 index 0000000..5f4692b --- /dev/null +++ b/cmd/nodkey/brokerreload_linux.go @@ -0,0 +1,114 @@ +package main + +import ( + "encoding/json" + "errors" + "fmt" + "io" + "os" + "strconv" + "strings" + "syscall" +) + +// noFollow makes an open fail on a symbolic link. +const noFollow = syscall.O_NOFOLLOW + +// procStart returns the start time of the process pid, field 22 of +// /proc/PID/stat, in clock ticks after the boot. +func procStart(pid int) (uint64, error) { + data, err := os.ReadFile("/proc/" + strconv.Itoa(pid) + "/stat") + if err != nil { + return 0, err + } + // The second field, the name, is in parentheses and may hold spaces; + // the fields after the last ")" have none. + s := string(data) + i := strings.LastIndexByte(s, ')') + if i < 0 { + return 0, errors.New("the stat file of the process has no name field") + } + f := strings.Fields(s[i+1:]) + // f[0] is field 3; field 22 is f[19]. + if len(f) < 20 { + return 0, errors.New("the stat file of the process is too short") + } + return strconv.ParseUint(f[19], 10, 64) +} + +// readReloadStatus reads the status file at path. It must be a regular +// file, not a symbolic link, and not owned by root: the broker never runs +// as root. It returns the owner too. +func readReloadStatus(path string) (reloadStatus, uint32, error) { + var st reloadStatus + f, err := os.OpenFile(path, os.O_RDONLY|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0) //nolint:gosec // G304: the status file next to the audit log of the config. + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return st, 0, fmt.Errorf("%s is missing: the broker does not run, or it is older than the reload "+ + "command; start the broker (systemctl start nodkey), as root", path) + } + return st, 0, fmt.Errorf("cannot read %s: %w", path, err) + } + defer func() { _ = f.Close() }() + fi, err := f.Stat() + if err != nil { + return st, 0, fmt.Errorf("cannot read %s: %w", path, err) + } + sys, ok := fi.Sys().(*syscall.Stat_t) + if !fi.Mode().IsRegular() || !ok || sys.Uid == 0 { + return st, 0, fmt.Errorf("%s is not a regular file of the broker's user; remove it, then restart the "+ + "broker (systemctl restart nodkey), as root", path) + } + data, err := io.ReadAll(io.LimitReader(f, maxReloadStatus+1)) + if err == nil && len(data) > maxReloadStatus { + err = errors.New("the file is too large") + } + if err == nil { + err = json.Unmarshal(data, &st) + } + if err != nil || st.PID <= 1 || st.Start == 0 { + return reloadStatus{}, 0, fmt.Errorf("%s does not hold a status of the broker; restart the broker "+ + "(systemctl restart nodkey), as root", path) + } + return st, sys.Uid, nil +} + +// signalBroker sends SIGHUP to the broker of st, after it checked that the +// process is the one that wrote the status file: it runs as uid, the owner +// of the file, and it has the same start time. So a stale file never makes +// the signal go to another process. +func signalBroker(st reloadStatus, uid uint32) error { + fi, err := os.Stat("/proc/" + strconv.Itoa(st.PID)) + start, serr := procStart(st.PID) + sys, ok := statOwner(fi) + if err != nil || serr != nil || !ok || sys != uid || start != st.Start { + return errors.New("the broker that wrote the status file does not run any more; start it " + + "(systemctl start nodkey), as root") + } + if err := syscall.Kill(st.PID, syscall.SIGHUP); err != nil { + return fmt.Errorf("cannot send SIGHUP to the broker: %w; run nodkey broker reload as root, or as the "+ + "broker's user", err) + } + return nil +} + +// statOwner returns the owner of fi. +func statOwner(fi os.FileInfo) (uint32, bool) { + if fi == nil { + return 0, false + } + sys, ok := fi.Sys().(*syscall.Stat_t) + if !ok { + return 0, false + } + return sys.Uid, true +} + +// ownStart returns the start time of this process, for the status file. +func ownStart() uint64 { + start, err := procStart(os.Getpid()) + if err != nil { + return 0 + } + return start +} diff --git a/cmd/nodkey/brokerreload_other.go b/cmd/nodkey/brokerreload_other.go new file mode 100644 index 0000000..cb54b4c --- /dev/null +++ b/cmd/nodkey/brokerreload_other.go @@ -0,0 +1,18 @@ +//go:build !linux + +package main + +import "errors" + +// noFollow is 0 here: the broker runs only on Linux. +const noFollow = 0 + +var errReloadLinux = errors.New("nodkey broker reload works only on Linux, where the broker runs") + +func readReloadStatus(string) (reloadStatus, uint32, error) { + return reloadStatus{}, 0, errReloadLinux +} + +func signalBroker(reloadStatus, uint32) error { return errReloadLinux } + +func ownStart() uint64 { return 0 } diff --git a/cmd/nodkey/main.go b/cmd/nodkey/main.go index 90f4a9d..bef6110 100644 --- a/cmd/nodkey/main.go +++ b/cmd/nodkey/main.go @@ -96,7 +96,7 @@ func run(args []string, stdin io.Reader, stdout, stderr io.Writer) int { name, rest := fs.Arg(0), fs.Args()[1:] switch name { case "broker": - return runBroker(rest, stderr) + return runBroker(rest, stdout, stderr) case "try": return runTry(rest, stdin, stdout, stderr) case "keygen": diff --git a/docs/setup.md b/docs/setup.md index 1d8c89a..57eb4fd 100644 --- a/docs/setup.md +++ b/docs/setup.md @@ -1055,7 +1055,8 @@ of an agent machine: the agent can change that file. Keep all host keys of a host in the file: ssh may use any of them. When a host gets a new host key, check its fingerprint in the same way, -change the file, and restart the broker. +change the file, and reload the config: `nodkey broker reload`, as root +([section 10](#change-the-config-without-a-restart)). ## 7. The config file @@ -1597,13 +1598,46 @@ first anchor. When something is wrong, the broker logs why ([section 14](#14-when-something-does-not-work)). A restart of the broker ends all grants, and denies the requests that -wait for a tap. The config is read only at the start, so restart the -broker after each change of the config or of `known_hosts`: +wait for a tap: ```sh systemctl restart nodkey ``` +### Change the config without a restart + +After most changes of the config or of `known_hosts`, a restart is not +needed. Run this as root on the broker host: + +```sh +nodkey broker reload +``` + +It asks the broker to load the config again (with `SIGHUP`), and waits +for the result. The broker checks the new config as at a start. + +- When the new config is good, the broker uses it at once, and the + command exits with 0. Grants that the new config still allows stay, + with the same uses left and the same end. Grants of a client, secret or + key that you removed or changed end at once. A removed client, or a + client with a new pin, cannot go on with a connection that it opened + before. +- When the new config is not good, the broker goes on with the old one. + The command prints why, and exits with 1. The journal says why too. Fix + the config, and run the command again. + +A reload can change `[client.*]`, `[secret.*]`, `[key.*]` and +`known_hosts`. A change of `listen`, `tls_key`, `audit_log`, +`[telegram]`, `[ntfy]`, `[updates]`, `[preapprove]`, `[standing]` or a +`[store.*]` section needs a restart: the reload refuses it, and says so. +A new key file in `${CREDENTIALS_DIRECTORY}` also needs a restart, because +systemd loads the credentials only at the start. + +The broker writes the result of each reload to `reload-status.json`, next +to the audit log (`/var/lib/nodkey/reload-status.json`). Each reload that +the broker took has a `config_reloaded` entry in the audit log +([DESIGN.md section 14](../DESIGN.md#reload)). + Check the setup with `sudo nodkey doctor broker` ([section 14](#14-when-something-does-not-work)). @@ -1621,8 +1655,9 @@ nodkey keygen --out ~/.config/nodkey/client.key `nodkey keygen` prints the pin of the new key. On the broker host, add a section for the client to `/etc/nodkey/broker.toml`, add its name to the -`clients` lists of the secrets and keys that it may use, and restart the -broker: +`clients` lists of the secrets and keys that it may use, and reload the +config with `nodkey broker reload`, as root +([section 10](#change-the-config-without-a-restart)): ```toml [client.laptop] @@ -1633,7 +1668,7 @@ The agent can copy this key. With it, someone can only ask, like the agent (THREAT-MODEL.md T11). They get what this client gets: what needs a tap still needs your tap, but a grant or a "never ask" rule of this client works for them too. To block a stolen key, remove its pin from -the broker config, and restart the broker. Each machine needs its own +the broker config, and run `nodkey broker reload` as root. Each machine needs its own key: the broker refuses two clients with the same pin. Write the client config, `~/.config/nodkey/client.toml`, with the @@ -2222,14 +2257,17 @@ put the new pin into `broker_pin` on each client, and restart ### A new key for a client, or a client that must go To give a client a new key, make it with `nodkey keygen` in a new file, -put its pin into the `[client.NAME]` section, restart the broker, and +put its pin into the `[client.NAME]` section, run `nodkey broker reload` +as root, and change `tls_key` in the client config. Remove the old key file. Restart `nodkey ssh-agent` too: it reads the config and the key only when it starts. To block a client, for example after a strange request, remove its `[client.NAME]` section, and its name from every `clients` list, and -restart the broker. Its key no longer works. +run `nodkey broker reload` as root. Its key no longer works, also on a +connection that is open, and its grants end. The grants of the other +clients stay. ### A new bot token diff --git a/internal/approval/approval.go b/internal/approval/approval.go index 8de1ce3..f3a61e7 100644 --- a/internal/approval/approval.go +++ b/internal/approval/approval.go @@ -36,6 +36,7 @@ import ( "io" "log/slog" "sync" + "sync/atomic" "time" "github.com/code6x/nodkey/internal/audit" @@ -95,7 +96,7 @@ type Options struct { // Core decides requests. Make it with New, and call Run. It is safe for // use by many goroutines. type Core struct { - cfg *config.Broker + cfgp atomic.Pointer[config.Broker] // the config; Reload changes it approver Approver clock Clock logger *slog.Logger @@ -154,7 +155,7 @@ func New(cfg *config.Broker, a Approver, opts Options) (*Core, error) { } stopCtx, stopRuns := context.WithCancel(context.Background()) c := &Core{ - cfg: cfg, + approver: a, clock: opts.Clock, logger: opts.Logger, @@ -168,6 +169,7 @@ func New(cfg *config.Broker, a Approver, opts Options) (*Core, error) { grantWake: make(chan struct{}, 1), uses: make(map[useKey]dayUses), } + c.cfgp.Store(cfg) if c.clock == nil { c.clock = realClock{} } @@ -216,6 +218,9 @@ func (c *Core) Run(ctx context.Context) error { return ErrStopped } +// conf returns the config that the core uses now. Reload changes it. +func (c *Core) conf() *config.Broker { return c.cfgp.Load() } + // start marks that Run has started. It reports false if Run ran before. func (c *Core) start() bool { c.mu.Lock() @@ -258,10 +263,10 @@ func (c *Core) callContext(ctx context.Context, d time.Duration) (context.Contex // of secrets. func (c *Core) timeout(k audit.Kind) time.Duration { if k == audit.KindSSHLogin { - _, ssh := c.cfg.Timeouts() + _, ssh := c.conf().Timeouts() return ssh } - t, _ := c.cfg.Timeouts() + t, _ := c.conf().Timeouts() return t } @@ -269,17 +274,17 @@ func (c *Core) timeout(k audit.Kind) time.Duration { // that is the configured user ID. With [ntfy], the ntfy approver has // checked the value of the request already, and a has no user ID. func (c *Core) fromApprover(a Answer) bool { - if c.cfg.Ntfy != nil { + if c.conf().Ntfy != nil { return a.UserID == 0 } - return a.UserID > 0 && a.UserID == c.cfg.Telegram.UserID + return a.UserID > 0 && a.UserID == c.conf().Telegram.UserID } // channel returns the entry text that says which channel an answer came // by, or "" for Telegram, where the user ID says it. func (c *Core) channel() string { - if c.cfg.Ntfy != nil { - return audit.ChannelNtfyPrefix + c.cfg.Ntfy.AnswerTopic + if c.conf().Ntfy != nil { + return audit.ChannelNtfyPrefix + c.conf().Ntfy.AnswerTopic } return "" } diff --git a/internal/approval/check.go b/internal/approval/check.go index f903a17..242e59a 100644 --- a/internal/approval/check.go +++ b/internal/approval/check.go @@ -34,8 +34,9 @@ var remoteUserPattern = regexp.MustCompile(`^[A-Za-z0-9._][A-Za-z0-9._-]{0,31}$` // - fields of another kind of request, such as a host key in a secrets // request. func (c *Core) check(r *Request, p *pending) bool { + p.cfg = c.conf() // one config for all the checks, also when a reload comes ok := true - if _, known := c.cfg.Clients[r.Client]; known { + if _, known := p.cfg.Clients[r.Client]; known { p.client = r.Client p.subject.Client = r.Client } else { @@ -99,7 +100,7 @@ func (c *Core) checkSecrets(r *Request, p *pending) bool { continue // two variables for one secret } seen[name] = true - s, known := c.cfg.Secrets[name] + s, known := p.cfg.Secrets[name] if !known || p.client == "" || !slices.Contains(s.Clients, p.client) { ok = false continue @@ -132,7 +133,7 @@ func (c *Core) checkLogin(r *Request, p *pending) bool { } p.hostKey = string(r.HostKey.Marshal()) p.subject.HostKey = ssh.FingerprintSHA256(r.HostKey) - host, known := c.cfg.KnownHosts.NameFor(r.HostKey) + host, known := p.cfg.KnownHosts.NameFor(r.HostKey) if !known || !keyOK || (len(k.Hosts) > 0 && !slices.Contains(k.Hosts, host)) { return false } @@ -201,7 +202,7 @@ func (c *Core) addGrantHosts(names []string, k config.Key, keyOK bool, p *pendin return false, false } for _, h := range hosts { - keys := c.cfg.KnownHosts.KeysFor(h) + keys := p.cfg.KnownHosts.KeysFor(h) if len(keys) == 0 || !keyOK || (len(k.Hosts) > 0 && !slices.Contains(k.Hosts, h)) { ok = false continue @@ -222,7 +223,7 @@ func (c *Core) addGrantHosts(names []string, k config.Key, keyOK bool, p *pendin // rule cannot have. The number of uses and the span must be inside the // limits of [preapprove]; a limit that is not set (0) allows nothing. func (c *Core) checkPreapprove(r *Request, p *pending) bool { - lim := c.cfg.Preapprove + lim := p.cfg.Preapprove ok := r.HostKey == nil && r.For == 0 allowed := func(rule config.Rule, clients []string) bool { return rule.Preapprove && rule.Ask != config.AskNever && validRule(rule, clients) @@ -237,7 +238,7 @@ func (c *Core) checkPreapprove(r *Request, p *pending) bool { continue } seen[name] = true - s, known := c.cfg.Secrets[name] + s, known := p.cfg.Secrets[name] if !known || p.client == "" || !slices.Contains(s.Clients, p.client) || !allowed(s.Rule, s.Clients) { ok = false continue @@ -292,7 +293,7 @@ func (c *Core) checkPreapprove(r *Request, p *pending) bool { // checkKey checks that the key exists, that the client may use it and // that its rule is valid. It adds the key to p. func (c *Core) checkKey(name string, p *pending) (config.Key, bool) { - k, known := c.cfg.Keys[name] + k, known := p.cfg.Keys[name] if !known || p.client == "" || !slices.Contains(k.Clients, p.client) || !validRule(k.Rule, k.Clients) { return config.Key{}, false } diff --git a/internal/approval/decide.go b/internal/approval/decide.go index 4ee4f98..aa1ab8b 100644 --- a/internal/approval/decide.go +++ b/internal/approval/decide.go @@ -34,6 +34,9 @@ type item struct { // pending is one request inside Decide. type pending struct { + // cfg is the config that the checks of the request used (Core.conf at + // the start of check). + cfg *config.Broker id, code string client string kind audit.Kind @@ -237,12 +240,12 @@ func (c *Core) begin(p *pending) *Result { // The grants that are approved and not ended, and the requests // that wait for a tap (StartGrants checks the first again). if p.kind == audit.KindStanding { - if c.standingCount(p.client)+cs.standingPending >= c.cfg.Standing.MaxGrants { + if c.standingCount(p.client)+cs.standingPending >= c.conf().Standing.MaxGrants { return c.denied(p, audit.CauseLimit) } cs.standingPending++ } else { - if c.scheduledCount(p.client)+cs.preapproving >= c.cfg.Preapprove.MaxGrants { + if c.scheduledCount(p.client)+cs.preapproving >= c.conf().Preapprove.MaxGrants { return c.denied(p, audit.CauseLimit) } cs.preapproving++ @@ -703,6 +706,7 @@ func (c *Core) newResult(p *pending) *Result { Message: p.text, Subject: s, core: c, + cfg: p.cfg, at: c.clock.Wall(), } } diff --git a/internal/approval/export_test.go b/internal/approval/export_test.go index 7a5ca45..e5c77c9 100644 --- a/internal/approval/export_test.go +++ b/internal/approval/export_test.go @@ -37,9 +37,9 @@ func (c *Core) Live() (ids, codes, waiting int) { func (c *Core) Uses(client string, key bool, name string, at time.Time) int { c.mu.Lock() defer c.mu.Unlock() - rule := c.cfg.Secrets[name].Rule + rule := c.conf().Secrets[name].Rule if key { - rule = c.cfg.Keys[name].Rule + rule = c.conf().Keys[name].Rule } return c.uses[useKey{client, ref{key: key, name: name}}].on(useDay(rule, at)) } diff --git a/internal/approval/grants.go b/internal/approval/grants.go index a12d3cd..dba4080 100644 --- a/internal/approval/grants.go +++ b/internal/approval/grants.go @@ -725,11 +725,11 @@ func (c *Core) startScheduled(r *Result, now moment) { if g.standing() { // A Deny of another request does not touch a standing grant // (DESIGN.md section 8). - if c.standingCount(g.client) >= c.cfg.Standing.MaxGrants { + if c.standingCount(g.client) >= c.conf().Standing.MaxGrants { return } } else { - if c.scheduledCount(g.client) >= c.cfg.Preapprove.MaxGrants { + if c.scheduledCount(g.client) >= c.conf().Preapprove.MaxGrants { return } for _, ref := range g.refs { diff --git a/internal/approval/reload.go b/internal/approval/reload.go new file mode 100644 index 0000000..11ad55f --- /dev/null +++ b/internal/approval/reload.go @@ -0,0 +1,87 @@ +package approval + +import ( + "github.com/code6x/nodkey/internal/audit" + "github.com/code6x/nodkey/internal/config" +) + +// Reload makes next the config of the core (DESIGN.md section 14, +// "Reload"). next must come from config.LoadBroker, and must pass +// ReloadFixed against the config that the core has now; nothing may change +// it later. sameKey reports whether the key file of the [key.*] section +// name holds the same key as before; the core cannot read key files. +// +// Every grant that the new config does not allow in the same way ends at +// once: its client, a secret or the key is gone or changed, the client +// lost access to it, or a host of a key grant is no longer allowed. The +// other grants stay, with the same uses left and the same end. The entries +// of the scheduled and standing grants that end (cause config_changed) go +// to the function of OnGrantEnded, and their messages lose the Revoke +// button. +// +// A request that the core checked with the old config, and that waits +// for a tap, is still decided, but its approval starts no grant. The +// broker checks it again before it hands out a value or signs. +// +// Reload returns how many grants ended. +func (c *Core) Reload(next *config.Broker, sameKey func(name string) bool) int { + c.mu.Lock() + defer c.mu.Unlock() + old := c.conf() + c.cfgp.Store(next) + n := 0 + c.queueEnded(c.endWhere(func(g *grant) audit.EndCause { + if keeps(old, next, sameKey, g) { + return "" + } + n++ + return audit.EndConfig + })) + return n +} + +// Config returns the config that the core used to check the request of +// r. The broker compares it with the config of now before it hands out +// anything (DESIGN.md section 14, "Reload"). +func (r *Result) Config() *config.Broker { + if r == nil { + return nil + } + return r.cfg +} + +// SkippedByReload reports whether StartGrants started nothing because a +// reload came after the core checked the request of r. +func (r *Result) SkippedByReload() bool { + if r == nil || r.core == nil { + return false + } + r.core.mu.Lock() + defer r.core.mu.Unlock() + return r.reloaded +} + +// keeps reports whether the grant g of the config old is allowed in the +// same way by the config next. +func keeps(old, next *config.Broker, sameKey func(string) bool, g *grant) bool { + if !old.SameClient(next, g.client) || len(g.refs) == 0 { + return false + } + for _, r := range g.refs { + if !r.key { + if !old.SameSecret(next, r.name, g.client) { + return false + } + continue + } + if sameKey == nil || !sameKey(r.name) || !old.SameKey(next, r.name, g.client) { + return false + } + for _, hk := range g.hostKeys { + if !next.KeyHost(r.name, []byte(hk)) { + return false + } + } + } + return true +} diff --git a/internal/approval/reload_test.go b/internal/approval/reload_test.go new file mode 100644 index 0000000..c5c1e0b --- /dev/null +++ b/internal/approval/reload_test.go @@ -0,0 +1,181 @@ +package approval_test + +import ( + "maps" + "testing" + "testing/synctest" + "time" + + "github.com/code6x/nodkey/internal/audit" + "github.com/code6x/nodkey/internal/config" +) + +// copyConfig returns a copy of cfg whose maps can change without changing +// cfg, as a reload loads a new config. +func copyConfig(cfg *config.Broker) *config.Broker { + next := *cfg + next.Clients = maps.Clone(cfg.Clients) + next.Secrets = maps.Clone(cfg.Secrets) + next.Keys = maps.Clone(cfg.Keys) + return &next +} + +// sameKeys says that every key file holds the key it held before. +func sameKeys(string) bool { return true } + +// TestReloadKeepsMatchingGrants: a reload keeps a scheduled grant that the +// new config allows in the same way, with the same uses left and the same +// end, also when another client is added to its secret. A grant for a +// secret that the new config removed ends at once, with cause +// config_changed and its uses left. +func TestReloadKeepsMatchingGrants(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + e := preEnv(t) + defer e.stop() + var rec endedRecorder + e.core.OnGrantEnded(rec.add) + a := e.grant(preSecrets("laptop", 5, 0, time.Hour, "github-token"), 1) + b := e.grant(preSecrets("laptop", 2, 0, time.Hour, "npm-token"), 2) + req := secretsReq("laptop", "github-token") + e.uses(req, a.GrantID) + + next := copyConfig(e.cfg) + delete(next.Secrets, "npm-token") + s := next.Secrets["github-token"] + s.Clients = append(s.Clients[:len(s.Clients):len(s.Clients)], "ci") + next.Secrets["github-token"] = s + if n := e.core.Reload(next, sameKeys); n != 1 { + t.Fatalf("Reload ended %d grants, want 1", n) + } + synctest.Wait() + got := rec.all() + if len(got) != 1 || got[0].GrantID != b.GrantID || got[0].Cause != audit.EndConfig || got[0].UsesLeft != 2 { + t.Fatalf("ended %+v; want the npm-token grant, with cause config_changed and 2 uses left", got) + } + // The same uses left: 4 of 5. + for range 4 { + e.uses(req, a.GrantID) + } + e.needsMessage(req, 3) + // The same end: a new grant ends at the old time. + c := e.grant(preSecrets("laptop", 5, 0, time.Hour, "github-token"), 4) + e.clock.Advance(time.Hour - time.Nanosecond) + e.uses(req, c.GrantID) + e.clock.Advance(time.Nanosecond) + e.needsMessage(req, 5) + }) +} + +// TestReloadEndsChangedGrants: a reload ends each grant whose client has a +// new pin, whose secret has another store item or rule, or whose key may +// no longer log in to the host of the grant. Each such request needs a +// message again. A grant that still matches stays. +func TestReloadEndsChangedGrants(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + e := newEnv(t, nil) + defer e.stop() + h := e.hosts + e.approveWindow(secretsReq("laptop", "github-token"), 1) + e.approveWindow(secretsReq("build", "github-token"), 2) + e.approveWindow(secretsReq("laptop", "npm-token"), 3) + e.approveWindow(loginReq("laptop", "web", h.web1, "deploy"), 4) + e.approveWindow(loginReq("laptop", "web", h.web2, "deploy"), 5) + + next := copyConfig(e.cfg) + next.Clients["build"] = config.Client{Name: "build", Pin: config.Pin{1}} // a new pin + s := next.Secrets["npm-token"] + s.ID = "other-item" + next.Secrets["npm-token"] = s + k := next.Keys["web"] + k.Hosts = []string{"web-1"} + next.Keys["web"] = k + if n := e.core.Reload(next, sameKeys); n != 3 { + t.Fatalf("Reload ended %d grants, want 3", n) + } + checkAllowed(t, e.now(secretsReq("laptop", "github-token"))) + checkAllowed(t, e.now(loginReq("laptop", "web", h.web1, "deploy"))) + e.needsTap(secretsReq("build", "github-token"), 6) + e.needsTap(secretsReq("laptop", "npm-token"), 7) + // web-2 is no longer a host of the key: the core refuses it. + checkDenied(t, e.now(loginReq("laptop", "web", h.web2, "deploy")), audit.CauseNotAllowed) + }) +} + +// TestReloadEndsGrantsOfChangedKeyFile: a key file that holds another key +// after a reload ends the grants of its key. +func TestReloadEndsGrantsOfChangedKeyFile(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + e := newEnv(t, nil) + defer e.stop() + h := e.hosts + e.approveWindow(loginReq("laptop", "deploy", h.web1, "deploy"), 1) + e.approveWindow(loginReq("laptop", "web", h.web1, "deploy"), 2) + n := e.core.Reload(copyConfig(e.cfg), func(name string) bool { return name != "deploy" }) + if n != 1 { + t.Fatalf("Reload ended %d grants, want 1", n) + } + checkAllowed(t, e.now(loginReq("laptop", "web", h.web1, "deploy"))) + e.needsTap(loginReq("laptop", "deploy", h.web1, "deploy"), 3) + }) +} + +// TestReloadRemovedClient: a client that the new config does not have +// loses its grants and is refused at once. +func TestReloadRemovedClient(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + e := newEnv(t, nil) + defer e.stop() + e.approveWindow(secretsReq("build", "github-token"), 1) + next := copyConfig(e.cfg) + delete(next.Clients, "build") + s := next.Secrets["github-token"] + s.Clients = []string{"laptop"} + next.Secrets["github-token"] = s + if n := e.core.Reload(next, sameKeys); n != 1 { + t.Fatalf("Reload ended %d grants, want 1", n) + } + e.refuses(secretsReq("build", "github-token"), audit.CauseNotAllowed) + }) +} + +// TestReloadDuringWait: a request that waits for a tap during a reload is +// still decided, but its "Approve N min" starts no grant: the core checked +// it with the old config. +func TestReloadDuringWait(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + e := newEnv(t, nil) + defer e.stop() + ch := e.decide(secretsReq("laptop", "github-token")) + mustWait(t, ch) + e.asked(1) + e.core.Reload(copyConfig(e.cfg), sameKeys) + e.tap(1, audit.AnswerApproveWindow) + res := mustResult(t, ch) + checkAllowed(t, res) + res.StartGrants() + if !res.SkippedByReload() { + t.Error("SkippedByReload is false") + } + e.needsTap(secretsReq("laptop", "github-token"), 2) + }) +} + +// TestReloadDuringWaitPreapprove: a scheduled grant approved after a +// reload does not start. +func TestReloadDuringWaitPreapprove(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + e := preEnv(t) + defer e.stop() + ch := e.decide(preSecrets("laptop", 2, 0, time.Hour, "github-token")) + mustWait(t, ch) + e.asked(1) + e.core.Reload(copyConfig(e.cfg), sameKeys) + e.tap(1, audit.AnswerApproveOnce) + res := mustResult(t, ch) + checkAllowed(t, res) + res.StartGrants() + if res.GrantStarted() { + t.Fatal("a grant approved after a reload started") + } + }) +} diff --git a/internal/approval/renew.go b/internal/approval/renew.go index b2b31dd..314c726 100644 --- a/internal/approval/renew.go +++ b/internal/approval/renew.go @@ -55,7 +55,7 @@ func (c *Core) OnRenewed(f func(audit.Decision) error) { // was approved at now: warnDays before its end. A grant that is not longer // than that gets no warning (zero). func (c *Core) warnTime(g *grant, now time.Time) time.Time { - at := g.end.wall.Add(-time.Duration(c.cfg.Standing.WarnDays) * 24 * time.Hour) + at := g.end.wall.Add(-time.Duration(c.conf().Standing.WarnDays) * 24 * time.Hour) if !at.After(now) { return time.Time{} } @@ -158,7 +158,7 @@ func (c *Core) renew(a Answer) error { case c.onRenewed == nil: return ErrNotAllowed } - lim := c.cfg.Standing + lim := c.conf().Standing // The new grant takes the place of g: g is counted, n is not. if c.standingCount(g.client) > lim.MaxGrants { return ErrNotAllowed diff --git a/internal/approval/request.go b/internal/approval/request.go index 62c271b..89a14b0 100644 --- a/internal/approval/request.go +++ b/internal/approval/request.go @@ -6,6 +6,7 @@ import ( "golang.org/x/crypto/ssh" "github.com/code6x/nodkey/internal/audit" + "github.com/code6x/nodkey/internal/config" ) // Request is one request, after the broker's own checks (DESIGN.md @@ -114,9 +115,11 @@ type Result struct { GrantFrom, GrantUntil time.Time core *Core - at time.Time // when the request was decided, on the wall clock - plan *grant // the scheduled grant of an approved KindPreapprove request - msg MessageID // with plan: the message of the request, and its prompt + cfg *config.Broker // the config that the checks used + reloaded bool // StartGrants started nothing: a reload came after the checks + at time.Time // when the request was decided, on the wall clock + plan *grant // the scheduled grant of an approved KindPreapprove request + msg MessageID // with plan: the message of the request, and its prompt prompt Prompt su *standingUse // the use path of a standing grant usedID string // the scheduled grant that allowed the request @@ -263,6 +266,12 @@ func (r *Result) StartGrants() { return } r.started = true + if r.cfg != c.conf() { + // A reload came after the checks: the grants start only from a + // request that the config of now checked (Reload). + r.reloaded = true + return + } now := c.now() if r.plan != nil { c.startScheduled(r, now) diff --git a/internal/approval/standing.go b/internal/approval/standing.go index ae1ad43..43e1a69 100644 --- a/internal/approval/standing.go +++ b/internal/approval/standing.go @@ -65,7 +65,7 @@ func toWindow(s WindowSpec) (window, bool) { // than max_window, and the uses per window inside max_uses. A config without // [standing] allows nothing. The count of grants is checked in begin. func (c *Core) checkStanding(r *Request, p *pending) bool { - lim := c.cfg.Standing + lim := p.cfg.Standing ok := lim.Enabled && r.HostKey == nil && r.For == 0 && r.Key == "" && r.RemoteUser == "" && len(r.Hosts) == 0 && len(r.Secrets) > 0 && p.client != "" && r.TokenHash != [32]byte{} var refs []ref @@ -75,7 +75,7 @@ func (c *Core) checkStanding(r *Request, p *pending) bool { continue } seen[name] = true - s, known := c.cfg.Secrets[name] + s, known := p.cfg.Secrets[name] if !known || p.client == "" || !slices.Contains(s.Clients, p.client) || !s.Rule.Standing || s.Rule.Ask == config.AskNever || !validRule(s.Rule, s.Clients) { ok = false diff --git a/internal/approval/summary.go b/internal/approval/summary.go index 3776c98..ee951d0 100644 --- a/internal/approval/summary.go +++ b/internal/approval/summary.go @@ -29,13 +29,13 @@ func nextSummary(wall time.Time, day time.Weekday, minute int) time.Time { // not tried again until next week. func (c *Core) watchSummary(ctx context.Context) { s, ok := c.approver.(Summarizer) - if !ok || !c.cfg.Standing.Enabled { + if !ok || !c.conf().Standing.Enabled { return } var sent time.Time // the target of the last summary for { now := c.now() - target := nextSummary(now.wall, c.cfg.Standing.SummaryDay, c.cfg.Standing.SummaryMinute) + target := nextSummary(now.wall, c.conf().Standing.SummaryDay, c.conf().Standing.SummaryMinute) t := c.clock.TimerAt(now.mono.Add(target.Sub(now.wall))) select { case <-t.C(): diff --git a/internal/audit/event.go b/internal/audit/event.go index 7c9bee9..4f88396 100644 --- a/internal/audit/event.go +++ b/internal/audit/event.go @@ -98,6 +98,9 @@ const ( EndRevoked EndCause = "revoked" // the person pressed Revoke EndAlarm EndCause = "alarm" // a standing grant met an old token or too many uses (DESIGN.md section 8) EndRenewed EndCause = "renewed" // a standing grant was replaced by its renewal + // EndConfig: a reload of the config took away what the grant allowed + // (DESIGN.md section 14, "Reload"). + EndConfig EndCause = "config_changed" ) // TapCause says why a button press did not count (DESIGN.md section 7). @@ -202,6 +205,18 @@ type BrokerStarted struct { // BrokerStopped is the last entry of a broker run that ends cleanly. type BrokerStopped struct{} +// ConfigReloaded records a reload of the broker config that the broker +// took (DESIGN.md section 14, "Reload"). A refused reload has no entry: +// nothing changed. +type ConfigReloaded struct { + // ConfigSHA256 is the SHA-256 of the new config file, in hex. + ConfigSHA256 string `json:"config_sha256"` + // GrantsEnded is how many grants the reload ended, of every kind. The + // scheduled and standing ones among them also have a grant_ended + // entry each, with cause config_changed. + GrantsEnded int `json:"grants_ended"` +} + // Request records a new request, after the broker's checks. type Request struct { Subject @@ -340,6 +355,7 @@ type logRecovered struct { const ( eventBrokerStarted = "broker_started" eventBrokerStopped = "broker_stopped" + eventConfigReload = "config_reloaded" eventRequest = "request" eventDecision = "decision" eventGrantUsed = "grant_used" @@ -353,19 +369,20 @@ const ( eventLogRecovered = "log_recovered" ) -func (BrokerStarted) eventName() string { return eventBrokerStarted } -func (BrokerStopped) eventName() string { return eventBrokerStopped } -func (Request) eventName() string { return eventRequest } -func (Decision) eventName() string { return eventDecision } -func (GrantUsed) eventName() string { return eventGrantUsed } -func (GrantEnded) eventName() string { return eventGrantEnded } -func (GrantAlarm) eventName() string { return eventGrantAlarm } -func (SecretSent) eventName() string { return eventSecretSent } -func (Signed) eventName() string { return eventSigned } -func (Denied) eventName() string { return eventDenied } -func (ForeignTap) eventName() string { return eventForeignTap } -func (Failure) eventName() string { return eventError } -func (logRecovered) eventName() string { return eventLogRecovered } +func (BrokerStarted) eventName() string { return eventBrokerStarted } +func (BrokerStopped) eventName() string { return eventBrokerStopped } +func (ConfigReloaded) eventName() string { return eventConfigReload } +func (Request) eventName() string { return eventRequest } +func (Decision) eventName() string { return eventDecision } +func (GrantUsed) eventName() string { return eventGrantUsed } +func (GrantEnded) eventName() string { return eventGrantEnded } +func (GrantAlarm) eventName() string { return eventGrantAlarm } +func (SecretSent) eventName() string { return eventSecretSent } +func (Signed) eventName() string { return eventSigned } +func (Denied) eventName() string { return eventDenied } +func (ForeignTap) eventName() string { return eventForeignTap } +func (Failure) eventName() string { return eventError } +func (logRecovered) eventName() string { return eventLogRecovered } // Patterns for the checked fields. var ( @@ -488,6 +505,16 @@ func (e BrokerStarted) validate() error { func (BrokerStopped) validate() error { return nil } +func (e ConfigReloaded) validate() error { + if !sha256Hex.MatchString(e.ConfigSHA256) || e.GrantsEnded < 0 { + return errors.New("it needs the SHA-256 of the config in hex and a number of grants that is not negative") + } + return nil +} + +// sha256Hex is a SHA-256 in lower-case hex. +var sha256Hex = regexp.MustCompile(`^[0-9a-f]{64}$`) + func (e Request) validate() error { return e.Subject.validate(true) } func (e SecretSent) validate() error { return e.Subject.validate(true) } func (e Signed) validate() error { return e.Subject.validate(true) } @@ -579,7 +606,7 @@ func (e GrantEnded) validate() error { return errors.New("it needs a preapprove request, a grant_id and a number of uses left") } switch e.Cause { - case EndUsedUp, EndEnded, EndCancelled, EndDenied, EndStopped, EndRevoked, EndAlarm, EndRenewed: + case EndUsedUp, EndEnded, EndCancelled, EndDenied, EndStopped, EndRevoked, EndAlarm, EndRenewed, EndConfig: return nil } return errors.New("cause is not known") @@ -635,7 +662,7 @@ func encodeEvent(ev Event) (string, []byte, error) { // pointer has the methods as well, and a nil pointer would panic in // them. switch ev.(type) { - case BrokerStarted, BrokerStopped, Request, Decision, GrantUsed, GrantEnded, GrantAlarm, SecretSent, Signed, Denied, ForeignTap, Failure, + case BrokerStarted, BrokerStopped, ConfigReloaded, Request, Decision, GrantUsed, GrantEnded, GrantAlarm, SecretSent, Signed, Denied, ForeignTap, Failure, logRecovered: default: return "", nil, errors.New("not an event of the audit package; pass an event such as audit.Signed{...} as a value") diff --git a/internal/broker/broker.go b/internal/broker/broker.go index 5849933..ec897e5 100644 --- a/internal/broker/broker.go +++ b/internal/broker/broker.go @@ -180,11 +180,11 @@ type Parts struct { // to transport.NewServer as the handler, call Run, and Close it when the // broker stops. Its methods are safe for use by many goroutines. type Service struct { - cfg *config.Broker + // cur is the config and the keys of now; Reload changes it. A request + // takes it once, at its start (request.v). + cur atomic.Pointer[view] audit AuditLog stores map[string]store.Store - keys map[string]sshauth.Key - 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 @@ -203,7 +203,8 @@ type Service struct { maxPerClient int // bodies holds a place for each body that is read and decoded now; - // clientBodies holds the places of each client, by name. + // clientBodies holds the places of each client, by name. Read it with + // mu held (clientPlaces). bodies chan struct{} clientBodies map[string]chan struct{} @@ -280,11 +281,8 @@ func New(p Parts) (*Service, error) { } stopCtx, stop := context.WithCancel(context.Background()) s := &Service{ - cfg: cfg, audit: p.Audit, stores: maps.Clone(p.Stores), - keys: maps.Clone(p.Keys), - keysOf: keysOf, log: logger, random: rand.Reader, maxPerClient: MaxRequestsPerClient, @@ -303,7 +301,39 @@ func New(p Parts) (*Service, error) { if version.Valid(p.Version) { s.version = p.Version } - for name := range cfg.Clients { + s.cur.Store(&view{cfg: cfg, keys: maps.Clone(p.Keys), keysOf: keysOf}) + s.addClients(cfg) + s.refusedLines = newRateLimit(refusedLinesPerMinute, func(n int) { + s.log.Info("broker: did not log refused requests, to keep the log small", slog.Int("dropped", n), + slog.Int("limit_per_minute", refusedLinesPerMinute)) + }) + s.bg.Add(1) + go s.writeTaps() + return s, nil +} + +// view is the config of the Service, with the keys that fit it. Nothing +// may change a view; Reload makes a new one. +type view struct { + cfg *config.Broker + keys map[string]sshauth.Key + keysOf map[string]api.KeysAnswer // the answer of PathKeys, by client +} + +// view returns the config and the keys of now. +func (s *Service) view() *view { return s.cur.Load() } + +// addClients adds the state of each client of cfg that the Service does +// not have yet: its places for bodies, its limit of refused entries, and +// its notes about versions. The state of a removed client stays; it is +// small, and its requests are refused. s.mu must not be held. +func (s *Service) addClients(cfg *config.Broker) { + s.mu.Lock() + defer s.mu.Unlock() + for _, name := range slices.Sorted(maps.Keys(cfg.Clients)) { + if s.clientBodies[name] != nil { + continue + } s.versionNotes[name] = &versionNote{} s.securityNotes[name] = &securityNote{} s.clientBodies[name] = make(chan struct{}, maxBodiesPerClient) @@ -312,13 +342,14 @@ func New(p Parts) (*Service, error) { slog.String("client", name), slog.Int("requests", n), slog.Int("limit_per_minute", refusedEntriesPerMinute)) }) } - s.refusedLines = newRateLimit(refusedLinesPerMinute, func(n int) { - s.log.Info("broker: did not log refused requests, to keep the log small", slog.Int("dropped", n), - slog.Int("limit_per_minute", refusedLinesPerMinute)) - }) - s.bg.Add(1) - go s.writeTaps() - return s, nil +} + +// clientPlaces returns the places for bodies of client, or nil for a +// client that the Service never had. +func (s *Service) clientPlaces(client string) chan struct{} { + s.mu.Lock() + defer s.mu.Unlock() + return s.clientBodies[client] } // checkStores checks that stores has a store for each [store.*] section, @@ -472,7 +503,10 @@ func (s *Service) Close() { s.requests.Wait() close(s.tapsStop) s.bg.Wait() - for _, l := range s.refusedEntries { + s.mu.Lock() + limits := slices.Collect(maps.Values(s.refusedEntries)) + s.mu.Unlock() + for _, l := range limits { l.stop() } s.refusedLines.stop() @@ -599,7 +633,7 @@ func (s *Service) rateLimited(name string) { s.mu.Unlock() if send { service := "1Password" - if s.cfg.Stores[name].Type == config.StoreInfisical { + if s.view().cfg.Stores[name].Type == config.StoreInfisical { service = "Infisical" } s.sendAlert(fmt.Sprintf("The store %q reached the rate limit of %s. The broker denies its secrets "+ diff --git a/internal/broker/handler.go b/internal/broker/handler.go index 9b1b69c..af0796e 100644 --- a/internal/broker/handler.go +++ b/internal/broker/handler.go @@ -12,6 +12,7 @@ import ( "github.com/code6x/nodkey/internal/api" "github.com/code6x/nodkey/internal/approval" "github.com/code6x/nodkey/internal/audit" + "github.com/code6x/nodkey/internal/config" "github.com/code6x/nodkey/internal/store" "github.com/code6x/nodkey/internal/transport" ) @@ -76,7 +77,8 @@ func (s *Service) ServeHTTP(w http.ResponseWriter, r *http.Request) { } adm := s.enter(client) defer s.leave(client, adm) - rq := &request{s: s, w: w, client: client, kind: ep.kind, core: adm.core} + rq := &request{s: s, v: s.view(), w: w, client: client, kind: ep.kind, core: adm.core} + rq.pin, rq.hasPin = transport.ClientPin(r.Context()) if adm.status != 0 { if ep.kind != "" && adm.cause != "" { // These refusals do not count toward the cap, so their @@ -187,11 +189,22 @@ func (r receipt) valid() bool { return r.last.Seq > 0 } // request is one request in progress. type request struct { - s *Service - w http.ResponseWriter - client string - kind audit.Kind - core *approval.Core + s *Service + // v is the config and the keys of the start of the request. Before it + // hands out a value or signs, the request checks that a reload since + // then changed nothing that it uses (stillAllowed). + v *view + // pin is the pin of the client's key, as the server checked it, and + // hasPin says that the server gave one. + pin config.Pin + hasPin bool + // decided is the config that the core decided a request for secrets + // with (approval.Result.Config). + decided *config.Broker + w http.ResponseWriter + client string + kind audit.Kind + core *approval.Core // pre holds the names that passed the broker's checks so far, and the // claimed text once it is safe, before the core gives the request an @@ -236,7 +249,7 @@ func (rq *request) answer(status int) { // that waits longer than bodyWait is denied for a limit (429). It reports // whether the request got a place; if not, the request has its answer. func (rq *request) takeBody(ctx context.Context) bool { - mine, all := rq.s.clientBodies[rq.client], rq.s.bodies + mine, all := rq.s.clientPlaces(rq.client), rq.s.bodies if mine == nil { rq.answer(http.StatusForbidden) // never: every client of the config has places return false @@ -345,7 +358,9 @@ func (rq *request) recordDenied(sub audit.Subject, cause audit.Cause, code audit // refusedAllowed reports whether the entries of one more refused request // of client may be written now (refusedEntriesPerMinute). func (s *Service) refusedAllowed(client string) bool { + s.mu.Lock() l := s.refusedEntries[client] + s.mu.Unlock() return l == nil || l.allow() } diff --git a/internal/broker/preapprove.go b/internal/broker/preapprove.go index af2337c..00d262b 100644 --- a/internal/broker/preapprove.go +++ b/internal/broker/preapprove.go @@ -147,7 +147,7 @@ func (s *Service) checkStanding(client string, req *api.PreapproveRequest) (sub continue } seen[name] = true - sec, known := s.cfg.Secrets[name] + sec, known := s.view().cfg.Secrets[name] if !known || !slices.Contains(sec.Clients, client) { ok = false continue @@ -224,7 +224,7 @@ func (s *Service) checkPreapprove(client string, req *api.PreapproveRequest) (su continue } seen[name] = true - sec, known := s.cfg.Secrets[name] + sec, known := s.view().cfg.Secrets[name] if !known || !slices.Contains(sec.Clients, client) { ok = false continue @@ -233,7 +233,7 @@ func (s *Service) checkPreapprove(client string, req *api.PreapproveRequest) (su } return sub, from, until, ok } - k, known := s.cfg.Keys[req.Key] + k, known := s.view().cfg.Keys[req.Key] if known && slices.Contains(k.Clients, client) { sub.Key = k.Name } else { diff --git a/internal/broker/reload.go b/internal/broker/reload.go new file mode 100644 index 0000000..2312d92 --- /dev/null +++ b/internal/broker/reload.go @@ -0,0 +1,124 @@ +package broker + +import ( + "bytes" + "errors" + "log/slog" + "maps" + "net/http" + + "github.com/code6x/nodkey/internal/audit" + "github.com/code6x/nodkey/internal/config" + "github.com/code6x/nodkey/internal/sshauth" +) + +// ErrNotRecorded is the error of Reload when the new config is in use, but +// its config_reloaded entry could not be written. +var ErrNotRecorded = errors.New("broker: the new config is in use, but its config_reloaded entry is not in the " + + "audit log; check the disk and the journal") + +// Reload makes cfg and keys the config and the keys of the Service, and +// of the core of Run (DESIGN.md section 14, "Reload"). nodkey broker loads +// them as at the start, and checks them with config.Broker.ReloadFixed. +// Reload checks that they fit the stores of the Service, which a reload +// cannot change, as New does. On an error nothing changes. +// +// The core ends every grant that the new config does not allow in the same +// way (approval.Core.Reload). A request that started before the reload is +// checked again before it hands out a value or signs (stillAllowed). Then +// Reload writes a config_reloaded entry with sum, the SHA-256 of the new +// config file in hex. It returns how many grants ended. +// +// The caller must change the clients of the server (transport.Server. +// SetClients) before it calls Reload, so a removed client has no new +// request by then. +func (s *Service) Reload(cfg *config.Broker, keys map[string]sshauth.Key, sum string) (int, error) { + if cfg == nil { + return 0, errors.New("broker: Reload needs the config") + } + if err := checkStores(cfg, s.stores); err != nil { + return 0, err + } + keysOf, err := checkKeys(cfg, keys) + if err != nil { + return 0, err + } + s.mu.Lock() + core, closed := s.core, s.closed + s.mu.Unlock() + if core == nil || closed || s.coreStopped.Load() { + return 0, errors.New("broker: the service does not run, so it cannot take a new config") + } + old := s.view() + next := &view{cfg: cfg, keys: maps.Clone(keys), keysOf: keysOf} + s.addClients(cfg) + // The core first: its grants that the new config takes away end before + // any request sees the new config. A request that takes the old view + // after this is checked again against the new one (stillAllowed). + ended := core.Reload(cfg, func(name string) bool { return sameKey(old, next, name) }) + s.cur.Store(next) + if _, err := s.append(audit.ConfigReloaded{ConfigSHA256: sum, GrantsEnded: ended}); err != nil { + // The new config is in use already; the failed write is logged, + // and after audit.ErrBroken every request is denied. + return ended, ErrNotRecorded + } + return ended, nil +} + +// cfgOrNil returns the config of v, or nil for no view. +func (v *view) cfgOrNil() *config.Broker { + if v == nil { + return nil + } + return v.cfg +} + +// sameKey reports whether the [key.*] section name holds the same public +// key in both views. +func sameKey(old, next *view, name string) bool { + a, ok1 := old.keys[name] + b, ok2 := next.keys[name] + return ok1 && ok2 && a.PublicKey() != nil && b.PublicKey() != nil && + bytes.Equal(a.PublicKey().Marshal(), b.PublicKey().Marshal()) +} + +// stillAllowed checks, before a value is fetched or a login signed, and +// again before the answer, that no reload since the start of the request +// took away what it uses: the client and the pin of its key, each of the +// secrets, or the key and the host (hostKey in SSH wire form). It compares +// the config of now with the config of the start of the request (rq.v) +// and with the config that the core decided with (decided). When the +// request may not go on, it is denied (cause not_allowed, 403) and the +// broker logs why; it reports false. +func (rq *request) stillAllowed(decided *config.Broker, secrets []string, key string, hostKey []byte) bool { + cur := rq.s.view() + ok := rq.v != nil && decided != nil && rq.hasPin + if ok { + c, known := cur.cfg.Clients[rq.client] + ok = known && c.Pin == rq.pin + } + for _, from := range []*config.Broker{rq.v.cfgOrNil(), decided} { + if !ok || from == cur.cfg { + continue + } + ok = from.SameClient(cur.cfg, rq.client) + for _, name := range secrets { + ok = ok && from.SameSecret(cur.cfg, name, rq.client) + } + if key != "" { + ok = ok && from.SameKey(cur.cfg, key, rq.client) && cur.cfg.KeyHost(key, hostKey) + } + } + if ok && key != "" && rq.v != cur { + ok = sameKey(rq.v, cur, key) + } + if ok { + return true + } + rq.s.log.Warn("broker: a reload of the config took away what an approved request uses; the request is denied", + slog.String("request_id", rq.sub.RequestID), slog.String("client", rq.client)) + rq.ended = true + _, _ = rq.s.append(audit.Denied{Subject: rq.sub, Cause: audit.CauseNotAllowed}) + rq.answer(http.StatusForbidden) + return false +} diff --git a/internal/broker/reload_test.go b/internal/broker/reload_test.go new file mode 100644 index 0000000..20313ff --- /dev/null +++ b/internal/broker/reload_test.go @@ -0,0 +1,50 @@ +package broker + +import ( + "maps" + "net/http" + "net/http/httptest" + "testing" + + "github.com/code6x/nodkey/internal/audit" + "github.com/code6x/nodkey/internal/config" +) + +// TestStillAllowed: before a value goes out, the request needs the pin +// that the config of now has for its client, and its secret must be the +// same from the config of its start and from the config that the core +// decided with, to the config of now. Each failure is a deny with 403 and +// an entry. +func TestStillAllowed(t *testing.T) { + e := newEnv(t, envOptions{}) + cfg := e.svc.view().cfg + pin := cfg.Clients["laptop"].Pin + otherItem := *cfg + otherItem.Secrets = maps.Clone(cfg.Secrets) + s := otherItem.Secrets["github-token"] + s.ID = "00000000-0000-4000-8000-0000000000ff" + otherItem.Secrets["github-token"] = s + for _, c := range []struct { + name string + hasPin bool + pin config.Pin + decided *config.Broker + want bool + }{ + {"all the same", true, pin, cfg, true}, + {"no pin from the server", false, pin, cfg, false}, + {"another pin", true, config.Pin{7}, cfg, false}, + {"no decided config", true, pin, nil, false}, + {"the core decided with another item", true, pin, &otherItem, false}, + } { + w := httptest.NewRecorder() + rq := &request{s: e.svc, v: e.svc.view(), w: w, client: "laptop", kind: audit.KindSecrets, + hasPin: c.hasPin, pin: c.pin} + if got := rq.stillAllowed(c.decided, []string{"github-token"}, "", nil); got != c.want { + t.Errorf("%s: %v, want %v", c.name, got, c.want) + } + if !c.want && w.Code != http.StatusForbidden { + t.Errorf("%s: status %d, want 403", c.name, w.Code) + } + } +} diff --git a/internal/broker/secrets.go b/internal/broker/secrets.go index 6d47317..0dfbe08 100644 --- a/internal/broker/secrets.go +++ b/internal/broker/secrets.go @@ -67,10 +67,16 @@ func (rq *request) secrets(ctx context.Context, r *http.Request) { // The decision entry is on disk: the grants of an "Approve N min" tap // may start, before anything is handed out. res.StartGrants() + rq.decided = res.Config() values, ok := rq.fetch(ctx, rc, names) if !ok { return } + // A fetch can take long: a reload during it may have taken the access + // away. The values are in rq.values, so they are cleared. + if !rq.stillAllowed(rq.decided, names, "", nil) { + return + } ans := api.SecretsAnswer{Secrets: make([]api.SecretValue, len(req.Secrets))} if tok, rotated := res.NewToken(); rotated { ans.GrantToken = tok.Reveal() @@ -121,7 +127,7 @@ func (s *Service) checkSecrets(client string, req *api.SecretsRequest) (names, p ok = false } vars[ref.Var] = true - sec, known := s.cfg.Secrets[ref.Name] + sec, known := s.view().cfg.Secrets[ref.Name] if !known || !slices.Contains(sec.Clients, client) { ok = false continue @@ -170,6 +176,9 @@ func (rq *request) fetch(ctx context.Context, rc receipt, names []string) (map[s rq.fail(audit.CodeStoreFailed) return nil, false } + if !rq.stillAllowed(rq.decided, names, "", nil) { + return nil, false + } rq.failCode = audit.CodeStoreFailed values := make(map[string]store.Value, len(names)) for _, name := range names { @@ -181,7 +190,7 @@ func (rq *request) fetch(ctx context.Context, rc receipt, names []string) (map[s rq.cancelled() return nil, false } - sec := rq.s.cfg.Secrets[name] + sec := rq.v.cfg.Secrets[name] fctx, cancel := context.WithTimeout(ctx, fetchTimeout) ref := sec.ID switch { diff --git a/internal/broker/ssh.go b/internal/broker/ssh.go index cad38f2..c8efabe 100644 --- a/internal/broker/ssh.go +++ b/internal/broker/ssh.go @@ -29,7 +29,7 @@ func (rq *request) listKeys(r *http.Request) { rq.answered = true return } - rq.answerJSON(rq.s.keysOf[rq.client]) + rq.answerJSON(rq.v.keysOf[rq.client]) } // sign answers a request for the signature of one SSH login: nodkey @@ -45,7 +45,7 @@ func (rq *request) sign(ctx context.Context, r *http.Request) { rq.releaseBody() // The key is the one with exactly these bytes, among the keys this // client may use. Otherwise there is no message. - key, ok := rq.s.keyFor(rq.client, req.PublicKey) + key, ok := rq.v.keyFor(rq.client, req.PublicKey) if !ok { rq.deny(rq.pre, audit.CauseNotAllowed, "", http.StatusForbidden) return @@ -55,7 +55,7 @@ func (rq *request) sign(ctx context.Context, r *http.Request) { // message or place in a queue. A Login holds only checked data. login, err := key.Check(sshauth.Request{ Key: req.PublicKey, Data: req.Data, Flags: agent.SignatureFlags(req.Flags), Binds: req.Binds, - }, rq.s.cfg.KnownHosts) + }, rq.v.cfg.KnownHosts) if err != nil { rq.deny(rq.pre, audit.CauseNotAllowed, "", http.StatusForbidden) // The errors of sshauth are fixed texts, without data from the @@ -84,10 +84,16 @@ func (rq *request) sign(ctx context.Context, r *http.Request) { rq.cancelled() return } + if !rq.stillAllowed(res.Config(), nil, key.Name(), login.HostKey().Marshal()) { + return + } sig, ok := rq.signLogin(rc, key, login) if !ok { return } + if !rq.stillAllowed(res.Config(), nil, key.Name(), login.HostKey().Marshal()) { + return + } if !rq.handOut(rc, audit.Signed{Subject: rq.sub}) { return } @@ -96,10 +102,10 @@ func (rq *request) sign(ctx context.Context, r *http.Request) { // keyFor returns the key whose public key is exactly blob, among the keys // that client may use. -func (s *Service) keyFor(client string, blob []byte) (sshauth.Key, bool) { - for _, k := range s.keysOf[client].Keys { +func (v *view) keyFor(client string, blob []byte) (sshauth.Key, bool) { + for _, k := range v.keysOf[client].Keys { if bytes.Equal(k.PublicKey, blob) { - return s.keys[k.Name], true + return v.keys[k.Name], true } } return sshauth.Key{}, false @@ -161,6 +167,12 @@ func (rq *request) grant(ctx context.Context, r *http.Request) { return } res.StartGrants() + if res.SkippedByReload() { + // A reload came after the checks: no grant started (DESIGN.md + // section 14, "Reload"). + rq.stillAllowed(nil, nil, "", nil) // denies, with its entry and log line + return + } rq.ended = true rq.answerJSON(api.GrantAnswer{Minutes: res.WindowMinutes}) } @@ -175,7 +187,7 @@ func (rq *request) grant(ctx context.Context, r *http.Request) { func (s *Service) checkGrant(client string, req *api.GrantRequest) (audit.Subject, bool) { var sub audit.Subject ok := true - k, known := s.cfg.Keys[req.Key] + k, known := s.view().cfg.Keys[req.Key] if known && slices.Contains(k.Clients, client) { sub.Key = k.Name window := int64(k.Rule.Window / time.Second) @@ -198,7 +210,7 @@ func (s *Service) checkGrant(client string, req *api.GrantRequest) (audit.Subjec // the window of its key, when the key may be used by client and has the // "window" rule. It returns the window then. func (s *Service) grantTooLong(client string, req *api.GrantRequest) (time.Duration, bool) { - k, known := s.cfg.Keys[req.Key] + k, known := s.view().cfg.Keys[req.Key] if !known || !slices.Contains(k.Clients, client) || k.Rule.Ask != config.AskWindow { return 0, false } @@ -217,7 +229,7 @@ func (s *Service) checkHosts(k config.Key, known bool, names []string, sub *audi continue // a host that is listed twice counts once } seen[h] = true - if len(s.cfg.KnownHosts.KeysFor(h)) == 0 || (known && len(k.Hosts) > 0 && !slices.Contains(k.Hosts, h)) { + if len(s.view().cfg.KnownHosts.KeysFor(h)) == 0 || (known && len(k.Hosts) > 0 && !slices.Contains(k.Hosts, h)) { ok = false continue } diff --git a/internal/config/broker.go b/internal/config/broker.go index de584c7..9961c4e 100644 --- a/internal/config/broker.go +++ b/internal/config/broker.go @@ -1,6 +1,8 @@ package config import ( + "crypto/sha256" + "encoding/hex" "errors" "fmt" "io/fs" @@ -43,6 +45,9 @@ const MaxTimeout = 10 * time.Minute // in it are expanded with ExpandPath and absolute. type Broker struct { Path string // the config file it was loaded from + // Sum is the SHA-256 of the bytes of the config file that were read, + // in hex. nodkey broker reload compares it (DESIGN.md section 14). + Sum string // Listen is the address to listen on: one IP address, never 0.0.0.0 or // ::, never a multicast or broadcast address, never an IPv6 link-local // address, with no zone, and a port from 1 to 65535. @@ -412,6 +417,8 @@ func loadBroker(path string) (*Broker, error) { return nil, fmt.Errorf("config %s: %w", path, err) } b.Path = path + sum := sha256.Sum256(data) + b.Sum = hex.EncodeToString(sum[:]) return b, nil } diff --git a/internal/config/reload.go b/internal/config/reload.go new file mode 100644 index 0000000..d1e6bd9 --- /dev/null +++ b/internal/config/reload.go @@ -0,0 +1,108 @@ +package config + +import ( + "fmt" + "reflect" + "slices" + + "golang.org/x/crypto/ssh" +) + +// The rules of a reload (DESIGN.md section 14, "Reload"). A reload may +// change the clients, the secrets, the SSH keys and the known hosts. Every +// other part needs a restart: the broker refuses a reload that changes +// one, and keeps the config it has. + +// ReloadFixed checks that next changes only the parts of b that a reload +// may change. It returns an error that names the first part that needs a +// restart, or nil. +func (b *Broker) ReloadFixed(next *Broker) error { + // All but the parts that a reload may change must be equal. So a part + // that is added to Broker later needs a restart, until someone decides + // otherwise here. + x, y := *b, *next + for _, c := range []*Broker{&x, &y} { + c.Path, c.Sum, c.KnownHostsFile, c.KnownHosts = "", "", "", nil + c.Clients, c.Secrets, c.Keys = nil, nil, nil + } + type part struct { + name string + old, new any + } + parts := []part{ + {"listen", b.Listen, next.Listen}, + {"tls_key", b.TLSKey, next.TLSKey}, + {"audit_log", b.AuditLog, next.AuditLog}, + {"[telegram]", b.Telegram, next.Telegram}, + {"[ntfy]", b.Ntfy, next.Ntfy}, + {"[updates]", b.Updates, next.Updates}, + {"[preapprove]", b.Preapprove, next.Preapprove}, + {"[standing]", b.Standing, next.Standing}, + {"[store.*]", b.Stores, next.Stores}, + } + parts = append(parts, part{"the config", x, y}) // a part that is not named above + for _, p := range parts { + if !reflect.DeepEqual(p.old, p.new) { + return fmt.Errorf("%s: is changed, and a reload cannot change it; restart the broker to use the new "+ + "config, or undo the change (DESIGN.md section 14, \"Reload\")", p.name) + } + } + return nil +} + +// SameClient reports whether the client name has the same TLS pin in b and +// in next. A client that next does not have, or that has a new pin, is +// not the same: its grants end. +func (b *Broker) SameClient(next *Broker, name string) bool { + old, ok1 := b.Clients[name] + cur, ok2 := next.Clients[name] + return ok1 && ok2 && old == cur +} + +// SameSecret reports whether client may still use the secret name in +// next as it could in b: the client is the same (SameClient), next lists it +// in the section, and the section is the same in all else (its store, its +// item and its rule). Another client added to or removed from the list does +// not count. +func (b *Broker) SameSecret(next *Broker, name, client string) bool { + old, ok1 := b.Secrets[name] + cur, ok2 := next.Secrets[name] + if !ok1 || !ok2 || !b.SameClient(next, client) || !slices.Contains(old.Clients, client) || + !slices.Contains(cur.Clients, client) { + return false + } + old.Clients, cur.Clients = nil, nil + return reflect.DeepEqual(old, cur) +} + +// SameKey reports whether client may still use the SSH key name in next as +// it could in b: the client is the same (SameClient), next lists it in the +// section, and the file and the rule are the same. The hosts may change; +// check each host with KeyHost. It does not read the key file: the caller +// must also check that the file holds the same key. +func (b *Broker) SameKey(next *Broker, name, client string) bool { + old, ok1 := b.Keys[name] + cur, ok2 := next.Keys[name] + if !ok1 || !ok2 || !b.SameClient(next, client) || !slices.Contains(old.Clients, client) || + !slices.Contains(cur.Clients, client) { + return false + } + old.Clients, cur.Clients, old.Hosts, cur.Hosts = nil, nil, nil, nil + return reflect.DeepEqual(old, cur) +} + +// KeyHost reports whether the SSH key name of b may log in to the host +// whose host key is hostKey, in SSH wire form: known_hosts names the +// host key, and the section's hosts list the name, or are empty. +func (b *Broker) KeyHost(name string, hostKey []byte) bool { + k, ok := b.Keys[name] + if !ok || b.KnownHosts == nil { + return false + } + pub, err := ssh.ParsePublicKey(hostKey) + if err != nil { + return false + } + host, ok := b.KnownHosts.NameFor(pub) + return ok && (len(k.Hosts) == 0 || slices.Contains(k.Hosts, host)) +} diff --git a/internal/config/reload_test.go b/internal/config/reload_test.go new file mode 100644 index 0000000..4b679c0 --- /dev/null +++ b/internal/config/reload_test.go @@ -0,0 +1,112 @@ +package config + +import ( + "maps" + "strings" + "testing" + "time" +) + +func reloadBase() *Broker { + return &Broker{ + Telegram: Telegram{UserID: 1, Timeout: time.Minute}, + Clients: map[string]Client{"laptop": {Name: "laptop", Pin: Pin{1}}, "ci": {Name: "ci", Pin: Pin{2}}}, + Stores: map[string]Store{"main": {Name: "main", Type: StoreBWS}}, + Secrets: map[string]Secret{"token": {Name: "token", Store: "main", ID: "a", Clients: []string{"laptop"}, + Rule: Rule{Ask: AskWindow, Window: time.Minute}}}, + Keys: map[string]Key{"deploy": {Name: "deploy", File: "/k", Clients: []string{"laptop"}, Hosts: []string{"web-1"}, + Rule: Rule{Ask: AskAlways}}}, + } +} + +func reloadCopy(b *Broker) *Broker { + n := *b + n.Clients, n.Stores, n.Secrets, n.Keys = maps.Clone(b.Clients), maps.Clone(b.Stores), maps.Clone(b.Secrets), maps.Clone(b.Keys) + return &n +} + +// TestReloadFixed: a reload refuses a change of each part that needs a +// restart, and takes changes of clients, secrets and keys. +func TestReloadFixed(t *testing.T) { + old := reloadBase() + for name, change := range map[string]func(*Broker){ + "tls_key": func(b *Broker) { b.TLSKey = "/other" }, + "audit_log": func(b *Broker) { b.AuditLog = "/other" }, + "[telegram]": func(b *Broker) { b.Telegram.Timeout = 2 * time.Minute }, + "[ntfy]": func(b *Broker) { b.Ntfy = &Ntfy{} }, + "[updates]": func(b *Broker) { b.Updates.Check = true }, + "[preapprove]": func(b *Broker) { b.Preapprove.MaxUses = 9 }, + "[standing]": func(b *Broker) { b.Standing.Enabled = true }, + "[store.*]": func(b *Broker) { b.Stores["main"] = Store{Name: "main", Type: StoreBWS, Program: "/x"} }, + } { + next := reloadCopy(old) + change(next) + if err := old.ReloadFixed(next); err == nil || !strings.HasPrefix(err.Error(), name+": is changed") { + t.Errorf("%s: %v", name, err) + } + } + next := reloadCopy(old) + delete(next.Secrets, "token") + delete(next.Keys, "deploy") + next.Clients["new"] = Client{Name: "new", Pin: Pin{3}} + if err := old.ReloadFixed(next); err != nil { + t.Errorf("clients, secrets and keys: %v", err) + } +} + +// TestReloadSame: SameSecret and SameKey are true only for the same client +// with the same pin, still in the list, and the same section in all else. +func TestReloadSame(t *testing.T) { + old := reloadBase() + if !old.SameSecret(reloadCopy(old), "token", "laptop") || !old.SameKey(reloadCopy(old), "deploy", "laptop") { + t.Fatal("the same config is not the same") + } + if old.SameSecret(reloadCopy(old), "token", "ci") || old.SameKey(reloadCopy(old), "deploy", "ci") { + t.Error("a client that is not in the list") + } + type tc struct { + name string + change func(*Broker) + secret bool // the change keeps the secret + key bool // the change keeps the key + } + for _, c := range []tc{ + {"a new pin", func(b *Broker) { b.Clients["laptop"] = Client{Name: "laptop", Pin: Pin{9}} }, false, false}, + {"no client", func(b *Broker) { delete(b.Clients, "laptop") }, false, false}, + {"another client added", func(b *Broker) { + s := b.Secrets["token"] + s.Clients = []string{"laptop", "ci"} + b.Secrets["token"] = s + k := b.Keys["deploy"] + k.Clients = []string{"ci", "laptop"} + b.Keys["deploy"] = k + }, true, true}, + {"another item", func(b *Broker) { s := b.Secrets["token"]; s.ID = "b"; b.Secrets["token"] = s }, false, true}, + {"another store", func(b *Broker) { s := b.Secrets["token"]; s.Store = "x"; b.Secrets["token"] = s }, false, true}, + {"a longer window", func(b *Broker) { + s := b.Secrets["token"] + s.Rule.Window = time.Hour + b.Secrets["token"] = s + }, false, true}, + {"the client removed", func(b *Broker) { + s := b.Secrets["token"] + s.Clients = []string{"ci"} + b.Secrets["token"] = s + k := b.Keys["deploy"] + k.Clients = nil + b.Keys["deploy"] = k + }, false, false}, + {"another key file", func(b *Broker) { k := b.Keys["deploy"]; k.File = "/j"; b.Keys["deploy"] = k }, true, false}, + {"other hosts", func(b *Broker) { k := b.Keys["deploy"]; k.Hosts = nil; b.Keys["deploy"] = k }, true, true}, + {"gone", func(b *Broker) { delete(b.Secrets, "token"); delete(b.Keys, "deploy") }, false, false}, + } { + next := reloadCopy(old) + c.change(next) + if got := old.SameSecret(next, "token", "laptop"); got != c.secret { + t.Errorf("%s: SameSecret %v", c.name, got) + } + if got := old.SameKey(next, "deploy", "laptop"); got != c.key { + t.Errorf("%s: SameKey %v", c.name, got) + } + } +} diff --git a/internal/ntfy/text.go b/internal/ntfy/text.go index 4dcd9fc..6ba3091 100644 --- a/internal/ntfy/text.go +++ b/internal/ntfy/text.go @@ -372,6 +372,8 @@ func grantEndLine(p approval.Prompt, e approval.Ending) string { return "🚨 ended by an alarm at " + at case audit.EndRenewed: return "🔁 renewed at " + at + "; a new grant replaced this one" + case audit.EndConfig: + return "⛔ ended by a config change at " + at + left } return "⛔ dropped: the broker stopped at " + at + left } diff --git a/internal/telegram/text.go b/internal/telegram/text.go index bb3dd37..8c3a9cc 100644 --- a/internal/telegram/text.go +++ b/internal/telegram/text.go @@ -624,6 +624,8 @@ func grantEndLines(p approval.Prompt, e approval.Ending) string { end = "⛔ ended by a later Deny at " + at + ", with " + strconv.Itoa(e.UsesLeft) + " use(s) left" case audit.EndRevoked: end = "⛔ revoked by you at " + at + ", with " + strconv.Itoa(e.UsesLeft) + " use(s) left" + case audit.EndConfig: + end = "⛔ ended by a config change at " + at + ", with " + strconv.Itoa(e.UsesLeft) + " use(s) left" default: // audit.EndStopped end = "⛔ dropped: the broker stopped at " + at + ", with " + strconv.Itoa(e.UsesLeft) + " use(s) left" } @@ -650,6 +652,8 @@ func standingEndLine(cause audit.EndCause, at string) string { return "🚨 ended by an alarm at " + at case audit.EndRenewed: return "🔁 renewed at " + at + "; a new grant replaced this one" + case audit.EndConfig: + return "⛔ ended by a config change at " + at case audit.EndStopped: return "⛔ dropped: the broker stopped at " + at } diff --git a/internal/transport/server.go b/internal/transport/server.go index da17210..b46add0 100644 --- a/internal/transport/server.go +++ b/internal/transport/server.go @@ -15,6 +15,7 @@ import ( "strconv" "strings" "sync" + "sync/atomic" "time" "github.com/code6x/nodkey/internal/config" @@ -66,6 +67,47 @@ type Server struct { // cause, lines with the pin of a key that no client has, and lines // about a request of a pinned client. connLimit, pinLimit, keyLimit *logLimiter + // own is the pin of the broker's own key. byPin holds the client name + // of each pin; SetClients changes it. + own config.Pin + byPin atomic.Pointer[map[config.Pin]string] +} + +// SetClients makes clients the clients of the server, for a reload of the +// config (DESIGN.md section 14, "Reload"). It checks them as NewServer +// does, and on an error keeps the clients it has. A new connection is +// checked against the new clients. A connection that is open already ends +// at its next request when its client is gone or has a new pin: the +// request gets 403, and the connection closes. A request in progress is +// not stopped here; the broker checks it again before it hands out +// anything. +func (s *Server) SetClients(clients map[string]config.Client) error { + byPin, err := clientsByPin(clients, s.own) + if err != nil { + return err + } + s.byPin.Store(&byPin) + return nil +} + +// clientPinKey is the context key of the pin of the client's key. +type clientPinKey struct{} + +// ClientPin returns the pin of the key that the client of the request of +// ctx used, as the server checked it. The broker compares it with the +// config of now before it hands out anything, because a reload can give +// the client a new pin while its request runs. +func ClientPin(ctx context.Context) (config.Pin, bool) { + pin, ok := ctx.Value(clientPinKey{}).(config.Pin) + return pin, ok && pin != config.Pin{} +} + +// pinOfRequest returns the pin of the client key of r, or the zero pin. +func pinOfRequest(r *http.Request) config.Pin { + if r.TLS == nil || len(r.TLS.PeerCertificates) != 1 { + return config.Pin{} + } + return PinOf(r.TLS.PeerCertificates[0]) } // NewServer returns the broker's server. @@ -103,11 +145,12 @@ func NewServer(key tls.Certificate, clients map[string]config.Client, handler ht if err != nil { return nil, err } - s := &Server{log: logger} + s := &Server{log: logger, own: own} + s.byPin.Store(&byPin) s.connLimit = newLogLimiter(func(n int) { s.reportDropped("connections", n) }) s.pinLimit = newLogLimiter(func(n int) { s.reportDropped("unknown keys", n) }) s.keyLimit = newLogLimiter(func(n int) { s.reportDropped("requests of pinned clients", n) }) - s.tls = s.tlsConfig(key, byPin) + s.tls = s.tlsConfig(key) var protocols http.Protocols protocols.SetHTTP1(true) s.http = &http.Server{ @@ -171,7 +214,7 @@ func clientsByPin(clients map[string]config.Client, own config.Pin) (map[config. // handshake, and the peer is in that context (see NewServer). A handshake // without a peer fails. The returned config itself never accepts a client // on its own: its VerifyConnection always fails. -func (s *Server) tlsConfig(key tls.Certificate, byPin map[config.Pin]string) *tls.Config { +func (s *Server) tlsConfig(key tls.Certificate) *tls.Config { conn := &tls.Config{ MinVersion: tls.VersionTLS13, MaxVersion: tls.VersionTLS13, @@ -202,7 +245,7 @@ func (s *Server) tlsConfig(key tls.Certificate, byPin map[config.Pin]string) *tl } c := conn.Clone() c.VerifyConnection = func(cs tls.ConnectionState) error { - name, pin, err := verifyClient(cs, byPin) + name, pin, err := verifyClient(cs, *s.byPin.Load()) if errors.Is(err, ErrUnknownClient) { // The pin is the hash of the peer's public key, not text // that the peer chose. It helps to add a new client. @@ -306,13 +349,21 @@ func (s *Server) verified(next http.Handler) http.Handler { } }() name, ok := verifiedClient(r) + if ok && (*s.byPin.Load())[pinOfRequest(r)] != name { + // A reload removed the client, or gave it a new pin: its open + // connection ends now (SetClients). + ok = false + w.Header().Set("Connection", "close") + } if !ok { s.logLine(s.keyLimit, slog.LevelError, "transport: refused a request without a verified client name", "remote", remoteText(r.RemoteAddr)) WriteError(w, http.StatusForbidden) return } - next.ServeHTTP(w, r.WithContext(context.WithValue(r.Context(), clientKey{}, name))) + ctx := context.WithValue(r.Context(), clientKey{}, name) + ctx = context.WithValue(ctx, clientPinKey{}, pinOfRequest(r)) + next.ServeHTTP(w, r.WithContext(ctx)) }) }