Обсуждение: pgsql: Add support for GSSAPI authentication.
Log Message:
-----------
Add support for GSSAPI authentication.
Documentation still being written, will be committed later.
Henry B. Hotz and Magnus Hagander
Modified Files:
--------------
pgsql:
configure (r1.549 -> r1.550)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/configure.diff?r1=1.549&r2=1.550)
configure.in (r1.516 -> r1.517)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/configure.in.diff?r1=1.516&r2=1.517)
pgsql/src/backend/libpq:
auth.c (r1.148 -> r1.149)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/libpq/auth.c.diff?r1=1.148&r2=1.149)
hba.c (r1.160 -> r1.161)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/libpq/hba.c.diff?r1=1.160&r2=1.161)
pg_hba.conf.sample (r1.62 -> r1.63)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/libpq/pg_hba.conf.sample.diff?r1=1.62&r2=1.63)
pqcomm.c (r1.192 -> r1.193)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/libpq/pqcomm.c.diff?r1=1.192&r2=1.193)
pgsql/src/backend/postmaster:
postmaster.c (r1.530 -> r1.531)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/postmaster/postmaster.c.diff?r1=1.530&r2=1.531)
pgsql/src/backend/utils/misc:
guc.c (r1.404 -> r1.405)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/misc/guc.c.diff?r1=1.404&r2=1.405)
postgresql.conf.sample (r1.218 -> r1.219)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/misc/postgresql.conf.sample.diff?r1=1.218&r2=1.219)
pgsql/src/include:
pg_config.h.in (r1.117 -> r1.118)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/pg_config.h.in.diff?r1=1.117&r2=1.118)
pgsql/src/include/libpq:
hba.h (r1.45 -> r1.46)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/libpq/hba.h.diff?r1=1.45&r2=1.46)
libpq-be.h (r1.58 -> r1.59)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/libpq/libpq-be.h.diff?r1=1.58&r2=1.59)
pqcomm.h (r1.104 -> r1.105)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/libpq/pqcomm.h.diff?r1=1.104&r2=1.105)
pgsql/src/interfaces/libpq:
Makefile (r1.154 -> r1.155)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/interfaces/libpq/Makefile.diff?r1=1.154&r2=1.155)
fe-auth.c (r1.123 -> r1.124)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/interfaces/libpq/fe-auth.c.diff?r1=1.123&r2=1.124)
fe-connect.c (r1.347 -> r1.348)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/interfaces/libpq/fe-connect.c.diff?r1=1.347&r2=1.348)
libpq-int.h (r1.121 -> r1.122)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/interfaces/libpq/libpq-int.h.diff?r1=1.121&r2=1.122)
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
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
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
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
Magnus Hagander <magnus@hagander.net> writes:
> 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?
I'd go with DEBUG2 or so for messages that might be significant to
someone wondering "why can't Joe log in?", and 3 or 4 for messages
that would only be interesting to someone trying to debug the code.
Of the two messages at issue here, I think the first one is at the lower
level, if needed at all, because it's purely a progress message.
If you're going to log the username at the point where you decide it's
bad, then there's no need to log it when you get it.
regards, tom lane
On Tue, Jul 10, 2007 at 02:26:26PM -0400, Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: > > 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? > > I'd go with DEBUG2 or so for messages that might be significant to > someone wondering "why can't Joe log in?", and 3 or 4 for messages > that would only be interesting to someone trying to debug the code. > > Of the two messages at issue here, I think the first one is at the lower > level, if needed at all, because it's purely a progress message. > If you're going to log the username at the point where you decide it's > bad, then there's no need to log it when you get it. I've applied the fixes. I had already removed the progress message because as you say, when you really need it you get it in the error message. //Magnus