password option: support separate STDIN prompting per dir/host

Background:
To match the behavior of the standard MySQL client, Skeema's password option
supports the ability to prompt for the password on STDIN if the option is
supplied in a way that explicitly lacks a value on the command line or option
file. Previously, this prompt-on-STDIN behavior required either supplying
--password on the CLI without a value, or having a bare "password" line (with
no equals sign or value) in a *global* option file.

Behavior change in this commit:
This commit permits handling of a bare "password" line to be processed
properly in *any* option file, not just a global one. Skeema commands,
which generally execute recursively from the working directory, will now
prompt for the password whenever operating on a directory whose .skeema file
contains a bare "password" line. The prompt text will indicate which DB host
the password is for (or which directory, if the host is not specified yet).

Once the password is entered, this value will then apply to all subdirs of
that directory as well, unless those directories have a .skeema file which
overrides the password option again in turn.

Practical usage:
When operating on multiple database instances at once (e.g. `skeema diff` from
the top of a schema repo containing distinct schemas for different hosts), it
is now possible to put bare "password" lines in each of the .skeema files at
the host level, and Skeema will prompt for separate passwords for each host.
Previously, this was not possible without placing the plaintext password
values into the .skeema files, which is extremely inadvisable for security
reasons.

Related future directions:
Subsequent commits will add the ability to use arbitrary ENV variables for
options, including the password option. This will provide a non-interactive
alternative for using different passwords for each host, again without
requiring raw passwords in .skeema files.

cmd_diff.go

