Kerberos brokenness and oops question in 8.1beta2

"Magnus Hagander" <mha@sollentuna.net> writes:

Anyway. This makes it impossible for a 8.1 client to connect to a 8.0
server, or a 8.0 client to a 8.1 server, in any case where the service
name has changed - such as a win32 active directory deployment, but I'm
sure many others as well.

How important is that really? How many win32 users are likely to be
using Kerberos auth with 8.0?

The only real advantage to how it is now is that it's "cleaner". The
argument that it protects against a security hole in MIT KRB5 doesn't
hold any more because there is a patch out, and we can't take
responsibility for people who haven't patched.

I don't really buy that argument. ISTM we should fix the code to do the
right thing, especially if the right thing is more secure. If I
understood what you said properly, hardwiring it as "postgres" is the
correct thing, and loss of compatibility in marginal cases is just the
price we pay for having done it wrong originally.

regards, tom lane

Anyway. This makes it impossible for a 8.1 client to

connect to a 8.0

server, or a 8.0 client to a 8.1 server, in any case where

the service

name has changed - such as a win32 active directory deployment, but
I'm sure many others as well.

How important is that really? How many win32 users are
likely to be using Kerberos auth with 8.0?

Not all that many - especially since it required a recompile to work
with AD. But some, I know of at least a couple who have mailed me about
instructions on how to do it, for example.

I don't know how many other cases changed principal names are used in,
though - we had the functionality to change it in the backend long
before we supported kerberos on windows.

The only real advantage to how it is now is that it's

"cleaner". The

argument that it protects against a security hole in MIT

KRB5 doesn't

hold any more because there is a patch out, and we can't take
responsibility for people who haven't patched.

I don't really buy that argument. ISTM we should fix the
code to do the right thing, especially if the right thing is
more secure. If I understood what you said properly,
hardwiring it as "postgres" is the correct thing, and loss of
compatibility in marginal cases is just the price we pay for
having done it wrong originally.

I said it was probably cleaner, which may or may not be the same as
"correct". It's very hard to find good documentation about the
krb5_sendauth/recvauth calls, so I'm not very sure about that - that's
why I'm asking before coding. The best I've found now that I searched
some more states:

"The paramter appl_version is a string describing the application
protocol version which the client is expecting to use for this exchange.
If the server is using a different application protocol, an error will
be returned."

But we already deal with protocol versions outside of this, so there's
not need ot use that functionality. Then again, there is nothing in the
spec that prevents us from using it the way we have previously done
either.

Quick code-check shows that if we set it to NULL instead it disables the
check on MIT Kerberos for to fix exactly this kind of issue, but it
looks like it would cause a crash on Heimdal, so that's not realliy a
good idea either.

The point is I'm having a hard time seeing what the actual gain is in
not changing it back. If the principal name mismatches, we're going to
get rejected anyway, so it's not really a problem there. Even though the
gain in changing it back isn't all that big either, why should we
introduce abackwards-incompatibility if there is no real gain in a
different part of the code.

//Magnus

The point is I'm having a hard time seeing what the actual
gain is in not changing it back. If the principal name
mismatches, we're going to get rejected anyway, so it's not
really a problem there. Even though the gain in changing it
back isn't all that big either, why should we introduce
abackwards-incompatibility if there is no real gain in a
different part of the code.

Here's a patch that fixes the big problem and reverts the behaviour of
appl_version to be compatible with 8.0. It's easy enough to isolate the
changes that are around the appl_version - one line in
backend/libpq/auth.c call to krb5_recvauth and one in
interfaces/libpq/fe-auth.c call to krb5_sendauth.

The call in backend/libpq/auth.c to krb5_sname_to_principal in 8.1beta2
was completely broken for a scenario where you *didn't* use virtual
hosts, by setting pg_krb5_server to NULL... The call is needed there as
well.

//Magnus

"Magnus Hagander" <mha@sollentuna.net> writes:

Here's a patch that fixes the big problem and reverts the behaviour of
appl_version to be compatible with 8.0.

Applied with trivial stylistic cleanups.

BTW, the documentation seems a bit broken:

krb_server_hostname (string)

Sets the hostname part of the service principal. This, combined
with krb_srvname, is used to generate the complete service
principal, i.e. krb_server_hostname/krb_server_hostname@REALM.

I suppose one of those last two should be "krb_srvname", but which?

regards, tom lane

Here's a patch that fixes the big problem and reverts the

behaviour of

appl_version to be compatible with 8.0.

Applied with trivial stylistic cleanups.

BTW, the documentation seems a bit broken:

krb_server_hostname (string)

Sets the hostname part of the service principal. This, combined
with krb_srvname, is used to generate the complete service
principal, i.e. krb_server_hostname/krb_server_hostname@REALM.

I suppose one of those last two should be "krb_srvname", but which?

Yes.
It should be krb_srvname/krb_server_hostname@REALM.

Now that you mention it, krb_server_hostname should probably mentioned
in chapter 19.2.3 somewhere.

//Magnus

"Magnus Hagander" <mha@sollentuna.net> writes:

I suppose one of those last two should be "krb_srvname", but which?

Yes.
It should be krb_srvname/krb_server_hostname@REALM.

OK, will fix.

Now that you mention it, krb_server_hostname should probably mentioned
in chapter 19.2.3 somewhere.

Yeah, it just says

hostname is the fully qualified host name of the server machine.

which is clearly insufficient detail. Please send some proposed text.

regards, tom lane

BTW, it appears to me that this patch has also broken the claim in the
manual that

If [krb_server_hostname is] not set, the default is to allow any
service principal matching an entry in the keytab.

The reason that was true was that we passed a NULL "server" value to
krb5_recvauth(), which with this patch we never do anymore.

I'm not sure if this represents a serious loss of flexibility or not,
but in any case the documentation needs an update.

regards, tom lane

I need a comment on this.

---------------------------------------------------------------------------

Tom Lane wrote:

"Magnus Hagander" <mha@sollentuna.net> writes:

Here's a patch that fixes the big problem and reverts the behaviour of
appl_version to be compatible with 8.0.

Applied with trivial stylistic cleanups.

BTW, the documentation seems a bit broken:

krb_server_hostname (string)

Sets the hostname part of the service principal. This, combined
with krb_srvname, is used to generate the complete service
principal, i.e. krb_server_hostname/krb_server_hostname@REALM.

I suppose one of those last two should be "krb_srvname", but which?

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

I need a comment on this.

---------------------------------------------------------------------------

Tom Lane wrote:

BTW, it appears to me that this patch has also broken the claim in the
manual that

If [krb_server_hostname is] not set, the default is to allow any
service principal matching an entry in the keytab.

The reason that was true was that we passed a NULL "server" value to
krb5_recvauth(), which with this patch we never do anymore.

I'm not sure if this represents a serious loss of flexibility or not,
but in any case the documentation needs an update.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Tom has already applied a fix for this:
http://archives.postgresql.org/pgsql-committers/2005-10/msg00114.php

//Magnus

"Magnus Hagander" <mha@sollentuna.net> writes:

Tom has already applied a fix for this:
http://archives.postgresql.org/pgsql-committers/2005-10/msg00114.php

I requested more docs changes from you, though.

regards, tom lane

(Tom, this is the other one you were referring to, I hope.)

I'm still planning to do this, but I'm very pressed for time right now.
I'll try to get it done as soon as possible, but worst case it may be
around two weeks before I can do it. Sorry. If someone else wants to
beat me to it go right ahead, otherwise - it's on it's way eventually.
I think a general overview to make sure the different parts (config
section vs kerberos auth section) are actually in sync is required.

//Magnus

We don't have two weeks, I think. If I can catch you on IM I can make
the modifications, with a little hand-holding.

---------------------------------------------------------------------------

Magnus Hagander wrote:

(Tom, this is the other one you were referring to, I hope.)

I'm still planning to do this, but I'm very pressed for time right now.
I'll try to get it done as soon as possible, but worst case it may be
around two weeks before I can do it. Sorry. If someone else wants to
beat me to it go right ahead, otherwise - it's on it's way eventually.
I think a general overview to make sure the different parts (config
section vs kerberos auth section) are actually in sync is required.

//Magnus

I need a comment on this.

--------------------------------------------------------------
-------------

Tom Lane wrote:

BTW, it appears to me that this patch has also broken the

claim in the

manual that

If [krb_server_hostname is] not set, the default is to allow any
service principal matching an entry in the keytab.

The reason that was true was that we passed a NULL "server"

value to

krb5_recvauth(), which with this patch we never do anymore.

I'm not sure if this represents a serious loss of

flexibility or not,

but in any case the documentation needs an update.

regards, tom lane

