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