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.
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.
Would you mind taking over this ticket and solve those other cases as
well? You are more experienced in this area.