Skip to content

Comments

Enip rust 3958 v10#10083

Closed
catenacyber wants to merge 2 commits intoOISF:masterfrom
catenacyber:enip-rust-3958-v10
Closed

Enip rust 3958 v10#10083
catenacyber wants to merge 2 commits intoOISF:masterfrom
catenacyber:enip-rust-3958-v10

Conversation

@catenacyber
Copy link
Contributor

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

Describe changes:

  • convert enip parser to rust
  • integer keywords now support hexadecimal notation

Alon the way, also

  • transactions are now bidirectional
  • there is a enip logger
  • gap support is improved with probing for resync
  • frames
  • events
  • enip_command keyword accepts now string enumeration as values.
  • more keywords

#10072 with rust style imroved as suggested per Jason

SV_BRANCH=pr/1521

OISF/suricata-verify#1521

Does the first commit deserve its own redmine ticket ?
And the one in 4a49352 also ?

So that we can write enip.revision: 0x203
Ticket: 3958

- transactions are now bidirectional
- there is a logger
- gap support is improved with probing for resync
- frames support
- app-layer events
- enip_command keyword accepts now string enumeration as values.
- add enip.status keyword
- add keywords :
    enip.product_name, enip.protocol_version, enip.revision,
    enip.identity_status, enip.state, enip.serial, enip.product_code,
    enip.device_type, enip.vendor_id, enip.capabilities,
    enip.cip_attribute, enip.cip_class, enip.cip_instance,
    enip.cip_status, enip.cip_extendedstatus
@codecov
Copy link

codecov bot commented Dec 21, 2023

Codecov Report

Attention: 1998 lines in your changes are missing coverage. Please review.

Comparison is base (5cc872f) 82.19% compared to head (5cf3db0) 81.60%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10083      +/-   ##
==========================================
- Coverage   82.19%   81.60%   -0.60%     
==========================================
  Files         975      994      +19     
  Lines      271940   274306    +2366     
==========================================
+ Hits       223523   223834     +311     
- Misses      48417    50472    +2055     
Flag Coverage Δ
fuzzcorpus 62.08% <33.90%> (-0.85%) ⬇️
suricata-verify 60.70% <40.61%> (-0.73%) ⬇️
unittests 62.33% <12.12%> (-0.52%) ⬇️

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

@victorjulien
Copy link
Member

Yeah, please add tickets for those detect improvements. Will also need doc and SV updates. Might be good to break it out into its own PR.

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 17169

@catenacyber
Copy link
Contributor Author

Yeah, please add tickets for those detect improvements. Will also need doc and SV updates. Might be good to break it out into its own PR.

Tracking ticket https://redmine.openinfosecfoundation.org/issues/6644

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.

Doc changes look good to me! Do I understand correctly that many of the new enip keywords descriptions can later on be updated, once #10122 gets merged?

@catenacyber
Copy link
Contributor Author

Do I understand correctly that many of the new enip keywords descriptions can later on be updated, once #10122 gets merged?

Correct indeed

@catenacyber catenacyber mentioned this pull request Jan 16, 2024
@catenacyber
Copy link
Contributor Author

Replaced by #10178

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants