On Wed, Feb 24, 2016 at 10:31:31AM +0100, Pavel Březina wrote: > On 02/23/2016 02:09 PM, Sumit Bose wrote: > >On Tue, Feb 23, 2016 at 01:24:30PM +0100, Pavel Březina wrote: > >>https://fedorahosted.org/sssd/ticket/2934 > > > >> From 94ae3c5231dc7f1cd9f9d172d13a11a8afcacd16 Mon Sep 17 00:00:00 2001 > >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com> > >>Date: Tue, 23 Feb 2016 11:02:42 +0100 > >>Subject: [PATCH] remove user certificate if not found on the server > >> > >>If the user is not found by cert lookup when the user is already > >>cached, two things may happen: > >>1) cert was removed from the user object > >>2) user was removed > >> > >>Instead of issuing another cert lookup we will just remove cert > >>attribute from the cache not touching the expiration timestamp so > >>the user may be updated later when needed. > >> > >>Resolves: > >>https://fedorahosted.org/sssd/ticket/2934 > >>--- > >> src/db/sysdb.h | 3 ++- > >> src/db/sysdb_ops.c | 47 > >> ++++++++++++++++++++++++++++++++++++++++++++ > >> src/providers/ldap/ldap_id.c | 10 ++++++++++ > >> 3 files changed, 59 insertions(+), 1 deletion(-) > >> > >>diff --git a/src/db/sysdb.h b/src/db/sysdb.h > >>index > >>2e797fd7fa39163c2ab6a10e51228e0f1af3f9e3..d074d4d661554a798151caee831cc672a927712f > >> 100644 > >>--- a/src/db/sysdb.h > >>+++ b/src/db/sysdb.h > >>@@ -1154,7 +1154,8 @@ errno_t sysdb_search_user_by_cert(TALLOC_CTX *mem_ctx, > >> const char *cert, > >> struct ldb_result **res); > >> > >>- > >>+errno_t sysdb_remove_cert(struct sss_domain_info *domain, > >>+ const char *cert); > >> > >> /* === Functions related to GPOs === */ > >> > >>diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c > >>index > >>ab0d59ca6db620dfbf7e74a93745df242b6fc3a3..aa688f42f9a6f7f0f86e1171df0a5f0346a59ea5 > >> 100644 > >>--- a/src/db/sysdb_ops.c > >>+++ b/src/db/sysdb_ops.c > >>@@ -3764,6 +3764,53 @@ errno_t sysdb_search_user_by_cert(TALLOC_CTX > >>*mem_ctx, > >> return sysdb_search_object_by_cert(mem_ctx, domain, cert, user_attrs, > >> res); > >> } > >> > >>+static errno_t sysdb_remove_user_cert(struct sss_domain_info *domain, > >>+ const char *name) > >>+{ > >>+ struct ldb_message_element el = { 0, SYSDB_USER_CERT, 0, NULL }; > >>+ struct sysdb_attrs attrs = { 1, &el }; > >>+ > >>+ DEBUG(SSSDBG_TRACE_FUNC, "Removing certificate from user %s@%s", > >>+ name, domain->name); > >>+ > >>+ return sysdb_set_user_attr(domain, name, &attrs, SYSDB_MOD_DEL); > > > >I would recommend to use sysdb_set_entry_attr() because you already have > >the dn as res->msgs[0]->dn in sysdb_remove_cert. With this I think you > >can move everything into sysdb_remove_cert() as well without making to > >more complex or longer. > > > >bye, > >Sumit > > Good idea, see attached patch. >
Thank you, the patch is working as expected and passes CI as well. Nevertheless see comment below. > From 04acbe4139a560049b66c15ce8526c16ef668d54 Mon Sep 17 00:00:00 2001 > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com> > Date: Tue, 23 Feb 2016 11:02:42 +0100 > Subject: [PATCH] remove user certificate if not found on the server > > If the user is not found by cert lookup when the user is already > cached, two things may happen: > 1) cert was removed from the user object > 2) user was removed > > Instead of issuing another cert lookup we will just remove cert > attribute from the cache not touching the expiration timestamp so > the user may be updated later when needed. > > Resolves: > https://fedorahosted.org/sssd/ticket/2934 > --- > src/db/sysdb.h | 3 ++- > src/db/sysdb_ops.c | 31 +++++++++++++++++++++++++++++++ > src/providers/ldap/ldap_id.c | 10 ++++++++++ > 3 files changed, 43 insertions(+), 1 deletion(-) > > diff --git a/src/db/sysdb.h b/src/db/sysdb.h > index > 2e797fd7fa39163c2ab6a10e51228e0f1af3f9e3..d074d4d661554a798151caee831cc672a927712f > 100644 > --- a/src/db/sysdb.h > +++ b/src/db/sysdb.h > @@ -1154,7 +1154,8 @@ errno_t sysdb_search_user_by_cert(TALLOC_CTX *mem_ctx, > const char *cert, > struct ldb_result **res); > > - > +errno_t sysdb_remove_cert(struct sss_domain_info *domain, > + const char *cert); > > /* === Functions related to GPOs === */ > > diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c > index > ab0d59ca6db620dfbf7e74a93745df242b6fc3a3..573f3de001ffabd6e6aa17d8e808bf9363531608 > 100644 > --- a/src/db/sysdb_ops.c > +++ b/src/db/sysdb_ops.c > @@ -3764,6 +3764,37 @@ errno_t sysdb_search_user_by_cert(TALLOC_CTX *mem_ctx, > return sysdb_search_object_by_cert(mem_ctx, domain, cert, user_attrs, > res); > } > > +errno_t sysdb_remove_cert(struct sss_domain_info *domain, > + const char *cert) > +{ > + struct ldb_message_element el = { 0, SYSDB_USER_CERT, 0, NULL }; > + struct sysdb_attrs del_attrs = { 1, &el }; > + const char *attrs[] = {SYSDB_NAME, NULL}; > + struct ldb_result *res = NULL; > + errno_t ret; > + > + ret = sysdb_search_object_by_cert(NULL, domain, cert, attrs, &res); > + if (ret == ENOENT || res == NULL || res->count == 0) { > + ret = EOK; > + goto done; > + } else if (ret != EOK) { > + DEBUG(SSSDBG_MINOR_FAILURE, "Unable to lookup object by cert " > + "[%d]: %s\n", ret, sss_strerror(ret)); > + goto done; > + } else if (res->count > 1) { I think this case might actually happen and is valid. If the certificate is moved from one user to another one on the server and the new news is looked up the certificate will be in the user entry of the old and new owner because we do not force uniqueness here. Authentication won't work because we cannot find a unique user but nonetheless the case might happen. To solve this I would suggest to remove the certificate from all entries and reset SYSDB_CACHE_EXPIRE as well. This way the certificate is remove from the old owner and will be read again for the new owner. Additionally I think SYSDB_CACHE_EXPIRE should be reset always. Since it is possible to assign multiple certificates to a user the user should be looked up again in case only a single certificate is removed on the server side. bye, Sumit > + DEBUG(SSSDBG_MINOR_FAILURE, "More then one result found!"); > + ret = ERR_INTERNAL; > + goto done; > + } > + > + ret = sysdb_set_entry_attr(domain->sysdb, res->msgs[0]->dn, > + &del_attrs, SYSDB_MOD_DEL); > + > +done: > + talloc_free(res); > + return ret; > +} > + > errno_t sysdb_get_sids_of_members(TALLOC_CTX *mem_ctx, > struct sss_domain_info *dom, > const char *group_name, > diff --git a/src/providers/ldap/ldap_id.c b/src/providers/ldap/ldap_id.c > index > b7cef4e13d70f738428cdbf70b661de92a87b4a5..8923e7e0c3fdae4614d6bfef2665de7854b62e8e > 100644 > --- a/src/providers/ldap/ldap_id.c > +++ b/src/providers/ldap/ldap_id.c > @@ -529,6 +529,16 @@ static void users_get_done(struct tevent_req *subreq) > */ > break; > > + case BE_FILTER_CERT: > + ret = sysdb_remove_cert(state->domain, state->name); > + if (ret != EOK) { > + DEBUG(SSSDBG_CRIT_FAILURE, "Unable to remove user > certificate" > + "[%d]: %s\n", ret, sss_strerror(ret)); > + tevent_req_error(req, ret); > + return; > + } > + break; > + > default: > tevent_req_error(req, EINVAL); > return; > -- > 2.1.0 > > _______________________________________________ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
