{"thread":{"id":"46671","subject":"git signed push server-side","startedAt":"2017-08-25T21:48:50Z","lastAt":"2017-08-26T09:12:10Z","messageCount":6,"participants":["Ian Jackson","Junio C Hamano","Jonathan Nieder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"327224","messageId":"22944.38288.91698.811743@chiark.greenend.org.uk","threadId":"46671","inReplyTo":null,"subject":"git signed push server-side","fromName":"Ian Jackson","fromEmail":"ijackson@chiark.greenend.org.uk","sentAt":"2017-08-25T21:24:32Z","receivedAt":"2017-08-25T21:48:50Z","isPatch":false,"sender":{"key":"ijackson@chiark.greenend.org.uk","avatar":null},"body":"I have been investigating git signed pushes.  I found a number of\ninfelicities in the server side implementation which make using this\nin practice rather difficult.  I'm emailing here (before writing\npatches) to see what people think of my proposed changes.\n\n\n1. PUSH_CERT_KEY has truncated keyid (Debian #852647)\n\nI see this:\n  GIT_PUSH_CERT_KEY=A3DBCBC039B13D8A\n\nThere is almost no purpose for which this 64-bit keyid can be safely\nused.  The full key fingerprint should be provided instead.\n\nProposed change: provide the full fingerprint instead.  Do this\nfor every caller of gpg-interface.c.\n\n\n2. git-receive-pack calls gpg (Debian #852684)\n\nIt would be better if it called gpgv.  gpg does all sorts of\ncomplicated things, including automatically starting or connecting to\na gpg-agent, which are not appropriate for use in a daemon on a\nserver.\n\nAdditionally, I find that passing -c gpg.program=/usr/bin/gpgv\nto git receive-pack is not effective, and there seems to be no\nsensible way to specify the keyrings to use (although that could be\ndone by setting GNUPGHOME perhaps).\n\nProposed change: call gpgv instead (and make any needed changes to\nadapt to gpgv).  Do this only when we are in git-receive-pack; other\ncall sites of gpg-interface.c will continue to use gpg.\n\n\n3. No way to specify keyring (Debian #852684, side note)\n\nThere should be a way to specify the keyring used by\ngit-receive-pack's gpgv invocation.  This should probably be done with\na config option, receive.certKeyring perhaps.\n\n\n4. Trouble with the nonce (Debian #852688 part 2)\n\nTo use the signed push feature it is necessary to provide a nonce seed\nto git-receive-pack.\n\nThe docs say the seed must be secret but there is no documented way to\npass this seed to git that does not either write it to a git\nconfiguration file somewhere, or pass it on a command line.  The git\nconfiguration system is unsuited to keeping secrets.  Command lines\ncan be seen in ps etc.\n\nAdditionally, the seed should be changed occasionally (since\nunchangeable secrets are a bad idea).  Ideally this should be done\nautomatically.\n\nAnalysis: the seed is used to allow the server to mostly-statelessly\nverify the freshness of a client's nonce.  (This is necessary only in\nconnectionless transport protocols, \"stateless_rpc\" as\nbuiltin/receive-pack.c has it.)\n\nProposed fix (in two parts):\n\n(i) Provide a new config option receive.certNonceSeedsFile.  It\ncontains seeds, one per line.  When stateless_rpc, we send a nonce\ncomputed from the first seed.  We accept nonces computed from any of\nthe listed seeds.  The documentation will say that the file should\nnormally contain two seeds; rollover is achieved by mving into place a\nnew seed at the top, and dropping one from the bottom.  An example\nscript will be provided.\n\n(ii) At some later point, the following enhancement: When\n!stateless_rpc, certNonceSeedsFile is ignored except that if neither\nit nor the old certNonceSeed is set, the signed push feature is\ndisabled.  In this state we always get a fresh nonce (from a suitable\nsystem random source).  Nontrivial because current git doesn't seem to\nhave a \"get suitable random number\" function, and the mess that is the\nsemantics of /dev/*random* files means that providing one is going to\nbe controversial.\n\n\n5. There are no docs on how to use this feature properly\n   (Debian #852695, #852688 part 1)\n\nUsing the signed push feature requires careful programming on the\nserver side.  There should be a doc explaining how to do this.\n\nProposed fix: provide a .txt file containing much the same contents as\nseen here:\n  https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=852695\n\n\nIf I send patches like the above, would they be welcomed (subject to\ndetailed review, of course) ?\n\nThanks,\nIan.\n\n-- \nIan Jackson <ijackson@chiark.greenend.org.uk>   These opinions are my own.\n\nIf I emailed you from an address @fyvzl.net or @evade.org.uk, that is\na private address which bypasses my fierce spamfilter.\n"},{"id":"327225","messageId":"xmqqzianqow2.fsf@gitster.mtv.corp.google.com","threadId":"46671","inReplyTo":"22944.38288.91698.811743@chiark.greenend.org.uk","subject":"Re: git signed push server-side","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-25T22:11:09Z","receivedAt":"2017-08-25T22:11:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ian Jackson <ijackson@chiark.greenend.org.uk> writes:\n\n> I have been investigating git signed pushes.  I found a number of\n> infelicities in the server side implementation which make using this\n> in practice rather difficult.  I'm emailing here (before writing\n> patches) to see what people think of my proposed changes.\n> ...\n> If I send patches like the above, would they be welcomed (subject to\n> detailed review, of course) ?\n\nWhen I did the signed push, the primary focus was to get the\nprotocol extension right, and what the server end does with the\nreceived certificate was left as something that can be refined later\nby those who are really into it.  \n\nSo I do find it a welcome development that you are looking into it\n(but remember, I do not represent \"people\"---I am merely one of them).\n\nI and others may or may not have objections and may or may not offer\nbetter ideas over what you are going to write in your design docs\nand patches---I cannot promise anything before seeing them.\n\n\n\n"},{"id":"327226","messageId":"20170826003229.GL13924@aiede.mtv.corp.google.com","threadId":"46671","inReplyTo":"22944.38288.91698.811743@chiark.greenend.org.uk","subject":"Re: git signed push server-side","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-08-26T00:32:29Z","receivedAt":"2017-08-26T00:32:38Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"+Dave Borowitz, who implemented push cert handling in JGit and Gerrit\nHi Ian,\n\nIan Jackson wrote[1]:\n\n> I have been investigating git signed pushes.  I found a number of\n> infelicities in the server side implementation which make using this\n> in practice rather difficult.  I'm emailing here (before writing\n> patches) to see what people think of my proposed changes.\n>\n> 1. PUSH_CERT_KEY has truncated keyid (Debian #852647)\n>\n> I see this:\n>   GIT_PUSH_CERT_KEY=A3DBCBC039B13D8A\n>\n> There is almost no purpose for which this 64-bit keyid can be safely\n> used.  The full key fingerprint should be provided instead.\n>\n> Proposed change: provide the full fingerprint instead.  Do this\n> for every caller of gpg-interface.c.\n\nSounds sane.\n\n> 2. git-receive-pack calls gpg (Debian #852684)\n>\n> It would be better if it called gpgv.  gpg does all sorts of\n> complicated things, including automatically starting or connecting to\n> a gpg-agent, which are not appropriate for use in a daemon on a\n> server.\n>\n> Additionally, I find that passing -c gpg.program=/usr/bin/gpgv\n> to git receive-pack is not effective, and there seems to be no\n> sensible way to specify the keyrings to use (although that could be\n> done by setting GNUPGHOME perhaps).\n>\n> Proposed change: call gpgv instead (and make any needed changes to\n> adapt to gpgv).  Do this only when we are in git-receive-pack; other\n> call sites of gpg-interface.c will continue to use gpg.\n\nI think respecting gpg.program would be nicer.  Is there a reason not\nto do that?\n\nI suspect receive-pack just forgot to call git_gpg_config.\n\n> 3. No way to specify keyring (Debian #852684, side note)\n>\n> There should be a way to specify the keyring used by\n> git-receive-pack's gpgv invocation.  This should probably be done with\n> a config option, receive.certKeyring perhaps.\n\nHow is the keyring configured for other commands that use GPG, like\n\"git tag -v\"?  (Forgive my laziness in not looking it up.)\n\n> 4. Trouble with the nonce (Debian #852688 part 2)\n>\n> To use the signed push feature it is necessary to provide a nonce seed\n> to git-receive-pack.\n>\n> The docs say the seed must be secret but there is no documented way to\n> pass this seed to git that does not either write it to a git\n> configuration file somewhere, or pass it on a command line.  The git\n> configuration system is unsuited to keeping secrets.  Command lines\n> can be seen in ps etc.\n[...]\n> Proposed fix (in two parts):\n>\n> (i) Provide a new config option receive.certNonceSeedsFile.  It\n> contains seeds, one per line.  When stateless_rpc, we send a nonce\n> computed from the first seed.  We accept nonces computed from any of\n> the listed seeds.  The documentation will say that the file should\n> normally contain two seeds; rollover is achieved by mving into place a\n> new seed at the top, and dropping one from the bottom.  An example\n> script will be provided.\n\nI like it.\n\nI also wonder why you say the git configuration system is unsuited to\nkeeping secrets.  E.g. passing an include.path setting with -c or\nGIT_CONFIG_PARAMETERS should avoid the kinds of trouble you described.\nIs there a change we could make to make it work better?  That said, I\nthink being able to name a file is a good idea.\n\n> (ii) At some later point, the following enhancement: When\n> !stateless_rpc, certNonceSeedsFile is ignored except that if neither\n> it nor the old certNonceSeed is set, the signed push feature is\n> disabled.\n\nThat seems like an awkward interface.  Shouldn't there be at least\nanother config variable to enable signed push without making up a seed\nor filename?\n\n>            In this state we always get a fresh nonce (from a suitable\n> system random source).\n\nHow does this work with stateless_rpc?  (See \"Session State\" in\nDocumentation/technical/http-protocol.txt.)\n\n>                         Nontrivial because current git doesn't seem to\n> have a \"get suitable random number\" function, and the mess that is the\n> semantics of /dev/*random* files means that providing one is going to\n> be controversial.\n\nI think you're overestimating how much pushback adding such a thing\nwould get.\n\n> 5. There are no docs on how to use this feature properly\n>    (Debian #852695, #852688 part 1)\n>\n> Using the signed push feature requires careful programming on the\n> server side.  There should be a doc explaining how to do this.\n>\n> Proposed fix: provide a .txt file containing much the same contents as\n> seen here:\n>   https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=852695\n\nYes, that sounds like a very welcome kind of thing to add.\n\nMore references:\n\n- JGit's push cert handling:\n  https://git.eclipse.org/r/#/q/message:cert\n\n- Gerrit's push cert handling:\n  https://gerrit-review.googlesource.com/q/project:gerrit+message:gpg\n\nI haven't been able to find much in terms of docs for the feature.\nThere is https://gerrit-review.googlesource.com/Documentation/config-gerrit.html#receive.trustedKey\nand https://gerrit-review.googlesource.com/Documentation/config-project-config.html#receive.enableSignedPush.\nIf signed push is enabled but not required for a repository then if I\nremember correctly it is able to show whether an upload was signed by\na trusted key, as context during a review.\n\nThanks and hope that helps,\nJonathan\n\n[1] https://public-inbox.org/git/22944.38288.91698.811743@chiark.greenend.org.uk/\n"},{"id":"327228","messageId":"xmqqshgfqh0g.fsf@gitster.mtv.corp.google.com","threadId":"46671","inReplyTo":"20170826003229.GL13924@aiede.mtv.corp.google.com","subject":"Re: git signed push server-side","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-26T01:01:19Z","receivedAt":"2017-08-26T01:01:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> +Dave Borowitz, who implemented push cert handling in JGit and Gerrit\n> Hi Ian,\n>\n> Ian Jackson wrote[1]:\n>\n>> I have been investigating git signed pushes.  I found a number of\n>> infelicities in the server side implementation which make using this\n>> in practice rather difficult.  I'm emailing here (before writing\n>> patches) to see what people think of my proposed changes.\n>>\n>> 1. PUSH_CERT_KEY has truncated keyid (Debian #852647)\n>>\n>> I see this:\n>>   GIT_PUSH_CERT_KEY=A3DBCBC039B13D8A\n>>\n>> There is almost no purpose for which this 64-bit keyid can be safely\n>> used.  The full key fingerprint should be provided instead.\n>>\n>> Proposed change: provide the full fingerprint instead.  Do this\n>> for every caller of gpg-interface.c.\n>\n> Sounds sane.\n\nI probably should react a bit stronger against that \"instead\", as\nIan will not be writing the world's first server side hook that uses\nthis interface.  A different variable that lets you read the full\nlength \"in addition\" I wouldn't have a problem with, as existing\nscripts will continue working the same way if you did so.\n\nBut on the other hand, the value of this environment is not meant to\nbe used to make decision by the hook anyway, so it perhaps is OK to\nchange it in a backward incompatible way to break those who have\nbeen using the value for any serious purpose.\n\nThe purpose of the signed push is not to replace authentication and\nauthorization.  The primary goal behind the signed push mechanism is\nto allow server side hook to implement a way to store these certs in\nthe order it receives without losing them, and that gives a way for\nthe server operators to protect against claims that they are showing\nwhat the pusher did not intend to publish.  They can say \"the tip of\nthese branches are at this commit, because, see this, a signed cert\nby the pusher asking us to put these commits at these branches\" with\nsuch a mechanism---as opposed to \"we authenticated the user and here\nis our server log that says that user pushed to update these refs\",\nwhich is much weaker claim that they can make without the signed\npush mechanism.\n"},{"id":"327229","messageId":"xmqqo9r3qgak.fsf@gitster.mtv.corp.google.com","threadId":"46671","inReplyTo":"20170826003229.GL13924@aiede.mtv.corp.google.com","subject":"Re: git signed push server-side","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-26T01:16:51Z","receivedAt":"2017-08-26T01:16:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> I think respecting gpg.program would be nicer.  Is there a reason not\n> to do that?\n>\n> I suspect receive-pack just forgot to call git_gpg_config.\n\nThat would be a good change.\n\n> How is the keyring configured for other commands that use GPG, like\n> \"git tag -v\"?  (Forgive my laziness in not looking it up.)\n\nAFAIR we never do anything special, so you should be able to point\nGNUPGHOME to wherever you like to use the desired configuration.\n\n> I also wonder why you say the git configuration system is unsuited to\n> keeping secrets.  E.g. passing an include.path setting with -c or\n> GIT_CONFIG_PARAMETERS should avoid the kinds of trouble you described.\n> Is there a change we could make to make it work better?  That said, I\n> think being able to name a file is a good idea.\n\nI also wonder that too.  The configuration file that has the\nfilename could be made just as secret and unreadable from public as\nthe new file that stores the seed with the same mechanism, I would\nimagine.\n\n>> 5. There are no docs on how to use this feature properly\n>>    (Debian #852695, #852688 part 1)\n>>\n>> Using the signed push feature requires careful programming on the\n>> server side.  There should be a doc explaining how to do this.\n\nThis was rather deliberately left underspecified, hoping that the\nBCP would emerge after people gain experience.  As Ian is looking\ninto this and hopefully gain real-world experience, we can have a\ngood BCP description after he is done with his project ;-)\n\n> Yes, that sounds like a very welcome kind of thing to add.\n\nIndeed.\n"},{"id":"327243","messageId":"22945.15202.337224.529980@chiark.greenend.org.uk","threadId":"46671","inReplyTo":"20170826003229.GL13924@aiede.mtv.corp.google.com","subject":"Re: git signed push server-side [and 3 more messages]","fromName":"Ian Jackson","fromEmail":"ijackson@chiark.greenend.org.uk","sentAt":"2017-08-26T09:12:02Z","receivedAt":"2017-08-26T09:12:10Z","isPatch":false,"sender":{"key":"ijackson@chiark.greenend.org.uk","avatar":null},"body":"Hi.  Thanks to both of you for your helpful comments.\n\nJonathan Nieder writes (\"Re: git signed push server-side\"):\n> Ian Jackson wrote[1]:\n> > 2. git-receive-pack calls gpg (Debian #852684)\n> >\n> > It would be better if it called gpgv.\n...\n> think respecting gpg.program would be nicer.  Is there a reason not\n> to do that?\n\nI think it very unlikely that anyone would want git-receive-pack's\nsigned push facility to end up running gpg rather than gpgv.  But, the\ngit-receive-pack functions here are building blocks which need quite a\nlot of extra work to use, anyway.  So having all callers have to pass\n-c gpg.program would be quite tolerable, if a bit ugly.\n\n> I suspect receive-pack just forgot to call git_gpg_config.\n\nSo, I will send a patch to fix that.\n\n> > 3. No way to specify keyring (Debian #852684, side note)\n> >\n> > There should be a way to specify the keyring used by\n> > git-receive-pack's gpgv invocation.  This should probably be done with\n> > a config option, receive.certKeyring perhaps.\n> \n> How is the keyring configured for other commands that use GPG, like\n> \"git tag -v\"?  (Forgive my laziness in not looking it up.)\n\nIt's not.  You just get whatever keyring and trust db etc. your gpg\nhas as the default.  As Junio says, being able to set the keyring\nexplicitly is not essential to use the signed push feature; one can\nmake a wrapper for gpgv, or set GNUPGHOME.\n\nIt just seemed to me that the current interface is not very\nconvenient.  If it is controversial to provide a more convenient\ninterface, then I will work with what there is.\n\n> > 4. Trouble with the nonce (Debian #852688 part 2)\n...\n> I also wonder why you say the git configuration system is unsuited to\n> keeping secrets.  E.g. passing an include.path setting with -c or\n> GIT_CONFIG_PARAMETERS should avoid the kinds of trouble you described.\n\nI was not aware of include.path.  That would solve this aspect of the\nproblem (but not the rollover aspect, of course).\nGIT_CONFIG_PARAMETERS is not documented.\n\n> > (ii) At some later point, the following enhancement: When\n> > !stateless_rpc, certNonceSeedsFile is ignored except that if neither\n> > it nor the old certNonceSeed is set, the signed push feature is\n> > disabled.\n> \n> That seems like an awkward interface.  Shouldn't there be at least\n> another config variable to enable signed push without making up a seed\n> or filename?\n\nPerhaps.  I don't have a strong opinion about the config parameter\nnames and semantics.  This seems like a matter of taste and I'm happy\nto just do whatever others want.\n\n> >            In this state we always get a fresh nonce (from a suitable\n> > system random source).\n> \n> How does this work with stateless_rpc?  (See \"Session State\" in\n> Documentation/technical/http-protocol.txt.)\n\nIt doesn't, which is why I propose this only if !stateless_rpc.\n\n> >                         Nontrivial because current git doesn't seem to\n> > have a \"get suitable random number\" function, and the mess that is the\n> > semantics of /dev/*random* files means that providing one is going to\n> > be controversial.\n> \n> I think you're overestimating how much pushback adding such a thing\n> would get.\n\nWell, I'm happy to give it ago.  (NB: I will use /dev/urandom on\nLinux.)\n\n> More references:\n> \n> - JGit's push cert handling:\n>   https://git.eclipse.org/r/#/q/message:cert\n\nReally helpful, thanks.  I will read these.  (Shame that I can't even\nview that information without running their javascript!)\n\nI ended up reading this\n https://github.com/sitaramc/gitolite/blob/cf062b8bb6b21a52f7c5002d33fbc950762c1aa7/contrib/hooks/repo-specific/save-push-signatures\n\n> - Gerrit's push cert handling:\n>   https://gerrit-review.googlesource.com/q/project:gerrit+message:gpg\n\n(This is an empty page for me, even with javascript turned on.)\n\nI'll probably implement the refs/meta/push-certs:REF@{cert} special\nbranch thing from JGit.  Can anyone point me at a public repo which\nhas such push-certs recorded, so I can check the details and make my\nthing do stuff the same way ?\n\nJunio C Hamano writes (\"Re: git signed push server-side\"):\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n> > Ian Jackson wrote[1]:\n> >> Proposed change: provide the full fingerprint instead.  Do this\n> >> for every caller of gpg-interface.c.\n> >\n> > Sounds sane.\n> \n> I probably should react a bit stronger against that \"instead\", as\n> Ian will not be writing the world's first server side hook that uses\n> this interface.\n\nAnyone who is verifying signatures and doing anything with the\n16-character key id other than logging it, has a serious security bug.\n\n> But on the other hand, the value of this environment is not meant to\n> be used to make decision by the hook anyway, so it perhaps is OK to\n> change it in a backward incompatible way to break those who have\n> been using the value for any serious purpose.\n\nPrecisely.\n\n> The purpose of the signed push is not to replace authentication and\n> authorization.  The primary goal behind the signed push mechanism is\n> to allow server side hook to implement a way to store these certs in\n> the order it receives without losing them, and that gives a way for\n> the server operators to protect against claims that they are showing\n> what the pusher did not intend to publish.  They can say \"the tip of\n> these branches are at this commit, because, see this, a signed cert\n> by the pusher asking us to put these commits at these branches\" with\n> such a mechanism---as opposed to \"we authenticated the user and here\n> is our server log that says that user pushed to update these refs\",\n> which is much weaker claim that they can make without the signed\n> push mechanism.\n\nI don't understand why the signed push system could (or should) not be\nused for A&A.  It seems eminently suited to it, in circumstances where\ntraceability is important (and the pushers can be expected to be gpg\nusers).\n\nJunio C Hamano writes (\"Re: git signed push server-side\"):\n> When I did the signed push, the primary focus was to get the\n> protocol extension right, and what the server end does with the\n> received certificate was left as something that can be refined later\n> by those who are really into it.  \n\nRight.  I looked at the protocol in some detail (I have a background\nin crypto protocol design) and it seemed good to me.  (Although, I\nhave not done a formal verification.)\n\n> I and others may or may not have objections and may or may not offer\n> better ideas over what you are going to write in your design docs\n> and patches---I cannot promise anything before seeing them.\n\nOf course.  But it is helpful for me to get some input on what\ndirection to go in, before I write and test code.\n\nFTR, I don't expect to do anything particularly quickly.  And, of\ncourse, I'd welcome any other suggestions/input in the meantime.\n\nThanks,\nIan.\n\n-- \nIan Jackson <ijackson@chiark.greenend.org.uk>   These opinions are my own.\n\nIf I emailed you from an address @fyvzl.net or @evade.org.uk, that is\na private address which bypasses my fierce spamfilter.\n"}]}