Issue 12858048: Interpret ASN.1 time

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

Created:
10 years, 8 months ago by ekasper

Modified:
10 years, 8 months ago

Reviewers:
Eran, Ben Laurie (Google)

CC:
ctlog-opensource-review_google.com

Visibility:
Public.

More Reviews

Patch Set 2 : sync method name #

Total comments: 6

Patch Set 3 : comments #

Created: 10 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+239 lines, -18 lines)			Patch
M	src/python/Makefile	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	src/python/ct/crypto/asn1/types.py	View		2 chunks	+9 lines, -15 lines	0 comments	Download
M	src/python/ct/crypto/asn1/x509.py	View		2 chunks	+7 lines, -3 lines	0 comments	Download
A	src/python/ct/crypto/asn1/x509_time.py	View	1 2	1 chunk	+111 lines, -0 lines	0 comments	Download
A	src/python/ct/crypto/asn1/x509_time_test.py	View	1 2	1 chunk	+36 lines, -0 lines	0 comments	Download
M	src/python/ct/crypto/cert.py	View		4 chunks	+51 lines, -0 lines	0 comments	Download
M	src/python/ct/crypto/cert_test.py	View		2 chunks	+24 lines, -0 lines	0 comments	Download

Messages

Total messages: 3

Expand All Messages | Collapse All Messages

Eran

Overall LGTM, few small comments. https://codereview.appspot.com/12858048/diff/3001/src/python/ct/crypto/asn1/x509_time.py File src/python/ct/crypto/asn1/x509_time.py (right): https://codereview.appspot.com/12858048/diff/3001/src/python/ct/crypto/asn1/x509_time.py#newcode43 src/python/ct/crypto/asn1/x509_time.py:43: if len(string_time) != 13 ...

10 years, 8 months ago (2013-08-23 08:20:46 UTC) #2

ekasper

10 years, 8 months ago (2013-08-23 10:12:12 UTC) #3

Thanks for picking this up!

I also ran linter on the new files, and am submitting now.

https://codereview.appspot.com/12858048/diff/3001/src/python/ct/crypto/asn1/x...
File src/python/ct/crypto/asn1/x509_time.py (right):

https://codereview.appspot.com/12858048/diff/3001/src/python/ct/crypto/asn1/x...
src/python/ct/crypto/asn1/x509_time.py:43: if len(string_time) != 13 or
string_time[-1] != "Z":
On 2013/08/23 08:20:47, Eran wrote:
> Have a constant describing where '13' comes from.

Done.

https://codereview.appspot.com/12858048/diff/3001/src/python/ct/crypto/asn1/x...
src/python/ct/crypto/asn1/x509_time.py:80: if len(string_time) != 15 or
string_time[-1] != "Z":
On 2013/08/23 08:20:47, Eran wrote:
> This check is very similar to the one above, except for the expected length.
> Could they be generalized?

Even though the check is now exactly the same, I don't think it belongs in the
base class (which is just an interface that doesn't know about the ASN.1 string
rep) so I'm leaving it as is.

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

https://codereview.appspot.com/12858048/diff/3001/src/python/ct/crypto/cert.p...
src/python/ct/crypto/cert.py:47: def __cache_expiry(self):
On 2013/08/23 08:20:47, Eran wrote:
> nit: Name this 'copy expiry' rather than cache expiry, since this object seems
> immutable (so there's no 'invalidation' of the 'cached' data)

I think I prefer to leave it as is (in sync with __cached_der).

Expand All Messages | Collapse All Messages