nice tests! just had a comment about hashing

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @ZhouXing19)

pkg/cli/clisqlshell/sql.go, line 1194 at r1 (raw file):

				`ALTER USER %s WITH LOGIN PASSWORD '%s'`,
				userName,
				stripANSIFromString(string(password1)))

if i understand the motivation of #48543 correctly, we want to avoid sending the password in cleartext here. now that #72579 is completed, the CLI can pre-hash the password and then send it in the ALTER USER command.

you can see the compareWithCleartextPassword functions in pkg/security/password.go to see the code for how to hash passwords. (the hashing code will need to be extracted to a separate function so it can be used here)

I think we want this \password command to use SCRAM. I'm not sure if it should be configurable. perhaps @knz has a thought here.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @ZhouXing19)

-- commits, line 9 at r1:
this should be "Release note (cli change)"

also, add more details about what the command actually does. (i.e. just say "the command lets the current user alter their password")

also, if we do pre-hashing as per my other comment, that should be added here

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @ZhouXing19)

pkg/cli/clisqlshell/sql.go, line 1167 at r1 (raw file):

		fmt.Fprintf(c.iCtx.stdout, "Enter new password: \n")

		password1, err := term.ReadPassword(int(c.ins.Stdin().Fd()))

Please use security.PromptForPassword() here instead of rolling out your own logic.

pkg/cli/clisqlshell/sql.go, line 1194 at r1 (raw file):

Agreed with Rafi. As-is, this logic will not improve the situation too much, because customers will still complain the password is visible in SQL logs.

Pre-hashing will be the right way to avoid that.

pkg/cli/clisqlshell/sql.go, line 2132 at r1 (raw file):

}

// stripANSIFromString returns str with all ANSI escape sequences removed.

Why is this logic important? I don't understand why you added this. What problem is this supposed to solve?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz, @rafiss, and @ZhouXing19)

pkg/cli/clisqlshell/sql.go, line 1167 at r1 (raw file):

Done.

pkg/cli/clisqlshell/sql.go, line 1194 at r1 (raw file):

Added pre-hashing. Here I hard-coded the cost for hashing the cleartext with SCRAM. This is not ideal but I can't think of other better ways.

pkg/cli/clisqlshell/sql.go, line 2132 at r1 (raw file):

This is because when I try copying and pasting a password to the cli, ANSIF prefix and suffix are automatically added.
E.g. the password I wanted to pass was

SCRAM-SHA-256$119680:Hw/dhibRaBVbObr6bATlqw==$BTFlXuZT2ZNxA3DxogkGgK0wVlOyNtxGoqDI62hyLUo=:3eZMZdAossPInncABj7/N/jkGtRZEUrz7uGkkNibxxx=

But fmt.Fprintf(c.iCtx.stdout, "password1:%#v\n", password1), I got

