Skip to content

Conversation

@kaddujames501-ship-it
Copy link

@kaddujames501-ship-it kaddujames501-ship-it commented Nov 2, 2025

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

Contribution style:

Our Contribution agreements:

Changes (if applicable):

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

Screenshot from 2025-11-02 17-08-31

Describe changes:

  • Add NFSv2 WRITE procedure parsing and file transaction handling
  • Improve file transaction completion tracking in mark_response_tx_done
  • Don't prematurely set response_done flag for file transactions
  • Add NFSv2 procedure name detection support in detect.rs
  • Add unit tests for NFSv2 WRITE procedure matching

This enables full NFSv2 WRITE support for file extraction and detection.

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#2736
SU_REPO=
SU_BRANCH=

James Kaddu: [email protected]

of WRITE requests and responses. This enhances NFSv2 protocol coverage

and prepares for further testing and validation.

Fixes: OISF#4946
@victorjulien victorjulien added the outreachy Contributions made by Outreachy applicants label Nov 2, 2025
@github-actions
Copy link

github-actions bot commented Nov 2, 2025

NOTE: This PR may contain new authors.

@codecov
Copy link

codecov bot commented Nov 2, 2025

Codecov Report

❌ Patch coverage is 84.13284% with 43 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.14%. Comparing base (cdd4ea0) to head (42856ad).
⚠️ Report is 48 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14251      +/-   ##
==========================================
- Coverage   84.14%   84.14%   -0.01%     
==========================================
  Files        1013     1013              
  Lines      262313   262539     +226     
==========================================
+ Hits       220733   220910     +177     
- Misses      41580    41629      +49     
Flag Coverage Δ
fuzzcorpus 63.33% <80.08%> (+<0.01%) ⬆️
livemode 18.69% <0.00%> (-0.08%) ⬇️
pcap 44.63% <64.50%> (+0.06%) ⬆️
suricata-verify 64.86% <67.98%> (-0.02%) ⬇️
unittests 59.16% <25.37%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

* 02110-1301, USA.
*/

/* RFC 1094, section '2.2 Server Procedures' */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should reuse #14178 commit and build on top of it

match ctx {
DetectNfsProcedureData::VersionLiteral(ver) => {
if nfs_version < 4 {
if nfs_version == 2 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can he have nfs version 1 ?

if state.nfs_version == 2 && tx.procedure == NFSPROC2_GETATTR {
return 1;
}
if state.nfs_version == 3 && tx.procedure == NFSPROC3_GETATTR {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about NFSPROC4_GETATTR ?

tx.is_file_closed = true;
sc_app_layer_parser_trigger_raw_stream_inspection(flow, Direction::ToClient as i32);
sc_app_layer_parser_trigger_raw_stream_inspection(
flow,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rustfmt should be its own commit

Copy link
Contributor

@catenacyber catenacyber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think commit history needs a bit of rework to split things

Copy link
Contributor

@jufajardini jufajardini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On top of observing Philippe's comments, please ensure that you're following our commit guidelines, with your next iteration :)
https://docs.suricata.io/en/suricata-8.0.1/devguide/contributing/code-submission-process.html#commits

@jufajardini
Copy link
Contributor

Hi there, considering this was a work started during the contribution phase, and that there are a few comments here unanswered for more than a week, I am closing this PR as stale. Thanks for your time and interest in contributing to our project!

@catenacyber catenacyber reopened this Nov 12, 2025
@catenacyber
Copy link
Contributor

I wonder if we should take it over as this is an interesting work beginning...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

outreachy Contributions made by Outreachy applicants

Development

Successfully merging this pull request may close these issues.

4 participants