[SSSD] [PATCH] p11: enable ocsp checks

Lukas Slebodnik lslebodn at redhat.com
Tue Nov 10 11:40:23 UTC 2015


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

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

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

LS


More information about the sssd-devel mailing list