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

Proposed by rick_dean
Status: Merged
Merged at revision: not available
Proposed branch: lp:~rick-fdd/pyopenssl/pkcs12_mod_and_export2
Merge into: lp:~exarkun/pyopenssl/trunk
Diff against target: None lines
To merge this branch: bzr merge lp:~rick-fdd/pyopenssl/pkcs12_mod_and_export2
Reviewer Review Type Date Requested Status
Jean-Paul Calderone Approve
Review via email: mp+8962@code.launchpad.net
To post a comment you must log in.
Revision history for this message
rick_dean (rick-fdd) wrote :

It's got everything but the changelog.

Revision history for this message
Jean-Paul Calderone (exarkun) wrote :

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
123. By rick_dean

Implemented some of the PKCS12 fixups requested by JP.

Revision history for this message
rick_dean (rick-fdd) wrote :
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...

Read more...

124. By rick_dean

broke the monster PKCS12 test into little ones, and other PKCS12 test additions.

125. By rick_dean

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

The PyObjets of struct crypto_PKCS12Obj are never NULL so stop checking.

Revision history for this message
rick_dean (rick-fdd) wrote :

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

127. By rick_dean

Convert crypto_PKCS12Obj->cacerts to a tuple as Jean-Paul pointed out.

128. By rick_dean

Change the API for setting and getting friendlyNames of PKCS12

129. By rick_dean

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 :

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.

130. By rick_dean

doc string fix

Revision history for this message
Jean-Paul Calderone (exarkun) 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.

131. By rick_dean

Convert cacerts to tuple() before type checking so iterators can't play games with us. Jean-Paul's find.

132. By rick_dean

Fix return type of crypto_PKCS12_set_friendlyname(). Jean-Paul's find.

133. By rick_dean

Use PyString_CheckExact() in crypto_PKCS12_set_friendlyname(). Jean-Paul's find.

134. By rick_dean

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

Stop leaking a STACK_OF(X509) in crypto_PKCS12_export. Jean-Paul's find.

136. By rick_dean

More fixes of STACK_OF(X509) in crypto_PKCS12_New().

137. By rick_dean

Only allocate a STACK_OF(X509) in crypto_PKCS12_export() when needed.

Revision history for this message
rick_dean (rick-fdd) wrote :

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.

138. By rick_dean

Change arg name of _runopenssl() from ''pem'' to ''stdin''.

Revision history for this message
Jean-Paul Calderone (exarkun) 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

Revision history for this message
rick_dean (rick-fdd) wrote :

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 :

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 :

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

Read more...

Revision history for this message
rick_dean (rick-fdd) wrote :
Download full text (3.9 KiB)

Agreed that pyOpenSSL should avoid exit().

Agreed the code is ready to be committed.
It does not break any old functionality, and the
API is stable.

There is a difference between testing the
bindings and testing openssl. The unit tests
are responsible for the former, yet they
do some of the later.

--
Rick

On Sun, Jul 26, 2009 at 02:14:18PM -0000, Jean-Paul Calderone wrote:
> >
> > 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:
> > >
> > > ht...

Read more...

Revision history for this message
rick_dean (rick-fdd) wrote :

Yes, I was thinking of malloc as the likely Py_BuildValue()
failure. Either we drop the friendly name, or abort the
whole method. I don't care much, as long as we haven't
leaked or corrupted memory, which keeps me happy.
If Py_BuildValue() fails, then an exception is likely
futile anyway.

--
Rick

On Sun, Jul 26, 2009 at 02:04:30PM -0000, Jean-Paul Calderone wrote:
> >
> > 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
>
> --
> 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.

Revision history for this message
Jean-Paul Calderone (exarkun) wrote :

Accepted this a while ago.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
524524
525PKCS12 objects have the following methods:525PKCS12 objects have the following methods:
526526
527\begin{methoddesc}[PKCS12]{export}{\optional{passphrase=None}\optional{, friendly_name=None}\optional{, iter=2000}\optional{, maciter=0}}
528Returns a PKCS12 object as a string.
529
530The optional \var{passphrase} must be a string not a callback.
531
532See also the man page for the C function \function{PKCS12_create}.
533\end{methoddesc}
534
535\begin{methoddesc}[PKCS12]{get_ca_certificates}{}
536Return CA certificates within the PKCS12 object as a tuple. Returns
537\constant{None} if no CA certificates are present.
538\end{methoddesc}
539
527\begin{methoddesc}[PKCS12]{get_certificate}{}540\begin{methoddesc}[PKCS12]{get_certificate}{}
528Return certificate portion of the PKCS12 structure.541Return certificate portion of the PKCS12 structure.
529\end{methoddesc}542\end{methoddesc}
@@ -532,9 +545,18 @@
532Return private key portion of the PKCS12 structure545Return private key portion of the PKCS12 structure
533\end{methoddesc}546\end{methoddesc}
534547
535\begin{methoddesc}[PKCS12]{get_ca_certificates}{}548\begin{methoddesc}[PKCS12]{set_ca_certificates}{cacerts}
536Return CA certificates within the PKCS12 object as a tuple. Returns549Replace or set the CA certificates within the PKCS12 object with the sequence \var{cacerts}.
537None if no CA certificates are present.550
551Set \var{cacerts} to \constant{None} to remove all CA certificates.
552\end{methoddesc}
553
554\begin{methoddesc}[PKCS12]{set_certificate}{cert}
555Replace or set the certificate portion of the PKCS12 structure.
556\end{methoddesc}
557
558\begin{methoddesc}[PKCS12]{set_privatekey}{pkey}
559Replace or set private key portion of the PKCS12 structure
538\end{methoddesc}560\end{methoddesc}
539561
540\subsubsection{X509Extension objects \label{openssl-509ext}}562\subsubsection{X509Extension objects \label{openssl-509ext}}
541563
=== 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 @@
12#include <Python.h>12#include <Python.h>
13#define crypto_MODULE13#define crypto_MODULE
14#include "crypto.h"14#include "crypto.h"
15#include "pkcs12.h"
1516
16static char crypto_doc[] = "\n\17static char crypto_doc[] = "\n\
17Main file of crypto sub module.\n\18Main file of crypto sub module.\n\
@@ -493,7 +494,6 @@
493static PyObject *494static PyObject *
494crypto_load_pkcs12(PyObject *spam, PyObject *args)495crypto_load_pkcs12(PyObject *spam, PyObject *args)
495{496{
496 crypto_PKCS12Obj *crypto_PKCS12_New(PKCS12 *, char *);
497 int len;497 int len;
498 char *buffer, *passphrase = NULL;498 char *buffer, *passphrase = NULL;
499 BIO *bio;499 BIO *bio;
500500
=== 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 @@
36 return self->cert;36 return self->cert;
37}37}
3838
39static char crypto_PKCS12_set_certificate_doc[] = "\n\
40Replace the certificate portion of the PKCS12 structure\n\
41\n\
42@param cert: The new certificate.\n\
43@type cert: L{X509}\n\
44@return: X509 object containing the certificate\n\
45";
46static crypto_PKCS12Obj *
47crypto_PKCS12_set_certificate(crypto_PKCS12Obj *self, PyObject *args, PyObject *keywds)
48{
49 PyObject *cert = NULL;
50 static char *kwlist[] = {"cert", NULL};
51
52 if (!PyArg_ParseTupleAndKeywords(args, keywds, "O:set_certificate",
53 kwlist, &cert))
54 return NULL;
55
56 if (cert != Py_None && ! PyObject_IsInstance(cert, (PyObject *) &crypto_X509_Type)) {
57 PyErr_SetString(PyExc_TypeError, "cert must be type X509 or None");
58 return NULL;
59 }
60
61 Py_INCREF(cert); /* Make consistent before calling Py_DECREF() */
62 if(self->cert) {
63 Py_DECREF(self->cert);
64 }
65 self->cert = cert;
66
67 Py_INCREF(self);
68 return self;
69}
70
39static char crypto_PKCS12_get_privatekey_doc[] = "\n\71static char crypto_PKCS12_get_privatekey_doc[] = "\n\
40Return private key portion of the PKCS12 structure\n\72Return private key portion of the PKCS12 structure\n\
41\n\73\n\
42@returns: PKey object containing the private key\n\74@returns: PKey object containing the private key\n\
43";75";
44static PyObject *76//static PyObject *
77static crypto_PKeyObj *
45crypto_PKCS12_get_privatekey(crypto_PKCS12Obj *self, PyObject *args)78crypto_PKCS12_get_privatekey(crypto_PKCS12Obj *self, PyObject *args)
46{79{
47 if (!PyArg_ParseTuple(args, ":get_privatekey"))80 if (!PyArg_ParseTuple(args, ":get_privatekey"))
48 return NULL;81 return NULL;
4982
50 Py_INCREF(self->key);83 Py_INCREF(self->key);
51 return self->key;84 return (crypto_PKeyObj *) self->key;
85}
86
87static char crypto_PKCS12_set_privatekey_doc[] = "\n\
88Replace or set the certificate portion of the PKCS12 structure\n\
89\n\
90@param pkey: The new private key.\n\
91@type pkey: L{PKey}\n\
92@return: None\n\
93";
94static crypto_PKCS12Obj *
95crypto_PKCS12_set_privatekey(crypto_PKCS12Obj *self, PyObject *args, PyObject *keywds)
96{
97 PyObject *pkey = NULL;
98 static char *kwlist[] = {"pkey", NULL};
99
100 if (!PyArg_ParseTupleAndKeywords(args, keywds, "O:set_privatekey",
101 kwlist, &pkey))
102 return NULL;
103
104 if (pkey != Py_None && ! PyObject_IsInstance(pkey, (PyObject *) &crypto_PKey_Type)) {
105 PyErr_SetString(PyExc_TypeError, "pkey must be type X509 or None");
106 return NULL;
107 }
108
109 Py_INCREF(pkey); /* Make consistent before calling Py_DECREF() */
110 if(self->key) {
111 Py_DECREF(self->key);
112 }
113 self->key = pkey;
114
115 Py_INCREF(self);
116 return self;
52}117}
53118
54static char crypto_PKCS12_get_ca_certificates_doc[] = "\n\119static char crypto_PKCS12_get_ca_certificates_doc[] = "\n\
@@ -67,6 +132,118 @@
67 return self->cacerts;132 return self->cacerts;
68}133}
69134
135static char crypto_PKCS12_set_ca_certificates_doc[] = "\n\
136Replace or set the CA certificates withing the PKCS12 object.\n\
137\n\
138@param cacerts: The new CA certificates.\n\
139@type cacerts: Sequence of L{X509}\n\
140@return: None\n\
141";
142static crypto_PKCS12Obj *
143crypto_PKCS12_set_ca_certificates(crypto_PKCS12Obj *self, PyObject *args, PyObject *keywds)
144{
145 PyObject *cacerts;
146 static char *kwlist[] = {"cacerts", NULL};
147 int i; /* Py_ssize_t for Python 2.5+ */
148
149 if (!PyArg_ParseTupleAndKeywords(args, keywds, "O:set_ca_certificates",
150 kwlist, &cacerts))
151 return NULL;
152 if (cacerts == Py_None) {
153 /* We are good. */
154 } else if (PySequence_Check(cacerts)) { /* is iterable */
155 for(i = 0;i < PySequence_Length(cacerts);i++) { /* For each CA cert */
156 PyObject *obj;
157 obj = PySequence_GetItem(cacerts, i);
158 if (PyObject_Type(obj) != (PyObject *) &crypto_X509_Type) {
159 Py_DECREF(obj);
160 PyErr_SetString(PyExc_TypeError, "cacerts iterable must only contain X509Type");
161 return NULL;
162 }
163 Py_DECREF(obj);
164 }
165 } else {
166 PyErr_SetString(PyExc_TypeError, "cacerts must be an iterable or None");
167 return NULL;
168 }
169
170 Py_INCREF(cacerts); /* Make consistent before calling Py_DECREF() */
171 if(self->cacerts) {
172 Py_DECREF(self->cacerts);
173 }
174 self->cacerts = cacerts;
175
176 Py_INCREF(self);
177 return self;
178}
179
180static char crypto_PKCS12_export_doc[] = "\n\
181export([passphrase=None][, friendly_name=None][, iter=2048][, maciter=1]\n\
182Dump a PKCS12 object as a string. See also \"man PKCS12_create\".\n\
183\n\
184@param passphrase: used to encrypt the PKCS12\n\
185@type passphrase: L{str}\n\
186@param friendly_name: A descriptive comment\n\
187@type friendly_name: L{str}\n\
188@param iter: How many times to repeat the encryption\n\
189@type iter: L{int}\n\
190@param maciter: How many times to repeat the MAC\n\
191@type maciter: L{int}\n\
192@return: The string containing the PKCS12\n\
193";
194static PyObject *
195crypto_PKCS12_export(crypto_PKCS12Obj *self, PyObject *args, PyObject *keywds)
196{
197 int buf_len;
198 PyObject *buffer;
199 char *temp, *passphrase = NULL, *friendly_name = NULL;
200 BIO *bio;
201 PKCS12 *p12;
202 EVP_PKEY *pkey = NULL;
203 STACK_OF(X509) *cacerts = NULL;
204 X509 *x509 = NULL;
205 int iter = 0; /* defaults to PKCS12_DEFAULT_ITER */
206 int maciter = 0;
207 static char *kwlist[] = {"passphrase", "friendly_name", "iter", "maciter", NULL};
208
209 if (!PyArg_ParseTupleAndKeywords(args, keywds, "|zzii:export",
210 kwlist, &passphrase, &friendly_name, &iter, &maciter))
211 return NULL;
212
213 if (self->key && self->key != Py_None) {
214 pkey = ((crypto_PKeyObj*) self->key)->pkey;
215 }
216 if (self->cert && self->cert != Py_None) {
217 x509 = ((crypto_X509Obj*) self->cert)->x509;
218 }
219 cacerts = sk_X509_new_null();
220 if (self->cacerts && self->cacerts != Py_None) {
221 int i; /* Py_ssize_t for Python 2.5+ */
222 PyObject *obj;
223 for(i = 0;i < PySequence_Length(self->cacerts);i++) { /* For each CA cert */
224 obj = PySequence_GetItem(self->cacerts, i);
225 /* assert(PyObject_IsInstance(obj, (PyObject *) &crypto_X509_Type )); */
226 sk_X509_push(cacerts, (( crypto_X509Obj* ) obj)->x509);
227 Py_DECREF(obj);
228 }
229 }
230
231 p12 = PKCS12_create(passphrase, friendly_name, pkey, x509, cacerts,
232 NID_pbe_WithSHA1And3_Key_TripleDES_CBC,
233 NID_pbe_WithSHA1And3_Key_TripleDES_CBC,
234 iter, maciter, 0);
235 if( p12 == NULL ) {
236 exception_from_error_queue(crypto_Error);
237 return NULL;
238 }
239 bio = BIO_new(BIO_s_mem());
240 i2d_PKCS12_bio(bio, p12);
241 buf_len = BIO_get_mem_data(bio, &temp);
242 buffer = PyString_FromStringAndSize(temp, buf_len);
243 BIO_free(bio);
244 return buffer;
245}
246
70/*247/*
71 * ADD_METHOD(name) expands to a correct PyMethodDef declaration248 * ADD_METHOD(name) expands to a correct PyMethodDef declaration
72 * { 'name', (PyCFunction)crypto_PKCS12_name, METH_VARARGS, crypto_PKCS12_name_doc }249 * { 'name', (PyCFunction)crypto_PKCS12_name, METH_VARARGS, crypto_PKCS12_name_doc }
@@ -74,11 +251,17 @@
74 */251 */
75#define ADD_METHOD(name) \252#define ADD_METHOD(name) \
76 { #name, (PyCFunction)crypto_PKCS12_##name, METH_VARARGS, crypto_PKCS12_##name##_doc }253 { #name, (PyCFunction)crypto_PKCS12_##name, METH_VARARGS, crypto_PKCS12_##name##_doc }
254#define ADD_KW_METHOD(name) \
255 { #name, (PyCFunction)crypto_PKCS12_##name, METH_VARARGS | METH_KEYWORDS, crypto_PKCS12_##name##_doc }
77static PyMethodDef crypto_PKCS12_methods[] =256static PyMethodDef crypto_PKCS12_methods[] =
78{257{
79 ADD_METHOD(get_certificate),258 ADD_METHOD(get_certificate),
259 ADD_KW_METHOD(set_certificate),
80 ADD_METHOD(get_privatekey),260 ADD_METHOD(get_privatekey),
261 ADD_KW_METHOD(set_privatekey),
81 ADD_METHOD(get_ca_certificates),262 ADD_METHOD(get_ca_certificates),
263 ADD_KW_METHOD(set_ca_certificates),
264 ADD_KW_METHOD(export),
82 { NULL, NULL }265 { NULL, NULL }
83};266};
84#undef ADD_METHOD267#undef ADD_METHOD
@@ -108,7 +291,7 @@
108 cacerts = sk_X509_new_null();291 cacerts = sk_X509_new_null();
109292
110 /* parse the PKCS12 lump */293 /* parse the PKCS12 lump */
111 if (!(cacerts && PKCS12_parse(p12, passphrase, &pkey, &cert, &cacerts)))294 if (p12 && !(cacerts && PKCS12_parse(p12, passphrase, &pkey, &cert, &cacerts)))
112 {295 {
113 exception_from_error_queue(crypto_Error);296 exception_from_error_queue(crypto_Error);
114 return NULL;297 return NULL;
@@ -122,11 +305,20 @@
122 Py_INCREF(Py_None);305 Py_INCREF(Py_None);
123 self->cacerts = Py_None;306 self->cacerts = Py_None;
124307
125 if ((self->cert = (PyObject *)crypto_X509_New(cert, 1)) == NULL)308 if (cert == NULL) {
126 goto error;309 Py_INCREF(Py_None);
127310 self->cert = Py_None;
128 if ((self->key = (PyObject *)crypto_PKey_New(pkey, 1)) == NULL)311 } else {
129 goto error;312 if ((self->cert = (PyObject *)crypto_X509_New(cert, 1)) == NULL)
313 goto error;
314 }
315 if (pkey == NULL) {
316 Py_INCREF(Py_None);
317 self->key = Py_None;
318 } else {
319 if ((self->key = (PyObject *)crypto_PKey_New(pkey, 1)) == NULL)
320 goto error;
321 }
130322
131 /* Make a tuple for the CA certs */323 /* Make a tuple for the CA certs */
132 cacert_count = sk_X509_num(cacerts);324 cacert_count = sk_X509_num(cacerts);
@@ -154,6 +346,23 @@
154 return NULL;346 return NULL;
155}347}
156348
349static char crypto_PKCS12_doc[] = "\n\
350PKCS12() -> PKCS12 instance\n\
351\n\
352Create a new empty PKCS12 object.\n\
353\n\
354@returns: The PKCS12 object\n\
355";
356static PyObject *
357crypto_PKCS12_new(PyTypeObject *subtype, PyObject *args, PyObject *kwargs)
358{
359 if (!PyArg_ParseTuple(args, ":PKCS12")) {
360 return NULL;
361 }
362
363 return (PyObject *)crypto_PKCS12_New(NULL, NULL);
364}
365
157/*366/*
158 * Find attribute367 * Find attribute
159 *368 *
@@ -245,9 +454,24 @@
245 NULL, /* setattro */454 NULL, /* setattro */
246 NULL, /* as_buffer */455 NULL, /* as_buffer */
247 Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_GC,456 Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_GC,
248 NULL, /* doc */457 crypto_PKCS12_doc,
249 (traverseproc)crypto_PKCS12_traverse,458 (traverseproc)crypto_PKCS12_traverse,
250 (inquiry)crypto_PKCS12_clear,459 (inquiry)crypto_PKCS12_clear,
460 NULL, /* tp_richcompare */
461 0, /* tp_weaklistoffset */
462 NULL, /* tp_iter */
463 NULL, /* tp_iternext */
464 crypto_PKCS12_methods, /* tp_methods */
465 NULL, /* tp_members */
466 NULL, /* tp_getset */
467 NULL, /* tp_base */
468 NULL, /* tp_dict */
469 NULL, /* tp_descr_get */
470 NULL, /* tp_descr_set */
471 0, /* tp_dictoffset */
472 NULL, /* tp_init */
473 NULL, /* tp_alloc */
474 crypto_PKCS12_new, /* tp_new */
251};475};
252476
253/*477/*
@@ -262,6 +486,10 @@
262 return 0;486 return 0;
263 }487 }
264488
489 if (PyModule_AddObject(module, "PKCS12", (PyObject *)&crypto_PKCS12_Type) != 0) {
490 return 0;
491 }
492
265 if (PyModule_AddObject(module, "PKCS12Type", (PyObject *)&crypto_PKCS12_Type) != 0) {493 if (PyModule_AddObject(module, "PKCS12Type", (PyObject *)&crypto_PKCS12_Type) != 0) {
266 return 0;494 return 0;
267 }495 }
268496
=== 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 @@
27 PyObject *cacerts;27 PyObject *cacerts;
28} crypto_PKCS12Obj;28} crypto_PKCS12Obj;
2929
30crypto_PKCS12Obj *
31crypto_PKCS12_New(PKCS12 *p12, char *passphrase);
32
30#endif33#endif
3134
=== 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 @@
17from OpenSSL.crypto import dump_certificate, load_certificate_request17from OpenSSL.crypto import dump_certificate, load_certificate_request
18from OpenSSL.crypto import dump_certificate_request, dump_privatekey18from OpenSSL.crypto import dump_certificate_request, dump_privatekey
19from OpenSSL.crypto import PKCS7Type, load_pkcs7_data19from OpenSSL.crypto import PKCS7Type, load_pkcs7_data
20from OpenSSL.crypto import PKCS12Type, load_pkcs1220from OpenSSL.crypto import PKCS12Type, load_pkcs12, PKCS12
21from OpenSSL.crypto import NetscapeSPKI, NetscapeSPKIType21from OpenSSL.crypto import NetscapeSPKI, NetscapeSPKIType
22from OpenSSL.test.util import TestCase22from OpenSSL.test.util import TestCase
2323
24
25cleartextCertificatePEM = """-----BEGIN CERTIFICATE-----24cleartextCertificatePEM = """-----BEGIN CERTIFICATE-----
26MIIC7TCCAlagAwIBAgIIPQzE4MbeufQwDQYJKoZIhvcNAQEFBQAwWDELMAkGA1UE25MIIC7TCCAlagAwIBAgIIPQzE4MbeufQwDQYJKoZIhvcNAQEFBQAwWDELMAkGA1UE
27BhMCVVMxCzAJBgNVBAgTAklMMRAwDgYDVQQHEwdDaGljYWdvMRAwDgYDVQQKEwdU26BhMCVVMxCzAJBgNVBAgTAklMMRAwDgYDVQQHEwdDaGljYWdvMRAwDgYDVQQKEwdU
@@ -709,6 +708,123 @@
709 cert = load_certificate(FILETYPE_PEM, self.pemData)708 cert = load_certificate(FILETYPE_PEM, self.pemData)
710 self.assertEqual(cert.get_notBefore(), "20090325123658Z")709 self.assertEqual(cert.get_notBefore(), "20090325123658Z")
711710
711class PKCS12Tests(TestCase):
712 """
713 Tests functions in the L{OpenSSL.crypto.PKCS12} module.
714 """
715 pemData = cleartextCertificatePEM + cleartextPrivateKeyPEM
716
717 def test_construction(self):
718 p12 = PKCS12()
719 self.assertEqual(None, p12.get_certificate())
720 self.assertEqual(None, p12.get_privatekey())
721 self.assertEqual(None, p12.get_ca_certificates())
722
723 def test_type_errors(self):
724 p12 = PKCS12()
725 self.assertRaises(TypeError, p12.set_certificate, 3)
726 self.assertRaises(TypeError, p12.set_privatekey, 3)
727 self.assertRaises(TypeError, p12.set_ca_certificates, 3)
728 self.assertRaises(TypeError, p12.set_ca_certificates, X509())
729 self.assertRaises(TypeError, p12.set_ca_certificates, (3, 4))
730
731 def test_key_only(self):
732 """
733 L{OpenSSL.crypto.PKCS12.export} and load a PKCS without a key
734 """
735 passwd = 'blah'
736 p12 = PKCS12()
737 pkey = load_privatekey(FILETYPE_PEM, cleartextPrivateKeyPEM)
738 p12.set_privatekey( pkey )
739 self.assertEqual(None, p12.get_certificate())
740 self.assertEqual(pkey, p12.get_privatekey())
741 dumped_p12 = p12.export(passphrase=passwd, iter=2, maciter=3)
742 p12 = load_pkcs12(dumped_p12, passwd)
743 self.assertEqual(None, p12.get_ca_certificates())
744 self.assertEqual(None, p12.get_certificate())
745 # It's actually in the pkcs12, but we silently don't find it (a key without a cert)
746 #self.assertEqual(cleartextPrivateKeyPEM, dump_privatekey(FILETYPE_PEM, p12.get_privatekey()))
747
748 def test_cert_only(self):
749 """
750 L{OpenSSL.crypto.PKCS12.export} and load a PKCS without a key.
751 Strangely, OpenSSL converts it to a CA cert.
752 """
753 passwd = 'blah'
754 p12 = PKCS12()
755 cert = load_certificate(FILETYPE_PEM, cleartextCertificatePEM)
756 p12.set_certificate( cert )
757 self.assertEqual(cert, p12.get_certificate())
758 self.assertEqual(None, p12.get_privatekey())
759 dumped_p12 = p12.export(passphrase=passwd, iter=2, maciter=3)
760 p12 = load_pkcs12(dumped_p12, passwd)
761 self.assertEqual(None, p12.get_privatekey())
762 self.assertEqual(None, p12.get_certificate())
763 self.assertEqual(cleartextCertificatePEM, dump_certificate(FILETYPE_PEM, p12.get_ca_certificates()[0]))
764
765
766 def test_export_and_load(self):
767 """
768 L{OpenSSL.crypto.PKCS12.export} and others
769 """
770 # use openssl program to create a p12 then load it
771 from OpenSSL.test.test_ssl import client_cert_pem, client_key_pem, server_cert_pem, server_key_pem, root_cert_pem
772 passwd = 'whatever'
773 pem = client_key_pem + client_cert_pem
774 p12_str = _runopenssl(pem, "pkcs12", '-export', '-clcerts', '-passout', 'pass:'+passwd)
775 p12 = load_pkcs12(p12_str, passwd)
776 # verify p12 using pkcs12 get_* functions
777 cert_pem = dump_certificate(FILETYPE_PEM, p12.get_certificate())
778 self.assertEqual(cert_pem, client_cert_pem)
779 key_pem = dump_privatekey(FILETYPE_PEM, p12.get_privatekey())
780 self.assertEqual(key_pem, client_key_pem)
781 self.assertEqual(None, p12.get_ca_certificates())
782 # dump cert and verify it using the openssl program
783 dumped_p12 = p12.export(passphrase=passwd, iter=2, maciter=0, friendly_name='blueberry')
784 recovered_key = _runopenssl(dumped_p12, "pkcs12", '-nocerts', '-nodes', '-passin', 'pass:'+passwd)
785 self.assertEqual(recovered_key[-len(client_key_pem):], client_key_pem)
786 recovered_cert = _runopenssl(dumped_p12, "pkcs12", '-clcerts', '-nodes', '-passin', 'pass:'+passwd, '-nokeys')
787 self.assertEqual(recovered_cert[-len(client_cert_pem):], client_cert_pem)
788 # change the cert and key
789 p12.set_certificate(load_certificate(FILETYPE_PEM, server_cert_pem))
790 p12.set_privatekey(load_privatekey(FILETYPE_PEM, server_key_pem))
791 root_cert = load_certificate(FILETYPE_PEM, root_cert_pem)
792 p12.set_ca_certificates( [ root_cert ] )
793 p12.set_ca_certificates( ( root_cert, ) )
794 self.assertEqual(1, len(p12.get_ca_certificates()))
795 self.assertEqual(root_cert, p12.get_ca_certificates()[0])
796 # recover changed cert and key using the openssl program
797 dumped_p12 = p12.export(passphrase=passwd, iter=2, maciter=0, friendly_name='Serverlicious')
798 recovered_key = _runopenssl(dumped_p12, "pkcs12", '-nocerts', '-nodes', '-passin', 'pass:'+passwd)
799 self.assertEqual(recovered_key[-len(server_key_pem):], server_key_pem)
800 recovered_cert = _runopenssl(dumped_p12, "pkcs12", '-clcerts', '-nodes', '-passin', 'pass:'+passwd, '-nokeys')
801 self.assertEqual(recovered_cert[-len(server_cert_pem):], server_cert_pem)
802 recovered_cert = _runopenssl(dumped_p12, "pkcs12", '-cacerts', '-nodes', '-passin', 'pass:'+passwd, '-nokeys')
803 self.assertEqual(recovered_cert[-len(root_cert_pem):], root_cert_pem)
804 # Test other forms of no password
805 passwd = ''
806 dumped_p12_empty = p12.export(passphrase=passwd, iter=2, maciter=0, friendly_name='Sewer')
807 dumped_p12_none = p12.export(passphrase=None, iter=2, maciter=0, friendly_name='Sewer')
808 dumped_p12_nopw = p12.export( iter=2, maciter=0, friendly_name='Sewer')
809 recovered_empty = _runopenssl(dumped_p12_empty, "pkcs12", '-nodes', '-passin', 'pass:'+passwd )
810 recovered_none = _runopenssl(dumped_p12_none, "pkcs12", '-nodes', '-passin', 'pass:'+passwd )
811 recovered_nopw = _runopenssl(dumped_p12_nopw, "pkcs12", '-nodes', '-passin', 'pass:'+passwd )
812 self.assertEqual(recovered_none, recovered_nopw)
813 self.assertEqual(recovered_none, recovered_empty)
814 # Test removing CA certs
815 p12.set_ca_certificates( None )
816 self.assertEqual(None, p12.get_ca_certificates())
817 # Test without MAC
818 dumped_p12 = p12.export(maciter=-1, passphrase=passwd, iter=2)
819 recovered_key = _runopenssl(dumped_p12, "pkcs12", '-nocerts', '-nodes', '-passin', 'pass:'+passwd, '-nomacver' )
820 self.assertEqual(recovered_key[-len(server_key_pem):], server_key_pem)
821 # We can't load PKCS12 without MAC, because we use PCKS_parse()
822 #p12 = load_pkcs12(dumped_p12, passwd)
823 # Test export without any args
824 dumped_p12 = p12.export()
825 recovered_key = _runopenssl(dumped_p12, "pkcs12", '-nodes', '-passin', 'pass:' )
826 self.assertEqual(recovered_key[-len(server_key_pem):], server_key_pem)
827
712828
713 def test_get_notAfter(self):829 def test_get_notAfter(self):
714 """830 """
@@ -732,6 +848,18 @@
732848
733849
734850
851def _runopenssl(pem, *args):
852 """
853 Run the command line openssl tool with the given arguments and write
854 the given PEM to its stdin.
855 """
856 write, read = popen2(" ".join(("openssl",) + args), "b")
857 write.write(pem)
858 write.close()
859 return read.read()
860
861
862
735class FunctionTests(TestCase):863class FunctionTests(TestCase):
736 """864 """
737 Tests for free-functions in the L{OpenSSL.crypto} module.865 Tests for free-functions in the L{OpenSSL.crypto} module.
@@ -801,17 +929,6 @@
801 self.assertEqual(loadedKey.bits(), key.bits())929 self.assertEqual(loadedKey.bits(), key.bits())
802930
803931
804 def _runopenssl(self, pem, *args):
805 """
806 Run the command line openssl tool with the given arguments and write
807 the given PEM to its stdin.
808 """
809 write, read = popen2(" ".join(("openssl",) + args), "b")
810 write.write(pem)
811 write.close()
812 return read.read()
813
814
815 def test_dump_certificate(self):932 def test_dump_certificate(self):
816 """933 """
817 L{dump_certificate} writes PEM, DER, and text.934 L{dump_certificate} writes PEM, DER, and text.
@@ -821,13 +938,13 @@
821 dumped_pem = dump_certificate(FILETYPE_PEM, cert)938 dumped_pem = dump_certificate(FILETYPE_PEM, cert)
822 self.assertEqual(dumped_pem, cleartextCertificatePEM)939 self.assertEqual(dumped_pem, cleartextCertificatePEM)
823 dumped_der = dump_certificate(FILETYPE_ASN1, cert)940 dumped_der = dump_certificate(FILETYPE_ASN1, cert)
824 good_der = self._runopenssl(dumped_pem, "x509", "-outform", "DER")941 good_der = _runopenssl(dumped_pem, "x509", "-outform", "DER")
825 self.assertEqual(dumped_der, good_der)942 self.assertEqual(dumped_der, good_der)
826 cert2 = load_certificate(FILETYPE_ASN1, dumped_der)943 cert2 = load_certificate(FILETYPE_ASN1, dumped_der)
827 dumped_pem2 = dump_certificate(FILETYPE_PEM, cert2)944 dumped_pem2 = dump_certificate(FILETYPE_PEM, cert2)
828 self.assertEqual(dumped_pem2, cleartextCertificatePEM)945 self.assertEqual(dumped_pem2, cleartextCertificatePEM)
829 dumped_text = dump_certificate(FILETYPE_TEXT, cert)946 dumped_text = dump_certificate(FILETYPE_TEXT, cert)
830 good_text = self._runopenssl(dumped_pem, "x509", "-noout", "-text")947 good_text = _runopenssl(dumped_pem, "x509", "-noout", "-text")
831 self.assertEqual(dumped_text, good_text)948 self.assertEqual(dumped_text, good_text)
832949
833950
@@ -840,13 +957,13 @@
840 self.assertEqual(dumped_pem, cleartextPrivateKeyPEM)957 self.assertEqual(dumped_pem, cleartextPrivateKeyPEM)
841 dumped_der = dump_privatekey(FILETYPE_ASN1, key)958 dumped_der = dump_privatekey(FILETYPE_ASN1, key)
842 # XXX This OpenSSL call writes "writing RSA key" to standard out. Sad.959 # XXX This OpenSSL call writes "writing RSA key" to standard out. Sad.
843 good_der = self._runopenssl(dumped_pem, "rsa", "-outform", "DER")960 good_der = _runopenssl(dumped_pem, "rsa", "-outform", "DER")
844 self.assertEqual(dumped_der, good_der)961 self.assertEqual(dumped_der, good_der)
845 key2 = load_privatekey(FILETYPE_ASN1, dumped_der)962 key2 = load_privatekey(FILETYPE_ASN1, dumped_der)
846 dumped_pem2 = dump_privatekey(FILETYPE_PEM, key2)963 dumped_pem2 = dump_privatekey(FILETYPE_PEM, key2)
847 self.assertEqual(dumped_pem2, cleartextPrivateKeyPEM)964 self.assertEqual(dumped_pem2, cleartextPrivateKeyPEM)
848 dumped_text = dump_privatekey(FILETYPE_TEXT, key)965 dumped_text = dump_privatekey(FILETYPE_TEXT, key)
849 good_text = self._runopenssl(dumped_pem, "rsa", "-noout", "-text")966 good_text = _runopenssl(dumped_pem, "rsa", "-noout", "-text")
850 self.assertEqual(dumped_text, good_text)967 self.assertEqual(dumped_text, good_text)
851968
852969
@@ -858,13 +975,13 @@
858 dumped_pem = dump_certificate_request(FILETYPE_PEM, req)975 dumped_pem = dump_certificate_request(FILETYPE_PEM, req)
859 self.assertEqual(dumped_pem, cleartextCertificateRequestPEM)976 self.assertEqual(dumped_pem, cleartextCertificateRequestPEM)
860 dumped_der = dump_certificate_request(FILETYPE_ASN1, req)977 dumped_der = dump_certificate_request(FILETYPE_ASN1, req)
861 good_der = self._runopenssl(dumped_pem, "req", "-outform", "DER")978 good_der = _runopenssl(dumped_pem, "req", "-outform", "DER")
862 self.assertEqual(dumped_der, good_der)979 self.assertEqual(dumped_der, good_der)
863 req2 = load_certificate_request(FILETYPE_ASN1, dumped_der)980 req2 = load_certificate_request(FILETYPE_ASN1, dumped_der)
864 dumped_pem2 = dump_certificate_request(FILETYPE_PEM, req2)981 dumped_pem2 = dump_certificate_request(FILETYPE_PEM, req2)
865 self.assertEqual(dumped_pem2, cleartextCertificateRequestPEM)982 self.assertEqual(dumped_pem2, cleartextCertificateRequestPEM)
866 dumped_text = dump_certificate_request(FILETYPE_TEXT, req)983 dumped_text = dump_certificate_request(FILETYPE_TEXT, req)
867 good_text = self._runopenssl(dumped_pem, "req", "-noout", "-text")984 good_text = _runopenssl(dumped_pem, "req", "-noout", "-text")
868 self.assertEqual(dumped_text, good_text)985 self.assertEqual(dumped_text, good_text)
869986
870987

Subscribers

People subscribed via source and target branches

to status/vote changes: