git signed push server-side

git signed push server-side

[Ian Jackson]

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.


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.


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.


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.

Additionally, the seed should be changed occasionally (since
unchangeable secrets are a bad idea).  Ideally this should be done
automatically.

Analysis: the seed is used to allow the server to mostly-statelessly
verify the freshness of a client's nonce.  (This is necessary only in
connectionless transport protocols, "stateless_rpc" as
builtin/receive-pack.c has it.)

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.

(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.  In this state we always get a fresh nonce (from a suitable
system random source).  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.


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


If I send patches like the above, would they be welcomed (subject to
detailed review, of course) ?

Thanks,
Ian.

-- 
Ian Jackson <ijackson@chiark.greenend.org.uk>   These opinions are my own.

If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
a private address which bypasses my fierce spamfilter.

Re: git signed push server-side

[Junio C Hamano]

Ian Jackson <ijackson@chiark.greenend.org.uk> writes:

> 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.
> ...
> If I send patches like the above, would they be welcomed (subject to
> detailed review, of course) ?

When I did the signed push, the primary focus was to get the
protocol extension right, and what the server end does with the
received certificate was left as something that can be refined later
by those who are really into it.  

So I do find it a welcome development that you are looking into it
(but remember, I do not represent "people"---I am merely one of them).

I and others may or may not have objections and may or may not offer
better ideas over what you are going to write in your design docs
and patches---I cannot promise anything before seeing them.

Re: git signed push server-side

[Jonathan Nieder]

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

Ian Jackson wrote[1]:

> 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.

> 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.

> 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.)

> 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.
[...]
> 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.

> 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/

Re: git signed push server-side

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> +Dave Borowitz, who implemented push cert handling in JGit and Gerrit
> Hi Ian,
>
> Ian Jackson wrote[1]:
>
>> 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.

I probably should react a bit stronger against that "instead", as
Ian will not be writing the world's first server side hook that uses
this interface.  A different variable that lets you read the full
length "in addition" I wouldn't have a problem with, as existing
scripts will continue working the same way if you did so.

But on the other hand, the value of this environment is not meant to
be used to make decision by the hook anyway, so it perhaps is OK to
change it in a backward incompatible way to break those who have
been using the value for any serious purpose.

The purpose of the signed push is not to replace authentication and
authorization.  The primary goal behind the signed push mechanism is
to allow server side hook to implement a way to store these certs in
the order it receives without losing them, and that gives a way for
the server operators to protect against claims that they are showing
what the pusher did not intend to publish.  They can say "the tip of
these branches are at this commit, because, see this, a signed cert
by the pusher asking us to put these commits at these branches" with
such a mechanism---as opposed to "we authenticated the user and here
is our server log that says that user pushed to update these refs",
which is much weaker claim that they can make without the signed
push mechanism.

Re: git signed push server-side

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> 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.

That would be a good change.

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

AFAIR we never do anything special, so you should be able to point
GNUPGHOME to wherever you like to use the desired configuration.

> 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.

I also wonder that too.  The configuration file that has the
filename could be made just as secret and unreadable from public as
the new file that stores the seed with the same mechanism, I would
imagine.

>> 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.

This was rather deliberately left underspecified, hoping that the
BCP would emerge after people gain experience.  As Ian is looking
into this and hopefully gain real-world experience, we can have a
good BCP description after he is done with his project ;-)

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

Indeed.

Re: git signed push server-side [and 3 more messages]

[Ian Jackson]

Hi.  Thanks to both of you for your helpful comments.

Jonathan Nieder writes ("Re: git signed push server-side"):
> Ian Jackson wrote[1]:
> > 2. git-receive-pack calls gpg (Debian #852684)
> >
> > It would be better if it called gpgv.
...
> think respecting gpg.program would be nicer.  Is there a reason not
> to do that?

I think it very unlikely that anyone would want git-receive-pack's
signed push facility to end up running gpg rather than gpgv.  But, the
git-receive-pack functions here are building blocks which need quite a
lot of extra work to use, anyway.  So having all callers have to pass
-c gpg.program would be quite tolerable, if a bit ugly.

> I suspect receive-pack just forgot to call git_gpg_config.

So, I will send a patch to fix that.

> > 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.)

