On Thu, Nov 12, 2015 at 01:32:13PM +0100, Sumit Bose wrote:
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.
I think removing a circular dependency is always a good idea. We have
too many of those..
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.
I also agree here.
[...]
New version attached.
bye,
Sumit
Can you rebase the patch please? I'm afraid it conflicts with the one
that added the diag_cmd. In the meantime, I check the diff..
From 3ff6cf406fa40e0b621794013e18afaabc071986 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 | 2 ++
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 | 25 +++++++++++++++++++-
src/tests/cmocka/test_cert_utils.c | 2 +-
src/tests/cmocka/test_pam_srv.c | 41 +++++++++++++++++++++++++-------
src/util/cert.h | 1 +
src/util/cert/libcrypto/cert.c | 1 +
src/util/cert/nss/cert.c | 15 +++++++++++-
src/util/util.c | 46 ++++++++++++++++++++++++++++++++++++
src/util/util.h | 3 +++
18 files changed, 209 insertions(+), 22 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index a0abb8fb3a9cc19cfabad8066c6c65d78e78d206..2c3baa788d7411a66afb55e42565c6095a81cc6a
100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -3156,6 +3156,7 @@ 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) \
@@ -3165,6 +3166,7 @@ p11_child_CFLAGS = \
p11_child_LDADD = \
libsss_debug.la \
$(TALLOC_LIBS) \
+ $(DHASH_LIBS) \
$(POPT_LIBS) \
$(NSS_LIBS) \
libsss_crypt.la \
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
Can you clarify here what does the not set do?
+ </para>
+ </listitem>
+ </varlistentry>
</variablelist>
</para>
</refsect2>
The code itself looks good to me, but I only read the diff.