Hello!

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

[...]

> # HG changeset patch
> # User Piotr Sikora <piotr@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

_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel