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.

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

Fix PLAIN mechanism authentication incorrectly working #4163

Merged
merged 5 commits into from Mar 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
37 changes: 32 additions & 5 deletions integration/users/connection_test.go
Expand Up @@ -26,6 +26,7 @@ import (
"go.mongodb.org/mongo-driver/mongo/options"
"go.mongodb.org/mongo-driver/x/mongo/driver/topology"

"github.com/FerretDB/FerretDB/integration"
"github.com/FerretDB/FerretDB/integration/setup"
"github.com/FerretDB/FerretDB/internal/util/testutil/testtb"
)
Expand Down Expand Up @@ -353,12 +354,20 @@ func TestAuthenticationEnableNewAuthPLAIN(t *testing.T) {
}).Err()
require.NoError(t, err, "cannot create user")

err = db.RunCommand(ctx, bson.D{
{"createUser", "scram-user"},
{"roles", bson.A{}},
{"pwd", "correct"},
{"mechanisms", bson.A{"SCRAM-SHA-1", "SCRAM-SHA-256"}},
}).Err()
require.NoError(t, err, "cannot create user")

testCases := map[string]struct {
username string
password string
mechanism string

err string
err *mongo.CommandError
}{
"Success": {
username: "plain-user",
Expand All @@ -369,13 +378,31 @@ func TestAuthenticationEnableNewAuthPLAIN(t *testing.T) {
username: "plain-user",
password: "wrong",
mechanism: "PLAIN",
err: "AuthenticationFailed",
err: &mongo.CommandError{
Code: 18,
Name: "AuthenticationFailed",
Message: "Authentication failed",
},
},
"NonExistentUser": {
username: "not-found-user",
password: "something",
mechanism: "PLAIN",
err: "AuthenticationFailed",
err: &mongo.CommandError{
Code: 18,
Name: "AuthenticationFailed",
Message: "Authentication failed",
},
},
"NonPLAINUser": {
username: "scram-user",
password: "correct",
mechanism: "PLAIN",
err: &mongo.CommandError{
Code: 334,
Name: "ErrMechanismUnavailable",
Message: "Unable to use PLAIN based authentication for user without any PLAIN credentials registered",
},
},
}

Expand Down Expand Up @@ -403,8 +430,8 @@ func TestAuthenticationEnableNewAuthPLAIN(t *testing.T) {
c := client.Database(db.Name()).Collection(cName)
_, err = c.InsertOne(ctx, bson.D{{"ping", "pong"}})

if tc.err != "" {
require.ErrorContains(t, err, tc.err)
if tc.err != nil {
integration.AssertEqualCommandError(t, *tc.err, err)
return
}

Expand Down
2 changes: 1 addition & 1 deletion internal/backends/mysql/metadata/registry.go
Expand Up @@ -124,7 +124,7 @@ func (r *Registry) getPool(ctx context.Context) (*fsql.DB, error) {
return nil, lazyerrors.New("no connection pool")
}
} else {
username, password := connInfo.Auth()
username, password, _ := connInfo.Auth()

var err error
if p, err = r.p.Get(username, password); err != nil {
Expand Down
2 changes: 1 addition & 1 deletion internal/backends/postgresql/metadata/registry.go
Expand Up @@ -129,7 +129,7 @@ func (r *Registry) getPool(ctx context.Context) (*pgxpool.Pool, error) {
}
}
} else {
username, password := connInfo.Auth()
username, password, _ := connInfo.Auth()

var err error
if p, err = r.p.Get(username, password); err != nil {
Expand Down
12 changes: 7 additions & 5 deletions internal/clientconn/conninfo/conn_info.go
Expand Up @@ -35,6 +35,7 @@ type ConnInfo struct {
PeerAddr string
username string // protected by rw
password string // protected by rw
mechanism string // protected by rw
metadataRecv bool // protected by rw

sc *scram.ServerConversation // protected by rw
Expand Down Expand Up @@ -62,21 +63,22 @@ func (connInfo *ConnInfo) Username() string {
return connInfo.username
}

// Auth returns stored username and password.
func (connInfo *ConnInfo) Auth() (username, password string) {
// Auth returns stored username, password and mechanism.
func (connInfo *ConnInfo) Auth() (username, password, mechanism string) {
connInfo.rw.RLock()
defer connInfo.rw.RUnlock()

return connInfo.username, connInfo.password
return connInfo.username, connInfo.password, connInfo.mechanism
}

// SetAuth stores username and password.
func (connInfo *ConnInfo) SetAuth(username, password string) {
// SetAuth stores username, password.
func (connInfo *ConnInfo) SetAuth(username, password, mechanism string) {
connInfo.rw.Lock()
defer connInfo.rw.Unlock()

connInfo.username = username
connInfo.password = password
connInfo.mechanism = mechanism
}

// Conv returns stored SCRAM server conversation.
Expand Down
31 changes: 19 additions & 12 deletions internal/handler/authenticate.go
Expand Up @@ -54,7 +54,18 @@
return lazyerrors.Error(err)
}

username, userPassword := conninfo.Get(ctx).Auth()
username, userPassword, mechanism := conninfo.Get(ctx).Auth()

switch mechanism {
case "SCRAM-SHA-256", "SCRAM-SHA-1":
// SCRAM calls back scramCredentialLookup each time Step is called,
// and that checks the authentication.
return nil
case "PLAIN":
break
default:
return lazyerrors.Errorf("Unsupported authentication mechanism %q", mechanism)

Check warning on line 67 in internal/handler/authenticate.go

View check run for this annotation

Codecov / codecov/patch

internal/handler/authenticate.go#L66-L67

Added lines #L66 - L67 were not covered by tests
}

// For `PLAIN` mechanism $db field is always `$external` upon saslStart.
// For `SCRAM-SHA-1` and `SCRAM-SHA-256` mechanisms $db field contains
Expand Down Expand Up @@ -121,19 +132,15 @@

credentials := must.NotFail(storedUser.Get("credentials")).(*types.Document)

switch {
case credentials.Has("SCRAM-SHA-256"), credentials.Has("SCRAM-SHA-1"):
// SCRAM calls back scramCredentialLookup each time Step is called,
// and that checks the authentication.
return nil
case credentials.Has("PLAIN"):
break
default:
panic("credentials does not contain a known mechanism")
v, _ := credentials.Get("PLAIN")
if v == nil {
return handlererrors.NewCommandErrorMsgWithArgument(
handlererrors.ErrMechanismUnavailable,
"Unable to use PLAIN based authentication for user without any PLAIN credentials registered",
"authenticate",
)
}

v := must.NotFail(credentials.Get("PLAIN"))

doc, ok := v.(*types.Document)
if !ok {
return lazyerrors.Errorf("field 'PLAIN' has type %T, expected Document", v)
Expand Down
2 changes: 1 addition & 1 deletion internal/handler/msg_logout.go
Expand Up @@ -25,7 +25,7 @@ import (

// MsgLogout implements `logout` command.
func (h *Handler) MsgLogout(ctx context.Context, msg *wire.OpMsg) (*wire.OpMsg, error) {
conninfo.Get(ctx).SetAuth("", "")
conninfo.Get(ctx).SetAuth("", "", "")

var reply wire.OpMsg
must.NoError(reply.SetSections(wire.MakeOpMsgSection(
Expand Down
4 changes: 3 additions & 1 deletion internal/handler/msg_saslstart.go
Expand Up @@ -78,7 +78,7 @@ func (h *Handler) saslStart(ctx context.Context, dbName string, document *types.
conninfo.Get(ctx).SetBypassBackendAuth()
}

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

var emptyPayload types.Binary

Expand All @@ -95,6 +95,8 @@ func (h *Handler) saslStart(ctx context.Context, dbName string, document *types.
)
}

conninfo.Get(ctx).SetAuth("", "", mechanism)

response, err := h.saslStartSCRAM(ctx, dbName, mechanism, document)
if err != nil {
return nil, err
Expand Down