On Fri, 2016-04-01 at 13:05 +0200, Lukas Slebodnik wrote:
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 <simo(a)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@(a)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.
Thanks for finding this!
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;
>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.
I'll fix the detection.
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.
The above looks ok to me.
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
Thanks!
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);
Thanks, IIRC the int-instead of enum use is intentional, I will look at
the others.
Simo.
--
Simo Sorce * Red Hat, Inc * New York