Merge into trunk : pkcs12_mod_and_export2 : Code : pyOpenSSL

Reviewer	Review Type	Date Requested	Status
Jean-Paul Calderone		2009-07-17	Approve on 2010-02-11
Review via email: mp+8962@code.launchpad.net

Revision history for this message

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

#

It's got everything but the changelog.

Revision history for this message

Jean-Paul Calderone (exarkun) wrote on 2009-07-18:

#

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:

  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.
  2. There's a left over C99-style comment near the definition of crypto_PKCS12_get_privatekey which should likely just be deleted.
  3. crypto_PKCS12_set_privatekey has one of the same problems as crypto_PKCS12_set_certificate with respect to its return value.
  4. crypto_PKCS12_set_ca_certificates does as well.
  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.
  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.
  10. Test method docstrings. I would prefer docstrings which describe some desired behavior which the test is verifying pyOpenSSL actually provides.
  11. test_export_and_load is too long. :) Any chance this can be broken into a few shorter pieces?

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

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.
  2. There's a left over C99-style comment near the definition of crypto_PKCS12_get_privatekey which should likely just be deleted.
  3. crypto_PKCS12_set_privatekey has one of the same problems as crypto_PKCS12_set_certificate with respect to its return value.
  4. crypto_PKCS12_set_ca_certificates does as well.
  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.
  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.
  10. Test method docstrings.  I would prefer docstrings which describe some desired behavior which the test is verifying pyOpenSSL actually provides.
  11. test_export_and_load is too long. :)  Any chance this can be broken into a few shorter pieces?

I kinda skimmed the export code, I'm too sleepy at this point to do it justice.  I'll have another look soon.  Thanks.

review: Needs Fixing

lp:~rick-fdd/pyopenssl/pkcs12_mod_and_export2 updated on 2009-07-18

123. By rick_dean on 2009-07-18: Implemented some of the PKCS12 fixups requested by JP.

Revision history for this message

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

#

Download full text (4.2 KiB)

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

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

lp:~rick-fdd/pyopenssl/pkcs12_mod_and_export2 updated on 2009-07-18

124. By rick_dean on 2009-07-18: broke the monster PKCS12 test into little ones, and other PKCS12 test additions.
125. By rick_dean on 2009-07-18: Move all the PEM to test_crypto.py, so both it and test_ssl.py can use them without circular includes.
126. By rick_dean on 2009-07-18: The PyObjets of struct crypto_PKCS12Obj are never NULL so stop checking.

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:

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

Yeah, good point. crypto_PKCS12_export() has a commented-out assert
that could fix this, but I like your tuple conversion idea better.
I'll give it a try.

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

Agreed. The PyObjects of crypto_PKCS12Obj are never NULL.
Fixed. The checks are gone.

Can we ever traverse an object after a clear? I left in that NULL check.

In theory could the PyDECREF() of self->key in crypto_PKCS12_clear()
call a desctructor which could access the PKCS12 object and cause
trouble with the now NULL self->cert? I suppose we should try
for a test case. Agressively eliminating NULL checks makes
me nervious, and if you let me, I'd leave them in.

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

Can PySequence_Length() fail after PySequence_Check() retuned
success? There is little point to PySequence_Check() then, as we
should just call PySequence_Length() and compare to zero. Likewise,
for PySequence_GetItem() if we within the range of PySequence_length().
I'd love to see a failing test case for this, if you can make one.

> 11. test_export_and_load is too long. :) Any chance this can be broken into a few shorter pieces?

Fixed. The monster has been slain. Where is my knighthood? :-)

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

We should create set and get functions for friendly_name
instead of a parameter of export, so our API can better support
reading them from loaded certificates.

--
Rick

On Sat, Jul 18, 2009 at 03:04:46AM -0000, Jean-Paul Calderone wrote:

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

Yeah, good point.  crypto_PKCS12_export() has a commented-out assert 
that could fix this, but I like your tuple conversion idea better.
I'll give it a try.

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

Agreed.  The PyObjects of crypto_PKCS12Obj are never NULL.
Fixed.  The checks are gone.

Can we ever traverse an object after a clear?  I left in that NULL check.

In theory could the PyDECREF() of self->key in crypto_PKCS12_clear()
call a desctructor which could access the PKCS12 object and cause 
trouble with the now NULL self->cert?  I suppose we should try
for a test case.  Agressively eliminating NULL checks makes
me nervious, and if you let me, I'd leave them in.

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

Can PySequence_Length() fail after PySequence_Check() retuned
success?  There is little point to PySequence_Check() then, as we
should just call PySequence_Length() and compare to zero.  Likewise,
for PySequence_GetItem() if we within the range of PySequence_length().
I'd love to see a failing test case for this, if you can make one.

>   11. test_export_and_load is too long. :)  Any chance this can be broken into a few shorter pieces?

Fixed.  The monster has been slain.  Where is my knighthood?  :-)

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

We should create set and get functions for friendly_name 
instead of a parameter of export, so our API can better support 
reading them from loaded certificates.

-- 
Rick

lp:~rick-fdd/pyopenssl/pkcs12_mod_and_export2 updated on 2009-07-20

127. By rick_dean on 2009-07-18: Convert crypto_PKCS12Obj->cacerts to a tuple as Jean-Paul pointed out.
128. By rick_dean on 2009-07-20: Change the API for setting and getting friendlyNames of PKCS12
129. By rick_dean on 2009-07-20: Handle error cases of PySequence_Length() and PySequence_GetItem(). Add an test case for zero length CA.

Revision history for this message

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

#

All of JP's comments have been implemented except
for the Py_ssize_t which I don't have the energy
to test. This branch is ready for merge.

The commit list shows the more recent commits
but the diff does not.

lp:~rick-fdd/pyopenssl/pkcs12_mod_and_export2 updated on 2009-07-21

130. By rick_dean on 2009-07-21: doc string fix

Revision history for this message

