diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e8f6d46..5887242 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -188,6 +188,31 @@ jobs: } Write-Host "installer parses, web requests basic-parsed, no unguarded exit, arch survives without .NET, no preference leaks" + # Four vulnerabilities in the standard library this repo compiled against + # went unnoticed until somebody ran this by hand. Nothing here was looking, + # so nothing was going to say so - not on the next release either. + # + # govulncheck reports only what the code actually reaches, which is why it + # can be a failing check rather than a list to triage: a finding here is a + # path from this CLI to the bug, and the fix is usually a toolchain or a + # dependency bump. + vulnerabilities: + name: Known vulnerabilities + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-go@v5 + with: + go-version-file: go.mod + cache: true + + # A fixed version rather than @latest: a check that can fail the build + # should not change under it on the morning somebody tags that module. + - name: govulncheck + run: go run golang.org/x/vuln/cmd/govulncheck@v1.8.0 ./... + # The other half of the same gap: scripts/install.sh is the way in for every # member on macOS and Linux, and nothing had ever run it end to end. It was # syntax-checked and its failure path exercised by hand; the path that diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 108e65a..e3d5b32 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -32,6 +32,12 @@ on: permissions: contents: write + # For the attestation below. The runner asks GitHub's OIDC provider for a + # token that says which workflow, in which repository, at which commit is + # running, and that token is what signs the archives - there is no key here + # to keep, rotate or lose. + id-token: write + attestations: write jobs: release: @@ -44,18 +50,36 @@ jobs: go-version-file: go.mod cache: true + # Through the environment, and checked before it goes anywhere near a + # shell. A workflow_dispatch input pasted straight into a `run:` block is + # substituted before bash ever sees it, so whatever was typed into the + # box becomes part of the script - and every step below puts this value + # on a command line. Only something shaped like one of our tags gets + # past here. - name: Resolve the tag id: tag - run: echo "value=${{ github.event.inputs.tag || github.ref_name }}" >> "$GITHUB_OUTPUT" + env: + INPUT_TAG: ${{ github.event.inputs.tag }} + REF_NAME: ${{ github.ref_name }} + run: | + set -euo pipefail + tag="${INPUT_TAG:-$REF_NAME}" + if ! printf '%s' "$tag" | grep -Eq '^v[0-9]+[.][0-9]+[.][0-9]+([-][A-Za-z0-9.]+)?$'; then + echo "not a version tag: $tag" >&2 + exit 1 + fi + echo "value=$tag" >> "$GITHUB_OUTPUT" # The version shown by `nan --version` is a constant in the source # (internal/tui.Version). A tag that disagrees with it ships a binary # that lies about which build it is, and the first person to report a # bug reports the wrong version. - name: Check the tag matches internal/tui.Version + env: + TAG: ${{ steps.tag.outputs.value }} run: | declared="$(grep -oP 'const Version = "\K[^"]+' internal/tui/tui.go)" - tag="${{ steps.tag.outputs.value }}" + tag="$TAG" if [ "v$declared" != "$tag" ]; then echo "tag $tag does not match internal/tui.Version ($declared)" >&2 echo "bump the constant or retag" >&2 @@ -95,30 +119,54 @@ jobs: # Installing what we are about to publish, the same way a member would, # so a broken artifact fails here instead of on their machine. - name: Smoke test the linux/amd64 archive + env: + TAG: ${{ steps.tag.outputs.value }} run: | set -euo pipefail - tar -xzf "dist/nan-cli_${{ steps.tag.outputs.value }}_linux_amd64.tar.gz" -C /tmp + tar -xzf "dist/nan-cli_${TAG}_linux_amd64.tar.gz" -C /tmp /tmp/nan --version /tmp/nan --help > /dev/null # The Windows binaries cannot be run here, but an archive that unpacks # to nothing, or to a name Windows will not execute, can still be caught. - name: Check the Windows archives carry an .exe + env: + TAG: ${{ steps.tag.outputs.value }} run: | set -euo pipefail for arch in amd64 arm64; do - zip="dist/nan-cli_${{ steps.tag.outputs.value }}_windows_${arch}.zip" + zip="dist/nan-cli_${TAG}_windows_${arch}.zip" unzip -l "$zip" | grep -q "nan.exe" || { echo "$zip does not contain nan.exe" >&2 exit 1 } done + # checksums.txt proves the bytes arrived whole. It proves nothing about + # where they came from: whoever can replace an archive on a release can + # replace the checksum sitting next to it, and the installer would verify + # the download against the attacker's own number and call it good. + # + # This signs the archives themselves. GitHub keeps the attestation, so + # anyone can ask what built a binary they already have: + # + # gh attestation verify nan-cli_v1.2.3_linux_amd64.tar.gz --repo helmcode/nan-cli + # + # and get back the workflow, the repository and the commit - none of + # which a release someone else published can produce. + - name: Attest what was built + uses: actions/attest-build-provenance@v4 + with: + subject-path: | + dist/*.tar.gz + dist/*.zip + - name: Publish the release env: GH_TOKEN: ${{ github.token }} + TAG: ${{ steps.tag.outputs.value }} run: | - gh release create "${{ steps.tag.outputs.value }}" \ - --title "${{ steps.tag.outputs.value }}" \ + gh release create "$TAG" \ + --title "$TAG" \ --generate-notes \ dist/*.tar.gz dist/*.zip dist/checksums.txt diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index cc9f6bc..7f02e01 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -4,7 +4,8 @@ Thanks for your interest in contributing. This document covers how to get the pr ## Prerequisites -- **Go 1.26+** — the project uses [mise](https://mise.jdx.dev/) to pin the version. Run `mise install` in the repo root and Go will be available automatically. +- **Go 1.26.8+** — the version in `go.mod`, which is where the standard + library carries the current security fixes. The project uses [mise](https://mise.jdx.dev/) to pin the version. Run `mise install` in the repo root and Go will be available automatically. - A [nan.builders](https://nan.builders) account with an API key (needed to test the TUI at runtime). ## Getting started diff --git a/README.md b/README.md index ef82947..e695b6a 100644 --- a/README.md +++ b/README.md @@ -21,14 +21,32 @@ irm https://nan.builders/install.ps1 | iex ``` Same work: latest release, the `.zip` for your architecture, checksum verified, -`nan.exe` into `%LOCALAPPDATA%\Programs -an` and that directory added to your +`nan.exe` into `%LOCALAPPDATA%\Programs\nan` and that directory added to your user `PATH`. Override with `-InstallDir`: ```powershell -& ([scriptblock]::Create((irm https://nan.builders/install.ps1))) -InstallDir "C: ools" +& ([scriptblock]::Create((irm https://nan.builders/install.ps1))) -InstallDir "C:\tools" ``` +## Verifying a download + +Both installers check the archive against `checksums.txt` from the same +release. That catches a download that arrived damaged; it cannot catch a +release that was published by somebody else, because whoever replaces an +archive can replace the checksum beside it. + +Every release is signed with [build provenance](https://docs.github.com/actions/security-guides/using-artifact-attestations-to-establish-provenance-for-builds), +which is not something a release you did not build can carry. If you have the +[GitHub CLI](https://cli.github.com), the installers check it for you, and you +can ask about any archive yourself: + +```bash +gh attestation verify nan-cli_v0.1.19_linux_amd64.tar.gz --repo helmcode/nan-cli +``` + +It answers with the workflow, the repository and the commit the binary was +built from. + ## Usage Run `nan` to open the TUI dashboard: @@ -70,7 +88,9 @@ inside each tool later. ## Build from source -Requires Go 1.26+ ([mise](https://mise.jdx.dev/) recommended): +Requires Go 1.26.8+ ([mise](https://mise.jdx.dev/) recommended) — the version +in `go.mod`, which is where the standard library carries the current security +fixes: ```bash git clone https://github.com/helmcode/nan-cli diff --git a/SECURITY.md b/SECURITY.md new file mode 100644 index 0000000..02f0182 --- /dev/null +++ b/SECURITY.md @@ -0,0 +1,61 @@ +# Security policy + +## Reporting a vulnerability + +Use GitHub's **[Report a vulnerability](https://github.com/helmcode/nan-cli/security/advisories/new)** +button, under the Security tab. It opens a private thread with the maintainers +— nothing is public until there is a fix. + +Please don't open a normal issue for a security problem. If the button is not +there yet, open an issue asking for a private channel and leave the details +out of it. + +What helps, roughly in order: + +- what an attacker gets, and what they need in the first place +- the steps to reproduce it, with the version (`nan --version`) and the OS +- whether it has already been exploited or disclosed anywhere + +You'll get an acknowledgement as soon as somebody has read it, and an +assessment before anything ships. If you want to be credited in the advisory, +say so and say how. + +## What this covers + +This repository: the `nan` CLI, the two installers under `scripts/`, and the +release workflow that builds and signs what they download. + +The nan.builders platform — the API the CLI talks to and the keys it issues — +is a separate system. A report about it is welcome through the same channel and +will be passed on. + +## Versions + +Fixes go into the next release. There are no backports: the installers fetch +the latest release, and `nan --version` against +[the releases page](https://github.com/helmcode/nan-cli/releases) is the whole +of the support matrix. + +## Known, accepted, and not a finding + +Two things about this CLI look like vulnerabilities and are deliberate, so +you're not wasting your time reporting them: + +**Your API key is stored in plaintext.** It goes into `~/.config/nan/session.json` +and into each configured tool's own config file. Those tools read a literal key +out of their configs, so there is nowhere else to put it. Every file the CLI +writes it into is created `0600` and rewritten through a temp file so the mode +holds even where the file already existed, and `nan auth logout` takes the key +back out of every one of them. Hermes is the exception, because it has a +secrets file of its own: there the key goes into its `.env` and the config +carries a `${NAN_API_KEY}` reference, which is also why it never reaches a +command line. + +**On Windows the mode bits do nothing.** The files inherit the ACL of the user +profile they sit in, which already excludes other non-administrator accounts, +and an administrator can take ownership regardless. If you keep your home +directory on a network share, this is worth knowing about. + +What *is* worth reporting: the key reaching somewhere neither of those covers — +a log, an error message, a command line, a crash dump, a file with a wider +mode, or a tool's config after you have logged out. diff --git a/cmd/auth.go b/cmd/auth.go index 4259cf5..4fe704f 100644 --- a/cmd/auth.go +++ b/cmd/auth.go @@ -3,18 +3,21 @@ package cmd import ( "bufio" "fmt" + "io" "os" "strings" "github.com/nxssie/nan-cli/internal/auth" "github.com/nxssie/nan-cli/internal/session" + "github.com/nxssie/nan-cli/internal/tui" "github.com/spf13/cobra" ) var ( - tokenFlag string - emailFlag string - linkFlag string + tokenFlag string + emailFlag string + linkFlag string + keepToolsFlag bool ) var authCmd = &cobra.Command{ @@ -39,8 +42,9 @@ func init() { authCmd.AddCommand(loginCmd) authCmd.AddCommand(logoutCmd) loginCmd.Flags().StringVar(&emailFlag, "email", "", "Email to send the sign-in link to") - loginCmd.Flags().StringVar(&linkFlag, "link", "", "Finish the login with the link from the email") - loginCmd.Flags().StringVar(&tokenFlag, "token", "", "Save a nan_session token directly, skipping the email") + loginCmd.Flags().StringVar(&linkFlag, "link", "", `Finish the login with the link from the email ("-" reads it from stdin, keeping the token out of your shell history)`) + loginCmd.Flags().StringVar(&tokenFlag, "token", "", `Save a nan_session token directly, skipping the email ("-" reads it from stdin, keeping it out of your shell history)`) + logoutCmd.Flags().BoolVar(&keepToolsFlag, "keep-tools", false, "Leave the API key in the tools this CLI configured") } // The platform signs in by emailed link. It used to be Discord OAuth, and this @@ -49,14 +53,22 @@ func init() { // and the command then asked for a cookie that no longer existed. func runLogin(cmd *cobra.Command, args []string) error { if tokenFlag != "" { - return saveToken(tokenFlag) + token, err := flagValue(tokenFlag) + if err != nil { + return err + } + return saveToken(token) } // `--link` picks the flow up at its second half, for a shell that cannot // answer a prompt: a script, a CI step, or a terminal that runs one command // at a time. if linkFlag != "" { - token, err := auth.TokenFromLink(strings.TrimSpace(linkFlag)) + pasted, err := flagValue(linkFlag) + if err != nil { + return err + } + token, err := auth.TokenFromLink(pasted) if err != nil { return err } @@ -131,11 +143,52 @@ func runLogin(cmd *cobra.Command, args []string) error { return saveToken(sessionToken) } +// A flag whose value is a credential, with "-" meaning stdin. +// +// Both of these take a secret, and a secret spelled out on a command line is +// not one for long: the shell writes it to ~/.bash_history or ~/.zsh_history, +// and while the command runs `ps` shows the whole line to anything running as +// the member - on Linux /proc//cmdline to other accounts as well. A CI +// step or a script can pipe it in instead: +// +// echo "$NAN_LINK" | nan auth login --link - +func flagValue(value string) (string, error) { + if value != "-" { + return strings.TrimSpace(value), nil + } + read, err := io.ReadAll(os.Stdin) + if err != nil { + return "", fmt.Errorf("could not read it from stdin: %w", err) + } + value = strings.TrimSpace(string(read)) + if value == "" { + return "", fmt.Errorf("nothing arrived on stdin") + } + return value, nil +} + func runLogout(cmd *cobra.Command, args []string) error { if err := session.Delete(); err != nil { return err } fmt.Println("Logged out.") + + // Deleting session.json is only half of what logging out means here. The + // API key was copied into every tool `nan` configured, and it goes on + // working from those files: a member who logs out on a machine they are + // giving back would have been logged out of everything except the cluster + // their key bills. + if keepToolsFlag { + fmt.Println("Your API key is still in the tools — run it again without --keep-tools to take it out.") + return nil + } + removed, failed := tui.RemoveNanFromTools() + if len(removed) > 0 { + fmt.Println("Removed your API key from " + strings.Join(removed, ", ") + ".") + } + if len(failed) > 0 { + return fmt.Errorf("your API key is still in %s", strings.Join(failed, ", ")) + } return nil } diff --git a/cmd/auth_test.go b/cmd/auth_test.go new file mode 100644 index 0000000..3b32ddd --- /dev/null +++ b/cmd/auth_test.go @@ -0,0 +1,64 @@ +package cmd + +import ( + "os" + "path/filepath" + "testing" +) + +// `--token` and `--link` both take a secret, and a secret spelled out on a +// command line is in ~/.bash_history and in `ps` for as long as the command +// runs. "-" is the way out for a script or a CI step, so it has to actually +// read stdin - and has to say so rather than saving an empty token when +// nothing arrives. +func TestFlagValueReadsStdinForADash(t *testing.T) { + for _, c := range []struct { + name string + flag string + stdin string + want string + wantErr bool + }{ + {name: "a value stays a value", flag: "a-token", want: "a-token"}, + {name: "trimmed", flag: " a-token\n", want: "a-token"}, + {name: "a dash reads stdin", flag: "-", stdin: "from-stdin\n", want: "from-stdin"}, + {name: "an empty stdin is not a token", flag: "-", stdin: " \n", wantErr: true}, + } { + t.Run(c.name, func(t *testing.T) { + if c.flag == "-" { + withStdin(t, c.stdin) + } + got, err := flagValue(c.flag) + if c.wantErr { + if err == nil { + t.Fatalf("took %q as a credential", got) + } + return + } + if err != nil { + t.Fatal(err) + } + if got != c.want { + t.Errorf("flagValue(%q) = %q, want %q", c.flag, got, c.want) + } + }) + } +} + +func withStdin(t *testing.T, content string) { + t.Helper() + path := filepath.Join(t.TempDir(), "stdin") + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + f, err := os.Open(path) + if err != nil { + t.Fatal(err) + } + original := os.Stdin + os.Stdin = f + t.Cleanup(func() { + os.Stdin = original + f.Close() + }) +} diff --git a/go.mod b/go.mod index 35801b7..dcd01d2 100644 --- a/go.mod +++ b/go.mod @@ -1,16 +1,18 @@ module github.com/nxssie/nan-cli -go 1.26.3 +go 1.26.8 -require github.com/spf13/cobra v1.10.2 +require ( + github.com/charmbracelet/bubbles v1.0.0 + github.com/charmbracelet/bubbletea v1.3.10 + github.com/charmbracelet/lipgloss v1.1.0 + github.com/spf13/cobra v1.10.2 +) require ( github.com/atotto/clipboard v0.1.4 // indirect github.com/aymanbagabas/go-osc52/v2 v2.0.1 // indirect - github.com/charmbracelet/bubbles v1.0.0 // indirect - github.com/charmbracelet/bubbletea v1.3.10 // indirect github.com/charmbracelet/colorprofile v0.4.1 // indirect - github.com/charmbracelet/lipgloss v1.1.0 // indirect github.com/charmbracelet/x/ansi v0.11.6 // indirect github.com/charmbracelet/x/cellbuf v0.0.15 // indirect github.com/charmbracelet/x/term v0.2.2 // indirect @@ -29,6 +31,6 @@ require ( github.com/rivo/uniseg v0.4.7 // indirect github.com/spf13/pflag v1.0.9 // indirect github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e // indirect - golang.org/x/sys v0.38.0 // indirect - golang.org/x/text v0.3.8 // indirect + golang.org/x/sys v0.44.0 // indirect + golang.org/x/text v0.39.0 // indirect ) diff --git a/go.sum b/go.sum index 2cf427b..c144769 100644 --- a/go.sum +++ b/go.sum @@ -51,10 +51,12 @@ github.com/spf13/pflag v1.0.9/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e h1:JVG44RsyaB9T2KIHavMF/ppJZNG9ZpyihvCd0w101no= github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e/go.mod h1:RbqR21r5mrJuqunuUZ/Dhy/avygyECGrLceyNeo4LiM= go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg= +golang.org/x/exp v0.0.0-20231006140011-7918f672742d h1:jtJma62tbqLibJ5sFQz8bKtEM8rJBtfilJ2qTU199MI= +golang.org/x/exp v0.0.0-20231006140011-7918f672742d/go.mod h1:ldy0pHrwJyGW56pPQzzkH36rKxoZW1tw7ZJpeKx+hdo= golang.org/x/sys v0.0.0-20210809222454-d867a43fc93e/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.38.0 h1:3yZWxaJjBmCWXqhN1qh02AkOnCQ1poK6oF+a7xWL6Gc= -golang.org/x/sys v0.38.0/go.mod h1:OgkHotnGiDImocRcuBABYBEXf8A9a87e/uXjp9XT3ks= -golang.org/x/text v0.3.8 h1:nAL+RVCQ9uMn3vJZbV+MRnydTJFPf8qqY42YiA6MrqY= -golang.org/x/text v0.3.8/go.mod h1:E6s5w1FMmriuDzIBO73fBruAKo1PCIq6d2Q6DHfQ8WQ= +golang.org/x/sys v0.44.0 h1:ildZl3J4uzeKP07r2F++Op7E9B29JRUy+a27EibtBTQ= +golang.org/x/sys v0.44.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= +golang.org/x/text v0.39.0 h1:UbZz4pLOvn600D6Oh6GGEI6VAmndrEBLv8/6BEXzyus= +golang.org/x/text v0.39.0/go.mod h1:3UwRclnC2g0TU9x8PZiyfOajCd1zaUNHF9cvqcQZ+ZM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= diff --git a/internal/api/client.go b/internal/api/client.go index a5d73df..7f642a1 100644 --- a/internal/api/client.go +++ b/internal/api/client.go @@ -5,10 +5,15 @@ import ( "fmt" "io" "net/http" + "time" ) const BaseURL = "https://cloud-api.nan.builders/api" +// A client with no timeout waits forever on a connection that is accepted and +// then never answered, which in the panel is a spinner that never stops. +const requestTimeout = 30 * time.Second + type Client struct { token string http *http.Client @@ -18,7 +23,7 @@ type Client struct { } func New(token string) *Client { - return &Client{token: token, http: &http.Client{}, baseURL: BaseURL} + return &Client{token: token, http: &http.Client{Timeout: requestTimeout}, baseURL: BaseURL} } // Token is what this client sends. The panel builds a client once, at start, @@ -94,7 +99,7 @@ func ListModels(apiKey string) ([]string, error) { } req.Header.Set("Authorization", "Bearer "+apiKey) - resp, err := (&http.Client{}).Do(req) + resp, err := (&http.Client{Timeout: requestTimeout}).Do(req) if err != nil { return nil, err } diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 3e0ceac..1dabbda 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -15,20 +15,31 @@ import ( "net/http" "net/url" "strings" + "time" ) const ( loginRequestURL = "https://cloud-api.nan.builders/api/auth/login/request" loginVerifyURL = "https://cloud-api.nan.builders/api/auth/login/verify" sessionCookie = "nan_session" + // The domain the platform sends its sign-in links from. Subdomains count: + // the link lands on the web app, the API lives next door. + linkDomain = "nan.builders" ) +// A timeout, because http.DefaultClient has none: a connection that is +// accepted and then never answered - a captive portal, a hotel network, a +// firewall that drops instead of refusing - hangs the panel on a spinner with +// no way out but ctrl-c. +const requestTimeout = 30 * time.Second + func RequestSignInLink(email string) error { body, err := json.Marshal(map[string]string{"email": email}) if err != nil { return err } - resp, err := http.Post(loginRequestURL, "application/json", bytes.NewReader(body)) + client := &http.Client{Timeout: requestTimeout} + resp, err := client.Post(loginRequestURL, "application/json", bytes.NewReader(body)) if err != nil { return fmt.Errorf("could not reach nan.builders: %w", err) } @@ -55,9 +66,19 @@ func TokenFromLink(pasted string) (string, error) { if err != nil { return "", fmt.Errorf("that does not parse as a link: %w", err) } + // Ours, or nothing. The token in a link from somewhere else is not one + // this exchange can spend, and a member who has been sent a link that + // only looks like ours is better told that here than after we have + // posted whatever was in it and read back an error about it. + if !isLinkHost(u.Hostname()) { + return "", fmt.Errorf("that link points at %s, not %s", u.Hostname(), linkDomain) + } token := u.Query().Get("token") if token == "" { - return "", fmt.Errorf("that link carries no token: %s", pasted) + // The host and not the link: a link with no `token` in its query + // can still be carrying one somewhere this does not read, and the + // message is rendered in the panel and copied into issues. + return "", fmt.Errorf("that %s link carries no token", u.Hostname()) } return token, nil } @@ -67,12 +88,18 @@ func TokenFromLink(pasted string) (string, error) { return pasted, nil } +func isLinkHost(host string) bool { + host = strings.ToLower(host) + return host == linkDomain || strings.HasSuffix(host, "."+linkDomain) +} + // The browser flow ends on a page that POSTs the token and gets the session // cookie back. This does the same POST and keeps the cookie instead of // following the redirect, which is the whole reason the old flow had to send // people into DevTools. func ExchangeToken(token string) (string, error) { client := &http.Client{ + Timeout: requestTimeout, CheckRedirect: func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse }, diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go new file mode 100644 index 0000000..c3223ff --- /dev/null +++ b/internal/auth/auth_test.go @@ -0,0 +1,71 @@ +package auth + +import ( + "strings" + "testing" +) + +// A sign-in link is a credential in a URL, and this is the only place that +// decides what counts as one. A link from anywhere else carries a token this +// exchange cannot spend, so the answer is no before anything is posted +// anywhere - and the refusal names the host rather than echoing the link, +// because the message is rendered in the panel and pasted into issues. +func TestOnlyNanLinksAreAccepted(t *testing.T) { + for _, c := range []struct { + name string + pasted string + want string + }{ + {"the platform", "https://nan.builders/auth/verify?token=abc", "abc"}, + {"a subdomain of it", "https://cloud-api.nan.builders/api/auth/login/verify?token=abc", "abc"}, + {"the host in caps", "https://NAN.BUILDERS/auth/verify?token=abc", "abc"}, + {"a bare token", "abc", "abc"}, + } { + t.Run(c.name, func(t *testing.T) { + got, err := TokenFromLink(c.pasted) + if err != nil { + t.Fatalf("refused one of ours: %v", err) + } + if got != c.want { + t.Errorf("token = %q, want %q", got, c.want) + } + }) + } + + for _, c := range []struct { + name string + pasted string + }{ + {"somebody else's domain", "https://nan.builders.evil.example/verify?token=abc"}, + {"a lookalike", "https://nan-builders.example/verify?token=abc"}, + {"a name ours is a suffix of", "https://notnan.builders/verify?token=abc"}, + {"plain http on an unrelated host", "http://localhost:8080/verify?token=abc"}, + } { + t.Run(c.name, func(t *testing.T) { + token, err := TokenFromLink(c.pasted) + if err == nil { + t.Fatalf("took a token from %s", c.pasted) + } + if token != "" { + t.Errorf("returned %q anyway", token) + } + }) + } +} + +// The error is shown on screen and copied into issues, so it says which host +// it got rather than handing the whole link - and whatever is in its query - +// along with it. +func TestLinkErrorsDoNotEchoTheLink(t *testing.T) { + const secret = "a-token-nobody-should-see" + for _, pasted := range []string{ + "https://nan.builders/auth/verify#token=" + secret, + "https://elsewhere.example/verify?token=" + secret, + } { + if _, err := TokenFromLink(pasted); err == nil { + t.Fatalf("accepted %s", pasted) + } else if strings.Contains(err.Error(), secret) { + t.Errorf("the error repeats the token: %v", err) + } + } +} diff --git a/internal/session/session.go b/internal/session/session.go index 04ab16b..a94c20c 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -5,6 +5,7 @@ import ( "errors" "os" "path/filepath" + "runtime" ) type Session struct { @@ -51,6 +52,16 @@ func Load() (*Session, error) { return &s, json.Unmarshal(data, &s) } +// Save writes the session, which holds both the token and the API key, so how +// it is written matters as much as where. +// +// os.WriteFile applies its mode only where it CREATES the file: a session.json +// left behind by an older version, or by a restore that widened it, kept +// whatever mode it already had and the 0o600 here did nothing at all. Writing +// to a temp file created 0600 and renaming it over the top is the only version +// of this that ends with the mode we asked for whatever was there before - and +// it never leaves a half-written session.json behind either, which used to +// read on the next run as not being logged in. func Save(s *Session) error { d, err := dir() if err != nil { @@ -63,7 +74,29 @@ func Save(s *Session) error { if err != nil { return err } - return os.WriteFile(filepath.Join(d, "session.json"), data, 0o600) + + tmp, err := os.CreateTemp(d, ".session-*.tmp") + if err != nil { + return err + } + name := tmp.Name() + defer os.Remove(name) // does nothing once the rename has taken it + + if _, err := tmp.Write(data); err != nil { + tmp.Close() + return err + } + // On Windows the mode bits are not the mechanism - the file inherits the + // ACL of the profile directory - so a filesystem that will not take the + // chmod is no reason to fail the login that is being saved. + if err := tmp.Chmod(0o600); err != nil && runtime.GOOS != "windows" { + tmp.Close() + return err + } + if err := tmp.Close(); err != nil { + return err + } + return os.Rename(name, filepath.Join(d, "session.json")) } func Delete() error { diff --git a/internal/session/session_test.go b/internal/session/session_test.go new file mode 100644 index 0000000..4afbd03 --- /dev/null +++ b/internal/session/session_test.go @@ -0,0 +1,81 @@ +package session + +import ( + "os" + "path/filepath" + "runtime" + "testing" +) + +func tempHome(t *testing.T) string { + t.Helper() + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) // os.UserHomeDir reads this one on Windows + return home +} + +// session.json holds the session token and the API key. os.WriteFile applies +// its mode only where it CREATES the file, so a session.json left behind wide +// open - by an older version, by a restore, by a copy out of a backup - kept +// whatever mode it had and went on holding both secrets in the clear. +func TestSavingTightensASessionThatWasWideOpen(t *testing.T) { + tempHome(t) + if err := Save(&Session{Token: "a-token"}); err != nil { + t.Fatal(err) + } + if err := os.Chmod(Path(), 0o644); err != nil { + t.Fatal(err) + } + + if err := Save(&Session{Token: "a-token", APIKey: "a-key"}); err != nil { + t.Fatal(err) + } + if runtime.GOOS == "windows" { + // The mode bits are not the mechanism there - the file inherits the + // ACL of the profile directory it sits in. + return + } + info, err := os.Stat(Path()) + if err != nil { + t.Fatal(err) + } + if mode := info.Mode().Perm(); mode&0o077 != 0 { + t.Errorf("session.json is mode %04o, readable by more than the member", mode) + } +} + +// And it still round-trips, temp file and rename included. +func TestSaveAndLoad(t *testing.T) { + tempHome(t) + want := &Session{Token: "a-token", APIKey: "a-key", EnabledTools: map[string]bool{"Codex": false}} + if err := Save(want); err != nil { + t.Fatal(err) + } + got, err := Load() + if err != nil { + t.Fatal(err) + } + if got.Token != want.Token || got.APIKey != want.APIKey || got.EnabledTools["Codex"] { + t.Errorf("loaded %+v, want %+v", got, want) + } + + // Nothing half-written left beside it: the write goes to a temp file in + // the same directory and is renamed over the top. + entries, err := os.ReadDir(filepath.Dir(Path())) + if err != nil { + t.Fatal(err) + } + for _, e := range entries { + if e.Name() != "session.json" { + t.Errorf("left %s behind in the config directory", e.Name()) + } + } +} + +func TestLoadWithoutASessionSaysSo(t *testing.T) { + tempHome(t) + if _, err := Load(); err != ErrNotLoggedIn { + t.Errorf("Load() on a clean machine = %v, want %v", err, ErrNotLoggedIn) + } +} diff --git a/internal/tui/config_test.go b/internal/tui/config_test.go index 76d40df..3c135e1 100644 --- a/internal/tui/config_test.go +++ b/internal/tui/config_test.go @@ -48,6 +48,35 @@ func readJSON(t *testing.T, path string) map[string]any { return out } +func readFile(t *testing.T, path string) string { + t.Helper() + data, err := os.ReadFile(path) + if os.IsNotExist(err) { + return "" + } + if err != nil { + t.Fatalf("reading %s: %v", path, err) + } + return string(data) +} + +// A file this CLI put a key into is the member's and nobody else's on the +// machine. Windows does not carry the mode bits - the file inherits the ACL of +// the profile directory it sits in - so there is nothing to assert there. +func assertNotWorldReadable(t *testing.T, path string) { + t.Helper() + if runtime.GOOS == "windows" { + return + } + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if mode := info.Mode().Perm(); mode&0o077 != 0 { + t.Errorf("%s is mode %04o, readable by more than the member", filepath.Base(path), mode) + } +} + func TestOpencodeConfigPublishesEveryModelWithItsWindow(t *testing.T) { path := tempConfig(t, "opencode.json") if err := writeOpencodeConfig(path, testKey); err != nil { @@ -686,10 +715,65 @@ func recordHermes(t *testing.T) *[][]string { calls = append(calls, args) return nil } - t.Cleanup(func() { runHermesConfig = original }) + + // And `config get`, which the writer uses to confirm Hermes resolved the + // reference. The fake resolves it the same way Hermes does - out of the + // .env this CLI just wrote - so the check is exercised rather than + // stubbed past. + originalRead := readHermesConfig + readHermesConfig = func(home string, args ...string) (string, error) { + for _, line := range strings.Split(readFile(t, hermesEnvPath(home)), "\n") { + if name, value, ok := strings.Cut(line, "="); ok && strings.TrimSpace(name) == hermesKeyVar { + return value, nil + } + } + return "", nil + } + + t.Cleanup(func() { + runHermesConfig = original + readHermesConfig = originalRead + }) return &calls } +// The reference is only worth writing if Hermes turns it back into the key. +// Nothing in this CLI controls that, so the writer asks - and says so plainly +// rather than leaving a member with a 401 that names nothing. +func TestHermesIsRefusedWhenItDoesNotResolveTheReference(t *testing.T) { + recordHermes(t) + original := readHermesConfig + readHermesConfig = func(home string, args ...string) (string, error) { + return hermesKeyRef, nil // an unexpanded literal, as an older build would + } + t.Cleanup(func() { readHermesConfig = original }) + + err := writeHermesConfig(t.TempDir(), testKey) + if err == nil { + t.Fatal("a Hermes that never resolved the key was reported as configured") + } + if strings.Contains(err.Error(), testKey) { + t.Errorf("the error carries the key: %v", err) + } +} + +// And a Hermes that cannot answer the question at all is not the same as one +// that answered wrongly: an older build without `config get` says nothing +// either way, and failing the step over a question we could not ask would be +// its own bug. +func TestHermesThatCannotAnswerIsNotTreatedAsBroken(t *testing.T) { + recordHermes(t) + original := readHermesConfig + readHermesConfig = func(home string, args ...string) (string, error) { + return "", errors.New("unknown command: get") + } + t.Cleanup(func() { readHermesConfig = original }) + + if err := writeHermesConfig(t.TempDir(), testKey); err != nil { + t.Errorf("an unanswerable check failed the whole step: %v", err) + } +} + func TestHermesIsConfiguredThroughItsOwnConfigCommand(t *testing.T) { calls := recordHermes(t) if err := writeHermesConfig(t.TempDir(), testKey); err != nil { @@ -701,8 +785,9 @@ func TestHermesIsConfiguredThroughItsOwnConfigCommand(t *testing.T) { // endpoint. Its aliases (ollama, vllm, llamacpp) all map to this one. "model.provider": "custom", "model.base_url": "https://api.nan.builders/v1", - "model.api_key": testKey, - "model.default": catalog.Coding, + // The reference, not the secret. The test below is the why. + "model.api_key": hermesKeyRef, + "model.default": catalog.Coding, } got := map[string]string{} for _, c := range *calls { @@ -719,6 +804,72 @@ func TestHermesIsConfiguredThroughItsOwnConfigCommand(t *testing.T) { } } +// The one tool here that is configured by running something, and therefore the +// one place a key could leave this process as an argument. It must not. +// +// An argument is public on the machine for as long as the process lives: `ps` +// hands the whole line to anything running as the member, and on Linux +// /proc//cmdline to other accounts as well. So the key goes into Hermes' +// own .env, the config points at it with ${NAN_API_KEY}, and nothing readable +// from outside this process ever holds the secret itself. +func TestHermesNeverReceivesTheKeyAsAnArgument(t *testing.T) { + calls := recordHermes(t) + home := t.TempDir() + if err := writeHermesConfig(home, testKey); err != nil { + t.Fatal(err) + } + + for _, c := range *calls { + for _, arg := range c { + if strings.Contains(arg, testKey) { + t.Fatalf("the key was handed to hermes as an argument: %v", c) + } + } + } + + env := readFile(t, hermesEnvPath(home)) + if !strings.Contains(env, hermesKeyVar+"="+testKey) { + t.Errorf(".env does not carry the key:\n%s", env) + } + assertNotWorldReadable(t, hermesEnvPath(home)) +} + +// The .env is Hermes', not ours: other providers' keys live in it, with the +// member's own notes around them. +func TestHermesEnvKeepsEverythingElseInTheFile(t *testing.T) { + recordHermes(t) + home := t.TempDir() + envPath := hermesEnvPath(home) + existing := "# their notes\nOPENROUTER_API_KEY=theirs\n" + hermesKeyVar + "=an-older-one\n" + if err := os.WriteFile(envPath, []byte(existing), 0o600); err != nil { + t.Fatal(err) + } + + if err := writeHermesConfig(home, testKey); err != nil { + t.Fatal(err) + } + after := readFile(t, envPath) + for _, want := range []string{"# their notes", "OPENROUTER_API_KEY=theirs", hermesKeyVar + "=" + testKey} { + if !strings.Contains(after, want) { + t.Errorf("after writing, .env has no %q:\n%s", want, after) + } + } + if strings.Contains(after, "an-older-one") { + t.Errorf("the key it replaced is still in .env:\n%s", after) + } + + if err := removeHermesConfig(home); err != nil { + t.Fatal(err) + } + after = readFile(t, envPath) + if strings.Contains(after, testKey) { + t.Errorf("removal left the key in .env:\n%s", after) + } + if !strings.Contains(after, "OPENROUTER_API_KEY=theirs") { + t.Errorf("removal took somebody else's key with it:\n%s", after) + } +} + // Hermes with a custom endpoint asks the cluster what it serves instead of // reading a list we write, so there is no model catalogue in this config and // no window to keep in step - the one thing it needs told is which model to @@ -739,9 +890,19 @@ func TestHermesIsNotSentAModelCatalogue(t *testing.T) { func TestHermesRemovalTakesOnlyWhatWeWrote(t *testing.T) { calls := recordHermes(t) - if err := removeHermesConfig(t.TempDir()); err != nil { + home := t.TempDir() + // A config that is ours. Without one the removal has nothing to unset and + // skips the four calls this is here to check, rather than spawning Hermes + // four times over a config it never touched. + if err := os.WriteFile(hermesConfigPath(home), []byte("model:\n base_url: https://api.nan.builders/v1\n"), 0o600); err != nil { t.Fatal(err) } + if err := removeHermesConfig(home); err != nil { + t.Fatal(err) + } + if len(*calls) == 0 { + t.Fatal("removal unset nothing at all") + } for _, c := range *calls { if c[0] != "unset" { t.Errorf("removal called %v, which is not an unset", c) @@ -801,11 +962,20 @@ func TestHermesConfigAgainstTheRealBinary(t *testing.T) { t.Fatalf("hermes wrote no config: %v", err) } written := string(data) - for _, want := range []string{"provider: custom", "base_url: https://api.nan.builders/v1", "default: " + catalog.Coding, testKey} { + for _, want := range []string{"provider: custom", "base_url: https://api.nan.builders/v1", "default: " + catalog.Coding, hermesKeyRef} { if !strings.Contains(written, want) { t.Errorf("config.yaml has no %q:\n%s", want, written) } } + // Hermes expands ${VAR} in a config value against its own .env, which is + // the whole reason the key can stay out of the command line. If a release + // of Hermes ever stops doing that, this is where it surfaces. + if strings.Contains(written, testKey) { + t.Errorf("config.yaml carries the key itself:\n%s", written) + } + if env := readFile(t, hermesEnvPath(home)); !strings.Contains(env, testKey) { + t.Errorf(".env does not carry the key:\n%s", env) + } if err := removeHermesConfig(home); err != nil { t.Fatal(err) @@ -814,6 +984,9 @@ func TestHermesConfigAgainstTheRealBinary(t *testing.T) { if strings.Contains(string(data), "api.nan.builders") { t.Errorf("removal left the cluster behind:\n%s", data) } + if env := readFile(t, hermesEnvPath(home)); strings.Contains(env, testKey) { + t.Errorf("removal left the key in .env:\n%s", env) + } } // ── the API key, and what the platform will and will not tell us ───────────── @@ -854,21 +1027,36 @@ func TestSetupSaysWhenTheAccountHasNoKeyAtAll(t *testing.T) { } } -// Once there is a key in the field the hint is noise, and the key itself is -// never printed. +// Once there is a key in the field, the hint about where to go and fetch one +// is noise - and the key itself is never printed, here or anywhere. func TestSetupHidesTheHintAndTheKeyOnceOneIsSet(t *testing.T) { m := setupModel(t, &session.Session{APIKey: testKey}) m.keyStatus = &api.KeyStatus{Exists: true, Alias: "an-alias"} out := m.renderSetup(newLayout(80, 24)) - if strings.Contains(out, "an-alias") || strings.Contains(out, "cloud.nan.builders") { - t.Error("the hint is still shown after a key was set") + if strings.Contains(out, "an-alias") || strings.Contains(out, "copy it from") { + t.Error("the hint about fetching a key is still shown after one was set") } if strings.Contains(out, testKey) { t.Error("the API key is printed on screen") } } +// What to do about a key that has been seen by somebody. This CLI cannot +// revoke one - the platform issues and retires them - so the least it can do +// is say where, rather than leaving a member to guess whether replacing it is +// even possible. +func TestSetupSaysHowToReplaceAKeyThatLeaked(t *testing.T) { + m := setupModel(t, &session.Session{APIKey: testKey}) + + out := m.renderSetup(newLayout(80, 24)) + for _, want := range []string{"replace it at cloud.nan.builders", "press c"} { + if !strings.Contains(out, want) { + t.Errorf("the tab does not say %q", want) + } + } +} + // A key the cluster refuses used to be found out five times over, as a 401 // inside each tool it had been written into. It is said once, here, before // anything is written at all. diff --git a/internal/tui/security_test.go b/internal/tui/security_test.go new file mode 100644 index 0000000..8df170d --- /dev/null +++ b/internal/tui/security_test.go @@ -0,0 +1,191 @@ +// Package tui, and what a member's API key is allowed to touch. +// +// Everything here is one question asked five ways: once the key is in this +// process, where can it end up? On disk with the wrong mode, in an argument +// list, in an error rendered on screen, or left behind in a tool's config +// after the member believes they have signed out. Each of those was true at +// some point, which is why each of them is a test. +package tui + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// The tools' configs are theirs, not ours: by the time `nan` writes a key into +// one it usually exists already, created by the tool itself and commonly 0644. +// os.WriteFile does not change the mode of a file it did not create, so the +// key went into a world-readable file and the 0o600 in the call did nothing. +func TestWritingAKeyTightensAConfigThatWasWideOpen(t *testing.T) { + home := toolHome(t) + for _, path := range toolPaths(home) { + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + // As the tool would have left it. + if err := os.WriteFile(path, []byte("{}"), 0o644); err != nil { + t.Fatal(err) + } + } + + if _, _, failed := configureTools(testKey, nil); len(failed) != 0 { + t.Fatalf("tools failed on a clean run: %v", failed) + } + for name, path := range toolPaths(home) { + if !strings.Contains(readFile(t, path), testKey) { + t.Errorf("%s was not configured at all", name) + continue + } + assertNotWorldReadable(t, path) + } +} + +// The tools that are already pointing at the cluster are the ones that have +// held a key the longest, and the writers leave them alone: there is nothing +// to change. Their mode is still whatever it was when the key went in, which +// for a config the tool created itself is world-readable. +func TestConfiguringTightensAToolThatNeedsNothingWritten(t *testing.T) { + home := toolHome(t) + paths := toolPaths(home) + for _, path := range paths { + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte("{}"), 0o600); err != nil { + t.Fatal(err) + } + } + if _, _, failed := configureTools(testKey, nil); len(failed) != 0 { + t.Fatalf("tools failed on a clean run: %v", failed) + } + // As an older version of this CLI would have left them. + for _, path := range paths { + if err := os.Chmod(path, 0o644); err != nil { + t.Fatal(err) + } + } + + // A second run has nothing to write, and has to fix them anyway. + if _, _, failed := configureTools(testKey, nil); len(failed) != 0 { + t.Fatalf("tools failed on the second run: %v", failed) + } + for name, path := range paths { + if !strings.Contains(readFile(t, path), testKey) { + t.Errorf("%s lost its key on the second run", name) + } + assertNotWorldReadable(t, path) + } +} + +// A config this CLI cannot parse is the one case where it must not write: the +// readers used to discard the unmarshal error, read the file as nothing, and +// write it again from scratch - taking every other provider in it, and their +// keys, with them. +func TestAConfigThatDoesNotParseIsLeftExactlyAsItIs(t *testing.T) { + const theirs = `{"provider": {"anthropic": {"apiKey": "theirs"}},,,` + + for _, c := range []struct { + name string + file string + write func(path, key string) error + }{ + {"Factory AI", "settings.json", writeFactoryConfig}, + {"OpenCode", "opencode.json", writeOpencodeConfig}, + {"Pi", "models.json", writePiConfig}, + } { + t.Run(c.name, func(t *testing.T) { + path := tempConfig(t, c.file) + if err := os.WriteFile(path, []byte(theirs), 0o600); err != nil { + t.Fatal(err) + } + + err := c.write(path, testKey) + if err == nil { + t.Fatal("a config that does not parse was written over without a word") + } + if strings.Contains(err.Error(), testKey) { + t.Errorf("the error carries the key: %v", err) + } + if got := readFile(t, path); got != theirs { + t.Errorf("the file was rewritten:\n%s", got) + } + }) + } +} + +// Signing out deletes session.json, which holds the token and the key. It used +// to stop there, and the key was still in every tool configured from the +// panel - working, billing, and out of reach of anyone who thought signing out +// was the end of it. +func TestSigningOutTakesTheKeyOutOfTheTools(t *testing.T) { + home := toolHome(t) + for _, path := range toolPaths(home) { + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, nil, 0o600); err != nil { + t.Fatal(err) + } + } + if _, _, failed := configureTools(testKey, nil); len(failed) != 0 { + t.Fatalf("tools failed on a clean run: %v", failed) + } + + removed, failed := RemoveNanFromTools() + if len(failed) != 0 { + t.Fatalf("removal failed: %v", failed) + } + if len(removed) == 0 { + t.Fatal("removal found nothing to take out, having just written five") + } + for name, path := range toolPaths(home) { + if strings.Contains(readFile(t, path), testKey) { + t.Errorf("%s still carries the key after signing out", name) + } + } + if strings.Contains(readFile(t, hermesEnvPath(filepath.Join(home, "hermes"))), testKey) { + t.Error("Hermes still carries the key after signing out") + } +} + +// The reason a failed `hermes config set` prints. The value is the key. +func TestHermesFailuresDoNotPrintTheValue(t *testing.T) { + err := hermesConfigError([]string{"set", "model.api_key", testKey}, "unknown key\n") + if strings.Contains(err.Error(), testKey) { + t.Fatalf("the key is in the error the panel renders: %v", err) + } + for _, want := range []string{"set", "model.api_key", "unknown key"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("the error no longer says %q: %v", want, err) + } + } +} + +// ── the shared harness ─────────────────────────────────────────────────────── + +// A home directory of this test's own, Hermes included, so nothing here +// reaches the machine it is running on. +func toolHome(t *testing.T) string { + t.Helper() + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) // os.UserHomeDir reads this one on Windows + hermesDir := filepath.Join(home, "hermes") + t.Setenv("HERMES_HOME", hermesDir) + if err := os.MkdirAll(hermesDir, 0o700); err != nil { + t.Fatal(err) + } + recordHermes(t) // no hermes process is spawned, here or anywhere + return home +} + +func toolPaths(home string) map[string]string { + return map[string]string{ + "Factory AI": filepath.Join(home, ".factory", "settings.json"), + "OpenCode": filepath.Join(home, ".config", "opencode", "opencode.json"), + "Pi": filepath.Join(home, ".pi", "agent", "models.json"), + "Codex": filepath.Join(home, ".codex", "config.toml"), + } +} diff --git a/internal/tui/tui.go b/internal/tui/tui.go index 1b300f0..ea1e9e4 100644 --- a/internal/tui/tui.go +++ b/internal/tui/tui.go @@ -249,6 +249,12 @@ func newModel(client *api.Client, sess *session.Session) model { ti := textinput.New() ti.Placeholder = "paste your NaN API key here" ti.CharLimit = 512 + // A key typed in the clear is a key in the scrollback, and from there in + // whatever the member was screen-sharing or recording at the time. The + // saved one is already shown as bullets a few lines down the same tab; + // there was no reason for the field that takes it to be the exception. + ti.EchoMode = textinput.EchoPassword + ti.EchoCharacter = '•' ti.PromptStyle = lipgloss.NewStyle().Foreground(cCyan) ti.TextStyle = lipgloss.NewStyle().Foreground(cText) ti.PlaceholderStyle = lipgloss.NewStyle().Foreground(cDimGray) @@ -333,6 +339,7 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { if msg.err != nil { m.loginMsg = "error: " + msg.err.Error() m.loginStage = loginAskEmail + m.loginInput.EchoMode = textinput.EchoNormal return m, m.loginInput.Focus() } m.wizard = wizardLink @@ -342,6 +349,10 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.loginInput.SetValue("") m.loginInput.Placeholder = "https://nan.builders/...?token=..." m.loginInput.Prompt = "Paste the link: " + // The link carries the token in its query, so from here the field is + // holding a credential and stops echoing one. + m.loginInput.EchoMode = textinput.EchoPassword + m.loginInput.EchoCharacter = '•' return m, m.loginInput.Focus() case signedInMsg: @@ -563,7 +574,7 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { } if !m.confirmSignOut { m.confirmSignOut = true - m.setupMsg = "press o again to sign out, any other key to keep the session" + m.setupMsg = "press o again to sign out — it also takes your key back out of the tools; any other key to keep the session" m.active = tabIndex(tabSetup) break } @@ -575,11 +586,18 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // Everything the session was holding goes with it: the key lives in // the same file, and the tabs are full of answers that were true // for somebody else. + // + // And the key does not only live there. Every tool configured from + // this panel got a copy, where deleting session.json leaves it + // working - so a member who signs out because they are handing the + // machine over would have been signed out of everything except the + // cluster their key bills. + removed, failed := RemoveNanFromTools() m.sess = &session.Session{} m.client = api.New("") m.cache = make(map[tabID]any) m.keyStatus, m.keyAsked, m.keyCheck = nil, false, "" - m.configured, m.setupMsg = nil, "signed out" + m.configured, m.setupMsg = nil, signOutMessage(removed, failed) m.err = nil return m, m.resumeSetup() @@ -678,6 +696,9 @@ func (m *model) startLogin() tea.Cmd { m.loginInput.SetValue("") m.loginInput.Placeholder = "you@example.com" m.loginInput.Prompt = "Email: " + // An email address is not a secret and hiding it only makes it harder to + // see a typo in the thing the link is about to be sent to. + m.loginInput.EchoMode = textinput.EchoNormal return m.loginInput.Focus() } @@ -1834,6 +1855,15 @@ func configureTools(apiKey string, enabledTools map[string]bool) (string, []stri case "Hermes": err = writeHermesConfig(filepath.Dir(t.configPath), apiKey) } + if err == nil { + // Every writer above can decide it has nothing to change - a + // tool already pointing at the cluster is left alone - and + // that file is exactly the one that has been sitting there + // with a key in it since before this CLI started tightening + // the mode on its way past. So the mode is checked whether or + // not anything was written. + err = tightenKeyFiles(t) + } if err != nil { failed = append(failed, toolFailure{t.name, err.Error()}) } else { @@ -1841,19 +1871,7 @@ func configureTools(apiKey string, enabledTools map[string]bool) (string, []stri written = append(written, t.name) } } else if t.configured { - var err error - switch t.name { - case "Factory AI": - err = removeFactoryConfig(t.configPath) - case "OpenCode": - err = removeOpencodeConfig(t.configPath) - case "Pi": - err = removePiConfig(t.configPath) - case "Codex": - err = removeCodexConfig(t.configPath) - case "Hermes": - err = removeHermesConfig(filepath.Dir(t.configPath)) - } + err := removeFromTool(t) if err != nil { failed = append(failed, toolFailure{t.name, err.Error()}) } else { @@ -1882,19 +1900,205 @@ func configureTools(apiKey string, enabledTools map[string]bool) (string, []stri return strings.Join(parts, " · "), written, failed } +// The files that end up holding the key, per tool. Hermes is the odd one: its +// config.yaml carries the reference and its .env carries the secret. +func keyFilesFor(t toolInfo) []string { + if t.name == "Hermes" { + return []string{hermesEnvPath(filepath.Dir(t.configPath))} + } + return []string{t.configPath} +} + +// A config written before this CLI wrote through a temp file kept whatever +// mode the tool gave it, which for most of them is 0644. Rewriting it is not +// always on the table - the writers stop early where there is nothing to +// change - so the mode is put right on its own. +func tightenKeyFiles(t toolInfo) error { + if runtime.GOOS == "windows" { + // Not the mechanism there: the file inherits the ACL of the profile + // directory it sits in. + return nil + } + for _, path := range keyFilesFor(t) { + info, err := os.Stat(path) + if os.IsNotExist(err) { + continue + } + if err != nil { + return err + } + if info.Mode().Perm()&0o077 == 0 { + continue + } + if err := os.Chmod(path, 0o600); err != nil { + return err + } + } + return nil +} + +func removeFromTool(t toolInfo) error { + switch t.name { + case "Factory AI": + return removeFactoryConfig(t.configPath) + case "OpenCode": + return removeOpencodeConfig(t.configPath) + case "Pi": + return removePiConfig(t.configPath) + case "Codex": + return removeCodexConfig(t.configPath) + case "Hermes": + return removeHermesConfig(filepath.Dir(t.configPath)) + } + return nil +} + +// RemoveNanFromTools takes the key back out of every tool this CLI wrote it +// into, and says which ones it managed and which it could not. +// +// Exported because signing out happens in two places - `o` in the panel and +// `nan auth logout` - and deleting session.json is only half of what a member +// means by it. The key was copied into as many as five files that have nothing +// to do with the session any more, and it goes on working from there: a +// machine handed back, a laptop sold, a shared account left behind. +func RemoveNanFromTools() (removed []string, failed []string) { + for _, t := range detectTools() { + // Installed is the only gate. `configured` is read off the tool's own + // config file, and the case that matters most here is the one where + // that file is not the whole story: a run that wrote Hermes' .env and + // then failed before its config.yaml left a key on disk that this + // would have walked straight past. Every remover below is a no-op + // where there is nothing of ours to take out. + if !t.installed { + continue + } + if err := removeFromTool(t); err != nil { + failed = append(failed, t.name+" ("+err.Error()+")") + continue + } + if t.configured { + removed = append(removed, t.name) + } + } + return removed, failed +} + +// What to say afterwards. Signing out and leaving the key behind in four other +// files is worth a sentence either way. +func signOutMessage(removed, failed []string) string { + if len(failed) > 0 { + return "error: signed out, but the key is still in " + strings.Join(failed, ", ") + } + if len(removed) > 0 { + return "signed out · key removed from " + strings.Join(removed, ", ") + } + return "signed out" +} + func factoryCustomID(displayName string, index int) string { return fmt.Sprintf("custom:%s-%d", strings.ReplaceAll(displayName, " ", "-"), index) } -func writeFactoryConfig(cfgPath, apiKey string) error { - // Use map to preserve unknown top-level fields (logoAnimation, etc.) +// ── writing a file that carries a key ──────────────────────────────────────── + +// writeConfigFile writes a tool's config the way a file holding an API key has +// to be written, which is not what os.WriteFile does on its own. +// +// Its mode argument applies only where the file is CREATED, and the usual case +// here is the opposite one: ~/.codex/config.toml and the rest already exist, +// written by the tool itself, commonly 0644. The key went into them and the +// 0o600 in the call did nothing whatsoever - the file kept the mode it had, +// readable by every account on the machine. +// +// And a plain write truncates first: interrupted halfway it leaves a member's +// config - every provider in it, not only ours - cut in two. So the bytes go +// to a temp file in the same directory, which is created 0600, and that file +// is renamed over the destination. The rename happens once or not at all, and +// it carries the temp file's mode with it, which is where the 0600 sticks. +func writeConfigFile(path string, data []byte) error { + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + return err + } + // Managed dotfiles are symlinks into a repo. Renaming over the link would + // replace it with a regular file and quietly detach the config from the + // thing that manages it, so the write follows the link first. + if resolved, err := filepath.EvalSymlinks(path); err == nil { + path = resolved + } + + tmp, err := os.CreateTemp(filepath.Dir(path), ".nan-*.tmp") + if err != nil { + return err + } + name := tmp.Name() + defer os.Remove(name) // does nothing once the rename has taken it + + if _, err := tmp.Write(data); err != nil { + tmp.Close() + return err + } + // On Windows the mode bits are not the mechanism - the file inherits the + // ACL of the profile directory it sits in - so a filesystem that will not + // take the chmod is no reason to refuse to write the config at all. + if err := tmp.Chmod(0o600); err != nil && runtime.GOOS != "windows" { + tmp.Close() + return err + } + if err := tmp.Close(); err != nil { + return err + } + if err := os.Rename(name, path); err != nil { + // Windows will not replace a file another process is holding open, + // where the plain write this replaced would have gone through. The + // mode bits are not the mechanism there in any case, so falling back + // to that write leaves nothing worse than what came before it. On + // every other platform a rename that failed is a failure. + if runtime.GOOS == "windows" { + return os.WriteFile(path, data, 0o600) + } + return err + } + return nil +} + +// readJSONConfig reads a tool's config, and refuses to guess at one it cannot +// parse. +// +// These files are shared: opencode.json and Pi's models.json hold every +// provider a member has, other vendors' API keys included. The readers here +// used to drop the unmarshal error on the floor, which meant a file this CLI +// could not parse - a stray comma, a half-finished hand edit, a schema the +// tool has since moved on to - was read as nothing and then written again from +// scratch, taking every other provider in it along with it. Not being able to +// read the file is precisely the case where it must not be touched. +func readJSONConfig(path string) (map[string]any, error) { + data, err := os.ReadFile(path) + if os.IsNotExist(err) { + return map[string]any{}, nil + } + if err != nil { + return nil, err + } + if strings.TrimSpace(string(data)) == "" { + return map[string]any{}, nil + } var cfg map[string]any - if data, err := os.ReadFile(cfgPath); err == nil { - _ = json.Unmarshal(data, &cfg) + if err := json.Unmarshal(data, &cfg); err != nil { + return nil, fmt.Errorf("%s does not parse as JSON, so it was left exactly as it is: %w", + filepath.Base(path), err) } if cfg == nil { cfg = map[string]any{} } + return cfg, nil +} + +func writeFactoryConfig(cfgPath, apiKey string) error { + // A map, so unknown top-level fields (logoAnimation, etc.) survive. + cfg, err := readJSONConfig(cfgPath) + if err != nil { + return err + } // Extract existing customModels var models []map[string]any @@ -1955,23 +2159,17 @@ func writeFactoryConfig(cfgPath, apiKey string) error { } } - if err := os.MkdirAll(filepath.Dir(cfgPath), 0o700); err != nil { - return err - } data, err := json.MarshalIndent(cfg, "", " ") if err != nil { return err } - return os.WriteFile(cfgPath, data, 0o600) + return writeConfigFile(cfgPath, data) } func writeOpencodeConfig(cfgPath, apiKey string) error { - var cfg map[string]any - if data, err := os.ReadFile(cfgPath); err == nil { - _ = json.Unmarshal(data, &cfg) - } - if cfg == nil { - cfg = map[string]any{} + cfg, err := readJSONConfig(cfgPath) + if err != nil { + return err } providers, _ := cfg["provider"].(map[string]any) @@ -2036,7 +2234,7 @@ func writeOpencodeConfig(cfgPath, apiKey string) error { if err != nil { return err } - return os.WriteFile(cfgPath, data, 0o600) + return writeConfigFile(cfgPath, data) } } } @@ -2055,14 +2253,11 @@ func writeOpencodeConfig(cfgPath, apiKey string) error { } cfg["provider"] = providers - if err := os.MkdirAll(filepath.Dir(cfgPath), 0o700); err != nil { - return err - } data, err := json.MarshalIndent(cfg, "", " ") if err != nil { return err } - return os.WriteFile(cfgPath, data, 0o600) + return writeConfigFile(cfgPath, data) } // Pi takes a provider two ways: a models.json, which is data, or an extension @@ -2075,12 +2270,9 @@ func writeOpencodeConfig(cfgPath, apiKey string) error { func writePiConfig(cfgPath, apiKey string) error { // Unlike the extension file, models.json is shared: other providers live in // it and none of them are ours to touch. - var cfg map[string]any - if data, err := os.ReadFile(cfgPath); err == nil { - _ = json.Unmarshal(data, &cfg) - } - if cfg == nil { - cfg = map[string]any{} + cfg, err := readJSONConfig(cfgPath) + if err != nil { + return err } providers, _ := cfg["providers"].(map[string]any) @@ -2110,14 +2302,11 @@ func writePiConfig(cfgPath, apiKey string) error { } cfg["providers"] = providers - if err := os.MkdirAll(filepath.Dir(cfgPath), 0o700); err != nil { - return err - } data, err := json.MarshalIndent(cfg, "", " ") if err != nil { return err } - if err := os.WriteFile(cfgPath, data, 0o600); err != nil { + if err := writeConfigFile(cfgPath, data); err != nil { return err } return writePiDefaults(piSettingsPath(cfgPath)) @@ -2128,12 +2317,9 @@ func writePiConfig(cfgPath, apiKey string) error { // optional" for a reason: with models.json alone Pi goes on calling its // factory provider, and what the member sees is a 401 that names neither file. func writePiDefaults(settingsPath string) error { - var settings map[string]any - if data, err := os.ReadFile(settingsPath); err == nil { - _ = json.Unmarshal(data, &settings) - } - if settings == nil { - settings = map[string]any{} + settings, err := readJSONConfig(settingsPath) + if err != nil { + return err } // A member who already picked a default picked it, and ours is one more @@ -2145,14 +2331,11 @@ func writePiDefaults(settingsPath string) error { settings["defaultProvider"] = "nan" settings["defaultModel"] = catalog.Coding - if err := os.MkdirAll(filepath.Dir(settingsPath), 0o700); err != nil { - return err - } out, err := json.MarshalIndent(settings, "", " ") if err != nil { return err } - return os.WriteFile(settingsPath, out, 0o600) + return writeConfigFile(settingsPath, out) } // settings.json sits next to models.json in Pi's agent directory. @@ -2208,7 +2391,7 @@ func removePiConfig(cfgPath string) error { if err != nil { return err } - return os.WriteFile(cfgPath, out, 0o600) + return writeConfigFile(cfgPath, out) } // Only the default we wrote, and only while it still points at us: anything @@ -2236,7 +2419,7 @@ func removePiDefaults(settingsPath string) error { if err != nil { return err } - return os.WriteFile(settingsPath, out, 0o600) + return writeConfigFile(settingsPath, out) } func writeCodexConfig(cfgPath, apiKey string) error { @@ -2262,10 +2445,7 @@ base_url = "https://api.nan.builders/v1" experimental_bearer_token = %q wire_api = "chat" `, codexModel.ID, codexModel.Context, apiKey) - if err := os.MkdirAll(filepath.Dir(cfgPath), 0o700); err != nil { - return err - } - return os.WriteFile(cfgPath, []byte(content), 0o600) + return writeConfigFile(cfgPath, []byte(content)) } // Existing config: only append the provider section; preserve user's model/provider choices. @@ -2280,10 +2460,7 @@ experimental_bearer_token = %q wire_api = "chat" `, codexModel.Context, apiKey) content := strings.TrimRight(string(data), "\n") + "\n" + section - if err := os.MkdirAll(filepath.Dir(cfgPath), 0o700); err != nil { - return err - } - return os.WriteFile(cfgPath, []byte(content), 0o600) + return writeConfigFile(cfgPath, []byte(content)) } // ── Hermes ─────────────────────────────────────────────────────────────────── @@ -2310,11 +2487,41 @@ var runHermesConfig = func(home string, args ...string) error { cmd.Env = append(os.Environ(), "HERMES_HOME="+home) out, err := cmd.CombinedOutput() if err != nil { - return fmt.Errorf("hermes config %s: %s", strings.Join(args, " "), strings.TrimSpace(string(out))) + return hermesConfigError(args, string(out)) } return nil } +// The verb and the key it was called with, and never the value. +// +// This used to join every argument into the message, and the message is +// rendered in the panel: one `set` that Hermes refused printed the member's +// API key onto their screen, into their scrollback, and into the issue they +// then pasted all of it into. Nothing past the key belongs in an error. +func hermesConfigError(args []string, out string) error { + said := args + if len(said) > 2 { + said = said[:2] + } + return fmt.Errorf("hermes config %s: %s", strings.Join(said, " "), strings.TrimSpace(out)) +} + +// Reading a value back, for the one thing that has to be confirmed rather than +// assumed. Swapped in tests alongside the writer above. +var readHermesConfig = func(home string, args ...string) (string, error) { + cmd := exec.Command("hermes", append([]string{"config"}, args...)...) + cmd.Env = append(os.Environ(), "HERMES_HOME="+home) + out, err := cmd.Output() + if err != nil { + var exit *exec.ExitError + if errors.As(err, &exit) { + return "", hermesConfigError(args, string(exit.Stderr)) + } + return "", hermesConfigError(args, err.Error()) + } + return strings.TrimSpace(string(out)), nil +} + // The resolution Hermes itself uses: HERMES_HOME, then the platform default. func hermesHome() string { if home := strings.TrimSpace(os.Getenv("HERMES_HOME")); home != "" { @@ -2335,11 +2542,41 @@ func hermesConfigPath(home string) string { return filepath.Join(home, "config.yaml") } +// Hermes keeps its secrets in a .env beside its config - it has a `hermes +// config env-path` for exactly this - and expands ${VAR} in any config value +// against it when it loads. So the config gets the placeholder and the .env +// gets the key. +const ( + hermesEnvFile = ".env" + hermesKeyVar = "NAN_API_KEY" + hermesKeyRef = "${" + hermesKeyVar + "}" + hermesKeyNote = "# Written by nan-cli. config.yaml points at it as " + hermesKeyRef + "." +) + +func hermesEnvPath(home string) string { + return filepath.Join(home, hermesEnvFile) +} + func writeHermesConfig(home, apiKey string) error { + // The key goes into Hermes' .env, and the config points at it. Not because + // the config is a worse place to keep it - same machine, same member, same + // 0600 - but because of the way it would have to get there. + // + // Every value here is written by running `hermes config set `, + // and an argument is not private: on Linux /proc//cmdline is readable + // by other accounts on the box, and `ps` hands it to anything running as + // the member. A key passed that way is exposed for as long as the process + // lives, to readers this CLI never meant to hand it to, and there is no + // taking it back afterwards. The placeholder is not a secret and can go + // through argv; the key is written straight to disk and appears in no + // argument list at all. + if err := writeHermesEnvKey(hermesEnvPath(home), apiKey); err != nil { + return err + } settings := [][2]string{ {"model.provider", "custom"}, {"model.base_url", "https://api.nan.builders/v1"}, - {"model.api_key", apiKey}, + {"model.api_key", hermesKeyRef}, {"model.default", catalog.Coding}, } for _, s := range settings { @@ -2347,18 +2584,115 @@ func writeHermesConfig(home, apiKey string) error { return err } } + return confirmHermesResolvesTheKey(home, apiKey) +} + +// Everything above rests on Hermes expanding ${VAR} in a config value against +// its own .env, which it does today and which nothing here controls. Asking it +// what it ended up with is the only way to know it happened: a build that +// stopped expanding would leave the literal ${NAN_API_KEY} in place as the +// key, and what a member would see is a 401 from the cluster naming nothing in +// particular. +// +// The answer is the key itself, so it is compared and dropped on the spot. It +// does not go into a message, a log or an error. +func confirmHermesResolvesTheKey(home, apiKey string) error { + resolved, err := readHermesConfig(home, "get", "model.api_key") + if err != nil { + // No answer is not a wrong answer. An older Hermes without this + // subcommand, or one that could not be run twice, says nothing either + // way about what it will do with the config - and failing the whole + // step over a question we could not ask would be its own bug. + return nil + } + if resolved != apiKey { + return fmt.Errorf("Hermes did not resolve %s from its .env, so it has no usable key - "+ + "nan.builders/docs/hermes has the manual steps", hermesKeyRef) + } return nil } +// One variable set in the .env, and the rest of the file - a long commented +// document with the member's other providers in it - left as it was. +func writeHermesEnvKey(envPath, apiKey string) error { + var lines []string + if data, err := os.ReadFile(envPath); err == nil { + if body := strings.TrimRight(string(data), "\n"); body != "" { + lines = strings.Split(body, "\n") + } + } else if !os.IsNotExist(err) { + return err + } + + entry := hermesKeyVar + "=" + apiKey + replaced := false + for i, line := range lines { + if isHermesKeyLine(line) { + lines[i] = entry + replaced = true + } + } + if !replaced { + if len(lines) > 0 { + lines = append(lines, "") + } + lines = append(lines, hermesKeyNote, entry) + } + return writeConfigFile(envPath, []byte(strings.Join(lines, "\n")+"\n")) +} + +// The assignment, and not a commented-out example of the same name: blanking +// one of those would leave the real one further down the file still winning. +func isHermesKeyLine(line string) bool { + name, _, ok := strings.Cut(line, "=") + return ok && strings.TrimSpace(name) == hermesKeyVar +} + // The same four keys and nothing else: a member's channels, skills and // persona live in this file too. func removeHermesConfig(home string) error { - for _, key := range []string{"model.api_key", "model.base_url", "model.default", "model.provider"} { - if err := runHermesConfig(home, "unset", key); err != nil { - return err + // Four processes, so only where there is something to unset. The .env + // below is checked either way: it is written before the first of these + // calls, so a run that failed part-way through leaves the key there and + // nothing in config.yaml to say so. + if cfg, err := os.ReadFile(hermesConfigPath(home)); err == nil && strings.Contains(string(cfg), "api.nan.builders") { + for _, key := range []string{"model.api_key", "model.base_url", "model.default", "model.provider"} { + if err := runHermesConfig(home, "unset", key); err != nil { + return err + } } } - return nil + // And the key the config was pointing at. Unsetting model.api_key drops + // the reference, which leaves the secret itself sitting in the .env. + return removeHermesEnvKey(hermesEnvPath(home)) +} + +func removeHermesEnvKey(envPath string) error { + data, err := os.ReadFile(envPath) + if os.IsNotExist(err) { + return nil + } + if err != nil { + return err + } + + var kept []string + dropped := false + for _, line := range strings.Split(strings.TrimRight(string(data), "\n"), "\n") { + if isHermesKeyLine(line) || line == hermesKeyNote { + dropped = true + continue + } + kept = append(kept, line) + } + if !dropped { + return nil + } + // A .env that held nothing but our two lines was ours to begin with. + if len(kept) == 0 { + return os.Remove(envPath) + } + return writeConfigFile(envPath, []byte(strings.Join(kept, "\n")+"\n")) } func removeCodexConfig(cfgPath string) error { @@ -2366,6 +2700,11 @@ func removeCodexConfig(cfgPath string) error { if err != nil { return nil } + // Nothing of ours in it, so nothing to rewrite: this is called on every + // sign-out, against a config.toml that may never have been ours at all. + if !strings.Contains(string(data), "[model_providers.nan]") { + return nil + } lines := strings.Split(string(data), "\n") var out []string inNanSection := false @@ -2389,7 +2728,7 @@ func removeCodexConfig(cfgPath string) error { if result == "\n" { return os.Remove(cfgPath) } - return os.WriteFile(cfgPath, []byte(result), 0o600) + return writeConfigFile(cfgPath, []byte(result)) } func removeFactoryConfig(cfgPath string) error { @@ -2440,7 +2779,7 @@ func removeFactoryConfig(cfgPath string) error { if err != nil { return err } - return os.WriteFile(cfgPath, out, 0o600) + return writeConfigFile(cfgPath, out) } func removeOpencodeConfig(cfgPath string) error { @@ -2465,7 +2804,7 @@ func removeOpencodeConfig(cfgPath string) error { if err != nil { return err } - return os.WriteFile(cfgPath, out, 0o600) + return writeConfigFile(cfgPath, out) } // The sign-in questions, drawn where the tab content would be. @@ -2706,6 +3045,14 @@ func (m model) renderSetup(l layout) string { b.WriteString(l.indent + labelStyle.Render("Key:") + accentStyle.Render(strings.Repeat("•", 24)) + " " + dimStyle.Render("e to edit") + "\n") + // The one thing a member needs and had nowhere to read: what to do if + // this key has been somewhere it should not have been. Nothing in this + // CLI can revoke it - the platform issues and retires keys - so the + // most useful thing the tab can do is name the steps and where the + // first one happens. + b.WriteString(l.indent + dimStyle.Render( + "if anyone has seen it: replace it at cloud.nan.builders, "+ + "paste the new one here (e), then press c") + "\n") } else { b.WriteString(l.indent + warnStyle.Render("No API key set") + " " + dimStyle.Render("e to set") + "\n") diff --git a/scripts/install.ps1 b/scripts/install.ps1 index 1b79c4a..2a7411c 100644 --- a/scripts/install.ps1 +++ b/scripts/install.ps1 @@ -133,6 +133,71 @@ function Assert-Checksum($file, $expected) { } } +# The checksum says the download arrived whole. It does not say who built it: +# the release that serves the archive serves checksums.txt too, so a release +# somebody else published matches its own numbers perfectly. The build +# provenance answers that, and `gh` is what reads it. +# +# Where gh is not installed this says so and carries on - refusing to install +# without a tool most people do not have would only teach everyone to skip the +# step. Where gh IS installed and refuses, the difference between a bad archive +# and an unreachable GitHub is the whole question, so it asks the API something +# trivial to tell them apart. +function Assert-Provenance($file) { + if (-not (Get-Command gh -ErrorAction SilentlyContinue)) { + Write-Step "gh is not installed, so the build provenance was not checked" + Write-Step " to check it yourself later: gh attestation verify --repo $Repo" + return + } + + # stderr to a file rather than merged with 2>&1: in Windows PowerShell 5.1 + # merging a native command's stderr wraps every line in an ErrorRecord, which + # under ErrorActionPreference Stop throws on output that is not an error. + $log = [System.IO.Path]::GetTempFileName() + try { + & gh attestation verify $file --repo $Repo > $null 2> $log + $code = $LASTEXITCODE + $said = Get-Content -Raw -Path $log -ErrorAction SilentlyContinue + } finally { + Remove-Item $log -Force -ErrorAction SilentlyContinue + } + + if ($code -eq 0) { + Write-Done "provenance verified: built by $Repo on GitHub Actions" + return + } + + # Nothing recorded against these bytes, which is a 404 from the attestations + # API. A release from before this repo signed anything looks exactly like an + # archive that is not the one it signed, because a replaced archive has a + # digest nothing was ever signed for. So this is a notice and not a + # guarantee, and NAN_REQUIRE_PROVENANCE makes it refuse. + if ($said -match 'HTTP 404' -or $said -match 'no attestations found') { + if ($env:NAN_REQUIRE_PROVENANCE) { + throw @" +no build provenance is recorded for this archive +nothing was installed, because NAN_REQUIRE_PROVENANCE is set +"@ + } + Write-Warn "no build provenance is recorded for this archive" + Write-Warn "releases published before this repo started signing carry none" + Write-Warn " set NAN_REQUIRE_PROVENANCE=1 to refuse those" + return + } + + # Something was recorded and it did not match, or gh could not ask at all. + & gh api rate_limit > $null 2> $null + if ($LASTEXITCODE -eq 0) { + throw @" +the build provenance of this archive does not check out +it is not what $Repo published, whatever its checksum says +nothing was installed +"@ + } + Write-Warn "could not reach GitHub to check the build provenance" + Write-Warn "the checksum did match, so this is most likely the network" +} + function Add-ToUserPath($dir) { # The user PATH, not the machine one: no elevation, and it survives reboots, # which setting it only in this process would not. @@ -199,6 +264,9 @@ function Install-NanCli { } Assert-Checksum (Join-Path $tmp $archive) $expected + Write-Step 'checking who built it...' + Assert-Provenance (Join-Path $tmp $archive) + Expand-Archive -Path (Join-Path $tmp $archive) -DestinationPath $tmp -Force $binary = Join-Path $tmp 'nan.exe' if (-not (Test-Path $binary)) { diff --git a/scripts/install.sh b/scripts/install.sh index ef5c44f..08f6466 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -98,8 +98,13 @@ verify_checksum() { elif command -v shasum &>/dev/null; then actual="$(shasum -a 256 "$file" | cut -d' ' -f1)" else - warn "no sha256 tool found, skipping checksum verification" - return 0 + # Not a warning. The checksum is the only thing standing between this + # script and a binary that is not the one the release published, so + # carrying on without it installs exactly what the check exists to catch. + err "no sha256 tool found (sha256sum or shasum), so the download cannot be verified" + err "install one of them, or take the release from" + err " https://github.com/$REPO/releases" + exit 1 fi if [ "$actual" != "$expected" ]; then err "checksum mismatch" @@ -109,6 +114,63 @@ verify_checksum() { fi } +# The checksum above says the download arrived whole. It does not say who +# built it: the release that serves the archive serves checksums.txt too, so a +# release someone else published matches its own numbers perfectly. +# +# The build provenance is the part that answers that, and `gh` is what reads +# it. Not everyone has gh, and refusing to install without it would only teach +# people to skip the step - so this asks where it can, and is careful about the +# difference between "this binary is not what it claims to be" and "I could not +# reach GitHub to find out". +verify_provenance() { + local file="$1" out + + if ! command -v gh &>/dev/null; then + info "gh is not installed, so the build provenance was not checked" + info " to check it yourself later: gh attestation verify --repo $REPO" + return 0 + fi + + if out="$(gh attestation verify "$file" --repo "$REPO" 2>&1)"; then + log "provenance verified: built by $REPO on GitHub Actions" + return 0 + fi + + # Nothing recorded against these bytes, which is a 404 from the attestations + # API. Two different things look identical from out here: a release from + # before this repo signed anything, and an archive that is not the one it + # signed - because a replaced archive has a digest nothing was ever signed + # for. So this is a notice by default and not a guarantee, and it is fatal + # for anyone who sets REQUIRE_PROVENANCE, which every release from now on + # can satisfy. + case "$out" in + *"HTTP 404"*|*"no attestations found"*) + if [ -n "${REQUIRE_PROVENANCE:-}" ]; then + err "no build provenance is recorded for this archive" + err "nothing was installed, because REQUIRE_PROVENANCE is set" + exit 1 + fi + warn "no build provenance is recorded for this archive" + warn "releases published before this repo started signing carry none" + warn " REQUIRE_PROVENANCE=1 refuses to install those" + return 0 + ;; + esac + + # Something was recorded and it did not match, or gh could not ask. Asking + # the API something trivial tells those apart: if it answers, gh works, and + # the refusal above was about this archive. + if gh api rate_limit >/dev/null 2>&1; then + err "the build provenance of this archive does not check out" + err "it is not what $REPO published, whatever its checksum says" + err "nothing was installed" + exit 1 + fi + warn "could not reach GitHub to check the build provenance" + warn "the checksum did match, so this is most likely the network" +} + install_bin() { local src="$1" dest_dir="$2" if install -d "$dest_dir" && install -m 755 "$src" "$dest_dir/nan" 2>/dev/null; then @@ -160,6 +222,9 @@ main() { expected_checksum="$(curl -fsSL "$checksums_url" | grep "$archive" | cut -d' ' -f1)" verify_checksum "$tmpdir/$archive" "$expected_checksum" + info "checking who built it..." + verify_provenance "$tmpdir/$archive" + tar xz -C "$tmpdir" -f "$tmpdir/$archive" info "installing to ${INSTALL_DIR}/nan..."