It's not.  You just get whatever keyring and trust db etc. your gpg
has as the default.  As Junio says, being able to set the keyring
explicitly is not essential to use the signed push feature; one can
make a wrapper for gpgv, or set GNUPGHOME.

It just seemed to me that the current interface is not very
convenient.  If it is controversial to provide a more convenient
interface, then I will work with what there is.

> > 4. Trouble with the nonce (Debian #852688 part 2)
...
> 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.

I was not aware of include.path.  That would solve this aspect of the
problem (but not the rollover aspect, of course).
GIT_CONFIG_PARAMETERS is not documented.

> > (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?

Perhaps.  I don't have a strong opinion about the config parameter
names and semantics.  This seems like a matter of taste and I'm happy
to just do whatever others want.

> >            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.)

It doesn't, which is why I propose this only if !stateless_rpc.

> >                         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.

Well, I'm happy to give it ago.  (NB: I will use /dev/urandom on
Linux.)

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

Really helpful, thanks.  I will read these.  (Shame that I can't even
view that information without running their javascript!)

I ended up reading this
 https://github.com/sitaramc/gitolite/blob/cf062b8bb6b21a52f7c5002d33fbc950762c1aa7/contrib/hooks/repo-specific/save-push-signatures

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

(This is an empty page for me, even with javascript turned on.)

I'll probably implement the refs/meta/push-certs:REF@{cert} special
branch thing from JGit.  Can anyone point me at a public repo which
has such push-certs recorded, so I can check the details and make my
thing do stuff the same way ?

Junio C Hamano writes ("Re: git signed push server-side"):
> Jonathan Nieder <jrnieder@gmail.com> writes:
> > Ian Jackson wrote[1]:
> >> Proposed change: provide the full fingerprint instead.  Do this
> >> for every caller of gpg-interface.c.
> >
> > Sounds sane.
> 
> I probably should react a bit stronger against that "instead", as
> Ian will not be writing the world's first server side hook that uses
> this interface.

Anyone who is verifying signatures and doing anything with the
16-character key id other than logging it, has a serious security bug.

> But on the other hand, the value of this environment is not meant to
> be used to make decision by the hook anyway, so it perhaps is OK to
> change it in a backward incompatible way to break those who have
> been using the value for any serious purpose.

Precisely.

> The purpose of the signed push is not to replace authentication and
> authorization.  The primary goal behind the signed push mechanism is
> to allow server side hook to implement a way to store these certs in
> the order it receives without losing them, and that gives a way for
> the server operators to protect against claims that they are showing
> what the pusher did not intend to publish.  They can say "the tip of
> these branches are at this commit, because, see this, a signed cert
> by the pusher asking us to put these commits at these branches" with
> such a mechanism---as opposed to "we authenticated the user and here
> is our server log that says that user pushed to update these refs",
> which is much weaker claim that they can make without the signed
> push mechanism.

I don't understand why the signed push system could (or should) not be
used for A&A.  It seems eminently suited to it, in circumstances where
traceability is important (and the pushers can be expected to be gpg
users).

Junio C Hamano writes ("Re: git signed push server-side"):
> When I did the signed push, the primary focus was to get the
> protocol extension right, and what the server end does with the
> received certificate was left as something that can be refined later
> by those who are really into it.  

Right.  I looked at the protocol in some detail (I have a background
in crypto protocol design) and it seemed good to me.  (Although, I
have not done a formal verification.)

> I and others may or may not have objections and may or may not offer
> better ideas over what you are going to write in your design docs
> and patches---I cannot promise anything before seeing them.

Of course.  But it is helpful for me to get some input on what
direction to go in, before I write and test code.

FTR, I don't expect to do anything particularly quickly.  And, of
course, I'd welcome any other suggestions/input in the meantime.

Thanks,
Ian.

-- 
Ian Jackson <ijackson@chiark.greenend.org.uk>   These opinions are my own.

If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
a private address which bypasses my fierce spamfilter.