@@ -29,8 +29,7 @@ func init() {
 // DiffHandler is the handler method for `skeema diff`
 func DiffHandler(cfg *mybase.Config) error {
 	// We just delegate to PushHandler, forcing dry-run to be enabled
-	cfg.CLI.OptionValues["dry-run"] = "1"
-	cfg.MarkDirty()
+	cfg.SetRuntimeOverride("dry-run", "1")
 	return PushHandler(cfg)
 }
 

cmd_format.go

@@ -93,18 +93,25 @@ func formatDir(dir *fs.Dir) error {
 		return NewExitValue(CodeBadConfig, err.Error())
 	}
 
+	wsType, _ := dir.Config.GetEnum("workspace", "temp-schema", "docker")
+	if wsType != "docker" {
+		if err := dir.PromptPasswordIfRequested(); err != nil {
+			log.Warn(err)
+		}
+	}
+
 	// Get workspace options for dir. This involves connecting to the first defined
 	// instance, so that any auto-detect-related settings work properly. However,
 	// with workspace=docker we can ignore connection errors; we'll get reasonable
 	// defaults from workspace.OptionsForDir if inst is nil as long as flavor is set.
 	var wsOpts workspace.Options
 	if len(dir.LogicalSchemas) > 0 {
 		inst, err := dir.FirstInstance()
-		if wsType, _ := dir.Config.GetEnum("workspace", "temp-schema", "docker"); wsType != "docker" || !dir.Config.Changed("flavor") {
+		if wsType != "docker" || !dir.Config.Changed("flavor") {
 			if err != nil {
 				return NewExitValue(CodeBadConfig, err.Error())
 			} else if inst == nil {
-				return NewExitValue(CodeBadConfig, "With workspace=temp-schema, this command needs a host to operate on, but no host is defined for environment %q", dir.Config.Get("environment"))
+				return NewExitValue(CodeBadConfig, "This command needs either a host (with workspace=temp-schema) or flavor (with workspace=docker), but one is not configured for environment %q", dir.Config.Get("environment"))
 			}
 		}
 		if wsOpts, err = workspace.OptionsForDir(dir, inst); err != nil {

cmd_lint.go

@@ -130,18 +130,25 @@ func lintDir(dir *fs.Dir) *linter.Result {
 		return linter.BadConfigResult(dir, err)
 	}
 
+	wsType, _ := dir.Config.GetEnum("workspace", "temp-schema", "docker")
+	if wsType != "docker" {
+		if err := dir.PromptPasswordIfRequested(); err != nil {
+			log.Warn(err)
+		}
+	}
+
 	// Get workspace options for dir. This involves connecting to the first defined
 	// instance, so that any auto-detect-related settings work properly. However,
 	// with workspace=docker we can ignore connection errors; we'll get reasonable
 	// defaults from workspace.OptionsForDir if inst is nil as long as flavor is set.
 	var wsOpts workspace.Options
 	if len(dir.LogicalSchemas) > 0 {
 		inst, err := dir.FirstInstance()
-		if wsType, _ := dir.Config.GetEnum("workspace", "temp-schema", "docker"); wsType != "docker" || !dir.Config.Changed("flavor") {
+		if wsType != "docker" || !dir.Config.Changed("flavor") {
 			if err != nil {
 				return linter.BadConfigResult(dir, err)
 			} else if inst == nil {
-				return linter.BadConfigResult(dir, fmt.Errorf("With workspace=temp-schema, this command needs a host to operate on, but no host is defined for environment %q", dir.Config.Get("environment")))
+				return linter.BadConfigResult(dir, fmt.Errorf("This command needs either a host (with workspace=temp-schema) or flavor (with workspace=docker), but one is not configured for environment %q", dir.Config.Get("environment")))
 			}
 		}
 		if wsOpts, err = workspace.OptionsForDir(dir, inst); err != nil {

cmd_pull.go

@@ -60,6 +60,9 @@ func pullWalker(dir *fs.Dir, maxDepth int) error {
 		log.Warnf("Skipping %s: %s", dir, dir.ParseError)
 		return NewExitValue(CodeBadConfig, "")
 	}
+	if err := dir.PromptPasswordIfRequested(); err != nil {
+		log.Warn(err)
+	}
 
 	var instance *tengo.Instance
 	var err error

go.mod

@@ -12,7 +12,7 @@ require (
 	github.com/mitchellh/go-wordwrap v1.0.0
 	github.com/pmezard/go-difflib v1.0.0
 	github.com/sirupsen/logrus v1.8.1
-	github.com/skeema/mybase v1.0.15
+	github.com/skeema/mybase v1.0.16-0.20221004204712-f777a7f377af
 	golang.org/x/lint v0.0.0-20210508222113-6edffad5e616
 	golang.org/x/sync v0.0.0-20220601150217-0de741cfad7f
 	golang.org/x/term v0.0.0-20210927222741-03fcf44c2211

go.sum

@@ -763,8 +763,8 @@ github.com/sirupsen/logrus v1.6.0/go.mod h1:7uNnSEd1DgxDLC74fIahvMZmmYsHGZGEOFrf
 github.com/sirupsen/logrus v1.7.0/go.mod h1:yWOB1SBYBC5VeMP7gHvWumXLIWorT60ONWic61uBYv0=
 github.com/sirupsen/logrus v1.8.1 h1:dJKuHgqk1NNQlqoA6BTlM1Wf9DOH3NBjQyu0h9+AZZE=
 github.com/sirupsen/logrus v1.8.1/go.mod h1:yWOB1SBYBC5VeMP7gHvWumXLIWorT60ONWic61uBYv0=
-github.com/skeema/mybase v1.0.15 h1:oC0ff0vNJlk4Qr85VHhuXizjiLepZs2MzD9TQVm6kQ0=
-github.com/skeema/mybase v1.0.15/go.mod h1:RJIQU667zLYletaM3hMf8jtmrCaLgJm0OaOFb6DfXe0=
+github.com/skeema/mybase v1.0.16-0.20221004204712-f777a7f377af h1:iR4frsQHxjp2eWgVjX0HIBf4cWYHRKsKBgsURYxKASE=
+github.com/skeema/mybase v1.0.16-0.20221004204712-f777a7f377af/go.mod h1:RJIQU667zLYletaM3hMf8jtmrCaLgJm0OaOFb6DfXe0=
 github.com/smartystreets/assertions v0.0.0-20180927180507-b2de0cb4f26d/go.mod h1:OnSkiWE9lh6wB0YB77sQom3nweQdgAjqCqsofrRNTgc=
 github.com/smartystreets/goconvey v0.0.0-20190330032615-68dc04aab96a/go.mod h1:syvi0/a8iFYH4r/RixwvyeAJjdLS9QV7WQ/tjFTllLA=
 github.com/smartystreets/goconvey v1.6.4/go.mod h1:syvi0/a8iFYH4r/RixwvyeAJjdLS9QV7WQ/tjFTllLA=

internal/applier/target.go

@@ -108,6 +108,9 @@ func TargetsForDir(dir *fs.Dir, maxDepth int) (targets []*Target, skipCount int)
 		log.Errorf("Skipping %s: %s\n", dir.Path, dir.ParseError)
 		return nil, 1
 	}
+	if err := dir.PromptPasswordIfRequested(); err != nil {
+		log.Warn(err)
+	}
 	if dir.Config.Changed("host") && dir.HasSchema() {
 		var instances []*tengo.Instance
 		instances, skipCount = instancesForDir(dir)

internal/fs/dir.go

@@ -675,6 +675,41 @@ func (dir *Dir) Generator() (major, minor, patch int, edition string) {
 	return int(version.Major()), int(version.Minor()), int(version.Patch()), edition
 }
 
+// PromptPasswordIfRequested checks if this dir's configuration indicates a
+// password should be read from STDIN, due to a .skeema file having a bare
+// "password" line with no equals sign or value. If successful, the password
+// will be stored in the directory's configuration as a runtime override. An
+// error is returned if a password should be prompted but cannot, for example
+// due to STDIN not being a TTY.
+func (dir *Dir) PromptPasswordIfRequested() error {
+	// Don't prompt if password option not supplied at all (left at default of no
+	// password) or supplied with some value (even if that value is a blank string,
+	// which also indicates intentionally no password).
+	// TODO replace this with a more efficient check once env var support is
+	// fleshed out and the default for password option is no longer ""
+	if !dir.Config.Supplied("password") || dir.Config.SuppliedWithValue("password") {
+		return nil
+	}
+
+	// Since different dirs/hosts may have different passwords, indicate in the
+	// prompt text which one is being requested
+	var promptArg string
+	if !dir.Config.Changed("host-wrapper") && dir.Config.Changed("host") && !strings.Contains(dir.Config.Get("host"), ",") {
+		promptArg = dir.Config.Get("host")
+		if dir.Config.Changed("port") {
+			promptArg = fmt.Sprintf("%s:%d", promptArg, dir.Config.GetIntOrDefault("port"))
+		}
+	} else {
+		promptArg = "directory " + dir.RelPath()
+	}
+	val, err := util.PromptPassword("Enter password for %s: ", promptArg)
+	if err != nil {
+		return fmt.Errorf("Unable to prompt password for %s: %w", promptArg, err)
+	}
+	dir.Config.SetRuntimeOverride("password", val)
+	return nil
+}
+
 // parseContents reads the .skeema and *.sql files in the dir, populating
 // fields of dir accordingly. This method modifies dir in-place. Any fatal
 // error will populate dir.ParseError.

internal/fs/dir_test.go

@@ -1,6 +1,7 @@
 package fs
 
 import (
+	"fmt"
 	"net/url"
 	"os"
 	"path/filepath"
@@ -600,6 +601,99 @@ func TestAncestorPaths(t *testing.T) {
 	}
 }
 
+func TestPromptPasswordIfRequested(t *testing.T) {
+	defer func() {
+		util.PasswordPromptInput = util.PasswordInputSource(util.NoInteractiveInput)
+	}()
+
+	// If a parent dir .skeema file has a bare "password" line, pw should be
+	// prompted there but not redundantly for its subdirs
+	dir := getDir(t, "testdata/pwprompt/basedir")
+	util.PasswordPromptInput = util.NewMockPasswordInput("basedir")
+	if err := dir.PromptPasswordIfRequested(); err != nil {
+		t.Fatalf("Unexpected error from mock password input: %v", err)
+	} else if actual := dir.Config.Get("password"); actual != "basedir" {
+		t.Errorf("Unexpected configuration for password: %q (expected %q)", actual, "basedir")
+	}
+	util.PasswordPromptInput = util.NewMockPasswordInput("different value to ensure not re-prompted")
+	subdirs, err := dir.Subdirs()
+	if err != nil {
+		t.Fatalf("Unexpected error from Subdirs: %v", err)
+	}
+	for _, subdir := range subdirs {
+		if err := subdir.PromptPasswordIfRequested(); err != nil {
+			t.Fatalf("Unexpected error from mock password input: %v", err)
+		} else if actual := subdir.Config.Get("password"); actual != "basedir" {
+			t.Errorf("Unexpected configuration for password: %q (expected %q)", actual, "basedir")
+		}
+	}
+
+	// If parent dir doesn't have bare "password" but both subdirs do, they should
+	// each prompt password separately
+	dir = getDir(t, "testdata/pwprompt/leafdir")
+	util.PasswordPromptInput = util.NewMockPasswordInput("basedir")
+	if err := dir.PromptPasswordIfRequested(); err != nil {
+		t.Fatalf("Unexpected error from mock password input: %v", err)
+	} else if dir.Config.Changed("password") {
+		t.Errorf("password value unexpectedly changed for base dir %s", dir)
+	}
+	subdirs, err = dir.Subdirs()
+	if err != nil {
+		t.Fatalf("Unexpected error from Subdirs: %v", err)
+	}
+	for n, subdir := range subdirs {
+		leafPassword := fmt.Sprintf("leaf-%d", n)
+		util.PasswordPromptInput = util.NewMockPasswordInput(leafPassword)
+		if err := subdir.PromptPasswordIfRequested(); err != nil {
+			t.Fatalf("Unexpected error from mock password input: %v", err)
+		} else if actual := subdir.Config.Get("password"); actual != leafPassword {
+			t.Errorf("Unexpected configuration for password: %q (expected %q)", actual, leafPassword)
+		}
+	}
+
+	// If an equals sign is present, but no value or an empty string, this means
+	// empty password (not prompt) for compat with MySQL client behavior
+	util.PasswordPromptInput = util.NewMockPasswordInput("this should not show up")
+	dir = getDir(t, "testdata/pwprompt/noprompt/a")
+	if err := dir.PromptPasswordIfRequested(); err != nil {
+		t.Fatalf("Unexpected error from mock password input: %v", err)
+	} else if dir.Config.Changed("password") {
+		t.Errorf("password value unexpectedly changed for dir %s", dir)
+	}
+	dir = getDir(t, "testdata/pwprompt/noprompt/b")
+	if err := dir.PromptPasswordIfRequested(); err != nil {
+		t.Fatalf("Unexpected error from mock password input: %v", err)
+	} else if dir.Config.Changed("password") {
+		t.Errorf("password value unexpectedly changed for dir %s", dir)
+	}
+
+	// The prompt string and error strings should normally contain the host, or
+	// host:port if port is nonstandard, or directory name if no host or multiple
+	// hosts. We cannot directly test the prompt string, but we can confirm it via
+	// error strings.
+	util.PasswordPromptInput = util.PasswordInputSource(util.NoInteractiveInput)
+	dir = getDir(t, "testdata/pwprompt/basedir") // no host
+	if err := dir.PromptPasswordIfRequested(); err == nil || !strings.Contains(err.Error(), "testdata/pwprompt/basedir") {
+		t.Errorf("Error value did not contain expected string: %v", err)
+	}
+	dir = getDir(t, "testdata/pwprompt/leafdir/a") // host, and port, but port is set to 3306 which shouldn't display
+	if err := dir.PromptPasswordIfRequested(); err == nil || !strings.Contains(err.Error(), "1.2.3.4") || strings.Contains(err.Error(), ":3306") {
+		t.Errorf("Error value did not contain expected string: %v", err)
+	}
+	dir = getDir(t, "testdata/pwprompt/leafdir/b") // host, and non-3306 port
+	if err := dir.PromptPasswordIfRequested(); err == nil || !strings.Contains(err.Error(), "1.2.3.4:3307") {
+		t.Errorf("Error value did not contain expected string: %v", err)
+	}
+	dir = getDir(t, "testdata/pwprompt/multihost/a") // multiple hosts comma-separated
+	if err := dir.PromptPasswordIfRequested(); err == nil || !strings.Contains(err.Error(), "testdata/pwprompt/multihost/a") {
+		t.Errorf("Error value did not contain expected string: %v", err)
+	}
+	dir = getDir(t, "testdata/pwprompt/multihost/b") // using host-wrapper
+	if err := dir.PromptPasswordIfRequested(); err == nil || !strings.Contains(err.Error(), "testdata/pwprompt/multihost/b") {
+		t.Errorf("Error value did not contain expected string: %v", err)
+	}
+}
+
 func getValidConfigWithCLI(t *testing.T, cliOptions string) *mybase.Config {
 	t.Helper()
 	cmd := mybase.NewCommand("fstest", "", "", nil)

internal/fs/testdata/pwprompt/basedir/.skeema

@@ -0,0 +1,2 @@
+# should prompt for password once in this parent dir
+password

internal/fs/testdata/pwprompt/basedir/a/.skeema

@@ -0,0 +1 @@
+host=1.2.3.4

internal/fs/testdata/pwprompt/basedir/b/.skeema

@@ -0,0 +1 @@
+host=5.6.7.8

internal/fs/testdata/pwprompt/leafdir/.skeema

@@ -0,0 +1 @@
+host=1.2.3.4

internal/fs/testdata/pwprompt/leafdir/a/.skeema

@@ -0,0 +1,5 @@
+# should prompt for password in this dir
+schema=a
+user=a
+port=3306
+password

internal/fs/testdata/pwprompt/leafdir/b/.skeema

@@ -0,0 +1,5 @@
+# should prompt for password in this dir
+schema=b
+user=b
+port=3307
+password

internal/fs/testdata/pwprompt/multihost/a/.skeema

@@ -0,0 +1,2 @@
+host=1.2.3.4,5.6.7.8
+password

internal/fs/testdata/pwprompt/multihost/b/.skeema

@@ -0,0 +1,3 @@
+host-wrapper=/usr/local/bin/mywrapper.sh
+host=foo
+password

internal/fs/testdata/pwprompt/noprompt/a/.skeema

@@ -0,0 +1,2 @@
+password=
+host=1.2.3.4

internal/fs/testdata/pwprompt/noprompt/b/.skeema

@@ -0,0 +1,2 @@
+password=''
+host=5.6.7.8

internal/util/config.go

@@ -113,16 +113,14 @@ func ProcessSpecialGlobalOptions(cfg *mybase.Config) error {
 	// STDIN like mysql client does.
 	if !cfg.Supplied("password") {
 		if val := os.Getenv("MYSQL_PWD"); val != "" {
-			cfg.CLI.OptionValues["password"] = val
-			cfg.MarkDirty()
+			cfg.SetRuntimeOverride("password", val)
 		}
 	} else if !cfg.SuppliedWithValue("password") {
-		var err error
-		cfg.CLI.OptionValues["password"], err = PromptPassword()
-		cfg.MarkDirty()
+		val, err := PromptPassword()
 		if err != nil {
 			return err
 		}
+		cfg.SetRuntimeOverride("password", val)
 	}
 
 	if cfg.GetBool("debug") {
@@ -132,6 +130,47 @@ func ProcessSpecialGlobalOptions(cfg *mybase.Config) error {
 	return nil
 }
 
+// PasswordInputSource is a function that can be used to obtain a password
+// interactively.
+type PasswordInputSource func() (string, error)
+
+// InteractivePasswordInput reads a password from STDIN. This only works if
+// STDIN is a terminal.
+func InteractivePasswordInput() (string, error) {
+	stdin := int(os.Stdin.Fd())
+	bytePassword, err := terminal.ReadPassword(stdin)
+	return string(bytePassword), err
+}
+
+// NoInteractiveInput always returns an error instead of attempting to read a
+// password.
+func NoInteractiveInput() (string, error) {
+	return "", errors.New("STDIN must be a TTY to read password")
+}
+
+// NewMockPasswordInput returns a PasswordInputSource function which always
+// returns the specified string.
+func NewMockPasswordInput(mockPassword string) PasswordInputSource {
+	return PasswordInputSource(func() (string, error) {
+		fmt.Fprintf(os.Stderr, strings.Repeat("*", len(mockPassword)))
+		return mockPassword, nil
+	})
+}
+
+// PasswordPromptInput is the input source used by PromptPassword to obtain a
+// password interactively, or to mock such an input for testing purposes.
+var PasswordPromptInput PasswordInputSource
+
+func init() {
+	// Don't attempt interactive password prompt if STDIN isn't a TTY, or if
+	// running a test suite
+	if !terminal.IsTerminal(int(os.Stdin.Fd())) || strings.HasSuffix(os.Args[0], ".test") || strings.HasSuffix(os.Args[0], ".test.exe") {
+		PasswordPromptInput = PasswordInputSource(NoInteractiveInput)
+	} else {
+		PasswordPromptInput = PasswordInputSource(InteractivePasswordInput)
+	}
+}
+
 // PromptPassword reads a password from STDIN without echoing the typed
 // characters. Requires that STDIN is a TTY. Optionally supply args to build
 // a custom prompt string; first arg must be a string if so, with args behaving
@@ -142,24 +181,15 @@ func PromptPassword(promptArgs ...interface{}) (string, error) {
 	if len(promptArgs) == 0 {
 		promptArgs = append(promptArgs, "Enter password: ")
 	}
-	stdin := int(os.Stdin.Fd())
-	if !terminal.IsTerminal(stdin) {
-		return "", errors.New("STDIN must be a TTY to read password")
-	}
 
 	w := os.Stderr
 	if !terminal.IsTerminal(int(w.Fd())) && terminal.IsTerminal(int(os.Stdout.Fd())) {
 		w = os.Stdout
 	}
 	fmt.Fprintf(w, promptArgs[0].(string), promptArgs[1:]...)
-
-	bytePassword, err := terminal.ReadPassword(stdin)
-	if err != nil {
-		return "", err
-	}
-
-	fmt.Fprintln(w) // since ReadPassword also won't echo the ENTER key as a newline!
-	return string(bytePassword), nil
+	pw, err := PasswordPromptInput()
+	fmt.Fprintln(w) // since password input funcs won't echo the ENTER key as a newline
+	return pw, err
 }
 
 // SplitConnectOptions takes a string containing a comma-separated list of