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
+83 −68

Conversation

talyz
Copy link
Contributor

@talyz talyz commented Sep 14, 2019 •
edited
Loading

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

Sorry, something went wrong.

@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Sep 14, 2019
@talyz talyz mentioned this pull request Sep 14, 2019
Closed
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Sep 14, 2019
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.

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.

👍

@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 added 2 commits October 3, 2019 09:02
@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

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
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?

@flokli flokli requested review from aanderse, globin and fpletz October 4, 2019 18:06
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

@schneefux schneefux

@globin globin

@fpletz fpletz

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
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