Bug 117214

Created attachment 98173 [details]
Patch to allow changes to the default domain to take effect

The patch is trivial -- just make the default_realm and its associated length
non-static.

I've now tested this in both Kerberos realms -- both at work and at home -- and
it works beautifully.	I don't think the slight loss of performance really
makes much difference.

Comment on attachment 98173 [details]
Patch to allow changes to the default domain to take effect

--- ./src/lib/krb5/krb/parse.c.realm-change	2004-03-02 16:54:34.563715147
+0000
+++ ./src/lib/krb5/krb/parse.c	2004-03-02 16:59:12.023582016 +0000
@@ -76,8 +76,8 @@
	const char	*parsed_realm = NULL;
	int		fcompsize[FCOMPNUM];
	int		realmsize = 0;
-	static char	*default_realm = NULL;
-	static int	default_realm_size = 0;
+	char		*default_realm = NULL;
+	int		default_realm_size = 0;
	char		*tmpdata;
	krb5_principal	principal;
	krb5_error_code retval;
@@ -215,6 +215,7 @@
	if (tmpdata == 0) {
		krb5_xfree(principal->data);
		krb5_xfree(principal);
+		krb5_xfree(default_realm);
		return ENOMEM;
	}
	krb5_princ_set_realm_length(context, principal, realmsize);
@@ -228,6 +229,7 @@
			krb5_xfree(krb5_princ_realm(context, principal)->data);
			krb5_xfree(principal->data);
			krb5_xfree(principal);
+			krb5_xfree(default_realm);
			return(ENOMEM);
		}
		krb5_princ_component(context, principal, i)->data = tmpdata;
@@ -272,6 +274,7 @@
	*q++ = '\0';
	if (!parsed_realm)
		strcpy(krb5_princ_realm(context, principal)->data,
default_realm);
+	krb5_xfree(default_realm);
	/*
	 * Alright, we're done.  Now stuff a pointer to this monstrosity
	 * into the return variable, and let's get out of here.

Created attachment 98198 [details]
Updated patch

(Sorry about the previous comment, I thought I was editing the attachment).

I like valgrind -- the previous version of the patch lost the storage for the
default_realm if it was acquired.   This patch corrects that.

I'm disappointed that this patch still hasn't made it into krb5; I'm
in the process of installing the beta and I've just been through the
problem of not being able to log in again because I forgot to rebuild
krb5 with the fix applied.

I've been using this patch since March and it hasn't given me any
problems at all.    Please include it in the RHEL3 U3.   Please!

This would change the behavior of krb5_parse_name() to conflict directly with
the API documentation, so I doubt upstream would go for it.  Rather, it looks to
me like the problem is squarely in pam_krb5.  Reassigning for tracking.

Oops, the docs use "local" and "default" to refer to the same realm.

I've spent an hour or two looking into my original fix.   Here's what I've found:

OK, so I'm looking at the comment for krb5_parse_name() in
src/lib/krb5/krb/parse.c (this is for krb5-1.3.4, not that it matters).  It
says, among other things:

 * get_default_realm() is called; it may return other errors.

It's lying, or at least being economical with the truth,
krb5_get_default_realm() is called just the once, the first time you call
krb_parse_name().

OK, so now in src/lib/krb5/os/def_realm.c we have krb5_get_default_realm() and
krb5_set_default_realm().  The comments say that get_default_realm() will get
the default realm from a config file, it actually only does that the first time,
subsequently it uses a static value (the one that set_default_realm()) sets
which rather makes parse_name()'s cache redundant.

The documentation for krb5_set_default_realm() says
-------------------
\begin{funcdecl}{krb5_set_default_realm}{krb5_error_code}{\funcin}
\funcarg{krb5_context}{context}
\funcarg{char *}{realm}
\end{funcdecl}

Sets the default realm to be used if no user-specified realm is
available (e.g. to interpret a user-typed principal name with the
realm omitted for convenience). (c.f. krb5_get_default_realm)

If \funcparam{realm} is NULL, then the operating system default value
will used.

Returns system errors.
------------------

It's interesting to note that it makes no mention of only being able to set the
default realm before the first call to krb5_parse_name() and it's this behaviour
that the current code enforces.   I'm sure that you can check doc/api/libos.tex
for yourself, but there's no similar sort of caveat for krb5_get_default_realm().


Interestingly in doc/api/krb5.tex, the documentation for krb5_parse_name() says
"If the realm is not specified, the local realm is used."   This is at odds with
the documentation for krb5_set_default_realm() (above) and with what the code
actually does.  Since the only reference (in the code or documentation) to a
function called get_local_realm() says that it returns a list of local realms
rather (which one would parse_name() use?) that does rather indicate that the
documentation should say "default" rather than "local".

Given all this, I  original fix still stands (and also applies to Kerberos 1.3,
slightly re-worked) and I would be amazed if it was rejected upstream.

My apologies for not being more verbose in comment 9. I'm agreeing with you.
Cloning for tracking against other releases.

An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on the solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.

http://rhn.redhat.com/errata/RHBA-2006-0404.html