Detect origin keyword 6794 v3 #11787

scottfgjordan · 2024-09-17T18:58:12Z

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 updated the User Guide (in doc/userguide/) to reflect the changes made
I have created a ticket at
https://redmine.openinfosecfoundation.org/projects/suricata/issues

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

Describe changes:

Amended git commits to correct author typo
Rebased so that fuzzers can run

Add option of role to live device configuration. Possible roles are : trusted, untrusted, and unknown. Configurable via suricata.yaml. AF-Packet and DPDK runmodes supported, others default to unknown role.

Allows for matching against packets based on the role of the live device. The origin of a packet refers to the role of the live device where the flow originated from.

Adds the copy device to the live device structure. Valid only in IPS mode.

Allows for matching against packets based on the role of the copy live device. For IPS only. The destination of a packet refers to the role of the copy live device.

codecov · 2024-09-17T19:11:29Z

Codecov Report

Attention: Patch coverage is 90.59829% with 44 lines in your changes missing coverage. Please review.

Project coverage is 82.57%. Comparing base (d3eb656) to head (43bb899).
Report is 87 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11787      +/-   ##
==========================================
+ Coverage   82.53%   82.57%   +0.03%     
==========================================
  Files         919      921       +2     
  Lines      248979   249432     +453     
==========================================
+ Hits       205506   205971     +465     
+ Misses      43473    43461      -12

Flag	Coverage Δ
fuzzcorpus	`60.31% <13.46%> (-0.02%)`	⬇️
livemode	`18.72% <35.89%> (+0.01%)`	⬆️
pcap	`44.13% <13.46%> (-0.02%)`	⬇️
suricata-verify	`61.84% <13.46%> (-0.04%)`	⬇️
unittests	`59.06% <83.76%> (+0.06%)`	⬆️

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

jufajardini

Fuzzers checks are ok! /o/

We should also add documentation for the new rule keywords, including rule examples and also their limitations. Not sure what that section should be titled, though. Role keywords?

nit-ish: could you please add the ticket number at the end of the related commit messages?

After the commit body, a

Task #6794

should do :)

jufajardini · 2024-09-17T20:28:16Z

src/util-device.c

+int LiveRegisterDeviceNameAndRole(const char *dev, const char *copy_dev, const char *role)
+{
+    LiveDeviceName *pd = NULL;


Can't we try to dedup the code in these two functions? their structures are so similar...

Yeah that's a good idea. I will take a look cleaning this up.

jufajardini · 2024-09-17T20:38:24Z

doc/userguide/setting-up-ipsinline-for-linux.rst

+in IPS mode as device tenancy is not supported. Currently only
+supported when running with afpacket or dpdk. For example:
+
+::
+  dpdk:
+    interfaces:
+      - interface: 0000:3b:00.1
+        role: trusted


I see that in the suricata.yaml file this setting can be configured in several places. Is there a master config setting place that applies to all of them? If so, I think that should be mentioned here. If not, I think that we should point out all places where this option can be defined, since not defining it results in role being unknown and could lead to user confusion.

I don't believe that there would be a master config as it is device / interface / capture mode specific. I have only been using DPDK & af-packet, but I suppose that wherever an interface is defined and a LiveDevice is created the role could be defined. I think I had covered all the potential spots where the role could be defined in suricata.yaml.in, but I will confirm.

I think that the configuration file itself is covered, my comment was more to list here all such cases, so that people know this from the documentation, what do you think?

jufajardini · 2024-09-17T21:40:28Z

src/detect-destination.c

+            "(trusted/untrusted) of the interface.";
+    sigmatch_table[DETECT_DESTINATION].Match = DetectDestinationMatch;


once we have the keyword docs, then we can add its URL reference here with url attribute

scottfgjordan · 2024-09-19T14:51:48Z

Fuzzers checks are ok! /o/

We should also add documentation for the new rule keywords, including rule examples and also their limitations. Not sure what that section should be titled, though. Role keywords?

nit-ish: could you please add the ticket number at the end of the related commit messages?

After the commit body, a
Task #6794
should do :)

Sounds good! I will add some documentation for the keywords. I didn't know what to call the section either 😄 I'm happy to go with your suggestion.

jlucovsky

I'd really like to see

suricata-verify tests
The use cases for this.

jlucovsky · 2024-09-20T14:47:09Z

src/util-device.c

 *
 *  \retval 0 on success.
 *  \retval -1 on failure.
 */
-int LiveRegisterDeviceName(const char *dev)
+int LiveRegisterDeviceName(const char *dev, const char *copy_dev)


I'd suggest adding a new function ... e.g., LiveRegisterDeviceNameWithCopy and remove the changes to the source/runmode for the packet capture code. It's a bit complicated and one was missed (ipfw)

jlucovsky · 2024-09-20T14:47:59Z

src/util-device.c

 *
 *  \retval 0 on success.
 *  \retval -1 on failure.
 */
-int LiveRegisterDevice(const char *dev)
+int LiveRegisterDevice(const char *dev, const char *role)


Similarly, suggest leaving this interface alone and adding LiveRegisterDeviceWithRole.

scottfgjordan · 2024-09-23T18:08:54Z

I'd really like to see

suricata-verify tests

The use cases for this.

Thanks for the feedback!

I will include suricata-verify tests in the next PR.
The motivation behind this comes from running in IPS mode. I wanted a way to tie a signature to a specific device/interface. With the idea that traffic could be blocked or allowed depending on the device/interface that a flow originated from. Multi-tenancy seemed like it could be a good fit, but wasn't supported in IPS mode. The origin/destination keywords alongside the role configuration gave a nice way to achieve the same goal. It also simplifies the ruleset a bit in comparison to multi-tenancy as there is no need for splitting up rulesets specific to a tenant, you can just include the origin/destination keywords and configure your device roles instead.

jufajardini · 2024-09-30T18:15:33Z

I'd really like to see

suricata-verify tests

The use cases for this.

Thanks for the feedback!

1. I will include suricata-verify tests in the next PR.

2. The motivation behind this comes from running in IPS mode. I wanted a way to tie a signature to a specific device/interface. With the idea that traffic could be blocked or allowed depending on the device/interface that a flow originated from. Multi-tenancy seemed like it could be a good fit, but wasn't supported in IPS mode. The origin/destination keywords alongside the role configuration gave a nice way to achieve the same goal. It also simplifies the ruleset a bit in comparison to multi-tenancy as there is no need for splitting up rulesets specific to a tenant, you can just include the origin/destination keywords and configure your device roles instead.

I think that some of this can be used for the keywords docs, or something that covers this :)

scottfgjordan added 5 commits September 17, 2024 12:11

device: add role to live device

4ef7b17

Add option of role to live device configuration. Possible roles are : trusted, untrusted, and unknown. Configurable via suricata.yaml. AF-Packet and DPDK runmodes supported, others default to unknown role.

detect: implement origin keyword

9f0b85d

Allows for matching against packets based on the role of the live device. The origin of a packet refers to the role of the live device where the flow originated from.

device: add copy device to live device

f13461d

Adds the copy device to the live device structure. Valid only in IPS mode.

detect: implement destination keyword

e904686

Allows for matching against packets based on the role of the copy live device. For IPS only. The destination of a packet refers to the role of the copy live device.

doc: Add description for interface role

43bb899

scottfgjordan requested review from jufajardini, victorjulien and a team as code owners September 17, 2024 18:58

scottfgjordan mentioned this pull request Sep 17, 2024

Detect origin keyword 6794 v2 #11770

Closed

4 tasks

jufajardini requested changes Sep 17, 2024

View reviewed changes

jlucovsky reviewed Sep 20, 2024

View reviewed changes

inashivb added the needs rebase Needs rebase to master label Oct 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect origin keyword 6794 v3 #11787

Detect origin keyword 6794 v3 #11787

scottfgjordan commented Sep 17, 2024

codecov bot commented Sep 17, 2024 •

edited

Loading

jufajardini left a comment •

edited

Loading

jufajardini Sep 17, 2024 •

edited

Loading

scottfgjordan Sep 19, 2024

jufajardini Sep 17, 2024

scottfgjordan Sep 19, 2024

jufajardini Sep 30, 2024

jufajardini Sep 17, 2024

scottfgjordan commented Sep 19, 2024 •

edited

Loading

jlucovsky left a comment

jlucovsky Sep 20, 2024

jlucovsky Sep 20, 2024

scottfgjordan commented Sep 23, 2024

jufajardini commented Sep 30, 2024

		"(trusted/untrusted) of the interface.";
		sigmatch_table[DETECT_DESTINATION].Match = DetectDestinationMatch;

Detect origin keyword 6794 v3 #11787

Are you sure you want to change the base?

Detect origin keyword 6794 v3 #11787

Conversation

scottfgjordan commented Sep 17, 2024

Contribution style:

Our Contribution agreements:

Changes (if applicable):

codecov bot commented Sep 17, 2024 • edited Loading

Codecov Report

jufajardini left a comment • edited Loading

Choose a reason for hiding this comment

jufajardini Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

scottfgjordan Sep 19, 2024

Choose a reason for hiding this comment

jufajardini Sep 17, 2024

Choose a reason for hiding this comment

scottfgjordan Sep 19, 2024

Choose a reason for hiding this comment

jufajardini Sep 30, 2024

Choose a reason for hiding this comment

jufajardini Sep 17, 2024

Choose a reason for hiding this comment

scottfgjordan commented Sep 19, 2024 • edited Loading

jlucovsky left a comment

Choose a reason for hiding this comment

jlucovsky Sep 20, 2024

Choose a reason for hiding this comment

jlucovsky Sep 20, 2024

Choose a reason for hiding this comment

scottfgjordan commented Sep 23, 2024

jufajardini commented Sep 30, 2024

codecov bot commented Sep 17, 2024 •

edited

Loading

jufajardini left a comment •

edited

Loading

jufajardini Sep 17, 2024 •

edited

Loading

scottfgjordan commented Sep 19, 2024 •

edited

Loading