feat(api): add ASN-aggregated IOC statistics . CLOSES #458 #718
Conversation
regulartim
left a comment
regulartim
left a comment
There was a problem hiding this comment.
Nice, thank you!
A few general things:
- In my mind the aggregation would be done on a database level. Your approach also works, of course, but I am a little bit concerned regarding performance on instances with millions of IoCs.
- Ordering is kind of tricky here. I tested your code and I was not able to order by
ioc_count. But intuitively I would expect that to work. I think we should support to order by any field that is present in the output, if sensible (ordering byhoneypotsis not). - The IOC model has more field that have not been included here yet. Do you think you could also include
first_seenandlast_seen? Or do you think that this is not useful?
Hi, @regulartim, Thanks a lot for the review and the feedback! I wanted to explain a bit why I initially went with Python-level aggregation and where I’m currently unsure, so I’d really appreciate your guidance. My first thought was also to aggregate at the database level using values("asn") + annotate(...). However, I ran into a couple of structural issues with the current setup of get_queryset, which is shared by other API: get_queryset applies slicing ([:feed_size]) after ordering. Once a queryset is sliced, Django does not allow further aggregation, which makes DB-level grouping impossible unless we disable slicing using a flag (doSlice=True). However i was afraid to change the fx which is used by other apis without your permission.
And If we slice before aggregation, the aggregation sees only the first feed_size raw IOCs. This produces incorrect totals (ioc_count, attack_count, etc.), because other IOCs outside the slice are ignored. For example, if ASN 13335 has 10,000 IOCs, slicing at 5000 will undercount. When aggregating by ASN and joining against general_honeypot, each IOC can appear multiple times at the sql level (one row per honeypot). This leads to inflated sums (e.g., attack_count, interaction_count). For example, a single IOC linked to two honeypots is counted twice in SUM(attack_count) in aggregation . I guees it is many to many joins and duplicated rows issue. I think that get_queryset needs a big refactor here I’d love to hear what you think is the best tradeoff here, especially given the shared nature of get_queryset. I’m happy to adapt the implementation based on your recommendation. |
|
Hey @drona-gyawali ! Thanks for your detailed explanation. I think the best approach would be to aggregate on the DB level. If this makes it necessary to refactor or even split up
This is kind of strange. If a aggregation function is used on the |
Extremely sorry for the late reply! I used ArrayAgg(distinct=True) for honeypots and distinct=True on all other fields to avoid inflated sums, since thing were failing without it. The iocs_qs comes from get_queryset. raw code for reference: I’ll push the new version soon. |
Don't worry, we all have other stuff to do! :)
Cool, looking forward to that! |
|
Hi @regulartim , In this new version, I implemented DB-level aggregation for the ASN feed. While building this, I had to introduce a few things: New serializer (ASNFeedsOrderingSerializer) – I inherited from FeedsRequestSerializer because the base serializer already provides default handling for parameters like max_age, feed_type, and attack_type. The main reason for creating the new serializer was that the base serializer’s ordering validation is strict and only allows model fields. Since aggregation introduces annotated/non-model fields (like ioc_count, total_attack_count), I needed to add custom validation here. resolve_aggregation_ordering utility – This ensures that our aggregation endpoint defaults to ordering by -ioc_count instead of -last_seen, bypassing the default injection from feed_params. I tried to make this dynamic so that any future aggregation API can leverage the same pattern without reinventing the wheel. The overall goal was to make the developer experience better and avoid complexity when building future aggregation endpoints. Anyone adding a new aggregation API only needs to inherit and customize the ordering/validation, without rewriting everything. I hope this aligns with your expectations. If you feel any part of this design needs changes, I’m always open to feedback and happy to adjust. |
| # aggregated endpoints should operate on the full queryset | ||
| # to compute sums, counts, and other metrics correctly. | ||
| if not is_aggregated: | ||
| iocs = iocs.order_by(feed_params.ordering) | ||
| iocs = iocs[: int(feed_params.feed_size)] | ||
|
|
| ioc_count=Count("id", distinct=True), | ||
| total_attack_count=Sum("attack_count", distinct=True), | ||
| total_interaction_count=Sum("interaction_count", distinct=True), | ||
| total_login_attempts=Sum("login_attempts", distinct=True), | ||
| expected_ioc_count=Sum("recurrence_probability", distinct=True), | ||
| expected_interactions=Sum("expected_interactions", distinct=True), |
There was a problem hiding this comment.
Can you please explain why you are using distinct=True here? For the Count it does not really do anything, because id is unique and for Sum I can't find any documentation of what the distinct argument actually does.
There was a problem hiding this comment.
Thank you for pointing that out. To be frank, I had used distinct as a quick workaround, but the root cause of the error was row duplication in the SQL query. This happened because of the Many-to-Many join from .filter(general_honeypot__active=True) combined with ArrayAgg on the honeypot field in get_queryset.
This duplication caused Count("id") and other aggregated fields to be inflated (e.g., 4 instead of 2), which led to the test failures. At first, I thought it was related to ordering or feed_size in get_queryset, and even considered a possible test case issue, but the real problem was the join itself.
Using distinct=True on Count was only a workaround it masks the issue but doesn’t fix it properly and can break Sum when values repeat and sorry , I realized this issue later after you pointed out.
The clean solution i think to separate numeric aggregation from the M2M honeypot aggregation in asn_aggreated_queryset and little refactor in get_queryset. That way, counts and sums stay accurate without duplication tricks.
The changes in get_queryset would roughly reflect this approach.
iocs = (
IOC.objects.filter(**query_dict)
.exclude(ip_reputation__in=feed_params.exclude_reputation)
.annotate(value=F("name"))
.distinct()
)
# aggregated endpoints should operate on the full queryset
# to compute sums, counts, and other metrics correctly.
if not is_aggregated:
iocs= iocs.filter(general_honeypot__active=True)
iocs = iocs.annotate(honeypots=ArrayAgg("general_honeypot__name"))
iocs = iocs.order_by(feed_params.ordering)
iocs = iocs[:int(feed_params.feed_size)]
what do you think?
|
|
||
| if field_name not in self.ALLOWED_ORDERING_FIELDS: | ||
| raise serializers.ValidationError( | ||
| {f"Invalid ordering field for ASN aggregated feed: '{field_name}'. Allowed fields: {', '.join(sorted(self.ALLOWED_ORDERING_FIELDS))}"} |
There was a problem hiding this comment.
This is a set literal but should be a string, right?
| def resolve_aggregation_ordering(ordering, *, default, fallback_fields=None): | ||
| """ | ||
| Resolve effective ordering for aggregated endpoints. | ||
| Args | ||
| ordering (str or None): The user-provided ordering string from query params. | ||
| default (str): The default ordering to use if `ordering` is None or in fallback_fields. | ||
| fallback_fields (set[str], optional): A set of orderings that are allowed in other | ||
| contexts but should be overridden here. Defaults to None. | ||
| Returns | ||
| str: A safe ordering string to use directly in the aggregation query. | ||
| """ | ||
| fallback_fields = fallback_fields or set() | ||
|
|
||
| if not ordering or ordering in fallback_fields: | ||
| return default | ||
|
|
||
| return ordering |
There was a problem hiding this comment.
This is a little overengineered in my opinion. I think instead of this, we can just return -ioc_count in the ASNFeedsOrderingSerializer if the validation fails. Then, if a user requests a supported ordering, it just works and if not, the results are ordered by -ioc_count. Or am I missing something?
There was a problem hiding this comment.
The default ordering (-ioc_count) cannot be enforced in the serializer because by the time the aggregator runs, feed_params.ordering is already populated (default -last_seen) from FeedRequestParams. So the serializer never sees it as missing.
I acknowledge that using resolve_aggregation_ordering was over-engineered, and I agree that we can simplify it by adding a small override directly in asn_aggregated_queryset, like this:
if not ordering or ordering.strip() in {"", "-last_seen"}:
ordering = "-ioc_count"This keeps things simple while ensuring the default ordering works correctly. If you’re okay with it, I can push these changes but if you have a better approach, I’d love to apply that instead.
| resolved_ordering = resolve_aggregation_ordering( | ||
| ordering=feed_params.ordering, | ||
| default="-ioc_count", | ||
| fallback_fields={"-last_seen"}, | ||
| ) | ||
|
|
||
| direction = "-" if resolved_ordering.startswith("-") else "" | ||
| field = resolved_ordering.lstrip("-").strip() | ||
|
|
||
| aggregated = aggregated.order_by(f"{direction}{field}") |
There was a problem hiding this comment.
Also very complicated. Can we also rely on the Serializer here?
2 participants
Description
This change introduces a new authenticated API endpoint that aggregates IOC data by ASN. The endpoint groups all matching IOCs under their respective ASNs and computes summary statistics, including IOC count, total attack count, total interaction count, total login attempts, expected IOC count (derived from recurrence probability), and expected interactions. It also returns the set of unique honeypots associated with each ASN. The implementation reuses the same filtering and authentication logic as the Advanced Feeds API to avoid code duplication, while intentionally returning a JSON-only response tailored for aggregated data use cases.
Related issues
closes #458
Type of change
Please delete options that are not relevant.
Checklist
develop.Black,Flake,Isort) gave 0 errors. If you have correctly installed pre-commit, it does these checks and adjustments on your behalf.Important Rules