After you add the comment, feel free to merge.
Reviewed-by: Richard Hansen <rhan...@bbn.com>
Thanks,
Richard
On 2015-05-21 21:53, David Mandelberg wrote:
> On 2015-05-21 15:28, Richard Hansen wrote:
>> Hi David,
>>
>> I only see minor stuff; it can be merged as-is if you don't feel like
>> making any changes.
>>
>> General comment: Do we have any tests for TAL file parsing? If not,
>> perhaps we should add some before making this change.
>
> We don't have any automatic tests for TAL parsing, but I did test this
> patch if I remember correctly. We also have a step in the pre release
> checklist that would catch any major bugs in TAL parsing. Also, if there
> were an error in TAL parsing code, it would be very easy to diagnose
> because of how simple TALs are and how they only change when a person
> changes them. In short, TAL parsing tests might be nice to have at some
> point, but I don't think they're all that important now.
>
>
>> Specific comments in-line below.
>>
>> On 2015-04-06 20:55, David Mandelberg wrote:
>>> draft-ietf-sidr-rfc6490-bis-03 defines a new format for TALs. This
>>
>> drop the -03
>
> I like including the revision number, since it makes it easier to
> update code to a newer revision (or published RFC) if there are any
> changes to the draft in the future. I've also found it helpful in the
> past when I've gone through code that was written to an older revision
> of a draft than I was familiar with.
>
>
>>> commit adds support for both old and new style TALs.
>>> ---
>>> bin/rpki/updateTA.py.in | 47
>>> ++++++++++++++++++++++++++++++++---------------
>>> 1 file changed, 32 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/bin/rpki/updateTA.py.in b/bin/rpki/updateTA.py.in
>>> index 146e54f..6c803df 100644
>>> --- a/bin/rpki/updateTA.py.in
>>> +++ b/bin/rpki/updateTA.py.in
>>> @@ -223,21 +223,38 @@ def delete_other_TAs(keep_paths):
>>>
>>> def parse_TAL_file(tal_file):
>>> """Return [rsyncURI, subjectPublicKeyInfo] as a pair of
>>> strings."""
>>> - f = open(tal_file, "r")
>>> -
>>> - # First non-blank line is the TA rsync URI
>>> - rsync_uri = None
>>> - while not rsync_uri:
>>> - rsync_uri = f.readline().strip()
>>> -
>>> - # Base64-encoded SubjectPublicKeyInfo is the rest of the file,
>>> - # with all whitespace removed.
>>> - words = []
>>> - for line in f:
>>> - for w in line.strip().split():
>>> - words.append(w)
>>> - pubkey_b64 = "".join(words)
>>> - return [rsync_uri, pubkey_b64]
>>> + with open(tal_file, "r") as f:
>>
>> you can use open(tal_file, "rU") to convert all line endings to '\n'
>
> Thanks, I didn't know about that. I don't think that affects
> correctness here, and it only affects readability minimally, so I'm not
> going to change it now. (But I'll keep it in mind for the future.) If
> I'm wrong about it affecting correctness, please correct me.
>
>
>>> + lines = f.readlines()
>>> +
>>> + rsync_uris = []
>>> + pubkey_b64_words = []
>>> +
>>> + if '\r\n' in lines or '\n' in lines:
>>> + # Parse as in draft-ietf-sidr-rfc6490-bis-03
>>
>> drop the -03
>
> [same explanation as above]
>
>
>>> + in_uri_section = True
>>> + for line in lines:
>>> + if in_uri_section:
>>> + if line in ('\r\n', '\n'):
>>> + in_uri_section = False
>>> + continue
>>> + elif line.lower().startswith('rsync://'):
>>> + rsync_uris.append(line.strip())
>>
>> There should be a comment here explaining that we check if the line
>> starts with 'rsync://' for forward compatibility with non-rsync URIs.
>
> Good point. I'll add one.
>
>
>>> + else:
>>> + pubkey_b64_words.extend(line.split())
>>> +
>>> + else:
>>> + # Parse as in RFC6490
>>> + if lines[0].lower().startswith('rsync://'):
>>
>> I'm not sure why you test if it starts with 'rsync://' here.
>
> Two related reasons:
> * To trigger the RuntimeError('TAL file has no rsync URIs: ' +
> tal_file) below if there's a typo in the TAL that causes us to be unable
> to extract the rsync URI.
> * To prevent us from running commands like `rsync
> jOMAHWzGm6osrgoogqwakxfZC8zEUSorhv1sTwWBIwJe4I
> /path/to/cache/trustanchors/jOMAHWzGm6osrgoogqwakxfZC8zEUSorhv1sTwWBIwJe4I`
> if the first line of the TAL was accidentally removed.
>
>>
>>> + rsync_uris.append(lines[0].strip())
>>> + for line in lines[1:]:
>>> + pubkey_b64_words.extend(line.split())
>>> +
>>> + if not rsync_uris:
>>> + raise RuntimeError('TAL file has no rsync URIs: ' +
>>> tal_file)
>>> + elif not pubkey_b64_words:
>>> + raise RuntimeError('TAL file has no public key: ' +
>>> tal_file)
>>> +
>>> + return [rsync_uris[0], ''.join(pubkey_b64_words)]
>>
>> The above logic looks correct and is quite readable, but it does
>> multiple passes over the lines and I think it's a bit more complex
>> than
>> it needs to be. What about something like this:
>>
>> uris = []
>> pubkey_b64_words = []
>>
>> with open(tal_file, 'r') as f:
>> in_uri_section = True
>> for line in (l.strip() for l in f):
>> # base64 encoded data doesn't have colons, URIs do
>> if in_uri_section and ':' not in line:
>> in_uri_section = False
>> if in_uri_section:
>> uris.append(line)
>> else:
>> pubkey_b64_words.extend(line.split())
>>
>> rsync_uri = next(
>> (x for x in uris if x.lower().startswith('rsync://')), None)
>>
>> if rsync_uri is None:
>> raise RuntimeError('TAL file has no rsync URIs: ' + tal_file)
>> elif not pubkey_b64_words:
>> raise RuntimeError('TAL file has no public key: ' + tal_file)
>>
>> return [rsync_uri, ''.join(pubkey_b64_words)]
>>
>> The above is maybe a bit inscrutable, however.
>
> TAL files are pretty small. I made a conscious decision to read the
> entire file into memory and do multiple passes, in order to make the
> code more readable.
------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
rpstir-devel mailing list
rpstir-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/rpstir-devel
