Skip to content

Commit

Permalink
Fix PLAIN mechanism authentication incorrectly working (#4163)
Browse files Browse the repository at this point in the history
Closes #1877.
  • Loading branch information
chilagrow committed Mar 11, 2024
1 parent e98d536 commit f460194
Show file tree
Hide file tree
Showing 7 changed files with 64 additions and 26 deletions.
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 @@ func (h *Handler) authenticate(ctx context.Context) error {
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)
}

// 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 @@ func (h *Handler) authenticate(ctx context.Context) error {

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

0 comments on commit f460194

Please sign in to comment.