sql,pgwire,server: factor the password expiry code (scram 6/10)

[knz]

(PR peeled away from #74301; previous PR in series #74845; next PR in series: #74847)
Epic CRDB-5349

To understand this commit, it is perhaps useful to remember that
password credentials, in Postgres's SQL dialect, have their own
expiration timestamp, set via the VALID UNTIL role option. The
timestamp is attached to the password, not the user account, such that
a user can still log in using a separate authentication method even
when their password is expired.

Additionally, it is perhaps useful to understand that throughout the
authentication stack, we distinguish the phase where we determine
whether a principal exists and is allowed to log in (i.e. it has the
LOGIN privilege); and the phase where we load its password credentials
from storage (or from an in-RAM cache). In particular, we are keen to
skip the loading of the password credentials in the case where the
client is authenticating using another method than passwords (for
example TLS client certs, which is still pretty common).

In this context, this commit performs two changes.

The first change is an API cleanup.

Prior to this commit, we were loading the password expiration
timestamp in the first phase (checking that the principal exists),
rather than the second phase (loading the credentials). This was a
misdesign because, as explained above, the expiry timestamp is a
property of the password, and thus needs not be looked at when
passwords are not used.

We fix this in this commit by making the expiration timestamp part of
the second phase, as it should have been from the start.

The second change in this commit is a reduction of security risk.

Prior to this commit, the password expiration timestamp was checked
(compared to the current time) again and again in each of the call
points for the password retrieval function. It was the responsibility
of each caller to perform this check. This constituted a security risk
because a new caller could forget to add the expiry check and bypass
the expiration mechanism entirely.

We fix this by performing the expiry validation in a central location,
in the common function that retrieves the credentials from storage (or
cache). If the password is expired, the function also hides the
password hash by returning the new "password missing" pseudo-hash, so
that the caller cannot any more mistakenly attempt to validate a
password despite the expiration.

Release note: None

[cockroach-teamcity]

This change is 

[rafiss]

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

---

pkg/sql/user.go, line 117 at r1 (raw file):

			})
			if validUntil != nil {
				if validUntil.Time.Sub(timeutil.Now()) < 0 {

i don't believe we need this here -- as per the comment a few lines below, the root user cannot have password expiry.

---

pkg/sql/pgwire/auth_methods.go, line 157 at r1 (raw file):

		if expired {
			c.LogAuthFailed(ctx, eventpb.AuthFailReason_CREDENTIALS_EXPIRED, nil)
			// TODO(knz): Consider omitting the error return here, and let the

what's the reasoning for using a different error? i thought we would not want to reveal the cause for security reasons (which is how other password auth errors are treated)

[knz]

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

---

pkg/sql/user.go, line 117 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i don't believe we need this here -- as per the comment a few lines below, the root user cannot have password expiry.

Nice catch. Fixed.

---

pkg/sql/pgwire/auth_methods.go, line 157 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

what's the reasoning for using a different error? i thought we would not want to reveal the cause for security reasons (which is how other password auth errors are treated)

You're right.
What we really need is two different error paths:

• one error return, that results in a log.Info in the session log, so that an operator can troubleshoot failed logins.
• separately, a "client error" to return to the client, that's the same regardless of the precise failure reason.

I think for the 2nd point it's better to concentrate that client error logic in the .Authenticate call in auth.go. After all, the other code paths above here do return err and are also misbehaving in that sense.
Are you OK with me handling this in a separate PR?

Also IMHO this TODO was bogus, in the simple password case there's already no distinction of cause. So I'm removing it.

[rafiss]

lgtm!

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

---

pkg/sql/pgwire/auth_methods.go, line 157 at r1 (raw file):

Previously, knz (kena) wrote…

You're right.
What we really need is two different error paths:

• one error return, that results in a log.Info in the session log, so that an operator can troubleshoot failed logins.
• separately, a "client error" to return to the client, that's the same regardless of the precise failure reason.

I think for the 2nd point it's better to concentrate that client error logic in the .Authenticate call in auth.go. After all, the other code paths above here do return err and are also misbehaving in that sense.
Are you OK with me handling this in a separate PR?

Also IMHO this TODO was bogus, in the simple password case there's already no distinction of cause. So I'm removing it.

yes a separate PR for the client error handling sounds good

[knz]

Followup issue to do the error handling better: #47595

[knz]

NB: I'm holding off merging this PR until all the PRs in the sequence are ready (otherwise, the entire chain will be auto-closed by github).