Skip to content

Commit

Permalink
Merge #74706 #74835 #74922
Browse files Browse the repository at this point in the history 
74706: sql: add NOSQLLOGIN role which restricts SQL CLI only r=rafiss,kylepatron-cockroachlabs a=dhartunian

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 #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.

74835: kvserver: add AdminSplit and AdminScatter to secondary tenants API r=shralex a=shralex

This adds support for AdminSplit and AdminScatter for secondary tenants. This API allows indicating to KV that more data will be ingested, and so the range should be split and re-distributed across the cluster. This API will not be exposed through SQL, and in the future we might change it to give KV more control over whether and how to deal with expected ingest load. More discussion can be found in the github issue: #74389 and Epic: https://cockroachlabs.atlassian.net/browse/CRDB-10720

Release Note: None

74922: sql: clean up mutable not-null columns hack r=RaduBerinde a=RaduBerinde

Mutation columns in some cases need to be scanned even if they haven't
been backfilled yet, which means that we may retrieve NULL values even
if they are marked as not-nullable.

We currently have a hack in the table descriptor which changes the
nullable flags in the column descriptors when `ReadableColumns()` is
used. It is very surprising that we can get different descriptors for
a given ColumnID depending if we look for it in `ReadableColumns()` or
in `AllColumns()` (e.g. via FindColumnWithID).

This commit cleans this up, changing the scanning code to check for
`Public()` instead.

Release note: None

Co-authored-by: David Hartunian <davidh@cockroachlabs.com>
Co-authored-by: shralex <shralex@gmail.com>
Co-authored-by: Radu Berinde <radu@cockroachlabs.com>
  • Loading branch information
