On Thu, Feb 25, 2016 at 11:36:43AM +0100, Pavel Březina wrote:
> On 02/25/2016 11:07 AM, Sumit Bose wrote:
> >On Thu, Feb 25, 2016 at 10:53:03AM +0100, Pavel Březina wrote:
> >>On 02/24/2016 02:34 PM, Sumit Bose wrote:
> >>>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?=
<pbrezina(a)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?= <pbrezina(a)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.
> >>
> >>Hmm. If certificate is moved to other user this code won't trigger at
the
> >>moment since it is triggered only when certificate is not found.
> >
> >Yes, you are right. This was a bad example. Nevertheless if the
> >certificate was not found on the server but found multiple times in the
> >cache I think it is save to remove them from all cached entries.
> >
> >>
> >>>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.
> >>
> >>Then we should remove only the one certificate not the whole attribute.
> >
> >Yes, this would be possible as well. But I think we do not have to be
> >too clever here. Since it is obvious that something changed on the
> >server I think it is valid to reset SYSDB_CACHE_EXPIRE. Especially given
> >the fact that it is still possible that the user entry was removed
> >completely on the server.
> >
> >>
> >>Would you mind taking over this ticket and solve those other cases as well?
> >>You are more experienced in this area.
> >
> >Sure, however I wouldn't add anything beyond what I suggested.
>
> Ok, see attached patch. I did not test it though, hope it works.
>
ACK, CI:
http://sssd-ci.duckdns.org/logs/job/38/28/summary.html
bye,
Sumit
* master: 659232f194f83ec7c450ce89c3fd41e4e74409f2
* sssd-1-13: 90bd6598f0d8ad9fa8d05419c7e14b64e09e8a54