From 15e1558d68a86ccdd59e4f549f8d82852bb2b73d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 24 Mar 2026 20:56:38 +0000 Subject: [PATCH 1/8] Initial plan From 03a5c3f0a2f5561c114a026fe4876b3f34a85c81 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 24 Mar 2026 21:17:15 +0000 Subject: [PATCH 2/8] feat: add firewall policy rule attribution to audit command Add PolicyManifest, PolicyRule Go structs and audit.jsonl JSONL parser. Port enrichWithPolicyRules/domainMatchesRule from TypeScript to Go. Auto-detect firewall audit artifacts (policy-manifest.json + audit.jsonl). Integrate enriched policy analysis into audit console and JSON output. Add comprehensive unit tests for all new functionality. Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw/sessions/36ac2fc6-303e-452b-b36f-cadde5ac87f8 --- pkg/cli/audit.go | 8 + pkg/cli/audit_report.go | 2 + pkg/cli/audit_report_render.go | 85 ++++++ pkg/cli/firewall_policy.go | 368 ++++++++++++++++++++++++ pkg/cli/firewall_policy_test.go | 484 ++++++++++++++++++++++++++++++++ pkg/cli/logs_models.go | 2 + 6 files changed, 949 insertions(+) create mode 100644 pkg/cli/firewall_policy.go create mode 100644 pkg/cli/firewall_policy_test.go diff --git a/pkg/cli/audit.go b/pkg/cli/audit.go index 12bcc2f104b..eab09877d7e 100644 --- a/pkg/cli/audit.go +++ b/pkg/cli/audit.go @@ -322,6 +322,12 @@ func AuditWorkflowRun(ctx context.Context, runID int64, owner, repo, hostname st fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to analyze firewall logs: %v", err))) } + // Analyze firewall policy artifacts if available (policy-manifest.json + audit.jsonl) + policyAnalysis, err := analyzeFirewallPolicy(runOutputDir, verbose) + if err != nil && verbose { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to analyze firewall policy: %v", err))) + } + // Analyze redacted domains if available redactedDomainsAnalysis, err := analyzeRedactedDomains(runOutputDir, verbose) if err != nil && verbose { @@ -344,6 +350,7 @@ func AuditWorkflowRun(ctx context.Context, runID int64, owner, repo, hostname st processedRun := ProcessedRun{ Run: run, FirewallAnalysis: firewallAnalysis, + PolicyAnalysis: policyAnalysis, RedactedDomainsAnalysis: redactedDomainsAnalysis, MissingTools: missingTools, MissingData: missingData, @@ -415,6 +422,7 @@ func AuditWorkflowRun(ctx context.Context, runID int64, owner, repo, hostname st Metrics: metrics, AccessAnalysis: accessAnalysis, FirewallAnalysis: firewallAnalysis, + PolicyAnalysis: policyAnalysis, RedactedDomainsAnalysis: redactedDomainsAnalysis, MissingTools: missingTools, MissingData: missingData, diff --git a/pkg/cli/audit_report.go b/pkg/cli/audit_report.go index 125ca960a16..0a068c57559 100644 --- a/pkg/cli/audit_report.go +++ b/pkg/cli/audit_report.go @@ -32,6 +32,7 @@ type AuditData struct { Noops []NoopReport `json:"noops,omitempty"` MCPFailures []MCPFailureReport `json:"mcp_failures,omitempty"` FirewallAnalysis *FirewallAnalysis `json:"firewall_analysis,omitempty"` + PolicyAnalysis *PolicyAnalysis `json:"policy_analysis,omitempty"` RedactedDomainsAnalysis *RedactedDomainsAnalysis `json:"redacted_domains_analysis,omitempty"` Errors []ErrorInfo `json:"errors,omitempty"` Warnings []ErrorInfo `json:"warnings,omitempty"` @@ -329,6 +330,7 @@ func buildAuditData(processedRun ProcessedRun, metrics LogMetrics, mcpToolUsage Noops: processedRun.Noops, MCPFailures: processedRun.MCPFailures, FirewallAnalysis: processedRun.FirewallAnalysis, + PolicyAnalysis: processedRun.PolicyAnalysis, RedactedDomainsAnalysis: processedRun.RedactedDomainsAnalysis, Errors: errors, Warnings: warnings, diff --git a/pkg/cli/audit_report_render.go b/pkg/cli/audit_report_render.go index 1882e1b5942..6e4b0a2cc15 100644 --- a/pkg/cli/audit_report_render.go +++ b/pkg/cli/audit_report_render.go @@ -3,9 +3,11 @@ package cli import ( "encoding/json" "fmt" + "math" "os" "path/filepath" "strconv" + "time" "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/sliceutil" @@ -120,6 +122,13 @@ func renderConsole(data AuditData, logsPath string) { renderFirewallAnalysis(data.FirewallAnalysis) } + // Firewall Policy Analysis Section (enriched with rule attribution) + if data.PolicyAnalysis != nil && len(data.PolicyAnalysis.RuleHits) > 0 { + fmt.Fprintln(os.Stderr, console.FormatSectionHeader("Firewall Policy Analysis")) + fmt.Fprintln(os.Stderr) + renderPolicyAnalysis(data.PolicyAnalysis) + } + // Redacted Domains Section if data.RedactedDomainsAnalysis != nil && data.RedactedDomainsAnalysis.TotalDomains > 0 { fmt.Fprintln(os.Stderr, console.FormatSectionHeader("🔒 Redacted URL Domains")) @@ -563,3 +572,79 @@ func renderPerformanceMetrics(metrics *PerformanceMetrics) { fmt.Fprintln(os.Stderr) } + +// renderPolicyAnalysis renders the enriched firewall policy analysis with rule attribution +func renderPolicyAnalysis(analysis *PolicyAnalysis) { + auditReportLog.Printf("Rendering policy analysis: rules=%d, denied=%d", len(analysis.RuleHits), analysis.DeniedCount) + + // Policy summary + fmt.Fprintf(os.Stderr, " Policy: %s\n", analysis.PolicySummary) + fmt.Fprintln(os.Stderr) + + // Summary statistics + fmt.Fprintf(os.Stderr, " Total Requests : %d\n", analysis.TotalRequests) + fmt.Fprintf(os.Stderr, " Allowed : %d\n", analysis.AllowedCount) + fmt.Fprintf(os.Stderr, " Denied : %d\n", analysis.DeniedCount) + fmt.Fprintf(os.Stderr, " Unique Domains : %d\n", analysis.UniqueDomains) + fmt.Fprintln(os.Stderr) + + // Rule hit table + if len(analysis.RuleHits) > 0 { + fmt.Fprintln(os.Stderr, " Policy Rules:") + fmt.Fprintln(os.Stderr) + + ruleConfig := console.TableConfig{ + Headers: []string{"Rule", "Action", "Description", "Hits"}, + Rows: make([][]string, 0, len(analysis.RuleHits)), + } + + for _, rh := range analysis.RuleHits { + row := []string{ + stringutil.Truncate(rh.Rule.ID, 30), + rh.Rule.Action, + stringutil.Truncate(rh.Rule.Description, 50), + strconv.Itoa(rh.Hits), + } + ruleConfig.Rows = append(ruleConfig.Rows, row) + } + + fmt.Fprint(os.Stderr, console.RenderTable(ruleConfig)) + fmt.Fprintln(os.Stderr) + } + + // Denied requests detail + if len(analysis.DeniedRequests) > 0 { + fmt.Fprintf(os.Stderr, " Denied Requests (%d):\n", len(analysis.DeniedRequests)) + fmt.Fprintln(os.Stderr) + + deniedConfig := console.TableConfig{ + Headers: []string{"Time", "Domain", "Rule", "Reason"}, + Rows: make([][]string, 0, len(analysis.DeniedRequests)), + } + + for _, req := range analysis.DeniedRequests { + timeStr := formatUnixTimestamp(req.Timestamp) + row := []string{ + timeStr, + stringutil.Truncate(req.Host, 40), + stringutil.Truncate(req.RuleID, 25), + stringutil.Truncate(req.Reason, 40), + } + deniedConfig.Rows = append(deniedConfig.Rows, row) + } + + fmt.Fprint(os.Stderr, console.RenderTable(deniedConfig)) + fmt.Fprintln(os.Stderr) + } +} + +// formatUnixTimestamp converts a Unix timestamp (float64) to a human-readable time string (HH:MM:SS). +func formatUnixTimestamp(ts float64) string { + if ts <= 0 { + return "-" + } + sec := int64(math.Floor(ts)) + nsec := int64((ts - float64(sec)) * 1e9) + t := time.Unix(sec, nsec).UTC() + return t.Format("15:04:05") +} diff --git a/pkg/cli/firewall_policy.go b/pkg/cli/firewall_policy.go new file mode 100644 index 00000000000..a0c865f3150 --- /dev/null +++ b/pkg/cli/firewall_policy.go @@ -0,0 +1,368 @@ +package cli + +import ( + "bufio" + "encoding/json" + "fmt" + "os" + "path/filepath" + "sort" + "strings" + + "github.com/github/gh-aw/pkg/logger" +) + +var firewallPolicyLog = logger.New("cli:firewall_policy") + +// PolicyManifest represents the policy-manifest.json file generated by AWF. +// It describes the firewall policy rules and configuration used during a workflow run. +type PolicyManifest struct { + Version int `json:"version"` + GeneratedAt string `json:"generatedAt"` + Rules []PolicyRule `json:"rules"` + DangerousPorts []int `json:"dangerousPorts,omitempty"` + DNSServers []string `json:"dnsServers,omitempty"` + SSLBumpEnabled bool `json:"sslBumpEnabled"` + DLPEnabled bool `json:"dlpEnabled"` +} + +// PolicyRule represents a single firewall policy rule from the manifest. +type PolicyRule struct { + ID string `json:"id"` + Order int `json:"order"` + Action string `json:"action"` // "allow" or "deny" + ACLName string `json:"aclName"` + Protocol string `json:"protocol"` // "both", "http", "https" + Domains []string `json:"domains"` + Description string `json:"description"` +} + +// AuditLogEntry represents a single entry from audit.jsonl. +// This is the JSONL format written by the AWF audit subsystem. +type AuditLogEntry struct { + Timestamp float64 `json:"ts"` + Client string `json:"client,omitempty"` + Host string `json:"host"` + Dest string `json:"dest,omitempty"` + Method string `json:"method,omitempty"` + Status int `json:"status"` + Decision string `json:"decision,omitempty"` + URL string `json:"url,omitempty"` +} + +// EnrichedRequest represents a firewall request enriched with policy rule attribution. +type EnrichedRequest struct { + Timestamp float64 `json:"ts"` + Host string `json:"host"` + Status int `json:"status"` + RuleID string `json:"rule_id"` + Action string `json:"action"` // "allow" or "deny" + Reason string `json:"reason,omitempty"` +} + +// RuleHitStats tracks hit counts per policy rule. +type RuleHitStats struct { + Rule PolicyRule `json:"rule"` + Hits int `json:"hits"` +} + +// PolicyAnalysis contains the enriched policy analysis results. +type PolicyAnalysis struct { + PolicySummary string `json:"policy_summary"` + RuleHits []RuleHitStats `json:"rule_hits"` + DeniedRequests []EnrichedRequest `json:"denied_requests,omitempty"` + TotalRequests int `json:"total_requests"` + AllowedCount int `json:"allowed_count"` + DeniedCount int `json:"denied_count"` + UniqueDomains int `json:"unique_domains"` +} + +// loadPolicyManifest loads and parses a policy-manifest.json file. +func loadPolicyManifest(manifestPath string) (*PolicyManifest, error) { + firewallPolicyLog.Printf("Loading policy manifest: %s", manifestPath) + + data, err := os.ReadFile(manifestPath) + if err != nil { + return nil, fmt.Errorf("failed to read policy manifest: %w", err) + } + + var manifest PolicyManifest + if err := json.Unmarshal(data, &manifest); err != nil { + return nil, fmt.Errorf("failed to parse policy manifest: %w", err) + } + + // Sort rules by order for deterministic matching + sort.Slice(manifest.Rules, func(i, j int) bool { + return manifest.Rules[i].Order < manifest.Rules[j].Order + }) + + firewallPolicyLog.Printf("Loaded policy manifest: version=%d, rules=%d, ssl_bump=%v, dlp=%v", + manifest.Version, len(manifest.Rules), manifest.SSLBumpEnabled, manifest.DLPEnabled) + + return &manifest, nil +} + +// parseAuditJSONL parses an audit.jsonl file and returns a slice of audit log entries. +func parseAuditJSONL(jsonlPath string) ([]AuditLogEntry, error) { + firewallPolicyLog.Printf("Parsing audit JSONL: %s", jsonlPath) + + file, err := os.Open(jsonlPath) + if err != nil { + return nil, fmt.Errorf("failed to open audit JSONL: %w", err) + } + defer file.Close() + + var entries []AuditLogEntry + scanner := bufio.NewScanner(file) + // Set larger buffer for potentially long lines + scanner.Buffer(make([]byte, 0, 64*1024), 1024*1024) + + lineNum := 0 + for scanner.Scan() { + lineNum++ + line := strings.TrimSpace(scanner.Text()) + if line == "" { + continue + } + + var entry AuditLogEntry + if err := json.Unmarshal([]byte(line), &entry); err != nil { + firewallPolicyLog.Printf("Skipping malformed JSONL line %d: %v", lineNum, err) + continue + } + entries = append(entries, entry) + } + + if err := scanner.Err(); err != nil { + return nil, fmt.Errorf("error reading audit JSONL: %w", err) + } + + firewallPolicyLog.Printf("Parsed %d audit log entries from %d lines", len(entries), lineNum) + return entries, nil +} + +// domainMatchesRule checks if a domain matches a policy rule. +// Domain matching follows AWF conventions: +// - ".github.com" matches both "github.com" and "*.github.com" (e.g., "api.github.com") +// - "github.com" matches only exactly "github.com" +func domainMatchesRule(host string, rule PolicyRule) bool { + // Strip port from host if present + domain := host + if idx := strings.LastIndex(host, ":"); idx != -1 { + domain = host[:idx] + } + domain = strings.ToLower(domain) + + for _, pattern := range rule.Domains { + pattern = strings.ToLower(pattern) + + if strings.HasPrefix(pattern, ".") { + // ".github.com" matches "github.com" and "*.github.com" + baseDomain := pattern[1:] // Remove leading dot + if domain == baseDomain || strings.HasSuffix(domain, pattern) { + return true + } + } else { + // Exact match + if domain == pattern { + return true + } + } + } + return false +} + +// findMatchingRule finds the first matching policy rule for a given host. +// Rules are evaluated in order; the first matching rule wins. +func findMatchingRule(host string, rules []PolicyRule) *PolicyRule { + for i := range rules { + if domainMatchesRule(host, rules[i]) { + return &rules[i] + } + } + return nil +} + +// enrichWithPolicyRules enriches audit log entries with policy rule attribution. +func enrichWithPolicyRules(entries []AuditLogEntry, manifest *PolicyManifest) *PolicyAnalysis { + firewallPolicyLog.Printf("Enriching %d entries with %d policy rules", len(entries), len(manifest.Rules)) + + ruleHitMap := make(map[string]int) + var deniedRequests []EnrichedRequest + uniqueDomains := make(map[string]bool) + allowedCount := 0 + deniedCount := 0 + + for _, entry := range entries { + host := entry.Host + if host == "" || host == "-" { + continue + } + + // Strip port for domain tracking + domain := host + if idx := strings.LastIndex(host, ":"); idx != -1 { + domain = host[:idx] + } + uniqueDomains[domain] = true + + rule := findMatchingRule(host, manifest.Rules) + + var enriched EnrichedRequest + enriched.Timestamp = entry.Timestamp + enriched.Host = host + enriched.Status = entry.Status + + if rule != nil { + enriched.RuleID = rule.ID + enriched.Action = rule.Action + ruleHitMap[rule.ID]++ + + if rule.Action == "deny" { + enriched.Reason = rule.Description + deniedRequests = append(deniedRequests, enriched) + deniedCount++ + } else { + allowedCount++ + } + } else { + // No matching rule — implicit deny + enriched.RuleID = "(implicit-deny)" + enriched.Action = "deny" + enriched.Reason = "No matching policy rule" + deniedRequests = append(deniedRequests, enriched) + deniedCount++ + } + } + + // Build rule hits table, preserving rule order + var ruleHits []RuleHitStats + for _, rule := range manifest.Rules { + hits := ruleHitMap[rule.ID] + ruleHits = append(ruleHits, RuleHitStats{ + Rule: rule, + Hits: hits, + }) + } + + // Build policy summary string + sslBump := "disabled" + if manifest.SSLBumpEnabled { + sslBump = "enabled" + } + dlp := "disabled" + if manifest.DLPEnabled { + dlp = "enabled" + } + policySummary := fmt.Sprintf("%d rules, SSL Bump %s, DLP %s", len(manifest.Rules), sslBump, dlp) + + totalProcessed := allowedCount + deniedCount + + firewallPolicyLog.Printf("Enrichment complete: total=%d, allowed=%d, denied=%d, unique_domains=%d", + totalProcessed, allowedCount, deniedCount, len(uniqueDomains)) + + return &PolicyAnalysis{ + PolicySummary: policySummary, + RuleHits: ruleHits, + DeniedRequests: deniedRequests, + TotalRequests: totalProcessed, + AllowedCount: allowedCount, + DeniedCount: deniedCount, + UniqueDomains: len(uniqueDomains), + } +} + +// detectFirewallAuditArtifacts looks for policy-manifest.json and audit.jsonl in the run directory. +// Returns the paths if found, or empty strings if not present. +func detectFirewallAuditArtifacts(runDir string) (manifestPath, auditJSONLPath string) { + firewallPolicyLog.Printf("Detecting firewall audit artifacts in: %s", runDir) + + // Check for artifacts in sandbox/firewall/audit/ (primary path after artifact download) + auditDir := filepath.Join(runDir, "sandbox", "firewall", "audit") + manifestCandidate := filepath.Join(auditDir, "policy-manifest.json") + auditCandidate := filepath.Join(auditDir, "audit.jsonl") + + if _, err := os.Stat(manifestCandidate); err == nil { + manifestPath = manifestCandidate + firewallPolicyLog.Printf("Found policy manifest: %s", manifestPath) + } + + if _, err := os.Stat(auditCandidate); err == nil { + auditJSONLPath = auditCandidate + firewallPolicyLog.Printf("Found audit JSONL: %s", auditJSONLPath) + } + + // Also check for firewall-audit-logs directory (artifact name based path) + if manifestPath == "" || auditJSONLPath == "" { + entries, err := os.ReadDir(runDir) + if err != nil { + return + } + for _, entry := range entries { + if !entry.IsDir() { + continue + } + name := entry.Name() + if strings.HasPrefix(name, "firewall-audit") { + dir := filepath.Join(runDir, name) + if manifestPath == "" { + candidate := filepath.Join(dir, "policy-manifest.json") + if _, err := os.Stat(candidate); err == nil { + manifestPath = candidate + firewallPolicyLog.Printf("Found policy manifest in %s: %s", name, manifestPath) + } + } + if auditJSONLPath == "" { + candidate := filepath.Join(dir, "audit.jsonl") + if _, err := os.Stat(candidate); err == nil { + auditJSONLPath = candidate + firewallPolicyLog.Printf("Found audit JSONL in %s: %s", name, auditJSONLPath) + } + } + } + } + } + + return manifestPath, auditJSONLPath +} + +// analyzeFirewallPolicy loads policy manifest and audit JSONL, then enriches with rule attribution. +// Returns nil if no policy artifacts are found (graceful fallback to basic analysis). +func analyzeFirewallPolicy(runDir string, verbose bool) (*PolicyAnalysis, error) { + manifestPath, auditJSONLPath := detectFirewallAuditArtifacts(runDir) + + if manifestPath == "" { + firewallPolicyLog.Print("No policy manifest found, skipping policy analysis") + return nil, nil + } + + manifest, err := loadPolicyManifest(manifestPath) + if err != nil { + return nil, fmt.Errorf("failed to load policy manifest: %w", err) + } + + if auditJSONLPath == "" { + // We have a manifest but no audit log — return policy summary without request enrichment + firewallPolicyLog.Print("No audit JSONL found, returning manifest-only analysis") + sslBump := "disabled" + if manifest.SSLBumpEnabled { + sslBump = "enabled" + } + dlp := "disabled" + if manifest.DLPEnabled { + dlp = "enabled" + } + + return &PolicyAnalysis{ + PolicySummary: fmt.Sprintf("%d rules, SSL Bump %s, DLP %s", len(manifest.Rules), sslBump, dlp), + RuleHits: make([]RuleHitStats, 0), + }, nil + } + + entries, err := parseAuditJSONL(auditJSONLPath) + if err != nil { + return nil, fmt.Errorf("failed to parse audit JSONL: %w", err) + } + + return enrichWithPolicyRules(entries, manifest), nil +} diff --git a/pkg/cli/firewall_policy_test.go b/pkg/cli/firewall_policy_test.go new file mode 100644 index 00000000000..7e4fecf108e --- /dev/null +++ b/pkg/cli/firewall_policy_test.go @@ -0,0 +1,484 @@ +//go:build !integration + +package cli + +import ( + "encoding/json" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestDomainMatchesRule(t *testing.T) { + tests := []struct { + name string + host string + rule PolicyRule + expected bool + }{ + { + name: "exact match without port", + host: "github.com", + rule: PolicyRule{ + Domains: []string{"github.com"}, + }, + expected: true, + }, + { + name: "exact match with port", + host: "github.com:443", + rule: PolicyRule{ + Domains: []string{"github.com"}, + }, + expected: true, + }, + { + name: "wildcard match - subdomain", + host: "api.github.com:443", + rule: PolicyRule{ + Domains: []string{".github.com"}, + }, + expected: true, + }, + { + name: "wildcard match - base domain", + host: "github.com:443", + rule: PolicyRule{ + Domains: []string{".github.com"}, + }, + expected: true, + }, + { + name: "wildcard match - deep subdomain", + host: "api.v2.github.com:443", + rule: PolicyRule{ + Domains: []string{".github.com"}, + }, + expected: true, + }, + { + name: "no match - different domain", + host: "evil.com:443", + rule: PolicyRule{ + Domains: []string{".github.com"}, + }, + expected: false, + }, + { + name: "no match - suffix collision", + host: "notgithub.com:443", + rule: PolicyRule{ + Domains: []string{".github.com"}, + }, + expected: false, + }, + { + name: "case insensitive match", + host: "API.GitHub.COM:443", + rule: PolicyRule{ + Domains: []string{".github.com"}, + }, + expected: true, + }, + { + name: "multiple domains in rule", + host: "npmjs.org:443", + rule: PolicyRule{ + Domains: []string{".github.com", "npmjs.org"}, + }, + expected: true, + }, + { + name: "no match - empty domains", + host: "example.com", + rule: PolicyRule{ + Domains: []string{}, + }, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := domainMatchesRule(tt.host, tt.rule) + assert.Equal(t, tt.expected, result, "domainMatchesRule(%q) should return %v", tt.host, tt.expected) + }) + } +} + +func TestFindMatchingRule(t *testing.T) { + rules := []PolicyRule{ + { + ID: "allow-github", + Order: 1, + Action: "allow", + Domains: []string{".github.com"}, + }, + { + ID: "allow-npm", + Order: 2, + Action: "allow", + Domains: []string{"registry.npmjs.org"}, + }, + { + ID: "deny-all", + Order: 3, + Action: "deny", + Domains: []string{"."}, + }, + } + + t.Run("matches first rule", func(t *testing.T) { + rule := findMatchingRule("api.github.com:443", rules) + require.NotNil(t, rule, "Should find a matching rule") + assert.Equal(t, "allow-github", rule.ID, "Should match allow-github rule") + }) + + t.Run("matches second rule", func(t *testing.T) { + rule := findMatchingRule("registry.npmjs.org:443", rules) + require.NotNil(t, rule, "Should find a matching rule") + assert.Equal(t, "allow-npm", rule.ID, "Should match allow-npm rule") + }) + + t.Run("no match returns nil", func(t *testing.T) { + // evil.com does not match any of the specific rules, and the deny-all + // rule has domain "." which won't match via our matching logic + rule := findMatchingRule("evil.com:443", rules[:2]) + assert.Nil(t, rule, "Should not find a matching rule for unlisted domain") + }) + + t.Run("first matching rule wins", func(t *testing.T) { + // github.com matches both allow-github (.github.com) and potentially others + rule := findMatchingRule("github.com:443", rules) + require.NotNil(t, rule, "Should find a matching rule") + assert.Equal(t, "allow-github", rule.ID, "First matching rule should win") + }) +} + +func TestLoadPolicyManifest(t *testing.T) { + t.Run("valid manifest", func(t *testing.T) { + dir := t.TempDir() + manifestPath := filepath.Join(dir, "policy-manifest.json") + + manifest := PolicyManifest{ + Version: 1, + GeneratedAt: "2026-01-01T00:00:00Z", + Rules: []PolicyRule{ + {ID: "rule-b", Order: 2, Action: "deny", Domains: []string{".evil.com"}, Description: "Block evil"}, + {ID: "rule-a", Order: 1, Action: "allow", Domains: []string{".github.com"}, Description: "Allow GitHub"}, + }, + SSLBumpEnabled: false, + DLPEnabled: false, + } + + data, err := json.Marshal(manifest) + require.NoError(t, err, "Should marshal manifest") + require.NoError(t, os.WriteFile(manifestPath, data, 0644), "Should write manifest file") + + loaded, err := loadPolicyManifest(manifestPath) + require.NoError(t, err, "Should load manifest without error") + require.NotNil(t, loaded, "Loaded manifest should not be nil") + + assert.Equal(t, 1, loaded.Version, "Version should be 1") + assert.Len(t, loaded.Rules, 2, "Should have 2 rules") + // Rules should be sorted by order + assert.Equal(t, "rule-a", loaded.Rules[0].ID, "First rule should be rule-a (order 1)") + assert.Equal(t, "rule-b", loaded.Rules[1].ID, "Second rule should be rule-b (order 2)") + }) + + t.Run("missing file", func(t *testing.T) { + _, err := loadPolicyManifest("/nonexistent/path/policy-manifest.json") + assert.Error(t, err, "Should return error for missing file") + }) + + t.Run("invalid JSON", func(t *testing.T) { + dir := t.TempDir() + manifestPath := filepath.Join(dir, "policy-manifest.json") + require.NoError(t, os.WriteFile(manifestPath, []byte("not json"), 0644)) + + _, err := loadPolicyManifest(manifestPath) + assert.Error(t, err, "Should return error for invalid JSON") + }) +} + +func TestParseAuditJSONL(t *testing.T) { + t.Run("valid JSONL", func(t *testing.T) { + dir := t.TempDir() + jsonlPath := filepath.Join(dir, "audit.jsonl") + + lines := `{"ts":1761074374.646,"client":"172.30.0.20","host":"api.github.com:443","dest":"140.82.114.22:443","method":"CONNECT","status":200,"decision":"TCP_TUNNEL","url":"api.github.com:443"} +{"ts":1761074375.100,"client":"172.30.0.20","host":"evil.com:443","dest":"1.2.3.4:443","method":"CONNECT","status":403,"decision":"NONE_NONE","url":"evil.com:443"} +{"ts":1761074376.200,"client":"172.30.0.20","host":"registry.npmjs.org:443","dest":"104.16.0.1:443","method":"CONNECT","status":200,"decision":"TCP_TUNNEL","url":"registry.npmjs.org:443"} +` + require.NoError(t, os.WriteFile(jsonlPath, []byte(lines), 0644)) + + entries, err := parseAuditJSONL(jsonlPath) + require.NoError(t, err, "Should parse JSONL without error") + assert.Len(t, entries, 3, "Should parse 3 entries") + + // Check first entry + assert.InDelta(t, 1761074374.646, entries[0].Timestamp, 0.001, "Timestamp should match") + assert.Equal(t, "api.github.com:443", entries[0].Host, "Host should match") + assert.Equal(t, 200, entries[0].Status, "Status should match") + + // Check denied entry + assert.Equal(t, "evil.com:443", entries[1].Host, "Second host should match") + assert.Equal(t, 403, entries[1].Status, "Second status should be 403") + }) + + t.Run("empty lines skipped", func(t *testing.T) { + dir := t.TempDir() + jsonlPath := filepath.Join(dir, "audit.jsonl") + + lines := `{"ts":1.0,"host":"a.com:443","status":200} + +{"ts":2.0,"host":"b.com:443","status":200} +` + require.NoError(t, os.WriteFile(jsonlPath, []byte(lines), 0644)) + + entries, err := parseAuditJSONL(jsonlPath) + require.NoError(t, err, "Should parse JSONL without error") + assert.Len(t, entries, 2, "Should parse 2 entries, skipping empty line") + }) + + t.Run("malformed lines skipped", func(t *testing.T) { + dir := t.TempDir() + jsonlPath := filepath.Join(dir, "audit.jsonl") + + lines := `{"ts":1.0,"host":"valid.com:443","status":200} +not valid json +{"ts":2.0,"host":"also-valid.com:443","status":200} +` + require.NoError(t, os.WriteFile(jsonlPath, []byte(lines), 0644)) + + entries, err := parseAuditJSONL(jsonlPath) + require.NoError(t, err, "Should parse JSONL without error despite malformed line") + assert.Len(t, entries, 2, "Should parse 2 valid entries") + }) + + t.Run("missing file", func(t *testing.T) { + _, err := parseAuditJSONL("/nonexistent/path/audit.jsonl") + assert.Error(t, err, "Should return error for missing file") + }) +} + +func TestEnrichWithPolicyRules(t *testing.T) { + manifest := &PolicyManifest{ + Version: 1, + GeneratedAt: "2026-01-01T00:00:00Z", + Rules: []PolicyRule{ + { + ID: "allow-github", + Order: 1, + Action: "allow", + ACLName: "allowed_domains", + Domains: []string{".github.com"}, + Description: "Allow GitHub and subdomains", + }, + { + ID: "allow-npm", + Order: 2, + Action: "allow", + ACLName: "npm_domains", + Domains: []string{"registry.npmjs.org"}, + Description: "Allow npm registry", + }, + { + ID: "deny-all", + Order: 3, + Action: "deny", + ACLName: "blocked_all", + Domains: []string{"."}, + Description: "Deny all other traffic", + }, + }, + SSLBumpEnabled: false, + DLPEnabled: false, + } + + entries := []AuditLogEntry{ + {Timestamp: 1761074374.646, Host: "api.github.com:443", Status: 200}, + {Timestamp: 1761074375.100, Host: "github.com:443", Status: 200}, + {Timestamp: 1761074376.200, Host: "registry.npmjs.org:443", Status: 200}, + {Timestamp: 1761074377.300, Host: "evil.com:443", Status: 403}, + } + + analysis := enrichWithPolicyRules(entries, manifest) + require.NotNil(t, analysis, "Analysis should not be nil") + + t.Run("summary statistics", func(t *testing.T) { + assert.Equal(t, 4, analysis.TotalRequests, "Should have 4 total requests") + assert.Equal(t, 3, analysis.AllowedCount, "Should have 3 allowed requests") + assert.Equal(t, 1, analysis.DeniedCount, "Should have 1 denied request") + assert.Equal(t, 4, analysis.UniqueDomains, "Should have 4 unique domains") + }) + + t.Run("policy summary", func(t *testing.T) { + assert.Contains(t, analysis.PolicySummary, "3 rules", "Policy summary should mention 3 rules") + assert.Contains(t, analysis.PolicySummary, "SSL Bump disabled", "Should mention SSL Bump status") + assert.Contains(t, analysis.PolicySummary, "DLP disabled", "Should mention DLP status") + }) + + t.Run("rule hits", func(t *testing.T) { + require.Len(t, analysis.RuleHits, 3, "Should have 3 rule hit entries") + // Rules should be in order + assert.Equal(t, "allow-github", analysis.RuleHits[0].Rule.ID, "First rule should be allow-github") + assert.Equal(t, 2, analysis.RuleHits[0].Hits, "allow-github should have 2 hits") + assert.Equal(t, "allow-npm", analysis.RuleHits[1].Rule.ID, "Second rule should be allow-npm") + assert.Equal(t, 1, analysis.RuleHits[1].Hits, "allow-npm should have 1 hit") + }) + + t.Run("denied requests", func(t *testing.T) { + // evil.com does not match any specific rule's domains; it gets implicit deny + require.Len(t, analysis.DeniedRequests, 1, "Should have 1 denied request") + assert.Equal(t, "evil.com:443", analysis.DeniedRequests[0].Host, "Denied request host should match") + assert.Equal(t, "deny", analysis.DeniedRequests[0].Action, "Action should be deny") + }) + + t.Run("entries with empty host skipped", func(t *testing.T) { + emptyEntries := []AuditLogEntry{ + {Timestamp: 1.0, Host: "", Status: 200}, + {Timestamp: 2.0, Host: "-", Status: 200}, + {Timestamp: 3.0, Host: "valid.com:443", Status: 200}, + } + result := enrichWithPolicyRules(emptyEntries, manifest) + // valid.com doesn't match any rule, gets implicit deny + assert.Equal(t, 1, result.TotalRequests, "Only valid entries should be counted") + }) +} + +func TestDetectFirewallAuditArtifacts(t *testing.T) { + t.Run("sandbox/firewall/audit path", func(t *testing.T) { + dir := t.TempDir() + auditDir := filepath.Join(dir, "sandbox", "firewall", "audit") + require.NoError(t, os.MkdirAll(auditDir, 0755)) + + manifestPath := filepath.Join(auditDir, "policy-manifest.json") + auditPath := filepath.Join(auditDir, "audit.jsonl") + require.NoError(t, os.WriteFile(manifestPath, []byte("{}"), 0644)) + require.NoError(t, os.WriteFile(auditPath, []byte(""), 0644)) + + foundManifest, foundAudit := detectFirewallAuditArtifacts(dir) + assert.Equal(t, manifestPath, foundManifest, "Should find policy manifest") + assert.Equal(t, auditPath, foundAudit, "Should find audit JSONL") + }) + + t.Run("firewall-audit-logs directory", func(t *testing.T) { + dir := t.TempDir() + auditDir := filepath.Join(dir, "firewall-audit-logs") + require.NoError(t, os.MkdirAll(auditDir, 0755)) + + manifestPath := filepath.Join(auditDir, "policy-manifest.json") + auditPath := filepath.Join(auditDir, "audit.jsonl") + require.NoError(t, os.WriteFile(manifestPath, []byte("{}"), 0644)) + require.NoError(t, os.WriteFile(auditPath, []byte(""), 0644)) + + foundManifest, foundAudit := detectFirewallAuditArtifacts(dir) + assert.Equal(t, manifestPath, foundManifest, "Should find policy manifest in firewall-audit-logs") + assert.Equal(t, auditPath, foundAudit, "Should find audit JSONL in firewall-audit-logs") + }) + + t.Run("no artifacts", func(t *testing.T) { + dir := t.TempDir() + foundManifest, foundAudit := detectFirewallAuditArtifacts(dir) + assert.Empty(t, foundManifest, "Should not find manifest") + assert.Empty(t, foundAudit, "Should not find audit JSONL") + }) +} + +func TestAnalyzeFirewallPolicy(t *testing.T) { + t.Run("full enrichment", func(t *testing.T) { + dir := t.TempDir() + auditDir := filepath.Join(dir, "sandbox", "firewall", "audit") + require.NoError(t, os.MkdirAll(auditDir, 0755)) + + // Write policy manifest + manifest := PolicyManifest{ + Version: 1, + GeneratedAt: "2026-01-01T00:00:00Z", + Rules: []PolicyRule{ + {ID: "allow-github", Order: 1, Action: "allow", Domains: []string{".github.com"}, Description: "Allow GitHub"}, + {ID: "deny-blocked", Order: 2, Action: "deny", Domains: []string{".evil.com"}, Description: "Block evil domains"}, + }, + } + manifestData, err := json.Marshal(manifest) + require.NoError(t, err) + require.NoError(t, os.WriteFile(filepath.Join(auditDir, "policy-manifest.json"), manifestData, 0644)) + + // Write audit JSONL + jsonl := `{"ts":1.0,"host":"api.github.com:443","status":200} +{"ts":2.0,"host":"evil.com:443","status":403} +` + require.NoError(t, os.WriteFile(filepath.Join(auditDir, "audit.jsonl"), []byte(jsonl), 0644)) + + analysis, err := analyzeFirewallPolicy(dir, false) + require.NoError(t, err, "Should analyze without error") + require.NotNil(t, analysis, "Analysis should not be nil") + + assert.Equal(t, 2, analysis.TotalRequests, "Should have 2 total requests") + assert.Equal(t, 1, analysis.AllowedCount, "Should have 1 allowed request") + assert.Equal(t, 1, analysis.DeniedCount, "Should have 1 denied request") + assert.Len(t, analysis.DeniedRequests, 1, "Should have 1 denied request detail") + }) + + t.Run("manifest only - no audit.jsonl", func(t *testing.T) { + dir := t.TempDir() + auditDir := filepath.Join(dir, "sandbox", "firewall", "audit") + require.NoError(t, os.MkdirAll(auditDir, 0755)) + + manifest := PolicyManifest{ + Version: 1, + Rules: []PolicyRule{{ID: "r1", Order: 1, Action: "allow", Domains: []string{".example.com"}}}, + SSLBumpEnabled: true, + } + manifestData, err := json.Marshal(manifest) + require.NoError(t, err) + require.NoError(t, os.WriteFile(filepath.Join(auditDir, "policy-manifest.json"), manifestData, 0644)) + + analysis, err := analyzeFirewallPolicy(dir, false) + require.NoError(t, err, "Should not error with manifest only") + require.NotNil(t, analysis, "Should return analysis with manifest-only data") + assert.Contains(t, analysis.PolicySummary, "SSL Bump enabled", "Should reflect SSL Bump enabled") + }) + + t.Run("no artifacts returns nil", func(t *testing.T) { + dir := t.TempDir() + analysis, err := analyzeFirewallPolicy(dir, false) + require.NoError(t, err, "Should not error when no artifacts found") + assert.Nil(t, analysis, "Should return nil when no artifacts found") + }) +} + +func TestFormatUnixTimestamp(t *testing.T) { + tests := []struct { + name string + ts float64 + expected string + }{ + { + name: "valid timestamp", + ts: 1761074374.646, + expected: "19:19:34", + }, + { + name: "zero timestamp", + ts: 0, + expected: "-", + }, + { + name: "negative timestamp", + ts: -1.0, + expected: "-", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := formatUnixTimestamp(tt.ts) + assert.Equal(t, tt.expected, result, "formatUnixTimestamp(%v)", tt.ts) + }) + } +} diff --git a/pkg/cli/logs_models.go b/pkg/cli/logs_models.go index 912da697645..16fae3eeb43 100644 --- a/pkg/cli/logs_models.go +++ b/pkg/cli/logs_models.go @@ -71,6 +71,7 @@ type ProcessedRun struct { Run WorkflowRun AccessAnalysis *DomainAnalysis FirewallAnalysis *FirewallAnalysis + PolicyAnalysis *PolicyAnalysis RedactedDomainsAnalysis *RedactedDomainsAnalysis MissingTools []MissingToolReport MissingData []MissingDataReport @@ -181,6 +182,7 @@ type RunSummary struct { Metrics LogMetrics `json:"metrics"` // Extracted log metrics AccessAnalysis *DomainAnalysis `json:"access_analysis"` // Network access analysis FirewallAnalysis *FirewallAnalysis `json:"firewall_analysis"` // Firewall log analysis + PolicyAnalysis *PolicyAnalysis `json:"policy_analysis,omitempty"` // Firewall policy rule attribution RedactedDomainsAnalysis *RedactedDomainsAnalysis `json:"redacted_domains_analysis"` // Redacted URL domains analysis MissingTools []MissingToolReport `json:"missing_tools"` // Missing tool reports MissingData []MissingDataReport `json:"missing_data"` // Missing data reports From 2101eba08fe1c94914a818c9043543252547eaae Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 24 Mar 2026 21:19:00 +0000 Subject: [PATCH 3/8] fix: render policy analysis section when only manifest is present Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw/sessions/36ac2fc6-303e-452b-b36f-cadde5ac87f8 --- pkg/cli/audit_report_render.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cli/audit_report_render.go b/pkg/cli/audit_report_render.go index 6e4b0a2cc15..3d0a0799979 100644 --- a/pkg/cli/audit_report_render.go +++ b/pkg/cli/audit_report_render.go @@ -123,7 +123,7 @@ func renderConsole(data AuditData, logsPath string) { } // Firewall Policy Analysis Section (enriched with rule attribution) - if data.PolicyAnalysis != nil && len(data.PolicyAnalysis.RuleHits) > 0 { + if data.PolicyAnalysis != nil && (len(data.PolicyAnalysis.RuleHits) > 0 || data.PolicyAnalysis.PolicySummary != "") { fmt.Fprintln(os.Stderr, console.FormatSectionHeader("Firewall Policy Analysis")) fmt.Fprintln(os.Stderr) renderPolicyAnalysis(data.PolicyAnalysis) From 89e65a601a626fd6083c04f267523bef5483cc36 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 24 Mar 2026 21:41:41 +0000 Subject: [PATCH 4/8] fix: address code review - aclName all, protocol matching, decision validation, regex, missing fields, squid filter - findMatchingRule now takes full AuditLogEntry instead of just host string - Add aclName "all" catch-all handling for default deny rules - Add protocol matching (CONNECT = HTTPS, else HTTP) - Add observed-decision validation (rule action must match observed outcome) - Add regex domain matching via aclName "regex" detection or metachar detection - Add missing PolicyManifest fields (hostAccessEnabled, allowHostPorts) - Filter error:transaction-end-before-headers Squid operational entries - Add new tests: TestProtocolMatching, TestIsEntryHTTPS, TestIsEntryAllowed, TestContainsRegexMeta, hostAccessEnabled manifest test, squid filter test - Update existing tests to use full AuditLogEntry with method/status/decision Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw/sessions/8a6f8f24-239c-4326-b935-1553bf8d7487 --- pkg/cli/firewall_policy.go | 148 ++++++++++++++++++-- pkg/cli/firewall_policy_test.go | 239 +++++++++++++++++++++++++++----- 2 files changed, 338 insertions(+), 49 deletions(-) diff --git a/pkg/cli/firewall_policy.go b/pkg/cli/firewall_policy.go index a0c865f3150..9aef692a1c1 100644 --- a/pkg/cli/firewall_policy.go +++ b/pkg/cli/firewall_policy.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "path/filepath" + "regexp" "sort" "strings" @@ -17,13 +18,15 @@ var firewallPolicyLog = logger.New("cli:firewall_policy") // PolicyManifest represents the policy-manifest.json file generated by AWF. // It describes the firewall policy rules and configuration used during a workflow run. type PolicyManifest struct { - Version int `json:"version"` - GeneratedAt string `json:"generatedAt"` - Rules []PolicyRule `json:"rules"` - DangerousPorts []int `json:"dangerousPorts,omitempty"` - DNSServers []string `json:"dnsServers,omitempty"` - SSLBumpEnabled bool `json:"sslBumpEnabled"` - DLPEnabled bool `json:"dlpEnabled"` + Version int `json:"version"` + GeneratedAt string `json:"generatedAt"` + Rules []PolicyRule `json:"rules"` + DangerousPorts []int `json:"dangerousPorts,omitempty"` + DNSServers []string `json:"dnsServers,omitempty"` + SSLBumpEnabled bool `json:"sslBumpEnabled"` + DLPEnabled bool `json:"dlpEnabled"` + HostAccessEnabled bool `json:"hostAccessEnabled"` + AllowHostPorts *string `json:"allowHostPorts"` // nullable → pointer } // PolicyRule represents a single firewall policy rule from the manifest. @@ -145,6 +148,8 @@ func parseAuditJSONL(jsonlPath string) ([]AuditLogEntry, error) { // Domain matching follows AWF conventions: // - ".github.com" matches both "github.com" and "*.github.com" (e.g., "api.github.com") // - "github.com" matches only exactly "github.com" +// - Regex patterns (detected via aclName containing "regex" or regex metacharacters) are +// compiled and matched against the domain. func domainMatchesRule(host string, rule PolicyRule) bool { // Strip port from host if present domain := host @@ -153,6 +158,21 @@ func domainMatchesRule(host string, rule PolicyRule) bool { } domain = strings.ToLower(domain) + // Detect regex-based rules: aclName contains "regex" or domains contain regex metacharacters + isRegex := strings.Contains(strings.ToLower(rule.ACLName), "regex") + if !isRegex { + for _, d := range rule.Domains { + if containsRegexMeta(d) { + isRegex = true + break + } + } + } + + if isRegex { + return domainMatchesRegex(domain, rule.Domains) + } + for _, pattern := range rule.Domains { pattern = strings.ToLower(pattern) @@ -172,12 +192,111 @@ func domainMatchesRule(host string, rule PolicyRule) bool { return false } -// findMatchingRule finds the first matching policy rule for a given host. +// containsRegexMeta checks if a string contains regex metacharacters. +func containsRegexMeta(s string) bool { + for _, c := range s { + switch c { + case '^', '$', '*', '+', '?', '(', ')', '[', ']', '{', '}', '|', '\\': + return true + } + } + return false +} + +// domainMatchesRegex checks if a domain matches any regex pattern in the list. +func domainMatchesRegex(domain string, patterns []string) bool { + for _, pattern := range patterns { + re, err := regexp.Compile(pattern) + if err != nil { + firewallPolicyLog.Printf("Invalid regex pattern %q: %v", pattern, err) + continue + } + if re.MatchString(domain) { + return true + } + } + return false +} + +// isEntryHTTPS determines if an audit log entry represents an HTTPS request. +// CONNECT method is the standard signal for HTTPS tunneling. +func isEntryHTTPS(entry AuditLogEntry) bool { + return strings.EqualFold(entry.Method, "CONNECT") +} + +// isEntryAllowed determines the observed outcome of a request. +// This mirrors the classification logic used by the firewall log parser. +func isEntryAllowed(entry AuditLogEntry) bool { + status := entry.Status + if status == 200 || status == 206 || status == 304 { + return true + } + if status == 403 || status == 407 { + return false + } + decision := entry.Decision + if strings.Contains(decision, "TCP_TUNNEL") || + strings.Contains(decision, "TCP_HIT") || + strings.Contains(decision, "TCP_MISS") { + return true + } + if strings.Contains(decision, "NONE_NONE") || + strings.Contains(decision, "TCP_DENIED") { + return false + } + return false +} + +// protocolMatches checks if a rule's protocol constraint matches the request. +// "both" matches everything; "https" matches only HTTPS; "http" matches only HTTP. +func protocolMatches(rule PolicyRule, isHTTPS bool) bool { + switch strings.ToLower(rule.Protocol) { + case "https": + return isHTTPS + case "http": + return !isHTTPS + default: + // "both" or empty — matches everything + return true + } +} + +// findMatchingRule finds the first matching policy rule for a given audit entry. // Rules are evaluated in order; the first matching rule wins. -func findMatchingRule(host string, rules []PolicyRule) *PolicyRule { +// +// This ports the TS audit-enricher.ts logic: +// 1. aclName === "all" rules match any domain (used for default deny rules) +// 2. Protocol matching: HTTPS-only rules won't match HTTP requests, and vice versa +// 3. Observed-decision validation: a rule only "matches" if its action matches the +// observed outcome (allow rule only credited for allowed traffic, deny for denied) +func findMatchingRule(entry AuditLogEntry, rules []PolicyRule) *PolicyRule { + isHTTPS := isEntryHTTPS(entry) + expectedAction := "deny" + if isEntryAllowed(entry) { + expectedAction = "allow" + } + for i := range rules { - if domainMatchesRule(host, rules[i]) { - return &rules[i] + rule := &rules[i] + + // Protocol check + if !protocolMatches(*rule, isHTTPS) { + continue + } + + // aclName "all" is a catch-all rule (typically the default deny) + if rule.ACLName == "all" { + if rule.Action == expectedAction { + return rule + } + continue + } + + // Domain match + if domainMatchesRule(entry.Host, *rule) { + if rule.Action == expectedAction { + return rule + } } } return nil @@ -199,6 +318,11 @@ func enrichWithPolicyRules(entries []AuditLogEntry, manifest *PolicyManifest) *P continue } + // Skip benign Squid operational entries (matches TS computeRuleStats filter) + if entry.URL == "error:transaction-end-before-headers" { + continue + } + // Strip port for domain tracking domain := host if idx := strings.LastIndex(host, ":"); idx != -1 { @@ -206,7 +330,7 @@ func enrichWithPolicyRules(entries []AuditLogEntry, manifest *PolicyManifest) *P } uniqueDomains[domain] = true - rule := findMatchingRule(host, manifest.Rules) + rule := findMatchingRule(entry, manifest.Rules) var enriched EnrichedRequest enriched.Timestamp = entry.Timestamp diff --git a/pkg/cli/firewall_policy_test.go b/pkg/cli/firewall_policy_test.go index 7e4fecf108e..e4eb5431e59 100644 --- a/pkg/cli/firewall_policy_test.go +++ b/pkg/cli/firewall_policy_test.go @@ -99,6 +99,33 @@ func TestDomainMatchesRule(t *testing.T) { }, expected: false, }, + { + name: "regex match - IP pattern", + host: "192.168.1.1", + rule: PolicyRule{ + ACLName: "dst_ipv4_regex", + Domains: []string{`^192\.168\.`}, + }, + expected: true, + }, + { + name: "regex match - metachar detection", + host: "test.example.com", + rule: PolicyRule{ + ACLName: "some_acl", + Domains: []string{`^.*\.example\.com$`}, + }, + expected: true, + }, + { + name: "regex no match", + host: "other.com", + rule: PolicyRule{ + ACLName: "dst_ipv4_regex", + Domains: []string{`^192\.168\.`}, + }, + expected: false, + }, } for _, tt := range tests { @@ -112,50 +139,148 @@ func TestDomainMatchesRule(t *testing.T) { func TestFindMatchingRule(t *testing.T) { rules := []PolicyRule{ { - ID: "allow-github", - Order: 1, - Action: "allow", - Domains: []string{".github.com"}, + ID: "allow-github", + Order: 1, + Action: "allow", + ACLName: "allowed_domains", + Protocol: "both", + Domains: []string{".github.com"}, }, { - ID: "allow-npm", - Order: 2, - Action: "allow", - Domains: []string{"registry.npmjs.org"}, + ID: "allow-npm", + Order: 2, + Action: "allow", + ACLName: "npm_domains", + Protocol: "both", + Domains: []string{"registry.npmjs.org"}, }, { - ID: "deny-all", - Order: 3, - Action: "deny", - Domains: []string{"."}, + ID: "deny-all", + Order: 3, + Action: "deny", + ACLName: "all", + Protocol: "both", + Domains: []string{}, }, } - t.Run("matches first rule", func(t *testing.T) { - rule := findMatchingRule("api.github.com:443", rules) + t.Run("matches first rule - allowed HTTPS", func(t *testing.T) { + entry := AuditLogEntry{Host: "api.github.com:443", Method: "CONNECT", Status: 200, Decision: "TCP_TUNNEL"} + rule := findMatchingRule(entry, rules) require.NotNil(t, rule, "Should find a matching rule") assert.Equal(t, "allow-github", rule.ID, "Should match allow-github rule") }) t.Run("matches second rule", func(t *testing.T) { - rule := findMatchingRule("registry.npmjs.org:443", rules) + entry := AuditLogEntry{Host: "registry.npmjs.org:443", Method: "CONNECT", Status: 200, Decision: "TCP_TUNNEL"} + rule := findMatchingRule(entry, rules) require.NotNil(t, rule, "Should find a matching rule") assert.Equal(t, "allow-npm", rule.ID, "Should match allow-npm rule") }) - t.Run("no match returns nil", func(t *testing.T) { - // evil.com does not match any of the specific rules, and the deny-all - // rule has domain "." which won't match via our matching logic - rule := findMatchingRule("evil.com:443", rules[:2]) - assert.Nil(t, rule, "Should not find a matching rule for unlisted domain") + t.Run("aclName all catches unmatched denied traffic", func(t *testing.T) { + entry := AuditLogEntry{Host: "evil.com:443", Method: "CONNECT", Status: 403, Decision: "NONE_NONE"} + rule := findMatchingRule(entry, rules) + require.NotNil(t, rule, "Should find the catch-all deny rule") + assert.Equal(t, "deny-all", rule.ID, "Should match deny-all rule via aclName 'all'") + }) + + t.Run("aclName all skipped for allowed traffic", func(t *testing.T) { + // If a domain doesn't match specific rules but traffic was allowed, + // the deny-all rule should NOT match (action mismatch) + entry := AuditLogEntry{Host: "unknown.com:443", Method: "CONNECT", Status: 200, Decision: "TCP_TUNNEL"} + rule := findMatchingRule(entry, rules) + assert.Nil(t, rule, "deny-all rule should not match allowed traffic") }) t.Run("first matching rule wins", func(t *testing.T) { - // github.com matches both allow-github (.github.com) and potentially others - rule := findMatchingRule("github.com:443", rules) + entry := AuditLogEntry{Host: "github.com:443", Method: "CONNECT", Status: 200, Decision: "TCP_TUNNEL"} + rule := findMatchingRule(entry, rules) require.NotNil(t, rule, "Should find a matching rule") assert.Equal(t, "allow-github", rule.ID, "First matching rule should win") }) + + t.Run("observed-decision validation - allow rule skipped for denied traffic", func(t *testing.T) { + // Domain matches allow-github, but traffic was denied — allow rule shouldn't be credited + entry := AuditLogEntry{Host: "api.github.com:443", Method: "CONNECT", Status: 403, Decision: "NONE_NONE"} + rule := findMatchingRule(entry, rules) + // Falls through to deny-all since allow-github action doesn't match observed denial + require.NotNil(t, rule, "Should fall through to deny-all") + assert.Equal(t, "deny-all", rule.ID, "Should match deny-all, not allow-github") + }) +} + +func TestProtocolMatching(t *testing.T) { + rules := []PolicyRule{ + { + ID: "allow-https-only", + Order: 1, + Action: "allow", + ACLName: "https_domains", + Protocol: "https", + Domains: []string{".github.com"}, + }, + { + ID: "deny-all", + Order: 2, + Action: "deny", + ACLName: "all", + Protocol: "both", + Domains: []string{}, + }, + } + + t.Run("HTTPS rule matches CONNECT request", func(t *testing.T) { + entry := AuditLogEntry{Host: "api.github.com:443", Method: "CONNECT", Status: 200, Decision: "TCP_TUNNEL"} + rule := findMatchingRule(entry, rules) + require.NotNil(t, rule, "Should match HTTPS rule") + assert.Equal(t, "allow-https-only", rule.ID, "HTTPS rule should match CONNECT request") + }) + + t.Run("HTTPS rule skipped for HTTP request", func(t *testing.T) { + entry := AuditLogEntry{Host: "api.github.com:80", Method: "GET", Status: 403, Decision: "NONE_NONE"} + rule := findMatchingRule(entry, rules) + // HTTPS-only rule skipped for GET → falls through to deny-all + require.NotNil(t, rule, "Should fall through to deny-all") + assert.Equal(t, "deny-all", rule.ID, "HTTPS rule should not match HTTP GET request") + }) +} + +func TestIsEntryHTTPS(t *testing.T) { + assert.True(t, isEntryHTTPS(AuditLogEntry{Method: "CONNECT"}), "CONNECT should be HTTPS") + assert.True(t, isEntryHTTPS(AuditLogEntry{Method: "connect"}), "connect (lowercase) should be HTTPS") + assert.False(t, isEntryHTTPS(AuditLogEntry{Method: "GET"}), "GET should not be HTTPS") + assert.False(t, isEntryHTTPS(AuditLogEntry{Method: ""}), "Empty method should not be HTTPS") +} + +func TestIsEntryAllowed(t *testing.T) { + tests := []struct { + name string + entry AuditLogEntry + expected bool + }{ + {"status 200 is allowed", AuditLogEntry{Status: 200}, true}, + {"status 206 is allowed", AuditLogEntry{Status: 206}, true}, + {"status 304 is allowed", AuditLogEntry{Status: 304}, true}, + {"status 403 is denied", AuditLogEntry{Status: 403}, false}, + {"status 407 is denied", AuditLogEntry{Status: 407}, false}, + {"TCP_TUNNEL decision is allowed", AuditLogEntry{Status: 0, Decision: "TCP_TUNNEL:HIER_DIRECT"}, true}, + {"NONE_NONE decision is denied", AuditLogEntry{Status: 0, Decision: "NONE_NONE:HIER_NONE"}, false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, isEntryAllowed(tt.entry), "isEntryAllowed should return %v", tt.expected) + }) + } +} + +func TestContainsRegexMeta(t *testing.T) { + assert.True(t, containsRegexMeta(`^192\.168`), "Should detect caret") + assert.True(t, containsRegexMeta(`foo.*bar`), "Should detect asterisk") + assert.True(t, containsRegexMeta(`[0-9]+`), "Should detect brackets") + assert.False(t, containsRegexMeta("github.com"), "Plain domain should not be regex") + assert.False(t, containsRegexMeta(".github.com"), "Dot-prefix domain should not be regex") } func TestLoadPolicyManifest(t *testing.T) { @@ -189,6 +314,29 @@ func TestLoadPolicyManifest(t *testing.T) { assert.Equal(t, "rule-b", loaded.Rules[1].ID, "Second rule should be rule-b (order 2)") }) + t.Run("manifest with hostAccessEnabled and allowHostPorts", func(t *testing.T) { + dir := t.TempDir() + manifestPath := filepath.Join(dir, "policy-manifest.json") + + ports := "8080,9090" + manifest := PolicyManifest{ + Version: 1, + Rules: []PolicyRule{}, + HostAccessEnabled: true, + AllowHostPorts: &ports, + } + + data, err := json.Marshal(manifest) + require.NoError(t, err, "Should marshal manifest with extra fields") + require.NoError(t, os.WriteFile(manifestPath, data, 0644)) + + loaded, err := loadPolicyManifest(manifestPath) + require.NoError(t, err, "Should load manifest") + assert.True(t, loaded.HostAccessEnabled, "HostAccessEnabled should be true") + require.NotNil(t, loaded.AllowHostPorts, "AllowHostPorts should not be nil") + assert.Equal(t, "8080,9090", *loaded.AllowHostPorts, "AllowHostPorts should match") + }) + t.Run("missing file", func(t *testing.T) { _, err := loadPolicyManifest("/nonexistent/path/policy-manifest.json") assert.Error(t, err, "Should return error for missing file") @@ -275,6 +423,7 @@ func TestEnrichWithPolicyRules(t *testing.T) { Order: 1, Action: "allow", ACLName: "allowed_domains", + Protocol: "both", Domains: []string{".github.com"}, Description: "Allow GitHub and subdomains", }, @@ -283,6 +432,7 @@ func TestEnrichWithPolicyRules(t *testing.T) { Order: 2, Action: "allow", ACLName: "npm_domains", + Protocol: "both", Domains: []string{"registry.npmjs.org"}, Description: "Allow npm registry", }, @@ -290,8 +440,9 @@ func TestEnrichWithPolicyRules(t *testing.T) { ID: "deny-all", Order: 3, Action: "deny", - ACLName: "blocked_all", - Domains: []string{"."}, + ACLName: "all", + Protocol: "both", + Domains: []string{}, Description: "Deny all other traffic", }, }, @@ -300,10 +451,10 @@ func TestEnrichWithPolicyRules(t *testing.T) { } entries := []AuditLogEntry{ - {Timestamp: 1761074374.646, Host: "api.github.com:443", Status: 200}, - {Timestamp: 1761074375.100, Host: "github.com:443", Status: 200}, - {Timestamp: 1761074376.200, Host: "registry.npmjs.org:443", Status: 200}, - {Timestamp: 1761074377.300, Host: "evil.com:443", Status: 403}, + {Timestamp: 1761074374.646, Host: "api.github.com:443", Method: "CONNECT", Status: 200, Decision: "TCP_TUNNEL"}, + {Timestamp: 1761074375.100, Host: "github.com:443", Method: "CONNECT", Status: 200, Decision: "TCP_TUNNEL"}, + {Timestamp: 1761074376.200, Host: "registry.npmjs.org:443", Method: "CONNECT", Status: 200, Decision: "TCP_TUNNEL"}, + {Timestamp: 1761074377.300, Host: "evil.com:443", Method: "CONNECT", Status: 403, Decision: "NONE_NONE"}, } analysis := enrichWithPolicyRules(entries, manifest) @@ -329,24 +480,37 @@ func TestEnrichWithPolicyRules(t *testing.T) { assert.Equal(t, 2, analysis.RuleHits[0].Hits, "allow-github should have 2 hits") assert.Equal(t, "allow-npm", analysis.RuleHits[1].Rule.ID, "Second rule should be allow-npm") assert.Equal(t, 1, analysis.RuleHits[1].Hits, "allow-npm should have 1 hit") + assert.Equal(t, "deny-all", analysis.RuleHits[2].Rule.ID, "Third rule should be deny-all") + assert.Equal(t, 1, analysis.RuleHits[2].Hits, "deny-all should have 1 hit (evil.com)") }) - t.Run("denied requests", func(t *testing.T) { - // evil.com does not match any specific rule's domains; it gets implicit deny + t.Run("denied requests attributed to deny-all rule", func(t *testing.T) { require.Len(t, analysis.DeniedRequests, 1, "Should have 1 denied request") assert.Equal(t, "evil.com:443", analysis.DeniedRequests[0].Host, "Denied request host should match") assert.Equal(t, "deny", analysis.DeniedRequests[0].Action, "Action should be deny") + assert.Equal(t, "deny-all", analysis.DeniedRequests[0].RuleID, "Should be attributed to deny-all rule") }) t.Run("entries with empty host skipped", func(t *testing.T) { emptyEntries := []AuditLogEntry{ {Timestamp: 1.0, Host: "", Status: 200}, {Timestamp: 2.0, Host: "-", Status: 200}, - {Timestamp: 3.0, Host: "valid.com:443", Status: 200}, + {Timestamp: 3.0, Host: "valid.com:443", Method: "CONNECT", Status: 403, Decision: "NONE_NONE"}, } result := enrichWithPolicyRules(emptyEntries, manifest) - // valid.com doesn't match any rule, gets implicit deny + // valid.com is denied → matches deny-all via aclName "all" assert.Equal(t, 1, result.TotalRequests, "Only valid entries should be counted") + require.Len(t, result.DeniedRequests, 1, "Should have 1 denied request") + assert.Equal(t, "deny-all", result.DeniedRequests[0].RuleID, "Should match deny-all rule") + }) + + t.Run("error:transaction-end-before-headers entries filtered", func(t *testing.T) { + squidEntries := []AuditLogEntry{ + {Timestamp: 1.0, Host: "api.github.com:443", Method: "CONNECT", Status: 200, Decision: "TCP_TUNNEL", URL: "api.github.com:443"}, + {Timestamp: 2.0, Host: "api.github.com:443", Method: "CONNECT", Status: 0, Decision: "NONE_NONE", URL: "error:transaction-end-before-headers"}, + } + result := enrichWithPolicyRules(squidEntries, manifest) + assert.Equal(t, 1, result.TotalRequests, "Squid error entries should be filtered out") }) } @@ -400,8 +564,8 @@ func TestAnalyzeFirewallPolicy(t *testing.T) { Version: 1, GeneratedAt: "2026-01-01T00:00:00Z", Rules: []PolicyRule{ - {ID: "allow-github", Order: 1, Action: "allow", Domains: []string{".github.com"}, Description: "Allow GitHub"}, - {ID: "deny-blocked", Order: 2, Action: "deny", Domains: []string{".evil.com"}, Description: "Block evil domains"}, + {ID: "allow-github", Order: 1, Action: "allow", ACLName: "allowed_domains", Protocol: "both", Domains: []string{".github.com"}, Description: "Allow GitHub"}, + {ID: "deny-all", Order: 2, Action: "deny", ACLName: "all", Protocol: "both", Domains: []string{}, Description: "Block all other traffic"}, }, } manifestData, err := json.Marshal(manifest) @@ -409,8 +573,8 @@ func TestAnalyzeFirewallPolicy(t *testing.T) { require.NoError(t, os.WriteFile(filepath.Join(auditDir, "policy-manifest.json"), manifestData, 0644)) // Write audit JSONL - jsonl := `{"ts":1.0,"host":"api.github.com:443","status":200} -{"ts":2.0,"host":"evil.com:443","status":403} + jsonl := `{"ts":1.0,"host":"api.github.com:443","method":"CONNECT","status":200,"decision":"TCP_TUNNEL"} +{"ts":2.0,"host":"evil.com:443","method":"CONNECT","status":403,"decision":"NONE_NONE"} ` require.NoError(t, os.WriteFile(filepath.Join(auditDir, "audit.jsonl"), []byte(jsonl), 0644)) @@ -421,7 +585,8 @@ func TestAnalyzeFirewallPolicy(t *testing.T) { assert.Equal(t, 2, analysis.TotalRequests, "Should have 2 total requests") assert.Equal(t, 1, analysis.AllowedCount, "Should have 1 allowed request") assert.Equal(t, 1, analysis.DeniedCount, "Should have 1 denied request") - assert.Len(t, analysis.DeniedRequests, 1, "Should have 1 denied request detail") + require.Len(t, analysis.DeniedRequests, 1, "Should have 1 denied request detail") + assert.Equal(t, "deny-all", analysis.DeniedRequests[0].RuleID, "Denied request should be attributed to deny-all") }) t.Run("manifest only - no audit.jsonl", func(t *testing.T) { From 5522ac7d417bcaa423a422aac4b2111d7159676a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 24 Mar 2026 22:14:36 +0000 Subject: [PATCH 5/8] feat: update daily firewall report workflow to leverage policy rule attribution data Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw/sessions/d0283a81-24a3-4d01-bf28-71ac57201e36 --- .github/workflows/daily-firewall-report.md | 114 ++++++++++++++++++++- 1 file changed, 110 insertions(+), 4 deletions(-) diff --git a/.github/workflows/daily-firewall-report.md b/.github/workflows/daily-firewall-report.md index cb6d5d9f5c4..612e3a31b04 100644 --- a/.github/workflows/daily-firewall-report.md +++ b/.github/workflows/daily-firewall-report.md @@ -235,6 +235,7 @@ The audit tool returns structured firewall analysis data including: - Total requests, allowed requests, blocked requests - Lists of allowed and blocked domains - Request statistics per domain +- **Policy rule attribution** (when `policy-manifest.json` and `audit.jsonl` artifacts are present) **Example of extracting firewall data from audit result:** ```javascript @@ -243,13 +244,24 @@ result.firewall_analysis.blocked_domains // Array of blocked domain names result.firewall_analysis.allowed_domains // Array of allowed domain names result.firewall_analysis.total_requests // Total number of network requests result.firewall_analysis.blocked_requests // Number of blocked requests + +// Policy rule attribution (enriched data — may be null if artifacts are absent): +result.policy_analysis.policy_summary // e.g., "12 rules, SSL Bump disabled, DLP disabled" +result.policy_analysis.rule_hits // Array of {rule: {id, action, description, ...}, hits: N} +result.policy_analysis.denied_requests // Array of {ts, host, status, rule_id, action, reason} +result.policy_analysis.total_requests // Total enriched request count +result.policy_analysis.allowed_count // Allowed requests (rule-attributed) +result.policy_analysis.denied_count // Denied requests (rule-attributed) +result.policy_analysis.unique_domains // Unique domain count ``` **Important:** Do NOT manually download and parse firewall log files. Always use the `audit` tool which provides structured firewall analysis data. ### Step 3: Parse and Analyze Firewall Logs -Use the JSON output from the `audit` tool to extract firewall information. The `firewall_analysis` field in the audit JSON contains: +Use the JSON output from the `audit` tool to extract firewall information. + +**Basic firewall analysis** — The `firewall_analysis` field in the audit JSON contains: - `total_requests` - Total number of network requests - `allowed_requests` - Count of allowed requests - `blocked_requests` - Count of blocked requests @@ -257,6 +269,17 @@ Use the JSON output from the `audit` tool to extract firewall information. The ` - `blocked_domains` - Array of unique blocked domains - `requests_by_domain` - Object mapping domains to request statistics (allowed/blocked counts) +**Policy rule attribution** — The `policy_analysis` field (when present) contains enriched data that attributes each request to a specific firewall policy rule: +- `policy_summary` - Human-readable summary (e.g., "12 rules, SSL Bump disabled, DLP disabled") +- `rule_hits` - Array of objects: `{rule: {id, order, action, aclName, protocol, domains, description}, hits: N}` — how many requests each rule handled +- `denied_requests` - Array of objects: `{ts, host, status, rule_id, action, reason}` — every denied request with its matching rule and reason +- `total_requests` - Total enriched request count +- `allowed_count` - Allowed requests count (rule-attributed) +- `denied_count` - Denied requests count (rule-attributed) +- `unique_domains` - Unique domain count + +**Note:** `policy_analysis` is only present when the workflow run produced `policy-manifest.json` and `audit.jsonl` artifacts. If absent, fall back to `firewall_analysis` for basic domain-count data. + **Example jq filter for aggregating blocked domains:** ```bash # Get only blocked domains across multiple runs @@ -269,6 +292,19 @@ gh aw audit --json | jq -r ' select(.value.blocked > 0) | "\(.key): \(.value.blocked) blocked, \(.value.allowed) allowed" ' + +# Get policy rule hit counts (when policy_analysis is available) +gh aw audit --json | jq -r ' + .policy_analysis.rule_hits // [] | + .[] | select(.hits > 0) | + "\(.rule.id) (\(.rule.action)): \(.hits) hits" +' + +# Get denied requests with rule attribution +gh aw audit --json | jq -r ' + .policy_analysis.denied_requests // [] | + .[] | "\(.host) → \(.rule_id): \(.reason)" +' ``` For each workflow run with firewall data (see standardized metric names in scratchpad/metrics-glossary.md): @@ -279,6 +315,10 @@ For each workflow run with firewall data (see standardized metric names in scrat - Blocked requests count (`firewall_requests_blocked`) - List of unique blocked domains (`firewall_domains_blocked`) - Domain-level statistics (from `requests_by_domain`) +3. If `policy_analysis` is present, also track: + - Policy rule hit counts (which rules are handling traffic) + - Denied requests with rule attribution (which rule denied each request and why) + - Policy summary (rules count, SSL Bump/DLP status) ### Step 4: Aggregate Results @@ -292,6 +332,18 @@ Combine data from all workflows (using standardized metric names): - Total blocked domains (`firewall_domains_blocked`) - unique count - Total blocked requests (`firewall_requests_blocked`) +**Policy rule attribution aggregation** (when `policy_analysis` data is available): +5. Aggregate policy rule hit counts across all runs: + - Build a cross-run rule hit table: rule ID → total hits across all runs + - Identify the most active allow rules and deny rules +6. Aggregate denied requests with rule attribution: + - Collect all denied requests across runs with their matching rule and reason + - Group by rule ID to show which deny rules are doing the most work + - Group by domain to show which domains trigger which deny rules +7. Track policy configuration across runs: + - Note any runs with SSL Bump or DLP enabled + - Note any differences in policy rule counts between runs + ### Step 5: Generate Report Create a comprehensive markdown report following the formatting guidelines above. Structure your report as follows: @@ -326,7 +378,60 @@ A table showing the most frequently blocked domains: Sort by frequency (most blocked first), show top 20. -#### Section 4: Detailed Request Patterns (In `
` Tags) +#### Section 4: Policy Rule Attribution (Always Visible — when data available) + +**Include this section when `policy_analysis` data was available for at least one run.** + +This section provides rule-level insights that go beyond simple domain counts, showing *which policy rules* are handling traffic and *why* specific requests were denied. + +**4a. Policy Configuration** + +Show the policy summary from the most recent run: +- Number of rules, SSL Bump status, DLP status +- Example: "📋 Policy: 12 rules, SSL Bump disabled, DLP disabled" + +**4b. Policy Rule Hit Table** + +Show aggregated rule hit counts across all analyzed runs: + +```markdown +| Rule | Action | Description | Total Hits | +|------|--------|-------------|------------| +| allow-github | allow | Allow GitHub domains | 523 | +| allow-npm | allow | Allow npm registry | 187 | +| deny-blocked-plain | deny | Deny all other HTTP/HTTPS | 12 | +| deny-default | deny | Default deny | 3 | +``` + +- Sort by hits (descending) +- Include all rules that had at least 1 hit +- Use 🟢 for allow rules and 🔴 for deny rules in the Action column + +**4c. Denied Requests with Rule Attribution** + +Show denied requests grouped by rule, with domain details: + +```markdown +| Domain | Deny Rule | Reason | Occurrences | +|--------|-----------|--------|-------------| +| evil.com:443 | deny-blocked-plain | Domain not in allowlist | 5 | +| tracker.io:443 | deny-blocked-plain | Domain not in allowlist | 3 | +| unknown.host:80 | deny-default | Default deny | 1 | +``` + +- Group by domain + rule combination +- Sort by occurrences (descending) +- Show top 30 entries; wrap the full list in `
` if more than 30 + +**4d. Rule Effectiveness Summary** + +Provide a brief analysis: +- Which deny rules are doing the most work (catching the most unauthorized traffic) +- Which allow rules handle the most traffic (busiest legitimate pathways) +- Any rules with zero hits that could be removed or indicate unused policy entries +- Any `(implicit-deny)` attributions that indicate gaps in the policy (traffic denied without matching any explicit rule) + +#### Section 5: Detailed Request Patterns (In `
` Tags) **IMPORTANT**: Wrap this entire section in a collapsible `
` block: ```markdown @@ -351,7 +456,7 @@ For each workflow that had blocked domains, provide a detailed breakdown:
``` -#### Section 5: Complete Blocked Domains List (In `
` Tags) +#### Section 6: Complete Blocked Domains List (In `
` Tags) **IMPORTANT**: Wrap this entire section in a collapsible `
` block: ```markdown @@ -368,12 +473,13 @@ An alphabetically sorted list of all unique blocked domains:
``` -#### Section 6: Security Recommendations (Always Visible) +#### Section 7: Security Recommendations (Always Visible) Based on the analysis, provide actionable insights: - Domains that appear to be legitimate services that should be allowlisted - Potential security concerns (e.g., suspicious domains) - Suggestions for network permission improvements - Workflows that might need their network permissions updated +- Policy rule suggestions (e.g., rules with zero hits that could be removed, domains that should be added to allow rules) ### Step 6: Create Discussion From 1b1d7e5ea5035b10f92485dc94c1c53686308b13 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 24 Mar 2026 22:15:41 +0000 Subject: [PATCH 6/8] fix: align emoji example in policy rule hit table documentation Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw/sessions/d0283a81-24a3-4d01-bf28-71ac57201e36 --- .github/workflows/daily-firewall-report.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/daily-firewall-report.md b/.github/workflows/daily-firewall-report.md index 612e3a31b04..fbd908bb359 100644 --- a/.github/workflows/daily-firewall-report.md +++ b/.github/workflows/daily-firewall-report.md @@ -397,10 +397,10 @@ Show aggregated rule hit counts across all analyzed runs: ```markdown | Rule | Action | Description | Total Hits | |------|--------|-------------|------------| -| allow-github | allow | Allow GitHub domains | 523 | -| allow-npm | allow | Allow npm registry | 187 | -| deny-blocked-plain | deny | Deny all other HTTP/HTTPS | 12 | -| deny-default | deny | Default deny | 3 | +| allow-github | 🟢 allow | Allow GitHub domains | 523 | +| allow-npm | 🟢 allow | Allow npm registry | 187 | +| deny-blocked-plain | 🔴 deny | Deny all other HTTP/HTTPS | 12 | +| deny-default | 🔴 deny | Default deny | 3 | ``` - Sort by hits (descending) From 2b29236a426e29835fc64538480315853e2f6c39 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 24 Mar 2026 22:28:43 +0000 Subject: [PATCH 7/8] refactor: use console pkg formatters in renderPolicyAnalysis Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw/sessions/6842f536-c9de-4410-8020-70b30473730c --- pkg/cli/audit_report.go | 9 +++++++++ pkg/cli/audit_report_render.go | 22 +++++++++++----------- pkg/cli/firewall_policy.go | 8 ++------ 3 files changed, 22 insertions(+), 17 deletions(-) diff --git a/pkg/cli/audit_report.go b/pkg/cli/audit_report.go index 0a068c57559..cbbd36697ad 100644 --- a/pkg/cli/audit_report.go +++ b/pkg/cli/audit_report.go @@ -183,6 +183,15 @@ type MCPServerStats struct { ErrorCount int `json:"error_count,omitempty" console:"header:Errors,omitempty"` } +// PolicySummaryDisplay is a display-optimized version of PolicyAnalysis for console rendering +type PolicySummaryDisplay struct { + Policy string `console:"header:Policy"` + TotalRequests int `console:"header:Total Requests"` + Allowed int `console:"header:Allowed"` + Denied int `console:"header:Denied"` + UniqueDomains int `console:"header:Unique Domains"` +} + // OverviewDisplay is a display-optimized version of OverviewData for console rendering type OverviewDisplay struct { RunID int64 `console:"header:Run ID"` diff --git a/pkg/cli/audit_report_render.go b/pkg/cli/audit_report_render.go index 3d0a0799979..cc455035265 100644 --- a/pkg/cli/audit_report_render.go +++ b/pkg/cli/audit_report_render.go @@ -577,20 +577,20 @@ func renderPerformanceMetrics(metrics *PerformanceMetrics) { func renderPolicyAnalysis(analysis *PolicyAnalysis) { auditReportLog.Printf("Rendering policy analysis: rules=%d, denied=%d", len(analysis.RuleHits), analysis.DeniedCount) - // Policy summary - fmt.Fprintf(os.Stderr, " Policy: %s\n", analysis.PolicySummary) - fmt.Fprintln(os.Stderr) - - // Summary statistics - fmt.Fprintf(os.Stderr, " Total Requests : %d\n", analysis.TotalRequests) - fmt.Fprintf(os.Stderr, " Allowed : %d\n", analysis.AllowedCount) - fmt.Fprintf(os.Stderr, " Denied : %d\n", analysis.DeniedCount) - fmt.Fprintf(os.Stderr, " Unique Domains : %d\n", analysis.UniqueDomains) + // Policy summary using RenderStruct + display := PolicySummaryDisplay{ + Policy: analysis.PolicySummary, + TotalRequests: analysis.TotalRequests, + Allowed: analysis.AllowedCount, + Denied: analysis.DeniedCount, + UniqueDomains: analysis.UniqueDomains, + } + fmt.Fprint(os.Stderr, console.RenderStruct(display)) fmt.Fprintln(os.Stderr) // Rule hit table if len(analysis.RuleHits) > 0 { - fmt.Fprintln(os.Stderr, " Policy Rules:") + fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Policy Rules:")) fmt.Fprintln(os.Stderr) ruleConfig := console.TableConfig{ @@ -614,7 +614,7 @@ func renderPolicyAnalysis(analysis *PolicyAnalysis) { // Denied requests detail if len(analysis.DeniedRequests) > 0 { - fmt.Fprintf(os.Stderr, " Denied Requests (%d):\n", len(analysis.DeniedRequests)) + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Denied Requests (%d):", len(analysis.DeniedRequests)))) fmt.Fprintln(os.Stderr) deniedConfig := console.TableConfig{ diff --git a/pkg/cli/firewall_policy.go b/pkg/cli/firewall_policy.go index 9aef692a1c1..9cd566db44d 100644 --- a/pkg/cli/firewall_policy.go +++ b/pkg/cli/firewall_policy.go @@ -7,6 +7,7 @@ import ( "os" "path/filepath" "regexp" + "slices" "sort" "strings" @@ -161,12 +162,7 @@ func domainMatchesRule(host string, rule PolicyRule) bool { // Detect regex-based rules: aclName contains "regex" or domains contain regex metacharacters isRegex := strings.Contains(strings.ToLower(rule.ACLName), "regex") if !isRegex { - for _, d := range rule.Domains { - if containsRegexMeta(d) { - isRegex = true - break - } - } + isRegex = slices.ContainsFunc(rule.Domains, containsRegexMeta) } if isRegex { From f991b87d785e33f13bd68a87ec90c8195b15f246 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 24 Mar 2026 22:35:46 +0000 Subject: [PATCH 8/8] fix: handle unattributed-allow for unmatched allowed traffic and normalize domain case Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw/sessions/f7144292-b3b2-4f1f-ab3d-d759920e039f --- pkg/cli/firewall_policy.go | 23 +++++++++++++-------- pkg/cli/firewall_policy_test.go | 36 +++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 8 deletions(-) diff --git a/pkg/cli/firewall_policy.go b/pkg/cli/firewall_policy.go index 9cd566db44d..ad10cb0c9c1 100644 --- a/pkg/cli/firewall_policy.go +++ b/pkg/cli/firewall_policy.go @@ -319,12 +319,12 @@ func enrichWithPolicyRules(entries []AuditLogEntry, manifest *PolicyManifest) *P continue } - // Strip port for domain tracking + // Strip port for domain tracking and normalize case domain := host if idx := strings.LastIndex(host, ":"); idx != -1 { domain = host[:idx] } - uniqueDomains[domain] = true + uniqueDomains[strings.ToLower(domain)] = true rule := findMatchingRule(entry, manifest.Rules) @@ -346,12 +346,19 @@ func enrichWithPolicyRules(entries []AuditLogEntry, manifest *PolicyManifest) *P allowedCount++ } } else { - // No matching rule — implicit deny - enriched.RuleID = "(implicit-deny)" - enriched.Action = "deny" - enriched.Reason = "No matching policy rule" - deniedRequests = append(deniedRequests, enriched) - deniedCount++ + // No matching rule — derive outcome from observed entry decision + if isEntryAllowed(entry) { + enriched.RuleID = "(unattributed-allow)" + enriched.Action = "allow" + enriched.Reason = "Allowed (rule not identified)" + allowedCount++ + } else { + enriched.RuleID = "(implicit-deny)" + enriched.Action = "deny" + enriched.Reason = "No matching policy rule" + deniedRequests = append(deniedRequests, enriched) + deniedCount++ + } } } diff --git a/pkg/cli/firewall_policy_test.go b/pkg/cli/firewall_policy_test.go index e4eb5431e59..d321e365753 100644 --- a/pkg/cli/firewall_policy_test.go +++ b/pkg/cli/firewall_policy_test.go @@ -512,6 +512,42 @@ func TestEnrichWithPolicyRules(t *testing.T) { result := enrichWithPolicyRules(squidEntries, manifest) assert.Equal(t, 1, result.TotalRequests, "Squid error entries should be filtered out") }) + + t.Run("unattributed-allow for allowed traffic with no matching rule", func(t *testing.T) { + // Manifest without a catch-all rule — allowed traffic that doesn't match + // any allow rule should be classified as (unattributed-allow), not (implicit-deny) + limitedManifest := &PolicyManifest{ + Version: 1, + Rules: []PolicyRule{ + {ID: "allow-github", Order: 1, Action: "allow", ACLName: "allowed_domains", Protocol: "both", Domains: []string{".github.com"}, Description: "Allow GitHub"}, + }, + } + unattribEntries := []AuditLogEntry{ + // Allowed request that matches the allow-github rule + {Timestamp: 1.0, Host: "api.github.com:443", Method: "CONNECT", Status: 200, Decision: "TCP_TUNNEL"}, + // Allowed request that does NOT match any rule — should be (unattributed-allow) + {Timestamp: 2.0, Host: "unknown.example.com:443", Method: "CONNECT", Status: 200, Decision: "TCP_TUNNEL"}, + // Denied request that does NOT match any rule — should be (implicit-deny) + {Timestamp: 3.0, Host: "evil.com:443", Method: "CONNECT", Status: 403, Decision: "NONE_NONE"}, + } + result := enrichWithPolicyRules(unattribEntries, limitedManifest) + assert.Equal(t, 3, result.TotalRequests, "Should process all 3 entries") + assert.Equal(t, 2, result.AllowedCount, "Should have 2 allowed (1 attributed + 1 unattributed)") + assert.Equal(t, 1, result.DeniedCount, "Should have 1 denied (implicit)") + require.Len(t, result.DeniedRequests, 1, "Should have 1 denied request") + assert.Equal(t, "(implicit-deny)", result.DeniedRequests[0].RuleID, "Denied should be implicit-deny") + assert.Equal(t, "evil.com:443", result.DeniedRequests[0].Host, "Denied host should match") + }) + + t.Run("unique domains case normalized", func(t *testing.T) { + caseEntries := []AuditLogEntry{ + {Timestamp: 1.0, Host: "API.GITHUB.COM:443", Method: "CONNECT", Status: 200, Decision: "TCP_TUNNEL"}, + {Timestamp: 2.0, Host: "api.github.com:443", Method: "CONNECT", Status: 200, Decision: "TCP_TUNNEL"}, + {Timestamp: 3.0, Host: "Api.GitHub.com:443", Method: "CONNECT", Status: 200, Decision: "TCP_TUNNEL"}, + } + result := enrichWithPolicyRules(caseEntries, manifest) + assert.Equal(t, 1, result.UniqueDomains, "Mixed-case hosts for same domain should count as 1 unique domain") + }) } func TestDetectFirewallAuditArtifacts(t *testing.T) {