8000 Refactored and tested version of rule-ctl script by theseion · Pull Request #2193 · coreruleset/coreruleset · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Refactored and tested version of rule-ctl script #2193

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 19 commits into from
Dec 6, 2021
Merged

Refactored and tested version of rule-ctl script #2193

merged 19 commits into from
Dec 6, 2021

Conversation

theseion
Copy link
Contributor
@theseion theseion commented Sep 6, 2021 •
edited
Loading

This updates #2068.

The contents of this PR are not yet perfect but should be a good starting point.

@dune73 dune73 mentioned this pull request Sep 6, 2021
Closed
@airween airween self-assigned this Sep 7, 2021
@airween
Copy link
Contributor
airween commented Sep 21, 2021

Hi @theseion,

I had some time and started to review this modified script. Here are some notes after the reviewing of tag handling.

  • in the examples there is python3 util/rule-ctl/rule-ctl.py but the correct path is python3 util/rule_ctl/rule_ctl.py (underscores instead of hyphens)
  • would be fine to set the shebang (#!/usr/bin/env python3) and set the executable flag
  • --add-tag adds the tag into the chained rules too, and not just the first part (tag will duplicated inside of chained rules)
    I saw the --skip-chained switch but it has no effect
  • I can't find the information how the --rename-tag works. I assume it expects two arguments, but a small example would be really good (I couldn't tested)
  • "--target-file: Set the target file where changes will be saved (default: use file set by --config)" - this does not work for me (the "default" condition):
python3 util/rule_ctl/rule_ctl.py --config rules/REQUEST-933-APPLICATION-ATTACK-PHP.conf --filter-rule-id ^933.+ --append-tag foo --skip-chain 
Traceback (most recent call last):
  File "util/rule_ctl/rule_ctl.py", line 793, in <module>
    run()
  File "util/rule_ctl/rule_ctl.py", line 790, in run
    write_output(context)
  File "util/rule_ctl/rule_ctl.py", line 776, in write_output
    with open(path, 'w') as handle:
TypeError: expected str, bytes or os.PathLike object, not NoneType

After a quick view, I found these issues.

@theseion
Copy link
Contributor Author

Great, thanks @airween! I'll get those fixed soon.

@theseion
Copy link
Contributor Author
theseion commented Oct 3, 2021

The simple stuff is fixed. I want to write tests for the other things so that will take a little more time.

This was referenced Oct 4, 2021
Closed
Closed
@theseion
Copy link
Contributor Author

@airween Updated with fixes and tests for all your finds. Thanks again!

@airween
Copy link
Contributor
airween commented Oct 19, 2021

@airween Updated with fixes and tests for all your finds. Thanks again!

Thanks, I'll take a look soon.

@dune73 dune73 mentioned this pull request Nov 1, 2021
Closed
@airween
Copy link
Contributor
airween commented Nov 13, 2021

Hi @theseion,

I had some time and could review the whole script.

First, let me congratulate you and @theMiddleBlue - I think this will be a very useful tool. It's also good that someone has finally used the parser :).

These are the items what I have found:

  • in the examples there is python3 util/rule-ctl/rule-ctl.py but the correct path is python3 util/rule_ctl/rule_ctl.py (underscores instead of hyphens)
  • \ No newline at end of file
  • still can't find the information how the --rename-tag works. I saw in the help output that it expects the old and the new tags separated by a comma - still would be placed this into the documentation
  • it's not clear for me, what's the different between --replace-variable and --replace-variable-name
  • the --replace-variable didn't work for me
  • transformation has added into a new line, but CRS stores all of them in single line
  • if I remove a transformation, which is in same line as the others, then the next line has moved into the previous line:
-    t:none,t:lowercase,\
-    msg:'PHP Injection Attack: PHP Script File Upload Found',\
+    t:none,msg:'PHP Injection Attack: PHP Script File Upload Found',\
  • the --uncond-replace-action doesn't work. Looks like it's not implemented yet (I haven't found in the source)
python3 util/rule_ctl/rule_ctl.py --config rules/REQUEST-933... --uncond-replace-action 'msg:this is a new message for rule 125'`
...
rule_ctl.py: error: unrecognized arguments: --uncond-replace-action msg:this is a new message for rule 125

no matter how do I quote the action

Actually, these are what I have found.

Let me know, if I can help you any other thing.

@dune73
Copy link
Member
dune73 commented Nov 29, 2021

Thanks for refactoring @theseion and the feedback @airween.

Do you have time to look into the comments @theseion?

How does it look from your side @theMiddleBlue?

@theseion
8000 Copy link
Contributor Author

Thanks for the reminder. Yes, I should have time to address the comments.

@dune73
Copy link
Member
dune73 commented Nov 30, 2021

Thank you.

@theseion
Copy link
Contributor Author
theseion commented Dec 1, 2021 •
edited
Loading

Thanks a lot @airween!

First, let me congratulate you and @theMiddleBlue - I think this will be a very useful tool. It's also good that someone has finally used the parser :).

Thanks :)

These are the items what I have found:

  • in the examples there is python3 util/rule-ctl/rule-ctl.py but the correct path is python3 util/rule_ctl/rule_ctl.py (underscores instead of hyphens)

Fixed

  • \ No newline at end of file

Will fix.

  • still can't find the information how the --rename-tag works. I saw in the help output that it expects the old and the new tags separated by a comma - still would be placed this into the documentation

It's already in the documentation (lines 53 and 74 of the README).

  • it's not clear for me, what's the different between --replace-variable and --replace-variable-name

--replace-variable-name only replaces the name of a variable, e.g. from TX to DOH. --replace-variable can also change the following:

  • modify the counter (&TX -> TX)
  • modify negation (!TX -> TX)
  • modify the "variable" part (TX:homer -> TX:marge)
  • modify quotes (no quotes, single, double)

I will add documentation and a test.

  • the --replace-variable didn't work for me
  • transformation has added into a new line, but CRS stores all of them in single line

I'll add a test for that.

  • if I remove a transformation, which is in same line as the others, then the next line has moved into the previous line:
-    t:none,t:lowercase,\
-    msg:'PHP Injection Attack: PHP Script File Upload Found',\
+    t:none,msg:'PHP Injection Attack: PHP Script File Upload Found',\

I'll add a test for that.

  • the --uncond-replace-action doesn't work. Looks like it's not implemented yet (I haven't found in the source)

I removed it as --replace-action can do the same. I'll update the documentation.

@theseion
Fixed variable replacement
35355b3 
Variable replacement can now also change the quote type
@theseion
Copy link
Contributor Author
theseion commented Dec 3, 2021

@airween I've addressed all your findings. From my point of view this is ready to merge. More bugs will probably crop up when people start using the script in more exotic contexts but with the test suite they should be pretty easy to fix.

@dune73
Copy link
Member
dune73 commented Dec 3, 2021

Cool stuff.

@fzipi fzipi mentioned this pull request Dec 4, 2021
Closed
@airween airween merged commit 675ecd2 into coreruleset:v3.4/dev Dec 6, 2021
@dune73
Copy link
Member
dune73 commented Dec 6, 2021

Thank you @theMiddleBlue, @theseion and @airween. Gorgeous script!

@theseion theseion deleted the rule-ctl-refactoring branch December 7, 2021 06:27
@fzipi fzipi mentioned this pull request Jan 2, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@airween airween

Labels
👀 Needs action ⏲️ In progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@theseion @airween @dune73 @theMiddleBlue
0