serf-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel Shahaf <...@daniel.shahaf.name>
Subject Re: svn commit: r1773567 - in /serf/branches/ocsp-verification: BRANCH-README serf.h serf_bucket_types.h src/context.c
Date Sun, 11 Dec 2016 13:55:04 GMT
brane@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

Mime
View raw message