Jean-Paul Calderone (exarkun) wrote on 2009-07-24:

#

I wrote a nicer review, but firefox crashed and ate it. This one's a bit terser, sorry.

  1. crypto_PKCS12_set_ca_certificates should convert cacerts to a tuple first, then iterate over that tuple and check the types of the elements. A broken or weird __iter__ could sneak a non-x509 past the checks as the code is now.
  2. The return type of crypto_PKCS12_get_friendlyname shouldn't really be crypto_PKeyObj, should it?
  3. crypto_PKCS12_set_friendlyname should use PyString_CheckExact, to reject str subclasses.
  4. It's hard to tell for sure, but I *think* PKCS12_create doesn't take over the cacerts stack passed to it. So I think crypto_PKCS12_export is currently leaking a STACK_OF(X509).
  5. In crypto_PKCS12_New, cacerts is leaked in all the error cases.

lp:~rick-fdd/pyopenssl/pkcs12_mod_and_export2 updated on 2009-07-24

131. By rick_dean on 2009-07-24: Convert cacerts to tuple() before type checking so iterators can't play games with us. Jean-Paul's find.
132. By rick_dean on 2009-07-24: Fix return type of crypto_PKCS12_set_friendlyname(). Jean-Paul's find.
133. By rick_dean on 2009-07-24: Use PyString_CheckExact() in crypto_PKCS12_set_friendlyname(). Jean-Paul's find.
134. By rick_dean on 2009-07-24: Stop leaking a STACK_OF(X509) on error cases of crypto_PKCS12_New(). Add a test case of that. Jean-Paul's find.
135. By rick_dean on 2009-07-24: Stop leaking a STACK_OF(X509) in crypto_PKCS12_export. Jean-Paul's find.
136. By rick_dean on 2009-07-24: More fixes of STACK_OF(X509) in crypto_PKCS12_New().
137. By rick_dean on 2009-07-24: Only allocate a STACK_OF(X509) in crypto_PKCS12_export() when needed.

Revision history for this message

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

#

Excellent points! Thanks for the lesson. I pushed all five
fixes to lp:~rick-fdd/pyopenssl/pkcs12_mod_and_export2

--
Rick

On Fri, Jul 24, 2009 at 04:16:38AM -0000, Jean-Paul Calderone wrote:
> I wrote a nicer review, but firefox crashed and ate it. This one's a bit terser, sorry.
>
> 1. crypto_PKCS12_set_ca_certificates should convert cacerts to a tuple first, then iterate over that tuple and check the types of the elements. A broken or weird __iter__ could sneak a non-x509 past the checks as the code is now.
> 2. The return type of crypto_PKCS12_get_friendlyname shouldn't really be crypto_PKeyObj, should it?
> 3. crypto_PKCS12_set_friendlyname should use PyString_CheckExact, to reject str subclasses.
> 4. It's hard to tell for sure, but I *think* PKCS12_create doesn't take over the cacerts stack passed to it. So I think crypto_PKCS12_export is currently leaking a STACK_OF(X509).
> 5. In crypto_PKCS12_New, cacerts is leaked in all the error cases.
>
> --
> 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.

lp:~rick-fdd/pyopenssl/pkcs12_mod_and_export2 updated on 2009-07-24

138. By rick_dean on 2009-07-24: Change arg name of _runopenssl() from ''pem'' to ''stdin''.

Revision history for this message

Jean-Paul Calderone (exarkun) wrote on 2009-07-26:

#

>
> Excellent points! Thanks for the lesson. I pushed all five
> fixes to lp:~rick-fdd/pyopenssl/pkcs12_mod_and_export2
>

Awesome, thanks for all your work on this. :) I made a bunch of cosmetic changes (I should put
together a style guide for pyOpenSSL, but I'm still not sure what it would say), but notices a
couple more actual potential issues, which I also tried to tackle. My changes are in a branch
based on yours, <lp:~exarkun/pyopenssl/pkcs12_mod_and_export2>. The revisions which actually
contain interesting changes are:

  http://bazaar.launchpad.net/~exarkun/pyopenssl/pkcs12_mod_and_export2/revision/149
  http://bazaar.launchpad.net/~exarkun/pyopenssl/pkcs12_mod_and_export2/revision/156
  http://bazaar.launchpad.net/~exarkun/pyopenssl/pkcs12_mod_and_export2/revision/157

I think I'm happy with the code now, so if you think these changes are sensible, I think I'll go ahead and merge them into trunk.

Jean-Paul

Revision history for this message

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

#

I didn't know whose curly bracket style that was, yours
or openssl's. As long as we don't adopt the thousand
line function style of openssl, I'll be happy. :-)

#149 looks good. If Py_BuildValue returned NULL, it
was like the alias didn't exist. I contemplated an
error message, but chose not to. With the new code
maybe we should.

On Sun, Jul 26, 2009 at 01:57:57AM -0000, Jean-Paul Calderone wrote:
> >
> > Excellent points! Thanks for the lesson. I pushed all five
> > fixes to lp:~rick-fdd/pyopenssl/pkcs12_mod_and_export2
> >
>
> Awesome, thanks for all your work on this. :) I made a bunch of cosmetic changes (I should put
> together a style guide for pyOpenSSL, but I'm still not sure what it would say), but notices a
> couple more actual potential issues, which I also tried to tackle. My changes are in a branch
> based on yours, <lp:~exarkun/pyopenssl/pkcs12_mod_and_export2>. The revisions which actually
> contain interesting changes are:
>
> http://bazaar.launchpad.net/~exarkun/pyopenssl/pkcs12_mod_and_export2/revision/149
> http://bazaar.launchpad.net/~exarkun/pyopenssl/pkcs12_mod_and_export2/revision/156
> http://bazaar.launchpad.net/~exarkun/pyopenssl/pkcs12_mod_and_export2/revision/157
>
> I think I'm happy with the code now, so if you think these changes are sensible, I think I'll go ahead and merge them into trunk.
>
> Jean-Paul
>
> --
> 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.

--
Rick

Revision history for this message

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

#

My previous email was sent too early.

#149 I'm fine without a warning.

#156 Great catch. I hadn't noticed that PKCS12_parse()
would free the CA list, but not NULL it. For shame!

#157 Okay. Error paths are so hard to check.
A friend in a large company (that incidentally
uses a *lot* of python) says the culture there
is to always call exit() on errors. You can
log a message first if you like, but don't bother
wasting code on an error path which is futile
to get right.

--
Rick

On Sun, Jul 26, 2009 at 01:57:57AM -0000, Jean-Paul Calderone wrote:
> >
> > Excellent points! Thanks for the lesson. I pushed all five
> > fixes to lp:~rick-fdd/pyopenssl/pkcs12_mod_and_export2
> >
>
> Awesome, thanks for all your work on this. :) I made a bunch of cosmetic changes (I should put
> together a style guide for pyOpenSSL, but I'm still not sure what it would say), but notices a
> couple more actual potential issues, which I also tried to tackle. My changes are in a branch
> based on yours, <lp:~exarkun/pyopenssl/pkcs12_mod_and_export2>. The revisions which actually
> contain interesting changes are:
>
> http://bazaar.launchpad.net/~exarkun/pyopenssl/pkcs12_mod_and_export2/revision/149
> http://bazaar.launchpad.net/~exarkun/pyopenssl/pkcs12_mod_and_export2/revision/156
> http://bazaar.launchpad.net/~exarkun/pyopenssl/pkcs12_mod_and_export2/revision/157
>
> I think I'm happy with the code now, so if you think these changes are sensible, I think I'll go ahead and merge them into trunk.
>
> Jean-Paul
>
> --
> 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.

--
Rick

Revision history for this message

Jean-Paul Calderone (exarkun) wrote on 2009-07-26:

#

>
> I didn't know whose curly bracket style that was, yours
> or openssl's. As long as we don't adopt the thousand
> line function style of openssl, I'll be happy. :-)
>
> #149 looks good. If Py_BuildValue returned NULL, it
> was like the alias didn't exist. I contemplated an
> error message, but chose not to. With the new code
> maybe we should.
>

I wasn't sure how that Py_BuildValue might be induced to fail. I thought it would have to be
an internal CPython malloc failure or something like that. If the alias doesn't exist, doesn't
that mean the whole block with Py_BuildValue will be skipped? Or am I misunderstanding something?

Jean-Paul

Revision history for this message

Jean-Paul Calderone (exarkun) wrote on 2009-07-26:

#

Download full text (3.3 KiB)

>
> My previous email was sent too early.
>

Oops, didn't see this message before posting my last message. :)

> #149 I'm fine without a warning.
>
> #156 Great catch. I hadn't noticed that PKCS12_parse()
> would free the CA list, but not NULL it. For shame!
>
> #157 Okay. Error paths are so hard to check.
> A friend in a large company (that incidentally
> uses a *lot* of python) says the culture there
> is to always call exit() on errors. You can
> log a message first if you like, but don't bother
> wasting code on an error path which is futile
> to get right.
>

Luckily for me, you basically provided the test case for this error path. :) I probably wouldn't have noticed these problems if not for the commented-out attempt to load that MAC-less PKCS12 blob in one of the tests you wrote. From there, it wasn't too hard to figure out what the fix was. Of course, since the OpenSSL API is so bleh, there may be some other case that's still not tested and doesn't work right... If I were going to pursue a rigorous testing policy for this code, I'd probably start by collecting a lot of PKCS12 data from a variety of sources and run them all through these pyOpenSSL APIs as part of the automated test suite. Off the top of my head, I'm not sure where to go to find such data, though (I guess web browsers can spit it out somehow? Or do they only load it?).

I think the current level of test coverage is good, even if it's not perfect. Problems discovered later can be fixed later. :)

I'm sort of a fan of exiting immediately on errors, at least in certain circumstances. I'd feel really bad putting that into a Python library, since I don't think most Python programmers expect libraries to do that. Maybe it's actually worth doing for error paths that we're not able to construct tests for. If people run into them in practice, that behavior would be a good motivation for them to construct a test case and contribute it back to the project. :) For the time being though, I'll probably leave things as-is.

> --
> Rick
>
> On Sun, Jul 26, 2009 at 01:57:57AM -0000, Jean-Paul Calderone wrote:
> > >
> > > Excellent points! Thanks for the lesson. I pushed all five
> > > fixes to lp:~rick-fdd/pyopenssl/pkcs12_mod_and_export2
> > >
> >
> > Awesome, thanks for all your work on this. :) I made a bunch of cosmetic
> changes (I should put
> > together a style guide for pyOpenSSL, but I'm still not sure what it would
> say), but notices a
> > couple more actual potential issues, which I also tried to tackle. My
> changes are in a branch
> > based on yours, <lp:~exarkun/pyopenssl/pkcs12_mod_and_export2>. The
> revisions which actually
> > contain interesting changes are:
> >
> > http://bazaar.launchpad.net/~exarkun/pyopenssl/pkcs12_mod_and_export2/revi
> sion/149
> > http://bazaar.launchpad.net/~exarkun/pyopenssl/pkcs12_mod_and_export2/revi
> sion/156
> > http://bazaar.launchpad.net/~exarkun/pyopenssl/pkcs12_mod_and_export2/revi
> sion/157
> >
> > I think I'm happy with the code now, so if you think these changes are
> sensible, I think I'll go ahead and merge them into trunk.
> >
> > Jean-Paul
> >
> > --
> > https://code.launchpad.net/~rick-
>...

pyOpenSSL

Merge lp:~rick-fdd/pyopenssl/pkcs12_mod_and_export2 into lp:~exarkun/pyopenssl/trunk

Commit message

Description of the change

Preview Diff

