search

Exa-Networks / exabgp

The BGP swiss army knife of networking
Other
2.06k stars 441 forks source link

Flowspec flags incorrectly returned on API #1155

Closed adrian62 closed 1 year ago

adrian62 commented 1 year ago

Flowspec allows flag matches (TCP and fragment) and the encoding is described in RFC 5575. We are looking specifically at Type 9 - TCP Flags but the issue applies to fragments also.

Say we configure a rule matching two flags with the match bit set. In exabgp flow configuration we have:

tcp-flags [ =fin+push ];

Per the RFC this is encoded as 3 bytes, the type (9) and one operator byte and one flag bit mask byte:

09 81 09

Exabgp will correctly send this match to the peer, but on the receiving end (also exabgp) the API delivers:

'tcp-flags': ['=fin', '=push'], 'string': 'flow destination-ipv4 0.0.1.16/28 tcp-flags =fin+push'

The flag values in 'tcp-flags' key is incorrect because that would be encoded in a pair of operator/bitmask octets and would match the flags individually not together. However in the 'string' key they are correct.

To Reproduce

Set up a pair of exabgp to peer with each other; Configure the first instance to send a flowspec route with a TCP flag match like the one described above; Read updates from the json API on the receiving exabgp.

Expected behavior

The tcp-flags key should contain the correct flags match on receive instance,

Environment (please complete the following information):

Additional context

The problem applies equally to expression without the match bit set and also regardless whether the not bit is set or not. And as mentioned it applies to fragment flags too which suggests that the same code is reused for both types of matches, always the correct way to develop! Thank you!

thomas-mangin commented 1 year ago

Thank you, there was an error with the generation.

thomas-mangin commented 1 year ago

@adrian62 if you can and want to share your linuxkit configuration, I would gladly add it to the repository.

adrian62 commented 1 year ago

  1. thank you for the exceptionally quick resolution of this issue

  2. I don't build the exabgp containers with linuxkit, but I guess that the image I use (python-3.8-alpine linux official image from the python project) is built with linuxkit, hence the marking in the kernel name.

  3. I looked at your patch and I noticed something unrelated to this issue but -incidentally- of interest to me: lines 618, 619 shown here:

                if rule.ID == 0x0C and s[-1]:
                     s[-1] = s[-1].replace('!is-fragment', rule.value.names.get(rule.value.NOT))

    In my use of exabgp I receive flowspec updates from the json API and I had to add exception code to revert 'not-a-fragment' to '!is-fragment' such that the code used to interpret flag expressions be the same for all flags. Could you please tell me why is-fragment name has its negated complement not-a-fragment while other flags do not? Just want to understand if I could safely avoid it.

thomas-mangin commented 1 year ago

Hello @adrian62 - I could not recall why this rule is there and that is why while working on the patch I did not touch it.

Looking into it, the reason is that fragments have the following options:

class Fragment(BitResource):
    NAME = 'fragment'

    NOT = 0x00
    DONT = 0x01
    IS = 0x02
    FIRST = 0x04
    LAST = 0x08
    # reserved = 0xF0

    codes = dict(
        (k.lower().replace('_', '-'), v)
        for (k, v) in {
            'NOT-A-FRAGMENT': NOT,
            'DONT-FRAGMENT': DONT,
            'IS-FRAGMENT': IS,
            'FIRST-FRAGMENT': FIRST,
            'LAST-FRAGMENT': LAST,
        }.items()
    )

So it allows the name display to be a valid option for setting the fragment. I do not believe it applies otherwise.

But it may have been a misguided attempt to make things better. I should have a bit of time in the coming weeks to close remaining open issues and look into this more.

adrian62 commented 1 year ago

So not-a-fragment is a pseudo-flag to mean no flag is set? To the best of my understanding the RFC defines only the latter 4 flags -- which correspond to bits set in your above dict.

While doing more tests of various flag combinations I ran into a similar issue as the one originally reported:

