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