{"thread":{"id":"33584","subject":"[PATCH] http.c: Add config options/parsing for SSL engine vars","startedAt":"2013-04-23T23:03:27Z","lastAt":"2013-04-30T21:05:22Z","messageCount":8,"participants":["Jerry Qassar","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"215278","messageId":"1366758207-7724-1-git-send-email-jqassar@gmail.com","threadId":"33584","inReplyTo":null,"subject":"[PATCH] http.c: Add config options/parsing for SSL engine vars","fromName":"Jerry Qassar","fromEmail":"jqassar@gmail.com","sentAt":"2013-04-23T23:03:27Z","receivedAt":"2013-04-23T23:03:27Z","isPatch":true,"sender":{"key":"jqassar@gmail.com","avatar":"https://gravatar.com/avatar/3352ceac69b346f383b7f980aac62b69bfec5a69ec7880feb41ab04ede73cc2f?d=mp&s=160"},"body":"\ncurl provides many options for configuring the way it negotiates an SSL\nconnection (with its default OpenSSL support), including ways to define\nthe SSL engine used, and parameters to set the format of the key and\ncertificate used.  Unfortunately, git does not parse some of the\ncritical ones needed by curl to support PKCS#11.\n\nAdd the following git config variables (and direct env-set variables):\n\n* http.sslengine\n  A string variable that sets CURLOPT_SSLENGINE on the back end.\n  Can be set from environment using GIT_SSL_ENGINE.\n* http.sslkeytype\n  A string variable, either PEM/DER/ENG, that sets CURLOPT_SSLKEYTYPE.\n  Can be set from environment using GIT_SSL_KEYTYPE.\n* http.sslcerttype\n  A string variable, either PEM/DER/ENG, that sets CURLOPT_SSLCERTTYPE.\n  Can be set from environment using GIT_SSL_CERTTYPE.\n\nParsing these new variables combined with related patches to curl\nwill allow git to support native authentication with smart cards.\n\nNote: It's difficult to test this without the related curl patches,\nwhich I will be submitting soon.  At the very least, leaving these new\noptions unset doesn't break anything, and setting them has little\neffect without the back-end curl changes needed to 'turn on' PKCS#11.\nAny suggestions would be greatly appreciated.\n\nSigned-off-by: Jerry Qassar <jqassar@gmail.com>\n---\n Documentation/config.txt | 13 +++++++++++++\n http.c                   | 36 ++++++++++++++++++++++++++++++++++++\n 2 files changed, 49 insertions(+)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex c67038b..d155620 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1440,16 +1440,29 @@ http.sslVerify::\n \tover HTTPS. Can be overridden by the 'GIT_SSL_NO_VERIFY' environment\n \tvariable.\n \n+http.sslEngine::\n+\tString specifying the SSL engine to be used by curl.  This can be used to\n+\tspecify non-default or dynamically loaded engines.  Can be overridden by\n+\tthe 'GIT_SSL_ENGINE' environment variable.\n+\n http.sslCert::\n \tFile containing the SSL certificate when fetching or pushing\n \tover HTTPS. Can be overridden by the 'GIT_SSL_CERT' environment\n \tvariable.\n \n+http.sslCertType::\n+\tSpecifies the format of the certificate to curl as one of (PEM|DER|ENG).\n+\tCan be overridden by the 'GIT_SSL_CERTTYPE' environment variable.\n+\n http.sslKey::\n \tFile containing the SSL private key when fetching or pushing\n \tover HTTPS. Can be overridden by the 'GIT_SSL_KEY' environment\n \tvariable.\n \n+http.sslKeyType::\n+\tSpecifies the format of the private key to curl as one of (PEM|DER|ENG).\n+\tCan be overridden by the 'GIT_SSL_KEYTYPE' environment variable.\n+\n http.sslCertPasswordProtected::\n \tEnable Git's password prompt for the SSL certificate.  Otherwise\n \tOpenSSL will prompt the user, possibly many times, if the\ndiff --git a/http.c b/http.c\nindex 92aba59..06cb22e 100644\n--- a/http.c\n+++ b/http.c\n@@ -49,6 +49,10 @@ static struct credential http_auth = CREDENTIAL_INIT;\n static int http_proactive_auth;\n static const char *user_agent;\n \n+static const char *ssl_keytype;\n+static const char *ssl_certtype;\n+static const char *ssl_engine;\n+\n #if LIBCURL_VERSION_NUM >= 0x071700\n /* Use CURLOPT_KEYPASSWD as is */\n #elif LIBCURL_VERSION_NUM >= 0x070903\n@@ -211,6 +215,17 @@ static int http_options(const char *var, const char *value, void *cb)\n \tif (!strcmp(\"http.useragent\", var))\n \t\treturn git_config_string(&user_agent, var, value);\n \n+\t/* Adding parsing for curl options relating to engines and */\n+\t/* key/cert types.  This is necessary if attempting to     */\n+\t/* specify an external engine (e.g. for smartcards.)       */\n+\n+\tif (!strcmp(\"http.sslkeytype\", var))\n+\t\treturn git_config_string(&ssl_keytype, var, value);\n+\tif (!strcmp(\"http.sslcerttype\", var))\n+\t\treturn git_config_string(&ssl_certtype, var, value);\n+\tif (!strcmp(\"http.sslengine\", var))\n+\t\treturn git_config_string(&ssl_engine, var, value);\n+\n \t/* Fall back on the default ones */\n \treturn git_default_config(var, value, cb);\n }\n@@ -321,6 +336,22 @@ static CURL *get_curl_handle(void)\n \t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n \t}\n \n+\t/* Adding setting of engine-related curl SSL options. */\n+\tif (ssl_engine != NULL) {\n+\t\tcurl_easy_setopt(result, CURLOPT_SSLENGINE, ssl_engine);\n+\n+\t\t/* Within the lifetime of a single git execution, setting\n+\t\t * the default does nothing interesting.  When curl properly\n+\t\t * duplicates handles, the engine choice will propagate.\n+\t\t */\n+\t\t/* curl_easy_setopt(result, CURLOPT_SSLENGINE_DEFAULT, 1L); */\n+\t}\n+\n+\tif (ssl_keytype != NULL)\n+\t\tcurl_easy_setopt(result, CURLOPT_SSLKEYTYPE, ssl_keytype);\n+\tif (ssl_certtype != NULL)\n+\t\tcurl_easy_setopt(result, CURLOPT_SSLCERTTYPE, ssl_certtype);\n+\n \treturn result;\n }\n \n@@ -405,6 +436,11 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)\n \t\t\tssl_cert_password_required = 1;\n \t}\n \n+\t/* Added environment variables for expanded engine-related options. */\n+\tset_from_env(&ssl_keytype, \"GIT_SSL_KEYTYPE\");\n+\tset_from_env(&ssl_certtype, \"GIT_SSL_CERTTYPE\");\n+\tset_from_env(&ssl_engine, \"GIT_SSL_ENGINE\");\n+\n #ifndef NO_CURL_EASY_DUPHANDLE\n \tcurl_default = get_curl_handle();\n #endif\n-- \n1.8.2.1.342.gfa7285d.dirty\n"},{"id":"216001","messageId":"7v61z4ezlz.fsf@alter.siamese.dyndns.org","threadId":"33584","inReplyTo":"1366758207-7724-1-git-send-email-jqassar@gmail.com","subject":"Re: [PATCH] http.c: Add config options/parsing for SSL engine vars","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-30T16:45:44Z","receivedAt":"2013-04-30T16:45:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Does anybody familiar with the http codepath have comments on this?\n\nThe authoritative source\n\nhttps://github.com/bagder/curl/blob/master/docs/libcurl/symbols-in-versions\n\ntells me that the CURLOPT_* used in this patch are available since\ncURL 7.9.3, but I see a #if LIBCURL_VERSION_NUM < 0x070704 in http.h\nso these may have to be protected in a similar way.\n\n[the entire message unsnipped for reference]\nJerry Qassar <jqassar@gmail.com> writes:\n\n> curl provides many options for configuring the way it negotiates an SSL\n> connection (with its default OpenSSL support), including ways to define\n> the SSL engine used, and parameters to set the format of the key and\n> certificate used.  Unfortunately, git does not parse some of the\n> critical ones needed by curl to support PKCS#11.\n>\n> Add the following git config variables (and direct env-set variables):\n>\n> * http.sslengine\n>   A string variable that sets CURLOPT_SSLENGINE on the back end.\n>   Can be set from environment using GIT_SSL_ENGINE.\n> * http.sslkeytype\n>   A string variable, either PEM/DER/ENG, that sets CURLOPT_SSLKEYTYPE.\n>   Can be set from environment using GIT_SSL_KEYTYPE.\n> * http.sslcerttype\n>   A string variable, either PEM/DER/ENG, that sets CURLOPT_SSLCERTTYPE.\n>   Can be set from environment using GIT_SSL_CERTTYPE.\n>\n> Parsing these new variables combined with related patches to curl\n> will allow git to support native authentication with smart cards.\n>\n> Note: It's difficult to test this without the related curl patches,\n> which I will be submitting soon.  At the very least, leaving these new\n> options unset doesn't break anything, and setting them has little\n> effect without the back-end curl changes needed to 'turn on' PKCS#11.\n> Any suggestions would be greatly appreciated.\n>\n> Signed-off-by: Jerry Qassar <jqassar@gmail.com>\n> ---\n>  Documentation/config.txt | 13 +++++++++++++\n>  http.c                   | 36 ++++++++++++++++++++++++++++++++++++\n>  2 files changed, 49 insertions(+)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index c67038b..d155620 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1440,16 +1440,29 @@ http.sslVerify::\n>  \tover HTTPS. Can be overridden by the 'GIT_SSL_NO_VERIFY' environment\n>  \tvariable.\n>  \n> +http.sslEngine::\n> +\tString specifying the SSL engine to be used by curl.  This can be used to\n> +\tspecify non-default or dynamically loaded engines.  Can be overridden by\n> +\tthe 'GIT_SSL_ENGINE' environment variable.\n> +\n>  http.sslCert::\n>  \tFile containing the SSL certificate when fetching or pushing\n>  \tover HTTPS. Can be overridden by the 'GIT_SSL_CERT' environment\n>  \tvariable.\n>  \n> +http.sslCertType::\n> +\tSpecifies the format of the certificate to curl as one of (PEM|DER|ENG).\n> +\tCan be overridden by the 'GIT_SSL_CERTTYPE' environment variable.\n> +\n>  http.sslKey::\n>  \tFile containing the SSL private key when fetching or pushing\n>  \tover HTTPS. Can be overridden by the 'GIT_SSL_KEY' environment\n>  \tvariable.\n>  \n> +http.sslKeyType::\n> +\tSpecifies the format of the private key to curl as one of (PEM|DER|ENG).\n> +\tCan be overridden by the 'GIT_SSL_KEYTYPE' environment variable.\n> +\n>  http.sslCertPasswordProtected::\n>  \tEnable Git's password prompt for the SSL certificate.  Otherwise\n>  \tOpenSSL will prompt the user, possibly many times, if the\n> diff --git a/http.c b/http.c\n> index 92aba59..06cb22e 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -49,6 +49,10 @@ static struct credential http_auth = CREDENTIAL_INIT;\n>  static int http_proactive_auth;\n>  static const char *user_agent;\n>  \n> +static const char *ssl_keytype;\n> +static const char *ssl_certtype;\n> +static const char *ssl_engine;\n> +\n>  #if LIBCURL_VERSION_NUM >= 0x071700\n>  /* Use CURLOPT_KEYPASSWD as is */\n>  #elif LIBCURL_VERSION_NUM >= 0x070903\n> @@ -211,6 +215,17 @@ static int http_options(const char *var, const char *value, void *cb)\n>  \tif (!strcmp(\"http.useragent\", var))\n>  \t\treturn git_config_string(&user_agent, var, value);\n>  \n> +\t/* Adding parsing for curl options relating to engines and */\n> +\t/* key/cert types.  This is necessary if attempting to     */\n> +\t/* specify an external engine (e.g. for smartcards.)       */\n> +\n> +\tif (!strcmp(\"http.sslkeytype\", var))\n> +\t\treturn git_config_string(&ssl_keytype, var, value);\n> +\tif (!strcmp(\"http.sslcerttype\", var))\n> +\t\treturn git_config_string(&ssl_certtype, var, value);\n> +\tif (!strcmp(\"http.sslengine\", var))\n> +\t\treturn git_config_string(&ssl_engine, var, value);\n> +\n>  \t/* Fall back on the default ones */\n>  \treturn git_default_config(var, value, cb);\n>  }\n> @@ -321,6 +336,22 @@ static CURL *get_curl_handle(void)\n>  \t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n>  \t}\n>  \n> +\t/* Adding setting of engine-related curl SSL options. */\n> +\tif (ssl_engine != NULL) {\n> +\t\tcurl_easy_setopt(result, CURLOPT_SSLENGINE, ssl_engine);\n> +\n> +\t\t/* Within the lifetime of a single git execution, setting\n> +\t\t * the default does nothing interesting.  When curl properly\n> +\t\t * duplicates handles, the engine choice will propagate.\n> +\t\t */\n> +\t\t/* curl_easy_setopt(result, CURLOPT_SSLENGINE_DEFAULT, 1L); */\n> +\t}\n> +\n> +\tif (ssl_keytype != NULL)\n> +\t\tcurl_easy_setopt(result, CURLOPT_SSLKEYTYPE, ssl_keytype);\n> +\tif (ssl_certtype != NULL)\n> +\t\tcurl_easy_setopt(result, CURLOPT_SSLCERTTYPE, ssl_certtype);\n> +\n>  \treturn result;\n>  }\n>  \n> @@ -405,6 +436,11 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)\n>  \t\t\tssl_cert_password_required = 1;\n>  \t}\n>  \n> +\t/* Added environment variables for expanded engine-related options. */\n> +\tset_from_env(&ssl_keytype, \"GIT_SSL_KEYTYPE\");\n> +\tset_from_env(&ssl_certtype, \"GIT_SSL_CERTTYPE\");\n> +\tset_from_env(&ssl_engine, \"GIT_SSL_ENGINE\");\n> +\n>  #ifndef NO_CURL_EASY_DUPHANDLE\n>  \tcurl_default = get_curl_handle();\n>  #endif\n"},{"id":"216037","messageId":"20130430182732.GB1972@sigill.intra.peff.net","threadId":"33584","inReplyTo":"7v61z4ezlz.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] http.c: Add config options/parsing for SSL engine vars","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-30T18:27:33Z","receivedAt":"2013-04-30T18:27:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 30, 2013 at 09:45:44AM -0700, Junio C Hamano wrote:\n\n> The authoritative source\n> \n> https://github.com/bagder/curl/blob/master/docs/libcurl/symbols-in-versions\n> \n> tells me that the CURLOPT_* used in this patch are available since\n> cURL 7.9.3, but I see a #if LIBCURL_VERSION_NUM < 0x070704 in http.h\n> so these may have to be protected in a similar way.\n\nYeah, I think we'd want to protect it with an #if. More on the exact\nversion below.\n\n> [the entire message unsnipped for reference]\n> Jerry Qassar <jqassar@gmail.com> writes:\n> \n> > curl provides many options for configuring the way it negotiates an SSL\n> > connection (with its default OpenSSL support), including ways to define\n> > the SSL engine used, and parameters to set the format of the key and\n> > certificate used.  Unfortunately, git does not parse some of the\n> > critical ones needed by curl to support PKCS#11.\n\nI don't know anything about curl's PKCS#11 support, so I can't comment\ntoo much on how these options work. But from my reading of the\ncurl_easy_setopt(3), these look right, and I can comment on the http.c\nbits.\n\n> > * http.sslkeytype\n> >   A string variable, either PEM/DER/ENG, that sets CURLOPT_SSLKEYTYPE.\n> >   Can be set from environment using GIT_SSL_KEYTYPE.\n> > * http.sslcerttype\n> >   A string variable, either PEM/DER/ENG, that sets CURLOPT_SSLCERTTYPE.\n> >   Can be set from environment using GIT_SSL_CERTTYPE.\n> >\n> > Parsing these new variables combined with related patches to curl\n> > will allow git to support native authentication with smart cards.\n\nCurrent curl seems to take ENG only for the key, and assumes you have\nthe certificate on disk (which should be fine, as it is not a secret). I\ncan imagine that the user experience with a smartcard is better, though,\nif it can use ENG to just pull the cert straight from the card. Is that\nwhat your curl patches are for (my comments below are all predicated on\nthat assumption)?\n\nSince these options are selected from a finite set, we need to consider\nwho is responsible for complaining to the user when they set\n\"http.sslkeytype\" to \"foobar\". It probably makes sense to pass that\nresponsibility along to curl, so that as it grows in capabilities, we do\nnot have to keep adjusting our list (and it would already catch passing\nsslcerttype=ENG to versions of curl without your patches).\n\nWould we need the #if for the libcurl version to reference the new\nrelease of curl that has your patches? I think we don't, as existing\nversions would just complain when getting sslcerttype=ENG. IOW, it is\nnot a build-time problem, but a run-time problem.\n\n> > Note: It's difficult to test this without the related curl patches,\n> > which I will be submitting soon.  At the very least, leaving these new\n> > options unset doesn't break anything, and setting them has little\n> > effect without the back-end curl changes needed to 'turn on' PKCS#11.\n> > Any suggestions would be greatly appreciated.\n\nI think you'd be able to set the types to DER and test that, though the\ncurl_easy_setopt manpage notes that a bug in OpenSSL prevents this from\nworking.\n\nThe ENG method, even with the right curl support, is probably going to\nbe hard to test (unless there is some mock engine we can plug into\nOpenSSL for testing).\n\nI don't think we currently test even the basics of client certs in the\ntest suite. A good first step might be to check that the existing sslkey\nand sslcert options work, followed by this patch and checking whether\nsslkeytype=DER works.\n\n> >  http.sslKey::\n> >  \tFile containing the SSL private key when fetching or pushing\n> >  \tover HTTPS. Can be overridden by the 'GIT_SSL_KEY' environment\n> >  \tvariable.\n> >  \n> > +http.sslKeyType::\n> > +\tSpecifies the format of the private key to curl as one of (PEM|DER|ENG).\n> > +\tCan be overridden by the 'GIT_SSL_KEYTYPE' environment variable.\n> > +\n\nSetting this to ENG impacts how http.sslKey is interpreted (and of\ncourse whether http.sslEngine is used at all). We might want to mention\nthat here.\n\n> > @@ -211,6 +215,17 @@ static int http_options(const char *var, const char *value, void *cb)\n> >  \tif (!strcmp(\"http.useragent\", var))\n> >  \t\treturn git_config_string(&user_agent, var, value);\n> >  \n> > +\t/* Adding parsing for curl options relating to engines and */\n> > +\t/* key/cert types.  This is necessary if attempting to     */\n> > +\t/* specify an external engine (e.g. for smartcards.)       */\n\n  /*\n   * Style nit: our multi-line comments usually look\n   * like this. No right-hand border, and no text on\n   * the top or bottom lines.\n   */\n\n> > +\tif (!strcmp(\"http.sslkeytype\", var))\n> > +\t\treturn git_config_string(&ssl_keytype, var, value);\n> > +\tif (!strcmp(\"http.sslcerttype\", var))\n> > +\t\treturn git_config_string(&ssl_certtype, var, value);\n> > +\tif (!strcmp(\"http.sslengine\", var))\n> > +\t\treturn git_config_string(&ssl_engine, var, value);\n\nWe aren't checking the validity of the \"type\" fields here, which I\nthink is OK. But then later, we do:\n\n> > +\tif (ssl_keytype != NULL)\n> > +\t\tcurl_easy_setopt(result, CURLOPT_SSLKEYTYPE, ssl_keytype);\n> > +\tif (ssl_certtype != NULL)\n> > +\t\tcurl_easy_setopt(result, CURLOPT_SSLCERTTYPE, ssl_certtype);\n\nShouldn't we be checking the result of curl_easy_setopt for errors here\n(and when the engine cannot be loaded)?  I think we should probably die\nif the engine can't be loaded, but at the very least we'd want to warn\nthe user that their settings are being ignored.\n\n> > @@ -321,6 +336,22 @@ static CURL *get_curl_handle(void)\n> [...ssl handle setup...]\n\nGood, this is the right function in which to do that setup (I mention it\nmostly because some of the setup happens in get_active_slot, and we just\nfixed a bug the other day where some setup was in the wrong spot).\n\n> > +\t/* Adding setting of engine-related curl SSL options. */\n> > +\tif (ssl_engine != NULL) {\n> > +\t\tcurl_easy_setopt(result, CURLOPT_SSLENGINE, ssl_engine);\n\nThis comment does not seem to say anything that is not already obvious,\nand seems more for the reader of the diff than a reader of the code in\ngeneral. Such comments should stay in the commit message; I think you\nhave already explained yourself well there, so we can just drop it here.\n\n> > +\t\t/* Within the lifetime of a single git execution, setting\n> > +\t\t * the default does nothing interesting.  When curl properly\n> > +\t\t * duplicates handles, the engine choice will propagate.\n> > +\t\t */\n> > +\t\t/* curl_easy_setopt(result, CURLOPT_SSLENGINE_DEFAULT, 1L); */\n\nIf it is not doing anything, we can just drop this whole block, can't\nwe?\n\nI am unclear from reading the curl docs whether one would need to call\nSSLENGINE_DEFAULT or not, though. Maybe your patches have some impact on\nthat.\n\n> > @@ -405,6 +436,11 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)\n> >  \t\t\tssl_cert_password_required = 1;\n> >  \t}\n> >  \n> > +\t/* Added environment variables for expanded engine-related options. */\n> > +\tset_from_env(&ssl_keytype, \"GIT_SSL_KEYTYPE\");\n> > +\tset_from_env(&ssl_certtype, \"GIT_SSL_CERTTYPE\");\n> > +\tset_from_env(&ssl_engine, \"GIT_SSL_ENGINE\");\n\nThese come after the config but before we have used the values in\nget_curl_handle, so they should properly take precedence. Good.\n\n\nOverall, I think it is looking good. Aside from a few style/cleanup\nissues, my only real complaint is the lack of error-checking from\ncurl_easy_setopt.\n\nAnd of course adding some tests while you are working in the area would\nbe very nice. :)\n\n-Peff\n"},{"id":"216053","messageId":"CAJC3a=pU2m8LpUh0u0XXXgOiDiPo-QAdA471orGs9jcyDJTU5g@mail.gmail.com","threadId":"33584","inReplyTo":"20130430182732.GB1972@sigill.intra.peff.net","subject":"Re: [PATCH] http.c: Add config options/parsing for SSL engine vars","fromName":"Jerry Qassar","fromEmail":"jqassar@gmail.com","sentAt":"2013-04-30T20:04:17Z","receivedAt":"2013-04-30T20:04:17Z","isPatch":true,"sender":{"key":"jqassar@gmail.com","avatar":"https://gravatar.com/avatar/3352ceac69b346f383b7f980aac62b69bfec5a69ec7880feb41ab04ede73cc2f?d=mp&s=160"},"body":"On Tue, Apr 30, 2013 at 11:27 AM, Jeff King <peff@peff.net> wrote:\n> On Tue, Apr 30, 2013 at 09:45:44AM -0700, Junio C Hamano wrote:\n>\n>> The authoritative source\n>>\n>> https://github.com/bagder/curl/blob/master/docs/libcurl/symbols-in-versions\n>>\n>> tells me that the CURLOPT_* used in this patch are available since\n>> cURL 7.9.3, but I see a #if LIBCURL_VERSION_NUM < 0x070704 in http.h\n>> so these may have to be protected in a similar way.\n>\n> Yeah, I think we'd want to protect it with an #if. More on the exact\n> version below.\n\nFirst, thanks very much for taking a look at this.  I wasn't 100% certain about\nthe versioning to use for it (specifically the version-to-0x mapping), so any\ninput on that would be a big help.  I'll try to answer your questions below...\n\n>\n>> [the entire message unsnipped for reference]\n>> Jerry Qassar <jqassar@gmail.com> writes:\n>>\n>> > curl provides many options for configuring the way it negotiates an SSL\n>> > connection (with its default OpenSSL support), including ways to define\n>> > the SSL engine used, and parameters to set the format of the key and\n>> > certificate used.  Unfortunately, git does not parse some of the\n>> > critical ones needed by curl to support PKCS#11.\n>\n> I don't know anything about curl's PKCS#11 support, so I can't comment\n> too much on how these options work. But from my reading of the\n> curl_easy_setopt(3), these look right, and I can comment on the http.c\n> bits.\n>\n>> > * http.sslkeytype\n>> >   A string variable, either PEM/DER/ENG, that sets CURLOPT_SSLKEYTYPE.\n>> >   Can be set from environment using GIT_SSL_KEYTYPE.\n>> > * http.sslcerttype\n>> >   A string variable, either PEM/DER/ENG, that sets CURLOPT_SSLCERTTYPE.\n>> >   Can be set from environment using GIT_SSL_CERTTYPE.\n>> >\n>> > Parsing these new variables combined with related patches to curl\n>> > will allow git to support native authentication with smart cards.\n>\n> Current curl seems to take ENG only for the key, and assumes you have\n> the certificate on disk (which should be fine, as it is not a secret). I\n> can imagine that the user experience with a smartcard is better, though,\n> if it can use ENG to just pull the cert straight from the card. Is that\n> what your curl patches are for (my comments below are all predicated on\n> that assumption)?\n\nThe concomitant curl patch(es; I need to separate them) are mentioned here:\n\nhttp://curl.haxx.se/mail/lib-2013-04/0336.html\n\nCurl already does support engine-based certificates (in code and\nhelp).  Its problem\nis that a) it doesn't yet read your engine defs out of OpenSSL config,\nand b) a bug\nin copying the engine data, once that's patched, to the handle that\ncalling apps use.\n\nOn git's side we just need to expose the proper options for setopt\nconfiguration.  No\nspecial work is needed to support them otherwise.\n\n>\n> Since these options are selected from a finite set, we need to consider\n> who is responsible for complaining to the user when they set\n> \"http.sslkeytype\" to \"foobar\". It probably makes sense to pass that\n> responsibility along to curl, so that as it grows in capabilities, we do\n> not have to keep adjusting our list (and it would already catch passing\n> sslcerttype=ENG to versions of curl without your patches).\n>\n> Would we need the #if for the libcurl version to reference the new\n> release of curl that has your patches? I think we don't, as existing\n> versions would just complain when getting sslcerttype=ENG. IOW, it is\n> not a build-time problem, but a run-time problem.\n\nI can answer this for you as I runtime-tested this with much swearing\nafter making\nthe initial git changes. :)  It's curl's responsibility to parse what\nit gets from the\nsetopt calls and shoot errors back--for example, when I fixed the\nconf-reading and\nthe git option-setting but not the handle cloning, I would get:\n\nerror: crypto engine not set, can't load certificate while accessing ...\n\ngit doesn't do any validation of any curl options that I see, other\nthan type matching.\nFor example, git doesn't check the value of ssl_key itself, it just\npasses it on.\n\n>> > Note: It's difficult to test this without the related curl patches,\n>> > which I will be submitting soon.  At the very least, leaving these new\n>> > options unset doesn't break anything, and setting them has little\n>> > effect without the back-end curl changes needed to 'turn on' PKCS#11.\n>> > Any suggestions would be greatly appreciated.\n>\n> I think you'd be able to set the types to DER and test that, though the\n> curl_easy_setopt manpage notes that a bug in OpenSSL prevents this from\n> working.\n>\n> The ENG method, even with the right curl support, is probably going to\n> be hard to test (unless there is some mock engine we can plug into\n> OpenSSL for testing).\n>\n> I don't think we currently test even the basics of client certs in the\n> test suite. A good first step might be to check that the existing sslkey\n> and sslcert options work, followed by this patch and checking whether\n> sslkeytype=DER works.\n\nI can't think of a way to reliably provide a hardware-dependent engine\nimplementation\nto the suite.  So ENG is probably out, but I can think of something to\nverify PEM and\nDER once I figure out how the test suite works.\n\n>\n>> >  http.sslKey::\n>> >     File containing the SSL private key when fetching or pushing\n>> >     over HTTPS. Can be overridden by the 'GIT_SSL_KEY' environment\n>> >     variable.\n>> >\n>> > +http.sslKeyType::\n>> > +   Specifies the format of the private key to curl as one of (PEM|DER|ENG).\n>> > +   Can be overridden by the 'GIT_SSL_KEYTYPE' environment variable.\n>> > +\n>\n> Setting this to ENG impacts how http.sslKey is interpreted (and of\n> course whether http.sslEngine is used at all). We might want to mention\n> that here.\n>\n\nGood point.  I'll add something in for it.\n\n>> > @@ -211,6 +215,17 @@ static int http_options(const char *var, const char *value, void *cb)\n>> >     if (!strcmp(\"http.useragent\", var))\n>> >             return git_config_string(&user_agent, var, value);\n>> >\n>> > +   /* Adding parsing for curl options relating to engines and */\n>> > +   /* key/cert types.  This is necessary if attempting to     */\n>> > +   /* specify an external engine (e.g. for smartcards.)       */\n>\n>   /*\n>    * Style nit: our multi-line comments usually look\n>    * like this. No right-hand border, and no text on\n>    * the top or bottom lines.\n>    */\n>\n\nYes, my fault.\n\n>> > +   if (!strcmp(\"http.sslkeytype\", var))\n>> > +           return git_config_string(&ssl_keytype, var, value);\n>> > +   if (!strcmp(\"http.sslcerttype\", var))\n>> > +           return git_config_string(&ssl_certtype, var, value);\n>> > +   if (!strcmp(\"http.sslengine\", var))\n>> > +           return git_config_string(&ssl_engine, var, value);\n>\n> We aren't checking the validity of the \"type\" fields here, which I\n> think is OK. But then later, we do:\n>\n>> > +   if (ssl_keytype != NULL)\n>> > +           curl_easy_setopt(result, CURLOPT_SSLKEYTYPE, ssl_keytype);\n>> > +   if (ssl_certtype != NULL)\n>> > +           curl_easy_setopt(result, CURLOPT_SSLCERTTYPE, ssl_certtype);\n>\n> Shouldn't we be checking the result of curl_easy_setopt for errors here\n> (and when the engine cannot be loaded)?  I think we should probably die\n> if the engine can't be loaded, but at the very least we'd want to warn\n> the user that their settings are being ignored.\n\nErrors are handled by curl (up to this point):\n\n1) Setting the cert type to FOO:\nerror: not supported file type 'FOO' for certificate...\nfatal: HTTP request failed\n\n2) Setting the key type to FOO:\nerror: not supported file type for private key...\nfatal: HTTP request failed\n\n3) Setting engine type to something invalid:\n * SSL Engine 'pkcsfoo' not found (only with GIT_CURL_VERBOSE set)\nerror: crypto engine not set, can't load certificate...\nfatal: HTTP request failed\n\n...that last one could probably be a little better, but it's curl-managed.\n\n>\n>> > @@ -321,6 +336,22 @@ static CURL *get_curl_handle(void)\n>> [...ssl handle setup...]\n>\n> Good, this is the right function in which to do that setup (I mention it\n> mostly because some of the setup happens in get_active_slot, and we just\n> fixed a bug the other day where some setup was in the wrong spot).\n>\n>> > +   /* Adding setting of engine-related curl SSL options. */\n>> > +   if (ssl_engine != NULL) {\n>> > +           curl_easy_setopt(result, CURLOPT_SSLENGINE, ssl_engine);\n>\n> This comment does not seem to say anything that is not already obvious,\n> and seems more for the reader of the diff than a reader of the code in\n> general. Such comments should stay in the commit message; I think you\n> have already explained yourself well there, so we can just drop it here.\n>\n>> > +           /* Within the lifetime of a single git execution, setting\n>> > +            * the default does nothing interesting.  When curl properly\n>> > +            * duplicates handles, the engine choice will propagate.\n>> > +            */\n>> > +           /* curl_easy_setopt(result, CURLOPT_SSLENGINE_DEFAULT, 1L); */\n>\n> If it is not doing anything, we can just drop this whole block, can't\n> we?\n>\n> I am unclear from reading the curl docs whether one would need to call\n> SSLENGINE_DEFAULT or not, though. Maybe your patches have some impact on\n> that.\n>\n\nNo, they don't have any obvious impact and I can probably drop both comments.\nSince git executions are finite, if you start with the engine you're\nending with the\nsame engine, and making it default for this operation doesn't affect much.\n\nMy feeling in this case is that if you step into the lair of the\nengine dragon you void\nyour warranty.  A proper warning sign in the documentation about the\nspecification\nof engines is probably a good idea.\n\n>> > @@ -405,6 +436,11 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)\n>> >                     ssl_cert_password_required = 1;\n>> >     }\n>> >\n>> > +   /* Added environment variables for expanded engine-related options. */\n>> > +   set_from_env(&ssl_keytype, \"GIT_SSL_KEYTYPE\");\n>> > +   set_from_env(&ssl_certtype, \"GIT_SSL_CERTTYPE\");\n>> > +   set_from_env(&ssl_engine, \"GIT_SSL_ENGINE\");\n>\n> These come after the config but before we have used the values in\n> get_curl_handle, so they should properly take precedence. Good.\n>\n>\n> Overall, I think it is looking good. Aside from a few style/cleanup\n> issues, my only real complaint is the lack of error-checking from\n> curl_easy_setopt.\n>\n> And of course adding some tests while you are working in the area would\n> be very nice. :)\n>\n\nThanks very much for the constructive input.  Once I make the corrections and\ndetermine how to make some appropriate tests I'll resubmit.  I guess\nmy open question\nis, if you wish to wrap the prop setting in a curl version #if, what\nversion is desired?\n"},{"id":"216057","messageId":"7vzjwfbwow.fsf@alter.siamese.dyndns.org","threadId":"33584","inReplyTo":"CAJC3a=pU2m8LpUh0u0XXXgOiDiPo-QAdA471orGs9jcyDJTU5g@mail.gmail.com","subject":"Re: [PATCH] http.c: Add config options/parsing for SSL engine vars","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-30T20:17:03Z","receivedAt":"2013-04-30T20:17:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jerry Qassar <jqassar@gmail.com> writes:\n\n> Curl already does support engine-based certificates (in code and\n> help).  Its problem is that a) it doesn't yet read your engine\n> defs out of OpenSSL config, and b) a bug in copying the engine\n> data, once that's patched, to the handle that calling apps use.\n\nSo once the problem (a) is fixed, if the user has OpenSSL config\nthen the user doesn't need configuration from setopt() side?  That\nmakes it sound like you do not need to patch us at all, but there\nmust be something else going on...\n\n> On git's side we just need to expose the proper options for setopt\n> configuration.  No special work is needed to support them\n> otherwise.\n\n... I am somewhat puzzled.\n\n>>> > +   if (ssl_keytype != NULL)\n>>> > +           curl_easy_setopt(result, CURLOPT_SSLKEYTYPE, ssl_keytype);\n>>> > +   if (ssl_certtype != NULL)\n>>> > +           curl_easy_setopt(result, CURLOPT_SSLCERTTYPE, ssl_certtype);\n>>\n>> Shouldn't we be checking the result of curl_easy_setopt for errors here\n>> (and when the engine cannot be loaded)?  I think we should probably die\n>> if the engine can't be loaded, but at the very least we'd want to warn\n>> the user that their settings are being ignored.\n>\n> Errors are handled by curl (up to this point):\n>\n> 1) Setting the cert type to FOO:\n> error: not supported file type 'FOO' for certificate...\n> fatal: HTTP request failed\n>\n> 2) Setting the key type to FOO:\n> error: not supported file type for private key...\n> fatal: HTTP request failed\n>\n> 3) Setting engine type to something invalid:\n>  * SSL Engine 'pkcsfoo' not found (only with GIT_CURL_VERBOSE set)\n> error: crypto engine not set, can't load certificate...\n> fatal: HTTP request failed\n\nWhere do \"error:\" and \"fatal:\" happen in the codeflow?\n\nI am guessing that \"error:\" may come from these easy_setopt() calls, but\nthe \"fatal: HTTP request failed\" come from us, much later in the\ncallpath when we actually make http request.\n\nBetween these two times, aren't we throwing user data at the cURL\nlibrary and possibly over the wire to the remote side (with a SSL\nconfiguration that is different from what the user intended to use),\nno?\n\nThat does not sound like a \"managed by cURL\" solution to me.\nShouldn't we notice the first error and abort without doing any\nfurther damage?\n\n>> Overall, I think it is looking good. Aside from a few style/cleanup\n>> issues, my only real complaint is the lack of error-checking from\n>> curl_easy_setopt.\n>>\n>> And of course adding some tests while you are working in the area would\n>> be very nice. :)\n> ...\n> I guess my open question is, if you wish to wrap the\n> prop setting in a curl version #if, what version is desired?\n\nhttps://github.com/bagder/curl/blob/master/docs/libcurl/symbols-in-versions\n\nsays that SSLKEYTYPE, SSLCERTTYPE, etc. come from 7.9.3, so it would\nbe \n\n        #if LIBCURL_VERSION_NUM >= 0x070903\n            ...\n        #endif\n\nThanks.\n"},{"id":"216058","messageId":"20130430202242.GA3247@sigill.intra.peff.net","threadId":"33584","inReplyTo":"CAJC3a=pU2m8LpUh0u0XXXgOiDiPo-QAdA471orGs9jcyDJTU5g@mail.gmail.com","subject":"Re: [PATCH] http.c: Add config options/parsing for SSL engine vars","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-30T20:22:42Z","receivedAt":"2013-04-30T20:22:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 30, 2013 at 01:04:17PM -0700, Jerry Qassar wrote:\n\n> First, thanks very much for taking a look at this.  I wasn't 100% certain about\n> the versioning to use for it (specifically the version-to-0x mapping), so any\n> input on that would be a big help.  I'll try to answer your questions below...\n\nThe explanation is hiding in a comment in curlver.h:\n\n  This is the numeric version of the libcurl version number, meant for\n  easier parsing and comparions by programs. The LIBCURL_VERSION_NUM\n  define will always follow this syntax:\n\n         0xXXYYZZ\n\n  Where XX, YY and ZZ are the main version, release and patch numbers in\n  hexadecimal (using 8 bits each). All three numbers are always\n  represented using two digits.  1.2 would appear as \"0x010200\" while\n  version 9.11.7 appears as \"0x090b07\".\n\nSo I think you'd just want to check:\n\n  #if LIBCURL_VERSION_NUM >= 0x070903\n\nIt looks like we already have such an #if block for ssl_key, so you\ncould just add the new options there.\n\n> > Current curl seems to take ENG only for the key, and assumes you have\n> > the certificate on disk [...]\n> [...]\n> Curl already does support engine-based certificates (in code and\n> help).\n\nMy \"seems to...\" above was based on reading the curl_easy_setopt\nmanpage. It sounds like that documentation may be out of date.\n\n> I can't think of a way to reliably provide a hardware-dependent engine\n> implementation to the suite.  So ENG is probably out, but I can think\n> of something to verify PEM and DER once I figure out how the test\n> suite works.\n\nI think the trickiest part for testing PEM/DER support will actually be\nsetting up the apache config to authenticate a user based on a client\nside certificate. I was hoping you might know off-hand since that is\nobviously the goal of your patch (though of course you might not be\nusing apache). The relevant config would be in t/lib-httpd/apache.conf,\nand the tests could probably go into t/t5551-http-fetch.sh.\n\n> > Shouldn't we be checking the result of curl_easy_setopt for errors here\n> > (and when the engine cannot be loaded)?  I think we should probably die\n> > if the engine can't be loaded, but at the very least we'd want to warn\n> > the user that their settings are being ignored.\n> \n> Errors are handled by curl (up to this point):\n> \n> 1) Setting the cert type to FOO:\n> error: not supported file type 'FOO' for certificate...\n> fatal: HTTP request failed\n> \n> 2) Setting the key type to FOO:\n> error: not supported file type for private key...\n> fatal: HTTP request failed\n> \n> 3) Setting engine type to something invalid:\n>  * SSL Engine 'pkcsfoo' not found (only with GIT_CURL_VERBOSE set)\n> error: crypto engine not set, can't load certificate...\n> fatal: HTTP request failed\n> \n> ...that last one could probably be a little better, but it's curl-managed.\n\nHmm. So all of that happens when we actually try to make the request. I\nthink that should be OK. Curl is presumably smart enough to fail early\nin the *_perform() functions, when it notices the handle is in a bogus\nstate.\n\n> Thanks very much for the constructive input.  Once I make the\n> corrections and determine how to make some appropriate tests I'll\n> resubmit.  I guess my open question is, if you wish to wrap the prop\n> setting in a curl version #if, what version is desired?\n\n>From my reading of the curl docs, it's 7.9.3.\n\n-Peff\n"},{"id":"216059","messageId":"20130430202908.GB3247@sigill.intra.peff.net","threadId":"33584","inReplyTo":"7vzjwfbwow.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] http.c: Add config options/parsing for SSL engine vars","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-30T20:29:08Z","receivedAt":"2013-04-30T20:29:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 30, 2013 at 01:17:03PM -0700, Junio C Hamano wrote:\n\n> Jerry Qassar <jqassar@gmail.com> writes:\n> \n> > Curl already does support engine-based certificates (in code and\n> > help).  Its problem is that a) it doesn't yet read your engine\n> > defs out of OpenSSL config, and b) a bug in copying the engine\n> > data, once that's patched, to the handle that calling apps use.\n> \n> So once the problem (a) is fixed, if the user has OpenSSL config\n> then the user doesn't need configuration from setopt() side?  That\n> makes it sound like you do not need to patch us at all, but there\n> must be something else going on...\n\nMy understanding is that we first have to tell curl \"yes, use the\nengine\", and then the engine-specific OpenSSL config can be loaded by\ncurl. But I am just guessing from the conversation up until now; I know\nnothing about ssl crypto engines.\n\nAs an aside, curl can be linked against gnutls, too. Does any of this\nwork with gnutls? I think we don't have to care; curl abstracts all of\nthat away from us, and it is up to the user to choose an engine that\nmatches their library versions. But it might be a good point of\nreference when somebody later comes to the list and says \"I followed the\ndocumentation, but it doesn't work\".\n\n> > Errors are handled by curl (up to this point):\n> >\n> > 1) Setting the cert type to FOO:\n> > error: not supported file type 'FOO' for certificate...\n> > fatal: HTTP request failed\n> >\n> > 2) Setting the key type to FOO:\n> > error: not supported file type for private key...\n> > fatal: HTTP request failed\n> >\n> > 3) Setting engine type to something invalid:\n> >  * SSL Engine 'pkcsfoo' not found (only with GIT_CURL_VERBOSE set)\n> > error: crypto engine not set, can't load certificate...\n> > fatal: HTTP request failed\n> \n> Where do \"error:\" and \"fatal:\" happen in the codeflow?\n>\n> I am guessing that \"error:\" may come from these easy_setopt() calls, but\n> the \"fatal: HTTP request failed\" come from us, much later in the\n> callpath when we actually make http request.\n\nThose are almost certainly from curl_errorstr() when we make the\ninfo/refs http request.\n\n> Between these two times, aren't we throwing user data at the cURL\n> library and possibly over the wire to the remote side (with a SSL\n> configuration that is different from what the user intended to use),\n> no?\n\nI assume that curl is smart enough not to send any data over the wire,\nand that it is noticing early in the process that something is wrong and\nis barfing there.\n\nIt would be nicer to notice earlier (when we are setting up the handle),\nbut in practice I don't think it matters. We start off all http\nconversations by making a short GET, and we don't do any significant\nwork beforehand. So as long as curl does not do significant work before\nhitting those errors internally, it probably does not matter much either\nway.\n\n-Peff\n"},{"id":"216063","messageId":"CAJC3a=qu8jxJ2pD=4kbPVqQDBgv6vxXVXkt6ezqRoPdtMEfjRA@mail.gmail.com","threadId":"33584","inReplyTo":"20130430202908.GB3247@sigill.intra.peff.net","subject":"Re: [PATCH] http.c: Add config options/parsing for SSL engine vars","fromName":"Jerry Qassar","fromEmail":"jqassar@gmail.com","sentAt":"2013-04-30T21:05:22Z","receivedAt":"2013-04-30T21:05:22Z","isPatch":true,"sender":{"key":"jqassar@gmail.com","avatar":"https://gravatar.com/avatar/3352ceac69b346f383b7f980aac62b69bfec5a69ec7880feb41ab04ede73cc2f?d=mp&s=160"},"body":"On Tue, Apr 30, 2013 at 1:29 PM, Jeff King <peff@peff.net> wrote:\n> On Tue, Apr 30, 2013 at 01:17:03PM -0700, Junio C Hamano wrote:\n>\n>> Jerry Qassar <jqassar@gmail.com> writes:\n>>\n>> > Curl already does support engine-based certificates (in code and\n>> > help).  Its problem is that a) it doesn't yet read your engine\n>> > defs out of OpenSSL config, and b) a bug in copying the engine\n>> > data, once that's patched, to the handle that calling apps use.\n>>\n>> So once the problem (a) is fixed, if the user has OpenSSL config\n>> then the user doesn't need configuration from setopt() side?  That\n>> makes it sound like you do not need to patch us at all, but there\n>> must be something else going on...\n>\n> My understanding is that we first have to tell curl \"yes, use the\n> engine\", and then the engine-specific OpenSSL config can be loaded by\n> curl. But I am just guessing from the conversation up until now; I know\n> nothing about ssl crypto engines.\n\nThat's correct.  Putting the engine definitions into OpenSSL\nconfiguration makes them available to OpenSSL only.  I made changes to\nhave curl/libcurl read the config files (it didn't before).  git needs\nchanges to let it set the appropriate libcurl parameters to set the\nengine and key/cert type.  Otherwise libcurl has the capability but\ngit can't utilize it.\n\n>\n> As an aside, curl can be linked against gnutls, too. Does any of this\n> work with gnutls? I think we don't have to care; curl abstracts all of\n> that away from us, and it is up to the user to choose an engine that\n> matches their library versions. But it might be a good point of\n> reference when somebody later comes to the list and says \"I followed the\n> documentation, but it doesn't work\".\n\ngnutls doesn't let you specify an 'engine' per se and the underlying\nengine calls return CURLE_NOT_BUILT_IN, so these changes would have no\neffect.  That doesn't mean that gnutls can't work (I know for a fact\nthat it's possible for it to speak CAC), but I'm not sure that\nanything git does internally matters to it.\n\nMaybe the documentation also needs to say specifically that it is for OpenSSL?\n\n>> > Errors are handled by curl (up to this point):\n>> >\n>> > 1) Setting the cert type to FOO:\n>> > error: not supported file type 'FOO' for certificate...\n>> > fatal: HTTP request failed\n>> >\n>> > 2) Setting the key type to FOO:\n>> > error: not supported file type for private key...\n>> > fatal: HTTP request failed\n>> >\n>> > 3) Setting engine type to something invalid:\n>> >  * SSL Engine 'pkcsfoo' not found (only with GIT_CURL_VERBOSE set)\n>> > error: crypto engine not set, can't load certificate...\n>> > fatal: HTTP request failed\n>>\n>> Where do \"error:\" and \"fatal:\" happen in the codeflow?\n>>\n>> I am guessing that \"error:\" may come from these easy_setopt() calls, but\n>> the \"fatal: HTTP request failed\" come from us, much later in the\n>> callpath when we actually make http request.\n>\n> Those are almost certainly from curl_errorstr() when we make the\n> info/refs http request.\n>\n>> Between these two times, aren't we throwing user data at the cURL\n>> library and possibly over the wire to the remote side (with a SSL\n>> configuration that is different from what the user intended to use),\n>> no?\n>\n> I assume that curl is smart enough not to send any data over the wire,\n> and that it is noticing early in the process that something is wrong and\n> is barfing there.\n>\n> It would be nicer to notice earlier (when we are setting up the handle),\n> but in practice I don't think it matters. We start off all http\n> conversations by making a short GET, and we don't do any significant\n> work beforehand. So as long as curl does not do significant work before\n> hitting those errors internally, it probably does not matter much either\n> way.\n>\n> -Peff\n\n Your surmise is correct here; you don't make it past the handshake.\n\n--Jerry\n"}]}