[WIP] Add packet attribute triage function #353
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce new functions and modify existing logic in the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
kimhanbeom
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
src/event/dns.rs (2)
245-246: Enhance warning message for invalid DNS attribute fieldsThe current warning message does not specify which attribute is invalid, which may make debugging difficult. Including the invalid attribute's name in the log will provide more context.
Propose the following change:
_ => { - warn!("invalid dns's attribute field"); + warn!("Invalid DNS attribute field: {}", attr.attr_name); false }
250-250: Typographical Error in CommentThere's a grammatical error in the comment. It should read "weight always exists."
Propose the following change:
if result { - score = attr.weight.unwrap(); //weight always exist. + score = attr.weight.unwrap(); // weight always exists. }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/event.rs (3 hunks)
- src/event/dns.rs (2 hunks)
🔇 Additional comments (1)
src/event/dns.rs (1)
221-248: Attribute Comparison Logic VerifiedThe match arms for attribute comparisons correctly handle each DNS attribute using the appropriate comparison functions. The logic for processing each attribute is sound and aligns with expected behavior.
src/event/dns.rs
Outdated
| fn score_by_packet_attr(&self, triage: &TriagePolicy) -> f64 { | ||
| let mut attr_total_score = 0.0; | ||
|
|
||
| for attr in &triage.packet_attr { | ||
| let mut score = 0.0; | ||
| let result = match attr.attr_name.as_str() { | ||
| DNS_S_IP => process_attr_compare_addr(self.src_addr, attr), | ||
| DNS_S_PORT => process_attr_compare_number::<u16, i64>(self.src_port, attr), | ||
| DNS_D_IP => process_attr_compare_addr(self.dst_addr, attr), | ||
| DNS_D_PORT => process_attr_compare_number::<u16, i64>(self.dst_port, attr), | ||
| DNS_PROTO => process_attr_compare_number::<u8, i64>(self.proto, attr), | ||
| DNS_QUERY => process_attr_compare_string(&self.query, attr), | ||
| DNS_ANSWER => self | ||
| .answer | ||
| .iter() | ||
| .any(|answer| process_attr_compare_string(answer, attr)), | ||
| DNS_TRANS_ID => process_attr_compare_number::<u16, i64>(self.trans_id, attr), | ||
| DNS_RTT => process_attr_compare_number::<i64, i64>(self.rtt, attr), | ||
| DNS_QCLASS => process_attr_compare_number::<u16, i64>(self.qclass, attr), | ||
| DNS_QTYPE => process_attr_compare_number::<u16, i64>(self.qtype, attr), | ||
| DNS_RCODE => process_attr_compare_number::<u16, i64>(self.rcode, attr), | ||
| DNS_AA_FLAG => process_attr_compare_bool(self.aa_flag, attr), | ||
| DNS_TC_FLAG => process_attr_compare_bool(self.tc_flag, attr), | ||
| DNS_RD_FLAG => process_attr_compare_bool(self.rd_flag, attr), | ||
| DNS_RA_FLAG => process_attr_compare_bool(self.ra_flag, attr), | ||
| DNS_TTL => self | ||
| .ttl | ||
| .iter() | ||
| .any(|ttl| process_attr_compare_number::<i32, i64>(*ttl, attr)), | ||
| _ => { | ||
| warn!("invalid dns's attribute field"); | ||
| false | ||
| } | ||
| }; | ||
| if result { | ||
| score = attr.weight.unwrap(); //weight always exist. | ||
| } | ||
| attr_total_score = score; | ||
| } | ||
| attr_total_score |
There was a problem hiding this comment.
Potential Logic Error: attr_total_score is overwritten instead of accumulated
In the score_by_packet_attr method, attr_total_score is being overwritten in each iteration of the loop. This means that only the score from the last attribute is considered. If the intention is to accumulate the scores from all matching attributes, you should sum the scores instead.
Apply the following diff to fix the accumulation:
for attr in &triage.packet_attr {
let mut score = 0.0;
let result = match attr.attr_name.as_str() {
// attribute comparisons
};
if result {
score = attr.weight.unwrap(); // weight always exists.
}
- attr_total_score = score;
+ attr_total_score += score;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn score_by_packet_attr(&self, triage: &TriagePolicy) -> f64 { | |
| let mut attr_total_score = 0.0; | |
| for attr in &triage.packet_attr { | |
| let mut score = 0.0; | |
| let result = match attr.attr_name.as_str() { | |
| DNS_S_IP => process_attr_compare_addr(self.src_addr, attr), | |
| DNS_S_PORT => process_attr_compare_number::<u16, i64>(self.src_port, attr), | |
| DNS_D_IP => process_attr_compare_addr(self.dst_addr, attr), | |
| DNS_D_PORT => process_attr_compare_number::<u16, i64>(self.dst_port, attr), | |
| DNS_PROTO => process_attr_compare_number::<u8, i64>(self.proto, attr), | |
| DNS_QUERY => process_attr_compare_string(&self.query, attr), | |
| DNS_ANSWER => self | |
| .answer | |
| .iter() | |
| .any(|answer| process_attr_compare_string(answer, attr)), | |
| DNS_TRANS_ID => process_attr_compare_number::<u16, i64>(self.trans_id, attr), | |
| DNS_RTT => process_attr_compare_number::<i64, i64>(self.rtt, attr), | |
| DNS_QCLASS => process_attr_compare_number::<u16, i64>(self.qclass, attr), | |
| DNS_QTYPE => process_attr_compare_number::<u16, i64>(self.qtype, attr), | |
| DNS_RCODE => process_attr_compare_number::<u16, i64>(self.rcode, attr), | |
| DNS_AA_FLAG => process_attr_compare_bool(self.aa_flag, attr), | |
| DNS_TC_FLAG => process_attr_compare_bool(self.tc_flag, attr), | |
| DNS_RD_FLAG => process_attr_compare_bool(self.rd_flag, attr), | |
| DNS_RA_FLAG => process_attr_compare_bool(self.ra_flag, attr), | |
| DNS_TTL => self | |
| .ttl | |
| .iter() | |
| .any(|ttl| process_attr_compare_number::<i32, i64>(*ttl, attr)), | |
| _ => { | |
| warn!("invalid dns's attribute field"); | |
| false | |
| } | |
| }; | |
| if result { | |
| score = attr.weight.unwrap(); //weight always exist. | |
| } | |
| attr_total_score = score; | |
| } | |
| attr_total_score | |
| fn score_by_packet_attr(&self, triage: &TriagePolicy) -> f64 { | |
| let mut attr_total_score = 0.0; | |
| for attr in &triage.packet_attr { | |
| let mut score = 0.0; | |
| let result = match attr.attr_name.as_str() { | |
| DNS_S_IP => process_attr_compare_addr(self.src_addr, attr), | |
| DNS_S_PORT => process_attr_compare_number::<u16, i64>(self.src_port, attr), | |
| DNS_D_IP => process_attr_compare_addr(self.dst_addr, attr), | |
| DNS_D_PORT => process_attr_compare_number::<u16, i64>(self.dst_port, attr), | |
| DNS_PROTO => process_attr_compare_number::<u8, i64>(self.proto, attr), | |
| DNS_QUERY => process_attr_compare_string(&self.query, attr), | |
| DNS_ANSWER => self | |
| .answer | |
| .iter() | |
| .any(|answer| process_attr_compare_string(answer, attr)), | |
| DNS_TRANS_ID => process_attr_compare_number::<u16, i64>(self.trans_id, attr), | |
| DNS_RTT => process_attr_compare_number::<i64, i64>(self.rtt, attr), | |
| DNS_QCLASS => process_attr_compare_number::<u16, i64>(self.qclass, attr), | |
| DNS_QTYPE => process_attr_compare_number::<u16, i64>(self.qtype, attr), | |
| DNS_RCODE => process_attr_compare_number::<u16, i64>(self.rcode, attr), | |
| DNS_AA_FLAG => process_attr_compare_bool(self.aa_flag, attr), | |
| DNS_TC_FLAG => process_attr_compare_bool(self.tc_flag, attr), | |
| DNS_RD_FLAG => process_attr_compare_bool(self.rd_flag, attr), | |
| DNS_RA_FLAG => process_attr_compare_bool(self.ra_flag, attr), | |
| DNS_TTL => self | |
| .ttl | |
| .iter() | |
| .any(|ttl| process_attr_compare_number::<i32, i64>(*ttl, attr)), | |
| _ => { | |
| warn!("invalid dns's attribute field"); | |
| false | |
| } | |
| }; | |
| if result { | |
| score = attr.weight.unwrap(); //weight always exist. | |
| } | |
| attr_total_score += score; | |
| } | |
| attr_total_score |
src/event.rs
Outdated
| fn process_attr_compare_string(attr_val: &str, packet_attr: &PacketAttr) -> bool { | ||
| deserialize::<String>(&packet_attr.first_value).is_some_and(|compare_val| { | ||
| let cmp_result = attr_val.contains(&compare_val); | ||
| match packet_attr.cmp_kind { | ||
| AttrCmpKind::Contain => return cmp_result, | ||
| AttrCmpKind::NotContain => return !cmp_result, | ||
| _ => false, | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify closures by removing unnecessary return statements
In the function process_attr_compare_string, the return statements within the closure are unnecessary. Since they are the last expressions in their match arms, they can be omitted to make the code cleaner.
Apply this diff to refine the function:
fn process_attr_compare_string(attr_val: &str, packet_attr: &PacketAttr) -> bool {
deserialize::<String>(&packet_attr.first_value).is_some_and(|compare_val| {
let cmp_result = attr_val.contains(&compare_val);
match packet_attr.cmp_kind {
- AttrCmpKind::Contain => return cmp_result,
- AttrCmpKind::NotContain => return !cmp_result,
+ AttrCmpKind::Contain => cmp_result,
+ AttrCmpKind::NotContain => !cmp_result,
_ => false,
}
})
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn process_attr_compare_string(attr_val: &str, packet_attr: &PacketAttr) -> bool { | |
| deserialize::<String>(&packet_attr.first_value).is_some_and(|compare_val| { | |
| let cmp_result = attr_val.contains(&compare_val); | |
| match packet_attr.cmp_kind { | |
| AttrCmpKind::Contain => return cmp_result, | |
| AttrCmpKind::NotContain => return !cmp_result, | |
| _ => false, | |
| } | |
| }) | |
| } | |
| fn process_attr_compare_string(attr_val: &str, packet_attr: &PacketAttr) -> bool { | |
| deserialize::<String>(&packet_attr.first_value).is_some_and(|compare_val| { | |
| let cmp_result = attr_val.contains(&compare_val); | |
| match packet_attr.cmp_kind { | |
| AttrCmpKind::Contain => cmp_result, | |
| AttrCmpKind::NotContain => !cmp_result, | |
| _ => false, | |
| } | |
| }) | |
| } |
src/event.rs
Outdated
| fn process_attr_compare_bool(attr_val: bool, packet_attr: &PacketAttr) -> bool { | ||
| deserialize::<bool>(&packet_attr.first_value).is_some_and(|compare_val| { | ||
| match packet_attr.cmp_kind { | ||
| AttrCmpKind::Equal => return attr_val == compare_val, | ||
| AttrCmpKind::NotEqual => return attr_val != compare_val, | ||
| _ => false, | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary return statements inside closures
In the function process_attr_compare_bool, the return statements inside the closure are unnecessary because they are the last expressions in their respective match arms. Removing them improves code readability.
Apply this diff to simplify the code:
fn process_attr_compare_bool(attr_val: bool, packet_attr: &PacketAttr) -> bool {
deserialize::<bool>(&packet_attr.first_value).is_some_and(|compare_val| {
match packet_attr.cmp_kind {
- AttrCmpKind::Equal => return attr_val == compare_val,
- AttrCmpKind::NotEqual => return attr_val != compare_val,
+ AttrCmpKind::Equal => attr_val == compare_val,
+ AttrCmpKind::NotEqual => attr_val != compare_val,
_ => false,
}
})
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn process_attr_compare_bool(attr_val: bool, packet_attr: &PacketAttr) -> bool { | |
| deserialize::<bool>(&packet_attr.first_value).is_some_and(|compare_val| { | |
| match packet_attr.cmp_kind { | |
| AttrCmpKind::Equal => return attr_val == compare_val, | |
| AttrCmpKind::NotEqual => return attr_val != compare_val, | |
| _ => false, | |
| } | |
| }) | |
| } | |
| fn process_attr_compare_bool(attr_val: bool, packet_attr: &PacketAttr) -> bool { | |
| deserialize::<bool>(&packet_attr.first_value).is_some_and(|compare_val| { | |
| match packet_attr.cmp_kind { | |
| AttrCmpKind::Equal => attr_val == compare_val, | |
| AttrCmpKind::NotEqual => attr_val != compare_val, | |
| _ => false, | |
| } | |
| }) | |
| } |
src/event.rs
Outdated
| #[allow(clippy::question_mark)] | ||
| fn check_second_value<'de, T, K>(kind: AttrCmpKind, value: &'de Option<Vec<u8>>) -> Option<T> | ||
| where | ||
| T: TryFrom<K> + std::cmp::PartialOrd, | ||
| K: Deserialize<'de>, | ||
| { | ||
| match kind { | ||
| AttrCmpKind::CloseRange | ||
| | AttrCmpKind::LeftOpenRange | ||
| | AttrCmpKind::RightOpenRange | ||
| | AttrCmpKind::NotOpenRange | ||
| | AttrCmpKind::NotCloseRange | ||
| | AttrCmpKind::NotLeftOpenRange | ||
| | AttrCmpKind::NotRightOpenRange => { | ||
| let Some(value_result) = value else { | ||
| return None; | ||
| }; | ||
| let Some(de_second_value) = deserialize::<K>(value_result) else { | ||
| return None; | ||
| }; | ||
| let Ok(convert_second_value) = T::try_from(de_second_value) else { | ||
| return None; | ||
| }; | ||
| return Some(convert_second_value); | ||
| } | ||
| _ => {} | ||
| } | ||
| None | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor check_second_value to use the ? operator
The function check_second_value can be simplified by using the ? operator for error handling. This refactoring enhances code clarity and removes the need for the #[allow(clippy::question_mark)] attribute.
Apply this diff to refactor the function:
-#[allow(clippy::question_mark)]
fn check_second_value<'de, T, K>(kind: AttrCmpKind, value: &'de Option<Vec<u8>>) -> Option<T>
where
T: TryFrom<K> + std::cmp::PartialOrd,
K: Deserialize<'de>,
{
- match kind {
- AttrCmpKind::CloseRange
- | AttrCmpKind::LeftOpenRange
- | AttrCmpKind::RightOpenRange
- | AttrCmpKind::NotOpenRange
- | AttrCmpKind::NotCloseRange
- | AttrCmpKind::NotLeftOpenRange
- | AttrCmpKind::NotRightOpenRange => {
- let Some(value_result) = value else {
- return None;
- };
- let Some(de_second_value) = deserialize::<K>(value_result) else {
- return None;
- };
- let Ok(convert_second_value) = T::try_from(de_second_value) else {
- return None;
- };
- return Some(convert_second_value);
- }
- _ => {}
- }
- None
+ if matches!(
+ kind,
+ AttrCmpKind::CloseRange
+ | AttrCmpKind::LeftOpenRange
+ | AttrCmpKind::RightOpenRange
+ | AttrCmpKind::NotOpenRange
+ | AttrCmpKind::NotCloseRange
+ | AttrCmpKind::NotLeftOpenRange
+ | AttrCmpKind::NotRightOpenRange
+ ) {
+ let value_result = value.as_ref()?;
+ let de_second_value = deserialize::<K>(value_result)?;
+ let convert_second_value = T::try_from(de_second_value).ok()?;
+ Some(convert_second_value)
+ } else {
+ None
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[allow(clippy::question_mark)] | |
| fn check_second_value<'de, T, K>(kind: AttrCmpKind, value: &'de Option<Vec<u8>>) -> Option<T> | |
| where | |
| T: TryFrom<K> + std::cmp::PartialOrd, | |
| K: Deserialize<'de>, | |
| { | |
| match kind { | |
| AttrCmpKind::CloseRange | |
| | AttrCmpKind::LeftOpenRange | |
| | AttrCmpKind::RightOpenRange | |
| | AttrCmpKind::NotOpenRange | |
| | AttrCmpKind::NotCloseRange | |
| | AttrCmpKind::NotLeftOpenRange | |
| | AttrCmpKind::NotRightOpenRange => { | |
| let Some(value_result) = value else { | |
| return None; | |
| }; | |
| let Some(de_second_value) = deserialize::<K>(value_result) else { | |
| return None; | |
| }; | |
| let Ok(convert_second_value) = T::try_from(de_second_value) else { | |
| return None; | |
| }; | |
| return Some(convert_second_value); | |
| } | |
| _ => {} | |
| } | |
| None | |
| } | |
| fn check_second_value<'de, T, K>(kind: AttrCmpKind, value: &'de Option<Vec<u8>>) -> Option<T> | |
| where | |
| T: TryFrom<K> + std::cmp::PartialOrd, | |
| K: Deserialize<'de>, | |
| { | |
| if matches!( | |
| kind, | |
| AttrCmpKind::CloseRange | |
| | AttrCmpKind::LeftOpenRange | |
| | AttrCmpKind::RightOpenRange | |
| | AttrCmpKind::NotOpenRange | |
| | AttrCmpKind::NotCloseRange | |
| | AttrCmpKind::NotLeftOpenRange | |
| | AttrCmpKind::NotRightOpenRange | |
| ) { | |
| let value_result = value.as_ref()?; | |
| let de_second_value = deserialize::<K>(value_result)?; | |
| let convert_second_value = T::try_from(de_second_value).ok()?; | |
| Some(convert_second_value) | |
| } else { | |
| None | |
| } | |
| } |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #353 +/- ##
==========================================
+ Coverage 67.71% 68.00% +0.28%
==========================================
Files 95 94 -1
Lines 19897 20301 +404
==========================================
+ Hits 13474 13805 +331
- Misses 6423 6496 +73 ☔ View full report in Codecov by Sentry. |
src/event/dns.rs
Outdated
| const DNS_S_IP: &str = "dns-id.orig_h"; | ||
| const DNS_S_PORT: &str = "dns-id.orig_p"; | ||
| const DNS_D_IP: &str = "dns-id.resp_h"; | ||
| const DNS_D_PORT: &str = "dns-id.resp_p"; | ||
| const DNS_PROTO: &str = "dns-proto"; | ||
| const DNS_QUERY: &str = "dns-query"; | ||
| const DNS_ANSWER: &str = "dns-answers"; | ||
| const DNS_TRANS_ID: &str = "dns-trans_id"; | ||
| const DNS_RTT: &str = "dns-rtt"; | ||
| const DNS_QCLASS: &str = "dns-qclass"; | ||
| const DNS_QTYPE: &str = "dns-qtype"; | ||
| const DNS_RCODE: &str = "dns-rcode"; | ||
| const DNS_AA_FLAG: &str = "dns-AA"; | ||
| const DNS_TC_FLAG: &str = "dns-TC"; | ||
| const DNS_RD_FLAG: &str = "dns-RD"; | ||
| const DNS_RA_FLAG: &str = "dns-RA"; | ||
| const DNS_TTL: &str = "dns-TTLs"; |
There was a problem hiding this comment.
I think it would be much better to define the attributes of the enum type. Since the definition is likely to be shared across our modules, we can manage it in a new repository.
kimhanbeom
force-pushed
the
david/add-packet-attr-triage-function
branch
from
d708f78 to
26af354
Compare
|
@sehkone |
kimhanbeom
force-pushed
the
david/add-packet-attr-triage-function
branch
from
26af354 to
bd05b0b
Compare
kimhanbeom
force-pushed
the
david/add-packet-attr-triage-function
branch
from
bd05b0b to
3c3f6e7
Compare
kimhanbeom
force-pushed
the
david/add-packet-attr-triage-function
branch
from
3c3f6e7 to
9880f6d
Compare
kimhanbeom
force-pushed
the
david/add-packet-attr-triage-function
branch
from
9880f6d to
829f5a4
Compare
kimhanbeom
changed the title
kimhanbeom
force-pushed
the
david/add-packet-attr-triage-function
branch
2 times, most recently
from
826f656 to
31ab52f
Compare
|
Since aicers/attrievent#2 was merged into the main branch of the attrievent repository, I have modified the code to use the enum from that repository. |
|
@kimhanbeom, I think it would be better to rename |
src/tables/triage_policy.rs
Outdated
| @@ -135,6 +138,7 @@ impl Ord for Ti { | |||
|
|
|||
| #[derive(Clone, PartialEq, Deserialize, Serialize)] | |||
| pub struct PacketAttr { | |||
| pub raw_event_type: String, | |||
There was a problem hiding this comment.
How about using its corresponding enum instead of a String?
CHANGELOG.md
Outdated
| `TorConnection`) and implementations within that module have been moved to | ||
| `crate::event::http`. | ||
| - Fixed HTTP detection events to consistently use `referrer` instead of | ||
| `referrer` and `referrer` interchangeably. |
There was a problem hiding this comment.
I suspect there might be a typo.
src/tables/triage_policy.rs
Outdated
| SInteger, | ||
| UInteger, |
There was a problem hiding this comment.
- Could you add some comments to explain the range of these values?
- If
SIntegermeansi32, don't we needi64? - If
SIntegerisi32andUIntegerisu32, I think the pair ofIntegerandUIntegerwould be better.
There was a problem hiding this comment.
SInteger means i64 and UInteger means u64. I'll add a comment about that.
There was a problem hiding this comment.
SIntegermeansi64andUIntegermeansu64.
In this case, I think the pair of Integer and UInteger is still better. Besides, SInteger even seems to mean "Small Integer".
| - Changed the type of fields in the detection event structure for some raw | ||
| event. This change allows users to see meaningful values directly without | ||
| having to do any special conversion for that field. | ||
| - `post_body`(HTTP): `Vec<u8>` to `String`. |
There was a problem hiding this comment.
If you mean HttpThreat::post_body, I suggest the following:
- `HttpThreat::post_body`: `Vec<u8>` to `String`.
There was a problem hiding this comment.
This part represents the post_body field of all detection events of the HTTP protocol type.
There was a problem hiding this comment.
- `post_body` inside structs of `event::http`: `Vec<u8>` to `String`.
How about the above for clarity?
CHANGELOG.md
Outdated
| - Added a new enum type `AttrValue`. This type is used to convert the | ||
| attribute value of each raw event to its corresponding type to perform | ||
| comparison operations. | ||
| - Added the `target_attribute` to the `Match` trait to generate an `AttrValue` |
There was a problem hiding this comment.
I think the target_attribute method should be renamed to to_attr_value, because it literally returns AttrValue.
src/event/dhcp.rs
Outdated
| @@ -167,9 +212,9 @@ impl BlockListDhcp { | |||
| message: fields.message, | |||
| renewal_time: fields.renewal_time, | |||
| rebinding_time: fields.rebinding_time, | |||
| class_id: fields.class_id, | |||
| class_id: String::from_utf8(fields.class_id.clone()).unwrap_or_default(), | |||
There was a problem hiding this comment.
I think the following is a better choice:
class_id: std::str::from_utf8(&fields.class_id).unwrap_or_default().to_string(),
src/event/dhcp.rs
Outdated
| @@ -68,7 +113,7 @@ impl fmt::Display for BlockListDhcpFields { | |||
| self.message.to_string(), | |||
| self.renewal_time.to_string(), | |||
| self.rebinding_time.to_string(), | |||
| to_hardware_address(&self.class_id), | |||
| String::from_utf8(self.class_id.clone()).unwrap_or_default(), | |||
There was a problem hiding this comment.
I suggest:
std::str::from_utf8(&fields.class_id).unwrap_or_default().to_string(),There was a problem hiding this comment.
Since str has an advantage over String in terms of performance, and there will be virtually no cloning if unwrap fails, I believe this approach is slightly better.
|
For the Petabi team, I think I need to explain why this new crate attrievent has been introduced. The kinds of raw events and their attributes change as our software evolves. The purpose of attrievent is to provide a comprehensive list of attributes for both the REview and the UI simultaneously. |
kimhanbeom
force-pushed
the
david/add-packet-attr-triage-function
branch
from
31ab52f to
b1eaa7e
Compare
- Add triage functionality for scoring with attributes of each raw
event.
- Introduced a new crate attrievent. The kinds of raw events and their
attributes change as our software evolves. The purpose of attrievent
is to provide a comprehensive list of attributes for both review and
the UI simultaneously.
- Add the `to_attr_value` to the `Match` trait.
- Implement `score_by_attr` under `Match` trait.
- Modify the `ValueKind` enum to support different types of input.
- Remove the `tor` module file. The structures (`HttpEventFields`,
`TorConnection`) and implementations within that module have been
moved to `crate::event::http`.
- Fix HTTP detection events to consistently use `referrer` instead of
`referrer` and `referer` interchangeably.
- Change the type of fields in the detection event structure for some
raw event.
- `post_body`: `Vec<u8>` to `String`.
- `chaddr`: `Vec<u8>` to `String`.
- `class_id`: `Vec<u8>` to `String`.
- `client_id`: `Vec<u8>` to `String`.
Close: petabi#354
kimhanbeom
force-pushed
the
david/add-packet-attr-triage-function
branch
from
b1eaa7e to
b8d9cc7
Compare
|
It would be beneficial to break down this PR into smaller, more focused ones. The formatting fixes for old CHANGELOG entries and spelling fixes in the code are great, but they're not directly related to the main changes in this PR. Would you mind separating those into their own PRs? That way, we can review and merge them independently, making it easier to track changes and test the main functionality of this PR. It'll also make it smaller and more manageable. |
Regarding the changes to the CHANGELOG, I think I need to share how the team in Seoul recently adopted a method for handling text files such as CHANGELOG and README. The team is now using an extension for VS Code, Prettier, to format these files. As a result, even a small change to either file causes the entire content to be reformatted, automatically on save. |
sehkone
changed the title
|
This needs to be rebased, updated to use the release of attrievent, and modified according to the reviewers' comments. Once that is done and [WIP] is removed, the review can proceed. |
@sehkone
Regarding the addition of packet attribute functionality, I have added this functionality to
DnsCovertChannelas follows. I would appreciate any feedback on this modification.closes #354
Summary by CodeRabbit
New Features
Bug Fixes
Documentation