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.
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 <simo(a)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 <cheimes(a)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 <cheimes(a)redhat.com>
Reviewd-by: Simo Sorce <simo(a)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