pyOpenSSL

Code review comment for lp:~rick-fdd/pyopenssl/pkcs12_mod_and_export2

pkcs12_mod_and_export2
Merge into trunk

Revision history for this message

rick_dean (rick-fdd) wrote on 2009-07-18:

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, <lp:~phil-mayers/pyopenssl/crl+morepkcs12>, 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 :-)

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, <lp:~phil-mayers/pyopenssl/crl+morepkcs12>, 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  :-)

« Back to merge proposal