On Tue, Nov 10, 2015 at 12:40:23PM +0100, Lukas Slebodnik wrote:
On (10/11/15 11:36), Sumit Bose wrote:
>Hi,
>
>this patch switches on the Online Certificate Status Protocol (OCSP)
>checks while validation the certificate. This is done by calling
>CERT_EnableOCSPChecking() before doing the validation. The main part of
>the patch makes this configurable.
>
>Since I expect that certificate validation will need more tuning option
>in future I didn't add a new option of switch OCSP off and on but a more
>generic one called 'certificate_verification' which accepts a comma
>separated list of parameters. Currently only 'no_ocsp' is supported.
>
>Currently this option is tested indirectly because I generated the test
>certificates with IPA they contain the OCSP data. But since the OCSP
>check, which is now on by default, requires on-line access to the
>referenced OCSP server the OCSP check must be disabled for the
>unit-test.
>
>bye,
>Sumit
Just few "build-related" comments.
>From 64701f85a3c76ce420b974b6fd94186007e53257 Mon Sep 17 00:00:00 2001
>From: Sumit Bose <sbose(a)redhat.com>
>Date: Thu, 5 Nov 2015 18:20:27 +0100
>Subject: [PATCH] p11: enable ocsp checks
>
>This patch enables the Online Certificate Status Protocol in NSS and
>adds an option to disable it if needed. To make further tuning of
>certificate verification more easy it is not an option on its own but an
>option to the new certificate_verification configuration option.
>
>Resolves
https://fedorahosted.org/sssd/ticket/2812
>---
> Makefile.am | 5 ++--
> src/confdb/confdb.h | 1 +
> src/config/SSSDConfig/__init__.py.in | 1 +
> src/config/SSSDConfigTest.py | 3 ++-
> src/config/etc/sssd.api.conf | 1 +
> src/man/sssd.conf.5.xml | 28 ++++++++++++++++++++++
> src/p11_child/p11_child_nss.c | 25 ++++++++++++++++++--
> src/responder/pam/pamsrv.h | 1 +
> src/responder/pam/pamsrv_cmd.c | 14 ++++++++++-
> src/responder/pam/pamsrv_p11.c | 21 ++++++++++------
> src/responder/ssh/sshsrv_cmd.c | 15 +++++++++++-
> src/tests/cmocka/test_cert_utils.c | 2 +-
> src/tests/cmocka/test_pam_srv.c | 34 ++++++++++++++++++++------
> src/util/cert.h | 3 +++
> src/util/cert/cert_common.c | 46 ++++++++++++++++++++++++++++++++++++
> src/util/cert/libcrypto/cert.c | 1 +
> src/util/cert/nss/cert.c | 20 ++++++++++++++++
> 17 files changed, 199 insertions(+), 22 deletions(-)
>
>diff --git a/Makefile.am b/Makefile.am
>index
a0abb8fb3a9cc19cfabad8066c6c65d78e78d206..ee97d095e07f6d485232a1afd87f9e4057688f22 100644
>--- a/Makefile.am
>+++ b/Makefile.am
>@@ -2579,8 +2579,7 @@ test_cert_utils_LDADD = \
> $(CRYPTO_LIBS) \
> libsss_debug.la \
> libsss_test_common.la \
>- libsss_cert.la \
>- libsss_crypt.la \
>+ libsss_util.la \
> $(NULL)
libss_cert and libsss_crypt chould not be removed.
Otherwise I cannot build the test.
src/tests/cmocka/test_cert_utils-test_cert_utils.o: In function
`test_sss_cert_der_to_pem':
/dev/shm/gcc/../sssd/src/tests/cmocka/test_cert_utils.c:193: undefined reference to
`sss_cert_der_to_pem'
/dev/shm/gcc/../sssd/src/tests/cmocka/test_cert_utils.c:196: undefined reference to
`sss_cert_der_to_pem'
src/tests/cmocka/test_cert_utils-test_cert_utils.o: In function
`test_sss_cert_pem_to_der':
/dev/shm/gcc/../sssd/src/tests/cmocka/test_cert_utils.c:213: undefined reference to
`sss_cert_pem_to_der'
/dev/shm/gcc/../sssd/src/tests/cmocka/test_cert_utils.c:216: undefined reference to
`sss_cert_pem_to_der'
src/tests/cmocka/test_cert_utils-test_cert_utils.o: In function
`test_sss_cert_derb64_to_pem':
/dev/shm/gcc/../sssd/src/tests/cmocka/test_cert_utils.c:232: undefined reference to
`sss_cert_derb64_to_pem'
/dev/shm/gcc/../sssd/src/tests/cmocka/test_cert_utils.c:235: undefined reference to
`sss_cert_derb64_to_pem'
src/tests/cmocka/test_cert_utils-test_cert_utils.o: In function
`test_sss_cert_pem_to_derb64':
/dev/shm/gcc/../sssd/src/tests/cmocka/test_cert_utils.c:250: undefined reference to
`sss_cert_pem_to_derb64'
/dev/shm/gcc/../sssd/src/tests/cmocka/test_cert_utils.c:253: undefined reference to
`sss_cert_pem_to_derb64'
src/tests/cmocka/test_cert_utils-test_cert_utils.o: In function
`test_bin_to_ldap_filter_value':
/dev/shm/gcc/../sssd/src/tests/cmocka/test_cert_utils.c:279: undefined reference to
`bin_to_ldap_filter_value'
/dev/shm/gcc/../sssd/src/tests/cmocka/test_cert_utils.c:283: undefined reference to
`bin_to_ldap_filter_value'
src/tests/cmocka/test_cert_utils-test_cert_utils.o: In function
`test_sss_cert_derb64_to_ldap_filter':
/dev/shm/gcc/../sssd/src/tests/cmocka/test_cert_utils.c:300: undefined reference to
`sss_cert_derb64_to_ldap_filter'
/dev/shm/gcc/../sssd/src/tests/cmocka/test_cert_utils.c:303: undefined reference to
`sss_cert_derb64_to_ldap_filter'
src/tests/cmocka/test_cert_utils-test_cert_utils.o: In function
`test_cert_to_ssh_key':
/dev/shm/gcc/../sssd/src/tests/cmocka/test_cert_utils.c:352: undefined reference to
`sss_base64_decode'
/dev/shm/gcc/../sssd/src/tests/cmocka/test_cert_utils.c:355: undefined reference to
`sss_base64_decode'
/dev/shm/gcc/../sssd/src/tests/cmocka/test_cert_utils.c:358: undefined reference to
`cert_to_ssh_key'
src/tests/cmocka/test_cert_utils-test_cert_utils.o: In function `main':
/dev/shm/gcc/../sssd/src/tests/cmocka/test_cert_utils.c:422: undefined reference to
`nspr_nss_cleanup'
collect2: error: ld returned 1 exit status
Makefile:10360: recipe for target 'test_cert_utils' failed
the root of proble was caused by following change in this patch.
>
> test_data_provider_be_SOURCES = \
>@@ -3156,6 +3155,7 @@ proxy_child_LDADD = \
> p11_child_SOURCES = \
> src/p11_child/p11_child_nss.c \
> src/util/atomic_io.c \
>+ src/util/util.c \
This file was added here because the function split_on_separator
is used in file src/util/cert/cert_common.c (parse_cert_verify_opts)
But the file src/util/cert/cert_common.c is part of libsss_cert.so
and not part of p11_child.
> $(NULL)
> p11_child_CFLAGS = \
> $(AM_CFLAGS) \
>@@ -3168,6 +3168,7 @@ p11_child_LDADD = \
> $(POPT_LIBS) \
> $(NSS_LIBS) \
> libsss_crypt.la \
>+ libsss_cert.la \
> $(NULL)
Following diff should fix it.
DHASH_LIBS are required because functions from libdhash
are used in src/util/util.c.
Thank you for testing the build and your comments. I hope you don't mind
that I didn't took you patch but shuffled to code a bit around to
remove the circular dependency between libsss_util and libsss_cert. The
patch now passes the debian test in CI for me.
You suggested on irc to split out the calls which need DHASH_LIBS from
util.c. I think this is a good idea, would you like to open a ticket for
this or shall I? For the main SSSD daemon it does not matter much since
most of them need nearly everything form libsss_util anyway. But I think
it would make sense to trim down the list of loaded libraries for the
child processes especially the ones which have the SETUID or SETGID bit
set. I'm not an expert here and I'm not sure if this might be
exploitable somehow but I think the less code is in the address space of
the process the less code is there to get exploited.
index ee97d09..7a9468d 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -777,6 +777,7 @@ pkglib_LTLIBRARIES += libsss_cert.la
libsss_cert_la_SOURCES = \
$(SSS_CERT_SOURCES) \
+ src/util/util.c \
$(NULL)
libsss_cert_la_CFLAGS = \
$(AM_CFLAGS) \
@@ -785,6 +786,7 @@ libsss_cert_la_CFLAGS = \
libsss_cert_la_LIBADD = \
$(SSS_CERT_LIBS) \
$(TALLOC_LIBS) \
+ $(DHASH_LIBS) \
libsss_crypt.la \
libsss_debug.la \
$(NULL)
@@ -2579,7 +2581,8 @@ test_cert_utils_LDADD = \
$(CRYPTO_LIBS) \
libsss_debug.la \
libsss_test_common.la \
- libsss_util.la \
+ libsss_cert.la \
+ libsss_crypt.la \
$(NULL)
test_data_provider_be_SOURCES = \
@@ -3155,7 +3158,6 @@ proxy_child_LDADD = \
p11_child_SOURCES = \
src/p11_child/p11_child_nss.c \
src/util/atomic_io.c \
- src/util/util.c \
$(NULL)
p11_child_CFLAGS = \
$(AM_CFLAGS) \
diff --git a/src/util/cert/nss/cert.c b/src/util/cert/nss/cert.c
>
> memberof_la_SOURCES = \
>diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
>index
37b5fd7c7629e2618a1699e3ffd58110171db605..c6a5e3f61d8bfd045eb2699d0f5e279cb7d89f86 100644
>--- a/src/confdb/confdb.h
>+++ b/src/confdb/confdb.h
>@@ -71,6 +71,7 @@
> #define CONFDB_MONITOR_DEFAULT_DOMAIN "default_domain_suffix"
> #define CONFDB_MONITOR_OVERRIDE_SPACE "override_space"
> #define CONFDB_MONITOR_USER_RUNAS "user"
>+#define CONFDB_MONITOR_CERT_VERIFICATION "certificate_verification"
>
> /* Both monitor and domains */
> #define CONFDB_NAME_REGEX "re_expression"
>diff --git a/src/config/SSSDConfig/__init__.py.in
b/src/config/SSSDConfig/__init__.py.in
>index
bf61c402796122050fa43cf41128faec4771c5d2..2cb857013fe4bddfd2e79e589d3ba9721dc3ca4f 100644
>--- a/src/config/SSSDConfig/__init__.py.in
>+++ b/src/config/SSSDConfig/__init__.py.in
>@@ -60,6 +60,7 @@ option_strings = {
> 'krb5_rcache_dir' : _('Directory on the filesystem where SSSD should
store Kerberos replay cache files.'),
> 'default_domain_suffix' : _('Domain to add to names without a domain
component.'),
> 'user' : _('The user to drop privileges to'),
>+ 'certificate_verification' : _('Tune certificate
verification'),
>
> # [nss]
> 'enum_cache_timeout' : _('Enumeration cache timeout length
(seconds)'),
>diff --git a/src/config/SSSDConfigTest.py b/src/config/SSSDConfigTest.py
>index
45562214da5d227b45914abbcb298e043048adf5..8277d1ee4198f342b5cff50f8d44c973fce4a29b 100755
>--- a/src/config/SSSDConfigTest.py
>+++ b/src/config/SSSDConfigTest.py
>@@ -307,7 +307,8 @@ class SSSDConfigTestSSSDService(unittest.TestCase):
> 'reconnection_retries',
> 'fd_limit',
> 'client_idle_timeout',
>- 'description']
>+ 'description',
>+ 'certificate_verification']
>
> self.assertTrue(type(options) == dict,
> "Options should be a dictionary")
>diff --git a/src/config/etc/sssd.api.conf b/src/config/etc/sssd.api.conf
>index
72abb8b3f427e9bcf3cf3dcaa67aaf4e3e50226c..b6a396a75e564355d0828fa24858337eb06ff4bf 100644
>--- a/src/config/etc/sssd.api.conf
>+++ b/src/config/etc/sssd.api.conf
>@@ -25,6 +25,7 @@ full_name_format = str, None, false
> krb5_rcache_dir = str, None, false
> user = str, None, false
> default_domain_suffix = str, None, false
>+certificate_verification = str, None, false
>
> [nss]
> # Name service
>diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml
>index
573f421a7d885d28d5dbc03423e6c6dd84d7b8fd..05dcb4555e02da9e94c76baee65f1d7168f8026b 100644
>--- a/src/man/sssd.conf.5.xml
>+++ b/src/man/sssd.conf.5.xml
>@@ -365,6 +365,34 @@
> </para>
> </listitem>
> </varlistentry>
>+ <varlistentry>
>+ <term>certificate_verification (string)</term>
>+ <listitem>
>+ <para>
>+ With this parameter the certificate verification
>+ can be tuned with a comma separated list of
>+ options. Supported options are:
>+ <variablelist>
>+ <varlistentry>
>+ <term>no_ocsp</term>
>+ <listitem>
>+ <para>Disables Online Certificate
Status
>+ Protocol (OCSP) checks. This might be
>+ needed if the OCSP servers defined in
>+ the certificate are not reachable from
>+ the client.</para>
>+ </listitem>
>+ </varlistentry>
>+ </variablelist>
>+ </para>
>+ <para>
>+ Unknown options are reported but ignored.
>+ </para>
>+ <para>
>+ Default: not set
>+ </para>
>+ </listitem>
>+ </varlistentry>
> </variablelist>
> </para>
> </refsect2>
>diff --git a/src/p11_child/p11_child_nss.c b/src/p11_child/p11_child_nss.c
>index
22e449d547c64c94a3ee9e29939377c98443b47d..de5494d03fbec2f66a882521638e94942e964f90 100644
>--- a/src/p11_child/p11_child_nss.c
>+++ b/src/p11_child/p11_child_nss.c
>@@ -38,10 +38,12 @@
> #include <keyhi.h>
> #include <pk11pub.h>
> #include <prerror.h>
>+#include <ocsp.h>
>
> #include "util/child_common.h"
> #include "providers/dp_backend.h"
> #include "util/crypto/sss_crypto.h"
>+#include "util/cert.h"
>
> enum op_mode {
> OP_NONE,
>@@ -68,7 +70,7 @@ static char *password_passthrough(PK11SlotInfo *slot, PRBool retry,
void *arg)
>
>
> int do_work(TALLOC_CTX *mem_ctx, const char *nss_db, const char *slot_name_in,
>- enum op_mode mode, const char *pin, char **cert,
>+ enum op_mode mode, const char *pin, bool do_ocsp, char **cert,
> char **token_name_out)
> {
> int ret;
>@@ -261,6 +263,14 @@ int do_work(TALLOC_CTX *mem_ctx, const char *nss_db, const char
*slot_name_in,
> return EIO;
> }
>
>+ if (do_ocsp) {
>+ rv = CERT_EnableOCSPChecking(handle);
>+ if (rv != SECSuccess) {
>+ DEBUG(SSSDBG_OP_FAILURE, "CERT_EnableOCSPChecking failed:
[%d].\n",
>+ PR_GetError());
>+ return EIO;
>+ }
>+ }
>
> found_cert = NULL;
> DEBUG(SSSDBG_TRACE_ALL, "Filtered certificates:\n");
>@@ -456,6 +466,8 @@ int main(int argc, const char *argv[])
> char *slot_name_in = NULL;
> char *token_name_out = NULL;
> char *nss_db = NULL;
>+ bool do_ocsp = true;
>+ char *verify_opts = NULL;
>
> struct poptOption long_options[] = {
> POPT_AUTOHELP
>@@ -475,6 +487,8 @@ int main(int argc, const char *argv[])
> {"pin", 0, POPT_ARG_NONE, NULL, 'i', _("Expect PIN on
stdin"), NULL},
> {"keypad", 0, POPT_ARG_NONE, NULL, 'k', _("Expect PIN
on keypad"),
> NULL},
>+ {"verify", 0, POPT_ARG_STRING, &verify_opts, 0 , _("Tune
validation"),
>+ NULL},
> {"nssdb", 0, POPT_ARG_STRING, &nss_db, 0, _("NSS DB to
use"),
> NULL},
> POPT_TABLEEND
>@@ -614,6 +628,13 @@ int main(int argc, const char *argv[])
> }
> talloc_steal(main_ctx, debug_prg_name);
>
>+ if (verify_opts != NULL) {
>+ ret = parse_cert_verify_opts(verify_opts, &do_ocsp);
>+ if (ret != EOK) {
>+ DEBUG(SSSDBG_FATAL_FAILURE, "Failed to parse verifiy
option.\n");
>+ goto fail;
>+ }
>+ }
This change caused a warning
CC src/util/cert/nss/libsss_cert_la-cert.lo
../sssd/src/util/cert/nss/cert.c: In function ‘cert_to_ssh_key’:
../sssd/src/util/cert/nss/cert.c:246:15: error: implicit declaration of function
‘parse_cert_verify_opts’ [-Werror=implicit-function-declaration]
ret = parse_cert_verify_opts(verify_opts, &do_ocsp);
^
cc1: all warnings being treated as errors
You forgot to include "util/cert.h" to this file.
BTW if you don't mind you can also remove util/util.h from this file.
It is not used. It might have been included only for config.h
Thanks, I included the changes.
diff --git a/src/util/cert/nss/cert.c b/src/util/cert/nss/cert.c
index ab3113d..f95a87b 100644
--- a/src/util/cert/nss/cert.c
+++ b/src/util/cert/nss/cert.c
@@ -18,7 +18,7 @@
along with this program. If not, see <
http://www.gnu.org/licenses/>.
*/
-#include "util/util.h"
+#include "config.h"
#include <nss.h>
#include <cert.h>
@@ -26,9 +26,11 @@
#include <key.h>
#include <prerror.h>
#include <ocsp.h>
+#include <talloc.h>
#include "util/crypto/sss_crypto.h"
#include "util/crypto/nss/nss_util.h"
+#include "util/cert.h"
#define NS_CERT_HEADER "-----BEGIN CERTIFICATE-----"
#define NS_CERT_TRAILER "-----END CERTIFICATE-----"
New version attached.
bye,
Sumit