From: Alex Dubov <[email protected]>
There appear to be only 3 issues remaining with Heimdal after removal of
compulsory dependency on libgssglue:
1. On some systems, only libroken.so is available (small fix to kerberos5.m4)
2. krb5_util.c:check_for_target - Heimdal variant constructs a "pattern"
principal and uses krb5_cc_retrieve_cred to get a matching credential.
This should work on mit-krb5, so old method of iterating over every
credential in cache may possibly be dropped outright and "#$if" guard
omitted.
For the sake of the above I reformatted the old approach to make it a bit
more clear what's going on there.
3. krb5_util.c:gssd_k5_err_msg - krb5_get_err_text is marked as deprecated,
at least on Heimdal. If krb5_get_error_message is available, it should not
be reached at all, thus "#elif" guard.
Signed-off-by: Alex Dubov <[email protected]>
---
aclocal/kerberos5.m4 | 7 +++--
utils/gssd/krb5_util.c | 55 ++++++++++++++++++++++++++++++++++++-----------
2 files changed, 46 insertions(+), 16 deletions(-)
diff --git a/aclocal/kerberos5.m4 b/aclocal/kerberos5.m4
index 0bf35d3..ebf6f20 100644
--- a/aclocal/kerberos5.m4
+++ b/aclocal/kerberos5.m4
@@ -56,9 +56,10 @@ AC_DEFUN([AC_KERBEROS_V5],[
break
dnl The following ugly hack brought on by the split installation
dnl of Heimdal Kerberos on SuSe
- elif test \( -f $dir/include/heim_err.h -o\
- -f $dir/include/heimdal/heim_err.h \) -a \
- -f $dir/lib/libroken.a; then
+ elif test \( \( -f $dir/include/heim_err.h -o\
+ -f $dir/include/heimdal/heim_err.h \) -a \
+ \( -f $dir/lib/libroken.a -o\
+ -f $dir/lib/libroken.so \) \) ; then
AC_DEFINE(HAVE_HEIMDAL, 1, [Define this if you have Heimdal Kerberos libraries])
KRBDIR="$dir"
gssapi_lib=gssapi
diff --git a/utils/gssd/krb5_util.c b/utils/gssd/krb5_util.c
index 20b55b3..adef268 100644
--- a/utils/gssd/krb5_util.c
+++ b/utils/gssd/krb5_util.c
@@ -958,29 +958,57 @@ check_for_tgt(krb5_context context, krb5_ccache ccache,
{
krb5_error_code ret;
krb5_creds creds;
- krb5_cc_cursor cur;
int found = 0;
+#if HAVE_HEIMDAL
+ krb5_creds pattern;
+ krb5_const_realm client_realm;
+
+ krb5_cc_clear_mcred(&pattern);
+
+ client_realm = krb5_principal_get_realm(context, principal);
+
+ ret = krb5_make_principal(context, &pattern.server,
+ client_realm, KRB5_TGS_NAME, client_realm,
+ NULL);
+ if (ret) {
+ krb5_err(context, 1, ret, "krb5_make_principal");
+ return 0;
+ }
+
+ pattern.client = principal;
+
+ ret = krb5_cc_retrieve_cred(context, ccache, 0, &pattern, &creds);
+ krb5_free_principal(context, pattern.server);
+
+ if (ret) {
+ if (ret != KRB5_CC_END)
+ krb5_err(context, 1, ret, "krb5_cc_retrieve_cred");
+ } else
+ found = creds.times.endtime > time(NULL);
+
+ krb5_free_cred_contents(context, &creds);
+#else
+ krb5_cc_cursor cur;
ret = krb5_cc_start_seq_get(context, ccache, &cur);
if (ret)
return 0;
while (!found &&
(ret = krb5_cc_next_cred(context, ccache, &cur, &creds)) == 0) {
- if (creds.server->length == 2 &&
- data_is_equal(creds.server->realm,
- principal->realm) &&
- creds.server->data[0].length == 6 &&
- memcmp(creds.server->data[0].data,
- "krbtgt", 6) == 0 &&
- data_is_equal(creds.server->data[1],
- principal->realm) &&
- creds.times.endtime > time(NULL))
- found = 1;
+ if (
+ creds.server->length == 2
+ && data_is_equal(creds.server->realm, principal->realm)
+ && creds.server->data[0].length == 6
+ && memcmp(creds.server->data[0].data, "krbtgt", 6) == 0
+ && data_is_equal(creds.server->data[1],
+ principal->realm)
+ && creds.times.endtime > time(NULL)
+ ) found = 1;
krb5_free_cred_contents(context, &creds);
}
krb5_cc_end_seq_get(context, ccache, &cur);
-
+#endif
return found;
}
@@ -1326,12 +1354,13 @@ gssd_k5_err_msg(krb5_context context, krb5_error_code code)
return msg;
#if HAVE_KRB5
return strdup(error_message(code));
-#else
+#elif !HAVE_KRB5_GET_ERROR_MESSAGE
if (context != NULL)
return strdup(krb5_get_err_text(context, code));
else
return strdup(error_message(code));
#endif
+ return NULL;
}
/*
--
1.7.4.5
On Fri, 2013-03-29 at 20:48 +1100, [email protected] wrote:
> From: Alex Dubov <[email protected]>
>
> There appear to be only 3 issues remaining with Heimdal after removal of
> compulsory dependency on libgssglue:
>
> 1. On some systems, only libroken.so is available (small fix to kerberos5.m4)
>
> 2. krb5_util.c:check_for_target - Heimdal variant constructs a "pattern"
> principal and uses krb5_cc_retrieve_cred to get a matching credential.
> This should work on mit-krb5, so old method of iterating over every
> credential in cache may possibly be dropped outright and "#$if" guard
> omitted.
> For the sake of the above I reformatted the old approach to make it a bit
> more clear what's going on there.
>
> 3. krb5_util.c:gssd_k5_err_msg - krb5_get_err_text is marked as deprecated,
> at least on Heimdal. If krb5_get_error_message is available, it should not
> be reached at all, thus "#elif" guard.
>
> Signed-off-by: Alex Dubov <[email protected]>
> ---
> aclocal/kerberos5.m4 | 7 +++--
> utils/gssd/krb5_util.c | 55 ++++++++++++++++++++++++++++++++++++-----------
> 2 files changed, 46 insertions(+), 16 deletions(-)
>
> diff --git a/aclocal/kerberos5.m4 b/aclocal/kerberos5.m4
> index 0bf35d3..ebf6f20 100644
> --- a/aclocal/kerberos5.m4
> +++ b/aclocal/kerberos5.m4
> @@ -56,9 +56,10 @@ AC_DEFUN([AC_KERBEROS_V5],[
> break
> dnl The following ugly hack brought on by the split installation
> dnl of Heimdal Kerberos on SuSe
> - elif test \( -f $dir/include/heim_err.h -o\
> - -f $dir/include/heimdal/heim_err.h \) -a \
> - -f $dir/lib/libroken.a; then
> + elif test \( \( -f $dir/include/heim_err.h -o\
> + -f $dir/include/heimdal/heim_err.h \) -a \
> + \( -f $dir/lib/libroken.a -o\
> + -f $dir/lib/libroken.so \) \) ; then
> AC_DEFINE(HAVE_HEIMDAL, 1, [Define this if you have Heimdal Kerberos libraries])
> KRBDIR="$dir"
> gssapi_lib=gssapi
This hunk looks good.
> diff --git a/utils/gssd/krb5_util.c b/utils/gssd/krb5_util.c
> index 20b55b3..adef268 100644
> --- a/utils/gssd/krb5_util.c
> +++ b/utils/gssd/krb5_util.c
> @@ -958,29 +958,57 @@ check_for_tgt(krb5_context context, krb5_ccache ccache,
> {
> krb5_error_code ret;
> krb5_creds creds;
> - krb5_cc_cursor cur;
> int found = 0;
>
> +#if HAVE_HEIMDAL
> + krb5_creds pattern;
> + krb5_const_realm client_realm;
> +
> + krb5_cc_clear_mcred(&pattern);
> +
> + client_realm = krb5_principal_get_realm(context, principal);
> +
> + ret = krb5_make_principal(context, &pattern.server,
> + client_realm, KRB5_TGS_NAME, client_realm,
> + NULL);
> + if (ret) {
> + krb5_err(context, 1, ret, "krb5_make_principal");
> + return 0;
> + }
> +
> + pattern.client = principal;
> +
> + ret = krb5_cc_retrieve_cred(context, ccache, 0, &pattern, &creds);
> + krb5_free_principal(context, pattern.server);
> +
> + if (ret) {
> + if (ret != KRB5_CC_END)
> + krb5_err(context, 1, ret, "krb5_cc_retrieve_cred");
> + } else
> + found = creds.times.endtime > time(NULL);
> +
> + krb5_free_cred_contents(context, &creds);
> +#else
> + krb5_cc_cursor cur;
> ret = krb5_cc_start_seq_get(context, ccache, &cur);
> if (ret)
> return 0;
krb5_cc_retrieve_cred is available in MIT too, I would try to avoid
using ifedfs here and come up with common code that works for both to
avoid bugs that may cause different behavior in parallel code paths
depending on which library is used.
> while (!found &&
> (ret = krb5_cc_next_cred(context, ccache, &cur, &creds)) == 0) {
> - if (creds.server->length == 2 &&
> - data_is_equal(creds.server->realm,
> - principal->realm) &&
> - creds.server->data[0].length == 6 &&
> - memcmp(creds.server->data[0].data,
> - "krbtgt", 6) == 0 &&
> - data_is_equal(creds.server->data[1],
> - principal->realm) &&
> - creds.times.endtime > time(NULL))
> - found = 1;
> + if (
> + creds.server->length == 2
> + && data_is_equal(creds.server->realm, principal->realm)
> + && creds.server->data[0].length == 6
> + && memcmp(creds.server->data[0].data, "krbtgt", 6) == 0
> + && data_is_equal(creds.server->data[1],
> + principal->realm)
> + && creds.times.endtime > time(NULL)
> + ) found = 1;
This looks like just gratuitous re-indentation, I would drop it it's not
needed and makes the patch bigger.
> krb5_free_cred_contents(context, &creds);
> }
> krb5_cc_end_seq_get(context, ccache, &cur);
> -
> +#endif
> return found;
> }
>
> @@ -1326,12 +1354,13 @@ gssd_k5_err_msg(krb5_context context, krb5_error_code code)
> return msg;
> #if HAVE_KRB5
> return strdup(error_message(code));
> -#else
> +#elif !HAVE_KRB5_GET_ERROR_MESSAGE
> if (context != NULL)
> return strdup(krb5_get_err_text(context, code));
> else
> return strdup(error_message(code));
> #endif
> + return NULL;
MIT doesn't have krb5_get_err_text at all so it shouldn't be tried out
at all if MIT is available.
I think you should ratrher add a keberos.m4 macro to check for
krb5_get_err_text then change this to something like:
char *msg = NULL;
#if HAVE_KRB5_GET_ERROR_MESSAGE
... <- same code as in the current tree
#elif HAVE_KRB_GET_ERR_TEXT
... <- code in current else branch of 'if HAVE_KRB5'
#else
msg = strdup(error_message(code));
#endif
return msg;
> }
>
> /*
HTH,
Simo.
--
Simo Sorce * Red Hat, Inc * New York
----- Original Message -----
> From: Simo Sorce <[email protected]>
>
>
> krb5_cc_retrieve_cred is available in MIT too, I would try to avoid
> using ifedfs here and come up with common code that works for both to
> avoid bugs that may cause different behavior in parallel code paths
> depending on which library is used.
>
> This looks like just gratuitous re-indentation, I would drop it it's not
> needed and makes the patch bigger.
I did it on purpose and said so in the patch description header, because
I knew you are going to optimize it out anyway. I don't think that
iterating through every credential was ever considered a proper way to
implement check_for_taregt function.
>
> MIT doesn't have krb5_get_err_text at all so it shouldn't be tried out
> at all if MIT is available.
>
> I think you should ratrher add a keberos.m4 macro to check for
> krb5_get_err_text then change this to something like:
If it's not available on mit-krb5 and is deprecated on Heimdal, why bother
with krb5_get_err_text at all?