Docker image improvements and Makefile targets #563

alchzh · 2025-01-07T20:58:22Z

I have reviewed the OFRAK contributor guide and attest that this pull request is in accordance with it.

One sentence summary of this PR (This should go in the CHANGELOG!)
Fix small issues with Docker images and add Makefile targets for creating/starting them.

Link to Related Issue(s)
#547

Please describe the changes in your request.

Reorder packages so ofrak_patch_maker comes first for better caching.
Add Makefile targets ofrak-, start-ofrak-, and super-start-ofrak-* for creating/starting Docker images
Change entrypoints in some docker images to auto accept the license (is this what we want to do? See Fix ofrak-dev, ofrak-ghidra, ofrak-tutorial entrypoints #547)

Anyone you think should look at this, specifically?

…ad section and building as well as expecting a license files to be included

rbs-jacob · 2025-01-08T17:33:12Z

I do not think we should move forward with most of the changes in this pull request.

The Makefile comments almost exclusively add noise, and are redundant – we don't need a comment for a single command that is the same length as the comment describing it
- In general, comments should document why decisions were made, not what the code is doing
The license terms need to be manually agreed by a user – there should not be code in the Makefile or entrypoint to automatically agree
Many of the complex Makefile functions should not be in the Makefile at all – part of the reason we have build_image.py is to avoid complex, hard-to-maintain logic in Makefiles and shell scripts

The only change that might be alright is changing the order of ofrak_patch_maker in the YAML files, and I'm hesitant about that. It would be good for caching, but the order in which OFRAK packages are installed in Docker impacts whether the OFRAK version we are trying to build from source gets overridden by the pip version. We need to manually confirm that this is not happening in the images that get built before I am okay with moving ahead on this particular subset of the proposed changeset.

ANogin · 2025-01-13T02:11:38Z

The only change that might be alright is changing the order of ofrak_patch_maker in the YAML files, and I'm hesitant about that. It would be good for caching, but the order in which OFRAK packages are installed in Docker impacts whether the OFRAK version we are trying to build from source gets overridden by the pip version.

Yes. the order in yaml must agree with the dependencies.

We need to manually confirm that this is not happening in the images that get built before I am okay with moving ahead on this particular subset of the proposed changeset.

We need to finally land #218, then the check would no longer have to be manual.

Jepson2k · 2025-01-13T03:01:37Z

@rbs-jacob I can remove the comments and the setup.py related commands but otherwise I think the rest of the changes are worthwhile. The current method for downloading, building, and running ofrak in a container as outlined by the documenta is cumbersome. Moreover there is no easy way to run the tests locally that are ran on the remote (github) without examining the test-all.yml. Which again, in my opinion, is cumbersome. Users shouldn't have to have a deep understanding of the ofrak build system in order to simply build and run it in a container.

Take the following two key changes for example:

.PHONY: ofrak-%
ofrak-%: ## Build OFRAK image using ofrak-<name>
	@echo "Building OFRAK image using config: ofrak-$*.yml"
	python3 build_image.py --config ofrak-$*.yml --base --finish

.PHONY: start-ofrak-%
start-ofrak-%: ## Start OFRAK image using ofrak-<name>
	make ensure-ofrak-license image_name=$*
	@echo "Starting OFRAK image using config: ofrak-$*.yml..."
	docker run \
	  --rm \
	  --detach \
	  --hostname ofrak \
	  --name ofrak-$* \
	  --interactive \
	  --tty \
	  --publish 8877:80 \
	  --volume $(PWD)/ofrak.license:/ofrak.license \
	  redballoonsecurity/ofrak/$*:latest

I would argue these are barely more complex than the normal commands that the user would execute to build and run ofrak in a container. For example if a user wanted to use the ofrak-dev.yml config file they would either need to edit the existing makefile or run:

python3 build_image.py --config ofrak-dev.yml --base --finish
docker run --rm --detach --hostname ofrak --name ofrak-dev --interactive --tty --publish 8877:80 --volume $(PWD)/ofrak.license:/ofrak.license redballoonsecurity/ofrak/dev:latest

Theres a fair number of unintuitive arguments for each command as shown by the fact that we have a ~600 word documentation section just to describe the two commands needed to build and run docker. But by adding these to the Makefile we simplify it for the user such they they would only have to run:

make ofrak-dev
make start-ofrak-dev

Moving patch_maker to the top beginning of the yaml made a big difference in how often that layer's cache becomes invalidated by changes in other layers. Given the time required to download and build this stage (easily ~30 minutes on some systems), this small change makes a big difference. If the order of the yaml matters, we should clearly document it and explore other ways of keeping patch_maker's layer cache from becoming invalid.

As for the licensing it requires the user to have generated the ofrak.license file, a step that is automatically invoked if the file is missing when the user runs either of the start commands. This brings up the ofrak license prompt and requires the user to select the license agreement and type "I agree" in order for it to copy the ofrak.license file out of the docker container and continue successfully with the rest of the command. If this method is not satisfactory please propose a new method that doesn't require the user to type "I agree" every time they start a container.

ANogin · 2025-01-22T17:26:58Z

Makefile

 	autoflake --in-place --remove-all-unused-imports --ignore-init-module-imports -r . -c

 .PHONY: inspect
-inspect: autoflake check-black
+inspect: ## Runs autoflake then check-black
+	autoflake


This should be $(MAKE) autoflake, otherwise you are dropping all the arguments

ANogin · 2025-01-22T17:27:22Z

Makefile

-inspect: autoflake check-black
+inspect: ## Runs autoflake then check-black
+	autoflake
+	make check-black


It's better to use $(MAKE) for recursive make calls.

ANogin · 2025-01-22T17:29:45Z

Makefile

+ensure-ofrak-license:
+	@if [ ! -f "$(license_file)" ]; then \
+		echo "No local 'ofrak.license' found."; \
+		echo "Launching container to obtain license from IMAGE: 'redballoonsecurity/ofrak/$(image_name):latest'..."; \
+		docker run --name ofrak-license-check \
+			-it \
+			--entrypoint '' \
+			redballoonsecurity/ofrak/$(image_name):latest \
+			ofrak license; \
+		if [ "$$?" -eq 0 ]; then \
+			echo "License accepted. Copying license file out of container..."; \
+			docker cp ofrak-license-check:/ofrak_core/ofrak/license/license.json "$(license_file)"; \
+		else \
+			echo "License was NOT accepted or command failed. Exiting..."; \
+			docker rm ofrak-license-check >/dev/null 2>&1 || true; \
+			exit 1; \
+		fi; \
+		docker rm ofrak-license-check >/dev/null 2>&1 || true; \
+	fi


I think at the very least somewhere in this process we need to tell the user where to place their commercial license file in.

ANogin · 2025-01-22T17:30:18Z

Makefile

+#   make start-ofrak-tutorial    -> starts ofrak-tutorial image
+.PHONY: start-ofrak-%
+start-ofrak-%: ## Start OFRAK image using ofrak-<name>
+	make ensure-ofrak-license image_name=$*


Pleas use $(MAKE)

ANogin · 2025-01-22T17:31:15Z

Makefile

+	  $(call volume_mounts,$(PACKAGES)) --entrypoint bash \
+	  redballoonsecurity/ofrak/$*:latest \
+	  -c 'make develop \
+	  && ofrak license --community --i-agree \


Should not have the automatic ofrak license step.

ANogin · 2025-01-22T17:32:27Z

Makefile

+		--entrypoint bash \
+		--volume "$(PWD)":/ofrak \
+		redballoonsecurity/ofrak/angr:latest \
+		-c "ofrak license --community --i-agree \
