Skip to content

Commit

Permalink
sql: add NOSQLLOGIN role which restricts SQL CLI only
Browse files Browse the repository at this point in the history 
Previously, in order to restrict user login ability, the `LOGIN` and
`NOLOGIN` role options were available which would restrict both SQL and
DB Console login ability.

This change adds the `NOSQLLOGIN` (and its inverse: `SQLLOGIN`) role
option in order to provide the ability to disable SQL CLI logins from
users while retaining DB Console login ability.

Resolves cockroachdb#74482

Release note (sql change): A new role option is now available,
`NOSQLLOGIN` (and its inverse `SQLLOGIN`), which restricts SQL CLI login
ability for a user while retaining their ability to login to the DB
Console (as opposed to `NOLOGIN` which restricts both SQL and DB
Console). Without any role options all login behavior remains permitted
as it does today. OIDC logins to the DB Console continue to be permitted
with `NOSQLLOGIN` set.
  • Loading branch information
@dhartunian
dhartunian committed Jan 14, 2022
1 parent 35eeb81 commit f8997c5
Show file tree
Hide file tree
Showing 11 changed files with 212 additions and 84 deletions.
4 changes: 4 additions & 0 deletions docs/generated/sql/bnf/stmt_block.bnf
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1039,6 +1039,7 @@ unreserved_keyword ::=
| 'NOLOGIN'
| 'NOMODIFYCLUSTERSETTING'
| 'NONVOTERS'
| 'NOSQLLOGIN'
| 'NOVIEWACTIVITY'
| 'NOVIEWACTIVITYREDACTED'
| 'NOWAIT'
Expand Down Expand Up @@ -1150,6 +1151,7 @@ unreserved_keyword ::=
| 'SNAPSHOT'
| 'SPLIT'
| 'SQL'
| 'SQLLOGIN'
| 'START'
| 'STATEMENTS'
| 'STATISTICS'
Expand Down Expand Up @@ -2370,6 +2372,8 @@ role_option ::=
| 'NOCANCELQUERY'
| 'MODIFYCLUSTERSETTING'
| 'NOMODIFYCLUSTERSETTING'
| 'SQLLOGIN'
| 'NOSQLLOGIN'
| password_clause
| valid_until_clause

