bgpd: fix ecommunity parsing for AS4 - #13181
Conversation
9d2c98e to
b125513
Compare
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-10545/ This is a comment from an automated CI system. |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests Ubuntu 18.04 amd64 part 9: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO9U18AMD64-10546/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 9 Topotests Ubuntu 18.04 amd64 part 7: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO7U18AMD64-10546/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 7 Successful on other platforms/tests
|
|
ci:rerun |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests Ubuntu 18.04 arm8 part 6: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 6: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-10554/artifact/TOPO6U18ARM8/TopotestDetails/ Topotests Ubuntu 18.04 arm8 part 6: No useful log foundTopotests Ubuntu 18.04 i386 part 3: Failed (click for details)Topotests Ubuntu 18.04 i386 part 3: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-10554/artifact/TOPO3U18I386/TopotestDetails/Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO3U18I386-10554/test Topology Tests failed for Topotests Ubuntu 18.04 i386 part 3 Successful on other platforms/tests
|
ton31337
left a comment
ton31337
left a comment
There was a problem hiding this comment.
Can we add a topotest to cover this case as well? Other than that, LGTM.
| as = strtoul(buf, &endptr, 10); | ||
| if (*endptr != '\0' || as == BGP_AS4_MAX) | ||
| tmp_as = strtoull(buf, &endptr, 10); | ||
| if (*endptr != '\0' || tmp_as > BGP_AS4_MAX) |
There was a problem hiding this comment.
One question though: should ecommunity work with asdot/asdot+ formats? :)
There was a problem hiding this comment.
I'm not sure if/how asdot works - I haven't examined any of those codepaths. The existing soo CLI doesn't look like it interprets x.y:z as 4-byte:2-byte encoding, even with the as-notation dot config in place:
ub20(config)# router bgp 1 as-notation dot
ub20(config-router)# neighbor 1.1.1.1 remote-as external
ub20(config-router)# add ipv4 uni
ub20(config-router-af)# neighbor 1.1.1.1 soo 16.16:1
ub20(config-router-af)# do sh run | include soo
neighbor 1.1.1.1 soo 16.0.0.16:1 <<<<< parser assumes "16.16" is an ipv4 address since it includes "."
This PR isn't attempting to bridge that gap, just to fix a bug in the existing functionality.
I think adding parser functionality for dot/dot+ notation should probably come via a separate PR where the time/effort has been spent to cover all the cases for the as-dot feature.
There was a problem hiding this comment.
No, it was just a question (for discussion) 😄
There was a problem hiding this comment.
yeah... I'm not really sure how asdot notation is supposed to work wrt to ecommunity parsing/encoding.
I'd guess this same function would need some work, since this is where we do string -> eval conversion... 🤷
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-10554/ This is a comment from an automated CI system. |
The parser for extended communities was incorrectly disallowing an operator from configuring "Route Origin" extended communities (e.g. RD/RT/SoO) with a 4-byte value matching BGP_AS4_MAX (UINT32_MAX) and allowed the user to overflow UINT32_MAX. This updates the parser to read the value as a uint64_t so that we can do proper checks on the upper bounds (> BGP_AS4_MAX || errno). before: ``` TORC11(config-router-af)# neighbor uplink-1 soo 4294967296:65 TORC11(config-router-af)# do sh run | include soo neighbor uplink-1 soo 0:65 TORC11(config-router-af)# neighbor uplink-1 soo 4294967295:65 % Malformed SoO extended community TORC11(config-router-af)# ``` after: ``` TORC11(config-router-af)# neighbor uplink-1 soo 4294967296:65 % Malformed SoO extended community TORC11(config-router-af)# neighbor uplink-1 soo 4294967295:65 TORC11(config-router-af)# do sh run | include soo neighbor uplink-1 soo 4294967295:65 TORC11(config-router-af)# ``` Signed-off-by: Trey Aspelund <taspelund@nvidia.com>
Signed-off-by: Trey Aspelund <taspelund@nvidia.com>
b125513 to
f615e6d
Compare
|
Added a test + tweaked the commit to 1) also update the possible int truncation for ecommunity6 case, 2) go back to |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-10606/ This is a comment from an automated CI system. |
|
@Mergifyio backport stable/8.5 stable/8.4 |
✅ Backports have been createdDetails
|
bgpd: fix ecommunity parsing for AS4 (backport #13181)
bgpd: fix ecommunity parsing for AS4 (backport #13181)
5 participants
The parser for extended communities was incorrectly disallowing an operator from configuring "Route Origin" extended communities (e.g. RD/RT/SoO) with a 4-byte value matching BGP_AS4_MAX (UINT32_MAX) and allowed the user to overflow UINT32_MAX:
This updates the parser to read the value as a uint64_t so that we can do proper checks on the upper bounds (> BGP_AS4_MAX):