Owasp makefile

[lfservin]

This Pull Request relates to issue #1120. Fixes broken build and improves on the build process

[tghosth]

I need to review this to understand what exactly is happening here. Please don't merge it until I have done so

[lfservin]

Glad to have a quick call to go over it together. Let me know your availability

[tghosth]

OK that is great, thanks for your efforts on this.

Pulling in @ike as well.

Before I merge this can you confirm where we currently are with the new outputs generation. Specifically, can you confirm: a) Which output formats are now simply supported out of the box. b) Does the makefile support the translations. c) Have you verified that the output formats match the existing ones generated using the legacy approach?

[lfservin]

I'll reply to @tghosth in-line:

Before I merge this can you confirm where we currently are with the new outputs generation. Specifically, can you confirm: a) Which output formats are now simply supported out of the box.

make all will produce these formats: pdf docx json json_flat csv xml as can be seen here

b) Does the makefile support the translations.

Version 5 has no translations. The more interesting question is whether the non-pdf formats are also to be produced?

c) Have you verified that the output formats match the existing ones generated using the legacy approach?

When applied to 4.0 branch the result was the same, though the pdfs did look different. The other formats shouldn't be affected since they are still produced with the python scripts.

[tghosth]

ok so d) can we make both work the old mechanism and the new mechanism work in both 4.0 and 5.0 so that we can verify that everything matches? I don't mind that the PDF looks different as long as it looks similar to the DOCX and it looks good.

[ike]

@tghosth this is a good idea for testing parity. I am going to test this branch out this week, and I can attempt to verify this.

[ike]

@lfservin I am getting some PDF errors on build. I am gonna keep working thru them, but it might be worth looking into.

Also, how can we make the certificate copy optional? Do you have a preferred method? Could we put a dummy cert in there as the default?

[lfservin]

I thought the cert was already optional.

Could you help me with the error message? I don't get one. Or rather I was getting one and fixed it in the pull request I made, now that I remember.

Check out my pull request. It builds clearly with that. I used it to demo the process at the appsec conference Europe.

[tghosth]

Thanks so @ike and @lfservin please let me know when you have confirmed the parity between them

[ike]

Thanks @tghosth, hoping to get that parity check completed soon.

---

@lfservin here's what I've got.

I thought the cert was already optional.

When I try to build with the cert, I get this error.

COPY failed: file not found in build context or excluded by .dockerignore: stat none: file does not exist

When I comment out these lines, I can build successfully. https://github.com/OWASP/ASVS/blob/cb3249636bf794b85df20abd5cfd5042add06039/docker/Dockerfile#L29-L31

Could you help me with the error message? I don't get one. Or rather I was getting one and fixed it in the pull request I made, now that I remember.

The Error message I am getting is this:

! Package fontspec Error: The font "DejaVu Sans" cannot be found.

Check out my pull request. It builds clearly with that. I used it to demo the process at the appsec conference Europe.

It's very possible this has something to do with my own docker environment. I am running this on Manjaro linux with Docker 20.10.17.

[ike]

Okay those changes fixed the build! The cert thing is still not optional for me. I can test the parity now tho! Thanks.

[tghosth]

Ok so @ike will you let me know when you have run the parity check? Note also that there are merge conflicts...

[lfservin]

Okay those changes fixed the build! The cert thing is still not optional for me. I can test the parity now tho! Thanks.

In moving to alpine some things have been lost. Among them the optional cert, which is part of this dockerfile. I'll merge with the upstream changes and push again. I'll fix the docker file in the main branch as well

[tghosth]

So just let me know how you both progress :)

[tghosth]

@ike @lfservin any update on this?

[tghosth]

@ike @lfservin any update on this?

[ike]

Alright, I've done some work to get the legacy 4.0 generation to work here. A breakdown of what's happening here:

• A docker container is used to do all the builds. It is configured with the dependencies for both the old-style generation, and @lfservin's new templating system for 5.0.
• For 4.0, we use the generate-all.sh, as usual, but we run it in the docker container.
• For 5.0+, we use the new templating system

My changes to bring this branch to-date with upstream, and to re-enable legacy 4.0 generation are here: https://github.com/lfservin/ASVS/pull/1

Specifically, these changes enable legacy building:

