From fidencio at redhat.com Fri Aug 26 06:10:28 2016 Content-Type: multipart/mixed; boundary="===============3305703087186332282==" MIME-Version: 1.0 From: =?utf-8?q?Fabiano_Fid=C3=AAncio_=3Cfidencio_at_redhat=2Ecom=3E?= To: sssd-devel at lists.fedorahosted.org Subject: [SSSD] Re: [PATCH] Check whether a secret exists before trying to delete it Date: Fri, 26 Aug 2016 08:10:12 +0200 Message-ID: In-Reply-To: 20160825115610.bqk7wwzbevgzaxib@hendrix --===============3305703087186332282== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Jakub, On Thu, Aug 25, 2016 at 1:56 PM, Jakub Hrozek wrote: > On Wed, Aug 17, 2016 at 03:13:57PM +0200, Fabiano Fid=C3=AAncio 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=C3=AAncio > > Sorry about the late reply. The patch works fine as I would expect: > > sudo curl -H "Content-Type: application/json" --unix-socket /var/run/secr= ets.socket -XDELETE http://localhost/secrets/xxx > > > 404 Not Found > >

Not Found

>

The requested resource was not found.

> > > Nonetheless, I think the patch can be a bit simpler: > >> From bc39f7ba0ffa75a9ca972266bbc2741cbb0c8c73 Mon Sep 17 00:00:00 2001 >> From: =3D?UTF-8?q?Fabiano=3D20Fid=3DC3=3DAAncio?=3D >> 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=3DUTF-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=C3=AAncio >> --- >> 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[] =3D { "secret", NULL }; >> struct ldb_dn *dn; >> + struct ldb_result *res; >> int ret; >> >> ret =3D local_db_dn(mem_ctx, lctx->ldb, req_path, &dn); >> if (ret !=3D EOK) goto done; >> >> + ret =3D ldb_search(lctx->ldb, mem_ctx, &res, dn, LDB_SCOPE_BASE, >> + attrs, "%s", LOCAL_SIMPLE_FILTER); >> + if (ret !=3D EOK) { >> + ret =3D ENOENT; >> + goto done; >> + } >> + >> + if (res->count =3D=3D 0) { >> + ret =3D ENOENT; >> + goto done; >> + } >> + >> ret =3D 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 =3D 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=C3=AAncio --===============3305703087186332282== Content-Type: text/x-patch MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: attachment; filename="v2-0001-SECRETS-Return-ENOENT-when_deleting-a-non-existen.patch" RnJvbSAzZTcxZjkwMWFiMTJmNWU1YzU3MGRjZjA3OTZlZjQ3ZTM3OWU3YTUzIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiA9P1VURi04P3E/RmFiaWFubz0yMEZpZD1DMz1BQW5jaW8/PSA8 ZmlkZW5jaW9AcmVkaGF0LmNvbT4KRGF0ZTogV2VkLCAxNyBBdWcgMjAxNiAxMzoxMjoyMSArMDIw MApTdWJqZWN0OiBbUEFUQ0ggdjJdIFNFQ1JFVFM6IFJldHVybiBFTk9FTlQgd2hlbl9kZWxldGlu ZyBhIG5vbi1leGlzdGVudCBzZWNyZXQKTUlNRS1WZXJzaW9uOiAxLjAKQ29udGVudC1UeXBlOiB0 ZXh0L3BsYWluOyBjaGFyc2V0PVVURi04CkNvbnRlbnQtVHJhbnNmZXItRW5jb2Rpbmc6IDhiaXQK ClRoZSBhcHByb2FjaCB0YWtlbiBpcyBxdWl0ZSBzaW1pbGFyIHRvIHdoYXQgaXMgZG9uZSBieQpz eXNkYl9kZWxldGVfY2FjaGVfZW50cnkoKS4KClJlc29sdmVzOgpodHRwczovL2ZlZG9yYWhvc3Rl ZC5vcmcvc3NzZC90aWNrZXQvMzEyNQoKU2lnbmVkLW9mZi1ieTogRmFiaWFubyBGaWTDqm5jaW8g PGZpZGVuY2lvQHJlZGhhdC5jb20+Ci0tLQogc3JjL3Jlc3BvbmRlci9zZWNyZXRzL2xvY2FsLmMg fCAxNCArKysrKysrKy0tLS0tLQogMSBmaWxlIGNoYW5nZWQsIDggaW5zZXJ0aW9ucygrKSwgNiBk ZWxldGlvbnMoLSkKCmRpZmYgLS1naXQgYS9zcmMvcmVzcG9uZGVyL3NlY3JldHMvbG9jYWwuYyBi L3NyYy9yZXNwb25kZXIvc2VjcmV0cy9sb2NhbC5jCmluZGV4IGU5MTc2NmYuLmMzMmFmOGMgMTAw NjQ0Ci0tLSBhL3NyYy9yZXNwb25kZXIvc2VjcmV0cy9sb2NhbC5jCisrKyBiL3NyYy9yZXNwb25k ZXIvc2VjcmV0cy9sb2NhbC5jCkBAIC0zODAsMTUgKzM4MCwxNyBAQCBpbnQgbG9jYWxfZGJfZGVs ZXRlKFRBTExPQ19DVFggKm1lbV9jdHgsCiAgICAgaW50IHJldDsKIAogICAgIHJldCA9IGxvY2Fs X2RiX2RuKG1lbV9jdHgsIGxjdHgtPmxkYiwgcmVxX3BhdGgsICZkbik7Ci0gICAgaWYgKHJldCAh PSBFT0spIGdvdG8gZG9uZTsKKyAgICBpZiAocmV0ICE9IEVPSykgcmV0dXJuIHJldDsKIAogICAg IHJldCA9IGxkYl9kZWxldGUobGN0eC0+bGRiLCBkbik7Ci0gICAgaWYgKHJldCAhPSBFT0spIHsK LSAgICAgICAgcmV0ID0gRUlPOworICAgIHN3aXRjaCAocmV0KSB7CisgICAgY2FzZSBMREJfU1VD Q0VTUzoKKyAgICAgICAgcmV0dXJuIEVPSzsKKyAgICBjYXNlIExEQl9FUlJfTk9fU1VDSF9PQkpF Q1Q6CisgICAgICAgIHJldHVybiBFTk9FTlQ7CisgICAgZGVmYXVsdDoKKyAgICAgICAgcmV0dXJu IEVJTzsKICAgICB9Ci0KLWRvbmU6Ci0gICAgcmV0dXJuIHJldDsKIH0KIAogaW50IGxvY2FsX2Ri X2NyZWF0ZShUQUxMT0NfQ1RYICptZW1fY3R4LAotLSAKMi43LjQKCg== --===============3305703087186332282==--