bgp / stayrtr

RPKI-To-Router server implementation in Go
BSD 3-Clause "New" or "Revised" License
91 stars 13 forks source link

dev branch does not accept JSON in "AS1234" format #13

Closed ties closed 3 years ago

ties commented 3 years ago

I am running (three instances) of rtrmon to compare multiple JSON endpoints:

Yesterday, when rebasing my changes (#9) on top of dev, I found out that stayrtr no longer accepts the JSON from some of the endpoints.

 go build cmd/rtrmon/rtrmon.go   && ./rtrmon -secondary.host http://[url]:8888/objects/validated -primary.host https://[internal-endpoint]/[path]/roa-prefixes -secondary.refresh 30s -primary.refresh 30s 2>&1 | head -n 10  
time="2021-07-20T10:34:49+02:00" level=info msg="1: Fetching https://ba-apps.prepdev.ripe.net/certification/api/monitoring/roa-prefixes"
time="2021-07-20T10:34:49+02:00" level=info msg="2: Fetching http://alias.student.utwente.nl:8888/objects/validated"
time="2021-07-20T10:34:51+02:00" level=error msg="2: exploration error for {2.0.0.0/12 17 3215 ripe 1626832357} asn: Could not decode ASN: 3215 as part of VRP"
time="2021-07-20T10:34:51+02:00" level=error msg="2: exploration error for {2.9.0.0/17 24 3215 ripe 1626832357} asn: Could not decode ASN: 3215 as part of VRP"
time="2021-07-20T10:34:51+02:00" level=error msg="2: exploration error for {2.9.128.0/17 24 3215 ripe 1626832357} asn: Could not decode ASN: 3215 as part of VRP"

The root cause appears to be a difference in JSON format - which now is :

// internal endpoint
    {
      "asn": "AS0",
      "prefix": "95.214.130.0/24",
      "maxLength": 24
    },
// rpki-client
    {
      "asn": 0,
      "prefix": "95.214.130.0/24",
      "maxLength": 24,
      "ta": "ripe",
      "expires": 1626853914
    },
// routinator
    {
      "asn": "AS0",
      "prefix": "95.214.130.0/24",
      "maxLength": 24,
      "ta": "ripe"
    },

While I do agree that rpki-client matches the syntax from rfc8416 and the others use a legacy, implicitly documented format, I think that breaking compatibility here causes operational issues in mixed environments.

If it helps I'm willing to maintain the parsing code for this json :-).

job commented 3 years ago

@ties I prefer to get rid of the "could be string could be integer" pattern, it pollutes the code for no great reason. I think changing the format is short term pain, but long term beneficial. StayRTR will probably be using other specific aspects such as metadata.buildtime and the expires keys.

Is it an option to transform legacy JSON to new JSON where needed?

I don't think this will cause operational issues, as StayRTR is pretty new to the game.

job commented 3 years ago

BTW feel free to ignore the dev branch: the only relevant branch to consider when preparing pull requests is master. I should move dev to my personal fork :-)

ties commented 3 years ago

I'm higher level languages myself (so I did not see this concern), but I heard from more BGP developers that prefer "asn is definitely an integer". I think you heard similar input and released the change in rpki-client's output somewhere after https://github.com/rpki-client/rpki-client-openbsd/commit/46b333866691c90eeeb414de918994200675da34.

I was helping someone troubleshoot a situation where stayrtr was not updating from rpki-client. We did not find a root cause - but given it can cause issues like this - because of the robustness principle I'd like it if we accept this format a little longer. I'm willing to do some work to prevent clutter.

For the VRPs I think all implementations have asn and prefix, nothing else is guaranteed to be available. For the metadata... There is not even a guarantee that the metadata is present and stayrtr is leading. I think this still needs to evolve.

Short-term, I can see 2-3 fields becoming standard (buildtime, buildmachine, and hopefully a field that tags which implementation and version made the file)

job commented 3 years ago

I am not sure the robustness principle applies in context of (security) software which has to follow a 'hard decision' tree. :-)

The tendrils of "String vs Integer" complicate the code enormously because at multiple places 'fix-up' or 'fix-down' has to be done.

If upon import both String and Integer are accepted; but throughout the code paths and on output it always is an uint32 I think that's preferable to the current situation. This would mean there only is a fix-up in one place. What do you think of that approach?

ties commented 3 years ago

The robustness principle definitely does not apply in the context of (security) software that has to follow a fixed decision-tree. But if we limit it to the input here... I think it does.

If upon import both String and Integer are accepted; but throughout the code paths and on output it always is an uint32 I think that's preferable to the current situation. This would mean there only is a fix-up in one place. What do you think of that approach?

That sounds good. Accept both representations on import. By doing so the parsing of both forms is limited to one location. And use one (canonical, asn = uint32) form for output and everywhere else.