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

DHCP DoS Logger for DHCP DoS Mitigation Feature #18947

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

asraza07
Copy link

Why I did it

Added code for new daemon process responsible for detecting and logging DHCP DoS attack attempts (violation of DHCP rate limit)

How I did it

Added service and handler files for new systemd process dhcp_dos_logger

How to verify it

tc show command is used to identify dropped packets due to rate limiting

@asraza07 asraza07 changed the title Buildimage Changes for DHCP DoS Mitigation Feature DHCP DoS Logger for DHCP DoS Mitigation Feature May 24, 2024
logger.log_info(f"Port {port}: Current DHCP drop counter is {dropped_count}")
drop_pkts[port] = dropped_count
else:
logger.log_warning(f"No new dropped packets found on port {port}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why no dropped packets (Line 47 and line 49 log_warning) is treated as abnormal but dropped packets is treated as expected(line 44 log_info)? With this logic, whether actual DHCP packet rate low than tc rate treated as abnormal?

Copy link
Author

Choose a reason for hiding this comment

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

Hi @yaqiangz , thanks for pointing that out. I have fixed this in the code. The systemd process will only log when dropped packets are observed, and it will do so as a warning, not as info.

@asraza07
Copy link
Author

@yaqiangz @lguohan , kindly help review this PR, thanks in advance!

Copy link
Contributor

@yaqiangz yaqiangz left a comment

Choose a reason for hiding this comment

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

LGTM

@ridahanif96
Copy link
Contributor

Hi @qiluo-msft can you pls help review and merged this PR. HLD is merged.

@ridahanif96
Copy link
Contributor

@qiluo-msft pls help merge this one

@ridahanif96
Copy link
Contributor

@qiluo-msft please help review this PR


# Main handler function
def handler():
"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add test case and check test coverage?

Copy link
Contributor

Choose a reason for hiding this comment

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

test coverage required for handler? currently, as per practice we don't add test cases and coverage here. The same is followed by all other features in image_config.

@ridahanif96
Copy link
Contributor

Hi Qi,

Please review this PR.

@wajahatrazi
Copy link

Hi @qiluo-msft / @xincunli-sonic

Changes have been made as per your suggestions, please review to merge this PR.

Regards

@wajahatrazi
Copy link

Hi @qiluo-msft / @lguohan

Kindly review and help merge this PR.

We are reaching out weekly, requesting for the above. Please support.

Regards

@ridahanif96
Copy link
Contributor

@qiluo-msft help merge this PR, pending for long, unable to understand merge delay!

@zhangyanzhao
Copy link
Collaborator

@lguohan this PR has been approved by reviewers, can you please check if you are ok to merge it? Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants