Re: krb_match_realm

Thank you very much. This helps, but I'm still evaluating how much.

I *can* point at one problem though: you do a strchr(gbuf.value,
'@') and then error out if there isn't a Kerberos realm there. In
fact that is exactly the default username of at least one of the
GSSAPI implementations I've tested if the realm is the same as the
local default realm.

I'm not entirely sure what the intended semantics of krb_match_realm
are, but if you're trying to match the GSSAPI-authenticated name
against "value_of(PGUSER)@value_of(krb_match_realm)" then you need to
construct that string, gss_import_name() it, and then gss_compare_name
() the imported name with the authenticated name that GSSAPI already
gave you. I know the API overhead of doing that is a PITA, but
that's what's going to work.

I also notice you have some code to do case insensitive name
matching. I assume this is to take care of the fact that Microsoft
Kerberos does case insensitive name matching (contrary to the
standard and the other Kerberos implementations out there). I
suspect issues there, but it will be 3-6 months before I will have an
environment where I can easily test this. Most likely, the way to
handle this is by figuring out what case Microsoft uses for each name
inside the protocol and then pre-mapping to that case before feeding
things to (non-Microsoft) GSSAPI.

I don't regard the case mapping issues as serious. We may not have
the intended level of Windows/Unix compatibility, but I don't expect
other issues. In other words I'm not even going to think about it
until it's easy for me to investigate.

On Nov 1, 2007, at 5:27 AM, Magnus Hagander wrote:

> Attached patch implements krb_match_realm for krb5, gssapi and sspi
> per
> complaint from Henry. Comments welcome.
>
> Working on documentation which will of course be ready when it's
> committed :)
>
> Oh, and it changes the krb username handling to be the same as the
> gssapi one. I've never heard of anybody actually using the other
> version
> that it used to support, and the comment clearly states that it was
> broken for the really complex scenarios anyway - something nobody has
> complained about.

Well, *I* complained about it. ;-)

Sorry if I'm the only one, but I can't change the deployment
environment I'm going to have to deal with.

> //Magnus
> Index: backend/libpq/auth.c
> ===================================================================
> RCS file: /cvsroot/pgsql/src/backend/libpq/auth.c,v
> retrieving revision 1.156
> diff -c -r1.156 auth.c
> *** backend/libpq/auth.c 14 Sep 2007 15:58:02 -0000 1.156
> --- backend/libpq/auth.c 1 Nov 2007 11:28:54 -0000
> ***************
> *** 42,47 ****
> --- 42,48 ----
> char *pg_krb_srvnam;
> bool pg_krb_caseins_users;
> char *pg_krb_server_hostname = NULL;
> + char *pg_krb_match_realm = NULL;
>
> #ifdef USE_PAM
> #ifdef HAVE_PAM_PAM_APPL_H
> ***************
> *** 103,132 ****
> #endif
>
> /*
> - * pg_an_to_ln -- return the local name corresponding to an
> authentication
> - * name
> - *
> - * XXX Assumes that the first aname component is the user name.
> This is NOT
> - * necessarily so, since an aname can actually be something
> out of your
> - * worst X.400 nightmare, like
> - * ORGANIZATION=U. C. Berkeley/NAME=Paul M. Aoki(at)CS(dot)BERKELEY(dot)EDU
> - * Note that the MIT an_to_ln code does the same thing if you
> don't
> - * provide an aname mapping database...it may be a better idea
> to use
> - * krb5_an_to_ln, except that it punts if multiple components
> are found,
> - * and we can't afford to punt.
> - */
> - static char *
> - pg_an_to_ln(char *aname)
> - {
> - char *p;
> -
> - if ((p = strchr(aname, '/')) || (p = strchr(aname, '@')))
> - *p = '\0';
> - return aname;
> - }
> -
> -
> - /*
> * Various krb5 state which is not connection specfic, and a flag to
> * indicate whether we have initialised it yet.
> */
> --- 104,109 ----
> ***************
> *** 216,221 ****
> --- 193,199 ----
> krb5_auth_context auth_context = NULL;
> krb5_ticket *ticket;
> char *kusername;
> + char *cp;
>
> if (get_role_line(port->user_name) == NULL)
> return STATUS_ERROR;
> ***************
> *** 240,247 ****
> * The "client" structure comes out of the ticket and is therefore
> * authenticated. Use it to check the username obtained from the
> * postmaster startup packet.
> - *
> - * I have no idea why this is considered necessary.
> */
> #if defined(HAVE_KRB5_TICKET_ENC_PART2)
> retval = krb5_unparse_name(pg_krb5_context,
> --- 218,223 ----
> ***************
> *** 263,269 ****
> return STATUS_ERROR;
> }
>
> ! kusername = pg_an_to_ln(kusername);
> if (pg_krb_caseins_users)
> ret = pg_strncasecmp(port->user_name, kusername,
> SM_DATABASE_USER);
> else
> --- 239,280 ----
> return STATUS_ERROR;
> }
>
> ! cp = strchr(kusername, '@');
> ! if (cp)
> ! {
> ! *cp = '\0';
> ! cp++;
> !
> ! if (pg_krb_match_realm != NULL && strlen(pg_krb_match_realm))
> ! {
> ! /* Match realm against configured */
> ! if (pg_krb_caseins_users)
> ! ret = pg_strcasecmp(pg_krb_match_realm, cp);
> ! else
> ! ret = strcmp(pg_krb_match_realm, cp);
> !
> ! if (ret)
> ! {
> ! elog(DEBUG2,
> ! "krb5 realm (%s) and configured realm (%s) don't match",
> ! cp, pg_krb_match_realm);
> !
> ! krb5_free_ticket(pg_krb5_context, ticket);
> ! krb5_auth_con_free(pg_krb5_context, auth_context);
> ! return STATUS_ERROR;
> ! }
> ! }
> ! }
> ! else if (pg_krb_match_realm && strlen(pg_krb_match_realm))

I assume this case is in the existing kerb5 method.

