apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Graham Leggett <minf...@sharp.fm>
Subject Re: apr_crypto API review
Date Mon, 19 Oct 2009 10:47:43 GMT
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
--

Mime
View raw message