8000 Make ModSecurity CRS repositories easier to manage · Issue #1600 · coreruleset/coreruleset · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Make ModSecurity CRS repositories easier to manage #1600

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

Closed
CRS-migration-bot opened this issue May 13, 2020 · 23 comments
Closed

Make ModSecurity CRS repositories easier to manage #1600

CRS-migration-bot opened this issue May 13, 2020 · 23 comments
Labels
⌛ Stale issue This issue has been open 120 days with no activity.

Comments

@CRS-migration-bot
Copy link

Issue originally created by user soufianebenali on date 2019-10-17 15:11:02.
Link to original issue: SpiderLabs/owasp-modsecurity-crs#1600.

As discussed by csanders-git and bittner on Slack, and related to #1346 and #1420, we're proposing to simplify the repository structure and branching model of all repositories related to ModSecurity CRS.

  1. SpiderLabs/owasp-modsecurity-crs
  2. CRS-support/modsecurity-docker
  3. CRS-support/modsecurity-crs-docker

In a nutshell, we propose to:

  • flatten the branches in the first 2 repos above into a single branch,
  • placing the content of the branches in folders in that main branch, and
  • move the maintenance of the owasp/modsecurity-crs Docker image to a dedicated repository.

We also think it's worth to align the naming/wording with other popular free software projects and common best practices.

1. Refactor owasp-modsecurity-crs

Planned tasks:

  • Create a new folder tests in the root folder
  • Move util/regression-tests/ -> tests/regression/, and util/integration/ -> tests/integration
  • Rename folder documentation/ to docs/
  • Create folder examples/ and move crs-setup.conf.example -> examples/crs-setup.conf
  • Inside the rules/ folder create a folder for every version of rules, e.g. rules/v3.1/, rules/v3.2/, rules/v3.3/
  • Switch from branch-based versioning to folder-based versioning, on a single main branch (e.g. master)

2. Refactor modsecurity-docker

Planned tasks:

  • Switch from branch-based versioning to folder-based versioning, on a single main branch (e.g. master)
  • Clean up Dockerfile implementations for all supported combinations of ModSecurity and Apache/Nginx versions (inherit from existing, stable images as much as makes sense)
  • Automate building images for all supported combinations of ModSecurity and Apache/Nginx versions

3. Refactor modsecurity-crs-docker

Planned tasks:

  • Move the Docker setup from owasp-modsecurity-crs to the new modsecurity-crs-docker repository
  • Automate building images for all supported CRS versions on the various flavors of the modsecurity-docker images

Final comments

In essence, this is a flattening of the branching model, moving from a version-based branching to a trunk-based branching where the various versions (and technology combinations) are in subdirectories of the repository. The resulting repository structure should make it easier to overview and manage the code base.

A simple example of how this could look like may be appuio/container-oc. Please take a look at the structure and how we try to make updates easy by fully scripting the adaptions across all supported versions.

Please, let us know your thoughts! When we agree on this approach we would attempt doing the refactoring in a very short time frame, so the disruption is minimal and we can avoid any kind of "transition phase".

@CRS-migration-bot
Copy link
Author

User dune73 commented on date 2019-10-17 20:47:54:

Thank you for this detailed description. I've labelled it, so we will discuss this in the next community chat on Nov 4, 2030 CET on owasp.slack.com, channel coreruleset.
Are you and bittner available?

@CRS-migration-bot
Copy link
Author

User bittner commented on date 2019-10-18 16:19:34:

Monday, Nov 4 20:30 CET is possible in theory.

However, can we discuss the matters before that? This issue is actually coming out from an agreement with csanders-git a few weeks ago. He mentioned he would discuss it with some co-maintainers.

Full disclosure: We, vshn, are allocating resources for this project. It's important for us to ensure a solid basis of the software. There is no afterthought, we have no hidden agenda. (We love Open Source, and the alternatives to ModSecurity are closed source products.)

@CRS-migration-bot
Copy link
Author

User spartantri commented on date 2019-10-18 16:28:36:

