Support reading PEM files in TLSLite

Support reading PEM files in TLSLite

BUG=none
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124637

[Ryan Sleevi, 2012-02-29 00:27:01]

Nico,
   Would you mind reviewing this for Python-style and functionality? Note that I
haven't included the .path file (until review is finished). Upstream has also
switched to 0.4.0, but there's enough of a refactoring/API break that I'm not
yet ready to shave that yak, so this is just a local fork. They've upstreamed
all our other patches though, so I don't think this will be a problem.

Palmer,
   Does this fix the issue with your patchset?

[Nico, 2012-02-29 00:28:21]

http://codereview.chromium.org/9515015/diff/1/third_party/tlslite/README.chro...
File third_party/tlslite/README.chromium (right):

http://codereview.chromium.org/9515015/diff/1/third_party/tlslite/README.chro...
third_party/tlslite/README.chromium:30: - patches/parse_chain.patch:
tlslite/X509.py and tlslite/X509CertChain.py were
.patch file missing.

Did you try to upstream this change?

[Ryan Sleevi, 2012-02-29 00:29:46]

On 2012/02/29 00:28:21, Nico wrote:
>
http://codereview.chromium.org/9515015/diff/1/third_party/tlslite/README.chro...
> File third_party/tlslite/README.chromium (right):
> 
>
http://codereview.chromium.org/9515015/diff/1/third_party/tlslite/README.chro...
> third_party/tlslite/README.chromium:30: - patches/parse_chain.patch:
> tlslite/X509.py and tlslite/X509CertChain.py were
> .patch file missing.
> 
> Did you try to upstream this change?

No, because upstream is at 0.4.0 and we're still at 0.3.8, and it's a
non-trivial/non-high-priority effort to get to 0.4.0.

[Nico, 2012-02-29 00:33:34]

http://codereview.chromium.org/9515015/diff/1/third_party/tlslite/tlslite/X50...
File third_party/tlslite/tlslite/X509CertChain.py (right):

http://codereview.chromium.org/9515015/diff/1/third_party/tlslite/tlslite/X50...
third_party/tlslite/tlslite/X509CertChain.py:28: def parseChain(self, s):
Could use a basic unit test

http://codereview.chromium.org/9515015/diff/1/third_party/tlslite/tlslite/X50...
third_party/tlslite/tlslite/X509CertChain.py:31: @type s: str
s isn't a great parameter name. parameters are part of the interface and hence
should have decent names.

http://codereview.chromium.org/9515015/diff/1/third_party/tlslite/tlslite/X50...
third_party/tlslite/tlslite/X509CertChain.py:48: @param index: The current
offset within s to begin iterating from
@ivar index?

