[WIP] gmpctl fixes#1938
Conversation
Best effort fixes thanks to Gemini security scanning. Signed-off-by: bwplotka <bwplotka@google.com>
Signed-off-by: bwplotka <bwplotka@google.com>
Signed-off-by: bwplotka <bwplotka@google.com>
Signed-off-by: bwplotka <bwplotka@google.com>
Signed-off-by: bwplotka <bwplotka@google.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the gmpctl tool by automating Go version detection from Dockerfiles, resolving OpenTelemetry schema version conflicts, adding automated pull request creation via the GitHub CLI, and introducing a thread-safe cache for NVD API severity lookups. Feedback on these changes focuses on improving robustness and performance, such as handling non-interactive environments gracefully in TTY detection, utilizing filepath.WalkDir and find -prune for more efficient directory traversal, capturing standard error in command execution, using idiomatic string comparisons, and falling back to CVSS V3.0 metrics when V3.1 is missing.
| func mustGetTTY() string { | ||
| ttyCmd := exec.Command("tty") | ||
| ttyOut, err := ttyCmd.Output() | ||
| if err != nil { | ||
| panicf(err.Error()) | ||
| } | ||
| return strings.TrimSpace(string(ttyOut)) | ||
| } |
There was a problem hiding this comment.
If the script is run in a non-interactive environment (such as a CI/CD bot or automated pipeline), the tty command will fail, causing mustGetTTY to panic and crash the entire program. Returning an empty string on error allows the program to handle the absence of a TTY gracefully.
| func mustGetTTY() string { | |
| ttyCmd := exec.Command("tty") | |
| ttyOut, err := ttyCmd.Output() | |
| if err != nil { | |
| panicf(err.Error()) | |
| } | |
| return strings.TrimSpace(string(ttyOut)) | |
| } | |
| func mustGetTTY() string { | |
| ttyCmd := exec.Command("tty") | |
| ttyOut, err := ttyCmd.Output() | |
| if err != nil { | |
| return "" | |
| } | |
| return strings.TrimSpace(string(ttyOut)) | |
| } |
| // Ensure TTY is set for GPG signing. | ||
| envs := []string{"GPG_TTY=" + mustGetTTY()} |
There was a problem hiding this comment.
Only set the GPG_TTY environment variable if a valid TTY was successfully detected, ensuring compatibility with non-interactive environments.
| // Ensure TTY is set for GPG signing. | |
| envs := []string{"GPG_TTY=" + mustGetTTY()} | |
| // Ensure TTY is set for GPG signing if available. | |
| var envs []string | |
| if tty := mustGetTTY(); tty != "" { | |
| envs = append(envs, "GPG_TTY="+tty) | |
| } |
| err := filepath.Walk(dir, func(path string, info os.FileInfo, err error) error { | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if info.IsDir() { | ||
| name := info.Name() | ||
| if name == "third_party" || name == "ui" || name == "vendor" || name == "node_modules" || name == ".git" { | ||
| return filepath.SkipDir | ||
| } | ||
| return nil | ||
| } | ||
| if strings.HasPrefix(info.Name(), "Dockerfile") { | ||
| dockerfiles = append(dockerfiles, path) | ||
| } | ||
| return nil | ||
| }) |
There was a problem hiding this comment.
Using filepath.Walk is less efficient because it queries the file system for every file. Since Go 1.16, filepath.WalkDir is preferred as it avoids calling os.Lstat on every file, significantly improving performance.
| err := filepath.Walk(dir, func(path string, info os.FileInfo, err error) error { | |
| if err != nil { | |
| return err | |
| } | |
| if info.IsDir() { | |
| name := info.Name() | |
| if name == "third_party" || name == "ui" || name == "vendor" || name == "node_modules" || name == ".git" { | |
| return filepath.SkipDir | |
| } | |
| return nil | |
| } | |
| if strings.HasPrefix(info.Name(), "Dockerfile") { | |
| dockerfiles = append(dockerfiles, path) | |
| } | |
| return nil | |
| }) | |
| err := filepath.WalkDir(dir, func(path string, d os.DirEntry, err error) error { | |
| if err != nil { | |
| return err | |
| } | |
| if d.IsDir() { | |
| name := d.Name() | |
| if name == "third_party" || name == "ui" || name == "vendor" || name == "node_modules" || name == ".git" { | |
| return filepath.SkipDir | |
| } | |
| return nil | |
| } | |
| if strings.HasPrefix(d.Name(), "Dockerfile") { | |
| dockerfiles = append(dockerfiles, path) | |
| } | |
| return nil | |
| }) |
| cmd := exec.Command("go", "run", "gmpctl_tmp_schema.go") | ||
| cmd.Dir = dir | ||
| out, err := cmd.Output() |
There was a problem hiding this comment.
When running the temporary schema detector, any compilation or runtime errors printed to stderr are currently lost, making debugging failures extremely difficult. Setting cmd.Stderr = os.Stderr ensures that these errors are printed to the terminal for easier troubleshooting.
cmd := exec.Command("go", "run", "gmpctl_tmp_schema.go")
cmd.Dir = dir
cmd.Stderr = os.Stderr
out, err := cmd.Output()| if err := filepath.Walk(dir, func(path string, info os.FileInfo, err error) error { | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if info.IsDir() { | ||
| name := info.Name() | ||
| if name == "vendor" || name == "third_party" || name == ".git" { | ||
| return filepath.SkipDir | ||
| } | ||
| return nil | ||
| } | ||
| if !strings.HasSuffix(info.Name(), ".go") { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Using filepath.Walk is less efficient because it queries the file system for every file. Since Go 1.16, filepath.WalkDir is preferred as it avoids calling os.Lstat on every file, significantly improving performance.
| if err := filepath.Walk(dir, func(path string, info os.FileInfo, err error) error { | |
| if err != nil { | |
| return err | |
| } | |
| if info.IsDir() { | |
| name := info.Name() | |
| if name == "vendor" || name == "third_party" || name == ".git" { | |
| return filepath.SkipDir | |
| } | |
| return nil | |
| } | |
| if !strings.HasSuffix(info.Name(), ".go") { | |
| return nil | |
| } | |
| if err := filepath.WalkDir(dir, func(path string, d os.DirEntry, err error) error { | |
| if err != nil { | |
| return err | |
| } | |
| if d.IsDir() { | |
| name := d.Name() | |
| if name == "vendor" || name == "third_party" || name == ".git" { | |
| return filepath.SkipDir | |
| } | |
| return nil | |
| } | |
| if !strings.HasSuffix(d.Name(), ".go") { | |
| return nil | |
| } |
| case strings.Compare(branch, "main") == 0: | ||
| return PrometheusEngine, true |
There was a problem hiding this comment.
Using strings.Compare for simple equality checks is a Go anti-pattern. It is clearer, more idiomatic, and faster to use the standard == operator.
| case strings.Compare(branch, "main") == 0: | |
| return PrometheusEngine, true | |
| case branch == "main": | |
| return PrometheusEngine, true |
| return 1 | ||
| fi | ||
| find "${dir}" -name "Dockerfile*" | grep -v "${dir}/third_party/" | grep -v "${dir}/hack/" | grep -v "${dir}/ui/" | grep -v "vendor/" | grep -v "node_modules/" | ||
| find "${dir}" -name "Dockerfile*" | grep -v "${dir}/third_party/" | grep -v "${dir}/ui/" | grep -v "vendor/" | grep -v "node_modules/" |
There was a problem hiding this comment.
The current find command traverses all directories (including large ones like node_modules, vendor, and .git) and then filters them out using grep. This is extremely slow. Using -prune allows find to skip these directories entirely, resulting in a massive performance improvement.
| find "${dir}" -name "Dockerfile*" | grep -v "${dir}/third_party/" | grep -v "${dir}/ui/" | grep -v "vendor/" | grep -v "node_modules/" | |
| find "${dir}" \( -name "third_party" -o -name "ui" -o -name "vendor" -o -name "node_modules" -o -name ".git" \) -prune -o -name "Dockerfile*" -print |
| if len(nvdResponse.Vulnerabilities) > 0 { | ||
| metrics := nvdResponse.Vulnerabilities[0].CVE.Metrics | ||
| if len(metrics.CVSSMetricV31) > 0 { | ||
| return metrics.CVSSMetricV31[0].CVSSData.BaseSeverity, nil | ||
| sev := metrics.CVSSMetricV31[0].CVSSData.BaseSeverity | ||
| cacheMu.Lock() | ||
| severityCache[cveID] = sev | ||
| cacheMu.Unlock() | ||
| return sev, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Fall back to checking CVSS V3.0 metrics if CVSS V3.1 metrics are not present, ensuring accurate severity classification for a wider range of CVEs.
| if len(nvdResponse.Vulnerabilities) > 0 { | |
| metrics := nvdResponse.Vulnerabilities[0].CVE.Metrics | |
| if len(metrics.CVSSMetricV31) > 0 { | |
| return metrics.CVSSMetricV31[0].CVSSData.BaseSeverity, nil | |
| sev := metrics.CVSSMetricV31[0].CVSSData.BaseSeverity | |
| cacheMu.Lock() | |
| severityCache[cveID] = sev | |
| cacheMu.Unlock() | |
| return sev, nil | |
| } | |
| } | |
| if len(nvdResponse.Vulnerabilities) > 0 { | |
| metrics := nvdResponse.Vulnerabilities[0].CVE.Metrics | |
| var sev string | |
| if len(metrics.CVSSMetricV31) > 0 { | |
| sev = metrics.CVSSMetricV31[0].CVSSData.BaseSeverity | |
| } else if len(metrics.CVSSMetricV30) > 0 { | |
| sev = metrics.CVSSMetricV30[0].CVSSData.BaseSeverity | |
| } | |
| if sev != "" { | |
| cacheMu.Lock() | |
| severityCache[cveID] = sev | |
| cacheMu.Unlock() | |
| return sev, nil | |
| } | |
| } |
Signed-off-by: bwplotka <bwplotka@google.com>
Signed-off-by: bwplotka <bwplotka@google.com>
1 participant
Preview of incoming fixes. We could remove disable interactive mode this should be running as a bot.
Notable alternative is renovatebot. I checked and it's AGPL, (so no CLI use for us) and we need to bake a lot of internal logic (e.g. semconv version sync, de-sync of go.mod vs Dockerfiles, etc). Using gmpctl could be a short-term help though.