nfsv2: implement WRITE procedure support.

[kaddujames501-ship-it]

Issue #4946
Make sure these boxes are checked accordingly before submitting your Pull Request -- thank you.

Contribution style:

• I have read the contributing guide lines at
  https://docs.suricata.io/en/latest/devguide/contributing/contribution-process.html

Our Contribution agreements:

• I have signed the Open Information Security Foundation contribution agreement at
  https://suricata.io/about/contribution-agreement/ (note: this is only required once)

Changes (if applicable):

• I have fixed a ticket at
  https://redmine.openinfosecfoundation.org/projects/suricata/issues

Link to ticket: https://redmine.openinfosecfoundation.org/issues/4946

Describe changes:

• Convert unittests to new FAIL/PASS API - stream-tcp-reassemble.c file

Provide values to any of the below to override the defaults.

• To use a Suricata-Verify or Suricata-Update pull request,
  link to the pull request in the respective _BRANCH variable.
• Leave unused overrides blank or remove.

SV_REPO=
SV_BRANCH=OISF/suricata-verify#2729
SU_REPO=
SU_BRANCH=

James Kaddu: [email protected]

[github-actions]

NOTE: This PR may contain new authors.

[victorjulien]

Did this pass your local testing?

[victorjulien]

Should pass NFS SV tests (and all other tests)

[kaddujames501-ship-it]

Should pass NFS SV tests (and all other tests)

I think this is because the NFSv2 implementation is now properly detecting WRITE and READ operations, which trigger multiple alerts.

[kaddujames501-ship-it]

According to what I have found, failing tests I think are false negatives - the code is working correctly, but the test expectations need to be updated to match the improved NFSv2 support. Kindly correct me if I am wrong.

[victorjulien]

According to what I have found, failing tests I think are false negatives - the code is working correctly, but the test expectations need to be updated to match the improved NFSv2 support. Kindly correct me if I am wrong.

Can you explain for each of the failing tests why you believe they are false negatives?

[kaddujames501-ship-it]

According to what I have found, failing tests I think are false negatives - the code is working correctly, but the test expectations need to be updated to match the improved NFSv2 support. Kindly correct me if I am wrong.

Can you explain for each of the failing tests why you believe they are false negatives?

I think, the two (issue-3277-nfsv2-filestore and nfs-udp-only ) were written before NFSv2 WRITE support was fully implemented. They expected only 1 alert because the previous code wasn't detecting the WRITE operations in the PCAP. Now that WRITE support is implemented, the code correctly detects all file transactions in the PCAP.

[victorjulien]

Can you be a bit more specific? Review the test pcaps to confirm the addition WRITE's exist, that Suricata logs them, etc.

Also, please explain the failure in nfs-write-001. I assume you created and tested it locally, but it fails in our CI. Also in your screenshot btw, so something went wrong. Can you explain?

[jufajardini]

In addition to Victor's comments:

• as stated by Philippe on the previous PR, in the commit message, what you do mean by "prepares for further testing and validation"? (cf #14171 (review))
• this PR seems to contain lots of changes that are just formatting changes. Formatting changes that are unrelated to the work done in the commit should either be avoided entirely, or, if not possible, be part of a separate commit, please. This makes it much easier to review, and also ensures commits are semantically related to the changes they bring in.

[kaddujames501-ship-it]

Can you be a bit more specific? Review the test pcaps to confirm the addition WRITE's exist, that Suricata logs them, etc.

Also, please explain the failure in nfs-write-001. I assume you created and tested it locally, but it fails in our CI. Also in your screenshot btw, so something went wrong. Can you explain?

Hello Victor, the delay was to be very sure, and I had to start afresh. Here are the findings and I am requesting for a hand here.

[kaddujames501-ship-it]

In addition to Victor's comments:

* as stated by Philippe on the previous PR, in the commit message, what you do mean by "prepares for further testing and validation"? (cf [#14171 (review)](https://github.com/OISF/suricata/pull/14171#pullrequestreview-3389226618))

* this PR seems to contain lots of changes that are just formatting changes. Formatting changes that are unrelated to the work done in the commit should either be avoided entirely, or, if not possible, be part of a separate commit, please. This makes it much easier to review, and also ensures commits are semantically related to the changes they bring in.

Thanks for the review, the format was about a script I wrote to make sure no formatting errors, but it shouldnt really be so.

[jufajardini]

In addition to Victor's comments:

* as stated by Philippe on the previous PR, in the commit message, what you do mean by "prepares for further testing and validation"? (cf [#14171 (review)](https://github.com/OISF/suricata/pull/14171#pullrequestreview-3389226618))

* this PR seems to contain lots of changes that are just formatting changes. Formatting changes that are unrelated to the work done in the commit should either be avoided entirely, or, if not possible, be part of a separate commit, please. This makes it much easier to review, and also ensures commits are semantically related to the changes they bring in.

Thanks for the review, the format was about a script I wrote to make sure no formatting errors, but it shouldnt really be so.

I understand, but we already have formatting checks, and, in all cases, those should go in separate commits :)

Could you please address Philippe's question, since the commit message for this PR still states the same?

[kaddujames501-ship-it]

requested changes have been addressed in this PR #14251