Re: [Openvpn-devel] [PATCH 3/3] openssl: implement crl-persist related functions

2016-10-20 Thread David Sommerseth
On 20/10/16 10:42, Antonio Quartulli wrote:
> On Wed, Oct 19, 2016 at 02:22:31PM +0800, Antonio Quartulli wrote:
>> Implement the functions needed by the crl-persist logic when openssl
>> is enabled. Such functions are used in the ssl_verify module.
>>
>> Note that the CRL file is stored in an adhoc data structure and no
>> openssl specific object is used. The data structure being used is a
>> sorted array or serials that can later be looked up in log(N) with
>> a binary search, thus guaranteeing a fast lookup operation.
>>
>> Such data structure may be changed in the future with an optimized
>> openssl specific object.
>>
>> Tests have been performed by using a CRL file having size 143MB.
>> Original delay upon client connection was around 5-8 seconds.
>> With this patch the delay gets close to 0.
>>
>> Signed-off-by: Antonio Quartulli 
> 
> As discussed on IRC, it might be better to first change the CRL handling code 
> in
> the OpenSSL module to use the internal routines provided by the OpenSSL 
> library.
> (apparently a patch to implement this change is in the work on somebody else's
> desk)
> 
> 
> At that point my patch could be changed to re-use the same code instead of
> implementing my own optimized logic.
> 
> 
> Note: also OpenSSL uses a sorted array + bsearch for CRL handling, therefore
> the performance of OpenSSL vs my approach should be similar.
> 
> 
> Does anybody else have any opinion against this?

First of all, thank you very much for this patch set!  As I said on the
IRC, this patch-set is well done when it comes to overall style and
layout.   I have not dug into the details, but the overall quick look
gave a good impression.

This new feature sounds reasonable.  The improved OpenSSL CRL handling
will be most welcome.  So basing your work on top of that makes a lot of
sense, and hopefully it can simplify your patch as well.

But I also think that we should look more carefully at the whole CRL
handling as a hole unit.  To me the --crl-persist looks like a
reasonable feature, but I'm still hoping to see more responses to this
feature from others as well.  But if it turns out that we can implement
this feature and also improve the current CRL handling, that will
hopefully give even less code duplication and an even simpler patch to
when adding the --crl-persist feature.

On IRC we also briefly discussed that the mbedTLS CRL code oaths is not
too clever and efficient (like, no btree search, just a simple
for-loop).  I would encourage you to look if you can improve the mbedTLS
code too, and provide that patch to them.  I'm sure the mbedTLS guys
will appreciate that effort.  This will again indirectly benefit us, and
we need to have even less specialized code to make the mbedTLS
integration perform well.  So it's a win-win for all of us :)

And I'm happy to see more people looking into these parts of the OpenVPN
code as well!  So thank you!


-- 
kind regards,

David Sommerseth




signature.asc
Description: OpenPGP digital signature
--
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH 3/3] openssl: implement crl-persist related functions

2016-10-20 Thread Antonio Quartulli
On Wed, Oct 19, 2016 at 02:22:31PM +0800, Antonio Quartulli wrote:
> Implement the functions needed by the crl-persist logic when openssl
> is enabled. Such functions are used in the ssl_verify module.
> 
> Note that the CRL file is stored in an adhoc data structure and no
> openssl specific object is used. The data structure being used is a
> sorted array or serials that can later be looked up in log(N) with
> a binary search, thus guaranteeing a fast lookup operation.
> 
> Such data structure may be changed in the future with an optimized
> openssl specific object.
> 
> Tests have been performed by using a CRL file having size 143MB.
> Original delay upon client connection was around 5-8 seconds.
> With this patch the delay gets close to 0.
> 
> Signed-off-by: Antonio Quartulli 

As discussed on IRC, it might be better to first change the CRL handling code in
the OpenSSL module to use the internal routines provided by the OpenSSL library.
(apparently a patch to implement this change is in the work on somebody else's
desk)


At that point my patch could be changed to re-use the same code instead of
implementing my own optimized logic.


Note: also OpenSSL uses a sorted array + bsearch for CRL handling, therefore
the performance of OpenSSL vs my approach should be similar.


Does anybody else have any opinion against this?



Cheers,

-- 
Antonio Quartulli


signature.asc
Description: Digital signature
--
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH 3/3] openssl: implement crl-persist related functions

2016-10-19 Thread Antonio Quartulli
Implement the functions needed by the crl-persist logic when openssl
is enabled. Such functions are used in the ssl_verify module.

Note that the CRL file is stored in an adhoc data structure and no
openssl specific object is used. The data structure being used is a
sorted array or serials that can later be looked up in log(N) with
a binary search, thus guaranteeing a fast lookup operation.

Such data structure may be changed in the future with an optimized
openssl specific object.

Tests have been performed by using a CRL file having size 143MB.
Original delay upon client connection was around 5-8 seconds.
With this patch the delay gets close to 0.

Signed-off-by: Antonio Quartulli 
---
 src/openvpn/ssl_verify_openssl.c | 183 +++
 1 file changed, 183 insertions(+)

diff --git a/src/openvpn/ssl_verify_openssl.c b/src/openvpn/ssl_verify_openssl.c
index a4b9432..c5649ca 100644
--- a/src/openvpn/ssl_verify_openssl.c
+++ b/src/openvpn/ssl_verify_openssl.c
@@ -47,6 +47,9 @@
 #include 
 #include 
 
+#define __STDC_FORMAT_MACROS
+#include 
+
 int
 verify_callback (int preverify_ok, X509_STORE_CTX * ctx)
 {
@@ -625,6 +628,186 @@ x509_write_pem(FILE *peercert_file, X509 *peercert)
   return SUCCESS;
 }
 
+static int
+x509_serial_cmp(const void *a, const void *b)
+{
+  const ASN1_INTEGER **serial1 = (const ASN1_INTEGER **)a;
+  const ASN1_INTEGER **serial2 = (const ASN1_INTEGER **)b;
+
+  ASSERT(serial1 && *serial1 && serial2 && *serial2);
+
+  return ASN1_INTEGER_cmp(*serial1, *serial2);
+}
+
+result_t
+x509_verify_crl_mem(const openvpn_x509_crl_t *crl, X509 *cert,
+   const char *subject)
+{
+  result_t ret = FAILURE;
+  struct gc_arena gc = gc_new();
+  ASN1_INTEGER *serial;
+
+  ASSERT(crl && cert);
+
+  /* authorize the client if the CRL list is empty */
+  if (crl->num == 0)
+{
+  ret = SUCCESS;
+  goto end;
+}
+
+  /*
+   * make sure the issuer of the CRL and the certificate are the same,
+   * otherwise the check cannot be performed
+   */
+  if (X509_NAME_cmp(crl->issuer, X509_get_issuer_name(cert)) != 0)
+  {
+msg(M_WARN, "CRL: CRL [in-memory] is from a different issuer than the 
issuer of certificate %s",
+   subject);
+ret = SUCCESS;
+goto end;
+  }
+
+  serial = X509_get_serialNumber(cert);
+  if (!serial)
+{
+  msg(M_WARN, "CRL: can't extract serial from cert");
+  goto end;
+}
+
+  /*
+   * perform binary search on sorted CRL array. The cmp function assumes that
+   * the arguments are all pointers to pointers, therefore the address of the
+   * "key" object has to be passed instead of the object itself.
+   */
+  if (bsearch(, crl->buff, crl->num, sizeof(ASN1_INTEGER *),
+ x509_serial_cmp))
+{
+   fprintf(stderr, "!\n");
+  msg(D_HANDSHAKE, "CRL CHECK FAILED: %s (serial %s) is REVOKED", subject,
+ format_hex_ex(serial->data, serial->length, 0, 1, ":", ));
+  goto end;
+}
+
+  /* serial not found in CRL: certificate is valid */
+  ret = SUCCESS;
+  msg(D_HANDSHAKE, "CRL CHECK OK: %s", subject);
+
+end:
+  gc_free();
+
+  return ret;
+}
+
+void
+x509_crl_free(openvpn_x509_crl_t *crl)
+{
+  int i;
+
+  ASSERT(crl);
+
+  for (i = 0; i < crl->num; i++)
+ASN1_INTEGER_free(crl->buff[i]);
+
+  free(crl->buff);
+  X509_NAME_free(crl->issuer);
+  crl->buff = NULL;
+  crl->issuer = NULL;
+  crl->num = 0;
+}
+
+result_t
+x509_load_crl_mem(openvpn_x509_crl_t *crl, const char *crl_file)
+{
+  result_t ret = FAILURE;
+  X509_CRL *x509_crl = NULL;
+  X509_REVOKED *revoked;
+  X509_NAME *issuer;
+  BIO *in = NULL;
+  uint64_t num, i;
+
+  ASSERT(crl && crl_file);
+
+  msg(D_TLS_DEBUG_LOW, "CRL-persist: loading file: %s", crl_file);
+
+  /* free previously allocated data - useful in case or context update */
+  if (crl->buff)
+x509_crl_free(crl);
+
+  in = BIO_new_file(crl_file, "r");
+  if (!in)
+{
+  msg(M_WARN, "CRL: cannot read: %s", crl_file);
+  goto end;
+}
+
+  x509_crl = PEM_read_bio_X509_CRL(in, NULL, NULL, NULL);
+  if (!x509_crl)
+{
+  msg(M_WARN, "CRL: cannot read CRL from file %s", crl_file);
+  goto end;
+}
+
+  crl->num = 0;
+  crl->buff = NULL;
+  crl->issuer = NULL;
+
+  issuer = X509_CRL_get_issuer(x509_crl);
+  if (issuer)
+crl->issuer = X509_NAME_dup(issuer);
+
+  num = sk_X509_REVOKED_num(X509_CRL_get_REVOKED(x509_crl));
+  /* CRL is empty */
+  if (num == (uint64_t)-1)
+{
+  ret = SUCCESS;
+  goto end;
+}
+
+  crl->num = num;
+  crl->buff = malloc(sizeof(ASN1_INTEGER **) * num);
+  if (!crl->buff)
+{
+  msg(M_WARN, "CRL: cannot allocate CRL buffer for file %s", crl_file);
+  goto end;
+}
+
+  /* store each serial number in the buffer array as ASN1_INTEGER objects */
+  for (i = 0; i < num; i++)
+{
+  revoked = sk_X509_REVOKED_value(X509_CRL_get_REVOKED(x509_crl), i);
+  crl->buff[i] = ASN1_INTEGER_dup(revoked->serialNumber);
+  if