Subscribers

 === modified file 'doc/pyOpenSSL.tex'
 --- doc/pyOpenSSL.tex	2009-07-16 18:09:53 +0000
 +++ doc/pyOpenSSL.tex	2009-07-17 19:49:48 +0000
@@ -524,6 +524,19 @@
  PKCS12 objects have the following methods:
++\begin{methoddesc}[PKCS12]{export}{\optional{passphrase=None}\optional{, friendly_name=None}\optional{, iter=2000}\optional{, maciter=0}}
++Returns a PKCS12 object as a string.
++
++The optional \var{passphrase} must be a string not a callback.
++
++See also the man page for the C function \function{PKCS12_create}.
++\end{methoddesc}
++
++\begin{methoddesc}[PKCS12]{get_ca_certificates}{}
++Return CA certificates within the PKCS12 object as a tuple. Returns
++\constant{None} if no CA certificates are present.
++\end{methoddesc}
++
  \begin{methoddesc}[PKCS12]{get_certificate}{}
  Return certificate portion of the PKCS12 structure.
  \end{methoddesc}
@@ -532,9 +545,18 @@
  Return private key portion of the PKCS12 structure
  \end{methoddesc}
--\begin{methoddesc}[PKCS12]{get_ca_certificates}{}
--Return CA certificates within the PKCS12 object as a tuple. Returns
--None if no CA certificates are present.
++\begin{methoddesc}[PKCS12]{set_ca_certificates}{cacerts}
++Replace or set the CA certificates within the PKCS12 object with the sequence \var{cacerts}.
++
++Set \var{cacerts} to \constant{None} to remove all CA certificates.
++\end{methoddesc}
++
++\begin{methoddesc}[PKCS12]{set_certificate}{cert}
++Replace or set the certificate portion of the PKCS12 structure.
++\end{methoddesc}
++
++\begin{methoddesc}[PKCS12]{set_privatekey}{pkey}
++Replace or set private key portion of the PKCS12 structure
  \end{methoddesc}
  \subsubsection{X509Extension objects \label{openssl-509ext}}
 === modified file 'src/crypto/crypto.c'
 --- src/crypto/crypto.c	2009-07-16 20:25:19 +0000
 +++ src/crypto/crypto.c	2009-07-17 17:50:12 +0000
@@ -12,6 +12,7 @@
  #include <Python.h>
  #define crypto_MODULE
  #include "crypto.h"
++#include "pkcs12.h"
  static char crypto_doc[] = "\n\
  Main file of crypto sub module.\n\
@@ -493,7 +494,6 @@
  static PyObject *
  crypto_load_pkcs12(PyObject *spam, PyObject *args)
+ {
--    crypto_PKCS12Obj *crypto_PKCS12_New(PKCS12 *, char *);
      int len;
      char *buffer, *passphrase = NULL;
      BIO *bio;
 === modified file 'src/crypto/pkcs12.c'
 --- src/crypto/pkcs12.c	2009-07-08 16:48:33 +0000
 +++ src/crypto/pkcs12.c	2009-07-17 20:21:23 +0000
@@ -36,19 +36,84 @@
      return self->cert;
+ }
++static char crypto_PKCS12_set_certificate_doc[] = "\n\
++Replace the certificate portion of the PKCS12 structure\n\
++\n\
++@param cert: The new certificate.\n\
++@type cert: L{X509}\n\
++@return: X509 object containing the certificate\n\
++";
++static crypto_PKCS12Obj *
++crypto_PKCS12_set_certificate(crypto_PKCS12Obj *self, PyObject *args, PyObject *keywds)
++{
++    PyObject *cert = NULL;
++    static char *kwlist[] = {"cert", NULL};
++
++    if (!PyArg_ParseTupleAndKeywords(args, keywds, "O:set_certificate",
++        kwlist, &cert))
++        return NULL;
++
++    if (cert != Py_None && ! PyObject_IsInstance(cert, (PyObject *) &crypto_X509_Type)) {
++        PyErr_SetString(PyExc_TypeError, "cert must be type X509 or None");
++        return NULL;
++    }
++
++    Py_INCREF(cert);  /* Make consistent before calling Py_DECREF() */
++    if(self->cert) {
++        Py_DECREF(self->cert);
++    }
++    self->cert = cert;
++
++    Py_INCREF(self);
++    return self;
++}
++
  static char crypto_PKCS12_get_privatekey_doc[] = "\n\
  Return private key portion of the PKCS12 structure\n\
  \n\
  @returns: PKey object containing the private key\n\
  ";
--static PyObject *
++//static PyObject *
++static crypto_PKeyObj *
  crypto_PKCS12_get_privatekey(crypto_PKCS12Obj *self, PyObject *args)
+ {
      if (!PyArg_ParseTuple(args, ":get_privatekey"))
          return NULL;
      Py_INCREF(self->key);
--    return self->key;
++    return (crypto_PKeyObj *) self->key;
++}
++
++static char crypto_PKCS12_set_privatekey_doc[] = "\n\
++Replace or set the certificate portion of the PKCS12 structure\n\
++\n\
++@param pkey: The new private key.\n\
++@type pkey: L{PKey}\n\
++@return: None\n\
++";
++static crypto_PKCS12Obj *
++crypto_PKCS12_set_privatekey(crypto_PKCS12Obj *self, PyObject *args, PyObject *keywds)
++{
++    PyObject *pkey = NULL;
++    static char *kwlist[] = {"pkey", NULL};
++
++    if (!PyArg_ParseTupleAndKeywords(args, keywds, "O:set_privatekey",
++        kwlist, &pkey))
++        return NULL;
++
++    if (pkey != Py_None && ! PyObject_IsInstance(pkey, (PyObject *) &crypto_PKey_Type)) {
++        PyErr_SetString(PyExc_TypeError, "pkey must be type X509 or None");
++        return NULL;
++    }
++
++    Py_INCREF(pkey);  /* Make consistent before calling Py_DECREF() */
++    if(self->key) {
++        Py_DECREF(self->key);
++    }
++    self->key = pkey;
++
++    Py_INCREF(self);
++    return self;
+ }
  static char crypto_PKCS12_get_ca_certificates_doc[] = "\n\
@@ -67,6 +132,118 @@
      return self->cacerts;
