New issue
Advanced search Search tips

Issue 7750 link

Starred by 3 users

Issue metadata

Status: Released
Owner: ----
Closed: Sep 26
Components:



Sign in to add a comment

signed pushes do not seem to be enforced

Reported by webczat_...@poczta.onet.pl, Nov 13 2017

Issue description

I have gerrit 2.15-rc2.
I have receive.enableSignedPush set to true, and I have registered my public gpg key. In All-Projects I have both enable and receive signed push set to true.
I have created the test project, but when trying to push to it with --no-signed, push is allowed even though it probably should not be.
I tried to explicitly require signed pushes in the respective test project instead of All-Projects, same effect.
 
Project Member

Comment 1 by logan@google.com, Nov 14 2017

Components: -PolyGerrit Backend
Project Member

Comment 2 by jrn@google.com, Nov 24 2017

Components: -Backend ReceiveCommits docs
You can configure requiring signed push on a per-project basis: https://gerrit-review.googlesource.com/Documentation/config-project-config.html#receive.requireSignedPush.

Would a link from https://gerrit-review.googlesource.com/Documentation/config-gerrit.html#receive.enableSignedPush to that setting have helped here? (Bonus points if you can suggest wording, or even better, a patch against Documentation/config-gerrit.txt. SUBMITTING_PATCHES has details on how that works.)
Project Member

Comment 3 by jrn@google.com, Nov 24 2017

Components: -docs
Hm, you already mentioned that you're tried to require signed pushes there. Do you have a copy of the project.config or a screenshot illustrating what happened?
Project Member

Comment 4 by jrn@google.com, Nov 24 2017

[receive] enableSignedPush gets set in etc/gerrit.config.
[receive] requireSignedPush gets set in project.config in refs/meta/config.

What is the output when you push with and without --signed?
hmmm I checked a few times (unless I did it wrong). both enable and require signed push are enabled in All-Project actually, but I also tried doing it explicitly on the specific child project. gerrit.enableSignedPush is true. but it doesn't require and also rejects signed pushes. and yes, it gives me ability to upload gpg keys, does that signify signed pushes being globally enabled, or not?
won't give you exact output because it would require testing it now. I did it once. When I tried to push without --signed, the push went through, and I don't have the push.gpgSign option set. When --signed was used, the server just rejected the push, as if signed pushes were disabled. It is still possible I made a mistake, but I still think that stopped working.
Project Member

Comment 7 by jrn@google.com, Sep 16

Status: AwaitingInformation (was: New)
> both enable and require signed push are enabled in All-Project actually

You need to set

  [receive]
    enableSignedPush = true

in your etc/gerrit.config.
Project Member

Comment 8 by jrn@google.com, Sep 16

Labels: Hotlist-SignedPush
From what I see it is set.
Project Member

Comment 10 by david.os...@gmail.com, Sep 17

Status: Accepted (was: AwaitingInformation)
It doesn't work on master either.

It seems, that even though the hooks are added in SignedPushModule:

  if (ps.is(BooleanProjectConfig.REQUIRE_SIGNED_PUSH)) {
      hooks.add(SignedPushPreReceiveHook.Required.INSTANCE);
  }

e.g.: SignedPushPreReceiveHook instance is added to hooks, the hook
doesn't get called and that why a non-signed push is not checked
and rejected.

It seems that the hooks are added too late.
I the beginning only AsyncReceiveCommits preReceiveHook is set:

  receivePack.setPreReceiveHook(this);

Then from the ReceivePack.service() line: 280
this single preReceiveHook gets called: [1]

  preReceive.onPreReceive(this, filterCommands(Result.NOT_ATTEMPTED));

in the same call stack Worker instance is created and initialized,
and only then the other hooks (with SignedPushPreReceiveHook) gets added
and the same receivePack instance is mutated and hook chain is
assigned, but they don't get invoked any more: [2].

  rp.setPreReceiveHook(PreReceiveHookChain.newChain(hooks));

[1] http://paste.openstack.org/show/730202
[2] http://paste.openstack.org/show/730203
Project Member

Comment 11 by david.os...@gmail.com, Sep 17

The reason it works on stable-2.14 branch:

The hooks are initialized before the first hook is invoked:[1]:

  preReceive.onPreReceive(this, filterCommands(Result.NOT_ATTEMPTED));

[1] http://paste.openstack.org/show/730204
Project Member

Comment 12 by david.os...@gmail.com, Sep 17

I think that this change is related: [1]. I added a comment there.

[1] https://gerrit-review.googlesource.com/c/gerrit/+/118292
Project Member

Comment 13 by david.os...@gmail.com, Sep 17

Status: Started (was: Accepted)
Project Member

Comment 14 by david.os...@gmail.com, Sep 17

Status: ChangeUnderReview (was: Started)
https://gerrit-review.googlesource.com/c/gerrit/+/196230
Project Member

Comment 15 by david.os...@gmail.com, Sep 18

Acceptance test is added to catch similar regression in future gerrit releases:

https://gerrit-review.googlesource.com/c/gerrit/+/196231
Project Member

Comment 16 by david.os...@gmail.com, Sep 19

Labels: Blocking-2.16
Project Member

Comment 17 by david.os...@gmail.com, Sep 19

Labels: Triaged-Yes Priority-1
Project Member

Comment 18 by david.os...@gmail.com, Sep 26

Status: Submitted (was: ChangeUnderReview)
Labels: FixedIn-2.15.4
Status: Released (was: Submitted)

Sign in to add a comment