diff --git a/kv/dist_sender_server_test.go b/kv/dist_sender_server_test.go index 0e96d4e0f3eb..737db20d75bc 100644 --- a/kv/dist_sender_server_test.go +++ b/kv/dist_sender_server_test.go @@ -258,10 +258,11 @@ func TestSingleRangeReverseScan(t *testing.T) { t.Errorf("expected 2 rows; got %d", l) } // Case 3: Test proto.KeyMax + // TODO(marc): this depends on the sql system objects. if rows, err := db.ReverseScan("g", proto.KeyMax, 0); err != nil { t.Fatalf("unexpected error on ReverseScan: %s", err) - } else if l := len(rows); l != 8 { - t.Errorf("expected 2 rows; got %d", l) + } else if l := len(rows); l != 10 { + t.Errorf("expected 10 rows; got %d", l) } // Case 4: Test keys.SystemMax if rows, err := db.ReverseScan(keys.SystemMax, "b", 0); err != nil { diff --git a/server/node_test.go b/server/node_test.go index 741948a86d5b..404365172689 100644 --- a/server/node_test.go +++ b/server/node_test.go @@ -121,6 +121,7 @@ func TestBootstrapCluster(t *testing.T) { for _, kv := range rows { keys = append(keys, kv.Key) } + // TODO(marc): this depends on the sql system objects. var expectedKeys = []proto.Key{ proto.MakeKey(proto.Key("\x00\x00meta1"), proto.KeyMax), proto.MakeKey(proto.Key("\x00\x00meta2"), proto.KeyMax), @@ -133,9 +134,11 @@ func TestBootstrapCluster(t *testing.T) { proto.Key("\xff\n\x02\n\x01\tsystem\x00\x01\n\x03"), proto.Key("\xff\n\x02\n\x01\n\x01descriptor\x00\x01\n\x03"), proto.Key("\xff\n\x02\n\x01\n\x01namespace\x00\x01\n\x03"), + proto.Key("\xff\n\x02\n\x01\n\x01users\x00\x01\n\x03"), proto.Key("\xff\n\x03\n\x01\n\x01\n\x02"), proto.Key("\xff\n\x03\n\x01\n\x02\n\x02"), proto.Key("\xff\n\x03\n\x01\n\x03\n\x02"), + proto.Key("\xff\n\x03\n\x01\n\x04\n\x02"), } if !reflect.DeepEqual(keys, expectedKeys) { t.Errorf("expected keys mismatch:\n%s\n -- vs. -- \n\n%s", diff --git a/sql/create_test.go b/sql/create_test.go index babe97e9b47b..5bae92af9e9c 100644 --- a/sql/create_test.go +++ b/sql/create_test.go @@ -72,7 +72,8 @@ func TestDatabaseDescriptor(t *testing.T) { if kvs, err := kvDB.Scan(start, start.PrefixEnd(), 0); err != nil { t.Fatal(err) } else { - if a, e := len(kvs), 3; a != e { + // TODO(marc): this is the number of system tables + 1. + if a, e := len(kvs), 4; a != e { t.Fatalf("expected %d keys to have been written, found %d keys", e, a) } } diff --git a/sql/database.go b/sql/database.go index b83cd328cebe..47953232efcc 100644 --- a/sql/database.go +++ b/sql/database.go @@ -38,7 +38,7 @@ func (dk databaseKey) Name() string { func makeDatabaseDesc(p *parser.CreateDatabase) DatabaseDescriptor { return DatabaseDescriptor{ Name: p.Name.String(), - Privileges: NewDefaultDatabasePrivilegeDescriptor(), + Privileges: NewDefaultPrivilegeDescriptor(), } } diff --git a/sql/privilege.go b/sql/privilege.go index 86fd272ec323..70ce144381e8 100644 --- a/sql/privilege.go +++ b/sql/privilege.go @@ -98,22 +98,22 @@ func (p *PrivilegeDescriptor) removeUser(user string) { p.Users = p.Users[:len(p.Users)-1] } -// NewSystemObjectPrivilegeDescriptor returns a privilege descriptor for system -// databases and tables. -func NewSystemObjectPrivilegeDescriptor() *PrivilegeDescriptor { +// NewPrivilegeDescriptor returns a privilege descriptor for the given +// user with the specified list of privileges. +func NewPrivilegeDescriptor(user string, priv privilege.List) *PrivilegeDescriptor { return &PrivilegeDescriptor{ Users: []*UserPrivileges{ { - User: security.RootUser, - Privileges: privilege.SELECT.Mask() | privilege.GRANT.Mask(), + User: user, + Privileges: priv.ToBitField(), }, }, } } -// NewDefaultDatabasePrivilegeDescriptor returns a privilege descriptor +// NewDefaultPrivilegeDescriptor returns a privilege descriptor // with ALL privileges for the root user. -func NewDefaultDatabasePrivilegeDescriptor() *PrivilegeDescriptor { +func NewDefaultPrivilegeDescriptor() *PrivilegeDescriptor { return &PrivilegeDescriptor{ Users: []*UserPrivileges{ { @@ -181,10 +181,11 @@ func (p *PrivilegeDescriptor) Revoke(user string, privList privilege.List) { } } -// Validate is called when writing a descriptor. -// If 'isSystem' is true, perform validation for system -// database and tables. -func (p *PrivilegeDescriptor) Validate(isSystem bool) error { +// Validate is called when writing a database or table descriptor. +// It takes the descriptor ID which is used to determine if +// it belongs to a system descriptor, in which case the maximum +// set of allowed privileges is looked up and applied. +func (p *PrivilegeDescriptor) Validate(id ID) error { if p == nil { return fmt.Errorf("missing privilege descriptor") } @@ -192,10 +193,15 @@ func (p *PrivilegeDescriptor) Validate(isSystem bool) error { if !ok { return fmt.Errorf("user %s does not have privileges", security.RootUser) } - if isSystem { - // System databases and tables must have read-only permissions - // only (SELECT and GRANT). These must remain set for the root user. - allowedPrivileges := privilege.SELECT.Mask() | privilege.GRANT.Mask() + if IsSystemID(id) { + // System databases and tables have custom maximum allowed privileges. + objectPrivileges, ok := SystemAllowedPrivileges[id] + if !ok { + return fmt.Errorf("no allowed privileges found for system object with ID=%d", id) + } + + // The root user must have all the allowed privileges. + allowedPrivileges := objectPrivileges.ToBitField() if userPriv.Privileges&allowedPrivileges != allowedPrivileges { return fmt.Errorf("user %s must have %s privileges on system objects", security.RootUser, privilege.ListFromBitField(allowedPrivileges)) diff --git a/sql/privilege/privilege.go b/sql/privilege/privilege.go index 3d5f19c88560..180ea0a4a425 100644 --- a/sql/privilege/privilege.go +++ b/sql/privilege/privilege.go @@ -42,6 +42,13 @@ const ( UPDATE ) +// Predefined sets of privileges. +var ( + ReadData = List{GRANT, SELECT} + WriteData = List{INSERT, DELETE, UPDATE} + ReadWriteData = List{GRANT, SELECT, INSERT, DELETE, UPDATE} +) + // Mask returns the bitmask for a given privilege. func (k Kind) Mask() uint32 { return 1 << k diff --git a/sql/privilege_test.go b/sql/privilege_test.go index ab032adf6abd..574b975fe052 100644 --- a/sql/privilege_test.go +++ b/sql/privilege_test.go @@ -20,6 +20,7 @@ package sql_test import ( "testing" + "github.com/cockroachdb/cockroach/security" "github.com/cockroachdb/cockroach/sql" "github.com/cockroachdb/cockroach/sql/privilege" "github.com/cockroachdb/cockroach/util/leaktest" @@ -27,7 +28,7 @@ import ( func TestPrivilege(t *testing.T) { defer leaktest.AfterTest(t) - descriptor := sql.NewDefaultDatabasePrivilegeDescriptor() + descriptor := sql.NewDefaultPrivilegeDescriptor() testCases := []struct { grantee string // User to grant/revoke privileges on. @@ -35,31 +36,31 @@ func TestPrivilege(t *testing.T) { show []sql.UserPrivilegeString }{ {"", nil, nil, - []sql.UserPrivilegeString{{"root", "ALL"}}, + []sql.UserPrivilegeString{{security.RootUser, "ALL"}}, }, - {"root", privilege.List{privilege.ALL}, nil, - []sql.UserPrivilegeString{{"root", "ALL"}}, + {security.RootUser, privilege.List{privilege.ALL}, nil, + []sql.UserPrivilegeString{{security.RootUser, "ALL"}}, }, - {"root", privilege.List{privilege.INSERT, privilege.DROP}, nil, - []sql.UserPrivilegeString{{"root", "ALL"}}, + {security.RootUser, privilege.List{privilege.INSERT, privilege.DROP}, nil, + []sql.UserPrivilegeString{{security.RootUser, "ALL"}}, }, {"foo", privilege.List{privilege.INSERT, privilege.DROP}, nil, - []sql.UserPrivilegeString{{"foo", "DROP,INSERT"}, {"root", "ALL"}}, + []sql.UserPrivilegeString{{"foo", "DROP,INSERT"}, {security.RootUser, "ALL"}}, }, {"bar", nil, privilege.List{privilege.INSERT, privilege.ALL}, - []sql.UserPrivilegeString{{"foo", "DROP,INSERT"}, {"root", "ALL"}}, + []sql.UserPrivilegeString{{"foo", "DROP,INSERT"}, {security.RootUser, "ALL"}}, }, {"foo", privilege.List{privilege.ALL}, nil, - []sql.UserPrivilegeString{{"foo", "ALL"}, {"root", "ALL"}}, + []sql.UserPrivilegeString{{"foo", "ALL"}, {security.RootUser, "ALL"}}, }, {"foo", nil, privilege.List{privilege.SELECT, privilege.INSERT}, - []sql.UserPrivilegeString{{"foo", "CREATE,DELETE,DROP,GRANT,UPDATE"}, {"root", "ALL"}}, + []sql.UserPrivilegeString{{"foo", "CREATE,DELETE,DROP,GRANT,UPDATE"}, {security.RootUser, "ALL"}}, }, {"foo", nil, privilege.List{privilege.ALL}, - []sql.UserPrivilegeString{{"root", "ALL"}}, + []sql.UserPrivilegeString{{security.RootUser, "ALL"}}, }, // Validate checks that root still has ALL privileges, but we do not call it here. - {"root", nil, privilege.List{privilege.ALL}, + {security.RootUser, nil, privilege.List{privilege.ALL}, []sql.UserPrivilegeString{}, }, } @@ -90,60 +91,119 @@ func TestPrivilege(t *testing.T) { } } +// TestPrivilegeValidate exercises validation for non-system descriptors. func TestPrivilegeValidate(t *testing.T) { defer leaktest.AfterTest(t) - descriptor := sql.NewDefaultDatabasePrivilegeDescriptor() - if err := descriptor.Validate(false); err != nil { + id := sql.MaxReservedDescID + 1 + descriptor := sql.NewDefaultPrivilegeDescriptor() + if err := descriptor.Validate(id); err != nil { t.Fatal(err) } descriptor.Grant("foo", privilege.List{privilege.ALL}) - if err := descriptor.Validate(false); err != nil { + if err := descriptor.Validate(id); err != nil { t.Fatal(err) } - descriptor.Grant("root", privilege.List{privilege.SELECT}) - if err := descriptor.Validate(false); err != nil { + descriptor.Grant(security.RootUser, privilege.List{privilege.SELECT}) + if err := descriptor.Validate(id); err != nil { t.Fatal(err) } - descriptor.Revoke("root", privilege.List{privilege.SELECT}) - if err := descriptor.Validate(false); err == nil { + descriptor.Revoke(security.RootUser, privilege.List{privilege.SELECT}) + if err := descriptor.Validate(id); err == nil { t.Fatal("unexpected success") } // TODO(marc): validate fails here because we do not aggregate // privileges into ALL when all are set. - descriptor.Grant("root", privilege.List{privilege.SELECT}) - if err := descriptor.Validate(false); err == nil { + descriptor.Grant(security.RootUser, privilege.List{privilege.SELECT}) + if err := descriptor.Validate(id); err == nil { t.Fatal("unexpected success") } - descriptor.Revoke("root", privilege.List{privilege.ALL}) - if err := descriptor.Validate(false); err == nil { + descriptor.Revoke(security.RootUser, privilege.List{privilege.ALL}) + if err := descriptor.Validate(id); err == nil { t.Fatal("unexpected success") } } +// TestSystemPrivilegeValidate exercises validation for system descriptors. +// We use 1 (the system database ID). func TestSystemPrivilegeValidate(t *testing.T) { defer leaktest.AfterTest(t) - descriptor := sql.NewSystemObjectPrivilegeDescriptor() - if err := descriptor.Validate(true); err != nil { - t.Fatal(err) - } - descriptor.Grant("foo", privilege.List{privilege.SELECT, privilege.GRANT}) - if err := descriptor.Validate(true); err != nil { - t.Fatal(err) + id := sql.ID(1) + allowedPrivileges := sql.SystemAllowedPrivileges[id] + + hasPrivilege := func(pl privilege.List, p privilege.Kind) bool { + for _, i := range pl { + if i == p { + return true + } + } + return false } - descriptor.Grant("root", privilege.List{privilege.SELECT}) - if err := descriptor.Validate(true); err != nil { + + // Exhaustively grant/revoke all privileges. + // Due to the way validation is done after Grant/Revoke, + // we need to revert the just-performed change after errors. + descriptor := sql.NewPrivilegeDescriptor(security.RootUser, allowedPrivileges) + if err := descriptor.Validate(id); err != nil { t.Fatal(err) } - descriptor.Revoke("root", privilege.List{privilege.SELECT}) - if err := descriptor.Validate(true); err == nil { - t.Fatal("unexpected success") - } - descriptor.Grant("root", privilege.List{privilege.INSERT}) - if err := descriptor.Validate(true); err == nil { - t.Fatal("unexpected success") - } - descriptor.Revoke("root", privilege.List{privilege.ALL}) - if err := descriptor.Validate(true); err == nil { - t.Fatal("unexpected success") + for _, p := range privilege.ByValue { + if hasPrivilege(allowedPrivileges, p) { + // Grant allowed privileges. Either they are already + // on (noop), or they're accepted. + descriptor.Grant(security.RootUser, privilege.List{p}) + if err := descriptor.Validate(id); err != nil { + t.Fatal(err) + } + descriptor.Grant("foo", privilege.List{p}) + if err := descriptor.Validate(id); err != nil { + t.Fatal(err) + } + + // Remove allowed privileges. This fails for root, + // but passes for other users. + descriptor.Revoke(security.RootUser, privilege.List{p}) + if err := descriptor.Validate(id); err == nil { + t.Fatal("unexpected success") + } + descriptor.Grant(security.RootUser, privilege.List{p}) + } else { + // Granting non-allowed privileges always. + descriptor.Grant(security.RootUser, privilege.List{p}) + if err := descriptor.Validate(id); err == nil { + t.Fatal("unexpected success") + } + descriptor.Revoke(security.RootUser, privilege.List{p}) + descriptor.Grant(security.RootUser, allowedPrivileges) + + descriptor.Grant("foo", privilege.List{p}) + if err := descriptor.Validate(id); err == nil { + t.Fatal("unexpected success") + } + descriptor.Revoke("foo", privilege.List{p}) + descriptor.Grant("foo", allowedPrivileges) + + // Revoking non-allowed privileges always succeeds, + // except when removing ALL for root. + if p == privilege.ALL { + // We need to reset privileges as Revoke(ALL) will clear + // all bits. + descriptor.Revoke(security.RootUser, privilege.List{p}) + if err := descriptor.Validate(id); err == nil { + t.Fatal("unexpected success") + } + descriptor.Grant(security.RootUser, allowedPrivileges) + } else { + descriptor.Revoke(security.RootUser, privilege.List{p}) + if err := descriptor.Validate(id); err != nil { + t.Fatal(err) + } + } + } + + // We can always revoke anything from non-root users. + descriptor.Revoke("foo", privilege.List{p}) + if err := descriptor.Validate(id); err != nil { + t.Fatal(err) + } } } diff --git a/sql/structured.go b/sql/structured.go index cb51baa999a2..42b0e863975b 100644 --- a/sql/structured.go +++ b/sql/structured.go @@ -256,7 +256,7 @@ func (desc *TableDescriptor) Validate() error { } } // Validate the privilege descriptor. - return desc.Privileges.Validate(IsSystemID(desc.GetID())) + return desc.Privileges.Validate(desc.GetID()) } // FindColumnByName finds the column with specified name. @@ -337,5 +337,5 @@ func (desc *DatabaseDescriptor) Validate() error { return fmt.Errorf("invalid database ID 0") } // Validate the privilege descriptor. - return desc.Privileges.Validate(IsSystemID(desc.GetID())) + return desc.Privileges.Validate(desc.GetID()) } diff --git a/sql/structured_test.go b/sql/structured_test.go index 1b5f1e42851b..753da5d8c1b5 100644 --- a/sql/structured_test.go +++ b/sql/structured_test.go @@ -42,7 +42,7 @@ func TestAllocateIDs(t *testing.T) { {Name: "d", ColumnNames: []string{"b", "a"}}, {Name: "e", ColumnNames: []string{"b"}}, }, - Privileges: sql.NewDefaultDatabasePrivilegeDescriptor(), + Privileges: sql.NewDefaultPrivilegeDescriptor(), } if err := desc.AllocateIDs(); err != nil { t.Fatal(err) @@ -63,7 +63,7 @@ func TestAllocateIDs(t *testing.T) { {ID: 3, Name: "e", ColumnIDs: []sql.ColumnID{2}, ColumnNames: []string{"b"}, ImplicitColumnIDs: []sql.ColumnID{1}}, }, - Privileges: sql.NewDefaultDatabasePrivilegeDescriptor(), + Privileges: sql.NewDefaultPrivilegeDescriptor(), NextColumnID: 4, NextIndexID: 4, } diff --git a/sql/system.go b/sql/system.go index a31dfead83fa..4e3b98b4d7f5 100644 --- a/sql/system.go +++ b/sql/system.go @@ -22,7 +22,9 @@ import ( "github.com/cockroachdb/cockroach/keys" "github.com/cockroachdb/cockroach/proto" + "github.com/cockroachdb/cockroach/security" "github.com/cockroachdb/cockroach/sql/parser" + "github.com/cockroachdb/cockroach/sql/privilege" ) const ( @@ -34,12 +36,10 @@ const ( RootNamespaceID ID = 0 // System IDs should remain <= MaxReservedDescID. - // systemDatabaseID is the ID of the system database. - systemDatabaseID ID = 1 - // namespaceTableID is the ID of the namespace table. - namespaceTableID ID = 2 - // descriptorTableID is the ID of the descriptor table. + systemDatabaseID ID = 1 + namespaceTableID ID = 2 descriptorTableID ID = 3 + usersTableID ID = 4 // sql CREATE commands and full schema for each system table. // TODO(marc): wouldn't it be better to use a pre-parsed version? @@ -56,14 +56,22 @@ CREATE TABLE system.descriptor ( "id" INT PRIMARY KEY, "desc" BLOB );` + + usersTableSchema = ` +CREATE TABLE system.users ( + "username" CHAR PRIMARY KEY, + "hashedPassword" BLOB +);` ) var ( // SystemDB is the descriptor for the system database. SystemDB = DatabaseDescriptor{ - Name: "system", - ID: systemDatabaseID, - Privileges: NewSystemObjectPrivilegeDescriptor(), + Name: "system", + ID: systemDatabaseID, + // Assign max privileges to root user. + Privileges: NewPrivilegeDescriptor(security.RootUser, + SystemAllowedPrivileges[systemDatabaseID]), } // NamespaceTable is the descriptor for the namespace table. @@ -71,6 +79,20 @@ var ( // DescriptorTable is the descriptor for the descriptor table. DescriptorTable = createSystemTable(descriptorTableID, descriptorTableSchema) + + // UsersTable is the descriptor for the users table. + UsersTable = createSystemTable(usersTableID, usersTableSchema) + + // SystemAllowedPrivileges describes the privileges allowed for each + // system object. No user may have more than those privileges, and + // the root user must have exactly those privileges. + // CREATE|DROP|ALL should always be denied. + SystemAllowedPrivileges = map[ID]privilege.List{ + systemDatabaseID: privilege.ReadData, + namespaceTableID: privilege.ReadData, + descriptorTableID: privilege.ReadData, + usersTableID: privilege.ReadWriteData, + } ) func createSystemTable(id ID, cmd string) TableDescriptor { @@ -84,7 +106,10 @@ func createSystemTable(id ID, cmd string) TableDescriptor { log.Fatal(err) } - desc.Privileges = SystemDB.Privileges + // Assign max privileges to root user. + desc.Privileges = NewPrivilegeDescriptor(security.RootUser, + SystemAllowedPrivileges[id]) + desc.ID = id if err := desc.AllocateIDs(); err != nil { log.Fatal(err) @@ -103,6 +128,7 @@ func GetInitialSystemValues() []proto.KeyValue { {RootNamespaceID, &SystemDB}, {SystemDB.ID, &NamespaceTable}, {SystemDB.ID, &DescriptorTable}, + {SystemDB.ID, &UsersTable}, } numEntries := 1 + len(systemData)*2 diff --git a/sql/testdata/system b/sql/testdata/system index 490cf1c0562b..35d2bfda1385 100644 --- a/sql/testdata/system +++ b/sql/testdata/system @@ -9,6 +9,7 @@ SHOW TABLES FROM system ---- descriptor namespace +users query ITTB EXPLAIN (DEBUG) SELECT * FROM system.namespace @@ -17,6 +18,7 @@ EXPLAIN (DEBUG) SELECT * FROM system.namespace 1 /namespace/primary/0/'test'/id 1000 true 2 /namespace/primary/1/'descriptor'/id 3 true 3 /namespace/primary/1/'namespace'/id 2 true +4 /namespace/primary/1/'users'/id 4 true query ITI SELECT * FROM system.namespace @@ -25,6 +27,7 @@ SELECT * FROM system.namespace 0 test 1000 1 descriptor 3 1 namespace 2 +1 users 4 query I SELECT id FROM system.descriptor @@ -32,6 +35,7 @@ SELECT id FROM system.descriptor 1 2 3 +4 1000 query TTT @@ -49,6 +53,11 @@ SHOW GRANTS ON system.descriptor ---- descriptor root GRANT,SELECT +query TTT +SHOW GRANTS ON system.users +---- +users root DELETE,GRANT,INSERT,SELECT,UPDATE + # Non-root users can have privileges on system objects, but limited to GRANT, SELECT. statement error user testuser must not have ALL privileges on system objects GRANT ALL ON DATABASE system TO testuser