On (30/03/16 12:31), Simo Sorce wrote: >This patchset implements a new responder like service in SSSD called >secrets. It uses the Custodia project API to offer a service where >applications/users can store secrets in a way that makes requests >remotizable and routable with a high degree of configurability (esp, in >conjunction with a Custodia proxy). > >Included are also accessory patches to change the monitor and other >aspects of service startup and monitoring necessary to have this new >kind of service which is more independent than the pam/nss based >services. > >There is no testsuite for the service yet. > >The work is also not complete in that the monitor does not start the >service yet, I have an experimental unit file I am working on but it is >not fully functional and not included yet.. > >I do not expect all patches to be accepted right away, but they all work >individually (manually tested), but I think it is a good time to start >review and bring in what works, as we are going to spread some of the >remaining work across multiple people. > >HTH, >Simo. >
I do not plan to do a full review. Jakub voluntered IIRC :-) But here are few comments. >From cd731590f1830ab9686949af1fa66d2b7463eafc Mon Sep 17 00:00:00 2001 >From: Simo Sorce <s...@redhat.com> >Date: Tue, 12 Jan 2016 20:07:59 -0500 >Subject: [PATCH 01/15] Util: Add watchdog helper > >The watchdog uses a kernel timer to issue a signal to the process. >It checks if the ticker is not being reset by the main event loop, which >would indicate that the process got stuck. >At the same time it sets a tevent timer to clear the watchdog ticker, so >that the watchdog handler is kept happy. > >If the watchdog detects that the timer event failed to reset the watchdog for >three times in a row then the process is killed. >Normally the monitor will detect the child terminated and will rescheduled it. > >Related: >https://fedorahosted.org/sssd/ticket/2921 >--- >diff --git a/src/util/util_watchdog.c b/src/util/util_watchdog.c >new file mode 100644 >index >0000000000000000000000000000000000000000..64faaaf03213319d3127fde3946124ee26c7794a >--- /dev/null >+++ b/src/util/util_watchdog.c //snip >+int setup_watchdog(struct tevent_context *ev, int interval) >+{ >+ struct sigevent sev; >+ struct itimerspec its; >+ int signum = SIGRTMIN; >+ int ret; >+ >+ CatchSignal(signum, watchdog_handler); >+ >+ sev.sigev_notify = SIGEV_SIGNAL; >+ sev.sigev_signo = signum; >+ sev.sigev_value.sival_ptr = &watchdog_ctx.timerid; >+ errno = 0; >+ ret = timer_create(CLOCK_MONOTONIC, &sev, &watchdog_ctx.timerid); I got a valgrind error here on rhel6 ==2376== Syscall param timer_create(evp) points to uninitialised byte(s) ==2376== at 0x5B0B08D: timer_create@@GLIBC_2.3.3 (in /lib64/librt-2.12.so) ==2376== by 0x54942CD: setup_watchdog (util_watchdog.c:88) ==2376== by 0x40414F: server_setup (server.c:651) ==2376== by 0x403059: test_run_as_root_fg (test_server.c:104) ==2376== by 0x5243A94: ??? (in /usr/lib64/libcmocka.so.0.3.1) ==2376== by 0x5243DA9: _cmocka_run_group_tests (in /usr/lib64/libcmocka.so.0.3.1) ==2376== by 0x402CA1: main (test_server.c:204) ==2376== Address 0x7fefff920 is on thread 1's stack Following diff fixed it. diff --git a/src/util/util_watchdog.c b/src/util/util_watchdog.c index 64faaaf..d6f71ad 100644 --- a/src/util/util_watchdog.c +++ b/src/util/util_watchdog.c @@ -74,7 +74,7 @@ static void watchdog_event_handler(struct tevent_context *ev, int setup_watchdog(struct tevent_context *ev, int interval) { - struct sigevent sev; + struct sigevent sev = { 0, }; struct itimerspec its; int signum = SIGRTMIN; int ret; >+ if (ret == -1) { >+ ret = errno; >+ DEBUG(SSSDBG_FATAL_FAILURE, >+ "Failed to create watchdog timer (%d) [%s]\n", >+ ret, strerror(ret)); >+ return ret; >+ } >+ >+ if (interval == 0) { >+ interval = WATCHDOG_DEF_INTERVAL; >+ } >+ watchdog_ctx.interval.tv_sec = interval; >+ watchdog_ctx.interval.tv_usec = 0; >+ >+ /* Start the timer */ >+ /* we give 1 second head start to the watchdog event */ >+ its.it_value.tv_sec = interval + 1; >+ its.it_value.tv_nsec = 0; >+ its.it_interval.tv_sec = interval; >+ its.it_interval.tv_nsec = 0; >+ errno = 0; >+ ret = timer_settime(watchdog_ctx.timerid, 0, &its, NULL); >+ if (ret == -1) { >+ ret = errno; >+ DEBUG(SSSDBG_FATAL_FAILURE, >+ "Failed to create watchdog timer (%d) [%s]\n", >+ ret, strerror(ret)); >+ return ret; >+ } >+ >+ /* Add the watchdog event and make it fire as fast as the timer */ >+ watchdog_event_handler(ev, NULL, tevent_timeval_zero(), NULL); >+ >+ return EOK; >+} >+ >+void teardown_watchdog(void) >+{ >+ int ret; >+ >+ /* Disarm the timer */ >+ errno = 0; >+ ret = timer_delete(watchdog_ctx.timerid); >+ if (ret == -1) { >+ ret = errno; >+ DEBUG(SSSDBG_FATAL_FAILURE, >+ "Failed to destroy watchdog timer (%d) [%s]\n", >+ ret, strerror(ret)); >+ } >+ >+ /* and kill the watchdog event */ >+ talloc_free(watchdog_ctx.te); >+} >+ >-- >2.5.5 > >From 33c3b8ad97f37ca06f00eaf6be747360e9f4947b Mon Sep 17 00:00:00 2001 >From: Simo Sorce <s...@redhat.com> >Date: Fri, 8 Jan 2016 10:56:17 -0500 >Subject: [PATCH 07/15] Secrets: Add autoconf macros to build with secrets > >Prepares autoconf for the new Secrets Provider > >Related: >https://fedorahosted.org/sssd/ticket/2913 >--- > configure.ac | 3 +++ > src/conf_macros.m4 | 43 +++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 46 insertions(+) > >diff --git a/configure.ac b/configure.ac >index >2c36049ca04f57fe94f359c5dc19a506dd2b9388..783372dd6b5d86793c218ac513a93b65e97d4c06 > 100644 >--- a/configure.ac >+++ b/configure.ac >@@ -145,6 +145,9 @@ WITH_NFS > WITH_NFS_LIB_PATH > WITH_LIBWBCLIENT > WITH_SSSD_USER >+SSSD_RUNSTATEDIR >+WITH_SECRETS >+WITH_SECRETS_DB_PATH > > m4_include([src/external/pkg.m4]) > m4_include([src/external/libpopt.m4]) >diff --git a/src/conf_macros.m4 b/src/conf_macros.m4 >index >c8774b5f5b7d1be09ee74195bc0732984dd551d7..e0a64470da4ac0c885274bfbb066b35873e0911b > 100644 >--- a/src/conf_macros.m4 >+++ b/src/conf_macros.m4 >@@ -847,3 +847,46 @@ AC_DEFUN([ENABLE_POLKIT_RULES_PATH], > > AM_CONDITIONAL([HAVE_POLKIT_RULES_D], [test x$HAVE_POLKIT_RULES_D != x]) > ]) >+ >+dnl BAckwards compat for older autoconf >+AC_DEFUN([SSSD_RUNSTATEDIR], >+ [ >+ if test x"$runstatedir" = x; then >+ AC_SUBST([runstatedir], >+ ["${localstatedir}/run"]) >+ fi >+ ]) >+ >+AC_DEFUN([WITH_SECRETS], >+ [ AC_ARG_WITH([secrets], >+ [AC_HELP_STRING([--with-secrets], >+ [Whether to build with secrets support [yes]] >+ ) >+ ], >+ [with_secrets=$withval], >+ with_secrets=yes >+ ) >+ >+ if test x"$with_secrets" = xyes; then >+ AC_DEFINE(BUILD_SECRETS, 1, [whether to build with SECRETS support]) >+ fi >+ AM_CONDITIONAL([BUILD_SECRETS], [test x"$with_secrets" = xyes]) >+ ]) >+ >+AC_DEFUN([WITH_SECRETS_DB_PATH], >+ [ AC_ARG_WITH([secrets-db-path], >+ [AC_HELP_STRING([--with-secrets-db-path=PATH], >+ [Path to the SSSD databases >[/var/lib/sss/secrets]] >+ ) >+ ] >+ ) >+ config_secdbpath="\"SSS_STATEDIR\"/secrets" >+ secdbpath="${localstatedir}/lib/sss/secrets" >+ if test x"$with_secrets_db_path" != x; then >+ config_secdbpath=$with_secrets_db_path >+ secdbpath=$with_secrets_db_path >+ fi >+ AC_SUBST(secdbpath) >+ AC_DEFINE_UNQUOTED(SECRETS_DB_PATH, "$config_secdbpath", [Path to the >SSSD Secrets databases]) >+ ]) >+ >-- >2.5.5 > >From deafb4c8a72a4500353b03765fd97406759ee4e0 Mon Sep 17 00:00:00 2001 >From: Christian Heimes <chei...@redhat.com> >Date: Fri, 8 Jan 2016 13:26:22 +0100 >Subject: [PATCH 08/15] Secrets: m4 macros for jansson and http-parser > >Prepares autoconf for the new Secrets Provider dependencies > >Related: >https://fedorahosted.org/sssd/ticket/2913 > >Signed-off-by: Christian Heimes <chei...@redhat.com> >Reviewd-by: Simo Sorce <s...@redhat.com> >--- > configure.ac | 2 ++ > src/external/libhttp_parser.m4 | 15 +++++++++++++++ > src/external/libjansson.m4 | 15 +++++++++++++++ > 3 files changed, 32 insertions(+) > create mode 100644 src/external/libhttp_parser.m4 > create mode 100644 src/external/libjansson.m4 > >diff --git a/configure.ac b/configure.ac >index >783372dd6b5d86793c218ac513a93b65e97d4c06..f7430ca71f07b1085f49a7635070f71894f1a4a9 > 100644 >--- a/configure.ac >+++ b/configure.ac >@@ -185,6 +185,8 @@ m4_include([src/external/libnfsidmap.m4]) > m4_include([src/external/cwrap.m4]) > m4_include([src/external/libresolv.m4]) > m4_include([src/external/intgcheck.m4]) >+m4_include([src/external/libhttp_parser.m4]) >+m4_include([src/external/libjansson.m4]) This patch broke build in mock and CI. because you added new dependencies which are not as BuildRequires in spec. and moreover these dependencies are strictly detected at configure time even though secrect service can be build optionally. One solution would be to remove conitional build of secret service or another to fix detection of build dependencies at configure time. e.g. diff --git a/configure.ac b/configure.ac index 2a8329c..9ddd9da 100644 --- a/configure.ac +++ b/configure.ac @@ -186,8 +186,11 @@ m4_include([src/external/libnfsidmap.m4]) m4_include([src/external/cwrap.m4]) m4_include([src/external/libresolv.m4]) m4_include([src/external/intgcheck.m4]) -m4_include([src/external/libhttp_parser.m4]) -m4_include([src/external/libjansson.m4]) + +if test x$with_secrets = xyes; then + m4_include([src/external/libhttp_parser.m4]) + m4_include([src/external/libjansson.m4]) +fi if test x$build_config_lib = xyes; then m4_include([src/external/libaugeas.m4]) or you can move if condition directly to m4_included files. And here is a diff for adding missing build dependencies diff --git a/contrib/ci/deps.sh b/contrib/ci/deps.sh index c9a8a63..8120347 100644 --- a/contrib/ci/deps.sh +++ b/contrib/ci/deps.sh @@ -114,6 +114,8 @@ if [[ "$DISTRO_BRANCH" == -debian-* ]]; then python-ldap ldap-utils slapd + libhttp-parser-dev + libjansson-dev ) DEPS_INTGCHECK_SATISFIED=true fi diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in index 979f390..c0709ea 100644 --- a/contrib/sssd.spec.in +++ b/contrib/sssd.spec.in @@ -162,6 +162,9 @@ BuildRequires: nfs-utils-lib-devel BuildRequires: samba4-devel BuildRequires: libsmbclient-devel +BuildRequires: http-parser-devel +BuildRequires: jansson-devel + %description Provides a set of daemons to manage access to remote directories and authentication mechanisms. It provides an NSS and PAM interface toward There are also some warnings reported by static analyzers. Error: PW.MIXED_ENUM_TYPE: [#def4] sssd-1.13.90/src/responder/secrets/providers.c:318: mixed_enum_type: enumerated type mixed with another type # 316| req->length = 0; # 317| # 318|-> cmd = http_method_str(method); # 319| if (cmd == NULL) return EINVAL; # 320| Error: PW.MIXED_ENUM_TYPE: [#def5] sssd-1.13.90/src/responder/secrets/proxy.c:249: mixed_enum_type: enumerated type mixed with another type # 247| /* Request-Line */ # 248| req->data = talloc_asprintf(req, "%s %s HTTP/1.1\r\n", # 249|-> http_method_str(secreq->method), http_uri); # 250| if (!req->data) { # 251| ret = ENOMEM; Error: UNUSED_VALUE (CWE-563): [#def9] sssd-1.13.90/src/util/server.c:651: value_overwrite: Overwriting previous write to "ret" with value from "setup_watchdog(ctx->event_ctx, watchdog_interval)". sssd-1.13.90/src/util/server.c:648: returned_value: Assigning value from "confdb_get_int(ctx->confdb_ctx, conf_entry, "timeout", 0, &watchdog_interval)" to "ret" here, but that stored value is overwritten before it can be used. # 646| # 647| /* Setup the internal watchdog */ # 648|-> ret = confdb_get_int(ctx->confdb_ctx, conf_entry, # 649| CONFDB_DOMAIN_TIMEOUT, # 650| 0, &watchdog_interval); Following two seems to be the same. One is from coverity and second from clang Error: UNINIT (CWE-457): [#def7] sssd-1.13.90/src/responder/secrets/proxy.c:925: var_decl: Declaring variable "reply" without initializer. sssd-1.13.90/src/responder/secrets/proxy.c:939: uninit_use: Using uninitialized value "reply". # 937| } # 938| # 939|-> ret = sec_http_reply_with_headers(state->secreq, &state->secreq->reply, # 940| reply->status_code, reply->reason_phrase, # 941| reply->headers, reply->num_headers, Error: CLANG_WARNING: [#def8] sssd-1.13.90/src/responder/secrets/proxy.c:940:39: warning: Dereference of undefined pointer value # reply->status_code, reply->reason_phrase, # ^~~~~~~~~~~~~~~~~~ sssd-1.13.90/src/responder/secrets/proxy.c:925:5: note: 'reply' declared without an initial value # struct proxy_http_reply *reply; # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.13.90/src/responder/secrets/proxy.c:931:11: note: Calling 'proxy_http_req_recv' # ret = proxy_http_req_recv(subreq, state, &reply); # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.13.90/src/responder/secrets/proxy.c:550:5: note: Taking true branch # TEVENT_REQ_RETURN_ON_ERROR(req); # ^ sssd-1.13.90/src/util/util.h:104:5: note: expanded from macro 'TEVENT_REQ_RETURN_ON_ERROR' # if (tevent_req_is_error(req, &TRROEstate, &TRROEerr)) { \ # ^ sssd-1.13.90/src/responder/secrets/proxy.c:550:5: note: Assuming 'TRROEstate' is equal to TEVENT_REQ_USER_ERROR # TEVENT_REQ_RETURN_ON_ERROR(req); # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.13.90/src/util/util.h:105:13: note: expanded from macro 'TEVENT_REQ_RETURN_ON_ERROR' # if (TRROEstate == TEVENT_REQ_USER_ERROR) { \ # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.13.90/src/responder/secrets/proxy.c:550:5: note: Taking true branch sssd-1.13.90/src/util/util.h:105:9: note: expanded from macro 'TEVENT_REQ_RETURN_ON_ERROR' # if (TRROEstate == TEVENT_REQ_USER_ERROR) { \ # ^ sssd-1.13.90/src/responder/secrets/proxy.c:931:11: note: Returning from 'proxy_http_req_recv' # ret = proxy_http_req_recv(subreq, state, &reply); # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.13.90/src/responder/secrets/proxy.c:934:9: note: Assuming 'ret' is equal to 0 # if (ret != EOK) { # ^~~~~~~~~~ sssd-1.13.90/src/responder/secrets/proxy.c:934:5: note: Taking false branch # if (ret != EOK) { # ^ sssd-1.13.90/src/responder/secrets/proxy.c:940:39: note: Dereference of undefined pointer value # reply->status_code, reply->reason_phrase, # ^~~~~~~~~~~~~~~~~~ # 938| # 939| ret = sec_http_reply_with_headers(state->secreq, &state->secreq->reply, # 940|-> reply->status_code, reply->reason_phrase, # 941| reply->headers, reply->num_headers, # 942| &reply->body); LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
