apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Orton <jor...@redhat.com>
Subject apr_crypto API review
Date Fri, 16 Oct 2009 15:45:30 GMT
Sorry I haven't sent out this review sooner.

Broader issues:

** Having both apr_crypto_init() and apr_crypto_get_driver() seems to be 
redundant (unnecessary complexity in API and code).  The lowest common 
denominator in initialization is NSS, which requires process-global 
configuration when initialized.  You cannot obtain a "driver" of either 
crypto toolkit which is configured differently to how it was initialized 
by apr_crypto_init().

** Is the caller of this code expected to be crypto-toolkit agnostic or 
not?  I am struggling to imagine in Fedora, why we'd want to build 
APR(-util) with support for *both* crypto toolkits at run-time.  Why not 
just pick one at build time, like every other project in the entire 
world does?

** Generally the naming of the object types exposed seems to be highly 
non-intuitive.  A "driver" creates a "factory" -- so APR has a genuine 
factory-building-factory!  Joel Spolsky would be proud.  A "factory" 
creates a "block context".  A "block context" can encrypt/decrypt data.  
So a "block context" is really a "cipher context"?  Anybody with a 
passing knowledge of crypto would understand the latter but not the 
former, and still be bewildered by the use of "driver" and "factory".

Specific issues:

** initialization via "params" array headers everywhere is completely 
unclear.  there's no indication of what those params arrays should 
contain for either init/get_driver or apr_crypto_factory.

** the structure used to represent the "factory" object doesn't use 
"factory" in its name, which is obscure.  also, why it is not opaque?  
Likewise the naming of apr_crypto_cleanup without reference to the 
"factory" is non-intuitive.

** given that a factory has a one-to-one relationship with a driver 
(right?) why must the poor caller pass both into functions?

** similarly for apr_crypto_block_t * object; a one->one relatioship 
between the "block" context and the driver, so why pass both any time?  

** the description of the "factory" mentions keys, certs, and 
algorithms, but none of the backends appear to do anything at all with 
keys, certs, and algorithms in the factory init function.

** enum constants in apr_crypto_block_key_type_e and 
apr_crypto_block_key_mode_e are not namespace-safe (lack APR_ prefix)

** various code style issues:

a) the brace following the function definition should come on the 
new-line. line-wrapped function definitions are not indented correctly - 
see the example at http://httpd.apache.org/dev/styleguide.html


static apr_status_t crypto_init(apr_pool_t *pool,
        const apr_array_header_t *params, int *rc) {


static apr_status_t crypto_init(apr_pool_t *pool,
                                const apr_array_header_t *params, 
                                int *rc) 

b) lots of unnecessary casts from void *.  don't do that.

c) lots of apr_*alloc return value checks.  don't do that.

d) also some chunks of commented-out code

** there should be #defines for the driver names, so the consumer of the 
API doesn't have to pluck them out of the air, if the consumer is 
expected to use those directly.

** the apu_err_t structure seems to be unnecessary, and pretending this 
is more general by putting it in apu_errno.h is wrong.  It would be 
equivalent, and better API (no structures exposed, so easier to extend 
in the future) to have:

void apr_crypto_error(const apr_crypto_t *f,
     		      const char **reason,
		      const char **message,
		      int *rc);

to return whatever is stored in f->result.  note also that the OpenSSL 
backend initializes this structure but never uses it AFAICT.  Returning 
the "int rc" at all is wrong because it's exposing an error code 
specific to the underlying toolkit, breaking the abstraction, per 
previous discussion.

** there are a bunch of #defines which are unused: 

View raw message