Thread: Please review: Authentication after fork

Open issues:

* The postmaster will allow max_backends * 2 connections to exist, to
  prevent unauthenticated backends to fill up the limit.  When the
  max_backend+1'th backend tries to register itself for the shared
  resources I get:

NOTICE:  SIBackendInit: no free procState slot available
psql: FATAL 1:  Backend cache invalidation initialization failed
[terminates]

  I'd like to catch this case earlier, to avoid having to wire in such a
  fundamental setting so deeply into the resource management.  But I
  couldn't find a good interface to count the already-registered backends.

* Is it okay to ignore the count field in the password packet and read
  the actual password like a null-terminated string?  That was only there
  for the postmaster way of handling incomplete packets, right?

* Maybe we need to keep the "poor man's multitasking" code in the
  postmaster to allow for piecewise-arriving startup packets.  Surely it
  could be simplified a lot, but I didn't bother with this yet.


Index: src/backend/libpq/auth.c
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/libpq/auth.c,v
retrieving revision 1.52
diff -c -r1.52 auth.c
*** src/backend/libpq/auth.c    2001/03/22 03:59:30    1.52
--- src/backend/libpq/auth.c    2001/06/16 16:38:42
***************
*** 12,55 ****
   *
   *-------------------------------------------------------------------------
   */
! /*
!  * INTERFACE ROUTINES
!  *
!  *       backend (postmaster) routines:
!  *        be_recvauth                receive authentication information
!  */
! #include <sys/param.h>            /* for MAXHOSTNAMELEN on most */
! #ifndef  MAXHOSTNAMELEN
! #include <netdb.h>                /* for MAXHOSTNAMELEN on some */
! #endif
! #include <pwd.h>
! #include <ctype.h>

  #include <sys/types.h>            /* needed by in.h on Ultrix */
  #include <netinet/in.h>
  #include <arpa/inet.h>

- #include "postgres.h"
-
  #include "libpq/auth.h"
  #include "libpq/crypt.h"
  #include "libpq/hba.h"
  #include "libpq/libpq.h"
  #include "libpq/password.h"
  #include "miscadmin.h"

- static void sendAuthRequest(Port *port, AuthRequest areq, PacketDoneProc handler);
- static int    handle_done_auth(void *arg, PacketLen len, void *pkt);
- static int    handle_krb4_auth(void *arg, PacketLen len, void *pkt);
- static int    handle_krb5_auth(void *arg, PacketLen len, void *pkt);
- static int    handle_password_auth(void *arg, PacketLen len, void *pkt);
- static int    readPasswordPacket(void *arg, PacketLen len, void *pkt);
- static int    pg_passwordv0_recvauth(void *arg, PacketLen len, void *pkt);
  static int    checkPassword(Port *port, char *user, char *password);
  static int    old_be_recvauth(Port *port);
  static int    map_old_to_new(Port *port, UserAuth old, int status);
  static void auth_failed(Port *port);


  char       *pg_krb_server_keyfile;

--- 12,41 ----
   *
   *-------------------------------------------------------------------------
   */
!
! #include "postgres.h"

  #include <sys/types.h>            /* needed by in.h on Ultrix */
  #include <netinet/in.h>
  #include <arpa/inet.h>

  #include "libpq/auth.h"
  #include "libpq/crypt.h"
  #include "libpq/hba.h"
  #include "libpq/libpq.h"
  #include "libpq/password.h"
+ #include "libpq/pqformat.h"
  #include "miscadmin.h"
+
+ static void sendAuthRequest(Port *port, AuthRequest areq);

  static int    checkPassword(Port *port, char *user, char *password);
  static int    old_be_recvauth(Port *port);
  static int    map_old_to_new(Port *port, UserAuth old, int status);
  static void auth_failed(Port *port);

+ static int    recv_and_check_password_packet(Port *port);
+ static int    recv_and_check_passwordv0(Port *port);

  char       *pg_krb_server_keyfile;

***************
*** 325,349 ****
  /*
   * Handle a v0 password packet.
   */
-
  static int
