Rm dep openssl #1742

NobodyXu · 2023-04-21T05:21:18Z

This is split from #1738 , to remove dependency on openssl in src/bin/sccache-dist/token_check.rs.

This also disabled default-feature of reqwest and dev-dep thirtyfour_sync which pulled in openssl which was a mistake in #1737 since I didn't realize that reqwest pulls in openssl by default.

Signed-off-by: Jiahao XU [email protected]

codecov-commenter · 2023-04-21T05:29:32Z

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (6ff88eb) 30.87% compared to head (99f01f9) 30.66%.

Files	Patch %	Lines
src/util.rs	0.00%	12 Missing ⚠️
src/dist/http.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1742      +/-   ##
==========================================
- Coverage   30.87%   30.66%   -0.21%     
==========================================
  Files          51       51              
  Lines       19419    19505      +86     
  Branches     9348     9425      +77     
==========================================
- Hits         5995     5981      -14     
- Misses       7790     7955     +165     
+ Partials     5634     5569      -65

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sylvestre · 2023-04-21T05:47:18Z

I guess you saw : error[E0599]: the method context exists for enum Result<UintRef<'_>, Error>, but its trait bounds were not satisfied
--> src/bin/sccache-dist/token_check.rs:41:44
|
41 | let e_bn = pkcs1::UintRef::new(&e).context("Failed to create pkcs1 bignum from e")?;

NobodyXu · 2023-04-21T06:00:03Z

@sylvestre Thanks, I have fixed this error.

NobodyXu · 2023-04-21T06:01:16Z

This PR is currently blocked on RustCrypto/formats#1013 to avoid pkcs8 to be pulled in.

NobodyXu · 2023-04-21T14:17:27Z

That PR has merged, but latest der v0.7.4 also requires rust 1.65.0

Since current msrv for sccache is 1.64.0, I think it's ok to bump it to 1.65.0

NobodyXu · 2023-04-22T09:43:18Z

Blocked on tomaka/rouille#272

NobodyXu · 2023-04-24T05:03:38Z

@Xuanwo Can we re-open this?
I didn't know that using the fix to refer to a PR would close it.

NobodyXu · 2023-04-24T05:06:10Z

Thank you @Xuanwo !
Once tomaka/rouille#272 is merged, I will mark this PR as ready-to-review.

NobodyXu · 2023-04-24T13:37:28Z

@sylvestre It seems that disabling default-feature default-tls of reqwest (which pulls in openssl) might be what breaks the "ci / test ubuntu-22.04 rust 1.65.0 dist-server".

It seems that reqwest prefers openssl when it is enabled by default, so when we disable default-tls, we forces reqwest to use rustls and that seems to be what causes the failure.

NobodyXu · 2023-04-24T13:59:21Z

The error in "ci/test ubuntu-22.04 rust 1.65.0 dist-server" actually changed to:

test test_dist_basic ... FAILED
[2023-04-24T13:54:52Z DEBUG sccache::config] Attempting to read config file at "/tmp/sccache_dist_testvfnrqa/sccache-cfg.json"
test test_dist_failingserver ... sccache: Starting the server...
[2023-04-24T13:54:52Z DEBUG sccache::config] Attempting to read config file at "/tmp/sccache_dist_testvfnrqa/sccache-cfg.json"
[2023-04-24T13:55:22Z ERROR rustls::conn] TLS alert received: AlertMessagePayload {
        level: Fatal,
        description: BadCertificate,
    }
ok
[2023-04-24T13:55:22Z DEBUG sccache::config] Attempting to read config file at "/tmp/sccache_dist_testiVFYye/sccache-cfg.json"
test test_dist_nobuilder ... sccache: Starting the server...
[2023-04-24T13:55:22Z DEBUG sccache::config] Attempting to read config file at "/tmp/sccache_dist_testiVFYye/sccache-cfg.json"
ok
test test_dist_restartedserver ... FAILED

failures:

