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

feat(api): update WAF lists #797

Merged
merged 1 commit into from
Aug 10, 2024
Merged

feat(api): update WAF lists #797

merged 1 commit into from
Aug 10, 2024

Conversation

favonia
Copy link
Owner

@favonia favonia commented Jul 14, 2024

Close #646. Close #684.

Progress

  • Implement and test the new low-level update operations in api
  • Implement, document, and test the high-level update operations in setter
  • Implement, document, and test the detect-and-update logic in updater
  • Implement, document, and test the new logic in config
  • README
    • Advertisement at the top
    • Detailed option explanations

UI

  • WAF_LISTS=list1,list2
  • WAF_LIST_DESCRIPTION=blah blah blah

Design issues

  • Delete all lists when exiting if DELETE_ON_STOP=1? YES
  • Should IPv4/6 addresses be updated if no IPv4/6 was detected? NO
  • Preserving comments and all matching ranges? YES

Copy link

codecov bot commented Jul 14, 2024

Codecov Report

Attention: Patch coverage is 98.98734% with 4 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@bdf154c). Learn more about missing BASE report.

Files Patch % Lines
internal/api/cloudflare_waf.go 98.05% 2 Missing and 1 partial ⚠️
internal/setter/setter.go 97.91% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #797   +/-   ##
=======================================
  Coverage        ?   94.71%           
=======================================
  Files           ?       58           
  Lines           ?     2573           
  Branches        ?        0           
=======================================
  Hits            ?     2437           
  Misses          ?      123           
  Partials        ?       13           
Flag Coverage Δ
unittests 94.71% <98.98%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@favonia favonia changed the title Update lists feat(api): update IP lists Jul 15, 2024
@favonia favonia force-pushed the update-lists branch 12 times, most recently from 0e997a9 to 5fe86fb Compare July 18, 2024 07:17
@favonia favonia changed the title feat(api): update IP lists feat(api): update WAF lists Jul 18, 2024
@favonia favonia force-pushed the update-lists branch 10 times, most recently from f93c439 to c567a9a Compare July 19, 2024 12:43
@favonia
Copy link
Owner Author

favonia commented Jul 19, 2024

@WolfRamAlpha12 @jdvuyk @Gamerou I am happy to report that the code in this PR is working (updating my personal list as expected)! But I still need to write good messages, test cases, and documentation. Some design questions for you:

  1. Do WAF_LISTS and WAF_LIST_DESCRIPTION look intuitive and reasonable to you? (That is, do you know how to use them without reading the documentation?)
  2. If there's already a very wide IP range covering the detected IP, should we keep it intact? For example, maybe the IP is 10.0.0.1 and there's already a super wide range 10.0.0.1/8 in the list. Should we keep the wide range instead?
  3. The current code will destroy all comments attached to individual IP addresses/ranges and might not preserve their order between updates. Please let me know if this would be an issue. (I might change the code to preserve all comments before merging this PR, though.)

@Gamerou
Copy link

Gamerou commented Jul 19, 2024

@WolfRamAlpha12 @jdvuyk @Gamerou I am happy to report that the code in this PR is working (updating my personal list as expected)! But I still need to write test cases and documentation. Some design questions for you:

  1. Do WAF_LISTS and WAF_LIST_DESCRIPTION look intuitive and reasonable to you? (That is, do you know how to use them without reading the documentation?)

  2. If there's already a very wide IP range covering the detected IP, should we keep it intact? For example, maybe the IP is 10.0.0.1 and there's already a super wide range 10.0.0.1/8 in the list. Should we keep the wide range instead?

  3. The current code will destroy all comments attached to individual IP addresses/ranges and might not preserve their order between updates. Please let me know if this would be an issue. (I might change the code to preserve all comments before merging this PR, though.)

Hi,

Great to hear that the code is working as expected! Here are my thoughts on your design questions:

  1. WAF_LISTS and WAF_LIST_DESCRIPTION:
    They seem intuitive and reasonable to me. Without having seen the documentation, I can understand their intended use.

  2. Handling Wide IP Ranges:
    If there's already a very wide IP range covering the detected IP, it might be wise to keep it intact. For example, if the IP is 10.0.0.1 and there's already a range like 10.0.0.1/8, keeping the wide range could prevent redundancy and maintain a cleaner configuration.

  3. Preserving Comments and Order:
    Destroying comments and potentially losing their order could be problematic, especially if those comments hold significant context or notes that help in managing the IPs/ranges. Preserving comments and their order would be beneficial for maintaining clarity and ease of management. If feasible, updating the code to retain comments before merging the PR would be a good idea.

Please let me know if you need further input or assistance! And again: Thank you very much for doing this for us!

@favonia
Copy link
Owner Author

favonia commented Jul 19, 2024

@Gamerou Thank you. About the (wide) IP ranges and their order, I have a follow-up question. Suppose the current list is

99.0.0.0/20
10.0.0.0/8
10.0.0.0/16
10.0.0.1
99.0.0.0/10
10.0.0.0/24

and the detected IP is 10.0.0.1. Do you want to keep all items starting with "10" in their current order (but remove the two entries starting with "99")? That is, should the new list be

10.0.0.0/8
10.0.0.0/16
10.0.0.1
10.0.0.0/24

@favonia favonia force-pushed the update-lists branch 24 times, most recently from 77d877d to 740cc0f Compare August 10, 2024 09:14
@favonia
Copy link
Owner Author

favonia commented Aug 10, 2024

The current testing misses 4 lines of code and the documentation has not started, but this PR is dragging for too long... let's merge it!

@favonia favonia enabled auto-merge (squash) August 10, 2024 09:19
@favonia favonia merged commit 180bcd7 into main Aug 10, 2024
8 checks passed
@favonia favonia deleted the update-lists branch August 10, 2024 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants