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.
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
> 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'
> + 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
> + 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.
> + 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.
> + 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.
-Richard
>
> def compute_local_path(rsync_uri, local_repo_path):
> if rsync_uri.startswith("rsync://"):
>
------------------------------------------------------------------------------
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
