@pingou If I'm reading this correctly, it generates an SSH key pair for each mirror hook activated, right?

By the way, if this is the case, then it shouldn't be that bad to extend this to support pull (inbound) mirroring, too (#1987). That deals with the main problem of using the same key (Gitolite would consider each user "unique" with separate keys).

That said, public repos with anonymous access allowed do not require any of this for inbound mirroring, just the ability to be pulled via git://, http://, or https:// protocols.

@pingou If I'm reading this correctly, it generates an SSH key pair for each mirror hook activated, right?

Yes

That deals with the main problem of using the same key (Gitolite would consider each user "unique" with separate keys).

For this I think I prefer to expand the support of the deploy keys, keep concerns separate.

Pretty please pagure-ci rebuild

This code looks good to me. The test failures look like redis isn't set up correctly in the test environment, so I'm going to assume the tests pass normally.

So... :thumbsup:

The test failures look like redis isn't set up correctly in the test environment, so I'm going to assume the tests pass normally.

Thanks, just the pointer I needed to find the error.

Pushing a fix, let's see if it works :)

Pretty please pagure-ci rebuild

Drop trollius-redis stuff.

Drop trollius-redis stuff.

Actually, the trollius stuff can also go away since the service is using celery (which would be good to list as dependency)

Why don't we just require pagure? That's already technically a real requirement, since most of the code is there...

@pingou This needs to be rebased for current master.

This is the mirror service, not the logcom service. :)

https://pagure.io/pagure/pull-request/1105#_3,63

I think we'd want to run this as a different user.

Note that this assumes that this GIT_DIR will exist when the runner gets to running it. This might not be the case.

I do not think that ~/.ssh is the best place for this.
This is service data, how about /var/lib/pagure/sshkeys/?

This writes the private key as chmod 0644.
SSH won't even accept these keys, they'll need to be 0600.

Note that the directory containing private keys must be 0700.

I would say that if a key exists at the moment mirroring is enabled, there's definitely something wrong (previous key wasn't cleared up), and it should probably error out?

I would actually say to not catch this. When you get here, the key really ought to exist, if it doesn't, we need to error out probably, so an admin looks at it.

Given that, if I read the code correctly, this is done in the main project repo, this would mean you add a remote for each target persistently, and on future pushes, this will error out.
But also, if you edit e.g. the first line in the remote, it will never update that one, etc.
Perhaps it's better to just do this in a new, clean clone of the repository?

You have no checking at all on the remote strings as far as I can see, this would mean that I could fill in a target of "/srv/git/repositories/pagure.git" in my mirror from attack.git, and since this runs as a user that has R/W access to all repos, would (probably) overwrite the target repository.
Next to checking of the target URLs, I really think this should run as a limited user.

I think that in this PR, you still need:

• docs
• tests (you currently have UI tests, tests on that the pushing itself works would probably be great)

Indeed that is the idea, I'll make it more explicit by changing the default here, thanks

How could it not? This is the hook so it's installed in a subfolder in the GIT_DIR

I guess it should be a configuration variable then, defaulting to this path indeed.

:thumbsup:

:thumbsup:

I'll add a check to ensure the remote is an (ssh) url.

sounds good

Oups, thanks

Let's adjust it :)

In this case since we're removing the key I'm tempted to let it as is.

We should probably make sure that if it already exists, it's not a symlink or owned by someone else.
Otherwise, if the containing directory is widely writable, someone with access to the system might be able to instead put in a symlink to a place they control.

Done :)

test

1. User A creates repo X
2. Repo X is setup with outbound mirroring
3. Repo X is pushed to
4. Worker is very busy
5. Repo X is deleted by User A
6. Worker gets to running queued mirror job

GIT_DIR now does not exist.

Maybe add os.O_EXCL as well, so that we make very sure that we create this file, rather than overwrite a file an attacker might've put in place?
That will make sure that if the file already existed, it errors out.

Note that this overwrites the log attribute with just the log of the very last push.
Maybe an idea to accumulate this variable over the for idx, remote in enumerate(remotes): run, so you can actually return the log for all of the mirrors?

+1 to the patch as is, but might fix the two last comments.
They should be small enough and not impact the security constraints, so no need to re-review if just the os.O_EXCL and the log fix.

Hm, good point, I'll adjust the task to check if the absolute path still exists before continuing.

Thanks :)

will do

Good catch, fixed via:

+    logs = []
     for idx, remote in enumerate(remotes):
         remote_name = '%s_%s' % (public_key_name, idx)
         _log.info(
@@ -239,10 +241,12 @@ def mirror_project(self, session, username, namespace, name):
             env={'GIT_SSH_COMMAND': 'ssh -i %s' % private_key_file})
         log = "Output from the push:\n  stdout: %s\n  stderr: %s" % (
             stdout, stderr)
-        project.mirror_hook.last_log = log
+        logs.append(log)
+    if logs:
+        project.mirror_hook.last_log = '\n'.join(logs)
         session.add(project.mirror_hook)
         session.commit()
-        _log.info(log)
+        _log.info('\n'.join(logs))

Jenkins is all green, let's merge :)

Thanks for the review @puiterwijk!! :)