On Wed, Aug 03, 2016 at 06:08:44PM +0200, Lukas Slebodnik wrote:
> On (03/08/16 18:05), Jakub Hrozek wrote:
> >Hi,
> >
> >these two patches add error handling to the code. The first prevents
> >operating on ldb_message if retrieving the message fails. I only tested
> >an SSH login with a UPN to make sure we actually hit this codepath. I
> >don't like the deep indendation nesting, so I welcome suggestions how to
> >fix the code better.
> >
> >The second just checks a return value.
>
> >From a27529da7c03a27f8d87c624e4b6660fd5b22edf Mon Sep 17 00:00:00 2001
> >From: Jakub Hrozek <jhro...@redhat.com>
> >Date: Wed, 3 Aug 2016 18:03:59 +0200
> >Subject: [PATCH 2/2] IPA: Check the return value of sss_parse_internal_fqname
> >
> >---
> > src/providers/ipa/ipa_subdomains_id.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> >diff --git a/src/providers/ipa/ipa_subdomains_id.c
> >b/src/providers/ipa/ipa_subdomains_id.c
> >index
> >76fdaa8a1213069bd6b45e0b69b6cdb0d034d721..886813dc648f04c8fadd234524fce94455f31ee4
> > 100644
> >--- a/src/providers/ipa/ipa_subdomains_id.c
> >+++ b/src/providers/ipa/ipa_subdomains_id.c
> >@@ -509,6 +509,12 @@ static void ipa_get_subdom_acct_connected(struct
> >tevent_req *subreq)
> > } else {
> > ret = sss_parse_internal_fqname(req_input, state->filter,
> > &shortname, NULL);
> >+ if (ret != EOK) {
> >+ DEBUG(SSSDBG_CRIT_FAILURE,
> >+ "Cannot parse internal name [%s]: %d\n",
> >+ state->filter, ret);
> >+ }
> >+
> We should either fail or use less verbose debug_level.
Of cours,we need to fail. See the new patches.
>From 1e67ab6596ac73d12c97abc5feebe2ee1fca6a3f Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Wed, 3 Aug 2016 17:43:14 +0200
Subject: [PATCH 1/2] PAM: Do not act on ldb_message in case of a failure
---
src/responder/pam/pamsrv_cmd.c | 33 ++++++++++++++++++---------------
1 file changed, 18 insertions(+), 15 deletions(-)
diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c
index
66564f5d301a53dcdb5967f43ef4afdb897e9974..91ac3673d8965018ceac033c2bfe86edeeef17ea
100644
--- a/src/responder/pam/pamsrv_cmd.c
+++ b/src/responder/pam/pamsrv_cmd.c
@@ -1534,21 +1534,24 @@ static int pam_check_user_search(struct pam_auth_req
*preq)
if (preq->pd->name_is_upn) {
ret = sysdb_search_user_by_upn(preq, dom, name, user_attrs, &msg);
-
- /* Since sysdb_search_user_by_upn() searches the whole cache we
- * have to set the domain so that it matches the result. */
- sysdb_name = ldb_msg_find_attr_as_string(msg, SYSDB_NAME, NULL);
- if (sysdb_name == NULL) {
- DEBUG(SSSDBG_CRIT_FAILURE, "Cached entry has no name.\n");
- return EINVAL;
- }
- preq->domain = find_domain_by_object_name(get_domains_head(dom),
- sysdb_name);
- if (preq->domain == NULL) {
- DEBUG(SSSDBG_CRIT_FAILURE,
- "Cannot find matching domain for [%s].\n",
- sysdb_name);
- return EINVAL;
+ if (ret == EOK) {
+ /* Since sysdb_search_user_by_upn() searches the whole cache we
+ * have to set the domain so that it matches the result. */
+ sysdb_name = ldb_msg_find_attr_as_string(msg,
+ SYSDB_NAME, NULL);
+ if (sysdb_name == NULL) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "Cached entry has no name.\n");
+ return EINVAL;
+ }
+ preq->domain = find_domain_by_object_name(
+ get_domains_head(dom),
+ sysdb_name);
+ if (preq->domain == NULL) {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ "Cannot find matching domain for [%s].\n",
+ sysdb_name);
+ return EINVAL;
+ }
}
} else {
ret = sysdb_getpwnam_with_views(preq, dom, name, &res);
--
2.4.11
>From 31273617f97973332d8b265dcb01679025d16cbd Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Wed, 3 Aug 2016 18:03:59 +0200
Subject: [PATCH 2/2] IPA: Check the return value of sss_parse_internal_fqname
We should fail the request if sss_parse_internal_fqname() fails.
---
src/providers/ipa/ipa_subdomains_id.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/src/providers/ipa/ipa_subdomains_id.c
b/src/providers/ipa/ipa_subdomains_id.c
index
76fdaa8a1213069bd6b45e0b69b6cdb0d034d721..5369ec4c624544f7f3aec88ddaa30eac91c51735
100644
--- a/src/providers/ipa/ipa_subdomains_id.c
+++ b/src/providers/ipa/ipa_subdomains_id.c
@@ -509,6 +509,14 @@ static void ipa_get_subdom_acct_connected(struct
tevent_req *subreq)
} else {
ret = sss_parse_internal_fqname(req_input, state->filter,
&shortname, NULL);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ "Cannot parse internal name [%s]: %d\n",
+ state->filter, ret);
+ tevent_req_error(req, ret);
+ return;
+ }
+
req_input->inp.name = talloc_steal(req_input, shortname);
}
if (req_input->inp.name == NULL) {
--
2.4.11
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
