Clarify interaction between signed pushes and push options

In commit c714e45 ("receive-pack: implement advertising and receiving push options", 2016-07-14), receive-pack was taught to (among other things) advertise that it understood push options, depending on configuration. It was documented that it advertised such ability by default; however, it actually does not. (In that commit, notice that advertise_push_options defaults to 0, unlike advertise_atomic_push which defaults to 1.)

Update the documentation to state that it does not advertise the ability by default.

Signed-off-by Jonathan Tan <[email protected]>

---
 Documentation/config.txt | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Documentation/config.txt +2 -3

index 475e874d5..f49a2f3cb 100644
@@ -2620,9 +2620,8 @@ receive.advertiseAtomic::
 	capability, set this variable to false.
 
 receive.advertisePushOptions::
-	By default, git-receive-pack will advertise the push options
-	capability to its clients. If you don't want to advertise this
-	capability, set this variable to false.
+	When set to true, git-receive-pack will advertise the push options
+	capability to its clients.
 
 receive.autogc::
 	By default, git-receive-pack will run "git-gc --auto" after

Signature · 1 line

2.13.0.rc1.294.g07d810a77f-goog

Quoted 7 lines · Jonathan Tan

In commit c714e45 ("receive-pack: implement advertising and receiving push options", 2016-07-14), receive-pack was taught to (among other things) advertise that it understood push options, depending on configuration. It was documented that it advertised such ability by default; however, it actually does not. (In that commit, notice that advertise_push_options defaults to 0, unlike advertise_atomic_push which defaults to 1.)

This looks like a good fix to the documentation as advertise_push_options does indeed default to 0.

In commit c714e45 ("receive-pack: implement advertising and receiving push options", 2016-07-14), receive-pack was taught to (among other things) advertise that it understood push options, depending on configuration. It was documented that it advertised such ability by default; however, it actually does not. (In that commit, notice that advertise_push_options defaults to 0, unlike advertise_atomic_push which defaults to 1.)

Update the documentation to state that it does not advertise the ability by default.

Signed-off-by Jonathan Tan <[email protected]>

---
 Documentation/config.txt | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Documentation/config.txt +0 -0

index 475e874d5..f49a2f3cb 100644

In commit f6a4e61 ("push: accept push options", 2016-07-14), send-pack was taught to include push options both within the signed cert (if the push is a signed push) and outside the signed cert; however, receive-pack ignores push options within the cert, only handling push options outside the cert.

Teach receive-pack, in the case that push options are provided for a signed push, to verify that the push options both within the cert and outside the cert are consistent, and to provide the results of that verification to the receive hooks as an environment variable (just like it currently does for the nonce verification).

This sets in stone the requirement that send-pack redundantly send its push options in 2 places, but I think that this is better than the alternatives. Sending push options only within the cert is backwards-incompatible with existing Git servers (which read push options only from outside the cert), and sending push options only outside the cert means that the push options are not signed for.

Signed-off-by Jonathan Tan <[email protected]>

---
 Documentation/git-receive-pack.txt | 10 ++++++++
 builtin/receive-pack.c             | 49 ++++++++++++++++++++++++++++++++++----
 t/t5534-push-signed.sh             | 15 ++++++++++++
 3 files changed, 70 insertions(+), 4 deletions(-)

Documentation/git-receive-pack.txt +10 -0

index 86a4b32f0..f50ca0f29 100644
@@ -106,6 +106,16 @@ the following environment variables:
        Also read about `receive.certNonceSlop` variable in
        linkgit:git-config[1].
 
+`GIT_PUSH_CERT_OPTION_STATUS`::
+`BAD`;;
+       For backwards compatibility reasons, when accepting a signed
+       push, receive-pack requires that push options be written both
+       inside and outside the certificate. ("git push" does this
+       automatically.) `BAD` is returned if they are inconsistent.
+`OK`;;
+       The push options inside and outside the certificate are
+       consistent.
+
 This hook is called before any refname is updated and before any
 fast-forward checks are performed.
 

builtin/receive-pack.c +45 -4

index f96834f42..fe26c2f72 100644
@@ -81,6 +81,9 @@ static long nonce_stamp_slop;
 static unsigned long nonce_stamp_slop_limit;
 static struct ref_transaction *transaction;
 
