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

Fixes inability to authenticate through the older cmdQuery mechanism #4066

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fixes inability to authenticate through the older cmdQuery mechanism #4066

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 14 additions & 2 deletions internal/handler/cmd_query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"fmt"
"strings"

"github.com/FerretDB/FerretDB/internal/clientconn/conninfo"
"github.com/FerretDB/FerretDB/internal/handler/common"
"github.com/FerretDB/FerretDB/internal/handler/handlererrors"
"github.com/FerretDB/FerretDB/internal/types"
Expand All @@ -28,19 +29,30 @@ import (

// CmdQuery implements deprecated OP_QUERY message handling.
func (h *Handler) CmdQuery(ctx context.Context, query *wire.OpQuery) (*wire.OpReply, error) {
cmd := query.Query().Command()
body := query.Query()
cmd := body.Command()
collection := query.FullCollectionName

// both are valid and are allowed to be run against any database as we don't support authorization yet
if (cmd == "ismaster" || cmd == "isMaster") && strings.HasSuffix(collection, ".$cmd") {
return common.IsMaster(ctx, query.Query(), h.TCPHost, h.ReplSetName)
return common.IsMaster(ctx, body, h.TCPHost, h.ReplSetName)
}

// TODO https://github.com/FerretDB/FerretDB/issues/3008

// database name typically is either "$external" or "admin"

if cmd == "saslStart" && strings.HasSuffix(collection, ".$cmd") {
mechanism, err := common.GetRequiredParam[string](body, "mechanism")
if err == nil && mechanism == "PLAIN" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's return the error for the case where err != nil. 🤗

Let's also handle the case where mechanism != "PLAIN". I think we can handle it similarly to how it's done in msg_saslstart.go

Copy link
Author

@Evengard Evengard Feb 12, 2024 •
edited

Choose a reason for hiding this comment

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

The idea was to provide extreme backwards compatibility by just falling back to the "stub" behavior if something went wrong, as I have no ideas if that would break any other clients. But I don't mind returning explicit errors here...

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for your quick response 🤗. Let us check, @b1ron do you know which other driver uses cmd_query.go? My guess is, it doesn't work for those drivers? If that's the case having an appropriate error is sensible here for a client attempting to authenticate with other mechanisms.

Copy link
Contributor

Choose a reason for hiding this comment

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

It depends on the specific driver and version. Looking at this compatibility matrix drivers that do not support MongoDB 6.0 will use legacy opcodes. I suspect @Evengard is using mongocsharpdriver version 2.15 or older.

OP_QUERY
Deprecated in MongoDB 5.0. Removed in MongoDB 5.1.

Fixing #3121 might be easier.

Copy link
Member

Choose a reason for hiding this comment

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

Removed in MongoDB 5.1.

Narrator: It wasn't actually removed.

But yeah, to move forward, we have to know the driver version.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was able to force an OP_MSG using version 2.19.1, strange.

Copy link
Author

Choose a reason for hiding this comment

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

The thing is I can force an OP_MSG authentication with StableAPI, it's the inserts which bugs then.

I can do a PR in attempts to ignore theese apiVersions parameters probably instead. But looking that the actual issue seemed abandoned wasn't sure it is the way to go.

Copy link
Author

Choose a reason for hiding this comment

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

#4067 Went ahead with the StableAPI parameter ignoring... It works with 2.23.1 too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @Evengard, and apologies for my confusion. I was able to reproduce the error in both 2.19.1 and 2.23.1.

Unhandled exception. MongoDB.Driver.MongoCommandException: Command ping failed: [msg_ping.go:52 handler.(*Handler).MsgPing] [backend.go:67 postgresql.(*backend).Status] [registry.go:259 metadata.(*Registry).DatabaseList] [registry.go:132 metadata.(*Registry).getPool] [pool.go:128 pool.(*Pool).Get] [opendb.go:95 pool.openDB] [opendb.go:110 pool.checkConnection] failed to connect to `host=postgres user= database=ferretdb`: server error (FATAL: no PostgreSQL user name specified in startup packet (SQLSTATE 28000)).

#4067 fixes the issue I also tested it. The core team will review it.

Copy link
Member

Choose a reason for hiding this comment

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

We should do FerretDB/dance#769 first

username, password, err := saslStartPlain(body)
if err != nil {
return nil, err
}

conninfo.Get(ctx).SetAuth(username, password)
}

var emptyPayload types.Binary
reply := wire.OpReply{
NumberReturned: 1,
Expand Down