Skip to content

Commit

Permalink
Adding user account expiration and user/role login privilege.
Browse files Browse the repository at this point in the history
Release note (sql change): Added LOGIN option to enable login for roles/users.
When using CREATE USER syntax, LOGIN is enabled by default whereas when using CREATE ROLE,
LOGIN is not enabled by default.

Release note (sql change): Added VALID UNTIL option to indicate passwords are only valid until a given timestamp.
VALID UNTIL only affects PASSWORDS - if passwords are disabled, VALID UNTIL has no effect.
(Can login even with expired password if passwords are not required to login - same as PG)

Release note (sql change): SHOW USERS now shows all roles/users that have LOGIN enabled.

Release note (security update): User and role principals can now be prevented from logging in
using the NOLOGIN attribute which can be set using ALTER USER/ROLE.

Release note (security update): The password field (used exclusively for password-based authentication)
can now be configured to have an expiry date using the VALID UNTIL attribute, which can be set with ALTER USER/ROLE.
Note that the attribute sets an expiry date for the password, not the user account.
This is consistent with PostgreSQL.

Release note (security update): Any role created prior to 20.1 will be able to log in into
clusters started with --insecure, unless/until they are granted the NOLOGIN privilege.

Fixes cockroachdb#41396
  • Loading branch information
RichardJCai committed Mar 6, 2020
1 parent 6108aba commit 8ddd724
Show file tree
Hide file tree
Showing 21 changed files with 445 additions and 103 deletions.
10 changes: 10 additions & 0 deletions docs/generated/sql/bnf/stmt_block.bnf
Original file line number Diff line number Diff line change
Expand Up @@ -737,6 +737,7 @@ unreserved_keyword ::=
| 'LIST'
| 'LOCAL'
| 'LOCKED'
| 'LOGIN'
| 'LOOKUP'
| 'LOW'
| 'MATCH'
Expand All @@ -754,6 +755,7 @@ unreserved_keyword ::=
| 'NORMAL'
| 'NO_INDEX_JOIN'
| 'NOCREATEROLE'
| 'NOLOGIN'
| 'NOWAIT'
| 'NULLS'
| 'IGNORE_FOREIGN_KEYS'
Expand Down Expand Up @@ -870,6 +872,7 @@ unreserved_keyword ::=
| 'UNKNOWN'
| 'UNLOGGED'
| 'UNSPLIT'
| 'UNTIL'
| 'UPDATE'
| 'UPSERT'
| 'UUID'
Expand Down Expand Up @@ -1768,7 +1771,10 @@ sequence_option_list ::=
role_option ::=
'CREATEROLE'
| 'NOCREATEROLE'
| 'LOGIN'
| 'NOLOGIN'
| password_clause
| valid_until_clause

single_table_pattern_list ::=
( table_name ) ( ( ',' table_name ) )*
Expand Down Expand Up @@ -2053,6 +2059,10 @@ password_clause ::=
'PASSWORD' string_or_placeholder
| 'PASSWORD' 'NULL'

valid_until_clause ::=
'VALID' 'UNTIL' string_or_placeholder
| 'VALID' 'UNTIL' 'NULL'

