Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1246703005:
Add a script for formatting the VerifySignedData() test data. (Closed)
DescriptionAdd a script for formatting the VerifySignedData() test data.
Primarily it calls "openssl asn1parse -i" for each of the DER data fields and annotates the base64 with this.
This is a one-off script intended to be called manually when adding new test data, or modifying existing test data.
BUG=410574
Committed: https://crrev.com/bd530dfd6e9f7b2f4ce449325344563f9279a50a
Cr-Commit-Position: refs/heads/master@{#340852}
Patch Set 1 #Patch Set 2 : remove innacurate comment #Patch Set 3 : cleanup a bit #
Total comments: 20
Patch Set 4 : Address Matt's comments #
Total comments: 2
Patch Set 5 : remove leading undercore from pattern name #Patch Set 6 : switch to re.finditer() #
Total comments: 5
Patch Set 7 : use non-greedy matching #Patch Set 8 : fix (don't use matching for comment) #
Dependent Patchsets: Messages
Total messages: 18 (3 generated)
eroman@chromium.org changed reviewers: + davidben@chromium.org
I suspect I have a number of python style issues, but figured it would be useful to have this checked in. (I used it to format the test files in other CL)
eroman@chromium.org changed reviewers: + mattm@chromium.org
Also add in Matt as a possible reviewer. Nothing real fancy in here, just a test helper script.
https://codereview.chromium.org/1246703005/diff/40001/net/data/verify_signed_... File net/data/verify_signed_data_unittest/annotate_test_data.py (right): https://codereview.chromium.org/1246703005/diff/40001/net/data/verify_signed_... net/data/verify_signed_data_unittest/annotate_test_data.py:40: if len(user_comment) != 0: "if user_comment:" https://codereview.chromium.org/1246703005/diff/40001/net/data/verify_signed_... net/data/verify_signed_data_unittest/annotate_test_data.py:49: result += '\n' + generated_comment + '\n' So this is putting the text formatted version of the data after the pem-encoded version? The normal openssl output puts the formatted version before the pem data (eg, look net/data/ssl/certificates/*.pem) https://codereview.chromium.org/1246703005/diff/40001/net/data/verify_signed_... net/data/verify_signed_data_unittest/annotate_test_data.py:68: end = text.find('-----BEGIN ') slightly more pythonic: text.split('-----BEGIN ')[0].strip('\n') https://codereview.chromium.org/1246703005/diff/40001/net/data/verify_signed_... net/data/verify_signed_data_unittest/annotate_test_data.py:78: pos = comment.find('$ openssl asn1parse -i') same as previous comment https://codereview.chromium.org/1246703005/diff/40001/net/data/verify_signed_... net/data/verify_signed_data_unittest/annotate_test_data.py:95: def GetFilePaths(): GetPemFilePaths https://codereview.chromium.org/1246703005/diff/40001/net/data/verify_signed_... net/data/verify_signed_data_unittest/annotate_test_data.py:99: for filename in glob.iglob('*.pem'): glob includes the path in the results, so you could just do: return glob.iglob(os.path.join(base_dir, '*.pem')) https://codereview.chromium.org/1246703005/diff/40001/net/data/verify_signed_... net/data/verify_signed_data_unittest/annotate_test_data.py:104: with open (path, 'r') as f: no space before () https://codereview.chromium.org/1246703005/diff/40001/net/data/verify_signed_... net/data/verify_signed_data_unittest/annotate_test_data.py:153: header_pattern = re.compile('-----BEGIN ([\w ]+)-----') pretty nitpicky for a little helper script, but.. calling re.compile inside a loop is a bit odd.. I'd just use re.search or call re.compile outside the function definition (with a leading underscore in the variable name). https://codereview.chromium.org/1246703005/diff/40001/net/data/verify_signed_... net/data/verify_signed_data_unittest/annotate_test_data.py:167: footer_end = footer_start + len(footer) you could do something fancier and make this whole parsing loop with re.finditer and re.DOTALL. Or not. Not that important for a little helper script like this.
Thanks for the review and python tips! https://codereview.chromium.org/1246703005/diff/40001/net/data/verify_signed_... File net/data/verify_signed_data_unittest/annotate_test_data.py (right): https://codereview.chromium.org/1246703005/diff/40001/net/data/verify_signed_... net/data/verify_signed_data_unittest/annotate_test_data.py:40: if len(user_comment) != 0: On 2015/07/28 00:51:15, mattm wrote: > "if user_comment:" Done. https://codereview.chromium.org/1246703005/diff/40001/net/data/verify_signed_... net/data/verify_signed_data_unittest/annotate_test_data.py:49: result += '\n' + generated_comment + '\n' On 2015/07/28 00:51:15, mattm wrote: > So this is putting the text formatted version of the data after the pem-encoded > version? Correct. Examples at https://codereview.chromium.org/1209283004/ > The normal openssl output puts the formatted version before the pem > data (eg, look net/data/ssl/certificates/*.pem) I initially had the comments on top to match, but I ran into some ambiguity issues since the file level description appears as a comment for the first block (and I wanted to offset them differently). I solved this by simply putting block comments after the PEM. I am sure I can make the comments above approach work too (and update this code to match), however I think it is simpler to read and parse in the bottom manner. What do you think? https://codereview.chromium.org/1246703005/diff/40001/net/data/verify_signed_... net/data/verify_signed_data_unittest/annotate_test_data.py:68: end = text.find('-----BEGIN ') On 2015/07/28 00:51:15, mattm wrote: > slightly more pythonic: text.split('-----BEGIN ')[0].strip('\n') Done. https://codereview.chromium.org/1246703005/diff/40001/net/data/verify_signed_... net/data/verify_signed_data_unittest/annotate_test_data.py:78: pos = comment.find('$ openssl asn1parse -i') On 2015/07/28 00:51:15, mattm wrote: > same as previous comment Done. https://codereview.chromium.org/1246703005/diff/40001/net/data/verify_signed_... net/data/verify_signed_data_unittest/annotate_test_data.py:95: def GetFilePaths(): On 2015/07/28 00:51:15, mattm wrote: > GetPemFilePaths Done. https://codereview.chromium.org/1246703005/diff/40001/net/data/verify_signed_... net/data/verify_signed_data_unittest/annotate_test_data.py:99: for filename in glob.iglob('*.pem'): On 2015/07/28 00:51:15, mattm wrote: > glob includes the path in the results, so you could just do: > return glob.iglob(os.path.join(base_dir, '*.pem')) Done, much better! In fact I don't think my previous code made sense -- I was still depending on the CWD even though I meant not to ;) https://codereview.chromium.org/1246703005/diff/40001/net/data/verify_signed_... net/data/verify_signed_data_unittest/annotate_test_data.py:104: with open (path, 'r') as f: On 2015/07/28 00:51:14, mattm wrote: > no space before () Done. https://codereview.chromium.org/1246703005/diff/40001/net/data/verify_signed_... net/data/verify_signed_data_unittest/annotate_test_data.py:153: header_pattern = re.compile('-----BEGIN ([\w ]+)-----') On 2015/07/28 00:51:15, mattm wrote: > pretty nitpicky for a little helper script, but.. calling re.compile inside a > loop is a bit odd.. I'd just use re.search or call re.compile outside the > function definition (with a leading underscore in the variable name). Done, thanks! https://codereview.chromium.org/1246703005/diff/40001/net/data/verify_signed_... net/data/verify_signed_data_unittest/annotate_test_data.py:167: footer_end = footer_start + len(footer) On 2015/07/28 00:51:15, mattm wrote: > you could do something fancier and make this whole parsing loop with re.finditer > and re.DOTALL. Or not. Not that important for a little helper script like this. Thanks I will look that up. Will probably keep it as is for now.
https://codereview.chromium.org/1246703005/diff/40001/net/data/verify_signed_... File net/data/verify_signed_data_unittest/annotate_test_data.py (right): https://codereview.chromium.org/1246703005/diff/40001/net/data/verify_signed_... net/data/verify_signed_data_unittest/annotate_test_data.py:49: result += '\n' + generated_comment + '\n' On 2015/07/28 01:41:24, eroman wrote: > On 2015/07/28 00:51:15, mattm wrote: > > So this is putting the text formatted version of the data after the > pem-encoded > > version? > > Correct. Examples at https://codereview.chromium.org/1209283004/ > > > The normal openssl output puts the formatted version before the pem > > data (eg, look net/data/ssl/certificates/*.pem) > > I initially had the comments on top to match, but I ran into some ambiguity > issues since the file level description appears as a comment for the first block > (and I wanted to offset them differently). I solved this by simply putting block > comments after the PEM. > > I am sure I can make the comments above approach work too (and update this code > to match), however I think it is simpler to read and parse in the bottom manner. > What do you think? I guess it's not a big deal either way. Having it above would be nice for consistency, but this is good enough, no need to spend more time on it. https://codereview.chromium.org/1246703005/diff/60001/net/data/verify_signed_... File net/data/verify_signed_data_unittest/annotate_test_data.py (right): https://codereview.chromium.org/1246703005/diff/60001/net/data/verify_signed_... net/data/verify_signed_data_unittest/annotate_test_data.py:139: _header_pattern = re.compile('-----BEGIN ([\w ]+)-----') Actually meant outside the function (or if you leave it inside, name without the leading underscore).
https://codereview.chromium.org/1246703005/diff/60001/net/data/verify_signed_... File net/data/verify_signed_data_unittest/annotate_test_data.py (right): https://codereview.chromium.org/1246703005/diff/60001/net/data/verify_signed_... net/data/verify_signed_data_unittest/annotate_test_data.py:139: _header_pattern = re.compile('-----BEGIN ([\w ]+)-----') On 2015/07/28 04:22:14, mattm wrote: > Actually meant outside the function (or if you leave it inside, name without the > leading underscore). Done, removed underscore
https://codereview.chromium.org/1246703005/diff/40001/net/data/verify_signed_... File net/data/verify_signed_data_unittest/annotate_test_data.py (right): https://codereview.chromium.org/1246703005/diff/40001/net/data/verify_signed_... net/data/verify_signed_data_unittest/annotate_test_data.py:167: footer_end = footer_start + len(footer) On 2015/07/28 01:41:23, eroman wrote: > On 2015/07/28 00:51:15, mattm wrote: > > you could do something fancier and make this whole parsing loop with > re.finditer > > and re.DOTALL. Or not. Not that important for a little helper script like > this. > > Thanks I will look that up. > > Will probably keep it as is for now. Done - switched to re.finditer()
https://codereview.chromium.org/1246703005/diff/100001/net/data/verify_signed... File net/data/verify_signed_data_unittest/annotate_test_data.py (right): https://codereview.chromium.org/1246703005/diff/100001/net/data/verify_signed... net/data/verify_signed_data_unittest/annotate_test_data.py:136: regex = re.compile(r'-----BEGIN ([\w ]+)-----(.*)-----END \1-----', re.DOTALL) probably want to use non-greedy match, in case the same block name appears multiple times (add a question mark after the .*).
https://codereview.chromium.org/1246703005/diff/100001/net/data/verify_signed... File net/data/verify_signed_data_unittest/annotate_test_data.py (right): https://codereview.chromium.org/1246703005/diff/100001/net/data/verify_signed... net/data/verify_signed_data_unittest/annotate_test_data.py:136: regex = re.compile(r'-----BEGIN ([\w ]+)-----(.*)-----END \1-----', re.DOTALL) On 2015/07/28 22:27:00, mattm wrote: > probably want to use non-greedy match, in case the same block name appears > multiple times (add a question mark after the .*). Done. I also incorporated the comment into the regexp (rather than using GetTextUntilNextPemBlock).
https://codereview.chromium.org/1246703005/diff/100001/net/data/verify_signed... File net/data/verify_signed_data_unittest/annotate_test_data.py (right): https://codereview.chromium.org/1246703005/diff/100001/net/data/verify_signed... net/data/verify_signed_data_unittest/annotate_test_data.py:136: regex = re.compile(r'-----BEGIN ([\w ]+)-----(.*)-----END \1-----', re.DOTALL) On 2015/07/29 01:07:55, eroman wrote: > On 2015/07/28 22:27:00, mattm wrote: > > probably want to use non-greedy match, in case the same block name appears > > multiple times (add a question mark after the .*). > > Done. > > I also incorporated the comment into the regexp (rather than using > GetTextUntilNextPemBlock). Hm, did you test that? I wasn't sure a non-greedy match at the end of a pattern would work like that or just match nothing.
https://codereview.chromium.org/1246703005/diff/100001/net/data/verify_signed... File net/data/verify_signed_data_unittest/annotate_test_data.py (right): https://codereview.chromium.org/1246703005/diff/100001/net/data/verify_signed... net/data/verify_signed_data_unittest/annotate_test_data.py:136: regex = re.compile(r'-----BEGIN ([\w ]+)-----(.*)-----END \1-----', re.DOTALL) On 2015/07/29 01:17:47, mattm wrote: > On 2015/07/29 01:07:55, eroman wrote: > > On 2015/07/28 22:27:00, mattm wrote: > > > probably want to use non-greedy match, in case the same block name appears > > > multiple times (add a question mark after the .*). > > > > Done. > > > > I also incorporated the comment into the regexp (rather than using > > GetTextUntilNextPemBlock). > > Hm, did you test that? I wasn't sure a non-greedy match at the end of a pattern > would work like that or just match nothing. You are correct, that did not work, thanks! Reverted that change.
lgtm https://codereview.chromium.org/1246703005/diff/100001/net/data/verify_signed... File net/data/verify_signed_data_unittest/annotate_test_data.py (right): https://codereview.chromium.org/1246703005/diff/100001/net/data/verify_signed... net/data/verify_signed_data_unittest/annotate_test_data.py:136: regex = re.compile(r'-----BEGIN ([\w ]+)-----(.*)-----END \1-----', re.DOTALL) On 2015/07/29 01:38:47, eroman wrote: > On 2015/07/29 01:17:47, mattm wrote: > > On 2015/07/29 01:07:55, eroman wrote: > > > On 2015/07/28 22:27:00, mattm wrote: > > > > probably want to use non-greedy match, in case the same block name appears > > > > multiple times (add a question mark after the .*). > > > > > > Done. > > > > > > I also incorporated the comment into the regexp (rather than using > > > GetTextUntilNextPemBlock). > > > > Hm, did you test that? I wasn't sure a non-greedy match at the end of a > pattern > > would work like that or just match nothing. > > You are correct, that did not work, thanks! > Reverted that change. I guess you could also use a lookahead like (.*?)(?=-----BEGIN |$)', but that might be getting too clever. This is fine.
The CQ bit was checked by eroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1246703005/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1246703005/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/bd530dfd6e9f7b2f4ce449325344563f9279a50a Cr-Commit-Position: refs/heads/master@{#340852} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
