-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Rust add external parsers 2 #2794
Conversation
Should we really have --enable-rust-external? I'm unclear what it gets us. What happens if we use an external nom parser for something more critical like TLS or HTTP2? Would these be wrapped behind --enable-rust-external as well? |
The --enable-rust-external is meant to be a clear separation between the parser that we develop/bundle, review and support on one side, and the parsers that depend on (potentially lots of) external crates that are developed by third parties, have different licenses and ownership, different goals and release schedules on the other side. I consider it an experiment within the Rust experiment. Perhaps in the future we can do away with this, but for now I want to keep things manageable and cleanly separated. This will also mean that 'core' code cannot depend on these modules for now. |
Just to be a bit pedantic, NFS and DNS rely on external crates that are developed by third parties, as well. The name to me means external Rust code will not be pulled in (a technical thing). Where it is actually a support level type of thing. If its short term, its probably OK. If its going to exist long term, perhaps the name needs a rethink? --enable-rust-experimental to enable experimental rust modules? |
I'm ok with finding another name if that makes things more clear. |
In general, with Rust and C all the same, we need to be careful about accepting external dependencies. Every one of them is a liability. I've spent countless hours on working around issues in libraries we use. We also see problems with original developers going away, libhtp is the biggest example for that, but also lua plugins that are buggy but unmaintained, etc. In my mind something like nom and crc is more comparable to libpcre, libnss, etc. General purpose libraries/frameworks that are (hopefully) well maintained. While the protocol dissectors, at this stage, are more comparable to libhtp. Small separate projects that are much more likely to need updates by us if we would depend on them. That's why we maintain libhtp, we need to change things in there too often. Maybe things will be better because of cargo, but it remains to be seen how distro's handle packaging. Already some crates are broken on Ubuntu LTS' version of rust. |
Hi, Some random thoughts: |
Sounds like in the long run, given that external crates are going to have different licenses, and different levels of quality, that a more fine grained approach might be needed, such as: --enable-ntp And so on, rather than having one flag pull in a grab bag of licenses. This can probably wait though. Note: I've left "rust" out of the examples above. An end user will need to know that Rust is required to build Suricata (when it becomes required). They do not need to know, and should not need to care that a feature is written in Rust or C. But I think my main issue with the name "external" now has nothing to do with licenses, support, etc. It just sounds like it disables pulling in any external Rust code. |
Ok, so what do you prefer (for this pull request, not on the long term):
I have no clear preference, I would just like to avoid one word to block the PR :) |
|
||
/* Check if NTP UDP detection is enabled. If it does not exist in | ||
* the configuration file then it will be enabled by default. */ | ||
if (AppLayerProtoDetectConfProtoDetectionEnabled("tcp", proto_name)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tcp used instead of udp
} | ||
else { | ||
|
||
if (!AppLayerProtoDetectPPParseConfPorts("tcp", IPPROTO_UDP, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tcp
|
||
if (AppLayerParserConfParserEnabled("udp", proto_name)) { | ||
|
||
SCLogNotice("Registering NTP protocol parser."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notice
NTPGetEvents); | ||
} | ||
else { | ||
SCLogNotice("NTP protocol parsing disabled."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notice
@@ -0,0 +1,3 @@ | |||
alert ntp any any -> any any (msg:"SURICATA NTP malformed request data"; flow:to_server; app-layer-event:ntp.malformed_data; classtype:protocol-command-decode; sid:2222001; rev:1;) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a reference to the sid assignment page?
} | ||
} | ||
|
||
impl Drop for NTPTransaction { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you see this being used? I think all tx frees are explicit, or at least they should be as the C-side is responsible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Drop Trait is called when the object is destroyed, this is a destructor.
Transactions are freed here:
https://github.com/inliniac/suricata/pull/2794/files#diff-1652b119a6a57d40eb26e9b50c527dcdR115
I made the self.transactions.clear()
explicit, but it is not required (vector is cleared when the object becomes out of reach).
When C calls rs_ntp_state_free
, the state is freed, automatically freeing the transactions.
The other code path to free a transaction is rs_ntp_state_tx_free
, which gets the object in a temporary variable dropped immediately (calling the Drop
Trait).
See free_tx
:
https://github.com/inliniac/suricata/pull/2794/files#diff-1652b119a6a57d40eb26e9b50c527dcdR134
On 21-06-17 08:45, Pierre Chifflier wrote:
Ok, so what do you prefer (for this pull request, not on the long term):
* something like --enable-rust-experimental ?
* something like --enable-rust-third-party, --enable-rust-external or
--enable-rust-unsupported to emphasize on the fact there is a part
not supported by OISF
* any other idea
I have no clear preference, I would just like to avoid one word to block
the PR :)
No strong preference on my end. So Jason if you have a strong preference
let us know, otherwise I'm just going to accept as-is for now.
|
I think I'd lean to towards --enable-rust-experimental? Reason being, in the past 2 days I've referred to external crates 2 times now, in reference to nom, and a time handling one I thought about pulling but might not be needed now since Ubuntu updated its Rust. Though I suppose saying "crate" and "external" together is redundant. |
Replaced by #2798 |
Make sure these boxes are signed before submitting your Pull Request -- thank you.
Describe changes:
Thanks,
Pierre