Extra note of caution: the latest commit ec8c8f6 updates the CI so that all scanners are built with the new Makefiles. It would be really nice if someone could give it a review and trigger a run.

Some more thoughts:

• I remember that in the previous set-up, the integration tests for some scanners were disabled. Do we want to keep that behavior?
• In the ZAP advanced makefile, the ZAP parser is automatically built in the docker-build target. In the pipeline, we instead use the docker build push action and detect whether to build the parser/scanner based on the presence of their Dockerfile. Any good ideas to ensure that in the pipeline the ZAP parser is built before the ZAP advanced job starts?

Replace CI amass job by Github Matrix / run them one by one in a single job (open for discussion; I believe kind cluster create is pretty expensive in the pipeline and a matrix job might explode that time..; on the other hand matrix would reduce oveall pipeline duration if the pipeline has enough resources)

Yes good point. We've planned to change the pipeline so that it looks for changed files in the relevant folder. So that we would only run the builds for a specific scanner if files for it were changed, or if a central component e.g. operator, lurker, parser-sdk were changed. This should speed this up quite a lot as most pipelines would only run the build and tests for the changed scanner(s).

I remember that in the previous set-up, the integration tests for some scanners were disabled. Do we want to keep that behavior?

I think the only intentionally disabled integration-tests are the ones for the zap-advanced as they were just taking too long to run the scans. That should hopefully also be kinda solved by only running the tests only when the acutal zap-advanced code was changed.

In the ZAP advanced makefile, the ZAP parser is automatically built in the docker-build target. In the pipeline, we instead use the docker build push action and detect whether to build the parser/scanner based on the presence of their Dockerfile. Any good ideas to ensure that in the pipeline the ZAP parser is built before the ZAP advanced job starts?

Ah yes, thats a kind of edge case 🤔 Could we overwrite the build parser make targets in the zap-advanced makefile enought to just build the parser form the zap folder? I think this would be easier than reusing the image build for zap.

Checked the pr out and tried the nmap makefile looks great already 🥳
I think there needs to be a small fix to the parser js test imports, will push these and trigger the ci.

Yes good point. We've planned to change the pipeline so that it looks for changed files in the relevant folder. So that we would only run the builds for a specific scanner if files for it were changed, or if a central component e.g. operator, lurker, parser-sdk were changed. This should speed this up quite a lot as most pipelines would only run the build and tests for the changed scanner(s).

That would be awesome. For now, lets keep this PR with a matrix job and once we're fully over to Makefiles, we can do the extra magic.

I think the only intentionally disabled integration-tests are the ones for the zap-advanced as they were just taking too long to run the scans. That should hopefully also be kinda solved by only running the tests only when the acutal zap-advanced code was changed.

You're right that zap-advanced was manually disabled, however I noticed that git-repo-scanner was never included. I can imagine that was due to GitHub rate-limiting?

Ah yes, thats a kind of edge case thinking Could we overwrite the build parser make targets in the zap-advanced makefile enought to just build the parser form the zap folder? I think this would be easier than reusing the image build for zap.

Yep, that's currently what happens in the Makefile for zap-advanced, check it out! However, in the pipeline, we make use of the docker action, skipping the Makefile's build target altogether. For your proposal to work in the pipeline, we would have to get rid of the Docker build action. Do you know if the ubuntu-latest VM runs docker build commands properly? In any case, that would also require the Makefile to also manage the tags and pushes right?

Checked the pr out and tried the nmap makefile looks great already partying_face
I think there needs to be a small fix to the parser js test imports, will push these and trigger the ci.

Great!

You're right that zap-advanced was manually disabled, however I noticed that git-repo-scanner was never included. I can imagine that was due to GitHub rate-limiting?

yeah rate limiting is a problem, also we try to keep the integration test completly offline, that pretty hard to do with the git-repo-scanner. We should "just" be able to disable these tests for now by overwriting the integration-tests make target in the makefiles for these scanners to a empty script, right? As a temporary workaround, running these only when the code was changed should hopefully decrease the number of requests enough to be pretty stable.

Yep, that's currently what happens in the Makefile for zap-advanced, check it out! However, in the pipeline, we make use of the docker action, skipping the Makefile's build target altogether. For your proposal to work in the pipeline, we would have to get rid of the Docker build action. Do you know if the ubuntu-latest VM runs docker build commands properly? In any case, that would also require the Makefile to also manage the tags and pushes right?

Mhh yeah... I'm not sure how easily we can / should migrate from the action, i does a lot of neat stuff in the background. I'll try to see if I can came up with a good solution to this 🤔

I'll try to see if I can came up with a good solution to this 🤔

@J12934 how's that going? I would like to finish the PR.

Hi @EndPositive
thanks for the reminder 🙏

I think the best way forward is, to do it like you suggested it by splitting out the "release docker build" to a seperate release pipeline and use the makefile builds in the normal commit / (and future pr) pipeline. Pushed this to a seperate branch just in case: https://github.com/secureCodeBox/secureCodeBox/tree/makefiles-split-pipeline
Looking pretty good already. Release pipeline will probably require a few test / beta releases to get completely right.

Will talk this over with some other team members tomorrow, and will create separate github issues to track and organize the remaining work on this feature.

@J12934 that sound good 😄.

Some points for your tracker:

• Migrate unit-tests to Makefiles completely (including CodeClimate support ?)
• Create Makefiles for hooks
• Ensure DockerHub descriptions are only updated on release
• Add Makefiles for Nikto (special case)
• Optionally add Makefiles for newly introduced scanners (WhatWeb, TYPO3, and Nuclei)
• Update secureCodeBox documentation related to integration tests and creating new scanners.

Let me know how the team meeting goes 😃

👋

Talked this over with my coworkers today, they are all exited about the makefiles and their possibilities and were all on board with the proposed changes.
I've opened up the related issues #610 , #611 , #612 , #613 plus we already had makefiles related issues the "old issues" #477 & #478.

Which unit tests are not yet migrated to the makefiles?
Just the zap-advanced & git-repo-scanner python tests?

I'm not sure if there is a good way to integrate code-climate into the makefiles, maybe the underlying linting tools it uses under the hood (like eslint) but I'm not sure how easy that would be to setup correctly (especially for the different languages we have in use)

Awesome work @J12934 !

Do you guys want to make a release (with the new scanners) before we start messing up main and the pipeline?

Also, let's try to keep the work a bit separated, so contributing becomes easier. Keep this PR only on creating makefiles for scanners (no pipeline changes) and then I'll create another PR for hook makefiles. Maybe you can then look at updating the pipeline, since I'm unable to properly test not being authorized to trigger pipelines.

Since this PR has gotten too bloated with further pipeline testing, I've decided to completely separate the changes for Makefiles and those for the pipeline. I'll close this PR in favor of #622 and further PR's.