Hi, On 03-11-16 19:21, David Sommerseth wrote: > On 28/10/16 17:54, Steffan Karger wrote: >> This patch refactors the CRL handling to rely more on the >> implementation of the crypto library. It will insert the CRL at >> the correct time to keep it up to date, but all additional >> verification logic is removed from ssl_verify_<backend>.c. "Less >> code of our own, less bugs of our own." > >> In practice, this means extra checks will be performed on the CRL, >> such as checking it validBefore and validAfter fields. > >> This patch was originally written by Ivo Manca, and then molded by >> Steffan before sending to the list. All bugs are Steffan's fault. > >> Thanks also go to Antonio Quartulli for useful feedback. He'll >> send follow-up patches to improve CRL handling performance. > >> Signed-off-by: Ivo Manca <ivo.ma...@fox-it.com> Signed-off-by: >> Steffan Karger <steffan.kar...@fox-it.com> --- Changes.rst >> | 6 +++ src/openvpn/ssl.c | 15 +++++++ >> src/openvpn/ssl_backend.h | 11 +++++ >> src/openvpn/ssl_mbedtls.c | 43 +++++++++++++++++- >> src/openvpn/ssl_mbedtls.h | 1 + src/openvpn/ssl_openssl.c >> | 58 ++++++++++++++++++++++++ src/openvpn/ssl_verify.c | 19 >> ++++---- src/openvpn/ssl_verify_backend.h | 19 +++----- >> src/openvpn/ssl_verify_mbedtls.c | 56 +++-------------------- >> src/openvpn/ssl_verify_openssl.c | 96 >> ++++++++++++++++------------------------ 10 files changed, 193 >> insertions(+), 131 deletions(-) > > This is a bit scary. I have nothing to complain about in the patch, > so I feel like I'm not seeing the large neon-pink elephant in the room.
What probably makes a difference here is that this patch was not originally written by me ;) Seriously though, the patch was written by my colleague, reviewed and slightly adjusted by me, then got some comments from Antonio and then finally molded to it's current form. So it already got quite some off-list review. Also, I ran our OpenVPN-NL test suite against (the mbed TLS version of) this patch, which verifies correct behaviour for a number of more advanced CA setups (including intermediate CAs, revoked intermediates, invalid CRLS, etc...). Still, this does change behaviour (e.g. rejecting expired CRLS), so will probably reject some connections that earlier versions would accept. But I'd consider that a fix, not a bug. > I've run lots of tests, both with OpenSSL and mbedTLS. Testing with > and without valgrind --leak-check=full. Tested configurations without > --crl, with --crl ca.crl and --crl crldir dir. > > Every thing works as expected, no memory leaks detected. It passes > 'make check' including t_client.sh runs. Traffic is passed over the > tunnel where certificate is not listed on a CRL. > > > There is one detail, which is not a fault of this patch ... but I'll > just mention it here. The client does not exit gracefully on > rejections due to CRL. It's not too odd, but should probably look > into if it would be possible to send an "AUTH_FAILED,REVOKED: > Certificate revoked" message back to the client. But that's for a > different patch. We can't send AUTH_FAILED, because we have no control channel to send it over yet (see other mail thread). > I can definitely give this one a Feature-ACK. I can also support a > full ACK too if one more developer does a thorough code review. Thanks! -Steffan
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------------ Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