! pg_passwordv0_recvauth(void *arg, PacketLen len, void *pkt)
  {
!     Port       *port;
      PasswordPacketV0 *pp;
      char       *user,
                 *password,
                 *cp,
                 *start;

!     port = (Port *) arg;
!     pp = (PasswordPacketV0 *) pkt;

      /*
       * The packet is supposed to comprise the user name and the password
       * as C strings.  Be careful the check that this is the case.
       */
-
      user = password = NULL;

      len -= sizeof(pp->unused);
--- 311,338 ----
  /*
   * Handle a v0 password packet.
   */
  static int
! recv_and_check_passwordv0(Port *port)
  {
!     int32        len;
!     char       *buf;
      PasswordPacketV0 *pp;
      char       *user,
                 *password,
                 *cp,
                 *start;
+
+     pq_getint(&len, 4);
+     len -= 4;
+     buf = palloc(len);
+     pq_getbytes(buf, len);

!     pp = (PasswordPacketV0 *) buf;

      /*
       * The packet is supposed to comprise the user name and the password
       * as C strings.  Be careful the check that this is the case.
       */
      user = password = NULL;

      len -= sizeof(pp->unused);
***************
*** 371,376 ****
--- 360,366 ----
          fputs(PQerrormsg, stderr);
          pqdebug("%s", PQerrormsg);

+         pfree(buf);
          auth_failed(port);
      }
      else
***************
*** 385,399 ****

          status = checkPassword(port, user, password);

          port->auth_method = saved;

          /* Adjust the result if necessary. */
-
          if (map_old_to_new(port, uaPassword, status) != STATUS_OK)
              auth_failed(port);
      }

!     return STATUS_OK;            /* don't close the connection yet */
  }


--- 375,389 ----

          status = checkPassword(port, user, password);

+         pfree(buf);
          port->auth_method = saved;

          /* Adjust the result if necessary. */
          if (map_old_to_new(port, uaPassword, status) != STATUS_OK)
              auth_failed(port);
      }

!     return STATUS_OK;
  }


***************
*** 413,419 ****
  static void
  auth_failed(Port *port)
  {
-     char        buffer[512];
      const char *authmethod = "Unknown auth method:";

      switch (port->auth_method)
--- 403,408 ----
***************
*** 440,459 ****
              authmethod = "Password";
              break;
      }
-
-     sprintf(buffer, "%s authentication failed for user '%s'",
-             authmethod, port->user);

!     PacketSendError(&port->pktInfo, buffer);
  }


  /*
!  * be_recvauth -- server demux routine for incoming authentication information
   */
  void
