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