Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[iptables module] Accept TOS to not provide 0x HEX format #32126

Merged
merged 3 commits into from
Oct 20, 2022

Conversation

lucabelluccini
Copy link
Contributor

@lucabelluccini lucabelluccini commented Jun 28, 2022

What does this PR do?

It might happen that some appliances send a TOS value without the 0x prefix for HEX values.

Some users hit this problem with NFLOG and ulogd2 extension.

As I am not an expert on this domain, I let you review this change.

Why is it important?

It can allow parsing logs for IPTables and be slightly more flexible on the TOS format.

Checklist

  • My code follows the style guidelines of this project
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

How to test this PR locally

TODO, need to add:

Jun 28 04:35:30 Abc-A1 [SOMETHING-1234-A] IN=abc.123 OUT=abc.123 MAC=0a:ea:10:00:f0:06:10:e1:21:31:61:20:01:00:41:00:00:01 SRC=10.251.1.1 DST=10.251.1.1 LEN=32 TOS=00 PREC=0x00 TTL=63 ID=12345 PROTO=UDP SPT=9000 DPT=9000 LEN=12 MARK=0 
Jun 28 04:30:32 Abc-A1 [SOMETHING-1234-A] IN=abc.123 OUT=abc.123 MAC=0a:ea:10:00:f0:06:10:e1:21:31:61:20:01:00:41:00:00:01 SRC=10.251.1.1 DST=10.251.1.1 LEN=84 TOS=00 PREC=0x00 TTL=63 ID=6789 PROTO=ICMP TYPE=8 CODE=0 ID=98765 SEQ=30123 MARK=0 

Related issues

None

Use cases

It might happen that some appliances send a `TOS` value without the `0x` prefix for HEX values.

Some users hit this problem with NFLOG and ulogd2 extension.

As I am not an expert on this domain, I let you review this change
@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Jun 28, 2022
@lucabelluccini lucabelluccini marked this pull request as ready for review June 28, 2022 09:48
@lucabelluccini lucabelluccini requested a review from a team as a code owner June 28, 2022 09:48
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations (Team:Integrations)

@elasticmachine
Copy link
Collaborator

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

@elasticmachine
Copy link
Collaborator

elasticmachine commented Jun 28, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-10-20T04:05:28.757+0000

  • Duration: 77 min 33 sec

Test stats 🧪

Test Results
Failed 0
Passed 2283
Skipped 166
Total 2449

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

Copy link
Contributor

@adriansr adriansr left a comment

Choose a reason for hiding this comment

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

Looks good so far.

It's going to need sample logs added to the test files, ideally if you can find ones with a TOS value that confirms this is still hexadecimal.

Also a changelog entry.

@adriansr adriansr added the needs_integration_sync Changes in this PR need synced to elastic/integrations. label Jun 29, 2022
@mergify
Copy link
Contributor

mergify bot commented Aug 16, 2022

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @lucabelluccini? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)
    To fixup this pull request, you need to add the backport labels for the needed
    branches, such as:
  • backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

@epixa epixa requested review from adriansr and removed request for adriansr September 22, 2022 01:07
@efd6
Copy link
Contributor

efd6 commented Oct 13, 2022

@lucabelluccini Are you able to find a real example that we can use in tests that does not include the 0x prefix and has a non-zero value? Ideally this would be a value that is definitively a hex number (i.e. with at least one digit not in [0-9]) or definitively a decimal number (i.e. three digits long) so that we can ensure that we are keeping the semantics correct.

@efd6 efd6 self-assigned this Oct 13, 2022
@efd6
Copy link
Contributor

efd6 commented Oct 20, 2022

OK. I have a confirmation on this. The ulogd v2 code clearly emits a two-digit hex value without a 0x prefix.

	buf_cur += sprintf(buf_cur,"LEN=%u TOS=%02X PREC=0x%02X TTL=%u ID=%u ", 
			   ikey_get_u16(&res[KEY_IP_TOTLEN]),
			   ikey_get_u8(&res[KEY_IP_TOS]) & IPTOS_TOS_MASK, 
			   ikey_get_u8(&res[KEY_IP_TOS]) & IPTOS_PREC_MASK,
			   ikey_get_u8(&res[KEY_IP_TTL]),
			   ikey_get_u16(&res[KEY_IP_ID]));

So I think we are good. I'll add the test examples that are included in the PR message.

@mergify
Copy link
Contributor

mergify bot commented Oct 20, 2022

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b iptables-enhance-tos-field upstream/iptables-enhance-tos-field
git merge upstream/main
git push upstream iptables-enhance-tos-field

@lucabelluccini
Copy link
Contributor Author

Hello @efd6 - I opened this following an example provided by a user.
I asked them details about the source but I never got an answer.

@efd6
Copy link
Contributor

efd6 commented Oct 20, 2022

@lucabelluccini No worries, I'm happy with the answer now. I think we are good now.

@efd6 efd6 merged commit d7d12d3 into main Oct 20, 2022
@efd6 efd6 deleted the iptables-enhance-tos-field branch October 20, 2022 22:02
chrisberkhout pushed a commit that referenced this pull request Jun 1, 2023
…non-0x prefixed TOS field (#32126)

The formatting of log lines by ulogd v2 does not prefix the TOS field with a hex
0x syntax[1].

	buf_cur += sprintf(buf_cur,"LEN=%u TOS=%02X PREC=0x%02X TTL=%u ID=%u ",
			   ikey_get_u16(&res[KEY_IP_TOTLEN]),
			   ikey_get_u8(&res[KEY_IP_TOS]) & IPTOS_TOS_MASK,
			   ikey_get_u8(&res[KEY_IP_TOS]) & IPTOS_PREC_MASK,
			   ikey_get_u8(&res[KEY_IP_TTL]),
			   ikey_get_u16(&res[KEY_IP_ID]));

So make sure that we allow log lines without this syntax to be parsed.

[1]https://github.com/bootc/ulogd2/blob/f985da47cbfa24b6ec0b6fcbebe1ada09e31ee35/util/printpkt.c#L213-L218
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement needs_integration_sync Changes in this PR need synced to elastic/integrations. Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants