Skip to content

Commit

Permalink
Merge cockroachdb#96464 cockroachdb#96466
Browse files Browse the repository at this point in the history 
96464: multiregionccl: skip TestColdStartLatency r=ajwerner a=ajwerner

See cockroachdb#96334

Epic: None

Release note: None

96466: sql,auth: fix broken NOSQLLOGIN global privilege r=ecwall a=rafiss

fixes cockroachdb#96465

In addition to the bug fix, this cleans up some code so we don't have to construct planners on the fly during authentication.

Tests were added, since the initial PR that added the NOSQLLOGIN global privilege did not have any tests.

Release note (bug fix): The global NOSQLLOGIN privilege had a bug that made CockroachDB ignore it entirely, so it had no effect. This is now fixed. The bug was introduced in v22.2.0-alpha.1. The NOSQLLOGIN role option is unaffected by this bug.

Co-authored-by: Andrew Werner <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>
  • Loading branch information
@ajwerner @rafiss
3 people committed Feb 3, 2023
3 parents ca70b82 + 39a54b6 + b0b396d commit be08998
Show file tree
Hide file tree
Showing 9 changed files with 111 additions and 107 deletions.
1 change: 1 addition & 0 deletions pkg/ccl/multiregionccl/cold_start_latency_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ import (
func TestColdStartLatency(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)
skip.WithIssue(t, 96334)
skip.UnderRace(t, "too slow")
skip.UnderStress(t, "too slow")
defer envutil.TestSetEnv(t, "COCKROACH_MR_SYSTEM_DATABASE", "1")()
Expand Down
13 changes: 12 additions & 1 deletion pkg/ccl/serverccl/role_authentication_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ func TestVerifyPassword(t *testing.T) {

{"richardc", "12345", "NOLOGIN", "", nil},
{"richardc2", "12345", "NOSQLLOGIN", "", nil},
{"has_global_nosqlogin", "12345", "", "", nil},
{"inherits_global_nosqlogin", "12345", "", "", 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 @@ -95,6 +97,12 @@ func TestVerifyPassword(t *testing.T) {
t.Fatalf("failed to grant admin: %s", err)
}

// Set up NOSQLLOGIN global privilege.
_, err = db.Exec("GRANT SYSTEM NOSQLLOGIN TO has_global_nosqlogin")
require.NoError(t, err)
_, err = db.Exec("GRANT has_global_nosqlogin TO inherits_global_nosqlogin")
require.NoError(t, err)

for _, tc := range []struct {
testName string
username string
Expand All @@ -119,8 +127,11 @@ func TestVerifyPassword(t *testing.T) {
{"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.
// The NOSQLLOGIN cases are the only cases 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 NOSQLLOGIN global privilege should fail SQL but succeed on DB Console", "has_global_nosqlogin", "12345", false, false, true},
{"user who inherits NOSQLLOGIN global privilege should fail SQL but succeed on DB Console", "inherits_global_nosqlogin", "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},
Expand Down
13 changes: 12 additions & 1 deletion pkg/server/authentication_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,8 @@ func TestVerifyPasswordDBConsole(t *testing.T) {

{"richardc", "12345", "NOLOGIN", "", nil},
{"richardc2", "12345", "NOSQLLOGIN", "", nil},
{"has_global_nosqlogin", "12345", "", "", nil},
{"inherits_global_nosqlogin", "12345", "", "", 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 @@ -246,6 +248,12 @@ func TestVerifyPasswordDBConsole(t *testing.T) {
}
}

// Set up NOSQLLOGIN global privilege.
_, err = db.Exec("GRANT SYSTEM NOSQLLOGIN TO has_global_nosqlogin")
require.NoError(t, err)
_, err = db.Exec("GRANT has_global_nosqlogin TO inherits_global_nosqlogin")
require.NoError(t, err)

for _, tc := range []struct {
testName string
username string
Expand All @@ -267,8 +275,11 @@ func TestVerifyPasswordDBConsole(t *testing.T) {
{"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.
// The NOSQLLOGIN cases are the only cases where SQL and DB Console login outcomes differ.
{"user with NOSQLLOGIN role option should succeed", "richardc2", "12345", true},
{"user with NOSQLLOGIN global privilege should succeed", "has_global_nosqlogin", "12345", true},
{"user who inherits NOSQLLOGIN global privilege should succeed", "inherits_global_nosqlogin", "12345", true},

{"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},
Expand Down
14 changes: 7 additions & 7 deletions pkg/sql/authorization.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,15 +141,15 @@ func (p *planner) HasPrivilege(
return false, errors.AssertionFailedf("cannot use CheckPrivilege without a txn")
}

// root, admin and node user should always have privileges.
// However, this allows us to short-circuit privilege checks for
// root, admin and node user should always have privileges, except NOSQLLOGIN.
// This allows us to short-circuit privilege checks for
// virtual object such that we don't have to query the system.privileges
// table. This is especially import for internal executor queries.
// Right now we only short-circuit non-descriptor backed objects.
// There are certain descriptor backed objects that we can't
// short-circuit, ie system tables.
// table. This is especially import for internal executor queries. Right now
// we only short-circuit non-descriptor backed objects. There are certain
// descriptor backed objects that we can't short-circuit, ie system tables.
if (user.IsRootUser() || user.IsAdminRole() || user.IsNodeUser()) &&
!privilegeObject.GetObjectType().IsDescriptorBacked() {
!privilegeObject.GetObjectType().IsDescriptorBacked() &&
privilegeKind != privilege.NOSQLLOGIN {
if privilege.GetValidPrivilegesForObject(
privilegeObject.GetObjectType(),
).Contains(privilegeKind) {
Expand Down
37 changes: 31 additions & 6 deletions pkg/sql/pgwire/testdata/auth/conn_log
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ sql
CREATE USER trusted;
CREATE USER usernologin WITH NOLOGIN PASSWORD '123';
CREATE USER usernosqllogin WITH NOSQLLOGIN PASSWORD '123';
CREATE USER userglobalnosqllogin WITH NOSQLLOGIN PASSWORD '123';
CREATE USER userinheritsnosqllogin WITH NOSQLLOGIN PASSWORD '123';
CREATE USER userexpired WITH PASSWORD '123' VALID UNTIL '2000-01-01'
----
ok
Expand Down Expand Up @@ -278,6 +280,29 @@ authlog 4
88 {"Duration":"NNN","EventType":"client_session_end","InstanceID":1,"Network":"unix","RemoteAddress":"XXX","Timestamp":"XXX"}
89 {"Duration":"NNN","EventType":"client_connection_end","InstanceID":1,"Network":"unix","RemoteAddress":"XXX","Timestamp":"XXX"}

connect_unix user=userglobalnosqllogin password=123
----
ERROR: userglobalnosqllogin does not have login privilege (SQLSTATE 28000)

authlog 4
.*client_connection_end
----
91 {"EventType":"client_authentication_info","Info":"HBA rule: local all all password # built-in CockroachDB default","InstanceID":1,"Method":"password","Network":"unix","RemoteAddress":"XXX","SystemIdentity":"userglobalnosqllogin","Timestamp":"XXX","Transport":"local"}
92 {"EventType":"client_authentication_failed","InstanceID":1,"Method":"password","Network":"unix","Reason":3,"RemoteAddress":"XXX","SystemIdentity":"userglobalnosqllogin","Timestamp":"XXX","Transport":"local","User":"userglobalnosqllogin"}
93 {"Duration":"NNN","EventType":"client_session_end","InstanceID":1,"Network":"unix","RemoteAddress":"XXX","Timestamp":"XXX"}
94 {"Duration":"NNN","EventType":"client_connection_end","InstanceID":1,"Network":"unix","RemoteAddress":"XXX","Timestamp":"XXX"}

connect_unix user=userinheritsnosqllogin password=123
----
ERROR: userinheritsnosqllogin does not have login privilege (SQLSTATE 28000)

authlog 4
.*client_connection_end
----
96 {"EventType":"client_authentication_info","Info":"HBA rule: local all all password # built-in CockroachDB default","InstanceID":1,"Method":"password","Network":"unix","RemoteAddress":"XXX","SystemIdentity":"userinheritsnosqllogin","Timestamp":"XXX","Transport":"local"}
97 {"EventType":"client_authentication_failed","InstanceID":1,"Method":"password","Network":"unix","Reason":3,"RemoteAddress":"XXX","SystemIdentity":"userinheritsnosqllogin","Timestamp":"XXX","Transport":"local","User":"userinheritsnosqllogin"}
98 {"Duration":"NNN","EventType":"client_session_end","InstanceID":1,"Network":"unix","RemoteAddress":"XXX","Timestamp":"XXX"}
99 {"Duration":"NNN","EventType":"client_connection_end","InstanceID":1,"Network":"unix","RemoteAddress":"XXX","Timestamp":"XXX"}

connect_unix user=userexpired password=123
----
Expand All @@ -286,12 +311,12 @@ ERROR: password is expired (SQLSTATE 28000)
authlog 6
.*client_connection_end
----
90 {"EventType":"client_connection_start","InstanceID":1,"Network":"unix","RemoteAddress":"XXX","Timestamp":"XXX"}
91 {"EventType":"client_authentication_info","Info":"HBA rule: local all all password # built-in CockroachDB default","InstanceID":1,"Method":"password","Network":"unix","RemoteAddress":"XXX","SystemIdentity":"userexpired","Timestamp":"XXX","Transport":"local"}
92 {"EventType":"client_authentication_failed","InstanceID":1,"Method":"password","Network":"unix","Reason":7,"RemoteAddress":"XXX","SystemIdentity":"userexpired","Timestamp":"XXX","Transport":"local","User":"userexpired"}
93 {"Detail":"password is expired","EventType":"client_authentication_failed","InstanceID":1,"Method":"password","Network":"unix","Reason":6,"RemoteAddress":"XXX","SystemIdentity":"userexpired","Timestamp":"XXX","Transport":"local","User":"userexpired"}
94 {"Duration":"NNN","EventType":"client_session_end","InstanceID":1,"Network":"unix","RemoteAddress":"XXX","Timestamp":"XXX"}
95 {"Duration":"NNN","EventType":"client_connection_end","InstanceID":1,"Network":"unix","RemoteAddress":"XXX","Timestamp":"XXX"}
100 {"EventType":"client_connection_start","InstanceID":1,"Network":"unix","RemoteAddress":"XXX","Timestamp":"XXX"}
101 {"EventType":"client_authentication_info","Info":"HBA rule: local all all password # built-in CockroachDB default","InstanceID":1,"Method":"password","Network":"unix","RemoteAddress":"XXX","SystemIdentity":"userexpired","Timestamp":"XXX","Transport":"local"}
102 {"EventType":"client_authentication_failed","InstanceID":1,"Method":"password","Network":"unix","Reason":7,"RemoteAddress":"XXX","SystemIdentity":"userexpired","Timestamp":"XXX","Transport":"local","User":"userexpired"}
103 {"Detail":"password is expired","EventType":"client_authentication_failed","InstanceID":1,"Method":"password","Network":"unix","Reason":6,"RemoteAddress":"XXX","SystemIdentity":"userexpired","Timestamp":"XXX","Transport":"local","User":"userexpired"}
104 {"Duration":"NNN","EventType":"client_session_end","InstanceID":1,"Network":"unix","RemoteAddress":"XXX","Timestamp":"XXX"}
105 {"Duration":"NNN","EventType":"client_connection_end","InstanceID":1,"Network":"unix","RemoteAddress":"XXX","Timestamp":"XXX"}

subtest end

Expand Down
2 changes: 0 additions & 2 deletions pkg/sql/sessioninit/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,9 @@ go_test(
"//pkg/security/securitytest",
"//pkg/security/username",
"//pkg/server",
"//pkg/settings/cluster",
"//pkg/sql",
"//pkg/sql/catalog/descpb",
"//pkg/sql/catalog/descs",
"//pkg/sql/sessiondatapb",
"//pkg/testutils/serverutils",
"//pkg/testutils/sqlutils",
"//pkg/testutils/testcluster",
Expand Down
15 changes: 6 additions & 9 deletions pkg/sql/sessioninit/cache.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,10 @@ type Cache struct {
type AuthInfo struct {
// UserExists is set to true if the user has a row in system.users.
UserExists bool
// CanLoginSQL is set to false if the user has the NOLOGIN or NOSQLLOGIN role option.
CanLoginSQL bool
// CanLoginDBConsole is set to false if the user has NOLOGIN role option.
CanLoginDBConsole bool
// CanLoginSQLRoleOpt is set to false if the user has the NOLOGIN or NOSQLLOGIN role option.
CanLoginSQLRoleOpt bool
// CanLoginDBConsoleRoleOpt is set to false if the user has NOLOGIN role option.
CanLoginDBConsoleRoleOpt bool
// HashedPassword is the hashed password and can be nil.
HashedPassword password.PasswordHash
// ValidUntil is the VALID UNTIL role option.
Expand Down Expand Up @@ -110,13 +110,10 @@ func (a *Cache) GetAuthInfo(
ctx context.Context,
db descs.DB,
username username.SQLUsername,
makePlanner func(opName string) (interface{}, func()),
settings *cluster.Settings,
) (AuthInfo, error),
makePlanner func(opName string) (interface{}, func()),
) (aInfo AuthInfo, err error) {
if !CacheEnabled.Get(&settings.SV) {
return readFromSystemTables(ctx, db, username, makePlanner, settings)
return readFromSystemTables(ctx, db, username)
}

var usersTableDesc catalog.TableDescriptor
Expand Down Expand Up @@ -153,7 +150,7 @@ func (a *Cache) GetAuthInfo(
val, err := a.loadValueOutsideOfCache(
ctx, fmt.Sprintf("authinfo-%s-%d-%d", username.Normalized(), usersTableVersion, roleOptionsTableVersion),
func(loadCtx context.Context) (interface{}, error) {
return readFromSystemTables(loadCtx, db, username, makePlanner, settings)
return readFromSystemTables(loadCtx, db, username)
})
if err != nil {
return aInfo, err
Expand Down
42 changes: 6 additions & 36 deletions pkg/sql/sessioninit/cache_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,9 @@ import (

"github.com/cockroachdb/cockroach/pkg/base"
"github.com/cockroachdb/cockroach/pkg/security/username"
"github.com/cockroachdb/cockroach/pkg/settings/cluster"
"github.com/cockroachdb/cockroach/pkg/sql"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
"github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb"
"github.com/cockroachdb/cockroach/pkg/sql/sessioninit"
"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
Expand Down Expand Up @@ -73,28 +71,17 @@ func TestCacheInvalidation(t *testing.T) {
return settings, didReadFromSystemTable, err
}
getAuthInfoFromCache := func() (sessioninit.AuthInfo, bool, error) {
makePlanner := func(opName string) (interface{}, func()) {
return sql.NewInternalPlanner(
opName,
execCfg.DB.NewTxn(ctx, opName),
username.RootUserName(),
&sql.MemoryMetrics{},
s.ExecutorConfig().(*sql.ExecutorConfig),
sessiondatapb.SessionData{},
)
}
didReadFromSystemTable := false
settings := s.ClusterSettings()
aInfo, err := execCfg.SessionInitCache.GetAuthInfo(
ctx,
settings,
s.InternalDB().(descs.DB),
username.TestUserName(),
func(ctx context.Context, f descs.DB, userName username.SQLUsername, makePlanner func(opName string) (interface{}, func()), settings *cluster.Settings) (sessioninit.AuthInfo, error) {
func(ctx context.Context, f descs.DB, userName username.SQLUsername) (sessioninit.AuthInfo, error) {
didReadFromSystemTable = true
return sessioninit.AuthInfo{}, nil
},
makePlanner)
})
return aInfo, didReadFromSystemTable, err
}

Expand Down Expand Up @@ -172,7 +159,7 @@ func TestCacheInvalidation(t *testing.T) {
require.NoError(t, err)
require.False(t, didReadFromSystemTable)
require.True(t, aInfo.UserExists)
require.True(t, aInfo.CanLoginSQL)
require.True(t, aInfo.CanLoginSQLRoleOpt)

// Verify that creating a different user invalidates the cache.
_, err = db.ExecContext(ctx, "CREATE USER testuser2")
Expand Down Expand Up @@ -228,17 +215,6 @@ func TestCacheSingleFlight(t *testing.T) {
wgFirstGetAuthInfoCallInProgress.Add(1)
wgForTestComplete.Add(3)

makePlanner := func(opName string) (interface{}, func()) {
return sql.NewInternalPlanner(
opName,
execCfg.DB.NewTxn(ctx, opName),
username.RootUserName(),
&sql.MemoryMetrics{},
s.ExecutorConfig().(*sql.ExecutorConfig),
sessiondatapb.SessionData{},
)
}

go func() {
didReadFromSystemTable := false
_, err := c.GetAuthInfo(
Expand All @@ -248,15 +224,13 @@ func TestCacheSingleFlight(t *testing.T) {
ctx context.Context,
f descs.DB,
userName username.SQLUsername,
makePlanner func(opName string) (interface{}, func()),
settings *cluster.Settings,
) (sessioninit.AuthInfo, error) {
wgFirstGetAuthInfoCallInProgress.Done()
wgForConcurrentReadWrite.Wait()
didReadFromSystemTable = true
return sessioninit.AuthInfo{}, nil
},
makePlanner)
)
require.NoError(t, err)
require.True(t, didReadFromSystemTable)
wgForTestComplete.Done()
Expand All @@ -280,13 +254,11 @@ func TestCacheSingleFlight(t *testing.T) {
ctx context.Context,
f descs.DB,
userName username.SQLUsername,
makePlanner func(opName string) (interface{}, func()),
settings *cluster.Settings,
) (sessioninit.AuthInfo, error) {
didReadFromSystemTable = true
return sessioninit.AuthInfo{}, nil
},
makePlanner)
)
require.NoError(t, err)
require.False(t, didReadFromSystemTable)
wgForTestComplete.Done()
Expand All @@ -309,13 +281,11 @@ func TestCacheSingleFlight(t *testing.T) {
ctx context.Context,
f descs.DB,
userName username.SQLUsername,
makePlanner func(opName string) (interface{}, func()),
settings *cluster.Settings,
) (sessioninit.AuthInfo, error) {
didReadFromSystemTable = true
return sessioninit.AuthInfo{}, nil
},
makePlanner)
)

require.NoError(t, err)
require.True(t, didReadFromSystemTable)
Expand Down
Loading

0 comments on commit be08998

Please sign in to comment.