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

Update to rustls-ffi 0.10.0 #10865

Closed
wants to merge 1 commit into from
Closed

Update to rustls-ffi 0.10.0 #10865

wants to merge 1 commit into from

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Mar 29, 2023

This brings in version 0.21.0 of the upstream rustls implementation, which notable includes support for IP address certificates. This allows enabling many of the unittests that relied on IP address certificates to communicate with localhost.

@github-actions github-actions bot added CI Continuous Integration tests labels Mar 29, 2023
@jsha
Copy link
Contributor Author

jsha commented Mar 29, 2023

Interesting. The test failure is under address_sanitizer, but appears to be a failed assertion indicating that a connection was reused more than expected (one connect vs two).

=================================== FAILURES ===================================
_________________ TestDownload.test_02_07_download_reuse[0-h2] _________________

self = <test_02_download.TestDownload object at 0x7f[29](https://github.com/curl/curl/actions/runs/4557958324/jobs/8040275432?pr=10865#step:24:30)b0feab90>
env = <testenv.env.Env object at 0x7f29b0e3d720>
httpd = <testenv.httpd.Httpd object at 0x7f29b0ffeb90>
nghttpx = <testenv.nghttpx.Nghttpx object at 0x7f29b0ffd1b0>, repeat = 0
proto = 'h2'

    @pytest.mark.parametrize("proto", ['h2', 'h3'])
    def test_02_07_download_reuse(self, env: Env,
                                  httpd, nghttpx, repeat, proto):
        if proto == 'h3' and not env.have_h3():
            pytest.skip("h3 not supported")
        count = 200
        curl = CurlClient(env=env)
        urln = f'https://{env.authority_for(env.domain1, proto)}/data.json?[0-{count-1}]'
        r = curl.http_download(urls=[urln], alpn_proto=proto,
                               with_stats=True, extra_args=[
            '--parallel', '--parallel-max', '200'
        ])
        r.check_exit_code(0)
        r.check_stats(count=count, exp_status=200)
        # should have used 2 connections only (test servers allow 100 req/conn)
>       assert r.total_connects == 2, "h2 should use fewer connections here"
E       AssertionError: h2 should use fewer connections here
E       assert 1 == 2
E        +  where 1 = ExecResult[code=0, exception=None, args=['/home/runner/work/curl/curl/src/curl', '-s', '--path-as-is', '-v', '--http2'... done: 0\n', '* Connection #0 to host one.http.curl.se left intact\n', '* Expire cleared (transfer 0x6220001[30](https://github.com/curl/curl/actions/runs/4557958324/jobs/8040275432?pr=10865#step:24:31)908)\n']].total_connects

tests/http/test_02_download.py:163: AssertionError
=========================== short test summary info ============================
FAILED tests/http/test_02_download.py::TestDownload::test_02_07_download_reuse[0-h2] - AssertionError: h2 should use fewer connections here
assert 1 == 2
 +  where 1 = ExecResult[code=0, exception=None, args=['/home/runner/work/curl/curl/src/curl', '-s', '--path-as-is', '-v', '--http2'... done: 0\n', '* Connection #0 to host one.http.curl.se left intact\n', '* Expire cleared (transfer 0x622000130908)\n']].total_connects
============= 1 failed, 67 passed, [52](https://github.com/curl/curl/actions/runs/4557958324/jobs/8040275432?pr=10865#step:24:53) skipped in 62.67s (0:01:02) ==============

@jsha
Copy link
Contributor Author

jsha commented Mar 29, 2023

@icing I see you've been working on the pytests. Any idea why this would fail just on address_sanitizer?

@icing
Copy link
Contributor

icing commented Mar 30, 2023

@icing I see you've been working on the pytests. Any idea why this would fail just on address_sanitizer?

First guess: the curl client becomes so slow, that apache answers the requests before the first connection runs out of space. So, a second one is never needed.

Hmm...we could change the assert to <= 2.

Added #10870 for exactly this.

@bagder bagder closed this in 041cf77 Mar 30, 2023
@bagder bagder reopened this Mar 30, 2023
@bagder
Copy link
Member

bagder commented Mar 30, 2023

Closed by mistake, sorry

@pheiduck
Copy link
Contributor

pheiduck commented Apr 3, 2023

This tests are still fail:

TESTFAIL: These test cases failed: 400 401 403 406 407 408 409 987 988 989 1112 

@bagder bagder added the TLS label Apr 10, 2023
Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests are failing

@jsha
Copy link
Contributor Author

jsha commented Apr 26, 2023

Yep, have been meaning to look into this more. Apologies for the delay.

@jsha
Copy link
Contributor Author

jsha commented Jun 23, 2023

I'm still working on diagnosing why those tests are failing but it turns out to not be IP address support. I've reverted the diffs to DISABLED so this now just bumps the ruslts-ffi dependency.

This brings in version 0.21.0 of the upstream rustls implementation,
which notable includes support for IP address certificates.
@jsha
Copy link
Contributor Author

jsha commented Jul 11, 2023

There are a handful of CI jobs that I see consistently failing on this branch, but I think I didn't touch anything related:

  • c-ares
  • LibreSSL http2
  • cmake clang LibreSSL
  • cmake gcc-12 LibreSSL

Here's some example output from cmake clang LibreSSL:

ld: warning: dylib (/usr/local/lib/libssh2.dylib) was built for newer macOS version (12.0) than being linked (10.15)
Undefined symbols for architecture x86_64:
  "_EVP_PKEY_get_bn_param", referenced from:
      _Curl_ossl_certchain in unity_0_c.c.o
  "_EVP_PKEY_get_id", referenced from:
      _Curl_ossl_certchain in unity_0_c.c.o
      _cert_stuff in unity_0_c.c.o

bch pushed a commit to bch/curl that referenced this pull request Jul 19, 2023
Only 1 connection will be used when curl is slow, happens when
address-sanitized in CI, for example

Closes curl#10865
@bagder
Copy link
Member

bagder commented Jul 22, 2023

The libressl problems were due to a home brew problem. Rebase and push again and they should be fixed.

@bagder
Copy link
Member

bagder commented Jul 22, 2023

(never mind) Thanks!

@bagder bagder closed this in 69c536b Jul 22, 2023
ptitSeb pushed a commit to wasix-org/curl that referenced this pull request Sep 25, 2023
This brings in version 0.21.0 of the upstream rustls implementation,
which notable includes support for IP address certificates.

Closes curl#10865
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration tests TLS
Development

Successfully merging this pull request may close these issues.

4 participants