Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

nixos/gitlab: Fix state directory permissions, clean up initializers directory #68721

Merged
merged 2 commits into from Oct 8, 2019
Merged

nixos/gitlab: Fix state directory permissions, clean up initializers directory #68721

merged 2 commits into from Oct 8, 2019

Conversation

talyz
Copy link
Contributor

@talyz talyz commented Sep 14, 2019 •
edited

Motivation for this change

Since the preStart script is no longer running in privileged mode, recursively reassign all files in the state directory to the user we're running as. Also, delete the database.yml symlink if it exists,
since we want to create a real file in its place.

I've tested this by manually changing the owner of gitlab_shell_secret, and symlinking database.yml and secrets.yml to an irrelevant file. Additional testing would be appreciated.

The config/initializers directory is populated with files from the gitlab distribution on start, but old files will be left in the state folder even if they're removed from the distribution, which can lead to
startup failures. Fix this by always purging the directory on start before populating it.

Should hopefully fix #68696; if so, it needs to be pulled into 19.09 too.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @flokli @globin @aanderse @FRidh

@talyz talyz mentioned this pull request Sep 14, 2019
Closed
schneefux
schneefux suggested changes Sep 14, 2019
View reviewed changes
Copy link
Contributor

@schneefux schneefux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested that this does not break an upgraded installation. If I link the two ymls to random files, they are replaced correctly. 👍
However, if the shell secret is owned by a different account than gitlab, the start script still fails.

nixos/modules/services/misc/gitlab.nix Outdated Show resolved Hide resolved
nixos/modules/services/misc/gitlab.nix Outdated Show resolved Hide resolved
schneefux
schneefux approved these changes Sep 14, 2019
View reviewed changes
Copy link
Contributor

@schneefux schneefux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

nixos/modules/services/misc/gitlab.nix Outdated Show resolved Hide resolved
@globin
Copy link
Member

globin commented Sep 14, 2019

Please don't do this as this takes ages on larger gitlab installations (~15 mins for us)..
I'd prefer fixing this for now with chown and running preStart as root again for 19.09 as a compatibility release with the right permissions and switching to preStart as gitlab in 20.03

@aanderse
Copy link
Member

If you restored the tmpfiles rules from prior to these changes but set tmpfiles mode to z for the individual files owned by root that would save a recursive chown + chmod, allow you to run the script unprivileged, and not require any further changes at a later time, I believe.

@talyz
Copy link
Contributor Author

talyz commented Sep 14, 2019

@globin Oh, for every config activation or just the first one? I guess it makes sense, since it has to stat all files recursively... :/

@aanderse That was my first thought, but it didn't work - systemd-tmpfiles emits the error message

Detected unsafe path transition /var/gitlab/state → /var/gitlab/state/gitlab_shell_secret during canonicalization of /var/gitlab/state/gitlab_shell_secret.

which it apparently does when a file in a directory owned by an unprivileged user is owned by root...

@talyz
Copy link
Contributor Author

talyz commented Sep 15, 2019

Okay, so I've gone with the earlier approach of running a privileged part and an unprivileged part of the preStart script, but implemented it locally in the gitlab module rather than the systemd module. Does this work better for you, @globin?

@talyz talyz mentioned this pull request Sep 22, 2019
Closed
@talyz talyz mentioned this pull request Oct 3, 2019
Merged
10 tasks
@talyz
nixos/gitlab: Fix state directory permissions
0f8133d 
Since the preStart script is no longer running in privileged mode, we
reassign the files in the state directory and its config subdirectory
to the user we're running as. This is done by splitting the preStart
script into a privileged and an unprivileged part where the privileged
part does the reassignment.

Also, delete the database.yml symlink if it exists, since we want to
create a real file in its place.

Fixes NixOS#68696.
@talyz
nixos/gitlab: Clean up the initializers on start
c6efa9f 
The initializers directory is populated with files from the gitlab
distribution on start, but old files will be left in the state folder
even if they're removed from the distribution, which can lead to
startup failures. Fix this by always purging the directory on start
before populating it.
@talyz talyz changed the title nixos/gitlab: Fix state directory permissions nixos/gitlab: Fix state directory permissions, clean up initializers directory Oct 3, 2019
@talyz
Copy link
Contributor Author

talyz commented Oct 3, 2019

I've pushed one additional commit that cleans up the config/initializers directory, rebased on master and tested it without any issues. This addresses the concern raised by @bgamari in #70216 (comment).

@globin Do you have any issues with the current approach or could this be merged?

aanderse
aanderse approved these changes Oct 4, 2019
View reviewed changes
Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not tested, nor ever used the gitlab module on NixOS but I understand and approve of the basic idea behind this change. From my limited perspective the actual implementation looks fine as well. 👍

@globin globin merged commit b648a71 into NixOS:master Oct 8, 2019
@talyz talyz deleted the gitlab-fix branch October 9, 2019 06:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aanderse aanderse aanderse approved these changes

@schneefux schneefux schneefux approved these changes

@globin globin Awaiting requested review from globin

@fpletz fpletz Awaiting requested review from fpletz

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

gitlab module permission denied after upgrade
4 participants
@talyz @globin @aanderse @schneefux