On Sat, Jul 18, 2009 at 03:04:46AM -0000, Jean-Paul Calderone wrote: > Review: Needs Fixing > I should have mentioned earlier that there is someone who recently showed up on the mailing list (over on sourceforge) who is also a bit interested in this area (although he says he's more keen on CRLs than PKCS12). He's pushed a branch, , which has some overlap with this one. I mostly prefer the PKCS12 code in this branch. I particularly like the documentation additions and many of the test suite additions. Here are some areas I think could be improved, though: Thanks. I just subscribed. It's great to be able to compare code. His crypto_PKCS12_set_certificate() leaks objects when trying to replace an existing one. Likewise for crypto_PKCS12_set_privatekey(). Setting of CA certificates isn't implemented. Except for export, our API is identical. > > 1. crypto_PKCS12_set_certificate documents its return type as an X509 object. This looks like a copy/paste mistake, I guess. The function does return self, the PKCS12 instance. It should probably return None, though, I think. Agreed. fixed. FWIW, returning the object is a very ruby thing to do, so you can chain together multple method calls on one line. > 2. There's a left over C99-style comment near the definition of crypto_PKCS12_get_privatekey which should likely just be deleted. Ooops. Fixed. > 3. crypto_PKCS12_set_privatekey has one of the same problems as crypto_PKCS12_set_certificate with respect to its return value. Agreed. fixed. > 4. crypto_PKCS12_set_ca_certificates does as well. Agreed. fixed. > 5. Hm. The way PKCS12 handles its CA certificates list has a hole. In trunk, crypto_PKCS12_New always creates cacerts, and makes it a tuple. This is good, since get_ca_certificates will return it directly, and if it were a list, callers could modify it in ways which would probably result in a crash later. With this branch, they can pass a list to set_ca_certificates. It might be valid at the time they pass it in, but then it can be mutated later. Requiring a tuple in crypto_PKCS12_set_ca_certificates instead of any sequence is probably a good idea. > 6. All the code handling the key, cert, and cacerts fields of crypto_PKCS12Obj structures would probably be simpler if only one of NULL or Py_None were possible values. I think that may really already be the case anyway. crypto_PKCS12_New starts of setting key and cert to NULL, but then immediately either sets them to a PyObject* or goes to the error case. What happens when the NULL checks for key, cert, and cacerts are removed, and just the Py_None checks are left in place (where necessary)? > 7. Regarding int vs Py_ssize_t, we should probably put a version-protected typedef into util.h or someplace, so we can spell this in a way that works with all supported versions and actually does the right thing on newer Pythons. Agreed, but I'm too lazy to test it right now. #if !defined(PY_MAJOR_VERSION) || PY_VERSION_HEX < 0x02050000 typedef int Py_ssize_t #endif > 8. Still in crypto_PKCS12_set_ca_certificates, PySequence_Length and PySequence_GetItem can have errors that should be checked for. I'm not sure if PyTuple_GetSize and PyTuple_GetItem (cf point 5) also can, maybe not. > 9. crypto_PKCS12_set_ca_certificates also returns self, should be None. same as #4. > 10. Test method docstrings. I would prefer docstrings which describe some desired behavior which the test is verifying pyOpenSSL actually provides. Good policy. Fixed (except for #11). > 11. test_export_and_load is too long. :) Any chance this can be broken into a few shorter pieces? Okay. I'm learning as I watch you merge my code. Will do. > > I kinda skimmed the export code, I'm too sleepy at this point to do it justice. I'll have another look soon. Thanks. I cleaned up the blank lines of the unittests. Some of the unit tests were in the wrong place, so PKCS12Tests now handles all PKCS12 and only PKCS12. > > -- > https://code.launchpad.net/~rick-fdd/pyopenssl/pkcs12_mod_and_export2/+merge/8962 > You are the owner of lp:~rick-fdd/pyopenssl/pkcs12_mod_and_export2. I need to get to sleep. I'll address the rest of this stuff later. Thanks for the feedback. -- Rick :-)