On Wed, Nov 21, 2007 at 09:01:55PM -0000, Graham Leggett wrote: > Author: minfrin > Date: Wed Nov 21 13:01:50 2007 > New Revision: 597209 > > URL: http://svn.apache.org/viewvc?rev=597209&view=rev > Log: > Expose the SSL EVP interface to encrypt and decrypt arbitrary > blocks of data, using symmetrical keys. Experimental support for > asymmetrical public/private keys as supported by OpenSSL v0.9.9.
That included an unrelated change to apr_buckets.h. General issues throughout this code: - don't check for pool allocation failure - don't cast from void * - follow the style guide http://httpd.apache.org/dev/styleguide.html - in particular switch/case indenting is wrong, line-wrapped conditional expressions should have the && conditional at the beginning of the line, space before parens, etc etc > +/** > + * @fn apr_status_t apr_evp_factory_create(apr_evp_factory_t **newFactory, Drop the @fn stuff. It's unnecessary if you have a correct function prototype. > + const char *privateKeyFilename, > + const char *certificateFilename, > + const char *cipherName, > + const char *passphrase, > + const char *engine, > + const char *digest, > + apr_evp_factory_type_e purpose, > + apr_pool_t *pool) > + * @brief Attempts to create an EVP "factory". The "factory" is then > + * used to create contexts to keep track of encryption. > + * @param newFactory The newly created factory > + * @param privateKeyFilename Private key filename to use for assetrical > encryption > + * @param certificateFilename X509 certificate file to use for assymetrical > encryption Private keys and certificates can be stored in many formats. The format this function expects needs to be documented in the API. > + * @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? ... > + */ > +APR_DECLARE(apr_status_t) apr_evp_crypt_init(apr_evp_factory_t *, missing paramater names in prototypes in several places. > +/** > + * @fn apr_status_t apr_evp_crypt_finish(apr_evp_crypt_t *, > + * unsigned char *out, > + * apr_size_t *outlen) > + * @brief Encrypt final data block, write it to out. > + * @note If necessary the final block will be written out after being > + * padded. After this call, the context is cleaned and can be > + * reused by apr_env_encrypt_init() or apr_env_decrypt_init(). > + * @param evp The evp context to use. > + * @param out Address of a buffer to which data will be written. > + * @param outlen Length of the output will be written here. as an output parameter only? How long must the buffer be? Or is it really an input/output parameter? > Modified: apr/apr-util/trunk/include/private/apr_ssl_openssl_private.h > URL: > http://svn.apache.org/viewvc/apr/apr-util/trunk/include/private/apr_ssl_openssl_private.h?rev=597209&r1=597208&r2=597209&view=diff > ============================================================================== > --- apr/apr-util/trunk/include/private/apr_ssl_openssl_private.h (original) > +++ apr/apr-util/trunk/include/private/apr_ssl_openssl_private.h Wed Nov 21 > 13:01:50 2007 ... > + const char *privateKeyFilename; > + const char *certificateFilename; > +#if HAVE_DECL_EVP_PKEY_CTX_NEW That macro doesn't appear to ever be defined anywhere. > Modified: apr/apr-util/trunk/ssl/apr_ssl_openssl.c > URL: > http://svn.apache.org/viewvc/apr/apr-util/trunk/ssl/apr_ssl_openssl.c?rev=597209&r1=597208&r2=597209&view=diff > ============================================================================== > --- apr/apr-util/trunk/ssl/apr_ssl_openssl.c (original) > +++ apr/apr-util/trunk/ssl/apr_ssl_openssl.c Wed Nov 21 13:01:50 2007 ... > +apr_status_t apr_evp_factory_create(apr_evp_factory_t **newFactory, Lacks APU_DECLARE decaration. > +... > + apu_evp_data_t *data = (apu_evp_data_t *)apr_pcalloc(pool, > sizeof(apu_evp_data_t)); > + if (!data) { > + return APR_ENOMEM; Variable declarations in the middle of a function is a C99 feature, again casting from void * and checking for pool allocation failure. ... > + switch (purpose) { > + case APR_EVP_FACTORY_ASYM: { Code style. don't indent "case" here. > +#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? ... > + case APR_EVP_FACTORY_SYM: { > + data->cipher = EVP_get_cipherbyname(cipherName); > + if (!data->cipher) { > + return APR_ENOCIPHER; > + } > + OpenSSL_add_all_digests(); Changes process-global state, that can't be done here.