Interesting 👍

@CRS-migration-bot
Copy link
Author

User dune73 commented on date 2019-10-18 22:01:17:

bittner, I knew you have been working on this for quite some time and I welcome the initiative. However, csanders-git has not discussed this with the rest of the project so far and I doubt he is making an agreement on behalf of our project that changes our complete repository structure without discussing it first. Having a contributor without commit rights tell a project about an agreement with a project leader sounds fishy. It's usually project leaders that announce agreements with non-team members. I suppose that's obvious.

There are very valid points in the proposal submitted by soufianebenali, but there are other aspects that are less valid or they are not immediately clear without the reasoning behind it. They may be obvious to you, but they are not obvious to me. I'm the n00b here, so if you manage to explain them in a way I get it, the rest of the team will also grasp it.

It is not impossible to have the project adopt important parts or most of your proposals. But it is going to be a hard sell, it is going to be a lot of work and it's going to take a lot of time.

For starters, explain us what is wrong with the way we are doing this. If you have explained this to csanders-git and you convinced him, then convince us too. Then explain why the proposed solution is better for all of us.

Finally: I see the refactoring of the crs repository as independent from the refactoring of the docker containers. Is this really the same issue? And if you are talking to csanders-git about docker, is franbuehler also part of the team? I know she is working on this too.

@CRS-migration-bot
Copy link
Author

User fgsch commented on date 2019-10-18 22:53:17:

I'd like to chime in.

There is a lot to unpack on this single issue. In the future it might be easier to have multiple issues so they are easier to discuss and the outcome of one doesn't affect the other.

I think there is merit on some of the changes proposed here, but they are largely aesthetic and therefore subjective as well.

I fail to see the justification for the folder-based versioning for owasp-modsecurity-crs however, nor how it would make things more manageable. In fact I believe this will be detrimental to the end user and I personally find this practice very odd.

Could you explain the reasoning behind this proposal in more detail so it can be evaluated with the full context?
Who is the target of these changes? What benefit will flattening the branches bring to the project, and in particular to the end user? Why would exploding all the branches inside a directory make things more manageable?

It'd be hard to find consensus without understanding what problems this is trying to fix.

@CRS-migration-bot
Copy link
Author

User bittner commented on date 2019-10-18 23:37:34:

Having a non-committer tell a project about an agreement with a project leader sounds fishy.

csanders-git Can you please speak up and clarify this?

Also, I am not a non-committer on this project.

It'd be hard to find consensus without understanding what problems this is trying to fix.

I fully agree. That is what this issue is all about. Otherwise we would have submitted a PR.

@CRS-migration-bot
Copy link
Author

User bittner commented on date 2019-10-19 00:56:41:

The problems we are hoping to solve:

Introduction

IIUC, this repository is meant to house the CRS, i.e. the rules files which make ModSecurity useful. Over time it has accumulated additional software and data in the spirit of adding tools, processes and artifacts to help manage the project. This is for the most part the content of the utils folder.

While there is value in the contributed tooling and structure, it makes understanding, reusing, extending and improving the code base hard. All proposed changes, therefore, aim at making it easier to understand, reuse, extend and improve - contribute to - this project.

In this vein the proposed changes are not to be seen as aesthetic modifications, but aim at an overall good state of the project (code base) for the sake of ease of maintenance, be it by unpaid volunteers - with typically limited resources - or corporations - who may have to justify the investment they have to take.

Guiding principles

  • Unix philosophy: Make a repository do one thing, and make it do that well.
  • Separation of concerns: Allow a higher degree of freedom for the single components of this project. Allow easier reuse as well as independent development and upgrade. Allow hiding implementation details behind (repository) interfaces.
  • Automation & QA: Avoid human error by making manual work reproducible. Incrementally add (automated) processes that remove manual labor, and allow to focus on discussion and thinking.
  • Do what has proven to work well: Be inspired by common practice in other popular free software projects. Make it easy to transfer knowledge for contributors experienced in other areas of software development. Do as others do.

