br...@apache.org wrote on Sun, Dec 11, 2016 at 12:32:57 -0000:
> Author: brane
> Date: Sun Dec 11 12:32:57 2016
> New Revision: 1773567
>
> URL: http://svn.apache.org/viewvc?rev=1773567&view=rev
> Log:
> On the ocsp-verification branch: Prepare prototypes and error codes
> for OCSP response creation and verification.
>
> * BRANCH-README: Update branch docs.
>
> * serf.h
> (SERF_ERROR_SSL_OCSP_RESPONSE_CERT_REVOKED,
> SERF_ERROR_SSL_OCSP_RESPONSE_CERT_UNKNOWN,
> SERF_ERROR_SSL_OCSP_RESPONSE_INVALID): New error codes.
> (SERF_OCSP_UNGOOD_ERROR): New error-checking utility macro.
>
> * serf_bucket_types.h
> (serf_ssl_ocsp_request_create,
> serf_ssl_ocsp_response_verify): New prototypes.
>
> * src/context.c
> (serf_error_string): Add error strings for the new error codes.
>
> +++ serf/branches/ocsp-verification/serf.h Sun Dec 11 12:32:57 2016
> @@ -143,6 +143,19 @@ typedef struct serf_config_t serf_config
> +#define SERF_OCSP_UNGOOD_ERROR(status) ((status) \
> + && ((SERF_ERROR_SSL_OCSP_CERT_REVOKED == (status)) \
> + ||(SERF_ERROR_SSL_OCSP_CERT_UNKNOWN == (status))))
The "(status) &&" conjunct is redundant. I assume the optimizer would
just remove the nonzero-p check unless __builtin_expect were used, but
I haven't checked the generated code.
> +++ serf/branches/ocsp-verification/serf_bucket_types.h Sun Dec 11 12:32:57
> 2016
> @@ -769,6 +769,53 @@ apr_status_t
> serf_ssl_check_cert_status_request(serf_ssl_context_t *ssl_ctx, int enabled);
>
> /**
> + * Constructs an OCSP verification request for @a server_cert with
> + * issuer certificate @a issuer_cert, Retyurns the DER encoded
Typo s/y//
> + * request in @a ocsp_request and its size in @a ocsp_request_size.
> + *
Maybe use a counted-string type to make the coupling explicit?
struct serf_string_t { const void *data; apr_size_t length; }
struct serf_string_t *ocsp_request,
> + * If @a nonce is not @c NULL, the request will contain a randomly
> + * generated nonce, which will be returned in @a *nonce and its
> + * size in @a nonce_size. If @a nonce is @c NULL, @a nonce_size
> + * is ignored.
What is the caller expected to do with the nonce? Why is it exposed in
the API? I'd consider making the nonce opaque (i.e.: hide its length
from the caller), to remove the opportunity for the caller to handle the
nonce wrongly.
For that matter, I wouldn't try to write code that generates or compares
nonces, either; I'd leave that to openssl. (Haven't looked at the
implementation.)
> + * The request and nonce will be allocated from @a pool.
Rename the argument to result_pool, then? (And add scratch_pool if needed)
> + */
> +apr_status_t serf_ssl_ocsp_request_create(
> + const serf_ssl_certificate_t *server_cert,
> + const serf_ssl_certificate_t *issuer_cert,
What will happen if an API caller passes these two parameters in the
wrong order?
> + const void **ocsp_request,
> + apr_size_t *ocsp_request_size,
> + const void **nonce,
> + apr_size_t *nonce_size,
> + apr_pool_t *pool);
> +
> +/**
> + * Check if the given @a ocsp_response of size @a ocsp_response_size
> + * is valid for the given @a server_cert, @a issuer_cert and @a nonce.
> + *
> + * If @a nonce is @c NULL, the response _must not_ contain a nonce.
> + * Otherwise, it must contain an identical nonce with size @a nonce_size.
> + *
Use doxygen bold/italic markup instead of underscores?
> + * The @a this_update, @a next_update and @a produced_at output arguments
> + * are described in RFC 2560, section 2.4 and, when not @c NULL, will be
> + * set from the parsed response. Any of these times that are not present
> + * in the response will be set to the epoch, i.e., @c APR_TIME_C(0).
> + *
> + * Uses @a pool for temporary allocations.
What error code is returned for an invalid response?
> + */
> +apr_status_t serf_ssl_ocsp_response_verify(
> + const void *ocsp_response,
> + apr_size_t ocsp_response_size,
Another opportunity to use the aforementioned counted-length string type.
> + const serf_ssl_certificate_t *server_cert,
> + const serf_ssl_certificate_t *issuer_cert,
Same ordering question as above.
> + const void *nonce,
> + apr_size_t nonce_size,
> + apr_time_t *this_update,
> + apr_time_t *next_update,
> + apr_time_t *produced_at,
> + apr_pool_t *pool);
Cheers,
Daniel
