Add script for stripping TinyPilot marker sections

jotaen4tinypilot commented 5 months ago

This PR adds a unified script to remove TinyPilot marker sections from files. We will use this script in a subsequent PR to simplify the corresponding instances in the unset-static-ip and change-hostname privileged scripts.

Notes

Unless someone feels strongly about the originally mentioned remove-tinypilot-lines script name (or any other name), I thought that strip-marker-sections sounded most expressive.
I added a comment to the lib/markers.sh file, to explain the marker idea in a bit more detail.
There will be automated bash tests for this in a subsequent PR.

mtlynch commented 5 months ago

We will use this script later to simplify the corresponding instances in the unset-static-ip and change-hostname privileged scripts.

Would it be possible to do that now? I find that code is much easier to review and give feedback on if we can see how it's being called.

jotaen4tinypilot commented 5 months ago

Would it be possible to do that now?

Sure! I’ll create a separate stacked PR for that. (We have to make a few other, non-trivial adjustments in the respective scripts.)

codeapprove[bot] commented 5 months ago

Automated comment from CodeApprove ➜

👀 @jotaen4tinypilot it's your turn, please take a look

jotaen4tinypilot commented 5 months ago

Re: @jotaen4tinypilot:

I put this script into /scripts, because it’s also supposed to be invoked by users directly.

I just realized that I’m not sure about this anymore: so far, I thought:

User-facing CLI scripts live in /opt/tinypilot/scripts/
“Internal”, programmatically used scripts live in /opt/tinypilot-privileged/scripts/

Is that true @mtlynch? I noticed that we seem to be regularly referencing privileged scripts in our FAQ for direct CLI usage.

I'd like to avoid duplicating the strings here, but I'm having trouble understanding why we can't without seeing how this script fits into our codebase.

I’ve opened https://github.com/tiny-pilot/tinypilot/pull/1722, which demonstrates how the strip-marker-sections script will be used in practice.

The issue with referencing the lib/markers.sh file is that we have two different invocation contexts:

In production, the canonical absolute path for sourcing the lib/markers.sh file would be /opt/tinypilot-privileged/scripts/lib/markers.sh.
When running our new bats tests (locally or on CI), the /opt/tinypilot-privileged folder doesn’t exist, unless we’d explicitly create it. Without /opt-tinypilot-privileged being available, the path for sourcing would have to be ../debian-pkg/opt/tinypilot-privileged/scripts/lib/markers.sh. That then wouldn’t work in production, though.

Seeing that my picture regarding /scripts<>/tinypilot-privileged/scripts may not be accurate, we could also consider moving the strip-marker-sections script to /tinypilot-privileged/scripts? That would then allow us to source the lib/markers.sh file in all environments reliably.

jotaen4tinypilot commented 5 months ago

(I’ll update the PR stack and then request a new review once it’s done.)

mtlynch commented 5 months ago

I just realized that I’m not sure about this anymore: so far, I thought:

User-facing CLI scripts live in /opt/tinypilot/scripts/

“Internal”, programmatically used scripts live in /opt/tinypilot-privileged/scripts/

The distinction is really about whether or not the script needs to run as root rather than whether it's executed by an end-user or programmatically.

strip-marker-sections technically doesn't require root, but I guess it shouldn't be writable by tinypilot because that would allow somewhat elevated privileges. For example, an attacker who compromises the tinypilot user could overwrite strip-marker-sections, and then they can overwrite /etc/dhcpcd.conf with arbitrary data.

So /opt/tinypilot-privileged is a good place.

jotaen4tinypilot commented 5 months ago

Thanks for clearing this up @mtlynch! I’ve updated all 3 PRs: the strip-marker-sections script is now located in /opt/tinypilot-privileged/scripts and sources the lib/markers.sh file.

In the PR description above, I’ve removed the two corresponding bullet points, which now have become obsolete:

I put this script into /scripts, because it’s also supposed to be invoked by users directly.

It’s tricky to include (source) the lib/markers.sh file, because when we reference it via an absolute path (/opt/tinypilot-privileged/...), then we cannot use the script in development or test environments. We also cannot reference it via a relative path ("${SCRIPT_DIR}/../debian-pkg/opt/tinypilot-privileged/..."), because debian-pkg is not there in the production environment. Due to the lack of better ideas, the duplication with a reference comment felt like the least bad option.

jotaen4tinypilot commented 5 months ago

Automated comment from CodeApprove ➜

⏳ @jdeanwallace please review this Pull Request

jotaen4tinypilot commented 5 months ago

@jdeanwallace did you potentially forget to approve, or did you want to take another look after the revision?

tiny-pilot / tinypilot