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

Tom Lane wrote:
> 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.

Ok. I'll change them.


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

No. By my reasoning, we should log the *username* when the password
fails. (I'm not saying we should specifically, I'm just saying that's
the analogy that works)


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

Note that with GSSAPI, there *is* no password to be logged.

Anyway. I can live with having it as a DEBUG message, and will change it
to that. What level of debug do you think is reasonable?


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

Right. That gets fixed when I change it to DEBUG.


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

Ok. I'll leave that as a separate thing though - will just check for the
error for now, and take that as a different patch.

//Magnus