+ }
++static char crypto_PKCS12_set_ca_certificates_doc[] = "\n\
++Replace or set the CA certificates withing the PKCS12 object.\n\
++\n\
++@param cacerts: The new CA certificates.\n\
++@type cacerts: Sequence of L{X509}\n\
++@return: None\n\
++";
++static crypto_PKCS12Obj *
++crypto_PKCS12_set_ca_certificates(crypto_PKCS12Obj *self, PyObject *args, PyObject *keywds)
++{
++    PyObject *cacerts;
++    static char *kwlist[] = {"cacerts", NULL};
++    int i; /* Py_ssize_t for Python 2.5+ */
++
++    if (!PyArg_ParseTupleAndKeywords(args, keywds, "O:set_ca_certificates",
++        kwlist, &cacerts))
++        return NULL;
++    if (cacerts == Py_None) {
++        /* We are good. */
++    } else if (PySequence_Check(cacerts)) {  /* is iterable */
++        for(i = 0;i < PySequence_Length(cacerts);i++) {  /* For each CA cert */
++            PyObject *obj;
++            obj = PySequence_GetItem(cacerts, i);
++            if (PyObject_Type(obj) != (PyObject *) &crypto_X509_Type) {
++                Py_DECREF(obj);
++                PyErr_SetString(PyExc_TypeError, "cacerts iterable must only contain X509Type");
++                return NULL;
++            }
++            Py_DECREF(obj);
++        }
++    } else {
++        PyErr_SetString(PyExc_TypeError, "cacerts must be an iterable or None");
++        return NULL;
++    }
++
++    Py_INCREF(cacerts); /* Make consistent before calling Py_DECREF() */
++    if(self->cacerts) {
++        Py_DECREF(self->cacerts);
++    }
++    self->cacerts = cacerts;
++
++    Py_INCREF(self);
++    return self;
++}
++
++static char crypto_PKCS12_export_doc[] = "\n\
++export([passphrase=None][, friendly_name=None][, iter=2048][, maciter=1]\n\
++Dump a PKCS12 object as a string.  See also \"man PKCS12_create\".\n\
++\n\
++@param passphrase: used to encrypt the PKCS12\n\
++@type passphrase: L{str}\n\
++@param friendly_name: A descriptive comment\n\
++@type friendly_name: L{str}\n\
++@param iter: How many times to repeat the encryption\n\
++@type iter: L{int}\n\
++@param maciter: How many times to repeat the MAC\n\
++@type maciter: L{int}\n\
++@return: The string containing the PKCS12\n\
++";
++static PyObject *
++crypto_PKCS12_export(crypto_PKCS12Obj *self, PyObject *args, PyObject *keywds)
++{
++    int buf_len;
++    PyObject *buffer;
++    char *temp, *passphrase = NULL, *friendly_name = NULL;
++    BIO *bio;
++    PKCS12 *p12;
++    EVP_PKEY *pkey = NULL;
++    STACK_OF(X509) *cacerts = NULL;
++    X509 *x509 = NULL;
++    int iter = 0;  /* defaults to PKCS12_DEFAULT_ITER */
++    int maciter = 0;
++    static char *kwlist[] = {"passphrase", "friendly_name", "iter", "maciter", NULL};
++
++    if (!PyArg_ParseTupleAndKeywords(args, keywds, "|zzii:export",
++        kwlist, &passphrase, &friendly_name, &iter, &maciter))
++        return NULL;
++
++    if (self->key && self->key != Py_None) {
++        pkey = ((crypto_PKeyObj*) self->key)->pkey;
++    }
++    if (self->cert && self->cert != Py_None) {
++        x509 = ((crypto_X509Obj*) self->cert)->x509;
++    }
++    cacerts = sk_X509_new_null();
++    if (self->cacerts && self->cacerts != Py_None) {
++        int i; /* Py_ssize_t for Python 2.5+ */
++        PyObject *obj;
++        for(i = 0;i < PySequence_Length(self->cacerts);i++) {  /* For each CA cert */
++            obj = PySequence_GetItem(self->cacerts, i);
++            /* assert(PyObject_IsInstance(obj, (PyObject *) &crypto_X509_Type )); */
++            sk_X509_push(cacerts, (( crypto_X509Obj* ) obj)->x509);
++            Py_DECREF(obj);
++        }
++    }
++
++    p12 = PKCS12_create(passphrase, friendly_name, pkey, x509, cacerts,
++                        NID_pbe_WithSHA1And3_Key_TripleDES_CBC,
++                        NID_pbe_WithSHA1And3_Key_TripleDES_CBC,
++                        iter, maciter, 0);
++    if( p12 == NULL ) {
++        exception_from_error_queue(crypto_Error);
++        return NULL;
++    }
++    bio = BIO_new(BIO_s_mem());
++    i2d_PKCS12_bio(bio, p12);
++    buf_len = BIO_get_mem_data(bio, &temp);
++    buffer = PyString_FromStringAndSize(temp, buf_len);
++    BIO_free(bio);
++    return buffer;
++}
++
  /*
   * ADD_METHOD(name) expands to a correct PyMethodDef declaration
   *   {  'name', (PyCFunction)crypto_PKCS12_name, METH_VARARGS, crypto_PKCS12_name_doc }
@@ -74,11 +251,17 @@
   */
  #define ADD_METHOD(name)        \
      { #name, (PyCFunction)crypto_PKCS12_##name, METH_VARARGS, crypto_PKCS12_##name##_doc }
++#define ADD_KW_METHOD(name)        \
++    { #name, (PyCFunction)crypto_PKCS12_##name, METH_VARARGS | METH_KEYWORDS, crypto_PKCS12_##name##_doc }
  static PyMethodDef crypto_PKCS12_methods[] =
+ {
      ADD_METHOD(get_certificate),
++    ADD_KW_METHOD(set_certificate),
      ADD_METHOD(get_privatekey),
++    ADD_KW_METHOD(set_privatekey),
      ADD_METHOD(get_ca_certificates),
++    ADD_KW_METHOD(set_ca_certificates),
++    ADD_KW_METHOD(export),
      { NULL, NULL }
  };
  #undef ADD_METHOD
@@ -108,7 +291,7 @@
      cacerts = sk_X509_new_null();
      /* parse the PKCS12 lump */
--    if (!(cacerts && PKCS12_parse(p12, passphrase, &pkey, &cert, &cacerts)))
++    if (p12 && !(cacerts && PKCS12_parse(p12, passphrase, &pkey, &cert, &cacerts)))
+     {
          exception_from_error_queue(crypto_Error);
          return NULL;
@@ -122,11 +305,20 @@
      Py_INCREF(Py_None);
      self->cacerts = Py_None;
--    if ((self->cert = (PyObject *)crypto_X509_New(cert, 1)) == NULL)
--        goto error;
--
--    if ((self->key = (PyObject *)crypto_PKey_New(pkey, 1)) == NULL)
--        goto error;
++    if (cert == NULL) {
++        Py_INCREF(Py_None);
++        self->cert = Py_None;
++    } else {
++        if ((self->cert = (PyObject *)crypto_X509_New(cert, 1)) == NULL)
++            goto error;
++    }
++    if (pkey == NULL) {
++        Py_INCREF(Py_None);
++        self->key = Py_None;
++    } else {
++        if ((self->key = (PyObject *)crypto_PKey_New(pkey, 1)) == NULL)
++            goto error;
++    }
      /* Make a tuple for the CA certs */
      cacert_count = sk_X509_num(cacerts);
@@ -154,6 +346,23 @@
      return NULL;
+ }
++static char crypto_PKCS12_doc[] = "\n\
++PKCS12() -> PKCS12 instance\n\
++\n\
++Create a new empty PKCS12 object.\n\
++\n\
++@returns: The PKCS12 object\n\
++";
++static PyObject *
++crypto_PKCS12_new(PyTypeObject *subtype, PyObject *args, PyObject *kwargs)
++{
++    if (!PyArg_ParseTuple(args, ":PKCS12")) {
++        return NULL;
++    }
++
++    return (PyObject *)crypto_PKCS12_New(NULL, NULL);
++}
++
  /*
   * Find attribute
+  *
@@ -245,9 +454,24 @@
      NULL, /* setattro */
      NULL, /* as_buffer */
      Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_GC,
--    NULL, /* doc */
++    crypto_PKCS12_doc,
      (traverseproc)crypto_PKCS12_traverse,
      (inquiry)crypto_PKCS12_clear,
++    NULL, /* tp_richcompare */
++    0, /* tp_weaklistoffset */
++    NULL, /* tp_iter */
++    NULL, /* tp_iternext */
++    crypto_PKCS12_methods, /* tp_methods */
++    NULL, /* tp_members */
++    NULL, /* tp_getset */
++    NULL, /* tp_base */
++    NULL, /* tp_dict */
++    NULL, /* tp_descr_get */
++    NULL, /* tp_descr_set */
++    0, /* tp_dictoffset */
++    NULL, /* tp_init */
++    NULL, /* tp_alloc */
++    crypto_PKCS12_new, /* tp_new */
  };
  /*
@@ -262,6 +486,10 @@
          return 0;
+     }
++    if (PyModule_AddObject(module, "PKCS12", (PyObject *)&crypto_PKCS12_Type) != 0) {
++        return 0;
++    }
++
      if (PyModule_AddObject(module, "PKCS12Type", (PyObject *)&crypto_PKCS12_Type) != 0) {
          return 0;
+     }
 === modified file 'src/crypto/pkcs12.h'
 --- src/crypto/pkcs12.h	2008-02-19 01:50:23 +0000
 +++ src/crypto/pkcs12.h	2009-07-17 17:22:16 +0000
@@ -27,4 +27,7 @@
      PyObject            *cacerts;
  } crypto_PKCS12Obj;
++crypto_PKCS12Obj *
++crypto_PKCS12_New(PKCS12 *p12, char *passphrase);
++
  #endif
 === modified file 'test/test_crypto.py'
 --- test/test_crypto.py	2009-07-05 16:54:05 +0000
 +++ test/test_crypto.py	2009-07-17 20:05:22 +0000
@@ -17,11 +17,10 @@
  from OpenSSL.crypto import dump_certificate, load_certificate_request
  from OpenSSL.crypto import dump_certificate_request, dump_privatekey
  from OpenSSL.crypto import PKCS7Type, load_pkcs7_data
--from OpenSSL.crypto import PKCS12Type, load_pkcs12
++from OpenSSL.crypto import PKCS12Type, load_pkcs12, PKCS12
  from OpenSSL.crypto import NetscapeSPKI, NetscapeSPKIType
  from OpenSSL.test.util import TestCase
--
  cleartextCertificatePEM = """-----BEGIN CERTIFICATE-----
  MIIC7TCCAlagAwIBAgIIPQzE4MbeufQwDQYJKoZIhvcNAQEFBQAwWDELMAkGA1UE
  BhMCVVMxCzAJBgNVBAgTAklMMRAwDgYDVQQHEwdDaGljYWdvMRAwDgYDVQQKEwdU
@@ -709,6 +708,123 @@
          cert = load_certificate(FILETYPE_PEM, self.pemData)
          self.assertEqual(cert.get_notBefore(), "20090325123658Z")
++class PKCS12Tests(TestCase):
++    """
++    Tests functions in the L{OpenSSL.crypto.PKCS12} module.
++    """
++    pemData = cleartextCertificatePEM + cleartextPrivateKeyPEM
++
++    def test_construction(self):
++        p12 = PKCS12()
++        self.assertEqual(None, p12.get_certificate())
++        self.assertEqual(None, p12.get_privatekey())
++        self.assertEqual(None, p12.get_ca_certificates())
++
++    def test_type_errors(self):
++        p12 = PKCS12()
++        self.assertRaises(TypeError, p12.set_certificate, 3)
++        self.assertRaises(TypeError, p12.set_privatekey, 3)
++        self.assertRaises(TypeError, p12.set_ca_certificates, 3)
++        self.assertRaises(TypeError, p12.set_ca_certificates, X509())
++        self.assertRaises(TypeError, p12.set_ca_certificates, (3, 4))
++
++    def test_key_only(self):
++        """
++        L{OpenSSL.crypto.PKCS12.export} and load a PKCS without a key
++        """
++        passwd = 'blah'
++        p12 = PKCS12()
++        pkey = load_privatekey(FILETYPE_PEM, cleartextPrivateKeyPEM)
++        p12.set_privatekey( pkey )
++        self.assertEqual(None, p12.get_certificate())
++        self.assertEqual(pkey, p12.get_privatekey())
++        dumped_p12 = p12.export(passphrase=passwd, iter=2, maciter=3)
++        p12 = load_pkcs12(dumped_p12, passwd)
++        self.assertEqual(None, p12.get_ca_certificates())
++        self.assertEqual(None, p12.get_certificate())
++        #  It's actually in the pkcs12, but we silently don't find it (a key without a cert)
++        #self.assertEqual(cleartextPrivateKeyPEM, dump_privatekey(FILETYPE_PEM, p12.get_privatekey()))
++
++    def test_cert_only(self):
++        """
++        L{OpenSSL.crypto.PKCS12.export} and load a PKCS without a key.
++        Strangely, OpenSSL converts it to a CA cert.
++        """
++        passwd = 'blah'
++        p12 = PKCS12()
++        cert = load_certificate(FILETYPE_PEM, cleartextCertificatePEM)
++        p12.set_certificate( cert )
++        self.assertEqual(cert, p12.get_certificate())
++        self.assertEqual(None, p12.get_privatekey())
++        dumped_p12 = p12.export(passphrase=passwd, iter=2, maciter=3)
++        p12 = load_pkcs12(dumped_p12, passwd)
++        self.assertEqual(None, p12.get_privatekey())
++        self.assertEqual(None, p12.get_certificate())
++        self.assertEqual(cleartextCertificatePEM, dump_certificate(FILETYPE_PEM, p12.get_ca_certificates()[0]))
++
++
++    def test_export_and_load(self):
++        """
++        L{OpenSSL.crypto.PKCS12.export} and others
++        """
++        # use openssl program to create a p12 then load it
++        from OpenSSL.test.test_ssl import client_cert_pem, client_key_pem, server_cert_pem, server_key_pem, root_cert_pem
++        passwd = 'whatever'
++        pem = client_key_pem + client_cert_pem
++        p12_str = _runopenssl(pem, "pkcs12", '-export', '-clcerts', '-passout', 'pass:'+passwd)
++        p12 = load_pkcs12(p12_str, passwd)
++        # verify p12 using pkcs12 get_* functions
++        cert_pem = dump_certificate(FILETYPE_PEM, p12.get_certificate())
++        self.assertEqual(cert_pem, client_cert_pem)
++        key_pem = dump_privatekey(FILETYPE_PEM, p12.get_privatekey())
++        self.assertEqual(key_pem, client_key_pem)
++        self.assertEqual(None, p12.get_ca_certificates())
++        # dump cert and verify it using the openssl program
++        dumped_p12 = p12.export(passphrase=passwd, iter=2, maciter=0, friendly_name='blueberry')
++        recovered_key = _runopenssl(dumped_p12, "pkcs12", '-nocerts', '-nodes', '-passin', 'pass:'+passwd)
++        self.assertEqual(recovered_key[-len(client_key_pem):], client_key_pem)
++        recovered_cert = _runopenssl(dumped_p12, "pkcs12", '-clcerts', '-nodes', '-passin', 'pass:'+passwd, '-nokeys')
++        self.assertEqual(recovered_cert[-len(client_cert_pem):], client_cert_pem)
++        # change the cert and key
++        p12.set_certificate(load_certificate(FILETYPE_PEM, server_cert_pem))
++        p12.set_privatekey(load_privatekey(FILETYPE_PEM, server_key_pem))
++        root_cert = load_certificate(FILETYPE_PEM, root_cert_pem)
++        p12.set_ca_certificates( [ root_cert ] )
++        p12.set_ca_certificates( ( root_cert, ) )
++        self.assertEqual(1, len(p12.get_ca_certificates()))
++        self.assertEqual(root_cert, p12.get_ca_certificates()[0])
++        # recover changed cert and key using the openssl program
++        dumped_p12 = p12.export(passphrase=passwd, iter=2, maciter=0, friendly_name='Serverlicious')
++        recovered_key = _runopenssl(dumped_p12, "pkcs12", '-nocerts', '-nodes', '-passin', 'pass:'+passwd)
++        self.assertEqual(recovered_key[-len(server_key_pem):], server_key_pem)
++        recovered_cert = _runopenssl(dumped_p12, "pkcs12", '-clcerts', '-nodes', '-passin', 'pass:'+passwd, '-nokeys')
++        self.assertEqual(recovered_cert[-len(server_cert_pem):], server_cert_pem)
++        recovered_cert = _runopenssl(dumped_p12, "pkcs12", '-cacerts', '-nodes', '-passin', 'pass:'+passwd, '-nokeys')
++        self.assertEqual(recovered_cert[-len(root_cert_pem):], root_cert_pem)
++        #  Test other forms of no password
++        passwd = ''
++        dumped_p12_empty = p12.export(passphrase=passwd, iter=2, maciter=0, friendly_name='Sewer')
++        dumped_p12_none  = p12.export(passphrase=None,   iter=2, maciter=0, friendly_name='Sewer')
++        dumped_p12_nopw  = p12.export(                   iter=2, maciter=0, friendly_name='Sewer')
++        recovered_empty = _runopenssl(dumped_p12_empty, "pkcs12", '-nodes', '-passin', 'pass:'+passwd )
++        recovered_none  = _runopenssl(dumped_p12_none, "pkcs12", '-nodes', '-passin', 'pass:'+passwd )
++        recovered_nopw  = _runopenssl(dumped_p12_nopw, "pkcs12", '-nodes', '-passin', 'pass:'+passwd )
++        self.assertEqual(recovered_none, recovered_nopw)
++        self.assertEqual(recovered_none, recovered_empty)
++        #  Test removing CA certs
++        p12.set_ca_certificates( None )
++        self.assertEqual(None, p12.get_ca_certificates())
++        #  Test without MAC
++        dumped_p12 = p12.export(maciter=-1, passphrase=passwd, iter=2)
++        recovered_key = _runopenssl(dumped_p12, "pkcs12", '-nocerts', '-nodes', '-passin', 'pass:'+passwd, '-nomacver' )
++        self.assertEqual(recovered_key[-len(server_key_pem):], server_key_pem)
++        #  We can't load PKCS12 without MAC, because we use PCKS_parse()
++        #p12 = load_pkcs12(dumped_p12, passwd)
++        # Test export without any args
++        dumped_p12 = p12.export()
++        recovered_key = _runopenssl(dumped_p12, "pkcs12", '-nodes', '-passin', 'pass:' )
++        self.assertEqual(recovered_key[-len(server_key_pem):], server_key_pem)
++
      def test_get_notAfter(self):
          """
@@ -732,6 +848,18 @@
++def _runopenssl(pem, *args):
++    """
++    Run the command line openssl tool with the given arguments and write
++    the given PEM to its stdin.
++    """
++    write, read = popen2(" ".join(("openssl",) + args), "b")
++    write.write(pem)
++    write.close()
++    return read.read()
++
++
++
  class FunctionTests(TestCase):
      """
      Tests for free-functions in the L{OpenSSL.crypto} module.
@@ -801,17 +929,6 @@
          self.assertEqual(loadedKey.bits(), key.bits())
--    def _runopenssl(self, pem, *args):
--        """
--        Run the command line openssl tool with the given arguments and write
--        the given PEM to its stdin.
--        """
--        write, read = popen2(" ".join(("openssl",) + args), "b")
--        write.write(pem)
--        write.close()
--        return read.read()
--
--
      def test_dump_certificate(self):
          """
          L{dump_certificate} writes PEM, DER, and text.
@@ -821,13 +938,13 @@
          dumped_pem = dump_certificate(FILETYPE_PEM, cert)
          self.assertEqual(dumped_pem, cleartextCertificatePEM)
          dumped_der = dump_certificate(FILETYPE_ASN1, cert)
--        good_der = self._runopenssl(dumped_pem, "x509", "-outform", "DER")
++        good_der = _runopenssl(dumped_pem, "x509", "-outform", "DER")
          self.assertEqual(dumped_der, good_der)
          cert2 = load_certificate(FILETYPE_ASN1, dumped_der)
          dumped_pem2 = dump_certificate(FILETYPE_PEM, cert2)
          self.assertEqual(dumped_pem2, cleartextCertificatePEM)
          dumped_text = dump_certificate(FILETYPE_TEXT, cert)
--        good_text = self._runopenssl(dumped_pem, "x509", "-noout", "-text")
++        good_text = _runopenssl(dumped_pem, "x509", "-noout", "-text")
          self.assertEqual(dumped_text, good_text)
@@ -840,13 +957,13 @@
          self.assertEqual(dumped_pem, cleartextPrivateKeyPEM)
          dumped_der = dump_privatekey(FILETYPE_ASN1, key)
          # XXX This OpenSSL call writes "writing RSA key" to standard out.  Sad.
--        good_der = self._runopenssl(dumped_pem, "rsa", "-outform", "DER")
++        good_der = _runopenssl(dumped_pem, "rsa", "-outform", "DER")
          self.assertEqual(dumped_der, good_der)
          key2 = load_privatekey(FILETYPE_ASN1, dumped_der)
          dumped_pem2 = dump_privatekey(FILETYPE_PEM, key2)
          self.assertEqual(dumped_pem2, cleartextPrivateKeyPEM)
          dumped_text = dump_privatekey(FILETYPE_TEXT, key)
--        good_text = self._runopenssl(dumped_pem, "rsa", "-noout", "-text")
++        good_text = _runopenssl(dumped_pem, "rsa", "-noout", "-text")
          self.assertEqual(dumped_text, good_text)
@@ -858,13 +975,13 @@
          dumped_pem = dump_certificate_request(FILETYPE_PEM, req)
          self.assertEqual(dumped_pem, cleartextCertificateRequestPEM)
          dumped_der = dump_certificate_request(FILETYPE_ASN1, req)
--        good_der = self._runopenssl(dumped_pem, "req", "-outform", "DER")
++        good_der = _runopenssl(dumped_pem, "req", "-outform", "DER")
          self.assertEqual(dumped_der, good_der)
          req2 = load_certificate_request(FILETYPE_ASN1, dumped_der)
          dumped_pem2 = dump_certificate_request(FILETYPE_PEM, req2)
          self.assertEqual(dumped_pem2, cleartextCertificateRequestPEM)
          dumped_text = dump_certificate_request(FILETYPE_TEXT, req)
--        good_text = self._runopenssl(dumped_pem, "req", "-noout", "-text")
++        good_text = _runopenssl(dumped_pem, "req", "-noout", "-text")
          self.assertEqual(dumped_text, good_text)