Bug 255009

Summary: www/gitea: do not chown existing directories
Product: Ports & Packages Reporter: Martin Birgmeier <d8zNeCFG>
Component: Individual Port(s)Assignee: Adam Weinberger <adamw>
Status: Closed FIXED    
Severity: Affects Only Me CC: adamw, stb
Priority: --- Flags: koobs: maintainer-feedback+
Version: Latest   
Hardware: Any   
OS: Any   
Attachments:
Description Flags
Move creating of work dirs from pkg-plist to startup script stb: maintainer-approval+

Description Martin Birgmeier 2021-04-12 17:03:04 UTC
Scenario:
- gitea installed
- setting gitea_user="gitea" in /etc/rc.conf (i.e., running gitea not as user git, but as user gitea)
- use /var/run/gitea/gitea.sock for communication
- upgrade gitea using a package created using portmaster

Result:
- on startup, gitea cannot create /var/run/gitea/gitea.sock anymore, and therefore cannot be interacted with
- /var/db/gitea, /var/run/gitea, and /var/log/gitea are all owned by git instead of gitea

Expected result:
- If the directories /var/db/gitea, /var/run/gitea, and /var/log/gitea already exist when installing their ownership shall not be changed.

-- Martin
Comment 1 stb 2021-04-13 08:24:49 UTC
The ownership of these directories is encoded in the manifest at package build time (pkg-plist), and pkg does not take rc.conf settings into account when installing a package. AFAIK this is current practice for many ports, not just gitea.

I've posted the question on freebsd-ports@ to get some guidance on what to do in this situation.
Comment 2 Adam Weinberger freebsd_committer 2021-04-18 15:23:42 UTC
There's no clear rule on what to do there, so there's no wrong way to do it.

I addressed this idea in a different way www/caddy in the rc(8) script:

  caddy_precmd()
  {
          # Create required directories and set permissions
          /usr/bin/install -d -m 755 -o "${caddy_user}" -g "${caddy_group}" ${caddy_directory}
          /usr/bin/install -d -m 700 -o "${caddy_user}" -g "${caddy_group}" ${caddy_directory}/config
          /usr/bin/install -d -m 700 -o "${caddy_user}" -g "${caddy_group}" ${caddy_directory}/data
          /usr/bin/install -d -m 755 -o "${caddy_user}" -g "${caddy_group}" ${caddy_logdir}
          /usr/bin/install -d -m 700 -o "${caddy_user}" -g "${caddy_group}" /var/run/caddy
  }

The benefit there is that changes in the rc.conf are reflected the next time it spins up. The drawback there is that it only changes the permissions on the directory itself; if you change from root:wheel (the default) to www:www, for example, the existing files can't be modified.

Another approach could be to simply check whether those dirs are owned by gitea_user in the rc(8) script and spit out an error message with instructions for what to chown -R.

Another option is to just do nothing, and there is plenty of precedent for that. It's my least favorite option, but my guess is that the majority of ports don't bother with it.

Stefan, it's your show. Do you have a feel for what approach you'd like to take?
Comment 3 stb 2021-04-26 07:45:48 UTC
At this point, I'd rather do nothing. I feel that modifying ownership/permissions in the rc script is a bandaid at best, and the wrong kind of "magic".

I think any solution should go into the plumbing, instead of individual ports.
Comment 4 Kubilay Kocak freebsd_committer freebsd_triage 2021-04-26 08:31:56 UTC
^Triage: feedback provided, set flag (+) accordingly. There's no patch to set maintainer-approval to "-" on.

(In reply to stb from comment #3)

Any relevent replies to your post yet? Can you link the thread too please
Comment 5 Adam Weinberger freebsd_committer 2021-07-24 19:16:43 UTC
There have been no replies to Stefan's message about his intent to leave things as they are. I'm going to close this out. Please comment or contact me if this should be reopened.
Comment 6 Martin Birgmeier 2021-07-27 06:35:46 UTC
Hello all,

This is a bug which breaks the operation of gitea every time an update is installed; the issue therefore should not be closed so lightly.

Since the directories named in the manifest are affected, a solution like the one proposed by Adam in comment #2 might be most appropriate, at least until the package infrastructure is improved to allow not changing ownerships at all.

Please allow me to re-open this issue.

Best regards,

Martin
Comment 7 stb 2021-07-27 09:13:46 UTC
I respectfully disagree.

Changing the ownership of these directories (and contained files) from what is prescribed in the manifest is a local decision, and needs to be maintained by the local user.

Until pkg has a facility to accomodate such changes, might I suggest fixing the ownership and permission after upgrading packages? On my systems, I have a number of customizations that collide with pkg upgrades, and I set up a script that re-applies my local changes after each run of pkg upgrade.

Why is it important to you to use "gitea" instead of "git"? How is Gitea updating the ssh keys in ~git/.ssh/authorized_keys if it is not running as "git"? On my systems, Gitea is the sole consumer of the "git" user.
Comment 8 Martin Birgmeier 2021-07-27 09:53:14 UTC
Well, on this system the "git" user has been used for other purposes for a long time before installing "gitea", and the way gitea manages the ssh authorized keys conflicts with those other purposes in a way which cannot be resolved (I tried it :-)).

Besides, what is the purpose of explicitly allowing another user in the rc script if it is then not really supported?

-- Martin
Comment 9 stb 2021-07-27 13:04:13 UTC
Created attachment 226736 [details]
Move creating of work dirs from pkg-plist to startup script

How about this, then?

I think this should cover the "normal" case where users simply install the package, and are happy to use the git user account, but also the case where the user has been changed from the default.

If the directories already exist, they are not changed. Only if they're not there, are they created at startup using the configured users.

As far as I can tell, /var/run/gitea was never used, so I removed it from the list.
Comment 10 stb 2021-07-27 13:09:09 UTC
(In reply to stb from comment #9)

Importantly, if the admin decides to change the gitea_user after installing the package, the admin needs to make sure ownership and permissions on /var/db/gitea and /var/log/gitea are updated appropriately.

Making scripted ownership and/or permission changes to existing files and directories is really error prone, so I really don't want to go there. Creating a directory in /var/db or /var/log when no such entry exists should be safe, I think.
Comment 11 Martin Birgmeier 2021-07-27 13:39:50 UTC
Thank you, on first sight this looks quite good.

/var/run/gitea should be kept, it contains gitea.sock (a Unix domain socket for local communication, used by the Apache reverse proxy in front of gitea - at least in the setup here).

-- Martin
Comment 12 Adam Weinberger freebsd_committer 2021-08-20 18:41:39 UTC
Forgot to note it in the commit, but this was committed along with bug #257973.