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

PMM-9320: fix username/password incorrect escape #372

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

ritbl
Copy link
Contributor

@ritbl ritbl commented May 3, 2022

PMM-9320

Password and Username are escaped According to RFC 3986 §3.2.1

// The RFC allows ';', ':', '&', '=', '+', '$', and ',' in
// userinfo, so we must escape only '@', '/', and '?'.
// The parsing of userinfo treats ':' as special so we must escape
// that too.
go1.18.1/src/net/url/url.go:143

But in MongoDB driver they decided to unscape it as encodePathSegment
using url.PathUnescape
here mongo-go-driver/x/mongo/driver/connstring/connstring.go:236
and
here ext/mongo-go-driver/x/mongo/driver/connstring/connstring.go:249


It can also be fixed with this PR


Related PRs:

FB:

@ritbl ritbl requested a review from BupycHuk as a code owner May 3, 2022 19:30

clientOptions := options.Client().
ApplyURI(dsn).
SetAuth(creds) //Workaround for PMM-9320
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixing here

ApplyURI(dsn).
opts, err := mongo_fix.ClientForDSN(dsn)
if err != nil {
panic(err)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is test, so we can panic

Copy link
Member

Choose a reason for hiding this comment

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

we can, but should we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, here we return error, so I think we can just propagate error.
but what about other cases, like in func OpenTestMongoDB(tb testing.TB, dsn string) *mongo.Client {
we don't have error in the API, do you think we should refactor API or we can panic (in tests only)

Copy link
Member

Choose a reason for hiding this comment

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

we can call require.NoError(tb, err) in that places. so it will stop test with failed result

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, good idea

@@ -0,0 +1,27 @@
package mongo_fix
Copy link
Contributor Author

Choose a reason for hiding this comment

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

called mongo_fix not to clash with mongo driver

// Workaround for PMM-9320
username := parsedDsn.User.Username()
password, _ := parsedDsn.User.Password()
if username != "" || password != "" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we need to set auth only when creds are provided, otherwise auth fill fail

client, err := mongo.Connect(context.Background(), options.Client().ApplyURI(dsn))
opts, err := mongo_fix.ClientForDSN(dsn)
if err != nil {
panic(err)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

panic in test

"go.mongodb.org/mongo-driver/mongo/options"
)

// ClientForDSN applies URI to Client
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci-lint] reported by reviewdog 🐶
Comment should end in a period (godot)

mongo_fix/mongo_fix.go Outdated Show resolved Hide resolved
Copy link
Member

@BupycHuk BupycHuk left a comment

Choose a reason for hiding this comment

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

Looks good to me. Can we add a few tests with this kind of DSNs?

ApplyURI(dsn).
opts, err := mongo_fix.ClientForDSN(dsn)
if err != nil {
panic(err)
Copy link
Member

Choose a reason for hiding this comment

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

we can, but should we?

)

// ClientForDSN applies URI to Client.
func ClientForDSN(dsn string) (*options.ClientOptions, error) {
Copy link
Member

Choose a reason for hiding this comment

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

probably we should call it ClientOptionsForDSN or include mongo.Connect to this function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, ClientOptions is more correct

@ritbl
Copy link
Contributor Author

ritbl commented May 4, 2022

Looks good to me. Can we add a few tests with this kind of DSNs?

sure, will do

@ritbl ritbl requested a review from BupycHuk May 4, 2022 09:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants