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
Open

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
e702a18
fix username/password incorrect escape
ritbl May 3, 2022
27a9c8a
fix empty password
ritbl May 3, 2022
8976716
style
ritbl May 3, 2022
9fabb3e
add copyright header
ritbl May 3, 2022
4c44b7f
style
ritbl May 3, 2022
9f69bb3
style
ritbl May 4, 2022
0a57917
add test
ritbl May 4, 2022
1f022eb
style
ritbl May 4, 2022
1cc70c5
style
ritbl May 4, 2022
68f8084
style
ritbl May 5, 2022
d4ce64f
Merge branch 'main' into PMM-9320-fix-mongo-creds-escape
ritbl May 5, 2022
2b88a51
add comment
ritbl May 5, 2022
c189e5f
Merge remote-tracking branch 'origin/PMM-9320-fix-mongo-creds-escape'…
ritbl May 5, 2022
32a33c0
Merge branch 'main' into PMM-9320-fix-mongo-creds-escape
YaroslavPodorvanov May 11, 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
9 changes: 7 additions & 2 deletions actions/mongodb_explain_action.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ import (
"github.com/pkg/errors"
"go.mongodb.org/mongo-driver/bson"
"go.mongodb.org/mongo-driver/mongo"
"go.mongodb.org/mongo-driver/mongo/options"

"github.com/percona/pmm-agent/mongo_fix"
"github.com/percona/pmm-agent/utils/templates"
)

Expand Down Expand Up @@ -68,7 +68,12 @@ func (a *mongodbExplainAction) Run(ctx context.Context) ([]byte, error) {
return nil, errors.WithStack(err)
}

client, err := mongo.Connect(ctx, options.Client().ApplyURI(dsn))
opts, err := mongo_fix.ClientForDSN(dsn)
if err != nil {
return nil, errors.WithStack(err)
}

client, err := mongo.Connect(ctx, opts)
if err != nil {
return nil, errors.WithStack(err)
}
Expand Down
9 changes: 7 additions & 2 deletions actions/mongodb_query_admincommand_action.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ import (
"github.com/pkg/errors"
"go.mongodb.org/mongo-driver/bson"
"go.mongodb.org/mongo-driver/mongo"
"go.mongodb.org/mongo-driver/mongo/options"

"github.com/percona/pmm-agent/mongo_fix"
"github.com/percona/pmm-agent/utils/templates"
)

Expand Down Expand Up @@ -77,7 +77,12 @@ func (a *mongodbQueryAdmincommandAction) Run(ctx context.Context) ([]byte, error
return nil, errors.WithStack(err)
}

client, err := mongo.Connect(ctx, options.Client().ApplyURI(dsn))
opts, err := mongo_fix.ClientForDSN(dsn)
if err != nil {
return nil, errors.WithStack(err)
}

client, err := mongo.Connect(ctx, opts)
if err != nil {
return nil, errors.WithStack(err)
}
Expand Down
11 changes: 8 additions & 3 deletions agents/mongodb/internal/profiler/collector/collector_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,9 @@ import (
"go.mongodb.org/mongo-driver/bson"
"go.mongodb.org/mongo-driver/bson/primitive"
"go.mongodb.org/mongo-driver/mongo"
"go.mongodb.org/mongo-driver/mongo/options"
"go.mongodb.org/mongo-driver/mongo/readpref"

"github.com/percona/pmm-agent/mongo_fix"
)

const (
Expand Down Expand Up @@ -202,8 +203,12 @@ func createSession(dsn string, agentID string) (*mongo.Client, error) {
ctx, cancel := context.WithTimeout(context.Background(), MgoTimeoutDialInfo)
defer cancel()

opts := 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.

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

}

opts = opts.
SetDirect(true).
SetReadPreference(readpref.Nearest()).
SetSocketTimeout(MgoTimeoutSessionSocket).
Expand Down
11 changes: 8 additions & 3 deletions agents/mongodb/internal/profiler/profiler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,11 @@ import (

"github.com/sirupsen/logrus"
"go.mongodb.org/mongo-driver/mongo"
"go.mongodb.org/mongo-driver/mongo/options"
"go.mongodb.org/mongo-driver/mongo/readpref"

"github.com/percona/pmm-agent/agents/mongodb/internal/profiler/aggregator"
"github.com/percona/pmm-agent/agents/mongodb/internal/profiler/sender"
"github.com/percona/pmm-agent/mongo_fix"
)

// New creates new Profiler
Expand Down Expand Up @@ -192,8 +192,13 @@ func signalReady(ready *sync.Cond) {
func createSession(dsn string, agentID string) (*mongo.Client, error) {
ctx, cancel := context.WithTimeout(context.Background(), MgoTimeoutDialInfo)
defer cancel()
opts := options.Client().
ApplyURI(dsn).

opts, err := mongo_fix.ClientForDSN(dsn)
if err != nil {
return nil, err
}

opts = opts.
SetDirect(true).
SetReadPreference(readpref.Nearest()).
SetSocketTimeout(MgoTimeoutSessionSocket).
Expand Down
11 changes: 9 additions & 2 deletions connectionchecker/connection_checker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,9 @@ import (
"github.com/sirupsen/logrus"
"go.mongodb.org/mongo-driver/bson"
"go.mongodb.org/mongo-driver/mongo"
"go.mongodb.org/mongo-driver/mongo/options"

"github.com/percona/pmm-agent/config"
"github.com/percona/pmm-agent/mongo_fix"
"github.com/percona/pmm-agent/tlshelpers"
"github.com/percona/pmm-agent/utils/templates"
)
Expand Down Expand Up @@ -162,7 +162,14 @@ func (cc *ConnectionChecker) checkMongoDBConnection(ctx context.Context, dsn str
return &res
}

client, err := mongo.Connect(ctx, options.Client().ApplyURI(dsn))
opts, err := mongo_fix.ClientForDSN(dsn)
if err != nil {
cc.l.Debugf("failed to parse DSN: %s", err)
res.Error = err.Error()
return &res
}

client, err := mongo.Connect(ctx, opts)
if err != nil {
cc.l.Debugf("checkMongoDBConnection: failed to Connect: %s", err)
res.Error = err.Error()
Expand Down
27 changes: 27 additions & 0 deletions mongo_fix/mongo_fix.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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

ritbl marked this conversation as resolved.
Show resolved Hide resolved

import (
"net/url"

"github.com/pkg/errors"
"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)

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

parsedDsn, err := url.Parse(dsn)
if err != nil {
return nil, errors.Wrap(err, "cannot parse DSN")
}

clientOptions := options.Client().ApplyURI(dsn)

// 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

clientOptions = clientOptions.SetAuth(options.Credential{Username: username, Password: password})
}

return clientOptions, nil
}
10 changes: 8 additions & 2 deletions utils/tests/mongodb.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ import (
"github.com/stretchr/testify/require"
"go.mongodb.org/mongo-driver/bson/primitive"
"go.mongodb.org/mongo-driver/mongo"
"go.mongodb.org/mongo-driver/mongo/options"

"github.com/percona/pmm-agent/mongo_fix"
)

// GetTestMongoDBDSN returns DNS for MongoDB test database.
Expand Down Expand Up @@ -104,7 +105,12 @@ func GetTestMongoDBReplicatedWithSSLDSN(tb testing.TB, pathToRoot string) (strin
func OpenTestMongoDB(tb testing.TB, dsn string) *mongo.Client {
tb.Helper()

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

}

client, err := mongo.Connect(context.Background(), opts)
require.NoError(tb, err)

require.NoError(tb, client.Ping(context.Background(), nil))
Expand Down