Re: pgsql: Add support for GSSAPI authentication. - Mailing list pgsql-committers

mha@postgresql.org (Magnus Hagander) writes:
> Add support for GSSAPI authentication.

I looked over this patch and have a few comments, mostly stylistic:

+     /* errmsg_internal, since translation of the first part must be
+      * done before calling this function anyway. */
+     ereport(severity,
+             (errmsg_internal("%s:%s\n%s", text, localmsg1, localmsg2)));

Newlines in errmsg texts are generally a bad idea; you shouldn't be
trying to impose formatting that way.  Perhaps localmsg2 needs to be an
errdetail, instead?  It's not real clear what any of the three parts
are supposed to be ... perhaps you should choose more helpful variable
names.

+         ereport(DEBUG4,
+                 (errmsg_internal("Processing received GSS token of length: %u",
+                                  gbuf.length)));

The standard locution for purely-internal debugging messages is
elog(DEBUGn, "msg", ...) --- ereport is just useless notational
complexity for this.  (Quite a few places besides here)

+     ereport(DEBUG1,
+             (errmsg("GSSAPI authenticated name: %s", (char *)gbuf.value)));

Should this still be here?  In fact, how much of this logging do we want
in the production version at all?  I'm a bit worried about exposing
security-sensitive info in the log.

+         ereport(ERROR,
+                 (errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
+                  errmsg("provided username and GSSAPI username don't match"),
+                  errdetail("provided: %s, GSSAPI: %s",
+                      port->user_name, (char *)gbuf.value)));

Same here: this is definitely exposing more information to the client side
than we think appropriate for any other auth type.  Normally the return
to the client should be no more than "GSSAPI authentication failed", and
it should not be coming out from this level of the code.

      if (MyProcPort != NULL)
      {
+ #ifdef ENABLE_GSS
+         OM_uint32    min_s;
+         /* Shutdown GSSAPI layer */
+         if (MyProcPort->gss->ctx)
+             gss_delete_sec_context(&min_s, MyProcPort->gss->ctx, NULL);
+
+         if (MyProcPort->gss->cred)
+             gss_release_cred(&min_s, MyProcPort->gss->cred);
+ #endif
+
          /* Cleanly shut down SSL layer */
          secure_close(MyProcPort);

Why is this delayed until here?  AFAICS we don't need the GSSAPI
infrastructure anymore after the auth cycle is done.

+     /*
+      * Allocate GSSAPI specific state struct
+      */
+ #ifdef ENABLE_GSS
+     port->gss = (pg_gssinfo *)calloc(1, sizeof(pg_gssinfo));
+ #endif

This is pretty horrid --- what if calloc fails?  Why not use palloc0?

+                     /*
+                      * We can be called repeatedly for the same buffer.
+                      * Avoid re-allocating the buffer in this case -
+                      * just re-use the old buffer.
+                      */
+                     if (llen != conn->ginbuf.length)
+                     {
+                         if (conn->ginbuf.value)
+                             free(conn->ginbuf.value);
+
+                         conn->ginbuf.length = llen;
+                         conn->ginbuf.value = malloc(llen);
+                     }

Again, lacking any check for malloc failure; though on this side you
have to handle the failure yourself.

            regards, tom lane