X509 module for parsing certificates

X509 module for parsing certificates

[ekasper, 2013-07-04 18:45:14]

This CL implements the first half of RFC 5280 (todo: extensions and time).

How to review this CL:
* it's probably helpful to first get familiar with pyasn1 at
http://pyasn1.sourceforge.net/. It's quite well documented.

* asn1/types.py is an ASN.1 type hierarchy on top of pyasn1, which allows us to
add custom functionality to pyasn1 base types. It's ugly-scary multiple
inheritance but it's needed to make arbitrary recursion work.

* asn1/oid.py, asn1/x509.py and asn1/x509_name.py are the modules that implement
X509-specific ASN.1 types. You can compare these to the old X509 module at
http://pyasn1.sourceforge.net/rfc2459.html

(Notice that x509.py has no tests as it's just spelling out the spec without
adding any new functionality, so I couldn't think of any meaningful tests to
add.)

* cert.py is the main certificate module.

* testdata/ contains a valid google cert

(I'll send output samples in a second.)

[ekasper, 2013-07-04 18:50:54]

I probably went a little OCD on the cert printing code but given how many
times a day I run 'openssl x509 -in cert.pem -text', it's probably worth
it. Below is the output from printing the test google cert. The stuff in
raw hex mostly corresponds to parts that need more custom decoding (keys,
extensions, parameters).

Certificate:
  tbsCertificate:
    version: v3
    serialNumber: 60:53:81:f5:00:01:00:00:88:bd
    signature:
      algorithm: RSA-SHA1
      parameters: 05:00
    issuer: C=US/O=Google Inc/CN=Google Internet Authority
    validity:
      notBefore: 130522154904Z
      notAfter: 131031235959Z
    subject: C=US/ST=California/L=Mountain View/O=Google Inc/CN=*.google.com
    subjectPublicKeyInfo:
      algorithm:
        algorithm: EC-PUBKEY
        parameters: 06:08:2a:86:48:ce:3d:03:01:07
      subjectPublicKey:

04:66:4a:92:14:6c:2a:a1:50:1a:b5:53:09:d1:02:2e:ff:4c:d4:a4:e9:6f:09:99:

ab:11:f8:8d:2b:fc:d2:c3:62:fe:72:02:96:c0:cf:c1:d7:5c:df:21:0d:db:ab:31:
        13:fa:27:2d:50:d1:42:0a:9b:29:6c:f0:73:c7:7b:1a:02
    extensions:
      0: extnID: 2.5.29.37, critical: False,
        extnValue:

04:16:30:14:06:08:2b:06:01:05:05:07:03:01:06:08:2b:06:01:05:05:07:03:
          02
      1: extnID: 2.5.29.15, critical: False, extnValue: 04:04:03:02:07:80
      2: extnID: 2.5.29.14, critical: False,
        extnValue:

04:16:04:14:53:78:d3:d0:d5:4d:46:05:39:66:29:d1:1c:6a:e5:ca:81:84:9e:
          86
      3: extnID: 2.5.29.35, critical: False,
        extnValue:

04:18:30:16:80:14:bf:c0:30:eb:f5:43:11:3e:67:ba:9e:91:fb:fc:6a:da:e3:
          6b:12:24
      4: extnID: 2.5.29.31, critical: False,
        extnValue:

04:54:30:52:30:50:a0:4e:a0:4c:86:4a:68:74:74:70:3a:2f:2f:77:77:77:2e:

67:73:74:61:74:69:63:2e:63:6f:6d:2f:47:6f:6f:67:6c:65:49:6e:74:65:72:

6e:65:74:41:75:74:68:6f:72:69:74:79:2f:47:6f:6f:67:6c:65:49:6e:74:65:
          72:6e:65:74:41:75:74:68:6f:72:69:74:79:2e:63:72:6c
      5: extnID: 1.3.6.1.5.5.7.1.1, critical: False,
        extnValue:

04:5a:30:58:30:56:06:08:2b:06:01:05:05:07:30:02:86:4a:68:74:74:70:3a:

2f:2f:77:77:77:2e:67:73:74:61:74:69:63:2e:63:6f:6d:2f:47:6f:6f:67:6c:

65:49:6e:74:65:72:6e:65:74:41:75:74:68:6f:72:69:74:79:2f:47:6f:6f:67:

6c:65:49:6e:74:65:72:6e:65:74:41:75:74:68:6f:72:69:74:79:2e:63:72:74
      6: extnID: 2.5.29.19, critical: True, extnValue: 04:02:30:00
      7: extnID: 2.5.29.17, critical: False,
        extnValue:

04:82:02:ba:30:82:02:b6:82:0c:2a:2e:67:6f:6f:67:6c:65:2e:63:6f:6d:82:

0d:2a:2e:61:6e:64:72:6f:69:64:2e:63:6f:6d:82:16:2a:2e:61:70:70:65:6e:

67:69:6e:65:2e:67:6f:6f:67:6c:65:2e:63:6f:6d:82:12:2a:2e:63:6c:6f:75:

64:2e:67:6f:6f:67:6c:65:2e:63:6f:6d:82:16:2a:2e:67:6f:6f:67:6c:65:2d:

61:6e:61:6c:79:74:69:63:73:2e:63:6f:6d:82:0b:2a:2e:67:6f:6f:67:6c:65:

2e:63:61:82:0b:2a:2e:67:6f:6f:67:6c:65:2e:63:6c:82:0e:2a:2e:67:6f:6f:

67:6c:65:2e:63:6f:2e:69:6e:82:0e:2a:2e:67:6f:6f:67:6c:65:2e:63:6f:2e:

6a:70:82:0e:2a:2e:67:6f:6f:67:6c:65:2e:63:6f:2e:75:6b:82:0f:2a:2e:67:

6f:6f:67:6c:65:2e:63:6f:6d:2e:61:72:82:0f:2a:2e:67:6f:6f:67:6c:65:2e:

63:6f:6d:2e:61:75:82:0f:2a:2e:67:6f:6f:67:6c:65:2e:63:6f:6d:2e:62:72:

82:0f:2a:2e:67:6f:6f:67:6c:65:2e:63:6f:6d:2e:63:6f:82:0f:2a:2e:67:6f:

6f:67:6c:65:2e:63:6f:6d:2e:6d:78:82:0f:2a:2e:67:6f:6f:67:6c:65:2e:63:

6f:6d:2e:74:72:82:0f:2a:2e:67:6f:6f:67:6c:65:2e:63:6f:6d:2e:76:6e:82:

0b:2a:2e:67:6f:6f:67:6c:65:2e:64:65:82:0b:2a:2e:67:6f:6f:67:6c:65:2e:

65:73:82:0b:2a:2e:67:6f:6f:67:6c:65:2e:66:72:82:0b:2a:2e:67:6f:6f:67:

6c:65:2e:68:75:82:0b:2a:2e:67:6f:6f:67:6c:65:2e:69:74:82:0b:2a:2e:67:

6f:6f:67:6c:65:2e:6e:6c:82:0b:2a:2e:67:6f:6f:67:6c:65:2e:70:6c:82:0b:

2a:2e:67:6f:6f:67:6c:65:2e:70:74:82:0f:2a:2e:67:6f:6f:67:6c:65:61:70:

69:73:2e:63:6e:82:14:2a:2e:67:6f:6f:67:6c:65:63:6f:6d:6d:65:72:63:65:

2e:63:6f:6d:82:0d:2a:2e:67:73:74:61:74:69:63:2e:63:6f:6d:82:0c:2a:2e:

75:72:63:68:69:6e:2e:63:6f:6d:82:10:2a:2e:75:72:6c:2e:67:6f:6f:67:6c:

65:2e:63:6f:6d:82:16:2a:2e:79:6f:75:74:75:62:65:2d:6e:6f:63:6f:6f:6b:

69:65:2e:63:6f:6d:82:0d:2a:2e:79:6f:75:74:75:62:65:2e:63:6f:6d:82:16:

2a:2e:79:6f:75:74:75:62:65:65:64:75:63:61:74:69:6f:6e:2e:63:6f:6d:82:

0b:2a:2e:79:74:69:6d:67:2e:63:6f:6d:82:0b:61:6e:64:72:6f:69:64:2e:63:

6f:6d:82:04:67:2e:63:6f:82:06:67:6f:6f:2e:67:6c:82:14:67:6f:6f:67:6c:

65:2d:61:6e:61:6c:79:74:69:63:73:2e:63:6f:6d:82:0a:67:6f:6f:67:6c:65:

2e:63:6f:6d:82:12:67:6f:6f:67:6c:65:63:6f:6d:6d:65:72:63:65:2e:63:6f:

6d:82:0a:75:72:63:68:69:6e:2e:63:6f:6d:82:08:79:6f:75:74:75:2e:62:65:

82:0b:79:6f:75:74:75:62:65:2e:63:6f:6d:82:14:79:6f:75:74:75:62:65:65:
          64:75:63:61:74:69:6f:6e:2e:63:6f:6d
  signatureAlgorithm:
    algorithm: RSA-SHA1
    parameters: 05:00
  signatureValue:

03:27:d0:ad:e3:df:28:42:f9:7f:ae:ca:1e:9e:00:16:b6:12:ae:f1:89:8e:7f:99:40:

7c:f3:a2:2b:d7:db:f1:96:e4:8c:34:a0:fa:9f:f8:98:f2:f1:e3:b6:b9:f4:06:1b:96:

f3:fb:e6:27:2b:9d:16:42:1c:10:35:18:13:88:af:40:c9:6c:7c:82:fb:48:e1:b0:e8:

e7:af:54:b9:a6:d3:66:a2:44:75:a3:e5:3d:b4:0e:cf:92:3a:9b:9e:61:41:b7:34:17:

07:3f:fe:48:ce:15:18:21:02:5b:1b:39:50:69:46:99:04:c6:90:78:2f:19:b2:53:0d:
    2d:62:36



On Thu, Jul 4, 2013 at 8:45 PM, <ekasper@google.com> wrote:

> Reviewers: Eran,
>
> Message:
> This CL implements the first half of RFC 5280 (todo: extensions and
> time).
>
> How to review this CL:
> * it's probably helpful to first get familiar with pyasn1 at
> http://pyasn1.sourceforge.net/**. It's quite well documented.
>
> * asn1/types.py is an ASN.1 type hierarchy on top of pyasn1, which
> allows us to add custom functionality to pyasn1 base types. It's
> ugly-scary multiple inheritance but it's needed to make arbitrary
> recursion work.
>
> * asn1/oid.py, asn1/x509.py and asn1/x509_name.py are the modules that
> implement X509-specific ASN.1 types. You can compare these to the old
> X509 module at
http://pyasn1.sourceforge.net/**rfc2459.html<http://pyasn1.sourceforge.net/rf...
>
> (Notice that x509.py has no tests as it's just spelling out the spec
> without adding any new functionality, so I couldn't think of any
> meaningful tests to add.)
>
> * cert.py is the main certificate module.
>
> * testdata/ contains a valid google cert
>
> (I'll send output samples in a second.)
>
>
>
>
>
> Description:
> X509 module for parsing certificates
>
> Please review this at
https://codereview.appspot.**com/10944043/<https://codereview.appspot.com/109...
>
> Affected files:
>   M src/python/Makefile
>   A + src/python/ct/crypto/asn1/__**init__.py
>   A src/python/ct/crypto/asn1/oid.**py
>   A src/python/ct/crypto/asn1/oid_**test.py
>   A src/python/ct/crypto/asn1/str_**util.py
>   A src/python/ct/crypto/asn1/str_**util_test.py
>   A src/python/ct/crypto/asn1/**types.py
>   A src/python/ct/crypto/asn1/**types_test.py
>   A src/python/ct/crypto/asn1/**x509.py
>   A src/python/ct/crypto/asn1/**x509_name.py
>   A src/python/ct/crypto/asn1/**x509_name_test.py
>   A src/python/ct/crypto/cert.py
>   A src/python/ct/crypto/cert_**test.py
>   M src/python/ct/crypto/error.py
>   A src/python/ct/crypto/testdata/**google_cert.der
>   A src/python/ct/crypto/testdata/**google_cert.pem
>
>
>

[Ben Laurie (Google), 2013-07-05 09:32:19]

DBC...

Also, why is testdata/google_cert.der empty?

https://codereview.appspot.com/10944043/diff/3001/src/python/ct/crypto/asn1/s...
File src/python/ct/crypto/asn1/str_util.py (right):

https://codereview.appspot.com/10944043/diff/3001/src/python/ct/crypto/asn1/s...
src/python/ct/crypto/asn1/str_util.py:5: '[1,0,0,1,1,0,1,0,0,1,0]' becomes
04:d2.
Perhaps explain that bytes are taken from the right? Or, alternatively, that it
is zero-padded to the left?

https://codereview.appspot.com/10944043/diff/3001/src/python/ct/crypto/asn1/s...
src/python/ct/crypto/asn1/str_util.py:34: 1234 (0x4d2) becomes 04:d2 and -1234
becomes '(-)04:d2'
From the code it looks like -1234 becomes ' -:04:d2'

https://codereview.appspot.com/10944043/diff/3001/src/python/ct/crypto/asn1/t...
File src/python/ct/crypto/asn1/types.py (right):

https://codereview.appspot.com/10944043/diff/3001/src/python/ct/crypto/asn1/t...
src/python/ct/crypto/asn1/types.py:227: # represent default type specification.
Its value is a NamedTypes object.
s/represent/represents/?

https://codereview.appspot.com/10944043/diff/3001/src/python/ct/crypto/asn1/t...
src/python/ct/crypto/asn1/types.py:292: if not print_component:
Isn't it an error for there to be nothing to print?

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

https://codereview.appspot.com/10944043/diff/3001/src/python/ct/crypto/asn1/x...
src/python/ct/crypto/asn1/x509.py:30: """
I'm assuming this has been modified? In which case, we should say so.

https://codereview.appspot.com/10944043/diff/3001/src/python/ct/crypto/asn1/x...
src/python/ct/crypto/asn1/x509.py:116:
namedtype.OptionalNamedType('issuerUniqueID', UniqueIdentifier().subtype(
wrap

https://codereview.appspot.com/10944043/diff/3001/src/python/ct/crypto/asn1/x...
src/python/ct/crypto/asn1/x509.py:118:
namedtype.OptionalNamedType('subjectUniqueID', UniqueIdentifier().subtype(
wrap

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

https://codereview.appspot.com/10944043/diff/3001/src/python/ct/crypto/asn1/x...
src/python/ct/crypto/asn1/x509_name.py:50: raise error.ASN1Error("Attempting to
decode an incomplete object %s" %
wrap

https://codereview.appspot.com/10944043/diff/3001/src/python/ct/crypto/asn1/x...
src/python/ct/crypto/asn1/x509_name.py:189: # The capitalization inconsistency
hurts my eyes but is taken verbatim from the RFC.
wrap

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

https://codereview.appspot.com/10944043/diff/3001/src/python/ct/crypto/asn1/x...
src/python/ct/crypto/asn1/x509_name_test.py:14:
self.assertRaises(error.UnknownASN1AttributeTypeError, unknown.value_type)
wrap

[ekasper, 2013-07-05 10:43:52]

the DER cert is not empty btw:
diff --git a/src/python/ct/crypto/testdata/google_cert.der
b/src/python/ct/crypto/testdata/google_cert.der
new file mode 100644
index 0000000..361af69
Binary files /dev/null and b/src/python/ct/crypto/testdata/google_cert.der
differ

I think Rietveld just chokes on a binary file?

https://codereview.appspot.com/10944043/diff/3001/src/python/ct/crypto/asn1/s...
File src/python/ct/crypto/asn1/str_util.py (right):

https://codereview.appspot.com/10944043/diff/3001/src/python/ct/crypto/asn1/s...
src/python/ct/crypto/asn1/str_util.py:5: '[1,0,0,1,1,0,1,0,0,1,0]' becomes
04:d2.
On 2013/07/05 09:32:20, Ben Laurie (Google) wrote:
> Perhaps explain that bytes are taken from the right? Or, alternatively, that
it
> is zero-padded to the left?

Done.

https://codereview.appspot.com/10944043/diff/3001/src/python/ct/crypto/asn1/s...
src/python/ct/crypto/asn1/str_util.py:34: 1234 (0x4d2) becomes 04:d2 and -1234
becomes '(-)04:d2'
On 2013/07/05 09:32:20, Ben Laurie (Google) wrote:
> From the code it looks like -1234 becomes ' -:04:d2'

Good catch, I changed it and forgot to update the doc. Fixed.

https://codereview.appspot.com/10944043/diff/3001/src/python/ct/crypto/asn1/t...
File src/python/ct/crypto/asn1/types.py (right):

https://codereview.appspot.com/10944043/diff/3001/src/python/ct/crypto/asn1/t...
src/python/ct/crypto/asn1/types.py:227: # represent default type specification.
Its value is a NamedTypes object.
On 2013/07/05 09:32:20, Ben Laurie (Google) wrote:
> s/represent/represents/?

Done.

https://codereview.appspot.com/10944043/diff/3001/src/python/ct/crypto/asn1/t...
src/python/ct/crypto/asn1/types.py:292: if not print_component:
On 2013/07/05 09:32:20, Ben Laurie (Google) wrote:
> Isn't it an error for there to be nothing to print?

You could argue that no type should ever specify an empty representation but I'm
hesitant to make it an error here...

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

https://codereview.appspot.com/10944043/diff/3001/src/python/ct/crypto/asn1/x...
src/python/ct/crypto/asn1/x509.py:30: """
On 2013/07/05 09:32:20, Ben Laurie (Google) wrote:
> I'm assuming this has been modified? In which case, we should say so.

I've made it "partially based". If there's a better stock wording I should use,
let me know.

https://codereview.appspot.com/10944043/diff/3001/src/python/ct/crypto/asn1/x...
src/python/ct/crypto/asn1/x509.py:116:
namedtype.OptionalNamedType('issuerUniqueID', UniqueIdentifier().subtype(
On 2013/07/05 09:32:20, Ben Laurie (Google) wrote:
> wrap

Done.

https://codereview.appspot.com/10944043/diff/3001/src/python/ct/crypto/asn1/x...
src/python/ct/crypto/asn1/x509.py:118:
namedtype.OptionalNamedType('subjectUniqueID', UniqueIdentifier().subtype(
On 2013/07/05 09:32:20, Ben Laurie (Google) wrote:
> wrap

Done.

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

https://codereview.appspot.com/10944043/diff/3001/src/python/ct/crypto/asn1/x...
src/python/ct/crypto/asn1/x509_name.py:50: raise error.ASN1Error("Attempting to
decode an incomplete object %s" %
On 2013/07/05 09:32:20, Ben Laurie (Google) wrote:
> wrap

Done.

https://codereview.appspot.com/10944043/diff/3001/src/python/ct/crypto/asn1/x...
src/python/ct/crypto/asn1/x509_name.py:189: # The capitalization inconsistency
hurts my eyes but is taken verbatim from the RFC.
On 2013/07/05 09:32:20, Ben Laurie (Google) wrote:
> wrap

Done.

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

https://codereview.appspot.com/10944043/diff/3001/src/python/ct/crypto/asn1/x...
src/python/ct/crypto/asn1/x509_name_test.py:14:
self.assertRaises(error.UnknownASN1AttributeTypeError, unknown.value_type)
On 2013/07/05 09:32:20, Ben Laurie (Google) wrote:
> wrap

Done.

[Eran, 2013-07-08 18:27:12]

Some comments. It's not a complete review, though.
I will take a closer look at the representation of types, though, tomorrow.

https://codereview.appspot.com/10944043/diff/22001/src/python/ct/crypto/asn1/...
File src/python/ct/crypto/asn1/__init__.py (right):

https://codereview.appspot.com/10944043/diff/22001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/__init__.py:1: #!/usr/bin/env python
This could simply be an empty file, no need for content in it.

https://codereview.appspot.com/10944043/diff/22001/src/python/ct/crypto/asn1/...
File src/python/ct/crypto/asn1/oid_test.py (right):

https://codereview.appspot.com/10944043/diff/22001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/oid_test.py:19: self.assertNotEqual(-1,
unknown.string_value().find("1.2.3.4"))
You could use:
 '1.2.3.4' in unknown.string_value()
rather than find and comparing to -1.

https://codereview.appspot.com/10944043/diff/22001/src/python/ct/crypto/asn1/...
File src/python/ct/crypto/asn1/str_util.py (right):

https://codereview.appspot.com/10944043/diff/22001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/str_util.py:1: # Utilities for printing ASN.1 values
Consider renaming this file to print_utils or similar. str_util hints at general
string utility functions (which, in Python, is not something I've ever seen
anyone need).

https://codereview.appspot.com/10944043/diff/22001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/str_util.py:6: For example, '[1,0,0,1,1,0,1,0,0,1,0]'
becomes 04:d2.
discrepancy between the documentation and test. Please clarify.

https://codereview.appspot.com/10944043/diff/22001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/str_util.py:8: byte_string: the bytes to convert.
discrepancy between the documentation and the argument name. Please fix.

https://codereview.appspot.com/10944043/diff/22001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/str_util.py:21:
byte_array.append(chr(int("".join(bitstring_array[i:i+8]), 2)))
You could use ast.literal_eval of '0b' + bitstring_array[i:i+8] rather than
joining and converting to int.

https://codereview.appspot.com/10944043/diff/22001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/str_util.py:31: return delimiter.join([("%02x" %
ord(b)) for b in byte_string])
Consider using binascii.hexlify

https://codereview.appspot.com/10944043/diff/22001/src/python/ct/crypto/asn1/...
File src/python/ct/crypto/asn1/types.py (right):

https://codereview.appspot.com/10944043/diff/22001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/types.py:67: pass
Raise an exception or return str() of the object? Otherwise, extending this
class and failing to implement the interface means the error would be very
confusing (trying to join None with lines from other objects).
That will actually make this class a proper mix-in.

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

https://codereview.appspot.com/10944043/diff/22001/src/python/ct/crypto/cert....
src/python/ct/crypto/cert.py:27: def __str__(self):
Single newline between methods.

[ekasper, 2013-07-09 10:47:26]

https://codereview.appspot.com/10944043/diff/22001/src/python/ct/crypto/asn1/...
File src/python/ct/crypto/asn1/__init__.py (right):

https://codereview.appspot.com/10944043/diff/22001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/__init__.py:1: #!/usr/bin/env python
On 2013/07/08 18:27:12, Eran wrote:
> This could simply be an empty file, no need for content in it.

Done.

https://codereview.appspot.com/10944043/diff/22001/src/python/ct/crypto/asn1/...
File src/python/ct/crypto/asn1/oid_test.py (right):

https://codereview.appspot.com/10944043/diff/22001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/oid_test.py:19: self.assertNotEqual(-1,
unknown.string_value().find("1.2.3.4"))
On 2013/07/08 18:27:12, Eran wrote:
> You could use:
>  '1.2.3.4' in unknown.string_value()
> rather than find and comparing to -1.

Doh. Done, everywhere.

https://codereview.appspot.com/10944043/diff/22001/src/python/ct/crypto/asn1/...
File src/python/ct/crypto/asn1/str_util.py (right):

https://codereview.appspot.com/10944043/diff/22001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/str_util.py:1: # Utilities for printing ASN.1 values
On 2013/07/08 18:27:12, Eran wrote:
> Consider renaming this file to print_utils or similar. str_util hints at
general
> string utility functions (which, in Python, is not something I've ever seen
> anyone need).

I agree, print_utils is better. Done. (I'm afraid this means you've lost the
review diff in Rietveld, sorry.)

https://codereview.appspot.com/10944043/diff/22001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/str_util.py:6: For example, '[1,0,0,1,1,0,1,0,0,1,0]'
becomes 04:d2.
On 2013/07/08 18:27:12, Eran wrote:
> discrepancy between the documentation and test. Please clarify.

If you mean the discrepancy between types, then I've fixed the test to take in
an array of ints rather than chars.

https://codereview.appspot.com/10944043/diff/22001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/str_util.py:8: byte_string: the bytes to convert.
On 2013/07/08 18:27:12, Eran wrote:
> discrepancy between the documentation and the argument name. Please fix.

Copy-paste... Done.

https://codereview.appspot.com/10944043/diff/22001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/str_util.py:21:
byte_array.append(chr(int("".join(bitstring_array[i:i+8]), 2)))
On 2013/07/08 18:27:12, Eran wrote:
> You could use ast.literal_eval of '0b' + bitstring_array[i:i+8] rather than
> joining and converting to int.

I've refactored this to something considerably shorter.

I find the conversion more readable than literal_eval, though - we don't need
safe evaluation of arbitrary input.

https://codereview.appspot.com/10944043/diff/22001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/str_util.py:31: return delimiter.join([("%02x" %
ord(b)) for b in byte_string])
On 2013/07/08 18:27:12, Eran wrote:
> Consider using binascii.hexlify

Afaict hexlify doesn't allow me to specify a delimiter?

https://codereview.appspot.com/10944043/diff/22001/src/python/ct/crypto/asn1/...
File src/python/ct/crypto/asn1/types.py (right):

https://codereview.appspot.com/10944043/diff/22001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/types.py:67: pass
On 2013/07/08 18:27:12, Eran wrote:
> Raise an exception or return str() of the object? Otherwise, extending this
> class and failing to implement the interface means the error would be very
> confusing (trying to join None with lines from other objects).
> That will actually make this class a proper mix-in.

I thought the abc.abstractmethod decorator meant you couldn't instantiate
without overriding?

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

https://codereview.appspot.com/10944043/diff/22001/src/python/ct/crypto/cert....
src/python/ct/crypto/cert.py:27: def __str__(self):
On 2013/07/08 18:27:12, Eran wrote:
> Single newline between methods.

Done.

[Eran, 2013-07-10 08:59:03]

https://codereview.appspot.com/10944043/diff/22001/src/python/ct/crypto/asn1/...
File src/python/ct/crypto/asn1/types.py (right):

https://codereview.appspot.com/10944043/diff/22001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/types.py:67: pass
On 2013/07/09 10:47:26, ekasper wrote:
> On 2013/07/08 18:27:12, Eran wrote:
> > Raise an exception or return str() of the object? Otherwise, extending this
> > class and failing to implement the interface means the error would be very
> > confusing (trying to join None with lines from other objects).
> > That will actually make this class a proper mix-in.
> 
> I thought the abc.abstractmethod decorator meant you couldn't instantiate
> without overriding?

Right - that also works.

https://codereview.appspot.com/10944043/diff/38001/src/python/ct/crypto/asn1/...
File src/python/ct/crypto/asn1/types.py (right):

https://codereview.appspot.com/10944043/diff/38001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/types.py:52: class AbstractBaseType(object):
As I commented offline, if you find that this class gains more responsibility,
it may be worth creating new mix-ins. The name implies that *everything* to do
with base types should go in here, while in practice the code is only concerned
about formatting.

https://codereview.appspot.com/10944043/diff/38001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/types.py:322: yield (self.getNameByPosition(idx),
value)
Would it be possible to generalize the components() method in the base class so
that the code is not repeated here? I notice the only difference is the
transformation applied on idx, so perhaps that could happen in a separate
method.

https://codereview.appspot.com/10944043/diff/38001/src/python/ct/crypto/asn1/...
File src/python/ct/crypto/asn1/types_test.py (right):

https://codereview.appspot.com/10944043/diff/38001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/types_test.py:37: self.assertNotEqual(-1,
i.find("123456789"))
self.assertTrue("123456789" in i) ?

https://codereview.appspot.com/10944043/diff/38001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/types_test.py:46: # TODO(ekasper): test some non-ascii
characters as well.
Here's some:
s = types.TeletexString('\xd7\xa9\xd7\x9c\xd7\x95\xd7\x9d').string_value()
assertTrue('שלום' in s)

(The source should be UTF-8 encoded for that to work!)

[ekasper, 2013-07-10 11:19:24]

https://codereview.appspot.com/10944043/diff/38001/src/python/ct/crypto/asn1/...
File src/python/ct/crypto/asn1/types.py (right):

https://codereview.appspot.com/10944043/diff/38001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/types.py:52: class AbstractBaseType(object):
On 2013/07/10 08:59:04, Eran wrote:
> As I commented offline, if you find that this class gains more responsibility,
> it may be worth creating new mix-ins. The name implies that *everything* to do
> with base types should go in here, while in practice the code is only
concerned
> about formatting.

I think the name implies that every type should inherit from here, and that
every functionality that every type should be able to do should go here :)

But yes, I agree that the base types shouldn't get polluted. Maybe the way to go
is a plugin interface for custom recursive actions; we'll see.

https://codereview.appspot.com/10944043/diff/38001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/types.py:322: yield (self.getNameByPosition(idx),
value)
On 2013/07/10 08:59:04, Eran wrote:
> Would it be possible to generalize the components() method in the base class
so
> that the code is not repeated here? I notice the only difference is the
> transformation applied on idx, so perhaps that could happen in a separate
> method.

Done.

https://codereview.appspot.com/10944043/diff/38001/src/python/ct/crypto/asn1/...
File src/python/ct/crypto/asn1/types_test.py (right):

https://codereview.appspot.com/10944043/diff/38001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/types_test.py:37: self.assertNotEqual(-1,
i.find("123456789"))
On 2013/07/10 08:59:04, Eran wrote:
> self.assertTrue("123456789" in i) ?

Sigh, I missed a file. Done.

https://codereview.appspot.com/10944043/diff/38001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/types_test.py:46: # TODO(ekasper): test some non-ascii
characters as well.
On 2013/07/10 08:59:04, Eran wrote:
> Here's some:
> s = types.TeletexString('\xd7\xa9\xd7\x9c\xd7\x95\xd7\x9d').string_value()
> assertTrue('שלום' in s)
> 
> (The source should be UTF-8 encoded for that to work!)

So it turns out pyasn1 treats all string types as opaque bytestrings and imposes
no alphabet constraints (not even on PrintableString!)

We should add custom validation but for now, I've changed all tests to take a
non-ascii bytestring, and added a comment to the types.

[Eran, 2013-07-11 13:29:33]

Overall LGTM. Minor comments below, I believe the printing code is clearer now.

https://codereview.appspot.com/10944043/diff/43001/src/python/ct/crypto/asn1/...
File src/python/ct/crypto/asn1/oid.py (right):

https://codereview.appspot.com/10944043/diff/43001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/oid.py:14: return _OID_NAME_DICT[self][1]
What's the benefit of looking up self in an external dictionary? A
straightforward way would be to have the short and long names as optional fields
and simply return them. You could then avoid this dict, which means:
* one less data structure around.
* Clear mapping between oid and names (no need to see where the oid is mapped to
a name).

https://codereview.appspot.com/10944043/diff/43001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/oid.py:18: return ".".join(map(str, tuple(self)))
Seem this try/except clause is common between this and the next method, consider
extracting a common one.

https://codereview.appspot.com/10944043/diff/43001/src/python/ct/crypto/asn1/...
File src/python/ct/crypto/asn1/print_util.py (right):

https://codereview.appspot.com/10944043/diff/43001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/print_util.py:15: bitstring = "0"*pad_length +
"".join(map(str, bit_array))
Would it be possible to get to the byte representation without going through the
string representation? Simply extend the input array by pad_length zeros and do
the same iteration?

https://codereview.appspot.com/10944043/diff/43001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/print_util.py:51: """Split long lines into multiple
chunks according to the wrap limit.
according to the wrap limit *and* newlines.

https://codereview.appspot.com/10944043/diff/43001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/print_util.py:63: lines = []
nit: you have line and lines, two variables which differ by one character only.
Rename one of them to clarify what's input and what's output.

https://codereview.appspot.com/10944043/diff/43001/src/python/ct/crypto/asn1/...
File src/python/ct/crypto/asn1/types_test.py (right):

https://codereview.appspot.com/10944043/diff/43001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/types_test.py:52: s =
types.PrintableString(hello).string_value()
You could iterate over the types:
for t in (types.PrintableString, types.UniversalString, ...):
  s = t(hello).string_value()
  self.assertTrue...

https://codereview.appspot.com/10944043/diff/43001/src/python/ct/crypto/asn1/...
File src/python/ct/crypto/asn1/x509_name.py (right):

https://codereview.appspot.com/10944043/diff/43001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/x509_name.py:16: return
_ATTRIBUTE_VALUE_TYPE_DICT[self]
Same comment as in oid.py

[ekasper, 2013-07-11 15:07:41]

Do you want to take another look or is the LGTM sticky?

https://codereview.appspot.com/10944043/diff/43001/src/python/ct/crypto/asn1/...
File src/python/ct/crypto/asn1/oid.py (right):

https://codereview.appspot.com/10944043/diff/43001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/oid.py:14: return _OID_NAME_DICT[self][1]
On 2013/07/11 13:29:33, Eran wrote:
> What's the benefit of looking up self in an external dictionary? A
> straightforward way would be to have the short and long names as optional
fields
> and simply return them. You could then avoid this dict, which means:
> * one less data structure around.
> * Clear mapping between oid and names (no need to see where the oid is mapped
to
> a name).

But who sets the name fields, and how? The ASN.1 wire encoding doesn't include
them...

https://codereview.appspot.com/10944043/diff/43001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/oid.py:18: return ".".join(map(str, tuple(self)))
On 2013/07/11 13:29:33, Eran wrote:
> Seem this try/except clause is common between this and the next method,
consider
> extracting a common one.

Done, and simplified a bit.

https://codereview.appspot.com/10944043/diff/43001/src/python/ct/crypto/asn1/...
File src/python/ct/crypto/asn1/print_util.py (right):

https://codereview.appspot.com/10944043/diff/43001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/print_util.py:15: bitstring = "0"*pad_length +
"".join(map(str, bit_array))
On 2013/07/11 13:29:33, Eran wrote:
> Would it be possible to get to the byte representation without going through
the
> string representation? Simply extend the input array by pad_length zeros and
do
> the same iteration?

I don't know of a more straightforward way to convert a 0-1 int array to a byte,
do you?

However going to chr only to go back to ord in bytes_to_hex is unnecessary, so
I've killed that.

https://codereview.appspot.com/10944043/diff/43001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/print_util.py:51: """Split long lines into multiple
chunks according to the wrap limit.
On 2013/07/11 13:29:33, Eran wrote:
> according to the wrap limit *and* newlines.

Clarified.

https://codereview.appspot.com/10944043/diff/43001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/print_util.py:63: lines = []
On 2013/07/11 13:29:33, Eran wrote:
> nit: you have line and lines, two variables which differ by one character
only.
> Rename one of them to clarify what's input and what's output.

Done.

https://codereview.appspot.com/10944043/diff/43001/src/python/ct/crypto/asn1/...
File src/python/ct/crypto/asn1/types_test.py (right):

https://codereview.appspot.com/10944043/diff/43001/src/python/ct/crypto/asn1/...
src/python/ct/crypto/asn1/types_test.py:52: s =
types.PrintableString(hello).string_value()
On 2013/07/11 13:29:33, Eran wrote:
> You could iterate over the types:
> for t in (types.PrintableString, types.UniversalString, ...):
>   s = t(hello).string_value()
>   self.assertTrue...

Done.