URL: https://github.com/SSSD/sssd/pull/883
Author: alexey-tikhonov
Title: #883: Minor fixes to util/sss_krb5
Action: synchronized
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/883/head:pr883
git checkout pr883
From 473ddf2cc3991a912681b28eebaedfa3c2ebc71a Mon Sep 17 00:00:00 2001
From: Alexey Tikhonov <atikh...@redhat.com>
Date: Tue, 3 Sep 2019 17:58:20 +0200
Subject: [PATCH 1/4] util/sss_krb5.c: elimination of unreachable code
It was wrong to check `kt_err` after
```
if (!principal_found) {
...
goto done;
}
```
since getting to this point of code would mean `kt_err` equals to 0 and
thus statement inside `if (kt_err != 0) ...` was unreachable.
Moreover it was logical error to do `goto done;` inside this statement
without setting `kerr`.
---
src/util/sss_krb5.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/src/util/sss_krb5.c b/src/util/sss_krb5.c
index c0cc28a75b..0086fca7c3 100644
--- a/src/util/sss_krb5.c
+++ b/src/util/sss_krb5.c
@@ -395,16 +395,16 @@ krb5_error_code find_principal_in_keytab(krb5_context ctx,
}
if (!principal_found) {
- kerr = KRB5_KT_NOTFOUND;
- DEBUG(SSSDBG_TRACE_FUNC,
- "No principal matching %s@%s found in keytab.\n",
- pattern_primary, pattern_realm);
- goto done;
- }
-
- /* check if we got any errors from krb5_kt_next_entry */
- if (kt_err != 0 && kt_err != KRB5_KT_END) {
- DEBUG(SSSDBG_CRIT_FAILURE, "Error while reading keytab.\n");
+ /* If principal was not found then 'kt_err' was set */
+ if (kt_err != KRB5_KT_END) {
+ kerr = kt_err;
+ DEBUG(SSSDBG_CRIT_FAILURE, "Error while reading keytab.\n");
+ } else {
+ kerr = KRB5_KT_NOTFOUND;
+ DEBUG(SSSDBG_TRACE_FUNC,
+ "No principal matching %s@%s found in keytab.\n",
+ pattern_primary, pattern_realm);
+ }
goto done;
}
@@ -413,7 +413,6 @@ krb5_error_code find_principal_in_keytab(krb5_context ctx,
DEBUG(SSSDBG_CRIT_FAILURE, "krb5_copy_principal failed.\n");
goto done;
}
-
kerr = 0;
done:
From 5cb8da80acf87db6f40c73a39f7366e99f0a3e02 Mon Sep 17 00:00:00 2001
From: Alexey Tikhonov <atikh...@redhat.com>
Date: Tue, 3 Sep 2019 19:02:33 +0200
Subject: [PATCH 2/4] util/sss_krb5: find_principal_in_keytab() was amended
- do not treat failure of krb5_kt_end_seq_get() as an fatal error
- avoid calling sss_krb5_free_keytab_entry_contents(null_struct)
as libkrb5 api docs do not specify explicitly behaviour in this case
---
src/util/sss_krb5.c | 47 +++++++++++++++++++--------------------------
1 file changed, 20 insertions(+), 27 deletions(-)
diff --git a/src/util/sss_krb5.c b/src/util/sss_krb5.c
index 0086fca7c3..f2850d1201 100644
--- a/src/util/sss_krb5.c
+++ b/src/util/sss_krb5.c
@@ -356,13 +356,14 @@ krb5_error_code find_principal_in_keytab(krb5_context ctx,
krb5_principal *princ)
{
krb5_error_code kerr;
- krb5_error_code kt_err;
- krb5_error_code kerr_d;
+ krb5_error_code kerr_dbg;
krb5_kt_cursor cursor;
krb5_keytab_entry entry;
bool principal_found = false;
memset(&cursor, 0, sizeof(cursor));
+ memset(&entry, 0, sizeof(entry));
+
kerr = krb5_kt_start_seq_get(ctx, keytab, &cursor);
if (kerr != 0) {
DEBUG(SSSDBG_CRIT_FAILURE, "krb5_kt_start_seq_get failed.\n");
@@ -371,15 +372,14 @@ krb5_error_code find_principal_in_keytab(krb5_context ctx,
DEBUG(SSSDBG_TRACE_ALL,
"Trying to find principal %s@%s in keytab.\n", pattern_primary, pattern_realm);
- memset(&entry, 0, sizeof(entry));
- while ((kt_err = krb5_kt_next_entry(ctx, keytab, &entry, &cursor)) == 0) {
+ while ((kerr = krb5_kt_next_entry(ctx, keytab, &entry, &cursor)) == 0) {
principal_found = match_principal(ctx, entry.principal, pattern_primary, pattern_realm);
if (principal_found) {
break;
}
- kerr = sss_krb5_free_keytab_entry_contents(ctx, &entry);
- if (kerr != 0) {
+ kerr_dbg = sss_krb5_free_keytab_entry_contents(ctx, &entry);
+ if (kerr_dbg != 0) {
DEBUG(SSSDBG_CRIT_FAILURE, "Failed to free keytab entry.\n");
}
memset(&entry, 0, sizeof(entry));
@@ -388,16 +388,23 @@ krb5_error_code find_principal_in_keytab(krb5_context ctx,
/* Close the keytab here. Even though we're using cursors, the file
* handle is stored in the krb5_keytab structure, and it gets
* overwritten by other keytab calls, creating a leak. */
- kerr = krb5_kt_end_seq_get(ctx, keytab, &cursor);
- if (kerr != 0) {
+ kerr_dbg = krb5_kt_end_seq_get(ctx, keytab, &cursor);
+ if (kerr_dbg != 0) {
DEBUG(SSSDBG_CRIT_FAILURE, "krb5_kt_end_seq_get failed.\n");
- goto done;
}
- if (!principal_found) {
- /* If principal was not found then 'kt_err' was set */
- if (kt_err != KRB5_KT_END) {
- kerr = kt_err;
+ if (principal_found) {
+ kerr = krb5_copy_principal(ctx, entry.principal, princ);
+ if (kerr != 0) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "krb5_copy_principal failed.\n");
+ }
+ kerr_dbg = sss_krb5_free_keytab_entry_contents(ctx, &entry);
+ if (kerr_dbg != 0) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "Failed to free keytab entry.\n");
+ }
+ } else {
+ /* If principal was not found then 'kerr' was set */
+ if (kerr != KRB5_KT_END) {
DEBUG(SSSDBG_CRIT_FAILURE, "Error while reading keytab.\n");
} else {
kerr = KRB5_KT_NOTFOUND;
@@ -405,20 +412,6 @@ krb5_error_code find_principal_in_keytab(krb5_context ctx,
"No principal matching %s@%s found in keytab.\n",
pattern_primary, pattern_realm);
}
- goto done;
- }
-
- kerr = krb5_copy_principal(ctx, entry.principal, princ);
- if (kerr != 0) {
- DEBUG(SSSDBG_CRIT_FAILURE, "krb5_copy_principal failed.\n");
- goto done;
- }
- kerr = 0;
-
-done:
- kerr_d = sss_krb5_free_keytab_entry_contents(ctx, &entry);
- if (kerr_d != 0) {
- DEBUG(SSSDBG_CRIT_FAILURE, "Failed to free keytab entry.\n");
}
return kerr;
From c108309069e5fc7dd1322597569d0cb5bcbdf820 Mon Sep 17 00:00:00 2001
From: Alexey Tikhonov <atikh...@redhat.com>
Date: Tue, 3 Sep 2019 19:58:25 +0200
Subject: [PATCH 3/4] util/sss_krb5: fixed few memory handling issues
---
src/util/sss_krb5.c | 25 +++++++++++++------------
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/src/util/sss_krb5.c b/src/util/sss_krb5.c
index f2850d1201..89e13ff504 100644
--- a/src/util/sss_krb5.c
+++ b/src/util/sss_krb5.c
@@ -126,10 +126,12 @@ errno_t select_principal_from_keytab(TALLOC_CTX *mem_ctx,
kerr = krb5_kt_default(krb_ctx, &keytab);
}
if (kerr) {
+ const char *krb5_err_msg = sss_krb5_get_error_message(krb_ctx, kerr);
DEBUG(SSSDBG_FATAL_FAILURE,
"Failed to read keytab [%s]: %s\n",
KEYTAB_CLEAN_NAME,
- sss_krb5_get_error_message(krb_ctx, kerr));
+ (krb5_err_msg ? krb5_err_msg : "- no error message available -"));
+ sss_krb5_free_error_message(krb_ctx, krb5_err_msg);
ret = EFAULT;
goto done;
}
@@ -187,7 +189,7 @@ errno_t select_principal_from_keytab(TALLOC_CTX *mem_ctx,
}
*_principal = talloc_strdup(mem_ctx, principal_string);
- free(principal_string);
+ sss_krb5_free_unparsed_name(krb_ctx, principal_string);
if (!*_principal) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_strdup failed\n");
ret = ENOMEM;
@@ -207,7 +209,7 @@ errno_t select_principal_from_keytab(TALLOC_CTX *mem_ctx,
}
*_primary = talloc_strdup(mem_ctx, principal_string);
- free(principal_string);
+ sss_krb5_free_unparsed_name(krb_ctx, principal_string);
if (!*_primary) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_strdup failed\n");
if (_principal) talloc_zfree(*_principal);
@@ -446,7 +448,9 @@ const char *KRB5_CALLCONV sss_krb5_get_error_message(krb5_context ctx,
void KRB5_CALLCONV sss_krb5_free_error_message(krb5_context ctx, const char *s)
{
#ifdef HAVE_KRB5_GET_ERROR_MESSAGE
- krb5_free_error_message(ctx, s);
+ if (s != NULL) {
+ krb5_free_error_message(ctx, s);
+ }
#else
free(s);
#endif
@@ -486,7 +490,9 @@ void KRB5_CALLCONV sss_krb5_get_init_creds_opt_free (krb5_context context,
void KRB5_CALLCONV sss_krb5_free_unparsed_name(krb5_context context, char *name)
{
#ifdef HAVE_KRB5_FREE_UNPARSED_NAME
- krb5_free_unparsed_name(context, name);
+ if (name != NULL) {
+ krb5_free_unparsed_name(context, name);
+ }
#else
if (name != NULL) {
memset(name, 0, strlen(name));
@@ -798,21 +804,16 @@ void sss_krb5_princ_realm(krb5_context context, krb5_const_principal princ,
}
#endif
-#ifdef HAVE_KRB5_FREE_KEYTAB_ENTRY_CONTENTS
krb5_error_code
sss_krb5_free_keytab_entry_contents(krb5_context context,
krb5_keytab_entry *entry)
{
+#ifdef HAVE_KRB5_FREE_KEYTAB_ENTRY_CONTENTS
return krb5_free_keytab_entry_contents(context, entry);
-}
#else
-krb5_error_code
-sss_krb5_free_keytab_entry_contents(krb5_context context,
- krb5_keytab_entry *entry)
-{
return krb5_kt_free_entry(context, entry);
-}
#endif
+}
#ifdef HAVE_KRB5_SET_TRACE_CALLBACK
From 3fc830421bd25ed67badd50e8601d1c090902d9d Mon Sep 17 00:00:00 2001
From: Alexey Tikhonov <atikh...@redhat.com>
Date: Thu, 12 Sep 2019 19:23:30 +0200
Subject: [PATCH 4/4] util/sss_krb5: debug messages fixes
select_principal_from_keytab() and find_principal_in_keytab() were changed
to output more clear error messages.
Resolves: https://pagure.io/SSSD/sssd/issue/4081
---
Makefile.am | 1 +
src/util/sss_krb5.c | 44 +++++++++++++++++++++---------------------
src/util/util_errors.c | 1 +
src/util/util_errors.h | 1 +
4 files changed, 25 insertions(+), 22 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index ed3a983539..71761ab57e 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -4615,6 +4615,7 @@ ldap_child_SOURCES = \
src/util/util_ext.c \
src/util/signal.c \
src/util/become_user.c \
+ src/util/util_errors.c \
$(NULL)
ldap_child_CFLAGS = \
$(AM_CFLAGS) \
diff --git a/src/util/sss_krb5.c b/src/util/sss_krb5.c
index 89e13ff504..e13018c559 100644
--- a/src/util/sss_krb5.c
+++ b/src/util/sss_krb5.c
@@ -27,6 +27,7 @@
#include "util/sss_iobuf.h"
#include "util/util.h"
+#include "util/util_errors.h"
#include "util/sss_krb5.h"
static char *
@@ -86,6 +87,7 @@ errno_t select_principal_from_keytab(TALLOC_CTX *mem_ctx,
char *principal_string;
const char *realm_name;
int realm_len;
+ const char *error_message = NULL;
/**
* The %s conversion is passed as-is, the %S conversion is translated to
@@ -115,7 +117,7 @@ errno_t select_principal_from_keytab(TALLOC_CTX *mem_ctx,
kerr = sss_krb5_init_context(&krb_ctx);
if (kerr) {
- DEBUG(SSSDBG_OP_FAILURE, "Failed to init Kerberos context\n");
+ error_message = "Failed to init Kerberos context";
ret = EFAULT;
goto done;
}
@@ -127,10 +129,8 @@ errno_t select_principal_from_keytab(TALLOC_CTX *mem_ctx,
}
if (kerr) {
const char *krb5_err_msg = sss_krb5_get_error_message(krb_ctx, kerr);
- DEBUG(SSSDBG_FATAL_FAILURE,
- "Failed to read keytab [%s]: %s\n",
- KEYTAB_CLEAN_NAME,
- (krb5_err_msg ? krb5_err_msg : "- no error message available -"));
+ error_message = (krb5_err_msg ?
+ talloc_strdup(tmp_ctx, krb5_err_msg) : NULL);
sss_krb5_free_error_message(krb_ctx, krb5_err_msg);
ret = EFAULT;
goto done;
@@ -183,15 +183,14 @@ errno_t select_principal_from_keytab(TALLOC_CTX *mem_ctx,
if (_principal) {
kerr = krb5_unparse_name(krb_ctx, client_princ, &principal_string);
if (kerr) {
- DEBUG(SSSDBG_CRIT_FAILURE, "krb5_unparse_name failed\n");
- ret = EFAULT;
+ error_message = "krb5_unparse_name failed (_principal)";
+ ret = EINVAL;
goto done;
}
*_principal = talloc_strdup(mem_ctx, principal_string);
sss_krb5_free_unparsed_name(krb_ctx, principal_string);
if (!*_principal) {
- DEBUG(SSSDBG_CRIT_FAILURE, "talloc_strdup failed\n");
ret = ENOMEM;
goto done;
}
@@ -203,15 +202,15 @@ errno_t select_principal_from_keytab(TALLOC_CTX *mem_ctx,
KRB5_PRINCIPAL_UNPARSE_NO_REALM,
&principal_string);
if (kerr) {
- DEBUG(SSSDBG_CRIT_FAILURE, "krb5_unparse_name failed\n");
- ret = EFAULT;
+ if (_principal) talloc_zfree(*_principal);
+ error_message = "krb5_unparse_name failed (_primary)";
+ ret = EINVAL;
goto done;
}
*_primary = talloc_strdup(mem_ctx, principal_string);
sss_krb5_free_unparsed_name(krb_ctx, principal_string);
if (!*_primary) {
- DEBUG(SSSDBG_CRIT_FAILURE, "talloc_strdup failed\n");
if (_principal) talloc_zfree(*_principal);
ret = ENOMEM;
goto done;
@@ -224,7 +223,7 @@ errno_t select_principal_from_keytab(TALLOC_CTX *mem_ctx,
&realm_name,
&realm_len);
if (realm_len == 0) {
- DEBUG(SSSDBG_CRIT_FAILURE, "sss_krb5_princ_realm failed.\n");
+ error_message = "sss_krb5_princ_realm failed";
if (_principal) talloc_zfree(*_principal);
if (_primary) talloc_zfree(*_primary);
ret = EINVAL;
@@ -234,7 +233,6 @@ errno_t select_principal_from_keytab(TALLOC_CTX *mem_ctx,
*_realm = talloc_asprintf(mem_ctx, "%.*s",
realm_len, realm_name);
if (!*_realm) {
- DEBUG(SSSDBG_CRIT_FAILURE, "talloc_asprintf failed\n");
if (_principal) talloc_zfree(*_principal);
if (_primary) talloc_zfree(*_primary);
ret = ENOMEM;
@@ -245,23 +243,22 @@ errno_t select_principal_from_keytab(TALLOC_CTX *mem_ctx,
ret = EOK;
} else {
- DEBUG(SSSDBG_MINOR_FAILURE, "No suitable principal found in keytab\n");
- ret = ENOENT;
+ ret = ERR_KRB5_PRINCIPAL_NOT_FOUND;
}
done:
if (ret != EOK) {
DEBUG(SSSDBG_FATAL_FAILURE, "Failed to read keytab [%s]: %s\n",
- KEYTAB_CLEAN_NAME, strerror(ret));
+ KEYTAB_CLEAN_NAME,
+ (error_message ? error_message : sss_strerror(ret)));
+
sss_log(SSS_LOG_ERR, "Failed to read keytab [%s]: %s\n",
- KEYTAB_CLEAN_NAME, strerror(ret));
+ KEYTAB_CLEAN_NAME,
+ (error_message ? error_message : sss_strerror(ret)));
}
if (keytab) krb5_kt_close(krb_ctx, keytab);
if (krb_ctx) krb5_free_context(krb_ctx);
- if (client_princ != NULL) {
- krb5_free_principal(krb_ctx, client_princ);
- client_princ = NULL;
- }
+ if (client_princ) krb5_free_principal(krb_ctx, client_princ);
talloc_free(tmp_ctx);
return ret;
}
@@ -368,7 +365,10 @@ krb5_error_code find_principal_in_keytab(krb5_context ctx,
kerr = krb5_kt_start_seq_get(ctx, keytab, &cursor);
if (kerr != 0) {
- DEBUG(SSSDBG_CRIT_FAILURE, "krb5_kt_start_seq_get failed.\n");
+ const char *krb5_err_msg = sss_krb5_get_error_message(ctx, kerr);
+ DEBUG(SSSDBG_CRIT_FAILURE, "krb5_kt_start_seq_get failed: %s\n",
+ (krb5_err_msg ? krb5_err_msg : "- no error message available -"));
+ sss_krb5_free_error_message(ctx, krb5_err_msg);
return kerr;
}
diff --git a/src/util/util_errors.c b/src/util/util_errors.c
index 94e641e8ef..9f36967324 100644
--- a/src/util/util_errors.c
+++ b/src/util/util_errors.c
@@ -35,6 +35,7 @@ struct err_string error_to_str[] = {
{ "Invalid data type" }, /* ERR_INVALID_DATA_TYPE */
{ "DP target is not configured" }, /* ERR_MISSING_DP_TARGET */
{ "Account Unknown" }, /* ERR_ACCOUNT_UNKNOWN */
+ { "No suitable principal found in keytab" }, /* ERR_KRB5_PRINCIPAL_NOT_FOUND */
{ "Invalid credential type" }, /* ERR_INVALID_CRED_TYPE */
{ "No credentials available" }, /* ERR_NO_CREDS */
{ "Credentials are expired" }, /* ERR_CREDS_EXPIRED */
diff --git a/src/util/util_errors.h b/src/util/util_errors.h
index e65ccbcd0c..ae21991caa 100644
--- a/src/util/util_errors.h
+++ b/src/util/util_errors.h
@@ -56,6 +56,7 @@ enum sssd_errors {
ERR_INVALID_DATA_TYPE,
ERR_MISSING_DP_TARGET,
ERR_ACCOUNT_UNKNOWN,
+ ERR_KRB5_PRINCIPAL_NOT_FOUND,
ERR_INVALID_CRED_TYPE,
ERR_NO_CREDS,
ERR_CREDS_EXPIRED,
_______________________________________________
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
Fedora Code of Conduct:
https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives:
https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org