opt_asc_desc ::=
'ASC'
| 'DESC'
Expand Down
1 change: 1 addition & 0 deletions pkg/ccl/gssapiccl/gssapi.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ func authGSS(
c pgwire.AuthConn,
tlsState tls.ConnectionState,
_ pgwire.PasswordRetrievalFn,
_ pgwire.PasswordValidUntilFn,
execCfg *sql.ExecutorConfig,
entry *hba.Entry,
) (security.UserAuthHook, error) {
Expand Down
63 changes: 63 additions & 0 deletions pkg/ccl/logictestccl/testdata/logic_test/role
Original file line number Diff line number Diff line change
Expand Up @@ -961,3 +961,66 @@ user testuser

statement ok
CREATE ROLE rolej

# Testing LOGIN and VALID UNTIL role privilege
user root

statement ok
DELETE FROM system.role_options WHERE NOT username in ('root', 'admin')

statement ok
CREATE ROLE rolewithlogin LOGIN

query TTT
SELECT * FROM system.role_options
----
admin CREATEROLE NULL
root CREATEROLE NULL

statement ok
CREATE ROLE rolewithnologin NOLOGIN

query TTT
SELECT * FROM system.role_options
----
admin CREATEROLE NULL
rolewithnologin NOLOGIN NULL
root CREATEROLE NULL

statement ok
ALTER ROLE rolewithlogin VALID UNTIL '2020-01-01'

query TTT
SELECT * FROM system.role_options
----
admin CREATEROLE NULL
rolewithlogin VALID UNTIL 2020-01-01 00:00:00+00:00
rolewithnologin NOLOGIN NULL
root CREATEROLE NULL

statement ok
ALTER ROLE rolewithlogin VALID UNTIL NULL

query TTT
SELECT * FROM system.role_options
----
admin CREATEROLE NULL
rolewithlogin VALID UNTIL NULL
rolewithnologin NOLOGIN NULL
root CREATEROLE NULL

statement ok
DROP ROLE rolewithlogin

query TTT
SELECT * FROM system.role_options
----
admin CREATEROLE NULL
rolewithnologin NOLOGIN NULL
root CREATEROLE NULL

statement error pq: conflicting role options
CREATE ROLE thisshouldntwork LOGIN NOLOGIN

statement error pq: redundant role options
CREATE ROLE thisshouldntwork LOGIN LOGIN
35 changes: 27 additions & 8 deletions pkg/server/authentication.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/types"
"github.com/cockroachdb/cockroach/pkg/util/log"
"github.com/cockroachdb/cockroach/pkg/util/protoutil"
"github.com/cockroachdb/cockroach/pkg/util/timeutil"
gwruntime "github.com/grpc-ecosystem/grpc-gateway/runtime"
"github.com/pkg/errors"
"google.golang.org/grpc"
Expand Down Expand Up @@ -103,10 +104,17 @@ func (s *authenticationServer) UserLogin(
}

// Verify the provided username/password pair.
verified, err := s.verifyPassword(ctx, username, req.Password)
verified, expired, err := s.verifyPassword(ctx, username, req.Password)
if err != nil {
return nil, apiInternalError(ctx, err)
}
if expired {
return nil, status.Errorf(
codes.Unauthenticated,
"the password for %s has expired",
username,
)
}
if !verified {
return nil, status.Errorf(
codes.Unauthenticated,
Expand Down Expand Up @@ -254,21 +262,32 @@ WHERE id = $1`
// not be completed.
func (s *authenticationServer) verifyPassword(
ctx context.Context, username string, password string,
) (bool, error) {
exists, pwRetrieveFn, err := sql.GetUserHashedPassword(
) (valid bool, expired bool, err error) {
exists, canLogin, pwRetrieveFn, validUntilFn, err := sql.GetUserHashedPassword(
ctx, s.server.execCfg.InternalExecutor, username,
)
if err != nil {
return false, err
return false, false, err
}
if !exists {
return false, nil
if !exists || !canLogin {
return false, false, nil
}
hashedPassword, err := pwRetrieveFn(ctx)
if err != nil {
return false, err
return false, false, err
}
return (security.CompareHashAndPassword(hashedPassword, password) == nil), nil

validUntil, err := validUntilFn(ctx)
if err != nil {
return false, false, err
}
if validUntil != nil {
if validUntil.Time.Sub(timeutil.Now()) < 0 {
return false, true, nil
}
}

return security.CompareHashAndPassword(hashedPassword, password) == nil, false, nil
}

// CreateAuthSecret creates a secret, hash pair to populate a session auth token.
Expand Down
52 changes: 43 additions & 9 deletions pkg/server/authentication_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/util"
"github.com/cockroachdb/cockroach/pkg/util/httputil"
"github.com/cockroachdb/cockroach/pkg/util/leaktest"
"github.com/cockroachdb/cockroach/pkg/util/timeutil"
"github.com/gogo/protobuf/jsonpb"
"github.com/lib/pq"
"github.com/pkg/errors"
Expand Down Expand Up @@ -173,8 +174,11 @@ func TestSSLEnforcement(t *testing.T) {

func TestVerifyPassword(t *testing.T) {
defer leaktest.AfterTest(t)()

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

ts := s.(*TestServer)

if util.RaceEnabled {
Expand All @@ -184,15 +188,37 @@ func TestVerifyPassword(t *testing.T) {
security.BcryptCost = bcrypt.MinCost
}

//location is used for timezone testing.
shanghaiLoc, err := time.LoadLocation("Asia/Shanghai")
if err != nil {
t.Fatal(err)
}

for _, user := range []struct {
username string
password string
username string
password string
loginFlag string
validUntilClause string
qargs []interface{}
}{
{"azure_diamond", "hunter2"},
{"druidia", "12345"},
{"azure_diamond", "hunter2", "", "", nil},
{"druidia", "12345", "", "", nil},

{"richardc", "12345", "NOLOGIN", "", nil},
{"epoch", "12345", "", "VALID UNTIL '1970-01-01'", nil},
{"cockroach", "12345", "", "VALID UNTIL '2100-01-01'", nil},
{"cthon98", "12345", "", "VALID UNTIL NULL", nil},

{"toolate", "12345", "", "VALID UNTIL $1",
[]interface{}{timeutil.Now().Add(-10 * time.Minute)}},
{"timelord", "12345", "", "VALID UNTIL $1",
[]interface{}{timeutil.Now().Add(59 * time.Minute).In(shanghaiLoc)}},
} {
cmd := fmt.Sprintf("CREATE USER %s WITH PASSWORD '%s'", user.username, user.password)
if _, err := db.Exec(cmd); err != nil {
cmd := fmt.Sprintf(
"CREATE USER %s WITH PASSWORD '%s' %s %s",
user.username, user.password, user.loginFlag, user.validUntilClause)

if _, err := db.Exec(cmd, user.qargs...); err != nil {
t.Fatalf("failed to create user: %s", err)
}
}
Expand All @@ -216,9 +242,17 @@ func TestVerifyPassword(t *testing.T) {
{"root", "", false, "crypto/bcrypt"},
{"", "", false, "does not exist"},
{"doesntexist", "zxcvbn", false, "does not exist"},

{"richardc", "12345", false,
"richardc does not have login privilege"},
{"epoch", "12345", false, ""},
{"cockroach", "12345", true, ""},
{"toolate", "12345", false, ""},
{"timelord", "12345", true, ""},
{"cthon98", "12345", true, ""},
} {
t.Run("", func(t *testing.T) {
valid, err := ts.authentication.verifyPassword(context.TODO(), tc.username, tc.password)
valid, expired, err := ts.authentication.verifyPassword(context.TODO(), tc.username, tc.password)
if err != nil {
t.Errorf(
"credentials %s/%s failed with error %s, wanted no error",
Expand All @@ -227,7 +261,7 @@ func TestVerifyPassword(t *testing.T) {
err,
)
}
if valid != tc.shouldAuthenticate {
if valid && !expired != tc.shouldAuthenticate {
t.Errorf(
"credentials %s/%s valid = %t, wanted %t",
tc.username,
Expand Down
36 changes: 27 additions & 9 deletions pkg/sql/alter_role.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ func (p *planner) AlterRoleNode(
return nil, err
}

roleOptions, err := kvOptions.ToRoleOptions(p.TypeAsString, opName)
roleOptions, err := kvOptions.ToRoleOptions(p.TypeAsStringOrNull, opName)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -128,11 +128,6 @@ func (n *alterRoleNode) startExec(params runParams) error {
return errors.Newf("role/user %s does not exist", normalizedUsername)
}

stmts, err := n.roleOptions.GetSQLStmts()
if err != nil {
return err
}

if n.roleOptions.Contains(roleoption.PASSWORD) {
hashedPassword, err := n.roleOptions.GetHashedPassword()
if err != nil {
Expand Down Expand Up @@ -175,21 +170,44 @@ func (n *alterRoleNode) startExec(params runParams) error {
}
}

for _, stmt := range stmts {
// Get a map of statements to execute for role options and their values.
stmts, err := n.roleOptions.GetSQLStmts()
if err != nil {
return err
}

for stmt, value := range stmts {
qargs := []interface{}{normalizedUsername}

if value != nil {
isNull, val, err := value()
if err != nil {
return err
}
if isNull {
// If the value of the role option is NULL, ensure that nil is passed
// into the statement placeholder, since val is string type "NULL"
// will not be interpreted as NULL by the InternalExecutor.
qargs = append(qargs, nil)
} else {
qargs = append(qargs, val)
}
}

rowsAffected, err := params.extendedEvalCtx.ExecCfg.InternalExecutor.ExecEx(
params.ctx,
opName,
params.p.txn,
sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser},
stmt,
normalizedUsername,
qargs...,
)
if err != nil {
return err
}

n.run.rowsAffected += rowsAffected
}

return nil
}

Expand Down
Loading

0 comments on commit 8ddd724

Please sign in to comment.