-
Notifications
You must be signed in to change notification settings - Fork 62
NO-ISSUE: add sharable psa audit run per-scenario #809
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,129 @@ | ||
| package steps | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "context" | ||
| "fmt" | ||
| "os" | ||
| "os/exec" | ||
| "path/filepath" | ||
| "strconv" | ||
| "strings" | ||
|
|
||
| "github.com/spf13/pflag" | ||
| ) | ||
|
|
||
| var ( | ||
| psaCheckBin string | ||
| psaCheckRequired bool | ||
| ) | ||
|
|
||
| func init() { | ||
| flagSet := pflag.CommandLine | ||
| flagSet.StringVar(&psaCheckBin, "psa.check-bin", os.Getenv("PSA_CHECK_BIN"), "Path to the cluster-debug-tools PSA checker; empty disables automatic PSA checks") | ||
| flagSet.BoolVar(&psaCheckRequired, "psa.check-required", psaCheckRequiredFromEnv(), "Fail scenarios when the PSA checker is unavailable") | ||
| } | ||
|
|
||
| func psaCheckRequiredFromEnv() bool { | ||
| required, err := strconv.ParseBool(os.Getenv("PSA_CHECK_REQUIRED")) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| return required | ||
| } | ||
|
|
||
| // runPSACheck evaluates the current scenario namespace before cleanup removes it. | ||
| // The checker is intentionally external because psa-check is distributed as a | ||
| // kubectl-dev_tool plugin rather than a package consumed by this repository. | ||
| func runPSACheck(ctx context.Context, sc *scenarioContext) error { | ||
| bin := strings.TrimSpace(psaCheckBin) | ||
| if bin == "" { | ||
| if psaCheckRequired { | ||
| return fmt.Errorf("PSA checker is required but --psa.check-bin or PSA_CHECK_BIN is unset") | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| if _, err := exec.LookPath(bin); err != nil { | ||
| if psaCheckRequired { | ||
| return fmt.Errorf("PSA checker %q is unavailable: %w", bin, err) | ||
| } | ||
| logger.Info("Skipping PSA check because checker is unavailable", "binary", bin, "error", err) | ||
| return nil | ||
| } | ||
|
|
||
| cmd := exec.CommandContext(ctx, bin, | ||
| "psa-check", | ||
| "--namespace", sc.namespace, | ||
| "--level", "restricted", | ||
| "--output", "json", | ||
| ) | ||
| cmd.Env = append(os.Environ(), fmt.Sprintf("KUBECONFIG=%s", kubeconfigPath)) | ||
|
|
||
| var stdout, stderr bytes.Buffer | ||
| cmd.Stdout = &stdout | ||
| cmd.Stderr = &stderr | ||
| err := cmd.Run() | ||
|
|
||
| artifactPath, artifactErr := psaArtifactPath(sc) | ||
| if artifactErr != nil { | ||
| return artifactErr | ||
| } | ||
| if artifactPath != "" { | ||
| if writeErr := os.WriteFile(artifactPath, stdout.Bytes(), 0o600); writeErr != nil { | ||
| return fmt.Errorf("write PSA result for scenario %q: %w", sc.scenarioName, writeErr) | ||
| } | ||
| if stderr.Len() > 0 { | ||
| stderrPath := strings.TrimSuffix(artifactPath, filepath.Ext(artifactPath)) + ".stderr" | ||
| if writeErr := os.WriteFile(stderrPath, stderr.Bytes(), 0o600); writeErr != nil { | ||
| return fmt.Errorf("write PSA stderr for scenario %q: %w", sc.scenarioName, writeErr) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if err == nil { | ||
| return nil | ||
| } | ||
|
|
||
| message := strings.TrimSpace(stdout.String()) | ||
| if stderrMessage := strings.TrimSpace(stderr.String()); stderrMessage != "" { | ||
| if message != "" { | ||
| message += "; " | ||
| } | ||
| message += stderrMessage | ||
| } | ||
| if message == "" { | ||
| message = "no diagnostic output" | ||
| } | ||
| return fmt.Errorf("PSA check failed for scenario %q in namespace %q: %w: %s", sc.scenarioName, sc.namespace, err, message) | ||
| } | ||
|
|
||
| func psaArtifactPath(sc *scenarioContext) (string, error) { | ||
| basePath := strings.TrimSpace(os.Getenv("ARTIFACT_PATH")) | ||
| if basePath == "" { | ||
| return "", nil | ||
| } | ||
|
|
||
| path := filepath.Join(basePath, "psa", sanitizePSAArtifactPart(sc.featureName), sanitizePSAArtifactPart(sc.id)) | ||
| if err := os.MkdirAll(path, 0o755); err != nil { | ||
| return "", fmt.Errorf("create PSA artifact directory %q: %w", path, err) | ||
| } | ||
| return filepath.Join(path, "psa.json"), nil | ||
| } | ||
|
|
||
| func sanitizePSAArtifactPart(value string) string { | ||
| value = strings.TrimSpace(value) | ||
| if value == "" { | ||
| return "unknown" | ||
| } | ||
| var b strings.Builder | ||
| for _, r := range value { | ||
| switch { | ||
| case r >= 'a' && r <= 'z', r >= 'A' && r <= 'Z', r >= '0' && r <= '9', r == '-', r == '_', r == '.': | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🛡️ Analyzed with Security Review | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# Trace the producers of the artifact path components.
ast-grep outline test/e2e/steps --items all --type function
rg -n -P -C 4 '(?:\bfeatureName\s*:|\b(?:sc\.)?featureName\s*=|\bid\s*:|\b(?:sc\.)?id\s*=)' test/e2e --glob '*.go'Repository: openshift/operator-framework-operator-controller Length of output: 7365 🏁 Script executed: #!/bin/bash
set -euo pipefail
cat -n test/e2e/steps/psa.go | sed -n '60,135p'
cat -n test/e2e/steps/hooks.go | sed -n '208,224p'Repository: openshift/operator-framework-operator-controller Length of output: 3490 Path Traversal CWE: CWE-22 — Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal') Reject dot-only artifact path components.
Proposed fix- return strings.Trim(b.String(), "-")
+ part := strings.Trim(b.String(), "-")
+ if part == "" || part == "." || part == ".." {
+ return "unknown"
+ }
+ return part🤖 Prompt for AI AgentsSource: Path instructions
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might want to appropriately handle |
||
| b.WriteRune(r) | ||
| default: | ||
| b.WriteByte('-') | ||
| } | ||
| } | ||
| return strings.Trim(b.String(), "-") | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| package steps | ||
|
|
||
| import ( | ||
| "context" | ||
| "os" | ||
| "path/filepath" | ||
| "strings" | ||
| "testing" | ||
| ) | ||
|
|
||
| func TestRunPSACheck(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| script string | ||
| required bool | ||
| wantErr bool | ||
| wantOutput string | ||
| }{ | ||
| { | ||
| name: "successful check writes JSON artifact", | ||
| script: "printf '%s' '{\"items\":[]}'", | ||
| wantOutput: `{"items":[]}`, | ||
| }, | ||
| { | ||
| name: "violations fail the check and preserve output", | ||
| script: "printf '%s' '{\"items\":[{\"namespace\":\"ns-test\"}]}' ; exit 1", | ||
| wantErr: true, | ||
| wantOutput: `{"items":[{"namespace":"ns-test"}]}`, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| bin := filepath.Join(t.TempDir(), "psa-check") | ||
| if err := os.WriteFile(bin, []byte("#!/bin/sh\n"+tt.script+"\n"), 0o700); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| artifactPath := t.TempDir() | ||
| t.Setenv("ARTIFACT_PATH", artifactPath) | ||
| oldBin, oldRequired, oldKubeconfig := psaCheckBin, psaCheckRequired, kubeconfigPath | ||
| psaCheckBin, psaCheckRequired, kubeconfigPath = bin, tt.required, "/tmp/test.kubeconfig" | ||
| t.Cleanup(func() { | ||
| psaCheckBin, psaCheckRequired, kubeconfigPath = oldBin, oldRequired, oldKubeconfig | ||
| }) | ||
|
|
||
| sc := &scenarioContext{id: "scenario-1", featureName: "install", scenarioName: tt.name, namespace: "ns-test"} | ||
| err := runPSACheck(context.Background(), sc) | ||
| if (err != nil) != tt.wantErr { | ||
| t.Fatalf("runPSACheck() error = %v, want error: %t", err, tt.wantErr) | ||
| } | ||
|
|
||
| result, err := os.ReadFile(filepath.Join(artifactPath, "psa", "install", "scenario-1", "psa.json")) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if string(result) != tt.wantOutput { | ||
| t.Fatalf("PSA artifact = %q, want %q", result, tt.wantOutput) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestRunPSACheckConfiguration(t *testing.T) { | ||
| t.Run("optional checker can be disabled", func(t *testing.T) { | ||
| oldBin, oldRequired := psaCheckBin, psaCheckRequired | ||
| psaCheckBin, psaCheckRequired = "", false | ||
| t.Cleanup(func() { psaCheckBin, psaCheckRequired = oldBin, oldRequired }) | ||
|
|
||
| err := runPSACheck(context.Background(), &scenarioContext{namespace: "ns-test"}) | ||
| if err != nil { | ||
| t.Fatalf("runPSACheck() error = %v", err) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("required checker must be configured", func(t *testing.T) { | ||
| oldBin, oldRequired := psaCheckBin, psaCheckRequired | ||
| psaCheckBin, psaCheckRequired = "", true | ||
| t.Cleanup(func() { psaCheckBin, psaCheckRequired = oldBin, oldRequired }) | ||
|
|
||
| err := runPSACheck(context.Background(), &scenarioContext{namespace: "ns-test"}) | ||
| if err == nil || !strings.Contains(err.Error(), "checker is required") { | ||
| t.Fatalf("runPSACheck() error = %v, want required-checker error", err) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func TestSanitizePSAArtifactPart(t *testing.T) { | ||
| if got, want := sanitizePSAArtifactPart("feature/name with spaces"), "feature-name-with-spaces"; got != want { | ||
| t.Fatalf("sanitizePSAArtifactPart() = %q, want %q", got, want) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it appropriate to use
.in flag names?