@dhartunian @shralex @RaduBerinde
4 people committed Jan 19, 2022
4 parents 9547bc9 + 1376c8d + 22f5264 + 21bace2 commit eb26eb3
Show file tree
Hide file tree
Showing 18 changed files with 258 additions and 123 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 @@ -1041,6 +1041,7 @@ unreserved_keyword ::=
| 'NOLOGIN'
| 'NOMODIFYCLUSTERSETTING'
| 'NONVOTERS'
| 'NOSQLLOGIN'
| 'NOVIEWACTIVITY'
| 'NOVIEWACTIVITYREDACTED'
| 'NOWAIT'
Expand Down Expand Up @@ -1152,6 +1153,7 @@ unreserved_keyword ::=
| 'SNAPSHOT'
| 'SPLIT'
| 'SQL'
| 'SQLLOGIN'
| 'START'
| 'STATEMENTS'
| 'STATISTICS'
Expand Down Expand Up @@ -2372,6 +2374,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
4 changes: 3 additions & 1 deletion pkg/rpc/auth_tenant.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,12 +149,14 @@ var reqMethodAllowlist = [...]bool{
roachpb.Scan: true,
roachpb.ReverseScan: true,
roachpb.EndTxn: true,
roachpb.AdminSplit: true,
roachpb.HeartbeatTxn: true,
roachpb.QueryTxn: true,
roachpb.QueryIntent: true,
roachpb.InitPut: true,
roachpb.AddSSTable: true,
roachpb.Export: true,
roachpb.AdminScatter: true,
roachpb.AddSSTable: true,
roachpb.Refresh: true,
roachpb.RefreshRange: true,
}
Expand Down
96 changes: 85 additions & 11 deletions pkg/rpc/auth_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,11 +163,21 @@ func TestTenantAuthRequest(t *testing.T) {
h := roachpb.RequestHeaderFromSpan(s)
return &roachpb.ScanRequest{RequestHeader: h}
}
makeAdminReq := func(key string) roachpb.Request {
makeDisallowedAdminReq := func(key string) roachpb.Request {
s := makeSpan(key)
h := roachpb.RequestHeader{Key: s.Key}
return &roachpb.AdminUnsplitRequest{RequestHeader: h}
}
makeAdminSplitReq := func(key string) roachpb.Request {
s := makeSpan(key)
h := roachpb.RequestHeaderFromSpan(s)
return &roachpb.AdminSplitRequest{RequestHeader: h, SplitKey: s.Key}
}
makeAdminScatterReq := func(key string) roachpb.Request {
s := makeSpan(key)
h := roachpb.RequestHeaderFromSpan(s)
return &roachpb.AdminScatterRequest{RequestHeader: h}
}
makeReqs := func(reqs ...roachpb.Request) []roachpb.RequestUnion {
ru := make([]roachpb.RequestUnion, len(reqs))
for i, r := range reqs {
Expand Down Expand Up @@ -249,35 +259,99 @@ func TestTenantAuthRequest(t *testing.T) {
},
{
req: &roachpb.BatchRequest{Requests: makeReqs(
makeAdminReq("a"),
makeDisallowedAdminReq("a"),
)},
expErr: `request \[1 AdmUnsplit\] not permitted`,
},
{
req: &roachpb.BatchRequest{Requests: makeReqs(
makeDisallowedAdminReq(prefix(10, "a")),
)},
expErr: `request \[1 AdmSplit\] not permitted`,
expErr: `request \[1 AdmUnsplit\] not permitted`,
},
{
req: &roachpb.BatchRequest{Requests: makeReqs(
makeAdminReq(prefix(10, "a")),
makeDisallowedAdminReq(prefix(50, "a")),
)},
expErr: `request \[1 AdmSplit\] not permitted`,
expErr: `request \[1 AdmUnsplit\] not permitted`,
},
{
req: &roachpb.BatchRequest{Requests: makeReqs(
makeAdminReq(prefix(50, "a")),
makeDisallowedAdminReq(prefix(10, "a")),
makeReq(prefix(10, "a"), prefix(10, "b")),
)},
expErr: `request \[1 AdmSplit\] not permitted`,
expErr: `request \[1 Scan, 1 AdmUnsplit\] not permitted`,
},
{
req: &roachpb.BatchRequest{Requests: makeReqs(
makeAdminReq(prefix(10, "a")),
makeReq(prefix(10, "a"), prefix(10, "b")),
makeDisallowedAdminReq(prefix(10, "a")),
)},
expErr: `request \[1 Scan, 1 AdmUnsplit\] not permitted`,
},
{
req: &roachpb.BatchRequest{Requests: makeReqs(
makeAdminSplitReq("a"),
)},
expErr: `request \[1 Scan, 1 AdmSplit\] not permitted`,
expErr: `requested key span a{-\\x00} not fully contained in tenant keyspace /Tenant/1{0-1}`,
},
{
req: &roachpb.BatchRequest{Requests: makeReqs(
makeAdminSplitReq(prefix(10, "a")),
)},
expErr: noError,
},
{
req: &roachpb.BatchRequest{Requests: makeReqs(
makeAdminSplitReq(prefix(50, "a")),
)},
expErr: `requested key span /Tenant/50"a{"-\\x00"} not fully contained in tenant keyspace /Tenant/1{0-1}`,
},
{
req: &roachpb.BatchRequest{Requests: makeReqs(
makeAdminSplitReq(prefix(10, "a")),
makeReq(prefix(10, "a"), prefix(10, "b")),
makeAdminReq(prefix(10, "a")),
)},
expErr: `request \[1 Scan, 1 AdmSplit\] not permitted`,
expErr: noError,
},
{
req: &roachpb.BatchRequest{Requests: makeReqs(
makeReq(prefix(10, "a"), prefix(10, "b")),
makeAdminSplitReq(prefix(10, "a")),
)},
expErr: noError,
},
{
req: &roachpb.BatchRequest{Requests: makeReqs(
makeAdminScatterReq("a"),
)},
expErr: `requested key span a{-\\x00} not fully contained in tenant keyspace /Tenant/1{0-1}`,
},
{
req: &roachpb.BatchRequest{Requests: makeReqs(
makeAdminScatterReq(prefix(10, "a")),
)},
expErr: noError,
},
{
req: &roachpb.BatchRequest{Requests: makeReqs(
makeAdminScatterReq(prefix(50, "a")),
)},
expErr: `requested key span /Tenant/50"a{"-\\x00"} not fully contained in tenant keyspace /Tenant/1{0-1}`,
},
{
req: &roachpb.BatchRequest{Requests: makeReqs(
makeAdminScatterReq(prefix(10, "a")),
makeReq(prefix(10, "a"), prefix(10, "b")),
)},
expErr: noError,
},
{
req: &roachpb.BatchRequest{Requests: makeReqs(
makeReq(prefix(10, "a"), prefix(10, "b")),
makeAdminScatterReq(prefix(10, "a")),
)},
expErr: noError,
},
{
req: &roachpb.BatchRequest{Requests: makeReqs(
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
Loading

0 comments on commit eb26eb3

Please sign in to comment.