-
Notifications
You must be signed in to change notification settings - Fork 887
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
FPC LRU cache related changes #2497
Conversation
update fpc lru cache related changes
fpc lru cache related changes
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.
@mmanoj, some things to improve; this is only a preliminary review.
The most obvious missing piece is the cache initialization! You must add it in ndpi_finalize_initialization
Please, be sure that the code compile (at least on your machine), before pushing it
Update for review changes
Update as per review comments
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.
Update as per review comments.Howeer need some clarifications regarding section of "first packet of this flow to be analyzed" in ndpi_main.c we can discuss it after this review.
All changes done and compiled in local env with latest dev branch. |
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 general idea of the cache is:
-
add an entry in the cache with DNS traffic. In the DNS code, if
ndpi_match_host_subprotocol()
return a protocol different thanNDPI_PROTOCOL_UNKNOWN
, we should do somethig likendpi_lru_add_to_cache(ndpi_str->fpc_dns_cache, key, ret.app_protocol, ndpi_get_current_time(flow));
.
The key is something like SRC_FLOW_IP + ip_required_in_the_dns_query -
perform a lookup in
ndpi_internal_detection_process_packet
just afterfpc_check_ip()
call. If there is an entry, call something likefpc_update(ndpi_str, flow, NDPI_PROTOCOL_UNKNOWN, protocol_returned_from_the_cache, NDPI_FPC_CONFIDENCE_DNS)
Any specific doubts?
src/include/ndpi_private.h
Outdated
@@ -361,6 +367,9 @@ struct ndpi_detection_module_struct { | |||
|
|||
/* NDPI_PROTOCOL_MSTEAMS */ | |||
struct ndpi_lru_cache *msteams_cache; | |||
|
|||
/* NDPI_FIRST_PKT_CLASSIFICATION_CACHE */ |
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.
As above
src/lib/ndpi_main.c
Outdated
@@ -10250,6 +10330,10 @@ int ndpi_get_lru_cache_stats(struct ndpi_global_context *g_ctx, | |||
case NDPI_LRUCACHE_MSTEAMS: | |||
ndpi_lru_get_stats(is_local ? ndpi_struct->msteams_cache : g_ctx->msteams_global_cache, stats); | |||
return 0; | |||
//ToDo:add fpc cache details |
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.
Remove the comment, please
can you explain bit more about this? SRC_FLOW_IP + ip_required_in_the_dns_query |
That was the idea, but I was wrong (because of ipv4 and ipv6 possible mismatch) We should use only one address as key. Anyway, let try together to analyze what we should do step by step.
If everything is implemented correctly we should got something like:
Am I missing something? |
Thanks for detailed clarification,let me go through the attached dns packet capture and described logic flow with ongoing FPC implementation and update you any clarifications required.Thank you very much for quick support and all valuable advice.Really appreciate this advice to narrow down the implementation. |
The logic seems fine, let me implement and perform the initial test and update you for any clarifications required. |
I almost implement the logic from dns.c side and ndpi_main.c, still need to fix some the key generation logic with IP address as it's not working as expected.However hard coded key it's working fine.I will perform more test and share the code. |
@IvanNardi
|
FPC review change updates with DNS cache
FPC review change updates with DNS cache
FPC review change updates with DNS cache
FPC review change updates with DNS cache
All changes updated with test pcap file also.Please review and advice if any changes required. ./ndpiReader -t -i fpc_dns.pcap -v2 1 TCP 192.168.1.8:39432 <-> 142.250.199.4:443 [proto: 91/TLS][IP: 126/Google][Encrypted][Confidence: DPI][FPC: 126/Google, Confidence: DNS][DPI packets: 3][cat: Web/5][405 pkts/47359 bytes <-> 742 pkts/962242 bytes][Goodput ratio: 44/95][4.54 sec][bytes ratio: -0.906 (Download)][IAT c2s/s2c min/avg/max/stddev: 0/0 12/5 1094/1181 74/52][Pkt Len c2s/s2c min/avg/max/stddev: 66/66 117/1297 1466/1466 208/436][Risk: ** Probing attempt **][Risk Score: 50][Risk Info: TLS/QUIC Probing][Plen Bins: 3,3,0,0,0,0,1,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,1,0,0,0,0,0,0,0,0,91,0,0,0,0] |
Update documetation. Fix key with ipv6 addreses over ipv4 flow. Update unit tests results.
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.
I pushed some small changes and I'll update the commit message.
But the code was almost good anyway...
Thanks a lot for that and for your perseverance
Thanks for the support and I was review the fixes you posted and it's good improvements which I was missed.I'm happy to do some valuable contribution to community and thank again for your advice and guidelines.Really appreciated. Hope this change will improve the detection accuracy overall as per the test results I can see good detection specially services based on CDN's. Once This pull request merge I will work on adding more IP lists from some reliable sources specially malware and C&C servers. |
Please sign (check) the below before submitting the Pull Request:
Link to the related issue:
Describe changes:
changes related to #2322
item# 2
2.1.Create a new LRU cache where the key is the pair "Client_IP - Resolved_IP" and the value is a protocol id:
2.2 For each flow (at the very first packet) lookup into the cache using "SRC IP - DST IP" as key and, if a match is found, save the protocol id into ndpi_flow_struct structure as a "fpc result"