RPKI Prefix Origin Validation Support #33
Conversation
- The configure script checks whether the shared library is available or not and supports the option ```—without-rpki``` - The BGPReader accepts multiple new parameters which allows setting all RPKI options: * ```--rpki``` enables the historical RPKI validation with default collector (standalone) * ```--rpki-live``` enables the live RPKI validation with a default RTR-Server (standlone) * ```--rpki-collectors``` specifies the collectors used for the RPKI validation (standalone), <br /> Format: "((\*|project):(\*|(collector(,collectors)*))(;)?)*", <br /> e.g. "PJ_1:CC_1,CC_11;PJ_2:CC_2" or "PJ_1:*;PJ_2:CC_2" or "\*:\*" * ```--rpki-unified``` enables an unified validation where the ROA dumps of the different collectors are combined * ```--rpki-ssh``` enables SSH encryption to the RTR Server, format: "username, hostkey_path, privatekey_path" - On basis of the parameters the BGPReader creates the ROAFetchlib configuration which is needed for the validation process and stores it in every BGP elem instance - During the output process every BGP elem will be validated and the validation result is added to the regular BGPReader output
|
@alistairking @digizeph ping |
1 similar comment
|
@alistairking @digizeph ping |
|
Folks, @alistairking @digizeph, can we get some ETA? Would be really nice to know when and how to proceed. Thanks! |
configure.ac
Outdated
| [AS_HELP_STRING([--without-rpki], | ||
| [do not compile with rpki support])], | ||
| [], | ||
| [with_rpki=yes]) |
There was a problem hiding this comment.
consider default no for rpki support. @alistairking what do you think?
There was a problem hiding this comment.
Definitely.
I think we have two options:
- if roafetchlib is installed, enable rpki by default
- disable rpki unless explicitly requested
I'm ok with the first option iff the rpki code stays dormant unless explicitly requested at run time, which, i'm assuming is how things work at the moment.
There was a problem hiding this comment.
Okay. Now RPKI support is disabled unless explicitly requested with with-rpki. I chose it because it's a bit more in line with the other options. I don't think the two approaches differ much in practice, because the ROAfetchlib is not included anyway.
| @@ -107,6 +107,19 @@ typedef enum { | |||
|
|
|||
| } bgpstream_elem_type_t; | |||
|
|
|||
| typedef struct struct_bgpstream_annotations_t { | |||
|
|
|||
| /** RPKI active */ | |||
There was a problem hiding this comment.
minor issue: this annotation struct seems to be only about rpki annotation. if we want to keep it generic, we can keep the struct name, and enclose rpki-related fields in #ifdef block. @alistairking
There was a problem hiding this comment.
We already had the same problem the other way around in the last pull request.
99e682f - "2. we always include all the possible fields in the public structures. this is much better since the API will be stable no matter what compile time options are used."
There was a problem hiding this comment.
hah yeah, you're right. thanks for being more awake than me. this should stay as-is.
| * Annotations from other libraries | ||
| */ | ||
| bgpstream_annotations_t annotations; | ||
|
|
There was a problem hiding this comment.
enclose the above with #ifdef ?
There was a problem hiding this comment.
no, let's go with your other suggestion above.
| @@ -211,8 +225,10 @@ static void usage() | |||
| fprintf(stderr, " -%c, --%-23s%-15s %s\n", OPTIONS[k].option.val, | |||
| OPTIONS[k].option.name, OPTIONS[k].usage, expl_buf); | |||
| } else { | |||
| #ifdef WITH_RPKI | |||
There was a problem hiding this comment.
@salsh why add this #ifdef?
There was a problem hiding this comment.
yeah, i'm confused by this too. and what are the 50x numbers in the argument config structs?
There was a problem hiding this comment.
The long args should be visible in usage (help section) only if RPKI support is active. Assumption: all long args belong to RPKI.
| if(isalpha(OPTIONS[k].option.val)) { | ||
| snprintf(short_options + size, sizeof(short_options) - size, | ||
| OPTIONS[k].option.has_arg ? "%c:": "%c", OPTIONS[k].option.val); | ||
| } |
There was a problem hiding this comment.
@salsh missing a else here?
There was a problem hiding this comment.
The "else" part is empty because the non-alpha options (long args) are included in long_options in every iteration anyway (short args also have long args but the RPKI related long args do not have short args).
tools/bgpreader.c
Outdated
| case 504: | ||
| bgpstream_rpki_parse_default(rpki_input); | ||
| break; | ||
| #endif |
There was a problem hiding this comment.
@salsh is the naming of these arguments normal? -50x is hard to understand without actually checking the source code.
There was a problem hiding this comment.
oh, i see. i guess you (@salsh) wanted long args without short args, so the 50x are a way to ensure no normal argument collides with them.
i think this is fine, but let's create an enum at the top of bgpreader that gives these values a more friendly name.
There was a problem hiding this comment.
oh, i see. i guess you (@salsh) wanted long args without short args, so the 50x are a way to ensure no normal argument collides with them.
Exactly.
i think this is fine, but let's create an enum at the top of bgpreader that gives these values a more friendly name.
Alright, done!
|
@salsh i think we're pretty close to merging this. see above for various minor comments |
|
@alistairking Sounds good. I know it's a bit of an effort, but I would appreciate it if any of you could really execute it. Just to make sure everything is acceptable to you guys (perhaps some details that are not necessarily visible through a code review). |
|
@salsh Thanks for the replies. I will start executing it later today. I'll get back to you real soon. |
|
hey @salsh , i got the following error when run with it then print out regular i configured bgpstream with |
|
@digizeph oh sorry, my mistake - I didn't think you would execute it so quickly. Currently only timestamps before 2018-07 are available because we are improving the broker infrastructure at the moment. |
|
Thanks @salsh ! I was able to get it working with an earlier timestamp. The output from Overall, the output looks good. |
|
Running with |
|
@salsh I assume this can be handled with better documentation. |
|
@salsh With Without |
|
@digizeph Thanks for executing.
All validation related outputs only affect the last element. The bgpstream output always remains unaffected. |
As pointed out in the last comment
Actually the error message should be clear: For live validation only RTR-Servers (all collectors with "RTR") are allowed e.g. |
Yes the actual difference is only visible if multiple collectors were defined e.g.: The different delimiters are for easier parsing and to avoid redundancy. |
Is the help section more understandable now? |
|
@salsh thanks for the responses. They all make sense to me. I think the current usage text can stay. |
|
@salsh We have merged your PR into master. Thank you for your contribution, Samir! |
|
Glad to hear it. @alistairking @digizeph - Thank you very much for merging the PR, your time and effort. I appreciate it very much. |
The configure script checks whether the shared library is available or not and supports the option
—without-rpkiThe BGPReader accepts multiple new parameters which allows setting all RPKI options:
--rpkienables the historical RPKI validation with default collector (standalone)--rpki-liveenables the live RPKI validation with a default RTR-Server (standlone)--rpki-collectorsspecifies the collectors used for the RPKI validation (standalone),Format: "((*|project):(*|(collector(,collectors)))(;)?)",
e.g. "PJ_1:CC_1,CC_11;PJ_2:CC_2" or "PJ_1:*;PJ_2:CC_2" or "*:*"
--rpki-unifiedenables an unified validation where the ROA dumps of the different collectors are combined--rpki-sshenables SSH encryption to the RTR Server, format: "username, hostkey_path, privatekey_path"On basis of the parameters the BGPReader creates the ROAFetchlib configuration which is needed for the validation process and stores it in every BGP elem instance
During the output process every BGP elem will be validated and the validation result is added to the regular BGPReader output