Better detail logging for password auth failures

Started by Tom Laneabout 10 years ago7 messages

tgl@sss.pgh.pa.us

about 10 years ago

1 attachment(s)

We often tell people to look in the postmaster log for more information
about authentication problems; but an off-list question prompted me to
notice that many of the common authentication failure cases don't actually
get any useful commentary in the log. The attached proposed patch
remedies this by adding specific log detail messages for all the
non-out-of-memory cases processed by md5_crypt_verify(). Essentially,
this is just covering cases that I omitted to cover in commit 64e43c59,
for reasons that no longer seem very good to me.

I did not bother with going through the other auth methods in similar
detail. It seems like only password authentication is in use by people
who are in need of this kind of help. (But if someone else wants to do
something similar for other auth methods, feel free.)

In passing, the patch gets rid of a vestigial CHECK_FOR_INTERRUPTS()
call; it was added by e710b65c and IMO should have been removed again
by 6647248e. There's certainly no very good reason to have one right
at that spot anymore.

Any objections?

regards, tom lane

Attachments:

more-password-log-info.patchtext/x-diff; charset=us-ascii; name=more-password-log-info.patchDownload

diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
index 97be944..64c25e8 100644
*** a/src/backend/libpq/crypt.c
--- b/src/backend/libpq/crypt.c
*************** md5_crypt_verify(const Port *port, const
*** 50,56 ****
--- 50,60 ----
  	/* Get role info from pg_authid */
  	roleTup = SearchSysCache1(AUTHNAME, PointerGetDatum(role));
  	if (!HeapTupleIsValid(roleTup))
+ 	{
+ 		*logdetail = psprintf(_("Role \"%s\" does not exist."),
+ 							  role);
  		return STATUS_ERROR;	/* no such user */
+ 	}
  
  	datum = SysCacheGetAttr(AUTHNAME, roleTup,
  							Anum_pg_authid_rolpassword, &isnull);
*************** md5_crypt_verify(const Port *port, const
*** 71,83 ****
  	ReleaseSysCache(roleTup);
  
  	if (*shadow_pass == '\0')
  		return STATUS_ERROR;	/* empty password */
! 
! 	CHECK_FOR_INTERRUPTS();
  
  	/*
  	 * Compare with the encrypted or plain password depending on the
! 	 * authentication method being used for this connection.
  	 */
  	switch (port->hba->auth_method)
  	{
--- 75,92 ----
  	ReleaseSysCache(roleTup);
  
  	if (*shadow_pass == '\0')
+ 	{
+ 		*logdetail = psprintf(_("User \"%s\" has an empty password."),
+ 							  role);
  		return STATUS_ERROR;	/* empty password */
! 	}
  
  	/*
  	 * Compare with the encrypted or plain password depending on the
! 	 * authentication method being used for this connection.  (We do not
! 	 * bother setting logdetail for pg_md5_encrypt failure: the only possible
! 	 * error is out-of-memory, which is unlikely, and if it did happen adding
! 	 * a psprintf call would only make things worse.)
  	 */
  	switch (port->hba->auth_method)
  	{
*************** md5_crypt_verify(const Port *port, const
*** 154,159 ****
--- 163,171 ----
  		else
  			retval = STATUS_OK;
  	}
+ 	else
+ 		*logdetail = psprintf(_("Password does not match for user \"%s\"."),
+ 							  role);
  
  	if (port->hba->auth_method == uaMD5)
  		pfree(crypt_pwd);

Michael Paquier

michael.paquier@gmail.com

about 10 years ago

In reply to: Tom Lane (#1)

Re: Better detail logging for password auth failures

On Wed, Dec 30, 2015 at 1:07 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

We often tell people to look in the postmaster log for more information
about authentication problems; but an off-list question prompted me to
notice that many of the common authentication failure cases don't actually
get any useful commentary in the log. The attached proposed patch
remedies this by adding specific log detail messages for all the
non-out-of-memory cases processed by md5_crypt_verify(). Essentially,
this is just covering cases that I omitted to cover in commit 64e43c59,
for reasons that no longer seem very good to me.

Any objections?

Not really. +1. And the patch looks good.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andres Freund

andres@anarazel.de

about 10 years ago

In reply to: Tom Lane (#1)

Re: Better detail logging for password auth failures

On 2015-12-29 11:07:26 -0500, Tom Lane wrote:

In passing, the patch gets rid of a vestigial CHECK_FOR_INTERRUPTS()
call; it was added by e710b65c and IMO should have been removed again
by 6647248e. There's certainly no very good reason to have one right
at that spot anymore.

Why? Doesn't seem like the worst place for an explicit interrupt check?
I think we don't really have a problem with too many such checks... We
surely could move it, but I don't really see how it's related to the
topic at hand nor do I think it's really worth pondering about
extensively.

Andres

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane

tgl@sss.pgh.pa.us

about 10 years ago

In reply to: Andres Freund (#3)

Re: Better detail logging for password auth failures

Andres Freund <andres@anarazel.de> writes:

On 2015-12-29 11:07:26 -0500, Tom Lane wrote:

In passing, the patch gets rid of a vestigial CHECK_FOR_INTERRUPTS()
call; it was added by e710b65c and IMO should have been removed again
by 6647248e. There's certainly no very good reason to have one right
at that spot anymore.

Why? Doesn't seem like the worst place for an explicit interrupt check?

The only reason there was one there at all was that e710b65c added
code like this:

+   /*
+    * Disable immediate interrupts while doing database access.  (Note
+    * we don't bother to turn this back on if we hit one of the failure
+    * conditions, since we can expect we'll just exit right away anyway.)
+    */
+   ImmediateInterruptOK = false;

... some catalog access here ...

+   /* Re-enable immediate response to SIGTERM/SIGINT/timeout interrupts */
+   ImmediateInterruptOK = true;
+   /* And don't forget to detect one that already arrived */
+   CHECK_FOR_INTERRUPTS();

In 6647248e you got rid of nine of these ten lines, leaving something
that's both pointless and undocumented. There are more than enough
CHECK_FOR_INTERRUPTS calls already in the auth code; there's not a
reason to expend code space on one here. (If MD5 ran long enough to
be worth interrupting, there would be an argument for a check inside
its hashing loop, but that still wouldn't be this check.)

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Noah Misch

noah@leadboat.com

about 10 years ago

In reply to: Tom Lane (#4)

Re: Better detail logging for password auth failures

On Wed, Dec 30, 2015 at 10:18:35AM -0500, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2015-12-29 11:07:26 -0500, Tom Lane wrote:

In passing, the patch gets rid of a vestigial CHECK_FOR_INTERRUPTS()
call; it was added by e710b65c and IMO should have been removed again
by 6647248e. There's certainly no very good reason to have one right
at that spot anymore.

Why? Doesn't seem like the worst place for an explicit interrupt check?
I think we don't really have a problem with too many such checks... We
surely could move it, but I don't really see how it's related to the
topic at hand nor do I think it's really worth pondering about
extensively.

Agreed.

The only reason there was one there at all was that e710b65c added
code like this:
+   /*
+    * Disable immediate interrupts while doing database access.  (Note
+    * we don't bother to turn this back on if we hit one of the failure
+    * conditions, since we can expect we'll just exit right away anyway.)
+    */
+   ImmediateInterruptOK = false;
... some catalog access here ...
+   /* Re-enable immediate response to SIGTERM/SIGINT/timeout interrupts */
+   ImmediateInterruptOK = true;
+   /* And don't forget to detect one that already arrived */
+   CHECK_FOR_INTERRUPTS();
In 6647248e you got rid of nine of these ten lines, leaving something
that's both pointless and undocumented. There are more than enough
CHECK_FOR_INTERRUPTS calls already in the auth code; there's not a
reason to expend code space on one here. (If MD5 ran long enough to
be worth interrupting, there would be an argument for a check inside
its hashing loop, but that still wouldn't be this check.)

I see no general benefit from being parsimonious with CHECK_FOR_INTERRUPTS
calls or documenting them. As you explain, it's probably fine to remove the
two calls that commit e710b65 had added. However, the sole connection to
$SUBJECT is one of those two calls sharing a screenful with lines $SUBJECT
changed. The removal, if worthwhile, is worth a freestanding patch.
Squashing the changes makes both topics harder to review.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Edson - Amplosoft Software

edson@openmailbox.org

about 10 years ago

In reply to: Noah Misch (#5)

Re: Better detail logging for password auth failures

Em 31 de dezembro de 2015 04:56:55 BRST, Noah Misch <noah@leadboat.com> escreveu:

On Wed, Dec 30, 2015 at 10:18:35AM -0500, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2015-12-29 11:07:26 -0500, Tom Lane wrote:

In passing, the patch gets rid of a vestigial

CHECK_FOR_INTERRUPTS()

call; it was added by e710b65c and IMO should have been removed

again

by 6647248e. There's certainly no very good reason to have one

right

at that spot anymore.

Why? Doesn't seem like the worst place for an explicit interrupt

check?

I think we don't really have a problem with too many such checks...

We

surely could move it, but I don't really see how it's related to

the

topic at hand nor do I think it's really worth pondering about
extensively.

Agreed.
The only reason there was one there at all was that e710b65c added
code like this:
+   /*
+    * Disable immediate interrupts while doing database access. 
(Note

+ * we don't bother to turn this back on if we hit one of the

failure

+ * conditions, since we can expect we'll just exit right away

anyway.)

+ */
+ ImmediateInterruptOK = false;

... some catalog access here ...

+ /* Re-enable immediate response to SIGTERM/SIGINT/timeout

interrupts */
+   ImmediateInterruptOK = true;
+   /* And don't forget to detect one that already arrived */
+   CHECK_FOR_INTERRUPTS();
In 6647248e you got rid of nine of these ten lines, leaving something
that's both pointless and undocumented. There are more than enough
CHECK_FOR_INTERRUPTS calls already in the auth code; there's not a
reason to expend code space on one here. (If MD5 ran long enough to
be worth interrupting, there would be an argument for a check inside
its hashing loop, but that still wouldn't be this check.)
I see no general benefit from being parsimonious with
CHECK_FOR_INTERRUPTS
calls or documenting them. As you explain, it's probably fine to
remove the
two calls that commit e710b65 had added. However, the sole connection
to
$SUBJECT is one of those two calls sharing a screenful with lines
$SUBJECT
changed. The removal, if worthwhile, is worth a freestanding patch.
Squashing the changes makes both topics harder to review.

nm

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Robert Haas

robertmhaas@gmail.com

about 10 years ago

In reply to: Edson - Amplosoft Software (#6)

Re: Better detail logging for password auth failures

On Thu, Dec 31, 2015 at 11:09 AM, Edson - Amplosoft Software
<edson@openmailbox.org> wrote:

[ quote of an existing message in its entirety ]
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Yes, that was extremely brief.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers