Skip to content

Comments

Enip rust 3958 v5#9937

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

Enip rust 3958 v5#9937
catenacyber wants to merge 2 commits intoOISF:masterfrom
catenacyber:enip-rust-3958-v5

Conversation

@catenacyber
Copy link
Contributor

@catenacyber catenacyber commented Dec 1, 2023

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

  • enip_command keyword accepts now string enumeration as values.
  • transactions are now bidirectional
  • there is a enip logger
  • gap support is improved with probing for resync
  • SEQUENCE_ADDR_ITEM value is fixed to 0x8002 instead of 0xB002
  • frames
  • events

#9850 rebased +

  • new 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
  • integer keywords now support hexadecimal notation
  • stringer for logging some enumerations such as vendor id

Draft as is this is not complete but want to get CI impression :

  • parse more + log and keywords for this
  • more S-V tests

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

SV_BRANCH=pr/1485

OISF/suricata-verify#1485

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

- enip_command keyword accepts now string enumeration as values.
- transactions are now bidirectional
- there is a logger
- gap support is improved with probing for resync
- SEQUENCE_ADDR_ITEM value is fixed to 0x8002 instead of 0xB002
- frames support
- app-layer events
- add enip.status keyword
- add identity 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
@catenacyber catenacyber mentioned this pull request Dec 1, 2023
@codecov
Copy link

codecov bot commented Dec 1, 2023

Codecov Report

Merging #9937 (4d9a67f) into master (9c3ab36) will decrease coverage by 0.52%.
The diff coverage is 37.01%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9937      +/-   ##
==========================================
- Coverage   82.35%   81.83%   -0.52%     
==========================================
  Files         972      984      +12     
  Lines      273060   274613    +1553     
==========================================
- Hits       224870   224728     -142     
- Misses      48190    49885    +1695     
Flag Coverage Δ
fuzzcorpus 63.38% <33.96%> (-0.77%) ⬇️
suricata-verify 60.51% <29.79%> (-0.59%) ⬇️
unittests 62.55% <11.97%> (-0.37%) ⬇️

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

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 16818

pub fn AppLayerParserStateIssetFlag(state: *mut c_void, flag: u16) -> u16;
pub fn AppLayerParserSetStreamDepth(ipproto: u8, alproto: AppProto, stream_depth: u32);
pub fn AppLayerParserConfParserEnabled(ipproto: *const c_char, proto: *const c_char) -> c_int;
pub fn AppLayerParserRegisterParserAcceptableDataDirection(ipproto: u8, alproto: AppProto, dir: u8);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it have its own commit despite being just one line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting, why so ?

My logic is to put in the commit, the line using it, ie calling AppLayerParserRegisterParserAcceptableDataDirection from rust

Copy link
Contributor

Choose a reason for hiding this comment

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

Your logic is undoubtedly sound.
However, considering that you're introducing a new function for the app layer, which can be reused in other protocols, I thought that creating a specific commit would provide a clear history in the git log.
This way, it can be easily traced back to its introduction if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The function is not new.

It makes it merely accessible to rust app-layers...

@catenacyber catenacyber mentioned this pull request Dec 1, 2023
@catenacyber
Copy link
Contributor Author

Replaced by #9940

@catenacyber catenacyber closed this Dec 1, 2023
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.

3 participants