apr_crypto API review

[Joe Orton]

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

wrong:

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

correct:

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:
APR_CRYPTO_CERT_BASE64, APR_CRYPTO_CERT_PFX, ...

[William A. Rowe Jr.]

Joe Orton wrote:
>
> ** 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?

They must be for using the API.  If they want to then do something 'more' and
address a toolkit directly, that's their perogative, but not something we should
even get involved in or claim to support (same issue as svn bdb assumptions).

Five practical illustrations related to httpd on win32 out of the box;

 * ht* support should not bind to/load into process the crypto/ssl libs, ever.
   Removing the ssl stub[s] due to local laws mustn't invalidate such programs.

 * user has desire to use ms crypto providers, support this, they must fight with
   their own registration of certs/keys in the registry.

 * user has desire to use openssl

 * user has desire to use openssl compiled as FIPS [these must be seperate libs,
   see recent dev@... discussions]

 * user has 3rd party module using nss directly, seeks to avoid incompatibilities
   (note the libld platforms suffer much worse than win32 in this respect).  I've
   seen this particular issue repeated year after year in new forms.

Fedora is relatively homogeneous so I doubt it would benefit, but again we can
offer the disable dso support flags for platforms who rather build in that manner.

[Graham Leggett]

Joe Orton wrote:

> Sorry I haven't sent out this review sooner.

Sorry I didn't volunteer to release APR v1.4 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().

You're not familiar then with apr_dbd.h, which follows this same
pattern. I have no strong opinion either way, I lean towards two simple
functions rather than one more complicated one.

What I do have a strong opinion on is to be consistent within APR. If we
are going to follow a particular pattern, then we must be consistent
across crypto, dbd and ldap. And if we choose to change it, it's a
change separate from the crypto code itself.

> ** 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?

You missed the crypto test cases, which include comprehensive
interoperability tests. You'll agree that it's difficult running an
interoperability test if you constantly have to ./configure;make;make
check to test the first crypto toolkit, and then ./configure;make;make
check for the second, and so on:

http://svn.apache.org/repos/asf/apr/apr-util/branches/1.4.x/test/testcrypto.c

You've also missed the objections to the fact that mod_ldap compiles
against one LDAP library at a time. As I don't relish doing a whole lot
of work to make wrowe and others happy, only to be shot down because I
make you unhappy, I would rather we agreed beforehand which route to
choose. Can you clarify your opinion, both for LDAP, and for crypto?

Can you clarify where it isn't clear that this code is supposed to be
crypto-toolkit agnostic? I though this was self evident, but if it
isn't, do we need need to improve the docs, or something further?

> ** 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".

Woa, this isn't java code or an attempt to build a "framework" (like we
need another one). The question you should have asked was "what problem
does this code try to solve?", and the answer to that is the need to set
up a key, and then reuse it cheaply until the need to generate a new key.

Obviously the naming can be improved, but can you be more specific? What
naming would you recommend?

I'll run this past some crypto people on my side to clarify what they
should think this should be called.

> 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.

You're right, the documentation could be improved. I would imagine that
each crypto implementation would need a header file with doxygen
comments giving the parameters accepted by each one? Or do you suggest
something different?

> ** 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.

Fair enough, the naming can definitely be improved. Do you have specific
suggestions?

> ** 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?  

Let me go through the test cases and ensure these can be collapsed.

> ** 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)
[snip]

Duly noted.

> ** the apu_err_t structure seems to be unnecessary, and pretending this
> is more general by putting it in apu_errno.h is wrong.

I disagree, developing three separate but almost identical mechanisms
for delivering errors, one for crypto, one for dbd, one for ldap is wrong.

I thought it was obvious, but clearly isn't: The intention of the
general functions is to be the basis of the new apr_ldap error reporting
mechanism, and then the same for apr_dbd in apr v2.0.

> 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.

Some advice from someone who has spent a lot of blood and tears making
abstraction APIs: never obscure the lower level error information from
the caller if the errors returned from the underlying API are not
trivial and well bounded (as in this case).

Obviously the caller is not expected to parse the errors returned from
the underlying toolkit, as that would break the abstraction. But writing
this information to log files or detailed error messages is critical, so
an end user can at best stick them into google and return an error back
to them, and at worst, the end user can ask on a list, and have someone
who understands the API identify exactly why that end user is
experiencing their specific behaviour. If the only way to coax the real
error out of an abstraction is to use gdb to do it, that abstraction has
failed.

As it turned out, the interoperability tests uncovered some significant
shortcomings in the NSS libraries where multiple failure paths would
return the same NSS error code. Each of these was fed back to the NSS
project, and they were quick to fix each case. None of this would have
been possible if the underlying error messages had been obscured.

> ** there are a bunch of #defines which are unused:
> APR_CRYPTO_CERT_BASE64, APR_CRYPTO_CERT_PFX, ...

The intention of these #defines is to collapse the duplication between
these #defines, and APR_LDAP_CERT_TYPE_BASE64 and friends in the
†apr_ldap code. This is all intentional.

Regards,
Graham
--