> ! {
> ! elog(DEBUG2,
> ! "krb5 did not return realm but realm matching was requested");
> !
> ! krb5_free_ticket(pg_krb5_context, ticket);
> ! krb5_auth_con_free(pg_krb5_context, auth_context);
> ! return STATUS_ERROR;
> ! }
> !
> if (pg_krb_caseins_users)
> ret = pg_strncasecmp(port->user_name, kusername,
> SM_DATABASE_USER);
> else
> ***************
> *** 509,522 ****
> maj_stat, min_stat);
>
> /*
> ! * Compare the part of the username that comes before the @
> ! * sign only (ignore realm). The GSSAPI libraries won't have
> ! * authenticated the user if he's from an invalid realm.
> */
> if (strchr(gbuf.value, '@'))
> {
> char *cp = strchr(gbuf.value, '@');
> *cp = '\0';
> }
>
> if (pg_krb_caseins_users)
> --- 520,561 ----
> maj_stat, min_stat);
>
> /*
> ! * Split the username at the realm separator
> */
> if (strchr(gbuf.value, '@'))
> {
> char *cp = strchr(gbuf.value, '@');
> *cp = '\0';
> + cp++;
> +
> + if (pg_krb_match_realm != NULL && strlen(pg_krb_match_realm))
> + {
> + /*
> + * Match the realm part of the name first
> + */
> + if (pg_krb_caseins_users)
> + ret = pg_strcasecmp(pg_krb_match_realm, cp);
> + else
> + ret = strcmp(pg_krb_match_realm, cp);
> +
> + if (ret)
> + {
> + /* GSS realm does not match */
> + elog(DEBUG2,
> + "GSSAPI realm (%s) and configured realm (%s) don't match",
> + cp, pg_krb_match_realm);
> + gss_release_buffer(&lmin_s, &gbuf);
> + return STATUS_ERROR;
> + }
> + }
> + }
> + else if (pg_krb_match_realm && strlen(pg_krb_match_realm))
> + {
> + elog(DEBUG2,
> + "GSSAPI did not return realm but realm matching was requested");
> +

Most likely reason is that the realm is the same as the local default
realm. This shouldn't be an error.

> + gss_release_buffer(&lmin_s, &gbuf);
> + return STATUS_ERROR;
> }
>
> if (pg_krb_caseins_users)
> ***************
> *** 792,797 ****
> --- 831,851 ----
>
> free(tokenuser);
>
> + /*
> + * Compare realm/domain if requested. In SSPI, always compare
> case insensitive.
> + */
> + if (pg_krb_match_realm && strlen(pg_krb_match_realm))
> + {
> + if (pg_strcasecmp(pg_krb_match_realm, domainname))
> + {
> + elog(DEBUG2,
> + "SSPI domain (%s) does and configured domain (%s) don't match",
> + domainname, pg_krb_match_realm);
> +
> + return STATUS_ERROR;
> + }
> + }
> +
> /*
> * We have the username (without domain/realm) in accountname,
> compare
> * to the supplied value. In SSPI, always compare case insensitive.
> Index: backend/utils/misc/guc.c
> ===================================================================
> RCS file: /cvsroot/pgsql/src/backend/utils/misc/guc.c,v
> retrieving revision 1.423
> diff -c -r1.423 guc.c
> *** backend/utils/misc/guc.c 26 Sep 2007 22:36:30 -0000 1.423
> --- backend/utils/misc/guc.c 1 Nov 2007 11:28:54 -0000
> ***************
> *** 2044,2049 ****
> --- 2044,2059 ----
> },
>
> {
> + {"krb_match_realm", PGC_POSTMASTER, CONN_AUTH_SECURITY,
> + gettext_noop("Sets realm to match Kerberos and GSSAPI users
> against."),
> + NULL,
> + GUC_SUPERUSER_ONLY
> + },
> + &pg_krb_match_realm,
> + NULL, NULL, NULL
> + },
> +
> + {
> {"krb_server_keyfile", PGC_POSTMASTER, CONN_AUTH_SECURITY,
> gettext_noop("Sets the location of the Kerberos server key
> file."),
> NULL,
> Index: backend/utils/misc/postgresql.conf.sample
> ===================================================================
> RCS file: /cvsroot/pgsql/src/backend/utils/misc/
> postgresql.conf.sample,v
> retrieving revision 1.231
> diff -c -r1.231 postgresql.conf.sample
> *** backend/utils/misc/postgresql.conf.sample 26 Sep 2007 22:36:30
> -0000 1.231
> --- backend/utils/misc/postgresql.conf.sample 1 Nov 2007 11:28:54
> -0000
> ***************
> *** 85,90 ****
> --- 85,91 ----
> #krb_server_hostname = '' # empty string matches any keytab entry
> # (change requires restart, kerberos only)
> #krb_caseins_users = off # (change requires restart)
> + #krb_match_realm = '' # (change requires restart)
>
> # - TCP Keepalives -
> # see 'man 7 tcp' for details
> Index: include/libpq/auth.h
> ===================================================================
> RCS file: /cvsroot/pgsql/src/include/libpq/auth.h,v
> retrieving revision 1.33
> diff -c -r1.33 auth.h
> *** include/libpq/auth.h 5 Jan 2007 22:19:55 -0000 1.33
> --- include/libpq/auth.h 1 Nov 2007 11:28:54 -0000
> ***************
> *** 20,25 ****
> --- 20,26 ----
> extern char *pg_krb_srvnam;
> extern bool pg_krb_caseins_users;
> extern char *pg_krb_server_hostname;
> + extern char *pg_krb_match_realm;
>
> extern void ClientAuthentication(Port *port);
>