Allow configuration of the authorized_keys file location used by gitlab-shell

Also added an alternative search location for authorized_keys to our docker sshd_config

We wil make use of this in the OpenShift template in order to get around root_squash issues with the git user's home directory.

cc\ @jacobvosmaer-gitlab @marin

mentioned in merge request !899 (merged)

How about keeping ssh_dir at the old value (~git/.ssh)? If gitlab.rb calls for a non-standard location, assume that directory exists.

I'm with @jacobvosmaer-gitlab on this one, auth_file could be on a completely separate location than the ssh_dir

Also, even if you use AuthorizedKeysFile in sshd_config, that does not change where sshd looks for ~git/.ssh/config (which we manage because of the bitbucket importer I think).

I would take out the %u. This sshd_config only allows 'git' to connect anyway. And it spares us the 'does authorized_keys have a parent directory' question.

I would also put this one line above. At the moment it doesn't make a difference but it might be good to split parse_git_data_dirs from GitlabRails to make the separation clearer.

Added 1 commit:

• 59788d3c - Allow configuration of the authorized_keys file location used by gitlab-shell

@jacobvosmaer-gitlab @marin feedback has been implemented

The current way I have it here doesn't handle selinux rules for the auth file. In order to get it working you have to run something like:

semanage fcontext -a -t sshd_key_t '/gitlab-data/authorized_keys'
restorecon -v '/gitlab-data/authorized_keys'

I'm thinking I should add that in to the recipe as well? Thoughts?

mentioned in issue #1305 (closed)

I think we can 'in-line' this at line 100 now.

Now that we have https://gitlab.com/gitlab-org/gitlab-shell/merge_requests/79 we can catch authorized_keys permissions issues, even if the file is not in ~git/.ssh, with:

command '/opt/gitlab/embedded/service/gitlab-shell/bin/gitlab-keys check-permissions' do
  user 'git'
  group 'git'
end

If we depend on policycoreutils then we can probably do the semanage stuff. Good that you thought of that @twk3 .

@marin I believe this is an old topic. Where are we on the idea of the (EL) packages depending on policycoreutils?

@jacobvosmaer-gitlab We didn't make a decision on that. The only thing that was clear in discussions about policycoreutils was that we are not bundling that, but maybe that changed in the meantime (highly doubt it). If we are to introduce another runtime dependency we need to make sure that not only CentOS 6/7 support it but that it is also supported on RedHat/Oracle/Scientific Linux . So we will have to do an analysis whether that is worth it.

Added 1 commit:

• d1d1a7ac - Run the gitlab-shell keys permission check during the shell recipe

@jacobvosmaer-gitlab @marin should I just add a chcon exec then for the file for now? To avoid the policycoreutils and to match what we are doing for the ssh home directory.

@twk3 Yes. Could you also create an issue about policycoreutils so we can investigate further what the cost of introducing it is?

Added 1 commit:

• 19ce4c42 - Use gitlab-shell bin/keys to create the authorized_keys file if it doesn't already exist

Updated with the chcon changes, I also brought back the auth_file creation, otherwise the chcon and the check permissions were failing. I am using the gitlab-shell keys clear command for it though.

A new issue for policycoreutils has been created: https://gitlab.com/gitlab-org/omnibus-gitlab/issues/1485

@twk3 you can also use the new gitlab-keys check-permissions command. That will create the file if missing and it does not clear it.

@jacobvosmaer-gitlab Like here: https://gitlab.com/gitlab-org/omnibus-gitlab/merge_requests/911/diffs#c37cc13b75668e6e186a630e395ef4886a0cfdd8_122_138 ? :)

@jacobvosmaer-gitlab I don't think check-permissions creates the file, it also complains if it doesn't exist

Added 52 commits:

• 19ce4c42...06b2dddf - 49 commits from branch master
• 8df5fe1c - Allow configuration of the authorized_keys file location used by gitlab-shell
• 69866a1e - Run the gitlab-shell keys permission check during the shell recipe
• 14f61764 - Use gitlab-shell bin/keys to create the authorized_keys file if it doesn't already exist

@twk3 yes you are right. I think the current combination is good. Although 'clear unless exists' does have a race condition.

We could also change check-permissions so that it creates the file. That might be better than having a race condition.

diff --git a/lib/gitlab_keys.rb b/lib/gitlab_keys.rb
index dc654fd..e10af83 100644
--- a/lib/gitlab_keys.rb
+++ b/lib/gitlab_keys.rb
@@ -101,7 +101,7 @@ class GitlabKeys
   end
 
   def check_permissions
-    open_auth_file('r+') { true }
+    open_auth_file(File::RDWR | File::CREAT) { true }
   rescue => ex
     puts "error: could not open #{auth_file}: #{ex}"
     if File.exist?(auth_file)

Would do it in gitlab-shell. Plus a test case that asserts file creation.

Added a MR for shell: https://gitlab.com/gitlab-org/gitlab-shell/merge_requests/83

Added 1 commit:

• f1c0b038 - Drop use of keys clear, and use the new check-permissions command for the key file creation

Updated with the latest changes, uses check-permissions to create the file.

Added 27 commits:

• f1c0b038...2bb3e43f - 22 commits from branch master
• 18a90aa9 - Allow configuration of the authorized_keys file location used by gitlab-shell
• 4d976f23 - Run the gitlab-shell keys permission check during the shell recipe
• f7865775 - Use gitlab-shell bin/keys to create the authorized_keys file if it doesn't already exist
• efb9beaa - Drop use of keys clear, and use the new check-permissions command for the key file creation
• 21c54b87 - Added chefspec tests for the authorized_keys config change

Added chefspec tests

Anything else? @jacobvosmaer-gitlab @marin

Looks good from my end @twk3 !

Just thinking for a moment about security angles about this. What do we know about the permissions on /gitlab-data/ ? Do we know anything?

I am thinking about the pathological scenario where gitlab-keys check-permissions successfully creates the file and sets its mode to 0600, yet the directory permissions allow some other user to remove the file and put a different one there. It might be better to create a subdirectory for this, with mode 0755 and owner git:git.

yeah, the directory, in the case I am trying to address with openshift, will be 0777 and owned by nobody:nogroup

If I want to use a subdirectory, it will have to be created during reconfigure, or during the docker command execution

@twk3 sounds like we definitely want to use a subdirectory then.

Remove the commented out code.

@twk3 I've picked 46f123b2 into one of my branches and merged to master in order to reuse some of the things you already wrote. You will need to rebase from master.

Added 22 commits:

• 21c54b87...07d58533 - 17 commits from branch master
• 923fd761 - Allow configuration of the authorized_keys file location used by gitlab-shell
• 5e20e585 - Run the gitlab-shell keys permission check during the shell recipe
• e751a044 - Use gitlab-shell bin/keys to create the authorized_keys file if it doesn't already exist
• de05e405 - Drop use of keys clear, and use the new check-permissions command for the key file creation
• 459e5d8a - Add in some of the missing tests from the rebase for the authorized_keys changes

rebased for the test changes

Added 1 commit:

• 18bb09a1 - Update the gitlab-shell recipe to make sure it create the directory for the auth…

Added 1 commit:

• 58de5cba - Add gitlab-shell recipe test for alternate auth_files directory permissions

@jacobvosmaer-gitlab I've updated the alternate authorized_keys location, and have made sure the omnibus willcreate and assign the correct privileges

@twk3 will this work with root_squash on NFS?

this MR is just for the authorized keys location change. Then I will update my original MR to use a new definition for the directory command. Which should still wok here.

+1

Reassigned to @marin

Status changed to merged

mentioned in commit 82269203