error: test failed, to rerun pass `--test dist`
---- test_dist_basic stdout ----
thread 'main' panicked at 'wait timed out, last error result: Connection refused (os error 111)', tests/harness/mod.rs:649:5
stack backtrace:
   0: rust_begin_unwind
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/panicking.rs:142:14
   2: dist::harness::wait_for
             at ./tests/harness/mod.rs:649:5
   3: dist::harness::wait_for_http
             at ./tests/harness/mod.rs:622:5
   4: dist::harness::DistSystem::wait_server_ready
             at ./tests/harness/mod.rs:442:9
   5: dist::harness::DistSystem::add_server
             at ./tests/harness/mod.rs:383:9
   6: dist::test_dist_basic
             at ./tests/dist.rs:73:5
   7: dist::test_dist_basic::{{closure}}
             at ./tests/dist.rs:63:1
   8: core::ops::function::FnOnce::call_once
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/ops/function.rs:248:5
   9: core::ops::function::FnOnce::call_once
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/ops/function.rs:248:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

NobodyXu · 2023-04-24T14:01:11Z

Seems like rustls failed due to bad certificate.
Since this also happened in #1739, I don't think the changes to create_https_cert_and_privkey is responsible for this.

sylvestre · 2023-05-15T10:09:22Z

ci / test ubuntu-22.04 rust 1.65.0 dist-server (pull_request) Failing after 6m still fails

NobodyXu · 2023-05-15T10:15:24Z

ci / test ubuntu-22.04 rust 1.65.0 dist-server (pull_request) Failing after 6m still fails

@sylvestre It's blocked on a new release from reqwest seanmonstar/reqwest#1791 (comment) , which updates rustls to 0.21

Support for IP address peer is added in rustls 0.21 rustls/rustls#184

drahnr · 2023-05-17T11:03:03Z

seanmonstar/reqwest#1834 0.11.18 was released, so there shouldn't be any blockers left

NobodyXu · 2023-05-17T11:19:55Z

@drahnr I've updated reqwest to v0.11.8 and rustls to v0.21.0, it indeed fixed the bad certificate error, but now we got a new error:

[2023-05-17T02:11:22Z ERROR rustls::conn] TLS alert received: AlertMessagePayload {
        level: Fatal,
        description: UnknownCA,
    }

Seems like it rejects the certificate due to unknown ca, but not sure how to fix this.

drahnr · 2023-05-17T11:35:05Z

src/dist/http.rs

-            .context("failed to create digest of x509 certificate")?
-            .as_ref()
-            .to_owned();
+        let mut rng = OsRng;


Could you add a feature gated test, that verifies the certificates generated are identical? It appears the issue seen after porting can originate from a variety of name validation issues i.e. briansmith/webpki#20 so I'd recommend to check consistency against the openssl generated cert first, verify such a openssl generated cert with the logic of rustls and then start debugging into the unit test that should start failing.

If it's not your cup of tea, leave a message here :)

I can certainly add a new unit test to ensure the two different methods produces identical certificates, but I'm a bit busy recently

Although I don't think this is caused by briansmith/webpki#20
dist-server CI uses a self-signed CA AFAIK, so it naturally does not have a ca and it matches the error message from rustls:

[2023-05-17T02:11:22Z ERROR rustls::conn] TLS alert received: AlertMessagePayload { level: Fatal, description: UnknownCA, }

In fact the new ca generation code explicitly excludes ca.

Some input here #1742 (comment)

Do we have an integration test to verify 1:1 reproduction of the certificate? We'd need that for the dist case to remain backward compatible with older "clients".

No, I'm going to write this by running external openssl command.

You only need to include a previously generated certificate (i.e. from the current master/main branch), and include it as a reference to compare against / ensure compatibility with.

@drahnr I don't think it's possible, since the original openssl and the new code both uses the current time when creating the certificate.

Otherwise I could just use a statically embedded certificate.

a. generate with rustls directly
b. openssl -> file fmt -> parse certificate with rustls

Compare all fields of a and b, except for those that are reasonable to expect to deviate.

sylvestre · 2023-05-17T11:41:02Z

Thanks & no worries, no rush :)

drahnr · 2023-11-20T10:47:23Z

Side quest @sylvestre - we should consider using cargo-deny to prevent future regressions on accidentally re-adding openssl through dependencies.

sylvestre · 2023-11-20T10:54:06Z

