Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

TOOLS-2779: Add --config option for password values #62

Merged
merged 4 commits into from Dec 14, 2020
Merged

TOOLS-2779: Add --config option for password values #62

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9b1ecd8
TOOLS-2779: Add --config option for password values
edobranov Nov 24, 2020
751694f
TOOLS-2779: Uncomment test type check
edobranov Nov 24, 2020
6f7b550
TOOLS-2779: Minor adjustments
edobranov Nov 25, 2020
8d06a63
TOOLS-2779: Use opts.parser instead of manual parsing
edobranov Dec 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions Gopkg.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions Gopkg.toml
View file
Open in desktop
Expand Up @@ -32,6 +32,10 @@
name = "github.com/jessevdk/go-flags"
version = "^v1.4.0"

[[constraint]]
name = "gopkg.in/yaml.v2"
version = "^v2.3.0"

[[constraint]]
name = "github.com/smartystreets/goconvey"
revision = "bf58a9a1291224109919756b4dcc469c670cc7e4"
Expand Down
71 changes: 67 additions & 4 deletions options/options.go
View file
Open in desktop
Expand Up @@ -10,6 +10,7 @@ package options

import (
"fmt"
"io/ioutil"
"os"
"regexp"
"runtime"
Expand All @@ -21,9 +22,11 @@ import (
"github.com/mongodb/mongo-tools-common/failpoint"
"github.com/mongodb/mongo-tools-common/log"
"github.com/mongodb/mongo-tools-common/util"
"github.com/pkg/errors"
"go.mongodb.org/mongo-driver/mongo/readpref"
"go.mongodb.org/mongo-driver/mongo/writeconcern"
"go.mongodb.org/mongo-driver/x/mongo/driver/connstring"
"gopkg.in/yaml.v2"
)

// XXX Force these true as the Go driver supports them always. Once the
Expand Down Expand Up @@ -107,8 +110,9 @@ func (ns Namespace) String() string {

// Struct holding generic options
type General struct {
Help bool `long:"help" description:"print usage"`
Version bool `long:"version" description:"print the tool version and exit"`
Help bool `long:"help" description:"print usage"`
Version bool `long:"version" description:"print the tool version and exit"`
ConfigPath string `long:"config" description:"path to a configuration file with password secrets"`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering do we need with password secrets since later we could introduce more fields inside?

Copy link
Collaborator Author

@edobranov edobranov Dec 3, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I don't think we really need it either. We could also drop that phrase later on when we do support more/all fields, but I'm not too attached to it. I can go ahead and remove it if @rychipman agrees.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think removing it makes sense!


MaxProcs int `long:"numThreads" hidden:"true"`
Failpoints string `long:"failpoints" hidden:"true"`
Expand Down Expand Up @@ -423,9 +427,14 @@ func (opts *ToolOptions) AddOptions(extraOpts ExtraOptions) {
}
}

// Parse the command line args. Returns any extra args not accounted for by
// parsing, as well as an error if the parsing returns an error.
// ParseArgs parses a potential config file followed by the command line args, overriding
// any values in the config file. Returns any extra args not accounted for by parsing,
// as well as an error if the parsing returns an error.
func (opts *ToolOptions) ParseArgs(args []string) ([]string, error) {
if err := opts.ParseConfigFile(args); err != nil {
return []string{}, err
}

args, err := opts.parser.ParseArgs(args)
if err != nil {
return []string{}, err
Expand All @@ -452,6 +461,60 @@ func (opts *ToolOptions) ParseArgs(args []string) ([]string, error) {
return args, err
}

// ParseConfigFile iterates over args to find a --config option. If not found, we return.
// If found, we read the contents of the specified config file in YAML format. We parse
// any values corresponding to --password, --uri and --sslPEMKeyPassword, and store them
// in the opts.
func (opts *ToolOptions) ParseConfigFile(args []string) error {
// Get config file path from the arguments, if specified.
configPath := ""
for i, arg := range args {
if strings.HasPrefix(arg, "--config=") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there going to be a difference between --config=foo and --config="foo"? Will the latter make the config path be \"foo\"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, interesting point. It seems the underlying call to os.Open() is smart enough to recognize that the surrounding quotation marks aren't intended unless they're explicitly escaped. So there should be no difference.

From trying it on the command line as-is:

$ echo "uri: mongodb://abc:@localhost:33333" > config.yaml

$ mongodump --config=config.yaml
Enter password:

$ mongodump --config="config.yaml"
Enter password:

$ mongodump --config=\"config.yaml\"
2020-11-25T14:19:02.953-0500	error parsing command line options: open "config.yaml": no such file or directory

$ mongodump --config="\"config.yaml\""
2020-11-25T14:19:12.025-0500	error parsing command line options: open "config.yaml": no such file or directory

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's the call to os.Open -- my understanding is that the shell consumes the quotes, so they are never passed as args in this specific example.

I think there are other issues here though. For example, does go-flags allow /-style flags on windows? If so, that's a whole separate class of thing that we have to manually check for here.

As I have noted in another PR, I'm generally skeptical of trying to match the library's flag-parsing behavior by manually scanning arguments

// No value after equal sign.
if len(arg) < 10 {
return fmt.Errorf("--config option specified but no file given")
}
configPath = arg[9:]
break
} else if arg == "--config" {
// No next argument.
if i+1 == len(args) {
return fmt.Errorf("--config option specified but no file given")
}
configPath = args[i+1]
break
}
}

// No --config option was found.
if configPath == "" {
return nil
}

configBytes, err := ioutil.ReadFile(configPath)
if err != nil {
return errors.Wrapf(err, "error opening file with --config")
}

// Unmarshal the config file as a top-level YAML file.
var config struct {
Password string `yaml:"password"`
ConnectionString string `yaml:"uri"`
SSLPEMKeyPassword string `yaml:"sslPEMKeyPassword"`
}
err = yaml.UnmarshalStrict(configBytes, &config)
if err != nil {
return errors.Wrapf(err, "error parsing config file %s", configPath)
}

// Assign each parsed value to its respective ToolOptions field.
opts.Auth.Password = config.Password
opts.URI.ConnectionString = config.ConnectionString
opts.SSL.SSLPEMKeyPassword = config.SSLPEMKeyPassword
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To bring up a comment I made in the tech design a while ago -- before storing each field here, we could actually check to see if the fields were already set by the call to opts.parser.ParseArgs() at the beginning of this func. This would allow us to log a message that a config file's value will be overridden by the respective command line option.

Might offer a bit of transparency but I don't feel strongly either way.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't recommend doing that because the fields aren't necessarily directly set inside cmd parameters, for example, the password could be set inside connectionString. In that case, you won't be able to catch the overridden.
If you really wanna log the warning message, this function should return a separate ToolOptions object and later used as the comparison with the ParseArgs() result.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, it's true we wouldn't know if a password comes from --uri vs. a positional connection string, but would that matter a lot?

It might be enough to just say something like warning: connection string in config file will be overridden by connection string on command line.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we already decided this wasn't a requirement for this project, I would also recommend that we not do this


return nil
}

func (opts *ToolOptions) setURIFromPositionalArg(args []string) ([]string, error) {
newArgs := []string{}
var foundURI bool
Expand Down
153 changes: 153 additions & 0 deletions options/options_test.go
View file
Open in desktop
Expand Up @@ -9,6 +9,7 @@ package options
import (
"bytes"
"fmt"
"io/ioutil"
"os"
"strings"

Expand Down Expand Up @@ -550,6 +551,158 @@ func TestParseAndSetOptions(t *testing.T) {
})
}

type configTester struct {
description string
yamlBytes []byte
expectedOpts *ToolOptions
outcome int
}

func runConfigFileTestCases(testCases []configTester) {
configFilePath := "./test-config.yaml"
args := []string{"--config", configFilePath}
defer os.Remove(configFilePath)

for _, testCase := range testCases {
if err := ioutil.WriteFile(configFilePath, testCase.yamlBytes, 0644); err != nil {
So(err, ShouldBeNil)
}
opts := New("test", "", "", "", false, EnabledOptions{true, true, true, true})
err := opts.ParseConfigFile(args)

var assertion func()
if testCase.outcome == ShouldSucceed {
assertion = func() {
So(err, ShouldBeNil)
So(opts.Auth.Password, ShouldEqual, testCase.expectedOpts.Auth.Password)
So(opts.URI.ConnectionString, ShouldEqual, testCase.expectedOpts.URI.ConnectionString)
So(opts.SSL.SSLPEMKeyPassword, ShouldEqual, testCase.expectedOpts.SSL.SSLPEMKeyPassword)
}
} else {
assertion = func() {
So(err, ShouldNotBeNil)
}
}

Convey(testCase.description, assertion)
}
}

func createExpectedOpts(pw string, uri string, ssl string) *ToolOptions {
opts := New("test", "", "", "", false, EnabledOptions{true, true, true, true})
opts.Auth.Password = pw
opts.URI.ConnectionString = uri
opts.SSL.SSLPEMKeyPassword = ssl
return opts
}

func TestParseConfigFile(t *testing.T) {
testtype.SkipUnlessTestType(t, testtype.UnitTestType)

Convey("should error with no config file specified", t, func() {
opts := New("test", "", "", "", false, EnabledOptions{})

// --config at beginning of args list
args := []string{"--config", "--database", "myDB"}
So(opts.ParseConfigFile(args), ShouldNotBeNil)

// --config at end of args list
args = []string{"--database", "myDB", "--config"}
So(opts.ParseConfigFile(args), ShouldNotBeNil)

// --config= at beginning of args list
args = []string{"--config=", "--database", "myDB"}
So(opts.ParseConfigFile(args), ShouldNotBeNil)

// --config= at end of args list
args = []string{"--database", "myDB", "--config="}
So(opts.ParseConfigFile(args), ShouldNotBeNil)
})

Convey("should error with non-existent config file specified", t, func() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the different positions here are overkill to just test a non-existent file.

opts := New("test", "", "", "", false, EnabledOptions{})

// --config with non-existent file
args := []string{"--config", "DoesNotExist.yaml", "--database", "myDB"}
So(opts.ParseConfigFile(args), ShouldNotBeNil)

// --config= with non-existent file
args = []string{"--config=DoesNotExist.yaml", "--database", "myDB"}
So(opts.ParseConfigFile(args), ShouldNotBeNil)
})

Convey("with an existing config file specified", t, func() {
runConfigFileTestCases([]configTester{
{
"containing nothing (empty file)",
[]byte(""),
createExpectedOpts("", "", ""),
ShouldSucceed,
},
{
"containing only password field",
[]byte("password: abc123"),
createExpectedOpts("abc123", "", ""),
ShouldSucceed,
},
{
"containing only uri field",
[]byte("uri: abc123"),
createExpectedOpts("", "abc123", ""),
ShouldSucceed,
},
{
"containing only sslPEMKeyPassword field",
[]byte("sslPEMKeyPassword: abc123"),
createExpectedOpts("", "", "abc123"),
ShouldSucceed,
},
{
"containing all of password, uri and sslPEMKeyPassword fields",
[]byte("password: abc123\nuri: def456\nsslPEMKeyPassword: ghi789"),
createExpectedOpts("abc123", "def456", "ghi789"),
ShouldSucceed,
},
{
"containing a duplicate field",
[]byte("password: abc123\npassword: def456"),
nil,
ShouldFail,
},
{
"containing an unsupported or misspelled field",
[]byte("pasword: abc123"),
nil,
ShouldFail,
},
})
})

Convey("with command line args that override config file values", t, func() {
configFilePath := "./test-config.yaml"
defer os.Remove(configFilePath)
if err := ioutil.WriteFile(configFilePath, []byte("password: abc123"), 0644); err != nil {
So(err, ShouldBeNil)
}

Convey("with --config followed by --password", func() {
args := []string{"--config=" + configFilePath, "--password=def456"}
opts := New("test", "", "", "", false, EnabledOptions{Auth: true})
_, err := opts.ParseArgs(args)
So(err, ShouldBeNil)
So(opts.Auth.Password, ShouldEqual, "def456")
})

Convey("with --password followed by --config", func() {
args := []string{"--password=ghi789", "--config=" + configFilePath}
opts := New("test", "", "", "", false, EnabledOptions{Auth: true})
_, err := opts.ParseArgs(args)
So(err, ShouldBeNil)
So(opts.Auth.Password, ShouldEqual, "ghi789")
})
})
}

type optionsTester struct {
options string
uri string
Expand Down
16 changes: 16 additions & 0 deletions vendor/gopkg.in/yaml.v2/.travis.yml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.