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..