Re: [PATCH] http.c: fix parsing of http.sslCertPasswordProtected variable

2013-07-14 Thread Junio C Hamano 
Mark Lodato loda...@gmail.com writes:

 On Fri, Jul 12, 2013 at 3:52 PM, Junio C Hamano gits...@pobox.com wrote:

 Jonathan Nieder jrnie...@gmail.com writes:

  FWIW the GIT_SSL_CERT_PASSWORD_PROTECTED envvar has a similar can
  only enable behavior, but since it's documented, that's not as big
  of a problem.  Do you remember why it was written that way?

 Not me ;-).

 Because that's how GIT_NO_VERIFY, GIT_CURL_FTP_NO_EPSV, and

s/GIT_NO_VERIFY/GIT_SSL_NO_VERIFY/, I think.

 GIT_CURL_VERBOSE (and perhaps others) work.  That said, I agree that
 parsing the variable's value as a boolean would make much more sense.
 Perhaps this is how all of those variables should work?

I think you are probably right.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] http.c: fix parsing of http.sslCertPasswordProtected variable

2013-07-13 Thread Kyle J. McKay 
On Jul 12, 2013, at 12:05, Jonathan Nieder wrote:
 Junio C Hamano wrote:

 The existing code triggers only when the configuration variable is
 set to true.  Once the variable is set to true in a more generic
 configuration file (e.g. ~/.gitconfig), it cannot be overriden to
 false in the repository specific one (e.g. .git/config).
 [...]
 --- a/http.c
 +++ b/http.c
 @@ -160,8 +160,7 @@ static int http_options(const char *var, const  
 char *value, void *cb)
  if (!strcmp(http.sslcainfo, var))
  return git_config_string(ssl_cainfo, var, value);
  if (!strcmp(http.sslcertpasswordprotected, var)) {
 -if (git_config_bool(var, value))
 -ssl_cert_password_required = 1;
 +ssl_cert_password_required = git_config_bool(var, value);

 Thanks for catching it.  The documentation doesn't say anything about
 this can only enable and cannot disable behavior and the usual
 pattern is to allow later settings to override earlier ones, so this
 change looks good.

 Reviewed-by: Jonathan Nieder jrnie...@gmail.com

Looks good to me too.

 FWIW the GIT_SSL_CERT_PASSWORD_PROTECTED envvar has a similar can
 only enable behavior, but since it's documented, that's not as big
 of a problem.

Hmmm.  git help config says:

 Can be overridden by the GIT_SSL_CERT_PASSWORD_PROTECTED environment
 variable.

in the http.sslCertPasswordProtected section of the help.  It doesn't
say it can only be overridden to on.  Is there some other documentation
for that somewhere I'm missing about being can-only-enable?

If not, perhaps a change something like the following could be added
to the patch:

diff --git a/http.c b/http.c
index 2d086ae..83fc6b4 100644
--- a/http.c
+++ b/http.c
@@ -404,11 +404,10 @@ void http_init(struct remote *remote, const char *url, 
int proactive_auth)
curl_ftp_no_epsv = 1;
 
if (url) {
+   int pwdreq = git_env_bool(GIT_SSL_CERT_PASSWORD_PROTECTED, 
-1);
credential_from_url(http_auth, url);
-   if (!ssl_cert_password_required 
-   getenv(GIT_SSL_CERT_PASSWORD_PROTECTED) 
-   !prefixcmp(url, https://;))
-   ssl_cert_password_required = 1;
+   if (pwdreq != -1  !prefixcmp(url, https://;))
+   ssl_cert_password_required = pwdreq;
}
 
 #ifndef NO_CURL_EASY_DUPHANDLE
-- 
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] http.c: fix parsing of http.sslCertPasswordProtected variable

2013-07-13 Thread Mark Lodato 
On Fri, Jul 12, 2013 at 3:52 PM, Junio C Hamano gits...@pobox.com wrote:

 Jonathan Nieder jrnie...@gmail.com writes:

  FWIW the GIT_SSL_CERT_PASSWORD_PROTECTED envvar has a similar can
  only enable behavior, but since it's documented, that's not as big
  of a problem.  Do you remember why it was written that way?

 Not me ;-).

Because that's how GIT_NO_VERIFY, GIT_CURL_FTP_NO_EPSV, and
GIT_CURL_VERBOSE (and perhaps others) work.  That said, I agree that
parsing the variable's value as a boolean would make much more sense.
Perhaps this is how all of those variables should work?

  When that setting was first added[1], there was some mention of
  autodetection if libcurl could learn to do that.  Did it happen?

 I do not think so, but let's see if our resident cURL guru has
 something to say about it.

I tried back in 2009 but eventually gave up, so unfortunately no.
Maybe the situation in libcurl has changed since then?  (If you are
interested in pursing this, please let me know and I can give you the
details of what I tried.)
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] http.c: fix parsing of http.sslCertPasswordProtected variable

2013-07-12 Thread Junio C Hamano 
The existing code triggers only when the configuration variable is
set to true.  Once the variable is set to true in a more generic
configuration file (e.g. ~/.gitconfig), it cannot be overriden to
false in the repository specific one (e.g. .git/config).

Signed-off-by: Junio C Hamano gits...@pobox.com
---
 http.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/http.c b/http.c
index 92aba59..37986f8 100644
--- a/http.c
+++ b/http.c
@@ -160,8 +160,7 @@ static int http_options(const char *var, const char *value, 
void *cb)
if (!strcmp(http.sslcainfo, var))
return git_config_string(ssl_cainfo, var, value);
if (!strcmp(http.sslcertpasswordprotected, var)) {
-   if (git_config_bool(var, value))
-   ssl_cert_password_required = 1;
+   ssl_cert_password_required = git_config_bool(var, value);
return 0;
}
if (!strcmp(http.ssltry, var)) {
-- 
1.8.3.2-942-gc84dfcb

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] http.c: fix parsing of http.sslCertPasswordProtected variable

2013-07-12 Thread Jonathan Nieder 
Junio C Hamano wrote:

 The existing code triggers only when the configuration variable is
 set to true.  Once the variable is set to true in a more generic
 configuration file (e.g. ~/.gitconfig), it cannot be overriden to
 false in the repository specific one (e.g. .git/config).
[...]
 --- a/http.c
 +++ b/http.c
 @@ -160,8 +160,7 @@ static int http_options(const char *var, const char 
 *value, void *cb)
   if (!strcmp(http.sslcainfo, var))
   return git_config_string(ssl_cainfo, var, value);
   if (!strcmp(http.sslcertpasswordprotected, var)) {
 - if (git_config_bool(var, value))
 - ssl_cert_password_required = 1;
 + ssl_cert_password_required = git_config_bool(var, value);

Thanks for catching it.  The documentation doesn't say anything about
this can only enable and cannot disable behavior and the usual
pattern is to allow later settings to override earlier ones, so this
change looks good.

Reviewed-by: Jonathan Nieder jrnie...@gmail.com

FWIW the GIT_SSL_CERT_PASSWORD_PROTECTED envvar has a similar can
only enable behavior, but since it's documented, that's not as big
of a problem.  Do you remember why it was written that way?

When that setting was first added[1], there was some mention of
autodetection if libcurl could learn to do that.  Did it happen?
(Please forgive my ignorance.)

Thanks,
Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/122793/focus=122816
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] http.c: fix parsing of http.sslCertPasswordProtected variable

2013-07-12 Thread Junio C Hamano 
Jonathan Nieder jrnie...@gmail.com writes:

 FWIW the GIT_SSL_CERT_PASSWORD_PROTECTED envvar has a similar can
 only enable behavior, but since it's documented, that's not as big
 of a problem.  Do you remember why it was written that way?

Not me ;-).

If I have to guess, it is probably that these two are thought to be
equally trivial way to defeat existing environment settings:

(unset GIT_SSL_CERT_PASSWORD_PROTECTED ; git cmd)
GIT_SSL_CERT_PASSWORD_PROTECTED=no git cmd

 When that setting was first added[1], there was some mention of
 autodetection if libcurl could learn to do that.  Did it happen?

I do not think so, but let's see if our resident cURL guru has
something to say about it.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html