Issue 14441057: Faster ASN.1 parsing
|
Created:
10 years, 6 months ago by ekasper Modified:
10 years, 5 months ago CC:
ctlog-opensource-review_google.com, ngalbreath Visibility:
Public. |
DescriptionFaster ASN.1 parsing
Patch Set 1 #Patch Set 2 : fix init #Patch Set 3 : fix encoding #Patch Set 4 : Fix metaclasses to not erase components #Patch Set 5 : 25% faster decoding #
Total comments: 22
Patch Set 6 : comments #Patch Set 7 : more doc #
Total comments: 22
Patch Set 8 : Ben's comments #Patch Set 9 : strict mode #Patch Set 10 : tighten allowed errors #
Total comments: 31
Patch Set 11 : Add tests, address comments #
Total comments: 2
Patch Set 12 : sync docstring #
MessagesTotal messages: 18
Congratulations Eran, you're the lucky winner and get to review this monster CL! Do not panic though: this is a "rewrite" of pyasn1 so the overall structure should be familiar to you. Obviously "faster" is a marketing term because it's not completely generic ASN.1 parsing. But it can decode certificates almost exactly 10 times faster. (Note: there will be regression as we add more extensions; but it'll also be fairly easy to switch to lazy decoding of extensions if we want to. Sadly one of the most expensive things to decode are names and alternative names, and you almost always want to decode those.) As you can see there are no unit tests yet - I figured you could start with the review while I work on those, and soak test the decoder on the entire log. I've also staged the code temporarily in a new directory, so the switch-over can happen in a separate CL.
Sign in to reply to this message.
(FYI: small update to the patch to fix metaclasses.) On 2013/10/10 18:11:36, ekasper wrote: > Congratulations Eran, you're the lucky winner and get to review this monster CL! > Do not panic though: this is a "rewrite" of pyasn1 so the overall structure > should be familiar to you. > > Obviously "faster" is a marketing term because it's not completely generic ASN.1 > parsing. But it can decode certificates almost exactly 10 times faster. > > (Note: there will be regression as we add more extensions; but it'll also be > fairly easy to switch to lazy decoding of extensions if we want to. Sadly one of > the most expensive things to decode are names and alternative names, and you > almost always want to decode those.) > > As you can see there are no unit tests yet - I figured you could start with the > review while I work on those, and soak test the decoder on the entire log. > > I've also staged the code temporarily in a new directory, so the switch-over can > happen in a separate CL.
Sign in to reply to this message.
And updated again. Any and Choice handling is slightly changed, the rest is just rephrasing. Each class now has a unified _value attribute which is initialized from either a decoded 'value' or a serialized 'serialized_value'. The two methods that control initialization are _decode_value() for decoding a serialized_value and _convert_value() for typechecking/converting the value. This basically gets rid of some function calls and duplicate conversion checks. I think it doesn't look _that much_ uglier, and the decoding time has gone down from 1.44ms to 1.06ms for my test cert. On 2013/10/15 09:20:23, ekasper wrote: > (FYI: small update to the patch to fix metaclasses.) > > On 2013/10/10 18:11:36, ekasper wrote: > > Congratulations Eran, you're the lucky winner and get to review this monster > CL! > > Do not panic though: this is a "rewrite" of pyasn1 so the overall structure > > should be familiar to you. > > > > Obviously "faster" is a marketing term because it's not completely generic > ASN.1 > > parsing. But it can decode certificates almost exactly 10 times faster. > > > > (Note: there will be regression as we add more extensions; but it'll also be > > fairly easy to switch to lazy decoding of extensions if we want to. Sadly one > of > > the most expensive things to decode are names and alternative names, and you > > almost always want to decode those.) > > > > As you can see there are no unit tests yet - I figured you could start with > the > > review while I work on those, and soak test the decoder on the entire log. > > > > I've also staged the code temporarily in a new directory, so the switch-over > can > > happen in a separate CL.
Sign in to reply to this message.
Some preliminary comments. Overall the Python code looks good and I've had minor comments so far. Note that I've only gone through half tags.py (it's huge!). Is there anyone with better ASN.1 understanding to review that aspect of the code? I really can't evaluate how good of a way it is to deal with ASN.1. https://codereview.appspot.com/14441057/diff/15001/src/python/ct/crypto/error.py File src/python/ct/crypto/error.py (right): https://codereview.appspot.com/14441057/diff/15001/src/python/ct/crypto/error... src/python/ct/crypto/error.py:97: # TODO(ekasper): ove ASN1 errors to the ASN1 module, reserving ct.crypto.error move? https://codereview.appspot.com/14441057/diff/15001/src/python/ct/crypto/my_as... File src/python/ct/crypto/my_asn1/types.py (right): https://codereview.appspot.com/14441057/diff/15001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/types.py:1: """ASN.1 types.""" Module-level documentation is missing - point to where ASN.1 is defined? https://codereview.appspot.com/14441057/diff/15001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/types.py:26: a string. Representing what? Length of the output should be mentioned, as it does not seem to be constant. https://codereview.appspot.com/14441057/diff/15001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/types.py:40: if value == -1 and int_bytes[-1] <= 127: Document the reasons behind adding 0xff / 0x00. https://codereview.appspot.com/14441057/diff/15001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/types.py:49: def decode_int(buf, signed=True): Can we get some unit tests for this? https://codereview.appspot.com/14441057/diff/15001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/types.py:84: leading *= 256 shift rather than multiply? https://codereview.appspot.com/14441057/diff/15001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/types.py:114: def read_length(buf): This method also returns a truncated buffer - consider treating it as a stream where the read bytes are "consumed" so the buffer no longer contains them. https://codereview.appspot.com/14441057/diff/15001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/types.py:124: a non-negative integer. Representing what? https://codereview.appspot.com/14441057/diff/15001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/types.py:130: return length, rest The return statement disagree with the documentation. https://codereview.appspot.com/14441057/diff/15001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/types.py:348: raise TypeError("Cannot initialize from None") What if both are not None? Looks like a factory method would be more suitable here (basically seems like you have the option to construct instances of this class from two different implementations). https://codereview.appspot.com/14441057/diff/15001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/types.py:358: pass Is this intentional?
Sign in to reply to this message.
Thanks! "Better ASN.1 understanding" is a mostly mythical beast. But adding benl@ in case he feels like chiming in on the review. Also note that while I wanted to have something sufficiently generic to be reusable for cryptographic structures other than X509v3, this is not "the complete ASN.1" but rather a restricted subset optimized for our application. I've added some comments highlighting the restrictions. https://codereview.appspot.com/14441057/diff/15001/src/python/ct/crypto/error.py File src/python/ct/crypto/error.py (right): https://codereview.appspot.com/14441057/diff/15001/src/python/ct/crypto/error... src/python/ct/crypto/error.py:97: # TODO(ekasper): ove ASN1 errors to the ASN1 module, reserving ct.crypto.error On 2013/10/17 13:07:34, Eran wrote: > move? Done. https://codereview.appspot.com/14441057/diff/15001/src/python/ct/crypto/my_as... File src/python/ct/crypto/my_asn1/types.py (right): https://codereview.appspot.com/14441057/diff/15001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/types.py:1: """ASN.1 types.""" On 2013/10/17 13:07:34, Eran wrote: > Module-level documentation is missing - point to where ASN.1 is defined? Pointed to a human-readable guide instead. https://codereview.appspot.com/14441057/diff/15001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/types.py:26: a string. On 2013/10/17 13:07:34, Eran wrote: > Representing what? Length of the output should be mentioned, as it does not seem > to be constant. Done. https://codereview.appspot.com/14441057/diff/15001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/types.py:40: if value == -1 and int_bytes[-1] <= 127: On 2013/10/17 13:07:34, Eran wrote: > Document the reasons behind adding 0xff / 0x00. Done. https://codereview.appspot.com/14441057/diff/15001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/types.py:49: def decode_int(buf, signed=True): On 2013/10/17 13:07:34, Eran wrote: > Can we get some unit tests for this? Sure, for ALL of this. Will add to the CL for review when they're done (and I don't expect an LGTM before that.) https://codereview.appspot.com/14441057/diff/15001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/types.py:84: leading *= 256 On 2013/10/17 13:07:34, Eran wrote: > shift rather than multiply? Done. https://codereview.appspot.com/14441057/diff/15001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/types.py:114: def read_length(buf): On 2013/10/17 13:07:34, Eran wrote: > This method also returns a truncated buffer - consider treating it as a stream > where the read bytes are "consumed" so the buffer no longer contains them. I did consider a stream but it's tricky: I sometimes have to rewind and retry (for example, when decoding optional components), and the arbitrary recursion makes it hard to just peek. https://codereview.appspot.com/14441057/diff/15001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/types.py:124: a non-negative integer. On 2013/10/17 13:07:34, Eran wrote: > Representing what? Clarified. https://codereview.appspot.com/14441057/diff/15001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/types.py:130: return length, rest On 2013/10/17 13:07:34, Eran wrote: > The return statement disagree with the documentation. Fixed. https://codereview.appspot.com/14441057/diff/15001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/types.py:348: raise TypeError("Cannot initialize from None") On 2013/10/17 13:07:34, Eran wrote: > What if both are not None? Hard to guide against every abuse in python, so I expect common sense from the caller here. Looks like a factory method would be more suitable > here (basically seems like you have the option to construct instances of this > class from two different implementations). Indeed. But python doesn't offer overloading of the constructor and I believe this is a valid replacement design pattern. A factory method would look cleaner, but this way is significantly faster because I can bypass __init__ checks when I already know that I've decoded the correct object. It's a tradeoff and I decided that I prefer speed. https://codereview.appspot.com/14441057/diff/15001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/types.py:358: pass On 2013/10/17 13:07:34, Eran wrote: > Is this intentional? Yes. the abstractmethod decorator already guarantees that you must implement. I raise above because there is no abstractmethod equivalent for class methods.
Sign in to reply to this message.
https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... File src/python/ct/crypto/my_asn1/oid.py (right): https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/oid.py:11: class ObjectIdentifier(types.Abstract): Might be nice to explain the encoding, or at least point to where it is defined. https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/oid.py:79: "an OID component", error.ASN1Warning) Should throw an error? https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/oid.py:112: (value[0], value[1])) Seems to me second component must be <= 175 (== 255 - 80) if first is 2, no? https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... File src/python/ct/crypto/my_asn1/types.py (right): https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/types.py:7: The decoder tolerates BER for a few small exceptions; the encoder is DER-only. Make BER tolerance optional? https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/types.py:91: error.ASN1Warning) Error? https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/types.py:95: "encoding", error.ASN1Warning) Error? https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/types.py:204: MyInteger = Explicit(0, tag.APPLICATION(Integer)) Should be: MyInteger = Explicit(0, tag.APPLICATION)(Integer) ? Or first example is wrong? https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/types.py:661: # int/long? Is this a FIXME? What is "allocate memory only"? https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/types.py:1111: error.ASN1Warning) Why only a warning?
Sign in to reply to this message.
Thanks, Ben! Caught quite a few inconsistencies there. https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... File src/python/ct/crypto/my_asn1/oid.py (right): https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/oid.py:11: class ObjectIdentifier(types.Abstract): On 2013/10/18 12:25:13, Ben Laurie (Google) wrote: > Might be nice to explain the encoding, or at least point to where it is defined. Pointer is in types.py, but I've added it to this file, too. https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/oid.py:79: "an OID component", error.ASN1Warning) On 2013/10/18 12:25:13, Ben Laurie (Google) wrote: > Should throw an error? See error.py Warnings can optionally be converted to exceptions. I don't want to always throw because this would abort parsing. We generally want to parse funky BER certs, and only reject them in a validation context. I suppose the cert class will have a "strict" option in the initializer that will default to true, and we'll flip it to false for the monitor. https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/oid.py:112: (value[0], value[1])) On 2013/10/18 12:25:13, Ben Laurie (Google) wrote: > Seems to me second component must be <= 175 (== 255 - 80) if first is 2, no? Uh, seems so. Fixed. https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... File src/python/ct/crypto/my_asn1/types.py (right): https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/types.py:7: The decoder tolerates BER for a few small exceptions; the encoder is DER-only. On 2013/10/18 12:25:13, Ben Laurie (Google) wrote: > Make BER tolerance optional? It is optional - you can make the warnings throw. https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/types.py:91: error.ASN1Warning) On 2013/10/18 12:25:13, Ben Laurie (Google) wrote: > Error? See above. https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/types.py:95: "encoding", error.ASN1Warning) On 2013/10/18 12:25:13, Ben Laurie (Google) wrote: > Error? See above. https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/types.py:204: MyInteger = Explicit(0, tag.APPLICATION(Integer)) On 2013/10/18 12:25:13, Ben Laurie (Google) wrote: > Should be: > > MyInteger = Explicit(0, tag.APPLICATION)(Integer) > > ? Or first example is wrong? It should. Fixed, thanks. https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/types.py:661: # int/long? On 2013/10/18 12:25:13, Ben Laurie (Google) wrote: > Is this a FIXME? What is "allocate memory only"? bitarray.bitarray(8) would create an 8-bit bitarray with uninitialized contents. I've actually tightened this to hide the bitarray, so the only publicly accepted initializer is either another BitString or a simple string of '0's and '1's. This way we can plug in a pure-python replacement if we wish (bitarray is C-backed). I've also made endianness explicit. https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/types.py:1111: error.ASN1Warning) On 2013/10/18 12:25:13, Ben Laurie (Google) wrote: > Why only a warning? Because we want to parse these structures. The application class (cert.Certificate in our case) can then choose when to throw, e.g. for e in c["tbsCertificate"]["extensions"]: if e["critical"] and not e["extnValue"].decoded: raise CertificateError("Undecoded critical extension")
Sign in to reply to this message.
+cc nickg Nick, I wanted to give you a heads up of the planned changes to ASN.1 parsing. The main motivation here is improved parsing speed, though I've also tried to provide a more consistent API. It'll take a little while to nail this CL but it shouldn't stop you from reporting issues with the current code - I will do the migration work once we're done here. Cheers, Emilia
Sign in to reply to this message.
https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... File src/python/ct/crypto/my_asn1/oid.py (right): https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/oid.py:79: "an OID component", error.ASN1Warning) On 2013/10/18 13:45:24, ekasper wrote: > On 2013/10/18 12:25:13, Ben Laurie (Google) wrote: > > Should throw an error? > > See error.py > > Warnings can optionally be converted to exceptions. I don't want to always throw > because this would abort parsing. We generally want to parse funky BER certs, > and only reject them in a validation context. That's a horrible way to deal with it, though - you'd be converting other warnings to errors, too. I'd suggest a specific mode for relaxed DER parsing, and throw an error if that mode is not set. > > I suppose the cert class will have a "strict" option in the initializer that > will default to true, and we'll flip it to false for the monitor.
Sign in to reply to this message.
https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... File src/python/ct/crypto/my_asn1/oid.py (right): https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/oid.py:79: "an OID component", error.ASN1Warning) On 2013/10/21 10:41:01, Ben Laurie (Google) wrote: > On 2013/10/18 13:45:24, ekasper wrote: > > On 2013/10/18 12:25:13, Ben Laurie (Google) wrote: > > > Should throw an error? > > > > See error.py > > > > Warnings can optionally be converted to exceptions. I don't want to always > throw > > because this would abort parsing. We generally want to parse funky BER certs, > > and only reject them in a validation context. > > That's a horrible way to deal with it, though - you'd be converting other > warnings to errors, too. > > I'd suggest a specific mode for relaxed DER parsing, and throw an error if that > mode is not set. You can selectively change the behaviour of each warning class individually, and even do that within a specific context, so it didn't seem like a terrible way to handle this. But reading the fine print has made me realize that the context manager is not thread safe, so I've nuked it before it bites me back. The recursive defaulted 'strict' argument is a little brittle to implement but I think forcing the caller to explicitly flip the switch is important, i.e., c.decode(buf) # normal usage c.decode(buf, strict=False) # explicitly disable is the safer API. Additionally, I've read the BER spec and realized that non-minimal integer encodings are not allowed in BER either, so I've made those unconditional errors, at least until I actually see one in practice. The only exceptions currently tolerated by strict=False are BER Boolean, and corrupt ANY. I also plan to treat non-conforming time formats, multiple extensions, etc similarly when I migrate the cert semantics. My hope is that we can parse all the weirdness in the world, eradicate it, and then remove the non-strict option :) > > > > > I suppose the cert class will have a "strict" option in the initializer that > > will default to true, and we'll flip it to false for the monitor. >
Sign in to reply to this message.
Also, PTAL now! On Tue, Oct 22, 2013 at 6:02 PM, <ekasper@google.com> wrote: > > https://codereview.appspot.**com/14441057/diff/30001/src/** > python/ct/crypto/my_asn1/oid.**py<https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_asn1/oid.py> > File src/python/ct/crypto/my_asn1/**oid.py (right): > > https://codereview.appspot.**com/14441057/diff/30001/src/** > python/ct/crypto/my_asn1/oid.**py#newcode79<https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_asn1/oid.py#newcode79> > src/python/ct/crypto/my_asn1/**oid.py:79: "an OID component", > error.ASN1Warning) > On 2013/10/21 10:41:01, Ben Laurie (Google) wrote: > >> On 2013/10/18 13:45:24, ekasper wrote: >> > On 2013/10/18 12:25:13, Ben Laurie (Google) wrote: >> > > Should throw an error? >> > >> > See error.py >> > >> > Warnings can optionally be converted to exceptions. I don't want to >> > always > >> throw >> > because this would abort parsing. We generally want to parse funky >> > BER certs, > >> > and only reject them in a validation context. >> > > That's a horrible way to deal with it, though - you'd be converting >> > other > >> warnings to errors, too. >> > > I'd suggest a specific mode for relaxed DER parsing, and throw an >> > error if that > >> mode is not set. >> > > You can selectively change the behaviour of each warning class > individually, and even do that within a specific context, so it didn't > seem like a terrible way to handle this. But reading the fine print has > made me realize that the context manager is not thread safe, so I've > nuked it before it bites me back. > > The recursive defaulted 'strict' argument is a little brittle to > implement but I think forcing the caller to explicitly flip the switch > is important, i.e., > > c.decode(buf) # normal usage > c.decode(buf, strict=False) # explicitly disable > > is the safer API. > > Additionally, I've read the BER spec and realized that non-minimal > integer encodings are not allowed in BER either, so I've made those > unconditional errors, at least until I actually see one in practice. The > only exceptions currently tolerated by strict=False are BER Boolean, and > corrupt ANY. I also plan to treat non-conforming time formats, multiple > extensions, etc similarly when I migrate the cert semantics. > > My hope is that we can parse all the weirdness in the world, eradicate > it, and then remove the non-strict option :) > > > > > >> > I suppose the cert class will have a "strict" option in the >> > initializer that > >> > will default to true, and we'll flip it to false for the monitor. >> > > > https://codereview.appspot.**com/14441057/<https://codereview.appspot.com/144... >
Sign in to reply to this message.
Some more comments. Overall the code looks good, I'm waiting for some tests that will prove some things which look suspicious are working :) (Also, I'll get to the bottom of types.py some day..) https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... File src/python/ct/crypto/my_asn1/oid.py (right): https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... src/python/ct/crypto/my_asn1/oid.py:12: @types.Universal(6, tag.PRIMITIVE) Are those values (i.e. 6) constants that are documented somewhere? https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... src/python/ct/crypto/my_asn1/oid.py:31: def short_name(self): Nit: short_name and long_name share the same implementation with a 1-bit difference. Delegate to a private, common method? https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... src/python/ct/crypto/my_asn1/oid.py:34: return _OID_NAME_DICT[self][1] looking up 'self' in a hash is rather unorthodox, but I can't find a concrete reason why to avoid it (other than being *very* careful in your __eq__ and __hash__ methods not to refer to any of the properties, which you are). What's the reason behind not storing short & long names in the object itself? https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... src/python/ct/crypto/my_asn1/oid.py:58: int_bytes.append(value % 128) Nit: I personally find binary operators (rather than arithmetic ones) more clear for such operations. Furthermore, I'm not sure a negative value here will be treated correctly. https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... src/python/ct/crypto/my_asn1/oid.py:68: """Parse a single component from a non-empty bytearray.""" I assume this is ASN.1 BER ? https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... src/python/ct/crypto/my_asn1/oid.py:91: int_bytes.append(self._value[0]*40 + self._value[1]) Nit: Explain why the first two bytes are encoded that way? Also, can 40 be made constant? (42 would have been obvious, 40, less so). https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... src/python/ct/crypto/my_asn1/oid.py:93: int_bytes += self._encode_component(self._value[i]) for v in self._value[2:]: int_bytes += self._encode_component(v) (in other words, I don't see the benefit of iterating over the range rather than the values themselves). https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... src/python/ct/crypto/my_asn1/oid.py:105: value = [int(v) for v in value] value = map(int, value). you could do it while splitting the value (3 lines above) https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... src/python/ct/crypto/my_asn1/oid.py:106: if not all([v >= 0 for v in value]): Nice! I did not know about the all function. https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... src/python/ct/crypto/my_asn1/oid.py:121: def _decode_value(cls, buf, strict=True): You have, in this class, the methods: _encode_component _parse_component encode_value _convert_value _decode_value None of the names hint at the source/destination format, so the only way for me to see if they are even correct is to hunt down callers and see the context in which they're used. What I suggest: * Consistent naming (encode/decode rather than encode/parse) * be a bit more verbose in the method name (e.g. encode_ber, convert_from_xxx_to_yyy). * Document the expected input and output formats. https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... File src/python/ct/crypto/my_asn1/tag.py (right): https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... src/python/ct/crypto/my_asn1/tag.py:14: IMPLICIT, EXPLICIT = range(2) Nice definition! Thanks for taking the time to define constants even for these trivial values. https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... src/python/ct/crypto/my_asn1/tag.py:17: class Tag(object): The documentation and constants in this class really help understanding how it's supposed to be used. https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... src/python/ct/crypto/my_asn1/tag.py:37: self.encoding = encoding Nit: verify that encoding is indeed either PRIMITIVE or CONSTRUCTED. https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... src/python/ct/crypto/my_asn1/tag.py:82: buf: a string or string buffer. Is said buffer encoded in any standard way? https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... File src/python/ct/crypto/my_asn1/types.py (right): https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... src/python/ct/crypto/my_asn1/types.py:223: tag_class: the tag class. One of tag.CONTEXT_SPECIFIC, Check this in the code? https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... src/python/ct/crypto/my_asn1/types.py:416: encoded_value = t.value + encoded_length + encoded_value Any reason you re-compute encode_length every time? Seems like the only thing changing in this loop is t.value. https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... src/python/ct/crypto/my_asn1/types.py:456: # logging.debug("%s: decoded value %s", cls.__name__, value) leftover debug statements?
Sign in to reply to this message.
Now with tests :) Other changes since last patch set: -- unify __eq__ across all types to delegate to value comparison. -- fix OID encoding bug revealed by tests. -- remove bitarray dependency: pure-python adds maybe ~5% to total cert decoding time, and I think getting rid of a non-standard external dependency is worth it. -- change the Any value type to always return the encoded value, and add a separate .decoded_value property. -- acknowledge that SET OF decoding is not strict DER, add a TODO. https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... File src/python/ct/crypto/my_asn1/oid.py (right): https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... src/python/ct/crypto/my_asn1/oid.py:12: @types.Universal(6, tag.PRIMITIVE) On 2013/10/25 11:08:45, Eran wrote: > Are those values (i.e. 6) constants that are documented somewhere? Yes, in the spec. I added a reference to the spec to the top. Didn't seem worth the effort to define constants here as they're only used once to define type. https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... src/python/ct/crypto/my_asn1/oid.py:31: def short_name(self): On 2013/10/25 11:08:45, Eran wrote: > Nit: short_name and long_name share the same implementation with a 1-bit > difference. Delegate to a private, common method? Done. https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... src/python/ct/crypto/my_asn1/oid.py:34: return _OID_NAME_DICT[self][1] On 2013/10/25 11:08:45, Eran wrote: > looking up 'self' in a hash is rather unorthodox, but I can't find a concrete > reason why to avoid it (other than being *very* careful in your __eq__ and > __hash__ methods not to refer to any of the properties, which you are). > > What's the reason behind not storing short & long names in the object itself? The names are not sent over the wire so you have to look them up from some local dictionary. I could construct the dictionary explicitly by the underlying value but since the type is immutable and just delegates eq and hash to its value, I think it's safe enough. https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... src/python/ct/crypto/my_asn1/oid.py:58: int_bytes.append(value % 128) On 2013/10/25 11:08:45, Eran wrote: > Nit: I personally find binary operators (rather than arithmetic ones) more clear > for such operations. Furthermore, I'm not sure a negative value here will be > treated correctly. No strong preference, so done. Negative values never make it past __init__. There are tests for this now :) https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... src/python/ct/crypto/my_asn1/oid.py:68: """Parse a single component from a non-empty bytearray.""" On 2013/10/25 11:08:45, Eran wrote: > I assume this is ASN.1 BER ? I'm not sure what you mean? It's ASN.1 DER. https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... src/python/ct/crypto/my_asn1/oid.py:91: int_bytes.append(self._value[0]*40 + self._value[1]) On 2013/10/25 11:08:45, Eran wrote: > Nit: Explain why the first two bytes are encoded that way? ASN.1 spec. (Though there was a bug that I've fixed and added a test for.) > Also, can 40 be made constant? (42 would have been obvious, 40, less so). It's not really a meaningful constant - I've just added a comment explaining where it comes from. 42 would have been nicer, I agree. https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... src/python/ct/crypto/my_asn1/oid.py:93: int_bytes += self._encode_component(self._value[i]) On 2013/10/25 11:08:45, Eran wrote: > for v in self._value[2:]: > int_bytes += self._encode_component(v) > > (in other words, I don't see the benefit of iterating over the range rather than > the values themselves). Done. https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... src/python/ct/crypto/my_asn1/oid.py:105: value = [int(v) for v in value] On 2013/10/25 11:08:45, Eran wrote: > value = map(int, value). you could do it while splitting the value (3 lines > above) Moved up. But style guide prefers list comprehension over map. https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... src/python/ct/crypto/my_asn1/oid.py:121: def _decode_value(cls, buf, strict=True): On 2013/10/25 11:08:45, Eran wrote: > You have, in this class, the methods: > _encode_component > _parse_component > encode_value > _convert_value > _decode_value > > None of the names hint at the source/destination format, so the only way for me > to see if they are even correct is to hunt down callers and see the context in > which they're used. > > What I suggest: > * Consistent naming (encode/decode rather than encode/parse) I renamed all "parse" methods to "read" at some point but forgot to update this one. Done now. The naming should be consistent throughout: "decode" decodes an entire buffer, while "read" reads from the beginning. > * be a bit more verbose in the method name (e.g. encode_ber, > convert_from_xxx_to_yyy). Everything is DER. > * Document the expected input and output formats. Done for _encode_component and _read_component. The rest are documented in the abstract base class: each derived class just overrides for their value type. https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... File src/python/ct/crypto/my_asn1/tag.py (right): https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... src/python/ct/crypto/my_asn1/tag.py:37: self.encoding = encoding On 2013/10/25 11:08:45, Eran wrote: > Nit: verify that encoding is indeed either PRIMITIVE or CONSTRUCTED. The idea is that users should never specify the encoding directly - the metaclasses in types.py automatically compute the correct encoding. But done, anyway. https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... src/python/ct/crypto/my_asn1/tag.py:82: buf: a string or string buffer. On 2013/10/25 11:08:45, Eran wrote: > Is said buffer encoded in any standard way? Raw ASN.1 bytes. Clarified. https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... File src/python/ct/crypto/my_asn1/types.py (right): https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... src/python/ct/crypto/my_asn1/types.py:223: tag_class: the tag class. One of tag.CONTEXT_SPECIFIC, On 2013/10/25 11:08:45, Eran wrote: > Check this in the code? It's now checked when constructing the tag. https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... src/python/ct/crypto/my_asn1/types.py:416: encoded_value = t.value + encoded_length + encoded_value On 2013/10/25 11:08:45, Eran wrote: > Any reason you re-compute encode_length every time? Seems like the only thing > changing in this loop is t.value. encoded_value changes recursively. ASN.1 is nuts. https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... src/python/ct/crypto/my_asn1/types.py:456: # logging.debug("%s: decoded value %s", cls.__name__, value) On 2013/10/25 11:08:45, Eran wrote: > leftover debug statements? I actually left them in intentionally with a remark that they're expensive (see above), so that nobody's tempted to sneak them in again later.
Sign in to reply to this message.
Gentle poke... On 2013/10/30 18:25:12, ekasper wrote: > Now with tests :) > > Other changes since last patch set: > -- unify __eq__ across all types to delegate to value comparison. > -- fix OID encoding bug revealed by tests. > -- remove bitarray dependency: pure-python adds maybe ~5% to total cert decoding > time, and I think getting rid of a non-standard external dependency is worth it. > -- change the Any value type to always return the encoded value, and add a > separate .decoded_value property. > -- acknowledge that SET OF decoding is not strict DER, add a TODO. > > https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... > File src/python/ct/crypto/my_asn1/oid.py (right): > > https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... > src/python/ct/crypto/my_asn1/oid.py:12: @types.Universal(6, tag.PRIMITIVE) > On 2013/10/25 11:08:45, Eran wrote: > > Are those values (i.e. 6) constants that are documented somewhere? > > Yes, in the spec. I added a reference to the spec to the top. > Didn't seem worth the effort to define constants here as they're only used once > to define type. > > https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... > src/python/ct/crypto/my_asn1/oid.py:31: def short_name(self): > On 2013/10/25 11:08:45, Eran wrote: > > Nit: short_name and long_name share the same implementation with a 1-bit > > difference. Delegate to a private, common method? > > Done. > > https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... > src/python/ct/crypto/my_asn1/oid.py:34: return _OID_NAME_DICT[self][1] > On 2013/10/25 11:08:45, Eran wrote: > > looking up 'self' in a hash is rather unorthodox, but I can't find a concrete > > reason why to avoid it (other than being *very* careful in your __eq__ and > > __hash__ methods not to refer to any of the properties, which you are). > > > > What's the reason behind not storing short & long names in the object itself? > > The names are not sent over the wire so you have to look them up from some local > dictionary. > > I could construct the dictionary explicitly by the underlying value but since > the type is immutable and just delegates eq and hash to its value, I think it's > safe enough. > > https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... > src/python/ct/crypto/my_asn1/oid.py:58: int_bytes.append(value % 128) > On 2013/10/25 11:08:45, Eran wrote: > > Nit: I personally find binary operators (rather than arithmetic ones) more > clear > > for such operations. Furthermore, I'm not sure a negative value here will be > > treated correctly. > > No strong preference, so done. > > Negative values never make it past __init__. There are tests for this now :) > > https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... > src/python/ct/crypto/my_asn1/oid.py:68: """Parse a single component from a > non-empty bytearray.""" > On 2013/10/25 11:08:45, Eran wrote: > > I assume this is ASN.1 BER ? > > I'm not sure what you mean? It's ASN.1 DER. > > https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... > src/python/ct/crypto/my_asn1/oid.py:91: int_bytes.append(self._value[0]*40 + > self._value[1]) > On 2013/10/25 11:08:45, Eran wrote: > > Nit: Explain why the first two bytes are encoded that way? > > ASN.1 spec. (Though there was a bug that I've fixed and added a test for.) > > > Also, can 40 be made constant? (42 would have been obvious, 40, less so). > > It's not really a meaningful constant - I've just added a comment explaining > where it comes from. 42 would have been nicer, I agree. > > https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... > src/python/ct/crypto/my_asn1/oid.py:93: int_bytes += > self._encode_component(self._value[i]) > On 2013/10/25 11:08:45, Eran wrote: > > for v in self._value[2:]: > > int_bytes += self._encode_component(v) > > > > (in other words, I don't see the benefit of iterating over the range rather > than > > the values themselves). > > Done. > > https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... > src/python/ct/crypto/my_asn1/oid.py:105: value = [int(v) for v in value] > On 2013/10/25 11:08:45, Eran wrote: > > value = map(int, value). you could do it while splitting the value (3 lines > > above) > > Moved up. But style guide prefers list comprehension over map. > > https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... > src/python/ct/crypto/my_asn1/oid.py:121: def _decode_value(cls, buf, > strict=True): > On 2013/10/25 11:08:45, Eran wrote: > > You have, in this class, the methods: > > _encode_component > > _parse_component > > encode_value > > _convert_value > > _decode_value > > > > None of the names hint at the source/destination format, so the only way for > me > > to see if they are even correct is to hunt down callers and see the context in > > which they're used. > > > > What I suggest: > > * Consistent naming (encode/decode rather than encode/parse) > > I renamed all "parse" methods to "read" at some point but forgot to update this > one. Done now. > > The naming should be consistent throughout: "decode" decodes an entire buffer, > while "read" reads from the beginning. > > > > * be a bit more verbose in the method name (e.g. encode_ber, > > convert_from_xxx_to_yyy). > > Everything is DER. > > > * Document the expected input and output formats. > > Done for _encode_component and _read_component. > > The rest are documented in the abstract base class: each derived class just > overrides for their value type. > > https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... > File src/python/ct/crypto/my_asn1/tag.py (right): > > https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... > src/python/ct/crypto/my_asn1/tag.py:37: self.encoding = encoding > On 2013/10/25 11:08:45, Eran wrote: > > Nit: verify that encoding is indeed either PRIMITIVE or CONSTRUCTED. > > The idea is that users should never specify the encoding directly - the > metaclasses in types.py automatically compute the correct encoding. > > But done, anyway. > > https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... > src/python/ct/crypto/my_asn1/tag.py:82: buf: a string or string buffer. > On 2013/10/25 11:08:45, Eran wrote: > > Is said buffer encoded in any standard way? > > Raw ASN.1 bytes. Clarified. > > https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... > File src/python/ct/crypto/my_asn1/types.py (right): > > https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... > src/python/ct/crypto/my_asn1/types.py:223: tag_class: the tag class. One of > tag.CONTEXT_SPECIFIC, > On 2013/10/25 11:08:45, Eran wrote: > > Check this in the code? > > It's now checked when constructing the tag. > > https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... > src/python/ct/crypto/my_asn1/types.py:416: encoded_value = t.value + > encoded_length + encoded_value > On 2013/10/25 11:08:45, Eran wrote: > > Any reason you re-compute encode_length every time? Seems like the only thing > > changing in this loop is t.value. > > encoded_value changes recursively. ASN.1 is nuts. > > https://codereview.appspot.com/14441057/diff/253001/src/python/ct/crypto/my_a... > src/python/ct/crypto/my_asn1/types.py:456: # logging.debug("%s: decoded value > %s", cls.__name__, value) > On 2013/10/25 11:08:45, Eran wrote: > > leftover debug statements? > > I actually left them in intentionally with a remark that they're expensive (see > above), so that nobody's tempted to sneak them in again later.
Sign in to reply to this message.
drive-by LGTM, modulo minor comments... https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... File src/python/ct/crypto/my_asn1/oid.py (right): https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/oid.py:112: (value[0], value[1])) On 2013/10/18 13:45:24, ekasper wrote: > On 2013/10/18 12:25:13, Ben Laurie (Google) wrote: > > Seems to me second component must be <= 175 (== 255 - 80) if first is 2, no? > > Uh, seems so. Fixed. I don't see a fix? https://codereview.appspot.com/14441057/diff/283001/src/python/ct/crypto/my_a... File src/python/ct/crypto/my_asn1/types.py (right): https://codereview.appspot.com/14441057/diff/283001/src/python/ct/crypto/my_a... src/python/ct/crypto/my_asn1/types.py:76: strict: if False, tolerate encoding with a non-minimal number of octets. Maybe "tolerate encodings seen in the wild"?
Sign in to reply to this message.
Is a drive-by LGTM good enough, or should I wait for Eran to chime in? :) https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... File src/python/ct/crypto/my_asn1/oid.py (right): https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... src/python/ct/crypto/my_asn1/oid.py:112: (value[0], value[1])) On 2013/11/06 12:23:15, Ben Laurie (Google) wrote: > On 2013/10/18 13:45:24, ekasper wrote: > > On 2013/10/18 12:25:13, Ben Laurie (Google) wrote: > > > Seems to me second component must be <= 175 (== 255 - 80) if first is 2, no? > > > > Uh, seems so. Fixed. > > I don't see a fix? > This comment applies to an earlier patch-set that had the fix. The fix was wrong: it's not, in fact, true that the second component must be <= 175. The correct fix is to encode 40*first + second as a proper component, which I've done now. Sorry for the confusion, I think your comments have crossed with Eran's here. https://codereview.appspot.com/14441057/diff/283001/src/python/ct/crypto/my_a... File src/python/ct/crypto/my_asn1/types.py (right): https://codereview.appspot.com/14441057/diff/283001/src/python/ct/crypto/my_a... src/python/ct/crypto/my_asn1/types.py:76: strict: if False, tolerate encoding with a non-minimal number of octets. On 2013/11/06 12:23:15, Ben Laurie (Google) wrote: > Maybe "tolerate encodings seen in the wild"? Stale docstring, fixed. I think this change got lost in the long thread of comments: I was wrong to assume non-minimal encodings were valid BER, and I haven't seen it in the wild so far, so I've nuked it.
Sign in to reply to this message.
On 6 November 2013 12:49, <ekasper@google.com> wrote: > Is a drive-by LGTM good enough, or should I wait for Eran to chime in? Good enough for me - I did review all changes since my last comments :-) > :) > > > > https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... > File src/python/ct/crypto/my_asn1/oid.py (right): > > https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... > src/python/ct/crypto/my_asn1/oid.py:112: (value[0], value[1])) > On 2013/11/06 12:23:15, Ben Laurie (Google) wrote: >> >> On 2013/10/18 13:45:24, ekasper wrote: >> > On 2013/10/18 12:25:13, Ben Laurie (Google) wrote: >> > > Seems to me second component must be <= 175 (== 255 - 80) if first > > is 2, no? >> >> > >> > Uh, seems so. Fixed. > > >> I don't see a fix? > > > > This comment applies to an earlier patch-set that had the fix. The fix > was wrong: it's not, in fact, true that the second component must be <= > 175. The correct fix is to encode 40*first + second as a proper > component, which I've done now. Aha. > Sorry for the confusion, I think your comments have crossed with Eran's > here. > > > https://codereview.appspot.com/14441057/diff/283001/src/python/ct/crypto/my_a... > File src/python/ct/crypto/my_asn1/types.py (right): > > https://codereview.appspot.com/14441057/diff/283001/src/python/ct/crypto/my_a... > src/python/ct/crypto/my_asn1/types.py:76: strict: if False, tolerate > encoding with a non-minimal number of octets. > On 2013/11/06 12:23:15, Ben Laurie (Google) wrote: >> >> Maybe "tolerate encodings seen in the wild"? > > > Stale docstring, fixed. I think this change got lost in the long thread > of comments: I was wrong to assume non-minimal encodings were valid BER, > and I haven't seen it in the wild so far, so I've nuked it. > > https://codereview.appspot.com/14441057/
Sign in to reply to this message.
Ok, I'll submit this to carry on with life and save Eran's time :) Eran - if there's something you definitely wanted to add then I'm of course happy to take comments and address in a follow-up CL. On 2013/11/06 13:10:37, Ben Laurie (Google) wrote: > On 6 November 2013 12:49, <mailto:ekasper@google.com> wrote: > > Is a drive-by LGTM good enough, or should I wait for Eran to chime in? > > Good enough for me - I did review all changes since my last comments :-) > > > :) > > > > > > > > > https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... > > File src/python/ct/crypto/my_asn1/oid.py (right): > > > > > https://codereview.appspot.com/14441057/diff/30001/src/python/ct/crypto/my_as... > > src/python/ct/crypto/my_asn1/oid.py:112: (value[0], value[1])) > > On 2013/11/06 12:23:15, Ben Laurie (Google) wrote: > >> > >> On 2013/10/18 13:45:24, ekasper wrote: > >> > On 2013/10/18 12:25:13, Ben Laurie (Google) wrote: > >> > > Seems to me second component must be <= 175 (== 255 - 80) if first > > > > is 2, no? > >> > >> > > >> > Uh, seems so. Fixed. > > > > > >> I don't see a fix? > > > > > > > > This comment applies to an earlier patch-set that had the fix. The fix > > was wrong: it's not, in fact, true that the second component must be <= > > 175. The correct fix is to encode 40*first + second as a proper > > component, which I've done now. > > Aha. > > > Sorry for the confusion, I think your comments have crossed with Eran's > > here. > > > > > > > https://codereview.appspot.com/14441057/diff/283001/src/python/ct/crypto/my_a... > > File src/python/ct/crypto/my_asn1/types.py (right): > > > > > https://codereview.appspot.com/14441057/diff/283001/src/python/ct/crypto/my_a... > > src/python/ct/crypto/my_asn1/types.py:76: strict: if False, tolerate > > encoding with a non-minimal number of octets. > > On 2013/11/06 12:23:15, Ben Laurie (Google) wrote: > >> > >> Maybe "tolerate encodings seen in the wild"? > > > > > > Stale docstring, fixed. I think this change got lost in the long thread > > of comments: I was wrong to assume non-minimal encodings were valid BER, > > and I haven't seen it in the wild so far, so I've nuked it. > > > > https://codereview.appspot.com/14441057/
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
