On Fri, Aug 26, 2016 at 9:49 AM, Lukas Slebodnik
<lslebodn(a)redhat.com> wrote:
> On (26/08/16 08:10), Fabiano Fidêncio wrote:
>>Jakub,
>>
>>On Thu, Aug 25, 2016 at 1:56 PM, Jakub Hrozek <jhrozek(a)redhat.com> wrote:
>>> On Wed, Aug 17, 2016 at 03:13:57PM +0200, Fabiano Fidêncio wrote:
>>>> The attached patch for #3125 [0]is based on Jakub's
"secrets" branch
>>>> on github [1], as there are (at least) a few issues with the current
>>>> secrets code.
>>>>
>>>> [0]:
https://fedorahosted.org/sssd/ticket/312
>>>> [1]:
https://github.com/jhrozek/sssd/tree/secrets
>>>>
>>>> Although I didn't fire a CI build, the local "make
intgcheck" passes
>>>> without issues.
>>>>
>>>> Best Regards,
>>>> --
>>>> Fabiano Fidêncio
>>>
>>> Sorry about the late reply. The patch works fine as I would expect:
>>>
>>> sudo curl -H "Content-Type: application/json" --unix-socket
/var/run/secrets.socket -XDELETE
http://localhost/secrets/xxx
>>> <html>
>>> <head>
>>> <title>404 Not Found</title></head>
>>> <body>
>>> <h1>Not Found</h1>
>>> <p>The requested resource was not found.</p>
>>> </body>
>>>
>>> Nonetheless, I think the patch can be a bit simpler:
>>>
>>>> From bc39f7ba0ffa75a9ca972266bbc2741cbb0c8c73 Mon Sep 17 00:00:00 2001
>>>> From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fidencio(a)redhat.com>
>>>> Date: Wed, 17 Aug 2016 13:12:21 +0200
>>>> Subject: [PATCH] SECRETS: Check whether a secret exists before trying
to
>>>> delete it
>>>> MIME-Version: 1.0
>>>> Content-Type: text/plain; charset=UTF-8
>>>> Content-Transfer-Encoding: 8bit
>>>>
>>>> Checking whether a secret exists or not before trying to delete it
>>>> allows SSSD to report the proper error code in case of trying to delete
>>>> a non-existent secret.
>>>>
>>>> Resolves:
>>>>
https://fedorahosted.org/sssd/ticket/3125
>>>>
>>>> Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com>
>>>> ---
>>>> src/responder/secrets/local.c | 14 ++++++++++++++
>>>> 1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/src/responder/secrets/local.c
b/src/responder/secrets/local.c
>>>> index e91766f..3985b6d 100644
>>>> --- a/src/responder/secrets/local.c
>>>> +++ b/src/responder/secrets/local.c
>>>> @@ -376,12 +376,26 @@ int local_db_delete(TALLOC_CTX *mem_ctx,
>>>> struct local_context *lctx,
>>>> const char *req_path)
>>>> {
>>>> + static const char *attrs[] = { "secret", NULL };
>>>> struct ldb_dn *dn;
>>>> + struct ldb_result *res;
>>>> int ret;
>>>>
>>>> ret = local_db_dn(mem_ctx, lctx->ldb, req_path, &dn);
>>>> if (ret != EOK) goto done;
>>>>
>>>> + ret = ldb_search(lctx->ldb, mem_ctx, &res, dn,
LDB_SCOPE_BASE,
>>>> + attrs, "%s", LOCAL_SIMPLE_FILTER);
>>>> + if (ret != EOK) {
>>>> + ret = ENOENT;
>>>> + goto done;
>>>> + }
>>>> +
>>>> + if (res->count == 0) {
>>>> + ret = ENOENT;
>>>> + goto done;
>>>> + }
>>>> +
>>>> ret = ldb_delete(lctx->ldb, dn);
>>>
>>> I think it's OK to just try to delete the secret and act on the return
>>> value. This is what we already have in sysdb_delete_cache_entry():
>>> 105 ret = ldb_delete(ldb, dn);
>>> 106 switch (ret) {
>>> 107 case LDB_SUCCESS:
>>> 108 return EOK;
>>> 109 case LDB_ERR_NO_SUCH_OBJECT:
>>> 110 if (ignore_not_found) {
>>> 111 return EOK;
>>> 112 }
>>>
>>> So I think we can use something else in secrets to return ENOENT on
>>> receiving LDB_ERR_NO_SUCH_OBJECT from ldb_delete().
>>
>>Suggestion taken. You can find the v2 attached to this email.
>>
>>Best Regards,
>>--
>>Fabiano Fidêncio
>
> >From 3e71f901ab12f5e5c570dcf0796ef47e379e7a53 Mon Sep 17 00:00:00 2001
>>From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fidencio(a)redhat.com>
>>Date: Wed, 17 Aug 2016 13:12:21 +0200
>>Subject: [PATCH v2] SECRETS: Return ENOENT when_deleting a non-existent secret
>>MIME-Version: 1.0
>>Content-Type: text/plain; charset=UTF-8
>>Content-Transfer-Encoding: 8bit
>>
>>The approach taken is quite similar to what is done by
>>sysdb_delete_cache_entry().
>>
>>Resolves:
>>https://fedorahosted.org/sssd/ticket/3125
>>
>>Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com>
>>---
>> src/responder/secrets/local.c | 14 ++++++++------
>> 1 file changed, 8 insertions(+), 6 deletions(-)
>>
>>diff --git a/src/responder/secrets/local.c b/src/responder/secrets/local.c
>>index e91766f..c32af8c 100644
>>--- a/src/responder/secrets/local.c
>>+++ b/src/responder/secrets/local.c
>>@@ -380,15 +380,17 @@ int local_db_delete(TALLOC_CTX *mem_ctx,
>> int ret;
>>
>> ret = local_db_dn(mem_ctx, lctx->ldb, req_path, &dn);
>>- if (ret != EOK) goto done;
>>+ if (ret != EOK) return ret;
>>
>> ret = ldb_delete(lctx->ldb, dn);
>>- if (ret != EOK) {
>>- ret = EIO;
>>+ switch (ret) {
>>+ case LDB_SUCCESS:
>>+ return EOK;
>>+ case LDB_ERR_NO_SUCH_OBJECT:
>>+ return ENOENT;
>>+ default:
>>+ return EIO;
>> }
> Is this mapping ldb_error to ernno different as in sysdb_error_to_errno?
> If no then it would be good to reuse existing fucntion.
Hmm. Interesting. Didn't even know about this function.
Suggestion taken, see v3 attached.
Best Regards,
--
Fabiano Fidêncio