Main areas

  1. This repository should house the CRS. - Documentation is already in a separate repository, that's good! The README is short and concise, that is awesome! - Apart from the tests, which probably make sense to be colocated with the rules they test, largely everything should be moved out (to a better place; we'll have to see where).
  2. Docker image code should go in separate repositories for each product. And it should be easy to make related changes (across versions, technologies, data) in a single commit.
  3. It should be easy to run tests. No laborious setup, in the cloud, requiring Git checkouts, etc. Simplicity makes things easier.

@CRS-migration-bot
Copy link
Author

User dune73 commented on date 2019-10-19 06:11:46:

Thank you for the clarifications.

Changed non-committer to contributor without commit rights. That's what I meant to say anyways.

@CRS-migration-bot
Copy link
Author

User franbuehler commented on date 2019-10-19 14:25:21:

I would like to chime in too.
Chaim and I discussed VSHN's idea of splitting the CRS Docker stuff into a new repository called modsecurity-crs-docker a few weeks ago. We both agreed that it is a good plan.
Then VHSN worked on a proposal and now it is here.

It is really hard to understand the current structure of the Docker stuff.

  • Different ModSecurity Docker images are built based on branches. We have a branch called v2/apache-apache for example.

  • Different CRS Docker images are built based on different Dockerfiles in the folder util/docker: Dockerfile-apache-2.9 for example.

We need to change it. It's not consistent.

I like VHSNs idea of working with folders to seperate the different NGINX/Apache-ModSecurity v2/v3 combinations.
And I totally agree that we need automated builds.
I agree with point 2 and 3.

I think point 1. Refactor owasp-modsecurity-crs is another topic. I don't like the idea of working with subfolders for the versioning of the CRS.

That is my point of view.

Well to make it even more complicated.
At our CRS Meetup in Bern a few weeks ago the idea of working with multistage builds came up. (Yes I know we already have, but to compile ModSec).
We could maintain ModSecurity and CRS in one Dockerfile and stop at a specific stage to build only ModSecurity Images.
First I don't know how common this setup is and second I don't know if it works for DockerHub.
Any opinions on this?

@CRS-migration-bot
Copy link
Author

User fzipi commented on date 2019-10-19 15:11:56:

I see the benefits of refactoring the CRS Docker repo.

I still can't see the benefits of merging branches to have everything in master in the CRS itself.

@CRS-migration-bot
Copy link
Author

User bittner commented on date 2019-10-21 14:00:27:

Thanks for the valuable feedback everyone!

It looks like there is a common understanding on what should be done. Hence, I'd suggest:

  • We tackle bullets 2 and 3 from above, refactoring and automating the building of the container images.
  • We'll also look at the details of the multistage build suggestion from above.
  • We'll align with franbuehler and csanders-git, and keep the rest of the community and maintainers informed transparently. Obviously, all the work will happen in the public here on GitHub.
  • We'll stick to only refactoring the content of the util folder and omit the flattening of the CRS rules repository branches from bullet 1 above. We'll apparently have to apply the changes to all existing branches.

Would that be okay for everyone?

@CRS-migration-bot
Copy link
Author

User dune73 commented on date 2019-10-21 14:13:27:

Thank you for your update bittner. This sounds doable.

From a project perspective, I would appreciate if we could separate CRS refactoring and the container stuff completely. Or are they depending on each other? If not, then let's do an issue for the CRS folder structure and a separate issue for the whole container setup.

