{"thread":{"id":"38326","subject":"[RFC/PATCH] receive-pack.c: only accept push-cert if push_cert_nonce was advertised","startedAt":"2015-01-09T20:47:17Z","lastAt":"2015-01-14T18:08:04Z","messageCount":11,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"254512","messageId":"1420836437-11068-1-git-send-email-sbeller@google.com","threadId":"38326","inReplyTo":null,"subject":"[RFC/PATCH] receive-pack.c: only accept push-cert if push_cert_nonce was advertised","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-09T20:47:17Z","receivedAt":"2015-01-09T20:47:17Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"If the server did not advertise the capability to have signed pushes\nit should not accept signed pushes as stated in\nDocumentation/technical/protocol-capabilities.txt:\n\n Client will then send a space separated list of capabilities it wants\n to be in effect. The client MUST NOT ask for capabilities the server\n did not say it supports.\n\n Server MUST diagnose and abort if capabilities it does not understand\n was sent. Server MUST NOT ignore capabilities that client requested\n and server advertised. As a consequence of these rules, server MUST\n NOT advertise capabilities it does not understand.\n\nAfter rereading the second paragraph I think they should also be reworded to\n\n Server MUST diagnose and abort if capabilities it did not advertise\n was sent.\n\n\nSuppose there would be hypothetical flaw in the capability of signed\npushes (or any capability for the current reasoning) which may harm\nthe server. This would require a bugfix release if it was severe and\nwould put us on time pressure to get it done.\n\nA change like the one proposed would allow us to tell the community to\nsimply configure the server to not advertise the feature and if not\nadvertised the flaw could not be abused.\n\nI am not saying there is a problem now, but I am rather saying patches\nsimilar to this one would buy us time in case of problems arising.\n\nSigned-off-by: Stefan Beller \n---\n\nNotes:\n As I discovered the idea while composing the\n atomic push series and the changes line of this\n patch is closeby, this applies on top of\n origin/sb/atomic-push (v12 as sent on Jan. 7th)\n \n This patch is RFC, thinking about security best practice.\n It's not enough to document the intended behavior in\n Documentation/technical/protocol-capabilities.txt, but\n rather enforce it in the code as well.\n \n Any thoughts on that welcome!\n \n Thanks,\n Stefan\n\n builtin/receive-pack.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 4c069c5..628d13a 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1276,7 +1276,8 @@ static struct command *read_head_info(struct sha1_array *shallow)\n \t\t\t\tuse_atomic = 1;\n \t\t}\n \n-\t\tif (!strcmp(line, \"push-cert\")) {\n+\t\tif (push_cert_nonce &&\n+\t\t !strcmp(line, \"push-cert\")) {\n \t\t\tint true_flush = 0;\n \t\t\tchar certbuf[1024];\n \n-- \n2.2.1.62.g3f15098\n"},{"id":"254515","messageId":"xmqqsifjbmu2.fsf@gitster.dls.corp.google.com","threadId":"38326","inReplyTo":"1420836437-11068-1-git-send-email-sbeller@google.com","subject":"Re: [RFC/PATCH] receive-pack.c: only accept push-cert if push_cert_nonce was advertised","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-09T22:39:33Z","receivedAt":"2015-01-09T22:39:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> If the server did not advertise the capability to have signed pushes\n> it should not accept signed pushes as stated in\n> Documentation/technical/protocol-capabilities.txt:\n>\n> Client will then send a space separated list of capabilities it wants\n> to be in effect. The client MUST NOT ask for capabilities the server\n> did not say it supports.\n>\n> Server MUST diagnose and abort if capabilities it does not understand\n> was sent. Server MUST NOT ignore capabilities that client requested\n> and server advertised. As a consequence of these rules, server MUST\n> NOT advertise capabilities it does not understand.\n>\n> After rereading the second paragraph I think they should also be reworded to\n>\n> Server MUST diagnose and abort if capabilities it did not advertise\n> was sent.\n\nExcept for s/was sent/was requested/, I think that rule makes sense\nvery much.\n\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 4c069c5..628d13a 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -1276,7 +1276,8 @@ static struct command *read_head_info(struct sha1_array *shallow)\n> \t\t\t\tuse_atomic = 1;\n> \t\t}\n> \n> -\t\tif (!strcmp(line, \"push-cert\")) {\n> +\t\tif (push_cert_nonce &&\n> +\t\t !strcmp(line, \"push-cert\")) {\n> \t\t\tint true_flush = 0;\n> \t\t\tchar certbuf[1024];\n\nThis implementation is somewhat questionable.\n\nThe server knows how to parse \"push-cert\" line, knows that what\nfollows after that line up to \"push-cert-end\" line are shaped very\ndifferently from protocol commands outside the push-cert block. In\nother words, it knows how to parse the request meant for the capable\nserver; it just wants to refuse to serve that request.\n\nThe patched code will make it fail by hoping that queue_command()\nthat only handles \"40-hex 40-hex ref\" will reject the line that\nbegins with \"push-cert\". Instead of relying on such a hidden\ndependency, wouldn't it be cleaner to actually parse the push-cert\nblock and then at the end notice and explictly say \"Your requests\nwere syntactically correct, but I am not going to honor your request\nto use the push-cert extension, because I never told you that I'd\noffer you that capability\", instead of rejecting the request with \"I\nwas expecting old/new/ref but you sent a line with 'push-cert' on\nit; what are you talking about?\"\n"},{"id":"254518","messageId":"xmqqfvbjblmd.fsf@gitster.dls.corp.google.com","threadId":"38326","inReplyTo":"xmqqsifjbmu2.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH] receive-pack.c: only accept push-cert if push_cert_nonce was advertised","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-09T23:05:46Z","receivedAt":"2015-01-09T23:05:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n>> index 4c069c5..628d13a 100644\n>> --- a/builtin/receive-pack.c\n>> +++ b/builtin/receive-pack.c\n>> @@ -1276,7 +1276,8 @@ static struct command *read_head_info(struct sha1_array *shallow)\n>> \t\t\t\tuse_atomic = 1;\n>> \t\t}\n>> \n>> -\t\tif (!strcmp(line, \"push-cert\")) {\n>> +\t\tif (push_cert_nonce &&\n>> +\t\t !strcmp(line, \"push-cert\")) {\n>> \t\t\tint true_flush = 0;\n>> \t\t\tchar certbuf[1024];\n>\n> This implementation is somewhat questionable.\n>\n> The server knows how to parse \"push-cert\" line, knows that what\n> follows after that line up to \"push-cert-end\" line are shaped very\n> differently from protocol commands outside the push-cert block. In\n> other words, it knows how to parse the request meant for the capable\n> server; it just wants to refuse to serve that request.\n>\n> The patched code will make it fail by hoping that queue_command()\n> that only handles \"40-hex 40-hex ref\" will reject the line that\n> begins with \"push-cert\". Instead of relying on such a hidden\n> dependency, wouldn't it be cleaner to actually parse the push-cert\n> block and then at the end notice and explictly say \"Your requests\n> were syntactically correct, but I am not going to honor your request\n> to use the push-cert extension, because I never told you that I'd\n> offer you that capability\", instead of rejecting the request with \"I\n> was expecting old/new/ref but you sent a line with 'push-cert' on\n> it; what are you talking about?\"\n\nNote that I said \"somewhat\" questionable, not \"horribly broken\",\nbecause another part of me wants to say that this implementation may\nbe better than \"parse and reject\" from security point of view. The\npusher cannot tell if the push failed because the server is old to\nknow about the extension, or the server is new but is refusing, by\nobserving the failure.\n\nBut the other side of the same coin is that it makes it harder to\ndiagnose the failure.\n\nWhen the protocol exchange gets to this state, in practice, we know\nwe are talking with somebody who has push privilege into the\nrepository, so revealing a bit more about the server by taking\n\"parse and reject\" route may be a reasonable price to give diagnosis\nthat is more useful to help the users in this case, though.\n"},{"id":"254519","messageId":"CAGZ79kZhUZBU886UkqbAXZwSO+xE90FyEqcJYB+AQZ9Aa_c9xg@mail.gmail.com","threadId":"38326","inReplyTo":"xmqqsifjbmu2.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH] receive-pack.c: only accept push-cert if push_cert_nonce was advertised","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-09T23:15:17Z","receivedAt":"2015-01-09T23:15:17Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jan 9, 2015 at 2:39 PM, Junio C Hamano wrote:\n> The patched code will make it fail by hoping that queue_command()\n> that only handles \"40-hex 40-hex ref\" will reject the line that\n> begins with \"push-cert\". Instead of relying on such a hidden\n> dependency, wouldn't it be cleaner to actually parse the push-cert\n> block and then at the end notice and explictly say \"Your requests\n> were syntactically correct, but I am not going to honor your request\n> to use the push-cert extension, because I never told you that I'd\n> offer you that capability\", instead of rejecting the request with \"I\n> was expecting old/new/ref but you sent a line with 'push-cert' on\n> it; what are you talking about?\"\n\nWhen reading the documentation at first I understood it the way:\n\"If I did not advertise the feature, I am expected to pretend it\ndoesn't even exist\". That lead me to the implementation as\nproposed.\n\nYour proposal to acknowledge the correctness of the message leads\nto more questions. How would we proceed? We would not accept the\nfeatured capability, but we could still do a normal push operation. This\nwould not be desired by a authentic user, so aborting the whole push\nprocess would be ok.\n\nI expect such behavior only from malicious clients which actively want\nto abuse a feature which wasn't advertised, so I wouldn't mind being\nslightly dishonest to the client and pretend we don't know that capability.\nBut relying on the hidden dependency is bad indeed. So maybe just\nanswer more rudely \"I did not advertise one of the capability you\nrequested, go away! (closes connection)\".\n\n\n> But the other side of the same coin is that it makes it harder to\n> diagnose the failure.\n\nThe push-cert case is special compared to other capabilities as it\nis quite intrusive to the protocol compared to say \"quiet\", so more\ndiagnosis may be helpful.\n\n> When the protocol exchange gets to this state, in practice, we know\n> we are talking with somebody who has push privilege into the\n> repository,\n\nYeah but what is one repository compared to the whole server?\n(Just painting the devil on the wall now:) Say you could abuse one\ncapability to gain access to the server or create a huge memory\nallocation such that the server becomes unresponsive for others.\n\nI know my argument is weak for the \"parse and reject route\", so\nI will see if I can adjust the patch to give diagnosis but still reject\nthe un-advertised capability.\n"},{"id":"254520","messageId":"xmqqbnm7bj8m.fsf@gitster.dls.corp.google.com","threadId":"38326","inReplyTo":"CAGZ79kZhUZBU886UkqbAXZwSO+xE90FyEqcJYB+AQZ9Aa_c9xg@mail.gmail.com","subject":"Re: [RFC/PATCH] receive-pack.c: only accept push-cert if push_cert_nonce was advertised","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-09T23:57:13Z","receivedAt":"2015-01-09T23:57:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Your proposal to acknowledge the correctness of the message leads\n> to more questions. How would we proceed?\n\nHow would it fail if we pretend that \"push-cert\" line had to be\nold/new/ref line? Failing the same way, but with a better\ndiagnosis, would be sufficient.\n\n> I expect such behavior only from malicious clients which actively\n> want to abuse a feature which wasn't advertised,...\n\nDo not assume malice; it is not 2005 anymore. You have to remember\nthat we are mature enough that there are many reimplementations of\nGit, all of which (us included ;-) start with a buggy version.\n\n>> When the protocol exchange gets to this state, in practice, we know\n>> we are talking with somebody who has push privilege into the\n>> repository,\n>\n> Yeah but what is one repository compared to the whole server?\n\nHuh? If an auth good enough for one repository allows things to\nanother repository, then I consider that to that other repository\nthe pusher also has push privilege. So what is the problem?\n\nBut again, our first version could just be \"pretend we do not know\nanything about push-cert\", with discussions on alternative\nconsidered in its log message. I do not think it is a blocker to\nlack the \"more helpful diagnosis\" feature.\n"},{"id":"254521","messageId":"1420849874-32013-1-git-send-email-sbeller@google.com","threadId":"38326","inReplyTo":"xmqqbnm7bj8m.fsf@gitster.dls.corp.google.com","subject":"[PATCH] receive-pack.c: don't miss exporting unsolicited push certificates","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-10T00:31:14Z","receivedAt":"2015-01-10T00:31:14Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"If the server is configured to not advertise push certificates,\na push certificate that gets pushed nevertheless will not be fully\nrecorded because push_cert_nonce is NULL.\n\nThe recording of GIT_PUSH_CERT_NONCE_STATUS should be dependent on\nthe status being there instead of push_cert_nonce being non NULL.\n\nWithout this patch an unsolicited nonce never makes to the stage when\nbeing exported with GIT_PUSH_CERT_NONCE_STATUS, because in the unsolicited\ncase push_cert_nonce is always NULL.\n\nSigned-off-by: Stefan Beller \n---\n builtin/receive-pack.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 628d13a..0e4878e 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -504,18 +504,18 @@ static void prepare_push_cert_sha1(struct child_process *proc)\n \t\t\t\t sigcheck.key ? sigcheck.key : \"\");\n \t\targv_array_pushf(&proc->env_array, \"GIT_PUSH_CERT_STATUS=%c\",\n \t\t\t\t sigcheck.result);\n-\t\tif (push_cert_nonce) {\n+\t\tif (push_cert_nonce)\n \t\t\targv_array_pushf(&proc->env_array,\n \t\t\t\t\t \"GIT_PUSH_CERT_NONCE=%s\",\n \t\t\t\t\t push_cert_nonce);\n+\t\tif (nonce_status)\n \t\t\targv_array_pushf(&proc->env_array,\n \t\t\t\t\t \"GIT_PUSH_CERT_NONCE_STATUS=%s\",\n \t\t\t\t\t nonce_status);\n-\t\t\tif (nonce_status == NONCE_SLOP)\n-\t\t\t\targv_array_pushf(&proc->env_array,\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\tif (nonce_status == NONCE_SLOP)\n+\t\t\targv_array_pushf(&proc->env_array,\n+\t\t\t\t\t \"GIT_PUSH_CERT_NONCE_SLOP=%ld\",\n+\t\t\t\t\t nonce_stamp_slop);\n \t}\n }\n \n-- \n2.2.1.62.g3f15098\n"},{"id":"254522","messageId":"xmqqmw5r9zck.fsf@gitster.dls.corp.google.com","threadId":"38326","inReplyTo":"1420849874-32013-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] receive-pack.c: don't miss exporting unsolicited push certificates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-10T01:52:11Z","receivedAt":"2015-01-10T01:52:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> If the server is configured to not advertise push certificates,\n> a push certificate that gets pushed nevertheless will not be fully\n> recorded because push_cert_nonce is NULL.\n\nSorry, but I do not quite see what you are trying to get at.\n\nWhen we did not advertise that this feature is supported, we know\nthe incoming nonce is meaningless junk because we didn't supply the\ncorrect answer the pusher can give us; we do not even have the\ncorrect answer ourselves.\n\nBesides, while reviewing the other patch, didn't we agree that we\nshould reject such a push? Then there is nothing to log anyway, no?\n\n ... reads the patch and code beyond the context while scratching\n head ...\n\nI notice that the above three lines do not correspond what you did\nin this patch. The certificate is exported via the blob interface\nfully with or without this change.\n\nWhat is not given to the hook is the push-cert-nonce-status. While\nit is sufficient to tell the hook that we are not getting a good\nnonce (i.e. the hook does not see GIT_PUSH_CERT_NONCE_STATUS=G), we\nare not showing that nonce-status is \"unsolicited\", even though we\ninternally compute and decide that; is that what you are trying to\nfix?\n\nStill puzzled...\n\n>\n> The recording of GIT_PUSH_CERT_NONCE_STATUS should be dependent on\n> the status being there instead of push_cert_nonce being non NULL.\n>\n> Without this patch an unsolicited nonce never makes to the stage when\n> being exported with GIT_PUSH_CERT_NONCE_STATUS, because in the unsolicited\n> case push_cert_nonce is always NULL.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> builtin/receive-pack.c | 12 ++++++------\n> 1 file changed, 6 insertions(+), 6 deletions(-)\n>\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 628d13a..0e4878e 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -504,18 +504,18 @@ static void prepare_push_cert_sha1(struct child_process *proc)\n> \t\t\t\t sigcheck.key ? sigcheck.key : \"\");\n> \t\targv_array_pushf(&proc->env_array, \"GIT_PUSH_CERT_STATUS=%c\",\n> \t\t\t\t sigcheck.result);\n> -\t\tif (push_cert_nonce) {\n> +\t\tif (push_cert_nonce)\n> \t\t\targv_array_pushf(&proc->env_array,\n> \t\t\t\t\t \"GIT_PUSH_CERT_NONCE=%s\",\n> \t\t\t\t\t push_cert_nonce);\n> +\t\tif (nonce_status)\n> \t\t\targv_array_pushf(&proc->env_array,\n> \t\t\t\t\t \"GIT_PUSH_CERT_NONCE_STATUS=%s\",\n> \t\t\t\t\t nonce_status);\n> -\t\t\tif (nonce_status == NONCE_SLOP)\n> -\t\t\t\targv_array_pushf(&proc->env_array,\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\tif (nonce_status == NONCE_SLOP)\n> +\t\t\targv_array_pushf(&proc->env_array,\n> +\t\t\t\t\t \"GIT_PUSH_CERT_NONCE_SLOP=%ld\",\n> +\t\t\t\t\t nonce_stamp_slop);\n> \t}\n> }\n"},{"id":"254523","messageId":"54B0A2C1.4040602@gmail.com","threadId":"38326","inReplyTo":"xmqqmw5r9zck.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] receive-pack.c: don't miss exporting unsolicited push certificates","fromName":"Stefan Beller","fromEmail":"stefanbeller@gmail.com","sentAt":"2015-01-10T03:55:45Z","receivedAt":"2015-01-10T03:55:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On 09.01.2015 17:52, Junio C Hamano wrote:\n> Stefan Beller writes:\n> \n>> If the server is configured to not advertise push certificates,\n>> a push certificate that gets pushed nevertheless will not be fully\n>> recorded because push_cert_nonce is NULL.\n> \n> Sorry, but I do not quite see what you are trying to get at.\n> \n> When we did not advertise that this feature is supported, we know\n> the incoming nonce is meaningless junk because we didn't supply the\n> correct answer the pusher can give us; we do not even have the\n> correct answer ourselves.\n\n> \n> Besides, while reviewing the other patch, didn't we agree that we\n> should reject such a push? Then there is nothing to log anyway, no?\n\nYes we did. This is unrelated to the previous patch. I stuffed it into\nthis thread as I found it was touching the same feature.\n\n> \n> ... reads the patch and code beyond the context while scratching\n> head ...\n> \n> I notice that the above three lines do not correspond what you did\n> in this patch. The certificate is exported via the blob interface\n> fully with or without this change.\n\nI had problems with wording the commit message because I have no\nexpertise with the feature. I am sorry for wasting your time there.\n\n> \n> What is not given to the hook is the push-cert-nonce-status. While\n> it is sufficient to tell the hook that we are not getting a good\n> nonce (i.e. the hook does not see GIT_PUSH_CERT_NONCE_STATUS=G), we\n> are not showing that nonce-status is \"unsolicited\", even though we\n> internally compute and decide that; is that what you are trying to\n> fix?\n\nyes that's what I was trying to hint at. The hook would just see\nit is unsolicited instead of not having the state available.\n\n\n> \n> Still puzzled...\n> \n>>\n>> The recording of GIT_PUSH_CERT_NONCE_STATUS should be dependent on\n>> the status being there instead of push_cert_nonce being non NULL.\n>>\n>> Without this patch an unsolicited nonce never makes to the stage when\n>> being exported with GIT_PUSH_CERT_NONCE_STATUS, because in the unsolicited\n>> case push_cert_nonce is always NULL.\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>> builtin/receive-pack.c | 12 ++++++------\n>> 1 file changed, 6 insertions(+), 6 deletions(-)\n>>\n>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n>> index 628d13a..0e4878e 100644\n>> --- a/builtin/receive-pack.c\n>> +++ b/builtin/receive-pack.c\n>> @@ -504,18 +504,18 @@ static void prepare_push_cert_sha1(struct child_process *proc)\n>> \t\t\t\t sigcheck.key ? sigcheck.key : \"\");\n>> \t\targv_array_pushf(&proc->env_array, \"GIT_PUSH_CERT_STATUS=%c\",\n>> \t\t\t\t sigcheck.result);\n>> -\t\tif (push_cert_nonce) {\n>> +\t\tif (push_cert_nonce)\n>> \t\t\targv_array_pushf(&proc->env_array,\n>> \t\t\t\t\t \"GIT_PUSH_CERT_NONCE=%s\",\n>> \t\t\t\t\t push_cert_nonce);\n>> +\t\tif (nonce_status)\n>> \t\t\targv_array_pushf(&proc->env_array,\n>> \t\t\t\t\t \"GIT_PUSH_CERT_NONCE_STATUS=%s\",\n>> \t\t\t\t\t nonce_status);\n>> -\t\t\tif (nonce_status == NONCE_SLOP)\n>> -\t\t\t\targv_array_pushf(&proc->env_array,\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\tif (nonce_status == NONCE_SLOP)\n>> +\t\t\targv_array_pushf(&proc->env_array,\n>> +\t\t\t\t\t \"GIT_PUSH_CERT_NONCE_SLOP=%ld\",\n>> +\t\t\t\t\t nonce_stamp_slop);\n>> \t}\n>> }\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n> \n"},{"id":"254554","messageId":"xmqqiogb95t4.fsf@gitster.dls.corp.google.com","threadId":"38326","inReplyTo":"54B0A2C1.4040602@gmail.com","subject":"Re: [PATCH] receive-pack.c: don't miss exporting unsolicited push certificates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-12T19:07:03Z","receivedAt":"2015-01-12T19:07:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> I had problems with wording the commit message because I have no\n> expertise with the feature. I am sorry for wasting your time there.\n\nHeh, remember, the time spent discussing Git on this list is not a\nwasted time.\n\n>> What is not given to the hook is the push-cert-nonce-status. While\n>> it is sufficient to tell the hook that we are not getting a good\n>> nonce (i.e. the hook does not see GIT_PUSH_CERT_NONCE_STATUS=G), we\n>> are not showing that nonce-status is \"unsolicited\", even though we\n>> internally compute and decide that; is that what you are trying to\n>> fix?\n>\n> yes that's what I was trying to hint at. The hook would just see\n> it is unsolicited instead of not having the state available.\n\nOK. That makes sort of sense. So if we:\n\n 1) did not apply either patch (i.e. we accept unsolicited\n certificate, and the fact that we did not exchange \"give me this\n nonce\" \"here is your nonce\" is conveyed to the hooks by the lack\n of GIT_PUSH_CERT_NONCE environment and possible presense of\n unsolicited nonce in the GIT_PUSH_CERT blob); and then\n\n 2) applied this patch first (i.e. we still allow unsolicited\n certificate, and the same fact is now conveyed to the hooks also\n by GIT_PUSH_CERT_NONCE_STATUS=UNSOLICITED if they sent a nonce,\n or GIT_PUSH_CERT_NONCE_STATUS=MISSING); and then finally\n\n 3) applied the other patch to reject unsolicited certificate.\n\nthen we can stop at any of these three steps and the behaviour of\nthree resulting systems make sense and the pre-receive hook can\nreject unsolicited certificates if it wants to, even at step #1.\n\nThe second step makes it easier for the hook to make that decision\nbecause it would make $GIT_PUSH_CERT_NONCE_STATUS the only thing it\nneeds to inspect, instead of checking it and also having to check\n$GIT_PUSH_CERT_NONCE, which is a simplification [*1*].\n\nAnd then in the third step, the hook does not even have to worry,\nwhich makes what #2 does more or less pointless.\n\nThis patch is still a good thing to do from the \"correctness\" point\nof view; the current code may accept certificates without nonce only\nbecause in an earlier iteration of the protocol design, the nonce\nwas optional and the code the earlier patch fixes is a remnant of\nthat. As we do not advertise push-cert without nonce in the final\nand current protocol, there is no reason to be loose anymore.\n\n\n[Footnote]\n\n*1* A hypothetical naive hook implementation:\n\n\tcase \"$GIT_PUSH_CERT_NONCE_STATUS\" in\n OK | SLOP)\n \t: good and happy\n exit 0;;\n\t*)\n \t: bad and unhappy\n exit 1;;\n\tesac\n\nwould diagnose an unsolicited certificate with or without nonce as\nbad. Though it cannot tell between unsolicited and missing cases,\nit would not be so serious a defect in practice.\n"},{"id":"254631","messageId":"CAGZ79kad=c1wXW163doxWBRdSvQ7Y9EkkD+ZXtOfawjtxEyeQw@mail.gmail.com","threadId":"38326","inReplyTo":"xmqqiogb95t4.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] receive-pack.c: don't miss exporting unsolicited push certificates","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-14T00:11:34Z","receivedAt":"2015-01-14T00:11:34Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Jan 12, 2015 at 11:07 AM, Junio C Hamano wrote:\n>>\n>> yes that's what I was trying to hint at. The hook would just see\n>> it is unsolicited instead of not having the state available.\n>\n> OK. That makes sort of sense. So if we:\n>\n> 1) did not apply either patch (i.e. we accept unsolicited\n> certificate, and the fact that we did not exchange \"give me this\n> nonce\" \"here is your nonce\" is conveyed to the hooks by the lack\n> of GIT_PUSH_CERT_NONCE environment and possible presense of\n> unsolicited nonce in the GIT_PUSH_CERT blob); and then\n>\n> 2) applied this patch first (i.e. we still allow unsolicited\n> certificate, and the same fact is now conveyed to the hooks also\n> by GIT_PUSH_CERT_NONCE_STATUS=UNSOLICITED if they sent a nonce,\n> or GIT_PUSH_CERT_NONCE_STATUS=MISSING); and then finally\n>\n> 3) applied the other patch to reject unsolicited certificate.\n>\n> then we can stop at any of these three steps and the behaviour of\n> three resulting systems make sense and the pre-receive hook can\n> reject unsolicited certificates if it wants to, even at step #1.\n\nI do not quite understand the intention of your mail. Are you saying I should\nmake a patch series to solve the problems as outlined above?\n"},{"id":"254669","messageId":"xmqqr3ux9qwr.fsf@gitster.dls.corp.google.com","threadId":"38326","inReplyTo":"CAGZ79kad=c1wXW163doxWBRdSvQ7Y9EkkD+ZXtOfawjtxEyeQw@mail.gmail.com","subject":"Re: [PATCH] receive-pack.c: don't miss exporting unsolicited push certificates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-14T18:08:04Z","receivedAt":"2015-01-14T18:08:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Mon, Jan 12, 2015 at 11:07 AM, Junio C Hamano wrote:\n>>>\n>>> yes that's what I was trying to hint at. The hook would just see\n>>> it is unsolicited instead of not having the state available.\n>>\n>> OK. That makes sort of sense. So if we:\n>>\n>> 1) did not apply either patch (i.e. we accept unsolicited\n>> certificate, and the fact that we did not exchange \"give me this\n>> nonce\" \"here is your nonce\" is conveyed to the hooks by the lack\n>> of GIT_PUSH_CERT_NONCE environment and possible presense of\n>> unsolicited nonce in the GIT_PUSH_CERT blob); and then\n>>\n>> 2) applied this patch first (i.e. we still allow unsolicited\n>> certificate, and the same fact is now conveyed to the hooks also\n>> by GIT_PUSH_CERT_NONCE_STATUS=UNSOLICITED if they sent a nonce,\n>> or GIT_PUSH_CERT_NONCE_STATUS=MISSING); and then finally\n>>\n>> 3) applied the other patch to reject unsolicited certificate.\n>>\n>> then we can stop at any of these three steps and the behaviour of\n>> three resulting systems make sense and the pre-receive hook can\n>> reject unsolicited certificates if it wants to, even at step #1.\n>\n> I do not quite understand the intention of your mail. Are you saying I should\n> make a patch series to solve the problems as outlined above?\n\nNot really. All I was saying was that a hypothetical patch series\nthat progressed in the order above would \"make sort-of sense\".\n\nI was hoping that readers would ask themselves this question: if we\nknow that our endgame will be #3, then does it still make much sense\nto have the state that only patch #2 is applied?\n\nI think #3 makes #2 unnecessary, as we always ask for nonce when\nadvertising push-cert capability in the released versions of Git, so\nwe might get an unsolicited push-cert (which #3 will reject), but we\nwill never see an unsolicited nonce in a push-cert, as long as we\ntold the pusher that it is OK to send a push-cert to us.\n"}]}