I wrote:
> I agree with doing *something*, but this particular thing seems to violate
> our very long-standing policy on how to deal with authentication failures,
> as well as being too vague to be really useful.
> What would be well within that policy is to log additional information
> into the postmaster log. I see that md5_crypt_verify knows perfectly
> well whether the problem is no password set, wrong password, or expired
> password. I don't see anything wrong with having it emit a log entry
> --- maybe not in the second case for fear of log-spam complaints, but
> certainly the third case and maybe the first one. Or possibly cleaner,
> have it return additional status so that auth_failed() can include the
> info in the main ereport using errdetail_log().
Attached is a proposed patch that does exactly that. This just addresses
the cases mentioned above; once the infrastructure is in place, there
might be more things that would be worth logging using this mechanism.
regards, tom lane
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 882dc8f..1974b57 100644
*** a/src/backend/libpq/auth.c
--- b/src/backend/libpq/auth.c
***************
*** 38,46 ****
*----------------------------------------------------------------
*/
static void sendAuthRequest(Port *port, AuthRequest areq);
! static void auth_failed(Port *port, int status);
static char *recv_password_packet(Port *port);
! static int recv_and_check_password_packet(Port *port);
/*----------------------------------------------------------------
--- 38,46 ----
*----------------------------------------------------------------
*/
static void sendAuthRequest(Port *port, AuthRequest areq);
! static void auth_failed(Port *port, int status, char *logdetail);
static char *recv_password_packet(Port *port);
! static int recv_and_check_password_packet(Port *port, char **logdetail);
/*----------------------------------------------------------------
*************** ClientAuthentication_hook_type ClientAut
*** 207,216 ****
* in use, and these are items that must be presumed known to an attacker
* anyway.
* Note that many sorts of failure report additional information in the
! * postmaster log, which we hope is only readable by good guys.
*/
static void
! auth_failed(Port *port, int status)
{
const char *errstr;
int errcode_return = ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION;
--- 207,217 ----
* in use, and these are items that must be presumed known to an attacker
* anyway.
* Note that many sorts of failure report additional information in the
! * postmaster log, which we hope is only readable by good guys. In
! * particular, if logdetail isn't NULL, we send that string to the log.
*/
static void
! auth_failed(Port *port, int status, char *logdetail)
{
const char *errstr;
int errcode_return = ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION;
*************** auth_failed(Port *port, int status)
*** 273,286 ****
}
if (port->hba)
! ereport(FATAL,
! (errcode(errcode_return),
! errmsg(errstr, port->user_name),
! errdetail_log("Connection matched pg_hba.conf line %d: \"%s\"", port->hba->linenumber, port->hba->rawline)));
! else
! ereport(FATAL,
! (errcode(errcode_return),
! errmsg(errstr, port->user_name)));
/* doesn't return */
}
--- 274,294 ----
}
if (port->hba)
! {
! char *cdetail;
!
! cdetail = psprintf(_("Connection matched pg_hba.conf line %d: \"%s\""),
! port->hba->linenumber, port->hba->rawline);
! if (logdetail)
! logdetail = psprintf("%s\n%s", logdetail, cdetail);
! else
! logdetail = cdetail;
! }
!
! ereport(FATAL,
! (errcode(errcode_return),
! errmsg(errstr, port->user_name),
! logdetail ? errdetail_log("%s", logdetail) : 0));
/* doesn't return */
}
*************** void
*** 294,299 ****
--- 302,308 ----
ClientAuthentication(Port *port)
{
int status = STATUS_ERROR;
+ char *logdetail = NULL;
/*
* Get the authentication method to use for this frontend/database
*************** ClientAuthentication(Port *port)
*** 507,518 ****
(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
errmsg("MD5 authentication is not supported when \"db_user_namespace\" is enabled")));
sendAuthRequest(port, AUTH_REQ_MD5);
! status = recv_and_check_password_packet(port);
break;
case uaPassword:
sendAuthRequest(port, AUTH_REQ_PASSWORD);
! status = recv_and_check_password_packet(port);
break;
case uaPAM:
--- 516,527 ----
(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
errmsg("MD5 authentication is not supported when \"db_user_namespace\" is enabled")));
sendAuthRequest(port, AUTH_REQ_MD5);
! status = recv_and_check_password_packet(port, &logdetail);
break;
case uaPassword:
sendAuthRequest(port, AUTH_REQ_PASSWORD);
! status = recv_and_check_password_packet(port, &logdetail);
break;
case uaPAM:
*************** ClientAuthentication(Port *port)
*** 552,558 ****
if (status == STATUS_OK)
sendAuthRequest(port, AUTH_REQ_OK);
else
! auth_failed(port, status);
/* Done with authentication, so we should turn off immediate interrupts */
ImmediateInterruptOK = false;
--- 561,567 ----
if (status == STATUS_OK)
sendAuthRequest(port, AUTH_REQ_OK);
else
! auth_failed(port, status, logdetail);
/* Done with authentication, so we should turn off immediate interrupts */
ImmediateInterruptOK = false;
*************** recv_password_packet(Port *port)
*** 680,688 ****
/*
* Called when we have sent an authorization request for a password.
* Get the response and check it.
*/
static int
! recv_and_check_password_packet(Port *port)
{
char *passwd;
int result;
--- 689,698 ----
/*
* Called when we have sent an authorization request for a password.
* Get the response and check it.
+ * On error, optionally store a detail string at *logdetail.
*/
static int
! recv_and_check_password_packet(Port *port, char **logdetail)
{
char *passwd;
int result;
*************** recv_and_check_password_packet(Port *por
*** 692,698 ****
if (passwd == NULL)
return STATUS_EOF; /* client wouldn't send password */
! result = md5_crypt_verify(port, port->user_name, passwd);
pfree(passwd);
--- 702,708 ----
if (passwd == NULL)
return STATUS_EOF; /* client wouldn't send password */
! result = md5_crypt_verify(port, port->user_name, passwd, logdetail);
pfree(passwd);
diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
index 56b3ea8..5451db6 100644
*** a/src/backend/libpq/crypt.c
--- b/src/backend/libpq/crypt.c
***************
*** 29,36 ****
#include "utils/timestamp.h"
int
! md5_crypt_verify(const Port *port, const char *role, char *client_pass)
{
int retval = STATUS_ERROR;
char *shadow_pass,
--- 29,42 ----
#include "utils/timestamp.h"
+ /*
+ * Check given password for given user, and return STATUS_OK or STATUS_ERROR.
+ * In the error case, optionally store a palloc'd string at *logdetail
+ * that will be sent to the postmaster log (but not the client).
+ */
int
! md5_crypt_verify(const Port *port, const char *role, char *client_pass,
! char **logdetail)
{
int retval = STATUS_ERROR;
char *shadow_pass,
*************** md5_crypt_verify(const Port *port, const
*** 58,63 ****
--- 64,71 ----
if (isnull)
{
ReleaseSysCache(roleTup);
+ *logdetail = psprintf(_("User \"%s\" has no password assigned."),
+ role);
return STATUS_ERROR; /* user has no password */
}
shadow_pass = TextDatumGetCString(datum);
*************** md5_crypt_verify(const Port *port, const
*** 148,154 ****
--- 156,166 ----
if (isnull)
retval = STATUS_OK;
else if (vuntil < GetCurrentTimestamp())
+ {
+ *logdetail = psprintf(_("User \"%s\" has an expired password."),
+ role);
retval = STATUS_ERROR;
+ }
else
retval = STATUS_OK;
}
diff --git a/src/include/libpq/crypt.h b/src/include/libpq/crypt.h
index 818e57e..b91024f 100644
*** a/src/include/libpq/crypt.h
--- b/src/include/libpq/crypt.h
***************
*** 15,21 ****
#include "libpq/libpq-be.h"
! extern int md5_crypt_verify(const Port *port, const char *user,
! char *client_pass);
#endif
--- 15,21 ----
#include "libpq/libpq-be.h"
! extern int md5_crypt_verify(const Port *port, const char *role,
! char *client_pass, char **logdetail);
#endif
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
