sql: add CREATE USER stmt and pgwire password-based authentication

[asubiotto]

Add mechanism to create users and then authenticate as them through a pgwire connection. This takes care of #6457.

@tamird @petermattis @cuongdo @mberhault

---

This change is

[tamird]

Haven't reviewed anything yet, but please avoid writing large PR descriptions; instead, include whatever information is necessary in the commit message. See "Good Commit Message" area of https://github.com/cockroachdb/cockroach/blob/master/CONTRIBUTING.md#code-review-workflow.

[petermattis]

Review status: 0 of 30 files reviewed at latest revision, 1 unresolved discussion.

---

security/password.go, line 62 at r1 (raw file):

      return "", err
  }
  return MD5Hash(password + username), nil

It's unfortunate that pgwire is forcing how we store hashed passwords. I don't keep up on these things, but I'm pretty sure md5 is frowned upon from a security perspective. Cc @bdarnell, @arjunravinarayan, @mberhault.

---

Comments from Reviewable

[rjnn]

security/password.go, line 62 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

It's unfortunate that pgwire is forcing how we store hashed passwords. I don't keep up on these things, but I'm pretty sure md5 is frowned upon from a security perspective. Cc @bdarnell, @arjunravinarayan, @mberhault.

Yes, `md5` is very much frowned upon as a password hash. These days, only `bcrypt` or `scrypt` are really acceptable (with an appropriately set work factor), but I don't know what the implications are for `pgwire` compatibility. We could simply `scrypt` the `md5sum`, so it's vulnerable in transit, but not on durable store.

---

Comments from Reviewable

[tamird]

Reviewed 28 of 30 files at r1.
Review status: 28 of 30 files reviewed at latest revision, 23 unresolved discussions.

---

cli/sql.go, line 503 at r1 (raw file):

  // Open the connection to make sure everything is OK before running any
  // statements. Performs authentication.
  if err = conn.ensureConn(); err != nil {

use := - you don't want to overwrite the err from above.

---

cli/user.go, line 136 at r1 (raw file):

  username := args[0]

  var err error

while you're here, move this inside the if - there is no need for it to be so broadly scoped.

---

cli/user.go, line 143 at r1 (raw file):

          panic(err)
      }
  case "-":

why remove support for this?

---

cli/cliflags/flags.go, line 235 at r1 (raw file):

      EnvVar: "COCKROACH_PASSWORD",
      Description: `
The created user's password. If provided, disables prompting. Pass '-' to

ditto: why remove this?

---

security/password.go, line 54 at r1 (raw file):

}

// PromptForPasswordAndHash prompts for a password on the stdin twice, and if

s/the//

---

security/password.go, line 55 at r1 (raw file):

// PromptForPasswordAndHash prompts for a password on the stdin twice, and if
// both match, returns the hash of the password + username. This hash is

remove the last sentence and just say "returns the md5 hash of the password concatenated with the username".

---

sql/logic_test.go, line 472 at r1 (raw file):

  }

  if _, err := t.db.Exec("CREATE USER testuser;"); err != nil {

can this be ("CREATE USER $1", security.TestUser")?

---

sql/pgwire_test.go, line 1225 at r1 (raw file):

  defer s.Stopper().Stop()

  rootPgURL, cleanupFn := sqlutils.PGUrl(t, s.ServingAddr(), "TestPGWireAuth", url.User(security.RootUser))

i don't remember the implications of this, but i'd certainly feel better if you only have one of these outstanding at a time.

---

sql/pgwire_test.go, line 1230 at r1 (raw file):

  // Authenticate as a user not in the system.users table.
  err := trivialQuery(testUserPgURL)

put this into the if so that err does not live beyond this block.

---

sql/pgwire_test.go, line 1232 at r1 (raw file):

  err := trivialQuery(testUserPgURL)
  if err == nil {
      t.Fatal("unexpected successful authentication")

no need for this case; IsError(nil, "foo") == false

---

sql/pgwire_test.go, line 1238 at r1 (raw file):

  // Authentication should be ok since auth is not implemented for root.
  err = trivialQuery(rootPgURL)

throughout: prefer := and reduce the scope of err by putting it in the if

---

sql/pgwire_test.go, line 1249 at r1 (raw file):

  defer db.Close()

  _, err = db.Exec(fmt.Sprintf("CREATE USER %s WITH PASSWORD '蟑螂';", server.TestUser))

can this use a placeholder instead?

---

sql/parser/stmt.go, line 120 at r1 (raw file):

// StatementType implements the Statement interface.
func (*CreateUser) StatementType() StatementType { return DDL }

is it a DDL? how'd you arrive at that?

---

sql/pgwire/v3.go, line 102 at r1 (raw file):

const (
  authOK  int32 = 0
  authMD5 int32 = 5

authMD5Password

---

sql/pgwire/v3.go, line 348 at r1 (raw file):

  c.writeBuf.putInt32(authMD5)
  c.writeBuf.write(salt)
  if err = c.writeBuf.finishMsg(c.wr); err != nil {

throughout: change these = to :=

---

sql/pgwire/v3.go, line 362 at r1 (raw file):

  if typ != clientMsgPassword {
      return errors.New("invalid response to authentication request")

use errors.Errorf and include the typ you got?

---

sql/pgwire/v3.go, line 364 at r1 (raw file):

      return errors.New("invalid response to authentication request")
  }
  if err = c.handleAuthentication(&c.readBuf, salt); err != nil {

return c.handle(...)

---

sql/pgwire/v3.go, line 382 at r1 (raw file):

  // PasswordMessage is computed by the client as:
  // concat('md5', md5(concat(md5(concat(password, username)), random-salt)))
  if !strings.HasPrefix(passwordMessage, "md5") || len(passwordMessage) == 3 {

== 3? md5 is always 16 bytes, so this should be 3 + md5.Size.

---

sql/pgwire/v3.go, line 393 at r1 (raw file):

  // have SELECT privilege on the system.users table.
  requestedUser := c.session.User
  c.session.User = security.RootUser

instead of modifying c.session, make a shallow copy of it and modify that

rootSession := c.session
rootSession.User = security.RootUser

---

sql/pgwire/v3.go, line 404 at r1 (raw file):

  } else if r.ResultList[0].Rows.Len() != 1 {
      return c.sendErrorWithCode(pgerror.CodeNoDataFoundError, sqlbase.MakeSrcCtx(0),
          "user does not exist")

include the user in this string

---

sql/testdata/user, line 1 at r1 (raw file):

statement error

what's the error?

---

sql/testdata/user, line 4 at r1 (raw file):

CREATE USER user1

statement error user already exists

this error should include the user whose creation was attempted

---

Comments from Reviewable

[mberhault]

Review status: 28 of 30 files reviewed at latest revision, 23 unresolved discussions.

---

security/password.go, line 62 at r1 (raw file):

Previously, arjunravinarayan (Arjun Narayan) wrote…

Yes, md5 is very much frowned upon as a password hash. These days, only bcrypt or scrypt are really acceptable (with an appropriately set work factor), but I don't know what the implications are for pgwire compatibility. We could simply scrypt the md5sum, so it's vulnerable in transit, but not on durable store.

chiming in as well, we definitely do not want to store md5 hashes, they're terribly insecure, even with salt. As Arjun mentioned, we could bcrypt the md5 hash instead of the actual password, although I have no idea what the security implications might be. even bcrypt is only considered "safe" in the long term if you keep adjusting the number of rounds, and we'll need to add a mechanism for this at some point.

As far as over-the-wire security, sending anything unencrypted will be a bad idea, so I'm not too worried there, you should still use SSL with server-side certificates when using passwords.

---

Comments from Reviewable

[cuongdo]

The non-SQL parts look good to me. Has anyone reviewed the changes to planner?

---

Review status: 28 of 30 files reviewed at latest revision, 26 unresolved discussions.

---

cli/sql_util.go, line 191 at r1 (raw file):

          user = url.User(connUser)
      } else {
          password, err := security.PromptForPassword()

There should also be a new command-line flag for specifying a password. This is useful for tests and command-line automation. Probably as a separate PR.

---

security/password.go, line 77 at r1 (raw file):

  h := md5.New()
  h.Write([]byte(s))
  return hex.EncodeToString(h.Sum(nil))

can you skip the line above and just encode h.Sum([]byte(s))?

---

sql/pgwire_test.go, line 1263 at r1 (raw file):

  }

  testUserPgURL.User = url.UserPassword(server.TestUser, "蟑螂")

could you also test with a Unicode user name?

---

Comments from Reviewable

[petermattis]

Review status: 28 of 30 files reviewed at latest revision, 26 unresolved discussions.

---

security/password.go, line 62 at r1 (raw file):

Previously, mberhault (marc) wrote…

chiming in as well, we definitely do not want to store md5 hashes, they're terribly insecure, even with salt. As Arjun mentioned, we could bcrypt the md5 hash instead of the actual password, although I have no idea what the security implications might be.
even bcrypt is only considered "safe" in the long term if you keep adjusting the number of rounds, and we'll need to add a mechanism for this at some point.

As far as over-the-wire security, sending anything unencrypted will be a bad idea, so I'm not too worried there, you should still use SSL with server-side certificates when using passwords.

@arjunravinarayan, @mberhault Please take a look at the pgwire MD5 auth protocol. If we support it I think storing the MD5-hashed password is unavoidable as we need to do `MD5(MD5(password)+salt)` in order to verify the checksum sent by the client. Perhaps I'm missing something.

---

Comments from Reviewable

[maddyblue]

Review status: 28 of 30 files reviewed at latest revision, 29 unresolved discussions, some commit checks failed.

---

cli/sql_test.go, line 40 at r1 (raw file):

  defer s.Stopper().Stop()

  pgurl, err := s.(*server.TestServer).Ctx.PGURL(url.User(security.RootUser))

Does it make sense to change security.RootUser to a url.User to avoid all these conversions?

---

sql/create.go, line 280 at r1 (raw file):

  } else if rowsAffected != 1 {
      return errors.Errorf(
          "%d rows affected by user creation; expected exactly one row affected.", rowsAffected,

no period at end of error message

---

sql/parser/create.go, line 607 at r1 (raw file):

  if node.Password != nil {
      buf.WriteString(" WITH PASSWORD ")
      node.Password.Format(buf, f)

This is problematic because it will save the password to the user's sql cli history file. I'm not sure what the correct thing to do is, though.

---

sql/parser/stmt.go, line 120 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

is it a DDL? how'd you arrive at that?

I think I recommended a DDL. But after consideration I think Ack may be a better choice. I don't remember why I thought DDL was good.

---

Comments from Reviewable

[maddyblue]

Review status: 28 of 30 files reviewed at latest revision, 29 unresolved discussions, some commit checks failed.

---

sql/pgwire/v3.go, line 382 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

== 3? md5 is always 16 bytes, so this should be 3 + md5.Size.

I think the 3 is there to make sure passwordMessage has more characters in it than just "md5".

---

sql/testdata/user, line 1 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

what's the error?

This looks like it should be `statement ok`. Our logic test runner has a bug that allows `statement error` with no message to be an OK.

---

Comments from Reviewable

[bdarnell]

Review status: 28 of 30 files reviewed at latest revision, 29 unresolved discussions, some commit checks failed.

---

security/password.go, line 62 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

@arjunravinarayan, @mberhault Please take a look at the pgwire MD5 auth protocol. If we support it I think storing the MD5-hashed password is unavoidable as we need to do MD5(MD5(password)+salt) in order to verify the checksum sent by the client. Perhaps I'm missing something.

Yes, the postgres MD5 auth protocol requires us to store passwords hashed with a single round of MD5 (applying bcrypt or scrypt to an md5 hash is a legitimate transitional strategy, but it won't work here because of the way the challenge/response protocol is designed). Therefore, we shouldn't use it, and should use the cleartext password mode instead. This will let us use bcrypt (or whatever we want - [argon2](https://github.com/p-h-c/phc-winner-argon2) is apparently the new state of the art, although I'd lean towards bcrypt or scrypt since they've been around longer) on the server side for password storage, and it's safe when combined with TLS.

The md5 protocol attempts to provide some protection for unencrypted connections, but it's not even very good at that. It may be better than nothing for unencrypted connections (depending on your threat model), so it's theoretically something we could use for insecure clusters, but I think it's better to just steer everyone who cares even a little bit about security to TLS instead of making insecure mode slightly less so.

The recent dropbox blog post on password storage is pretty good, in spite of (or more likely because of) their tarnished history of security.

FWIW, postgres is slowly moving towards a new authentication protocol called SCRAM, although it will take a while for this to be supported in client drivers. Here's a long thread about authentication protocols.

---

Comments from Reviewable

[asubiotto]

Review status: 28 of 30 files reviewed at latest revision, 29 unresolved discussions, some commit checks failed.

---

cli/sql_test.go, line 40 at r1 (raw file):

Previously, mjibson (Matt Jibson) wrote…

Does it make sense to change security.RootUser to a url.User to avoid all these conversions?

It does make sense. This would also require some changes elsewhere (e.g. sql/sqlbase/privilege.go). I think this PR is a bit big to include those changes as well so I might leave it to another PR. What do you think?

---

cli/user.go, line 143 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

why remove support for this?

I thought it was redundant since we prompt for the password when none is specified. Do you think I should add it back in?

---

security/password.go, line 62 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Yes, the postgres MD5 auth protocol requires us to store passwords hashed with a single round of MD5 (applying bcrypt or scrypt to an md5 hash is a legitimate transitional strategy, but it won't work here because of the way the challenge/response protocol is designed). Therefore, we shouldn't use it, and should use the cleartext password mode instead. This will let us use bcrypt (or whatever we want - argon2 is apparently the new state of the art, although I'd lean towards bcrypt or scrypt since they've been around longer) on the server side for password storage, and it's safe when combined with TLS.

The md5 protocol attempts to provide some protection for unencrypted connections, but it's not even very good at that. It may be better than nothing for unencrypted connections (depending on your threat model), so it's theoretically something we could use for insecure clusters, but I think it's better to just steer everyone who cares even a little bit about security to TLS instead of making insecure mode slightly less so.

The recent dropbox blog post on password storage is pretty good, in spite of (or more likely because of) their tarnished history of security.

FWIW, postgres is slowly moving towards a new authentication protocol called SCRAM, although it will take a while for this to be supported in client drivers. Here's a long thread about authentication protocols.

Sounds good, I think using cleartext and storing on disk w/ bcrypt would be best to achieve our goals and will change to that.

---

sql/pgwire_test.go, line 1225 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

i don't remember the implications of this, but i'd certainly feel better if you only have one of these outstanding at a time.

It creates on-disk copies of embedded certs in a directory prefixed (in this case) with "TestPGWireAuth". The cleanupFn removes this directory. I agree with you and think I'm going to set insecure=true to avoid cert code.

---

sql/parser/create.go, line 607 at r1 (raw file):

Previously, mjibson (Matt Jibson) wrote…

This is problematic because it will save the password to the user's sql cli history file. I'm not sure what the correct thing to do is, though.

Would it be correct to simply write a string lilke ""?

---

sql/parser/stmt.go, line 120 at r1 (raw file):

Previously, mjibson (Matt Jibson) wrote…

I think I recommended a DDL. But after consideration I think Ack may be a better choice. I don't remember why I thought DDL was good.

Will change to Ack

---

sql/testdata/user, line 1 at r1 (raw file):

Previously, mjibson (Matt Jibson) wrote…

This looks like it should be statement ok. Our logic test runner has a bug that allows statement error with no message to be an OK.

Thanks for catching

---

Comments from Reviewable

[maddyblue]

Review status: 28 of 30 files reviewed at latest revision, 29 unresolved discussions, some commit checks failed.

---

cli/sql_test.go, line 40 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

It does make sense. This would also require some changes elsewhere (e.g. sql/sqlbase/privilege.go). I think this PR is a bit big to include those changes as well so I might leave it to another PR. What do you think?

Sure, another PR for this is fine with me.

---

sql/parser/create.go, line 607 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

Would it be correct to simply write a string lilke ""?

Empty string is bad for other reasons, but I think it may be less bad than other choices. I'm ok with empty string here. @knz thoughts?

---

Comments from Reviewable

[knz]

Review status: 28 of 30 files reviewed at latest revision, 29 unresolved discussions, some commit checks failed.

---

sql/parser/create.go, line 607 at r1 (raw file):

Previously, mjibson (Matt Jibson) wrote…

Empty string is bad for other reasons, but I think it may be less bad than other choices. I'm ok with empty string here. @knz thoughts?

Woah good catch. A couple of things: 1) this code must definitely print out something else than the password. If anything, so that passwords do not end up in log files and traces. 2) I would be in favor of printing an empty string for empty passwords and a fixed non-empty string (like `"******"`) for non-empty passwords. I think it is useful to catch statements that set an empty password, but perhaps this is not the right approach. I can be swayed on this one. 3) the CLI interactive sql shell must be educated about this after point 1 is addressed. This is because the current code will not only save the pretty-printed string in the history; the pretty-printed string is also the statement sent to the server. I can help you addressing this if needed.

---

Comments from Reviewable

[tamird]

Review status: 28 of 30 files reviewed at latest revision, 29 unresolved discussions, some commit checks failed.

---

cli/user.go, line 143 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

I thought it was redundant since we prompt for the password when none is specified. Do you think I should add it back in?

Yeah, reading the password from stdin seems useful.

---

sql/pgwire_test.go, line 1225 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

It creates on-disk copies of embedded certs in a directory prefixed (in this case) with "TestPGWireAuth". The cleanupFn removes this directory. I agree with you and think I'm going to set insecure=true to avoid cert code.

That doesn't seem necessary. These `URL`s seem to be used one at a time - just clean one up before you create the next.

---

sql/pgwire/v3.go, line 382 at r1 (raw file):

Previously, mjibson (Matt Jibson) wrote…

I think the 3 is there to make sure passwordMessage has more characters in it than just "md5".

Yes, I understand, but it should always be exactly `len("md5")+mdg.Size`

---

sql/testdata/user, line 1 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

Thanks for catching

Please fix the bug mentioned by @mjibson if you can, as well.

---

Comments from Reviewable

[knz]

Review status: 28 of 30 files reviewed at latest revision, 29 unresolved discussions, some commit checks failed.

---

sql/parser/create.go, line 607 at r1 (raw file):

Previously, knz (kena) wrote…

Woah good catch. A couple of things:

1. this code must definitely print out something else than the password. If anything, so that passwords do not end up in log files and traces.
2. I would be in favor of printing an empty string for empty passwords and a fixed non-empty string (like "******") for non-empty passwords. I think it is useful to catch statements that set an empty password, but perhaps this is not the right approach. I can be swayed on this one.
3. the CLI interactive sql shell must be educated about this after point 1 is addressed. This is because the current code will not only save the pretty-printed string in the history; the pretty-printed string is also the statement sent to the server. I can help you addressing this if needed.

Ok I've thought about this some more and here is the solution. The situation with the CLI shell I will fix in a separate PR, so you don't have to care about this. I am still confident we want the default Format behavior to hide the password. Should we later find any use case where we need a faithful string render of the query with passwords included, we can solve this by introducing a new possible value for parser.FmtFlags, which selects to print the password in clear. The default value FmtSimple must not print the password, so a call site would then have to call `.Format()` with this new flag.

---

Comments from Reviewable

[asubiotto]

Review status: 15 of 29 files reviewed at latest revision, 29 unresolved discussions.

---

cli/sql_util.go, line 191 at r1 (raw file):

Previously, cuongdo (Cuong Do) wrote…

There should also be a new command-line flag for specifying a password. This is useful for tests and command-line automation. Probably as a separate PR.

Will file an issue

---

cli/user.go, line 136 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

while you're here, move this inside the if - there is no need for it to be so broadly scoped.

Switched the if back to a switch for reading passwords from stdin.

---

sql/logic_test.go, line 472 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

can this be ("CREATE USER $1", security.TestUser")?

It seems that the underlying implementation doesn't allow for placeholders. I will change this to `Exec(fmt.Sprintf("CREATE USER %s", server.TestUser))`

---

sql/pgwire_test.go, line 1225 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

That doesn't seem necessary. These URLs seem to be used one at a time - just clean one up before you create the next.

I'll be adding a test for a unicode username. Since we wouldn't have an embedded cert for that, I would need to stop and restart a server with new parameters to disable secure mode. I think it makes everything cleaner, faster, and quicker to identify problems if remove this certificate directory creation calls. I will make the change to use one url at a time though.

---

sql/pgwire_test.go, line 1249 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

can this use a placeholder instead?

It can't. Same problem as in sql/logic_test.go. The underlying implementation doesn't support placeholders.

---

sql/pgwire_test.go, line 1263 at r1 (raw file):

Previously, cuongdo (Cuong Do) wrote…

could you also test with a Unicode user name?

Sure. I'll add a test for that in sql/testdata/user as well.

---

sql/parser/create.go, line 607 at r1 (raw file):

Previously, knz (kena) wrote…

Ok I've thought about this some more and here is the solution. The situation with the CLI shell I will fix in a separate PR, so you don't have to care about this.
I am still confident we want the default Format behavior to hide the password.
Should we later find any use case where we need a faithful string render of the query with passwords included, we can solve this by introducing a new possible value for parser.FmtFlags, which selects to print the password in clear. The default value FmtSimple must not print the password, so a call site would then have to call .Format() with this new flag.

I'll change this to print "*****" on non-empty passwords. Thanks for catching this Matt!

---

sql/pgwire/v3.go, line 393 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

instead of modifying c.session, make a shallow copy of it and modify that

rootSession := c.session
rootSession.User = security.RootUser

We talked offline. I will keep it as it is because a circular dependency between the session's executor's planner and the original session plus an inability to change the planner's pointer (due to the field not being public) means that it has no effect since when executing a query, a planner's session (in this case c.session) will be used to determine the User and therefore the privileges.

---

Comments from Reviewable

[asubiotto]

Review status: 15 of 29 files reviewed at latest revision, 29 unresolved discussions, some commit checks pending.

---

cli/sql.go, line 503 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

use := - you don't want to overwrite the err from above.

Done.

---

cli/sql_test.go, line 40 at r1 (raw file):

Previously, mjibson (Matt Jibson) wrote…

Sure, another PR for this is fine with me.

Done. #9876

---

cli/sql_util.go, line 191 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

Will file an issue

#9880

---

cli/user.go, line 143 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Yeah, reading the password from stdin seems useful.

Done. I'm allowing empty passwords to be in line with the CREATE USER statement.

---

security/password.go, line 54 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

s/the//

Changed comment.

---

security/password.go, line 55 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

remove the last sentence and just say "returns the md5 hash of the password concatenated with the username".

Removed md5.

---

security/password.go, line 62 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

Sounds good, I think using cleartext and storing on disk w/ bcrypt would be best to achieve our goals and will change to that.

Done.

---

security/password.go, line 77 at r1 (raw file):

Previously, cuongdo (Cuong Do) wrote…

can you skip the line above and just encode h.Sum([]byte(s))?

Removed md5.

---

sql/create.go, line 280 at r1 (raw file):

Previously, mjibson (Matt Jibson) wrote…

no period at end of error message

Done.

---

sql/pgwire_test.go, line 1230 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

put this into the if so that err does not live beyond this block.

Done.

---

sql/pgwire_test.go, line 1232 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

no need for this case; IsError(nil, "foo") == false

Done.

---

sql/pgwire_test.go, line 1238 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

throughout: prefer := and reduce the scope of err by putting it in the if

Done.

---

sql/pgwire/v3.go, line 348 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

throughout: change these = to :=

Done.

---

sql/pgwire/v3.go, line 362 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

use errors.Errorf and include the typ you got?

Done.

---

sql/pgwire/v3.go, line 364 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

return c.handle(...)

Done.

---

sql/pgwire/v3.go, line 382 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Yes, I understand, but it should always be exactly len("md5")+mdg.Size

Removed md5.

---

sql/pgwire/v3.go, line 404 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

include the user in this string

Done.

---

sql/testdata/user, line 1 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Please fix the bug mentioned by @mjibson if you can, as well.

Done.

---

sql/testdata/user, line 4 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

this error should include the user whose creation was attempted

Done.

---

Comments from Reviewable

[knz]

Review status: 15 of 29 files reviewed at latest revision, 29 unresolved discussions, some commit checks pending.

---

sql/logic_test.go, line 472 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

It seems that the underlying implementation doesn't allow for placeholders. I will change this to Exec(fmt.Sprintf("CREATE USER %s", server.TestUser))

Wait what? The underlying implementation should allow placeholders, that's what the Exec() method's signature says. It's being used in plenty of tests. What makes you say it's not supported?

---

Comments from Reviewable

[asubiotto]

Review status: 15 of 29 files reviewed at latest revision, 29 unresolved discussions, some commit checks failed.

---

sql/logic_test.go, line 472 at r1 (raw file):

Previously, knz (kena) wrote…

Wait what? The underlying implementation should allow placeholders, that's what the Exec() method's signature says. It's being used in plenty of tests. What makes you say it's not supported?

I was getting errors from the placeholder not being parsed so I asked Dan and he suggested it was the underlying implementation. This made sense especially after seeing the use of fmt.Sprintf on line 405 (`t.db.Exec(fmt.Sprintf("SET DATABASE = %s", inDBName))`).

---

Comments from Reviewable

[knz]

Review status: 15 of 29 files reviewed at latest revision, 25 unresolved discussions, some commit checks failed.

---

sql/logic_test.go, line 472 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

I was getting errors from the placeholder not being parsed so I asked Dan and he suggested it was the underlying implementation. This made sense especially after seeing the use of fmt.Sprintf on line 405 (t.db.Exec(fmt.Sprintf("SET DATABASE = %s", inDBName))).

Thanks for explaining. Just let my comment sleep for now.

---

Comments from Reviewable

[tamird]

Reviewed 14 of 15 files at r2.
Review status: 28 of 29 files reviewed at latest revision, 23 unresolved discussions, some commit checks failed.

---

security/password.go, line 54 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

Changed comment.

remove "the user"; this prompts on the command line, "the user" is overly specific.

---

security/password.go, line 38 at r2 (raw file):

// of this hash.
func HashPassword(s string) (string, error) {
  hashedPassword, err := bcrypt.GenerateFromPassword([]byte(s), bcryptCost)

does postgres allow non-UTF8 passwords? this commit makes a number of plumbing changes from []byte to string; if postgres allows non-UTF8 passwords, we should revert those changes.

---

sql/pgwire_test.go, line 161 at r2 (raw file):

                      // authentication should fail.
                      if !testutils.IsError(err, fmt.Sprintf("pq: user %s does not exist", server.TestUser)) {
                          t.Error(err)

this should be t.Errorf("unexpected error: %v", err) (in case err is unexpectedly nil)

---

sql/pgwire_test.go, line 1254 at r2 (raw file):

  defer s.Stopper().Stop()

  pgBaseUrl := sqlutils.PGBaseUrl(t, s.ServingAddr(), url.User(""))

nit: instead of making one of these in the outer scope and modifying it in various inner scopes, why not create one in each inner scope? seems cheap enough.

---

sql/pgwire_test.go, line 1262 at r2 (raw file):

      testUserPgUrl.User = url.User(server.TestUser)
      if err := trivialQuery(testUserPgUrl); !testutils.IsError(err, fmt.Sprintf("pq: user %s does not exist", server.TestUser)) {
          t.Fatal(err)

should be t.Fatalf("unexpected error: %v", err)

---

sql/pgwire_test.go, line 1299 at r2 (raw file):

          unicodeUserPgUrl.User = url.User(unicodeUser)
          if err := trivialQuery(unicodeUserPgUrl); !testutils.IsError(err, "pq: invalid password") {
              t.Fatal(err)

should be t.Fatalf("unexpected error: %v", err)

---

sql/pgwire_test.go, line 1316 at r2 (raw file):

      testUserPgUrl.User = url.User(server.TestUser)
      if err := trivialQuery(testUserPgUrl); !testutils.IsError(err, "pq: invalid password") {
          t.Fatal(err)

should be t.Fatalf("unexpected error: %v", err)

---

sql/parser/stmt.go, line 120 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

Will change to Ack

What does postgres do?

---

sql/pgwire/v3.go, line 393 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

We talked offline. I will keep it as it is because a circular dependency between the session's executor's planner and the original session plus an inability to change the planner's pointer (due to the field not being public) means that it has no effect since when executing a query, a planner's session (in this case c.session) will be used to determine the User and therefore the privileges.

Please add this explanation as a comment.

---

sql/pgwire/v3.go, line 353 at r2 (raw file):

  if typ != clientMsgPassword {
      return errors.Errorf("invalid response to authentication request %s", typ)

"request: %s"

---

sql/pgwire/v3.go, line 375 at r2 (raw file):

  c.session.User = security.RootUser
  r := c.executor.ExecuteStatements(
      c.session, "SELECT hashedPassword FROM system.users WHERE username=$1;", placeholderInfo,

nit: the trailing semicolon is not needed

---

sql/pgwire/v3.go, line 381 at r2 (raw file):

  if len(r.ResultList) != 1 || r.ResultList[0].Err != nil {
      fmt.Printf("Error: %v\n", r.ResultList[0].Err)

oops

---

sql/testdata/user, line 1 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

Done.

But the bug isn't fixed, is it?

---

testutils/sqlutils/pg_url.go, line 85 at r2 (raw file):

// PGBaseUrl returns a postgres connection url which connects to this server
// with the given user and SSL disabled.
func PGBaseUrl(t testing.TB, servingAddr string, user *url.Userinfo) url.URL {

Perhaps "Insecure" should be in the name. It's not really fair to call this "Base" since it includes sslmode=disable.

---

Comments from Reviewable

[maddyblue]

Review status: 28 of 29 files reviewed at latest revision, 23 unresolved discussions, some commit checks failed.

---

sql/parser/create.go, line 607 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

I'll change this to print "*****" on non-empty passwords. Thanks for catching this Matt!

OH!! What if we made passwords ONLY specifiable via placeholders?

---

Comments from Reviewable

[maddyblue]

Review status: 28 of 29 files reviewed at latest revision, 23 unresolved discussions, some commit checks failed.

---

sql/parser/stmt.go, line 120 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

What does postgres do?

postgres returns a command complete message with the "CREATE ROLE" tag. DDL and Ack both return a command complete, so it's unclear which one postgres thinks it is. I'm not aware of anything that cares about the difference between DDL and Ack.

---

Comments from Reviewable

[knz]

1. This implementation is currently silent about case sensitivity on the username. It appears that usernames are handled case-sensitively. Since usernames are treated as identifiers in the SQL statements I believe they should be matched case-insensitive, modulo the comments made in sql: Identifiers should always be case-folded unless double quoted #8862. Incidentally that's what PG does too. But regardless of what gets implemented you must absolutely document what you support w.r.t casing at least in the commit message and probably also in the various user/password accessor methods.
2. The commit message must also summarize which combinations of certificates / users / passwords are supported, to help the person who will document this. Thanks

---

Reviewed 14 of 61 files at r3, 12 of 19 files at r4, 9 of 9 files at r5.
Review status: all files reviewed at latest revision, 25 unresolved discussions, all commit checks successful.

---

pkg/base/config.go, line 154 at r5 (raw file):

      options.Add("sslmode", "disable")
  } else {
      if cfg.SSLCA == "" {

I'm not sure this is the right location for these checks. All our other flag consistency checks are in package cli, why did you choose to put these here?

---

pkg/base/config.go, line 158 at r5 (raw file):

      }

      // Check that cfg.SSLCert and cfg.SSLCertKey are either both empty or

We need a longer explanation, either here as a comment or in the commit message, of the rationale for accepting a missing certificate. I know you know but this is not written anywhere. Please explain in writing.

---

pkg/cli/sql_util.go, line 191 at r5 (raw file):

  }
  var user *url.Userinfo
  if !baseCfg.Insecure && connUser != security.RootUser && baseCfg.SSLCert == "" && baseCfg.SSLCertKey == "" {

What would be the proper way for a user to request mpassword authentication on top of certificate validation? (Like 2FA)?

---

pkg/cli/user.go, line 154 at r5 (raw file):

      }

      panic("empty passwords are not permitted")

I think we still want an error to protect against empty passwords, unless an explicit flag (--allow-empty-passwd) is also given.
This is because if the set user command is used in a script and the script contains an error, we want an error instead of silently opening the database wide up.

---

pkg/security/auth.go, line 131 at r3 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

Added a comment on the exported UserAuthHook type to explain this more clearly.

I think "only available for client connections" sounds better than "public".

---

pkg/security/password.go, line 52 at r5 (raw file):

  // a decent solution, although it does not work on windows.
  fmt.Print("Enter password: ")
  one, err := terminal.ReadPassword(0)

Try ReadPassword from the readline library we're already using in cli. This does the right thing on Windows too I think.

---

pkg/sql/create.go, line 238 at r5 (raw file):

// CreateUser creates a user.
// Privileges: security.RootUser user.
//   notes: postgres allows the creation of a user without a password.

What purpose does this note serve? Just stating "Postgres does X and CockroachDB does Y" tells something obvious (trivially obtained via testing) and raises the question of why. If you think this is important make a comment with either "We do not support creating a user without a password because ..." or "TODO(zzz): We do not yet support creating a user without a password." (IMHO, no need to refer to pg at all here)

---

pkg/sql/create.go, line 254 at r5 (raw file):

  }

  if p.session.User != security.RootUser {

We should define a new permission to create users rather than compare a username. Privilege separation is a thing, one may want to attribute the rights to create new users to someone who cannot drop databases or tables at will. Has this been discussed already? If you do not envision a straightforward solution to this, put a TODO here with a reference to a security issue (which you'd also file) requesting a discussion about this and a fix.

---

pkg/sql/create.go, line 272 at r5 (raw file):

  internalExecutor := InternalExecutor{LeaseManager: n.p.leaseMgr}
  rowsAffected, err := internalExecutor.ExecuteStatementInTransaction(

I'm sorry I'm late to the party, but can you explain your rationale for not using a ParseOne/newPlan solution here of the same kind we discussed for SHOW USERS? (Namely, reusing the same KV txn?) Or at least executor.ExecuteStatements on the same session/txn like you do already for GetUserHashedPassword()?

@bdarnell I am wary here that the internal executor is using a completely distinct KV txn object than the current one. As far as I understand, if (say) an admin creates a user then tries to grant permissions to that user, both operations inside the same SQL txn not yet committed, it may well be that the grant statement won't see the user created by the (separate) kv txn because the SQL txn where the create took place has an earlier timestamp. Correct?

---

pkg/sql/create.go, line 280 at r5 (raw file):

  )
  if err != nil {
      if _, ok := err.(*sqlbase.ErrUniquenessConstraintViolation); ok {

FYI there's a more elegant solution than checking the error: run INSERT ... ON CONFLICT DO NOTHING and check the number of rows affected by the statement. This would be incidentally much better if you are using the sub-plan solution.

---

pkg/sql/testdata/user, line 5 at r5 (raw file):

statement error user user1 already exists
CREATE USER user1

So the thing with SQL is that unquoted identifiers are case-insensitive.
I'm not sure your implementation supports this.
I think it should; but regardless of whether you choose to support case-insensitive usernames or not, you must absolutely make a test that demonstrates what you do w.r.t. username case sensitivity.

For reference with pg:

kena=# create user foo with password 'bla';
CREATE ROLE
kena=# create user Foo with password 'bla';
ERROR:  role "foo" already exists

---

Comments from Reviewable

[asubiotto]

I will add documentation to the commit message. Thanks for pointing this out.

---

Review status: all files reviewed at latest revision, 25 unresolved discussions, all commit checks successful.

---

pkg/base/config.go, line 154 at r5 (raw file):

Previously, knz (kena) wrote…

I'm not sure this is the right location for these checks. All our other flag consistency checks are in package cli, why did you choose to put these here?

Flag checks already existed here so I just modified them. I also think that it makes more sense to check that these flags are set properly at the moment of use

---

pkg/base/config.go, line 158 at r5 (raw file):

Previously, knz (kena) wrote…

We need a longer explanation, either here as a comment or in the commit message, of the rationale for accepting a missing certificate. I know you know but this is not written anywhere. Please explain in writing.

I will add it to the commit message.

---

pkg/cli/sql_util.go, line 191 at r5 (raw file):

Previously, knz (kena) wrote…

What would be the proper way for a user to request mpassword authentication on top of certificate validation? (Like 2FA)?

The quick answer to your question is that there is no proper way for a user to request 2FA.

Password authentication is treated as a fallback for users that do not want certificate authentication but do want some sort of authentication. I think 2FA is not something that we would support unless there seems to be a need for this.

---

pkg/cli/user.go, line 154 at r5 (raw file):

Previously, knz (kena) wrote…

I think we still want an error to protect against empty passwords, unless an explicit flag (--allow-empty-passwd) is also given.
This is because if the set user command is used in a script and the script contains an error, we want an error instead of silently opening the database wide up.

The behavior of `CREATE USER` allows users to be created with empty passwords. I'm not sure I understand what errors we would be protecting a user from but my opinion is that this set user command should be as close as possible to `CREATE USER` and (in the future) `ALTER USER`.

---

pkg/security/password.go, line 52 at r5 (raw file):

Previously, knz (kena) wrote…

Try ReadPassword from the readline library we're already using in cli. This does the right thing on Windows too I think.

Thanks for the suggestion, will try it out.

---

pkg/sql/create.go, line 238 at r5 (raw file):

Previously, knz (kena) wrote…

What purpose does this note serve? Just stating "Postgres does X and CockroachDB does Y" tells something obvious (trivially obtained via testing) and raises the question of why. If you think this is important make a comment with either "We do not support creating a user without a password because ..." or "TODO(zzz): We do not yet support creating a user without a password." (IMHO, no need to refer to pg at all here)

The note is there to explain why we allow the creation of a user without a password (because postgres does too). I will update the comment to make it more understandable.

---

pkg/sql/create.go, line 254 at r5 (raw file):

Previously, knz (kena) wrote…

We should define a new permission to create users rather than compare a username. Privilege separation is a thing, one may want to attribute the rights to create new users to someone who cannot drop databases or tables at will. Has this been discussed already? If you do not envision a straightforward solution to this, put a TODO here with a reference to a security issue (which you'd also file) requesting a discussion about this and a fix.

I will remove this and defer the privilege check to the `INSERT INTO` statement below so that anybody with the INSERT privilege on system.users will be able to create users. This makes more sense to me because somebody could still create a user by running a direct `INSERT INTO` statement.

---

pkg/sql/create.go, line 272 at r5 (raw file):

Previously, knz (kena) wrote…

I'm sorry I'm late to the party, but can you explain your rationale for not using a ParseOne/newPlan solution here of the same kind we discussed for SHOW USERS? (Namely, reusing the same KV txn?) Or at least executor.ExecuteStatements on the same session/txn like you do already for GetUserHashedPassword()?

@bdarnell I am wary here that the internal executor is using a completely distinct KV txn object than the current one. As far as I understand, if (say) an admin creates a user then tries to grant permissions to that user, both operations inside the same SQL txn not yet committed, it may well be that the grant statement won't see the user created by the (separate) kv txn because the SQL txn where the create took place has an earlier timestamp. Correct?

I'm all for using the same logic we discussed in SHOW USERS. You and I discussed what I should be using here offline a few weeks ago (including ParseOne/newPlan) and settled on InternalExecutor per andrei's suggestion. Will change this to ParseOne/newPlan.

---

pkg/sql/testdata/user, line 5 at r5 (raw file):

Previously, knz (kena) wrote…

So the thing with SQL is that unquoted identifiers are case-insensitive.
I'm not sure your implementation supports this.
I think it should; but regardless of whether you choose to support case-insensitive usernames or not, you must absolutely make a test that demonstrates what you do w.r.t. username case sensitivity.

For reference with pg:

kena=# create user foo with password 'bla';
CREATE ROLE
kena=# create user Foo with password 'bla';
ERROR:  role "foo" already exists

I will add this test and put it in writing in the commit message. We are case-sensitive wrt usernames and this makes more sense to me when it comes to usernames but can be convinced otherwise.

---

Comments from Reviewable

[bdarnell]

Reviewed 12 of 19 files at r4, 9 of 9 files at r5.
Review status: all files reviewed at latest revision, 25 unresolved discussions, all commit checks successful.

---

pkg/cli/sql_util.go, line 191 at r5 (raw file):

Previously, knz (kena) wrote…

What would be the proper way for a user to request mpassword authentication on top of certificate validation? (Like 2FA)?

Put a passphrase on the key? (although support for this in clients is admittedly poor)

Database passwords as commonly deployed don't really constitute a useful second factor; cert alone is just as good. I think we can defer this unless and until we support a proper 2FA solution (TOTP or similar).

---

pkg/cli/user.go, line 154 at r5 (raw file):

Previously, knz (kena) wrote…

I think we still want an error to protect against empty passwords, unless an explicit flag (--allow-empty-passwd) is also given.
This is because if the set user command is used in a script and the script contains an error, we want an error instead of silently opening the database wide up.

Please don't add another flag for this (would it apply on the client or server? what if someone uses the CREATE USER command from a different client?); we should choose a password policy and stick with it. I would prefer to disallow empty passwords when using password-based auth; the fact that postgres allows empty passwords is irrelevant.

---

pkg/sql/create.go, line 238 at r5 (raw file):

Previously, knz (kena) wrote…

What purpose does this note serve? Just stating "Postgres does X and CockroachDB does Y" tells something obvious (trivially obtained via testing) and raises the question of why. If you think this is important make a comment with either "We do not support creating a user without a password because ..." or "TODO(zzz): We do not yet support creating a user without a password." (IMHO, no need to refer to pg at all here)

We've done this for permission-related comments on all commands.

---

pkg/sql/create.go, line 254 at r5 (raw file):

Previously, knz (kena) wrote…

We should define a new permission to create users rather than compare a username. Privilege separation is a thing, one may want to attribute the rights to create new users to someone who cannot drop databases or tables at will. Has this been discussed already? If you do not envision a straightforward solution to this, put a TODO here with a reference to a security issue (which you'd also file) requesting a discussion about this and a fix.

I agree we'll need it, but I think it's fine to introduce this new permission later.

---

pkg/sql/create.go, line 272 at r5 (raw file):

Previously, knz (kena) wrote…

I'm sorry I'm late to the party, but can you explain your rationale for not using a ParseOne/newPlan solution here of the same kind we discussed for SHOW USERS? (Namely, reusing the same KV txn?) Or at least executor.ExecuteStatements on the same session/txn like you do already for GetUserHashedPassword()?

@bdarnell I am wary here that the internal executor is using a completely distinct KV txn object than the current one. As far as I understand, if (say) an admin creates a user then tries to grant permissions to that user, both operations inside the same SQL txn not yet committed, it may well be that the grant statement won't see the user created by the (separate) kv txn because the SQL txn where the create took place has an earlier timestamp. Correct?

Yeah, any commands which create a new transaction rather than using the existing one should be disallowed when there is an existing transaction. I would prefer for this to be done within the context of the current transaction; I don't know the best way to handle the privilege escalation here.

---

Comments from Reviewable

[knz]

Review status: all files reviewed at latest revision, 24 unresolved discussions, all commit checks successful.

---

pkg/base/config.go, line 154 at r5 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

Flag checks already existed here so I just modified them. I also think that it makes more sense to check that these flags are set properly at the moment of use

I'd still add a comment in the common location in cli to indicate that these flags get extra checking here.

---

pkg/cli/sql_util.go, line 191 at r5 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Put a passphrase on the key? (although support for this in clients is admittedly poor)

Database passwords as commonly deployed don't really constitute a useful second factor; cert alone is just as good. I think we can defer this unless and until we support a proper 2FA solution (TOTP or similar).

Ok thanks for explaining.

---

pkg/cli/user.go, line 154 at r5 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Please don't add another flag for this (would it apply on the client or server? what if someone uses the CREATE USER command from a different client?); we should choose a password policy and stick with it. I would prefer to disallow empty passwords when using password-based auth; the fact that postgres allows empty passwords is irrelevant.

Sorry only now it's clear to me that we do allow creating a user without a password. I somewhat question the wisdom of doing so (do we allow these users to log in without a valid client certificate?) but now the code is clear to me at least :)

---

pkg/sql/create.go, line 238 at r5 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

We've done this for permission-related comments on all commands.

Ok now it is clear to me too. Thanks for explaining.

---

pkg/sql/create.go, line 272 at r5 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Yeah, any commands which create a new transaction rather than using the existing one should be disallowed when there is an existing transaction. I would prefer for this to be done within the context of the current transaction; I don't know the best way to handle the privilege escalation here.

Ok I just checked and this happens to work. The internal executor takes the txn object to use as argument and the call here takes care to reuse the right txn object. So this code was doing the right thing already. Sorry for the noise. Although using `planner.exec()` directly doesn't hurt either.

---

pkg/sql/testdata/user, line 5 at r5 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

I will add this test and put it in writing in the commit message. We are case-sensitive wrt usernames and this makes more sense to me when it comes to usernames but can be convinced otherwise.

I am ok either way. @bdarnell do you have an opinion on this? If we keep usernames case-sensitive we need to highlight in docs this is relatively unusual. Be sure to highlight it in the commit message.

---

sql/logic_test.go, line 472 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

After this PR is in I'm going to work on allowing placeholders.

Ok FYI to allow placeholders here you do this as follows: 1. change the grammar to accept an expression 2. use analyzeExpr / Eval in the create node to get the value, like e.g. what LIMIT does

---

Comments from Reviewable

[asubiotto]

Review status: all files reviewed at latest revision, 24 unresolved discussions, all commit checks successful.

---

pkg/cli/user.go, line 154 at r5 (raw file):

Previously, knz (kena) wrote…

Sorry only now it's clear to me that we do allow creating a user without a password. I somewhat question the wisdom of doing so (do we allow these users to log in without a valid client certificate?) but now the code is clear to me at least :)

If users do not provide a certificate, authentication will default to password authentication. If the user has been created without a password, an empty password will result in a successful authentication. I think it's important to allow the creation of users without passwords (I can imagine a use case where cert authentication is always used and password creation is a burden on cockroach's ease of use). However, I don't think we should allow password authentication for users with empty passwords. I will add this change in, does that sound good?

---

pkg/sql/create.go, line 254 at r5 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

I agree we'll need it, but I think it's fine to introduce this new permission later.

I don't understand why we need a new permission. I think deferring to an INSERT privilege check system.users would work fine.

---

sql/logic_test.go, line 472 at r1 (raw file):

Previously, knz (kena) wrote…

Ok FYI to allow placeholders here you do this as follows:

1. change the grammar to accept an expression
2. use analyzeExpr / Eval in the create node to get the value, like e.g. what LIMIT does

Thanks :)

---

Comments from Reviewable

[tamird]

Reviewed 9 of 9 files at r5.
Review status: all files reviewed at latest revision, 24 unresolved discussions, all commit checks successful.

---

pkg/security/auth.go, line 131 at r3 (raw file):

Previously, knz (kena) wrote…

I think "only available for client connections" sounds better than "public".

+1, we should really just not use the word "public" anywhere.

---

Comments from Reviewable

[bdarnell]

Review status: all files reviewed at latest revision, 24 unresolved discussions, some commit checks failed.

---

pkg/sql/testdata/user, line 5 at r5 (raw file):

Previously, knz (kena) wrote…

I am ok either way. @bdarnell do you have an opinion on this?
If we keep usernames case-sensitive we need to highlight in docs this is relatively unusual. Be sure to highlight it in the commit message.

I'm surprised that postgres is case-insensitive; I would have assumed case-sensitivity here (by analogy to unix usernames). I don't feel strongly either way, though.

---

Comments from Reviewable

[asubiotto]

Review status: all files reviewed at latest revision, 24 unresolved discussions, some commit checks failed.

---

pkg/base/config.go, line 154 at r5 (raw file):

Previously, knz (kena) wrote…

I'd still add a comment in the common location in cli to indicate that these flags get extra checking here.

I think that having a comment on the functionality of another package confuses more than it helps. For what it's worth, we're checking the config. Additionally, there are other methods in this file that perform similar checks (e.g. `GetServerTLSConfig`)

---

pkg/security/auth.go, line 131 at r3 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

+1, we should really just not use the word "public" anywhere.

Changed variables to clientConnection

---

pkg/security/password.go, line 52 at r5 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

Thanks for the suggestion, will try it out.

I think I'm going to stick to ReadPassword, it seems the most frequently used way to ask a user for their password. The issue with windows is actually a mistake on our part: https://github.com/golang/go/issues/11914. I have changed the fd to `syscall.Stdin` so we should be fine.

---

pkg/sql/create.go, line 254 at r5 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

I don't understand why we need a new permission. I think deferring to an INSERT privilege check system.users would work fine.

I added explicit checking for INSERT privilege on system.users rather than checking that the user is root.

---

pkg/sql/create.go, line 272 at r5 (raw file):

Previously, knz (kena) wrote…

Ok I just checked and this happens to work. The internal executor takes the txn object to use as argument and the call here takes care to reuse the right txn object. So this code was doing the right thing already. Sorry for the noise. Although using planner.exec() directly doesn't hurt either.

I have to make a new planner anyway so I decided to stick with ExecuteStatementInTransaction

---

pkg/sql/create.go, line 280 at r5 (raw file):

Previously, knz (kena) wrote…

FYI there's a more elegant solution than checking the error: run INSERT ... ON CONFLICT DO NOTHING and check the number of rows affected by the statement. This would be incidentally much better if you are using the sub-plan solution.

There seems to be some weird behavior when using this statement in that I get 1 row affected on conflict. Will look into this more and change this code when I find the issue. For now I don't think this should be a blocker.

---

pkg/sql/testdata/user, line 5 at r5 (raw file):

Previously, knz (kena) wrote…

I am ok either way. @bdarnell do you have an opinion on this?
If we keep usernames case-sensitive we need to highlight in docs this is relatively unusual. Be sure to highlight it in the commit message.

Since it is highly unusual, I have changed our behavior to be case-insensitive wrt usernames. Have added this to the commit message.

---

Comments from Reviewable

[tamird]

Reviewed 1 of 61 files at r3, 14 of 14 files at r6.
Review status: all files reviewed at latest revision, 26 unresolved discussions, all commit checks successful.

---

pkg/sql/create.go, line 239 at r6 (raw file):

// Privileges: INSERT on system.users.
//   notes: postgres allows the creation of users with an empty password. We do
//          as well, but disallow password authentication for these users.

indentation is off?

---

pkg/sql/pgwire/server.go, line 265 at r6 (raw file):

              // Normalize the username contained in the certificate.
              tlsState.PeerCertificates[0].Subject.CommonName = sqlbase.NormalizeString(
                  tlsState.PeerCertificates[0].Subject.CommonName)

optional, not even a nit: opinions vary on this, but I like to put the closing paren on the next line (you'll need to add a comma). The symmetry is nicer, in my opinion, but you should make up your own mind since we don't have a consistent style on this.

---

Comments from Reviewable

[tamird]

Reviewed 1 of 19 files at r4.
Review status: all files reviewed at latest revision, 25 unresolved discussions, all commit checks successful.

---

Comments from Reviewable

[knz]

modulo the comment by Tamir.

---

Reviewed 1 of 14 files at r6.
Review status: all files reviewed at latest revision, 23 unresolved discussions, all commit checks successful.

---

pkg/cli/user.go, line 154 at r5 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

If users do not provide a certificate, authentication will default to password authentication. If the user has been created without a password, an empty password will result in a successful authentication. I think it's important to allow the creation of users without passwords (I can imagine a use case where cert authentication is always used and password creation is a burden on cockroach's ease of use). However, I don't think we should allow password authentication for users with empty passwords. I will add this change in, does that sound good?

Yes.

---

pkg/sql/create.go, line 254 at r5 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

I added explicit checking for INSERT privilege on system.users rather than checking that the user is root.

👍

---

pkg/sql/create.go, line 280 at r5 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

There seems to be some weird behavior when using this statement in that I get 1 row affected on conflict. Will look into this more and change this code when I find the issue. For now I don't think this should be a blocker.

Ok but if you're not following on this immediately please file an issue.

---

Comments from Reviewable

[knz]

Could you either extend this PR or follow-up with another soon after to test the command-line flags for authentication. I believe we don't have any test that does this so far. Suggested by @songhao in #10175.

---

Review status: all files reviewed at latest revision, 22 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[asubiotto]

Sounds good, I'll add this test in a separate PR. I'm going to wait either for a second LGTM or the end of the day on Monday to merge.

---

Review status: 24 of 35 files reviewed at latest revision, 23 unresolved discussions, some commit checks pending.

---

pkg/sql/create.go, line 280 at r5 (raw file):

Previously, knz (kena) wrote…

Ok but if you're not following on this immediately please file an issue.

Filed an issue: #10264

---

pkg/sql/create.go, line 239 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

indentation is off?

Done. Could you check again?

---

pkg/sql/pgwire/server.go, line 265 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

optional, not even a nit: opinions vary on this, but I like to put the closing paren on the next line (you'll need to add a comma). The symmetry is nicer, in my opinion, but you should make up your own mind since we don't have a consistent style on this.

I like having the closing paren on the next line too, just changed it.

---

Comments from Reviewable

[tamird]

Reviewed 12 of 12 files at r7.
Review status: all files reviewed at latest revision, 23 unresolved discussions, all commit checks successful.

---

pkg/sql/create.go, line 239 at r6 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

Done. Could you check again?

Still looks off, but I just checked and just about all instances of these "notes" are similarly verkakte.

---

pkg/sql/parser/name_part.go, line 84 at r7 (raw file):

// NormalizeString normalizes to lowercase and Unicode Normalization Form C
// (NFC).
func NormalizeString(s string) string {

bad rebase? callers that need this can use ReNormalizeName below?

---

Comments from Reviewable

[tamird]

Reviewed 3 of 3 files at r8.
Review status: all files reviewed at latest revision, 22 unresolved discussions, some commit checks pending.

---

Comments from Reviewable

[asubiotto]

Review status: all files reviewed at latest revision, 23 unresolved discussions, some commit checks pending.

---

pkg/sql/parser/name_part.go, line 84 at r7 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

bad rebase? callers that need this can use ReNormalizeName below?

Done.

---

Comments from Reviewable

[knz]

---

Reviewed 1 of 14 files at r6, 1 of 12 files at r7.
Review status: all files reviewed at latest revision, 21 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[asubiotto]

@knz Reworked pgwire authentication to take 90b9b38 into account. PTAL.

[knz]

Looking good! Just minor nits left.

---

Reviewed 12 of 12 files at r9.
Review status: all files reviewed at latest revision, 23 unresolved discussions, all commit checks successful.

---

pkg/sql/pgwire/server.go, line 282 at r9 (raw file):

      }

      v3conn.sessionArgs.User = parser.ReNormalizeName(v3conn.sessionArgs.User)

parser.Name(v3conn.sessionArgs.User).Normalize()

ReNormalizeName is only there to renormalize a name previously stored unnormalized in the database.

---

pkg/sql/pgwire/v3.go, line 218 at r9 (raw file):

      // password in case password authentication is needed.
      hashedPassword, err := sql.GetUserHashedPassword(
          ctx, c.executor, &c.metrics.SQLMemMetrics, c.sessionArgs.User,

use the internalMemMetrics for this. It's defined on Server in server/server.go so there's some plumbing needed. Probably via pgwire.Server.

---

Comments from Reviewable

[asubiotto]

Review status: 28 of 36 files reviewed at latest revision, 23 unresolved discussions.

---

pkg/sql/pgwire/server.go, line 282 at r9 (raw file):

Previously, knz (kena) wrote…

parser.Name(v3conn.sessionArgs.User).Normalize()

ReNormalizeName is only there to renormalize a name previously stored unnormalized in the database.

Done. Fixed other instances of this as well.

---

pkg/sql/pgwire/v3.go, line 218 at r9 (raw file):

Previously, knz (kena) wrote…

use the internalMemMetrics for this. It's defined on Server in server/server.go so there's some plumbing needed. Probably via pgwire.Server.

Done.

---

Comments from Reviewable

[knz]

---

Reviewed 8 of 8 files at r10.
Review status: all files reviewed at latest revision, 21 unresolved discussions, some commit checks pending.

---

Comments from Reviewable

[tamird]

Reviewed 8 of 12 files at r9, 8 of 8 files at r10.
Review status: all files reviewed at latest revision, 22 unresolved discussions, all commit checks successful.

---

pkg/sql/pgwire/v3.go, line 247 at r10 (raw file):

      }

      if err := authenticationHook(c.sessionArgs.User, true /* public */); err != nil {

nit: this is no longer called "public"

---

Comments from Reviewable