{"thread":{"id":"45883","subject":"[PATCH 0/3] Clarify interaction between signed pushes and push options","startedAt":"2017-05-05T23:46:56Z","lastAt":"2017-05-09T23:44:37Z","messageCount":31,"participants":["Jonathan Tan","Brandon Williams","Stefan Beller","Jonathan Nieder","Junio C Hamano","Ævar Arnfjörð Bjarmason","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"318912","messageId":"cover.1494027001.git.jonathantanmy@google.com","threadId":"45883","inReplyTo":null,"subject":"[PATCH 0/3] Clarify interaction between signed pushes and push options","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-05T23:46:41Z","receivedAt":"2017-05-05T23:46:56Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"We noticed this when trying to use Git to make a signed push with push\noptions to a server using JGit (which rejects such pushes because the\nGit client makes requests that are, strictly speaking, incompatible with\nthe documented protocol).\n\nThere have been several commits (see the commits linked in the commit\nmessages of the patches sent in reply to this e-mail) to support push\noptions (that are read by receive hooks) when using \"git push\", but the\ninteraction between push options and signed pushes are not very clear.\nHere are some patches (containing both code and documentation) that\nclarify this interaction.\n\nI would appreciate a review of this - if we could make the protocol\nclear, we could then update JGit to use the updated protocol and be no\nlonger incompatible with existing Git clients.\n\nJonathan Tan (3):\n docs: correct receive.advertisePushOptions default\n receive-pack: verify push options in cert\n protocol docs: explain receive-pack push options\n\n Documentation/config.txt | 5 ++--\n Documentation/git-receive-pack.txt | 10 +++++++\n Documentation/technical/pack-protocol.txt | 32 ++++++++++++++++----\n builtin/receive-pack.c | 49 ++++++++++++++++++++++++++++---\n t/t5534-push-signed.sh | 15 ++++++++++\n 5 files changed, 98 insertions(+), 13 deletions(-)\n\n-- \n2.13.0.rc1.294.g07d810a77f-goog\n\n"},{"id":"318913","messageId":"28c40105e53a4d4d828092bc1697fff8992419d6.1494027001.git.jonathantanmy@google.com","threadId":"45883","inReplyTo":"cover.1494027001.git.jonathantanmy@google.com","subject":"[PATCH 1/3] docs: correct receive.advertisePushOptions default","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-05T23:46:42Z","receivedAt":"2017-05-05T23:47:01Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In commit c714e45 (\"receive-pack: implement advertising and receiving\npush options\", 2016-07-14), receive-pack was taught to (among other\nthings) advertise that it understood push options, depending on\nconfiguration. It was documented that it advertised such ability by\ndefault; however, it actually does not. (In that commit, notice that\nadvertise_push_options defaults to 0, unlike advertise_atomic_push which\ndefaults to 1.)\n\nUpdate the documentation to state that it does not advertise the ability\nby default.\n\nSigned-off-by: Jonathan Tan \n---\n Documentation/config.txt | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 475e874d5..f49a2f3cb 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2620,9 +2620,8 @@ receive.advertiseAtomic::\n \tcapability, set this variable to false.\n \n receive.advertisePushOptions::\n-\tBy default, git-receive-pack will advertise the push options\n-\tcapability to its clients. If you don't want to advertise this\n-\tcapability, set this variable to false.\n+\tWhen set to true, git-receive-pack will advertise the push options\n+\tcapability to its clients.\n \n receive.autogc::\n \tBy default, git-receive-pack will run \"git-gc --auto\" after\n-- \n2.13.0.rc1.294.g07d810a77f-goog\n\n"},{"id":"318914","messageId":"01e098de2da2e5f0b341ab6d967d032d840c365e.1494027001.git.jonathantanmy@google.com","threadId":"45883","inReplyTo":"cover.1494027001.git.jonathantanmy@google.com","subject":"[PATCH 2/3] receive-pack: verify push options in cert","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-05T23:46:43Z","receivedAt":"2017-05-05T23:47:03Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In commit f6a4e61 (\"push: accept push options\", 2016-07-14), send-pack\nwas taught to include push options both within the signed cert (if the\npush is a signed push) and outside the signed cert; however,\nreceive-pack ignores push options within the cert, only handling push\noptions outside the cert.\n\nTeach receive-pack, in the case that push options are provided for a\nsigned push, to verify that the push options both within the cert and\noutside the cert are consistent, and to provide the results of that\nverification to the receive hooks as an environment variable (just like\nit currently does for the nonce verification).\n\nThis sets in stone the requirement that send-pack redundantly send its\npush options in 2 places, but I think that this is better than the\nalternatives. Sending push options only within the cert is\nbackwards-incompatible with existing Git servers (which read push\noptions only from outside the cert), and sending push options only\noutside the cert means that the push options are not signed for.\n\nSigned-off-by: Jonathan Tan \n---\n Documentation/git-receive-pack.txt | 10 ++++++++\n builtin/receive-pack.c | 49 ++++++++++++++++++++++++++++++++++----\n t/t5534-push-signed.sh | 15 ++++++++++++\n 3 files changed, 70 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-receive-pack.txt b/Documentation/git-receive-pack.txt\nindex 86a4b32f0..f50ca0f29 100644\n--- a/Documentation/git-receive-pack.txt\n+++ b/Documentation/git-receive-pack.txt\n@@ -106,6 +106,16 @@ the following environment variables:\n \tAlso read about `receive.certNonceSlop` variable in\n \tlinkgit:git-config[1].\n \n+`GIT_PUSH_CERT_OPTION_STATUS`::\n+`BAD`;;\n+\tFor backwards compatibility reasons, when accepting a signed\n+\tpush, receive-pack requires that push options be written both\n+\tinside and outside the certificate. (\"git push\" does this\n+\tautomatically.) `BAD` is returned if they are inconsistent.\n+`OK`;;\n+\tThe push options inside and outside the certificate are\n+\tconsistent.\n+\n This hook is called before any refname is updated and before any\n fast-forward checks are performed.\n \ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex f96834f42..fe26c2f72 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -81,6 +81,9 @@ static long nonce_stamp_slop;\n static unsigned long nonce_stamp_slop_limit;\n static struct ref_transaction *transaction;\n \n+static const char *PUSH_OPTION_BAD = \"BAD\";\n+static const char *PUSH_OPTION_OK = \"OK\";\n+\n static enum {\n \tKEEPALIVE_NEVER = 0,\n \tKEEPALIVE_AFTER_NUL,\n@@ -473,7 +476,8 @@ static char *prepare_push_cert_nonce(const char *path, unsigned long stamp)\n * after dropping \"_commit\" from its name and possibly moving it out\n * of commit.c\n */\n-static char *find_header(const char *msg, size_t len, const char *key)\n+static char *find_header(const char *msg, size_t len, const char *key,\n+\t\t\t const char **next_line)\n {\n \tint key_len = strlen(key);\n \tconst char *line = msg;\n@@ -486,6 +490,8 @@ static char *find_header(const char *msg, size_t len, const char *key)\n \t\tif (line + key_len < eol &&\n \t\t !memcmp(line, key, key_len) && line[key_len] == ' ') {\n \t\t\tint offset = key_len + 1;\n+\t\t\tif (next_line)\n+\t\t\t\t*next_line = *eol ? eol + 1 : eol;\n \t\t\treturn xmemdupz(line + offset, (eol - line) - offset);\n \t\t}\n \t\tline = *eol ? eol + 1 : NULL;\n@@ -495,7 +501,7 @@ static char *find_header(const char *msg, size_t len, const char *key)\n \n static const char *check_nonce(const char *buf, size_t len)\n {\n-\tchar *nonce = find_header(buf, len, \"nonce\");\n+\tchar *nonce = find_header(buf, len, \"nonce\", NULL);\n \tunsigned long stamp, ostamp;\n \tchar *bohmac, *expect = NULL;\n \tconst char *retval = NONCE_BAD;\n@@ -575,9 +581,40 @@ static const char *check_nonce(const char *buf, size_t len)\n \treturn retval;\n }\n \n-static void prepare_push_cert_sha1(struct child_process *proc)\n+static const char *check_push_option(const char *buf, size_t len,\n+\t\t\t\t const struct string_list *push_options)\n+{\n+\tint options_seen = 0;\n+\tchar *option;\n+\tconst char *next_line;\n+\tconst char *retval = PUSH_OPTION_OK;\n+\n+\twhile ((option = find_header(buf, len, \"push-option\", &next_line))) {\n+\t\tlen -= (next_line - buf);\n+\t\tbuf = next_line;\n+\t\toptions_seen++;\n+\t\tif (options_seen > push_options->nr\n+\t\t || strcmp(option,\n+\t\t\t push_options->items[options_seen - 1].string)) {\n+\t\t\tretval = PUSH_OPTION_BAD;\n+\t\t\tgoto leave;\n+\t\t}\n+\t\tfree(option);\n+\t}\n+\n+\tif (options_seen != push_options->nr)\n+\t\tretval = PUSH_OPTION_BAD;\n+\n+leave:\n+\tfree(option);\n+\treturn retval;\n+}\n+\n+static void prepare_push_cert_sha1(struct child_process *proc,\n+\t\t\t\t const struct string_list *push_options)\n {\n \tstatic int already_done;\n+\tstatic const char *push_option_status;\n \n \tif (!push_cert.len)\n \t\treturn;\n@@ -609,6 +646,8 @@ static void prepare_push_cert_sha1(struct child_process *proc)\n \t\tstrbuf_release(&gpg_output);\n \t\tstrbuf_release(&gpg_status);\n \t\tnonce_status = check_nonce(push_cert.buf, bogs);\n+\t\tpush_option_status = check_push_option(push_cert.buf, bogs,\n+\t\t\t\t\t\t push_options);\n \t}\n \tif (!is_null_sha1(push_cert_sha1)) {\n \t\targv_array_pushf(&proc->env_array, \"GIT_PUSH_CERT=%s\",\n@@ -631,6 +670,8 @@ static void prepare_push_cert_sha1(struct child_process *proc)\n \t\t\t\t\t\t \"GIT_PUSH_CERT_NONCE_SLOP=%ld\",\n \t\t\t\t\t\t nonce_stamp_slop);\n \t\t}\n+\t\targv_array_pushf(&proc->env_array, \"GIT_PUSH_CERT_OPTION_STATUS=%s\",\n+\t\t\t\t push_option_status);\n \t}\n }\n \n@@ -683,7 +724,7 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,\n \t\tproc.err = muxer.in;\n \t}\n \n-\tprepare_push_cert_sha1(&proc);\n+\tprepare_push_cert_sha1(&proc, feed_state->push_options);\n \n \tcode = start_command(&proc);\n \tif (code) {\ndiff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh\nindex ecb8d446a..2607b8797 100755\n--- a/t/t5534-push-signed.sh\n+++ b/t/t5534-push-signed.sh\n@@ -124,6 +124,21 @@ test_expect_success GPG 'signed push sends push certificate' '\n \ttest_cmp expect dst/push-cert-status\n '\n \n+test_expect_success GPG 'signed push reports push option status in receive hook' '\n+\tprepare_dst &&\n+\tmkdir -p dst/.git/hooks &&\n+\tgit -C dst config receive.certnonceseed sekrit &&\n+\tgit -C dst config receive.advertisepushoptions 1 &&\n+\twrite_script dst/.git/hooks/post-receive <<-\\EOF &&\n+\t\t# record the push option status\n+\t\techo \"$GIT_PUSH_CERT_OPTION_STATUS\" > ../push-option-status\n+\tEOF\n+\n+\tgit push --push-option=\"foo\" --push-option=\"bar\" --signed dst noop ff &&\n+\n+\ttest \"$(cat dst/push-option-status)\" = \"OK\"\n+'\n+\n test_expect_success GPG 'fail without key and heed user.signingkey' '\n \tprepare_dst &&\n \tmkdir -p dst/.git/hooks &&\n-- \n2.13.0.rc1.294.g07d810a77f-goog\n\n"},{"id":"318915","messageId":"615007182a5e205a7074317a4c8788aa4eb009ff.1494027001.git.jonathantanmy@google.com","threadId":"45883","inReplyTo":"cover.1494027001.git.jonathantanmy@google.com","subject":"[PATCH 3/3] protocol docs: explain receive-pack push options","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-05T23:46:44Z","receivedAt":"2017-05-05T23:47:05Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Support for push options in the receive-pack protocol (and all Git\ncomponents that speak it) have been added over a few commits, but not\nfully documented (especially its interaction with signed pushes). Update\nthe protocol documentation to include the relevant details.\n\nSigned-off-by: Jonathan Tan \n---\n Documentation/technical/pack-protocol.txt | 32 +++++++++++++++++++++++++------\n 1 file changed, 26 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\nindex 5b0ba3ef2..cf7cb48c3 100644\n--- a/Documentation/technical/pack-protocol.txt\n+++ b/Documentation/technical/pack-protocol.txt\n@@ -473,13 +473,10 @@ that it wants to update, it sends a line listing the obj-id currently on\n the server, the obj-id the client would like to update it to and the name\n of the reference.\n \n-This list is followed by a flush-pkt. Then the push options are transmitted\n-one per packet followed by another flush-pkt. After that the packfile that\n-should contain all the objects that the server will need to complete the new\n-references will be sent.\n+This list is followed by a flush-pkt.\n \n ----\n- update-request = *shallow ( command-list | push-cert ) [packfile]\n+ update-request = *shallow ( command-list | push-cert )\n \n shallow = PKT-LINE(\"shallow\" SP obj-id)\n \n@@ -500,12 +497,35 @@ references will be sent.\n \t\t PKT-LINE(\"pusher\" SP ident LF)\n \t\t PKT-LINE(\"pushee\" SP url LF)\n \t\t PKT-LINE(\"nonce\" SP nonce LF)\n+\t\t *PKT-LINE(\"push-option\" SP push-option LF)\n \t\t PKT-LINE(LF)\n \t\t *PKT-LINE(command LF)\n \t\t *PKT-LINE(gpg-signature-lines LF)\n \t\t PKT-LINE(\"push-cert-end\" LF)\n \n- packfile = \"PACK\" 28*(OCTET)\n+ push-option = 1*CHAR\n+----\n+\n+If the server has advertised the 'push-options' capability and the client has\n+specified 'push-options' as part of the capability list above, the client then\n+sends its push options followed by a flush-pkt.\n+\n+----\n+ push-options = *PKT-LINE(push-option) flush-pkt\n+----\n+\n+For backwards compatibility with older Git servers, if the client sends a push\n+cert and push options, it SHOULD send its push options both embedded within the\n+push cert and after the push cert. (Note that the push options within the cert\n+are prefixed, but the push options after the cert are not.) Both these lists\n+SHOULD be consistent.\n+\n+After that the packfile that\n+should contain all the objects that the server will need to complete the new\n+references will be sent.\n+\n+----\n+ packfile = \"PACK\" 28*(OCTET)\n ----\n \n If the receiving end does not support delete-refs, the sending end MUST\n-- \n2.13.0.rc1.294.g07d810a77f-goog\n\n"},{"id":"318916","messageId":"20170505235017.GC55152@google.com","threadId":"45883","inReplyTo":"28c40105e53a4d4d828092bc1697fff8992419d6.1494027001.git.jonathantanmy@google.com","subject":"Re: [PATCH 1/3] docs: correct receive.advertisePushOptions default","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-05T23:50:17Z","receivedAt":"2017-05-05T23:50:25Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/05, Jonathan Tan wrote:\n> In commit c714e45 (\"receive-pack: implement advertising and receiving\n> push options\", 2016-07-14), receive-pack was taught to (among other\n> things) advertise that it understood push options, depending on\n> configuration. It was documented that it advertised such ability by\n> default; however, it actually does not. (In that commit, notice that\n> advertise_push_options defaults to 0, unlike advertise_atomic_push which\n> defaults to 1.)\n\nThis looks like a good fix to the documentation as advertise_push_options\ndoes indeed default to 0.\n\n> \n> Update the documentation to state that it does not advertise the ability\n> by default.\n> \n> Signed-off-by: Jonathan Tan \n> ---\n> Documentation/config.txt | 5 ++---\n> 1 file changed, 2 insertions(+), 3 deletions(-)\n> \n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 475e874d5..f49a2f3cb 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -2620,9 +2620,8 @@ receive.advertiseAtomic::\n> \tcapability, set this variable to false.\n> \n> receive.advertisePushOptions::\n> -\tBy default, git-receive-pack will advertise the push options\n> -\tcapability to its clients. If you don't want to advertise this\n> -\tcapability, set this variable to false.\n> +\tWhen set to true, git-receive-pack will advertise the push options\n> +\tcapability to its clients.\n> \n> receive.autogc::\n> \tBy default, git-receive-pack will run \"git-gc --auto\" after\n> -- \n> 2.13.0.rc1.294.g07d810a77f-goog\n> \n\n-- \nBrandon Williams\n"},{"id":"318917","messageId":"CAGZ79kaq5x4RdXiopcP+-NvAiwkLE6A1yBjbHVaghDVRvOu-6A@mail.gmail.com","threadId":"45883","inReplyTo":"20170505235017.GC55152@google.com","subject":"Re: [PATCH 1/3] docs: correct receive.advertisePushOptions default","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-05T23:53:04Z","receivedAt":"2017-05-05T23:53:09Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, May 5, 2017 at 4:50 PM, Brandon Williams wrote:\n> On 05/05, Jonathan Tan wrote:\n>> In commit c714e45 (\"receive-pack: implement advertising and receiving\n>> push options\", 2016-07-14), receive-pack was taught to (among other\n>> things) advertise that it understood push options, depending on\n>> configuration. It was documented that it advertised such ability by\n>> default; however, it actually does not. (In that commit, notice that\n>> advertise_push_options defaults to 0, unlike advertise_atomic_push which\n>> defaults to 1.)\n>\n> This looks like a good fix to the documentation as advertise_push_options\n> does indeed default to 0.\n>\n\nIndeed.\n\nThanks,\nStefan\n"},{"id":"318918","messageId":"20170505235857.GJ28740@aiede.svl.corp.google.com","threadId":"45883","inReplyTo":"28c40105e53a4d4d828092bc1697fff8992419d6.1494027001.git.jonathantanmy@google.com","subject":"Re: [PATCH 1/3] docs: correct receive.advertisePushOptions default","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-05-05T23:58:57Z","receivedAt":"2017-05-05T23:59:05Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Tan wrote:\n\n> In commit c714e45 (\"receive-pack: implement advertising and receiving\n> push options\", 2016-07-14), receive-pack was taught to (among other\n> things) advertise that it understood push options, depending on\n> configuration. It was documented that it advertised such ability by\n> default; however, it actually does not. (In that commit, notice that\n> advertise_push_options defaults to 0, unlike advertise_atomic_push which\n> defaults to 1.)\n>\n> Update the documentation to state that it does not advertise the ability\n> by default.\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> Documentation/config.txt | 5 ++---\n> 1 file changed, 2 insertions(+), 3 deletions(-)\n> \n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 475e874d5..f49a2f3cb 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -2620,9 +2620,8 @@ receive.advertiseAtomic::\n> \tcapability, set this variable to false.\n> \n> receive.advertisePushOptions::\n> -\tBy default, git-receive-pack will advertise the push options\n> -\tcapability to its clients. If you don't want to advertise this\n> -\tcapability, set this variable to false.\n> +\tWhen set to true, git-receive-pack will advertise the push options\n> +\tcapability to its clients.\n\nGood catch.\n\nReviewed-by: Jonathan Nieder \n\nPossible improvements:\n\n- Should this also say \"The default is false\"?\n\n- git-receive-pack(1) doesn't say anything about push options, so\n without more context it's hard for a new git admin taking over\n from someone who had set this up to understand what's going on.\n Should this have a pointer to the pre-receive + post-receive\n sections of githooks(5)?\n\n- Speaking of which, should git-receive-pack(1) say something\n about push options (for example to also have a pointer to\n githooks(5))?\n\n- git-push(1) has the same problem: when describing the -o option, it\n doesn't give a pointer to where to find more detail (though it's a\n little more helpful then this one since it includes the word \"hook\").\n\nThanks,\nJonathan\n"},{"id":"318919","messageId":"CAGZ79ka4vG1yd1JtK9bDBjWPUG0UCPMvw2XQUsfX_e_xFCpVLA@mail.gmail.com","threadId":"45883","inReplyTo":"01e098de2da2e5f0b341ab6d967d032d840c365e.1494027001.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/3] receive-pack: verify push options in cert","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-06T00:02:54Z","receivedAt":"2017-05-06T00:03:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, May 5, 2017 at 4:46 PM, Jonathan Tan wrote:\n> In commit f6a4e61 (\"push: accept push options\", 2016-07-14), send-pack\n> was taught to include push options both within the signed cert (if the\n> push is a signed push) and outside the signed cert; however,\n> receive-pack ignores push options within the cert, only handling push\n> options outside the cert.\n>\n> Teach receive-pack, in the case that push options are provided for a\n> signed push, to verify that the push options both within the cert and\n> outside the cert are consistent, and to provide the results of that\n> verification to the receive hooks as an environment variable (just like\n> it currently does for the nonce verification).\n>\n> This sets in stone the requirement that send-pack redundantly send its\n> push options in 2 places, but I think that this is better than the\n> alternatives. Sending push options only within the cert is\n> backwards-incompatible with existing Git servers (which read push\n> options only from outside the cert), and sending push options only\n> outside the cert means that the push options are not signed for.\n\nAs the combination of push certs and push options are broken\n(at least when using JGit as well, which are used in Gerrit\ninstallations), I would not feel too bad about actually going\nthe backwards-incompatible way.\n\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> Documentation/git-receive-pack.txt | 10 ++++++++\n> builtin/receive-pack.c | 49 ++++++++++++++++++++++++++++++++++----\n> t/t5534-push-signed.sh | 15 ++++++++++++\n> 3 files changed, 70 insertions(+), 4 deletions(-)\n>\n> diff --git a/Documentation/git-receive-pack.txt b/Documentation/git-receive-pack.txt\n> index 86a4b32f0..f50ca0f29 100644\n> --- a/Documentation/git-receive-pack.txt\n> +++ b/Documentation/git-receive-pack.txt\n> @@ -106,6 +106,16 @@ the following environment variables:\n> Also read about `receive.certNonceSlop` variable in\n> linkgit:git-config[1].\n>\n> +`GIT_PUSH_CERT_OPTION_STATUS`::\n> +`BAD`;;\n> + For backwards compatibility reasons, when accepting a signed\n> + push, receive-pack requires that push options be written both\n> + inside and outside the certificate. (\"git push\" does this\n> + automatically.) `BAD` is returned if they are inconsistent.\n> +`OK`;;\n> + The push options inside and outside the certificate are\n> + consistent.\n> +\n> This hook is called before any refname is updated and before any\n> fast-forward checks are performed.\n>\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index f96834f42..fe26c2f72 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -81,6 +81,9 @@ static long nonce_stamp_slop;\n> static unsigned long nonce_stamp_slop_limit;\n> static struct ref_transaction *transaction;\n>\n> +static const char *PUSH_OPTION_BAD = \"BAD\";\n> +static const char *PUSH_OPTION_OK = \"OK\";\n> +\n> static enum {\n> KEEPALIVE_NEVER = 0,\n> KEEPALIVE_AFTER_NUL,\n> @@ -473,7 +476,8 @@ static char *prepare_push_cert_nonce(const char *path, unsigned long stamp)\n> * after dropping \"_commit\" from its name and possibly moving it out\n> * of commit.c\n> */\n> -static char *find_header(const char *msg, size_t len, const char *key)\n> +static char *find_header(const char *msg, size_t len, const char *key,\n> + const char **next_line)\n> {\n> int key_len = strlen(key);\n> const char *line = msg;\n> @@ -486,6 +490,8 @@ static char *find_header(const char *msg, size_t len, const char *key)\n> if (line + key_len < eol &&\n> !memcmp(line, key, key_len) && line[key_len] == ' ') {\n> int offset = key_len + 1;\n> + if (next_line)\n> + *next_line = *eol ? eol + 1 : eol;\n> return xmemdupz(line + offset, (eol - line) - offset);\n> }\n> line = *eol ? eol + 1 : NULL;\n> @@ -495,7 +501,7 @@ static char *find_header(const char *msg, size_t len, const char *key)\n>\n> static const char *check_nonce(const char *buf, size_t len)\n> {\n> - char *nonce = find_header(buf, len, \"nonce\");\n> + char *nonce = find_header(buf, len, \"nonce\", NULL);\n> unsigned long stamp, ostamp;\n> char *bohmac, *expect = NULL;\n> const char *retval = NONCE_BAD;\n> @@ -575,9 +581,40 @@ static const char *check_nonce(const char *buf, size_t len)\n> return retval;\n> }\n>\n> -static void prepare_push_cert_sha1(struct child_process *proc)\n> +static const char *check_push_option(const char *buf, size_t len,\n> + const struct string_list *push_options)\n> +{\n> + int options_seen = 0;\n> + char *option;\n> + const char *next_line;\n> + const char *retval = PUSH_OPTION_OK;\n> +\n> + while ((option = find_header(buf, len, \"push-option\", &next_line))) {\n> + len -= (next_line - buf);\n> + buf = next_line;\n> + options_seen++;\n> + if (options_seen > push_options->nr\n> + || strcmp(option,\n> + push_options->items[options_seen - 1].string)) {\n> + retval = PUSH_OPTION_BAD;\n> + goto leave;\n> + }\n> + free(option);\n> + }\n> +\n> + if (options_seen != push_options->nr)\n> + retval = PUSH_OPTION_BAD;\n> +\n> +leave:\n> + free(option);\n> + return retval;\n> +}\n> +\n> +static void prepare_push_cert_sha1(struct child_process *proc,\n> + const struct string_list *push_options)\n> {\n> static int already_done;\n> + static const char *push_option_status;\n>\n> if (!push_cert.len)\n> return;\n> @@ -609,6 +646,8 @@ static void prepare_push_cert_sha1(struct child_process *proc)\n> strbuf_release(&gpg_output);\n> strbuf_release(&gpg_status);\n> nonce_status = check_nonce(push_cert.buf, bogs);\n> + push_option_status = check_push_option(push_cert.buf, bogs,\n> + push_options);\n> }\n> if (!is_null_sha1(push_cert_sha1)) {\n> argv_array_pushf(&proc->env_array, \"GIT_PUSH_CERT=%s\",\n> @@ -631,6 +670,8 @@ static void prepare_push_cert_sha1(struct child_process *proc)\n> \"GIT_PUSH_CERT_NONCE_SLOP=%ld\",\n> nonce_stamp_slop);\n> }\n> + argv_array_pushf(&proc->env_array, \"GIT_PUSH_CERT_OPTION_STATUS=%s\",\n> + push_option_status);\n> }\n> }\n>\n> @@ -683,7 +724,7 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,\n> proc.err = muxer.in;\n> }\n>\n> - prepare_push_cert_sha1(&proc);\n> + prepare_push_cert_sha1(&proc, feed_state->push_options);\n>\n> code = start_command(&proc);\n> if (code) {\n> diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh\n> index ecb8d446a..2607b8797 100755\n> --- a/t/t5534-push-signed.sh\n> +++ b/t/t5534-push-signed.sh\n> @@ -124,6 +124,21 @@ test_expect_success GPG 'signed push sends push certificate' '\n> test_cmp expect dst/push-cert-status\n> '\n>\n> +test_expect_success GPG 'signed push reports push option status in receive hook' '\n> + prepare_dst &&\n> + mkdir -p dst/.git/hooks &&\n> + git -C dst config receive.certnonceseed sekrit &&\n> + git -C dst config receive.advertisepushoptions 1 &&\n> + write_script dst/.git/hooks/post-receive <<-\\EOF &&\n> + # record the push option status\n> + echo \"$GIT_PUSH_CERT_OPTION_STATUS\" > ../push-option-status\n> + EOF\n> +\n> + git push --push-option=\"foo\" --push-option=\"bar\" --signed dst noop ff &&\n> +\n> + test \"$(cat dst/push-option-status)\" = \"OK\"\n> +'\n> +\n\nI think in this context we'd really want to test the negative way as\nwell, reporting BAD?\nI am unsure how easy it is to forge push options in the test, though.\nThe code looks good, but I only reviewed it for a minute.\n\nThanks,\nStefan\n"},{"id":"318920","messageId":"20170506000608.GD55152@google.com","threadId":"45883","inReplyTo":"CAGZ79ka4vG1yd1JtK9bDBjWPUG0UCPMvw2XQUsfX_e_xFCpVLA@mail.gmail.com","subject":"Re: [PATCH 2/3] receive-pack: verify push options in cert","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-06T00:06:08Z","receivedAt":"2017-05-06T00:06:15Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/05, Stefan Beller wrote:\n> On Fri, May 5, 2017 at 4:46 PM, Jonathan Tan wrote:\n> > In commit f6a4e61 (\"push: accept push options\", 2016-07-14), send-pack\n> > was taught to include push options both within the signed cert (if the\n> > push is a signed push) and outside the signed cert; however,\n> > receive-pack ignores push options within the cert, only handling push\n> > options outside the cert.\n> >\n> > Teach receive-pack, in the case that push options are provided for a\n> > signed push, to verify that the push options both within the cert and\n> > outside the cert are consistent, and to provide the results of that\n> > verification to the receive hooks as an environment variable (just like\n> > it currently does for the nonce verification).\n> >\n> > This sets in stone the requirement that send-pack redundantly send its\n> > push options in 2 places, but I think that this is better than the\n> > alternatives. Sending push options only within the cert is\n> > backwards-incompatible with existing Git servers (which read push\n> > options only from outside the cert), and sending push options only\n> > outside the cert means that the push options are not signed for.\n> \n> As the combination of push certs and push options are broken\n> (at least when using JGit as well, which are used in Gerrit\n> installations), I would not feel too bad about actually going\n> the backwards-incompatible way.\n\nyeah just think of the bits that could be saved!\n\nBut in all seriousness, I'd agree that doing the backwards-incompatible\nway would be cleaner in the longer run (esp since they are broken\ncurrently).\n\n\n-- \nBrandon Williams\n"},{"id":"318921","messageId":"20170506001002.GK28740@aiede.svl.corp.google.com","threadId":"45883","inReplyTo":"01e098de2da2e5f0b341ab6d967d032d840c365e.1494027001.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/3] receive-pack: verify push options in cert","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-05-06T00:10:02Z","receivedAt":"2017-05-06T00:10:10Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJonathan Tan wrote:\n\n> In commit f6a4e61 (\"push: accept push options\", 2016-07-14), send-pack\n> was taught to include push options both within the signed cert (if the\n> push is a signed push) and outside the signed cert; however,\n> receive-pack ignores push options within the cert, only handling push\n> options outside the cert.\n\nYikes. Thanks for fixing it.\n\n[...]\n> --- a/Documentation/git-receive-pack.txt\n> +++ b/Documentation/git-receive-pack.txt\n> @@ -106,6 +106,16 @@ the following environment variables:\n> \tAlso read about `receive.certNonceSlop` variable in\n> \tlinkgit:git-config[1].\n> \n> +`GIT_PUSH_CERT_OPTION_STATUS`::\n> +`BAD`;;\n> +\tFor backwards compatibility reasons, when accepting a signed\n> +\tpush, receive-pack requires that push options be written both\n> +\tinside and outside the certificate. (\"git push\" does this\n> +\tautomatically.) `BAD` is returned if they are inconsistent.\n\nIn this manual page the reader doesn't need to know it's for backword\ncompatibility. It is simply what the protocol requires.\n\nThe protocol doc and especially the commit message are better places\nto talk about rationale (as you already are doing nicely in patch 3).\n\n> +`OK`;;\n> +\tThe push options inside and outside the certificate are\n> +\tconsistent.\n\nHm. I would have thought it would be simpler to simply reject the\npush without invoking hooks in the BAD case. But\nGIT_PUSH_CERT_NONCE_STATUS provides precedent for this, forcing me to\nthink longer.\n\nIs the idea that invoking the hook despite a bad client is a way to\nprovide an opportunity to audit log the error?\n\n... okay, I've thought longer. I think this is a different kind of\nerror than a bad nonce: for example, a bad nonce might be the result\nof a misconfiguration that makes the client get the wrong nonce or a\nsign of a caller trying to brute-force a stable nonce. For\ncomparison, this error is a more simple protocol violation, like a\nmalformed pkt-line.\n\nWhen we see a malformed pkt-line, we error out and do not invoke the\npre-receive hook. For this error I think we should behave the same\nway: show an error to the user and abort the connection, without\ninvoking hooks.\n\n[...]\n> --- a/t/t5534-push-signed.sh\n> +++ b/t/t5534-push-signed.sh\n> @@ -124,6 +124,21 @@ test_expect_success GPG 'signed push sends push certificate' '\n> \ttest_cmp expect dst/push-cert-status\n> '\n> \n> +test_expect_success GPG 'signed push reports push option status in receive hook' '\n\nIs there a straightforward way to test the error case? (If there\nisn't, I can live with that --- it would just be nice to have a test\nthat the behavior introduced here is preserved.)\n\nThanks and hope that helps,\nJonathan\n"},{"id":"318922","messageId":"CAGZ79kb5vQ6_3j6r7szDvXtOdCSx6-HUeMuC9yjgMHL0jvA5BQ@mail.gmail.com","threadId":"45883","inReplyTo":"615007182a5e205a7074317a4c8788aa4eb009ff.1494027001.git.jonathantanmy@google.com","subject":"Re: [PATCH 3/3] protocol docs: explain receive-pack push options","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-06T00:10:54Z","receivedAt":"2017-05-06T00:11:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, May 5, 2017 at 4:46 PM, Jonathan Tan wrote:\n> Support for push options in the receive-pack protocol (and all Git\n> components that speak it) have been added over a few commits, but not\n> fully documented (especially its interaction with signed pushes). Update\n> the protocol documentation to include the relevant details.\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> Documentation/technical/pack-protocol.txt | 32 +++++++++++++++++++++++++------\n> 1 file changed, 26 insertions(+), 6 deletions(-)\n>\n> diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\n> index 5b0ba3ef2..cf7cb48c3 100644\n> --- a/Documentation/technical/pack-protocol.txt\n> +++ b/Documentation/technical/pack-protocol.txt\n> @@ -473,13 +473,10 @@ that it wants to update, it sends a line listing the obj-id currently on\n> the server, the obj-id the client would like to update it to and the name\n> of the reference.\n>\n> -This list is followed by a flush-pkt. Then the push options are transmitted\n> -one per packet followed by another flush-pkt. After that the packfile that\n> -should contain all the objects that the server will need to complete the new\n> -references will be sent.\n> +This list is followed by a flush-pkt.\n>\n> ----\n> - update-request = *shallow ( command-list | push-cert ) [packfile]\n> + update-request = *shallow ( command-list | push-cert )\n>\n> shallow = PKT-LINE(\"shallow\" SP obj-id)\n>\n> @@ -500,12 +497,35 @@ references will be sent.\n> PKT-LINE(\"pusher\" SP ident LF)\n> PKT-LINE(\"pushee\" SP url LF)\n> PKT-LINE(\"nonce\" SP nonce LF)\n> + *PKT-LINE(\"push-option\" SP push-option LF)\n> PKT-LINE(LF)\n> *PKT-LINE(command LF)\n> *PKT-LINE(gpg-signature-lines LF)\n> PKT-LINE(\"push-cert-end\" LF)\n>\n> - packfile = \"PACK\" 28*(OCTET)\n> + push-option = 1*CHAR\n> +----\n> +\n> +If the server has advertised the 'push-options' capability and the client has\n> +specified 'push-options' as part of the capability list above, the client then\n> +sends its push options followed by a flush-pkt.\n\nThe CHAR of the push-options SHOULD NOT include an LF. Well I guess that may\nbe obvious when looking at PKT-LINE(\"push-option\" SP push-option LF),\nnot sure if we want to mention it here.\n\n> +\n> +----\n> + push-options = *PKT-LINE(push-option) flush-pkt\n> +----\n> +\n> +For backwards compatibility with older Git servers, if the client sends a push\n> +cert and push options, it SHOULD send its push options both embedded within the\n> +push cert and after the push cert. (Note that the push options within the cert\n> +are prefixed, but the push options after the cert are not.) Both these lists\n> +SHOULD be consistent.\n\ns/SHOULD/MUST/ ?\n\n> +\n> +After that the packfile that\n> +should contain all the objects that the server will need to complete the new\n> +references will be sent.\n> +\n> +----\n> + packfile = \"PACK\" 28*(OCTET)\n> ----\n>\n> If the receiving end does not support delete-refs, the sending end MUST\n> --\n> 2.13.0.rc1.294.g07d810a77f-goog\n\nThanks for writing the docs.\n\nStefan\n\n\n>\n"},{"id":"318923","messageId":"20170506002007.GL28740@aiede.svl.corp.google.com","threadId":"45883","inReplyTo":"CAGZ79ka4vG1yd1JtK9bDBjWPUG0UCPMvw2XQUsfX_e_xFCpVLA@mail.gmail.com","subject":"Re: [PATCH 2/3] receive-pack: verify push options in cert","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-05-06T00:20:07Z","receivedAt":"2017-05-06T00:20:19Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n> On Fri, May 5, 2017 at 4:46 PM, Jonathan Tan wrote:\n\n>> This sets in stone the requirement that send-pack redundantly send its\n>> push options in 2 places, but I think that this is better than the\n>> alternatives. Sending push options only within the cert is\n>> backwards-incompatible with existing Git servers (which read push\n>> options only from outside the cert), and sending push options only\n>> outside the cert means that the push options are not signed for.\n>\n> As the combination of push certs and push options are broken\n> (at least when using JGit as well, which are used in Gerrit\n> installations), I would not feel too bad about actually going\n> the backwards-incompatible way.\n\nPer offline discussion, what you're proposing is to omit the second\ncopy of push options from the client request, so the server does not\nhave to see two copies. I agree that that would be a better protocol,\nbut I think that ship has sailed.\n\nCurrent versions of git the client and git the server are able to\ninteroperate without trouble (though the server-side bug is ugly in\nterms of what the push certs mean).\n\nCurrent versions of JGit the server *require* that the push options be\nomitted from the push certificate. I don't think there exists a\nsensible way to interoperate with that, so we can ignore that JGit\nserver behavior as a bug (like you've said).\n\nIf we want to omit the second copy of the push certs (which sounds\nlike a reasonable thing to want), that would require a new capability\nto be added to the protocol to do so. That way, interoperability with\nexisting versions of git the client wouldn't be broken. That could\nhappen on top of this series --- it is not needed for fixing the bug\nthat this series fixes.\n\nTo summarize:\n\n 1. I agree that we shouldn't feel bad about breaking compatibility\n with the JGit server behavior at issue, since there is no\n reasonable way to be compatible with it. And that's what this\n series does --- it breaks compatibility with the broken versions\n of JGit server and picks what the Git client has been doing\n instead.\n\n 2. I don't think we should break new versions of Git's ability to\n interoperate with current versions of the server, even though I\n consider the current server behavior to be broken.\n\n 3. If someone is interested in getting rid of the redundant second\n copy of push options, we have a way to do so, by introducing a\n new capability\n\nThanks,\nJonathan\n"},{"id":"318924","messageId":"20170506002605.GM28740@aiede.svl.corp.google.com","threadId":"45883","inReplyTo":"615007182a5e205a7074317a4c8788aa4eb009ff.1494027001.git.jonathantanmy@google.com","subject":"Re: [PATCH 3/3] protocol docs: explain receive-pack push options","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-05-06T00:26:05Z","receivedAt":"2017-05-06T00:26:13Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJonathan Tan wrote:\n\n> Support for push options in the receive-pack protocol (and all Git\n> components that speak it) have been added over a few commits, but not\n> fully documented (especially its interaction with signed pushes). Update\n> the protocol documentation to include the relevant details.\n\nThanks. This could be combined with the previous patch, since that\npatch is enforcing what this patch documents.\n\n[...]\n> -This list is followed by a flush-pkt. Then the push options are transmitted\n> -one per packet followed by another flush-pkt. After that the packfile that\n> -should contain all the objects that the server will need to complete the new\n> -references will be sent.\n> +This list is followed by a flush-pkt.\n\nI think this removed more than intended.\n\nBefore c714e45f (receive-pack: implement advertising and receiving\npush options, 2016-07-14), this said\n\n\tThis list is followed by a flush-pkt and then the packfile that should\n\tcontain all the objects that the server will need to complete the new\n\treferences.\n\nwhich I think would work fine.\n\n[...]\n> - update-request = *shallow ( command-list | push-cert ) [packfile]\n> + update-request = *shallow ( command-list | push-cert )\n\nThis seems confusing to me both before and after. How does\nupdate-request get used? Is it supposed to include the packfile or not?\n\nWhere do push-options fit into an unsigned request?\n\n[...]\n> @@ -500,12 +497,35 @@ references will be sent.\n> \t\t PKT-LINE(\"pusher\" SP ident LF)\n> \t\t PKT-LINE(\"pushee\" SP url LF)\n> \t\t PKT-LINE(\"nonce\" SP nonce LF)\n> +\t\t *PKT-LINE(\"push-option\" SP push-option LF)\n> \t\t PKT-LINE(LF)\n> \t\t *PKT-LINE(command LF)\n> \t\t *PKT-LINE(gpg-signature-lines LF)\n> \t\t PKT-LINE(\"push-cert-end\" LF)\n> \n> - packfile = \"PACK\" 28*(OCTET)\n> + push-option = 1*CHAR\n> +----\n> +\n> +If the server has advertised the 'push-options' capability and the client has\n> +specified 'push-options' as part of the capability list above, the client then\n> +sends its push options followed by a flush-pkt.\n> +\n> +----\n> + push-options = *PKT-LINE(push-option) flush-pkt\n> +----\n> +\n> +For backwards compatibility with older Git servers, if the client sends a push\n> +cert and push options, it SHOULD send its push options both embedded within the\n\nThis needs to be a MUST, not SHOULD.\n\n> +push cert and after the push cert. (Note that the push options within the cert\n> +are prefixed, but the push options after the cert are not.) Both these lists\n> +SHOULD be consistent.\n\nLikewise this one.\n\nWhat does it mean to be consistent?\n\n> +\n> +After that the packfile that\n> +should contain all the objects that the server will need to complete the new\n> +references will be sent.\n> +\n> +----\n> + packfile = \"PACK\" 28*(OCTET)\n> ----\n\nThanks,\nJonathan\n"},{"id":"319057","messageId":"xmqqa86n9a3d.fsf@gitster.mtv.corp.google.com","threadId":"45883","inReplyTo":"cover.1494027001.git.jonathantanmy@google.com","subject":"Re: [PATCH 0/3] Clarify interaction between signed pushes and push options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-08T05:44:06Z","receivedAt":"2017-05-08T05:44:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> We noticed this when trying to use Git to make a signed push with push\n> options to a server using JGit (which rejects such pushes because the\n> Git client makes requests that are, strictly speaking, incompatible with\n> the documented protocol).\n>\n> There have been several commits (see the commits linked in the commit\n> messages of the patches sent in reply to this e-mail) to support push\n> options (that are read by receive hooks) when using \"git push\", but the\n> interaction between push options and signed pushes are not very clear.\n> Here are some patches (containing both code and documentation) that\n> clarify this interaction.\n>\n> I would appreciate a review of this - if we could make the protocol\n> clear, we could then update JGit to use the updated protocol and be no\n> longer incompatible with existing Git clients.\n\nI've seen the exchanges on list on these three patches, and I agree\nwith what Jonathan said.\n\nThese are going in the right direction in general but a few nits\nneed to be picked before becoming ready for 'next'.\n\nThanks.\n\n>\n> Jonathan Tan (3):\n> docs: correct receive.advertisePushOptions default\n> receive-pack: verify push options in cert\n> protocol docs: explain receive-pack push options\n>\n> Documentation/config.txt | 5 ++--\n> Documentation/git-receive-pack.txt | 10 +++++++\n> Documentation/technical/pack-protocol.txt | 32 ++++++++++++++++----\n> builtin/receive-pack.c | 49 ++++++++++++++++++++++++++++---\n> t/t5534-push-signed.sh | 15 ++++++++++\n> 5 files changed, 98 insertions(+), 13 deletions(-)\n"},{"id":"319118","messageId":"01f61fa4-d69d-8f0b-76e1-bca3444f4101@google.com","threadId":"45883","inReplyTo":"20170506002605.GM28740@aiede.svl.corp.google.com","subject":"Re: [PATCH 3/3] protocol docs: explain receive-pack push options","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-08T21:27:12Z","receivedAt":"2017-05-08T21:27:20Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 05/05/2017 05:26 PM, Jonathan Nieder wrote:\n>> -This list is followed by a flush-pkt. Then the push options are transmitted\n>> -one per packet followed by another flush-pkt. After that the packfile that\n>> -should contain all the objects that the server will need to complete the new\n>> -references will be sent.\n>> +This list is followed by a flush-pkt.\n>\n> I think this removed more than intended.\n>\n> Before c714e45f (receive-pack: implement advertising and receiving\n> push options, 2016-07-14), this said\n>\n> \tThis list is followed by a flush-pkt and then the packfile that should\n> \tcontain all the objects that the server will need to complete the new\n> \treferences.\n>\n> which I think would work fine.\n\nThat wouldn't work fine because there is no mention of push options - \nhence the modification in c714e45f to talk about push options, but that \nis also not accurate because push options (and, more importantly, the \nassociated flush-pkt) are not sent if the client doesn't have any.\n\n> [...]\n>> - update-request = *shallow ( command-list | push-cert ) [packfile]\n>> + update-request = *shallow ( command-list | push-cert )\n>\n> This seems confusing to me both before and after. How does\n> update-request get used? Is it supposed to include the packfile or not?\n>\n> Where do push-options fit into an unsigned request?\n\nI've updated \"update-request\" to \"update-requests\" to show that this is \nthe \"list of reference update requests\" mentioned in the previous paragraph.\n\nThis is only the ref part - I've chosen to describe push options and \npackfile in separate sections, because there are very specific rules \nthat determine whether the push option section must be included or omitted.\n\n>\n> [...]\n>> @@ -500,12 +497,35 @@ references will be sent.\n>> \t\t PKT-LINE(\"pusher\" SP ident LF)\n>> \t\t PKT-LINE(\"pushee\" SP url LF)\n>> \t\t PKT-LINE(\"nonce\" SP nonce LF)\n>> +\t\t *PKT-LINE(\"push-option\" SP push-option LF)\n>> \t\t PKT-LINE(LF)\n>> \t\t *PKT-LINE(command LF)\n>> \t\t *PKT-LINE(gpg-signature-lines LF)\n>> \t\t PKT-LINE(\"push-cert-end\" LF)\n>>\n>> - packfile = \"PACK\" 28*(OCTET)\n>> + push-option = 1*CHAR\n>> +----\n>> +\n>> +If the server has advertised the 'push-options' capability and the client has\n>> +specified 'push-options' as part of the capability list above, the client then\n>> +sends its push options followed by a flush-pkt.\n>> +\n>> +----\n>> + push-options = *PKT-LINE(push-option) flush-pkt\n>> +----\n>> +\n>> +For backwards compatibility with older Git servers, if the client sends a push\n>> +cert and push options, it SHOULD send its push options both embedded within the\n>\n> This needs to be a MUST, not SHOULD.\n>\n>> +push cert and after the push cert. (Note that the push options within the cert\n>> +are prefixed, but the push options after the cert are not.) Both these lists\n>> +SHOULD be consistent.\n>\n> Likewise this one.\n>\n> What does it mean to be consistent?\n\nChanged to \"MUST be the same, modulo the prefix\".\n"},{"id":"319120","messageId":"cover.1494279020.git.jonathantanmy@google.com","threadId":"45883","inReplyTo":"cover.1494027001.git.jonathantanmy@google.com","subject":"[PATCH v2 0/2] Clarify interaction between signed pushes and push options","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-08T21:33:49Z","receivedAt":"2017-05-08T21:34:03Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Changes from v1:\n - merged last 2 patches\n - patch 1:\n - updated advertisePushOptions doc to say \"False by default\"\n - patch 2 (formerly 2-3):\n - reject mismatching push options (similar to how a pre-receive hook\n can reject a push) instead of merely reporting it\n - added test to check failure (in addition to success)\n - modified pack-protocol.txt to describe new behavior\n\nJonathan Tan (2):\n docs: correct receive.advertisePushOptions default\n receive-pack: verify push options in cert\n\n Documentation/config.txt | 5 ++-\n Documentation/technical/pack-protocol.txt | 32 +++++++++++++++----\n builtin/receive-pack.c | 51 +++++++++++++++++++++++++++++--\n t/t5534-push-signed.sh | 37 ++++++++++++++++++++++\n 4 files changed, 114 insertions(+), 11 deletions(-)\n\n-- \n2.13.0.rc1.294.g07d810a77f-goog\n\n"},{"id":"319121","messageId":"3d79124df1e235e1819cf16a41ecec27391f8609.1494279020.git.jonathantanmy@google.com","threadId":"45883","inReplyTo":"cover.1494279020.git.jonathantanmy@google.com","subject":"[PATCH v2 1/2] docs: correct receive.advertisePushOptions default","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-08T21:33:50Z","receivedAt":"2017-05-08T21:34:04Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In commit c714e45 (\"receive-pack: implement advertising and receiving\npush options\", 2016-07-14), receive-pack was taught to (among other\nthings) advertise that it understood push options, depending on\nconfiguration. It was documented that it advertised such ability by\ndefault; however, it actually does not. (In that commit, notice that\nadvertise_push_options defaults to 0, unlike advertise_atomic_push which\ndefaults to 1.)\n\nUpdate the documentation to state that it does not advertise the ability\nby default.\n\nSigned-off-by: Jonathan Tan \n---\n Documentation/config.txt | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 475e874d5..3a847a62e 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2620,9 +2620,8 @@ receive.advertiseAtomic::\n \tcapability, set this variable to false.\n \n receive.advertisePushOptions::\n-\tBy default, git-receive-pack will advertise the push options\n-\tcapability to its clients. If you don't want to advertise this\n-\tcapability, set this variable to false.\n+\tWhen set to true, git-receive-pack will advertise the push options\n+\tcapability to its clients. False by default.\n \n receive.autogc::\n \tBy default, git-receive-pack will run \"git-gc --auto\" after\n-- \n2.13.0.rc1.294.g07d810a77f-goog\n\n"},{"id":"319122","messageId":"cc21a5566dcf10e4683465ac253c76370b7b6a7f.1494279020.git.jonathantanmy@google.com","threadId":"45883","inReplyTo":"cover.1494279020.git.jonathantanmy@google.com","subject":"[PATCH v2 2/2] receive-pack: verify push options in cert","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-08T21:33:51Z","receivedAt":"2017-05-08T21:34:06Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In commit f6a4e61 (\"push: accept push options\", 2016-07-14), send-pack\nwas taught to include push options both within the signed cert (if the\npush is a signed push) and outside the signed cert; however,\nreceive-pack ignores push options within the cert, only handling push\noptions outside the cert.\n\nTeach receive-pack, in the case that push options are provided for a\nsigned push, to verify that the push options both within the cert and\noutside the cert are consistent.\n\nThis sets in stone the requirement that send-pack redundantly send its\npush options in 2 places, but I think that this is better than the\nalternatives. Sending push options only within the cert is\nbackwards-incompatible with existing Git servers (which read push\noptions only from outside the cert), and sending push options only\noutside the cert means that the push options are not signed for.\n\nSigned-off-by: Jonathan Tan \n---\n Documentation/technical/pack-protocol.txt | 32 +++++++++++++++----\n builtin/receive-pack.c | 51 +++++++++++++++++++++++++++++--\n t/t5534-push-signed.sh | 37 ++++++++++++++++++++++\n 3 files changed, 112 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\nindex 5b0ba3ef2..a34917153 100644\n--- a/Documentation/technical/pack-protocol.txt\n+++ b/Documentation/technical/pack-protocol.txt\n@@ -473,13 +473,10 @@ that it wants to update, it sends a line listing the obj-id currently on\n the server, the obj-id the client would like to update it to and the name\n of the reference.\n \n-This list is followed by a flush-pkt. Then the push options are transmitted\n-one per packet followed by another flush-pkt. After that the packfile that\n-should contain all the objects that the server will need to complete the new\n-references will be sent.\n+This list is followed by a flush-pkt.\n \n ----\n- update-request = *shallow ( command-list | push-cert ) [packfile]\n+ update-requests = *shallow ( command-list | push-cert )\n \n shallow = PKT-LINE(\"shallow\" SP obj-id)\n \n@@ -500,12 +497,35 @@ references will be sent.\n \t\t PKT-LINE(\"pusher\" SP ident LF)\n \t\t PKT-LINE(\"pushee\" SP url LF)\n \t\t PKT-LINE(\"nonce\" SP nonce LF)\n+\t\t *PKT-LINE(\"push-option\" SP push-option LF)\n \t\t PKT-LINE(LF)\n \t\t *PKT-LINE(command LF)\n \t\t *PKT-LINE(gpg-signature-lines LF)\n \t\t PKT-LINE(\"push-cert-end\" LF)\n \n- packfile = \"PACK\" 28*(OCTET)\n+ push-option = 1*( VCHAR | SP )\n+----\n+\n+If the server has advertised the 'push-options' capability and the client has\n+specified 'push-options' as part of the capability list above, the client then\n+sends its push options followed by a flush-pkt.\n+\n+----\n+ push-options = *PKT-LINE(push-option) flush-pkt\n+----\n+\n+For backwards compatibility with older Git servers, if the client sends a push\n+cert and push options, it MUST send its push options both embedded within the\n+push cert and after the push cert. (Note that the push options within the cert\n+are prefixed, but the push options after the cert are not.) Both these lists\n+MUST be the same, modulo the prefix.\n+\n+After that the packfile that\n+should contain all the objects that the server will need to complete the new\n+references will be sent.\n+\n+----\n+ packfile = \"PACK\" 28*(OCTET)\n ----\n \n If the receiving end does not support delete-refs, the sending end MUST\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex f96834f42..fff5c7a54 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -473,7 +473,8 @@ static char *prepare_push_cert_nonce(const char *path, unsigned long stamp)\n * after dropping \"_commit\" from its name and possibly moving it out\n * of commit.c\n */\n-static char *find_header(const char *msg, size_t len, const char *key)\n+static char *find_header(const char *msg, size_t len, const char *key,\n+\t\t\t const char **next_line)\n {\n \tint key_len = strlen(key);\n \tconst char *line = msg;\n@@ -486,6 +487,8 @@ static char *find_header(const char *msg, size_t len, const char *key)\n \t\tif (line + key_len < eol &&\n \t\t !memcmp(line, key, key_len) && line[key_len] == ' ') {\n \t\t\tint offset = key_len + 1;\n+\t\t\tif (next_line)\n+\t\t\t\t*next_line = *eol ? eol + 1 : eol;\n \t\t\treturn xmemdupz(line + offset, (eol - line) - offset);\n \t\t}\n \t\tline = *eol ? eol + 1 : NULL;\n@@ -495,7 +498,7 @@ static char *find_header(const char *msg, size_t len, const char *key)\n \n static const char *check_nonce(const char *buf, size_t len)\n {\n-\tchar *nonce = find_header(buf, len, \"nonce\");\n+\tchar *nonce = find_header(buf, len, \"nonce\", NULL);\n \tunsigned long stamp, ostamp;\n \tchar *bohmac, *expect = NULL;\n \tconst char *retval = NONCE_BAD;\n@@ -575,6 +578,45 @@ static const char *check_nonce(const char *buf, size_t len)\n \treturn retval;\n }\n \n+/*\n+ * Return 1 if there is no push_cert or if the push options in push_cert are\n+ * the same as those in the argument; 0 otherwise.\n+ */\n+static int check_cert_push_options(const struct string_list *push_options)\n+{\n+\tconst char *buf = push_cert.buf;\n+\tint len = push_cert.len;\n+\n+\tchar *option;\n+\tconst char *next_line;\n+\tint options_seen = 0;\n+\n+\tint retval = 1;\n+\n+\tif (!len)\n+\t\treturn 1;\n+\n+\twhile ((option = find_header(buf, len, \"push-option\", &next_line))) {\n+\t\tlen -= (next_line - buf);\n+\t\tbuf = next_line;\n+\t\toptions_seen++;\n+\t\tif (options_seen > push_options->nr\n+\t\t || strcmp(option,\n+\t\t\t push_options->items[options_seen - 1].string)) {\n+\t\t\tretval = 0;\n+\t\t\tgoto leave;\n+\t\t}\n+\t\tfree(option);\n+\t}\n+\n+\tif (options_seen != push_options->nr)\n+\t\tretval = 0;\n+\n+leave:\n+\tfree(option);\n+\treturn retval;\n+}\n+\n static void prepare_push_cert_sha1(struct child_process *proc)\n {\n \tstatic int already_done;\n@@ -1929,6 +1971,11 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \n \t\tif (use_push_options)\n \t\t\tread_push_options(&push_options);\n+\t\tif (!check_cert_push_options(&push_options)) {\n+\t\t\tstruct command *cmd;\n+\t\t\tfor (cmd = commands; cmd; cmd = cmd->next)\n+\t\t\t\tcmd->error_string = \"inconsistent push options\";\n+\t\t}\n \n \t\tprepare_shallow_info(&si, &shallow);\n \t\tif (!si.nr_ours && !si.nr_theirs)\ndiff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh\nindex ecb8d446a..0ef6f66b5 100755\n--- a/t/t5534-push-signed.sh\n+++ b/t/t5534-push-signed.sh\n@@ -124,6 +124,43 @@ test_expect_success GPG 'signed push sends push certificate' '\n \ttest_cmp expect dst/push-cert-status\n '\n \n+test_expect_success GPG 'inconsistent push options in signed push not allowed' '\n+\t# First, invoke receive-pack with dummy input to obtain its preamble.\n+\tprepare_dst &&\n+\tgit -C dst config receive.certnonceseed sekrit &&\n+\tgit -C dst config receive.advertisepushoptions 1 &&\n+\tprintf xxxx | test_might_fail git receive-pack dst >preamble &&\n+\n+\t# Then, invoke push. Simulate a receive-pack that sends the preamble we\n+\t# obtained, followed by a dummy packet.\n+\twrite_script myscript <<-\\EOF &&\n+\t\tcat preamble &&\n+\t\tprintf xxxx &&\n+\t\tcat >push\n+\tEOF\n+\ttest_might_fail git push --push-option=\"foo\" --push-option=\"bar\" \\\n+\t\t--receive-pack=\"\\\"$(pwd)/myscript\\\"\" --signed dst --delete ff &&\n+\n+\t# Replay the push output on a fresh dst, checking that ff is truly\n+\t# deleted.\n+\tprepare_dst &&\n+\tgit -C dst config receive.certnonceseed sekrit &&\n+\tgit -C dst config receive.advertisepushoptions 1 &&\n+\tgit receive-pack dst out &&\n+\tgit -C dst rev-parse ff &&\n+\tgrep \"inconsistent push options\" out\n+'\n+\n test_expect_success GPG 'fail without key and heed user.signingkey' '\n \tprepare_dst &&\n \tmkdir -p dst/.git/hooks &&\n-- \n2.13.0.rc1.294.g07d810a77f-goog\n\n"},{"id":"319151","messageId":"xmqqlgq64t5k.fsf@gitster.mtv.corp.google.com","threadId":"45883","inReplyTo":"cc21a5566dcf10e4683465ac253c76370b7b6a7f.1494279020.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 2/2] receive-pack: verify push options in cert","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-09T03:15:51Z","receivedAt":"2017-05-09T03:15:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Teach receive-pack, in the case that push options are provided for a\n> signed push, to verify that the push options both within the cert and\n> outside the cert are consistent.\n\nThanks. The idea was that the certificate should record how the\npush was made fully, hence we need two copies. The one outside the\ncertificate is meant to be actually used, but obviously we need to\nmake sure that matches what is recorded in the certificate.\n\nIn retrospect, we could have required the receiver who groks signed\npushes to only look inside the certificate for options etc. so that\nthe sender can omit the \"extra\" copies outside the certificate, but\nthat is not how the current protocol is structured, hence ...\n\n> This sets in stone the requirement that send-pack redundantly send its\n> push options in 2 places,...\n\n... this requirement.\n\nThanks.\n"},{"id":"319158","messageId":"xmqqfuge4sak.fsf@gitster.mtv.corp.google.com","threadId":"45883","inReplyTo":"cc21a5566dcf10e4683465ac253c76370b7b6a7f.1494279020.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 2/2] receive-pack: verify push options in cert","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-09T03:34:27Z","receivedAt":"2017-05-09T03:34:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> +\t# Tweak the push output to make the push option outside the cert\n> +\t# different, then replay it on a fresh dst, checking that ff is not\n> +\t# deleted.\n> +\tsed -i \"s/\\\\([^ ]\\\\)bar/\\\\1baz/\" push &&\n\nThis is not portable. Didn't you get an error from the lint\nchecker?\n\nThe attached may not be enough if \"push\" is a binary file, though,\nin which case perl may be your friend ;-)\n\n t/t5534-push-signed.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh\nindex 0ef6f66b5d..effe769688 100755\n--- a/t/t5534-push-signed.sh\n+++ b/t/t5534-push-signed.sh\n@@ -152,11 +152,11 @@ test_expect_success GPG 'inconsistent push options in signed push not allowed' '\n \t# Tweak the push output to make the push option outside the cert\n \t# different, then replay it on a fresh dst, checking that ff is not\n \t# deleted.\n-\tsed -i \"s/\\\\([^ ]\\\\)bar/\\\\1baz/\" push &&\n+\tsed -e \"s/\\\\([^ ]\\\\)bar/\\\\1baz/\" push >push.tweak &&\n \tprepare_dst &&\n \tgit -C dst config receive.certnonceseed sekrit &&\n \tgit -C dst config receive.advertisepushoptions 1 &&\n-\tgit receive-pack dst out &&\n+\tgit receive-pack dst out &&\n \tgit -C dst rev-parse ff &&\n \tgrep \"inconsistent push options\" out\n '\n\n\n"},{"id":"319206","messageId":"20170509164515.31942-1-jonathantanmy@google.com","threadId":"45883","inReplyTo":"xmqqfuge4sak.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] fixup! use perl instead of sed","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-09T16:45:15Z","receivedAt":"2017-05-09T16:45:28Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Signed-off-by: Jonathan Tan \n---\n\nThanks - I didn't realize the existence of the test lint. Here's a\nfixup. Let me know if you prefer a full reroll.\n\nI had to use perl because \"push\" is a binary file (the first line\ncontains a NUL).\n\n\n t/t5534-push-signed.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh\nindex 0ef6f66b5..3ee58dafb 100755\n--- a/t/t5534-push-signed.sh\n+++ b/t/t5534-push-signed.sh\n@@ -152,11 +152,11 @@ test_expect_success GPG 'inconsistent push options in signed push not allowed' '\n \t# Tweak the push output to make the push option outside the cert\n \t# different, then replay it on a fresh dst, checking that ff is not\n \t# deleted.\n-\tsed -i \"s/\\\\([^ ]\\\\)bar/\\\\1baz/\" push &&\n+\tperl -pe \"s/([^ ])bar/\\\\1baz/\" push >push.tweak &&\n \tprepare_dst &&\n \tgit -C dst config receive.certnonceseed sekrit &&\n \tgit -C dst config receive.advertisepushoptions 1 &&\n-\tgit receive-pack dst out &&\n+\tgit receive-pack dst out &&\n \tgit -C dst rev-parse ff &&\n \tgrep \"inconsistent push options\" out\n '\n-- \n2.13.0.rc2.291.g57267f2277-goog\n\n"},{"id":"319207","messageId":"CACBZZX5DF6sTTYSUpYnvas925N-XsoUGrM-bYbAN_67g8-w2GQ@mail.gmail.com","threadId":"45883","inReplyTo":"20170509164515.31942-1-jonathantanmy@google.com","subject":"Re: [PATCH] fixup! use perl instead of sed","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-09T17:00:14Z","receivedAt":"2017-05-09T17:00:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Tue, May 9, 2017 at 6:45 PM, Jonathan Tan wrote:\n> Signed-off-by: Jonathan Tan \n> ---\n>\n> Thanks - I didn't realize the existence of the test lint. Here's a\n> fixup. Let me know if you prefer a full reroll.\n>\n> I had to use perl because \"push\" is a binary file (the first line\n> contains a NUL).\n>\n>\n> t/t5534-push-signed.sh | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh\n> index 0ef6f66b5..3ee58dafb 100755\n> --- a/t/t5534-push-signed.sh\n> +++ b/t/t5534-push-signed.sh\n> @@ -152,11 +152,11 @@ test_expect_success GPG 'inconsistent push options in signed push not allowed' '\n> # Tweak the push output to make the push option outside the cert\n> # different, then replay it on a fresh dst, checking that ff is not\n> # deleted.\n> - sed -i \"s/\\\\([^ ]\\\\)bar/\\\\1baz/\" push &&\n> + perl -pe \"s/([^ ])bar/\\\\1baz/\" push >push.tweak &&\n> prepare_dst &&\n> git -C dst config receive.certnonceseed sekrit &&\n> git -C dst config receive.advertisepushoptions 1 &&\n> - git receive-pack dst out &&\n> + git receive-pack dst out &&\n\nThe test should have a PERL prerequisite now, that's missing.\n\nAlso using \\1 will likely be deprecated in future versions of perl at\nsome point:\n\n $ echo hifoo | perl -wpe \"s/([^ ])bar/\\\\1baz/\"\n \\1 better written as $1 at -e line 1.\n hifoo\n\nFinally, you can just use -i like you did with sed, no need for the tempfile:\n\n $ echo hibar >push\n $ perl -pi -e 's/([^ ])bar/$1baz/' push\n $ cat push\n hibaz\n\n> git -C dst rev-parse ff &&\n> grep \"inconsistent push options\" out\n> '\n> --\n> 2.13.0.rc2.291.g57267f2277-goog\n>\n"},{"id":"319219","messageId":"cover.1494357692.git.jonathantanmy@google.com","threadId":"45883","inReplyTo":"CACBZZX5DF6sTTYSUpYnvas925N-XsoUGrM-bYbAN_67g8-w2GQ@mail.gmail.com","subject":"[PATCH v3 0/2] Clarify interaction between signed pushes and push options","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-09T19:23:51Z","receivedAt":"2017-05-09T19:24:04Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Changes from v2:\n - incorporated Junio's suggestions\n - incorporated Ævar's suggestions\n\nJonathan Tan (2):\n docs: correct receive.advertisePushOptions default\n receive-pack: verify push options in cert\n\n Documentation/config.txt | 5 ++-\n Documentation/technical/pack-protocol.txt | 32 +++++++++++++++----\n builtin/receive-pack.c | 51 +++++++++++++++++++++++++++++--\n t/t5534-push-signed.sh | 37 ++++++++++++++++++++++\n 4 files changed, 114 insertions(+), 11 deletions(-)\n\n-- \n2.13.0.rc2.291.g57267f2277-goog\n\n"},{"id":"319220","messageId":"3d79124df1e235e1819cf16a41ecec27391f8609.1494357692.git.jonathantanmy@google.com","threadId":"45883","inReplyTo":"cover.1494357692.git.jonathantanmy@google.com","subject":"[PATCH v3 1/2] docs: correct receive.advertisePushOptions default","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-09T19:23:52Z","receivedAt":"2017-05-09T19:24:06Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In commit c714e45 (\"receive-pack: implement advertising and receiving\npush options\", 2016-07-14), receive-pack was taught to (among other\nthings) advertise that it understood push options, depending on\nconfiguration. It was documented that it advertised such ability by\ndefault; however, it actually does not. (In that commit, notice that\nadvertise_push_options defaults to 0, unlike advertise_atomic_push which\ndefaults to 1.)\n\nUpdate the documentation to state that it does not advertise the ability\nby default.\n\nSigned-off-by: Jonathan Tan \n---\n Documentation/config.txt | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 475e874d5..3a847a62e 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2620,9 +2620,8 @@ receive.advertiseAtomic::\n \tcapability, set this variable to false.\n \n receive.advertisePushOptions::\n-\tBy default, git-receive-pack will advertise the push options\n-\tcapability to its clients. If you don't want to advertise this\n-\tcapability, set this variable to false.\n+\tWhen set to true, git-receive-pack will advertise the push options\n+\tcapability to its clients. False by default.\n \n receive.autogc::\n \tBy default, git-receive-pack will run \"git-gc --auto\" after\n-- \n2.13.0.rc2.291.g57267f2277-goog\n\n"},{"id":"319221","messageId":"6be7c18fa6ceb46977484dabb2029a38cec32665.1494357692.git.jonathantanmy@google.com","threadId":"45883","inReplyTo":"cover.1494357692.git.jonathantanmy@google.com","subject":"[PATCH v3 2/2] receive-pack: verify push options in cert","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-09T19:23:53Z","receivedAt":"2017-05-09T19:24:09Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In commit f6a4e61 (\"push: accept push options\", 2016-07-14), send-pack\nwas taught to include push options both within the signed cert (if the\npush is a signed push) and outside the signed cert; however,\nreceive-pack ignores push options within the cert, only handling push\noptions outside the cert.\n\nTeach receive-pack, in the case that push options are provided for a\nsigned push, to verify that the push options both within the cert and\noutside the cert are consistent.\n\nThis sets in stone the requirement that send-pack redundantly send its\npush options in 2 places, but I think that this is better than the\nalternatives. Sending push options only within the cert is\nbackwards-incompatible with existing Git servers (which read push\noptions only from outside the cert), and sending push options only\noutside the cert means that the push options are not signed for.\n\nSigned-off-by: Jonathan Tan \n---\n Documentation/technical/pack-protocol.txt | 32 +++++++++++++++----\n builtin/receive-pack.c | 51 +++++++++++++++++++++++++++++--\n t/t5534-push-signed.sh | 37 ++++++++++++++++++++++\n 3 files changed, 112 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\nindex 5b0ba3ef2..a34917153 100644\n--- a/Documentation/technical/pack-protocol.txt\n+++ b/Documentation/technical/pack-protocol.txt\n@@ -473,13 +473,10 @@ that it wants to update, it sends a line listing the obj-id currently on\n the server, the obj-id the client would like to update it to and the name\n of the reference.\n \n-This list is followed by a flush-pkt. Then the push options are transmitted\n-one per packet followed by another flush-pkt. After that the packfile that\n-should contain all the objects that the server will need to complete the new\n-references will be sent.\n+This list is followed by a flush-pkt.\n \n ----\n- update-request = *shallow ( command-list | push-cert ) [packfile]\n+ update-requests = *shallow ( command-list | push-cert )\n \n shallow = PKT-LINE(\"shallow\" SP obj-id)\n \n@@ -500,12 +497,35 @@ references will be sent.\n \t\t PKT-LINE(\"pusher\" SP ident LF)\n \t\t PKT-LINE(\"pushee\" SP url LF)\n \t\t PKT-LINE(\"nonce\" SP nonce LF)\n+\t\t *PKT-LINE(\"push-option\" SP push-option LF)\n \t\t PKT-LINE(LF)\n \t\t *PKT-LINE(command LF)\n \t\t *PKT-LINE(gpg-signature-lines LF)\n \t\t PKT-LINE(\"push-cert-end\" LF)\n \n- packfile = \"PACK\" 28*(OCTET)\n+ push-option = 1*( VCHAR | SP )\n+----\n+\n+If the server has advertised the 'push-options' capability and the client has\n+specified 'push-options' as part of the capability list above, the client then\n+sends its push options followed by a flush-pkt.\n+\n+----\n+ push-options = *PKT-LINE(push-option) flush-pkt\n+----\n+\n+For backwards compatibility with older Git servers, if the client sends a push\n+cert and push options, it MUST send its push options both embedded within the\n+push cert and after the push cert. (Note that the push options within the cert\n+are prefixed, but the push options after the cert are not.) Both these lists\n+MUST be the same, modulo the prefix.\n+\n+After that the packfile that\n+should contain all the objects that the server will need to complete the new\n+references will be sent.\n+\n+----\n+ packfile = \"PACK\" 28*(OCTET)\n ----\n \n If the receiving end does not support delete-refs, the sending end MUST\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex f96834f42..fff5c7a54 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -473,7 +473,8 @@ static char *prepare_push_cert_nonce(const char *path, unsigned long stamp)\n * after dropping \"_commit\" from its name and possibly moving it out\n * of commit.c\n */\n-static char *find_header(const char *msg, size_t len, const char *key)\n+static char *find_header(const char *msg, size_t len, const char *key,\n+\t\t\t const char **next_line)\n {\n \tint key_len = strlen(key);\n \tconst char *line = msg;\n@@ -486,6 +487,8 @@ static char *find_header(const char *msg, size_t len, const char *key)\n \t\tif (line + key_len < eol &&\n \t\t !memcmp(line, key, key_len) && line[key_len] == ' ') {\n \t\t\tint offset = key_len + 1;\n+\t\t\tif (next_line)\n+\t\t\t\t*next_line = *eol ? eol + 1 : eol;\n \t\t\treturn xmemdupz(line + offset, (eol - line) - offset);\n \t\t}\n \t\tline = *eol ? eol + 1 : NULL;\n@@ -495,7 +498,7 @@ static char *find_header(const char *msg, size_t len, const char *key)\n \n static const char *check_nonce(const char *buf, size_t len)\n {\n-\tchar *nonce = find_header(buf, len, \"nonce\");\n+\tchar *nonce = find_header(buf, len, \"nonce\", NULL);\n \tunsigned long stamp, ostamp;\n \tchar *bohmac, *expect = NULL;\n \tconst char *retval = NONCE_BAD;\n@@ -575,6 +578,45 @@ static const char *check_nonce(const char *buf, size_t len)\n \treturn retval;\n }\n \n+/*\n+ * Return 1 if there is no push_cert or if the push options in push_cert are\n+ * the same as those in the argument; 0 otherwise.\n+ */\n+static int check_cert_push_options(const struct string_list *push_options)\n+{\n+\tconst char *buf = push_cert.buf;\n+\tint len = push_cert.len;\n+\n+\tchar *option;\n+\tconst char *next_line;\n+\tint options_seen = 0;\n+\n+\tint retval = 1;\n+\n+\tif (!len)\n+\t\treturn 1;\n+\n+\twhile ((option = find_header(buf, len, \"push-option\", &next_line))) {\n+\t\tlen -= (next_line - buf);\n+\t\tbuf = next_line;\n+\t\toptions_seen++;\n+\t\tif (options_seen > push_options->nr\n+\t\t || strcmp(option,\n+\t\t\t push_options->items[options_seen - 1].string)) {\n+\t\t\tretval = 0;\n+\t\t\tgoto leave;\n+\t\t}\n+\t\tfree(option);\n+\t}\n+\n+\tif (options_seen != push_options->nr)\n+\t\tretval = 0;\n+\n+leave:\n+\tfree(option);\n+\treturn retval;\n+}\n+\n static void prepare_push_cert_sha1(struct child_process *proc)\n {\n \tstatic int already_done;\n@@ -1929,6 +1971,11 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \n \t\tif (use_push_options)\n \t\t\tread_push_options(&push_options);\n+\t\tif (!check_cert_push_options(&push_options)) {\n+\t\t\tstruct command *cmd;\n+\t\t\tfor (cmd = commands; cmd; cmd = cmd->next)\n+\t\t\t\tcmd->error_string = \"inconsistent push options\";\n+\t\t}\n \n \t\tprepare_shallow_info(&si, &shallow);\n \t\tif (!si.nr_ours && !si.nr_theirs)\ndiff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh\nindex ecb8d446a..177b933a7 100755\n--- a/t/t5534-push-signed.sh\n+++ b/t/t5534-push-signed.sh\n@@ -124,6 +124,43 @@ test_expect_success GPG 'signed push sends push certificate' '\n \ttest_cmp expect dst/push-cert-status\n '\n \n+test_expect_success GPG,PERL 'inconsistent push options in signed push not allowed' '\n+\t# First, invoke receive-pack with dummy input to obtain its preamble.\n+\tprepare_dst &&\n+\tgit -C dst config receive.certnonceseed sekrit &&\n+\tgit -C dst config receive.advertisepushoptions 1 &&\n+\tprintf xxxx | test_might_fail git receive-pack dst >preamble &&\n+\n+\t# Then, invoke push. Simulate a receive-pack that sends the preamble we\n+\t# obtained, followed by a dummy packet.\n+\twrite_script myscript <<-\\EOF &&\n+\t\tcat preamble &&\n+\t\tprintf xxxx &&\n+\t\tcat >push\n+\tEOF\n+\ttest_might_fail git push --push-option=\"foo\" --push-option=\"bar\" \\\n+\t\t--receive-pack=\"\\\"$(pwd)/myscript\\\"\" --signed dst --delete ff &&\n+\n+\t# Replay the push output on a fresh dst, checking that ff is truly\n+\t# deleted.\n+\tprepare_dst &&\n+\tgit -C dst config receive.certnonceseed sekrit &&\n+\tgit -C dst config receive.advertisepushoptions 1 &&\n+\tgit receive-pack dst out &&\n+\tgit -C dst rev-parse ff &&\n+\tgrep \"inconsistent push options\" out\n+'\n+\n test_expect_success GPG 'fail without key and heed user.signingkey' '\n \tprepare_dst &&\n \tmkdir -p dst/.git/hooks &&\n-- \n2.13.0.rc2.291.g57267f2277-goog\n\n"},{"id":"319225","messageId":"4cb1d101-3efa-6376-64fd-2b9090d0b0c4@kdbg.org","threadId":"45883","inReplyTo":"CACBZZX5DF6sTTYSUpYnvas925N-XsoUGrM-bYbAN_67g8-w2GQ@mail.gmail.com","subject":"Re: [PATCH] fixup! use perl instead of sed","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-05-09T20:43:03Z","receivedAt":"2017-05-09T20:43:13Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 09.05.2017 um 19:00 schrieb Ævar Arnfjörð Bjarmason:\n> Finally, you can just use -i like you did with sed, no need for the tempfile:\n\nNope. Some implementations of perl attempt to remove the file that it \nhas just opened. That doesn't work on Windows. You have to supply a \nbackup file name as in `perl -i.bak ...` :-(\n\n> \n> $ echo hibar >push\n> $ perl -pi -e 's/([^ ])bar/$1baz/' push\n> $ cat push\n> hibaz\n\n-- Hannes\n"},{"id":"319226","messageId":"20170509210158.17898-1-jonathantanmy@google.com","threadId":"45883","inReplyTo":"cover.1494357692.git.jonathantanmy@google.com","subject":"[PATCH v3] fixup! don't use perl -i because it's not portable","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-09T21:01:58Z","receivedAt":"2017-05-09T21:02:07Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Signed-off-by: Jonathan Tan \n---\n\nAh...thanks, Johannes, for spotting this. Here's a fixup.\n\n t/t5534-push-signed.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh\nindex 177b933a7..807267b7f 100755\n--- a/t/t5534-push-signed.sh\n+++ b/t/t5534-push-signed.sh\n@@ -152,11 +152,11 @@ test_expect_success GPG,PERL 'inconsistent push options in signed push not allow\n \t# Tweak the push output to make the push option outside the cert\n \t# different, then replay it on a fresh dst, checking that ff is not\n \t# deleted.\n-\tperl -pi -e \"s/([^ ])bar/\\$1baz/\" push &&\n+\tperl -pe \"s/([^ ])bar/\\$1baz/\" push >push.tweak &&\n \tprepare_dst &&\n \tgit -C dst config receive.certnonceseed sekrit &&\n \tgit -C dst config receive.advertisepushoptions 1 &&\n-\tgit receive-pack dst out &&\n+\tgit receive-pack dst out &&\n \tgit -C dst rev-parse ff &&\n \tgrep \"inconsistent push options\" out\n '\n-- \n2.13.0.rc2.291.g57267f2277-goog\n\n"},{"id":"319227","messageId":"9353580b-d792-f3c5-a099-74eb49b867b0@google.com","threadId":"45883","inReplyTo":"4cb1d101-3efa-6376-64fd-2b9090d0b0c4@kdbg.org","subject":"Re: [PATCH] fixup! use perl instead of sed","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-09T21:04:16Z","receivedAt":"2017-05-09T21:04:30Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 05/09/2017 01:43 PM, Johannes Sixt wrote:\n> Am 09.05.2017 um 19:00 schrieb Ævar Arnfjörð Bjarmason:\n>> Finally, you can just use -i like you did with sed, no need for the\n>> tempfile:\n>\n> Nope. Some implementations of perl attempt to remove the file that it\n> has just opened. That doesn't work on Windows. You have to supply a\n> backup file name as in `perl -i.bak ...` :-(\n>\n>>\n>> $ echo hibar >push\n>> $ perl -pi -e 's/([^ ])bar/$1baz/' push\n>> $ cat push\n>> hibaz\n>\n> -- Hannes\n\nThanks - sent a fixup [1] in reply to my PATCH v3 cover letter (but \nforgot to CC you).\n\n[1] <20170509210158.17898-1-jonathantanmy@google.com>\n"},{"id":"319228","messageId":"CACBZZX6T6f_XzqgAM8RO7sNsUy+0Qsawdaha7B4BtADt5fJszQ@mail.gmail.com","threadId":"45883","inReplyTo":"4cb1d101-3efa-6376-64fd-2b9090d0b0c4@kdbg.org","subject":"Re: [PATCH] fixup! use perl instead of sed","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-09T21:08:37Z","receivedAt":"2017-05-09T21:09:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Tue, May 9, 2017 at 10:43 PM, Johannes Sixt wrote:\n> Am 09.05.2017 um 19:00 schrieb Ævar Arnfjörð Bjarmason:\n>>\n>> Finally, you can just use -i like you did with sed, no need for the\n>> tempfile:\n>\n>\n> Nope. Some implementations of perl attempt to remove the file that it has\n> just opened. That doesn't work on Windows. You have to supply a backup file\n> name as in `perl -i.bak ...` :-(\n\nThis should have been fixed in 2002, and is in 5.8, the oldest perl\nrelease we support: https://github.com/Perl/perl5/commit/c030f24b81 &\nhttp://www.nntp.perl.org/group/perl.perl5.porters/2002/05/msg60275.html\n\nBut maybe __CYGWIN__ isn't always defined on Windows, maybe this was a\nmingw build or something and perl was missing a test for this when\nsupport for that was added?\n\nThis is obviously a trivial issue for git, but if it's a bug in perl\nI'd like to fix that.\n\n>>\n>> $ echo hibar >push\n>> $ perl -pi -e 's/([^ ])bar/$1baz/' push\n>> $ cat push\n>> hibaz\n>\n>\n> -- Hannes\n"},{"id":"319233","messageId":"xmqqd1bh1wqy.fsf@gitster.mtv.corp.google.com","threadId":"45883","inReplyTo":"CACBZZX5DF6sTTYSUpYnvas925N-XsoUGrM-bYbAN_67g8-w2GQ@mail.gmail.com","subject":"Re: [PATCH] fixup! use perl instead of sed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-09T22:38:45Z","receivedAt":"2017-05-09T22:38:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Tue, May 9, 2017 at 6:45 PM, Jonathan Tan wrote:\n> ...\n>> # Tweak the push output to make the push option outside the cert\n>> # different, then replay it on a fresh dst, checking that ff is not\n>> # deleted.\n>> - sed -i \"s/\\\\([^ ]\\\\)bar/\\\\1baz/\" push &&\n>> + perl -pe \"s/([^ ])bar/\\\\1baz/\" push >push.tweak &&\n>> prepare_dst &&\n>> git -C dst config receive.certnonceseed sekrit &&\n>> git -C dst config receive.advertisepushoptions 1 &&\n>> - git receive-pack dst out &&\n>> + git receive-pack dst out &&\n>\n> The test should have a PERL prerequisite now, that's missing.\n\nFor a single-liner like this, our stance has always been that t/\nscripts can assume _some_ version of Perl interpreter available for\nuse, cf. t/README where it lists prerequisites and explains them:\n\n - PERL\n\n Git wasn't compiled with NO_PERL=YesPlease.\n\n Even without the PERL prerequisite, tests can assume there is a\n usable perl interpreter at $PERL_PATH, though it need not be\n particularly modern.\n\nSo unless \"receive-pack\" that is being tested here requires Perl at\nruntime, we do not want PERL prerequisite for this test.\n\n> Also using \\1 will likely be deprecated in future versions of perl at\n> some point:\n>\n> $ echo hifoo | perl -wpe \"s/([^ ])bar/\\\\1baz/\"\n> \\1 better written as $1 at -e line 1.\n> hifoo\n\nVery good advice from a Perl expert; thanks.\n"},{"id":"319240","messageId":"CACBZZX7diKKuz0r2uWQsAmxDkWm_2Afi=zWSTRMYRwiobEGwpw@mail.gmail.com","threadId":"45883","inReplyTo":"xmqqd1bh1wqy.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] fixup! use perl instead of sed","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-09T23:44:09Z","receivedAt":"2017-05-09T23:44:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Wed, May 10, 2017 at 12:38 AM, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> On Tue, May 9, 2017 at 6:45 PM, Jonathan Tan wrote:\n>> ...\n>>> # Tweak the push output to make the push option outside the cert\n>>> # different, then replay it on a fresh dst, checking that ff is not\n>>> # deleted.\n>>> - sed -i \"s/\\\\([^ ]\\\\)bar/\\\\1baz/\" push &&\n>>> + perl -pe \"s/([^ ])bar/\\\\1baz/\" push >push.tweak &&\n>>> prepare_dst &&\n>>> git -C dst config receive.certnonceseed sekrit &&\n>>> git -C dst config receive.advertisepushoptions 1 &&\n>>> - git receive-pack dst out &&\n>>> + git receive-pack dst out &&\n>>\n>> The test should have a PERL prerequisite now, that's missing.\n>\n> For a single-liner like this, our stance has always been that t/\n> scripts can assume _some_ version of Perl interpreter available for\n> use, cf. t/README where it lists prerequisites and explains them:\n>\n> - PERL\n>\n> Git wasn't compiled with NO_PERL=YesPlease.\n>\n> Even without the PERL prerequisite, tests can assume there is a\n> usable perl interpreter at $PERL_PATH, though it need not be\n> particularly modern.\n>\n> So unless \"receive-pack\" that is being tested here requires Perl at\n> runtime, we do not want PERL prerequisite for this test.\n\nOops, sorry about that.\n\n>> Also using \\1 will likely be deprecated in future versions of perl at\n>> some point:\n>>\n>> $ echo hifoo | perl -wpe \"s/([^ ])bar/\\\\1baz/\"\n>> \\1 better written as $1 at -e line 1.\n>> hifoo\n>\n> Very good advice from a Perl expert; thanks.\n\nNo problem. Hopefully my noisy advice will at least lead to fixing a\nbug in perl if there is one :)\n"}]}