Re: git signed push server-side

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Aug 26, 2017, 00:32 UTC
Message-ID: <20170826003229.GL13924@aiede.mtv.corp.google.com>
In-Reply-To: <22944.38288.91698.811743@chiark.greenend.org.uk>

+Dave Borowitz, who implemented push cert handling in JGit and Gerrit Hi Ian,

Ian Jackson wrote[1]:

Show 15 quoted lines

> I have been investigating git signed pushes.  I found a number of
> infelicities in the server side implementation which make using this
> in practice rather difficult.  I'm emailing here (before writing
> patches) to see what people think of my proposed changes.
>
> 1. PUSH_CERT_KEY has truncated keyid (Debian #852647)
>
> I see this:
>   GIT_PUSH_CERT_KEY=A3DBCBC039B13D8A
>
> There is almost no purpose for which this 64-bit keyid can be safely
> used.  The full key fingerprint should be provided instead.
>
> Proposed change: provide the full fingerprint instead.  Do this
> for every caller of gpg-interface.c.

Sounds sane.

Show 15 quoted lines

> 2. git-receive-pack calls gpg (Debian #852684)
>
> It would be better if it called gpgv.  gpg does all sorts of
> complicated things, including automatically starting or connecting to
> a gpg-agent, which are not appropriate for use in a daemon on a
> server.
>
> Additionally, I find that passing -c gpg.program=/usr/bin/gpgv
> to git receive-pack is not effective, and there seems to be no
> sensible way to specify the keyrings to use (although that could be
> done by setting GNUPGHOME perhaps).
>
> Proposed change: call gpgv instead (and make any needed changes to
> adapt to gpgv).  Do this only when we are in git-receive-pack; other
> call sites of gpg-interface.c will continue to use gpg.

I think respecting gpg.program would be nicer. Is there a reason not to do that?

I suspect receive-pack just forgot to call git_gpg_config.

Show 5 quoted lines

> 3. No way to specify keyring (Debian #852684, side note)
>
> There should be a way to specify the keyring used by
> git-receive-pack's gpgv invocation.  This should probably be done with
> a config option, receive.certKeyring perhaps.

How is the keyring configured for other commands that use GPG, like "git tag -v"? (Forgive my laziness in not looking it up.)

Show 10 quoted lines

> 4. Trouble with the nonce (Debian #852688 part 2)
>
> To use the signed push feature it is necessary to provide a nonce seed
> to git-receive-pack.
>
> The docs say the seed must be secret but there is no documented way to
> pass this seed to git that does not either write it to a git
> configuration file somewhere, or pass it on a command line.  The git
> configuration system is unsuited to keeping secrets.  Command lines
> can be seen in ps etc.

[...]

Show 9 quoted lines

> Proposed fix (in two parts):
>
> (i) Provide a new config option receive.certNonceSeedsFile.  It
> contains seeds, one per line.  When stateless_rpc, we send a nonce
> computed from the first seed.  We accept nonces computed from any of
> the listed seeds.  The documentation will say that the file should
> normally contain two seeds; rollover is achieved by mving into place a
> new seed at the top, and dropping one from the bottom.  An example
> script will be provided.

I like it.

I also wonder why you say the git configuration system is unsuited to keeping secrets. E.g. passing an include.path setting with -c or GIT_CONFIG_PARAMETERS should avoid the kinds of trouble you described. Is there a change we could make to make it work better? That said, I think being able to name a file is a good idea.

> (ii) At some later point, the following enhancement: When
> !stateless_rpc, certNonceSeedsFile is ignored except that if neither
> it nor the old certNonceSeed is set, the signed push feature is
> disabled.

That seems like an awkward interface. Shouldn't there be at least another config variable to enable signed push without making up a seed or filename?

>            In this state we always get a fresh nonce (from a suitable
> system random source).

How does this work with stateless_rpc? (See "Session State" in Documentation/technical/http-protocol.txt.)

>                         Nontrivial because current git doesn't seem to
> have a "get suitable random number" function, and the mess that is the
> semantics of /dev/*random* files means that providing one is going to
> be controversial.

I think you're overestimating how much pushback adding such a thing would get.

Show 9 quoted lines

> 5. There are no docs on how to use this feature properly
>    (Debian #852695, #852688 part 1)
>
> Using the signed push feature requires careful programming on the
> server side.  There should be a doc explaining how to do this.
>
> Proposed fix: provide a .txt file containing much the same contents as
> seen here:
>   https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=852695

Yes, that sounds like a very welcome kind of thing to add.

More references:

- JGit's push cert handling:
  https://git.eclipse.org/r/#/q/message:cert

- Gerrit's push cert handling:
  https://gerrit-review.googlesource.com/q/project:gerrit+message:gpg

I haven't been able to find much in terms of docs for the feature. There is https://gerrit-review.googlesource.com/Documentation/config-gerrit.html#receive.trustedKey and https://gerrit-review.googlesource.com/Documentation/config-project-config.html#receive.enableSignedPush. If signed push is enabled but not required for a repository then if I remember correctly it is able to show whether an upload was signed by a trusted key, as context during a review.

Thanks and hope that helps, Jonathan

[1] https://public-inbox.org/git/22944.38288.91698.811743@chiark.greenend.org.uk/