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

Use rustls for tls & trust-dns-resolver for dns resolution #1737

Merged
merged 4 commits into from Apr 19, 2023

Conversation

NobodyXu
Copy link
Contributor

@NobodyXu NobodyXu commented Apr 19, 2023

By default, reqwest uses native-tls, which is openssl on linux and that makes cross-compilation very hard as they need to compile openssl themselves and exposes it via pkg-config.

Enabling vendored-openssl does not help there because the dep:openssl is only enabled by dist-server.

reqwest by default uses libc's getaddrinfo, which is a poor solution that requires a dedicated thread and it is not nearly as reliable as trust-dns-resolver, not to mention libc implementation, e.g. musl, might deliberately leave out some features.

Signed-off-by: Jiahao XU Jiahao_XU@outlook.com

By default, reqwest uses native-tls, which is openssl on linux and that
makes cross-compilation very hard as they need to compile openssl
themselves and exposes it via pkg-config.

Enabling vendored-openssl does not help there because the dep:openssl is
only enabled by dist-server.

reqwest by default uses libc's getaddrinfo, which is a poor solution
that requires a dedicated thread and it is not nearly as reliable as
trust-dns-resolver, not to mention libc implementation, e.g. musl, might
deliberately leave out some features.

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Cargo.toml Show resolved Hide resolved
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu NobodyXu requested a review from Xuanwo April 19, 2023 12:35
@codecov-commenter
Copy link

codecov-commenter commented Apr 19, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.60 ⚠️

Comparison is base (6bbef54) 29.76% compared to head (c0c5c30) 29.16%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1737      +/-   ##
==========================================
- Coverage   29.76%   29.16%   -0.60%     
==========================================
  Files          49       49              
  Lines       17207    17452     +245     
  Branches     8321     8446     +125     
==========================================
- Hits         5121     5090      -31     
- Misses       7049     7292     +243     
- Partials     5037     5070      +33     

see 15 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Apr 19, 2023

The build failure is caused by quinn-udp: quinn-rs/quinn#1469

@Xuanwo I have disabled dns-over-quic for now.

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Copy link
Contributor Author

I've pushed again the fixed the Cargo.lock.

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Apr 19, 2023

@Xuanwo It seems that freebsd ci also failed on main with similar errors, so I think this PR should be ok to merge since it does not cause any regression.

Copy link
Collaborator

@drahnr drahnr left a comment

Choose a reason for hiding this comment

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

Did you verify the dependency tree is free of openssl? cargo tree -i openssl should do the trick, empty is good

@NobodyXu
Copy link
Contributor Author

@drahnr Yes, I've verified that with dist-server disabled, openssl is not pulled in.

I will open another PR to remove the openssl pulled in by dist-server with pkcs1::RsaPublicKey.

@NobodyXu
Copy link
Contributor Author

@drahnr I've created #1738 to be merged after this to fix the openssl pulled in by dist-servers.

It still needs some testing, since I'm using MacOS, I cannot test it locally without installing openssl for x86_64-linux, unless I rebase that PR above this one, which would make it a mess.

So I've pushed it and let the ci tested it out, hope it would be ok, though it's the first time for me to use pkcs1 and do anything related to crypto, so I might made some mistakes, hope you would review it carefully to verify that it has no such bug.

Thank you for taking your time to review my PR!

@drahnr drahnr merged commit 4a8b5f5 into mozilla:main Apr 19, 2023
36 of 37 checks passed
@NobodyXu NobodyXu deleted the use-rustls branch April 20, 2023 00:06
This was referenced Apr 20, 2023
@Ralith
Copy link

Ralith commented Aug 22, 2023

The build failure is caused by quinn-udp: quinn-rs/quinn#1469

That bug is for OpenBSD. Quinn CI covers FreeBSD, which seems to be working just fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants