apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Orton <jor...@redhat.com>
Subject veto on addition of ssl, evp code
Date Mon, 14 Apr 2008 20:42:54 GMT
Per the other thread, the review for this has mostly been ignored. I 
have no wish to waste time banging my head against a wall, so I am:

-1 on the addition of the SSL code, r415639 et al, on the basis that:

a) the API is undocumented in basic ways for key functions; e.g. it is 
unspecified to what purpose the certs, keys, and digest are passed in 
creating a "factory".

b) providing an SSL client implementation without any way to verify that 
the server identity matches the presented cert is a security issue.  
(CVE names have been allocated for manifestations of this problem in 
other software)

c) it is a leaky abstraction; the way errors are handled is by exposing 
the OpenSSL native error codes to the caller, which the caller can only 
interpret by knowing which SSL toolkit the code is linked against

d) there seems to be no demonstration that the abstraction is even 
*possible* to implement on anything other than OpenSSL; the WinSock code 
is mostly commented out or stubbed.

e) it has systematic error handling problems, returning '-1' as an 
apr_status_t value in many places.

f) systematically breaks APR code style by doing argument validation

g) the OpenSSL implementation plainly does not meet the documented 
(copy'n'pasted) API constraints in at least the handling of non-blocking 

and I am also -1 on the addition of the EVP code, r597209 et al, on the 
basis that:

h) the undocumented/unspecified API (e.g. in formats of cert/key files, 
naming of ciphers) is another leaky abstraction requiring the caller to 
know it using OpenSSL underneath; and hence may as well code to OpenSSL 

i) apr_evp_factory_create represents unnecessarily bad API design; 
requiring a single entry point for a dual-purpose function which ignores 
half its arguments depending on a "purpose" switch.

j) it has unnecessary dependencies on SSL_* interfaces in code 
purporting to do purely crypto

k) it makes use of interfaces from unreleased versions of OpenSSL (which 
may or may not change before release; who knows) - for one of the two 
major modes of operation, no less.

l) again, no demonstration that non-OpenSSL-based implementations are 
even possible, if an abstraction is the intent.


I hope that is considered sufficient technical justification for the 
vetos.  Note that most of the points above are not new and have been 
posted on this list six or twelve months back.

I know that the veto is a horrible uncivilised blunt instrument.  I am 
happy to see this code branched off somewhere where those interested in 
developing it further can do so.

I am also happy to do the grunt work of reversion if the authors are 
still unwilling to resolve these issues and don't want to (or don't have 
time to) do that themselves.



View raw message