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

Magnus Hagander <magnus@hagander.net> writes:
> Tom Lane wrote:
>> 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?

It's not very important, but I'd be inclined to change it just so
we don't have bad examples laying about the codebase.

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

Lots of people run with log level debug1.  I wouldn't complain so much
if it were at a low debug level, but I do not see why this is considered
so much more important than the other debugging messages.  Also see below.

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

No, I think it's at best DEBUG2, and at worst a security breach.
By your reasoning we should log the password when a password failure
occurs.  (I seem to recall that we actually did that once, and properly
got beat up for it.)  The reason I'm leery of this is that it's not that
difficult to enter a password where a username is expected and vice
versa; if you make a habit of logging usernames for failed logins you
will eventually capture somebody's password in the log.  The mere fact
of the auth failure will get logged anyway, and it would only be if
there were a lot of such errors that it would make sense for the admin
to crank up the debug level to see what's going on.

In any case it's contrary to the design of this part of the code to be
throwing ERROR rather than returning STATUS_ERROR.

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

Ah.  OK, fair enough.

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

It wouldn't be a bad idea to change it.  (Note you need to check which
context that code runs in.  Might be best to explicitly do
MemoryContextAllocZero(TopMemoryContext), just to be sure it won't go
away unexpectedly.)

            regards, tom lane