Yes, sounds good :)

NobodyXu · 2023-11-20T12:44:13Z

I'm currently trying to upgrade trust-dns to hickory-dns v0.24 by using ClientBuilder::dns_resolver, however it seems that this method isn't present in the blocking version of client used in utils::new_reqwest_blocking_client

Xuanwo · 2023-11-20T12:48:26Z

however it seems that this method isn't present in the blocking version of client used in utils::new_reqwest_blocking_client

hickory-dns only provides async API and it's blocking API is a tokio::block_on wrapper.

Maybe it's ok to just use libc's dns resovler in blocking API?

NobodyXu · 2023-11-20T12:56:18Z

Maybe it's ok to just use libc's dns resovler in blocking API?

That's a good advice, I've applied it and hope that the CI will succeed.

NobodyXu · 2023-11-23T11:46:53Z

Updated trust-dns-resolver v0.22 to latest v0.24 (renamed to hickory-dns) and also updates webpki-roots to v0.25.

Confirmed that rustls v0.21 has been removed, but CI failure persists.

I think it's likely something in the certificate generation that caused the error.

- Bump rouille from 3.5 => 3.6.2 rouille v3.6.2 fixed a bug: `rouille::Server::new_ssl` is now exposed when only `rustls` is enabled. - Disable default features of `reqwest` which pulls in openssl - Rm `openssl` pulled in dev - Bump reqwest from 0.11.17 => 0.11.18 Signed-off-by: Jiahao XU <[email protected]>

Signed-off-by: Jiahao XU <[email protected]>

Hoping that they will fix the CI test failure Signed-off-by: Jiahao XU <[email protected]>

Signed-off-by: Jiahao XU <[email protected]>

Use CRLF on windows and `\n` on Linux. Also fix formatting of `Cargo.toml` Signed-off-by: Jiahao XU <[email protected]>

Signed-off-by: Jiahao XU <[email protected]>

sylvestre · 2024-02-22T07:51:08Z

Sorry, needs rebasing

NobodyXu · 2024-02-22T08:35:51Z

Sorry I was a bit busy recently, will rebase when I got time.

And the CI error is really tricky, I wasn't able to get it fixed.

AJIOB · 2024-02-28T10:18:06Z

src/util.rs

@@ -926,6 +926,37 @@ pub fn daemonize() -> Result<()> {
    Ok(())
 }

