Re: Change authentication error message (patch)

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