Issue 10345043: Add a PEM module
|
Created:
10 years, 10 months ago by ekasper Modified:
10 years, 9 months ago CC:
ctlog-opensource-review_google.com Visibility:
Public. |
DescriptionAdd a PEM module
Patch Set 1 #Patch Set 2 : format #Patch Set 3 : upload the right patch #Patch Set 4 : add tests to Makefile #Patch Set 5 : use hasattr #Patch Set 6 : make it run #
Total comments: 19
Patch Set 7 : comments #Patch Set 8 : write methods #
Total comments: 23
Patch Set 9 : improve tests #Patch Set 10 : make marker constant #
MessagesTotal messages: 9
PEM is essentially base64-encoded binary data enclosed between -----BEGIN TYPE---- and -----END TYPE----- lines. This CL adds utility methods for reading and writing PEM blobs. The module itself is unaware of blob types, and is meant as a building block for parsing certs/keys/etc - there are a couple of places where you can see it in action already. Note: I am a Python n00b so nitpicking is very much appreciated. (The scripts in src/client might also benefit from this, but since there are no tests for them, I didn't feel like touching and breaking them... )
Sign in to reply to this message.
Just FYI, in case you already started looking - my setup was a bit bust with the migration to git so I've uploaded a newer patch that now really runs and passes tests :) On 2013/06/17 17:10:12, ekasper wrote: > PEM is essentially base64-encoded binary data enclosed between -----BEGIN > TYPE---- and -----END TYPE----- lines. > > This CL adds utility methods for reading and writing PEM blobs. The module > itself is unaware of blob types, and is meant as a building block for parsing > certs/keys/etc - there are a couple of places where you can see it in action > already. > > Note: I am a Python n00b so nitpicking is very much appreciated. > > (The scripts in src/client might also benefit from this, but since there are no > tests for them, I didn't feel like touching and breaking them... )
Sign in to reply to this message.
https://codereview.appspot.com/10345043/diff/11001/src/python/ct/crypto/pem.py File src/python/ct/crypto/pem.py (right): https://codereview.appspot.com/10345043/diff/11001/src/python/ct/crypto/pem.p... src/python/ct/crypto/pem.py:15: def __init__(self, fileobj, markers, strict=False): nit: change 'strict' to 'skip_invalid_blobs' (that'd save reading the documentation to figure out what the definition of 'strict' is in this context). https://codereview.appspot.com/10345043/diff/11001/src/python/ct/crypto/pem.p... src/python/ct/crypto/pem.py:25: if isinstance(markers, str): Somewhat surprising that you accept both string and list - I'd suggest only accepting lists. It may not be an anti-pattern, but off the top of my head, I can't think of any library that allows two types of arguments to the same function. https://codereview.appspot.com/10345043/diff/11001/src/python/ct/crypto/pem.p... src/python/ct/crypto/pem.py:37: def close(self): consider using context manager: http://www.python.org/dev/peps/pep-0343/ https://codereview.appspot.com/10345043/diff/11001/src/python/ct/crypto/pem.p... src/python/ct/crypto/pem.py:56: return cls(f, markers, strict) inline the call to open(pem_file). Also, use an explicit read-only open mode for the file. (2nd parameter to open()). https://codereview.appspot.com/10345043/diff/11001/src/python/ct/crypto/pem.p... src/python/ct/crypto/pem.py:96: if self.__strict and not self.__valid_blobs_read: __valid_blobs_read might as well be a boolean, if you don't care about the exact value. https://codereview.appspot.com/10345043/diff/11001/src/python/ct/crypto/pem.p... src/python/ct/crypto/pem.py:131: return self.next() The recursive call here is a bit uncommon in iterators. The pseudo-code for the common implementation of an iterator would be: while self.HasMoreBlocks(): block = self.NextBlock(): if IsValid(block): yield block elif self.__strict: raise PemError(...) https://codereview.appspot.com/10345043/diff/11001/src/python/ct/crypto/pem.p... src/python/ct/crypto/pem.py:262: with closing(PemReader.from_file(pem_file, markers, strict=True)) as reader: If you make the PemReader a context manager you wouldn't need to use the 'closing' bit. https://codereview.appspot.com/10345043/diff/11001/src/python/ct/crypto/pem.p... src/python/ct/crypto/pem.py:280: for ret in reader: Simply return the reader? If the PemReader's __iter__ method yields a new (parsed) block every time, then this iteration is superfluous. https://codereview.appspot.com/10345043/diff/11001/src/python/ct/crypto/pem.p... src/python/ct/crypto/pem.py:284: with closing(PemReader.from_file(pem_file, markers, ditto for context managers. https://codereview.appspot.com/10345043/diff/11001/src/python/ct/crypto/verif... File src/python/ct/crypto/verify.py (right): https://codereview.appspot.com/10345043/diff/11001/src/python/ct/crypto/verif... src/python/ct/crypto/verify.py:29: "ECDSA PUBLIC KEY")) Are these values ("ECDSA PUBLIC KEY") constant in the PEM data we read? If so, consider creating constants for them in the pem module.
Sign in to reply to this message.
Thanks! As we discussed, I've also simplified the newline handling to try to be helpful, but not go over the top. https://codereview.appspot.com/10345043/diff/11001/src/python/ct/crypto/pem.py File src/python/ct/crypto/pem.py (right): https://codereview.appspot.com/10345043/diff/11001/src/python/ct/crypto/pem.p... src/python/ct/crypto/pem.py:15: def __init__(self, fileobj, markers, strict=False): On 2013/06/18 13:49:49, Eran wrote: > nit: change 'strict' to 'skip_invalid_blobs' (that'd save reading the > documentation to figure out what the definition of 'strict' is in this > context). Done. https://codereview.appspot.com/10345043/diff/11001/src/python/ct/crypto/pem.p... src/python/ct/crypto/pem.py:25: if isinstance(markers, str): On 2013/06/18 13:49:49, Eran wrote: > Somewhat surprising that you accept both string and list - I'd suggest only > accepting lists. It may not be an anti-pattern, but off the top of my head, I > can't think of any library that allows two types of arguments to the same > function. It seemed to me that PemReader.from_file(cert.pem, "CERTIFICATE") would be a natural usage pattern to support. Also, note that if we don't check the type, PemReader(fileobj, "CERTIFICATE") will construct a reader with 'C', 'E', 'R', 'T', etc as the markers, which is probably not at all what the caller wants. But I've removed it anyway, assuming the callers are mostly limited to higher-level crypto classes that I'll write myself, anyway :) Following this suggestion, I've also separated the writing methods into two, one for single blobs and another for iterables. https://codereview.appspot.com/10345043/diff/11001/src/python/ct/crypto/pem.p... src/python/ct/crypto/pem.py:37: def close(self): On 2013/06/18 13:49:49, Eran wrote: > consider using context manager: > http://www.python.org/dev/peps/pep-0343/ I was hoping to just get away with "closing" :) Done. https://codereview.appspot.com/10345043/diff/11001/src/python/ct/crypto/pem.p... src/python/ct/crypto/pem.py:56: return cls(f, markers, strict) On 2013/06/18 13:49:49, Eran wrote: > inline the call to open(pem_file). Also, use an explicit read-only open mode for > the file. (2nd parameter to open()). Done. https://codereview.appspot.com/10345043/diff/11001/src/python/ct/crypto/pem.p... src/python/ct/crypto/pem.py:96: if self.__strict and not self.__valid_blobs_read: On 2013/06/18 13:49:49, Eran wrote: > __valid_blobs_read might as well be a boolean, if you don't care about the exact > value. I figured the exact value might come in handy later, so I've left it as is. https://codereview.appspot.com/10345043/diff/11001/src/python/ct/crypto/pem.p... src/python/ct/crypto/pem.py:131: return self.next() On 2013/06/18 13:49:49, Eran wrote: > The recursive call here is a bit uncommon in iterators. > The pseudo-code for the common implementation of an iterator would be: > while self.HasMoreBlocks(): > block = self.NextBlock(): > if IsValid(block): > yield block > elif self.__strict: > raise PemError(...) Oh yeah, this is me being brain dead - done. https://codereview.appspot.com/10345043/diff/11001/src/python/ct/crypto/pem.p... src/python/ct/crypto/pem.py:262: with closing(PemReader.from_file(pem_file, markers, strict=True)) as reader: On 2013/06/18 13:49:49, Eran wrote: > If you make the PemReader a context manager you wouldn't need to use the > 'closing' bit. Done. https://codereview.appspot.com/10345043/diff/11001/src/python/ct/crypto/pem.p... src/python/ct/crypto/pem.py:280: for ret in reader: On 2013/06/18 13:49:49, Eran wrote: > Simply return the reader? If the PemReader's __iter__ method yields a new > (parsed) block every time, then this iteration is superfluous. But then I can't take care of closing here, can I? These are module utility methods for simplest, most common usage, so I'd like it to be completely transparent to the caller that something is opened or closed. (Well, technically I don't have to explicitly close the StringIO - I think - but seems better to have both the string and file methods behave identically.) https://codereview.appspot.com/10345043/diff/11001/src/python/ct/crypto/pem.p... src/python/ct/crypto/pem.py:284: with closing(PemReader.from_file(pem_file, markers, On 2013/06/18 13:49:49, Eran wrote: > ditto for context managers. Done.
Sign in to reply to this message.
Very through tests, thanks for adding them. Just some style and Python comments. https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... File src/python/ct/crypto/pem_test.py (right): https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... src/python/ct/crypto/pem_test.py:31: for name in self.__tempfiles: Python tip: You could use the map function this way: map(os.remove, self.__tempfiles) But using map makes more sense when the results are significant, so a for loop in this case is equally good (maybe even better). https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... src/python/ct/crypto/pem_test.py:39: self.assertIsInstance(reader, pem.PemReader) This assertion is a bit superfluous - if the reader provides the blobs in an iterable way, does it matter what kind of instance it is? https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... src/python/ct/crypto/pem_test.py:46: reader.close() Close the reader before the assertions, so it's closed regardless of the test results. https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... src/python/ct/crypto/pem_test.py:49: reader = pem.PemReader.from_string(PemTest.pem_blob, [PemTest.marker]) Since the list of markers passed into 'from_string' should not be modified, use a tuple rather than a list: reader = pem.PemReader.from_string(PemTest.pem_blob, (PemTest.marker,)) https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... src/python/ct/crypto/pem_test.py:63: results = [r for r in reader] Since you're asserting for the same values in all test cases, I suggest a private method in the PemReaderTest class for that. https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... src/python/ct/crypto/pem_test.py:90: pem2 = """-----BEGIN CHUNK-----\nd29ybGQ=\n-----END CHUNK-----\n""" nit: Rather than hard-coding the base64 value, use base64.encodestring so it's clear what this value is. https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... src/python/ct/crypto/pem_test.py:104: pem2 = """-----BEGIN BLOB-----badbase64^&*\n-----END BLOB-----\n""" Make the blobs and their pem representation class-level constants? https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... src/python/ct/crypto/pem_test.py:119: self.assertRaises(pem.PemError, reader.next) Excellent assertion - that's exactly the right way to assert for exceptions in Python tests. https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... src/python/ct/crypto/pem_test.py:121: def test_reader_stops_no_valid_blocks(self): Would you expect the reader to be able to recover once it encountered an invalid block? If so, a test demonstrating that is probably a good idea. https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... src/python/ct/crypto/pem_test.py:166: def test_create_writer_from_file(self): If you've made the PemWriter module a context manager, I'd suggest having a test for this usage as well: Essentially it'd be an identical test except you'd do: with pem.PemWriter.from_file(...) as writer rather than closing explicitly. https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... src/python/ct/crypto/pem_test.py:213: def test_appends_newline(self): If you no longer watch for a newline in the input, then you can get rid of this test. https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/verif... File src/python/ct/crypto/verify.py (right): https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/verif... src/python/ct/crypto/verify.py:29: "ECDSA PUBLIC KEY")) Make this marker a constant as well (I understand that these constants are commonly used when reading certificates from PEM files. If that's the case, consider making them module-level constans in the pem module).
Sign in to reply to this message.
Thanks again. I think this is now the most thoroughly tested PEM utility in the history of ever :) https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... File src/python/ct/crypto/pem_test.py (right): https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... src/python/ct/crypto/pem_test.py:31: for name in self.__tempfiles: On 2013/06/19 10:35:01, Eran wrote: > Python tip: You could use the map function this way: > map(os.remove, self.__tempfiles) > But using map makes more sense when the results are significant, so a for loop > in this case is equally good (maybe even better). Ack (but leaving as is). https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... src/python/ct/crypto/pem_test.py:39: self.assertIsInstance(reader, pem.PemReader) On 2013/06/19 10:35:01, Eran wrote: > This assertion is a bit superfluous - if the reader provides the blobs in an > iterable way, does it matter what kind of instance it is? Hm, but I can also do other things with the returned reader (for example, close it), so seems reasonable to test that it returns a reader and not, say, a list. https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... src/python/ct/crypto/pem_test.py:46: reader.close() On 2013/06/19 10:35:01, Eran wrote: > Close the reader before the assertions, so it's closed regardless of the test > results. Done. https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... src/python/ct/crypto/pem_test.py:49: reader = pem.PemReader.from_string(PemTest.pem_blob, [PemTest.marker]) On 2013/06/19 10:35:01, Eran wrote: > Since the list of markers passed into 'from_string' should not be modified, use > a tuple rather than a list: > reader = pem.PemReader.from_string(PemTest.pem_blob, (PemTest.marker,)) Done, everywhere (and in calling code as well). https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... src/python/ct/crypto/pem_test.py:63: results = [r for r in reader] On 2013/06/19 10:35:01, Eran wrote: > Since you're asserting for the same values in all test cases, I suggest a > private method in the PemReaderTest class for that. Done. https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... src/python/ct/crypto/pem_test.py:90: pem2 = """-----BEGIN CHUNK-----\nd29ybGQ=\n-----END CHUNK-----\n""" On 2013/06/19 10:35:01, Eran wrote: > nit: Rather than hard-coding the base64 value, use base64.encodestring so it's > clear what this value is. I pondered that but actually figured that testing against hard-coded values is better. For example both base64.encodestring("hello") and "hello".encode("base64") append a newline but base64.b64encode("hello") doesn't. Probably matters less in this test (which is really about markers) but I think it helps to visually document the expected result anyway. On an unrelated note, I realized I should be testing multiline inputs too, so I've added two more tests. https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... src/python/ct/crypto/pem_test.py:104: pem2 = """-----BEGIN BLOB-----badbase64^&*\n-----END BLOB-----\n""" On 2013/06/19 10:35:01, Eran wrote: > Make the blobs and their pem representation class-level constants? I've done that for the default values but left them as is in tests that craft a specific invalid input - I find it easier to follow what property is being tested, this way. https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... src/python/ct/crypto/pem_test.py:121: def test_reader_stops_no_valid_blocks(self): On 2013/06/19 10:35:01, Eran wrote: > Would you expect the reader to be able to recover once it encountered an invalid > block? If so, a test demonstrating that is probably a good idea. test_reader_skips_invalid_blocks already tests that - note the middle blob is invalid. (I've added a comment.) https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... src/python/ct/crypto/pem_test.py:166: def test_create_writer_from_file(self): On 2013/06/19 10:35:01, Eran wrote: > If you've made the PemWriter module a context manager, I'd suggest having a test > for this usage as well: Essentially it'd be an identical test except you'd do: > with pem.PemWriter.from_file(...) as writer > rather than closing explicitly. Done for both reader and writer - I've used the construction from a file object to test that the context manager closes it. https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... src/python/ct/crypto/pem_test.py:213: def test_appends_newline(self): On 2013/06/19 10:35:01, Eran wrote: > If you no longer watch for a newline in the input, then you can get rid of this > test. I do watch for newline when constructing the file object myself. The watching is just "best effort" for writers constructed from already open'ed file objects. https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/verif... File src/python/ct/crypto/verify.py (right): https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/verif... src/python/ct/crypto/verify.py:29: "ECDSA PUBLIC KEY")) On 2013/06/19 10:35:01, Eran wrote: > Make this marker a constant as well Done (partial edit error). (I understand that these constants are > commonly used when reading certificates from PEM files. If that's the case, > consider making them module-level constans in the pem module). The public key handling here is sort of temporary glue, so I'll leave the markers here until we have a proper class encapsulating public/private keys.
Sign in to reply to this message.
On 19 June 2013 12:44, <ekasper@google.com> wrote: > Thanks again. I think this is now the most thoroughly tested PEM utility > in the history of ever :) > > > > https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... > File src/python/ct/crypto/pem_test.py (right): > > https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... > src/python/ct/crypto/pem_test.py:31: for name in self.__tempfiles: > On 2013/06/19 10:35:01, Eran wrote: >> >> Python tip: You could use the map function this way: >> map(os.remove, self.__tempfiles) >> But using map makes more sense when the results are significant, so a > > for loop >> >> in this case is equally good (maybe even better). > > > Ack (but leaving as is). > > > https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... > src/python/ct/crypto/pem_test.py:39: self.assertIsInstance(reader, > pem.PemReader) > On 2013/06/19 10:35:01, Eran wrote: >> >> This assertion is a bit superfluous - if the reader provides the blobs > > in an >> >> iterable way, does it matter what kind of instance it is? > > > Hm, but I can also do other things with the returned reader (for > example, close it), so seems reasonable to test that it returns a reader > and not, say, a list. AIUI, Python believes in duck typing, not "proper" types, so the Pythonic thing to do is check the returned value can do the things you expect, not that it is the type you expect (FWIW, capabilities tend to be like this, too, so obviously its a good pattern ;-). > > > https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... > src/python/ct/crypto/pem_test.py:46: reader.close() > On 2013/06/19 10:35:01, Eran wrote: >> >> Close the reader before the assertions, so it's closed regardless of > > the test >> >> results. > > > Done. > > > https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... > src/python/ct/crypto/pem_test.py:49: reader = > pem.PemReader.from_string(PemTest.pem_blob, [PemTest.marker]) > On 2013/06/19 10:35:01, Eran wrote: >> >> Since the list of markers passed into 'from_string' should not be > > modified, use >> >> a tuple rather than a list: >> reader = pem.PemReader.from_string(PemTest.pem_blob, > > (PemTest.marker,)) > > Done, everywhere (and in calling code as well). > > > https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... > src/python/ct/crypto/pem_test.py:63: results = [r for r in reader] > On 2013/06/19 10:35:01, Eran wrote: >> >> Since you're asserting for the same values in all test cases, I > > suggest a >> >> private method in the PemReaderTest class for that. > > > Done. > > > https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... > src/python/ct/crypto/pem_test.py:90: pem2 = """-----BEGIN > CHUNK-----\nd29ybGQ=\n-----END CHUNK-----\n""" > On 2013/06/19 10:35:01, Eran wrote: >> >> nit: Rather than hard-coding the base64 value, use base64.encodestring > > so it's >> >> clear what this value is. > > > I pondered that but actually figured that testing against hard-coded > values is better. For example both base64.encodestring("hello") and > "hello".encode("base64") append a newline but base64.b64encode("hello") > doesn't. > > Probably matters less in this test (which is really about markers) but I > think it helps to visually document the expected result anyway. > > On an unrelated note, I realized I should be testing multiline inputs > too, so I've added two more tests. > > > https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... > src/python/ct/crypto/pem_test.py:104: pem2 = """-----BEGIN > BLOB-----badbase64^&*\n-----END BLOB-----\n""" > On 2013/06/19 10:35:01, Eran wrote: >> >> Make the blobs and their pem representation class-level constants? > > > I've done that for the default values but left them as is in tests that > craft a specific invalid input - I find it easier to follow what > property is being tested, this way. > > > https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... > src/python/ct/crypto/pem_test.py:121: def > test_reader_stops_no_valid_blocks(self): > On 2013/06/19 10:35:01, Eran wrote: >> >> Would you expect the reader to be able to recover once it encountered > > an invalid >> >> block? If so, a test demonstrating that is probably a good idea. > > > test_reader_skips_invalid_blocks already tests that - note the middle > blob is invalid. (I've added a comment.) > > > https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... > src/python/ct/crypto/pem_test.py:166: def > test_create_writer_from_file(self): > On 2013/06/19 10:35:01, Eran wrote: >> >> If you've made the PemWriter module a context manager, I'd suggest > > having a test >> >> for this usage as well: Essentially it'd be an identical test except > > you'd do: >> >> with pem.PemWriter.from_file(...) as writer >> rather than closing explicitly. > > > Done for both reader and writer - I've used the construction from a file > object to test that the context manager closes it. > > > https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... > src/python/ct/crypto/pem_test.py:213: def test_appends_newline(self): > On 2013/06/19 10:35:01, Eran wrote: >> >> If you no longer watch for a newline in the input, then you can get > > rid of this >> >> test. > > > I do watch for newline when constructing the file object myself. The > watching is just "best effort" for writers constructed from already > open'ed file objects. > > > https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/verif... > File src/python/ct/crypto/verify.py (right): > > https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/verif... > src/python/ct/crypto/verify.py:29: "ECDSA PUBLIC KEY")) > On 2013/06/19 10:35:01, Eran wrote: >> >> Make this marker a constant as well > > > Done (partial edit error). > > > (I understand that these constants are >> >> commonly used when reading certificates from PEM files. If that's the > > case, >> >> consider making them module-level constans in the pem module). > > > The public key handling here is sort of temporary glue, so I'll leave > the markers here until we have a proper class encapsulating > public/private keys. > > > https://codereview.appspot.com/10345043/ > > -- > You received this message because you are subscribed to the Google Groups > "Certificate Transparency (external)" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to ctlog-opensource-review+unsubscribe@google.com. > For more options, visit > https://groups.google.com/a/google.com/groups/opt_out. > > > -- > You received this message because you are subscribed to the Google Groups > "Certificate Transparency Team (Internal)" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to ctlog-team+unsubscribe@google.com. > To post to this group, send an email to ctlog-team@google.com. > To view this discussion on the web, visit > https://groups.google.com/a/google.com/d/msgid/ctlog-team/bcaec502d4fafaaf5b0.... > > For more options, visit > https://groups.google.com/a/google.com/groups/opt_out. > >
Sign in to reply to this message.
On 2013/06/19 12:10:32, Ben Laurie (Google) wrote: > On 19 June 2013 12:44, <mailto:ekasper@google.com> wrote: > > Thanks again. I think this is now the most thoroughly tested PEM utility > > in the history of ever :) > > > > > > > > > https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... > > File src/python/ct/crypto/pem_test.py (right): > > > > > https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... > > src/python/ct/crypto/pem_test.py:31: for name in self.__tempfiles: > > On 2013/06/19 10:35:01, Eran wrote: > >> > >> Python tip: You could use the map function this way: > >> map(os.remove, self.__tempfiles) > >> But using map makes more sense when the results are significant, so a > > > > for loop > >> > >> in this case is equally good (maybe even better). > > > > > > Ack (but leaving as is). > > > > > > > https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... > > src/python/ct/crypto/pem_test.py:39: self.assertIsInstance(reader, > > pem.PemReader) > > On 2013/06/19 10:35:01, Eran wrote: > >> > >> This assertion is a bit superfluous - if the reader provides the blobs > > > > in an > >> > >> iterable way, does it matter what kind of instance it is? > > > > > > Hm, but I can also do other things with the returned reader (for > > example, close it), so seems reasonable to test that it returns a reader > > and not, say, a list. > > AIUI, Python believes in duck typing, not "proper" types, so the > Pythonic thing to do is check the returned value can do the things you > expect, not that it is the type you expect (FWIW, capabilities tend to > be like this, too, so obviously its a good pattern ;-). I'm happy to remove the assertion if y'all think it's non-pythonic but... this is about testing a factory. How would you efficiently duck-type-test "class method returns an object that can do exactly the same things as an explicitly constructed MyObject"? > > > > > > > https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... > > src/python/ct/crypto/pem_test.py:46: reader.close() > > On 2013/06/19 10:35:01, Eran wrote: > >> > >> Close the reader before the assertions, so it's closed regardless of > > > > the test > >> > >> results. > > > > > > Done. > > > > > > > https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... > > src/python/ct/crypto/pem_test.py:49: reader = > > pem.PemReader.from_string(PemTest.pem_blob, [PemTest.marker]) > > On 2013/06/19 10:35:01, Eran wrote: > >> > >> Since the list of markers passed into 'from_string' should not be > > > > modified, use > >> > >> a tuple rather than a list: > >> reader = pem.PemReader.from_string(PemTest.pem_blob, > > > > (PemTest.marker,)) > > > > Done, everywhere (and in calling code as well). > > > > > > > https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... > > src/python/ct/crypto/pem_test.py:63: results = [r for r in reader] > > On 2013/06/19 10:35:01, Eran wrote: > >> > >> Since you're asserting for the same values in all test cases, I > > > > suggest a > >> > >> private method in the PemReaderTest class for that. > > > > > > Done. > > > > > > > https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... > > src/python/ct/crypto/pem_test.py:90: pem2 = """-----BEGIN > > CHUNK-----\nd29ybGQ=\n-----END CHUNK-----\n""" > > On 2013/06/19 10:35:01, Eran wrote: > >> > >> nit: Rather than hard-coding the base64 value, use base64.encodestring > > > > so it's > >> > >> clear what this value is. > > > > > > I pondered that but actually figured that testing against hard-coded > > values is better. For example both base64.encodestring("hello") and > > "hello".encode("base64") append a newline but base64.b64encode("hello") > > doesn't. > > > > Probably matters less in this test (which is really about markers) but I > > think it helps to visually document the expected result anyway. > > > > On an unrelated note, I realized I should be testing multiline inputs > > too, so I've added two more tests. > > > > > > > https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... > > src/python/ct/crypto/pem_test.py:104: pem2 = """-----BEGIN > > BLOB-----badbase64^&*\n-----END BLOB-----\n""" > > On 2013/06/19 10:35:01, Eran wrote: > >> > >> Make the blobs and their pem representation class-level constants? > > > > > > I've done that for the default values but left them as is in tests that > > craft a specific invalid input - I find it easier to follow what > > property is being tested, this way. > > > > > > > https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... > > src/python/ct/crypto/pem_test.py:121: def > > test_reader_stops_no_valid_blocks(self): > > On 2013/06/19 10:35:01, Eran wrote: > >> > >> Would you expect the reader to be able to recover once it encountered > > > > an invalid > >> > >> block? If so, a test demonstrating that is probably a good idea. > > > > > > test_reader_skips_invalid_blocks already tests that - note the middle > > blob is invalid. (I've added a comment.) > > > > > > > https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... > > src/python/ct/crypto/pem_test.py:166: def > > test_create_writer_from_file(self): > > On 2013/06/19 10:35:01, Eran wrote: > >> > >> If you've made the PemWriter module a context manager, I'd suggest > > > > having a test > >> > >> for this usage as well: Essentially it'd be an identical test except > > > > you'd do: > >> > >> with pem.PemWriter.from_file(...) as writer > >> rather than closing explicitly. > > > > > > Done for both reader and writer - I've used the construction from a file > > object to test that the context manager closes it. > > > > > > > https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/pem_t... > > src/python/ct/crypto/pem_test.py:213: def test_appends_newline(self): > > On 2013/06/19 10:35:01, Eran wrote: > >> > >> If you no longer watch for a newline in the input, then you can get > > > > rid of this > >> > >> test. > > > > > > I do watch for newline when constructing the file object myself. The > > watching is just "best effort" for writers constructed from already > > open'ed file objects. > > > > > > > https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/verif... > > File src/python/ct/crypto/verify.py (right): > > > > > https://codereview.appspot.com/10345043/diff/24001/src/python/ct/crypto/verif... > > src/python/ct/crypto/verify.py:29: "ECDSA PUBLIC KEY")) > > On 2013/06/19 10:35:01, Eran wrote: > >> > >> Make this marker a constant as well > > > > > > Done (partial edit error). > > > > > > (I understand that these constants are > >> > >> commonly used when reading certificates from PEM files. If that's the > > > > case, > >> > >> consider making them module-level constans in the pem module). > > > > > > The public key handling here is sort of temporary glue, so I'll leave > > the markers here until we have a proper class encapsulating > > public/private keys. > > > > > > https://codereview.appspot.com/10345043/ > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Certificate Transparency (external)" group. > > To unsubscribe from this group and stop receiving emails from it, send an > > email to mailto:ctlog-opensource-review+unsubscribe@google.com. > > For more options, visit > > https://groups.google.com/a/google.com/groups/opt_out. > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Certificate Transparency Team (Internal)" group. > > To unsubscribe from this group and stop receiving emails from it, send an > > email to mailto:ctlog-team+unsubscribe@google.com. > > To post to this group, send an email to mailto:ctlog-team@google.com. > > To view this discussion on the web, visit > > > https://groups.google.com/a/google.com/d/msgid/ctlog-team/bcaec502d4fafaaf5b0.... > > > > For more options, visit > > https://groups.google.com/a/google.com/groups/opt_out. > > > >
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