+#[cfg(any(feature = "dist-server", feature = "dist-client"))]
+mod reqwest_dns_resolver {


What about moving it to the dedicated .rs file?

Try reqwest-hickory-resolver that maintained by me 😄

AJIOB · 2024-05-13T03:46:28Z

Hi @NobodyXu,

Do you have a time for finalizing this PR?

NobodyXu · 2024-05-13T04:01:04Z

Hello

Honestly I don't know how to fix the CI failure.

I don't know openssl good enough and don't have much time recently.

NobodyXu force-pushed the dist-server-rewrite branch from abaa06d to aea80fd Compare April 21, 2023 05:23

NobodyXu force-pushed the dist-server-rewrite branch from aea80fd to e30d256 Compare April 21, 2023 05:52

NobodyXu mentioned this pull request Apr 21, 2023

Fix feature alloc and pem in pkcs1 RustCrypto/formats#1013

Merged

NobodyXu force-pushed the dist-server-rewrite branch 2 times, most recently from 48bb9e1 to 9e44a5a Compare April 21, 2023 13:53

NobodyXu mentioned this pull request Apr 22, 2023

Bump msrv to 1.65.0 #1743

Merged

NobodyXu force-pushed the dist-server-rewrite branch from 9e44a5a to 80ca974 Compare April 22, 2023 08:19

NobodyXu marked this pull request as draft April 22, 2023 09:35

NobodyXu force-pushed the dist-server-rewrite branch from 80ca974 to 09bb44b Compare April 22, 2023 09:42

NobodyXu changed the title ~~Use pkcs1 in Jwk::to_der_pkcs1 instead of openssl~~ Rm dep openssl pulled in by feature dist-server Apr 22, 2023

NobodyXu force-pushed the dist-server-rewrite branch from 09bb44b to aad03a1 Compare April 22, 2023 12:05

Xuanwo closed this in #1743 Apr 24, 2023

Xuanwo reopened this Apr 24, 2023

NobodyXu force-pushed the dist-server-rewrite branch 2 times, most recently from bdbf677 to 1eeb97e Compare April 24, 2023 12:53

NobodyXu marked this pull request as ready for review April 24, 2023 13:04

NobodyXu force-pushed the dist-server-rewrite branch from 88eeb04 to e0e075b Compare April 24, 2023 13:18

NobodyXu mentioned this pull request Apr 24, 2023

Rm openssl pulled in linux #1739

Closed

NobodyXu force-pushed the dist-server-rewrite branch from 2604815 to 2006932 Compare May 1, 2023 08:22

NobodyXu force-pushed the dist-server-rewrite branch 2 times, most recently from b4eeeeb to 50a3fda Compare May 17, 2023 02:43

drahnr reviewed May 17, 2023

View reviewed changes

NobodyXu force-pushed the dist-server-rewrite branch 2 times, most recently from 2d1dce1 to 923d94a Compare November 20, 2023 10:04

NobodyXu force-pushed the dist-server-rewrite branch 2 times, most recently from 3dd8526 to 364a0de Compare November 22, 2023 21:52

NobodyXu added 5 commits November 27, 2023 00:18

dep: Upgrade trust-dns-resolver v0.22 to hickory-resolver v0.24

bbada42

Signed-off-by: Jiahao XU <[email protected]>

Update transitive deps related to http

58d36e2

Hoping that they will fix the CI test failure Signed-off-by: Jiahao XU <[email protected]>

Update reqwest and webpki-roots

9cc4cd6

Signed-off-by: Jiahao XU <[email protected]>

Fix line ending of dynamic key generated

1e819f3

Use CRLF on windows and `\n` on Linux. Also fix formatting of `Cargo.toml` Signed-off-by: Jiahao XU <[email protected]>

NobodyXu force-pushed the dist-server-rewrite branch from 364a0de to 1e819f3 Compare November 26, 2023 13:18

Fix typo: Default() => default()

99f01f9

Signed-off-by: Jiahao XU <[email protected]>

AJIOB reviewed Feb 28, 2024

View reviewed changes

Rm dep openssl #1742

Are you sure you want to change the base?

Rm dep openssl #1742

Conversation

NobodyXu commented Apr 21, 2023 • edited Loading

codecov-commenter commented Apr 21, 2023 • edited Loading

Codecov Report

sylvestre commented Apr 21, 2023

NobodyXu commented Apr 21, 2023

NobodyXu commented Apr 21, 2023

NobodyXu commented Apr 21, 2023

NobodyXu commented Apr 22, 2023

NobodyXu commented Apr 24, 2023

NobodyXu commented Apr 24, 2023

NobodyXu commented Apr 24, 2023

NobodyXu commented Apr 24, 2023 • edited Loading

NobodyXu commented Apr 24, 2023

sylvestre commented May 15, 2023

NobodyXu commented May 15, 2023

drahnr commented May 17, 2023

NobodyXu commented May 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NobodyXu May 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drahnr Nov 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sylvestre commented May 17, 2023 via email

drahnr commented Nov 20, 2023

sylvestre commented Nov 20, 2023 via email

NobodyXu commented Nov 20, 2023 • edited Loading

Xuanwo commented Nov 20, 2023 • edited Loading

NobodyXu commented Nov 20, 2023

NobodyXu commented Nov 23, 2023

sylvestre commented Feb 22, 2024

NobodyXu commented Feb 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AJIOB commented May 13, 2024

NobodyXu commented May 13, 2024

NobodyXu commented Apr 21, 2023 •

edited

Loading

codecov-commenter commented Apr 21, 2023 •

edited

Loading

NobodyXu commented Apr 24, 2023 •

edited

Loading

NobodyXu May 17, 2023 •

edited

Loading

drahnr Nov 24, 2023 •

edited

Loading

NobodyXu commented Nov 20, 2023 •

edited

Loading

Xuanwo commented Nov 20, 2023 •

edited

Loading