! be_recvauth(Port *port)
  {

      /*
       * Get the authentication method to use for this frontend/database
--- 429,449 ----
              authmethod = "Password";
              break;
      }

!     elog(FATAL, "%s authentication failed for user '%s'",
!          authmethod, port->user);
  }


+
  /*
!  * Client authentication starts here.  If there is an error, this
!  * function does not return and the backend process is terminated.
   */
  void
! ClientAuthentication(Port *port)
  {
+     int status;

      /*
       * Get the authentication method to use for this frontend/database
***************
*** 463,559 ****
       */

      if (hba_getauthmethod(port) != STATUS_OK)
!         PacketSendError(&port->pktInfo,
!                         "Missing or erroneous pg_hba.conf file, see postmaster log for details");

      else if (PG_PROTOCOL_MAJOR(port->proto) == 0)
      {
-         /* Handle old style authentication. */
-
          if (old_be_recvauth(port) != STATUS_OK)
              auth_failed(port);
      }
-     else
-     {
-         /* Handle new style authentication. */
-
-         AuthRequest areq = AUTH_REQ_OK;
-         PacketDoneProc auth_handler = NULL;

!         switch (port->auth_method)
!         {
!             case uaReject:
!
!                 /*
!                  * This could have come from an explicit "reject" entry in
!                  * pg_hba.conf, but more likely it means there was no
!                  * matching entry.    Take pity on the poor user and issue a
!                  * helpful error message.  NOTE: this is not a security
!                  * breach, because all the info reported here is known at
!                  * the frontend and must be assumed known to bad guys.
!                  * We're merely helping out the less clueful good guys.
!                  * NOTE 2: libpq-be.h defines the maximum error message
!                  * length as 99 characters.  It probably wouldn't hurt
!                  * anything to increase it, but there might be some client
!                  * out there that will fail.  So, be terse.
!                  */
!                 {
!                     char        buffer[512];
!                     const char *hostinfo = "localhost";
!
!                     if (port->raddr.sa.sa_family == AF_INET)
!                         hostinfo = inet_ntoa(port->raddr.in.sin_addr);
!                     sprintf(buffer,
!                             "No pg_hba.conf entry for host %s, user %s, database %s",
!                             hostinfo, port->user, port->database);
!                     PacketSendError(&port->pktInfo, buffer);
!                     return;
!                 }
!                 break;

!             case uaKrb4:
!                 areq = AUTH_REQ_KRB4;
!                 auth_handler = handle_krb4_auth;
!                 break;

!             case uaKrb5:
!                 areq = AUTH_REQ_KRB5;
!                 auth_handler = handle_krb5_auth;
!                 break;

!             case uaTrust:
!                 areq = AUTH_REQ_OK;
!                 auth_handler = handle_done_auth;
!                 break;

!             case uaIdent:
!                 if (authident(&port->raddr.in, &port->laddr.in,
!                               port->user, port->auth_arg) == STATUS_OK)
!                 {
!                     areq = AUTH_REQ_OK;
!                     auth_handler = handle_done_auth;
!                 }

!                 break;

!             case uaPassword:
!                 areq = AUTH_REQ_PASSWORD;
!                 auth_handler = handle_password_auth;
!                 break;

!             case uaCrypt:
!                 areq = AUTH_REQ_CRYPT;
!                 auth_handler = handle_password_auth;
!                 break;
!         }

!         /* Tell the frontend what we want next. */

!         if (auth_handler != NULL)
!             sendAuthRequest(port, areq, auth_handler);
!         else
!             auth_failed(port);
      }
  }


--- 453,529 ----
       */

      if (hba_getauthmethod(port) != STATUS_OK)
!         elog(FATAL, "Missing or erroneous pg_hba.conf file, see postmaster log for details");

+     /* Handle old style authentication. */
      else if (PG_PROTOCOL_MAJOR(port->proto) == 0)
      {
          if (old_be_recvauth(port) != STATUS_OK)
              auth_failed(port);
+         return;
      }

!     /* Handle new style authentication. */

!     switch (port->auth_method)
!     {
!         case uaReject:

!             /*
!              * This could have come from an explicit "reject" entry in
!              * pg_hba.conf, but more likely it means there was no
!              * matching entry.    Take pity on the poor user and issue a
!              * helpful error message.  NOTE: this is not a security
!              * breach, because all the info reported here is known at
!              * the frontend and must be assumed known to bad guys.
!              * We're merely helping out the less clueful good guys.
!              */
!         {
!             const char *hostinfo = "localhost";

!             if (port->raddr.sa.sa_family == AF_INET)
!                 hostinfo = inet_ntoa(port->raddr.in.sin_addr);
!             elog(FATAL,
!                  "No pg_hba.conf entry for host %s, user %s, database %s",
!                  hostinfo, port->user, port->database);
!             return;
!         }
!         break;

!         case uaKrb4:
!             sendAuthRequest(port, AUTH_REQ_KRB4);
!             status = pg_krb4_recvauth(port);
!             break;

!         case uaKrb5:
!             sendAuthRequest(port, AUTH_REQ_KRB5);
!             status = pg_krb5_recvauth(port);
!             break;

!         case uaIdent:
!             status = authident(&port->raddr.in, &port->laddr.in,
!                                port->user, port->auth_arg);
!             break;

!         case uaPassword:
!             sendAuthRequest(port, AUTH_REQ_PASSWORD);
!             status = recv_and_check_password_packet(port);
!             break;

!         case uaCrypt:
!             sendAuthRequest(port, AUTH_REQ_CRYPT);
!             status = recv_and_check_password_packet(port);
!             break;

!         case uaTrust:
!             status = STATUS_OK;
!             break;
      }
+
+     if (status == STATUS_OK)
+         sendAuthRequest(port, AUTH_REQ_OK);
+     else
+         auth_failed(port);
  }


***************
*** 562,695 ****
   */

  static void
! sendAuthRequest(Port *port, AuthRequest areq, PacketDoneProc handler)
  {
!     char       *dp,
!                *sp;
!     int            i;
!     uint32        net_areq;
!
!     /* Convert to a byte stream. */
!
!     net_areq = htonl(areq);

!     dp = port->pktInfo.pkt.ar.data;
!     sp = (char *) &net_areq;

-     *dp++ = 'R';
-
-     for (i = 1; i <= 4; ++i)
-         *dp++ = *sp++;
-
      /* Add the salt for encrypted passwords. */
-
      if (areq == AUTH_REQ_CRYPT)
      {
!         *dp++ = port->salt[0];
!         *dp++ = port->salt[1];
!         i += 2;
      }

!     PacketSendSetup(&port->pktInfo, i, handler, (void *) port);
  }


- /*
-  * Called when we have told the front end that it is authorised.
-  */

- static int
- handle_done_auth(void *arg, PacketLen len, void *pkt)
- {
-
-     /*
-      * Don't generate any more traffic.  This will cause the backend to
-      * start.
-      */
-
-     return STATUS_OK;
- }
-
-
- /*
-  * Called when we have told the front end that it should use Kerberos V4
-  * authentication.
-  */
-
- static int
- handle_krb4_auth(void *arg, PacketLen len, void *pkt)
- {
-     Port       *port = (Port *) arg;
-
-     if (pg_krb4_recvauth(port) != STATUS_OK)
-         auth_failed(port);
-     else
-         sendAuthRequest(port, AUTH_REQ_OK, handle_done_auth);
-
-     return STATUS_OK;
- }
-
-
- /*
-  * Called when we have told the front end that it should use Kerberos V5
-  * authentication.
-  */
-
- static int
- handle_krb5_auth(void *arg, PacketLen len, void *pkt)
- {
-     Port       *port = (Port *) arg;
-
-     if (pg_krb5_recvauth(port) != STATUS_OK)
-         auth_failed(port);
-     else
-         sendAuthRequest(port, AUTH_REQ_OK, handle_done_auth);
-
-     return STATUS_OK;
- }
-
-
  /*
-  * Called when we have told the front end that it should use password
-  * authentication.
-  */
-
- static int
- handle_password_auth(void *arg, PacketLen len, void *pkt)
- {
-     Port       *port = (Port *) arg;
-
-     /* Set up the read of the password packet. */
-
-     PacketReceiveSetup(&port->pktInfo, readPasswordPacket, (void *) port);
-
-     return STATUS_OK;
- }
-
-
- /*
   * Called when we have received the password packet.
   */

  static int
! readPasswordPacket(void *arg, PacketLen len, void *pkt)
  {
!     char        password[sizeof(PasswordPacket) + 1];
!     Port       *port = (Port *) arg;
!
!     /* Silently truncate a password that is too big. */
!
!     if (len > sizeof(PasswordPacket))
!         len = sizeof(PasswordPacket);
!
!     StrNCpy(password, ((PasswordPacket *) pkt)->passwd, len);
!
!     if (checkPassword(port, port->user, password) != STATUS_OK)
!         auth_failed(port);
!     else
!         sendAuthRequest(port, AUTH_REQ_OK, handle_done_auth);
!
!     return STATUS_OK;            /* don't close the connection yet */
  }


--- 532,576 ----
   */

  static void
! sendAuthRequest(Port *port, AuthRequest areq)
  {
!     StringInfoData buf;

!     pq_beginmessage(&buf);
!     pq_sendbyte(&buf, 'R');
!     pq_sendint(&buf, (int32) areq, sizeof(int32));

      /* Add the salt for encrypted passwords. */
      if (areq == AUTH_REQ_CRYPT)
      {
!         pq_sendint(&buf, port->salt[0], 1);
!         pq_sendint(&buf, port->salt[1], 1);
      }

!     pq_endmessage(&buf);
!     pq_flush();
  }



  /*
   * Called when we have received the password packet.
   */

  static int
