Skip to content
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

ENH: Improved Abuse.ch Feodo Tracker parser bot and documentation #2268

Merged
merged 1 commit into from
Jan 30, 2023

Conversation

gethvi
Copy link
Contributor

@gethvi gethvi commented Nov 15, 2022

This PR replaces two duplicate feeds with an unified and improved one with greatly simplified code. The JSON source from feodotracker.abuse.ch has the most complete data (compared to CSV and Browse HTML table).

@gethvi gethvi force-pushed the feodo-parser branch 5 times, most recently from 687d1da to 8e1e9f3 Compare November 22, 2022 12:40
@gethvi
Copy link
Contributor Author

gethvi commented Nov 23, 2022

Tests are failing because of #2278. Merging the PR #2279 should fix the failing tests.

@sebix
Copy link
Member

sebix commented Dec 21, 2022

Tests are failing because of #2278. Merging the PR #2279 should fix the failing tests.

PR #2279 is now merged

Copy link
Member

@sebix sebix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm missing the upgrade path. All user which have any of the two parsers (domain or IP) in place, will run into troubles with the upgrade.
Please add the removal of the two bots to the NEWS file and add an upgrade function to intelmq/lib/upgrades.py.

@gethvi gethvi force-pushed the feodo-parser branch 4 times, most recently from 6bdbc6d to 375474e Compare January 2, 2023 15:27
@gethvi gethvi requested a review from sebix January 2, 2023 15:38
@gethvi gethvi force-pushed the feodo-parser branch 4 times, most recently from 5e55172 to f31684e Compare January 5, 2023 12:51
@gethvi
Copy link
Contributor Author

gethvi commented Jan 10, 2023

This also fixes #1499.

Copy link
Member

@sebix sebix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for rewriting this parser and simplifying the setup by doing so.

I suggest to migrate the configuration directly instead of asking the user to do so. The user doesn't have a choice anyway.

Comment on lines +840 to +852
messages.append('A discontinued feed "Abuse.ch Feodo Tracker IPs" has been found'
'as bot %s.\nPlease manually replace with the feed'
'"Abuse.ch Feodo Tracker".' % ', '.join(sorted(found_abusech_feodotracker_csv)))

if found_abusech_feodotracker_browse:
messages.append('A discontinued feed "Abuse.ch Feodo Tracker Browse" has been found'
'as bot %s.\nPlease manually replace with the feed'
'"Abuse.ch Feodo Tracker".' % ', '.join(sorted(found_abusech_feodotracker_browse)))

if found_abusech_removed_parsers:
messages.append('A discontinued bot module has been found'
'as bot %s.' % ', '.join(sorted(found_abusech_removed_parsers)))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not changing the config directly? Replacing the module and adapting the http_url can be fully automated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made the explanation into an issue #2297.

Copy link
Contributor Author

@gethvi gethvi Jan 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you insist on changing the config directly I will do it. Just let me know.

However as a user I prefer only being notified that I need to change to the config. I like to understand the necessary changes even if it requires more (manual) effort. That's why I implemented the upgrades this way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd believe that the change shouldn't be done automatically - but it could be suggested. Either as "run command xyz to automatically update documentation" or "please change following things ... ".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either as "run command xyz to automatically update documentation"

Originally that was the purpose of the intelmqctl upgrade-config command.

intelmq/tests/test_conf.py Outdated Show resolved Hide resolved
@sebix sebix merged commit 680ce90 into certtools:develop Jan 30, 2023
@sebix sebix mentioned this pull request Feb 7, 2023
@gethvi gethvi deleted the feodo-parser branch March 1, 2024 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants