more privilege changes #3810

jycor · 2022-07-12T19:39:53Z

Changes:

add --user to dolt sql and dolt sql -q
default user is persisted immediately iff --user or --password option is given
default super user is root@localhost instead of root@%
only create doltcfgdir if we need to persist something
has more tests

Companion PR: dolthub/go-mysql-server#1127

Fix for: #3794
Also a fix for: #3881

Hydrocharged · 2022-07-20T12:41:00Z

This needs a few changes as per our earlier conversation offline (with some additional comments):

dolt sql does not need a --password argument, as the CLI has full access and it does not make logical sense to add password checking or even the ability to specify one
The default user is the super user which is not persisted at all, and should actually be invisible to queries on the mysql.user table
For both dolt sql and dolt sql-server, if the user given via --user exists then we assume that user. What this means for dolt sql-server is that, since users are attached to connecting clients, the --user directive is just ignored if the user exists. This would also mean that --password is ignored for existing users.
If no user is specified for dolt sql then we can default to root. Perhaps we should default to a unique user name to guarantee that you're always the super user if no user is specified, but defaulting to root is also fine.
If no user is specified for dolt sql-server and the privilege file does not exist then we create the default root super user. If a privilege file exists then we only create a super user if one was specified through --user. This prevents the situation where an admin is unaware that a root super user exists that others can log into (assuming they can connect as localhost).

Of course this would all also need to be in docs too.

…hell

…te.sh

…mes/users

Hydrocharged

Have to at least figure out why the tests are failing on CI. Current server behavior for --user restricts people from using root as their account name for the simplest use cases, which I'm confident someone will run into.

Hydrocharged · 2022-07-26T15:14:22Z

go/cmd/dolt/commands/sqlserver/server.go

+		}
+	} else {
+		// no privileges, must add superuser; will already be defaulted to root
+		sqlEngine.GetUnderlyingEngine().Analyzer.Catalog.MySQLDb.AddSuperUser(config.ServerUser, "%", config.ServerPass)


Shouldn't we be using config.ServerHost? Otherwise it looks like it's getting completely ignored. I'm assuming the default value is localhost, so it should be % (if it's not possible to have an empty default).

We should, but the Sysbench and Compatibility tests fail with permission denied.
Default is localhost.

We can modify both the sysbench and compatibility tests if necessary. They should be able to work with empty defaults though, as the resulting logic should end up equivalent unless they’re specifying a root user.

Functionally equivalent in this particular case I mean.

Hydrocharged · 2022-07-26T15:16:32Z

go/cmd/dolt/commands/sqlserver/server.go

+		// privileges specified, only add if superuser specified is not an existing user
+		userSpecified := config.ServerUser != defaultUser || config.ServerHost != defaultHost || config.ServerPass != defaultPass
+		superuser := sqlEngine.GetUnderlyingEngine().Analyzer.Catalog.MySQLDb.GetUser(config.ServerUser, "%", false)
+		if userSpecified && superuser == nil {
+			sqlEngine.GetUnderlyingEngine().Analyzer.Catalog.MySQLDb.AddSuperUser(config.ServerUser, "%", config.ServerPass)
+		}


