{"thread":{"id":"34421","subject":"[PATCH] http.c: fix parsing of http.sslCertPasswordProtected variable","startedAt":"2013-07-12T18:52:47Z","lastAt":"2013-07-15T15:54:45Z","messageCount":8,"participants":["Junio C Hamano","Jonathan Nieder","Kyle J. McKay","Mark Lodato"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"223201","messageId":"7v38rjlif4.fsf@alter.siamese.dyndns.org","threadId":"34421","inReplyTo":null,"subject":"[PATCH] http.c: fix parsing of http.sslCertPasswordProtected variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-12T18:52:47Z","receivedAt":"2013-07-12T18:52:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The existing code triggers only when the configuration variable is\nset to true. Once the variable is set to true in a more generic\nconfiguration file (e.g. ~/.gitconfig), it cannot be overriden to\nfalse in the repository specific one (e.g. .git/config).\n\nSigned-off-by: Junio C Hamano \n---\n http.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 92aba59..37986f8 100644\n--- a/http.c\n+++ b/http.c\n@@ -160,8 +160,7 @@ static int http_options(const char *var, const char *value, void *cb)\n \tif (!strcmp(\"http.sslcainfo\", var))\n \t\treturn git_config_string(&ssl_cainfo, var, value);\n \tif (!strcmp(\"http.sslcertpasswordprotected\", var)) {\n-\t\tif (git_config_bool(var, value))\n-\t\t\tssl_cert_password_required = 1;\n+\t\tssl_cert_password_required = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n \tif (!strcmp(\"http.ssltry\", var)) {\n-- \n1.8.3.2-942-gc84dfcb\n"},{"id":"223204","messageId":"20130712190531.GD8482@google.com","threadId":"34421","inReplyTo":"7v38rjlif4.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] http.c: fix parsing of http.sslCertPasswordProtected variable","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-07-12T19:05:31Z","receivedAt":"2013-07-12T19:05:31Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> The existing code triggers only when the configuration variable is\n> set to true. Once the variable is set to true in a more generic\n> configuration file (e.g. ~/.gitconfig), it cannot be overriden to\n> false in the repository specific one (e.g. .git/config).\n[...]\n> --- a/http.c\n> +++ b/http.c\n> @@ -160,8 +160,7 @@ static int http_options(const char *var, const char *value, void *cb)\n> \tif (!strcmp(\"http.sslcainfo\", var))\n> \t\treturn git_config_string(&ssl_cainfo, var, value);\n> \tif (!strcmp(\"http.sslcertpasswordprotected\", var)) {\n> -\t\tif (git_config_bool(var, value))\n> -\t\t\tssl_cert_password_required = 1;\n> +\t\tssl_cert_password_required = git_config_bool(var, value);\n\nThanks for catching it. The documentation doesn't say anything about\nthis \"can only enable and cannot disable\" behavior and the usual\npattern is to allow later settings to override earlier ones, so this\nchange looks good.\n\nReviewed-by: Jonathan Nieder \n\nFWIW the GIT_SSL_CERT_PASSWORD_PROTECTED envvar has a similar \"can\nonly enable\" behavior, but since it's documented, that's not as big\nof a problem. Do you remember why it was written that way?\n\nWhen that setting was first added[1], there was some mention of\nautodetection if libcurl could learn to do that. Did it happen?\n(Please forgive my ignorance.)\n\nThanks,\nJonathan\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/122793/focus=122816\n"},{"id":"223209","messageId":"7vtxjzk13q.fsf@alter.siamese.dyndns.org","threadId":"34421","inReplyTo":"20130712190531.GD8482@google.com","subject":"Re: [PATCH] http.c: fix parsing of http.sslCertPasswordProtected variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-12T19:52:09Z","receivedAt":"2013-07-12T19:52:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> FWIW the GIT_SSL_CERT_PASSWORD_PROTECTED envvar has a similar \"can\n> only enable\" behavior, but since it's documented, that's not as big\n> of a problem. Do you remember why it was written that way?\n\nNot me ;-).\n\nIf I have to guess, it is probably that these two are thought to be\nequally trivial way to defeat existing environment settings:\n\n\t(unset GIT_SSL_CERT_PASSWORD_PROTECTED ; git cmd)\n GIT_SSL_CERT_PASSWORD_PROTECTED=no git cmd\n\n> When that setting was first added[1], there was some mention of\n> autodetection if libcurl could learn to do that. Did it happen?\n\nI do not think so, but let's see if our resident cURL guru has\nsomething to say about it.\n"},{"id":"223250","messageId":"35166ba04d1ce901e0fb63a5c522970@f74d39fa044aa309eaea14b9f57fe79","threadId":"34421","inReplyTo":"20130712190531.GD8482@google.com","subject":"Re: [PATCH] http.c: fix parsing of http.sslCertPasswordProtected variable","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2013-07-13T19:28:15Z","receivedAt":"2013-07-13T19:28:15Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Jul 12, 2013, at 12:05, Jonathan Nieder wrote:\n> Junio C Hamano wrote:\n>\n>> The existing code triggers only when the configuration variable is\n>> set to true. Once the variable is set to true in a more generic\n>> configuration file (e.g. ~/.gitconfig), it cannot be overriden to\n>> false in the repository specific one (e.g. .git/config).\n> [...]\n>> --- a/http.c\n>> +++ b/http.c\n>> @@ -160,8 +160,7 @@ static int http_options(const char *var, const \n>> char *value, void *cb)\n>> \tif (!strcmp(\"http.sslcainfo\", var))\n>> \t\treturn git_config_string(&ssl_cainfo, var, value);\n>> \tif (!strcmp(\"http.sslcertpasswordprotected\", var)) {\n>> -\t\tif (git_config_bool(var, value))\n>> -\t\t\tssl_cert_password_required = 1;\n>> +\t\tssl_cert_password_required = git_config_bool(var, value);\n>\n> Thanks for catching it. The documentation doesn't say anything about\n> this \"can only enable and cannot disable\" behavior and the usual\n> pattern is to allow later settings to override earlier ones, so this\n> change looks good.\n>\n> Reviewed-by: Jonathan Nieder \n\nLooks good to me too.\n\n> FWIW the GIT_SSL_CERT_PASSWORD_PROTECTED envvar has a similar \"can\n> only enable\" behavior, but since it's documented, that's not as big\n> of a problem.\n\nHmmm. git help config says:\n\n> Can be overridden by the GIT_SSL_CERT_PASSWORD_PROTECTED environment\n> variable.\n\nin the http.sslCertPasswordProtected section of the help. It doesn't\nsay it can only be overridden to on. Is there some other documentation\nfor that somewhere I'm missing about being can-only-enable?\n\nIf not, perhaps a change something like the following could be added\nto the patch:\n\ndiff --git a/http.c b/http.c\nindex 2d086ae..83fc6b4 100644\n--- a/http.c\n+++ b/http.c\n@@ -404,11 +404,10 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)\n \t\tcurl_ftp_no_epsv = 1;\n \n \tif (url) {\n+\t\tint pwdreq = git_env_bool(\"GIT_SSL_CERT_PASSWORD_PROTECTED\", -1);\n \t\tcredential_from_url(&http_auth, url);\n-\t\tif (!ssl_cert_password_required &&\n-\t\t getenv(\"GIT_SSL_CERT_PASSWORD_PROTECTED\") &&\n-\t\t !prefixcmp(url, \"https://\"))\n-\t\t\tssl_cert_password_required = 1;\n+\t\tif (pwdreq != -1 && !prefixcmp(url, \"https://\"))\n+\t\t\tssl_cert_password_required = pwdreq;\n \t}\n \n #ifndef NO_CURL_EASY_DUPHANDLE\n-- \n"},{"id":"223257","messageId":"CAHREChiOE8oMWQYFY_7yzf1tz-4E2_L5W9--k33vwATFSrR==A@mail.gmail.com","threadId":"34421","inReplyTo":"7vtxjzk13q.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] http.c: fix parsing of http.sslCertPasswordProtected variable","fromName":"Mark Lodato","fromEmail":"lodatom@gmail.com","sentAt":"2013-07-14T00:37:58Z","receivedAt":"2013-07-14T00:37:58Z","isPatch":true,"sender":{"key":"lodatom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58860?v=4"},"body":"On Fri, Jul 12, 2013 at 3:52 PM, Junio C Hamano wrote:\n>\n> Jonathan Nieder writes:\n>\n> > FWIW the GIT_SSL_CERT_PASSWORD_PROTECTED envvar has a similar \"can\n> > only enable\" behavior, but since it's documented, that's not as big\n> > of a problem. Do you remember why it was written that way?\n>\n> Not me ;-).\n\nBecause that's how GIT_NO_VERIFY, GIT_CURL_FTP_NO_EPSV, and\nGIT_CURL_VERBOSE (and perhaps others) work. That said, I agree that\nparsing the variable's value as a boolean would make much more sense.\nPerhaps this is how all of those variables should work?\n\n> > When that setting was first added[1], there was some mention of\n> > autodetection if libcurl could learn to do that. Did it happen?\n>\n> I do not think so, but let's see if our resident cURL guru has\n> something to say about it.\n\nI tried back in 2009 but eventually gave up, so unfortunately no.\nMaybe the situation in libcurl has changed since then? (If you are\ninterested in pursing this, please let me know and I can give you the\ndetails of what I tried.)\n"},{"id":"223371","messageId":"7vzjtoea0a.fsf@alter.siamese.dyndns.org","threadId":"34421","inReplyTo":"CAHREChiOE8oMWQYFY_7yzf1tz-4E2_L5W9--k33vwATFSrR==A@mail.gmail.com","subject":"Re: [PATCH] http.c: fix parsing of http.sslCertPasswordProtected variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-15T04:13:09Z","receivedAt":"2013-07-15T04:13:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mark Lodato writes:\n\n> On Fri, Jul 12, 2013 at 3:52 PM, Junio C Hamano wrote:\n>>\n>> Jonathan Nieder writes:\n>>\n>> > FWIW the GIT_SSL_CERT_PASSWORD_PROTECTED envvar has a similar \"can\n>> > only enable\" behavior, but since it's documented, that's not as big\n>> > of a problem. Do you remember why it was written that way?\n>>\n>> Not me ;-).\n>\n> Because that's how GIT_NO_VERIFY, GIT_CURL_FTP_NO_EPSV, and\n\ns/GIT_NO_VERIFY/GIT_SSL_NO_VERIFY/, I think.\n\n> GIT_CURL_VERBOSE (and perhaps others) work. That said, I agree that\n> parsing the variable's value as a boolean would make much more sense.\n> Perhaps this is how all of those variables should work?\n\nI think you are probably right.\n"},{"id":"223384","messageId":"EFAADB2C-5774-4076-99CE-887AFB2B5CCA@gmail.com","threadId":"34421","inReplyTo":"7vzjtoea0a.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] http.c: fix parsing of http.sslCertPasswordProtected variable","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2013-07-15T06:37:44Z","receivedAt":"2013-07-15T06:37:44Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Jul 14, 2013, at 21:13, Junio C Hamano wrote:\n> Mark Lodato writes:\n>\n>> On Fri, Jul 12, 2013 at 3:52 PM, Junio C Hamano \n>> wrote:\n>>>\n>>> Jonathan Nieder writes:\n>>>\n>>>> FWIW the GIT_SSL_CERT_PASSWORD_PROTECTED envvar has a similar \"can\n>>>> only enable\" behavior, but since it's documented, that's not as big\n>>>> of a problem. Do you remember why it was written that way?\n>>>\n>>> Not me ;-).\n>>\n>> Because that's how GIT_NO_VERIFY, GIT_CURL_FTP_NO_EPSV, and\n>\n> s/GIT_NO_VERIFY/GIT_SSL_NO_VERIFY/, I think.\n>\n>> GIT_CURL_VERBOSE (and perhaps others) work. That said, I agree that\n>> parsing the variable's value as a boolean would make much more sense.\n>> Perhaps this is how all of those variables should work?\n>\n> I think you are probably right.\n\nThat works fine for GIT_SSL_CERT_PASSWORD_PROTECTED and \nGIT_CURL_VERBOSE, but it's a little bit awkward for GIT_SSL_NO_VERIFY \nand GIT_CURL_FTP_NO_EPSV since they have \"_NO_\" in their names.\n\nIf the user wants to override a \"http.sslVerify=false\" then \n\"GIT_SSL_NO_VERIFY=false\" is needed rather than \"GIT_SSL_VERIFY=true\".\n\nWe could:\n\n1) Introduce GIT_SSL_VERIFY and GIT_CURL_FTP_EPSV and say if they are \nset the \"_NO_\" version is ignored.\n\n2) Go ahead with \"GIT_SSL_NO_VERIFY=false\" to force http.sslVerify \nback to true (and similarly for EPSV).\n\n3) Just leave GIT_SSL_NO_VERIFY and GIT_CURL_FTP_NO_EPSV alone.\n\n4) Do something else, ideas?\n\nComments?\n"},{"id":"223444","messageId":"7v4nbvddiy.fsf@alter.siamese.dyndns.org","threadId":"34421","inReplyTo":"EFAADB2C-5774-4076-99CE-887AFB2B5CCA@gmail.com","subject":"Re: [PATCH] http.c: fix parsing of http.sslCertPasswordProtected variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-15T15:54:45Z","receivedAt":"2013-07-15T15:54:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kyle J. McKay\" writes:\n\n> That works fine for GIT_SSL_CERT_PASSWORD_PROTECTED and\n> GIT_CURL_VERBOSE, but it's a little bit awkward for GIT_SSL_NO_VERIFY\n> and GIT_CURL_FTP_NO_EPSV since they have \"_NO_\" in their names.\n>\n> If the user wants to override a \"http.sslVerify=false\" then\n> \"GIT_SSL_NO_VERIFY=false\" is needed rather than \"GIT_SSL_VERIFY=true\".\n>\n> We could:\n>\n> 1) Introduce GIT_SSL_VERIFY and GIT_CURL_FTP_EPSV and say if they are\n> set the \"_NO_\" version is ignored.\n>\n> 2) Go ahead with \"GIT_SSL_NO_VERIFY=false\" to force http.sslVerify\n> back to true (and similarly for EPSV).\n>\n> 3) Just leave GIT_SSL_NO_VERIFY and GIT_CURL_FTP_NO_EPSV alone.\n>\n> 4) Do something else, ideas?\n>\n> Comments?\n\nThe usual way we have done this kind of thing so far is:\n\n (1) Add GIT_SSL_VERIFY and GIT_CURL_FTP_EPSV support, that is\n boolean. If that is set, it takes precedence to\n GIT_SSL_NO_VERIFY and GIT_CURL_FTP_NO_EPSV. If not,\n GIT_SSL_NO_VERIFY and GIT_CURL_FTP_NO_EPSV are used just like\n before (setting it to any value will decline VERIFY or EPSV).\n\n (2) Issue a warning to tell users to use GIT_SSL_VERIFY or\n GIT_CURL_FTP_EPSV instead, when GIT_SSL_NO_VERIFY and\n GIT_CURL_FTP_NO_EPSV are used.\n\n (3) Later at a large version bump, remove support for *_NO_*\n variants.\n\nWe can stop at (1) and not do (2) and (3), if the resulting code\nafter (1) is not too cumbersome to maintain. If we do (2) then we\nmust follow it through with (3), and if we plan to do (3) then we\nmust precede it with (2).\n"}]}