http://codereview.chromium.org/9515015/diff/1/third_party/tlslite/tlslite/X50...
third_party/tlslite/tlslite/X509CertChain.py:65: self.index =
self.s.find("-----BEGIN CERTIFICATE-----",
Make these strings "constants"?

http://codereview.chromium.org/9515015/diff/1/third_party/tlslite/tlslite/X50...
third_party/tlslite/tlslite/X509CertChain.py:67: if (self.index == -1):
no parens

http://codereview.chromium.org/9515015/diff/1/third_party/tlslite/tlslite/X50...
third_party/tlslite/tlslite/X509CertChain.py:70: if (end == -1):
no parens

http://codereview.chromium.org/9515015/diff/1/third_party/tlslite/tlslite/X50...
third_party/tlslite/tlslite/X509CertChain.py:84: self.x509List = certs
Does `self.x509List = list(PEMIerator(s))` work as well?

[Nico, 2012-02-29 00:34:11]

On 2012/02/29 00:29:46, Ryan Sleevi wrote:
> On 2012/02/29 00:28:21, Nico wrote:
> >
>
http://codereview.chromium.org/9515015/diff/1/third_party/tlslite/README.chro...
> > File third_party/tlslite/README.chromium (right):
> > 
> >
>
http://codereview.chromium.org/9515015/diff/1/third_party/tlslite/README.chro...
> > third_party/tlslite/README.chromium:30: - patches/parse_chain.patch:
> > tlslite/X509.py and tlslite/X509CertChain.py were
> > .patch file missing.
> > 
> > Did you try to upstream this change?
> 
> No, because upstream is at 0.4.0 and we're still at 0.3.8, and it's a
> non-trivial/non-high-priority effort to get to 0.4.0.

So the plan is to create a fork of this? :-/

[Ryan Sleevi, 2012-02-29 00:35:46]

On 2012/02/29 00:34:11, Nico wrote:
> On 2012/02/29 00:29:46, Ryan Sleevi wrote:
> > On 2012/02/29 00:28:21, Nico wrote:
> > >
> >
>
http://codereview.chromium.org/9515015/diff/1/third_party/tlslite/README.chro...
> > > File third_party/tlslite/README.chromium (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/9515015/diff/1/third_party/tlslite/README.chro...
> > > third_party/tlslite/README.chromium:30: - patches/parse_chain.patch:
> > > tlslite/X509.py and tlslite/X509CertChain.py were
> > > .patch file missing.
> > > 
> > > Did you try to upstream this change?
> > 
> > No, because upstream is at 0.4.0 and we're still at 0.3.8, and it's a
> > non-trivial/non-high-priority effort to get to 0.4.0.
> 
> So the plan is to create a fork of this? :-/

We're already running a fork - see the README.chromium.

tlslite was unmaintained for almost 7 years. The author has since started
working on it, upgrading to 0.4.0 with some serious refactoring, but which also
incorporates all of our patches (slightly modified).

This is continuing our fork of 0.3.8 until we can upgrade to 0.4.0, upstream
this, then upgrade to 0.4.1 (presumed)

[Ryan Sleevi, 2012-02-29 01:01:58]

PTAL

http://codereview.chromium.org/9515015/diff/4001/third_party/tlslite/tlslite/...
File third_party/tlslite/tlslite/X509.py (right):

http://codereview.chromium.org/9515015/diff/4001/third_party/tlslite/tlslite/...
third_party/tlslite/tlslite/X509.py:102: return self
justification: matches line 45

[Nico, 2012-02-29 02:05:30]

Looks fine from a python style point of view now. Maybe you also want a review
from someone who actually knows what this does :-)

If not, lgtm with .patch file.

https://chromiumcodereview.appspot.com/9515015/diff/6001/third_party/tlslite/...
File third_party/tlslite/README.chromium (right):

https://chromiumcodereview.appspot.com/9515015/diff/6001/third_party/tlslite/...
third_party/tlslite/README.chromium:30: - patches/parse_chain.patch:
tlslite/X509CertChain.py and tlslite/X509.py were
Still missing this .patch file

https://chromiumcodereview.appspot.com/9515015/diff/6001/third_party/tlslite/...
File third_party/tlslite/tlslite/X509CertChain.py (right):

https://chromiumcodereview.appspot.com/9515015/diff/6001/third_party/tlslite/...
third_party/tlslite/tlslite/X509CertChain.py:31: @type s: str
Looks like this is still called s

[Ryan Sleevi, 2012-02-29 19:56:27]

agl: Would you mind reviewing/sanity checking/stamping this from a PEM parsing
logic standpoint?

http://codereview.chromium.org/9515015/diff/6001/third_party/tlslite/tlslite/...
File third_party/tlslite/tlslite/X509CertChain.py (right):

http://codereview.chromium.org/9515015/diff/6001/third_party/tlslite/tlslite/...
third_party/tlslite/tlslite/X509CertChain.py:31: @type s: str
On 2012/02/29 02:05:31, Nico wrote:
> Looks like this is still called s

This parameter matches the correlated X509.parse(self, s) declaration.

[Ryan Sleevi, 2012-03-02 01:18:30]

-agl, +wtc, as agl@ is out today.

wtc, would you mind taking a stamp^H^Hb at this?

[agl, 2012-03-02 01:24:51]

lgtm

http://codereview.chromium.org/9515015/diff/13001/third_party/tlslite/tlslite...
File third_party/tlslite/tlslite/X509CertChain.py (right):

http://codereview.chromium.org/9515015/diff/13001/third_party/tlslite/tlslite...
third_party/tlslite/tlslite/X509CertChain.py:51: _CERTIFICATE_HEADER =
"----BEGIN CERTIFICATE-----"
There's only four '-' at the beginning. Also, you might want to put \n at the
beginning and end (and update some of the indexing accordingly.)

[commit-bot: I haz the power, 2012-03-02 01:34:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/9515015/18002

[commit-bot: I haz the power, 2012-03-02 14:58:05]

Change committed as 124637

[wtc, 2012-03-02 23:32:18]

Patch set 5 LGTM.

http://codereview.chromium.org/9515015/diff/18002/third_party/tlslite/tlslite...
File third_party/tlslite/tlslite/X509.py (right):

http://codereview.chromium.org/9515015/diff/18002/third_party/tlslite/tlslite...
third_party/tlslite/tlslite/X509.py:102: return self

Is it a bug for a method to be missing a return statement
at the end?

http://codereview.chromium.org/9515015/diff/18002/third_party/tlslite/tlslite...
File third_party/tlslite/tlslite/X509CertChain.py (right):

http://codereview.chromium.org/9515015/diff/18002/third_party/tlslite/tlslite...
third_party/tlslite/tlslite/X509CertChain.py:29: """Parse a PEM-encoded X.509
certificate file chain file.

Typo: certificate file chain file => certificate chain file

http://codereview.chromium.org/9515015/diff/18002/third_party/tlslite/tlslite...
third_party/tlslite/tlslite/X509CertChain.py:32: @param s: A PEM-encoded (eg:
Base64) X.509 certificate file, with every

Should we say "certificate chain file" instead?

http://codereview.chromium.org/9515015/diff/18002/third_party/tlslite/tlslite...
third_party/tlslite/tlslite/X509CertChain.py:34: "-----END CERTIFICATE-----"
tags). Extraneous data outside such tags,

Remove the closing ')' after "tags".  Make the same fix on
line 44.

http://codereview.chromium.org/9515015/diff/18002/third_party/tlslite/tlslite...
third_party/tlslite/tlslite/X509CertChain.py:79: certStr =
self.data[self.index+len(self._CERTIFICATE_HEADER) :

Nit: add spaces around the '+'?  Or are you trying to stay
under the 80-char limit?