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.
--
David Eric Mandelberg / dseomn
http://david.mandelberg.org/
------------------------------------------------------------------------------
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
