Skip to content

Commit

Permalink
Enforce database-level privileges: #1830
Browse files Browse the repository at this point in the history
Enforce read/write db-level privileges.

I've annotated the enforced functions with the privileges required
and notes about what postgres and mysql do. This will be useful when
adding real privileges (eg: create|select|insert|delete|etc...).
  • Loading branch information
marc committed Aug 5, 2015
1 parent b75d93b commit 842efc8
Show file tree
Hide file tree
Showing 13 changed files with 366 additions and 117 deletions.
17 changes: 17 additions & 0 deletions sql/create.go
Expand Up @@ -18,17 +18,27 @@
package sql

import (
"fmt"

"github.com/cockroachdb/cockroach/keys"
"github.com/cockroachdb/cockroach/security"
"github.com/cockroachdb/cockroach/sql/parser"
"github.com/cockroachdb/cockroach/structured"
)

// CreateDatabase creates a database.
// Privileges: "root" user.
// Notes: postgres requires superuser or "CREATEDB".
// mysql uses the mysqladmin command.
func (p *planner) CreateDatabase(n *parser.CreateDatabase) (planNode, error) {
if n.Name == "" {
return nil, errEmptyDatabaseName
}

if p.user != security.RootUser {
return nil, fmt.Errorf("only %s is allowed to create databases", security.RootUser)
}

nameKey := keys.MakeNameMetadataKey(structured.RootNamespaceID, string(n.Name))
desc := makeDatabaseDesc(n)

Expand All @@ -39,6 +49,8 @@ func (p *planner) CreateDatabase(n *parser.CreateDatabase) (planNode, error) {
}

// CreateTable creates a table.
// Privileges: WRITE on database.
// Notes: postgres/mysql require CREATE on database.
func (p *planner) CreateTable(n *parser.CreateTable) (planNode, error) {
if err := p.normalizeTableName(n.Table); err != nil {
return nil, err
Expand All @@ -49,6 +61,11 @@ func (p *planner) CreateTable(n *parser.CreateTable) (planNode, error) {
return nil, err
}

if !dbDesc.HasPrivilege(p.user, parser.PrivilegeWrite) {
return nil, fmt.Errorf("user %s does not have %s privilege on database %s",
p.user, parser.PrivilegeWrite, dbDesc.Name)
}

desc, err := makeTableDesc(n)
if err != nil {
return nil, err
Expand Down
111 changes: 0 additions & 111 deletions sql/driver/driver_test.go
Expand Up @@ -22,7 +22,6 @@ import (
"fmt"
"testing"

"github.com/cockroachdb/cockroach/security"
"github.com/cockroachdb/cockroach/server"
"github.com/cockroachdb/cockroach/testutils"
"github.com/cockroachdb/cockroach/util/leaktest"
Expand Down Expand Up @@ -402,113 +401,3 @@ func TestUpdate(t *testing.T) {

}
}

func TestInsecure(t *testing.T) {
defer leaktest.AfterTest(t)
// Start test server in insecure mode.
s := &server.TestServer{}
s.Ctx = server.NewTestContext()
s.Ctx.Insecure = true
if err := s.Start(); err != nil {
t.Fatalf("Could not start server: %v", err)
}
t.Logf("Test server listening on %s: %s", s.Ctx.RequestScheme(), s.ServingAddr())
defer s.Stop()

// We can't attempt a connection through HTTPS since the client just retries forever.
// DB connection using plain HTTP.
db, err := sql.Open("cockroach", "http://root@"+s.ServingAddr())
if err != nil {
t.Fatal(err)
}
defer func() {
_ = db.Close()
}()
if _, err := db.Exec("CREATE DATABASE t"); err != nil {
t.Fatal(err)
}
}

func TestPrivileges(t *testing.T) {
defer leaktest.AfterTest(t)
s, db := setup(t)
defer cleanup(s, db)

testCases := []struct {
query string
errPattern string /* empty for success */
results [][]string /* nil for responses that do not return rows */
}{
{`CREATE DATABASE foo`, "", nil},
{`SHOW GRANTS ON DATABASE foo`, "",
[][]string{{"Database", "User", "Privileges"},
{"foo", security.RootUser, "READ, WRITE"}}},

// Only "ON DATABASE x" is supported for now.
{`SHOW GRANTS`, ".*TODO\\(marc\\): multiple targets not implemented", nil},
{`SHOW GRANTS ON DATABASE foo,bar`, ".*TODO\\(marc\\): multiple targets not implemented", nil},
{`SHOW GRANTS ON foo.tbl`, "syntax error .*", nil},
{`SHOW GRANTS ON tbl`, "syntax error .*", nil},

// Can't revoke permissions for root.
{`REVOKE READ ON DATABASE foo FROM root`, "root user does not have read privilege", nil},

{`GRANT ALL ON DATABASE foo TO readwrite, "test-user"`, "", nil},
{`SHOW GRANTS ON DATABASE foo`, "",
[][]string{{"Database", "User", "Privileges"},
{"foo", "readwrite", "READ, WRITE"},
{"foo", security.RootUser, "READ, WRITE"},
{"foo", "test-user", "READ, WRITE"}}},
{`SHOW GRANTS ON DATABASE foo FOR readwrite, "test-user"`, "",
[][]string{{"Database", "User", "Privileges"},
{"foo", "readwrite", "READ, WRITE"},
{"foo", "test-user", "READ, WRITE"}}},

{`REVOKE WRITE ON DATABASE foo FROM "test-user",readwrite`, "", nil},
{`SHOW GRANTS ON DATABASE foo`, "",
[][]string{{"Database", "User", "Privileges"},
{"foo", "readwrite", "READ"},
{"foo", security.RootUser, "READ, WRITE"},
{"foo", "test-user", "READ"}}},
{`SHOW GRANTS ON DATABASE foo FOR readwrite, "test-user"`, "",
[][]string{{"Database", "User", "Privileges"},
{"foo", "readwrite", "READ"},
{"foo", "test-user", "READ"}}},

{`REVOKE READ ON DATABASE foo FROM "test-user"`, "", nil},
{`SHOW GRANTS ON DATABASE foo`, "",
[][]string{{"Database", "User", "Privileges"},
{"foo", "readwrite", "READ"},
{"foo", security.RootUser, "READ, WRITE"}}},
{`SHOW GRANTS ON DATABASE foo FOR readwrite, "test-user"`, "",
[][]string{{"Database", "User", "Privileges"},
{"foo", "readwrite", "READ"}}},

{`REVOKE ALL ON DATABASE foo FROM readwrite,"test-user"`, "", nil},
{`SHOW GRANTS ON DATABASE foo`, "",
[][]string{{"Database", "User", "Privileges"},
{"foo", security.RootUser, "READ, WRITE"}}},
{`SHOW GRANTS ON DATABASE foo FOR readwrite, "test-user"`, "",
[][]string{{"Database", "User", "Privileges"}}},
}

for _, tc := range testCases {
rows, err := db.Query(tc.query)
if tc.errPattern == "" {
if err != nil {
t.Fatalf("query %q failed: %s", tc.query, err)
}
} else if err == nil {
t.Fatalf("query %q: unexpected success", tc.query)
} else if !isError(err, tc.errPattern) {
t.Fatalf("query %q: wrong error, expected %s, got %s", tc.query, tc.errPattern, err)
}
if tc.results != nil {
results := readAll(t, rows)
if err := verifyResults(asResultSlice(tc.results), results); err != nil {
t.Fatalf("query %q: expected results %q, got %q", tc.query, asResultSlice(tc.results), results)
}
}

}
}
190 changes: 190 additions & 0 deletions sql/driver/permission_test.go
@@ -0,0 +1,190 @@
// Copyright 2015 The Cockroach Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
// implied. See the License for the specific language governing
// permissions and limitations under the License. See the AUTHORS file
// for names of contributors.
//
// Author: Marc Berhault (marc@cockroachlabs.com)

package driver_test

import (
"database/sql"
"testing"

"github.com/cockroachdb/cockroach/security"
"github.com/cockroachdb/cockroach/server"
"github.com/cockroachdb/cockroach/util/leaktest"
)

var (
rootUser = security.RootUser
testUser = server.TestUser
)

// setupMultiuser creates a testserver and two clients, one for "root",
// the other for "testuser".
func setupMultiuser(t *testing.T) (*server.TestServer, *sql.DB, *sql.DB) {
s := server.StartTestServer(nil)
dbRoot, err := sql.Open("cockroach", "https://"+rootUser+"@"+s.ServingAddr()+"?certs=test_certs")
if err != nil {
t.Fatal(err)
}
dbTest, err := sql.Open("cockroach", "https://"+testUser+"@"+s.ServingAddr()+"?certs=test_certs")
if err != nil {
t.Fatal(err)
}
return s, dbRoot, dbTest
}

func cleanupMultiuser(s *server.TestServer, dbRoot, dbTest *sql.DB) {
_ = dbRoot.Close()
_ = dbTest.Close()
s.Stop()
}

func TestInsecure(t *testing.T) {
defer leaktest.AfterTest(t)
// Start test server in insecure mode.
s := &server.TestServer{}
s.Ctx = server.NewTestContext()
s.Ctx.Insecure = true
if err := s.Start(); err != nil {
t.Fatalf("Could not start server: %v", err)
}
t.Logf("Test server listening on %s: %s", s.Ctx.RequestScheme(), s.ServingAddr())
defer s.Stop()

// We can't attempt a connection through HTTPS since the client just retries forever.
// DB connection using plain HTTP.
db, err := sql.Open("cockroach", "http://root@"+s.ServingAddr())
if err != nil {
t.Fatal(err)
}
defer func() {
_ = db.Close()
}()
if _, err := db.Exec("CREATE DATABASE t"); err != nil {
t.Fatal(err)
}
}

func TestDefaultPrivileges(t *testing.T) {
defer leaktest.AfterTest(t)
s, dbRoot, dbTest := setupMultiuser(t)
defer cleanupMultiuser(s, dbRoot, dbTest)

// All statements must succeed with dbRoot.
// Statements with success==true must succeed with dbTest.
// They are evaluated in order, with dbTest first followed by dbRoot.
testCases := []struct {
query string
success bool
}{
/* Database-level permissions */
{`CREATE DATABASE foo`, false},

{`SHOW DATABASES`, true},
{`SET DATABASE = foo`, true},

{`CREATE TABLE tbl (id INT PRIMARY KEY)`, false},

{`SHOW TABLES`, true},
{`SHOW GRANTS ON DATABASE foo`, true},

{`GRANT ALL ON DATABASE foo TO bar`, false},
{`REVOKE ALL ON DATABASE foo FROM bar`, false},
}

for _, tc := range testCases {
if _, err := dbTest.Exec(tc.query); (err == nil) != tc.success {
t.Fatalf("statement %q using testuser has err=%s, expected success=%t", tc.query, err, tc.success)
}
if _, err := dbRoot.Exec(tc.query); err != nil {
t.Fatalf("statement %q using root failed: %v", tc.query, err)
}
}
}

func TestReadPrivileges(t *testing.T) {
defer leaktest.AfterTest(t)
s, dbRoot, dbTest := setupMultiuser(t)
defer cleanupMultiuser(s, dbRoot, dbTest)

if _, err := dbRoot.Exec(`CREATE DATABASE foo`); err != nil {
t.Fatal(err)
}
if _, err := dbRoot.Exec(`GRANT READ ON DATABASE foo TO testuser`); err != nil {
t.Fatal(err)
}

// All statements must succeed with dbRoot.
// Statements with success==true must succeed with dbTest.
// They are evaluated in order, with dbTest first followed by dbRoot.
testCases := []struct {
query string
success bool
}{
{`SHOW DATABASES`, true},
{`SET DATABASE = foo`, true},

{`CREATE TABLE tbl (id INT PRIMARY KEY)`, false},

{`SHOW TABLES`, true},
{`SHOW GRANTS ON DATABASE foo`, true},

{`GRANT ALL ON DATABASE foo TO bar`, false},
{`REVOKE ALL ON DATABASE foo FROM bar`, false},
}

for _, tc := range testCases {
if _, err := dbTest.Exec(tc.query); (err == nil) != tc.success {
t.Fatalf("statement %q using testuser has err=%s, expected success=%t", tc.query, err, tc.success)
}
}
}

func TestWritePrivileges(t *testing.T) {
defer leaktest.AfterTest(t)
s, dbRoot, dbTest := setupMultiuser(t)
defer cleanupMultiuser(s, dbRoot, dbTest)

if _, err := dbRoot.Exec(`CREATE DATABASE foo`); err != nil {
t.Fatal(err)
}
if _, err := dbRoot.Exec(`GRANT WRITE ON DATABASE foo TO testuser`); err != nil {
t.Fatal(err)
}

// Statements with success==true must succeed with dbTest.
testCases := []struct {
query string
success bool
}{
{`SHOW DATABASES`, true},
{`SET DATABASE = foo`, true},

{`CREATE TABLE tbl (id INT PRIMARY KEY)`, true},

{`SHOW TABLES`, true},
{`SHOW GRANTS ON DATABASE foo`, true},

{`GRANT ALL ON DATABASE foo TO bar`, true},
{`REVOKE ALL ON DATABASE foo FROM bar`, true},
}

for _, tc := range testCases {
if _, err := dbTest.Exec(tc.query); (err == nil) != tc.success {
t.Fatalf("statement %q using testuser has err=%s, expected success=%t", tc.query, err, tc.success)
}
}
}
10 changes: 10 additions & 0 deletions sql/grant.go
Expand Up @@ -18,6 +18,8 @@
package sql

import (
"fmt"

"github.com/cockroachdb/cockroach/keys"
"github.com/cockroachdb/cockroach/sql/parser"
"github.com/cockroachdb/cockroach/util"
Expand All @@ -30,6 +32,9 @@ import (
// TODO(marc): open questions:
// - should we have root always allowed and not present in the permissions list?
// - should we make users case-insensitive?
// Privileges: WRITE on database.
// Notes: postgres requires the object owner.
// mysql requires the "grant option" and the same privileges, and sometimes superuser.
func (p *planner) Grant(n *parser.Grant) (planNode, error) {
if len(n.Targets.Targets) == 0 {
return nil, errEmptyDatabaseName
Expand All @@ -45,6 +50,11 @@ func (p *planner) Grant(n *parser.Grant) (planNode, error) {
return nil, err
}

if !dbDesc.HasPrivilege(p.user, parser.PrivilegeWrite) {
return nil, fmt.Errorf("user %s does not have %s privilege on database %s",
p.user, parser.PrivilegeWrite, dbDesc.Name)
}

if err := dbDesc.Grant(n); err != nil {
return nil, err
}
Expand Down

0 comments on commit 842efc8

Please sign in to comment.