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

[Fortinet Fortigate] Change default TCP framing to RFC 6587 #7516

Merged
merged 2 commits into from
Aug 30, 2023

Conversation

chemamartinez
Copy link
Contributor

What does this PR do?

  • It enables the RFC 6587 by default on the TCP input. Regarding the Fortigate reference, when the syslog mode is set to reliable, it uses RFC 6587 for TCP forwarding.
  • Added a note in the docs, just in case it would help users to be warned about this.

The framing value has been set to rfc6587 by default because I assume that the reliable mode should be the default one when forwarding over TCP syslog from Fortigate, the other available option is legacy-reliable.

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

@chemamartinez chemamartinez marked this pull request as ready for review August 23, 2023 16:58
@chemamartinez chemamartinez requested a review from a team as a code owner August 23, 2023 16:58
@elasticmachine
Copy link

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

@elasticmachine
Copy link

elasticmachine commented Aug 23, 2023

💚 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: 2023-08-23T16:58:42.766+0000

  • Duration: 16 min 49 sec

Test stats 🧪

Test Results
Failed 0
Passed 9
Skipped 0
Total 9

🤖 GitHub comments

Expand to view the GitHub comments

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

  • /test : Re-trigger the build.

@elasticmachine
Copy link

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (1/1) 💚
Files 100.0% (4/4) 💚 3.35
Classes 100.0% (4/4) 💚 3.35
Methods 100.0% (40/40) 💚 8.137
Lines 91.386% (1167/1277) 👍 3.067
Conditionals 100.0% (0/0) 💚

Copy link
Contributor

@taylor-swanson taylor-swanson left a comment

Choose a reason for hiding this comment

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

This topic has always confused me and don't think I've ever gotten a firm answer on it. RFC 6587 specifies both octet counting framing (which is what framing: rfc6587 does here) and non-transparent framing (which is what framing: delimiter does). Were you able to confirm with PCAPs which method Fortigate uses?

Changing the default runs the risk of breaking existing deployments if they're relying on the currently established behavior. If, however, we should be using octet counting framing (and non-transparent framing was never supported by Fortigate), then it would be good to make this change.

Edit: Okay I misunderstood what framing: rfc6587 did, it can handle either framing type. That would explain why the system tests continue to pass 👍.

Copy link
Contributor

@taylor-swanson taylor-swanson left a comment

Choose a reason for hiding this comment

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

LGTM

@chemamartinez
Copy link
Contributor Author

Much appreciated your review @taylor-swanson! I would also like to have the opinion of @P1llus as he has been involved in the issue.

Copy link
Member

@P1llus P1llus left a comment

Choose a reason for hiding this comment

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

As long as we can say for certain that framing will be enabled by default from the fortigate side, this is fine. I was thinking worst case scenario, we might want to keep it commented out.
I was just thinking this should have been reported more often if framing was a big issue? Or maybe TCP is not used that frequently.
Except that, all LGTM :)

@chemamartinez
Copy link
Contributor Author

As long as we can say for certain that framing will be enabled by default from the fortigate side, this is fine. I was thinking worst case scenario, we might want to keep it commented out.
I was just thinking this should have been reported more often if framing was a big issue? Or maybe TCP is not used that frequently.

Based on this piece of docs
image
We have three mode values (udp, legacy-reliable and reliable), regarding the reliable mode description I would let RFC 6587 as the default value. It should be the default mode used for TCP forwarding.

@chemamartinez chemamartinez merged commit 7c5264f into elastic:main Aug 30, 2023
4 checks passed
@elasticmachine
Copy link

Package fortinet_fortigate - 1.16.1 containing this change is available at https://epr.elastic.co/search?package=fortinet_fortigate

gizas pushed a commit that referenced this pull request Sep 5, 2023
* Change default TCP framing to rfc6587

* Update changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants