serf-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Branko ─îibej <br...@apache.org>
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 14:57:44 GMT
On 11.12.2016 14:55, Daniel Shahaf wrote:
> 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.

Sure it is. I was just being consistent with SERF_BAD_RESPONSE_ERROR().

>> +++ 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//

Thanks ...

>> + * 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,

That would make things simpler for users; and could be used for the
nonce, too.

On the other hand, I could instead invent accessor functions and an
opaque serf_ssl_ocsp_request_t (and similarly for ..._response_t?). I
think that will make request and response handling simpler for the API
consumer.

>> + * 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.

The caller would send the nonce into serf_ssl_ocsp_request_verify() to
check that the response contains the same nonce. The nonce is optional
in the OCSP request, but can be used for avoiding replay attacks.
Apparently some OCSP responders do not handle requests with nonces, so
we can't just implicitly include one. Other than that, OpenSSL can

> 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.)

Well .. OpenSSL provides a highl-level function for that,
OCSP_check_nonce, but that needs the internal representation of both the
request and the response ... I suppose we could require the application
to hold on to the request body (as returned by
serf_ssl_ocsp_request_create()) and send that to the _verify() function.
That would reduce the 'nonce' parameter to a flag saying, 'yes, generate
a nonce'.

>> + * The request and nonce will be allocated from @a pool.
> Rename the argument to result_pool, then?  (And add scratch_pool if needed)

It's fun how I wanted to do that but, given that the Serf API doesn't
use separate result and scratch pools anywhere, I held back ...

>> + */
>> +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?

Most likely the OCSP responder will reject the request as invalid. I
wouldn't try to make the API any more foolproof.

>> +    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?

Ack.

>
>> + * 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?

Ah, one of the new ones ... I'll document that.


>
>> + */
>> +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.

Yup. Or the opaque-type + accessor approach.


>> +    const serf_ssl_certificate_t *server_cert,
>> +    const serf_ssl_certificate_t *issuer_cert,
> Same ordering question as above.

But different answer: in this case, the response will be flagged as
invalid during verification. RFC 2560 requires that the response a) is
for the server cert and b) is signed by either the issuer cert, or a
special responder cert that's issued by the issuer cert. Switch these
around and these conditions will fail.



-- Brane


Mime
View raw message