{"thread":{"id":"46714","subject":"Re: [RFC PATCH 0/2] Add named reference to latest push cert","startedAt":"2017-09-06T21:31:59Z","lastAt":"2017-12-03T06:05:48Z","messageCount":16,"participants":["Stefan Beller","Junio C Hamano","Shikher Verma","Santiago Torres","Todd Zullinger"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"327674","messageId":"CAGZ79kbxDh11KxrKCk_VjmN06kzp7x4iVO6XTV=a-qBmm39K5A@mail.gmail.com","threadId":"46714","inReplyTo":"20170906093913.21485-1-root@shikherverma.com","subject":"Re: [RFC PATCH 0/2] Add named reference to latest push cert","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-06T21:31:49Z","receivedAt":"2017-09-06T21:31:59Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Sep 6, 2017 at 2:39 AM, Shikher Verma <root@shikherverma.com> wrote:\n> Currently, git only stores push certificates if there is a receive hook\n> present. This may violate the principle of least surprise (e.g., I\n> pushed with --signed, and I don't see anything in upstream).\n> Additionally, push certificates could be more versatile if they are not\n> tightly bound to git hooks. Finally, it would be useful to verify the\n> signed pushes at later points of time with ease.\n>\n> A named ref is added for ease of access/tooling around push\n> certificates. If the last push was signed, ref/PUSH_CERT stores the\n> ref of the latest push cert otherwise it is empty.\n>\n> Sending patches as RFC since the documentation would have to be\n> updated and git gc might have to be patched to not garbage collect\n> the latest push certificate.\n>\n> This patch applies on master (3ec7d702a)\n\nWhat are performance implications for busy repositories at busy hosts?\n(think kernel.org / github) They may want to disable this new feature\nfor performance reasons or because they don't want to clutter the\nobject store. So at least a config option to turn it off would be useful.\n\nOn the ref to store the push certs:\n(a) Currently the ref points at the blob, I wonder if we'd rather want to\n    point at a commit? (Then we can build up a history of\n    push certs, instead of relying on the reflog to show all\n    push certs. Also the gc issue might be easier to solve using this)\n(b) When going with (a), we might want to change the name. Most\n    refs are 3 directories deep:\n      refs/heads/<branch name>\n      refs/pr/<pull request nr> # at github IIUC\n      refs/changes/<id> # Gerrit\n      refs/meta/config # Gerrit to e.g. configure ACLs of the repo\n    \"refs\" indicates it is a ref, whereas the second part can be seen\n    as a \"namespace\". Currently Git only uses the \"heads\" and \"tags\"\n    namespace, \"meta\" is used by more than just Gerrit, so maybe it is\n    not wise to use \"refs/meta/push_cert\", but go with refs/gitmeta/pushcert\n    instead?\n"},{"id":"327685","messageId":"xmqq4lsf2upu.fsf@gitster.mtv.corp.google.com","threadId":"46714","inReplyTo":"CAGZ79kbxDh11KxrKCk_VjmN06kzp7x4iVO6XTV=a-qBmm39K5A@mail.gmail.com","subject":"Re: [RFC PATCH 0/2] Add named reference to latest push cert","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-07T00:55:25Z","receivedAt":"2017-09-07T00:55:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> On the ref to store the push certs:\n> (a) Currently the ref points at the blob, I wonder if we'd rather want to\n>     point at a commit? (Then we can build up a history of\n>     push certs, instead of relying on the reflog to show all\n>     push certs. Also the gc issue might be easier to solve using this)\n> (b) When going with (a), we might want to change the name. Most\n>     refs are 3 directories deep:\n>       refs/heads/<branch name>\n>       refs/pr/<pull request nr> # at github IIUC\n>       refs/changes/<id> # Gerrit\n>       refs/meta/config # Gerrit to e.g. configure ACLs of the repo\n>     \"refs\" indicates it is a ref, whereas the second part can be seen\n>     as a \"namespace\". Currently Git only uses the \"heads\" and \"tags\"\n>     namespace, \"meta\" is used by more than just Gerrit, so maybe it is\n>     not wise to use \"refs/meta/push_cert\", but go with refs/gitmeta/pushcert\n>     instead?\n\nYou also need to worry about concurrent pushes.  The resulting\n\"history\" may not have to be sequencial, but two pushes that affect\nthe same ref must be serialized in the resulting push-cert store.\n\nThe original design deliberately punts all the complexity to hook\nexactly because we do not want to have a half-baked \"built-in\"\nimplementation that would only get in the way of those who wants to\ndo high-performance servers.  It is very likely that they want to\nhave a long-running daemon that listens to a port or a named pipe,\nwhere the only thing the hook would do is to write the value of\nGIT_PUSH_CERT to that daemon process, which acts as a serialization\npoint, can read from the object store that is used as a short-term\ntemporary area, and write the push cert to a more permanent store.\n\nHaving said all that, I am sympathetic to a wish to have an\neasy-to-enable-without-thinking example that is not so involved\n(e.g. no portability concern, does not have to perform very well but\nmust be correct).  If Shikher wants to add one, I think the right\napproach to do so would be to add and ship a sample hook.\n\nThanks.\n"},{"id":"327691","messageId":"20170907070827.sysqmszdwduruwis@weakknees.security.iitk.ac.in","threadId":"46714","inReplyTo":"20170906093913.21485-1-root@shikherverma.com","subject":"Re: [RFC PATCH 0/2] Add named reference to latest push cert","fromName":"Shikher Verma","fromEmail":"root@shikherverma.com","sentAt":"2017-09-07T07:08:27Z","receivedAt":"2017-09-07T07:08:39Z","isPatch":true,"sender":{"key":"root@shikherverma.com","avatar":null},"body":"Hey everyone,\n\nI felt like I should introduce myself since this is my first patch on\nthe git mailing list (or any mailing list actually) :D\n\nI am Shikher[1], currently in my 4th year undergrad at IIT Kanpur.\nThis summer I was lucky enough to intern at NYU Secure Systems Lab[2]\nmentored by Santiago. We looked into how signed pushes work and how \nwe can use them to increase the security of git. We encountered a\nstrange error in tests which resulted in a patch[3] and I wrote a\npython script to verify push certificates[4]. I was pretty surprised \nto not find any push certificate on the remote repo after I did a \nsigned push, hence this RFC.\n\nAnyway this is my first time trying to contribute to a large OSS so \nforgive me if I make any noob mistakes.\n\nThanks\nShikher Verma\n\n[1]http://shikherverma.com/\n[2]https://ssl.engineering.nyu.edu/\n[3]https://public-inbox.org/git/20170707220159.12752-1-santiago@nyu.edu/\n[4]https://gist.github.com/ShikherVerma/9204060b545c00597e7ad9b694cfeb9c\n\nOn Wed, Sep 06, 2017 at 03:09:11PM +0530, Shikher Verma wrote:\n> Currently, git only stores push certificates if there is a receive hook \n> present. This may violate the principle of least surprise (e.g., I \n> pushed with --signed, and I don't see anything in upstream). \n> Additionally, push certificates could be more versatile if they are not \n> tightly bound to git hooks. Finally, it would be useful to verify the \n> signed pushes at later points of time with ease.\n> \n> A named ref is added for ease of access/tooling around push \n> certificates. If the last push was signed, ref/PUSH_CERT stores the \n> ref of the latest push cert otherwise it is empty.\n>  \n> Sending patches as RFC since the documentation would have to be \n> updated and git gc might have to be patched to not garbage collect \n> the latest push certificate.\n> \n> This patch applies on master (3ec7d702a) \n> \n> Shikher Verma (2):\n>   Always write push cert to disk\n>   Store latest push cert ref in PUSH_CERT\n> \n>  builtin/receive-pack.c | 25 ++++++++++++++++++++-----\n>  path.c                 |  1 +\n>  path.h                 |  1 +\n>  3 files changed, 22 insertions(+), 5 deletions(-)\n> \n> -- \n> 2.14.1\n> \n\n"},{"id":"327694","messageId":"20170907085511.bpgtm4gqem6eukji@weakknees.security.iitk.ac.in","threadId":"46714","inReplyTo":"xmqq4lsf2upu.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH 0/2] Add named reference to latest push cert","fromName":"Shikher Verma","fromEmail":"root@shikherverma.com","sentAt":"2017-09-07T08:55:11Z","receivedAt":"2017-09-07T08:55:26Z","isPatch":true,"sender":{"key":"root@shikherverma.com","avatar":null},"body":"On Thu, Sep 07, 2017 at 09:55:25AM +0900, Junio C Hamano wrote:\n> Stefan Beller <sbeller@google.com> writes:\n> \n> > On the ref to store the push certs:\n> > (a) Currently the ref points at the blob, I wonder if we'd rather want to\n> >     point at a commit? (Then we can build up a history of\n> >     push certs, instead of relying on the reflog to show all\n> >     push certs. Also the gc issue might be easier to solve using this)\n> > (b) When going with (a), we might want to change the name. Most\n> >     refs are 3 directories deep:\n> >       refs/heads/<branch name>\n> >       refs/pr/<pull request nr> # at github IIUC\n> >       refs/changes/<id> # Gerrit\n> >       refs/meta/config # Gerrit to e.g. configure ACLs of the repo\n> >     \"refs\" indicates it is a ref, whereas the second part can be seen\n> >     as a \"namespace\". Currently Git only uses the \"heads\" and \"tags\"\n> >     namespace, \"meta\" is used by more than just Gerrit, so maybe it is\n> >     not wise to use \"refs/meta/push_cert\", but go with refs/gitmeta/pushcert\n> >     instead?\n> \n> You also need to worry about concurrent pushes.  The resulting\n> \"history\" may not have to be sequencial, but two pushes that affect\n> the same ref must be serialized in the resulting push-cert store.\n\nOh I see. I guess concurrency would be an issue. How does recieve-pack\nhandle concurrent pushes? Is there a lock that I could use to decide \nif named push cert ref has to be updated or not?\n\n> The original design deliberately punts all the complexity to hook\n> exactly because we do not want to have a half-baked \"built-in\"\n> implementation that would only get in the way of those who wants to\n> do high-performance servers.  It is very likely that they want to\n> have a long-running daemon that listens to a port or a named pipe,\n> where the only thing the hook would do is to write the value of\n> GIT_PUSH_CERT to that daemon process, which acts as a serialization\n> point, can read from the object store that is used as a short-term\n> temporary area, and write the push cert to a more permanent store.\n> \n> Having said all that, I am sympathetic to a wish to have an\n> easy-to-enable-without-thinking example that is not so involved\n> (e.g. no portability concern, does not have to perform very well but\n> must be correct).  If Shikher wants to add one, I think the right\n> approach to do so would be to add and ship a sample hook.\n> \n\nThis patch would only add one more object to write per push so I \nthink the performance penalty is not that high. We can have a config\nto turn it off so that it does not get in the way of those who want \nto do high-performance servers.\n\nPeople can use the recieve hook for advance use cases but I think git\nshould provide a builtin solution for the simple case. The reason I\nfavour a named ref over a sample hook is because decouping push\ncertificate from hook opens up more possibilities like we could store\na map of refs to the latest push cert which updated the ref. And \nserve the corresponding push cert whenever someone does \n`git pull --signed important-ref`. Effectively removing trust from \nthe server by preventing tampering with refs. This could really help \nthe Github generation developers like me. What do you think?\n\n> Thanks.\n\n"},{"id":"327696","messageId":"20170907091133.zygswf3f6z2bedfo@weakknees.security.iitk.ac.in","threadId":"46714","inReplyTo":"CAGZ79kbxDh11KxrKCk_VjmN06kzp7x4iVO6XTV=a-qBmm39K5A@mail.gmail.com","subject":"Re: [RFC PATCH 0/2] Add named reference to latest push cert","fromName":"Shikher Verma","fromEmail":"root@shikherverma.com","sentAt":"2017-09-07T09:11:33Z","receivedAt":"2017-09-07T09:11:50Z","isPatch":true,"sender":{"key":"root@shikherverma.com","avatar":null},"body":"On Wed, Sep 06, 2017 at 02:31:49PM -0700, Stefan Beller wrote: \n> On Wed, Sep 6, 2017 at 2:39 AM, Shikher Verma <root@shikherverma.com> wrote: \n> > Currently, git only stores push certificates if there is a receive hook \n> > present. This may violate the principle of least surprise (e.g., I \n> > pushed with --signed, and I don't see anything in upstream). \n> > Additionally, push certificates could be more versatile if they are not \n> > tightly bound to git hooks. Finally, it would be useful to verify the \n> > signed pushes at later points of time with ease. \n> > \n> > A named ref is added for ease of access/tooling around push \n> > certificates. If the last push was signed, ref/PUSH_CERT stores the \n> > ref of the latest push cert otherwise it is empty. \n> > \n> > Sending patches as RFC since the documentation would have to be \n> > updated and git gc might have to be patched to not garbage collect \n> > the latest push certificate. \n> > \n> > This patch applies on master (3ec7d702a) \n> \n> What are performance implications for busy repositories at busy hosts? \n> (think kernel.org / github) They may want to disable this new feature \n> for performance reasons or because they don't want to clutter the \n> object store. So at least a config option to turn it off would be useful. \n\nAny typical git push would write several objects to disk, this patch \nwould only add one more object per push so I think the performance \npenalty is not that high. But I agree that we can have a config to turn \nit off. \n> On the ref to store the push certs: \n> (a) Currently the ref points at the blob, I wonder if we'd rather want to \n> point at a commit? (Then we can build up a history of \n> push certs, instead of relying on the reflog to show all \n> push certs. Also the gc issue might be easier to solve using this) \n\nI am not sure how that would work. The ref points at the blob of push \ncertificate. Since each push can update multiple refs, each push \ncertificate can point to mutiple commits (tip of the updated refs). \nAlso if the named ref points at the commit then how will we get the \ncorresponding push certificate? \n\nI did think about keeping a history of push certificates but the problem \nis new pushes can delete refs and commits which were pointed to by \nprevious push certificates. This makes it really difficult to decide \nwhich push certificates to keep and which to gc. Also this history would \nbe different for different clones of the same repo. Since push \ncertificate are only meta data of the git workflow I think its best to \njust keep the latest push certificate and gc the old ones. People can \nuse the recieve hook if they want to do advance things like logging a \nhistory of push certificates. I think git should provide a builtin \nsolution for the simple case. \n\nAnother motivation to decouple push certificates from hooks was that \nlater we could store a map of refs to the latest push cert which \nupdated the ref. And serve the corresponding push cert whenever someone \ndoes `git pull --signed important-ref`. Effectively removing trust from \nthe server by preventing tampering with refs. This could really help \nthe Github generation developers like me.\n> (b) When going with (a), we might want to change the name. Most \n> refs are 3 directories deep: \n> refs/heads/<branch name> \n> refs/pr/<pull request nr> # at github IIUC \n> refs/changes/<id> # Gerrit \n> refs/meta/config # Gerrit to e.g. configure ACLs of the repo \n> \"refs\" indicates it is a ref, whereas the second part can be seen \n> as a \"namespace\". Currently Git only uses the \"heads\" and \"tags\" \n> namespace, \"meta\" is used by more than just Gerrit, so maybe it is \n> not wise to use \"refs/meta/push_cert\", but go with refs/gitmeta/pushcert \n> instead? \n\n"},{"id":"327721","messageId":"CAGZ79kb3vv0sfqRBjJwOb1vFGeRgztZRdbqinv89NmhQaAb9CA@mail.gmail.com","threadId":"46714","inReplyTo":"20170907070827.sysqmszdwduruwis@weakknees.security.iitk.ac.in","subject":"Re: [RFC PATCH 0/2] Add named reference to latest push cert","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-07T17:21:08Z","receivedAt":"2017-09-07T17:21:18Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Sep 7, 2017 at 12:08 AM, Shikher Verma <root@shikherverma.com> wrote:\n> Hey everyone,\n>\n> I felt like I should introduce myself since this is my first patch on\n> the git mailing list (or any mailing list actually) :D\n\nWelcome to the list. :)\n\n> I am Shikher[1], currently in my 4th year undergrad at IIT Kanpur.\n> This summer I was lucky enough to intern at NYU Secure Systems Lab[2]\n> mentored by Santiago. We looked into how signed pushes work and how\n> we can use them to increase the security of git. We encountered a\n> strange error in tests which resulted in a patch[3] and I wrote a\n> python script to verify push certificates[4]. I was pretty surprised\n> to not find any push certificate on the remote repo after I did a\n> signed push, hence this RFC.\n>\n> Anyway this is my first time trying to contribute to a large OSS so\n> forgive me if I make any noob mistakes.\n\nPatches are very welcome. Everyone was a noob once, but that\nchanges quickly.\n\nThanks,\nStefan\n"},{"id":"327722","messageId":"CAGZ79kZO_u21JeAsNq7p7X88i0wmP6kJcg=JbKp62Bxdm4d4Uw@mail.gmail.com","threadId":"46714","inReplyTo":"20170907091133.zygswf3f6z2bedfo@weakknees.security.iitk.ac.in","subject":"Re: [RFC PATCH 0/2] Add named reference to latest push cert","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-07T17:43:19Z","receivedAt":"2017-09-07T17:43:28Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Sep 7, 2017 at 2:11 AM, Shikher Verma <root@shikherverma.com> wrote:\n> On Wed, Sep 06, 2017 at 02:31:49PM -0700, Stefan Beller wrote:\n>> On Wed, Sep 6, 2017 at 2:39 AM, Shikher Verma <root@shikherverma.com> wrote:\n>> > Currently, git only stores push certificates if there is a receive hook\n>> > present. This may violate the principle of least surprise (e.g., I\n>> > pushed with --signed, and I don't see anything in upstream).\n>> > Additionally, push certificates could be more versatile if they are not\n>> > tightly bound to git hooks. Finally, it would be useful to verify the\n>> > signed pushes at later points of time with ease.\n>> >\n>> > A named ref is added for ease of access/tooling around push\n>> > certificates. If the last push was signed, ref/PUSH_CERT stores the\n>> > ref of the latest push cert otherwise it is empty.\n>> >\n>> > Sending patches as RFC since the documentation would have to be\n>> > updated and git gc might have to be patched to not garbage collect\n>> > the latest push certificate.\n>> >\n>> > This patch applies on master (3ec7d702a)\n>>\n>> What are performance implications for busy repositories at busy hosts?\n>> (think kernel.org / github) They may want to disable this new feature\n>> for performance reasons or because they don't want to clutter the\n>> object store. So at least a config option to turn it off would be useful.\n>\n> Any typical git push would write several objects to disk,\n\n(or just one pack file, [which may be renamed eventually, see 722ff7f8])\n\n> this patch\n> would only add one more object per push so I think the performance\n> penalty is not that high. But I agree that we can have a config to turn\n> it off.\n\nI personally do not run a high performance server, so I do not terribly mind,\nbut thought it would be nice for them to have at least an option ready made\ninstead of a potential performance regression.\n\n>> On the ref to store the push certs:\n>> (a) Currently the ref points at the blob, I wonder if we'd rather want to\n>> point at a commit? (Then we can build up a history of\n>> push certs, instead of relying on the reflog to show all\n>> push certs. Also the gc issue might be easier to solve using this)\n>\n> I am not sure how that would work. The ref points at the blob of push\n> certificate. Since each push can update multiple refs, each push\n> certificate can point to mutiple commits (tip of the updated refs).\n> Also if the named ref points at the commit then how will we get the\n> corresponding push certificate?\n>\n> I did think about keeping a history of push certificates but the problem\n> is new pushes can delete refs and commits which were pointed to by\n> previous push certificates. This makes it really difficult to decide\n> which push certificates to keep and which to gc. Also this history would\n> be different for different clones of the same repo. Since push\n> certificate are only meta data of the git workflow I think its best to\n> just keep the latest push certificate and gc the old ones. People can\n> use the recieve hook if they want to do advance things like logging a\n> history of push certificates. I think git should provide a builtin\n> solution for the simple case.\n\nWhat I had in mind was what would be achieved with a\nhook like this (untested):\n\n    #!/bin/sh\n    if test -z GIT_PUSH_CERT ; then\n        exit 0\n    fi\n\n    # add a new worktree 'tmp', checking out the magic ref:\n    git worktree add tmp refs/PUSH_CERT\n\n    cp $GIT_PUSH_CERT tmp/cert\n    git -C tmp add cert\n    git -C tmp commit -m \"new push cert\"\n    # maybe include GIT_PUSH_CERT_[NONCE_]STATUS\n    # in commit message?\n\n    # clean up, command doesn't exist yet:\n    git worktree delete tmp\n\nThis would not deal with concurrency as it re-uses the\nsame worktree, but illustrates what I had in mind\nfor the git history of that special ref.\n"},{"id":"328185","messageId":"20170916072106.yxpwmndzif3lr2dp@weakknees.security.iitk.ac.in","threadId":"46714","inReplyTo":"CAGZ79kZO_u21JeAsNq7p7X88i0wmP6kJcg=JbKp62Bxdm4d4Uw@mail.gmail.com","subject":"Re: [RFC PATCH 0/2] Add named reference to latest push cert","fromName":"Shikher Verma","fromEmail":"root@shikherverma.com","sentAt":"2017-09-16T07:21:06Z","receivedAt":"2017-09-16T07:36:29Z","isPatch":true,"sender":{"key":"root@shikherverma.com","avatar":null},"body":"On Thu, Sep 07, 2017 at 05:43:19PM +0000, Stefan Beller wrote:\n> On Thu, Sep 7, 2017 at 2:11 AM, Shikher Verma <root@shikherverma.com> wrote:\n> > On Wed, Sep 06, 2017 at 02:31:49PM -0700, Stefan Beller wrote:\n> >> On Wed, Sep 6, 2017 at 2:39 AM, Shikher Verma <root@shikherverma.com> wrote:\n> >> > Currently, git only stores push certificates if there is a receive hook\n> >> > present. This may violate the principle of least surprise (e.g., I\n> >> > pushed with --signed, and I don't see anything in upstream).\n> >> > Additionally, push certificates could be more versatile if they are not\n> >> > tightly bound to git hooks. Finally, it would be useful to verify the\n> >> > signed pushes at later points of time with ease.\n> >> >\n> >> > A named ref is added for ease of access/tooling around push\n> >> > certificates. If the last push was signed, ref/PUSH_CERT stores the\n> >> > ref of the latest push cert otherwise it is empty.\n> >> >\n> >> > Sending patches as RFC since the documentation would have to be\n> >> > updated and git gc might have to be patched to not garbage collect\n> >> > the latest push certificate.\n> >> >\n> >> > This patch applies on master (3ec7d702a)\n> >>\n> >> What are performance implications for busy repositories at busy hosts?\n> >> (think kernel.org / github) They may want to disable this new feature\n> >> for performance reasons or because they don't want to clutter the\n> >> object store. So at least a config option to turn it off would be useful.\n> >\n> > Any typical git push would write several objects to disk,\n> \n> (or just one pack file, [which may be renamed eventually, see 722ff7f8])\n> \n> > this patch\n> > would only add one more object per push so I think the performance\n> > penalty is not that high. But I agree that we can have a config to turn\n> > it off.\n> \n> I personally do not run a high performance server, so I do not terribly mind,\n> but thought it would be nice for them to have at least an option ready made\n> instead of a potential performance regression.\n> \n> >> On the ref to store the push certs:\n> >> (a) Currently the ref points at the blob, I wonder if we'd rather want to\n> >> point at a commit? (Then we can build up a history of\n> >> push certs, instead of relying on the reflog to show all\n> >> push certs. Also the gc issue might be easier to solve using this)\n> >\n> > I am not sure how that would work. The ref points at the blob of push\n> > certificate. Since each push can update multiple refs, each push\n> > certificate can point to mutiple commits (tip of the updated refs).\n> > Also if the named ref points at the commit then how will we get the\n> > corresponding push certificate?\n> >\n> > I did think about keeping a history of push certificates but the problem\n> > is new pushes can delete refs and commits which were pointed to by\n> > previous push certificates. This makes it really difficult to decide\n> > which push certificates to keep and which to gc. Also this history would\n> > be different for different clones of the same repo. Since push\n> > certificate are only meta data of the git workflow I think its best to\n> > just keep the latest push certificate and gc the old ones. People can\n> > use the recieve hook if they want to do advance things like logging a\n> > history of push certificates. I think git should provide a builtin\n> > solution for the simple case.\n> \n> What I had in mind was what would be achieved with a\n> hook like this (untested):\n> \n>     #!/bin/sh\n>     if test -z GIT_PUSH_CERT ; then\n>         exit 0\n>     fi\n> \n>     # add a new worktree 'tmp', checking out the magic ref:\n>     git worktree add tmp refs/PUSH_CERT\n> \n>     cp $GIT_PUSH_CERT tmp/cert\n>     git -C tmp add cert\n>     git -C tmp commit -m \"new push cert\"\n>     # maybe include GIT_PUSH_CERT_[NONCE_]STATUS\n>     # in commit message?\n> \n>     # clean up, command doesn't exist yet:\n>     git worktree delete tmp\n>\n\nThis might be a good starting point for a sample hook if we choose to go\nthat way. As Junio suggested.\n> This would not deal with concurrency as it re-uses the\n> same worktree, but illustrates what I had in mind\n> for the git history of that special ref.\n\nI personally feel that we should decouple push certificates from hooks\nand serve the push certificate whenever someone does\n`git pull --signed important-ref`. That way we remove trust from\nservices like Github, Gitlab, Bitbucket. This could be done if git\nstores a map of refs to last push certificate that updated that ref.\n\nPush certificates are preventing MiTM attack between pusher and server.\nIf we start serving push certificates on pull, it would prevent MiTM\nattack between pusher and puller! Compromise of server wouldn't mean\nvulnerabilities in your project.\n\nA sample hook solves the immediate problem of making push certs more\naccessible. But going with `git pull --signed` make push certs\ntremendously more accessible and useful. What are your thoughts on\nhaving git signed pull?\n\nWhat do you guys think I should do in regards to this patch series?\n"},{"id":"328240","messageId":"xmqqa81uumq6.fsf@gitster.mtv.corp.google.com","threadId":"46714","inReplyTo":"20170916072106.yxpwmndzif3lr2dp@weakknees.security.iitk.ac.in","subject":"Re: [RFC PATCH 0/2] Add named reference to latest push cert","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-17T01:40:01Z","receivedAt":"2017-09-17T01:40:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shikher Verma <root@shikherverma.com> writes:\n\n> This might be a good starting point for a sample hook if we choose to go\n> that way. As Junio suggested.\n>> This would not deal with concurrency as it re-uses the\n>> same worktree, but illustrates what I had in mind\n>> for the git history of that special ref.\n\nThat's Stefan; I wouldn't have suggested any approach that uses the\nblob whose sole purpose is to serve as a temporary storage area to\npass the information to the hook as part of the permanent record.\n\n\n"},{"id":"328294","messageId":"20170918142205.r5kwkq7ouy5zxisz@LykOS.localdomain","threadId":"46714","inReplyTo":"xmqqa81uumq6.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH 0/2] Add named reference to latest push cert","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2017-09-18T14:22:06Z","receivedAt":"2017-09-18T14:22:11Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"Hello, everyone.\n\nSorry for being late in this thread, I was making sure I didn't say\nanything outrageously wrong. \n\n> That's Stefan; I wouldn't have suggested any approach that uses the\n> blob whose sole purpose is to serve as a temporary storage area to\n> pass the information to the hook as part of the permanent record.\n> \n\nHmm, as far as I understand *this* is the status quo. We get an\nephemeral sha1/oid only if you have a hook attached. Otherwise, you will\nnever see the object at all, even if you have --signed set.\n\nI suggested preparing this RFC/Patch because of the following reasons\n(please let me know if my understanding of any of this is wrong...):\n\n    - I find it weird that the cli allows for a --signed push and\n      nowhere in the receive-pack's feedback you're told if the push\n      certificate is compute/stored/handled at all. I think that, at the\n      latest, receive pack should let the user know whether the signed\n      push succeeded, or if there is no hook attached to handle it.\n\n    - *if there is a hook* the blob is computed, but it is up to the\n      hook itself to store it *somewhere*. This makes me feel like it's\n      somewhat of a useless waste of computation if the hook is not\n      meant to handle it anyway(which is just a post-receive hook). I\n      find it rather weird that --signed is a builtin flag, and is\n      handled on the server side only partially (just my two cents).\n\n    - To me, the way push certificates are handled now feels like having\n      git commit -S producing a detached signature that you have to\n      embed somehow in the resulting commit object. (As a matter of\n      fact, many points on [1] seem to back this notion, and even recall\n      some drawbacks on push certificates the way they are handled\n      today)\n\nI understand the concurrency concerns, so I agree with stefan's\nsolution, although I don't know how big of a deal it would be, if git\nalready supports --atomic pushes (admittedly, I haven't checked if there\nare any guards in place for someone who pushes millions of refs\natomically). It'd be completely understandable to have a setting to\ndisable hadnling of --signed pushes and, ideally, a way to warn the user\nof this feature being disabled if --signed is sent.\n\nMaybe I'm missing the bigger picture, but to me it feels that a named\nref to the latest push certificate would make it easier to\nhandle/tool/sync around the push certificate solution?\n\nThanks,\n-Santiago.\n\n[1] https://public-inbox.org/git/CAJo=hJvWbjEM9E5AjPHgmQ=eY8xf=Q=xtukeu2Ur7auUqeabDg@mail.gmail.com/\n"},{"id":"328314","messageId":"CAGZ79kZnBoycG-LSftdivfsT8yE53R2JhNRDfVztwqmETeDFKA@mail.gmail.com","threadId":"46714","inReplyTo":"20170918142205.r5kwkq7ouy5zxisz@LykOS.localdomain","subject":"Re: [RFC PATCH 0/2] Add named reference to latest push cert","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-18T17:43:37Z","receivedAt":"2017-09-18T17:43:58Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Sep 18, 2017 at 7:22 AM, Santiago Torres <santiago@nyu.edu> wrote:\n> Hello, everyone.\n>\n> Sorry for being late in this thread, I was making sure I didn't say\n> anything outrageously wrong.\n>\n>> That's Stefan; I wouldn't have suggested any approach that uses the\n>> blob whose sole purpose is to serve as a temporary storage area to\n>> pass the information to the hook as part of the permanent record.\n>>\n\nI put out a vague design that seemed to have more advantages\nin my mind at the time of writing. :)\n\n>\n> Hmm, as far as I understand *this* is the status quo. We get an\n> ephemeral sha1/oid only if you have a hook attached. Otherwise, you will\n> never see the object at all, even if you have --signed set.\n>\n> I suggested preparing this RFC/Patch because of the following reasons\n> (please let me know if my understanding of any of this is wrong...):\n>\n>     - I find it weird that the cli allows for a --signed push and\n>       nowhere in the receive-pack's feedback you're told if the push\n>       certificate is compute/stored/handled at all. I think that, at the\n>       latest, receive pack should let the user know whether the signed\n>       push succeeded, or if there is no hook attached to handle it.\n\nHow would a user benefit from it?\n(Are there cases where the organisation wants the user to not know\ndeliberately what happened to their push cert? Do we care about these\ncases?)\n\n>     - *if there is a hook* the blob is computed, but it is up to the\n>       hook itself to store it *somewhere*. This makes me feel like it's\n>       somewhat of a useless waste of computation if the hook is not\n>       meant to handle it anyway(which is just a post-receive hook). I\n>       find it rather weird that --signed is a builtin flag, and is\n>       handled on the server side only partially (just my two cents).\n\nI agree, but many features in Git start out small and only partially.\n\n>     - To me, the way push certificates are handled now feels like having\n>       git commit -S producing a detached signature that you have to\n>       embed somehow in the resulting commit object. (As a matter of\n>       fact, many points on [1] seem to back this notion, and even recall\n>       some drawbacks on push certificates the way they are handled\n>       today)\n>\n> I understand the concurrency concerns, so I agree with stefan's\n> solution, although I don't know how big of a deal it would be, if git\n> already supports --atomic pushes (admittedly, I haven't checked if there\n> are any guards in place for someone who pushes millions of refs\n> atomically). It'd be completely understandable to have a setting to\n> disable hadnling of --signed pushes and, ideally, a way to warn the user\n> of this feature being disabled if --signed is sent.\n\nThat makes sense.\n"},{"id":"328328","messageId":"xmqqk20vsdmm.fsf@gitster.mtv.corp.google.com","threadId":"46714","inReplyTo":"20170918142205.r5kwkq7ouy5zxisz@LykOS.localdomain","subject":"Re: [RFC PATCH 0/2] Add named reference to latest push cert","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-19T01:04:01Z","receivedAt":"2017-09-19T01:04:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Santiago Torres <santiago@nyu.edu> writes:\n\n>     - *if there is a hook* the blob is computed, but it is up to the\n>       hook itself to store it *somewhere*. This makes me feel like it's\n>       somewhat of a useless waste of computation if the hook is not\n>       meant to handle it anyway(which is just a post-receive hook). I\n>       find it rather weird that --signed is a builtin flag, and is\n>       handled on the server side only partially (just my two cents).\n\nThe way it was envisioned to be used is that the repository meant to\nbe protected by collected push certs may not be trusted as the\npermanent store for push certs by all hosting sites.  The hook may\nbe told the name of a blob to read its contents and is expected to\nstore it away to somewhere else.\n\nThe only reason why we use blob is because creating a blob in\nrespose to pushes being executed in parallel will result in\ndifferent blobs unless there is hash collision.  Instead of us\nhaving to come up with and use a different mechanism to create a\nunique temporary filename and feed that to hook, reusing blob as\nsuch was the simplest.\n\n> I understand the concurrency concerns, so I agree with stefan's\n> solution, although I don't know how big of a deal it would be, if git\n> already supports --atomic pushes (admittedly, I haven't checked if there\n> are any guards in place for someone who pushes millions of refs\n> atomically). It'd be completely understandable to have a setting to\n> disable hadnling of --signed pushes and, ideally, a way to warn the user\n> of this feature being disabled if --signed is sent.\n\nI do not think atomic helps at all, when one atomic push updates\nbranch A while another atomic push updates branch B.  They can still\ngo in parallel, and their certificates must both be stored.  You can\nsomehow serialize them and create a single strand of pearls to\nadvance a single ref, or you can let both to fork two histories to\nstore the push certs from these two pushes and have somebody create\na merge commit to join the history.  \n\nBut the point is that we do not want such an overhead in core, as\nall of that is a useless waste of the cycle for a site that wants to\nstore the push certificate away outside of the repository itself.\n"},{"id":"328338","messageId":"xmqqtvzzqt5u.fsf@gitster.mtv.corp.google.com","threadId":"46714","inReplyTo":"xmqqk20vsdmm.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH 0/2] Add named reference to latest push cert","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-19T03:11:25Z","receivedAt":"2017-09-19T03:11:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> But the point is that we do not want such an overhead in core, as\n> all of that is a useless waste of the cycle for a site that wants to\n> store the push certificate away outside of the repository itself.\n\nBy this I do not mean that cert blobs shouldn't become part of the\nin-repository record in _all_ installations that receive signed push\ncertificates.  An easy to reuse example shipped together with our\nsource would be a good thing, and an easy to enable sample hook may\neven be better.  It's just that I do not want to have any \"solution\"\nin the core part that everybody that wants to accept push certs must\nrun, if the \"solution\" is not something we can endorse as the best\ncurrent practice.  And I do not yet see how anything along the lines\nof the patch that started this thread, or its extention by wrapping\nthem with commit chain, would become that \"best current practice\".\n\nA sample hook, on the other hand, is simpler for people to experiment\nand we might even come up with an unversally useful best current\nprctice out of such an experiment, though.\n"},{"id":"333950","messageId":"20171202091248.6037-1-root@shikherverma.com","threadId":"46714","inReplyTo":"xmqqtvzzqt5u.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] Add a sample hook which saves push certs as notes","fromName":"Shikher Verma","fromEmail":"root@shikherverma.com","sentAt":"2017-12-02T09:12:48Z","receivedAt":"2017-12-02T09:13:25Z","isPatch":true,"sender":{"key":"root@shikherverma.com","avatar":null},"body":"hooks--post-receive.sample: If push cert is present, add it as a git\nnote to the top most commit of the updated ref.\n\nSigned-off-by: Shikher Verma <root@shikherverma.com>\n---\n templates/hooks--post-receive.sample | 38 ++++++++++++++++++++++++++++++++++++\n 1 file changed, 38 insertions(+)\n create mode 100755 templates/hooks--post-receive.sample\n\ndiff --git a/templates/hooks--post-receive.sample b/templates/hooks--post-receive.sample\nnew file mode 100755\nindex 000000000..b4366e43f\n--- /dev/null\n+++ b/templates/hooks--post-receive.sample\n@@ -0,0 +1,38 @@\n+#!/bin/sh\n+#\n+# An example hook script to store push certificates as notes.\n+#\n+# To enable this hook, rename this file to \"post-receive\".\n+#\n+# The stdin of the hook will be one line for each updated ref:\n+# <old-id> <new-id> <refname>\n+#\n+# For each updated ref this script will :\n+# 1. Verify that the ref update matches that in push certificate.\n+# 2. add the push cert as note (namespace pushcerts) to <new-id>.\n+#\n+# If this hook is enabled on the server then clients can prevent\n+# git metadata tampering, by using signed pushes and \n+# doing the following while fetching :\n+# 1. fetch the git notes (of namespace pushcerts) from server.\n+#     $ git fetch origin refs/notes/pushcerts:refs/notes/pushcerts\n+# 2. Check that the fetched ref's top most commit has a note\n+#     containing a push certificate.\n+# 3. Verify the validity of the push certificate in the note and \n+#     check that the ref update matches that in push certificate.\n+#\n+\n+if test -z GIT_PUSH_CERT ; then\n+    exit 0\n+fi\n+\n+push_cert=$(git cat-file -p  $GIT_PUSH_CERT)\n+\n+while read oval nval ref\n+do\n+\t# Verify that the ref update matches that in push certificate.\n+\tif [[ $push_cert == *$oval\" \"$nval\" \"$ref* ]]; then\n+\t\t# add the push cert as note (namespaced pushcerts) to nval.\n+\t\tgit notes --ref=pushcerts add -m \"$push_cert\" $nval -f\n+\tfi\n+done\n-- \n2.15.0\n\n\n"},{"id":"333975","messageId":"20171203004543.GO3693@zaya.teonanacatl.net","threadId":"46714","inReplyTo":"20171202091248.6037-1-root@shikherverma.com","subject":"Re: [PATCH] Add a sample hook which saves push certs as notes","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2017-12-03T00:45:43Z","receivedAt":"2017-12-03T00:45:53Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Hi Shikher,\n\nI'm not familiar with push certs, but I did notice some general issues\nin the sample hook.  I hope they're helpful.\n\nShikher Verma wrote:\n> index 000000000..b4366e43f\n> --- /dev/null\n> +++ b/templates/hooks--post-receive.sample\n> +#!/bin/sh\n...\n> +if test -z GIT_PUSH_CERT ; then\n> +    exit 0\n> +fi\n\nThe $ is missing from GIT_PUSH_CERT.  test -z GIT_PUSH_CERT will\nalways be false. :)\n\nThe variable should also be quoted.  Not all sh implementations accept\na missing argument to test -z, as bash does.\n\nMore minor, Documentation/CodingGuidelines suggests placing 'then' on\na new line:\n\n    if test -z \"$GIT_PUSH_CERT\"\n    then\n        exit 0\n    fi\n\n(There is plenty of code that doesn't follow that, so I don't know how\nstrong that preference is.)\n\nThis could also be written as:\n\n    test -z \"$GIT_PUSH_CERT\" && exit 0\n\nI don't know if there's any general preference to shorten it in git's\ncode or not.\n\n> +push_cert=$(git cat-file -p  $GIT_PUSH_CERT)\n\nVery minor: there's an extra space before the variable here.\n\n(I also noticed the tests which use $GIT_PUSH_CERT, like t5534, use\n'cat-file blob ...' rather than 'cat-file -p ...'.  I don't know if\nthat's much safer/better than letting cat-file guess the object type\nin the hook.  I have no idea if there's a chance that \"$GIT_PUSH_CERT\"\nhas some unexpected, non-blob object type.)\n\n> +while read oval nval ref\n> +do\n> +\t# Verify that the ref update matches that in push certificate.\n> +\tif [[ $push_cert == *$oval\" \"$nval\" \"$ref* ]]; then\n\n[[ isn't portable across all the sh implementations git strives to\nsupport, as far as I know.\n\nThe minor point about 'then' on new line is applicable here too.  It\nwould also better match the outer 'while' loop.\n\n> +\t\t# add the push cert as note (namespaced pushcerts) to nval.\n> +\t\tgit notes --ref=pushcerts add -m \"$push_cert\" $nval -f\n> +\tfi\n> +done\n\n-- \nTodd\n~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\nLearn from the mistakes of others--you can never live long enough to\nmake them all yourself.\n    -- John Luther\n\n"},{"id":"333989","messageId":"xmqq609ojqhf.fsf@gitster.mtv.corp.google.com","threadId":"46714","inReplyTo":"20171203004543.GO3693@zaya.teonanacatl.net","subject":"Re: [PATCH] Add a sample hook which saves push certs as notes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-03T06:05:32Z","receivedAt":"2017-12-03T06:05:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Todd Zullinger <tmz@pobox.com> writes:\n\n> (I also noticed the tests which use $GIT_PUSH_CERT, like t5534, use\n> 'cat-file blob ...' rather than 'cat-file -p ...'.  I don't know if\n> that's much safer/better than letting cat-file guess the object type\n> in the hook.\n\nThe '-p' option is meant for human consumption and we promise that\nthe output from it _will_ change if it makes sense at the UI level.\n\nIn a script like this, you do care about the exact byte sequence.\nSo that is a more important reason why you should say \"blob\" not\n\"-p\".\n\n>> +\t# Verify that the ref update matches that in push certificate.\n>> +\tif [[ $push_cert == *$oval\" \"$nval\" \"$ref* ]]; then\n\nI am not sure what this expression is trying to do in the first\nplace.  The contents of the push certificate blob may contain these\nthree values, but has a lot more than that.\n\nA post-receive is run after all the receive processing is done, so\nits failing cannot abort the transfer.  I wonder how an almost\nsimultaneous push to a same ref, that would not fail normally\nwithout this new hook script, would behave.  One receive updates the\ntip from A to B and then starts running this script, while the other\nreceive updates the tip from B to C and then starts running another\ncopy of the script.  They both wants to update the notes database\nbut there can be only one winner in the race for its tip.  \n\nWhat happens then?  Don't we need to be running a script like this\nfrom a hook mechanism that runs under a lock or something?\n"}]}