password1:“\x1b[200~SCRAM-SHA-256$119680:Hw/dhibRaBVbObr6bATlqw==$BTFlXuZT2ZNxA3DxogkGgK0wVlOyNtxGoqDI62hyLUo=:3eZMZdAossPInncABj7/N/jkGtRZEUrz7uGkkNibxxx=\x1b[201~”

You can see that \x1b[200~ and \x1b[201~ are automatically added.
I think copy and paste a pre-hashed password can be a common usage, and this stripANSIFromString is to deal with this edge case.

Reviewed 1 of 4 files at r2, 3 of 4 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @ZhouXing19)

pkg/cli/clisqlshell/sql.go, line 2132 at r1 (raw file):

You're solving the wrong problem, see https://en.wikipedia.org/wiki/XY_problem

The real problem is that PromptForPassword() runs in a terminal mode with paste delimiters enabled. Which is strange; it should not do this. This is what needs to be fixed.
(We have other uses of PromptForPassword() in other places. They are all equally affected. But we don't process the password string in these other places, why is that so?)

pkg/cli/clisqlshell/sql.go, line 1208 at r4 (raw file):

			isPreHashed, _, _, _, _, err := security.CheckPasswordHashValidity(context.Background(), []byte(password1))
			if err != nil {
				fmt.Fprintf(c.iCtx.stderr, "Error checking password hash validity\n:%v", err)

nit: you can remove the word error. The fact it's an error is implied.

pkg/cli/clisqlshell/sql.go, line 1214 at r4 (raw file):

			if !isPreHashed {
				if c.checkPrehashScramEnabled() {

Why do you need this? Which problem is this aiming to solve?

pkg/cli/clisqlshell/sql.go, line 1217 at r4 (raw file):

					hashedPassword, err = security.HashPasswordUsingSCRAM(context.Background(), scramIterCount, password1)
					if err != nil {
						fmt.Fprintf(c.iCtx.stderr, "error hash password using scram\n:%v", err)

ditto, replace hash by hashing

pkg/cli/clisqlshell/sql.go, line 2175 at r4 (raw file):

// `store_client_pre_hashed_passwords_enabled` is set true.
func (c *cliState) checkPrehashScramEnabled() bool {
	passwrodEncrptMethod, err := c.getSessionVarValue("password_encryption")

passwrod -> password

pkg/security/password.go, line 433 at r4 (raw file):

// HashPasswordUsingSCRAM hashes the cleartext password with given cost with SCRAM.
func HashPasswordUsingSCRAM(ctx context.Context, cost int, cleartext string) ([]byte, error) {

Why introducing a new function instead of exporting the function that's already there?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz, @rafiss, and @ZhouXing19)

pkg/cli/clisqlshell/sql.go, line 1214 at r4 (raw file):

i believe the if !isPrehashed check i believe is needed because the user themselves may type in a pre-hashed password at the CLI prompt

and the c.checkPrehashScramEnabled() check is needed because we should not convert the plaintext into a hash unless the server supports pre-hashed passwords (and SCRAM) in the ALTER USER command.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @ZhouXing19)

pkg/cli/clisqlshell/sql.go, line 1214 at r4 (raw file):

the if !isPrehashed check i believe is needed because the user themselves may type in a pre-hashed password at the CLI prompt

Ok, this is not a realistic use case. Interactive password entry via \password must never be interpreted as pre-hashed. If a client already has a prehashed password, they will use ALTER USER WITH PASSWORD.

the c.checkPrehashScramEnabled() check is needed because we should not convert the plaintext into a hash unless the server supports pre-hashed passwords (and SCRAM) in the ALTER USER command.

Ok that sounds mildly more reasonable, but:

1. that check can be done just once when the session is opened instead of every time \password is invoked. We already store a couple of other server-side parameters.

2. we only need to check whether SCRAM is enabled though. The "recognize pre-hashed passwords" is enabled by default and I think the CLI \password option should always assume it is enabled. If an operator disables it, that is non-standard deployment, and I don't think we care much that \password will "not work" in that case.

lgtm, with some small nits

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz, @rafiss, and @ZhouXing19)

pkg/cli/clisqlshell/sql.go, line 1198 at r11 (raw file):

			return errState
		} else {
			var userName string

nit: this could be var userName security.SQLUsername, which is a bit safer (see below)

pkg/cli/clisqlshell/sql.go, line 1201 at r11 (raw file):

			if len(cmd) > 1 {
				userName = cmd[1]

would need to use userName = security.MakeSQLUsernameFromUserInput(cmd[1], security.UsernameValidation)

pkg/cli/clisqlshell/sql.go, line 1209 at r11 (raw file):

			}

			var hashedPassword = []byte(password1)

nit: i think this should just be initialized as var hashedPassword []byte

pkg/cli/clisqlshell/sql.go, line 1212 at r11 (raw file):

			sv := cluster.MakeClusterSettings().SV
			hashedPassword, err := security.HashPassword(context.Background(), &sv, c.passwordHashMethod, password1)

i may have missed this from the other comment thread, but why don't we need to make sure the server supports pre-hashed passwords? (i.e. expose the server.user_login.store_client_pre_hashed_passwords.enabled setting in some way)

pkg/cli/clisqlshell/sql.go, line 1223 at r11 (raw file):

				`ALTER USER %s WITH LOGIN PASSWORD '%s'`,
				userName,
				string(hashedPassword))

i think to be extra safe, this can be:

			c.concatLines = fmt.Sprintf(
				`ALTER USER %s WITH LOGIN PASSWORD %s`,
				userName.SQLIdentifier(),
				lexbase.EscapeSQLString(string(hashedPassword)),
			)

pkg/security/password.go, line 429 at r11 (raw file):

// using the currently configured method.
func HashPassword(
	ctx context.Context, sv *settings.Values, method HashMethod, password string,

nit: this diff might be in the wrong commit

Reviewed 1 of 4 files at r10, 4 of 4 files at r11, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @ZhouXing19)

pkg/cli/clisqlshell/sql.go, line 210 at r11 (raw file):

	// passwordHashMethod saves the hash method used for password encryption.
	// This is used by the `\password` cli command.
	passwordHashMethod security.HashMethod

I don't think cliState is the right place - that gets reset when the user uses \i (recursive execution). The c.iCtx is the one that's shared.

pkg/cli/clisqlshell/sql.go, line 1172 at r11 (raw file):

	case `\password`:
		if c.passwordHashMethod == security.HashInvalidMethod ||
			c.passwordHashMethod == security.HashMissingPassword {

I don't think it's possible to get HashMissingPassword here ever.

pkg/cli/clisqlshell/sql.go, line 1201 at r11 (raw file):

good catch 💯

pkg/cli/clisqlshell/sql.go, line 1212 at r11 (raw file):

My argument for this is that it's not the responsibility of the CLI to support this setting. I don't think we care to support more functionality here than a user would get if they connected to crdb via psql.

pkg/cli/clisqlshell/sql.go, line 1223 at r11 (raw file):

woah yes good catch 💯

Also, separately, any reason why this code can't use a placeholder for the password instead? i.e. ALTER USER ... WITH LOGIN PASSWORD $1?

pkg/security/password.go, line 157 at r11 (raw file):

func GetHashMethodFromString(s string) (HashMethod, error) {
	switch s {
	case "<invalid>":

please remove the two first cases. They are not valid input.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @rafiss)

pkg/cli/clisqlshell/sql.go, line 210 at r11 (raw file):

Done.

pkg/cli/clisqlshell/sql.go, line 1172 at r11 (raw file):

Removed.

pkg/cli/clisqlshell/sql.go, line 1198 at r11 (raw file):

Done.

pkg/cli/clisqlshell/sql.go, line 1201 at r11 (raw file):

Done.

pkg/cli/clisqlshell/sql.go, line 1209 at r11 (raw file):

Done.

pkg/cli/clisqlshell/sql.go, line 1212 at r11 (raw file):

Also, postgres always takes the input password as clear text and always encrypts it, see here and here.

pkg/cli/clisqlshell/sql.go, line 1223 at r11 (raw file):

Made the changes.
I didn't use a placeholder because only c.concatLines(a string) is passed to clisqlclient.MakeQuery() when the statement is being executed.

	c.exitErr = c.runWithInterruptableCtx(func(ctx context.Context) error {
		return c.sqlExecCtx.RunQueryAndFormatResults(ctx,
			c.conn, c.iCtx.stdout, c.iCtx.stderr,
			clisqlclient.MakeQuery(c.concatLines))
	})

I'm not sure how to achieve this with a placeholder for the password.

pkg/security/password.go, line 157 at r11 (raw file):

Done.

pkg/security/password.go, line 429 at r11 (raw file):

Hm I see it's sitting in security: let HashPassword take a HashMethod as an argument, which is intended and seems correct to me?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz, @rafiss, and @ZhouXing19)

pkg/cli/clisqlshell/sql.go, line 1212 at r11 (raw file):
well the big difference is that PG doesn't have anything like the server.user_login.store_client_pre_hashed_passwords.enabled setting.

so the PG server unconditionally does the following logic:

If the presented password string is already in MD5-encrypted or SCRAM-encrypted format, then it is stored as-is

and that is why the psql CLI can unconditionally pre-hash the password when using \password

However, if server.user_login.store_client_pre_hashed_passwords.enabled is false in CRDB, then the server will receive this pre-hashed password, and then hash it again at this point:

and if that happens, the user who just changed the password will now have no way to know what the password was just changed to.

Reviewed 1 of 4 files at r12, 4 of 4 files at r13, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @ZhouXing19)

pkg/cli/clisqlshell/sql.go, line 1212 at r11 (raw file):

ok so my position is and remains that there's no use case for anyone to use server.user_login.store_client_pre_hashed_passwords.enabled ever. I think the deployments using this will be in the minority (if any exist at all -- we could add telemetry to confirm this)

We only added the cluster setting as a troubleshooting mechanism in case something goes wrong in the logic (i.e. a bug on our side). If we ever find a bug, we can tell the affected customer(s) "you can use this cluster setting as a stop-gap until we fix the bug".

I am not keen adding extra logic in the CLI in the common path for a code path that's only used when troubleshooting a bug which we don't even know we have (and our testing suggests it's not even there).

If this remains a point of contention, I'd nearly tempted to suggest removing that cluster setting altogether -- if there's any bug, a customer could use INSERT INTO system.users manually. We could just make ALTER detect the hashes unconditionally.

pkg/cli/clisqlshell/sql.go, line 1223 at r11 (raw file):

yes, thanks for explaining.

pkg/cli/clisqlshell/sql.go, line 1188 at r13 (raw file):

		}

		fmt.Fprintf(c.iCtx.stdout, "password1:%#v\npassword2:%#v\n", password1, password2)

don't forget to remove this before this merges

pkg/cli/clisqlshell/sql.go, line 1206 at r13 (raw file):

			}
			if err != nil {
				fmt.Fprintf(c.iCtx.stderr, "normalize username: %v\n", err)

i don't think this fprintf is necessary. The error will never happen unless the user is injecting some incompatible and erroneous network tool in-between the CLI and the server.
In fact, I would be in favor of adding a new line instead

err = errors.NewAssertionFailureWithWrappedErrf(err, "server-side username does not pass validation")

lgtm after addressing Raphael's nits

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @ZhouXing19)

pkg/cli/clisqlshell/sql.go, line 1212 at r11 (raw file):

there's no use case for anyone to use server.user_login.store_client_pre_hashed_passwords.enabled ever. I think the deployments using this will be in the minority (if any exist at all -- we could add telemetry to confirm this)

i wasn't aware of this, thanks for the info. OK, I'm fine to not check in the CLI then

However, i'd then argue that server.user_login.store_client_pre_hashed_passwords.enabled should not be a public setting. that can be discussed/addressed later

pkg/security/password.go, line 429 at r11 (raw file):

i must have seen the wrong thing, my bad

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz, @rafiss, and @ZhouXing19)

pkg/cli/clisqlshell/sql.go, line 1212 at r11 (raw file):

#79954

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz, @rafiss, and @ZhouXing19)

pkg/cli/clisqlshell/sql.go, line 1188 at r13 (raw file):

Ah thanks! removed.

pkg/cli/clisqlshell/sql.go, line 1206 at r13 (raw file):

Made the change.

Reviewed 1 of 1 files at r14, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @rafiss and @ZhouXing19)