apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Graham Leggett" <minf...@sharp.fm>
Subject Re: svn commit: r597209 - in /apr/apr-util/trunk: CHANGES build/ssl.m4 include/apr_buckets.h include/apr_ssl.h include/private/apr_ssl_openssl_private.h ssl/apr_ssl_openssl.c ssl/apr_ssl_winsock.c
Date Thu, 22 Nov 2007 14:56:49 GMT
On Thu, November 22, 2007 4:40 pm, Joe Orton wrote:

> General issues throughout this code:

(Some initial observations without the code being in front of me)

> - don't check for pool allocation failure

Can you point out where exactly - I checked for this specifically very
thoroughly, but then looking at the same code continuously for a few days
means you will not see all of them.

> Drop the @fn stuff.  It's unnecessary if you have a correct function
> prototype.

Will drop throughout the file.

> Private keys and certificates can be stored in many formats.  The format
> this function expects needs to be documented in the API.

APR makes no claims as to the format the underlying toolkit accepts, this
same problem exists in the apr_ssl* code.

We also don't (yet) support the idea of certs being in places other than
files.

>> + * @param cipherName Name of cipher to use for symmetrical encryption
>
> Again, the format of this name must be documented in the API; "Advanced
> Encryption Standard" is the name of a cipher.  Will it work?

Again, this is the underlying toolkit's problem. Will document better.

> as an output parameter only?  How long must the buffer be?  Or is it
> really an input/output parameter?

As I understand this, the length of the final buffer is included by the
call that tells you the length of the initial buffer. OpenSSL isn't very
clear on this, it needs to be better documented.

>> +#if HAVE_DECL_EVP_PKEY_CTX_NEW
>
> That macro doesn't appear to ever be defined anywhere.

See the ssl.m4 file (if memory serves). EVP_PKEY_CTX_new is only available
in the as-yet-unreleased OpenSSL v0.9.9.

>> +#if HAVE_DECL_EVP_PKEY_CTX_NEW
>> +            /* load certs */
>> +            data->sslCtx = SSL_CTX_new(SSLv23_server_method());
>
> Again dead code, and weird code - why mess with an SSL_CTX if doing
> encryption?

See the OpenSSL v0.9.9 asymmetrical keys support, google for
EVP_PKEY_CTX_new.

Regards,
Graham
--



Mime
View raw message