[PATCH] OCSP stapling: better handling of successful OCSP responses.

[Maxim Dounin]

Hello!

On Tue, May 21, 2013 at 05:17:52PM -0700, Piotr Sikora wrote:

[...]

> # HG changeset patch
> # User Piotr Sikora <piotr at cloudflare.com>
> # Date 1369181290 25200
> # Node ID 7f52714a0b6a4229ab2fa9e640edbf80060eaf34
> # Parent  1d68b502088c9d6e6603e9699354e36d03d77f9c
> OCSP stapling: add ssl_stapling_strict directive.
> 
> When turned on (default), nginx will cache only OCSP responeses
> with "good" certificate status.
> 
> When turned off, nginx will cache all successful OCSP responseses,
> regardless of the certificate status.

Both directive name and flag name looks Directive name doesn't looks good.

> While there, log certificate's common name and revocation reason,
> because certificate status alone isn't very useful information.

You may want to submit this as a separate patch.  It might be also 
good idea to print certificate's information in 
ngx_ssl_ocsp_log_error().

It might be also good idea to use certificate file name instead of 
certificate subject (the same subject can be used for more than 
one certificate... on the other hand, there may be more than one 
certificate in a file - but only first one will be used for 
stapling).

[...]

> @@ -615,19 +621,47 @@
>          goto error;
>      }
> 
> -    if (n != V_OCSP_CERTSTATUS_GOOD) {
> -        ngx_log_error(NGX_LOG_ERR, ctx->log, 0,
> -                      "certificate status \"%s\" in the OCSP response",
> -                      OCSP_cert_status_str(n));
> -        goto error;
> -    }
> -

I don't think that changing check order is a good idea.

>      if (OCSP_check_validity(thisupdate, nextupdate, 300, -1) != 1) {
>          ngx_ssl_error(NGX_LOG_ERR, ctx->log, 0,
>                        "OCSP_check_validity() failed");
>          goto error;
>      }
> 
> +    if (n != V_OCSP_CERTSTATUS_GOOD) {
> +        ngx_str_set(&s, "unknown");
> +
> +        if (ctx->cert) {
> +            name = X509_get_subject_name(ctx->cert);
> +            if (name) {
> +                idx = X509_NAME_get_index_by_NID(name, NID_commonName, -1);
> +                if (idx != -1) {
> +                    entry = X509_NAME_get_entry(name, idx);
> +                    if (entry) {
> +                        str = X509_NAME_ENTRY_get_data(entry);
> +                        s.data = ASN1_STRING_data(str);
> +                        s.len = ASN1_STRING_length(str);
> +                    }
> +                }
> +            }
> +        }

See above, it probably should be moved to 
ngx_ssl_ocsp_log_error(), and changed to print certificate file 
name instead.

> +
> +        if (n == V_OCSP_CERTSTATUS_REVOKED && r != -1) {
> +            ngx_log_error(NGX_LOG_ERR, ctx->log, 0,
> +                          "certificate status \"%s\" (reason: \"%s\") in the "
> +                          "OCSP response for \"%V\"",
> +                          OCSP_cert_status_str(n), OCSP_crl_reason_str(r), &s);
> +
> +        } else {
> +            ngx_log_error(NGX_LOG_ERR, ctx->log, 0,
> +                          "certificate status \"%s\" in the OCSP response "
> +                          "for \"%V\"", OCSP_cert_status_str(n), &s);
> +        }

Is printing revocation reason is really needed in error logs?  It 
looks like too many lines of code just to print an additional 
minor detail.

[...]

-- 
Maxim Dounin
http://nginx.org/en/donation.html