diff --git a/commands.go b/commands.go index d6fad0a..2bf1830 100644 --- a/commands.go +++ b/commands.go @@ -272,6 +272,13 @@ func runCommandDepth(line string, depth int) bool { } comment := strings.Join(fields[1:], " ") git(DIR, "add", "--all", ".") + // last look before anything is committed: `add --all` sweeps up whatever + // is lying around, and with mirroring on it goes straight to a public + // server. Nothing has been committed yet, so declining costs nothing. + if !secretsApproved(DIR) { + errorln("push cancelled — your changes are staged but not committed") + break + } msg := strings.TrimSpace(fmt.Sprintf("[%s@%s] %s", USER, HOST, comment)) git(DIR, "commit", "-m", msg) // may be "nothing to commit"; continue anyway if !gitOK(DIR, "push") { @@ -711,7 +718,7 @@ var helpItems = []struct{ cmd, desc string }{ {"pull", "pull changes from git server"}, {"fetch", "fetch changes from git server"}, {"status [-a]", "short git status (-a: overview of all projects)"}, - {"overview", "status of all projects (dirty, ahead/behind)"}, + {"overview", "inventory of all projects, local and on the server"}, {"diff [args]", "show git diff"}, {"edit [number]", "edit last [number] commits (default is 10)"}, {"clone [-a] ", "clone repository from git server (-a for archive)"}, diff --git a/config.go b/config.go index 6d3b310..09f2cf6 100644 --- a/config.go +++ b/config.go @@ -38,6 +38,7 @@ type Config struct { RemoteType string // "gitea"|"github"|"gitlab" (auto-detected when empty) RemoteVis string // visibility of created repos: "private" (default)|"public" Mirror string // truthy -> `push` also mirrors via `pushremote` + SecretScan string // falsy -> `push` skips the credential scan Remotes []RemoteTarget RemoteNames string // "remotes": explicit, ordered subset of targets to use } @@ -370,6 +371,7 @@ func applyConfig(c *Config, m map[string]string) { set("remotevisibility", &c.RemoteVis) set("remotes", &c.RemoteNames) set("mirror", &c.Mirror) + set("secretscan", &c.SecretScan) applyRemoteTargets(c, m) } @@ -443,4 +445,5 @@ func applyEnv(c *Config) { env("MGSH_REMOTEVISIBILITY", &c.RemoteVis) env("MGSH_REMOTES", &c.RemoteNames) env("MGSH_MIRROR", &c.Mirror) + env("MGSH_SECRETSCAN", &c.SecretScan) } diff --git a/mgshrc.example b/mgshrc.example index 165cea7..1248aa9 100644 --- a/mgshrc.example +++ b/mgshrc.example @@ -42,6 +42,11 @@ gitpath = /home/git # # mirror = true # `push` also mirrors via pushremote +# --- safety --- +# `push` checks the staged diff for private keys and API tokens before it +# commits, and asks before continuing. Only an explicit "off" disables it. +# secretscan = off + # --- per-project overrides --- # A /.mgshrc overrides all of the above for that project only, except # base, gitname, gitemail and pushdefault, which stay global. Typical use: diff --git a/secrets.go b/secrets.go new file mode 100644 index 0000000..2e1519c --- /dev/null +++ b/secrets.go @@ -0,0 +1,206 @@ +package main + +// secrets.go — a last look at what `push` is about to commit. +// +// `push` runs `git add --all .`, so anything lying in the project — an .env, a +// stray key file, a token pasted into a config — is committed and pushed, and +// with `mirror = true` it reaches a *public* server in the same breath. That is +// the one action in mgsh that cannot be undone: a deleted server repository can +// come back from an archive, a published credential is burnt. +// +// So the staged diff is scanned for a small set of high-signal patterns before +// the commit is made. This is not a complete secret scanner and does not try to +// be one; it aims for a high hit rate on the things that actually leak, with +// few enough false alarms that the prompt still means something. Turn it off +// with `secretscan = off`. + +import ( + "fmt" + "regexp" + "strconv" + "strings" +) + +// secretHit is one suspicious added line. +type secretHit struct { + file string + lineNo int + kind string + text string +} + +// secretPattern matches one kind of credential. `certain` patterns are +// unmistakable and are reported as they are; the others match a shape that +// merely looks like a secret and are filtered through looksLikePlaceholder. +type secretPattern struct { + kind string + re *regexp.Regexp + certain bool +} + +var secretPatterns = []secretPattern{ + {"private key", regexp.MustCompile(`-----BEGIN (?:[A-Z]+ )?PRIVATE KEY-----`), true}, + {"GitHub token", regexp.MustCompile(`\bgh[pousr]_[A-Za-z0-9]{20,}`), true}, + {"GitLab token", regexp.MustCompile(`\bglpat-[A-Za-z0-9_-]{16,}`), true}, + {"AWS access key", regexp.MustCompile(`\b(?:AKIA|ASIA)[0-9A-Z]{16}\b`), true}, + {"Slack token", regexp.MustCompile(`\bxox[baprs]-[A-Za-z0-9-]{10,}`), true}, + {"PyPI token", regexp.MustCompile(`\bpypi-AgEIcHlwaS5vcmc[A-Za-z0-9_-]{10,}`), true}, + {"credential assignment", regexp.MustCompile( + `(?i)\b(?:password|passwd|secret|api[_-]?key|apikey|access[_-]?key|auth[_-]?token|token)\b` + + `\s*[:=]\s*(?:"([^"\s]{12,})"|'([^'\s]{12,})'|([^\s"';,]{20,}))\s*;?\s*$`), false}, +} + +var ( + diffFileRe = regexp.MustCompile(`^\+\+\+ b/(.*)$`) + diffHunkRe = regexp.MustCompile(`^@@ -\d+(?:,\d+)? \+(\d+)`) + // a value that is plainly a reference or a stand-in, not a credential + constRefRe = regexp.MustCompile(`^[A-Z][A-Z0-9_]*$`) + dottedRefRe = regexp.MustCompile(`^[\w-]+(?:\.[\w-]+)+$`) + maskedRe = regexp.MustCompile(`^[*x•.]+$`) +) + +// placeholderWords are the values people write when they mean "fill this in". +var placeholderWords = map[string]bool{ + "changeme": true, "change_me": true, "password": true, "secret": true, + "token": true, "your_token": true, "your-token": true, "yourtoken": true, + "todo": true, "none": true, "null": true, "example": true, "redacted": true, +} + +// looksLikePlaceholder reports whether a matched value is obviously not a real +// credential: a template slot, an environment reference, a constant name, or a +// masked stand-in. Documentation and example files are full of these, and every +// one of them that reaches the prompt makes the prompt worth less. +func looksLikePlaceholder(v string) bool { + v = strings.Trim(v, `"'`) + if v == "" { + return true + } + if strings.ContainsAny(v, "<>${}()") { // , ${VAR}, $(cmd), {{ tpl }} + return true + } + if maskedRe.MatchString(v) || constRefRe.MatchString(v) || dottedRefRe.MatchString(v) { + return true + } + if placeholderWords[strings.ToLower(v)] { + return true + } + // a value made of one repeated character carries no information + if strings.Count(v, string(v[0])) == len(v) { + return true + } + return false +} + +// scanDiff finds suspicious added lines in a unified diff. Only added lines are +// examined: removing a secret is what we want people to do. +func scanDiff(diff string) []secretHit { + var hits []secretHit + file := "" + lineNo := 0 + + for _, ln := range strings.Split(diff, "\n") { + switch { + case strings.HasPrefix(ln, "+++ "): + file = "" + if m := diffFileRe.FindStringSubmatch(ln); m != nil { + file = m[1] + } + continue + case strings.HasPrefix(ln, "@@"): + if m := diffHunkRe.FindStringSubmatch(ln); m != nil { + lineNo, _ = strconv.Atoi(m[1]) + } + continue + case strings.HasPrefix(ln, "---") || strings.HasPrefix(ln, "diff ") || + strings.HasPrefix(ln, "index ") || strings.HasPrefix(ln, "new file") || + strings.HasPrefix(ln, "deleted file") || strings.HasPrefix(ln, "similarity "): + continue + case strings.HasPrefix(ln, "-"): + continue // removed line: not our problem + case !strings.HasPrefix(ln, "+"): + lineNo++ // context line + continue + } + + text := ln[1:] + if kind := matchSecret(text); kind != "" { + hits = append(hits, secretHit{file: file, lineNo: lineNo, kind: kind, text: text}) + } + lineNo++ + } + return hits +} + +// allowMarker suppresses the check for one line. Any scanner needs a per-line +// escape: a project will eventually hold something credential-shaped on +// purpose, and switching the whole check off for that is far too blunt. +const allowMarker = "mgsh:allow" + +// matchSecret returns the kind of credential a line appears to contain, or "". +func matchSecret(text string) string { + if strings.Contains(text, allowMarker) { + return "" + } + for _, p := range secretPatterns { + m := p.re.FindStringSubmatch(text) + if m == nil { + continue + } + if p.certain { + return p.kind + } + // the first non-empty capture group is the value that was assigned + value := "" + for _, g := range m[1:] { + if g != "" { + value = g + break + } + } + if !looksLikePlaceholder(value) { + return p.kind + } + } + return "" +} + +// secretScanEnabled reports whether the scan runs. It is on unless explicitly +// switched off, so a typo in the setting leaves the safety net in place. +func secretScanEnabled() bool { return !falsy(cfg.SecretScan) } + +// secretsApproved scans what `push` has staged. With nothing suspicious found +// it returns true silently; otherwise it shows the findings and asks. Returns +// false when the push should stop. +func secretsApproved(dir string) bool { + if !secretScanEnabled() { + return true + } + diff, err := gitCapture(dir, "diff", "--cached", "-U0", "--no-color") + if err != nil { + return true // nothing staged, or no HEAD yet: not our call to block + } + hits := scanDiff(diff) + if len(hits) == 0 { + return true + } + + fmt.Println(col(cRed, fmt.Sprintf("%d possible credential(s) in what is about to be committed:", len(hits)))) + for _, h := range hits { + where := h.file + if h.lineNo > 0 { + where += ":" + strconv.Itoa(h.lineNo) + } + fmt.Printf(" %s %s\n %s\n", + col(cYellow, where), col(cGray, h.kind), col(cRed, ellipsis(strings.TrimSpace(h.text), 100))) + } + fmt.Println(col(cGray, " (set 'secretscan = off' to skip this check)")) + return yesno("push anyway?", false) +} + +// ellipsis shortens s to at most n characters. +func ellipsis(s string, n int) string { + if len(s) <= n { + return s + } + return s[:n-1] + "…" +} diff --git a/secrets_test.go b/secrets_test.go new file mode 100644 index 0000000..721ddae --- /dev/null +++ b/secrets_test.go @@ -0,0 +1,212 @@ +package main + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// TestMatchSecretCatchesRealCredentials: the shapes that actually leak. +func TestMatchSecretCatchesRealCredentials(t *testing.T) { + lines := []string{ + "-----BEGIN OPENSSH PRIVATE KEY-----", + "-----BEGIN RSA PRIVATE KEY-----", + " ghp_aB3dEfGh1jKlMn0pQrStUvWxYz012345678", + "GITLAB=glpat-aB3dEfGh1jKlMn0pQrSt", + `aws_access_key_id = AKIAIOSFODNN7EXAMPLE`, + "slack: xoxb-1234567890-abcdefghij", + `API_KEY="s3cr3tV4lu3W1thStuff"`, + "password = hunter2hunter2hunter2", + "token: 'aB3dEfGh1jKlMn0pQrSt'", + "auth-token=9f8e7d6c5b4a39281706abcdef123456", + } + for _, ln := range lines { + if matchSecret(ln) == "" { + t.Errorf("missed a credential in %q", ln) + } + } +} + +// TestMatchSecretIgnoresNoise: everyday code and documentation must not trip +// the prompt, or people learn to answer "yes" without reading it. +func TestMatchSecretIgnoresNoise(t *testing.T) { + lines := []string{ + "remotekey = ", // our own README + "# remotekey = ", // and mgshrc.example + "token = process.env.GITHUB_TOKEN", // reference, not a value + "const token = getToken()", // call + "password = ${DB_PASSWORD}", // template + `api_key = "changeme"`, // placeholder + "secret: TODO", // + "key gh***************xk", // masked, from `config` + "password = xxxxxxxxxxxxxxxxxxxxxxx", // masked + "// the token is never persisted in the repo", // prose + "apiKey := os.Getenv(\"MGSH_REMOTEKEY\")", // lookup + "token = SOME_CONSTANT_NAME", // constant + "secret = my.config.value", // dotted reference + "+++ b/token.go", // diff furniture + "password = short", // too short to be one + "Authorization: Basic ", // documentation + "remote.hub.key = ", + } + for _, ln := range lines { + if kind := matchSecret(ln); kind != "" { + t.Errorf("false positive (%s) on %q", kind, ln) + } + } +} + +// TestScanDiffReportsFileAndLine: the report has to point at the right place, +// and must ignore removed lines — deleting a secret is the desired action. +func TestScanDiffReportsFileAndLine(t *testing.T) { + diff := `diff --git a/.env b/.env +new file mode 100644 +--- /dev/null ++++ b/.env +@@ -0,0 +1,3 @@ ++HOME=/tmp ++API_KEY="s3cr3tV4lu3W1thStuff" ++DEBUG=1 +diff --git a/old.txt b/old.txt +--- a/old.txt ++++ b/old.txt +@@ -7,1 +7,0 @@ +-password = hunter2hunter2hunter2 +` + hits := scanDiff(diff) + if len(hits) != 1 { + t.Fatalf("expected exactly one hit, got %d: %+v", len(hits), hits) + } + h := hits[0] + if h.file != ".env" { + t.Errorf("file = %q, want .env", h.file) + } + if h.lineNo != 2 { + t.Errorf("lineNo = %d, want 2", h.lineNo) + } + if !strings.Contains(h.text, "API_KEY") { + t.Errorf("text = %q", h.text) + } +} + +// TestScanDiffCountsLinesAcrossHunks keeps the line numbers honest when a file +// is edited in several places. +func TestScanDiffCountsLinesAcrossHunks(t *testing.T) { + diff := `+++ b/config.yml +@@ -1,0 +1,1 @@ ++harmless: yes +@@ -40,0 +41,2 @@ ++also fine ++aws_key = AKIAIOSFODNN7EXAMPLE +` + hits := scanDiff(diff) + if len(hits) != 1 || hits[0].lineNo != 42 { + t.Fatalf("hits = %+v, want one at line 42", hits) + } +} + +// TestSecretsApprovedBlocksThePush drives the real thing: a staged .env, the +// scan, and the answer deciding whether push continues. +func TestSecretsApprovedBlocksThePush(t *testing.T) { + dir := t.TempDir() + mustGit(t, dir, "init", "-q") + mustGit(t, dir, "config", "user.name", "t") + mustGit(t, dir, "config", "user.email", "t@e") + mustGit(t, dir, "commit", "-q", "--allow-empty", "-m", "base") + + old := cfg + defer func() { cfg = old }() + cfg = Config{} + + // clean tree: no prompt, no interference + asked := fakeAnswers(t, false) + if !secretsApproved(dir) { + t.Fatal("a clean tree must not block the push") + } + if len(*asked) != 0 { + t.Fatalf("asked about a clean tree: %v", *asked) + } + + if err := os.WriteFile(filepath.Join(dir, ".env"), + []byte("API_KEY=\"s3cr3tV4lu3W1thStuff\"\n"), 0600); err != nil { + t.Fatal(err) + } + mustGit(t, dir, "add", "--all", ".") + + declined := fakeAnswers(t, false) + if secretsApproved(dir) { + t.Error("a staged credential must stop the push when declined") + } + if len(*declined) == 0 { + t.Error("the user was never asked") + } + + accepted := fakeAnswers(t, true) + if !secretsApproved(dir) { + t.Error("an explicit yes must let the push through") + } + if len(*accepted) == 0 { + t.Error("the user was never asked") + } + + // and the escape hatch really switches it off + cfg.SecretScan = "off" + never := fakeAnswers(t, false) + if !secretsApproved(dir) { + t.Error("secretscan = off must not block") + } + if len(*never) != 0 { + t.Errorf("secretscan = off still asked: %v", *never) + } +} + +// TestSecretScanDefaultsToOn: only a deliberate "off" disables it, so a typo +// leaves the safety net in place. +func TestSecretScanDefaultsToOn(t *testing.T) { + old := cfg + defer func() { cfg = old }() + for _, v := range []string{"", "true", "on", "yes", "wharrgarbl", "1"} { + cfg = Config{SecretScan: v} + if !secretScanEnabled() { + t.Errorf("secretscan = %q disabled the scan", v) + } + } + for _, v := range []string{"off", "0", "false", "no", " OFF "} { + cfg = Config{SecretScan: v} + if secretScanEnabled() { + t.Errorf("secretscan = %q did not disable the scan", v) + } + } +} + +// TestOwnDocsDoNotTripTheScanner: mgsh's own README and example config are full +// of credential-shaped text; committing mgsh itself must stay quiet. +func TestOwnDocsDoNotTripTheScanner(t *testing.T) { + for _, f := range []string{"README.md", "mgshrc.example"} { + data, err := os.ReadFile(f) + if err != nil { + t.Fatal(err) + } + for i, ln := range strings.Split(string(data), "\n") { + if kind := matchSecret(ln); kind != "" { + t.Errorf("%s:%d would trip the scanner (%s): %q", f, i+1, kind, ln) + } + } + } +} + +// TestAllowMarkerSuppressesOneLine: the per-line escape for something that only +// looks like a credential and is meant to stay. +func TestAllowMarkerSuppressesOneLine(t *testing.T) { + line := `API_KEY="s3cr3tV4lu3W1thStuff"` + if matchSecret(line) == "" { + t.Fatal("test line is not detected at all") + } + if kind := matchSecret(line + " # mgsh:allow — sample value"); kind != "" { + t.Errorf("mgsh:allow did not suppress the hit (%s)", kind) + } + if kind := matchSecret("-----BEGIN OPENSSH PRIVATE KEY----- mgsh:allow"); kind != "" { + t.Errorf("mgsh:allow did not suppress a certain pattern (%s)", kind) + } +} diff --git a/show_config.go b/show_config.go index 88fc88c..f0530c7 100644 --- a/show_config.go +++ b/show_config.go @@ -44,6 +44,7 @@ func showConfig() { {"pushdefault", cfg.PushDefault}, {"editor", cfg.Editor}, {"mirror", cfg.Mirror}, + {"secretscan", cfg.SecretScan}, {"remotes", cfg.RemoteNames}, } @@ -147,7 +148,7 @@ func configKeys() []string { "base", "githost", "gitport", "gituser", "gitpath", "gitkey", "gitname", "gitemail", "pushdefault", "editor", "remoteurl", "remotekey", "remotetype", "remotevisibility", - "remotes", "mirror", + "remotes", "mirror", "secretscan", } sort.Strings(keys) return keys diff --git a/util.go b/util.go index 13feedf..12b8bb1 100644 --- a/util.go +++ b/util.go @@ -39,6 +39,17 @@ func truthy(s string) bool { return false } +// falsy reports whether a config string explicitly means "off". It is not the +// negation of truthy: for a setting that defaults to on, an unset value or a +// typo must leave it on, and only a deliberate "off" may switch it off. +func falsy(s string) bool { + switch strings.ToLower(strings.TrimSpace(s)) { + case "0", "false", "no", "off": + return true + } + return false +} + func fileExists(p string) bool { fi, err := os.Stat(p) return err == nil && !fi.IsDir()