diff --git a/main.go b/main.go index 47c607bb8..c3fa2b370 100644 --- a/main.go +++ b/main.go @@ -54,6 +54,23 @@ import ( "k8s.io/git-sync/pkg/version" ) +// hideEnvDefault replaces a flag's displayed default when one of envKeys is +// set to a non-empty value. pflag prints DefValue in usage; the parsed value +// is left alone. An empty credential default stringifies as "[]", so this +// keys off the environment rather than DefValue != "". +func hideEnvDefault(fs *pflag.FlagSet, name string, envKeys ...string) { + f := fs.Lookup(name) + if f == nil { + return + } + for _, key := range envKeys { + if v, ok := os.LookupEnv(key); ok && v != "" { + f.DefValue = "" + return + } + } +} + var ( metricSyncDuration = prometheus.NewSummaryVec(prometheus.SummaryOpts{ Name: "git_sync_duration_seconds", @@ -405,6 +422,12 @@ func main() { // Parse and verify flags. Errors here are fatal. // + // These three take a default from env vars that can hold a token. + // pflag prints that default in --help and on a parse error. + hideEnvDefault(pflag.CommandLine, "repo", "GITSYNC_REPO", "GIT_SYNC_REPO") + hideEnvDefault(pflag.CommandLine, "credential", "GITSYNC_CREDENTIAL") + hideEnvDefault(pflag.CommandLine, "askpass-url", "GITSYNC_ASKPASS_URL", "GIT_SYNC_ASKPASS_URL", "GIT_ASKPASS_URL") + pflag.Parse() // Was --init-max-failures explicitly set (CLI or env)? envInt applies diff --git a/main_test.go b/main_test.go index 38eec7bf8..b1d331139 100644 --- a/main_test.go +++ b/main_test.go @@ -25,9 +25,70 @@ import ( "time" "github.com/go-logr/logr/funcr" + "github.com/spf13/pflag" "go.uber.org/goleak" ) +func TestHideEnvDefault(t *testing.T) { + secret := "https://user:REPOPAT_xyz789@github.com/o/r.git" + cred := `[{"username":"bot","password":"SENTINEL_PAT"}]` + t.Setenv("GITSYNC_REPO", secret) + t.Setenv("GITSYNC_CREDENTIAL", cred) + t.Setenv("GIT_ASKPASS_URL", "") + + fs := pflag.NewFlagSet("git-sync", pflag.ContinueOnError) + fs.String("repo", secret, "the git repository to sync") + fs.String("credential", cred, "credentials") + fs.String("askpass-url", "", "askpass") + fs.String("ref", "HEAD", "the git revision") + + hideEnvDefault(fs, "repo", "GITSYNC_REPO", "GIT_SYNC_REPO") + hideEnvDefault(fs, "credential", "GITSYNC_CREDENTIAL") + hideEnvDefault(fs, "askpass-url", "GITSYNC_ASKPASS_URL", "GIT_SYNC_ASKPASS_URL", "GIT_ASKPASS_URL") + hideEnvDefault(fs, "missing", "GITSYNC_REPO") + + if got := fs.Lookup("repo").DefValue; got != "" { + t.Fatalf("repo DefValue = %q", got) + } + if got := fs.Lookup("credential").DefValue; got != "" { + t.Fatalf("credential DefValue = %q", got) + } + if got := fs.Lookup("askpass-url").DefValue; got != "" { + t.Fatalf("empty askpass env was treated as set: %q", got) + } + if got := fs.Lookup("ref").DefValue; got != "HEAD" { + t.Fatalf("unrelated flag rewritten: %q", got) + } + + if err := fs.Parse(nil); err != nil { + t.Fatal(err) + } + gotRepo, err := fs.GetString("repo") + if err != nil { + t.Fatal(err) + } + if gotRepo != secret { + t.Fatalf("repo value = %q, want the env default kept", gotRepo) + } + + usage := fs.FlagUsages() + if strings.Contains(usage, "REPOPAT") || strings.Contains(usage, "SENTINEL_PAT") { + t.Fatalf("usage leaked a secret:\n%s", usage) + } + if !strings.Contains(usage, "") { + t.Fatalf("usage missing placeholder:\n%s", usage) + } + + // credential's zero value stringifies as "[]", which must stay visible + // when the env var is unset. + bare := pflag.NewFlagSet("git-sync", pflag.ContinueOnError) + bare.String("credential", "[]", "credentials") + hideEnvDefault(bare, "credential", "GITSYNC_CREDENTIAL_UNSET") + if got := bare.Lookup("credential").DefValue; got != "[]" { + t.Fatalf("unset credential DefValue = %q", got) + } +} + func TestMakeAbsPath(t *testing.T) { cases := []struct { path string