Tom Lane wrote:
> [EMAIL PROTECTED] (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
---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend
