-
Notifications
You must be signed in to change notification settings - Fork 8
Refactor: Unify event structure terminology #643
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
Conversation
0021193 to
2f80235
Compare
|
I rebased this PR onto the latest |
| impl Match for PortScan { | ||
| fn src_addrs(&self) -> &[IpAddr] { | ||
| std::slice::from_ref(&self.src_addr) | ||
| std::slice::from_ref(&self.orig_addr) |
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.
@syncpark I am aware of the discussion distinguishing "source/destination" as packet-level concepts and "originator/responder" as connection-level concepts.
In this context, since the fields are renamed in individual event protocols to orig_* and resp_*, I wonder if the functions within the Match trait should also be updated to maintain consistency. For example, should fn src_addrs be changed to fn orig_addrs?
Since you are in charge of the overall transition from src/dst to orig/resp, I would like to kindly ask for your thoughts on this matter.
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.
@sophie-cluml
I thought changing the function name would impact review-web, the GraphQL API, and even the UI.
If the work in this PR to change the field names in the event struct is part of aligning with Semi-supervised and Unsupervised Engine, the other tasks would be too extensive to handle in this PR, so I think it's best to handle them as separate issues.
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.
Thank you for clarifying your intention with this issue. I have no objection handling it in a separate issue.
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.
However, I believe we should not mention private repo's names in a public repo. Could you remove the names please? @syncpark
CHANGELOG.md
Outdated
| name. The key format is 4 bytes of `customer_id` (big-endian, using `u32::MAX` | ||
| for `None`) followed by the name bytes. | ||
| - **BREAKING**: Unified address/port field naming in session-level detection | ||
| event structures to align with Hog terminology. Renamed fields: |
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.
| event structures to align with Hog terminology. Renamed fields: | |
| event structures. Renamed fields: |
This suggestion is to stay aligned with the organization's private name referencing rules.
| impl Match for PortScan { | ||
| fn src_addrs(&self) -> &[IpAddr] { | ||
| std::slice::from_ref(&self.src_addr) | ||
| std::slice::from_ref(&self.orig_addr) |
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.
Thank you for clarifying your intention with this issue. I have no objection handling it in a separate issue.
| impl Match for PortScan { | ||
| fn src_addrs(&self) -> &[IpAddr] { | ||
| std::slice::from_ref(&self.src_addr) | ||
| std::slice::from_ref(&self.orig_addr) |
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.
However, I believe we should not mention private repo's names in a public repo. Could you remove the names please? @syncpark
CHANGELOG.md
Outdated
| Affected structures include: `BlocklistBootpFields`, `BlocklistConnFields`, | ||
| `BlocklistDceRpcFields`, `BlocklistDhcpFields`, `BlocklistDnsFields`, | ||
| `BlocklistFtpFields`, `BlocklistHttpFields`, `BlocklistKerberosFields`, | ||
| `BlocklistLdapFields`, `BlocklistMqttFields`, `BlocklistNfsFields`, | ||
| `BlocklistNtlmFields`, `BlocklistRadiusFields`, `BlocklistRdpFields`, | ||
| `BlocklistSmbFields`, `BlocklistSmtpFields`, `BlocklistSshFields`, | ||
| `BlocklistTlsFields`, `CryptocurrencyMiningPoolFields`, `DgaFields`, | ||
| `DnsEventFields`, `ExternalDdosFields`, `FtpBruteForceFields`, | ||
| `FtpPlainTextFields`, `HttpEventFields`, `HttpThreatFields`, | ||
| `LdapBruteForceFields`, `LdapPlainTextFields`, `MultiHostPortScanFields`, | ||
| `PortScanFields`, `RdpBruteForceFields`, `RepeatedHttpSessionsFields`, | ||
| `TorConnection`, and `TorConnectionConn`. |
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.
@octoaide, if all of the structures that have at least one of the renamed fields, are affected, I don't think it is beneficial to enumerate all event structures. I would appreciate if you could describe the scope in a concise manner.
sophie-cluml
left a comment
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.
Current PR needs to be rebased on main branch as well.
2f80235 to
858623e
Compare
|
Thanks for the reviews — I went through all the feedback and have applied the requested changes. What I did
About the Match trait rename suggestion
Status
Thanks again for the feedback — let me know if you’d like me to open a follow-up issue to track renaming the Match trait functions. |
This PR addresses issue #381 by unifying the terminology of detection event structures across
review-databaseto align withHog.Specifically, fields related to source and destination addresses/ports have been renamed to
orig_andresp_prefixes, respectively, for consistency across the system.Closes #381
Summary
I've completed the implementation of GitHub issue #381, which unified the terminology of detection event structures. Here's what was done:
Field Renames Applied
src_addr→orig_addrsrc_addrs→orig_addrssrc_port→orig_portdst_addr→resp_addrdst_addrs→resp_addrsdst_port→resp_portdst_ports→resp_portsFiles Modified
Event structure files:
src/event/bootp.rssrc/event/conn.rssrc/event/dcerpc.rssrc/event/dhcp.rssrc/event/dns.rssrc/event/ftp.rssrc/event/http.rssrc/event/kerberos.rssrc/event/ldap.rssrc/event/mqtt.rssrc/event/nfs.rssrc/event/ntlm.rssrc/event/radius.rssrc/event/rdp.rssrc/event/smb.rssrc/event/smtp.rssrc/event/ssh.rssrc/event/tls.rssrc/event/tor.rsOther files updated:
src/event.rs(tests and event handling code)src/event/common.rs(test code)src/backup.rs(test code)CHANGELOG.md(documentation)Changes Made in Each File
syslog_rfc5424()methodsDisplaytrait implementationsfind_*_attr_by_kind!macros (field access only, not theAttrenum variants)new()functionsMatchtrait implementations (field access only, trait method names preserved)Verification