Most of the people I talked to agree that we need to do something with the docker images (and we have been working on those for a long time, we're just not done yet, so any new energy is welcome). However with the CRS, we are quite happy with the way it is and even if the are not following best practices in the naming and structure of certain folders, the pain is a lot smaller there.

As for the container / docker: We are hosting another CRS Meetup in Bern Wednesday next week at Puzzle. Are you guys interested to join and we run a workshop to sort this out? We could do a conf call with other project members and come up with a proposal that we could then confirm at the
Nov 4 community chat on slack.

@CRS-migration-bot
Copy link
Author

User bittner commented on date 2019-10-22 07:55:15:

Absolutely!

  1. We'll create an issue and PRs in CRS-support/modsecurity-docker for the ModSecurity container images.
  2. We'll create an issue and PRs in CRS-support/modsecurity-crs-docker for the CRS container images.
  3. We'll create a PR here, in this repository for the minimal refactoring of the util folder. We'll probably need some support with adjusting the test setup and CI configuration related to that, but that can certainly happen in the PR.

We are verifying who of us will be able to attend the Meetup. (I already have an appointment for that evening, unfortunately.)

@CRS-migration-bot
Copy link
Author

User dune73 commented on date 2019-10-22 07:59:15:

If you could look into the multistage build until the meetup, that would be awesome.

Looking for a volunteer to guide you with the Travis integration now.

@CRS-migration-bot
Copy link
Author

User soufianebenali commented on date 2019-10-22 11:56:50:

Hi all,

I can join the Meetup in Bern Wednesday next week.

Thank you and see you there.

@CRS-migration-bot
Copy link
Author

User dune73 commented on date 2019-10-24 18:55:19:

Btw, fgsch is ready to help you with Travis, if there is a need.

@CRS-migration-bot
Copy link
Author

User bittner commented on date 2019-10-30 13:03:06:

As a preparation for today's Meetup we created two follow-up issues (see above).

As a logical consequence, in this repository it would make sense to make use of the then-built CRS image for running the test suite. The Docker build logic in the Travis configuration will be optional / unneeded at that point.

@CRS-migration-bot
Copy link
Author

User bittner commented on date 2019-11-20 09:35:15:

We opened PRs for part 1 of the discussed changes above.

  • The branches v3.0/dev and v3.0/master are obsolete (we didn't know that, sorry!) according to feedback in the PRs.
  • For v3.1/dev there doesn't seem be a corresponding v3.1/master branch in the repository (hence no PR for that).
  • Similar for v3.3/dev (which is probably fine).

Is there anything we should do with the master branch or any other branch?

Anything else we need to know or should have considered? Please let us know!

@CRS-migration-bot
Copy link
Author

User bittner commented on date 2019-12-10 19:35:05:

A quick update on our current efforts:

If you can help us get those tasks done or move faster your support is very welcome! 🙏

@CRS-migration-bot
Copy link
Author

User bittner commented on date 2020-01-16 10:20:34:

First 2020 update on our development efforts:

** We're currently waiting for a maintainer (csanders-git or s/o else?) to give us maintainer permissions on the modsecurity-crs-docker repository to allow us to add a token needed for the automated build and adjust settings. I also mentioned this on the OWASP Slack team a few days ago.

Please take a look at what we've done! We're happy about any feedback, be it critics or praise.

@CRS-migration-bot
Copy link
Author

User bittner commented on date 2020-01-24 10:03:04:

Quick update on the current status:

  • We have configured automatic builds successfully now also on the CRS-support/modsecurity-crs-docker repository, so that the new images are added to Docker Hub (image tags 3.3, 3.3-apache, apache, 3.3-nginx, nginx). For details see Create automated image builds modsecurity-crs-docker#1.
  • We now need to make sure that the test suite runs with (one of) the new image(s). Maybe fgsch can stand by for questions, in case we have any during the refactoring?
  • When this is done we can remove the entire util/docker folder from this repository. We'll open a PR for that.

Thanks everyone for watching and listening! Feedback is always welcome! 🙏

@CRS-migration-bot
Copy link
Author

User bittner commented on date 2020-02-11 20:39:55:

See also: #1684

@github-actions
Copy link
Contributor

This issue has been open 120 days with no activity. Remove the stale label or comment, or this will be closed in 14 days

@github-actions github-actions bot added the ⌛ Stale issue This issue has been open 120 days with no activity. label Sep 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⌛ Stale issue This issue has been open 120 days with no activity.
Projects
None yet
Development

No branches or pull requests

1 participant
0