+static const char *PUSH_OPTION_BAD = "BAD";
+static const char *PUSH_OPTION_OK = "OK";
+
 static enum {
        KEEPALIVE_NEVER = 0,
        KEEPALIVE_AFTER_NUL,

@@ -473,7 +476,8 @@ static char *prepare_push_cert_nonce(const char *path, unsigned long stamp)
  * after dropping "_commit" from its name and possibly moving it out
  * of commit.c
  */
-static char *find_header(const char *msg, size_t len, const char *key)
+static char *find_header(const char *msg, size_t len, const char *key,
+                        const char **next_line)
 {
        int key_len = strlen(key);
        const char *line = msg;

@@ -486,6 +490,8 @@ static char *find_header(const char *msg, size_t len, const char *key)
                if (line + key_len < eol &&
                    !memcmp(line, key, key_len) && line[key_len] == ' ') {
                        int offset = key_len + 1;
+                       if (next_line)
+                               *next_line = *eol ? eol + 1 : eol;
                        return xmemdupz(line + offset, (eol - line) - offset);
                }
                line = *eol ? eol + 1 : NULL;

@@ -495,7 +501,7 @@ static char *find_header(const char *msg, size_t len, const char *key)
 
 static const char *check_nonce(const char *buf, size_t len)
 {
-       char *nonce = find_header(buf, len, "nonce");
+       char *nonce = find_header(buf, len, "nonce", NULL);
        unsigned long stamp, ostamp;
        char *bohmac, *expect = NULL;
        const char *retval = NONCE_BAD;

@@ -575,9 +581,40 @@ static const char *check_nonce(const char *buf, size_t len)
        return retval;
 }
 
-static void prepare_push_cert_sha1(struct child_process *proc)
+static const char *check_push_option(const char *buf, size_t len,
+                                    const struct string_list *push_options)
+{
+       int options_seen = 0;
+       char *option;
+       const char *next_line;
+       const char *retval = PUSH_OPTION_OK;
+
+       while ((option = find_header(buf, len, "push-option", &next_line))) {
+               len -= (next_line - buf);
+               buf = next_line;
+               options_seen++;
+               if (options_seen > push_options->nr
+                   || strcmp(option,
+                             push_options->items[options_seen - 1].string)) {
+                       retval = PUSH_OPTION_BAD;
+                       goto leave;
+               }
+               free(option);
+       }
+
+       if (options_seen != push_options->nr)
+               retval = PUSH_OPTION_BAD;
+
+leave:
+       free(option);
+       return retval;
+}
+
+static void prepare_push_cert_sha1(struct child_process *proc,
+                                  const struct string_list *push_options)
 {
        static int already_done;
+       static const char *push_option_status;
 
        if (!push_cert.len)
                return;

@@ -609,6 +646,8 @@ static void prepare_push_cert_sha1(struct child_process *proc)
                strbuf_release(&gpg_output);
                strbuf_release(&gpg_status);
                nonce_status = check_nonce(push_cert.buf, bogs);
+               push_option_status = check_push_option(push_cert.buf, bogs,
+                                                      push_options);
        }
        if (!is_null_sha1(push_cert_sha1)) {
                argv_array_pushf(&proc->env_array, "GIT_PUSH_CERT=%s",

@@ -631,6 +670,8 @@ static void prepare_push_cert_sha1(struct child_process *proc)
                                                 "GIT_PUSH_CERT_NONCE_SLOP=%ld",
                                                 nonce_stamp_slop);
                }
+               argv_array_pushf(&proc->env_array, "GIT_PUSH_CERT_OPTION_STATUS=%s",
+                                push_option_status);
        }
 }
 

@@ -683,7 +724,7 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,
                proc.err = muxer.in;
        }
 
