nss: fix build with disabled proxy support #5667
Conversation
Avoid reference to fields that do not exist when CURL_DISABLE_PROXY is defined.
|
I wonder if we could introduce additional macros here to avoid the excessive use of |
|
That would be nice, but do you have any idea on how we could do it? |
|
Something like this? The patch applies on top of this PR but I can submit it as a separate PR on top of (or bellow) this PR if you like it: --- a/lib/vtls/nss.c
+++ b/lib/vtls/nss.c
@@ -1027,12 +1027,7 @@ static SECStatus BadCertHandler(void *arg, PRFileDesc *sock)
CERTCertificate *cert;
/* remember the cert verification result */
-#ifndef CURL_DISABLE_PROXY
- if(SSL_IS_PROXY())
- data->set.proxy_ssl.certverifyresult = err;
- else
-#endif
- data->set.ssl.certverifyresult = err;
+ SSL_SET_OPTION_LVALUE(certverifyresult) = err;
if(err == SSL_ERROR_BAD_CERT_DOMAIN && !SSL_CONN_CONFIG(verifyhost))
/* we are asked not to verify the host name */
@@ -1838,12 +1833,6 @@ static CURLcode nss_setup_connect(struct connectdata *conn, int sockindex)
CURLcode result;
bool second_layer = FALSE;
SSLVersionRange sslver_supported;
-#ifndef CURL_DISABLE_PROXY
- const char *hostname = SSL_IS_PROXY() ? conn->http_proxy.host.name :
- conn->host.name;
-#else
- const char *hostname = conn->host.name;
-#endif
SSLVersionRange sslver = {
SSL_LIBRARY_VERSION_TLS_1_0, /* min */
@@ -1948,12 +1937,7 @@ static CURLcode nss_setup_connect(struct connectdata *conn, int sockindex)
goto error;
/* not checked yet */
-#ifndef CURL_DISABLE_PROXY
- if(SSL_IS_PROXY())
- data->set.proxy_ssl.certverifyresult = 0;
- else
-#endif
- data->set.ssl.certverifyresult = 0;
+ SSL_SET_OPTION_LVALUE(certverifyresult) = 0;
if(SSL_BadCertHook(model, BadCertHandler, conn) != SECSuccess)
goto error;
@@ -2124,11 +2108,11 @@ static CURLcode nss_setup_connect(struct connectdata *conn, int sockindex)
goto error;
/* propagate hostname to the TLS layer */
- if(SSL_SetURL(backend->handle, hostname) != SECSuccess)
+ if(SSL_SetURL(backend->handle, SSL_HOST_NAME()) != SECSuccess)
goto error;
/* prevent NSS from re-using the session for a different hostname */
- if(SSL_SetSockPeerID(backend->handle, hostname) != SECSuccess)
+ if(SSL_SetSockPeerID(backend->handle, SSL_HOST_NAME()) != SECSuccess)
goto error;
return CURLE_OK;
@@ -2147,18 +2131,6 @@ static CURLcode nss_do_connect(struct connectdata *conn, int sockindex)
struct Curl_easy *data = conn->data;
CURLcode result = CURLE_SSL_CONNECT_ERROR;
PRUint32 timeout;
-#ifndef CURL_DISABLE_PROXY
- long * const certverifyresult = SSL_IS_PROXY() ?
- &data->set.proxy_ssl.certverifyresult : &data->set.ssl.certverifyresult;
- const char * const pinnedpubkey = SSL_IS_PROXY() ?
- data->set.str[STRING_SSL_PINNEDPUBLICKEY_PROXY] :
- data->set.str[STRING_SSL_PINNEDPUBLICKEY_ORIG];
-#else
- long * const certverifyresult = &data->set.ssl.certverifyresult;
- const char * const pinnedpubkey =
- data->set.str[STRING_SSL_PINNEDPUBLICKEY_ORIG];
-#endif
-
/* check timeout situation */
const timediff_t time_left = Curl_timeleft(data, NULL, TRUE);
@@ -2174,9 +2146,9 @@ static CURLcode nss_do_connect(struct connectdata *conn, int sockindex)
if(PR_GetError() == PR_WOULD_BLOCK_ERROR)
/* blocking direction is updated by nss_update_connecting_state() */
return CURLE_AGAIN;
- else if(*certverifyresult == SSL_ERROR_BAD_CERT_DOMAIN)
+ else if(SSL_SET_OPTION(certverifyresult) == SSL_ERROR_BAD_CERT_DOMAIN)
result = CURLE_PEER_FAILED_VERIFICATION;
- else if(*certverifyresult != 0)
+ else if(SSL_SET_OPTION(certverifyresult) != 0)
result = CURLE_PEER_FAILED_VERIFICATION;
goto error;
}
@@ -2204,7 +2176,7 @@ static CURLcode nss_do_connect(struct connectdata *conn, int sockindex)
}
}
- result = cmp_peer_pubkey(connssl, pinnedpubkey);
+ result = cmp_peer_pubkey(connssl, SSL_PINNED_PUB_KEY());
if(result)
/* status already printed */
goto error;
diff --git a/lib/vtls/vtls.h b/lib/vtls/vtls.h
index bcc844416..f4a67f39e 100644
--- a/lib/vtls/vtls.h
+++ b/lib/vtls/vtls.h
@@ -131,12 +131,23 @@ CURLcode Curl_none_md5sum(unsigned char *input, size_t inputlen,
CURL_SOCKET_BAD ? FIRSTSOCKET : SECONDARYSOCKET].state)
#define SSL_SET_OPTION(var) \
(SSL_IS_PROXY() ? data->set.proxy_ssl.var : data->set.ssl.var)
+#define SSL_SET_OPTION_LVALUE(var) \
+ (*(SSL_IS_PROXY() ? &data->set.proxy_ssl.var : &data->set.ssl.var))
#define SSL_CONN_CONFIG(var) \
(SSL_IS_PROXY() ? conn->proxy_ssl_config.var : conn->ssl_config.var)
+#define SSL_HOST_NAME() \
+ (SSL_IS_PROXY() ? conn->http_proxy.host.name : conn->host.name)
+#define SSL_PINNED_PUB_KEY() (SSL_IS_PROXY() \
+ ? data->set.str[STRING_SSL_PINNEDPUBLICKEY_PROXY] \
+ : data->set.str[STRING_SSL_PINNEDPUBLICKEY_ORIG])
#else
#define SSL_IS_PROXY() FALSE
#define SSL_SET_OPTION(var) data->set.ssl.var
+#define SSL_SET_OPTION_LVALUE(var) data->set.ssl.var
#define SSL_CONN_CONFIG(var) conn->ssl_config.var
+#define SSL_HOST_NAME() conn->host.name
+#define SSL_PINNED_PUB_KEY() \
+ data->set.str[STRING_SSL_PINNEDPUBLICKEY_ORIG]
#endif
bool Curl_ssl_config_matches(struct ssl_primary_config *data, |
|
@kdudka I think it'll be better if you do it as a separate PR. Also, take a look and see if the same macros/style can be used for the other backends, as it would certainly be cool if we could apply the same logic on them all. |
|
So should I wait till this and the related pull requests are merged, or open a separate pull request against master now and let others deal with all the conflicts? As for the other backends, I am pretty sure that it will simplify their code as well. But I will only patch the backends which I am able to locally compile curl against. |
You can use Buildroot to build curl against all it supported backends, one backend at a time. These currently include openssl, bearssl, gnutls, nss, mbedtls, and wolfssl. |
|
@baruchsiach How long is the edit-compile-test loop with Buildroot? Could you please point me to any tutorial that describes how to quickly check with Buildroot that my curl patch works (or at least compiles)? |
Here is a quick howto build curl for the ARM64 target. I'm using a pre-built toolchain here to save built time. To build against a different backend do Buildroot can also build a bootable image with qemu to boot it in an emulated environment. A terse readme is at |
That is probably the easiest. I'll merge this now and you can create a separate PR for your work. I can then chip in on a few TLS backends I can build. |
|
@baruchsiach Thanks for the howto. It looks like an interesting project but it would be overkill for my needs. I do not really need to cross-compile anything. After typing |
Buildroot builds host tar because of unfortunate effect of a (not so) recent tar bugfix on archives reproducibility: https://git.buildroot.net/buildroot/commit/?id=b8fa273d500b44153e9939f0a100e97db2ff63ed Buildroot depends on tar generating reproducible archive to verify hash of non-tar source trees. I have a locally built tar version 1.29 in my PATH to save Buildroot build time. The advantage of Buildroot in this case is the ease of build against various crypto backends. |
Avoid reference to fields that do not exist when CURL_DISABLE_PROXY is
defined.