Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

quic ngtcp2 not call CURLOPT_SSL_CTX_FUNCTION #10222

Closed
violetlige opened this issue Jan 4, 2023 · 6 comments
Closed

quic ngtcp2 not call CURLOPT_SSL_CTX_FUNCTION #10222

violetlige opened this issue Jan 4, 2023 · 6 comments
Assignees
Labels

Comments

@violetlige
Copy link

Package:

curl 7.87.0

File:

lib/vquic/ngtcp2.c

maybe should add this code in function 'Curl_quic_connect' (after quic_ssl_ctx)

because at mobile, verify cert through SSL_CTX_set_cert_verify_callback in CURLOPT_SSL_CTX_FUNCTION callback

if(data->set.ssl.fsslctx) {
    Curl_set_in_callback(data, true);
    result = (*data->set.ssl.fsslctx)(data, qs->sslctx,
                                      data->set.ssl.fsslctxp);
    Curl_set_in_callback(data, false);
    if(result) {
      failf(data, "error signaled by ssl ctx callback");
      return result;
    }
  }

and maybe should set SSL_VERIFY_PEER like this in function 'quic_ssl_ctx' (wolfssl and so on)

because at mobile, not config ssl_cafile and ssl_capath, verify cert through SSL_CTX_set_cert_verify_callback in CURLOPT_SSL_CTX_FUNCTION callback

  if(conn->ssl_config.verifypeer) {
    SSL_CTX_set_verify(ssl_ctx, SSL_VERIFY_PEER, NULL);

    const char * const ssl_cafile = conn->ssl_config.CAfile;
    const char * const ssl_capath = conn->ssl_config.CApath;
    if(ssl_cafile || ssl_capath) {
    .......
}
@bagder bagder added TLS HTTP/3 h3 or quic related libcurl API labels Jan 4, 2023
@icing icing self-assigned this Jan 4, 2023
@icing
Copy link
Contributor

icing commented Jan 4, 2023

This sounds like a very good idea!

@icing
Copy link
Contributor

icing commented Jan 5, 2023

Addressed the ctx callback in #10239.

Could you explain what you mean by the verify peer suggestion? I see support for this in the code, must be missing something. Thanks.

@violetlige
Copy link
Author

Addressed the ctx callback in #10239.

Could you explain what you mean by the verify peer suggestion? I see support for this in the code, must be missing something. Thanks.

@icing thank you for your quick response.

Because in my project(mobile products), i don't config STRING_SSL_CAPATH and STRING_SSL_CAFILE,
otherwise I use SSL_CTX_set_cert_verify_callback(through CURLOPT_SSL_CTX_FUNCTION callback to get ssl_ctx)
to verify server peer tls cert.In this way I can use system api(like 'SecTrust' api on ios) to verify server cert, and also
i can provide callback for others(who use my product) to verify server tls certs.

but in lib/vquic/ngtcp.c
if i not config STRING_SSL_CAPATH and STRING_SSL_CAFILE, SSL_VERIFY_PEER is not set.

curl/lib/vquic/ngtcp2.c

Lines 273 to 301 in c12fb3d

if(conn->ssl_config.verifypeer) {
const char * const ssl_cafile = conn->ssl_config.CAfile;
const char * const ssl_capath = conn->ssl_config.CApath;
if(ssl_cafile || ssl_capath) {
SSL_CTX_set_verify(ssl_ctx, SSL_VERIFY_PEER, NULL);
/* tell OpenSSL where to find CA certificates that are used to verify
the server's certificate. */
if(!SSL_CTX_load_verify_locations(ssl_ctx, ssl_cafile, ssl_capath)) {
/* Fail if we insist on successfully verifying the server. */
failf(data, "error setting certificate verify locations:"
" CAfile: %s CApath: %s",
ssl_cafile ? ssl_cafile : "none",
ssl_capath ? ssl_capath : "none");
return NULL;
}
infof(data, " CAfile: %s", ssl_cafile ? ssl_cafile : "none");
infof(data, " CApath: %s", ssl_capath ? ssl_capath : "none");
}
#ifdef CURL_CA_FALLBACK
else {
/* verifying the peer without any CA certificates won't work so
use openssl's built-in default as fallback */
SSL_CTX_set_default_verify_paths(ssl_ctx);
}
#endif
}
return ssl_ctx;
}

and in lib/vtls/openssl.c
SSL_VERIFY_PEER is only controlled by the verifypeer parameter(CURLOPT_SSL_VERIFYPEER)

curl/lib/vtls/openssl.c

Lines 3780 to 3785 in c12fb3d

/* OpenSSL always tries to verify the peer, this only says whether it should
* fail to connect if the verification fails, or if it should continue
* anyway. In the latter case the result of the verification is checked with
* SSL_get_verify_result() below. */
SSL_CTX_set_verify(backend->ctx,
verifypeer ? SSL_VERIFY_PEER : SSL_VERIFY_NONE, NULL);

and SSL_VERIFY_PEER is not binding with STRING_SSL_CAPATH and STRING_SSL_CAFILE

curl/lib/vtls/openssl.c

Lines 3258 to 3287 in c12fb3d

if(verifypeer && !imported_native_ca && (ssl_cafile || ssl_capath)) {
#if defined(OPENSSL_VERSION_MAJOR) && (OPENSSL_VERSION_MAJOR >= 3)
/* OpenSSL 3.0.0 has deprecated SSL_CTX_load_verify_locations */
if(ssl_cafile &&
!X509_STORE_load_file(store, ssl_cafile)) {
/* Fail if we insist on successfully verifying the server. */
failf(data, "error setting certificate file: %s", ssl_cafile);
return CURLE_SSL_CACERT_BADFILE;
}
if(ssl_capath &&
!X509_STORE_load_path(store, ssl_capath)) {
/* Fail if we insist on successfully verifying the server. */
failf(data, "error setting certificate path: %s", ssl_capath);
return CURLE_SSL_CACERT_BADFILE;
}
#else
/* tell OpenSSL where to find CA certificates that are used to verify the
server's certificate. */
if(!X509_STORE_load_locations(store, ssl_cafile, ssl_capath)) {
/* Fail if we insist on successfully verifying the server. */
failf(data, "error setting certificate verify locations:"
" CAfile: %s CApath: %s",
ssl_cafile ? ssl_cafile : "none",
ssl_capath ? ssl_capath : "none");
return CURLE_SSL_CACERT_BADFILE;
}
#endif
infof(data, " CAfile: %s", ssl_cafile ? ssl_cafile : "none");
infof(data, " CApath: %s", ssl_capath ? ssl_capath : "none");
}

so I think the code logic of openssl and ngtcp about SSL_VERIFY_PEER should be consistent。

icing added a commit to icing/curl that referenced this issue Jan 6, 2023
@bagder bagder linked a pull request Jan 6, 2023 that will close this issue
@icing
Copy link
Contributor

icing commented Jan 9, 2023

Understood, thanks for the explanation. I updated #10239 so that openssl setup works the same in ngtcp2 and plain TLS. This allows also the shared X509_STORE feature to work here.

@icing
Copy link
Contributor

icing commented Jan 10, 2023

@violetlige if this PR does what you expect, we'll merge it and close this ticket. WDYT?

@violetlige
Copy link
Author

@violetlige if this PR does what you expect, we'll merge it and close this ticket. WDYT?

@icing that is exactly what I expect, thank you very much for your quick processing.

@bagder bagder closed this as completed in 088c08a Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants