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

Tom Lane wrote:
> mha@postgresql.org (Magnus Hagander) writes:
>> Add support for GSSAPI authentication.
>
> I looked over this patch and have a few comments, mostly stylistic:

Thanks!


> +     /* 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.

I'll look into exactly how it is. The docs I've looked at so far haven't
really been forthcoming into what goes where either :-S


> +         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)

Ok. Want it changed for the lot of them, or just for future ref?


> +     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.

It's on DEBUG, so it wouldn't be in production, no? I think it makes
sense to keep it there, since the gssapi name can be different from the
specified username... (both in content and case)


> +         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.

Hm, it's all just information that the client provided, but I take it
you're worried about cases where a server (say a web server) is
reporting the error directly to a client that's not supposed to know it?

But it's certainly data you'd want in your server logs - it's going to
be impossible to debug otherwise. So what'd be the best way to get that
in there while not sending it back to the client?


>       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.

It was in the original patch, so I left it there. The main reason is
that if/when we implement GSSAPI encryption, we will need the
infrastructure until this point.


> +     /*
> +      * 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?

Wow. Can't beleive I missed that one :-S
Will fix.

The reason for not using palloc0 - well, I was modeling it on the call
to allocate the actual port structure, which uses calloc. Is that wrong?


> +                     /*
> +                      * 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.

Gah. I actually had code that checked for that error in a different copy
of the patch (the win32 branch) :-( Missed to sync it in.

(Will commit fixes to these once I'm back at the box where I can test
that it doesn't break things)


//Magnus