Joe Orton wrote: > 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 > I/O. > > 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 > directly > > 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. > > ---ends--- > > 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.
Joe, I've repeatedly said I'm happy to have the code removed. I've offered to move it into a branch. Do with the code as you will. No objections from me. david
