From 4597da99c85675a1d6d45e8b1d706582512307f4 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Sat, 8 Jun 2024 18:28:32 -0700 Subject: [PATCH] Add the idea of "env-flags" Env-flags are "flags" that can only be set by env var (see caveat below). All of the real flags have a corresponding env-flag (kind of, but not really). The real goal was to deprecate `--password` but keep the env var as a documented interface. This does that (though --password still works) and updates the usage and manual. This allows some future work to follow the pattern. We do not register every CLI flag as an env-flag because the help text would be duplicative. This probably wants a wrapper API that allows declaring of abstract flags, with CLI, env, or both sources. Caveat: ACTUALLY, these still have a flag, but the flag is specially named and hidden. This makes testing a little easier where passing flags is handled well but env vars is not. --- README.md | 52 +++++++++++++-------------- env.go | 101 ++++++++++++++++++++++++++++++++++++++++++++++++---- main.go | 92 +++++++++++++++++++++++++++++------------------ test_e2e.sh | 6 ++-- 4 files changed, 181 insertions(+), 70 deletions(-) diff --git a/README.md b/README.md index bcd6b335f..7f67c785f 100644 --- a/README.md +++ b/README.md @@ -143,7 +143,8 @@ OPTIONS Many options can be specified as either a commandline flag or an environment variable, but flags are preferred because a misspelled flag is a fatal - error while a misspelled environment variable is silently ignored. + error while a misspelled environment variable is silently ignored. Some + options can only be specified as an environment variable. --add-user, $GITSYNC_ADD_USER Add a record to /etc/passwd for the current UID/GID. This is @@ -161,11 +162,12 @@ OPTIONS --credential , $GITSYNC_CREDENTIAL Make one or more credentials available for authentication (see git - help credential). This is similar to --username and --password or - --password-file, but for specific URLs, for example when using - submodules. The value for this flag is either a JSON-encoded - object (see the schema below) or a JSON-encoded list of that same - object type. This flag may be specified more than once. + help credential). This is similar to --username and + $GITSYNC_PASSWORD or --password-file, but for specific URLs, for + example when using submodules. The value for this flag is either a + JSON-encoded object (see the schema below) or a JSON-encoded list + of that same object type. This flag may be specified more than + once. Object schema: - url: string, required @@ -294,16 +296,14 @@ OPTIONS --one-time, $GITSYNC_ONE_TIME Exit after one sync. - --password , $GITSYNC_PASSWORD + $GITSYNC_PASSWORD The password or personal access token (see github docs) to use for - git authentication (see --username). NOTE: for security reasons, - users should prefer --password-file or $GITSYNC_PASSWORD_FILE for - specifying the password. + git authentication (see --username). See also --password-file. --password-file , $GITSYNC_PASSWORD_FILE The file from which the password or personal access token (see github docs) to use for git authentication (see --username) will be - read. + read. See also $GITSYNC_PASSWORD. --period , $GITSYNC_PERIOD How long to wait between sync attempts. This must be at least @@ -376,8 +376,8 @@ OPTIONS --username , $GITSYNC_USERNAME The username to use for git authentication (see --password-file or - --password). If more than one username and password is required - (e.g. with submodules), use --credential. + $GITSYNC_PASSWORD). If more than one username and password is + required (e.g. with submodules), use --credential. -v, --verbose , $GITSYNC_VERBOSE Set the log verbosity level. Logs at this level and lower will be @@ -435,31 +435,31 @@ AUTHENTICATION and "git@example.com:repo" will try to use SSH. username/password - The --username (GITSYNC_USERNAME) and --password-file - (GITSYNC_PASSWORD_FILE) or --password (GITSYNC_PASSWORD) flags - will be used. To prevent password leaks, the --password-file flag - or GITSYNC_PASSWORD environment variable is almost always - preferred to the --password flag. + The --username ($GITSYNC_USERNAME) and $GITSYNC_PASSWORD or + --password-file ($GITSYNC_PASSWORD_FILE) flags will be used. To + prevent password leaks, the --password-file flag or + $GITSYNC_PASSWORD environment variable is almost always preferred + to the --password flag, which is deprecated. - A variant of this is --askpass-url (GITSYNC_ASKPASS_URL), which + A variant of this is --askpass-url ($GITSYNC_ASKPASS_URL), which consults a URL (e.g. http://metadata) to get credentials on each sync. When using submodules it may be necessary to specify more than one username and password, which can be done with --credential - (GITSYNC_CREDENTIAL). All of the username+password pairs, from - both --username/--password and --credential are fed into 'git - credential approve'. + ($GITSYNC_CREDENTIAL). All of the username+password pairs, from + both --username/$GITSYNC_PASSWORD and --credential are fed into + 'git credential approve'. SSH When an SSH transport is specified, the key(s) defined in - --ssh-key-file (GITSYNC_SSH_KEY_FILE) will be used. Users are + --ssh-key-file ($GITSYNC_SSH_KEY_FILE) will be used. Users are strongly advised to also use --ssh-known-hosts - (GITSYNC_SSH_KNOWN_HOSTS) and --ssh-known-hosts-file - (GITSYNC_SSH_KNOWN_HOSTS_FILE) when using SSH. + ($GITSYNC_SSH_KNOWN_HOSTS) and --ssh-known-hosts-file + ($GITSYNC_SSH_KNOWN_HOSTS_FILE) when using SSH. cookies - When --cookie-file (GITSYNC_COOKIE_FILE) is specified, the + When --cookie-file ($GITSYNC_COOKIE_FILE) is specified, the associated cookies can contain authentication information. HOOKS diff --git a/env.go b/env.go index cbf1fc0d6..80c64d7a9 100644 --- a/env.go +++ b/env.go @@ -17,11 +17,16 @@ limitations under the License. package main import ( + "cmp" "fmt" + "io" "os" + "slices" "strconv" "strings" "time" + + "github.com/spf13/pflag" ) func envString(def string, key string, alts ...string) string { @@ -30,12 +35,25 @@ func envString(def string, key string, alts ...string) string { } for _, alt := range alts { if val := os.Getenv(alt); val != "" { - fmt.Fprintf(os.Stderr, "env %s has been deprecated, use %s instead\n", alt, key) + fmt.Fprintf(os.Stderr, "env $%s has been deprecated, use $%s instead\n", alt, key) return val } } return def } +func envFlagString(key string, def string, usage string, alts ...string) *string { + registerEnvFlag(key, "string", usage) + val := envString(def, key, alts...) + // also expose it as a flag, for easier testing + flName := "__env__" + key + flHelp := "DO NOT SET THIS FLAG EXCEPT IN TESTS; use $" + key + newExplicitFlag(&val, flName, flHelp, pflag.String) + if err := pflag.CommandLine.MarkHidden(flName); err != nil { + fmt.Fprintf(os.Stderr, "FATAL: %v\n", err) + os.Exit(1) + } + return &val +} func envStringArray(def string, key string, alts ...string) []string { parse := func(s string) []string { @@ -47,7 +65,7 @@ func envStringArray(def string, key string, alts ...string) []string { } for _, alt := range alts { if val := os.Getenv(alt); val != "" { - fmt.Fprintf(os.Stderr, "env %s has been deprecated, use %s instead\n", alt, key) + fmt.Fprintf(os.Stderr, "env $%s has been deprecated, use $%s instead\n", alt, key) return parse(val) } } @@ -68,7 +86,7 @@ func envBoolOrError(def bool, key string, alts ...string) (bool, error) { } for _, alt := range alts { if val := os.Getenv(alt); val != "" { - fmt.Fprintf(os.Stderr, "env %s has been deprecated, use %s instead\n", alt, key) + fmt.Fprintf(os.Stderr, "env $%s has been deprecated, use $%s instead\n", alt, key) return parse(alt, val) } } @@ -98,7 +116,7 @@ func envIntOrError(def int, key string, alts ...string) (int, error) { } for _, alt := range alts { if val := os.Getenv(alt); val != "" { - fmt.Fprintf(os.Stderr, "env %s has been deprecated, use %s instead\n", alt, key) + fmt.Fprintf(os.Stderr, "env $%s has been deprecated, use $%s instead\n", alt, key) return parse(alt, val) } } @@ -128,7 +146,7 @@ func envFloatOrError(def float64, key string, alts ...string) (float64, error) { } for _, alt := range alts { if val := os.Getenv(alt); val != "" { - fmt.Fprintf(os.Stderr, "env %s has been deprecated, use %s instead\n", alt, key) + fmt.Fprintf(os.Stderr, "env $%s has been deprecated, use $%s instead\n", alt, key) return parse(alt, val) } } @@ -158,7 +176,7 @@ func envDurationOrError(def time.Duration, key string, alts ...string) (time.Dur } for _, alt := range alts { if val := os.Getenv(alt); val != "" { - fmt.Fprintf(os.Stderr, "env %s has been deprecated, use %s instead\n", alt, key) + fmt.Fprintf(os.Stderr, "env $%s has been deprecated, use $%s instead\n", alt, key) return parse(alt, val) } } @@ -173,3 +191,74 @@ func envDuration(def time.Duration, key string, alts ...string) time.Duration { } return val } + +// explicitFlag is a pflag.Value which only sets the real value if the flag is +// set to a non-zero-value. +type explicitFlag[T comparable] struct { + pflag.Value + realPtr *T + flagPtr *T +} + +// newExplicitFlag allocates an explicitFlag +func newExplicitFlag[T comparable](ptr *T, name, usage string, fn func(name string, value T, usage string) *T) { + h := &explicitFlag[T]{ + realPtr: ptr, + } + var zero T + h.flagPtr = fn(name, zero, usage) + fl := pflag.CommandLine.Lookup(name) + // wrap the original pflag.Value with our own + h.Value = fl.Value + fl.Value = h +} + +func (h *explicitFlag[T]) Set(val string) error { + if err := h.Value.Set(val); err != nil { + return err + } + var zero T + if v := *h.flagPtr; v != zero { + *h.realPtr = v + } + return nil +} + +// envFlag is like a flag in that it is declared with a type, validated, and +// shows up in help messages, but can only be set by env-var, not on the CLI. +// This is useful for things like passwords, which should not be on the CLI +// because it can be seen in `ps`. +type envFlag struct { + name string + typ string + help string +} + +var allEnvFlags = []envFlag{} + +// registerEnvFlag is internal. Use functions like envFlagString to actually +// create envFlags. +func registerEnvFlag(name, typ, help string) { + for _, ef := range allEnvFlags { + if ef.name == name { + fmt.Fprintf(os.Stderr, "FATAL: duplicate env var declared: %q\n", name) + os.Exit(1) + } + } + allEnvFlags = append(allEnvFlags, envFlag{name, typ, help}) +} + +// printEnvFlags prints "usage" for all registered envFlags. +func printEnvFlags(out io.Writer) { + width := 0 + for _, ef := range allEnvFlags { + if n := len(ef.name); n > width { + width = n + } + } + slices.SortFunc(allEnvFlags, func(l, r envFlag) int { return cmp.Compare(l.name, r.name) }) + + for _, ef := range allEnvFlags { + fmt.Fprintf(out, "% *s %s %*s%s\n", width+2, ef.name, ef.typ, max(8, 32-width), "", ef.help) + } +} diff --git a/main.go b/main.go index a93ffc4db..99d8500d6 100644 --- a/main.go +++ b/main.go @@ -141,7 +141,7 @@ func main() { flVersion := pflag.Bool("version", false, "print the version and exit") flHelp := pflag.BoolP("help", "h", false, "print help text and exit") - pflag.BoolVarP(flHelp, "__?", "?", false, "print help text and exit") // support -? as an alias to -h + pflag.BoolVarP(flHelp, "__?", "?", false, "") // support -? as an alias to -h mustMarkHidden("__?") flManual := pflag.Bool("man", false, "print the full manual and exit") @@ -230,9 +230,9 @@ func main() { flUsername := pflag.String("username", envString("", "GITSYNC_USERNAME", "GIT_SYNC_USERNAME"), "the username to use for git auth") - flPassword := pflag.String("password", - envString("", "GITSYNC_PASSWORD", "GIT_SYNC_PASSWORD"), - "the password or personal access token to use for git auth (prefer --password-file or this env var)") + flPassword := envFlagString("GITSYNC_PASSWORD", "", + "the password or personal access token to use for git auth", + "GIT_SYNC_PASSWORD") flPasswordFile := pflag.String("password-file", envString("", "GITSYNC_PASSWORD_FILE", "GIT_SYNC_PASSWORD_FILE"), "the file from which the password or personal access token for git auth will be sourced") @@ -280,30 +280,43 @@ func main() { flDeprecatedBranch := pflag.String("branch", envString("", "GIT_SYNC_BRANCH"), "DEPRECATED: use --ref instead") mustMarkDeprecated("branch", "use --ref instead") + flDeprecatedChmod := pflag.Int("change-permissions", envInt(0, "GIT_SYNC_PERMISSIONS"), "DEPRECATED: use --group-write instead") mustMarkDeprecated("change-permissions", "use --group-write instead") + flDeprecatedDest := pflag.String("dest", envString("", "GIT_SYNC_DEST"), "DEPRECATED: use --link instead") mustMarkDeprecated("dest", "use --link instead") + flDeprecatedMaxSyncFailures := pflag.Int("max-sync-failures", envInt(0, "GIT_SYNC_MAX_SYNC_FAILURES"), "DEPRECATED: use --max-failures instead") mustMarkDeprecated("max-sync-failures", "use --max-failures instead") + + flDeprecatedPassword := pflag.String("password", "", // the env vars are not deprecated + "DEPRECATED: use --password-file or $GITSYNC_PASSWORD instead") + mustMarkDeprecated("password", "use --password-file or $GITSYNC_PASSWORD instead") + flDeprecatedRev := pflag.String("rev", envString("", "GIT_SYNC_REV"), "DEPRECATED: use --ref instead") mustMarkDeprecated("rev", "use --ref instead") + _ = pflag.Bool("ssh", false, "DEPRECATED: this flag is no longer necessary") mustMarkDeprecated("ssh", "no longer necessary") + flDeprecatedSyncHookCommand := pflag.String("sync-hook-command", envString("", "GIT_SYNC_HOOK_COMMAND"), "DEPRECATED: use --exechook-command instead") mustMarkDeprecated("sync-hook-command", "use --exechook-command instead") + flDeprecatedTimeout := pflag.Int("timeout", envInt(0, "GIT_SYNC_TIMEOUT"), "DEPRECATED: use --sync-timeout instead") mustMarkDeprecated("timeout", "use --sync-timeout instead") + flDeprecatedV := pflag.Int("v", -1, "DEPRECATED: use -v or --verbose instead") mustMarkDeprecated("v", "use -v or --verbose instead") + flDeprecatedWait := pflag.Float64("wait", envFloat(0, "GIT_SYNC_WAIT"), "DEPRECATED: use --period instead") mustMarkDeprecated("wait", "use --period instead") @@ -316,9 +329,14 @@ func main() { if msg != "" { fmt.Fprintln(out, msg) } - fmt.Fprintln(out, "Usage:") + fmt.Fprintf(out, "Usage: %s [FLAGS...]\n", filepath.Base(os.Args[0])) + fmt.Fprintln(out, "") + fmt.Fprintln(out, " FLAGS:") pflag.CommandLine.SetOutput(out) pflag.PrintDefaults() + fmt.Fprintln(out, "") + fmt.Fprintln(out, " ENVIRONMENT VARIABLES:") + printEnvFlags(out) if msg != "" { fmt.Fprintln(out, msg) } @@ -492,12 +510,16 @@ func main() { } } + if *flDeprecatedPassword != "" { + log.V(0).Info("setting $GITSYNC_PASSWORD from deprecated --password") + *flPassword = *flDeprecatedPassword + } if *flUsername != "" { if *flPassword == "" && *flPasswordFile == "" { - fatalConfigError(log, true, "required flag: --password or --password-file must be specified when --username is specified") + fatalConfigError(log, true, "required flag: $GITSYNC_PASSWORD or --password-file must be specified when --username is specified") } if *flPassword != "" && *flPasswordFile != "" { - fatalConfigError(log, true, "invalid flag: only one of --password and --password-file may be specified") + fatalConfigError(log, true, "invalid flag: only one of $GITSYNC_PASSWORD and --password-file may be specified") } if u, err := url.Parse(*flRepo); err == nil { // it may not even parse as a URL, that's OK if u.User != nil { @@ -506,7 +528,7 @@ func main() { } } else { if *flPassword != "" { - fatalConfigError(log, true, "invalid flag: --password may only be specified when --username is specified") + fatalConfigError(log, true, "invalid flag: $GITSYNC_PASSWORD may only be specified when --username is specified") } if *flPasswordFile != "" { fatalConfigError(log, true, "invalid flag: --password-file may only be specified when --username is specified") @@ -1807,7 +1829,7 @@ func (git *repoSync) SetupGitSSH(setupKnownHosts bool, pathsToSSHSecrets []strin sshCmd += " -o StrictHostKeyChecking=no" } - git.log.V(9).Info("setting GIT_SSH_COMMAND", "value", sshCmd) + git.log.V(9).Info("setting $GIT_SSH_COMMAND", "value", sshCmd) if err := os.Setenv("GIT_SSH_COMMAND", sshCmd); err != nil { return fmt.Errorf("can't set $GIT_SSH_COMMAND: %w", err) } @@ -2151,7 +2173,8 @@ OPTIONS Many options can be specified as either a commandline flag or an environment variable, but flags are preferred because a misspelled flag is a fatal - error while a misspelled environment variable is silently ignored. + error while a misspelled environment variable is silently ignored. Some + options can only be specified as an environment variable. --add-user, $GITSYNC_ADD_USER Add a record to /etc/passwd for the current UID/GID. This is @@ -2169,11 +2192,12 @@ OPTIONS --credential , $GITSYNC_CREDENTIAL Make one or more credentials available for authentication (see git - help credential). This is similar to --username and --password or - --password-file, but for specific URLs, for example when using - submodules. The value for this flag is either a JSON-encoded - object (see the schema below) or a JSON-encoded list of that same - object type. This flag may be specified more than once. + help credential). This is similar to --username and + $GITSYNC_PASSWORD or --password-file, but for specific URLs, for + example when using submodules. The value for this flag is either a + JSON-encoded object (see the schema below) or a JSON-encoded list + of that same object type. This flag may be specified more than + once. Object schema: - url: string, required @@ -2302,16 +2326,14 @@ OPTIONS --one-time, $GITSYNC_ONE_TIME Exit after one sync. - --password , $GITSYNC_PASSWORD + $GITSYNC_PASSWORD The password or personal access token (see github docs) to use for - git authentication (see --username). NOTE: for security reasons, - users should prefer --password-file or $GITSYNC_PASSWORD_FILE for - specifying the password. + git authentication (see --username). See also --password-file. --password-file , $GITSYNC_PASSWORD_FILE The file from which the password or personal access token (see github docs) to use for git authentication (see --username) will be - read. + read. See also $GITSYNC_PASSWORD. --period , $GITSYNC_PERIOD How long to wait between sync attempts. This must be at least @@ -2384,8 +2406,8 @@ OPTIONS --username , $GITSYNC_USERNAME The username to use for git authentication (see --password-file or - --password). If more than one username and password is required - (e.g. with submodules), use --credential. + $GITSYNC_PASSWORD). If more than one username and password is + required (e.g. with submodules), use --credential. -v, --verbose , $GITSYNC_VERBOSE Set the log verbosity level. Logs at this level and lower will be @@ -2443,31 +2465,31 @@ AUTHENTICATION and "git@example.com:repo" will try to use SSH. username/password - The --username (GITSYNC_USERNAME) and --password-file - (GITSYNC_PASSWORD_FILE) or --password (GITSYNC_PASSWORD) flags - will be used. To prevent password leaks, the --password-file flag - or GITSYNC_PASSWORD environment variable is almost always - preferred to the --password flag. + The --username ($GITSYNC_USERNAME) and $GITSYNC_PASSWORD or + --password-file ($GITSYNC_PASSWORD_FILE) flags will be used. To + prevent password leaks, the --password-file flag or + $GITSYNC_PASSWORD environment variable is almost always preferred + to the --password flag, which is deprecated. - A variant of this is --askpass-url (GITSYNC_ASKPASS_URL), which + A variant of this is --askpass-url ($GITSYNC_ASKPASS_URL), which consults a URL (e.g. http://metadata) to get credentials on each sync. When using submodules it may be necessary to specify more than one username and password, which can be done with --credential - (GITSYNC_CREDENTIAL). All of the username+password pairs, from - both --username/--password and --credential are fed into 'git - credential approve'. + ($GITSYNC_CREDENTIAL). All of the username+password pairs, from + both --username/$GITSYNC_PASSWORD and --credential are fed into + 'git credential approve'. SSH When an SSH transport is specified, the key(s) defined in - --ssh-key-file (GITSYNC_SSH_KEY_FILE) will be used. Users are + --ssh-key-file ($GITSYNC_SSH_KEY_FILE) will be used. Users are strongly advised to also use --ssh-known-hosts - (GITSYNC_SSH_KNOWN_HOSTS) and --ssh-known-hosts-file - (GITSYNC_SSH_KNOWN_HOSTS_FILE) when using SSH. + ($GITSYNC_SSH_KNOWN_HOSTS) and --ssh-known-hosts-file + ($GITSYNC_SSH_KNOWN_HOSTS_FILE) when using SSH. cookies - When --cookie-file (GITSYNC_COOKIE_FILE) is specified, the + When --cookie-file ($GITSYNC_COOKIE_FILE) is specified, the associated cookies can contain authentication information. HOOKS diff --git a/test_e2e.sh b/test_e2e.sh index 6b294f5ff..15796ee19 100755 --- a/test_e2e.sh +++ b/test_e2e.sh @@ -1784,7 +1784,7 @@ function e2e::auth_http_password() { --root="$ROOT" \ --link="link" \ --username="wrong" \ - --password="testpass" + --__env__GITSYNC_PASSWORD="testpass" assert_file_absent "$ROOT/link/file" # Try with wrong password @@ -1795,7 +1795,7 @@ function e2e::auth_http_password() { --root="$ROOT" \ --link="link" \ --username="testuser" \ - --password="wrong" + --__env__GITSYNC_PASSWORD="wrong" assert_file_absent "$ROOT/link/file" # Try with the right password @@ -1805,7 +1805,7 @@ function e2e::auth_http_password() { --root="$ROOT" \ --link="link" \ --username="testuser" \ - --password="testpass" \ + --__env__GITSYNC_PASSWORD="testpass" \ assert_link_exists "$ROOT/link" assert_file_exists "$ROOT/link/file"