Skip to content
This repository has been archived by the owner on Jun 21, 2022. It is now read-only.

PMM-9320 MongoDB password escape special characters #1098

Closed
wants to merge 5 commits into from
Closed

PMM-9320 MongoDB password escape special characters #1098

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9db4121
PMM-9320 MongoDB password escape special characters
YaroslavPodorvanov Apr 29, 2022
ccc1fb6
Merge branch 'main' into PMM-9320-mongodb-password-escape-special-cha…
YaroslavPodorvanov Apr 29, 2022
417226a
PMM-9320 update tests after escape MongoDB password
YaroslavPodorvanov May 1, 2022
debce0c
PMM-9320 test MongoDB password contains plus
YaroslavPodorvanov May 1, 2022
4d1e74a
Merge branch 'main' into PMM-9320-mongodb-password-escape-special-cha…
YaroslavPodorvanov May 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion models/agent_model.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,13 +442,14 @@ func (s *Agent) DSN(service *Service, dialTimeout time.Duration, database string
}
switch {
case password != "":
u.User = url.UserPassword(username, password)
u.User = url.UserPassword(username, "__password__")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why just not to use url.UserPassword(username, url.QueryEscape(password)) ?

Copy link
Contributor Author

@YaroslavPodorvanov YaroslavPodorvanov Apr 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Why just not to use url.UserPassword(username, url.QueryEscape(password))

I'm research that solution too in https://gitlab.com/go-yp/mongodb-escape-password-research/:

SOLUTION SUCCESS TOTAL COMMENT
0 36 38 url.UserPassword(username, password)
1 10 38 url.UserPassword(username, url.QueryEscape(password))
2 35 38 "__plus__"
3 36 38 "__password__"
4 37 38 ":__password__@"
5 1 38

Generate 38 password with specials symbols:

	var symbols = "" +
		";,/?:@&=+$" +
		"-_.!~*'()" +
		"#" +
		" " +
		"%" +
		"[]" +
		"абвгґдеє" +
		"\""

	// one quote symbol ' failed always

	for i, symbol := range symbols {
		password := fmt.Sprintf("example%spassword", string(symbol))

		userPasswords = append(userPasswords, [3]string{
			"tester" + strconv.Itoa(i),
			password,
			"/tester_db",
		})
	}

test solutions:

func ping(username, password, db string, solution int) error {
	u := &url.URL{
		Scheme:   "mongodb",
		Host:     "localhost:27017",
		Path:     db,
		RawQuery: "connectTimeoutMS=2000&serverSelectionTimeoutMS=2000",
	}

	var (
		dsn     string
		setAuth = false
	)

	switch solution {
	// SOLUTION 0
	// SOLUTION NOW EXISTS IN pmm-manager
	// 36 / 38
	case solutionExists:
		u.User = url.UserPassword(username, password)

		dsn = u.String()
	// SOLUTION 1
	// Artem's propose solution https://github.com/percona/pmm-managed/pull/1098#discussion_r861838935
	// 10 / 38
	case solutionArtem:
		u.User = url.UserPassword(username, url.QueryEscape(password))

		dsn = u.String()
	// SOLUTION 2
	// Another solution https://github.com/percona/pmm-managed/pull/1003/files
	// 35 / 38
	case solutionPR:
		const plusPlaceholder = "__plus__" // There is a risk of error, when the password contains this string. Can change it to a more unique string.

		password = strings.ReplaceAll(password, "+", plusPlaceholder)

		u.User = url.UserPassword(username, password)

		dsn = u.String()

		dsn = strings.ReplaceAll(dsn, url.QueryEscape(plusPlaceholder), "%2B")
	// SOLUTION 3
	// 36 / 38
	case solutionMy1:
		u.User = url.UserPassword(username, "__password__")

		dsn = u.String()

		dsn = strings.Replace(dsn, "__password__", url.QueryEscape(password), 1)
	// SOLUTION 4
	// 37 / 38
	case solutionMy2:
		u.User = url.UserPassword(username, "__password__")

		dsn = u.String()

		dsn = strings.Replace(dsn, ":__password__@", ":"+url.QueryEscape(password)+"@", 1)
	// SOLUTION 5
	//  1 / 38
	case solutionMy3:
		dsn = u.String()

		setAuth = true
	}

	ctx := context.Background()

	opts := options.Client().ApplyURI(dsn)
	if setAuth {
		opts = opts.SetAuth(options.Credential{
			Username: username,
			Password: password,
		})
	}

	client, err := mongo.Connect(ctx, opts)
	if err != nil {
		return err
	}
	defer client.Disconnect(ctx) //nolint:errcheck

	if err = client.Ping(ctx, nil); err != nil {
		return err
	}

	return nil
}

So main problem that in one sender side standard Go package "url" use one logic for stringify password to valid URL, in receiver side go.mongodb.org/mongo-driver/mongo use another logic for escape this password from URL.

In standard Go we have:

// String returns the encoded userinfo information in the standard form
// of "username[:password]".
func (u *Userinfo) String() string {
	if u == nil {
		return ""
	}
	s := escape(u.username, encodeUserPassword)
	if u.passwordSet {
		s += ":" + escape(u.password, encodeUserPassword)
	}
	return s
}
// Package url parses URLs and implements query escaping.
package url

escape(u.password, encodeUserPassword)

In go.mongodb.org/mongo-driver/mongo :

p.Password, err = url.QueryUnescape(password)
// Package url parses URLs and implements query escaping.
package url

func QueryUnescape(s string) (string, error) {
	return unescape(s, encodeQueryComponent)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Damn. Also it seems that we are not alone with this problem: https://kecci.medium.com/handle-mongo-connection-with-percent-encoding-in-go-c8b52b58094f

But what is more important that net/url is not suitable for MongoDB URIs. Maybe we can use something that mongo driver uses instead of net/url here?

Copy link
Contributor Author

@YaroslavPodorvanov YaroslavPodorvanov May 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

MongoDB driver use "go.mongodb.org/mongo-driver/x/mongo/driver/connstring", but connstring work only with parsing https://pkg.go.dev/go.mongodb.org/mongo-driver/x/mongo/driver/connstring

ConnString.String() not stringify URL

type ConnString struct {
	Original                           string
	// ...
}

func (p *parser) parse(original string) error {
	p.Original = original
	// ...
	return nil
}

func (u *ConnString) String() string {
	return u.Original
}

Maybe we can use something that mongo driver uses instead of net/url here?

No, ConnString cannot do it, and i did search in MongoDB driver by *.go "Password:"

Copy link
Contributor

Choose a reason for hiding this comment

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

@YaroslavPodorvanov @artemgavrilov please take a look

case username != "":
u.User = url.User(username)
}
dsn := u.String()
dsn = strings.ReplaceAll(dsn, url.QueryEscape(tdp.Left), tdp.Left)
dsn = strings.ReplaceAll(dsn, url.QueryEscape(tdp.Right), tdp.Right)
dsn = strings.Replace(dsn, ":__password__@", ":"+url.QueryEscape(password)+"@", 1)
return dsn

case PostgresExporterType, QANPostgreSQLPgStatementsAgentType, QANPostgreSQLPgStatMonitorAgentType:
Expand Down