receive-pack: verify push options in cert
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. 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 <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jonathan Tan committed
May 9, 2017 at 12:23 UTC
cbaf82cc6b734244a9190c3341ca5f08f8d5acc4
3 files changed
+112
-8
Documentation/technical/pack-protocol.txt
+26
-6
@@ -468,13 +468,10 @@ that it wants to update, it sends a line listing the obj-id currently on
468
the server, the obj-id the client would like to update it to and the name
469
of the reference.
470
471
-This list is followed by a flush-pkt. Then the push options are transmitted
472
-one per packet followed by another flush-pkt. After that the packfile that
473
-should contain all the objects that the server will need to complete the new
474
-references will be sent.
471
+This list is followed by a flush-pkt.
472
473
----
477
- update-request = *shallow ( command-list | push-cert ) [packfile]
474
+ update-requests = *shallow ( command-list | push-cert )
475
476
shallow = PKT-LINE("shallow" SP obj-id)
477
@@ -495,12 +492,35 @@ references will be sent.
492
PKT-LINE("pusher" SP ident LF)
493
PKT-LINE("pushee" SP url LF)
494
PKT-LINE("nonce" SP nonce LF)
495
+ *PKT-LINE("push-option" SP push-option LF)
496
PKT-LINE(LF)
497
*PKT-LINE(command LF)
498
*PKT-LINE(gpg-signature-lines LF)
499
PKT-LINE("push-cert-end" LF)
500
503
- packfile = "PACK" 28*(OCTET)
501
+ push-option = 1*( VCHAR | SP )
502
+----
503
+
504
+If the server has advertised the 'push-options' capability and the client has
505
+specified 'push-options' as part of the capability list above, the client then
506
+sends its push options followed by a flush-pkt.
507
+
508
+----
509
+ push-options = *PKT-LINE(push-option) flush-pkt
510
+----
511
+
512
+For backwards compatibility with older Git servers, if the client sends a push
513
+cert and push options, it MUST send its push options both embedded within the
514
+push cert and after the push cert. (Note that the push options within the cert
515
+are prefixed, but the push options after the cert are not.) Both these lists
516
+MUST be the same, modulo the prefix.
517
+
518
+After that the packfile that
519
+should contain all the objects that the server will need to complete the new
520
+references will be sent.
521
+
522
+----
523
+ packfile = "PACK" 28*(OCTET)
524
----
525
526
If the receiving end does not support delete-refs, the sending end MUST
builtin/receive-pack.c
+49
-2
@@ -470,7 +470,8 @@ static char *prepare_push_cert_nonce(const char *path, unsigned long stamp)
470
* after dropping "_commit" from its name and possibly moving it out
471
* of commit.c
472
*/
473
-static char *find_header(const char *msg, size_t len, const char *key)
473
+static char *find_header(const char *msg, size_t len, const char *key,
474
+ const char **next_line)
475
{
476
int key_len = strlen(key);
477
const char *line = msg;
@@ -483,6 +484,8 @@ static char *find_header(const char *msg, size_t len, const char *key)
484
if (line + key_len < eol &&
485
!memcmp(line, key, key_len) && line[key_len] == ' ') {
486
int offset = key_len + 1;
487
+ if (next_line)
488
+ *next_line = *eol ? eol + 1 : eol;
489
return xmemdupz(line + offset, (eol - line) - offset);
490
}
491
line = *eol ? eol + 1 : NULL;
@@ -492,7 +495,7 @@ static char *find_header(const char *msg, size_t len, const char *key)
495
496
static const char *check_nonce(const char *buf, size_t len)
497
{
495
- char *nonce = find_header(buf, len, "nonce");
498
+ char *nonce = find_header(buf, len, "nonce", NULL);
499
unsigned long stamp, ostamp;
500
char *bohmac, *expect = NULL;
501
const char *retval = NONCE_BAD;
@@ -572,6 +575,45 @@ leave:
575
return retval;
576
}
577
578
+/*
579
+ * Return 1 if there is no push_cert or if the push options in push_cert are
580
+ * the same as those in the argument; 0 otherwise.
581
+ */
582
+static int check_cert_push_options(const struct string_list *push_options)
583
+{
584
+ const char *buf = push_cert.buf;
585
+ int len = push_cert.len;
586
+
587
+ char *option;
588
+ const char *next_line;
589
+ int options_seen = 0;
590
+
591
+ int retval = 1;
592
+
593
+ if (!len)
594
+ return 1;
595
+
596
+ while ((option = find_header(buf, len, "push-option", &next_line))) {
597
+ len -= (next_line - buf);
598
+ buf = next_line;
599
+ options_seen++;
600
+ if (options_seen > push_options->nr
601
+ || strcmp(option,
602
+ push_options->items[options_seen - 1].string)) {
603
+ retval = 0;
604
+ goto leave;
605
+ }
606
+ free(option);
607
+ }
608
+
609
+ if (options_seen != push_options->nr)
610
+ retval = 0;
611
+
612
+leave:
613
+ free(option);
614
+ return retval;
615
+}
616
+
617
static void prepare_push_cert_sha1(struct child_process *proc)
618
{
619
static int already_done;
@@ -1924,6 +1966,11 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
1966
1967
if (use_push_options)
1968
read_push_options(&push_options);
1969
+ if (!check_cert_push_options(&push_options)) {
1970
+ struct command *cmd;
1971
+ for (cmd = commands; cmd; cmd = cmd->next)
1972
+ cmd->error_string = "inconsistent push options";
1973
+ }
1974
1975
prepare_shallow_info(&si, &shallow);
1976
if (!si.nr_ours && !si.nr_theirs)
t/t5534-push-signed.sh
+37
@@ -124,6 +124,43 @@ test_expect_success GPG 'signed push sends push certificate' '
124
test_cmp expect dst/push-cert-status
125
'
126
127
+test_expect_success GPG 'inconsistent push options in signed push not allowed' '
128
+ # First, invoke receive-pack with dummy input to obtain its preamble.
129
+ prepare_dst &&
130
+ git -C dst config receive.certnonceseed sekrit &&
131
+ git -C dst config receive.advertisepushoptions 1 &&
132
+ printf xxxx | test_might_fail git receive-pack dst >preamble &&
133
+
134
+ # Then, invoke push. Simulate a receive-pack that sends the preamble we
135
+ # obtained, followed by a dummy packet.
136
+ write_script myscript <<-\EOF &&
137
+ cat preamble &&
138
+ printf xxxx &&
139
+ cat >push
140
+ EOF
141
+ test_might_fail git push --push-option="foo" --push-option="bar" \
142
+ --receive-pack="\"$(pwd)/myscript\"" --signed dst --delete ff &&
143
+
144
+ # Replay the push output on a fresh dst, checking that ff is truly
145
+ # deleted.
146
+ prepare_dst &&
147
+ git -C dst config receive.certnonceseed sekrit &&
148
+ git -C dst config receive.advertisepushoptions 1 &&
149
+ git receive-pack dst <push &&
150
+ test_must_fail git -C dst rev-parse ff &&
151
+
152
+ # Tweak the push output to make the push option outside the cert
153
+ # different, then replay it on a fresh dst, checking that ff is not
154
+ # deleted.
155
+ perl -pe "s/([^ ])bar/\$1baz/" push >push.tweak &&
156
+ prepare_dst &&
157
+ git -C dst config receive.certnonceseed sekrit &&
158
+ git -C dst config receive.advertisepushoptions 1 &&
159
+ git receive-pack dst <push.tweak >out &&
160
+ git -C dst rev-parse ff &&
161
+ grep "inconsistent push options" out
162
+'
163
+
164
test_expect_success GPG 'fail without key and heed user.signingkey' '
165
prepare_dst &&
166
mkdir -p dst/.git/hooks &&