• use the new dicttoxml2 library: https://github.com/lfservin/ASVS/pull/1/files#diff-d413ac48c09b879e36bf0184c616188f786cfc81cd8fd070f9ead443b35f579dR35
• re-enable the bash entrypoint, for conditional useage of the legacy generation tools: https://github.com/lfservin/ASVS/pull/1/files#diff-f34da55ca08f1a30591d8b0b3e885bcc678537b2a9a4aadea4f190806b374ddcR158
• update the new location of the python tools in 4.0: https://github.com/lfservin/ASVS/pull/1/files#diff-2ec59b3a9122978cd5acc5b2d951b906f613d0f5dfc359af6794db2d3c6b7bebR13

I know that @lfservin is hoping to get 4.0 to build using the new templates, which would be great! It would be awesome to have built-in pdf generation again for 4.0 (to my knowledge we haven't had this since 2018?). I do think we should continue that work in an additional MR, and begin testing the new 5.0 templates in earnest.

[tghosth]

ok so @ike do you think this is ready to merge? Also, you might want to throw things at me but can we just make this a GitHub action so that each time we commit to master it rebuilds the docs?

[ike]

Okay, this branch now supports multi-language in 5.0. Here is an example of the output, borrowing the French 4.0 localization: build-preview.zip

English 5.0 PDF from the new build process: OWASP_Application_Security_Verification_Standard-5_en.pdf

[ike]

@tghosth

ok so @ike do you think this is ready to merge? Also, you might want to throw things at me but can we just make this a GitHub action so that each time we commit to master it rebuilds the docs?

I need to verify 4.0 multi-language support, but once that's done I think it's ready to merge. We also need to test this on someone else's computer :)

Yes, I think a GitHub action would be feasible. Would be great to have this all automated! I'll make another issue and can report any research there.

[tghosth]

Thanks @ike, I want to test this but it looks like there are a lot of unrelated changes in this PR, are you able to trim it down to only the most crucial changes?

[ike]

I will trim it down as best I can yes! It is a fairly substantial change, on the face of it.

[ike]

@tghosth

Thanks @ike, I want to test this but it looks like there are a lot of unrelated changes in this PR, are you able to trim it down to only the most crucial changes?

Okay, should be ready to test. If you have docker running, you should be able to run make in the root directory, and it will build the docker image, and then build 4.0 and 5.0.

Let me know if you run into any issues.

[tghosth]

@ike

I get the following error after running make (this is the 2nd time, the first time it built the container and then gave me the error)

if ! docker image inspect asvs/documentbuilder:latest --format="checking docker image" &> /dev/null; then \
        docker build --build-arg CERT_FILE=docker/cert --tag asvs/documentbuilder:latest --network host docker; \
fi;
docker run --rm -v "`pwd`/5.0:/data" -v "`pwd`/docker:/scripts" -e "TARGET=5.0" -e "FORMATS=" asvs/documentbuilder
make: *** No rule to make target '/data/dist', needed by '/data/dist/en'.  Stop.
make: *** [Makefile:6: 5.0] Error 2
josh_grossman@cloudshell:~/ASVS$ 

(I am trying in Google cloud shell)

[ike]

@tghosth good catch! should be fixed now.

[tghosth]

Hi @ike

I added some review comments.

So as I understand it, "make" does not generate the 4.0 outputs, right? Just the 5.0 output? To be honest, I think that is ok but please can you run the old output generation code (using "generate-all.sh") to generate the CSV, JSON and XML outputs are still the same after your changes for 4.0 and also please can you run the old scripts for 5.0 so that I can see that the CSV, JSON and XML outputs for 5.0 using the new mechanism are the same as if they had been exported using the old mechanism.

Thanks,

Josh

[ike]

@tghosth it should be running 4.0 too -- I'll investigate if that's not the case. The expected behavior is that the makefile will generate all versions 4.0+, and use generate-all.sh for 4.0 specifically.

Thanks for the review -- I'll get back to you when these changes are made.

[tghosth]

Thanks, to be clear I would like you to generate the outputs using the old code for both 4.0 and 5.0 to demonstrate that 4.0 is the same as what the functionality in the master branch will generate and to demonstrate that the 5.0 files generated by the new method are the same as the files generated by the old method

[ike]

@tghosth okay, here's a zip file with the documents created by both old and new generators for both 4.0 and 5.0 in English. Let me know if there's any discrepancies!

compare_old_new_generators.zip

[tghosth]

Thanks for that. I am a little unsure how you reached these outputs as the files in the "old-method-4.0" folder are different to the output files in the current master branch which I would not expect. Please can you make sure you are using the correct "old" output generation scripts.

Example below: