[PATCH] Add a sample hook which saves push certs as notes

[Shikher Verma]

hooks--post-receive.sample: If push cert is present, add it as a git
note to the top most commit of the updated ref.

Signed-off-by: Shikher Verma <r...@shikherverma.com>
---
 templates/hooks--post-receive.sample | 38 
 1 file changed, 38 insertions(+)
 create mode 100755 templates/hooks--post-receive.sample

diff --git a/templates/hooks--post-receive.sample 
b/templates/hooks--post-receive.sample
new file mode 100755
index 0..b4366e43f
--- /dev/null
+++ b/templates/hooks--post-receive.sample
@@ -0,0 +1,38 @@
+#!/bin/sh
+#
+# An example hook script to store push certificates as notes.
+#
+# To enable this hook, rename this file to "post-receive".
+#
+# The stdin of the hook will be one line for each updated ref:
+#   
+#
+# For each updated ref this script will :
+# 1. Verify that the ref update matches that in push certificate.
+# 2. add the push cert as note (namespace pushcerts) to .
+#
+# If this hook is enabled on the server then clients can prevent
+# git metadata tampering, by using signed pushes and 
+# doing the following while fetching :
+# 1. fetch the git notes (of namespace pushcerts) from server.
+# $ git fetch origin refs/notes/pushcerts:refs/notes/pushcerts
+# 2. Check that the fetched ref's top most commit has a note
+# containing a push certificate.
+# 3. Verify the validity of the push certificate in the note and 
+# check that the ref update matches that in push certificate.
+#
+
+if test -z GIT_PUSH_CERT ; then
+exit 0
+fi
+
+push_cert=$(git cat-file -p  $GIT_PUSH_CERT)
+
+while read oval nval ref
+do
+   # Verify that the ref update matches that in push certificate.
+   if [[ $push_cert == *$oval" "$nval" "$ref* ]]; then
+   # add the push cert as note (namespaced pushcerts) to nval.
+   git notes --ref=pushcerts add -m "$push_cert" $nval -f
+   fi
+done
-- 
2.15.0

Re: [RFC PATCH 0/2] Add named reference to latest push cert

[Shikher Verma]

On Thu, Sep 07, 2017 at 05:43:19PM +, Stefan Beller wrote:
> On Thu, Sep 7, 2017 at 2:11 AM, Shikher Verma <r...@shikherverma.com> wrote:
> > On Wed, Sep 06, 2017 at 02:31:49PM -0700, Stefan Beller wrote:
> >> On Wed, Sep 6, 2017 at 2:39 AM, Shikher Verma <r...@shikherverma.com> 
> >> wrote:
> >> > Currently, git only stores push certificates if there is a receive hook
> >> > present. This may violate the principle of least surprise (e.g., I
> >> > pushed with --signed, and I don't see anything in upstream).
> >> > Additionally, push certificates could be more versatile if they are not
> >> > tightly bound to git hooks. Finally, it would be useful to verify the
> >> > signed pushes at later points of time with ease.
> >> >
> >> > A named ref is added for ease of access/tooling around push
> >> > certificates. If the last push was signed, ref/PUSH_CERT stores the
> >> > ref of the latest push cert otherwise it is empty.
> >> >
> >> > Sending patches as RFC since the documentation would have to be
> >> > updated and git gc might have to be patched to not garbage collect
> >> > the latest push certificate.
> >> >
> >> > This patch applies on master (3ec7d702a)
> >>
> >> What are performance implications for busy repositories at busy hosts?
> >> (think kernel.org / github) They may want to disable this new feature
> >> for performance reasons or because they don't want to clutter the
> >> object store. So at least a config option to turn it off would be useful.
> >
> > Any typical git push would write several objects to disk,
> 
> (or just one pack file, [which may be renamed eventually, see 722ff7f8])
> 
> > this patch
> > would only add one more object per push so I think the performance
> > penalty is not that high. But I agree that we can have a config to turn
> > it off.
> 
> I personally do not run a high performance server, so I do not terribly mind,
> but thought it would be nice for them to have at least an option ready made
> instead of a potential performance regression.
> 
> >> On the ref to store the push certs:
> >> (a) Currently the ref points at the blob, I wonder if we'd rather want to
> >> point at a commit? (Then we can build up a history of
> >> push certs, instead of relying on the reflog to show all
> >> push certs. Also the gc issue might be easier to solve using this)
> >
> > I am not sure how that would work. The ref points at the blob of push
> > certificate. Since each push can update multiple refs, each push
> > certificate can point to mutiple commits (tip of the updated refs).
> > Also if the named ref points at the commit then how will we get the
> > corresponding push certificate?
> >
> > I did think about keeping a history of push certificates but the problem
> > is new pushes can delete refs and commits which were pointed to by
> > previous push certificates. This makes it really difficult to decide
> > which push certificates to keep and which to gc. Also this history would
> > be different for different clones of the same repo. Since push
> > certificate are only meta data of the git workflow I think its best to
> > just keep the latest push certificate and gc the old ones. People can
> > use the recieve hook if they want to do advance things like logging a
> > history of push certificates. I think git should provide a builtin
> > solution for the simple case.
> 
> What I had in mind was what would be achieved with a
> hook like this (untested):
> 
> #!/bin/sh
> if test -z GIT_PUSH_CERT ; then
> exit 0
> fi
> 
> # add a new worktree 'tmp', checking out the magic ref:
> git worktree add tmp refs/PUSH_CERT
> 
> cp $GIT_PUSH_CERT tmp/cert
> git -C tmp add cert
> git -C tmp commit -m "new push cert"
> # maybe include GIT_PUSH_CERT_[NONCE_]STATUS
> # in commit message?
> 
> # clean up, command doesn't exist yet:
> git worktree delete tmp
>

This might be a good starting point for a sample hook if we choose to go
that way. As Junio suggested.
> This would not deal with concurrency as it re-uses the
> same worktree, but illustrates what I had in mind
> for the git history of that special ref.

I personally feel that we should decouple push certificates from hooks
and serve the push certificate whenever someone does
`git pull --signed important-ref`. That way we remove trust from
services like Github, Gitlab, Bitbucket. This could be done if git
stores a map of re

Re: [RFC PATCH 0/2] Add named reference to latest push cert

[Shikher Verma]

On Wed, Sep 06, 2017 at 02:31:49PM -0700, Stefan Beller wrote: 
> On Wed, Sep 6, 2017 at 2:39 AM, Shikher Verma <r...@shikherverma.com> wrote: 
> > Currently, git only stores push certificates if there is a receive hook 
> > present. This may violate the principle of least surprise (e.g., I 
> > pushed with --signed, and I don't see anything in upstream). 
> > Additionally, push certificates could be more versatile if they are not 
> > tightly bound to git hooks. Finally, it would be useful to verify the 
> > signed pushes at later points of time with ease. 
> > 
> > A named ref is added for ease of access/tooling around push 
> > certificates. If the last push was signed, ref/PUSH_CERT stores the 
> > ref of the latest push cert otherwise it is empty. 
> > 
> > Sending patches as RFC since the documentation would have to be 
> > updated and git gc might have to be patched to not garbage collect 
> > the latest push certificate. 
> > 
> > This patch applies on master (3ec7d702a) 
> 
> What are performance implications for busy repositories at busy hosts? 
> (think kernel.org / github) They may want to disable this new feature 
> for performance reasons or because they don't want to clutter the 
> object store. So at least a config option to turn it off would be useful. 

Any typical git push would write several objects to disk, this patch 
would only add one more object per push so I think the performance 
penalty is not that high. But I agree that we can have a config to turn 
it off. 
> On the ref to store the push certs: 
> (a) Currently the ref points at the blob, I wonder if we'd rather want to 
> point at a commit? (Then we can build up a history of 
> push certs, instead of relying on the reflog to show all 
> push certs. Also the gc issue might be easier to solve using this) 

I am not sure how that would work. The ref points at the blob of push 
certificate. Since each push can update multiple refs, each push 
certificate can point to mutiple commits (tip of the updated refs). 
Also if the named ref points at the commit then how will we get the 
corresponding push certificate? 

I did think about keeping a history of push certificates but the problem 
is new pushes can delete refs and commits which were pointed to by 
previous push certificates. This makes it really difficult to decide 
which push certificates to keep and which to gc. Also this history would 
be different for different clones of the same repo. Since push 
certificate are only meta data of the git workflow I think its best to 
just keep the latest push certificate and gc the old ones. People can 
use the recieve hook if they want to do advance things like logging a 
history of push certificates. I think git should provide a builtin 
solution for the simple case. 

Another motivation to decouple push certificates from hooks was that 
later we could store a map of refs to the latest push cert which 
updated the ref. And serve the corresponding push cert whenever someone 
does `git pull --signed important-ref`. Effectively removing trust from 
the server by preventing tampering with refs. This could really help 
the Github generation developers like me.
> (b) When going with (a), we might want to change the name. Most 
> refs are 3 directories deep: 
> refs/heads/ 
> refs/pr/ # at github IIUC 
> refs/changes/ # Gerrit 
> refs/meta/config # Gerrit to e.g. configure ACLs of the repo 
> "refs" indicates it is a ref, whereas the second part can be seen 
> as a "namespace". Currently Git only uses the "heads" and "tags" 
> namespace, "meta" is used by more than just Gerrit, so maybe it is 
> not wise to use "refs/meta/push_cert", but go with refs/gitmeta/pushcert 
> instead? 

Re: [RFC PATCH 0/2] Add named reference to latest push cert

[Shikher Verma]

On Thu, Sep 07, 2017 at 09:55:25AM +0900, Junio C Hamano wrote:
> Stefan Beller  writes:
> 
> > On the ref to store the push certs:
> > (a) Currently the ref points at the blob, I wonder if we'd rather want to
> > point at a commit? (Then we can build up a history of
> > push certs, instead of relying on the reflog to show all
> > push certs. Also the gc issue might be easier to solve using this)
> > (b) When going with (a), we might want to change the name. Most
> > refs are 3 directories deep:
> >   refs/heads/
> >   refs/pr/ # at github IIUC
> >   refs/changes/ # Gerrit
> >   refs/meta/config # Gerrit to e.g. configure ACLs of the repo
> > "refs" indicates it is a ref, whereas the second part can be seen
> > as a "namespace". Currently Git only uses the "heads" and "tags"
> > namespace, "meta" is used by more than just Gerrit, so maybe it is
> > not wise to use "refs/meta/push_cert", but go with refs/gitmeta/pushcert
> > instead?
> 
> You also need to worry about concurrent pushes.  The resulting
> "history" may not have to be sequencial, but two pushes that affect
> the same ref must be serialized in the resulting push-cert store.

Oh I see. I guess concurrency would be an issue. How does recieve-pack
handle concurrent pushes? Is there a lock that I could use to decide 
if named push cert ref has to be updated or not?

> The original design deliberately punts all the complexity to hook
> exactly because we do not want to have a half-baked "built-in"
> implementation that would only get in the way of those who wants to
> do high-performance servers.  It is very likely that they want to
> have a long-running daemon that listens to a port or a named pipe,
> where the only thing the hook would do is to write the value of
> GIT_PUSH_CERT to that daemon process, which acts as a serialization
> point, can read from the object store that is used as a short-term
> temporary area, and write the push cert to a more permanent store.
> 
> Having said all that, I am sympathetic to a wish to have an
> easy-to-enable-without-thinking example that is not so involved
> (e.g. no portability concern, does not have to perform very well but
> must be correct).  If Shikher wants to add one, I think the right
> approach to do so would be to add and ship a sample hook.
> 

This patch would only add one more object to write per push so I 
think the performance penalty is not that high. We can have a config
to turn it off so that it does not get in the way of those who want 
to do high-performance servers.

People can use the recieve hook for advance use cases but I think git
should provide a builtin solution for the simple case. The reason I
favour a named ref over a sample hook is because decouping push
certificate from hook opens up more possibilities like we could store
a map of refs to the latest push cert which updated the ref. And 
serve the corresponding push cert whenever someone does 
`git pull --signed important-ref`. Effectively removing trust from 
the server by preventing tampering with refs. This could really help 
the Github generation developers like me. What do you think?

> Thanks.

Re: [RFC PATCH 0/2] Add named reference to latest push cert

[Shikher Verma]

Hey everyone,

I felt like I should introduce myself since this is my first patch on
the git mailing list (or any mailing list actually) :D

I am Shikher[1], currently in my 4th year undergrad at IIT Kanpur.
This summer I was lucky enough to intern at NYU Secure Systems Lab[2]
mentored by Santiago. We looked into how signed pushes work and how 
we can use them to increase the security of git. We encountered a
strange error in tests which resulted in a patch[3] and I wrote a
python script to verify push certificates[4]. I was pretty surprised 
to not find any push certificate on the remote repo after I did a 
signed push, hence this RFC.

Anyway this is my first time trying to contribute to a large OSS so 
forgive me if I make any noob mistakes.

Thanks
Shikher Verma

[1]http://shikherverma.com/
[2]https://ssl.engineering.nyu.edu/
[3]https://public-inbox.org/git/20170707220159.12752-1-santi...@nyu.edu/
[4]https://gist.github.com/ShikherVerma/9204060b545c00597e7ad9b694cfeb9c

On Wed, Sep 06, 2017 at 03:09:11PM +0530, Shikher Verma wrote:
> Currently, git only stores push certificates if there is a receive hook 
> present. This may violate the principle of least surprise (e.g., I 
> pushed with --signed, and I don't see anything in upstream). 
> Additionally, push certificates could be more versatile if they are not 
> tightly bound to git hooks. Finally, it would be useful to verify the 
> signed pushes at later points of time with ease.
> 
> A named ref is added for ease of access/tooling around push 
> certificates. If the last push was signed, ref/PUSH_CERT stores the 
> ref of the latest push cert otherwise it is empty.
>  
> Sending patches as RFC since the documentation would have to be 
> updated and git gc might have to be patched to not garbage collect 
> the latest push certificate.
> 
> This patch applies on master (3ec7d702a) 
> 
> Shikher Verma (2):
>   Always write push cert to disk
>   Store latest push cert ref in PUSH_CERT
> 
>  builtin/receive-pack.c | 25 -
>  path.c |  1 +
>  path.h |  1 +
>  3 files changed, 22 insertions(+), 5 deletions(-)
> 
> -- 
> 2.14.1
> 

[RFC PATCH 1/2] Always write push cert to disk

[Shikher Verma]

receive-pack: store push cert irrespective of hook

Push certificates are being saved to disk only when hook was
attached on the server side, which may complicate tooling and
interaction with the certificates. Add write_push_cert_sha1() to
save push cert to disk regardless of this.

Signed-off-by: Shikher Verma <r...@shikherverma.com>
Helped-by: Santiago Torres-Arias <santi...@nyu.edu>
---
 builtin/receive-pack.c | 16 +++-
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 52c63ebfd..79195005f 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -619,7 +619,15 @@ static int check_cert_push_options(const struct 
string_list *push_options)
return retval;
 }
 
-static void prepare_push_cert_sha1(struct child_process *proc)
+static void write_push_cert_sha1()
+{
+   if (!push_cert.len)
+   return;
+   if (write_sha1_file(push_cert.buf, push_cert.len, "blob", 
push_cert_sha1))
+   hashclr(push_cert_sha1);
+}
+
+static void prepare_push_cert(struct child_process *proc)
 {
static int already_done;
 
@@ -632,9 +640,6 @@ static void prepare_push_cert_sha1(struct child_process 
*proc)
int bogs /* beginning_of_gpg_sig */;
 
already_done = 1;
-   if (write_sha1_file(push_cert.buf, push_cert.len, "blob", 
push_cert_sha1))
-   hashclr(push_cert_sha1);
-
memset(, '\0', sizeof(sigcheck));
sigcheck.result = 'N';
 
@@ -727,7 +732,7 @@ static int run_and_feed_hook(const char *hook_name, feed_fn 
feed,
proc.err = muxer.in;
}
 
-   prepare_push_cert_sha1();
+   prepare_push_cert();
 
code = start_command();
if (code) {
@@ -1980,6 +1985,7 @@ int cmd_receive_pack(int argc, const char **argv, const 
char *prefix)
for (cmd = commands; cmd; cmd = cmd->next)
cmd->error_string = "inconsistent push options";
}
+   write_push_cert_sha1();
 
prepare_shallow_info(, );
if (!si.nr_ours && !si.nr_theirs)
-- 
2.14.1

[RFC PATCH 0/2] Add named reference to latest push cert

[Shikher Verma]

Currently, git only stores push certificates if there is a receive hook 
present. This may violate the principle of least surprise (e.g., I 
pushed with --signed, and I don't see anything in upstream). 
Additionally, push certificates could be more versatile if they are not 
tightly bound to git hooks. Finally, it would be useful to verify the 
signed pushes at later points of time with ease.

A named ref is added for ease of access/tooling around push 
certificates. If the last push was signed, ref/PUSH_CERT stores the 
ref of the latest push cert otherwise it is empty.
 
Sending patches as RFC since the documentation would have to be 
updated and git gc might have to be patched to not garbage collect 
the latest push certificate.

This patch applies on master (3ec7d702a) 

Shikher Verma (2):
  Always write push cert to disk
  Store latest push cert ref in PUSH_CERT

 builtin/receive-pack.c | 25 -
 path.c |  1 +
 path.h |  1 +
 3 files changed, 22 insertions(+), 5 deletions(-)

-- 
2.14.1

[RFC PATCH 2/2] Store latest push cert ref in PUSH_CERT

[Shikher Verma]

path: Add named reference to push cert
builtin/receive-pack: update named push cert ref

Push certificates are "detached" objects on the git objects tree.
Add a named reference so the latest push certificate can be easily
fetched, verified and stored for later use. This eases tooling
around the push certificate feature.

Signed-off-by: Shikher Verma <r...@shikherverma.com>
Helped-by: Santiago Torres-Arias <santi...@nyu.edu>
---
 builtin/receive-pack.c | 9 +
 path.c | 1 +
 path.h | 1 +
 3 files changed, 11 insertions(+)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 79195005f..2fdeafe63 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1530,6 +1530,15 @@ static void execute_commands(struct command *commands,
 
if (shallow_update)
warn_if_skipped_connectivity_check(commands, si);
+
+   for (struct command *cmd = commands; cmd; cmd = cmd->next)
+   if (cmd->error_string)
+   return;
+   struct strbuf buf = STRBUF_INIT;
+   if (push_cert.len)
+   strbuf_addstr(, sha1_to_hex(push_cert_sha1));
+   strbuf_addch(, '\n');
+   write_file_buf(git_path_push_cert(), buf.buf, buf.len);
 }
 
 static struct command **queue_command(struct command **tail,
diff --git a/path.c b/path.c
index b533ec938..5dca2a7bf 100644
--- a/path.c
+++ b/path.c
@@ -1366,3 +1366,4 @@ GIT_PATH_FUNC(git_path_merge_mode, "MERGE_MODE")
 GIT_PATH_FUNC(git_path_merge_head, "MERGE_HEAD")
 GIT_PATH_FUNC(git_path_fetch_head, "FETCH_HEAD")
 GIT_PATH_FUNC(git_path_shallow, "shallow")
+GIT_PATH_FUNC(git_path_push_cert, "refs/PUSH_CERT")
diff --git a/path.h b/path.h
index 9541620c7..4bdeb1f07 100644
--- a/path.h
+++ b/path.h
@@ -78,5 +78,6 @@ const char *git_path_merge_mode(void);
 const char *git_path_merge_head(void);
 const char *git_path_fetch_head(void);
 const char *git_path_shallow(void);
+const char *git_path_push_cert(void);
 
 #endif /* PATH_H */
-- 
2.14.1