Assuming that people want to make a root account for adding privileges (so they'd spin up a server just to make the changes and potentially not worry about setting a password or changing the host), then they cannot use root. They're required to use the non-default password to recognize that we should be using the root account, even though it's specified as an argument.

Hydrocharged · 2022-07-26T15:18:56Z

integration-tests/bats/sql-privs.bats

@@ -45,6 +45,7 @@ delete_test_repo() {
 }

 setup() {
+    skiponwindows "no clue why this fails on CI"


Needs to be fixed, or we should at least have a clue as to why it's failing CI before a customer sees the same issues (especially since CI uses the same workflow that we recommend to customers)

Hydrocharged

A few simple changes and we're set!

Hydrocharged · 2022-07-27T15:20:51Z

go/cmd/dolt/commands/sqlserver/server.go

+	// Add superuser
+	//if sqlEngine.GetUnderlyingEngine().Analyzer.Catalog.MySQLDb.UserTable().Data().Count() != 0 {
+	//	// privileges specified, only add if superuser specified is not an existing user
+	//	userSpecified := config.ServerUser != defaultUser || config.ServerHost != defaultHost || config.ServerPass != defaultPass
+	//	superuser := sqlEngine.GetUnderlyingEngine().Analyzer.Catalog.MySQLDb.GetUser(config.ServerUser, serverHost, false)
+	//	if userSpecified && superuser == nil {
+	//		sqlEngine.GetUnderlyingEngine().Analyzer.Catalog.MySQLDb.AddSuperUser(config.ServerUser, serverHost, config.ServerPass)
+	//	}
+	//} else {
+	//	// no privileges, must add superuser; will already be defaulted to root
+	//	sqlEngine.GetUnderlyingEngine().Analyzer.Catalog.MySQLDb.AddSuperUser(config.ServerUser, serverHost, config.ServerPass)
+	//}


Delete this comment

Hydrocharged · 2022-07-27T15:21:55Z

go/cmd/dolt/commands/sqlserver/server.go

+	// should user "%" if host is empty string, same for 0.0.0.0
+	serverHost := config.ServerHost
+	if serverHost == "" || serverHost == "0.0.0.0" {
+		serverHost = "%"


Add a TODO to move this into GMS

Hydrocharged · 2022-07-27T15:22:20Z

go/cmd/dolt/commands/sqlserver/serverconfig.go

+	//if len(config.User()) == 0 {
+	//	return fmt.Errorf("user cannot be empty")
+	//}


Delete the comment

Hydrocharged · 2022-07-27T15:23:23Z

integration-tests/bats/migrate.bats

@@ -2,6 +2,7 @@
 load $BATS_TEST_DIRNAME/helper/common.bash

 setup() {
+    skip "temporary skip"


Remove this skip so that we'll properly see it fail, but since it's not from your changes we should still commit this

Hydrocharged · 2022-07-27T15:25:13Z

integration-tests/bats/sql-server.bats

@@ -231,7 +231,7 @@ SQL
    [[ "$output" =~ "one_pk" ]] || false

    # Add rows on the command line
-    run dolt sql -q "insert into one_pk values (1,1,1)"
+    run dolt sql --user=dolt -q "insert into one_pk values (1,1,1)"


We need a test that show the user assumption if the privilege file already exists. We can do this by setting a password in the privilege file and providing a different password as an argument, then attempting to log in.

James Cor added 11 commits July 12, 2022 12:36

moving config creation to same place

cc8def0

merge with main

735b4de

working so far, non server needs to check password

df7c21e

merge with other branch

c4df29e

Merge branch 'james/local-user' into james/users

c9855cf

not sure what to do next

9f4684d

Merge branch 'main' into james/users

4788d90

Merge branch 'main' into james/users

4cc1789

probably good enough

9e5ec16

fixing docs

40a4139

deleting comment

e75f92e

jycor changed the title ~~[WIP] more privilege changes~~ more privilege changes Jul 15, 2022

James Cor added 3 commits July 15, 2022 14:33

Merge branch 'main' into james/users

9764c53

changing superuser to just be local

a3d429f

fixing logic

9f9f5a6

jycor mentioned this pull request Jul 15, 2022

superuser is localhost dolthub/go-mysql-server#1118

Merged

James Cor added 7 commits July 18, 2022 10:51

need to add user to sql -q

743c9fc

missed a user

8b713b0

only start iff user exists

59661c3

adding tests

c2f9923

tests hang??

d063419

only immediately persist if it is dolt sql-server

0bedf7c

aaaaaaaaa

49af15f

James Cor added 6 commits July 20, 2022 09:40

Merge branch 'main' into james/users

6b6b7d8

remove hanging test for now, and remove password argument for local s…

30220ee

…hell

fixed?

5029d66

aaaaaaaaaaaa

ea1dcc2

Merge branch 'main' into james/users

39dda18

asdfasdfasdf

edc9c13

James Cor and others added 14 commits July 24, 2022 23:44

asdfas

160ae55

what the heck is going on

d35df20

adding back user length check

d3770c4

adding error for sql-client missing user argument

3c5b211

changing message

9dad81d

aa

d1766a3

adding tests

d54123b

merging with main

4f7300f

[ga-format-pr] Run go/utils/repofmt/format_repo.sh and go/Godeps/upda…

9981501

…te.sh

yeah yeah yeah

29525e9

Merge branch 'james/users' of https://github.com/dolthub/dolt into ja…

d7ed044

…mes/users

adding tests

a1235c9

adding test

7e9ddee

more conflicts woo

3bf7ad5

Hydrocharged reviewed Jul 26, 2022

View reviewed changes

James Cor added 9 commits July 26, 2022 10:02

changing fail message

d90e637

replacing serverhost with %

05028f7

let's see what tests fail now

9b27046

more conflicts

3a244ac

aa

7972b60

unskip

3d56d93

Merge branch 'main' into james/users

69e16d3

merge with main again

0c27290

aaaaaaaaaaaaaaaaa

e97debe

Hydrocharged approved these changes Jul 27, 2022

View reviewed changes

James Cor added 4 commits July 27, 2022 12:31

feedback

38fc8b1

adding test for logging in with bad user

ee474de

mac grep is weird

14075c3

maybe sleeping is the answer

7ca169e

jycor merged commit 9623166 into main Jul 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

more privilege changes #3810

more privilege changes #3810

jycor commented Jul 12, 2022 •

edited

Hydrocharged commented Jul 20, 2022

Hydrocharged left a comment

Hydrocharged Jul 26, 2022

jycor Jul 26, 2022

Hydrocharged Jul 26, 2022

Hydrocharged Jul 26, 2022

Hydrocharged Jul 26, 2022

Hydrocharged Jul 26, 2022

Hydrocharged left a comment

Hydrocharged Jul 27, 2022

Hydrocharged Jul 27, 2022

Hydrocharged Jul 27, 2022

Hydrocharged Jul 27, 2022

Hydrocharged Jul 27, 2022

more privilege changes #3810

more privilege changes #3810

Conversation

jycor commented Jul 12, 2022 • edited

Hydrocharged commented Jul 20, 2022

Hydrocharged left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hydrocharged left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jycor commented Jul 12, 2022 •

edited