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.