From 8ddd724a7806c0fa5af8bbdfdbfc53de14a3630f Mon Sep 17 00:00:00 2001 From: richardjcai Date: Fri, 7 Feb 2020 14:57:27 -0500 Subject: [PATCH] Adding user account expiration and user/role login privilege. 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 #41396 --- docs/generated/sql/bnf/stmt_block.bnf | 10 +++ pkg/ccl/gssapiccl/gssapi.go | 1 + pkg/ccl/logictestccl/testdata/logic_test/role | 63 ++++++++++++++ pkg/server/authentication.go | 35 ++++++-- pkg/server/authentication_test.go | 52 +++++++++-- pkg/sql/alter_role.go | 36 ++++++-- pkg/sql/create_role.go | 33 ++++++- pkg/sql/delegate/show_users.go | 3 +- pkg/sql/drop_role.go | 9 +- pkg/sql/logictest/testdata/logic_test/user | 2 +- pkg/sql/opt/exec/execbuilder/testdata/explain | 4 +- pkg/sql/parser/parse_test.go | 4 + pkg/sql/parser/sql.y | 42 ++++++--- pkg/sql/pgwire/auth.go | 14 ++- pkg/sql/pgwire/auth_methods.go | 36 ++++++-- pkg/sql/pgwire/testdata/auth/conn_log | 36 +++++++- pkg/sql/planner.go | 11 ++- pkg/sql/roleoption/option_string.go | 7 +- pkg/sql/roleoption/role_option.go | 45 +++++++--- pkg/sql/sem/tree/create.go | 19 ++-- pkg/sql/user.go | 86 ++++++++++++++----- 21 files changed, 445 insertions(+), 103 deletions(-) diff --git a/docs/generated/sql/bnf/stmt_block.bnf b/docs/generated/sql/bnf/stmt_block.bnf index 93a9b6daa82d..0378e4e8bdfb 100644 --- a/docs/generated/sql/bnf/stmt_block.bnf +++ b/docs/generated/sql/bnf/stmt_block.bnf @@ -737,6 +737,7 @@ unreserved_keyword ::= | 'LIST' | 'LOCAL' | 'LOCKED' + | 'LOGIN' | 'LOOKUP' | 'LOW' | 'MATCH' @@ -754,6 +755,7 @@ unreserved_keyword ::= | 'NORMAL' | 'NO_INDEX_JOIN' | 'NOCREATEROLE' + | 'NOLOGIN' | 'NOWAIT' | 'NULLS' | 'IGNORE_FOREIGN_KEYS' @@ -870,6 +872,7 @@ unreserved_keyword ::= | 'UNKNOWN' | 'UNLOGGED' | 'UNSPLIT' + | 'UNTIL' | 'UPDATE' | 'UPSERT' | 'UUID' @@ -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 ) )* @@ -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' diff --git a/pkg/ccl/gssapiccl/gssapi.go b/pkg/ccl/gssapiccl/gssapi.go index 8dd3bfe5fb5b..70cb709baf83 100644 --- a/pkg/ccl/gssapiccl/gssapi.go +++ b/pkg/ccl/gssapiccl/gssapi.go @@ -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) { diff --git a/pkg/ccl/logictestccl/testdata/logic_test/role b/pkg/ccl/logictestccl/testdata/logic_test/role index 622761aedce7..5ca147886613 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/role +++ b/pkg/ccl/logictestccl/testdata/logic_test/role @@ -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 diff --git a/pkg/server/authentication.go b/pkg/server/authentication.go index cf2ef48408e0..0cd5a175a457 100644 --- a/pkg/server/authentication.go +++ b/pkg/server/authentication.go @@ -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" @@ -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, @@ -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. diff --git a/pkg/server/authentication_test.go b/pkg/server/authentication_test.go index 0d3048f43403..db076c2d51f9 100644 --- a/pkg/server/authentication_test.go +++ b/pkg/server/authentication_test.go @@ -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" @@ -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 { @@ -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) } } @@ -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", @@ -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, diff --git a/pkg/sql/alter_role.go b/pkg/sql/alter_role.go index 03ff2cdf496e..2b94c8b00083 100644 --- a/pkg/sql/alter_role.go +++ b/pkg/sql/alter_role.go @@ -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 } @@ -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 { @@ -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 } diff --git a/pkg/sql/create_role.go b/pkg/sql/create_role.go index d7559cfe5677..d2220115aaae 100644 --- a/pkg/sql/create_role.go +++ b/pkg/sql/create_role.go @@ -65,8 +65,15 @@ func (p *planner) CreateRoleNode( return nil, err } - var roleOptions roleoption.List - roleOptions, err := kvOptions.ToRoleOptions(p.TypeAsString, opName) + roleOptions, err := kvOptions.ToRoleOptions(p.TypeAsStringOrNull, opName) + + // Using CREATE ROLE syntax enables NOLOGIN by default. + if isRole && !roleOptions.Contains(roleoption.LOGIN) && + !roleOptions.Contains(roleoption.NOLOGIN) { + roleOptions = append(roleOptions, + roleoption.RoleOption{Option: roleoption.NOLOGIN, HasValue: false}) + } + if err != nil { return nil, err } @@ -167,19 +174,37 @@ func (n *CreateRoleNode) startExec(params runParams) error { ) } + // 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 := range stmts { + 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 diff --git a/pkg/sql/delegate/show_users.go b/pkg/sql/delegate/show_users.go index b7a70f211ef1..1c16a90c754f 100644 --- a/pkg/sql/delegate/show_users.go +++ b/pkg/sql/delegate/show_users.go @@ -15,5 +15,6 @@ import "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" // ShowUsers returns all the users. // Privileges: SELECT on system.users. func (d *delegator) delegateShowUsers(n *tree.ShowUsers) (tree.Statement, error) { - return parse(`SELECT username AS user_name FROM system.users WHERE "isRole" = false ORDER BY 1`) + return parse(`SELECT username AS user_name FROM ` + + `system.role_options WHERE option = 'LOGIN' ORDER BY 1`) } diff --git a/pkg/sql/drop_role.go b/pkg/sql/drop_role.go index d03b83d80e86..96fa2a616b90 100644 --- a/pkg/sql/drop_role.go +++ b/pkg/sql/drop_role.go @@ -227,15 +227,8 @@ func (n *DropRoleNode) startExec(params runParams) error { } } - if numRoleOptionsDeleted > 0 { - // Some role options have been deleted, bump role_options table version to - // force a refresh of role membership. - if err := params.p.BumpRoleOptionsTableVersion(params.ctx); err != nil { - return err - } - } - n.run.numDeleted = numUsersDeleted + n.run.numDeleted += numRoleOptionsDeleted return nil } diff --git a/pkg/sql/logictest/testdata/logic_test/user b/pkg/sql/logictest/testdata/logic_test/user index cd9c81eab80b..7450b6366cd2 100644 --- a/pkg/sql/logictest/testdata/logic_test/user +++ b/pkg/sql/logictest/testdata/logic_test/user @@ -103,7 +103,7 @@ CREATE USER user4 statement error pq: user testuser does not have INSERT privilege on relation users UPSERT INTO system.users VALUES (user1, 'newpassword', false) -statement error pq: user testuser does not have SELECT privilege on relation users +statement error pq: user testuser does not have SELECT privilege on relation role_options SHOW USERS query TTT diff --git a/pkg/sql/opt/exec/execbuilder/testdata/explain b/pkg/sql/opt/exec/execbuilder/testdata/explain index 9f3ce3428fa2..e51c39e994cf 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/explain +++ b/pkg/sql/opt/exec/execbuilder/testdata/explain @@ -362,9 +362,9 @@ EXPLAIN SHOW USERS · vectorized true render · · └── scan · · -· table users@primary +· table role_options@primary · spans ALL -· filter "isRole" = false +· filter option = 'LOGIN' # EXPLAIN selecting from a sequence. statement ok diff --git a/pkg/sql/parser/parse_test.go b/pkg/sql/parser/parse_test.go index 8b5acc582e50..ab0428fdc214 100644 --- a/pkg/sql/parser/parse_test.go +++ b/pkg/sql/parser/parse_test.go @@ -1957,6 +1957,10 @@ $function$`, `CREATE USER 'foo' WITH PASSWORD 'bar'`}, {`CREATE USER foo PASSWORD NULL`, `CREATE USER 'foo' WITH PASSWORD NULL`}, + {`CREATE USER foo LOGIN VALID UNTIL NULL PASSWORD NULL`, + `CREATE USER 'foo' WITH LOGIN VALID UNTIL NULL PASSWORD NULL`}, + {`CREATE USER foo VALID UNTIL '1970-01-01'`, + `CREATE USER 'foo' WITH VALID UNTIL '1970-01-01'`}, {`DROP USER foo, bar`, `DROP USER 'foo', 'bar'`}, {`DROP USER IF EXISTS foo, bar`, diff --git a/pkg/sql/parser/sql.y b/pkg/sql/parser/sql.y index 840c33b3ab34..17737886ede2 100644 --- a/pkg/sql/parser/sql.y +++ b/pkg/sql/parser/sql.y @@ -562,12 +562,12 @@ func newNameFromStr(s string) *tree.Name { %token LANGUAGE LAST LATERAL LC_CTYPE LC_COLLATE %token LEADING LEASE LEAST LEFT LESS LEVEL LIKE LIMIT LIST LOCAL -%token LOCALTIME LOCALTIMESTAMP LOCKED LOOKUP LOW LSHIFT +%token LOCALTIME LOCALTIMESTAMP LOCKED LOGIN LOOKUP LOW LSHIFT %token MATCH MATERIALIZED MERGE MINVALUE MAXVALUE MINUTE MONTH -%token NAN NAME NAMES NATURAL NEXT NO NOCREATEROLE NO_INDEX_JOIN NONE NORMAL -%token NOT NOTHING NOTNULL NOWAIT NULL NULLIF NULLS NUMERIC +%token NAN NAME NAMES NATURAL NEXT NO NOCREATEROLE NOLOGIN NO_INDEX_JOIN +%token NONE NORMAL NOT NOTHING NOTNULL NOWAIT NULL NULLIF NULLS NUMERIC %token OF OFF OFFSET OID OIDS OIDVECTOR ON ONLY OPT OPTION OPTIONS OR %token ORDER ORDINALITY OTHERS OUT OUTER OVER OVERLAPS OVERLAY OWNED OPERATOR @@ -598,7 +598,7 @@ func newNameFromStr(s string) *tree.Name { %token TRACING %token UNBOUNDED UNCOMMITTED UNION UNIQUE UNKNOWN UNLOGGED UNSPLIT -%token UPDATE UPSERT USE USER USERS USING UUID +%token UPDATE UPSERT UNTIL USE USER USERS USING UUID %token VALID VALIDATE VALUE VALUES VARBIT VARCHAR VARIADIC VIEW VARYING VIRTUAL @@ -836,7 +836,7 @@ func newNameFromStr(s string) *tree.Name { %type name opt_name opt_name_parens %type privilege savepoint_name -%type role_option password_clause +%type role_option password_clause valid_until_clause %type subquery_op %type <*tree.UnresolvedName> func_name %type opt_collate @@ -5202,11 +5202,20 @@ role_option: { $$.val = tree.KVOption{Key: tree.Name($1), Value: nil} } - | NOCREATEROLE - { - $$.val = tree.KVOption{Key: tree.Name($1), Value: nil} - } - | password_clause +| NOCREATEROLE + { + $$.val = tree.KVOption{Key: tree.Name($1), Value: nil} + } +| LOGIN + { + $$.val = tree.KVOption{Key: tree.Name($1), Value: nil} + } +| NOLOGIN + { + $$.val = tree.KVOption{Key: tree.Name($1), Value: nil} + } +| password_clause +| valid_until_clause role_options: role_option @@ -5228,6 +5237,16 @@ opt_role_options: $$.val = nil } +valid_until_clause: + VALID UNTIL string_or_placeholder + { + $$.val = tree.KVOption{Key: tree.Name(fmt.Sprintf("%s_%s",$1, $2)), Value: $3.expr()} + } +| VALID UNTIL NULL + { + $$.val = tree.KVOption{Key: tree.Name(fmt.Sprintf("%s_%s",$1, $2)), Value: tree.DNull} + } + opt_view_recursive: /* EMPTY */ { /* no error */ } | RECURSIVE { return unimplemented(sqllex, "create recursive view") } @@ -9822,6 +9841,7 @@ unreserved_keyword: | LIST | LOCAL | LOCKED +| LOGIN | LOOKUP | LOW | MATCH @@ -9839,6 +9859,7 @@ unreserved_keyword: | NORMAL | NO_INDEX_JOIN | NOCREATEROLE +| NOLOGIN | NOWAIT | NULLS | IGNORE_FOREIGN_KEYS @@ -9955,6 +9976,7 @@ unreserved_keyword: | UNKNOWN | UNLOGGED | UNSPLIT +| UNTIL | UPDATE | UPSERT | UUID diff --git a/pkg/sql/pgwire/auth.go b/pkg/sql/pgwire/auth.go index 5be379c950fc..b77baaaadaaa 100644 --- a/pkg/sql/pgwire/auth.go +++ b/pkg/sql/pgwire/auth.go @@ -13,6 +13,7 @@ package pgwire import ( "context" "crypto/tls" + "fmt" "net" "github.com/cockroachdb/cockroach/pkg/security" @@ -83,18 +84,25 @@ func (c *conn) handleAuthentication( // Check that the requested user exists and retrieve the hashed // password in case password authentication is needed. - exists, pwRetrievalFn, err := sql.GetUserHashedPassword( + exists, canLogin, pwRetrievalFn, validUntilFn, err := sql.GetUserHashedPassword( ctx, authOpt.ie, c.sessionArgs.User, ) if err != nil { ac.Logf(ctx, "user retrieval failed for user=%q: %v", c.sessionArgs.User, err) return sendError(err) } + if !exists { ac.Logf(ctx, "user does not exist: %q", c.sessionArgs.User) return sendError(errors.Errorf(security.ErrPasswordUserAuthFailed, c.sessionArgs.User)) } + if !canLogin { + ac.Logf(ctx, "%q does not have login privilege", c.sessionArgs.User) + return sendError(errors.Errorf( + fmt.Sprintf("%s does not have login privilege", c.sessionArgs.User))) + } + // Retrieve the authentication method. tlsState, hbaEntry, methodFn, err := c.findAuthenticationMethod(authOpt) if err != nil { @@ -104,7 +112,9 @@ func (c *conn) handleAuthentication( ac.Logf(ctx, "connection matches HBA rule: %s", hbaEntry.Input) // Ask the method to authenticate. - authenticationHook, err := methodFn(ctx, ac, tlsState, pwRetrievalFn, execCfg, hbaEntry) + authenticationHook, err := methodFn(ctx, ac, tlsState, pwRetrievalFn, + validUntilFn, execCfg, hbaEntry) + if err != nil { ac.Logf(ctx, "authentication pre-hook failed: %v", err) return sendError(err) diff --git a/pkg/sql/pgwire/auth_methods.go b/pkg/sql/pgwire/auth_methods.go index c8f3590d219c..0aa6529ca12b 100644 --- a/pkg/sql/pgwire/auth_methods.go +++ b/pkg/sql/pgwire/auth_methods.go @@ -21,6 +21,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/hba" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" + "github.com/cockroachdb/cockroach/pkg/util/timeutil" "github.com/cockroachdb/errors" ) @@ -69,6 +70,7 @@ type AuthMethod func( c AuthConn, tlsState tls.ConnectionState, pwRetrieveFn PasswordRetrievalFn, + pwValidUntilFn PasswordValidUntilFn, execCfg *sql.ExecutorConfig, entry *hba.Entry, ) (security.UserAuthHook, error) @@ -77,13 +79,18 @@ type AuthMethod func( // password for the user logging in. type PasswordRetrievalFn = func(context.Context) ([]byte, error) +// PasswordValidUntilFn defines a method to retrieve the expiration time +// of the user's password. +type PasswordValidUntilFn = func(context.Context) (*tree.DTimestamp, error) + func authPassword( ctx context.Context, c AuthConn, - tlsState tls.ConnectionState, + _ tls.ConnectionState, pwRetrieveFn PasswordRetrievalFn, - execCfg *sql.ExecutorConfig, - entry *hba.Entry, + pwValidUntilFn PasswordValidUntilFn, + _ *sql.ExecutorConfig, + _ *hba.Entry, ) (security.UserAuthHook, error) { if err := c.SendAuthRequest(authCleartextPassword, nil /* data */); err != nil { return nil, err @@ -104,6 +111,17 @@ func authPassword( c.Logf(ctx, "user has no password defined") } + validUntil, err := pwValidUntilFn(ctx) + if err != nil { + return nil, err + } + if validUntil != nil { + if validUntil.Sub(timeutil.Now()) < 0 { + c.Logf(ctx, "password is expired") + return nil, errors.New("password is expired") + } + } + return security.UserAuthPasswordHook( false /*insecure*/, password, hashedPassword, ), nil @@ -121,9 +139,10 @@ func authCert( _ context.Context, _ AuthConn, tlsState tls.ConnectionState, - pwRetrieveFn PasswordRetrievalFn, - execCfg *sql.ExecutorConfig, - entry *hba.Entry, + _ PasswordRetrievalFn, + _ PasswordValidUntilFn, + _ *sql.ExecutorConfig, + _ *hba.Entry, ) (security.UserAuthHook, error) { if len(tlsState.PeerCertificates) == 0 { return nil, errors.New("no TLS peer certificates, but required for auth") @@ -140,6 +159,7 @@ func authCertPassword( c AuthConn, tlsState tls.ConnectionState, pwRetrieveFn PasswordRetrievalFn, + pwValidUntilFn PasswordValidUntilFn, execCfg *sql.ExecutorConfig, entry *hba.Entry, ) (security.UserAuthHook, error) { @@ -151,7 +171,7 @@ func authCertPassword( c.Logf(ctx, "client presented certificate, proceeding with certificate validation") fn = authCert } - return fn(ctx, c, tlsState, pwRetrieveFn, execCfg, entry) + return fn(ctx, c, tlsState, pwRetrieveFn, pwValidUntilFn, execCfg, entry) } func authTrust( @@ -159,6 +179,7 @@ func authTrust( _ AuthConn, _ tls.ConnectionState, _ PasswordRetrievalFn, + _ PasswordValidUntilFn, _ *sql.ExecutorConfig, _ *hba.Entry, ) (security.UserAuthHook, error) { @@ -170,6 +191,7 @@ func authReject( _ AuthConn, _ tls.ConnectionState, _ PasswordRetrievalFn, + _ PasswordValidUntilFn, _ *sql.ExecutorConfig, _ *hba.Entry, ) (security.UserAuthHook, error) { diff --git a/pkg/sql/pgwire/testdata/auth/conn_log b/pkg/sql/pgwire/testdata/auth/conn_log index 51859cf3d87a..52bd215006b2 100644 --- a/pkg/sql/pgwire/testdata/auth/conn_log +++ b/pkg/sql/pgwire/testdata/auth/conn_log @@ -5,7 +5,10 @@ sql CREATE USER userpw WITH PASSWORD 'pass'; CREATE USER usernopw; ALTER USER root WITH PASSWORD 'secureabc'; -CREATE USER trusted +CREATE USER trusted; +CREATE USER usernologin WITH NOLOGIN PASSWORD '123'; +CREATE USER userexpired WITH PASSWORD '123' VALID UNTIL '2000-01-01' + ---- ok @@ -233,4 +236,35 @@ I: [n1,client=XXX,local] 71 disconnected; duration: XXX subtest end +subtest conn_unix/nologin_expired_password + +connect_unix user=usernologin password=123 +---- +ERROR: usernologin does not have login privilege + +authlog 4 +.*disconnected +---- +I: [n1,client=XXX] 72 received connection +I: [n1,client=XXX,local,user=usernologin] 73 "usernologin" does not have login privilege +I: [n1,client=XXX,local,user=usernologin] 74 session terminated; duration: XXX +I: [n1,client=XXX,local] 75 disconnected; duration: XXX + +connect_unix user=userexpired password=123 +---- +ERROR: password is expired + +authlog 6 +.*disconnected +---- +I: [n1,client=XXX] 76 received connection +I: [n1,client=XXX,local,user=userexpired] 77 connection matches HBA rule: local all all password # built-in CockroachDB default +I: [n1,client=XXX,local,user=userexpired] 78 password is expired +I: [n1,client=XXX,local,user=userexpired] 79 authentication pre-hook failed: password is expired +I: [n1,client=XXX,local,user=userexpired] 80 session terminated; duration: XXX +I: [n1,client=XXX,local] 81 disconnected; duration: XXX + +subtest end + subtest end + diff --git a/pkg/sql/planner.go b/pkg/sql/planner.go index 7c420078d0d2..80456864641b 100644 --- a/pkg/sql/planner.go +++ b/pkg/sql/planner.go @@ -459,7 +459,7 @@ func (p *planner) LookupTableByID(ctx context.Context, tableID sqlbase.ID) (row. // TypeAsString enforces (not hints) that the given expression typechecks as a // string and returns a function that can be called to get the string value // during (planNode).Start. -// To also allow NULLs to be returned, use typeAsStringOrNull() instead. +// To also allow NULLs to be returned, use TypeAsStringOrNull() instead. func (p *planner) TypeAsString(e tree.Expr, op string) (func() (string, error), error) { typedE, err := tree.TypeCheckAndRequire(e, &p.semaCtx, types.String, op) if err != nil { @@ -478,6 +478,15 @@ func (p *planner) TypeAsString(e tree.Expr, op string) (func() (string, error), }, nil } +// TypeAsStringOrNull is like TypeAsString but allows NULLs. +func (p *planner) TypeAsStringOrNull(e tree.Expr, op string) (func() (bool, string, error), error) { + typedE, err := tree.TypeCheckAndRequire(e, &p.semaCtx, types.String, op) + if err != nil { + return nil, err + } + return p.makeStringEvalFn(typedE), nil +} + func (p *planner) makeStringEvalFn(typedE tree.TypedExpr) func() (bool, string, error) { return func() (bool, string, error) { d, err := typedE.Eval(p.EvalContext()) diff --git a/pkg/sql/roleoption/option_string.go b/pkg/sql/roleoption/option_string.go index af1c8e4ef803..efc2f84f8b3c 100644 --- a/pkg/sql/roleoption/option_string.go +++ b/pkg/sql/roleoption/option_string.go @@ -11,11 +11,14 @@ func _() { _ = x[CREATEROLE-1] _ = x[NOCREATEROLE-2] _ = x[PASSWORD-3] + _ = x[LOGIN-4] + _ = x[NOLOGIN-5] + _ = x[VALIDUNTIL-6] } -const _Option_name = "CREATEROLENOCREATEROLEPASSWORD" +const _Option_name = "CREATEROLENOCREATEROLEPASSWORDLOGINNOLOGINVALIDUNTIL" -var _Option_index = [...]uint8{0, 10, 22, 30} +var _Option_index = [...]uint8{0, 10, 22, 30, 35, 42, 52} func (i Option) String() string { i -= 1 diff --git a/pkg/sql/roleoption/role_option.go b/pkg/sql/roleoption/role_option.go index 3696fc1c4756..0bddcec0f160 100644 --- a/pkg/sql/roleoption/role_option.go +++ b/pkg/sql/roleoption/role_option.go @@ -29,8 +29,7 @@ type RoleOption struct { Option HasValue bool // Need to resolve value in Exec for the case of placeholders. - Value func() (string, error) - IsNull bool + Value func() (bool, string, error) } // KindList of role options. @@ -39,13 +38,19 @@ const ( CREATEROLE NOCREATEROLE PASSWORD + LOGIN + NOLOGIN + VALIDUNTIL ) -// toSQLStmt is a map of Kind -> SQL statement string for applying the +// toSQLStmts is a map of Kind -> SQL statement string for applying the // option to the role. -var toSQLStmt = map[Option]string{ +var toSQLStmts = map[Option]string{ CREATEROLE: `UPSERT INTO system.role_options (username, option) VALUES ($1, 'CREATEROLE')`, NOCREATEROLE: `DELETE FROM system.role_options WHERE username = $1 AND option = 'CREATEROLE'`, + LOGIN: `DELETE FROM system.role_options WHERE username = $1 AND option = 'NOLOGIN'`, + NOLOGIN: `UPSERT INTO system.role_options (username, option) VALUES ($1, 'NOLOGIN')`, + VALIDUNTIL: `UPSERT INTO system.role_options (username, option, value) VALUES ($1, 'VALID UNTIL', $2::timestamptz::string)`, } // Mask returns the bitmask for a given role option. @@ -58,6 +63,9 @@ var ByName = map[string]Option{ "CREATEROLE": CREATEROLE, "NOCREATEROLE": NOCREATEROLE, "PASSWORD": PASSWORD, + "LOGIN": LOGIN, + "NOLOGIN": NOLOGIN, + "VALID_UNTIL": VALIDUNTIL, } // ToOption takes a string and returns the corresponding Option. @@ -73,13 +81,16 @@ func ToOption(str string) (Option, error) { // List is a list of role options. type List []RoleOption -// GetSQLStmts returns a list of SQL stmts to apply each role option. -func (rol List) GetSQLStmts() (stmts []string, err error) { +// GetSQLStmts returns a map of SQL stmts to apply each role option. +// Maps stmts to values (value of the role option). +func (rol List) GetSQLStmts() (map[string]func() (bool, string, error), error) { if len(rol) <= 0 { - return stmts, nil + return nil, nil } - err = rol.CheckRoleOptionConflicts() + stmts := make(map[string]func() (bool, string, error), len(rol)) + + err := rol.CheckRoleOptionConflicts() if err != nil { return stmts, err } @@ -92,7 +103,13 @@ func (rol List) GetSQLStmts() (stmts []string, err error) { if ro.Option == PASSWORD { continue } - stmts = append(stmts, toSQLStmt[ro.Option]) + + stmt := toSQLStmts[ro.Option] + if ro.HasValue { + stmts[stmt] = ro.Value + } else { + stmts[stmt] = nil + } } return stmts, nil @@ -130,8 +147,10 @@ func (rol List) CheckRoleOptionConflicts() error { return err } - if roleOptionBits&CREATEROLE.Mask() != 0 && - roleOptionBits&NOCREATEROLE.Mask() != 0 { + if (roleOptionBits&CREATEROLE.Mask() != 0 && + roleOptionBits&NOCREATEROLE.Mask() != 0) || + (roleOptionBits&LOGIN.Mask() != 0 && + roleOptionBits&NOLOGIN.Mask() != 0) { return pgerror.Newf(pgcode.Syntax, "conflicting role options") } return nil @@ -143,11 +162,11 @@ func (rol List) GetHashedPassword() ([]byte, error) { var hashedPassword []byte for _, ro := range rol { if ro.Option == PASSWORD { - if ro.IsNull { + isNull, password, err := ro.Value() + if isNull { // Use empty byte array for hashedPassword. return hashedPassword, nil } - password, err := ro.Value() if err != nil { return hashedPassword, err } diff --git a/pkg/sql/sem/tree/create.go b/pkg/sql/sem/tree/create.go index b18ea5be2cd7..d370f1ca36e3 100644 --- a/pkg/sql/sem/tree/create.go +++ b/pkg/sql/sem/tree/create.go @@ -1236,7 +1236,7 @@ const ( // ToRoleOptions converts KVOptions to a roleoption.List using // typeAsString to convert exprs to strings. func (o KVOptions) ToRoleOptions( - typeAsString func(e Expr, op string) (func() (string, error), error), op string, + typeAsStringOrNull func(e Expr, op string) (func() (bool, string, error), error), op string, ) (roleoption.List, error) { roleOptions := make(roleoption.List, len(o)) @@ -1249,10 +1249,12 @@ func (o KVOptions) ToRoleOptions( if ro.Value != nil { if ro.Value == DNull { roleOptions[i] = roleoption.RoleOption{ - Option: option, HasValue: true, IsNull: true, + Option: option, HasValue: true, Value: func() (bool, string, error) { + return true, "", nil + }, } } else { - strFn, err := typeAsString(ro.Value, op) + strFn, err := typeAsStringOrNull(ro.Value, op) if err != nil { return nil, err } @@ -1261,7 +1263,7 @@ func (o KVOptions) ToRoleOptions( return nil, err } roleOptions[i] = roleoption.RoleOption{ - Option: option, Value: strFn, HasValue: true, IsNull: false, + Option: option, Value: strFn, HasValue: true, } } } else { @@ -1277,7 +1279,11 @@ func (o KVOptions) ToRoleOptions( func (o *KVOptions) formatAsRoleOptions(ctx *FmtCtx) { for _, option := range *o { ctx.WriteString(" ") - ctx.WriteString(strings.ToUpper(option.Key.String())) + ctx.WriteString( + // "_" replaces space (" ") in YACC for handling tree.Name formatting. + strings.ReplaceAll( + strings.ToUpper(option.Key.String()), "_", " "), + ) // Password is a special case. if strings.ToUpper(option.Key.String()) == "PASSWORD" { @@ -1290,6 +1296,9 @@ func (o *KVOptions) formatAsRoleOptions(ctx *FmtCtx) { } else if option.Value == DNull { ctx.WriteString(" ") ctx.FormatNode(option.Value) + } else if option.Value != nil { + ctx.WriteString(" ") + ctx.FormatNode(option.Value) } } } diff --git a/pkg/sql/user.go b/pkg/sql/user.go index 52925e50e386..575a4a2031b1 100644 --- a/pkg/sql/user.go +++ b/pkg/sql/user.go @@ -20,6 +20,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sqlbase" "github.com/cockroachdb/cockroach/pkg/util/contextutil" "github.com/cockroachdb/cockroach/pkg/util/log" + "github.com/cockroachdb/cockroach/pkg/util/timeutil" "github.com/cockroachdb/errors" ) @@ -53,7 +54,9 @@ func GetUserHashedPassword( ctx context.Context, ie *InternalExecutor, username string, ) ( exists bool, + canLogin bool, pwRetrieveFn func(ctx context.Context) (hashedPassword []byte, err error), + validUntilFn func(ctx context.Context) (timestamp *tree.DTimestamp, err error), err error, ) { normalizedUsername := tree.Name(username).Normalize() @@ -64,21 +67,29 @@ func GetUserHashedPassword( // immediately, and delay retrieving the password until strictly // necessary. rootFn := func(ctx context.Context) ([]byte, error) { - _, hashedPassword, err := retrieveUserAndPassword(ctx, ie, isRoot, normalizedUsername) + _, _, hashedPassword, _, err := retrieveUserAndPassword(ctx, ie, isRoot, normalizedUsername) return hashedPassword, err } - return true, rootFn, nil + + // Root user cannot have password expiry and must have login. + validUntilFn := func(ctx context.Context) (*tree.DTimestamp, error) { + return nil, nil + } + return true, true, rootFn, validUntilFn, nil } // Other users must reach for system.users no matter what, because // only that contains the truth about whether the user exists. - exists, hashedPassword, err := retrieveUserAndPassword(ctx, ie, isRoot, normalizedUsername) - return exists, func(ctx context.Context) ([]byte, error) { return hashedPassword, nil }, err + exists, canLogin, hashedPassword, validUntil, err := retrieveUserAndPassword(ctx, ie, isRoot, normalizedUsername) + return exists, canLogin, + func(ctx context.Context) ([]byte, error) { return hashedPassword, nil }, + func(ctx context.Context) (*tree.DTimestamp, error) { return validUntil, nil }, + err } func retrieveUserAndPassword( ctx context.Context, ie *InternalExecutor, isRoot bool, normalizedUsername string, -) (exists bool, hashedPassword []byte, err error) { +) (exists bool, canLogin bool, hashedPassword []byte, validUntil *tree.DTimestamp, err error) { // We may be operating with a timeout. timeout := userLoginTimeout.Get(&ie.s.cfg.Settings.SV) // We don't like long timeouts for root. @@ -94,10 +105,10 @@ func retrieveUserAndPassword( return contextutil.RunWithTimeout(ctx, "get-user-timeout", timeout, fn) } } - + var isRole bool // Perform the lookup with a timeout. err = runFn(func(ctx context.Context) error { - const getHashedPassword = `SELECT "hashedPassword" FROM system.users ` + + const getHashedPassword = `SELECT "hashedPassword", "isRole" FROM system.users ` + `WHERE username=$1` values, err := ie.QueryRowEx( ctx, "get-hashed-pwd", nil, /* txn */ @@ -109,7 +120,53 @@ func retrieveUserAndPassword( if values != nil { exists = true hashedPassword = []byte(*(values[0].(*tree.DBytes))) + isRole = bool(*(values[1].(*tree.DBool))) + } + + if !exists { + return nil + } + + getLoginDependencies := `SELECT option, value FROM system.role_options ` + + `WHERE username=$1 AND option IN ('NOLOGIN', 'VALID UNTIL')` + + loginDependencies, err := ie.QueryEx( + ctx, "get-login-dependencies", nil, /* txn */ + sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser}, + getLoginDependencies, + normalizedUsername, + ) + if err != nil { + return errors.Wrapf(err, "error looking up user %s", normalizedUsername) } + + // To support users created before 20.1, allow all USERS to login + // if NOLOGIN is not found. Only allow roles to LOGIN if login is found. + var noLoginFound = false + for _, row := range loginDependencies { + option := string(tree.MustBeDString(row[0])) + + if option == "NOLOGIN" { + noLoginFound = true + } + + if option == "VALID UNTIL" { + if tree.DNull.Compare(nil, row[1]) != 0 { + ts := string(tree.MustBeDString(row[1])) + // This is okay because the VALID UNTIL is stored as a string + // representation of a TimestampTZ which has the same underlying + // representation in the table as a Timestamp (UTC time). + timeCtx := tree.NewParseTimeContext(timeutil.Now()) + validUntil, err = tree.ParseDTimestamp(timeCtx, ts, time.Microsecond) + if err != nil { + return err + } + } + } + } + + canLogin = !noLoginFound && !isRole + return nil }) @@ -118,7 +175,7 @@ func retrieveUserAndPassword( log.Warningf(ctx, "user lookup for %q failed: %v", normalizedUsername, err) err = errors.HandledWithMessage(err, "internal error while retrieving user account") } - return exists, hashedPassword, err + return exists, canLogin, hashedPassword, validUntil, err } var userLoginTimeout = settings.RegisterPublicNonNegativeDurationSetting( @@ -129,7 +186,7 @@ var userLoginTimeout = settings.RegisterPublicNonNegativeDurationSetting( // GetAllRoles returns a "set" (map) of Roles -> true. func (p *planner) GetAllRoles(ctx context.Context) (map[string]bool, error) { - query := `SELECT username,"isRole" FROM system.users` + query := `SELECT username FROM system.users` rows, err := p.ExtendedEvalContext().ExecCfg.InternalExecutor.QueryEx( ctx, "read-users", p.txn, sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser}, @@ -158,14 +215,3 @@ func (p *planner) BumpRoleMembershipTableVersion(ctx context.Context) error { return p.writeSchemaChange(ctx, tableDesc, sqlbase.InvalidMutationID) } - -// BumpRoleOptionsTableVersion increases the table version for the -// role options table. -func (p *planner) BumpRoleOptionsTableVersion(ctx context.Context) error { - tableDesc, err := p.ResolveMutableTableDescriptor(ctx, RoleOptionsTableName, true, ResolveAnyDescType) - if err != nil { - return err - } - - return p.writeSchemaChange(ctx, tableDesc, sqlbase.InvalidMutationID) -}