On Fri, Nov 20, 2015 at 01:36:02PM +0100, Jakub Hrozek wrote:
On Tue, Nov 17, 2015 at 01:51:53PM +0100, Sumit Bose wrote:
> rebased version attached.
Thank you, the code looks good, I only have two questions inline.
> From 465e6b483786910c110d2ad48335a5ceb48d9f1a 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
[...]
> --- 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
Does it make sense to explicitly say here that the default is to perform
the check?
I added a small explanation
> + </para>
> + </listitem>
> + </varlistentry>
> </variablelist>
> </para>
> </refsect2>
[...]
> diff --git a/src/util/cert/libcrypto/cert.c b/src/util/cert/libcrypto/cert.c
> index
01f9554b990d6a139bb9a1d8d558c1c3f6bb745c..4e2dbe70cafcb31e9ca4cc232dacb0cc176b0dc0 100644
> --- a/src/util/cert/libcrypto/cert.c
> +++ b/src/util/cert/libcrypto/cert.c
> @@ -172,6 +172,7 @@ done:
>
> errno_t cert_to_ssh_key(TALLOC_CTX *mem_ctx, const char *ca_db,
> const uint8_t *der_blob, size_t der_size,
> + bool do_ocsp,
Does it make sense to either add a comment here or even a ticket to add
the matching functionality for libcrypto?
I opened
https://fedorahosted.org/sssd/ticket/2880 as a reminder and
there already is a comment in the libcrypto version of
cert_to_ssh_key().
> uint8_t **key, size_t *key_size)
> {
> int ret;
Otherwise ACK. I don't have the hardware required for testing (yet :-))
so I only read the code and submitted to Coverity and CI.
Version with updates man page attached.
bye,
Sumit