apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Orton <jor...@redhat.com>
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:40:40 GMT
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.


Mime
View raw message