! recv_and_check_password_packet(Port *port)
  {
!     StringInfoData buf;
!     int32        len;
!     int            result;
!
!     pq_getint(&len, 4);
!     initStringInfo(&buf);
!     pq_getstr(&buf);
!
!     result = checkPassword(port, port->user, buf.data);
!     pfree(buf.data);
!     return result;
  }


***************
*** 734,743 ****
              break;

          case STARTUP_PASSWORD_MSG:
!             PacketReceiveSetup(&port->pktInfo, pg_passwordv0_recvauth,
!                                (void *) port);
!
!             return STATUS_OK;

          default:
              fprintf(stderr, "Invalid startup message type: %u\n", msgtype);
--- 615,622 ----
              break;

          case STARTUP_PASSWORD_MSG:
!             status = recv_and_check_passwordv0(port);
!             break;

          default:
              fprintf(stderr, "Invalid startup message type: %u\n", msgtype);
***************
*** 760,767 ****
  {
      switch (port->auth_method)
      {
!             case uaCrypt:
!             case uaReject:
              status = STATUS_ERROR;
              break;

--- 639,646 ----
  {
      switch (port->auth_method)
      {
!         case uaCrypt:
!         case uaReject:
              status = STATUS_ERROR;
              break;

Index: src/backend/postmaster/postmaster.c
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/postmaster/postmaster.c,v
retrieving revision 1.219
diff -c -r1.219 postmaster.c
*** src/backend/postmaster/postmaster.c    2001/06/12 22:54:05    1.219
--- src/backend/postmaster/postmaster.c    2001/06/16 16:38:46
***************
*** 1200,1209 ****
          return STATUS_OK;        /* don't close the connection yet */
      }

-     /* Start the authentication itself. */
-
-     be_recvauth(port);
-
      return STATUS_OK;            /* don't close the connection yet */
  }

--- 1200,1205 ----
***************
*** 1280,1287 ****
          return "The Data Base System is starting up";
      if (FatalError)
          return "The Data Base System is in recovery mode";
!     /* Can't start backend if max backend count is exceeded. */
!     if (CountChildren() >= MaxBackends)
          return "Sorry, too many clients already";

      return NULL;
--- 1276,1284 ----
          return "The Data Base System is starting up";
      if (FatalError)
          return "The Data Base System is in recovery mode";
!     /* We allow more connections than we can have backends here
!        because some might fail authentication.  */
!     if (CountChildren() >= MaxBackends * 2)
          return "Sorry, too many clients already";

      return NULL;
Index: src/backend/tcop/postgres.c
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/tcop/postgres.c,v
retrieving revision 1.220
diff -c -r1.220 postgres.c
*** src/backend/tcop/postgres.c    2001/06/12 22:54:06    1.220
--- src/backend/tcop/postgres.c    2001/06/16 16:38:50
***************
*** 45,50 ****
--- 45,51 ----
  #include "libpq/pqformat.h"
  #include "libpq/pqsignal.h"
  #include "miscadmin.h"
+ #include "libpq/auth.h"
  #include "nodes/print.h"
  #include "optimizer/cost.h"
  #include "optimizer/planner.h"
***************
*** 1147,1152 ****
--- 1148,1156 ----
      }

      SetProcessingMode(InitProcessing);
+
+     if (IsUnderPostmaster)
+         ClientAuthentication(MyProcPort); /* might not return */

      /*
       * Set default values for command-line options.
Index: src/include/libpq/auth.h
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/include/libpq/auth.h,v
retrieving revision 1.16
diff -c -r1.16 auth.h
*** src/include/libpq/auth.h    2001/03/22 04:00:47    1.16
--- src/include/libpq/auth.h    2001/06/16 16:38:51
***************
*** 21,27 ****
   *----------------------------------------------------------------
   */

! void        be_recvauth(Port *port);

  #define PG_KRB4_VERSION "PGVER4.1"        /* at most KRB_SENDAUTH_VLEN chars */
  #define PG_KRB5_VERSION "PGVER5.1"
--- 21,27 ----
   *----------------------------------------------------------------
   */

! void        ClientAuthentication(Port *port);

  #define PG_KRB4_VERSION "PGVER4.1"        /* at most KRB_SENDAUTH_VLEN chars */
  #define PG_KRB5_VERSION "PGVER5.1"
===THE END

--
Peter Eisentraut   peter_e@gmx.net   http://funkturm.homeip.net/~peter

Peter Eisentraut <peter_e@gmx.net> writes:
>   When the
>   max_backend+1'th backend tries to register itself for the shared
>   resources I get:

> NOTICE:  SIBackendInit: no free procState slot available
> psql: FATAL 1:  Backend cache invalidation initialization failed
> [terminates]

This was the test I alluded to as needing to be moved up.

>   I'd like to catch this case earlier, to avoid having to wire in such a
>   fundamental setting so deeply into the resource management.  But I
>   couldn't find a good interface to count the already-registered backends.

That would not do: you'd have a race condition, since there might be
free slots when you count but none by the time you want to actually
register.  Checking for free slots and acquiring one has to be an atomic
operation.

I've been in that code before; if you like I'll take care of making this
part work more nicely.

> * Is it okay to ignore the count field in the password packet and read
>   the actual password like a null-terminated string?  That was only there
>   for the postmaster way of handling incomplete packets, right?

Seems like we ought to keep the packet-parsing rules the same, to avoid
possible introduction of client compatibility problems.

> * Maybe we need to keep the "poor man's multitasking" code in the
>   postmaster to allow for piecewise-arriving startup packets.  Surely it
>   could be simplified a lot, but I didn't bother with this yet.

Um, shouldn't collection of the startup packet be done after the fork?
I was envisioning that the top process do nothing except accept() and
fork().  So there's no need for pseudo-multitasking: you just have the
one connection to deal with in any one PM child.

            regards, tom lane

Tom Lane writes:

> > * Is it okay to ignore the count field in the password packet and read
> >   the actual password like a null-terminated string?  That was only there
> >   for the postmaster way of handling incomplete packets, right?
>
> Seems like we ought to keep the packet-parsing rules the same, to avoid
> possible introduction of client compatibility problems.

Hmm, the current code cuts off the password at 99 (+/-1) characters.  I
think there's a TODO item to get rid of those limits, and sending anything
else would be a (rather stupid) protocol violation anyway, so I think I
will keep this part.

> Um, shouldn't collection of the startup packet be done after the fork?

To handle query cancel requests we'd need to take a peek in the
postmaster, unless we want to start up a new backend for that.  Also, I'm
not sure how the SSL negotiation would work.  It's doable, might be
worthwhile, but should be a separate consideration.

--
Peter Eisentraut   peter_e@gmx.net   http://funkturm.homeip.net/~peter

I said:
> I've been in that code before; if you like I'll take care of making this
> part work more nicely.

Okay ... enforcement of the MaxBackends limit now happens during
InitPostgres, not in the postmaster ...

            regards, tom lane

Peter Eisentraut <peter_e@gmx.net> writes:
>> Um, shouldn't collection of the startup packet be done after the fork?

> To handle query cancel requests we'd need to take a peek in the
> postmaster, unless we want to start up a new backend for that.

Why?  The PM child will inherit the list of active backends from the
parent, so it could still issue the kill(SIGINT) without any problem
that I can see.  You do have a point: we'll need to generate the
CancelKey for a child much earlier than now, ie, before the fork.
(Drat, a few more cycles in the top postmaster.)  But that doesn't
look like a big problem.  Doesn't really matter if we generate a
cancel key for a child that never becomes a backend.

> Also, I'm not sure how the SSL negotiation would work.

Moving SSL negotiation out of the top process is one of the key points
of this whole change.  AFAIK, SSL negotation is another place where the
postmaster currently blocks on a single client, and thus is a trivial
attack point for DOS attempts.

It's not immediately clear to me whether InitSSL() should be called once
by the top postmaster process or once in each child that sees it is
getting an SSL connection request.  But other than that, I don't see
an issue...

            regards, tom lane