gerrit doesn't check permissions for git push --dry-run
Bailey, Darragh
I was recently trying to put together a script that could be given to
developers of various projects that are in a gerrit instance to help
verify that the permissions on the project in gerrit were set up
correctly. To do that I assumed that gerrit checked the ACL's even when
'--dry-run' was passed to the git push command similar to how gitolite
worked.
Turns out I was wrong. For any registered user, once you have
permissions to send changes to a project, a 'git push --dry-run gerrit
HEAD:master' will return success.
My understand of what happens is as follows:
Gerrit overrides the prereceive hook of JGit
This hook only get's called when JGit is going to process the changes,
i.e. it won't get run if --dry-run is set
It's only during this hook that Gerrit actually checks permissions based
on refs
Within the ssh stack it appears that gerrit performs a very basic check
to see if the user has any permissions at all to upload to the repository.
In order to check permissions for 'git push --dry-run ...', Gerrit would
need to perform the full acl permissions check either in the canUpload
method of the ReceiveCommits class, or with a new call before handing
over to the jgit library by calling "rp.receive(in, out, err);"
I'm wondering if this was because it was expedient to let the
org.eclipse.jgit.transport.ReceivePack do all the command processing
rather than try to extract what was in the git push and check it for
access, or if it was an explicit design decision and there's no
expectation of having gerrit perform a full acls check for any
'--dry-run' push.
--
Regards,
Darragh Bailey
Shawn Pearce
> I was recently trying to put together a script that could be given to
> developers of various projects that are in a gerrit instance to help
> verify that the permissions on the project in gerrit were set up
> correctly. To do that I assumed that gerrit checked the ACL's even when
> '--dry-run' was passed to the git push command similar to how gitolite
> worked.
How would gitolite possibly do this? A --dry-run doesn't send the
commands the client would use from client to server, so the server
can't tell the client if it would accept them or not. The entire
--dry-run thing is client side only.
> Turns out I was wrong. For any registered user, once you have
> permissions to send changes to a project, a 'git push --dry-run gerrit
> HEAD:master' will return success.
>
> My understand of what happens is as follows:
> Gerrit overrides the prereceive hook of JGit
> This hook only get's called when JGit is going to process the changes,
> i.e. it won't get run if --dry-run is set
> It's only during this hook that Gerrit actually checks permissions based
> on refs
IIRC --dry-run doesn't even send the commands to the server. There is
no way for JGit to do anything here.
Bailey, Darragh
On 23/03/12 16:05, Shawn Pearce wrote:
> On Fri, Mar 23, 2012 at 08:50, Bailey, Darragh<dba...@hp.com> wrote:
>> I was recently trying to put together a script that could be given to
>> developers of various projects that are in a gerrit instance to help
>> verify that the permissions on the project in gerrit were set up
>> correctly. To do that I assumed that gerrit checked the ACL's even when
>> '--dry-run' was passed to the git push command similar to how gitolite
>> worked.
> How would gitolite possibly do this? A --dry-run doesn't send the
> commands the client would use from client to server, so the server
> can't tell the client if it would accept them or not. The entire
> --dry-run thing is client side only.
It definitely sends the commands to the server, but doesn't include the
change. When they say 'do everything except actually send the updates.'
in the git-push manpage, they really mean that the commands are sent,
just that the packs aren't.
What gitolite does, is catch the command at the ssh level, and forwards
it to a ~/.gitolite/src/gl-auth-command before forwarding on the
commands to the git server.
>> Turns out I was wrong. For any registered user, once you have
>> permissions to send changes to a project, a 'git push --dry-run gerrit
>> HEAD:master' will return success.
>>
>> My understand of what happens is as follows:
>> Gerrit overrides the prereceive hook of JGit
>> This hook only get's called when JGit is going to process the changes,
>> i.e. it won't get run if --dry-run is set
>> It's only during this hook that Gerrit actually checks permissions based
>> on refs
> IIRC --dry-run doesn't even send the commands to the server. There is
> no way for JGit to do anything here.
It does, you can test this for yourself by adding a
log.info("canUpload") line to the canUpload method in
gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
and then running a 'git push --dry-run HEAD:master' on any project and
monitoring the error_log file. You'll see that runImpl method in
./gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/Receive.java
does actually call canUpload and subsequently call into jgit, but the
PreReceiveHook is never run, because there are no packs submitted with
the upload.
--
Regards,
Darragh Bailey
Shawn Pearce
> On 23/03/12 16:05, Shawn Pearce wrote:
>> On Fri, Mar 23, 2012 at 08:50, Bailey, Darragh<dba...@hp.com> wrote:
>>> I was recently trying to put together a script that could be given to
>>> developers of various projects that are in a gerrit instance to help
>>> verify that the permissions on the project in gerrit were set up
>>> correctly. To do that I assumed that gerrit checked the ACL's even when
>>> '--dry-run' was passed to the git push command similar to how gitolite
>>> worked.
>> How would gitolite possibly do this? A --dry-run doesn't send the
>> commands the client would use from client to server, so the server
>> can't tell the client if it would accept them or not. The entire
>> --dry-run thing is client side only.
> It definitely sends the commands to the server, but doesn't include the
> change. When they say 'do everything except actually send the updates.'
> in the git-push manpage, they really mean that the commands are sent,
> just that the packs aren't.
Uhm. OK, you want to claim the commands are sent? I disagree.
builtin/send-pack.c:
300
301 if (args->dry_run) {
302 ref->status = REF_STATUS_OK;
303 } else {
304 char *old_hex = sha1_to_hex(ref->old_sha1);
305 char *new_hex = sha1_to_hex(ref->new_sha1);
306 int quiet = quiet_supported && (args->quiet ||
!args->progress);
307
308 if (!cmds_sent && (status_report || use_sideband
|| args->quiet)) {
309 packet_buf_write(&req_buf, "%s %s %s%c%s%s%s",
310 old_hex, new_hex, ref->name, 0,
311 status_report ? " report-status" : "",
312 use_sideband ? " side-band-64k" : "",
313 quiet ? " quiet" : "");
314 }
315 else
316 packet_buf_write(&req_buf, "%s %s %s",
317 old_hex, new_hex, ref->name);
318 ref->status = status_report ?
319 REF_STATUS_EXPECTING_REPORT :
320 REF_STATUS_OK;
321 cmds_sent++;
322 }
I wrote a fair chunk of this file, or at least have hacked on it
enough to know the commands are only formatted in this loop, and the
formatting is clearly skipped if --dry-run is set.
Again, how could JGit possibly tell Gerrit Code Review what the user
wants to do... when the user hasn't told JGit?
Bailey, Darragh
On 23/03/12 16:45, Shawn Pearce wrote:
>
> Uhm. OK, you want to claim the commands are sent? I disagree.
> builtin/send-pack.c:
>
>
> 300
> 301 if (args->dry_run) {
> 302 ref->status = REF_STATUS_OK;
> 303 } else {
> 304 char *old_hex = sha1_to_hex(ref->old_sha1);
> 305 char *new_hex = sha1_to_hex(ref->new_sha1);
> !args->progress);
> 307
> 308 if (!cmds_sent&& (status_report || use_sideband
> || args->quiet)) {
> 309 packet_buf_write(&req_buf, "%s %s %s%c%s%s%s",
> 310 old_hex, new_hex, ref->name, 0,
> 311 status_report ? " report-status" : "",
> 312 use_sideband ? " side-band-64k" : "",
> 313 quiet ? " quiet" : "");
> 314 }
> 315 else
> 316 packet_buf_write(&req_buf, "%s %s %s",
> 317 old_hex, new_hex, ref->name);
> 318 ref->status = status_report ?
> 319 REF_STATUS_EXPECTING_REPORT :
> 320 REF_STATUS_OK;
> 321 cmds_sent++;
> 322 }
>
> I wrote a fair chunk of this file, or at least have hacked on it
> enough to know the commands are only formatted in this loop, and the
> formatting is clearly skipped if --dry-run is set.
>
> Again, how could JGit possibly tell Gerrit Code Review what the user
> wants to do... when the user hasn't told JGit?
Tbh, I can't comment on what gets sent across, I only know that there is
definitely a connection made, and I know it's doing something other than
simply reporting on the client. I presumed it was more than an empty
message, i.e. if you attempt a non-fast-forward update with --dry-run,
the server will tell you it's rejected. Add in --force and it will
report that the update is permitted so it must be including the
reference in what is communicated.
So what is it? Based on what your saying, not enough to fully work out
exactly what the client would request without the --dry-run option, but
it's definitely not just a client only operation. Some simple log.info()
calls showed that.
--
Darragh
Shawn Pearce
>
> Tbh, I can't comment on what gets sent across, I only know that there is
> definitely a connection made, and I know it's doing something other than
> simply reporting on the client. I presumed it was more than an empty
> message, i.e. if you attempt a non-fast-forward update with --dry-run,
> the server will tell you it's rejected. Add in --force and it will
> report that the update is permitted so it must be including the
> reference in what is communicated.
Nope. This is all done by the client.
> So what is it? Based on what your saying, not enough to fully work out
> exactly what the client would request without the --dry-run option, but
> it's definitely not just a client only operation. Some simple log.info()
> calls showed that.
A connection is made to the server to find out its current reference
state. Which for Gerrit amounts to "can you even see the repository at
all".
Maybe the compliant you have is Gerrit shows you the current
references even if you have no push access at all on the project?
Bailey, Darragh
On 23/03/12 17:42, Shawn Pearce wrote:
> On Fri, Mar 23, 2012 at 10:37, Bailey, Darragh<dba...@hp.com> wrote:
>> So what is it? Based on what your saying, not enough to fully work out
>> exactly what the client would request without the --dry-run option, but
>> it's definitely not just a client only operation. Some simple log.info()
>> calls showed that.
> A connection is made to the server to find out its current reference
> state. Which for Gerrit amounts to "can you even see the repository at
> all".
Bit slowing responding, but this is where the light finally began to
dawn on me.
Also got around to looking at git client code a bit and saw the
retrieval of the remote references.
> Maybe the compliant you have is Gerrit shows you the current
> references even if you have no push access at all on the project?
No, my complaint was I clearly didn't understand what was being
communicated :-).
Thanks for the patience and sorry for the noise.
--
Regards,
Darragh Bailey