From d4c91d94f5b46a0c5a4ab27495379b8868df2cf6 Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Thu, 27 Jan 2022 09:14:36 +1100 Subject: [PATCH 1/3] sql: check session_user can become user on deserialize_session Release note (bug fix): `crdb_internal.deserialize_session` now checks the session_user has the privilege to SET ROLE to the current_user before changing the session settings. --- pkg/sql/faketreeeval/evalctx.go | 7 ++++++ pkg/sql/sem/builtins/builtins.go | 3 +++ pkg/sql/sem/tree/eval.go | 4 ++++ pkg/sql/session_migration_test.go | 29 +++++++++++++++++++++++ pkg/sql/sessiondata/session_data.go | 2 +- pkg/sql/testdata/session_migration/errors | 18 ++++++++++++++ pkg/sql/user.go | 5 ++-- 7 files changed, 65 insertions(+), 3 deletions(-) diff --git a/pkg/sql/faketreeeval/evalctx.go b/pkg/sql/faketreeeval/evalctx.go index c81afc6daa3c..e6fb3a8b736b 100644 --- a/pkg/sql/faketreeeval/evalctx.go +++ b/pkg/sql/faketreeeval/evalctx.go @@ -216,6 +216,13 @@ func (ep *DummyEvalPlanner) UserHasAdminRole( return false, errors.WithStack(errEvalPlanner) } +// CheckCanBecomeUser is part of the EvalPlanner interface. +func (ep *DummyEvalPlanner) CheckCanBecomeUser( + ctx context.Context, becomeUser security.SQLUsername, +) error { + return errors.WithStack(errEvalPlanner) +} + // MemberOfWithAdminOption is part of the EvalPlanner interface. func (ep *DummyEvalPlanner) MemberOfWithAdminOption( ctx context.Context, member security.SQLUsername, diff --git a/pkg/sql/sem/builtins/builtins.go b/pkg/sql/sem/builtins/builtins.go index f92bfb8aa384..645baac5438b 100644 --- a/pkg/sql/sem/builtins/builtins.go +++ b/pkg/sql/sem/builtins/builtins.go @@ -6384,6 +6384,9 @@ table's zone configuration this will return NULL.`, "can only deserialize matching session users", ) } + if err := evalCtx.Planner.CheckCanBecomeUser(evalCtx.Context, sd.User()); err != nil { + return nil, err + } *evalCtx.SessionData() = *sd return tree.MakeDBool(true), nil }, diff --git a/pkg/sql/sem/tree/eval.go b/pkg/sql/sem/tree/eval.go index c3ea38483328..8ca7149b0a59 100644 --- a/pkg/sql/sem/tree/eval.go +++ b/pkg/sql/sem/tree/eval.go @@ -3226,6 +3226,10 @@ type EvalPlanner interface { // the `system.users` table UserHasAdminRole(ctx context.Context, user security.SQLUsername) (bool, error) + // CheckCanBecomeUser returns an error if the SessionUser cannot become the + // becomeUser. + CheckCanBecomeUser(ctx context.Context, becomeUser security.SQLUsername) error + // MemberOfWithAdminOption is used to collect a list of roles (direct and // indirect) that the member is part of. See the comment on the planner // implementation in authorization.go diff --git a/pkg/sql/session_migration_test.go b/pkg/sql/session_migration_test.go index f3dcf9e349f2..473c7aff5ed3 100644 --- a/pkg/sql/session_migration_test.go +++ b/pkg/sql/session_migration_test.go @@ -14,6 +14,7 @@ import ( "context" gosql "database/sql" "fmt" + "net/url" "strings" "testing" @@ -24,6 +25,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/testutils/testcluster" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/log" + "github.com/cockroachdb/cockroach/pkg/util/stop" "github.com/cockroachdb/datadriven" "github.com/stretchr/testify/require" ) @@ -57,6 +59,29 @@ func TestSessionMigration(t *testing.T) { defer func() { _ = dbConn.Close() }() + _, err := dbConn.Exec("CREATE USER testuser") + require.NoError(t, err) + + openUserConnFunc := func(user string) *gosql.DB { + + pgURL, cleanupGoDB, err := sqlutils.PGUrlE( + tc.Server(0).ServingSQLAddr(), + "StartServer", /* prefix */ + url.User(user), + ) + require.NoError(t, err) + pgURL.Path = "defaultdb" + + goDB, err := gosql.Open("postgres", pgURL.String()) + require.NoError(t, err) + + tc.Server(0).Stopper().AddCloser( + stop.CloserFn(func() { + cleanupGoDB() + })) + + return goDB + } vars := make(map[string]string) datadriven.RunTest(t, path, func(t *testing.T, d *datadriven.TestData) string { @@ -72,6 +97,10 @@ func TestSessionMigration(t *testing.T) { require.NoError(t, dbConn.Close()) dbConn = openConnFunc() return "" + case "user": + require.NoError(t, dbConn.Close()) + dbConn = openUserConnFunc(d.Input) + return "" case "exec": _, err := dbConn.Exec(getQuery()) if err != nil { diff --git a/pkg/sql/sessiondata/session_data.go b/pkg/sql/sessiondata/session_data.go index f82d0b1d5120..bd5870251ebe 100644 --- a/pkg/sql/sessiondata/session_data.go +++ b/pkg/sql/sessiondata/session_data.go @@ -148,7 +148,7 @@ func (s *SessionData) GetDateStyle() pgdate.DateStyle { // SessionUser retrieves the session_user. // The SessionUser is the username that originally logged into the session. // If a user applies SET ROLE, the SessionUser remains the same whilst the -// CurrentUser() changes. +// User() changes. func (s *SessionData) SessionUser() security.SQLUsername { if s.SessionUserProto == "" { return s.User() diff --git a/pkg/sql/testdata/session_migration/errors b/pkg/sql/testdata/session_migration/errors index 735a1bb845a6..56c50d51afaa 100644 --- a/pkg/sql/testdata/session_migration/errors +++ b/pkg/sql/testdata/session_migration/errors @@ -67,3 +67,21 @@ SELECT crdb_internal.deserialize_session( ) ---- pq: crdb_internal.deserialize_session(): can only deserialize matching session users + +# We cannot deserialize into a current_user we do not match. +user +testuser +---- + +exec +SELECT crdb_internal.deserialize_session( + decode( + -- minted by modifying `crdb_internal.serialize_session()` and setting + -- sd.SessionData.UserProto=root + -- sd.LocalOnlySessionData.SessionUser=testuser + '0a510a0964656661756c74646212102420636f636b726f6163682064656d6f1a04726f6f742204100222002802380842035554434a0524757365724a067075626c69635a0060808080207a0088010190018050124910904e3002380840026001680170017801880101d80101e00101f00101f80101900201b002808001c80201f202087465737475736572a00301a9030000000000408f40d00301e00301', + 'hex' + ) +) +---- +pq: crdb_internal.deserialize_session(): only root can become root diff --git a/pkg/sql/user.go b/pkg/sql/user.go index 333c63397c5c..bfbfe608eeb7 100644 --- a/pkg/sql/user.go +++ b/pkg/sql/user.go @@ -528,7 +528,7 @@ func (p *planner) setRole(ctx context.Context, local bool, s security.SQLUsernam } } - if err := p.checkCanBecomeUser(ctx, becomeUser); err != nil { + if err := p.CheckCanBecomeUser(ctx, becomeUser); err != nil { return err } @@ -573,7 +573,8 @@ func (p *planner) setRole(ctx context.Context, local bool, s security.SQLUsernam } -func (p *planner) checkCanBecomeUser(ctx context.Context, becomeUser security.SQLUsername) error { +// CheckCanBecomeUser implements the EvalPlanner interface. +func (p *planner) CheckCanBecomeUser(ctx context.Context, becomeUser security.SQLUsername) error { sessionUser := p.SessionData().SessionUser() // Switching to None can always succeed. From 3b1597be411fede0f52981f91bd9260c485f2c1a Mon Sep 17 00:00:00 2001 From: Tommy Reilly Date: Wed, 26 Jan 2022 17:48:50 -0500 Subject: [PATCH 2/3] sql: de-flake TestRecordBatchSerializerDeserializeMemoryEstimate Fixes: #73781 When BatchSize was low and each buffer was much smaller than capacity we could end up with newMemorySize more than %33 smaller than original. However the test is just making sure we aren't double counting memory so testing that newMemorySize - originalMemory < 4/3 original. I.e. its okay if it shrinks but we don't want it to be more than %33 bigger. Release note: None --- pkg/col/colserde/record_batch_test.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/pkg/col/colserde/record_batch_test.go b/pkg/col/colserde/record_batch_test.go index 58a4c1077072..8e1e5ca20637 100644 --- a/pkg/col/colserde/record_batch_test.go +++ b/pkg/col/colserde/record_batch_test.go @@ -341,12 +341,13 @@ func TestRecordBatchSerializerDeserializeMemoryEstimate(t *testing.T) { newMemorySize := colmem.GetBatchMemSize(dest) // We expect that the original and the new memory sizes are relatively close - // to each other (do not differ by more than a third). We cannot guarantee - // more precise bound here because the capacities of the underlying []byte - // slices is unpredictable. However, this check is sufficient to ensure that - // we don't double count memory under `Bytes.data`. + // to each other, specifically newMemorySize must less than + // 4/3*originalMemorySize. We cannot guarantee more precise bound here because + // the capacities of the underlying []byte slices is unpredictable. However, + // this check is sufficient to ensure that we don't double count memory under + // `Bytes.data`. const maxDeviation = float64(0.33) - deviation := math.Abs(float64(originalMemorySize-newMemorySize) / (float64(originalMemorySize))) + deviation := float64(newMemorySize-originalMemorySize) / float64(originalMemorySize) require.GreaterOrEqualf(t, maxDeviation, deviation, "new memory size %d is too far away from original %d", newMemorySize, originalMemorySize) } From 27df8cdcf12ac091e31f1eff84ab1e8f132e3a04 Mon Sep 17 00:00:00 2001 From: irfan sharif Date: Wed, 26 Jan 2022 20:56:22 -0500 Subject: [PATCH 3/3] logictest: de-flake TestLogic/5node-disk/distsql_enum Fixes #75570. Second attempt at 83f584d, though now with the right syntax. Release note: None --- pkg/sql/logictest/testdata/logic_test/distsql_enum | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/sql/logictest/testdata/logic_test/distsql_enum b/pkg/sql/logictest/testdata/logic_test/distsql_enum index 9b3523f9a13a..3dbdc7de6f5a 100644 --- a/pkg/sql/logictest/testdata/logic_test/distsql_enum +++ b/pkg/sql/logictest/testdata/logic_test/distsql_enum @@ -65,7 +65,7 @@ ALTER TABLE t2 INJECT STATISTICS '[ } ]' -query T nodeidx=1 retry +query T nodeidx=1,retry EXPLAIN (VEC) SELECT x from t1 WHERE EXISTS (SELECT x FROM t2 WHERE t1.x=t2.x AND t2.y='hello') ----