---------------------------(end of
broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

-- 
Bruce Momjian                        |  http://candle.pha.pa.us
pgman@candle.pha.pa.us               |  (610) 359-1001
+  If your life is a hard drive,     |  13 Roberts Road
+  Christ can be your backup.        |  Newtown Square, 
Pennsylvania 19073

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

We don't have two weeks, I think. If I can catch you on IM I can make
the modifications, with a little hand-holding.

For docs changes I think we do. I'm a bit worried however about whether
the loss of wildcard functionality is a problem requiring code changes.

regards, tom lane

We don't have two weeks, I think. If I can catch you on IM

I can make

the modifications, with a little hand-holding.

For docs changes I think we do. I'm a bit worried however
about whether the loss of wildcard functionality is a problem
requiring code changes.

Well, we didn't have it in 8.0. And I'm not even sure it ever worked in
8.1dev - it certainly didn't work in my case.

So. *I* think we're fine with removing it, as long as we update the docs
to be in sync. But perhaps someone who actually used it can confirm if
it's worthwhile?

//Magnus

"Magnus Hagander" <mha@sollentuna.net> writes:

For docs changes I think we do. I'm a bit worried however
about whether the loss of wildcard functionality is a problem
requiring code changes.

Well, we didn't have it in 8.0.

Oh, OK. I had been thinking it was pre-existing behavior, but if it's
not then there's no issue. I'm fine with pulling out a feature that
wasn't there in 8.0 --- if someone wants it, they can figure out how
to make it work properly and submit for 8.2 or later.

regards, tom lane

This has been fixed in current CVS:

<varname>krb_srvname</><literal>/</><varname>krb_server_hostname</><literal>@</>REALM.

---------------------------------------------------------------------------

Tom Lane wrote:

"Magnus Hagander" <mha@sollentuna.net> writes:

Here's a patch that fixes the big problem and reverts the behaviour of
appl_version to be compatible with 8.0.

Applied with trivial stylistic cleanups.

BTW, the documentation seems a bit broken:

krb_server_hostname (string)

Sets the hostname part of the service principal. This, combined
with krb_srvname, is used to generate the complete service
principal, i.e. krb_server_hostname/krb_server_hostname@REALM.

I suppose one of those last two should be "krb_srvname", but which?

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Magnus Hagander wrote:

Tom has already applied a fix for this:
http://archives.postgresql.org/pgsql-committers/2005-10/msg00114.php

Ah, I see now. Sorry I missed it.

---------------------------------------------------------------------------

//Magnus

I need a comment on this.

--------------------------------------------------------------
-------------

Tom Lane wrote:

"Magnus Hagander" <mha@sollentuna.net> writes:

Here's a patch that fixes the big problem and reverts the

behaviour

of appl_version to be compatible with 8.0.

Applied with trivial stylistic cleanups.

BTW, the documentation seems a bit broken:

krb_server_hostname (string)

Sets the hostname part of the service principal. This, combined
with krb_srvname, is used to generate the complete service
principal, i.e. krb_server_hostname/krb_server_hostname@REALM.

I suppose one of those last two should be "krb_srvname", but which?

regards, tom lane

---------------------------(end of
broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org

so that your

message can get through to the mailing list cleanly

-- 
Bruce Momjian                        |  http://candle.pha.pa.us
pgman@candle.pha.pa.us               |  (610) 359-1001
+  If your life is a hard drive,     |  13 Roberts Road
+  Christ can be your backup.        |  Newtown Square, 
Pennsylvania 19073

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Tom Lane wrote:

BTW, it appears to me that this patch has also broken the claim in the
manual that

If [krb_server_hostname is] not set, the default is to allow any
service principal matching an entry in the keytab.

The reason that was true was that we passed a NULL "server" value to
krb5_recvauth(), which with this patch we never do anymore.

I'm not sure if this represents a serious loss of flexibility or not,
but in any case the documentation needs an update.

I did some research on this and I think I have the answer. The original
patch came from here (I have CC'ed the author):

http://archives.postgresql.org/pgsql-patches/2005-06/msg00293.php

I applied his second patch. As part of that patch he states:

The second patch (kovert-krb5-patch-newbehavior.txt) makes the default
behavior to accept any principal in the keytab. This means that people
using kerberos will continue to work, but they'll be slightly more
broad in what they accept as a valid service principal (I suspect
there's very few people in the world who care about this since it still
needs to be something in the keytab).

Now, our code has been modified since his patch was applied, but we now
have:

/*
* If no hostname was specified, pg_krb_server_hostname is already
* NULL. If it's set to blank, force it to NULL.
*/
khostname = pg_krb_server_hostname;
if (khostname && khostname[0] == '\0')
khostname = NULL;

retval = krb5_sname_to_principal(pg_krb5_context,
khostname,
pg_krb_srvnam,
KRB5_NT_SRV_HST,
&pg_krb5_server);

The basic affect is if the GUC krb_server_hostname is empty/NULL,
krb5_sname_to_principal() gets called with a 2nd argument (hostname) of
NULL. The documentation for this function says for this argument:

http://publib.boulder.ibm.com/iseries/v5r1/ic2924/index.htm?info/apis/krb5list.htm

hostname (Input)

The host containing the desired service instance. The local host is used
if NULL is specified for this parameter.

Which says it doesn't accept any service entry in keytab, but rather
binds the server hostname to 'localhost'. I think this is why it wasn't
working for Magnus.

I have applied the following patch which updates the documentation to
reflect 'localhost', and improves the error message to always print the
server name as well as the service name. (We have had complaints about
poor Kerberos error messages before.)

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073