Runner installation: Simplify docker compose setup and instructions + add section on firewall considerations #1634

Incorrect. The fact that it lines up with your user on your machine is purely incidental.

The user in the image uses 1000:1000 because that's what the Dockerfile specifies. It has no correlation to anything else.

Ack! You're right, I hope the updated text gives more clarity.

the first user added on a new system with useradd

Incorrect. The majority of distributions ship with UID_MIN=1000, so 1001 would be the second user added.
And the goal here is to not match the main user, so as to limit what a compromised workflow can do to the system.

That's what I meant to say yeah, what I meant by "first user added", it's because most new systems install a user with ID 1000, so 1001 is the first "custom" user :)

Hope the update gives more clarity.

You still don't want this to match with a user on the system. The entire "matches with a user" is irrelevant at best, unwanted at worst.

The existing line is good enough. If you really must write something here, then it should be about picking a UID that will not be used for a user on the system.

I understand the structure of the documentation a bit in hindsight, so I had already stumbled upon the useradd --create-home runner in the "Binary installation" instructions, which I actually like :)

unwanted at worst.

That's what I want, too... I want the reader to actively decide to NOT use 1000 because that gives the Docker containers access to the default user... which is probably not what they want.

1001 is arbitrary in the sense that it may or may not match with an existing user.

The most deliberate action would be to create a user, then print out the ID, then use that ID?

I've adjusted the text a bit.

But I think to have a clear path through the instructions, we might actually be better off with a preceding useradd instruction that gives people a secure baseline, guaranteeing that they don't unintentionally pick a user ID that's associated with something else on the host system.

What do you think about adding that?

@benjaoming wrote in #1634 (comment):

The most deliberate action would be to create a user, then print out the ID, then use that ID?

No. I'll repeat: This UID should not match any user. "Create a user, then match the UID" is the opposite of "don't match a user", literally.
A UID should be picked for which there will never be a user on the system. As long as a user exists, there is a chance that it is given more permissions than it needs to.

But this shouldn't be elaborated in this document. That's what /admin/actions/security exists for. At most, this section should mention something like "pick a UID that doesn't match any user on the system", to not make the initial installation too complicated.
It's rather simple to change the UID afterwards.

I mean it's great if it matches a user that's designated to the purpose.

What I don't like about 1001 is that someone might unknowingly add a user later with that ID for a different purpose. Since 1001 isn't listed in /etc/passwd, it's hard to tell that it's actually being used for this setup.

I'll strip it out now, not pursuing anymore discussion, thanks for the feedback 👍

Removed the proposed text. I'll leave it here for reference:

The idea is avoid unintentionally matching your default user's ID (1000). Instead, we pick an unused ID or an ID that matches a user account that you have designated to the runner. We configure the following docker-compose.yml to user the same ID.

A UID should be picked for which there will never be a user on the system. As long as a user exists, there is a chance that it is given more permissions than it needs to.

That's also true 👍

As the line above says, this should be run as root. So the conventional prompt symbol would be #.
But why change this at all? It becomes much more tedious to copy this way.

Updated to fit in one-line copy paste

I don't think that creating a one-time setup.sh is conventional in a documentation. Blindly copy-pasting commands is like an anti-pattern that makes me, the reader, not actually read what I'm doing 😇

I hope the update is to your satisfaction.

@benjaoming wrote in #1634 (comment):

Blindly copy-pasting commands is like an anti-pattern

Yes, but we cannot control what the reader does, and the fact is that readers do copy&paste. The best we can do is to make it both readable and not-error prone at the same time.

For that I usually prefer a clear one command per line is better than extra symbols or fancy one-liners. I don't mind if you want to remove the shebang and other parts related to making it a script, but I don't see a good reason to change the content.

I'm just very convinced that if I don't add the prompt $, then the next reviewer will point out that all the other command-line instructions in the docs are using them 🙄

(btw I think this is a UI problem that has been solved in other documentation frameworks where the prompt symbol won't be part of the selected text for copy-pasting)

@benjaoming just to reassure you: once you have addressed @sclu1034 review comments to satisfaction it is extremely unlikely that someone else suggests something different. In my experience @sclu1034 reviews are very detailed and thorough.

That's cool, this is not worth discussing IMO, was just trying to get rid of a (for me) strange instruction to create a setup.sh file. I did some stuff, received more push-back, I hope we can conclude soon.

It is true that other shell code blocks usually do have that prompt symbol. I guess the current version is a good enough compromise, then.

Or would be, I guess my comment was late.

@benjaoming wrote in #1634 (comment):

I did some stuff, received more push-back, I hope we can conclude soon.

This is not push-back, this is the review cycle. When I'm on the receiving side of a review I tend to agree with the reviewer on all points, except when it makes no sense. As a pull request author (this is a personal view) I think I'm expected to propose a change that makes things better. Whatever a review asks me to do besides that objective I treat as a matter of preference and go along with it.

@earl-warren This is a meta thing about review culture etc... but I'm very grateful that people review and put an effort into it, and I think these are great comments, I just don't have the bandwidth to keep up.

My use of "push-back" comes from this understanding:

We are all selectively prioritizing where to put our efforts. For instance, I made the observation that I would like to remove the setup.sh-approach part and just write the commands as they are. At that point, I didn't have the follow-up review input in mind. In fact, I have no previous interaction experience in this repo IIRC.

Since my main purpose wasn't to update this part, then I would rather skip the change, given that a lot of scrutiny is applied here.

In an alternative version, no one would have cared and just said "yup, whatever" - that's what I mean by push-back. Not that it's ill-willed, but that it's a place where people specifically have opinions - as opposed to other places!

For instance, there was no review or comments on the firewall considerations, which surprises me 😄

but I'm very grateful that people review and put an effort into it, and I think these are great comments, I just don't have the bandwidth to keep up.

If you are pressed by time, doing what the review suggests is a lot faster than arguing against it.

@benjaoming wrote in #1634 (comment):

For instance, there was no review or comments on the firewall considerations, which surprises me 😄

They are generic considerations with specific examples that require interpretation from the reader. There is little room for misleading anyone and it can inspire someone. The accuracy of the information there is of no real consequence.

If you are pressed by time, doing what the review suggests is a lot faster than arguing against it.

That was the strategy, I think you've missed it.. but should be evident from the amount of commits 😉

Minor correction on terminology: The examples don't mount a "Docker volume", they (bind-)mount a host directory.

"Docker volume" specifically relates to something like docker volume create runner-data, and while technically an option, too, it would make accessing the config file much harder.

That's fair! I had this in mind when writing it:

 volumes:
 - ./data:/data

So it's a docker volume, but it's also a bind mount? 😄

I tried to reword to get out of the pickle. I ended up changing the flow of the text a bit.

I'm sceptical it's the right place. There is a separate chapter "Securing Forgejo Actions Deployments" where it would fit better. Apart from that, I'm also not sure whether providing example commands makes sense. There are so many programs to manage firewall rules... For example, Fedora has firewall-cmd. Are any of those commands applicable? What if someone uses a separate appliance?

In my opinion, it's better to explain what should be done in general terms and refer to the documentation of the respective tools for the details.

@aahlenst wrote in #1634 (comment):

There are so many programs to manage firewall rules... For example, Fedora has firewall-cmd. Are any of those commands applicable? What if someone uses a separate appliance?

By that logic, we shouldn't provide Docker examples either. Or Systemd.

But the reality is that certain applications are well known within their use cases, so they can serve as examples for others to translate from as well. And I would put IPTables into that category of "known by most".
So I don't see any harm in providing both the general description and examples for one well known tool.

By that logic, we shouldn't provide Docker examples either. Or Systemd.

That takes what I wrote to an extreme.

So I don't see any harm in providing both the general description and examples for one well known tool.

Right now, the proposed text includes both iptables and ufw. It also writes rules to a file instead of using the ufw command line tool. Ideally, there should be links that help me discover what each command does. For example, I'm curious whether /16 is the netmask.

I'm not against including example commands at all. But the emphasis should be on the concepts, less on the commands.

@aahlenst wrote in #1634 (comment):

By that logic, we shouldn't provide Docker examples either. Or Systemd.

That takes what I wrote to an extreme.

It doesn't draw arbitrary lines which tools are or aren't eligible to be named in examples.

@aahlenst wrote in #1634 (comment):

It also writes rules to a file instead of using the ufw command line tool.

The cli doesn't allow specifying a chain.

@aahlenst wrote in #1634 (comment):

I'm curious whether /16 is the netmask

It's quite safe to assume that any kind of <ip>/<number> in a networking context is going to be a CIDR, unless it's actually marked otherwise.

@aahlenst wrote in #1634 (comment):

Ideally, there should be links that help me discover what each command does

Like I said, too complicated doesn't help here. A lengthy explanation of setting up networking around Forgejo and the runner would be great, but needs an entirely different mindset of the user to actually sit down and work through.
A "quick tip" style like this one here, however, works best if it deliberately skips that and stays short and to the point.

Like I said, too complicated doesn't help here.

I am not asking for making it complicated or long. I'm asking for not providing commands to copy and paste without explaining what they do and what their consequences are. If explaining those consequences makes it too long or too complicated, then it's not a quick tip to begin with.

Apart from that, packet filtering is not required to get up and running. If that were the case, I wouldn't say anything. I don't want people showing up here complaining that their VPN doesn't work anymore because of a wrong netmask or that they cannot reach Forgejo (which is accidentally hosted on the same network) or that their workflows break in mysterious ways.

A quick tip that "stays short and to the point" would be: "If the runner is attached to a local network, consider adding a firewall rule to block Docker containers from accessing other resources on the network. Please consult the manual of your operating system or firewall for how to do so."

That needs a lot more nuance and considerations. For example, runners might need to access local caches. What can break?

Too complicated doesn't help either, because then the average person who isn't a network engineer will just give up.

For an article dedicated to networking around the runner it would make sense to elaborate, but for a quick "if you're hosting this at home, remember to exclude your home network" this looks fine to me.

"if you're hosting this at home, remember to exclude your home network"

That was EXACTLY the intention 🎯 Would it help if the word "home network" or "laptop" is added? I don't wanna scare very simple setups off - but I also don't think it's a good idea to leave the topic unaddressed so that the average user will have to do their own research.

Runners accessing other resources on a local network seem advanced enough, that I assume an administrator of such a setup would know what they're doing.

Btw regarding firewall-cmd, I think it could be added later by someone who uses it - since these are just quick examples, mentioning a few firewall configurations won't harm, since the intention is to provide initial guidance for users that would otherwise expose their home network 👍

from @aahlenst 's other comment:

There is a separate chapter "Securing Forgejo Actions Deployments" where it would fit better.

What about moving it there, but cross-referencing it from this guide? For instance with the text "If your setup is attached to a local network, for instance your home network or a portable laptop, then we recommend to see our Firewall Considerations [link]" ❓

For an article dedicated to networking around the runner it would make sense to elaborate, but for a quick "if you're hosting this at home, remember to exclude your home network" this looks fine to me.

I'm asking for the documentation to spell out the consequences. For example, what happens if Forgejo is hosted on the same network or someone uses the cache included in Forgejo Runner? Are those still accessible?

I'm asking for the documentation to spell out the consequences. For example, what happens if Forgejo is hosted on the same network or someone uses the cache included in Forgejo Runner? Are those still accessible?