Hi Jeremy, sorry for the delay! See my comments below. <snip> > > > Assumptions: > > > Two links between SUT and TG, one link is TG -> SUT, the > > > other SUT -> TG. > > > > > > Args: > > > - packet: The packet to modify. > > > + packets: The packets to modify. > > > expected: If :data:`True`, the direction is SUT -> TG, > > > otherwise the direction is TG -> SUT. > > > """ > > > - if expected: > > > - # The packet enters the TG from SUT > > > - # update l2 addresses > > > - packet.src = self._sut_port_egress.mac_address > > > - packet.dst = self._tg_port_ingress.mac_address > > > + ret_packets = [] > > > + for packet in packets: > > > + default_pkt_src = type(packet)().src > > > + default_pkt_dst = type(packet)().dst > > > > This is really just a probing question for my sake, but what is the > > difference between the solution you have above type(packet)().src and > > Ether().src? Is there a preferred means of doing this? > > There isn't really a functional difference at all under the assumption > that every packet we send will start with an Ethernet header. This > obviously isn't an unreasonable assumption to make, so maybe I was > reaching for flexibility that isn't really needed here by making it > work with any theoretical first layer that has a source address. I > wanted to do the same thing for the payload, but that causes issues > when the following layer with an address isn't the very next layer > after Ether.
Makes sense to me! It's probably best to not to make the Ether assumption regardless of whether or not it will likely always be present. > > > > > > + default_pkt_payload_src = IP().src if > > > hasattr(packet.payload, "src") else None > > > + default_pkt_payload_dst = IP().dst if > > > hasattr(packet.payload, "dst") else None > > > + # If `expected` is :data:`True`, the packet enters the TG > > > from SUT, otherwise the > > > + # packet leaves the TG towards the SUT > > > > > > - # The packet is routed from TG egress to TG ingress > > > - # update l3 addresses > > > - packet.payload.src = self._tg_ip_address_egress.ip.exploded > > > - packet.payload.dst = self._tg_ip_address_ingress.ip.exploded > > > > This is where it gets a little tricky. There will be circumstances, > > albeit probably infrequently, where a user-created packet has more > > than one IP layer, such as the ones I am using in the ipgre and nvgre > > test suites that I am writing. In these cases, you need to specify an > > index of the IP layer you want to modify, otherwise it will modify the > > outermost IP layer in the packet (the IP layer outside the GRE layer. > > See my previous comment for an example packet). Should be pretty easy > > to fix, you just need to check if a packet contains an GRE layer, and > > if it does, modify the packet by doing something like > > packet[IP][1].src = self._tg_ip_address_egress.ip.exploded. > > I'm not as familiar with how GRE affects the packets, do you need to > have the address on the inner IP layer at all times, or are you saying > you need it on both IP layers? Basically, GRE is a header that encapsulates a traditional packet. Practically speaking, this means that a scapy packet with GRE will look something like 'Ether() / IP() / GRE() / IP() / UDP() / Raw()'. If you try to modify layer 3 addresses in the way the framework does it now (packet.payload.src), and more than one IP layer is present in a given packet, it will modify the the front-most IP layer (in this case, the IP layer before the GRE layer is the packet I listed before). If there are multiple IP layers, you can choose which layer you want to modify by doing something like 'packet[IP][1] = address' to modify the inner IP layer. It is my understanding that GRE packets need to have an inner IP layer as well as an outer IP layer. Here is a quick readup on what GRE is (scroll to the bottom of the article and look at the diagram of a regular datagram vs a GRE datagram as the rest of the article isn't super important). https://ipwithease.com/generic-routing-encapsulation-gre/ <snip> -Nicholas
