You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@serf.apache.org by br...@apache.org on 2016/12/11 12:32:57 UTC
svn commit: r1773567 - in /serf/branches/ocsp-verification: BRANCH-README
serf.h serf_bucket_types.h src/context.c
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.
Modified:
serf/branches/ocsp-verification/BRANCH-README
serf/branches/ocsp-verification/serf.h
serf/branches/ocsp-verification/serf_bucket_types.h
serf/branches/ocsp-verification/src/context.c
Modified: serf/branches/ocsp-verification/BRANCH-README
URL: http://svn.apache.org/viewvc/serf/branches/ocsp-verification/BRANCH-README?rev=1773567&r1=1773566&r2=1773567&view=diff
==============================================================================
--- serf/branches/ocsp-verification/BRANCH-README (original)
+++ serf/branches/ocsp-verification/BRANCH-README Sun Dec 11 12:32:57 2016
@@ -46,22 +46,32 @@ These are the proposed changes:
through serf_ssl_load_cert_file() is neither easy nor sane).
-3. serf_ocsp_request_create()
+3. serf_ssl_ocsp_request_create()
Add a new function that can be used from within a request setup
handler to create an OCSP request:
- apr_status_t serf_ocsp_request_create(
+ apr_status_t serf_ssl_ocsp_request_create(
const serf_ssl_certificate_t *server_cert,
const serf_ssl_certificate_t *issuer_cert,
- const char **ocsp_request,
+ const void **ocsp_request,
+ apr_size_t *ocsp_request_size,
+ const void **nonce,
+ apr_size_t *nonce_size,
apr_pool_t *pool);
Docstring:
Constructs an OCSP verification request for @a server_cert with
- issuer certificate @a issuer_cert, returning the DER encoded
- request in @a ocsp_request, allocated from @a pool.
+ issuer certificate @a issuer_cert, Retyurns the DER encoded
+ request in @a ocsp_request and its size in @a ocsp_request_size.
+
+ 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.
+
+ The request and nonce will be allocated from @a pool.
Discussion:
@@ -72,6 +82,54 @@ These are the proposed changes:
request headers.
-4. serf_ocsp_response_parse()
+4. serf_ssl_ocsp_response_verify()
+
+ Add a new function that can be used from within a response handler
+ to verify an OCSP response:
+
+ apr_status_t serf_ssl_ocsp_response_verify(
+ const void *ocsp_response,
+ apr_size_t ocsp_response_size,
+ const serf_ssl_certificate_t *server_cert,
+ const serf_ssl_certificate_t *issuer_cert,
+ 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);
+
+ Docstring:
+
+ 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.
+
+ 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.
+
+ Discussion:
+
+ Parses and verifies the OCSP response received in the HTTP response
+ body as per RFC 2560, section 3.2.
+
+
+5. New error codes and macros
+
+ #define SERF_ERROR_SSL_OCSP_RESPONSE_CERT_REVOKED
+ #define SERF_ERROR_SSL_OCSP_RESPONSE_CERT_UNKNOWN
+ #define SERF_ERROR_SSL_OCSP_RESPONSE_INVALID
+
+ #define SERF_OCSP_UNGOOD_ERROR(status)
+
+ Discussion:
- TBD: Parse an OCSP response.
+ These error codes are returned from serf_ssl_ocsp_response_verify().
+ The SERF_OCSP_UNGOOD_ERROR() macro combines the _CERT_REVOKED
+ and _CERT_UNKNOWN error codes..
Modified: serf/branches/ocsp-verification/serf.h
URL: http://svn.apache.org/viewvc/serf/branches/ocsp-verification/serf.h?rev=1773567&r1=1773566&r2=1773567&view=diff
==============================================================================
--- serf/branches/ocsp-verification/serf.h (original)
+++ serf/branches/ocsp-verification/serf.h Sun Dec 11 12:32:57 2016
@@ -143,6 +143,19 @@ typedef struct serf_config_t serf_config
on a connection that uses HTTP pipelining. */
#define SERF_ERROR_SSL_NEGOTIATE_IN_PROGRESS (SERF_ERROR_START + 73)
+/* OCSP responder says that the certificate is revoked. */
+#define SERF_ERROR_SSL_OCSP_RESPONSE_CERT_REVOKED (SERF_ERROR_START + 74)
+
+/* OCSP responder says that the certificate is unknown. */
+#define SERF_ERROR_SSL_OCSP_RESPONSE_CERT_UNKNOWN (SERF_ERROR_START + 75)
+
+/* The response from an OCSP responder was not valid. */
+#define SERF_ERROR_SSL_OCSP_RESPONSE_INVALID (SERF_ERROR_START + 76)
+
+#define SERF_OCSP_UNGOOD_ERROR(status) ((status) \
+ && ((SERF_ERROR_SSL_OCSP_CERT_REVOKED == (status)) \
+ ||(SERF_ERROR_SSL_OCSP_CERT_UNKNOWN == (status))))
+
/* General authentication related errors */
#define SERF_ERROR_AUTHN_FAILED (SERF_ERROR_START + 90)
Modified: serf/branches/ocsp-verification/serf_bucket_types.h
URL: http://svn.apache.org/viewvc/serf/branches/ocsp-verification/serf_bucket_types.h?rev=1773567&r1=1773566&r2=1773567&view=diff
==============================================================================
--- serf/branches/ocsp-verification/serf_bucket_types.h (original)
+++ 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
+ * request in @a ocsp_request and its size in @a ocsp_request_size.
+ *
+ * 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.
+ *
+ * The request and nonce will be allocated from @a pool.
+ */
+apr_status_t serf_ssl_ocsp_request_create(
+ const serf_ssl_certificate_t *server_cert,
+ const serf_ssl_certificate_t *issuer_cert,
+ 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.
+ *
+ * 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.
+ */
+apr_status_t serf_ssl_ocsp_response_verify(
+ const void *ocsp_response,
+ apr_size_t ocsp_response_size,
+ const serf_ssl_certificate_t *server_cert,
+ const serf_ssl_certificate_t *issuer_cert,
+ 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);
+
+/**
* Enable or disable SSL compression on a SSL session.
* @a enabled = 1 to enable compression, 0 to disable compression.
* Default = disabled.
Modified: serf/branches/ocsp-verification/src/context.c
URL: http://svn.apache.org/viewvc/serf/branches/ocsp-verification/src/context.c?rev=1773567&r1=1773566&r2=1773567&view=diff
==============================================================================
--- serf/branches/ocsp-verification/src/context.c (original)
+++ serf/branches/ocsp-verification/src/context.c Sun Dec 11 12:32:57 2016
@@ -389,6 +389,13 @@ const char *serf_error_string(apr_status
return "An error occurred during SSL setup";
case SERF_ERROR_SSL_CERT_FAILED:
return "An SSL certificate related error occurred ";
+ case SERF_ERROR_SSL_OCSP_RESPONSE_CERT_REVOKED:
+ return "An OCSP responder declared an SSL certificate is revoked";
+ case SERF_ERROR_SSL_OCSP_RESPONSE_CERT_UNKNOWN:
+ return "An OCSP responder declared an SSL certificate is unknown";
+ case SERF_ERROR_SSL_OCSP_RESPONSE_INVALID:
+ return "An OCSP responder returned an invalid response";
+
case SERF_ERROR_AUTHN_FAILED:
return "An error occurred during authentication";
case SERF_ERROR_AUTHN_NOT_SUPPORTED:
Re: svn commit: r1773567 - in /serf/branches/ocsp-verification:
BRANCH-README serf.h serf_bucket_types.h src/context.c
Posted by Branko Čibej <br...@apache.org>.
On 11.12.2016 13:32, brane@apache.org wrote:
> 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.
[...]
Would appreciate another pair of eyes looking at the prototypes
serf_ssl_ocsp_request_create and serf_ssl_ocsp_response_verify. While
they're functionally complete, they are pretty hefty ... any suggestions
for improvements are very welcome.
-- Brane
Re: svn commit: r1773567 - in /serf/branches/ocsp-verification:
BRANCH-README serf.h serf_bucket_types.h src/context.c
Posted by Branko Čibej <br...@apache.org>.
On 11.12.2016 15:57, Branko \u010cibej wrote:
> 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
... generate a random nonce using its internal random generator, which I
tend to presume is, all things being equal, appropriate for use in
crypto applications.
-- Brane
Re: svn commit: r1773567 - in /serf/branches/ocsp-verification:
BRANCH-README serf.h serf_bucket_types.h src/context.c
Posted by Branko Čibej <br...@apache.org>.
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
Re: svn commit: r1773567 - in /serf/branches/ocsp-verification:
BRANCH-README serf.h serf_bucket_types.h src/context.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
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