From 4287b7b65087d518c20e2f10e86606e633b4bb1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Untersch=C3=BCtz?= Date: Sun, 15 Nov 2026 23:25:28 +0100 Subject: [PATCH] fix: harden config loading and ssh addresses, stop resolver on API errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - read netssh.yaml from the user config directory only, not from the working directory - refuse addresses from NetBox or the cache that are neither an IP address nor a hostname (ssh read "-oProxyCommand=…" as an option) - resolver chain stops at an API failure; only "no address" falls through to the next strategy - netssh --help and -h show netssh's help - start at most one background cache refresh per 5 minutes - validate the legacy netbox.url - page through all IP addresses of a host - CI: run format check, vet, and tests on pull requests and main - changelog for v0.0.25 Co-Authored-By: Claude Fable 5.2 --- .gitea/workflows/test.yml | 33 ++++++++++++++ CHANGELOG.md | 11 +++++ README.md | 2 +- cmd/netssh/main.go | 20 ++++++++- cmd/netssh/uninstall.go | 1 + internal/cache/cache.go | 20 +++++++++ internal/cache/cache_test.go | 20 +++++++++ internal/config/config.go | 5 ++- internal/config/config_test.go | 24 ++++++++++ internal/config/instances.go | 12 ++++- internal/netbox/client.go | 57 ++++++++++++++---------- internal/netbox/client_test.go | 45 +++++++++++++++++++ internal/resolver/chain.go | 7 +++ internal/resolver/chain_test.go | 14 ++++++ internal/resolver/interface_name.go | 6 +++ internal/resolver/interface_name_test.go | 14 ++++++ internal/ssh/options.go | 13 ++++++ internal/ssh/options_test.go | 14 ++++++ internal/storage/file_test.go | 29 ++++++++++++ 19 files changed, 317 insertions(+), 30 deletions(-) create mode 100644 .gitea/workflows/test.yml create mode 100644 internal/storage/file_test.go diff --git a/.gitea/workflows/test.yml b/.gitea/workflows/test.yml new file mode 100644 index 0000000..955b19a --- /dev/null +++ b/.gitea/workflows/test.yml @@ -0,0 +1,33 @@ +name: Test + +on: + pull_request: + push: + branches: + - main + +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + + - uses: actions/setup-go@v7 + with: + go-version-file: go.mod + cache: true + + - name: Check formatting + run: | + unformatted=$(gofmt -l .) + if [ -n "$unformatted" ]; then + echo "gofmt needed for:" + echo "$unformatted" + exit 1 + fi + + - name: Vet + run: go vet ./... + + - name: Run tests + run: go test -race ./... diff --git a/CHANGELOG.md b/CHANGELOG.md index 7191173..1e659e8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,16 @@ # Changelog +## v0.0.25 + +- Security: `netssh.yaml` is read from the user config directory only. A file of that name in the working directory (e.g. of a cloned repository) was loaded when no user config existed, and could set ssh options such as `ProxyCommand` or a NetBox URL. +- Security: an address from NetBox or the cache that is neither an IP address nor a hostname is refused. A value such as `-oProxyCommand=…` was passed to ssh as an option. +- The resolver chain stops at an API failure instead of trying the next strategy. Before, a NetBox error in `management_subnet` or `interface_name` connected to the primary IP and cached it. A strategy that finds no address (including a missing interface) still falls through. Note: a token without permission to read IP addresses now gives an error instead of silently using `primary_ip`. +- `netssh --help` and `netssh -h` show netssh's help instead of `unknown option --help` and ssh's usage. +- Background cache refreshes are started at most every 5 minutes. While NetBox was unreachable, every command and every new shell started another one. +- `netbox.url` is validated like the URLs of named instances (HTTP or HTTPS, no query or fragment). +- Hosts with more than 100 IP addresses: all pages are read, so `management_subnet` no longer misses addresses. +- CI: tests, `go vet`, and a format check run on pull requests and pushes to `main`. + ## v0.0.24 - Fix the `interface_name` resolver strategy: it sent filter names that NetBox ignores and therefore returned the host's first IP address instead of the named interface's. It now uses NetBox's `interface` and `vminterface` filters; a missing interface falls through to the next strategy. diff --git a/README.md b/README.md index d503e8f..a39bd03 100644 --- a/README.md +++ b/README.md @@ -200,7 +200,7 @@ shortcuts: Single-instance and shared settings can be overridden with environment variables (`NETSSH_NETBOX_URL`, `NETSSH_NETBOX_TOKEN`, etc.; see [Environment variables](#environment-variables)). -netssh reads `netssh.yaml` from the user config directory and, if none exists there, from the current working directory. Unknown keys are ignored; invalid values (an unknown strategy, a bad regex, an invalid SSH option) stop netssh with an error that names the key. +netssh reads `netssh.yaml` from the user config directory only; a file of that name in the working directory is ignored. Unknown keys are ignored; invalid values (an unknown strategy, a bad regex, an invalid SSH option) stop netssh with an error that names the key. ### Configuration reference diff --git a/cmd/netssh/main.go b/cmd/netssh/main.go index fd233bf..dda5228 100644 --- a/cmd/netssh/main.go +++ b/cmd/netssh/main.go @@ -62,6 +62,16 @@ func main() { return } + // ssh has neither option, so both can only mean netssh's own help. + if args[0] == "--help" || args[0] == "-h" { + root := rootCmd() + root.SetArgs([]string{"help"}) + if err := root.Execute(); err != nil { + os.Exit(1) + } + return + } + if managedSubcommands[args[0]] { if err := rootCmd().Execute(); err != nil { os.Exit(1) @@ -320,6 +330,9 @@ func findSSHHost(ctx context.Context, client *netbox.Client, name string, domain // connectHost applies the SSH options configured for the host and execs the client. func connectHost(cfg *config.Config, client internalssh.Client, flags internalssh.WrapperFlags, host resolvedHost, dests []int, args []string) { + if !internalssh.ValidAddress(host.IP) { + fatalf("refusing address %q for %s: not an IP address or hostname (check NetBox, or run netssh cache clear)", host.IP, host.Name) + } opts := hostOptions(cfg, host) if len(flags.Forwards) > 0 { forwardArgs, err := internalssh.ForwardArgs(flags.Forwards, opts.Forwards) @@ -1000,6 +1013,9 @@ func hookUninstallCmd() *cobra.Command { return cmd } +// refreshRetryInterval is the pause between background refresh attempts. +const refreshRetryInterval = 5 * time.Minute + // maybeBackgroundRefresh starts a background `netssh cache refresh` if the cache // has not been fully refreshed within the last 24 hours. func maybeBackgroundRefresh(cfg *config.Config, c *cache.Cache) { @@ -1007,7 +1023,9 @@ func maybeBackgroundRefresh(cfg *config.Config, c *cache.Cache) { if !cfg.HasNetBox() || os.Getenv("NETSSH_NO_REFRESH") != "" { return } - if !c.NeedsRefresh(24 * time.Hour) { + // While NetBox is unreachable the cache stays stale; without the retry + // interval every command and every new shell would start another refresh. + if !c.ClaimRefresh(24*time.Hour, refreshRetryInterval) { return } self, err := os.Executable() diff --git a/cmd/netssh/uninstall.go b/cmd/netssh/uninstall.go index dc01240..125e9cf 100644 --- a/cmd/netssh/uninstall.go +++ b/cmd/netssh/uninstall.go @@ -71,6 +71,7 @@ func buildUninstallPlan(cfg *config.Config, keepConfig bool) uninstallPlan { addPath(path) addPath(path + ".lock") addPath(filepath.Join(filepath.Dir(path), "last_refresh")) + addPath(filepath.Join(filepath.Dir(path), "refresh_attempt")) if strings.HasPrefix(filepath.Base(filepath.Dir(path)), "instances-") { addPath(filepath.Dir(path)) } diff --git a/internal/cache/cache.go b/internal/cache/cache.go index e53581a..7bf935d 100644 --- a/internal/cache/cache.go +++ b/internal/cache/cache.go @@ -83,6 +83,26 @@ func (c *Cache) NeedsRefresh(d time.Duration) bool { return time.Since(t) >= d } +// ClaimRefresh reports whether a background refresh should start now: the last +// full refresh is older than d and no attempt was claimed within retry. It +// records the attempt, so parallel shells and repeated commands start one +// refresh per retry interval instead of one each. +func (c *Cache) ClaimRefresh(d, retry time.Duration) bool { + if !c.NeedsRefresh(d) { + return false + } + attempt := filepath.Join(filepath.Dir(c.refreshStamp), "refresh_attempt") + if info, err := os.Stat(attempt); err == nil && time.Since(info.ModTime()) < retry { + return false + } + if err := os.MkdirAll(filepath.Dir(attempt), 0o755); err != nil { + return true // cannot record the attempt; refreshing is still right + } + // Best effort: two processes racing here may both refresh once. + _ = os.WriteFile(attempt, nil, 0o600) + return true +} + // SetRefreshed records the current time as the last successful full refresh. func (c *Cache) SetRefreshed() error { if c.refreshStamp == "" { diff --git a/internal/cache/cache_test.go b/internal/cache/cache_test.go index 7e1d649..989ffc5 100644 --- a/internal/cache/cache_test.go +++ b/internal/cache/cache_test.go @@ -557,3 +557,23 @@ func TestStaticHosts(t *testing.T) { t.Fatalf("removed static host still on disk:\n%s", data) } } + +func TestClaimRefreshThrottlesAttempts(t *testing.T) { + c := New(filepath.Join(t.TempDir(), "netssh", "hosts.json"), 60) + if !c.ClaimRefresh(24*time.Hour, time.Minute) { + t.Fatal("a never refreshed cache must claim a refresh") + } + // NetBox is down: the stamp stays stale, but the attempt is remembered. + if c.ClaimRefresh(24*time.Hour, time.Minute) { + t.Error("a second refresh within the retry interval must not start") + } + if !c.ClaimRefresh(24*time.Hour, 0) { + t.Error("after the retry interval a refresh must start again") + } + if err := c.SetRefreshed(); err != nil { + t.Fatal(err) + } + if c.ClaimRefresh(24*time.Hour, 0) { + t.Error("a freshly refreshed cache must not claim a refresh") + } +} diff --git a/internal/config/config.go b/internal/config/config.go index e8ee0ee..1a401c4 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -113,8 +113,9 @@ func Load() (*Config, error) { if err == nil { v.SetConfigName("netssh") v.SetConfigType("yaml") - v.AddConfigPath(filepath.Join(configDir)) - v.AddConfigPath(".") + // Only the user's own directory: a netssh.yaml in the working directory + // (e.g. of a cloned repository) could inject ssh options and a NetBox URL. + v.AddConfigPath(configDir) } v.SetEnvPrefix("NETSSH") diff --git a/internal/config/config_test.go b/internal/config/config_test.go index f9e6997..5d8a021 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -196,3 +196,27 @@ func TestLoad_InvalidShortcutRewrite(t *testing.T) { } } } + +// A netssh.yaml in the working directory (e.g. of a cloned repository) must +// not be loaded: it could inject ssh options such as ProxyCommand. +func TestLoad_IgnoresConfigInWorkingDirectory(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + t.Setenv("NETSSH_NETBOX_URL", "") + t.Chdir(writeConfig(t, "hosts:\n - name: evil\n address: 10.0.0.1\nssh:\n options:\n ProxyCommand: touch /tmp/pwned\n")) + cfg, err := Load() + if err != nil { + t.Fatalf("Load: %v", err) + } + if len(cfg.Hosts) != 0 || len(cfg.SSH.Options) != 0 { + t.Errorf("config from the working directory was loaded: %+v", cfg) + } +} + +func TestLoad_RejectsInvalidLegacyNetBoxURL(t *testing.T) { + for _, bad := range []string{"netbox.example.com", "ftp://netbox.example.com", "https://netbox.example.com/?x=1"} { + t.Setenv("XDG_CONFIG_HOME", writeConfig(t, "netbox:\n url: \""+bad+"\"\n token: t\n")) + if _, err := Load(); err == nil || !strings.Contains(err.Error(), "netbox.url") { + t.Errorf("%s: got %v, want a netbox.url error", bad, err) + } + } +} diff --git a/internal/config/instances.go b/internal/config/instances.go index 46691b3..74a704e 100644 --- a/internal/config/instances.go +++ b/internal/config/instances.go @@ -17,6 +17,9 @@ func (c *Config) ValidateNetBoxes() error { if len(c.NetBoxes) > 0 && c.NetBox.URL != "" { return fmt.Errorf("configure either netbox or netboxes, not both") } + if c.NetBox.URL != "" && !validNetBoxURL(c.NetBox.URL) { + return fmt.Errorf("netbox.url %q must be an HTTP(S) URL without query or fragment", c.NetBox.URL) + } seen := make(map[string]bool) for _, source := range c.NetBoxes { if !instanceName.MatchString(source.Name) { @@ -27,8 +30,7 @@ func (c *Config) ValidateNetBoxes() error { return fmt.Errorf("duplicate NetBox instance name %q", source.Name) } seen[name] = true - u, err := url.Parse(source.URL) - if err != nil || (u.Scheme != "http" && u.Scheme != "https") || u.Host == "" || u.RawQuery != "" || u.Fragment != "" { + if !validNetBoxURL(source.URL) { return fmt.Errorf("NetBox %q requires an HTTP(S) URL without query or fragment", source.Name) } if source.TokenVersion < 0 || source.TokenVersion > 2 { @@ -38,6 +40,12 @@ func (c *Config) ValidateNetBoxes() error { return nil } +// validNetBoxURL accepts base URLs that API paths can be appended to. +func validNetBoxURL(raw string) bool { + u, err := url.Parse(raw) + return err == nil && (u.Scheme == "http" || u.Scheme == "https") && u.Host != "" && u.RawQuery == "" && u.Fragment == "" +} + func (c *Config) HasNetBox() bool { return c.NetBox.URL != "" || len(c.NetBoxes) > 0 } // HasHosts reports whether netssh has anything to connect to: NetBox or static hosts. diff --git a/internal/netbox/client.go b/internal/netbox/client.go index c1653f8..e5e151a 100644 --- a/internal/netbox/client.go +++ b/internal/netbox/client.go @@ -176,24 +176,32 @@ func (c *Client) GetIPs(ctx context.Context, entry HostEntry) ([]string, error) } return source.GetIPs(ctx, host) } - var apiURL string switch entry.Kind { case "device": - apiURL = fmt.Sprintf("%s/api/ipam/ip-addresses/?device_id=%d&limit=100", c.baseURL, entry.ID) + return c.fetchAllIPs(ctx, fmt.Sprintf("device_id=%d", entry.ID)) case "vm": - apiURL = fmt.Sprintf("%s/api/ipam/ip-addresses/?virtual_machine_id=%d&limit=100", c.baseURL, entry.ID) - default: - return nil, fmt.Errorf("unknown host kind: %q", entry.Kind) + return c.fetchAllIPs(ctx, fmt.Sprintf("virtual_machine_id=%d", entry.ID)) } + return nil, fmt.Errorf("unknown host kind: %q", entry.Kind) +} - var resp netboxIPListResponse - if err := c.get(ctx, apiURL, &resp); err != nil { - return nil, err - } - - ips := make([]string, 0, len(resp.Results)) - for _, r := range resp.Results { - ips = append(ips, stripPrefix(r.Address)) +// fetchAllIPs pages through the IP addresses matching filterParams, so hosts +// with more addresses than one page do not lose their management address. +func (c *Client) fetchAllIPs(ctx context.Context, filterParams string) ([]string, error) { + const pageSize = 100 + var ips []string + for offset := 0; ; offset += pageSize { + apiURL := fmt.Sprintf("%s/api/ipam/ip-addresses/?%s&limit=%d&offset=%d", c.baseURL, filterParams, pageSize, offset) + var resp netboxIPListResponse + if err := c.get(ctx, apiURL, &resp); err != nil { + return nil, err + } + for _, r := range resp.Results { + ips = append(ips, stripPrefix(r.Address)) + } + if len(resp.Results) < pageSize || len(ips) >= resp.Count { + break + } } return ips, nil } @@ -203,16 +211,7 @@ func (c *Client) GetIPsWithFilter(ctx context.Context, filterParams string) ([]s if c.sources != nil { return nil, fmt.Errorf("IP filters require a host instance; use GetIPsWithFilterForHost") } - apiURL := fmt.Sprintf("%s/api/ipam/ip-addresses/?%s&limit=100", c.baseURL, filterParams) - var resp netboxIPListResponse - if err := c.get(ctx, apiURL, &resp); err != nil { - return nil, err - } - ips := make([]string, 0, len(resp.Results)) - for _, r := range resp.Results { - ips = append(ips, stripPrefix(r.Address)) - } - return ips, nil + return c.fetchAllIPs(ctx, filterParams) } func (c *Client) searchDevices(ctx context.Context, query, tag string) ([]HostEntry, error) { @@ -348,7 +347,7 @@ func (c *Client) request(ctx context.Context, method, apiURL string, body io.Rea } if resp.StatusCode != http.StatusOK { - return "", fmt.Errorf("netbox returned %d for %s", resp.StatusCode, apiURL) + return "", &StatusError{Code: resp.StatusCode, URL: apiURL} } if err := json.NewDecoder(resp.Body).Decode(out); err != nil { @@ -399,6 +398,16 @@ func stripPrefix(cidr string) string { return cidr } +// StatusError is an unexpected HTTP status from NetBox. +type StatusError struct { + Code int + URL string +} + +func (e *StatusError) Error() string { + return fmt.Sprintf("netbox returned %d for %s", e.Code, e.URL) +} + // ErrHostNotFound distinguishes a missing exact match from API failures or ambiguity. var ErrHostNotFound = errors.New("host not found") diff --git a/internal/netbox/client_test.go b/internal/netbox/client_test.go index 373efe1..d858451 100644 --- a/internal/netbox/client_test.go +++ b/internal/netbox/client_test.go @@ -3,8 +3,11 @@ package netbox import ( "context" "encoding/json" + "errors" + "fmt" "net/http" "net/http/httptest" + "strconv" "strings" "testing" ) @@ -499,3 +502,45 @@ func TestCommentConflictIsReported(t *testing.T) { t.Fatal("conflict ignored") } } + +// Hosts with more addresses than one page must not lose the later ones. +func TestGetIPs_Paginates(t *testing.T) { + const total = 230 + var offsets []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + offsets = append(offsets, r.URL.Query().Get("offset")) + offset, _ := strconv.Atoi(r.URL.Query().Get("offset")) + limit, _ := strconv.Atoi(r.URL.Query().Get("limit")) + var addrs []string + for i := offset; i < offset+limit && i < total; i++ { + addrs = append(addrs, fmt.Sprintf("10.0.%d.%d/24", i/250, i%250+1)) + } + resp := ipListResponse(addrs...) + resp.Count = total + json.NewEncoder(w).Encode(resp) + })) + defer srv.Close() + + ips, err := NewClient(srv.URL, "token", 0).GetIPs(context.Background(), HostEntry{ID: 1, Kind: "device"}) + if err != nil { + t.Fatalf("GetIPs: %v", err) + } + if len(ips) != total || ips[total-1] != "10.0.0.230" { + t.Errorf("got %d IPs (last %q), want %d", len(ips), ips[len(ips)-1], total) + } + if strings.Join(offsets, ",") != "0,100,200" { + t.Errorf("offsets %v, want 0,100,200", offsets) + } +} + +func TestRequest_StatusErrorCarriesCode(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusBadRequest) + })) + defer srv.Close() + _, err := NewClient(srv.URL, "token", 0).GetIPsWithFilter(context.Background(), "device_id=1&interface=nope") + var status *StatusError + if !errors.As(err, &status) || status.Code != http.StatusBadRequest { + t.Fatalf("got %v, want a StatusError with code 400", err) + } +} diff --git a/internal/resolver/chain.go b/internal/resolver/chain.go index 77309d3..c5aca51 100644 --- a/internal/resolver/chain.go +++ b/internal/resolver/chain.go @@ -2,6 +2,7 @@ package resolver import ( "context" + "errors" "fmt" "git.zb-server.de/Sebi/ssh-netbox-wrapper/internal/config" @@ -32,6 +33,12 @@ func (c *Chain) Resolve(ctx context.Context, entry *netbox.HostEntry, client *ne if err == nil { return ip, nil } + // Only "this strategy has no address" moves on. After an API failure + // the next strategy would connect to (and cache) a different address + // than the configured order asks for. + if !errors.Is(err, ErrNoIP) { + return "", fmt.Errorf("resolver strategy %q for %q: %w", s.Name(), entry.Name, err) + } } return "", fmt.Errorf("no strategy resolved an IP for %q", entry.Name) } diff --git a/internal/resolver/chain_test.go b/internal/resolver/chain_test.go index b40fb73..9350825 100644 --- a/internal/resolver/chain_test.go +++ b/internal/resolver/chain_test.go @@ -153,3 +153,17 @@ func TestNew_MultipleStrategies(t *testing.T) { t.Errorf("got %d strategies, want 2", len(c.strategies)) } } + +// An API failure must not fall through: the next strategy would connect to +// and cache a different address than the configured order asks for. +func TestChain_APIErrorStopsTheChain(t *testing.T) { + apiErr := errors.New("connection refused") + c := &Chain{strategies: []Strategy{ + &stubStrategy{name: "management_subnet", err: apiErr}, + &stubStrategy{name: "primary_ip", ip: "10.0.0.2"}, + }} + ip, err := c.Resolve(context.Background(), &netbox.HostEntry{Name: "host"}, nil) + if !errors.Is(err, apiErr) || ip != "" { + t.Fatalf("got %q, %v; want the API error and no address", ip, err) + } +} diff --git a/internal/resolver/interface_name.go b/internal/resolver/interface_name.go index a39d6f3..4dc6fff 100644 --- a/internal/resolver/interface_name.go +++ b/internal/resolver/interface_name.go @@ -2,7 +2,9 @@ package resolver import ( "context" + "errors" "fmt" + "net/http" "net/url" "git.zb-server.de/Sebi/ssh-netbox-wrapper/internal/netbox" @@ -31,6 +33,10 @@ func (s *InterfaceNameStrategy) Resolve(ctx context.Context, entry *netbox.HostE } ips, err := client.GetIPsWithFilterForHost(ctx, *entry, filterParam) + var status *netbox.StatusError + if errors.As(err, &status) && status.Code == http.StatusBadRequest { + return "", ErrNoIP // the host has no interface of this name + } if err != nil { return "", fmt.Errorf("fetching IPs for interface %q: %w", s.name, err) } diff --git a/internal/resolver/interface_name_test.go b/internal/resolver/interface_name_test.go index 3d06c7d..e639de1 100644 --- a/internal/resolver/interface_name_test.go +++ b/internal/resolver/interface_name_test.go @@ -2,6 +2,7 @@ package resolver import ( "context" + "errors" "net/http" "net/http/httptest" "net/url" @@ -54,3 +55,16 @@ func urlQuery(s string) (string, error) { q, err := url.ParseQuery(s) return q.Encode(), err } + +// Only the HTTP 400 of a missing interface means "no address here"; a server +// error must stop the chain instead of silently using the next strategy. +func TestInterfaceNameStrategyServerErrorIsNotNoIP(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusBadGateway) + })) + defer srv.Close() + _, err := (&InterfaceNameStrategy{name: "mgmt0"}).Resolve(context.Background(), &netbox.HostEntry{ID: 7, Kind: "device"}, netbox.NewClient(srv.URL, "t", 1)) + if err == nil || errors.Is(err, ErrNoIP) { + t.Fatalf("got %v, want an API error", err) + } +} diff --git a/internal/ssh/options.go b/internal/ssh/options.go index 34babb2..f4a3db6 100644 --- a/internal/ssh/options.go +++ b/internal/ssh/options.go @@ -2,6 +2,7 @@ package ssh import ( "fmt" + "net/netip" "regexp" "sort" "strings" @@ -142,6 +143,18 @@ type Result struct { Notice string } +var destinationName = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9._-]*$`) + +// ValidAddress reports whether addr is an IP address or a hostname. Addresses +// come from NetBox and the cache file; one such as "-oProxyCommand=…" would be +// read by ssh as an option and run a command. +func ValidAddress(addr string) bool { + if _, err := netip.ParseAddr(addr); err == nil { + return !strings.ContainsAny(addr, " \t\r\n") + } + return destinationName.MatchString(addr) +} + var plainHostname = regexp.MustCompile(`^[A-Za-z0-9]([A-Za-z0-9._-]*[A-Za-z0-9])?$`) // Apply replaces the destination at destIdx and inserts the configured -o diff --git a/internal/ssh/options_test.go b/internal/ssh/options_test.go index fc749c9..6a73d31 100644 --- a/internal/ssh/options_test.go +++ b/internal/ssh/options_test.go @@ -128,3 +128,17 @@ func TestQuote(t *testing.T) { t.Fatalf("got %s", got) } } + +func TestValidAddress(t *testing.T) { + for _, addr := range []string{"10.0.0.1", "fd00:30::60", "fe80::1%eth0", "web01", "web01.example.com", "db_1.lan"} { + if !ValidAddress(addr) { + t.Errorf("ValidAddress(%q) = false, want true", addr) + } + } + // ssh would read the first as an option and run the command. + for _, addr := range []string{"-oProxyCommand=touch /tmp/pwned", "-J", "", "a b", "host;id", "user@host", "host/path", "fe80::1%eth0 x", "$(id)"} { + if ValidAddress(addr) { + t.Errorf("ValidAddress(%q) = true, want false", addr) + } + } +} diff --git a/internal/storage/file_test.go b/internal/storage/file_test.go new file mode 100644 index 0000000..c0504a1 --- /dev/null +++ b/internal/storage/file_test.go @@ -0,0 +1,29 @@ +package storage + +import ( + "os" + "path/filepath" + "testing" +) + +func TestWriteFileReplacesAtomically(t *testing.T) { + dir := filepath.Join(t.TempDir(), "new", "dir") + path := filepath.Join(dir, "file") + for _, content := range []string{"first", "second"} { + if err := WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + got, err := os.ReadFile(path) + if err != nil || string(got) != content { + t.Fatalf("got %q, %v; want %q", got, err, content) + } + } + info, err := os.Stat(path) + if err != nil || info.Mode().Perm() != 0o600 { + t.Errorf("mode %v, %v; want 0600", info.Mode().Perm(), err) + } + entries, _ := os.ReadDir(dir) + if len(entries) != 1 { + t.Errorf("temporary files left behind: %v", entries) + } +}