When announcing: 03:17:48 | 10 | api | flow added to neighbor 0.0.0.2 local-ip 198.18.21.60 local-as 100 peer-as 100 router-id 198.18.21.60 family-allowed in-open : flow source-ipv6 2002:face:100e::4567:991a/128/0 tcp-flags [ ack+cwr&!fin+ece ]

I receive on the other end:

INFO:BRID2<BRID2>:BRID2------->> BGPIn RAW: update:[{'type': 'update', 'timestamp': 1681787048.0236416, 'direction': 'receive', 'neighbor': '198.18.21.60', 'origin': 'igp', 'local-preference': 100, 'operation': 'announce', 'family': 'ipv6 flow', 'next-hop': 'no-nexthop', 'source-ipv6': ['2002:face:100e::4567:991a/128/0'], 'tcp-flags': ['ack', 'cwr&!fin', '&!ece'], 'string': 'flow source-ipv6 2002:face:100e::4567:991a/128/0 tcp-flags [ ack+cwr&!fin+ece ]}]

The tcp-flags key is incorrect while the string equivalent is as expected. This is with the patch applied which in my build I stamped as version 4.2.21-1155 to make sure I can tell it apart from the original 4.2.21 before the fix back port.

thank You

thomas-mangin commented 1 year ago

I can not recall what the code has options for is and not but looking at rfc 8955, I can see your point. I also copied the flags in the file so I am unclear on why I thought it was a good idea !

                   0   1   2   3   4   5   6   7
                 +---+---+---+---+---+---+---+---+
                 | 0 | 0 | 0 | 0 |LF |FF |IsF|DF |
                 +---+---+---+---+---+---+---+---+

It may have a misguided understanding. So this should be removed. Thank you for pointing it out.

I will also check the tcp flags

thomas-mangin commented 1 year ago 
neighbor 127.0.0.1 {
    router-id 1.2.3.4;
    local-address 127.0.0.1;
    local-as 1;
    peer-as 1;
    group-updates false;

    family {
        ipv4 flow;
        ipv4 flow-vpn;
        ipv6 flow;
    }

    flow {
        route give-me-a-name {
            match {
                tcp-flags [ ack cwr&!fin&!ece ];
            }
            then {
                discard;
            }
        }
    }
}

with a test of

update ipv4 flow
FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF:0042:02:0000002B4001010040020040050400000064C010088006000000000000800E0F00018500000909001000804201C240
{ "exabgp": "5.0.0", "time": 1681807751.949034, "host" : "MacBook-Pro-2.local", "pid" : 4499, "ppid" : 4496, "counter": 1, "type": "update", "neighbor": {     "address": { "local": "127.0.0.1", "peer": "127.0.0.1" },     "asn": { "local": 65533, "peer": 65533 }     , "direction": "in", "message": { "update": { "attribute": { "origin": "igp", "local-preference": 100, "extended-community": [ { "value": 9225060886715039744, "string": "rate-limit:0" } ] }, "announce": { "ipv4 flow": { "no-nexthop": [ { "tcp-flags": [ "ack", "cwr&!fin&!ece" ], "string": "flow tcp-flags [ ack cwr&!fin&!ece ]" } ] } } } } } }

and it seems to work as expected. The test is at https://github.com/Exa-Networks/exabgp/blob/main/qa/json/bgp-flow-3

and the check is

./qa/bin/functional decoding
adrian62 commented 1 year ago

Thanks, your test case uses TCP flags match ack cwr&!fin&!ece which correctly encodes to the 4 pairs of [op, bit mask] pairs on the wire preceded by the type byte (09): 09 0010 0080 4201 c240 So note that there is a space between ack and cwr flags which means they are in separate bit mask groups.

My example above has a + between ack and cwr. Exabgp correctly encodes that into three pairs of [op,bitmask] on the wire: 09 0090 4201 c240

But on the receiving end the API reports the match as: 'tcp-flags': ['ack', 'cwr&!fin&!ece']

Which is incorrect. However, if we inspect the string key from the API, the match for tcp-flags is correct there:

'string': 'flow tcp-flags [ ack+cwr&!fin&!ece ]'

This issue appears very similar to the one originally reported. Perhaps there is another codepath taken for other operators?

thomas-mangin commented 1 year ago

I tested on 4.2 HEAD and main HEAD, also added a test in master too - for good measure and I can not reproduce I get for:

exabgp 4.2

12:53:30 | 55256  | configuration | > match            | 
12:53:30 | 55256  | configuration | . tcp-flags        | '[' 'ack+cwr&!fin+ece' ']'

{ "exabgp": "4.0.1", "time": 1681905210.646323, "host" : "MacBook-Pro-2.local", "pid" : 55256, "ppid" : 37930, "counter": 1, "type": "update", "neighbor": { "address": { "local": "127.0.0.1", "peer": "127.0.0.1" }, "asn": { "local": 1, "peer": 1 } , "direction": "in", "message": { "update": { "attribute": { "origin": "igp", "local-preference": 100, "extended-community": [ { "value": 9225060886715039744, "string": "rate-limit:0" } ] }, "announce": { "ipv4 flow": { "no-nexthop": [ { "tcp-flags": [ "ack+cwr&!fin+ece" ], "string": "flow tcp-flags [ ack+cwr&!fin+ece ]" } ] } } } } } }

main

12:56:24 55522  rib             insert flow tcp-flags [ ack+cwr&!fin+ece ] extended-community rate-limit:0
./sbin/exabgp decode FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF:003E:02:000000274001010040020040050400000064C010088006000000000000800E0B000185000005090090C241
{ "exabgp": "5.0.0", "time": 1681905452.011939, "host" : "MacBook-Pro-2.local", "pid" : 55717, "ppid" : 50442, "counter": 1, "type": "update", "neighbor": {     "address": { "local": "127.0.0.1", "peer": "127.0.0.1" },     "asn": { "local": 65533, "peer": 65533 }     , "direction": "in", "message": { "update": { "attribute": { "origin": "igp", "local-preference": 100, "extended-community": [ { "value": 9225060886715039744, "string": "rate-limit:0" } ] }, "announce": { "ipv4 flow": { "no-nexthop": [ { "tcp-flags": [ "ack+cwr&!fin+ece" ], "string": "flow tcp-flags [ ack+cwr&!fin+ece ]" } ] } } } } } }

but the encoding is from ExaBGP for this flow is 09 0090 C241 and not 09 0090 4201 c240

Can you please send me a copy of the whole UPDATE so I can see how it is decoded.

adrian62 commented 1 year ago

The difference you see in the encoding is because you have used a slightly different TCP flag match: My match is [ ack+cwr&!fin&!ece ] Your match is [ ack+cwr&!fin+ece ]

But the issue on my end is the same with both, I have tried the same with your match, see below:

Sender exabgp: 13:42:11 | 10 | outgoing-1 | sending TCP payload ( 62) FFFF FFFF FFFF FFFF FFFF FFFF FFFF FFFF 003E 0200 0000 2740 0101 0040 0200 4005 0400 0000 64C0 1008 8006 0000 0000 0000 800E 0B00 0185 0000 0509 0090 C241

Receiver exabgp: INFO:BRID2<BRID2>:BRID2------->> BGPIn RAW: jsonUpdate:b'{ "exabgp": "4.0.1", "time": 1681912669.6636498, "host" : "BRID2", "pid" : 10, "ppid" : 7, "counter": 25, "type": "update", "neighbor": { "address": { "local": "198.18.21.62", "peer": "198.18.21.30" }, "asn": { "local": 100, "peer": 100 } , "direction": "receive", "message": { "update": { "attribute": { "origin": "igp", "local-preference": 100, "extended-community": [ { "value": 9225060886715039744, "string": "rate-limit:0" } ] }, "announce": { "ipv4 flow": { "no-nexthop": [ { "tcp-flags": [ "ack", "cwr&!fin", "&!ece" ], "string": "flow tcp-flags [ ack+cwr&!fin+ece ]" } ] } } } } } }\n'

Note on the receiver side the tcp-flags key versus the tcp-flags in the 'string' key. Just to be clear, I use exabgp 4.2.21 because I believe that you backported the fix to it. Will try main later today/ Thank you.

thomas-mangin commented 1 year ago 
./sbin/exabgp decode FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF:0040:02:000000294001010040020040050400000064C010088006000000000000800E0D0001850000070900904201C240
{ "exabgp": "5.0.0", "time": 1681926212.2622669, "host" : "MacBook-Pro-2.local", "pid" : 23740, "ppid" : 2277, "counter": 1, "type": "update", "neighbor": {     "address": { "local": "127.0.0.1", "peer": "127.0.0.1" },     "asn": { "local": 65533, "peer": 65533 }     , "direction": "in", "message": { "update": { "attribute": { "origin": "igp", "local-preference": 100, "extended-community": [ { "value": 9225060886715039744, "string": "rate-limit:0" } ] }, "announce": { "ipv4 flow": { "no-nexthop": [ { "tcp-flags": [ "ack+cwr&!fin&!ece" ], "string": "flow tcp-flags [ ack+cwr&!fin&!ece ]" } ] } } } } } }
./sbin/exabgp decode "FFFF FFFF FFFF FFFF FFFF FFFF FFFF FFFF 003E 0200 0000 2740 0101 0040 0200 4005 0400 0000 64C0 1008 8006 0000 0000 0000 800E 0B00 0185 0000 0509 0090 C241"
{ "exabgp": "5.0.0", "time": 1681926248.969876, "host" : "MacBook-Pro-2.local", "pid" : 23814, "ppid" : 2277, "counter": 1, "type": "update", "neighbor": {     "address": { "local": "127.0.0.1", "peer": "127.0.0.1" },     "asn": { "local": 65533, "peer": 65533 }     , "direction": "in", "message": { "update": { "attribute": { "origin": "igp", "local-preference": 100, "extended-community": [ { "value": 9225060886715039744, "string": "rate-limit:0" } ] }, "announce": { "ipv4 flow": { "no-nexthop": [ { "tcp-flags": [ "ack+cwr&!fin+ece" ], "string": "flow tcp-flags [ ack+cwr&!fin+ece ]" } ] } } } } } }
./sbin/exabgp decode FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF:003E:02:000000274001010040020040050400000064C010088006000000000000800E0B000185000005090090C241
{ "exabgp": "5.0.0", "time": 1681926421.898805, "host" : "MacBook-Pro-2.local", "pid" : 24106, "ppid" : 2277, "counter": 1, "type": "update", "neighbor": {     "address": { "local": "127.0.0.1", "peer": "127.0.0.1" },     "asn": { "local": 65533, "peer": 65533 }     , "direction": "in", "message": { "update": { "attribute": { "origin": "igp", "local-preference": 100, "extended-community": [ { "value": 9225060886715039744, "string": "rate-limit:0" } ] }, "announce": { "ipv4 flow": { "no-nexthop": [ { "tcp-flags": [ "ack+cwr&!fin+ece" ], "string": "flow tcp-flags [ ack+cwr&!fin+ece ]" } ] } } } } } }

Please ensure you do not have multiple versions of exabgp (like pip and a local install or a packaged version), and try to run the code by using ./sbin/exabgp in a new cloned repository as the bug is fixed when I run it.

adrian62 commented 1 year ago

I think I misinterpreted where the fix was back ported to and I had cloned branch 4.2.21 --which is a tag but option -b to git clone can be a tag or a branch... Not sure why I was able to confirm the fix that way, but in any case, after cloning again branch 4.2 (HEAD) I don't see the issue, so you can close back and sorry for the detour. I see above exabgp 5.0, i assume that is master branch: Is there enough life left in branch 4.x or we need to start planning for migration? thanks

thomas-mangin commented 1 year ago

4.2 is the stable branch. However master should also be stable and I do not always backport fixes when they could break running code.

adrian62 commented 1 year ago

Thank you.