{"thread":{"id":"30674","subject":"[RFC] Deal with HTTP 401 by requesting credentials.","startedAt":"2012-05-31T22:24:55Z","lastAt":"2012-06-05T16:41:37Z","messageCount":8,"participants":["Kevin Stange","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"192625","messageId":"4FC7EFB7.4090704@steadfast.net","threadId":"30674","inReplyTo":null,"subject":"[RFC] Deal with HTTP 401 by requesting credentials.","fromName":"Kevin Stange","fromEmail":"kevin@steadfast.net","sentAt":"2012-05-31T22:24:55Z","receivedAt":"2012-05-31T22:24:55Z","isPatch":false,"sender":{"key":"kevin@steadfast.net","avatar":"https://gravatar.com/avatar/1971e61dae0b441b72fd423ba7fed34cf8534dbf0d7fe867d173c770bfeef86e?d=mp&s=160"},"body":"Request credentials from the user if none are already defined when a\nHTTP 401 is received on a restricted repository.  Then, resubmit the\nrequest and return the final result.\n\nThis allows all webdav transactions to obtain credentials without having\nto individually handle the case in each request.  Having push working\nwith HTTP auth is needed for a use case I have where storing the\ncredentials in .netrc or using SSH keys is inappropriate.\n\nApologies for anything wrong I might have done here.  I'm not used to\nprocedures for this sort of patch submission, or terribly familiar with\nthe code base.  I'm seeking advice on whether this approach is sane or\ncompletely crazy, and I'm willing to adjust it to make it suitable for\ninclusion.\n\nSigned-off-by: Kevin <kevin@steadfast.net>\n---\n http.c | 15 +++++++++++++++\n 1 file changed, 15 insertions(+)\n\ndiff --git a/http.c b/http.c\nindex 5cb87f1..e1c9e65 100644\n--- a/http.c\n+++ b/http.c\n@@ -668,6 +668,21 @@ void finish_active_slot(struct active_request_slot\n*slot)\n \tclosedown_active_slot(slot);\n \tcurl_easy_getinfo(slot->curl, CURLINFO_HTTP_CODE, &slot->http_code);\n\n+\tif (slot->http_code == 401) {\n+\t\tif(!http_auth.username && !http_auth.password) {\n+\t\t\tactive_requests++;\n+\t\t\tcredential_fill(&http_auth);\n+\t\t\tinit_curl_http_auth(slot->curl);\n+\t\t\t(*slot->finished) = 1;\n+\t\t\tif (start_active_slot(slot)) {\n+\t\t\t\trun_active_slot(slot);\n+\t\t\t\treturn;\n+\t\t\t}\n+\t\t} else {\n+\t\t\tfprintf(stderr,\"Authentication Failed!\\n\");\n+\t\t}\n+\t}\n+\n \tif (slot->finished != NULL)\n \t\t(*slot->finished) = 1;\n\n-- \n1.7.11.rc0.55.gb2478aa\n\n-- \nKevin Stange\nChief Technology Officer\nSteadfast Networks\nhttp://steadfast.net\nPhone: 312-602-2689 ext. 203 | Fax: 312-602-2688 | Cell: 312-320-5867\n"},{"id":"192627","messageId":"4FC7FC49.4030503@steadfast.net","threadId":"30674","inReplyTo":"4FC7EFB7.4090704@steadfast.net","subject":"Re: [RFC] Deal with HTTP 401 by requesting credentials.","fromName":"Kevin Stange","fromEmail":"kevin@steadfast.net","sentAt":"2012-05-31T23:18:33Z","receivedAt":"2012-05-31T23:18:33Z","isPatch":false,"sender":{"key":"kevin@steadfast.net","avatar":"https://gravatar.com/avatar/1971e61dae0b441b72fd423ba7fed34cf8534dbf0d7fe867d173c770bfeef86e?d=mp&s=160"},"body":"On 05/31/2012 05:24 PM, Kevin Stange wrote:\n> Apologies for anything wrong I might have done here.  I'm not used to\n> procedures for this sort of patch submission, or terribly familiar with\n> the code base.  I'm seeking advice on whether this approach is sane or\n> completely crazy, and I'm willing to adjust it to make it suitable for\n> inclusion.\n\nOf course, I failed to get my email client to send the patch unwrapped.  Only one line appears to have been affected, but I don't think this is a final patch.  I'll be sure to make a proper submission for any future patches I send.  Sorry!\n\n-- \nKevin Stange\nChief Technology Officer\nSteadfast Networks\nhttp://steadfast.net\nPhone: 312-602-2689 ext. 203 | Fax: 312-602-2688 | Cell: 312-320-5867\n"},{"id":"192631","messageId":"20120601083537.GA32340@sigill.intra.peff.net","threadId":"30674","inReplyTo":"4FC7EFB7.4090704@steadfast.net","subject":"Re: [RFC] Deal with HTTP 401 by requesting credentials.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-06-01T08:35:37Z","receivedAt":"2012-06-01T08:35:37Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 31, 2012 at 05:24:55PM -0500, Kevin Stange wrote:\n\n> Request credentials from the user if none are already defined when a\n> HTTP 401 is received on a restricted repository.  Then, resubmit the\n> request and return the final result.\n> \n> This allows all webdav transactions to obtain credentials without having\n> to individually handle the case in each request.  Having push working\n> with HTTP auth is needed for a use case I have where storing the\n> credentials in .netrc or using SSH keys is inappropriate.\n\nWe already do this at a higher level in http_request, which in turns\ncalls into finish_active_slot. So if we were going to go this route,\nwouldn't we also want to remove the 401 handling in http_request?\n\nThe dumb-http push code is the only thing that does not go through\nhttp_request these days. So another option would be to refactor it to go\nthrough that central point. I took a brief look at this when I was\nupdating the credential code a few months ago, but didn't consider it a\npriority, as most people should be using smart http these days. Is there\na reason you can't use smart-http? It's significantly more efficient.\n\nYou also don't necessarily need to handle 401 in every code path of\nhttp-push; once we see the credential once, we will use it everywhere,\nso you really only need to handle it on the initial request (assuming\nthat all requests will have the same authorization requirements).\n\n> Apologies for anything wrong I might have done here.  I'm not used to\n> procedures for this sort of patch submission, or terribly familiar with\n> the code base.  I'm seeking advice on whether this approach is sane or\n> completely crazy, and I'm willing to adjust it to make it suitable for\n> inclusion.\n> \n> Signed-off-by: Kevin <kevin@steadfast.net>\n> ---\n\nCover letter material (i.e., anything that would not go into the commit\nmessage of the final commit) should go below the \"---\".\n\n> diff --git a/http.c b/http.c\n> index 5cb87f1..e1c9e65 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -668,6 +668,21 @@ void finish_active_slot(struct active_request_slot\n> *slot)\n>  \tclosedown_active_slot(slot);\n>  \tcurl_easy_getinfo(slot->curl, CURLINFO_HTTP_CODE, &slot->http_code);\n> \n> +\tif (slot->http_code == 401) {\n> +\t\tif(!http_auth.username && !http_auth.password) {\n> +\t\t\tactive_requests++;\n> +\t\t\tcredential_fill(&http_auth);\n> +\t\t\tinit_curl_http_auth(slot->curl);\n> +\t\t\t(*slot->finished) = 1;\n> +\t\t\tif (start_active_slot(slot)) {\n> +\t\t\t\trun_active_slot(slot);\n> +\t\t\t\treturn;\n> +\t\t\t}\n> +\t\t} else {\n> +\t\t\tfprintf(stderr,\"Authentication Failed!\\n\");\n> +\t\t}\n> +\t}\n\nIs it safe to just run start_active_slot again without reinitializing\nthe request? The 401-handling code in http_request actually restarts a\nnew request. I don't immediately see any state that would need to be\nreset; we might have written some data to the output file if curl gave\nus any body data, but presumably it would not have done so for a 401.\n\nIn the \"else\" clause you add, I don't think there's any point in\nprinting an error. The 401 should get propagated back to the caller, who\nwill produce an error. However, you _should_ call credential_reject,\nsince you know that the credential you have doesn't work.\n\nSimilarly, you would want to call credential_accept after a successful\nrequest, so that helpers can store it.\n\n-Peff\n"},{"id":"192677","messageId":"7vtxyv2cft.fsf@alter.siamese.dyndns.org","threadId":"30674","inReplyTo":"20120601083537.GA32340@sigill.intra.peff.net","subject":"Re: [RFC] Deal with HTTP 401 by requesting credentials.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-01T17:01:58Z","receivedAt":"2012-06-01T17:01:58Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, May 31, 2012 at 05:24:55PM -0500, Kevin Stange wrote:\n>\n>> Request credentials from the user if none are already defined when a\n>> HTTP 401 is received on a restricted repository.  Then, resubmit the\n>> request and return the final result.\n>> \n>> This allows all webdav transactions to obtain credentials without having\n>> to individually handle the case in each request.  Having push working\n>> with HTTP auth is needed for a use case I have where storing the\n>> credentials in .netrc or using SSH keys is inappropriate.\n>\n> We already do this at a higher level in http_request, which in turns\n> calls into finish_active_slot. So if we were going to go this route,\n> wouldn't we also want to remove the 401 handling in http_request?\n\nWouldn't the higher levels know a lot more about the context this\nrequest was made, though?  What problem does the patch try to solve?\nSome higher level callers missing the \"if we got 401, then reset and\nretry\" logic?  Wouldn't it be saner to fix the breakage there?\n\n> The dumb-http push code is the only thing that does not go through\n> http_request these days. So another option would be to refactor it to go\n> through that central point.\n\nIt does sound like the right approach to take.\n\nThanks.\n"},{"id":"192678","messageId":"4FC8F590.2070308@steadfast.net","threadId":"30674","inReplyTo":"20120601083537.GA32340@sigill.intra.peff.net","subject":"Re: [RFC] Deal with HTTP 401 by requesting credentials.","fromName":"Kevin Stange","fromEmail":"kevin@steadfast.net","sentAt":"2012-06-01T17:02:08Z","receivedAt":"2012-06-01T17:02:08Z","isPatch":false,"sender":{"key":"kevin@steadfast.net","avatar":"https://gravatar.com/avatar/1971e61dae0b441b72fd423ba7fed34cf8534dbf0d7fe867d173c770bfeef86e?d=mp&s=160"},"body":"On 06/01/2012 03:35 AM, Jeff King wrote:\n> On Thu, May 31, 2012 at 05:24:55PM -0500, Kevin Stange wrote:\n> \n>> Request credentials from the user if none are already defined when a\n>> HTTP 401 is received on a restricted repository.  Then, resubmit the\n>> request and return the final result.\n>>\n>> This allows all webdav transactions to obtain credentials without having\n>> to individually handle the case in each request.  Having push working\n>> with HTTP auth is needed for a use case I have where storing the\n>> credentials in .netrc or using SSH keys is inappropriate.\n> \n> We already do this at a higher level in http_request, which in turns\n> calls into finish_active_slot. So if we were going to go this route,\n> wouldn't we also want to remove the 401 handling in http_request?\n\nI did see the work being done there, and considered removing it.  In fact, I\nused it as a reference for working out what was going on in the authentication\nprocess.  I decided not to do anything there until soliciting feedback and\ndeciding whether my approach was reasonable.\n\n> The dumb-http push code is the only thing that does not go through\n> http_request these days. So another option would be to refactor it to go\n> through that central point. I took a brief look at this when I was\n> updating the credential code a few months ago, but didn't consider it a\n> priority, as most people should be using smart http these days. Is there\n> a reason you can't use smart-http? It's significantly more efficient.\n\nSmart HTTP didn't come up in any of my Google searches.  With that as an\noption, I might just drop this work now.  I'd rather see incomplete methods\nthat aren't recommended go away than further facilitate their use, personally.\n\n> You also don't necessarily need to handle 401 in every code path of\n> http-push; once we see the credential once, we will use it everywhere,\n> so you really only need to handle it on the initial request (assuming\n> that all requests will have the same authorization requirements).\n\nI made the change where I did because I wasn't sure if the push code was\navoiding using http_request intentionally, and wasn't sure whether new code\nwould be written that avoid it as well.\n\nIf that's not the case, then I gather http-push would be better rewritten to\njust use http_request, if anything.\n\n>> Apologies for anything wrong I might have done here.  I'm not used to\n>> procedures for this sort of patch submission, or terribly familiar with\n>> the code base.  I'm seeking advice on whether this approach is sane or\n>> completely crazy, and I'm willing to adjust it to make it suitable for\n>> inclusion.\n>>\n>> Signed-off-by: Kevin <kevin@steadfast.net>\n>> ---\n> \n> Cover letter material (i.e., anything that would not go into the commit\n> message of the final commit) should go below the \"---\".\n\nThanks, will remember this for future reference.\n\n> Is it safe to just run start_active_slot again without reinitializing\n> the request? The 401-handling code in http_request actually restarts a\n> new request. I don't immediately see any state that would need to be\n> reset; we might have written some data to the output file if curl gave\n> us any body data, but presumably it would not have done so for a 401.\n\nIn my tests, this particular code flow never returns anything to the original\nrequest call because I interrupt it and start the request over.  Now that I\nlook at it again, there's a chance it leaks the curl response, but it doesn't\nreturn that response and the new request works fine, replacing the original.\n\n> In the \"else\" clause you add, I don't think there's any point in\n> printing an error. The 401 should get propagated back to the caller, who\n> will produce an error. However, you _should_ call credential_reject,\n> since you know that the credential you have doesn't work.\n> \n> Similarly, you would want to call credential_accept after a successful\n> request, so that helpers can store it.\n\nIf I decide to continue working on this, I will keep these in mind.  I'm\npretty sure that if I can get smart HTTP working, there's no reason to even\nbother with this from my perspective, unless you think there's substantial\nvalue in it.\n\nThanks for the detailed feedback on the proposed change and suggestions on\nalternative options.\n\n-- \nKevin Stange\nChief Technology Officer\nSteadfast Networks\nhttp://steadfast.net\nPhone: 312-602-2689 ext. 203 | Fax: 312-602-2688 | Cell: 312-320-5867\n\n"},{"id":"192918","messageId":"20120605162309.GA20915@sigill.intra.peff.net","threadId":"30674","inReplyTo":"7vtxyv2cft.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC] Deal with HTTP 401 by requesting credentials.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-06-05T16:23:09Z","receivedAt":"2012-06-05T16:23:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 01, 2012 at 10:01:58AM -0700, Junio C Hamano wrote:\n\n> >> Request credentials from the user if none are already defined when a\n> >> HTTP 401 is received on a restricted repository.  Then, resubmit the\n> >> request and return the final result.\n> >> \n> >> This allows all webdav transactions to obtain credentials without having\n> >> to individually handle the case in each request.  Having push working\n> >> with HTTP auth is needed for a use case I have where storing the\n> >> credentials in .netrc or using SSH keys is inappropriate.\n> >\n> > We already do this at a higher level in http_request, which in turns\n> > calls into finish_active_slot. So if we were going to go this route,\n> > wouldn't we also want to remove the 401 handling in http_request?\n> \n> Wouldn't the higher levels know a lot more about the context this\n> request was made, though?  What problem does the patch try to solve?\n> Some higher level callers missing the \"if we got 401, then reset and\n> retry\" logic?  Wouldn't it be saner to fix the breakage there?\n\nI don't know that the higher level really has much more context; the URL\nis really the only context we use. I think the problem is adequately\nsolved in http_request; it is simply that the http-push code paths do\nnot use it. The patch solves that by pushing it to a lower level.\n\n-Peff\n"},{"id":"192919","messageId":"20120605162824.GB20915@sigill.intra.peff.net","threadId":"30674","inReplyTo":"4FC8F590.2070308@steadfast.net","subject":"Re: [RFC] Deal with HTTP 401 by requesting credentials.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-06-05T16:28:24Z","receivedAt":"2012-06-05T16:28:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 01, 2012 at 12:02:08PM -0500, Kevin Stange wrote:\n\n> > The dumb-http push code is the only thing that does not go through\n> > http_request these days. So another option would be to refactor it to go\n> > through that central point. I took a brief look at this when I was\n> > updating the credential code a few months ago, but didn't consider it a\n> > priority, as most people should be using smart http these days. Is there\n> > a reason you can't use smart-http? It's significantly more efficient.\n> \n> Smart HTTP didn't come up in any of my Google searches.  With that as an\n> option, I might just drop this work now.  I'd rather see incomplete methods\n> that aren't recommended go away than further facilitate their use, personally.\n\nMe too. I would love it if dumb http push just went away. It's extremely\nneglected, and has very few advantages over smart http (really, the only\nadvantage is that the server does not need to run git). However, we do\nget bug reports on it occasionally, so I think people are still using\nit.\n\nSo far our approach has mostly been to prevent any serious regressions,\nand otherwise not worry too much about dragging it along with new\nfeatures.\n\n> I made the change where I did because I wasn't sure if the push code was\n> avoiding using http_request intentionally, and wasn't sure whether new code\n> would be written that avoid it as well.\n> \n> If that's not the case, then I gather http-push would be better rewritten to\n> just use http_request, if anything.\n\nYes, I think so. It's purely historical and accidental that the\nhttp-push code does not use it (and that the 401-handling happened to be\nput at that layer).  The push and fetch sides grew up independently and\norganically; I think if somebody were writing it now, they would\norganize http.c quite differently.\n\n> If I decide to continue working on this, I will keep these in mind.  I'm\n> pretty sure that if I can get smart HTTP working, there's no reason to even\n> bother with this from my perspective, unless you think there's substantial\n> value in it.\n\nNo, I don't think there's substantial value. If you can move to smart\nhttp, you are much better off.\n\n-Peff\n"},{"id":"192922","messageId":"4FCE36C1.3010807@steadfast.net","threadId":"30674","inReplyTo":"20120605162824.GB20915@sigill.intra.peff.net","subject":"Re: [RFC] Deal with HTTP 401 by requesting credentials.","fromName":"Kevin Stange","fromEmail":"kevin@steadfast.net","sentAt":"2012-06-05T16:41:37Z","receivedAt":"2012-06-05T16:41:37Z","isPatch":false,"sender":{"key":"kevin@steadfast.net","avatar":"https://gravatar.com/avatar/1971e61dae0b441b72fd423ba7fed34cf8534dbf0d7fe867d173c770bfeef86e?d=mp&s=160"},"body":"On 06/05/2012 11:28 AM, Jeff King wrote:\n> On Fri, Jun 01, 2012 at 12:02:08PM -0500, Kevin Stange wrote:\n> \n>>> The dumb-http push code is the only thing that does not go through\n>>> http_request these days. So another option would be to refactor it to go\n>>> through that central point. I took a brief look at this when I was\n>>> updating the credential code a few months ago, but didn't consider it a\n>>> priority, as most people should be using smart http these days. Is there\n>>> a reason you can't use smart-http? It's significantly more efficient.\n>>\n>> Smart HTTP didn't come up in any of my Google searches.  With that as an\n>> option, I might just drop this work now.  I'd rather see incomplete methods\n>> that aren't recommended go away than further facilitate their use, personally.\n> \n> Me too. I would love it if dumb http push just went away. It's extremely\n> neglected, and has very few advantages over smart http (really, the only\n> advantage is that the server does not need to run git). However, we do\n> get bug reports on it occasionally, so I think people are still using\n> it.\n> \n> So far our approach has mostly been to prevent any serious regressions,\n> and otherwise not worry too much about dragging it along with new\n> features.\n\nIt seemed a lot of \"guides\" that I found while searching suggested using the\n\"dumb\" system.  That may be why it remains in use.  I can't imagine too many\npeople have issues running a CGI to ensure everything works right.\n\n>> If I decide to continue working on this, I will keep these in mind.  I'm\n>> pretty sure that if I can get smart HTTP working, there's no reason to even\n>> bother with this from my perspective, unless you think there's substantial\n>> value in it.\n> \n> No, I don't think there's substantial value. If you can move to smart\n> http, you are much better off.\n\nI've gotten smart HTTP working perfectly.  Thank you for pointing me to it.  I\nam considering this resolved.\n\n-- \nKevin Stange\nChief Technology Officer\nSteadfast Networks\nhttp://steadfast.net\nPhone: 312-602-2689 ext. 203 | Fax: 312-602-2688 | Cell: 312-320-5867\n\n"}]}