DETAIL for wrong scram password
When md5 password authentication fails, the server log file has a helpful
detail to say why, usually one of:
DETAIL: Role "none" does not exist.
DETAIL: User "jjanes" has no password assigned.
DETAIL: User "jjanes" has an expired password.
DETAIL: Password does not match for user "jjanes".
But for scram authentication, only the first three of these will be
reported when applicable. If the password is simply incorrect, then you do
get a DETAIL line reporting which line of pg_hba was used, but you don't
get a DETAIL line reporting the reason for the failure. It is pretty
unfriendly to make the admin guess what the absence of a DETAIL is supposed
to mean. And as far as I can tell, this is not intentional.
Note that in one case you do get the "does not match" line. That is if the
user has a scram password assigned and the hba specifies plain-text
'password' as the method. So if the absence of the DETAIL is intentional,
it is not internally consistent.
The attached patch fixes the issue. I don't know if this is the correct
location to be installing the message, maybe verify_client_proof should be
doing it instead. I am also not happy to be testing 'doomed' here, but it
was already checked a few lines up so I didn't want to go to lengths to
avoid doing it here too.
Cheers,
Jeff
Attachments:
scram_password_mismatch.patchapplication/octet-stream; name=scram_password_mismatch.patchDownload
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index b9b6d464a0..4f6af23ae5 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -433,8 +433,14 @@ pg_be_scram_exchange(void *opaq, const char *input, int inputlen,
result = SASL_EXCHANGE_FAILURE;
}
- if (result == SASL_EXCHANGE_FAILURE && state->logdetail && logdetail)
- *logdetail = state->logdetail;
+ if (result == SASL_EXCHANGE_FAILURE && logdetail)
+ {
+ if (state->logdetail)
+ *logdetail = state->logdetail;
+ else if (!state->doomed)
+ *logdetail = psprintf(_("Password does not match for user \"%s\"."),
+ state->port->user_name);
+ }
if (*output)
*outputlen = strlen(*output);
On Sat, 2021-02-27 at 17:02 -0500, Jeff Janes wrote:
Note that in one case you do get the "does not match" line. That is
if the user has a scram password assigned and the hba specifies
plain-text 'password' as the method. So if the absence of the DETAIL
is intentional, it is not internally consistent.
Agreed.
The attached patch fixes the issue. I don't know if this is the
correct location to be installing the message,
maybe verify_client_proof should be doing it instead.
Hmm, I agree that the current location doesn't seem quite right. If
someone adds a new way to exit the SCRAM loop on failure, they'd
probably need to partially unwind this change to avoid printing the
wrong detail message. But in my opinion, verify_client_proof()
shouldn't be concerned with logging details...
What would you think about adding the additional detail right after
verify_client_proof() fails? I.e.
if (!verify_client_proof(state) || state->doomed)
{
/* <add logdetail here, if not doomed or already set> */
result = SASL_EXCHANGE_FAILURE;
break;
}
That way the mismatched-password detail is linked directly to the
mismatched-password logic.
Other notes:
- Did you have any thoughts around adding a regression test, since this
would be an easy thing to break in the future?
- I was wondering about timing attacks against the psprintf() call to
construct the logdetail string, but it looks like the other authn code
paths aren't worried about that.
--Jacob
On Tue, Mar 02, 2021 at 05:48:05PM +0000, Jacob Champion wrote:
What would you think about adding the additional detail right after
verify_client_proof() fails? I.e.
Agreed. Having that once all the code paths have been taken and the
client proof has been verified looks more solid. On top of what's
proposed, would it make sense to have a second logdetail for the case
of a mock authentication? We don't log that yet, so I guess that it
could be useful for audit purposes?
--
Michael
On Thu, 2021-03-25 at 16:41 +0900, Michael Paquier wrote:
On top of what's
proposed, would it make sense to have a second logdetail for the case
of a mock authentication? We don't log that yet, so I guess that it
could be useful for audit purposes?
It looks like the code paths that lead to a doomed authentication
already provide their own, more specific, logdetail (role doesn't
exist, role has no password, role doesn't have a SCRAM secret, etc.).
--Jacob
On Thu, Mar 25, 2021 at 03:54:10PM +0000, Jacob Champion wrote:
It looks like the code paths that lead to a doomed authentication
already provide their own, more specific, logdetail (role doesn't
exist, role has no password, role doesn't have a SCRAM secret, etc.).
Yes, you are right here. I missed the parts before
mock_scram_secret() gets called and there are comments in the whole
area. Hmm, at the end of the day, I think that would just have
verify_client_proof() fill in logdetail when the client proof does not
match, and use a wording different than what's proposed upthread to
outline that this is a client proof mismatch.
--
Michael
On Fri, Mar 26, 2021 at 09:49:00AM +0900, Michael Paquier wrote:
Yes, you are right here. I missed the parts before
mock_scram_secret() gets called and there are comments in the whole
area. Hmm, at the end of the day, I think that would just have
verify_client_proof() fill in logdetail when the client proof does not
match, and use a wording different than what's proposed upthread to
outline that this is a client proof mismatch.
Seeing no updates, this has been marked as RwF.
--
Michael