diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml new file mode 100644 index 0000000..61d182e --- /dev/null +++ b/.github/workflows/codeql.yml @@ -0,0 +1,40 @@ +name: CodeQL + +on: + push: + branches: [main] + pull_request: + branches: [main] + schedule: + - cron: '0 10 * * 1' # Weekly on Mondays at 10 AM UTC + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +permissions: + contents: read + security-events: write + +jobs: + analyze: + name: Analyze + runs-on: ubuntu-latest + timeout-minutes: 15 + + steps: + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + + - name: Initialize CodeQL + uses: github/codeql-action/init@4f3212b61783c3c68e8309a0f18a699764811cda # v3.28.1 + with: + languages: go + queries: security-extended + + - name: Autobuild + uses: github/codeql-action/autobuild@4f3212b61783c3c68e8309a0f18a699764811cda # v3.28.1 + + - name: Perform CodeQL Analysis + uses: github/codeql-action/analyze@4f3212b61783c3c68e8309a0f18a699764811cda # v3.28.1 + with: + category: "/language:go" diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index aac34c4..5136703 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -24,10 +24,17 @@ jobs: steps: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - - name: Run Gosec - uses: securego/gosec@424fc4cd9c82ea0fd6bee9cd49c2db2c3cc0c93f # v2.22.11 + - name: Set up Go + uses: actions/setup-go@7a3fe6cf4cb3a834922a1244abfce67bcef6a0c5 # v6.2.0 with: - args: ./... + go-version: '1.25' + cache: true + + - name: Install Gosec + run: curl -sfL https://raw.githubusercontent.com/securego/gosec/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v2.22.11 + + - name: Run Gosec + run: gosec ./... govulncheck: name: Go Vulnerability Check diff --git a/.golangci.yml b/.golangci.yml index 9bdcd12..3d2fd69 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -51,8 +51,9 @@ linters: - path: _test\.go text: "unused-parameter" - max-issues-per-linter: 50 - max-same-issues: 10 +issues: + max-issues-per-linter: 50 + max-same-issues: 10 formatters: enable: diff --git a/.idea/.gitignore b/.idea/.gitignore new file mode 100644 index 0000000..ab1f416 --- /dev/null +++ b/.idea/.gitignore @@ -0,0 +1,10 @@ +# Default ignored files +/shelf/ +/workspace.xml +# Ignored default folder with query files +/queries/ +# Datasource local storage ignored files +/dataSources/ +/dataSources.local.xml +# Editor-based HTTP Client requests +/httpRequests/ diff --git a/cmd/pyre/main.go b/cmd/pyre/main.go index 76adad1..f9e4ef8 100644 --- a/cmd/pyre/main.go +++ b/cmd/pyre/main.go @@ -22,7 +22,7 @@ func main() { var ( host = flag.String("host", "", "Firewall hostname or IP address") apiKey = flag.String("api-key", "", "API key for authentication") - insecure = flag.Bool("insecure", true, "Skip TLS certificate verification") + insecure = flag.Bool("insecure", false, "Skip TLS certificate verification (for self-signed certs)") configPath = flag.String("config", "", "Path to config file (default: ~/.pyre.yaml)") showHelp = flag.Bool("help", false, "Show help message") showVer = flag.Bool("version", false, "Show version") diff --git a/internal/api/client.go b/internal/api/client.go index 0db6dbf..490efb9 100644 --- a/internal/api/client.go +++ b/internal/api/client.go @@ -26,7 +26,7 @@ func WithInsecure(insecure bool) ClientOption { return func(c *Client) { if insecure { c.httpClient.Transport = &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, + TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, //nolint:gosec // #nosec G402 -- InsecureSkipVerify required for self-signed firewall certificates when user enables --insecure } } } @@ -44,9 +44,6 @@ func NewClient(host, apiKey string, opts ...ClientOption) *Client { apiKey: apiKey, httpClient: &http.Client{ Timeout: 30 * time.Second, - Transport: &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, - }, }, } @@ -104,8 +101,6 @@ func (c *Client) GetTarget() string { } func (c *Client) request(ctx context.Context, params url.Values) (*XMLResponse, error) { - params.Set("key", c.apiKey) - // Inject target parameter for Panorama routing if c.targetSerial != "" { params.Set("target", c.targetSerial) @@ -117,11 +112,15 @@ func (c *Client) request(ctx context.Context, params url.Values) (*XMLResponse, return nil, fmt.Errorf("creating request: %w", err) } + // Use X-PAN-KEY header instead of query parameter (PAN-OS 8.0+) + // This prevents API key from appearing in server/proxy logs + req.Header.Set("X-PAN-KEY", c.apiKey) + resp, err := c.httpClient.Do(req) if err != nil { return nil, fmt.Errorf("executing request: %w", err) } - defer resp.Body.Close() + defer func() { _ = resp.Body.Close() }() //nolint:errcheck // best effort cleanup body, err := io.ReadAll(resp.Body) if err != nil { diff --git a/internal/api/monitoring.go b/internal/api/monitoring.go index fb087c8..febce0b 100644 --- a/internal/api/monitoring.go +++ b/internal/api/monitoring.go @@ -182,7 +182,7 @@ func (c *Client) GetJobs(ctx context.Context) ([]models.Job, error) { // Parse progress - ignore error, zero value acceptable for non-numeric progress if e.Progress != "" { - job.Progress, _ = strconv.Atoi(strings.TrimSuffix(e.Progress, "%")) + job.Progress, _ = strconv.Atoi(strings.TrimSuffix(e.Progress, "%")) //nolint:errcheck // intentional - default to 0 on parse error } // Parse timestamps - PAN-OS typically uses format like "2024/01/15 10:30:45" @@ -248,7 +248,7 @@ func (c *Client) GetDiskUsage(ctx context.Context) ([]models.DiskUsage, error) { fields := strings.Fields(line) if len(fields) >= 6 { pctStr := strings.TrimSuffix(fields[4], "%") - pct, _ := strconv.ParseFloat(pctStr, 64) + pct, _ := strconv.ParseFloat(pctStr, 64) //nolint:errcheck // intentional - default to 0 on parse error disk := models.DiskUsage{ Filesystem: fields[0], @@ -266,6 +266,8 @@ func (c *Client) GetDiskUsage(ctx context.Context) ([]models.DiskUsage, error) { } // GetEnvironmentals retrieves hardware environmental sensor data +// +//nolint:misspell // "environmentals" is the PAN-OS XML API tag name func (c *Client) GetEnvironmentals(ctx context.Context) ([]models.Environmental, error) { resp, err := c.Op(ctx, "") if err != nil { diff --git a/internal/api/policies.go b/internal/api/policies.go index 6eebd02..ecc32c6 100644 --- a/internal/api/policies.go +++ b/internal/api/policies.go @@ -35,8 +35,8 @@ func (c *Client) GetSecurityPolicies(ctx context.Context) ([]models.SecurityRule if err != nil { return nil, err } - if err := CheckResponse(resp); err != nil { - return nil, err + if checkErr := CheckResponse(resp); checkErr != nil { + return nil, checkErr } // Handle empty result @@ -116,14 +116,14 @@ func (c *Client) GetSecurityPolicies(ctx context.Context) ([]models.SecurityRule var withWrapper struct { Entry []ruleEntry `xml:"rules>entry"` } - if err := xml.Unmarshal(WrapInner(resp.Result.Inner), &withWrapper); err == nil && len(withWrapper.Entry) > 0 { + if unmarshalErr := xml.Unmarshal(WrapInner(resp.Result.Inner), &withWrapper); unmarshalErr == nil && len(withWrapper.Entry) > 0 { entries = withWrapper.Entry } else { // Try parsing without wrapper (entries directly in result) var withoutWrapper struct { Entry []ruleEntry `xml:"entry"` } - if err := xml.Unmarshal(WrapInner(resp.Result.Inner), &withoutWrapper); err == nil { + if unmarshalErr := xml.Unmarshal(WrapInner(resp.Result.Inner), &withoutWrapper); unmarshalErr == nil { entries = withoutWrapper.Entry } } @@ -230,17 +230,17 @@ func (c *Client) GetSecurityPolicies(ctx context.Context) ([]models.SecurityRule for _, h := range hitResult.Entry { stats := hitStats{count: h.HitCount} if h.LastHit != "" && h.LastHit != "0" { - if ts, _ := strconv.ParseInt(h.LastHit, 10, 64); ts > 0 { + if ts, _ := strconv.ParseInt(h.LastHit, 10, 64); ts > 0 { //nolint:errcheck // intentional - default to zero time on parse error stats.lastHit = time.Unix(ts, 0) } } if h.FirstHit != "" && h.FirstHit != "0" { - if ts, _ := strconv.ParseInt(h.FirstHit, 10, 64); ts > 0 { + if ts, _ := strconv.ParseInt(h.FirstHit, 10, 64); ts > 0 { //nolint:errcheck // intentional - default to zero time on parse error stats.firstHit = time.Unix(ts, 0) } } if h.LastReset != "" && h.LastReset != "0" { - if ts, _ := strconv.ParseInt(h.LastReset, 10, 64); ts > 0 { + if ts, _ := strconv.ParseInt(h.LastReset, 10, 64); ts > 0 { //nolint:errcheck // intentional - default to zero time on parse error stats.lastReset = time.Unix(ts, 0) } } @@ -286,8 +286,8 @@ func (c *Client) GetNATRules(ctx context.Context) ([]models.NATRule, error) { if err != nil { return nil, err } - if err := CheckResponse(resp); err != nil { - return nil, err + if checkErr := CheckResponse(resp); checkErr != nil { + return nil, checkErr } // Handle empty result @@ -357,14 +357,14 @@ func (c *Client) GetNATRules(ctx context.Context) ([]models.NATRule, error) { var withWrapper struct { Entry []natEntry `xml:"rules>entry"` } - if err := xml.Unmarshal(WrapInner(resp.Result.Inner), &withWrapper); err == nil && len(withWrapper.Entry) > 0 { + if unmarshalErr := xml.Unmarshal(WrapInner(resp.Result.Inner), &withWrapper); unmarshalErr == nil && len(withWrapper.Entry) > 0 { entries = withWrapper.Entry } else { // Try parsing without wrapper var withoutWrapper struct { Entry []natEntry `xml:"entry"` } - if err := xml.Unmarshal(WrapInner(resp.Result.Inner), &withoutWrapper); err == nil { + if unmarshalErr := xml.Unmarshal(WrapInner(resp.Result.Inner), &withoutWrapper); unmarshalErr == nil { entries = withoutWrapper.Entry } } @@ -443,17 +443,17 @@ func (c *Client) GetNATRules(ctx context.Context) ([]models.NATRule, error) { for _, h := range hitResult.Entry { stats := hitStats{count: h.HitCount} if h.LastHit != "" && h.LastHit != "0" { - if ts, _ := strconv.ParseInt(h.LastHit, 10, 64); ts > 0 { + if ts, _ := strconv.ParseInt(h.LastHit, 10, 64); ts > 0 { //nolint:errcheck // intentional - default to zero time on parse error stats.lastHit = time.Unix(ts, 0) } } if h.FirstHit != "" && h.FirstHit != "0" { - if ts, _ := strconv.ParseInt(h.FirstHit, 10, 64); ts > 0 { + if ts, _ := strconv.ParseInt(h.FirstHit, 10, 64); ts > 0 { //nolint:errcheck // intentional - default to zero time on parse error stats.firstHit = time.Unix(ts, 0) } } if h.LastReset != "" && h.LastReset != "0" { - if ts, _ := strconv.ParseInt(h.LastReset, 10, 64); ts > 0 { + if ts, _ := strconv.ParseInt(h.LastReset, 10, 64); ts > 0 { //nolint:errcheck // intentional - default to zero time on parse error stats.lastReset = time.Unix(ts, 0) } } diff --git a/internal/api/sessions.go b/internal/api/sessions.go index 9194f30..68fc70b 100644 --- a/internal/api/sessions.go +++ b/internal/api/sessions.go @@ -98,7 +98,7 @@ func (c *Client) GetSessions(ctx context.Context, filter string) ([]models.Sessi var startTime time.Time // Ignore parse error - time format may vary, zero time acceptable if e.StartTime != "" { - startTime, _ = time.Parse("Mon Jan 2 15:04:05 2006", e.StartTime) + startTime, _ = time.Parse("Mon Jan 2 15:04:05 2006", e.StartTime) //nolint:errcheck // intentional - zero time acceptable } // Convert protocol number to name proto := protoToName(e.Proto) diff --git a/internal/api/system.go b/internal/api/system.go index 71579b7..2a2c32b 100644 --- a/internal/api/system.go +++ b/internal/api/system.go @@ -214,9 +214,9 @@ func (c *Client) GetSystemResources(ctx context.Context) (*models.Resources, err // Parse load average using regex // Ignore parse errors - optional fields, zero value acceptable if parsing fails if matches := loadAvgRegex.FindStringSubmatch(output); len(matches) >= 4 { - resources.Load1, _ = strconv.ParseFloat(matches[1], 64) - resources.Load5, _ = strconv.ParseFloat(matches[2], 64) - resources.Load15, _ = strconv.ParseFloat(matches[3], 64) + resources.Load1, _ = strconv.ParseFloat(matches[1], 64) //nolint:errcheck // intentional - zero value acceptable + resources.Load5, _ = strconv.ParseFloat(matches[2], 64) //nolint:errcheck // intentional - zero value acceptable + resources.Load15, _ = strconv.ParseFloat(matches[3], 64) //nolint:errcheck // intentional - zero value acceptable } lines := strings.Split(output, "\n") @@ -278,10 +278,10 @@ func (c *Client) GetSystemResources(ctx context.Context) (*models.Resources, err // Ignore parse errors - fields may have unexpected format, zero value acceptable cleanField := strings.TrimRight(f, ",%") if (cleanField == "total" || f == "total," || f == "total") && i > 0 { - total, _ = strconv.ParseFloat(strings.TrimRight(fields[i-1], ",%"), 64) + total, _ = strconv.ParseFloat(strings.TrimRight(fields[i-1], ",%"), 64) //nolint:errcheck // intentional } if (cleanField == "used" || f == "used," || f == "used") && i > 0 { - used, _ = strconv.ParseFloat(strings.TrimRight(fields[i-1], ",%"), 64) + used, _ = strconv.ParseFloat(strings.TrimRight(fields[i-1], ",%"), 64) //nolint:errcheck // intentional } } if total > 0 { diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 8f84085..90016eb 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -2,7 +2,10 @@ package auth import ( "context" + "fmt" + "net" "os" + "regexp" "strings" "sync" @@ -12,6 +15,9 @@ import ( "github.com/jp2195/pyre/internal/ssh" ) +// serialPattern validates Palo Alto device serial numbers (alphanumeric, typically 12-15 chars) +var serialPattern = regexp.MustCompile(`^[A-Za-z0-9]{8,20}$`) + type Session struct { mu sync.RWMutex ActiveFirewall string @@ -28,9 +34,8 @@ type Connection struct { Connected bool SSHEnabled bool - // SSH credentials from login (reused for SSH connection) + // SSH username from login (password must come from env var for security) SSHUsername string - SSHPassword string // Panorama fields IsPanorama bool @@ -66,10 +71,13 @@ func (s *Session) SetActiveFirewall(name string) bool { } func (s *Session) AddConnection(name string, fwConfig *config.FirewallConfig, apiKey string) *Connection { - return s.AddConnectionWithSSH(name, fwConfig, apiKey, "", "") + return s.AddConnectionWithSSH(name, fwConfig, apiKey, "", nil) } -func (s *Session) AddConnectionWithSSH(name string, fwConfig *config.FirewallConfig, apiKey, sshUsername, sshPassword string) *Connection { +// AddConnectionWithSSH creates a new connection with SSH username and optional pre-established SSH client. +// If sshClient is provided, it will be used directly. Otherwise, SSH can be established later +// using credentials from environment variables. +func (s *Session) AddConnectionWithSSH(name string, fwConfig *config.FirewallConfig, apiKey, sshUsername string, sshClient *ssh.Client) *Connection { s.mu.Lock() defer s.mu.Unlock() @@ -81,7 +89,8 @@ func (s *Session) AddConnectionWithSSH(name string, fwConfig *config.FirewallCon Client: client, Connected: true, SSHUsername: sshUsername, - SSHPassword: sshPassword, + SSHClient: sshClient, + SSHEnabled: sshClient != nil, } s.Connections[name] = conn @@ -133,14 +142,19 @@ type Credentials struct { func ResolveCredentials(cfg *config.Config, flags config.CLIFlags) *Credentials { creds := &Credentials{} + // CLI flags take highest priority if flags.Host != "" { creds.Host = flags.Host - creds.Insecure = flags.Insecure } if flags.APIKey != "" { creds.APIKey = flags.APIKey } + // If --insecure flag is explicitly true, use it + if flags.Insecure { + creds.Insecure = true + } + // Environment variables (if not set by flags) if envHost := os.Getenv("PYRE_HOST"); envHost != "" && creds.Host == "" { creds.Host = envHost } @@ -151,10 +165,14 @@ func ResolveCredentials(cfg *config.Config, flags config.CLIFlags) *Credentials creds.Insecure = true } + // Config file defaults (if not set by flags or env) if creds.Host == "" { if name, fw, ok := cfg.GetDefaultFirewall(); ok { creds.Host = fw.Host - creds.Insecure = fw.Insecure + // Use config insecure if not already set by flags or env + if !creds.Insecure && fw.Insecure { + creds.Insecure = true + } envKey := os.Getenv("PYRE_" + name + "_API_KEY") if envKey != "" && creds.APIKey == "" { @@ -178,18 +196,53 @@ func (c *Credentials) NeedsInteractiveAuth() bool { return c.Host == "" || c.APIKey == "" } +// validateSerial checks if the serial number has a valid format. +func validateSerial(serial string) error { + if serial == "" { + return nil + } + if !serialPattern.MatchString(serial) { + return fmt.Errorf("invalid serial number format: %s", serial) + } + return nil +} + +// validateIP checks if the IP address is valid. +func validateIP(ip string) error { + if ip == "" { + return nil + } + if net.ParseIP(ip) == nil { + return fmt.Errorf("invalid IP address: %s", ip) + } + return nil +} + // SetTarget sets the current target device for Panorama. // Pass nil to target Panorama itself. -func (c *Connection) SetTarget(device *models.ManagedDevice) { +// Returns an error if the device serial or IP is invalid. +func (c *Connection) SetTarget(device *models.ManagedDevice) error { if device == nil { c.TargetSerial = "" c.TargetIP = "" c.Client.ClearTarget() - } else { - c.TargetSerial = device.Serial - c.TargetIP = device.IPAddress - c.Client.SetTarget(device.Serial) + return nil } + + // Validate serial number format + if err := validateSerial(device.Serial); err != nil { + return err + } + + // Validate IP address format + if err := validateIP(device.IPAddress); err != nil { + return err + } + + c.TargetSerial = device.Serial + c.TargetIP = device.IPAddress + c.Client.SetTarget(device.Serial) + return nil } // GetTargetDevice returns the currently targeted managed device, or nil if targeting Panorama. @@ -247,6 +300,10 @@ func (c *Connection) ConnectSSH(ctx context.Context) error { // For Panorama with a target device, connect to the target's IP host := c.Config.Host if c.IsPanorama && c.TargetIP != "" { + // Validate target IP before using it + if err := validateIP(c.TargetIP); err != nil { + return fmt.Errorf("invalid target IP for SSH: %w", err) + } host = c.TargetIP } @@ -282,6 +339,7 @@ func (c *Connection) HasSSH() bool { } // getSSHConfig returns the SSH configuration, combining config file, env vars, and login credentials. +// Note: SSH passwords must come from environment variables (PYRE_SSH_PASSWORD) for security. func (c *Connection) getSSHConfig() config.SSHConfig { var sshCfg config.SSHConfig if c.Config != nil { @@ -291,10 +349,9 @@ func (c *Connection) getSSHConfig() config.SSHConfig { // Apply environment variable overrides sshCfg = resolveSSHCredentials(c.Name, sshCfg) - // Use login credentials if no username configured yet + // Use login username if no username configured yet if sshCfg.Username == "" && c.SSHUsername != "" { sshCfg.Username = c.SSHUsername - sshCfg.Password = c.SSHPassword } return sshCfg @@ -312,6 +369,9 @@ func resolveSSHCredentials(fwName string, cfg config.SSHConfig) config.SSHConfig if envKey := os.Getenv("PYRE_SSH_KEY_PATH"); envKey != "" && cfg.PrivateKeyPath == "" { cfg.PrivateKeyPath = envKey } + if os.Getenv("PYRE_SSH_INSECURE") == "true" { + cfg.Insecure = true + } // Per-firewall SSH password: PYRE__SSH_PASSWORD envName := strings.ToUpper(strings.ReplaceAll(fwName, "-", "_")) diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 2457188..b59c747 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -41,7 +41,7 @@ func TestConcurrentSetActiveFirewall(t *testing.T) { // Verify final state is valid conn := session.GetActiveConnection() if conn == nil { - t.Error("expected active connection after concurrent operations") + t.Fatal("expected active connection after concurrent operations") } // The active firewall should be one of the valid ones diff --git a/internal/auth/keygen.go b/internal/auth/keygen.go index 353036f..b4e33e6 100644 --- a/internal/auth/keygen.go +++ b/internal/auth/keygen.go @@ -33,26 +33,28 @@ func GenerateAPIKey(ctx context.Context, host, username, password string, insecu client := &http.Client{ Timeout: 30 * time.Second, Transport: &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: insecure}, + TLSClientConfig: &tls.Config{InsecureSkipVerify: insecure}, //nolint:gosec // #nosec G402 -- InsecureSkipVerify required for self-signed firewall certificates when user enables --insecure }, } - params := url.Values{} - params.Set("type", "keygen") - params.Set("user", username) - params.Set("password", password) + // Use POST with form body to keep credentials out of URLs/logs + reqURL := fmt.Sprintf("https://%s/api/", host) + formData := url.Values{} + formData.Set("type", "keygen") + formData.Set("user", username) + formData.Set("password", password) - reqURL := fmt.Sprintf("https://%s/api/?%s", host, params.Encode()) - req, err := http.NewRequestWithContext(ctx, http.MethodGet, reqURL, nil) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, reqURL, strings.NewReader(formData.Encode())) if err != nil { return nil, fmt.Errorf("creating keygen request: %w", err) } + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") resp, err := client.Do(req) if err != nil { return nil, fmt.Errorf("keygen request failed: %w", err) } - defer resp.Body.Close() + defer func() { _ = resp.Body.Close() }() //nolint:errcheck // best effort cleanup body, err := io.ReadAll(resp.Body) if err != nil { diff --git a/internal/auth/session_test.go b/internal/auth/session_test.go index 94373df..8ca1174 100644 --- a/internal/auth/session_test.go +++ b/internal/auth/session_test.go @@ -64,7 +64,9 @@ func TestSession_AddConnectionWithSSH(t *testing.T) { Insecure: true, } - conn := session.AddConnectionWithSSH("test-fw", fwConfig, "test-api-key", "admin", "password123") + // Note: SSH client is passed directly (established during login) + // Password is no longer stored - SSH must be established while credentials are in memory + conn := session.AddConnectionWithSSH("test-fw", fwConfig, "test-api-key", "admin", nil) if conn == nil { t.Fatal("expected non-nil connection") @@ -72,9 +74,6 @@ func TestSession_AddConnectionWithSSH(t *testing.T) { if conn.SSHUsername != "admin" { t.Errorf("expected SSH username 'admin', got %q", conn.SSHUsername) } - if conn.SSHPassword != "password123" { - t.Errorf("expected SSH password 'password123', got %q", conn.SSHPassword) - } } func TestSession_GetActiveConnection(t *testing.T) { @@ -453,7 +452,6 @@ func TestConnection_getSSHConfig(t *testing.T) { }, }, SSHUsername: "login-admin", - SSHPassword: "login-pass", } cfg := conn.getSSHConfig() @@ -472,6 +470,10 @@ func TestConnection_getSSHConfig(t *testing.T) { } func TestConnection_getSSHConfig_LoginFallback(t *testing.T) { + // Set SSH password env var for this test + os.Setenv("PYRE_SSH_PASSWORD", "env-pass") + defer os.Unsetenv("PYRE_SSH_PASSWORD") + conn := &Connection{ Name: "test-fw", Config: &config.FirewallConfig{ @@ -481,17 +483,17 @@ func TestConnection_getSSHConfig_LoginFallback(t *testing.T) { }, }, SSHUsername: "login-admin", - SSHPassword: "login-pass", } cfg := conn.getSSHConfig() - // Should fall back to login credentials + // Should fall back to login username if cfg.Username != "login-admin" { t.Errorf("expected Username 'login-admin', got %q", cfg.Username) } - if cfg.Password != "login-pass" { - t.Errorf("expected Password 'login-pass', got %q", cfg.Password) + // Password should come from env var + if cfg.Password != "env-pass" { + t.Errorf("expected Password 'env-pass', got %q", cfg.Password) } } @@ -500,12 +502,11 @@ func TestConnection_getSSHConfig_NilConfig(t *testing.T) { Name: "test-fw", Config: nil, SSHUsername: "login-admin", - SSHPassword: "login-pass", } cfg := conn.getSSHConfig() - // Should use login credentials + // Should use login username if cfg.Username != "login-admin" { t.Errorf("expected Username 'login-admin', got %q", cfg.Username) } diff --git a/internal/config/config.go b/internal/config/config.go index df1796f..f6436af 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1,6 +1,7 @@ package config import ( + "fmt" "os" "path/filepath" "time" @@ -12,14 +13,17 @@ type Config struct { DefaultFirewall string `yaml:"default_firewall"` Firewalls map[string]FirewallConfig `yaml:"firewalls"` Settings Settings `yaml:"settings"` + Warnings []string `yaml:"-"` // Security warnings from config validation } type SSHConfig struct { - Port int `yaml:"port"` // Default: 22 + Port int `yaml:"port"` // Default: 22 Username string `yaml:"username"` - Password string `yaml:"password"` // Or use key + Password string `yaml:"password,omitempty"` // Deprecated: use env vars instead PrivateKeyPath string `yaml:"private_key_path"` Timeout int `yaml:"timeout"` // Seconds, default: 30 + KnownHostsPath string `yaml:"known_hosts_path"` // Default: ~/.ssh/known_hosts + Insecure bool `yaml:"insecure"` // Skip host key verification (not recommended) } type FirewallConfig struct { @@ -57,7 +61,7 @@ func Load() (*Config, error) { } configPath := filepath.Join(homeDir, ".pyre.yaml") - data, err := os.ReadFile(configPath) + data, err := os.ReadFile(configPath) // #nosec G304 -- Path is constructed from user's home directory if err != nil { if os.IsNotExist(err) { return cfg, nil @@ -127,5 +131,20 @@ func LoadWithFlags(flags CLIFlags) (*Config, error) { } cfg.ApplyFlags(flags) + cfg.validateSecuritySettings() return cfg, nil } + +// validateSecuritySettings checks for deprecated or insecure configuration settings +// and adds warnings to the config. +func (c *Config) validateSecuritySettings() { + for name, fw := range c.Firewalls { + // Warn about SSH password in config file (deprecated) + if fw.SSH.Password != "" { + c.Warnings = append(c.Warnings, fmt.Sprintf( + "SECURITY WARNING: Firewall %q has SSH password in config file. "+ + "Use PYRE_SSH_PASSWORD or PYRE_%s_SSH_PASSWORD environment variable instead.", + name, name)) + } + } +} diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 5408ccf..bf625eb 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -164,7 +164,7 @@ settings: theme: dark default_view: policies ` - if err := os.WriteFile(configPath, []byte(configContent), 0644); err != nil { + if err := os.WriteFile(configPath, []byte(configContent), 0600); err != nil { t.Fatalf("failed to write test config: %v", err) } @@ -208,7 +208,7 @@ func TestLoadWithFlags_InvalidConfig(t *testing.T) { configPath := filepath.Join(tmpDir, "invalid.yaml") // Write invalid YAML - if err := os.WriteFile(configPath, []byte("invalid: yaml: content:"), 0644); err != nil { + if err := os.WriteFile(configPath, []byte("invalid: yaml: content:"), 0600); err != nil { t.Fatalf("failed to write test config: %v", err) } @@ -243,7 +243,7 @@ firewalls: config-fw: host: 10.0.0.1 ` - if err := os.WriteFile(configPath, []byte(configContent), 0644); err != nil { + if err := os.WriteFile(configPath, []byte(configContent), 0600); err != nil { t.Fatalf("failed to write test config: %v", err) } @@ -294,7 +294,7 @@ firewalls: private_key_path: /path/to/key timeout: 60 ` - if err := os.WriteFile(configPath, []byte(configContent), 0644); err != nil { + if err := os.WriteFile(configPath, []byte(configContent), 0600); err != nil { t.Fatalf("failed to write test config: %v", err) } @@ -338,7 +338,7 @@ firewalls: firewall: host: 10.0.0.2 ` - if err := os.WriteFile(configPath, []byte(configContent), 0644); err != nil { + if err := os.WriteFile(configPath, []byte(configContent), 0600); err != nil { t.Fatalf("failed to write test config: %v", err) } @@ -380,7 +380,7 @@ func TestConfig_NilFirewallsAfterLoad(t *testing.T) { settings: refresh_interval: 10s ` - if err := os.WriteFile(configPath, []byte(configContent), 0644); err != nil { + if err := os.WriteFile(configPath, []byte(configContent), 0600); err != nil { t.Fatalf("failed to write test config: %v", err) } @@ -471,7 +471,7 @@ firewalls: private_key_path: /path/to/key timeout: 120 ` - if err := os.WriteFile(configPath, []byte(configContent), 0644); err != nil { + if err := os.WriteFile(configPath, []byte(configContent), 0600); err != nil { t.Fatalf("failed to write test config: %v", err) } diff --git a/internal/ssh/client.go b/internal/ssh/client.go index c48d653..8e7b73e 100644 --- a/internal/ssh/client.go +++ b/internal/ssh/client.go @@ -3,13 +3,16 @@ package ssh import ( "bytes" "context" + "errors" "fmt" "net" "os" + "path/filepath" "strings" "time" "golang.org/x/crypto/ssh" + "golang.org/x/crypto/ssh/knownhosts" "github.com/jp2195/pyre/internal/config" ) @@ -51,7 +54,7 @@ func NewClient(host string, cfg config.SSHConfig) (*Client, error) { // Try private key auth first if cfg.PrivateKeyPath != "" { keyPath := expandPath(cfg.PrivateKeyPath) - key, err := os.ReadFile(keyPath) + key, err := os.ReadFile(keyPath) // #nosec G304 -- Path is from user config, directory traversal not applicable if err != nil { return nil, fmt.Errorf("failed to read private key: %w", err) } @@ -86,10 +89,15 @@ func NewClient(host string, cfg config.SSHConfig) (*Client, error) { return nil, fmt.Errorf("no authentication method provided") } + hostKeyCallback, err := getHostKeyCallback(cfg) + if err != nil { + return nil, fmt.Errorf("setting up host key verification: %w", err) + } + sshConfig := &ssh.ClientConfig{ User: cfg.Username, Auth: authMethods, - HostKeyCallback: ssh.InsecureIgnoreHostKey(), + HostKeyCallback: hostKeyCallback, Timeout: timeout, } @@ -115,7 +123,7 @@ func (c *Client) Connect(ctx context.Context) error { // Create SSH connection on top of TCP connection sshConn, chans, reqs, err := ssh.NewClientConn(conn, addr, c.config) if err != nil { - conn.Close() + _ = conn.Close() //nolint:errcheck // best effort cleanup return fmt.Errorf("failed to establish SSH connection: %w", err) } @@ -152,7 +160,7 @@ func (c *Client) Execute(ctx context.Context, cmd string) (*CommandResult, error Error: fmt.Errorf("failed to create session: %w", err), }, nil } - defer session.Close() + defer func() { _ = session.Close() }() //nolint:errcheck // best effort cleanup var stdout, stderr bytes.Buffer session.Stdout = &stdout @@ -166,7 +174,7 @@ func (c *Client) Execute(ctx context.Context, cmd string) (*CommandResult, error select { case <-ctx.Done(): - session.Signal(ssh.SIGTERM) + _ = session.Signal(ssh.SIGTERM) //nolint:errcheck // best effort signal return &CommandResult{ Command: cmd, Duration: time.Since(start), @@ -268,3 +276,82 @@ func expandPath(path string) string { } return path } + +// getHostKeyCallback returns the appropriate host key callback based on configuration. +// If cfg.Insecure is true, it returns an insecure callback that accepts any host key. +// Otherwise, it uses the known_hosts file for verification. +func getHostKeyCallback(cfg config.SSHConfig) (ssh.HostKeyCallback, error) { + if cfg.Insecure { + return ssh.InsecureIgnoreHostKey(), nil //nolint:gosec // #nosec G106 -- InsecureIgnoreHostKey used when user explicitly disables host key verification + } + + // Determine known_hosts path + knownHostsPath := cfg.KnownHostsPath + if knownHostsPath == "" { + home, err := os.UserHomeDir() + if err != nil { + return nil, fmt.Errorf("getting home directory: %w", err) + } + knownHostsPath = filepath.Join(home, ".ssh", "known_hosts") + } else { + knownHostsPath = expandPath(knownHostsPath) + } + + // Ensure .ssh directory exists + sshDir := filepath.Dir(knownHostsPath) + if err := os.MkdirAll(sshDir, 0700); err != nil { + return nil, fmt.Errorf("creating .ssh directory: %w", err) + } + + // Create known_hosts file if it doesn't exist + if _, err := os.Stat(knownHostsPath); os.IsNotExist(err) { + f, err := os.OpenFile(knownHostsPath, os.O_CREATE|os.O_WRONLY, 0600) // #nosec G304 -- Path is user's .ssh directory or from config + if err != nil { + return nil, fmt.Errorf("creating known_hosts file: %w", err) + } + _ = f.Close() //nolint:errcheck // best effort cleanup + } + + // Create host key callback from known_hosts + callback, err := knownhosts.New(knownHostsPath) + if err != nil { + return nil, fmt.Errorf("parsing known_hosts: %w", err) + } + + // Wrap the callback to provide a more helpful error message and optionally add new hosts + return func(hostname string, remote net.Addr, key ssh.PublicKey) error { + err := callback(hostname, remote, key) + if err != nil { + var keyErr *knownhosts.KeyError + if errors.As(err, &keyErr) { + if len(keyErr.Want) > 0 { + // Host key has changed - this could be a MITM attack + return fmt.Errorf("WARNING: host key for %s has changed! This could indicate a MITM attack. "+ + "If you trust this host, remove the old key from %s and try again", hostname, knownHostsPath) + } + // Host not in known_hosts - add it + if addErr := addHostKey(knownHostsPath, hostname, remote, key); addErr != nil { + return fmt.Errorf("host key verification failed and could not add to known_hosts: %w", addErr) + } + // Return nil to allow connection after adding the key + return nil + } + return err + } + return nil + }, nil +} + +// addHostKey appends a host key to the known_hosts file. +func addHostKey(knownHostsPath, hostname string, remote net.Addr, key ssh.PublicKey) error { + f, err := os.OpenFile(knownHostsPath, os.O_APPEND|os.O_WRONLY, 0600) // #nosec G304 -- Path is user's .ssh directory or from config + if err != nil { + return err + } + defer func() { _ = f.Close() }() //nolint:errcheck // best effort cleanup + + // Format the known_hosts line + line := knownhosts.Line([]string{hostname}, key) + _, err = fmt.Fprintln(f, line) + return err +} diff --git a/internal/ssh/client_test.go b/internal/ssh/client_test.go index 83db682..ad571c6 100644 --- a/internal/ssh/client_test.go +++ b/internal/ssh/client_test.go @@ -78,8 +78,8 @@ func TestClientWithMockServer(t *testing.T) { server.SetDefaultResponses() - if err := server.Start(); err != nil { - t.Fatalf("Failed to start mock server: %v", err) + if startErr := server.Start(); startErr != nil { + t.Fatalf("Failed to start mock server: %v", startErr) } defer server.Close() @@ -99,8 +99,8 @@ func TestClientWithMockServer(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - if err := client.Connect(ctx); err != nil { - t.Fatalf("Failed to connect: %v", err) + if connErr := client.Connect(ctx); connErr != nil { + t.Fatalf("Failed to connect: %v", connErr) } if !client.IsConnected() { @@ -334,8 +334,8 @@ func TestMockSSHServerCustomResponse(t *testing.T) { server.SetResponse("custom-command", "custom-response\n") - if err := server.Start(); err != nil { - t.Fatalf("Failed to start mock server: %v", err) + if startErr := server.Start(); startErr != nil { + t.Fatalf("Failed to start mock server: %v", startErr) } defer server.Close() @@ -354,8 +354,8 @@ func TestMockSSHServerCustomResponse(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - if err := client.Connect(ctx); err != nil { - t.Fatalf("Failed to connect: %v", err) + if connErr := client.Connect(ctx); connErr != nil { + t.Fatalf("Failed to connect: %v", connErr) } result, err := client.Execute(ctx, "custom-command") @@ -373,8 +373,8 @@ func TestMockSSHServerAddress(t *testing.T) { t.Fatalf("Failed to create mock server: %v", err) } - if err := server.Start(); err != nil { - t.Fatalf("Failed to start mock server: %v", err) + if startErr := server.Start(); startErr != nil { + t.Fatalf("Failed to start mock server: %v", startErr) } defer server.Close() @@ -403,8 +403,8 @@ func TestExecuteContextCancellation(t *testing.T) { // Set up a slow command response server.SetResponse("sleep-command", "started\n") - if err := server.Start(); err != nil { - t.Fatalf("Failed to start mock server: %v", err) + if startErr := server.Start(); startErr != nil { + t.Fatalf("Failed to start mock server: %v", startErr) } defer server.Close() @@ -424,8 +424,8 @@ func TestExecuteContextCancellation(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - if err := client.Connect(ctx); err != nil { - t.Fatalf("Failed to connect: %v", err) + if connErr := client.Connect(ctx); connErr != nil { + t.Fatalf("Failed to connect: %v", connErr) } // Create a context that we'll cancel @@ -466,8 +466,8 @@ func TestExecuteWithAlreadyCancelledContext(t *testing.T) { server.SetDefaultResponses() - if err := server.Start(); err != nil { - t.Fatalf("Failed to start mock server: %v", err) + if startErr := server.Start(); startErr != nil { + t.Fatalf("Failed to start mock server: %v", startErr) } defer server.Close() @@ -487,8 +487,8 @@ func TestExecuteWithAlreadyCancelledContext(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - if err := client.Connect(ctx); err != nil { - t.Fatalf("Failed to connect: %v", err) + if connErr := client.Connect(ctx); connErr != nil { + t.Fatalf("Failed to connect: %v", connErr) } // Create an already-cancelled context diff --git a/internal/ssh/mock_server.go b/internal/ssh/mock_server.go index 90ac451..b413ab8 100644 --- a/internal/ssh/mock_server.go +++ b/internal/ssh/mock_server.go @@ -66,7 +66,7 @@ func (s *MockSSHServer) Start() error { } s.listener = listener - addr := listener.Addr().(*net.TCPAddr) + addr := listener.Addr().(*net.TCPAddr) //nolint:errcheck // known type s.host = addr.IP.String() s.port = addr.Port @@ -116,20 +116,20 @@ func (s *MockSSHServer) acceptLoop() { } func (s *MockSSHServer) handleConnection(netConn net.Conn) { - defer netConn.Close() + defer func() { _ = netConn.Close() }() //nolint:errcheck // test helper sshConn, chans, reqs, err := ssh.NewServerConn(netConn, s.config) if err != nil { return } - defer sshConn.Close() + defer func() { _ = sshConn.Close() }() //nolint:errcheck // test helper // Discard out-of-band requests go ssh.DiscardRequests(reqs) for newChannel := range chans { if newChannel.ChannelType() != "session" { - newChannel.Reject(ssh.UnknownChannelType, "unknown channel type") + _ = newChannel.Reject(ssh.UnknownChannelType, "unknown channel type") //nolint:errcheck // test helper continue } @@ -143,43 +143,43 @@ func (s *MockSSHServer) handleConnection(netConn net.Conn) { } func (s *MockSSHServer) handleChannel(channel ssh.Channel, requests <-chan *ssh.Request) { - defer channel.Close() + defer func() { _ = channel.Close() }() //nolint:errcheck // test helper for req := range requests { switch req.Type { case "exec": if len(req.Payload) < 4 { - req.Reply(false, nil) + _ = req.Reply(false, nil) //nolint:errcheck // test helper continue } // Extract command from payload (length-prefixed string) cmdLen := int(req.Payload[0])<<24 | int(req.Payload[1])<<16 | int(req.Payload[2])<<8 | int(req.Payload[3]) if len(req.Payload) < 4+cmdLen { - req.Reply(false, nil) + _ = req.Reply(false, nil) //nolint:errcheck // test helper continue } cmd := string(req.Payload[4 : 4+cmdLen]) - req.Reply(true, nil) + _ = req.Reply(true, nil) //nolint:errcheck // test helper response := s.getResponse(cmd) - io.WriteString(channel, response) + _, _ = io.WriteString(channel, response) //nolint:errcheck // test helper // Send exit status - channel.SendRequest("exit-status", false, []byte{0, 0, 0, 0}) + _, _ = channel.SendRequest("exit-status", false, []byte{0, 0, 0, 0}) //nolint:errcheck // test helper // Close stdout to signal command completion - channel.CloseWrite() + _ = channel.CloseWrite() //nolint:errcheck // test helper return case "shell": - req.Reply(true, nil) + _ = req.Reply(true, nil) //nolint:errcheck // test helper // For shell requests, just close the channel return default: - req.Reply(false, nil) + _ = req.Reply(false, nil) //nolint:errcheck // test helper } } } diff --git a/internal/testutil/mock_server.go b/internal/testutil/mock_server.go index 2187c26..91d86e4 100644 --- a/internal/testutil/mock_server.go +++ b/internal/testutil/mock_server.go @@ -8,12 +8,12 @@ import ( ) type MockPANOS struct { - Server *httptest.Server - Hostname string - Model string - Serial string - Version string - IsPanorama bool + Server *httptest.Server + Hostname string + Model string + Serial string + Version string + IsPanorama bool } func NewMockPANOS() *MockPANOS { @@ -57,7 +57,16 @@ func (m *MockPANOS) Host() string { } func (m *MockPANOS) handleAPI(w http.ResponseWriter, r *http.Request) { + // Parse form for POST requests (keygen uses POST with form body) + if r.Method == http.MethodPost { + _ = r.ParseForm() //nolint:errcheck // test helper + } + + // Get type from query string or form apiType := r.URL.Query().Get("type") + if apiType == "" { + apiType = r.FormValue("type") + } cmd := r.URL.Query().Get("cmd") w.Header().Set("Content-Type", "application/xml") @@ -70,18 +79,25 @@ func (m *MockPANOS) handleAPI(w http.ResponseWriter, r *http.Request) { case "config": m.handleConfig(w, r) default: - w.Write([]byte(`Invalid request`)) + _, _ = w.Write([]byte(`Invalid request`)) //nolint:errcheck // test helper } } func (m *MockPANOS) handleKeygen(w http.ResponseWriter, r *http.Request) { + // Get user/password from query string or form (POST uses form body) user := r.URL.Query().Get("user") + if user == "" { + user = r.FormValue("user") + } password := r.URL.Query().Get("password") + if password == "" { + password = r.FormValue("password") + } if user == "admin" && password == "admin" { - w.Write([]byte(`LUFRPT1234567890abcdef==`)) + _, _ = w.Write([]byte(`LUFRPT1234567890abcdef==`)) //nolint:errcheck // test helper } else { - w.Write([]byte(`Invalid credentials`)) + _, _ = w.Write([]byte(`Invalid credentials`)) //nolint:errcheck // test helper } } @@ -110,7 +126,7 @@ func (m *MockPANOS) handleOp(w http.ResponseWriter, r *http.Request, cmd string) case strings.Contains(cmd, ""): m.respondManagedDevices(w) default: - w.Write([]byte(``)) + _, _ = w.Write([]byte(``)) //nolint:errcheck // test helper } } @@ -120,12 +136,13 @@ func (m *MockPANOS) handleConfig(w http.ResponseWriter, r *http.Request) { if strings.Contains(xpath, "security/rules") { m.respondSecurityRules(w) } else { - w.Write([]byte(``)) + _, _ = w.Write([]byte(``)) //nolint:errcheck // test helper } } +//nolint:errcheck // test helper func (m *MockPANOS) respondSystemInfo(w http.ResponseWriter) { - fmt.Fprintf(w, ` + _, _ = fmt.Fprintf(w, ` %s @@ -142,7 +159,8 @@ func (m *MockPANOS) respondSystemInfo(w http.ResponseWriter) { } func (m *MockPANOS) respondResources(w http.ResponseWriter) { - w.Write([]byte(` + //nolint:errcheck // test helper + _, _ = w.Write([]byte(` top - 14:32:18 up 15 days, 3:42, 0 users, load average: 0.45, 0.52, 0.48 Tasks: 150 total, 1 running, 149 sleeping, 0 stopped, 0 zombie @@ -153,7 +171,8 @@ KiB Mem: 16384000 total, 12288000 used, 4096000 free, 256000 buffers } func (m *MockPANOS) respondSessionInfo(w http.ResponseWriter) { - w.Write([]byte(` + //nolint:errcheck // test helper + _, _ = w.Write([]byte(` 15432 262144 @@ -164,7 +183,8 @@ func (m *MockPANOS) respondSessionInfo(w http.ResponseWriter) { } func (m *MockPANOS) respondSessions(w http.ResponseWriter) { - w.Write([]byte(` + //nolint:errcheck // test helper + _, _ = w.Write([]byte(` 12345 @@ -228,7 +248,8 @@ func (m *MockPANOS) respondSessions(w http.ResponseWriter) { } func (m *MockPANOS) respondHAStatus(w http.ResponseWriter) { - w.Write([]byte(` + //nolint:errcheck // test helper + _, _ = w.Write([]byte(` yes @@ -246,7 +267,8 @@ func (m *MockPANOS) respondHAStatus(w http.ResponseWriter) { } func (m *MockPANOS) respondInterfaces(w http.ResponseWriter) { - w.Write([]byte(` + //nolint:errcheck // test helper + _, _ = w.Write([]byte(` @@ -313,7 +335,8 @@ func (m *MockPANOS) respondInterfaces(w http.ResponseWriter) { } func (m *MockPANOS) respondSecurityRules(w http.ResponseWriter) { - w.Write([]byte(` + //nolint:errcheck // test helper + _, _ = w.Write([]byte(` no @@ -364,7 +387,8 @@ func (m *MockPANOS) respondSecurityRules(w http.ResponseWriter) { } func (m *MockPANOS) respondRuleHitCount(w http.ResponseWriter) { - w.Write([]byte(` + //nolint:errcheck // test helper + _, _ = w.Write([]byte(` @@ -399,7 +423,8 @@ func (m *MockPANOS) respondRuleHitCount(w http.ResponseWriter) { } func (m *MockPANOS) respondThreatCounters(w http.ResponseWriter) { - w.Write([]byte(` + //nolint:errcheck // test helper + _, _ = w.Write([]byte(` @@ -442,7 +467,8 @@ func (m *MockPANOS) respondThreatCounters(w http.ResponseWriter) { } func (m *MockPANOS) respondGlobalProtect(w http.ResponseWriter) { - w.Write([]byte(` + //nolint:errcheck // test helper + _, _ = w.Write([]byte(` jsmith @@ -473,7 +499,8 @@ func (m *MockPANOS) respondGlobalProtect(w http.ResponseWriter) { } func (m *MockPANOS) respondLicenseInfo(w http.ResponseWriter) { - w.Write([]byte(` + //nolint:errcheck // test helper + _, _ = w.Write([]byte(` @@ -507,10 +534,11 @@ func (m *MockPANOS) respondLicenseInfo(w http.ResponseWriter) { func (m *MockPANOS) respondManagedDevices(w http.ResponseWriter) { if !m.IsPanorama { - w.Write([]byte(`Command not available on this device`)) + _, _ = w.Write([]byte(`Command not available on this device`)) //nolint:errcheck // test helper return } - w.Write([]byte(` + //nolint:errcheck // test helper + _, _ = w.Write([]byte(` diff --git a/internal/troubleshoot/results.go b/internal/troubleshoot/results.go index 4cb0af3..36117ce 100644 --- a/internal/troubleshoot/results.go +++ b/internal/troubleshoot/results.go @@ -9,12 +9,12 @@ import ( type StepStatus string const ( - StepStatusPending StepStatus = "pending" - StepStatusRunning StepStatus = "running" - StepStatusPassed StepStatus = "passed" - StepStatusFailed StepStatus = "failed" - StepStatusSkipped StepStatus = "skipped" - StepStatusError StepStatus = "error" + StepStatusPending StepStatus = "pending" + StepStatusRunning StepStatus = "running" + StepStatusPassed StepStatus = "passed" + StepStatusFailed StepStatus = "failed" + StepStatusSkipped StepStatus = "skipped" + StepStatusError StepStatus = "error" ) // RunbookResult contains the complete result of a runbook execution. diff --git a/internal/troubleshoot/results_test.go b/internal/troubleshoot/results_test.go index ffbcc79..6837adb 100644 --- a/internal/troubleshoot/results_test.go +++ b/internal/troubleshoot/results_test.go @@ -408,6 +408,9 @@ func TestStepResult_Fields(t *testing.T) { if stepResult.Duration != 100*time.Millisecond { t.Errorf("expected duration 100ms, got %v", stepResult.Duration) } + if stepResult.Error != nil { + t.Errorf("expected nil error, got %v", stepResult.Error) + } if len(stepResult.Matches) != 1 { t.Errorf("expected 1 match, got %d", len(stepResult.Matches)) } @@ -429,9 +432,27 @@ func TestIssue_Fields(t *testing.T) { if issue.StepID != "step1" { t.Errorf("expected StepID 'step1', got %q", issue.StepID) } + if issue.StepName != "Step Name" { + t.Errorf("expected StepName 'Step Name', got %q", issue.StepName) + } + if issue.PatternID != "pattern1" { + t.Errorf("expected PatternID 'pattern1', got %q", issue.PatternID) + } + if issue.PatternName != "Pattern Name" { + t.Errorf("expected PatternName 'Pattern Name', got %q", issue.PatternName) + } if issue.Severity != SeverityError { t.Errorf("expected severity Error, got %s", issue.Severity) } + if issue.Message != "Error message" { + t.Errorf("expected Message 'Error message', got %q", issue.Message) + } + if issue.MatchedText != "matched text" { + t.Errorf("expected MatchedText 'matched text', got %q", issue.MatchedText) + } + if issue.Remediation != "Fix it like this" { + t.Errorf("expected Remediation 'Fix it like this', got %q", issue.Remediation) + } if len(issue.KBArticles) != 2 { t.Errorf("expected 2 KB articles, got %d", len(issue.KBArticles)) } diff --git a/internal/troubleshoot/runbook.go b/internal/troubleshoot/runbook.go index b0da380..d52b61e 100644 --- a/internal/troubleshoot/runbook.go +++ b/internal/troubleshoot/runbook.go @@ -48,8 +48,8 @@ type Step struct { Name string `yaml:"name"` Description string `yaml:"description"` Type StepType `yaml:"type"` - Command string `yaml:"command"` // For SSH steps - APICall string `yaml:"api_call"` // For API steps + Command string `yaml:"command"` // For SSH steps + APICall string `yaml:"api_call"` // For API steps Patterns []Pattern `yaml:"patterns"` Required bool `yaml:"required"` // Stop on failure? } diff --git a/internal/troubleshoot/runbook_test.go b/internal/troubleshoot/runbook_test.go index e5dec22..ec5f87d 100644 --- a/internal/troubleshoot/runbook_test.go +++ b/internal/troubleshoot/runbook_test.go @@ -211,6 +211,15 @@ func TestRunbook_Fields(t *testing.T) { if runbook.Name != "Test Name" { t.Errorf("expected Name 'Test Name', got %q", runbook.Name) } + if runbook.Description != "Test Description" { + t.Errorf("expected Description 'Test Description', got %q", runbook.Description) + } + if runbook.Category != "test-category" { + t.Errorf("expected Category 'test-category', got %q", runbook.Category) + } + if len(runbook.Tags) != 2 || runbook.Tags[0] != "tag1" { + t.Errorf("expected Tags ['tag1', 'tag2'], got %v", runbook.Tags) + } if !runbook.RequiresSSH { t.Error("expected RequiresSSH to be true") } @@ -249,9 +258,18 @@ func TestStep_Fields(t *testing.T) { if step.ID != "step-id" { t.Errorf("expected ID 'step-id', got %q", step.ID) } + if step.Name != "Step Name" { + t.Errorf("expected Name 'Step Name', got %q", step.Name) + } + if step.Description != "Step description" { + t.Errorf("expected Description 'Step description', got %q", step.Description) + } if step.Type != StepTypeSSH { t.Errorf("expected type SSH, got %s", step.Type) } + if step.Command != "show clock" { + t.Errorf("expected Command 'show clock', got %q", step.Command) + } if !step.Required { t.Error("expected Required to be true") } @@ -274,9 +292,21 @@ func TestPattern_Fields(t *testing.T) { if pattern.ID != "pattern-id" { t.Errorf("expected ID 'pattern-id', got %q", pattern.ID) } + if pattern.Name != "Pattern Name" { + t.Errorf("expected Name 'Pattern Name', got %q", pattern.Name) + } + if pattern.Regex != `error\s+\d+` { + t.Errorf("expected Regex 'error\\s+\\d+', got %q", pattern.Regex) + } if pattern.Severity != SeverityCritical { t.Errorf("expected severity Critical, got %s", pattern.Severity) } + if pattern.Message != "Critical error found" { + t.Errorf("expected Message 'Critical error found', got %q", pattern.Message) + } + if pattern.Remediation != "Follow KB001 to resolve" { + t.Errorf("expected Remediation 'Follow KB001 to resolve', got %q", pattern.Remediation) + } if len(pattern.KBArticles) != 2 { t.Errorf("expected 2 KB articles, got %d", len(pattern.KBArticles)) } diff --git a/internal/tui/app.go b/internal/tui/app.go index c35c990..070791c 100644 --- a/internal/tui/app.go +++ b/internal/tui/app.go @@ -11,7 +11,6 @@ import ( "github.com/jp2195/pyre/internal/auth" "github.com/jp2195/pyre/internal/config" - "github.com/jp2195/pyre/internal/troubleshoot" "github.com/jp2195/pyre/internal/tui/views" ) @@ -24,7 +23,6 @@ const ( ViewNATPolicies ViewSessions ViewInterfaces - ViewTroubleshoot ViewLogs ViewPicker ViewDevicePicker @@ -61,16 +59,11 @@ type Model struct { natPolicies views.NATPoliciesModel sessions views.SessionsModel interfaces views.InterfacesModel - troubleshoot views.TroubleshootModel logs views.LogsModel picker views.PickerModel devicePicker views.DevicePickerModel commandPalette views.CommandPaletteModel previousView ViewState // Track previous view for Esc to return - - // Troubleshooting - tsRegistry *troubleshoot.Registry - tsEngine *troubleshoot.Engine } func NewModel(cfg *config.Config, creds *auth.Credentials) Model { @@ -93,7 +86,7 @@ func NewModel(cfg *config.Config, creds *auth.Credentials) Model { } if creds.HasAPIKey() && creds.HasHost() { - // Look up full firewall config by host to get SSH settings + // Look up full firewall config by host var fwConfig *config.FirewallConfig var connName string for name, fw := range cfg.Firewalls { @@ -126,17 +119,11 @@ func NewModel(cfg *config.Config, creds *auth.Credentials) Model { m.natPolicies = views.NewNATPoliciesModel() m.sessions = views.NewSessionsModel() m.interfaces = views.NewInterfacesModel() - m.troubleshoot = views.NewTroubleshootModel() m.logs = views.NewLogsModel() m.picker = views.NewPickerModel(session) m.devicePicker = views.NewDevicePickerModel() m.commandPalette = views.NewCommandPaletteModel() - // Initialize troubleshooting registry - m.tsRegistry = troubleshoot.NewRegistry() - m.tsRegistry.LoadEmbedded() - m.troubleshoot = m.troubleshoot.SetRunbooks(m.tsRegistry.List()) - return m } @@ -175,7 +162,6 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.natPolicies = m.natPolicies.SetSize(msg.Width, contentHeight) m.sessions = m.sessions.SetSize(msg.Width, contentHeight) m.interfaces = m.interfaces.SetSize(msg.Width, contentHeight) - m.troubleshoot = m.troubleshoot.SetSize(msg.Width, contentHeight) m.logs = m.logs.SetSize(msg.Width, contentHeight) m.picker = m.picker.SetSize(msg.Width, contentHeight) m.devicePicker = m.devicePicker.SetSize(msg.Width, contentHeight) @@ -296,7 +282,10 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { case LoginSuccessMsg: m.loading = false - // Look up full firewall config by host to get SSH settings + // Clear password from login model immediately after success + m.login = m.login.ClearPassword() + + // Look up full firewall config by host var fwConfig *config.FirewallConfig var connName string loginHost := m.login.Host() @@ -311,12 +300,11 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { if fwConfig == nil { fwConfig = &config.FirewallConfig{ Host: loginHost, - Insecure: true, + Insecure: m.login.Insecure(), } connName = msg.Name } - // Pass login credentials for SSH reuse - conn := m.session.AddConnectionWithSSH(connName, fwConfig, msg.APIKey, msg.Username, msg.Password) + conn := m.session.AddConnection(connName, fwConfig, msg.APIKey) m.currentView = ViewDashboard cmds = append(cmds, m.fetchCurrentDashboardData(), m.detectPanorama(conn)) @@ -368,13 +356,6 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { case SessionsMsg: m.sessions = m.sessions.SetSessions(msg.Sessions, msg.Err) - case TroubleshootResultMsg: - m.loading = false - m.troubleshoot = m.troubleshoot.SetResult(msg.Result, msg.Err) - - case TroubleshootStepMsg: - m.troubleshoot = m.troubleshoot.UpdateStepProgress(msg.StepIndex, msg.Status, msg.Output) - case PanoramaDetectedMsg: conn := m.session.GetActiveConnection() if conn != nil { @@ -390,16 +371,6 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { conn.ManagedDevices = msg.Devices } - case SSHConnectedMsg: - m.troubleshoot = m.troubleshoot.SetSSHConnecting(false) - m.troubleshoot = m.troubleshoot.SetSSHAvailable(true) - m.troubleshoot = m.troubleshoot.SetSSHError(nil) - - case SSHErrorMsg: - m.troubleshoot = m.troubleshoot.SetSSHConnecting(false) - m.troubleshoot = m.troubleshoot.SetSSHAvailable(false) - m.troubleshoot = m.troubleshoot.SetSSHError(msg.Err) - case SystemLogsMsg: m.logs = m.logs.SetSystemLogs(msg.Logs, msg.Err) @@ -431,15 +402,6 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, m.fetchSessions() case ViewInterfaces: return m, m.fetchInterfaces() - case ViewTroubleshoot: - conn := m.session.GetActiveConnection() - sshConfigured := conn != nil && conn.HasSSH() - m.troubleshoot = m.troubleshoot.SetSSHConfigured(sshConfigured) - if sshConfigured && !conn.SSHEnabled { - m.troubleshoot = m.troubleshoot.SetSSHConnecting(true) - return m, m.connectSSH(conn) - } - return m, m.updateTroubleshootSSH() case ViewLogs: m.logs = m.logs.SetLoading(true) return m, m.fetchLogs() @@ -480,7 +442,7 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.dashboard = m.dashboard.SetDiskUsage(msg.Disks, msg.Err) case EnvironmentalsMsg: - m.dashboard = m.dashboard.SetEnvironmentals(msg.Environmentals, msg.Err) + m.dashboard = m.dashboard.SetEnvironmentals(msg.Environmentals, msg.Err) //nolint:misspell // "environmentals" is the PAN-OS XML API tag name case CertificatesMsg: m.dashboard = m.dashboard.SetCertificates(msg.Certificates, msg.Err) @@ -560,9 +522,6 @@ func (m Model) View() string { case ViewInterfaces: content = m.interfaces.View() - case ViewTroubleshoot: - content = m.troubleshoot.View() - case ViewLogs: content = m.logs.View() } diff --git a/internal/tui/commands.go b/internal/tui/commands.go index c38a536..6df0b48 100644 --- a/internal/tui/commands.go +++ b/internal/tui/commands.go @@ -1,38 +1,35 @@ package tui import ( - "context" - "fmt" - "time" - tea "github.com/charmbracelet/bubbletea" + "github.com/jp2195/pyre/internal/api" "github.com/jp2195/pyre/internal/auth" - "github.com/jp2195/pyre/internal/troubleshoot" "github.com/jp2195/pyre/internal/tui/views" ) func (m Model) doLogin() tea.Cmd { ctx := m.ctx + host := m.login.Host() + username := m.login.Username() + password := m.login.Password() + insecure := m.login.Insecure() + return func() tea.Msg { - result, err := auth.GenerateAPIKey( - ctx, - m.login.Host(), - m.login.Username(), - m.login.Password(), - true, - ) + result, err := auth.GenerateAPIKey(ctx, host, username, password, insecure) if err != nil { return LoginErrorMsg{Err: err} } if result.Error != nil { return LoginErrorMsg{Err: result.Error} } + + // Password is now out of scope and will be garbage collected return LoginSuccessMsg{ - Name: m.login.Host(), + Name: host, APIKey: result.APIKey, - Username: m.login.Username(), - Password: m.login.Password(), + Username: username, + Insecure: insecure, } } } @@ -248,6 +245,7 @@ func (m Model) fetchDiskUsage(conn *auth.Connection) tea.Cmd { } } +//nolint:misspell // "environmentals" is the PAN-OS XML API tag name func (m Model) fetchEnvironmentals(conn *auth.Connection) tea.Cmd { ctx := m.ctx return func() tea.Msg { @@ -405,47 +403,3 @@ func (m Model) refreshCurrentView() tea.Cmd { } return nil } - -func (m *Model) updateTroubleshootSSH() tea.Cmd { - conn := m.session.GetActiveConnection() - sshConfigured := conn != nil && conn.HasSSH() - hasSSH := conn != nil && conn.SSHEnabled && conn.SSHClient != nil - m.troubleshoot = m.troubleshoot.SetSSHConfigured(sshConfigured) - m.troubleshoot = m.troubleshoot.SetSSHAvailable(hasSSH) - return nil -} - -func (m Model) connectSSH(conn *auth.Connection) tea.Cmd { - ctx := m.ctx - return func() tea.Msg { - if !conn.HasSSH() { - return nil - } - // Use a timeout derived from the app context - ctx, cancel := context.WithTimeout(ctx, 30*time.Second) - defer cancel() - - if err := conn.ConnectSSH(ctx); err != nil { - return SSHErrorMsg{ConnectionName: conn.Name, Err: err} - } - return SSHConnectedMsg{ConnectionName: conn.Name} - } -} - -func (m Model) runTroubleshoot(runbook *troubleshoot.Runbook) tea.Cmd { - conn := m.session.GetActiveConnection() - if conn == nil { - return func() tea.Msg { - return TroubleshootResultMsg{Err: fmt.Errorf("no active connection")} - } - } - - // Create engine with current connections - engine := troubleshoot.NewEngine(conn.Client, conn.SSHClient, m.tsRegistry) - ctx := m.ctx - - return func() tea.Msg { - result, err := engine.RunRunbook(ctx, runbook) - return TroubleshootResultMsg{Result: result, Err: err} - } -} diff --git a/internal/tui/handlers.go b/internal/tui/handlers.go index de35a54..24fc807 100644 --- a/internal/tui/handlers.go +++ b/internal/tui/handlers.go @@ -3,6 +3,7 @@ package tui import ( "github.com/charmbracelet/bubbles/key" tea "github.com/charmbracelet/bubbletea" + "github.com/jp2195/pyre/internal/auth" "github.com/jp2195/pyre/internal/tui/views" ) @@ -71,7 +72,10 @@ func (m Model) handleDevicePickerKeys(msg tea.KeyMsg) (tea.Model, tea.Cmd) { conn := m.session.GetActiveConnection() if conn != nil { device := m.devicePicker.SelectedDevice() - conn.SetTarget(device) + if err := conn.SetTarget(device); err != nil { + m.err = err + return m, nil + } m.currentView = ViewDashboard return m, m.fetchCurrentDashboardData() } @@ -182,13 +186,6 @@ func (m Model) buildCommandRegistry() []views.Command { }, // Tools - diagnostic and config - { - ID: "tools-troubleshoot", - Label: "Troubleshoot", - Description: "Diagnostic runbooks", - Category: "Tools", - Action: func() tea.Msg { return SwitchViewMsg{ViewTroubleshoot} }, - }, { ID: "tools-config", Label: "Config", @@ -263,27 +260,6 @@ func (m Model) handleViewKeys(msg tea.KeyMsg) (tea.Model, tea.Cmd) { m.sessions, cmd = m.sessions.Update(msg) case ViewInterfaces: m.interfaces, cmd = m.interfaces.Update(msg) - case ViewTroubleshoot: - // Handle 'R' to retry SSH connection - if msg.String() == "R" && m.troubleshoot.Mode() == views.TroubleshootModeList { - conn := m.session.GetActiveConnection() - if conn != nil && conn.HasSSH() { - conn.DisconnectSSH() - m.troubleshoot = m.troubleshoot.SetSSHConnecting(true) - m.troubleshoot = m.troubleshoot.SetSSHError(nil) - return m, m.connectSSH(conn) - } - } - // Handle Enter to run runbook - if msg.String() == "enter" && m.troubleshoot.Mode() == views.TroubleshootModeList { - runbook := m.troubleshoot.Selected() - if runbook != nil { - m.loading = true - m.troubleshoot = m.troubleshoot.SetRunning(runbook) - return m, m.runTroubleshoot(runbook) - } - } - m.troubleshoot, cmd = m.troubleshoot.Update(msg) case ViewLogs: m.logs, cmd = m.logs.Update(msg) } diff --git a/internal/tui/messages.go b/internal/tui/messages.go index be03eeb..6f269c1 100644 --- a/internal/tui/messages.go +++ b/internal/tui/messages.go @@ -2,7 +2,6 @@ package tui import ( "github.com/jp2195/pyre/internal/models" - "github.com/jp2195/pyre/internal/troubleshoot" "github.com/jp2195/pyre/internal/tui/views" ) @@ -77,7 +76,8 @@ type LoginSuccessMsg struct { Name string APIKey string Username string - Password string + Insecure bool + // Password is intentionally not included - credentials should not persist in messages. } type LoginErrorMsg struct { @@ -90,17 +90,6 @@ type ErrorMsg struct { Err error } -type TroubleshootResultMsg struct { - Result *troubleshoot.RunbookResult - Err error -} - -type TroubleshootStepMsg struct { - StepIndex int - Status troubleshoot.StepStatus - Output string -} - type ManagedDevicesMsg struct { Devices []models.ManagedDevice Err error @@ -111,15 +100,6 @@ type PanoramaDetectedMsg struct { Model string } -type SSHConnectedMsg struct { - ConnectionName string -} - -type SSHErrorMsg struct { - ConnectionName string - Err error -} - type SystemLogsMsg struct { Logs []models.SystemLogEntry Err error @@ -163,6 +143,7 @@ type DiskUsageMsg struct { Err error } +//nolint:misspell // "environmentals" is the PAN-OS XML API tag name type EnvironmentalsMsg struct { Environmentals []models.Environmental Err error diff --git a/internal/tui/navigation.go b/internal/tui/navigation.go index 8314085..ced4285 100644 --- a/internal/tui/navigation.go +++ b/internal/tui/navigation.go @@ -2,6 +2,7 @@ package tui import ( tea "github.com/charmbracelet/bubbletea" + "github.com/jp2195/pyre/internal/tui/views" ) @@ -76,13 +77,6 @@ func (m Model) navigateToCurrentItem() (tea.Model, tea.Cmd) { cmd = m.fetchLogs() // Tools group - case "troubleshoot": - m.currentView = ViewTroubleshoot - m.updateTroubleshootSSH() - conn := m.session.GetActiveConnection() - if conn != nil && conn.HasSSH() && conn.SSHClient == nil { - cmd = m.connectSSH(conn) - } case "config": m.currentView = ViewDashboard m.currentDashboard = views.DashboardConfig @@ -122,8 +116,6 @@ func (m *Model) syncNavbarToCurrentView() { m.navbar = m.navbar.SetActiveByID("analyze", "interfaces") case ViewLogs: m.navbar = m.navbar.SetActiveByID("analyze", "logs") - case ViewTroubleshoot: - m.navbar = m.navbar.SetActiveByID("tools", "troubleshoot") case ViewPicker: m.navbar = m.navbar.SetActiveByID("connections", "picker") } diff --git a/internal/tui/render.go b/internal/tui/render.go index c94e832..aefcc6a 100644 --- a/internal/tui/render.go +++ b/internal/tui/render.go @@ -5,6 +5,7 @@ import ( "strings" "github.com/charmbracelet/lipgloss" + "github.com/jp2195/pyre/internal/tui/views" ) @@ -98,8 +99,6 @@ func (m Model) currentViewName() string { return "Sessions" case ViewInterfaces: return "Interfaces" - case ViewTroubleshoot: - return "Troubleshoot" case ViewLogs: return "Logs" case ViewPicker: diff --git a/internal/tui/views/dashboard.go b/internal/tui/views/dashboard.go index aaabfcd..9f41a0f 100644 --- a/internal/tui/views/dashboard.go +++ b/internal/tui/views/dashboard.go @@ -50,7 +50,7 @@ type DashboardModel struct { licenses []models.LicenseInfo jobs []models.Job diskUsage []models.DiskUsage - environmentals []models.Environmental + environmentals []models.Environmental //nolint:misspell // "environmentals" is the PAN-OS XML API tag name certificates []models.Certificate natPools []models.NATPoolInfo @@ -195,12 +195,12 @@ func (m DashboardModel) View() string { } // Add disk usage panel to left column (health metric) - if m.diskUsage != nil && len(m.diskUsage) > 0 { + if len(m.diskUsage) > 0 { leftPanels = append(leftPanels, m.renderDiskUsage(leftColWidth)) } // Add hardware status panel to left column (health metric) - if m.environmentals != nil && len(m.environmentals) > 0 { + if len(m.environmentals) > 0 { //nolint:misspell // "environmentals" is the PAN-OS XML API tag name leftPanels = append(leftPanels, m.renderEnvironmentals(leftColWidth)) } @@ -216,7 +216,7 @@ func (m DashboardModel) View() string { } // NAT Pool Utilization (NEW) - if m.natPools != nil && len(m.natPools) > 0 { + if len(m.natPools) > 0 { rightPanels = append(rightPanels, m.renderNATPoolUtilization(rightColWidth)) } @@ -224,7 +224,7 @@ func (m DashboardModel) View() string { rightPanels = append(rightPanels, m.renderContentVersions(rightColWidth)) // Licenses - if m.licenses != nil && len(m.licenses) > 0 { + if len(m.licenses) > 0 { rightPanels = append(rightPanels, m.renderLicenses(rightColWidth)) } @@ -234,7 +234,7 @@ func (m DashboardModel) View() string { } // Admins Online - if m.admins != nil && len(m.admins) > 0 { + if len(m.admins) > 0 { rightPanels = append(rightPanels, m.renderLoggedInAdmins(rightColWidth)) } @@ -244,12 +244,12 @@ func (m DashboardModel) View() string { } // Recent Jobs - if m.jobs != nil && len(m.jobs) > 0 { + if len(m.jobs) > 0 { rightPanels = append(rightPanels, m.renderJobs(rightColWidth)) } // Certificates (expiring/expired) - if m.certificates != nil && len(m.certificates) > 0 { + if len(m.certificates) > 0 { rightPanels = append(rightPanels, m.renderCertificates(rightColWidth)) } @@ -266,12 +266,12 @@ func (m DashboardModel) renderSingleColumn(width int) string { } // Disk usage (health) - if m.diskUsage != nil && len(m.diskUsage) > 0 { + if len(m.diskUsage) > 0 { panels = append(panels, m.renderDiskUsage(width)) } // Hardware status (health) - if m.environmentals != nil && len(m.environmentals) > 0 { + if len(m.environmentals) > 0 { //nolint:misspell // "environmentals" is the PAN-OS XML API tag name panels = append(panels, m.renderEnvironmentals(width)) } @@ -281,19 +281,19 @@ func (m DashboardModel) renderSingleColumn(width int) string { } // NAT Pool Utilization - if m.natPools != nil && len(m.natPools) > 0 { + if len(m.natPools) > 0 { panels = append(panels, m.renderNATPoolUtilization(width)) } - if m.licenses != nil && len(m.licenses) > 0 { + if len(m.licenses) > 0 { panels = append(panels, m.renderLicenses(width)) } - if m.jobs != nil && len(m.jobs) > 0 { + if len(m.jobs) > 0 { panels = append(panels, m.renderJobs(width)) } - if m.certificates != nil && len(m.certificates) > 0 { + if len(m.certificates) > 0 { panels = append(panels, m.renderCertificates(width)) } @@ -1139,121 +1139,8 @@ func (m DashboardModel) renderNATPoolUtilization(width int) string { return panelStyle().Width(width).Render(b.String()) } -func (m DashboardModel) renderInterfaces(width int) string { - var b strings.Builder - b.WriteString(titleStyle().Render("Network Interfaces")) - b.WriteString("\n\n") - - if m.ifaceErr != nil { - b.WriteString(errorStyle().Render("Error: " + m.ifaceErr.Error())) - return panelStyle().Width(width).Render(b.String()) - } - if m.interfaces == nil { - b.WriteString(dimStyle().Render("Loading...")) - return panelStyle().Width(width).Render(b.String()) - } - - if len(m.interfaces) == 0 { - b.WriteString(dimStyle().Render("No interfaces configured")) - return panelStyle().Width(width).Render(b.String()) - } - - // Calculate column widths based on available space - availWidth := width - 8 - nameW := 16 - stateW := 6 - zoneW := 12 - ipW := availWidth - nameW - stateW - zoneW - 6 - - if ipW < 10 { - ipW = 15 - } - - // Header - headerStyle := DetailLabelStyle.Bold(true) - - header := fmt.Sprintf("%-*s %-*s %-*s %-*s", - nameW, "Interface", - stateW, "State", - zoneW, "Zone", - ipW, "IP Address") - b.WriteString(headerStyle.Render(header)) - b.WriteString("\n") - b.WriteString(dimStyle().Render(strings.Repeat("─", minInt(availWidth, len(header))))) - b.WriteString("\n") - - // Show interfaces, prioritizing those with IPs - maxRows := 8 - shown := 0 - upWithIP := []models.Interface{} - upNoIP := []models.Interface{} - downIfaces := []models.Interface{} - - for _, iface := range m.interfaces { - if iface.State == "up" { - if iface.IP != "" { - upWithIP = append(upWithIP, iface) - } else { - upNoIP = append(upNoIP, iface) - } - } else { - downIfaces = append(downIfaces, iface) - } - } - - // Display order: up with IP, up without IP, down - displayOrder := append(upWithIP, upNoIP...) - displayOrder = append(displayOrder, downIfaces...) - - for _, iface := range displayOrder { - if shown >= maxRows { - break - } - - stateStr := "up" - stStyle := highlightStyle() - if iface.State != "up" { - stateStr = "down" - stStyle = dimStyle() - } - - zone := iface.Zone - if zone == "" { - zone = "-" - } - - ip := iface.IP - if ip == "" { - ip = "-" - } - - row := fmt.Sprintf("%-*s %s %-*s %-*s", - nameW, truncateDash(iface.Name, nameW), - stStyle.Render(fmt.Sprintf("%-*s", stateW, stateStr)), - zoneW, truncateDash(zone, zoneW), - ipW, truncateDash(ip, ipW)) - b.WriteString(row) - b.WriteString("\n") - shown++ - } - - if len(m.interfaces) > maxRows { - remaining := len(m.interfaces) - maxRows - b.WriteString(dimStyle().Render(fmt.Sprintf("... and %d more", remaining))) - } - - return panelStyle().Width(width).Render(b.String()) -} - // Helper functions -func formatRow(label, value string, labelWidth int) string { - if value == "" { - return "" - } - return labelStyle().Width(labelWidth).Render(label+":") + " " + valueStyle().Render(value) + "\n" -} - func renderBar(percent float64, width int, color string) string { if percent < 0 { percent = 0 @@ -1310,10 +1197,6 @@ func formatThroughput(kbps int64) string { return fmt.Sprintf("%d Kbps", kbps) } -func truncateDash(s string, maxLen int) string { - return truncateEllipsis(s, maxLen) -} - func formatTimeAgo(t time.Time) string { if t.IsZero() { return "" diff --git a/internal/tui/views/dashboard_config.go b/internal/tui/views/dashboard_config.go index 20e4012..1735c08 100644 --- a/internal/tui/views/dashboard_config.go +++ b/internal/tui/views/dashboard_config.go @@ -126,9 +126,10 @@ func (m ConfigDashboardModel) renderPolicyStats(width int) string { if !rule.Disabled { enabledRules++ } - if rule.Action == "allow" { + switch rule.Action { + case "allow": allowRules++ - } else if rule.Action == "deny" || rule.Action == "drop" { + case "deny", "drop": denyRules++ } if rule.HitCount == 0 && !rule.Disabled { @@ -311,11 +312,14 @@ func (m ConfigDashboardModel) renderZeroHitRules(width int) string { rule := zeroHitRules[i] name := truncateEllipsis(rule.Name, nameWidth) - actionStyle := dimStyle() - if rule.Action == "allow" { + var actionStyle lipgloss.Style + switch rule.Action { + case "allow": actionStyle = highlightStyle() - } else if rule.Action == "deny" || rule.Action == "drop" { + case "deny", "drop": actionStyle = errorStyle() + default: + actionStyle = dimStyle() } b.WriteString(labelStyle().Render(fmt.Sprintf("%3d. ", rule.Position))) @@ -378,11 +382,14 @@ func (m ConfigDashboardModel) renderMostHitRules(width int) string { name := truncateEllipsis(rule.Name, nameWidth) - actionStyle := dimStyle() - if rule.Action == "allow" { + var actionStyle lipgloss.Style + switch rule.Action { + case "allow": actionStyle = highlightStyle() - } else if rule.Action == "deny" || rule.Action == "drop" { + case "deny", "drop": actionStyle = errorStyle() + default: + actionStyle = dimStyle() } b.WriteString(valueStyle().Render(fmt.Sprintf("%-*s ", nameWidth, name))) diff --git a/internal/tui/views/dashboard_network.go b/internal/tui/views/dashboard_network.go index 29fe1dd..2cacd4a 100644 --- a/internal/tui/views/dashboard_network.go +++ b/internal/tui/views/dashboard_network.go @@ -264,7 +264,7 @@ func (m NetworkDashboardModel) renderARPSummary(width int) string { name string count int } - var counts []ifaceCount + counts := make([]ifaceCount, 0, len(ifaceCounts)) for name, count := range ifaceCounts { counts = append(counts, ifaceCount{name, count}) } diff --git a/internal/tui/views/dashboard_security.go b/internal/tui/views/dashboard_security.go index a3a6e63..2320604 100644 --- a/internal/tui/views/dashboard_security.go +++ b/internal/tui/views/dashboard_security.go @@ -276,11 +276,14 @@ func (m SecurityDashboardModel) renderZeroHitRules(width int) string { rule := zeroHitRules[i] name := truncateEllipsis(rule.Name, nameWidth) - actionStyle := dimStyle() - if rule.Action == "allow" { + var actionStyle lipgloss.Style + switch rule.Action { + case "allow": actionStyle = highlightStyle() - } else if rule.Action == "deny" || rule.Action == "drop" { + case "deny", "drop": actionStyle = errorStyle() + default: + actionStyle = dimStyle() } b.WriteString(labelStyle().Render(fmt.Sprintf("%3d. ", rule.Position))) @@ -343,11 +346,14 @@ func (m SecurityDashboardModel) renderMostHitRules(width int) string { name := truncateEllipsis(rule.Name, nameWidth) - actionStyle := dimStyle() - if rule.Action == "allow" { + var actionStyle lipgloss.Style + switch rule.Action { + case "allow": actionStyle = highlightStyle() - } else if rule.Action == "deny" || rule.Action == "drop" { + case "deny", "drop": actionStyle = errorStyle() + default: + actionStyle = dimStyle() } b.WriteString(valueStyle().Render(fmt.Sprintf("%-*s ", nameWidth, name))) diff --git a/internal/tui/views/dashboard_vpn.go b/internal/tui/views/dashboard_vpn.go index 88f7c0d..4a6700f 100644 --- a/internal/tui/views/dashboard_vpn.go +++ b/internal/tui/views/dashboard_vpn.go @@ -187,14 +187,18 @@ func (m VPNDashboardModel) renderIPSecTunnels(width int) string { tunnel := m.tunnels[i] // State indicator - stateStyle := errorStyle() - stateIcon := "x" - if tunnel.State == "up" { + var stateStyle lipgloss.Style + var stateIcon string + switch tunnel.State { + case "up": stateStyle = highlightStyle() stateIcon = "o" - } else if tunnel.State == "init" { + case "init": stateStyle = warningStyle() stateIcon = "~" + default: + stateStyle = errorStyle() + stateIcon = "x" } name := truncateEllipsis(tunnel.Name, nameWidth) diff --git a/internal/tui/views/interfaces.go b/internal/tui/views/interfaces.go index e2c7852..4135fdb 100644 --- a/internal/tui/views/interfaces.go +++ b/internal/tui/views/interfaces.go @@ -132,7 +132,7 @@ func (m InterfacesModel) Update(msg tea.Msg) (InterfacesModel, tea.Cmd) { // Delegate to TableBase for common navigation visible := m.visibleCards() - base, handled, cmd := m.TableBase.HandleNavigation(msg, len(m.filtered), visible) + base, handled, cmd := m.HandleNavigation(msg, len(m.filtered), visible) if handled { m.TableBase = base return m, cmd @@ -142,7 +142,7 @@ func (m InterfacesModel) Update(msg tea.Msg) (InterfacesModel, tea.Cmd) { } func (m InterfacesModel) updateFilterMode(msg tea.Msg) (InterfacesModel, tea.Cmd) { - base, exited, cmd := m.TableBase.HandleFilterMode(msg) + base, exited, cmd := m.HandleFilterMode(msg) m.TableBase = base if exited { m.applyFilter() @@ -500,7 +500,7 @@ func (m InterfacesModel) renderHelp() string { {"r", "refresh"}, } - var parts []string + parts := make([]string, 0, len(keys)) for _, k := range keys { parts = append(parts, keyStyle.Render(k.key)+descStyle.Render(":"+k.desc)) } diff --git a/internal/tui/views/login.go b/internal/tui/views/login.go index 10a6dd8..ef5d1cb 100644 --- a/internal/tui/views/login.go +++ b/internal/tui/views/login.go @@ -26,6 +26,7 @@ type LoginModel struct { err error width int height int + insecure bool } func NewLoginModel(creds *auth.Credentials) LoginModel { @@ -54,6 +55,7 @@ func NewLoginModel(creds *auth.Credentials) LoginModel { usernameInput: username, passwordInput: password, focusedField: fieldHost, + insecure: creds.Insecure, } if creds.Host != "" { @@ -108,6 +110,17 @@ func (m LoginModel) Password() string { return m.passwordInput.Value() } +func (m LoginModel) Insecure() bool { + return m.insecure +} + +// ClearPassword clears the password from memory after successful login. +// This is a security measure to minimize the time credentials are in memory. +func (m LoginModel) ClearPassword() LoginModel { + m.passwordInput.SetValue("") + return m +} + func (m LoginModel) CanSubmit() bool { return m.Host() != "" && m.Username() != "" && m.Password() != "" } diff --git a/internal/tui/views/login_test.go b/internal/tui/views/login_test.go index 24ef413..e89e62c 100644 --- a/internal/tui/views/login_test.go +++ b/internal/tui/views/login_test.go @@ -153,10 +153,13 @@ func TestLoginModel_Update(t *testing.T) { // Type in host field msg := tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune("a")} - m, _ = m.Update(msg) + updated, _ := m.Update(msg) // The input should have processed the key - // (Actual value depends on textinput implementation) + // Verify the update returned a valid model + if updated.View() == "" { + t.Error("expected non-empty view after update") + } } func TestLoginModel_View(t *testing.T) { diff --git a/internal/tui/views/logs.go b/internal/tui/views/logs.go index b902e83..e1bdab2 100644 --- a/internal/tui/views/logs.go +++ b/internal/tui/views/logs.go @@ -318,7 +318,7 @@ func (m LogsModel) Update(msg tea.Msg) (LogsModel, tea.Cmd) { // Delegate to TableBase for common navigation visible := m.visibleRows() - base, handled, cmd := m.TableBase.HandleNavigation(msg, m.filteredCount(), visible) + base, handled, cmd := m.HandleNavigation(msg, m.filteredCount(), visible) if handled { m.TableBase = base return m, cmd @@ -681,7 +681,14 @@ func (m LogsModel) renderSystemDetail(log models.SystemLogEntry) string { panelStyle := DetailPanelStyle.Width(m.Width - 2) labelStyle := DetailLabelStyle.Width(12) - var lines []string + // Word wrap the description for better readability + descWidth := m.Width - 10 + if descWidth > 100 { + descWidth = 100 + } + wrapped := wrapText(log.Description, descWidth) + + lines := make([]string, 0, 7+len(wrapped)) lines = append(lines, ViewTitleStyle.Render("System Log Details")) lines = append(lines, "") lines = append(lines, labelStyle.Render("Time")+DetailValueStyle.Render(log.Time.Format("2006-01-02 15:04:05"))) @@ -690,12 +697,6 @@ func (m LogsModel) renderSystemDetail(log models.SystemLogEntry) string { lines = append(lines, "") lines = append(lines, ViewTitleStyle.Render("Message")) - // Word wrap the description for better readability - descWidth := m.Width - 10 - if descWidth > 100 { - descWidth = 100 - } - wrapped := wrapText(log.Description, descWidth) for _, line := range wrapped { lines = append(lines, DetailValueStyle.Render(line)) } @@ -819,7 +820,7 @@ func (m LogsModel) renderHelp() string { {"r", "refresh"}, } - var parts []string + parts := make([]string, 0, len(keys)) for _, k := range keys { parts = append(parts, HelpKeyStyle.Render(k.key)+HelpDescStyle.Render(":"+k.desc)) } diff --git a/internal/tui/views/nat_policies.go b/internal/tui/views/nat_policies.go index f373d6a..607dd1c 100644 --- a/internal/tui/views/nat_policies.go +++ b/internal/tui/views/nat_policies.go @@ -149,7 +149,7 @@ func (m NATPoliciesModel) Update(msg tea.Msg) (NATPoliciesModel, tea.Cmd) { // Delegate to TableBase for common navigation visible := m.visibleRows() - base, handled, cmd := m.TableBase.HandleNavigation(msg, len(m.filtered), visible) + base, handled, cmd := m.HandleNavigation(msg, len(m.filtered), visible) if handled { m.TableBase = base return m, cmd @@ -160,7 +160,7 @@ func (m NATPoliciesModel) Update(msg tea.Msg) (NATPoliciesModel, tea.Cmd) { } func (m NATPoliciesModel) updateFilter(msg tea.Msg) (NATPoliciesModel, tea.Cmd) { - base, exited, cmd := m.TableBase.HandleFilterMode(msg) + base, exited, cmd := m.HandleFilterMode(msg) m.TableBase = base if exited { m.applyFilter() diff --git a/internal/tui/views/navbar.go b/internal/tui/views/navbar.go index 86685cf..754b67e 100644 --- a/internal/tui/views/navbar.go +++ b/internal/tui/views/navbar.go @@ -59,8 +59,7 @@ func NewNavbarModel() NavbarModel { Label: "Tools", Key: "3", Items: []NavItem{ - {ID: "troubleshoot", Label: "Troubleshoot", Key: "1"}, - {ID: "config", Label: "Config", Key: "2"}, + {ID: "config", Label: "Config", Key: "1"}, }, }, { diff --git a/internal/tui/views/navbar_test.go b/internal/tui/views/navbar_test.go index db300d9..4a36235 100644 --- a/internal/tui/views/navbar_test.go +++ b/internal/tui/views/navbar_test.go @@ -166,8 +166,8 @@ func TestNavbarModel_SetActiveByID(t *testing.T) { t.Errorf("expected activeItem=2, got %d", nav.activeItem) } - // Set to tools/troubleshoot - nav = nav.SetActiveByID("tools", "troubleshoot") + // Set to tools/config + nav = nav.SetActiveByID("tools", "config") if nav.activeGroup != 2 { t.Errorf("expected activeGroup=2, got %d", nav.activeGroup) } diff --git a/internal/tui/views/policies.go b/internal/tui/views/policies.go index 5ae7c7c..a6ae464 100644 --- a/internal/tui/views/policies.go +++ b/internal/tui/views/policies.go @@ -158,7 +158,7 @@ func (m PoliciesModel) Update(msg tea.Msg) (PoliciesModel, tea.Cmd) { // Delegate to TableBase for common navigation visible := m.visibleRows() - base, handled, cmd := m.TableBase.HandleNavigation(msg, len(m.filtered), visible) + base, handled, cmd := m.HandleNavigation(msg, len(m.filtered), visible) if handled { m.TableBase = base return m, cmd @@ -169,7 +169,7 @@ func (m PoliciesModel) Update(msg tea.Msg) (PoliciesModel, tea.Cmd) { } func (m PoliciesModel) updateFilter(msg tea.Msg) (PoliciesModel, tea.Cmd) { - base, exited, cmd := m.TableBase.HandleFilterMode(msg) + base, exited, cmd := m.HandleFilterMode(msg) m.TableBase = base if exited { m.applyFilter() @@ -393,7 +393,7 @@ func (m PoliciesModel) renderDetail(p models.SecurityRule) string { b.WriteString("\n") b.WriteString(labelStyle.Render("Source Zones:") + " " + valueStyle.Render(formatListFull(p.SourceZones)) + "\n") b.WriteString(labelStyle.Render("Source Addr:") + " " + formatAddresses(p.Sources, p.NegateSource, valueStyle, dimValueStyle) + "\n") - if len(p.SourceUsers) > 0 && !(len(p.SourceUsers) == 1 && p.SourceUsers[0] == "any") { + if len(p.SourceUsers) > 0 && (len(p.SourceUsers) != 1 || p.SourceUsers[0] != "any") { b.WriteString(labelStyle.Render("Source Users:") + " " + valueStyle.Render(formatListFull(p.SourceUsers)) + "\n") } b.WriteString(labelStyle.Render("Dest Zones:") + " " + valueStyle.Render(formatListFull(p.DestZones)) + "\n") @@ -405,7 +405,7 @@ func (m PoliciesModel) renderDetail(p models.SecurityRule) string { b.WriteString("\n") b.WriteString(labelStyle.Render("Applications:") + " " + valueStyle.Render(formatListFull(p.Applications)) + "\n") b.WriteString(labelStyle.Render("Services:") + " " + valueStyle.Render(formatListFull(p.Services)) + "\n") - if len(p.URLCategories) > 0 && !(len(p.URLCategories) == 1 && p.URLCategories[0] == "any") { + if len(p.URLCategories) > 0 && (len(p.URLCategories) != 1 || p.URLCategories[0] != "any") { b.WriteString(labelStyle.Render("URL Categories:") + " " + valueStyle.Render(formatListFull(p.URLCategories)) + "\n") } diff --git a/internal/tui/views/sessions.go b/internal/tui/views/sessions.go index ce829da..8d585b9 100644 --- a/internal/tui/views/sessions.go +++ b/internal/tui/views/sessions.go @@ -143,7 +143,7 @@ func (m SessionsModel) Update(msg tea.Msg) (SessionsModel, tea.Cmd) { // Delegate to TableBase for common navigation visible := m.visibleRows() - base, handled, cmd := m.TableBase.HandleNavigation(msg, len(m.filtered), visible) + base, handled, cmd := m.HandleNavigation(msg, len(m.filtered), visible) if handled { m.TableBase = base return m, cmd @@ -154,7 +154,7 @@ func (m SessionsModel) Update(msg tea.Msg) (SessionsModel, tea.Cmd) { } func (m SessionsModel) updateFilter(msg tea.Msg) (SessionsModel, tea.Cmd) { - base, exited, cmd := m.TableBase.HandleFilterMode(msg) + base, exited, cmd := m.HandleFilterMode(msg) m.TableBase = base if exited { m.applyFilter() diff --git a/internal/tui/views/table_base_test.go b/internal/tui/views/table_base_test.go index d7b3590..10a7c41 100644 --- a/internal/tui/views/table_base_test.go +++ b/internal/tui/views/table_base_test.go @@ -184,15 +184,18 @@ func TestTableBase_HandleNavigation(t *testing.T) { tb := NewTableBase("") tb.Cursor = tt.cursor - msg := tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune(tt.key)} - if tt.key == "down" { + var msg tea.KeyMsg + switch tt.key { + case "down": msg = tea.KeyMsg{Type: tea.KeyDown} - } else if tt.key == "up" { + case "up": msg = tea.KeyMsg{Type: tea.KeyUp} - } else if tt.key == "home" { + case "home": msg = tea.KeyMsg{Type: tea.KeyHome} - } else if tt.key == "end" { + case "end": msg = tea.KeyMsg{Type: tea.KeyEnd} + default: + msg = tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune(tt.key)} } result, handled, _ := tb.HandleNavigation(msg, tt.itemCount, tt.visibleRows)