search

SpiderLabs / owasp-modsecurity-crs

OWASP ModSecurity Core Rule Set (CRS) Project (Official Repository)
https://modsecurity.org/crs
Apache License 2.0
2.45k stars 727 forks source link

Make ModSecurity CRS repositories easier to manage #1600

Open soufianebenali opened 5 years ago

soufianebenali commented 5 years ago

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:

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:

2. Refactor modsecurity-docker

Planned tasks:

3. Refactor modsecurity-crs-docker

Planned tasks:

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Interesting :+1:

dune73 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

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 commented 5 years ago

Thank you for the clarifications.

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

franbuehler commented 5 years ago

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.

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 commented 5 years ago

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 commented 5 years ago

Thanks for the valuable feedback everyone!

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

Would that be okay for everyone?

dune73 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Hi all,

I can join the Meetup in Bern Wednesday next week.

Thank you and see you there.

dune73 commented 5 years ago

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

bittner commented 5 years ago

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 commented 5 years ago

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

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 commented 4 years ago

A quick update on our current efforts:

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

bittner commented 4 years ago

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 commented 4 years ago

Quick update on the current status:

Thanks everyone for watching and listening! Feedback is always welcome! :pray:

bittner commented 4 years ago

See also: #1684