client changes for packetIO

[satish153]

PacketIO feature is enabled by the command: $IPDK_RECIPE/install/bin/p4rt-ctl start-pktio br0

Created a TAP port to enable testing of packetIO feature.

In order to receive packets asynchronously, the p4rt-ctl must continue running and actively poll for incoming packets. To achieve this, an infinite loop is introduced within the pktio_rx function. Packets received are sent to the TAP port and can verified.

In order to test PacketOut, a thread is created to poll for packets on the TAP port. Packets received are sent to the server. With the appropriate forwarding rule, these packets can be verified on the partner device.

Limitation:
During packetIO, p4rt-ctl client remains connected to the server so other clients cannot connect. The current client needs to exit to do any other operations.

[ffoulkes]

Please note that Bandit reported a high-priority security issue in your changes.
This should be addressed.

[ffoulkes]

Also, our policy is that all commits need to be signed.

You can do this by specifying -s on your git commit command line.

If you fail to do so, your commit will be blocked by the DCO checker. I can override this on merge, but I'd rather not have to.

[satish153]

I tried other ways of enabling the TAP port but still facing the security issue. Removed the enabling part from the script. The user has to manually enable the port to see the packets on the TAP port.

The Bandit issue is:

>> Issue: [B605:start_process_with_a_shell] Starting a process with a shell, possible injection detected, security issue.
   Severity: High   Confidence: High
   CWE: CWE-78 (https://cwe.mitre.org/data/definitions/78.html)
   More Info: https://bandit.readthedocs.io/en/1.7.5/plugins/b605_start_process_with_a_shell.html
   Location: ./ipdk.recipe/clients/p4rt-ctl/p4rt-ctl.in:810:12
809	            # Bring the TAP interface up.
810	            os.system(f"sudo ip link set dev {tap_name} up")

The warning is about the possibility of command injection -- the interpolation of a possibly unchecked value ({tap_name}) into a command string. Command injection is a common source of security vulnerabilities.

The usual mitigations are (1) to check the value before you use it, or (2) to construct the value your self.

My suggestion would be to pass just the numeric component when you instantiate TapDevice, and then build up the string yourself.

tap_name="pktioTap{num:d}".format(num=tap_no)

If that doesn't work, try validating tap_no before you use it.

if tap_no < 0 or tap_no > 255:
  raise ValueError("TAP number {%u} is invalid" % tap_no)

One final suggestion: if I remember correctly, current thinking is that constructors should not be able to fail. Assuming that's the case, you move the active initialization to a separate method; then instantiate the object and invoke the method:

class TapDevice():
  def __init__(self, tap_no):
    self.tap_no = tap_no

  def open(self):
    if self.tap_no < 0 or self.tap_no > 255:
      raise ValueError("TAP number ({}) is out of range".format(self.tap_no))
    tap_name = "pktioTap{}".format(self.tap_no)
    print("tap_name: {}".format(tap_name))

tap_device = TapDevice(0)
tap_device.open()

YMMV. I don't know Bandit well enough to know what it takes to appease its checkers. If push comes to shove, (1) write your code so that it demonstrably defends itself against command injection, (2) verify that the defense works, and then (3) add whatever directive the static analyzer provides to disable this particular check on as small a portion of your code as you can. I'd also (4) add a brief explanatory comment so the maintainer knows that Bandit complained despite your attempts to appease it, and then (5) file a ticket with the folks responsible for Bandit, since it would appear that you've found a false positive.

[ffoulkes]

Another suggestion: instead of force-pushing your modifications, just keep adding and pushing commits. You can clean up by doing a squash and merge after the approval.

The immediate advantage is that you don't wipe out your prior commits while the PR is still in progress.

When I went back to respond to your comment, I discovered that there was no Bandit report for me to consult (so I could provide an analysis of the error message), and the original code was gone (so I couldn't review the issue in situ). Fortunately, (1) the log of the earlier run was still available on the GitHub Actions page, and (2) I had done a git fetch after your original commit and hadn't done another since then, so the original code was present in my local git repository. Having the original code to review (and possibly play with) is a big plus when diagnosing issues.

(There might be a forensic trick for recovering deleted history from GitHub, but I don't know it.)

[5abeel]

LGTM

[n-sandeep]

When manifest is updated with this SHA, please update ACC scripts as well to include fcntl and select python packages

[satish153]

ok

[nupuruttarwar]

Minor suggestions. Logic LGTM

[nupuruttarwar]

Do we want to have a print statement inside a continuous poll function? This will spam and slow down

[satish153]

Are you asking about print("Waiting for Rx packets...")? This line is outside the while loop which checks for packets continuously.

Output of start-pktio looks like below:

[root@CP-1049136 ipdk_recipe]# $IPDK_RECIPE/install/bin/p4rt-ctl start-pktio
br0
Created TAP device pktioTap0
Tx thread polling on TAP port
Waiting for Rx packets...

[nupuruttarwar]

Can we use a macro to define these contstants instead and add a comment that it's from linux/if_tun.h? https://peps.python.org/pep-0591/

[nupuruttarwar]

same comment applicable for all the values below

[satish153]

As I understand, uppercase variable names are used to indicate that a variable's value should be treated as constant. I understand the variable is not truly immutable but that's the convention used to declare consts.

Thanks for the reference on using 'Final'. I now, learnt that there is a way to enforce immutability. However, it involves importing an additional module which might be a overkill in this specific case.

[n-sandeep]

LGTM