Issue 11040043: Add a cert utility

Can't Edit
Can't Publish+Mail
Start Review

Created:
10 years, 9 months ago by ekasper

Modified:
10 years, 9 months ago

Reviewers:
Eran, Ben Laurie (Google)

CC:
ctlog-opensource-review_google.com

Visibility:
Public.

More Reviews

Patch Set 2 : more functionality #

Patch Set 4 : comments #

Created: 10 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+269 lines, -11 lines)			Patch
M	src/python/ct/crypto/cert.py	View	1 2 3	5 chunks	+52 lines, -6 lines	0 comments	Download
M	src/python/ct/crypto/cert_test.py	View	1 2 3	4 chunks	+33 lines, -4 lines	0 comments	Download
M	src/python/ct/crypto/testdata/google_cert.pem	View	1	1 chunk	+1 line, -1 line	0 comments	Download
A	src/python/ct/crypto/testdata/google_chain.pem	View	1	1 chunk	+57 lines, -0 lines	0 comments	Download
A	src/python/ct/crypto/tools/cert_util.py	View	1 2 3	1 chunk	+126 lines, -0 lines	0 comments	Download

Messages

Total messages: 8

Expand All Messages | Collapse All Messages

ekasper

This adds a little stand-alone tool on top of the X509 module. It's fairly straighforward ...

10 years, 9 months ago (2013-07-09 14:00:45 UTC) #1

Ben Laurie (Google)

LGTM. https://codereview.appspot.com/11040043/diff/1/src/python/ct/crypto/tools/cert_util.py File src/python/ct/crypto/tools/cert_util.py (right): https://codereview.appspot.com/11040043/diff/1/src/python/ct/crypto/tools/cert_util.py#newcode17 src/python/ct/crypto/tools/cert_util.py:17: cert_util.py print --subject cert.pem - print the subject ...

10 years, 9 months ago (2013-07-11 15:12:22 UTC) #3

ekasper

https://codereview.appspot.com/11040043/diff/1/src/python/ct/crypto/tools/cert_util.py File src/python/ct/crypto/tools/cert_util.py (right): https://codereview.appspot.com/11040043/diff/1/src/python/ct/crypto/tools/cert_util.py#newcode17 src/python/ct/crypto/tools/cert_util.py:17: cert_util.py print --subject cert.pem - print the subject name ...

10 years, 9 months ago (2013-07-11 17:37:23 UTC) #4

https://codereview.appspot.com/11040043/diff/1/src/python/ct/crypto/tools/cer...
File src/python/ct/crypto/tools/cert_util.py (right):

https://codereview.appspot.com/11040043/diff/1/src/python/ct/crypto/tools/cer...
src/python/ct/crypto/tools/cert_util.py:17: cert_util.py print --subject
cert.pem - print the subject name
On 2013/07/11 15:12:23, Ben Laurie (Google) wrote:
> Not hugely worried, but odd that you don't show the only other flag :-)

Forgot to update the doc. Done.

https://codereview.appspot.com/11040043/diff/1/src/python/ct/crypto/tools/cer...
src/python/ct/crypto/tools/cert_util.py:27: gflags.DEFINE_bool('subject', False,
"Print option: prints certificate subject")
On 2013/07/11 15:12:23, Ben Laurie (Google) wrote:
> Linter nags me about matching quotes (i.e. choose " or ', don't mix). So I
guess
> I should nag you :-)

Done. Also fixed an assortment of other linter errors in this file.

https://codereview.appspot.com/11040043/diff/1/src/python/ct/crypto/tools/cer...
src/python/ct/crypto/tools/cert_util.py:37: # are less helpful because we don't
know the expected file format.
On 2013/07/11 15:12:23, Ben Laurie (Google) wrote:
> I approve of this plan, but...
> 
> Add a way to force a format? Or a FIXME for it?

It was already a mental FIXME :) Done.

https://codereview.appspot.com/11040043/diff/1/src/python/ct/crypto/tools/cer...
src/python/ct/crypto/tools/cert_util.py:87: exit_with_message("Unknown extra
arguments: %s" % argv[1:])
On 2013/07/11 15:12:23, Ben Laurie (Google) wrote:
> Process all arguments as separate files?

Done.

> 
> Also, Eran has wished for a certificate dumper that can take multiple certs in
a
> single file and dump them all.

Done. Note --subject (or --subject --issuer) now dumps the issuer chain for
chain files and everything.

> 
> Also, I have wished for a dumper that would take straight b64 encoding (as
used
> in JSON) without the PEM delimeters.

I wasn't sure where you'd need that, as iirc the JSON returns TLS-encoded
structures rather than base64 cert arrays?

But happy to add it if you have a use case for single-cert base64 files that you
can't easily b64-decode into DERs.

> Feel free to add as TODOs.

ekasper

10 years, 9 months ago (2013-07-11 17:39:38 UTC) #5

Eran

Minor comments, sorry for the late review. https://codereview.appspot.com/11040043/diff/10001/src/python/ct/crypto/cert.py File src/python/ct/crypto/cert.py (right): https://codereview.appspot.com/11040043/diff/10001/src/python/ct/crypto/cert.py#newcode45 src/python/ct/crypto/cert.py:45: def all_from_pem(cls, ...

10 years, 9 months ago (2013-07-11 19:47:45 UTC) #6

ekasper

10 years, 9 months ago (2013-07-12 10:12:32 UTC) #8

https://codereview.appspot.com/11040043/diff/10001/src/python/ct/crypto/cert.py
File src/python/ct/crypto/cert.py (right):

https://codereview.appspot.com/11040043/diff/10001/src/python/ct/crypto/cert....
src/python/ct/crypto/cert.py:45: def all_from_pem(cls, pem_string):
On 2013/07/11 19:47:45, Eran wrote:
> This is a method I'd expect to see at a module-level, not in the class
itself...

The related from_pem() and from_pem_file() methods are class methods, but as you
prefer. Done.

https://codereview.appspot.com/11040043/diff/10001/src/python/ct/crypto/cert....
src/python/ct/crypto/cert.py:58: pem_string, cls._PEM_MARKERS,
skip_invalid_blobs=False):
On 2013/07/11 19:47:45, Eran wrote:
> Nit: Allow the caller to specify whether to skip invalid blobs or not?

Ok, but I've flipped the default.

https://codereview.appspot.com/11040043/diff/10001/src/python/ct/crypto/cert_...
File src/python/ct/crypto/cert_test.py (right):

https://codereview.appspot.com/11040043/diff/10001/src/python/ct/crypto/cert_...
src/python/ct/crypto/cert_test.py:49: self.assertTrue(all(map(lambda x:
isinstance(x, cert.Certificate),
On 2013/07/11 19:47:45, Eran wrote:
> My understanding is that the order is important, so I'd suggest verifying it.

Good call, done.

https://codereview.appspot.com/11040043/diff/10001/src/python/ct/crypto/tools...
File src/python/ct/crypto/tools/cert_util.py (right):

https://codereview.appspot.com/11040043/diff/10001/src/python/ct/crypto/tools...
src/python/ct/crypto/tools/cert_util.py:6: cert_util.py <command> [flags]
[cert_file ...]
On 2013/07/11 19:47:45, Eran wrote:
> Call this cert_print rather than cert_util, since it only does one thing now?
> It's not immediately clear which of the code in this file would be shared
> between different utilities, should we add more later on.

I certainly plan to add more functionality, so I'd rather keep it as is and
refactor shared parts when this happens.

Expand All Messages | Collapse All Messages