-       prepare_push_cert_sha1(&proc);
+       prepare_push_cert_sha1(&proc, feed_state->push_options);
 
        code = start_command(&proc);
        if (code) {

t/t5534-push-signed.sh +5 -0

index ecb8d446a..2607b8797 100755
@@ -124,6 +124,21 @@ test_expect_success GPG 'signed push sends push certificate' '
        test_cmp expect dst/push-cert-status
 '
 
+test_expect_success GPG 'signed push reports push option status in receive hook' '
+       prepare_dst &&
+       mkdir -p dst/.git/hooks &&
+       git -C dst config receive.certnonceseed sekrit &&
+       git -C dst config receive.advertisepushoptions 1 &&

Quoted 12 lines · Jonathan Tan

In commit f6a4e61 ("push: accept push options", 2016-07-14), send-pack was taught to include push options both within the signed cert (if the push is a signed push) and outside the signed cert; however, receive-pack ignores push options within the cert, only handling push options outside the cert.

Teach receive-pack, in the case that push options are provided for a signed push, to verify that the push options both within the cert and outside the cert are consistent, and to provide the results of that verification to the receive hooks as an environment variable (just like it currently does for the nonce verification).

Quoted 6 lines

This sets in stone the requirement that send-pack redundantly send its push options in 2 places, but I think that this is better than the alternatives. Sending push options only within the cert is backwards-incompatible with existing Git servers (which read push options only from outside the cert), and sending push options only outside the cert means that the push options are not signed for.

As the combination of push certs and push options are broken (at least when using JGit as well, which are used in Gerrit installations), I would not feel too bad about actually going the backwards-incompatible way.

Quoted 6 lines · Jonathan Tan

This sets in stone the requirement that send-pack redundantly send its push options in 2 places, but I think that this is better than the alternatives. Sending push options only within the cert is backwards-incompatible with existing Git servers (which read push options only from outside the cert), and sending push options only outside the cert means that the push options are not signed for.

As the combination of push certs and push options are broken (at least when using JGit as well, which are used in Gerrit installations), I would not feel too bad about actually going the backwards-incompatible way.

In commit f6a4e61 ("push: accept push options", 2016-07-14), send-pack was taught to include push options both within the signed cert (if the push is a signed push) and outside the signed cert; however, receive-pack ignores push options within the cert, only handling push options outside the cert.

Support for push options in the receive-pack protocol (and all Git components that speak it) have been added over a few commits, but not fully documented (especially its interaction with signed pushes). Update the protocol documentation to include the relevant details.

Signed-off-by Jonathan Tan <[email protected]>

---
 Documentation/technical/pack-protocol.txt | 32 +++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

Documentation/technical/pack-protocol.txt +3 -5

index 5b0ba3ef2..cf7cb48c3 100644
@@ -473,13 +473,10 @@ that it wants to update, it sends a line listing the obj-id currently on
 the server, the obj-id the client would like to update it to and the name
 of the reference.
 
-This list is followed by a flush-pkt. Then the push options are transmitted
-one per packet followed by another flush-pkt. After that the packfile that
-should contain all the objects that the server will need to complete the new
-references will be sent.
+This list is followed by a flush-pkt.
 
 ----
-  update-request    =  *shallow ( command-list | push-cert ) [packfile]
+  update-request    =  *shallow ( command-list | push-cert )
 
   shallow           =  PKT-LINE("shallow" SP obj-id)
 

@@ -500,12 +497,35 @@ references will be sent.
                      PKT-LINE("pusher" SP ident LF)
                      PKT-LINE("pushee" SP url LF)
                      PKT-LINE("nonce" SP nonce LF)
+                     *PKT-LINE("push-option" SP push-option LF)
                      PKT-LINE(LF)
                      *PKT-LINE(command LF)

Documentation/technical/pack-protocol.txt +7 -1

+
+After that the packfile that
+should contain all the objects that the server will need to complete the new
+references will be sent.
+
+----
+  packfile          =  "PACK" 28*(OCTET)
 ----
 
 If the receiving end does not support delete-refs, the sending end MUST
--

2.13.0.rc1.294.g07d810a77f-goog

Support for push options in the receive-pack protocol (and all Git components that speak it) have been added over a few commits, but not fully documented (especially its interaction with signed pushes). Update the protocol documentation to include the relevant details.

Documentation/technical/pack-protocol.txt +4 -1

@@ -500,12 +497,35 @@ references will be sent.
 		      PKT-LINE("pusher" SP ident LF)
 		      PKT-LINE("pushee" SP url LF)
 		      PKT-LINE("nonce" SP nonce LF)
+		      *PKT-LINE("push-option" SP push-option LF)
 		      PKT-LINE(LF)
 		      *PKT-LINE(command LF)
 		      *PKT-LINE(gpg-signature-lines LF)
 		      PKT-LINE("push-cert-end" LF)
 
-  packfile          = "PACK" 28*(OCTET)
+  push-option       =  1*CHAR
+----
+

Documentation/technical/pack-protocol.txt that it wants to update, it sends a line listing the obj-id currently on · line 476

Quoted 5 lines

Documentation/technical/pack-protocol.txt +1 -4

-This list is followed by a flush-pkt. Then the push options are transmitted
-one per packet followed by another flush-pkt. After that the packfile that
-should contain all the objects that the server will need to complete the new
-references will be sent.
+This list is followed by a flush-pkt.

I think this removed more than intended.

Before c714e45f (receive-pack: implement advertising and receiving push options, 2016-07-14), this said

This list is followed by a flush-pkt and then the packfile that should contain all the objects that the server will need to complete the new references.

which I think would work fine.

[...]

Documentation/technical/pack-protocol.txt that it wants to update, it sends a line listing the obj-id currently on · line 479

Quoted 2 lines

Documentation/technical/pack-protocol.txt +1 -1

-  update-request    =  *shallow ( command-list | push-cert ) [packfile]
+  update-request    =  *shallow ( command-list | push-cert )

This seems confusing to me both before and after. How does update-request get used? Is it supposed to include the packfile or not?

Where do push-options fit into an unsigned request?

[...]

Quoted 14 lines

Documentation/technical/pack-protocol.txt +4 -1

@@ -500,12 +497,35 @@ references will be sent.
 		      PKT-LINE("pusher" SP ident LF)
 		      PKT-LINE("pushee" SP url LF)
 		      PKT-LINE("nonce" SP nonce LF)
+		      *PKT-LINE("push-option" SP push-option LF)
 		      PKT-LINE(LF)
 		      *PKT-LINE(command LF)
 		      *PKT-LINE(gpg-signature-lines LF)
 		      PKT-LINE("push-cert-end" LF)
 
-  packfile          = "PACK" 28*(OCTET)
+  push-option       =  1*CHAR
+----
+

Documentation/technical/pack-protocol.txt references will be sent. · line 518

Quoted 10 lines

Documentation/technical/pack-protocol.txt +10 -0

+If the server has advertised the 'push-options' capability and the client has
+specified 'push-options' as part of the capability list above, the client then
+sends its push options followed by a flush-pkt.
+
+----
+  push-options      =  *PKT-LINE(push-option) flush-pkt
+----
+
+For backwards compatibility with older Git servers, if the client sends a push
+cert and push options, it SHOULD send its push options both embedded within the

This needs to be a MUST, not SHOULD.

Documentation/technical/pack-protocol.txt references will be sent. · line 521

Quoted 3 lines

Documentation/technical/pack-protocol.txt +3 -0

+push cert and after the push cert. (Note that the push options within the cert
+are prefixed, but the push options after the cert are not.) Both these lists
+SHOULD be consistent.

Likewise this one.

What does it mean to be consistent?

We noticed this when trying to use Git to make a signed push with push options to a server using JGit (which rejects such pushes because the Git client makes requests that are, strictly speaking, incompatible with the documented protocol).

There have been several commits (see the commits linked in the commit messages of the patches sent in reply to this e-mail) to support push options (that are read by receive hooks) when using "git push", but the interaction between push options and signed pushes are not very clear. Here are some patches (containing both code and documentation) that clarify this interaction.

I would appreciate a review of this - if we could make the protocol clear, we could then update JGit to use the updated protocol and be no longer incompatible with existing Git clients.

Jonathan Tan (3):
docs: correct receive.advertisePushOptions default
receive-pack: verify push options in cert protocol docs: explain receive-pack push options

 Documentation/config.txt                  |  5 ++--
 Documentation/git-receive-pack.txt        | 10 +++++++
 Documentation/technical/pack-protocol.txt | 32 ++++++++++++++++----
 builtin/receive-pack.c                    | 49 ++++++++++++++++++++++++++++---
 t/t5534-push-signed.sh                    | 15 ++++++++++
 5 files changed, 98 insertions(+), 13 deletions(-)

Teach receive-pack, in the case that push options are provided for a signed push, to verify that the push options both within the cert and outside the cert are consistent.

This sets in stone the requirement that send-pack redundantly send its push options in 2 places,...

Signed-off-by Jonathan Tan <[email protected]>

---

Thanks - I didn't realize the existence of the test lint. Here's a fixup. Let me know if you prefer a full reroll.

I had to use perl because "push" is a binary file (the first line contains a NUL).

 t/t5534-push-signed.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

t/t5534-push-signed.sh +0 -0

index 0ef6f66b5..3ee58dafb 100755

t/t5534-push-signed.sh +0 -1

        git -C dst rev-parse ff &&
        grep "inconsistent push options" out
 '
--

2.13.0.rc2.291.g57267f2277-goog

Finally, you can just use -i like you did with sed, no need for the tempfile:

     $ echo hibar >push
     $ perl -pi -e 's/([^ ])bar/$1baz/' push
     $ cat push
     hibaz

Am 09.05.2017 um 19:00 schrieb Ævar Arnfjörð Bjarmason:

Quoted 2 lines

Finally, you can just use -i like you did with sed, no need for the tempfile:

Nope. Some implementations of perl attempt to remove the file that it has just opened. That doesn't work on Windows. You have to supply a backup file name as in perl -i.bak ... :-(

Quoted 1 line

Quoted 4 lines

     $ echo hibar >push
     $ perl -pi -e 's/([^ ])bar/$1baz/' push
     $ cat push
     hibaz

-- Hannes

Am 09.05.2017 um 19:00 schrieb Ævar Arnfjörð Bjarmason:

Quoted 3 lines

Finally, you can just use -i like you did with sed, no need for the tempfile:

Nope. Some implementations of perl attempt to remove the file that it has just opened. That doesn't work on Windows. You have to supply a backup file name as in perl -i.bak ... :-(

Quoted 5 lines

     $ echo hibar >push
     $ perl -pi -e 's/([^ ])bar/$1baz/' push
     $ cat push
     hibaz

-- Hannes

On Tue, May 9, 2017 at 6:45 PM, Jonathan Tan <[email protected]> wrote:
...

t/t5534-push-signed.sh test_expect_success GPG 'inconsistent push options in signed push not allowed' ' · line 159

Quoted 10 lines

# Tweak the push output to make the push option outside the cert # different, then replay it on a fresh dst, checking that ff is not # deleted.

t/t5534-push-signed.sh +2 -2

-       sed -i "s/\\([^ ]\\)bar/\\1baz/" push &&
+       perl -pe "s/([^ ])bar/\\1baz/" push >push.tweak &&
        prepare_dst &&
        git -C dst config receive.certnonceseed sekrit &&
        git -C dst config receive.advertisepushoptions 1 &&
-       git receive-pack dst <push >out &&
+       git receive-pack dst <push.tweak >out &&

The test should have a PERL prerequisite now, that's missing.

Also using \1 will likely be deprecated in future versions of perl at some point:

    $ echo hifoo | perl -wpe "s/([^ ])bar/\\1baz/"
    \1 better written as $1 at -e line 1.
    hifoo

Quoted 13 lines · Ævar Arnfjörð Bjarmason

On Tue, May 9, 2017 at 6:45 PM, Jonathan Tan <[email protected]> wrote:
...

t/t5534-push-signed.sh test_expect_success GPG 'inconsistent push options in signed push not allowed' ' · line 159

Quoted 10 lines

# Tweak the push output to make the push option outside the cert # different, then replay it on a fresh dst, checking that ff is not # deleted.

t/t5534-push-signed.sh +2 -2

-       sed -i "s/\\([^ ]\\)bar/\\1baz/" push &&
+       perl -pe "s/([^ ])bar/\\1baz/" push >push.tweak &&
        prepare_dst &&
        git -C dst config receive.certnonceseed sekrit &&
        git -C dst config receive.advertisepushoptions 1 &&
-       git receive-pack dst <push >out &&
+       git receive-pack dst <push.tweak >out &&

Quoted 1 line

The test should have a PERL prerequisite now, that's missing.

For a single-liner like this, our stance has always been that t/ scripts can assume _some_ version of Perl interpreter available for use, cf. t/README where it lists prerequisites and explains them:

- PERL

Git wasn't compiled with NO_PERL=YesPlease.

Even without the PERL prerequisite, tests can assume there is a usable perl interpreter at $PERL_PATH, though it need not be particularly modern.

So unless "receive-pack" that is being tested here requires Perl at runtime, we do not want PERL prerequisite for this test.

Quoted 6 lines

Also using \1 will likely be deprecated in future versions of perl at some point:

    $ echo hifoo | perl -wpe "s/([^ ])bar/\\1baz/"
    \1 better written as $1 at -e line 1.
    hifoo

Very good advice from a Perl expert; thanks.