Expand Down
87 changes: 52 additions & 35 deletions pkg/ccl/serverccl/role_authentication_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ func TestVerifyPassword(t *testing.T) {
{"druidia", "12345", "LOGIN", "", nil},

{"richardc", "12345", "NOLOGIN", "", nil},
{"richardc2", "12345", "NOSQLLOGIN", "", nil},
{"before_epoch", "12345", "", "VALID UNTIL '1969-01-01'", nil},
{"epoch", "12345", "", "VALID UNTIL '1970-01-01'", nil},
{"cockroach", "12345", "", "VALID UNTIL '2100-01-01'", nil},
Expand All @@ -93,41 +94,42 @@ func TestVerifyPassword(t *testing.T) {
}

for _, tc := range []struct {
username string
password string
isSuperuser bool
shouldAuthenticate bool
expectedErrString string
testName string
username string
password string
isSuperuser bool
shouldAuthenticateSQL bool
shouldAuthenticateDBConsole bool
}{
{"azure_diamond", "hunter2", false, true, ""},
{"Azure_Diamond", "hunter2", false, false, ""},
{"azure_diamond", "hunter", false, false, "crypto/bcrypt"},
{"azure_diamond", "", false, false, "crypto/bcrypt"},
{"azure_diamond", "🍦", false, false, "crypto/bcrypt"},
{"azure_diamond", "hunter2345", false, false, "crypto/bcrypt"},
{"azure_diamond", "shunter2", false, false, "crypto/bcrypt"},
{"azure_diamond", "12345", false, false, "crypto/bcrypt"},
{"azure_diamond", "*******", false, false, "crypto/bcrypt"},
{"druidia", "12345", false, true, ""},
{"druidia", "hunter2", false, false, "crypto/bcrypt"},
{"root", "", true, false, "crypto/bcrypt"},
{"some_admin", "", true, false, "crypto/bcrypt"},
{"", "", false, false, "does not exist"},
{"doesntexist", "zxcvbn", false, false, "does not exist"},

{"richardc", "12345", false, false,
"richardc does not have login privilege"},
{"before_epoch", "12345", false, false, ""},
{"epoch", "12345", false, false, ""},
{"cockroach", "12345", false, true, ""},
{"toolate", "12345", false, false, ""},
{"timelord", "12345", false, true, ""},
{"cthon98", "12345", false, true, ""},
{"valid login", "azure_diamond", "hunter2", false, true, true},
{"wrong password", "azure_diamond", "hunter", false, false, false},
{"empty password", "azure_diamond", "", false, false, false},
{"wrong emoji password", "azure_diamond", "🍦", false, false, false},
{"correct password with suffix should fail", "azure_diamond", "hunter2345", false, false, false},
{"correct password with prefix should fail", "azure_diamond", "shunter2", false, false, false},
{"wrong password all numeric", "azure_diamond", "12345", false, false, false},
{"wrong password all stars", "azure_diamond", "*******", false, false, false},
{"valid login numeric password", "druidia", "12345", false, true, true},
{"wrong password matching other user", "druidia", "hunter2", false, false, false},
{"root with empty password should fail", "root", "", true, false, false},
{"some_admin with empty password should fail", "some_admin", "", true, false, false},
{"empty username and password should fail", "", "", false, false, false},
{"username does not exist should fail", "doesntexist", "zxcvbn", false, false, false},

{"user with NOLOGIN role option should fail", "richardc", "12345", false, false, false},
// This is the one test case where SQL and DB Console login outcomes differ.
{"user with NOSQLLOGIN role option should fail SQL but succeed on DB Console", "richardc2", "12345", false, false, true},
{"user with VALID UNTIL before the Unix epoch should fail", "before_epoch", "12345", false, false, false},
{"user with VALID UNTIL at Unix epoch should fail", "epoch", "12345", false, false, false},
{"user with VALID UNTIL future date should succeed", "cockroach", "12345", false, true, true},
{"user with VALID UNTIL 10 minutes ago should fail", "toolate", "12345", false, false, false},
{"user with VALID UNTIL future time in Shanghai time zone should succeed", "timelord", "12345", false, true, true},
{"user with VALID UNTIL NULL should succeed", "cthon98", "12345", false, true, true},
} {
t.Run("", func(t *testing.T) {
t.Run(tc.testName, func(t *testing.T) {
execCfg := s.ExecutorConfig().(sql.ExecutorConfig)
username := security.MakeSQLUsernameFromPreNormalizedString(tc.username)
exists, canLogin, isSuperuser, validUntil, _, pwRetrieveFn, err := sql.GetUserSessionInitInfo(
exists, canLoginSQL, canLoginDBConsole, isSuperuser, validUntil, _, pwRetrieveFn, err := sql.GetUserSessionInitInfo(
context.Background(), &execCfg, &ie, username, "", /* databaseName */
)

Expand All @@ -141,12 +143,17 @@ func TestVerifyPassword(t *testing.T) {
}

valid := true
validDBConsole := true
expired := false

if !exists || !canLogin {
if !exists || !canLoginSQL {
valid = false
}

if !exists || !canLoginDBConsole {
validDBConsole = false
}

hashedPassword, err := pwRetrieveFn(ctx)
if err != nil {
t.Errorf(
Expand All @@ -160,6 +167,7 @@ func TestVerifyPassword(t *testing.T) {
err = security.CompareHashAndPassword(ctx, hashedPassword, tc.password)
if err != nil {
valid = false
validDBConsole = false
}

if validUntil != nil {
Expand All @@ -168,13 +176,22 @@ func TestVerifyPassword(t *testing.T) {
}
}

if valid && !expired != tc.shouldAuthenticate {
if valid && !expired != tc.shouldAuthenticateSQL {
t.Errorf(
"sql credentials %s/%s valid = %t, wanted %t",
tc.username,
tc.password,
valid,
tc.shouldAuthenticateSQL,
)
}
if validDBConsole && !expired != tc.shouldAuthenticateDBConsole {
t.Errorf(
"credentials %s/%s valid = %t, wanted %t",
"db console credentials %s/%s valid = %t, wanted %t",
tc.username,
tc.password,
valid,
tc.shouldAuthenticate,
tc.shouldAuthenticateDBConsole,
)
}
require.Equal(t, tc.isSuperuser, isSuperuser)
Expand Down
2 changes: 1 addition & 1 deletion pkg/server/api_v2_auth.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ func (a *authenticationV2Server) login(w http.ResponseWriter, r *http.Request) {
username, _ := security.MakeSQLUsernameFromUserInput(r.Form.Get("username"), security.UsernameValidation)

// Verify the provided username/password pair.
verified, expired, err := a.authServer.verifyPassword(a.ctx, username, r.Form.Get("password"))
verified, expired, err := a.authServer.verifyPasswordDBConsole(a.ctx, username, r.Form.Get("password"))
if err != nil {
apiV2InternalError(r.Context(), err, w)
return
Expand Down
20 changes: 11 additions & 9 deletions pkg/server/authentication.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ func (s *authenticationServer) UserLogin(
username, _ := security.MakeSQLUsernameFromUserInput(req.Username, security.UsernameValidation)

// Verify the provided username/password pair.
verified, expired, err := s.verifyPassword(ctx, username, req.Password)
verified, expired, err := s.verifyPasswordDBConsole(ctx, username, req.Password)
if err != nil {
return nil, apiInternalError(ctx, err)
}
Expand Down Expand Up @@ -210,7 +210,7 @@ func (s *authenticationServer) demoLogin(w http.ResponseWriter, req *http.Reques
// without further normalization.
username, _ := security.MakeSQLUsernameFromUserInput(userInput, security.UsernameValidation)
// Verify the provided username/password pair.
verified, expired, err := s.verifyPassword(ctx, username, password)
verified, expired, err := s.verifyPasswordDBConsole(ctx, username, password)
if err != nil {
fail(err)
return
Expand Down Expand Up @@ -255,7 +255,7 @@ func (s *authenticationServer) UserLoginFromSSO(
// without further normalization.
username, _ := security.MakeSQLUsernameFromUserInput(reqUsername, security.UsernameValidation)

exists, canLogin, _, _, _, _, err := sql.GetUserSessionInitInfo(
exists, _, canLoginDBConsole, _, _, _, _, err := sql.GetUserSessionInitInfo(
ctx,
s.server.sqlServer.execCfg,
s.server.sqlServer.execCfg.InternalExecutor,
Expand All @@ -266,8 +266,7 @@ func (s *authenticationServer) UserLoginFromSSO(
if err != nil {
return nil, errors.Wrap(err, "failed creating session for username")
}

if !exists || !canLogin {
if !exists || !canLoginDBConsole {
return nil, errWebAuthenticationFailure
}

Expand Down Expand Up @@ -408,17 +407,20 @@ WHERE id = $1`
return true, username, nil
}

// verifyPassword verifies the passed username/password pair against the
// verifyPasswordDBConsole verifies the passed username/password pair against the
// system.users table. The returned boolean indicates whether or not the
// verification succeeded; an error is returned if the validation process could
// not be completed.
//
// This function should *not* be used to validate logins into the SQL
// shell since it checks a separate authentication scheme.
//
// The caller is responsible for ensuring that the username is normalized.
// (CockroachDB has case-insensitive usernames, unlike PostgreSQL.)
func (s *authenticationServer) verifyPassword(
func (s *authenticationServer) verifyPasswordDBConsole(
ctx context.Context, username security.SQLUsername, password string,
) (valid bool, expired bool, err error) {
exists, canLogin, _, validUntil, _, pwRetrieveFn, err := sql.GetUserSessionInitInfo(
exists, _, canLoginDBConsole, _, validUntil, _, pwRetrieveFn, err := sql.GetUserSessionInitInfo(
ctx,
s.server.sqlServer.execCfg,
s.server.sqlServer.execCfg.InternalExecutor,
Expand All @@ -428,7 +430,7 @@ func (s *authenticationServer) verifyPassword(
if err != nil {
return false, false, err
}
if !exists || !canLogin {
if !exists || !canLoginDBConsole {
return false, false, nil
}
hashedPassword, err := pwRetrieveFn(ctx)
Expand Down
125 changes: 100 additions & 25 deletions pkg/server/authentication_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,7 @@ func TestVerifyPassword(t *testing.T) {
{"druidia", "12345", "", "", nil},

{"richardc", "12345", "NOLOGIN", "", nil},
{"richardc2", "12345", "NOSQLLOGIN", "", nil},
{"before_epoch", "12345", "", "VALID UNTIL '1969-01-01'", nil},
{"epoch", "12345", "", "VALID UNTIL '1970-01-01'", nil},
{"cockroach", "12345", "", "VALID UNTIL '2100-01-01'", nil},
Expand All @@ -242,37 +243,111 @@ func TestVerifyPassword(t *testing.T) {
}

for _, tc := range []struct {
testName string
username string
password string
shouldAuthenticate bool
expectedErrString string
}{
{"azure_diamond", "hunter2", true, ""},
{"azure_diamond", "hunter", false, "crypto/bcrypt"},
{"azure_diamond", "", false, "crypto/bcrypt"},
{"azure_diamond", "🍦", false, "crypto/bcrypt"},
{"azure_diamond", "hunter2345", false, "crypto/bcrypt"},
{"azure_diamond", "shunter2", false, "crypto/bcrypt"},
{"azure_diamond", "12345", false, "crypto/bcrypt"},
{"azure_diamond", "*******", false, "crypto/bcrypt"},
{"druidia", "12345", true, ""},
{"druidia", "hunter2", false, "crypto/bcrypt"},
{"root", "", false, "crypto/bcrypt"},
{"", "", false, "does not exist"},
{"doesntexist", "zxcvbn", false, "does not exist"},

{"richardc", "12345", false,
"richardc does not have login privilege"},
{"before_epoch", "12345", false, ""},
{"epoch", "12345", false, ""},
{"cockroach", "12345", true, ""},
{"toolate", "12345", false, ""},
{"timelord", "12345", true, ""},
{"cthon98", "12345", true, ""},
{"valid login", "azure_diamond", "hunter2", true},
{"wrong password", "azure_diamond", "hunter", false},
{"empty password", "azure_diamond", "", false},
{"wrong emoji password", "azure_diamond", "🍦", false},
{"correct password with suffix should fail", "azure_diamond", "hunter2345", false},
{"correct password with prefix should fail", "azure_diamond", "shunter2", false},
{"wrong password all numeric", "azure_diamond", "12345", false},
{"wrong password all stars", "azure_diamond", "*******", false},
{"valid login numeric password", "druidia", "12345", true},
{"wrong password matching other user", "druidia", "hunter2", false},
{"root with empty password should fail", "root", "", false},
{"empty username and password should fail", "", "", false},
{"username does not exist should fail", "doesntexist", "zxcvbn", false},

{"user with NOLOGIN role option should fail", "richardc", "12345", false},
// This is the one test case where SQL and DB Console login outcomes differ.
{"user with NOSQLLOGIN role option should fail", "richardc2", "12345", false},
{"user with VALID UNTIL before the Unix epoch should fail", "before_epoch", "12345", false},
{"user with VALID UNTIL at Unix epoch should fail", "epoch", "12345", false},
{"user with VALID UNTIL future date should succeed", "cockroach", "12345", true},
{"user with VALID UNTIL 10 minutes ago should fail", "toolate", "12345", false},
{"user with VALID UNTIL future time in Shanghai time zone should succeed", "timelord", "12345", true},
{"user with VALID UNTIL NULL should succeed", "cthon98", "12345", true},
} {
t.Run("", func(t *testing.T) {
t.Run(tc.testName, func(t *testing.T) {
username := security.MakeSQLUsernameFromPreNormalizedString(tc.username)
valid, expired, err := ts.authentication.verifyPasswordDBConsole(context.Background(), username, tc.password)
if err != nil {
t.Errorf(
"credentials %s/%s failed with error %s, wanted no error",
tc.username,
tc.password,
err,
)
}
if valid && !expired != tc.shouldAuthenticate {
t.Errorf(
"credentials %s/%s valid = %t, wanted %t",
tc.username,
tc.password,
valid,
tc.shouldAuthenticate,
)
}
})
}
}

func TestVerifyPasswordDBConsole(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)

ctx := context.Background()
s, db, _ := serverutils.StartServer(t, base.TestServerArgs{})
defer s.Stopper().Stop(ctx)

ts := s.(*TestServer)

if util.RaceEnabled {
// The default bcrypt cost makes this test approximately 30s slower when the
// race detector is on.
security.BcryptCost.Override(ctx, &ts.Cfg.Settings.SV, int64(bcrypt.MinCost))
}

for _, user := range []struct {
username string
password string
loginFlag string
}{
{"azure_diamond", "hunter2", ""},
{"richardc", "12345", "NOLOGIN"},
{"richardc2", "12345", "NOSQLLOGIN"},
} {
username := security.MakeSQLUsernameFromPreNormalizedString(user.username)
cmd := fmt.Sprintf(
"CREATE USER %s WITH PASSWORD '%s' %s",
username.SQLIdentifier(), user.password, user.loginFlag)

if _, err := db.Exec(cmd); err != nil {
t.Fatalf("failed to create user: %s", err)
}
}

for _, tc := range []struct {
testName string
username string
password string
shouldAuthenticate bool
}{
{"valid login", "azure_diamond", "hunter2", true},
{"wrong password", "azure_diamond", "hunter", false},
{"empty password", "azure_diamond", "", false},

{"user with NOLOGIN role option should fail", "richardc", "12345", false},
// This is the one test case where SQL and DB Console login outcomes differ.
{"user with NOSQLLOGIN role option should succeed", "richardc2", "12345", true},
} {
t.Run(tc.testName, func(t *testing.T) {
username := security.MakeSQLUsernameFromPreNormalizedString(tc.username)
valid, expired, err := ts.authentication.verifyPassword(context.Background(), username, tc.password)
valid, expired, err := ts.authentication.verifyPasswordDBConsole(context.Background(), username, tc.password)
if err != nil {
t.Errorf(
"credentials %s/%s failed with error %s, wanted no error",
Expand Down
Loading

0 comments on commit f8997c5

Please sign in to comment.