Skip to content
This repository has been archived by the owner on May 14, 2020. It is now read-only.

Make ModSecurity CRS repositories easier to manage #1600

Open
soufianebenali opened this issue Oct 17, 2019 · 22 comments
Open

Make ModSecurity CRS repositories easier to manage #1600

soufianebenali opened this issue Oct 17, 2019 · 22 comments

Comments

@soufianebenali
Copy link
Contributor

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".

@dune73
Copy link
Contributor

dune73 commented Oct 17, 2019

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?

@bittner
Copy link
Contributor

bittner commented Oct 18, 2019

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.)

@spartantri
Copy link
Contributor

Interesting 👍

@dune73
Copy link
Contributor

dune73 commented Oct 18, 2019

@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.

@fgsch
Copy link
Contributor

fgsch commented Oct 18, 2019

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.

@bittner
Copy link
Contributor

bittner commented Oct 18, 2019

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.

@bittner
Copy link
Contributor

bittner commented Oct 19, 2019

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.

@dune73
Copy link
Contributor

dune73 commented Oct 19, 2019

Thank you for the clarifications.

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

@franbuehler
Copy link
Contributor

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?

@fzipi
Copy link
Contributor

fzipi commented Oct 19, 2019

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.

@bittner
Copy link
Contributor

bittner commented Oct 21, 2019

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?

@dune73
Copy link
Contributor

dune73 commented Oct 21, 2019

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.

@bittner
Copy link
Contributor

bittner commented Oct 22, 2019

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.)

@dune73
Copy link
Contributor

dune73 commented Oct 22, 2019

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.

@soufianebenali
Copy link
Contributor Author

Hi all,

I can join the Meetup in Bern Wednesday next week.

Thank you and see you there.

@dune73
Copy link
Contributor

dune73 commented Oct 24, 2019

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

@bittner
Copy link
Contributor

bittner commented Oct 30, 2019

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.

@bittner
Copy link
Contributor

bittner commented Nov 20, 2019

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!

@bittner
Copy link
Contributor

bittner commented Dec 10, 2019

A quick update on our current efforts:

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

@bittner
Copy link
Contributor

bittner commented Jan 16, 2020

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.

@bittner
Copy link
Contributor

bittner commented Jan 24, 2020

Quick update on the current status:

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

@bittner
Copy link
Contributor

bittner commented Feb 11, 2020

See also: #1684

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

No branches or pull requests

7 participants