Remove webpki crate - resolves #2183 #2185

Bravo555 · 2023-08-24T08:01:36Z

Proposed changes

Our dependencies were updated to latest versions so that they no longer use the unmaintained webpki crate.

Types of changes

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
Documentation Update (if none of the other choices apply)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

Checklist

I have read the CONTRIBUTING doc
I have signed the CLA (in all commits with git commit -s)
I ran cargo fmt as mentioned in CODING_GUIDELINES
I used cargo clippy as mentioned in CODING_GUIDELINES
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)

Further comments

github-actions · 2023-08-24T08:43:02Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
257	0	5	257	100	44m27.385999999s

codecov · 2023-08-27T21:34:16Z

Codecov Report

Merging #2185 (3cf6498) into main (e8881a3) will decrease coverage by 0.8%.
Report is 9 commits behind head on main.
The diff coverage is 45.4%.

Additional details and impacted files

Files Changed	Coverage Δ
...s/common/certificate/src/parse_root_certificate.rs	`67.1% <0.0%> (ø)`
crates/common/download/src/download.rs	`46.4% <0.0%> (-29.4%)`	⬇️
crates/common/mqtt_channel/src/config.rs	`49.1% <0.0%> (ø)`
...ore/tedge/src/cli/connect/c8y_direct_connection.rs	`0.0% <0.0%> (ø)`
crates/core/tedge/src/cli/connect/command.rs	`0.0% <0.0%> (ø)`
crates/core/tedge/src/cli/connect/jwt_token.rs	`49.4% <0.0%> (-1.9%)`	⬇️
crates/core/tedge/src/cli/mqtt/publish.rs	`40.7% <0.0%> (ø)`
crates/core/tedge/src/cli/mqtt/subscribe.rs	`0.0% <0.0%> (ø)`
...rates/extensions/tedge_downloader_ext/src/tests.rs	`0.0% <ø> (-93.8%)`	⬇️
crates/common/certificate/src/lib.rs	`87.4% <83.3%> (+6.6%)`	⬆️

... and 9 files with indirect coverage changes

didier-wenzek

The code is correct.

Before merging, it might be good to double check with @reubenmiller the impacts on the release build.

Cargo.toml

crates/common/certificate/src/lib.rs

didier-wenzek · 2023-08-29T08:55:25Z

Cargo.toml

 assert-json-diff = "2.0"
 assert_cmd = "2.0"
 assert_matches = "1.5"
 async-compat = "0.2.1"
 async-log = "2.0"
 async-trait = "0.1"
-async-tungstenite = { version = "0.18.0", features = [
-    "tokio-runtime",
-    "tokio-rustls-native-certs",


Removing the feature tokio-rustls-native-certs might be a source of confusion as a certificate can be trusted by all the thin-edge component expect the remote access plugin (which depends on async-tungstenite for web-sockets). See #1678

Just confirmed that with this change, c8y-remote-access-plugin fails when accepting a connection from Cumulocity, with the following message in MQTT:

$ tedge mqtt sub "#" ... c8y/s/us] 502,c8y_RemoteAccessConnect," Error: × Connecting to Websocket ├─▶ URL error: TLS support not compiled in ╰─▶ TLS support not compiled in "

A ticket was created to create an integration test to catch similar problems in the future: #2201

A fix was added in 0c356e3, and an issue to revert back into a regular dependency once a patched version is released was created #2202.

Just confirmed that with this change, c8y-remote-access-plugin fails when accepting a connection from Cumulocity, with the following message in MQTT

Thank you for going deeper. So even worse than we feared ;-)

didier-wenzek

Approved

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>

rumqttd, the rust MQTT broker that was used for the tests of the mqtt_channel crate, was updated to the latest version, 0.17. rumqttd requires rust version 1.70, so MSRV was bumped. Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>

Bravo555 · 2023-08-30T16:54:53Z

There are 2 problems leading to the issue with CI test check:

we're hitting github CI runner free space limit of 14GB
some tests of download crate do not terminate if there is no sufficient disk space

didier-wenzek · 2023-08-30T17:13:53Z

There are 2 problems leading to the issue with CI test check:

* we're hitting [github CI runner free space limit of 14GB](https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners#supported-runners-and-hardware-resources)

* some tests of `download` crate do not terminate if there is no sufficient disk space

What I propose to move forward is simply to mark as ignored the tests that block when there is no sufficient disk space.

Also, create a ticket to fix the issue related to these blocking downloads. If the disk is full a download should not block and should even remove all half-downloaded stuff on such errors.

Bravo555 · 2023-08-31T08:34:09Z

What I propose to move forward is simply to mark as ignored the tests that block when there is no sufficient disk space.

There's also the problem that some tests, as expected, fail if there is no disk space available.

didier-wenzek

Approving to #[ignore = "fails CI because of lack of disk space"]

Bravo555 temporarily deployed to Test Pull Request August 24, 2023 08:21 — with GitHub Actions Inactive

Bravo555 force-pushed the remove-webpki-crate branch from 24af9f1 to 7cdd2b9 Compare August 27, 2023 21:34

Bravo555 temporarily deployed to Test Pull Request August 27, 2023 21:48 — with GitHub Actions Inactive

Bravo555 temporarily deployed to Test Pull Request August 28, 2023 13:14 — with GitHub Actions Inactive

Bravo555 force-pushed the remove-webpki-crate branch from ef1f3c2 to ce18b99 Compare August 28, 2023 13:30

Bravo555 changed the title ~~Disable tungstenite features that depend on webpki~~ Remove webpki crate - resolves #2183 Aug 28, 2023

Bravo555 linked an issue Aug 28, 2023 that may be closed by this pull request

Resolving webpki security advisory #2183

Closed

4 tasks

Bravo555 force-pushed the remove-webpki-crate branch from ce18b99 to 13886bd Compare August 28, 2023 13:37

Bravo555 temporarily deployed to Test Pull Request August 28, 2023 15:40 — with GitHub Actions Inactive

Bravo555 temporarily deployed to Test Pull Request August 28, 2023 16:02 — with GitHub Actions Inactive

Bravo555 marked this pull request as ready for review August 28, 2023 16:10

Bravo555 requested a review from didier-wenzek August 28, 2023 16:10

didier-wenzek reviewed Aug 28, 2023

View reviewed changes

Cargo.toml Show resolved Hide resolved

crates/common/certificate/src/lib.rs Outdated Show resolved Hide resolved

Bravo555 temporarily deployed to Test Pull Request August 29, 2023 07:58 — with GitHub Actions Inactive

didier-wenzek reviewed Aug 29, 2023

View reviewed changes

This was referenced Aug 29, 2023

Create an integration test for c8y-remote-access-plugin #2201

Open

Replace ws_stream_tungstenite git dependency once patched version is published #2202

Closed

Bravo555 had a problem deploying to Test Pull Request August 29, 2023 14:24 — with GitHub Actions Failure

Bravo555 temporarily deployed to Test Pull Request August 29, 2023 14:53 — with GitHub Actions Inactive

didier-wenzek approved these changes Aug 29, 2023

View reviewed changes

Bravo555 force-pushed the remove-webpki-crate branch 2 times, most recently from f88b6f2 to a9c9784 Compare August 29, 2023 15:26

Bravo555 temporarily deployed to Test Pull Request August 29, 2023 15:39 — with GitHub Actions Inactive

Bravo555 added 5 commits August 30, 2023 13:29

Upgrade async-tungstenite and ws_stream_tungstenite

9afc398

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>

Update rumqttc from 0.18 to 0.22

937e781

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>

Update hyper-rustls from 0.23 to 0.24

50bbf08

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>

Upgrade rumqttd to 0.17 and bump MSRV to 1.70

a10bfce

rumqttd, the rust MQTT broker that was used for the tests of the mqtt_channel crate, was updated to the latest version, 0.17. rumqttd requires rust version 1.70, so MSRV was bumped. Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>

Fix integer underflow in download crate

6141405

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>

Bravo555 force-pushed the remove-webpki-crate branch from a9c9784 to 6141405 Compare August 30, 2023 13:30

Bravo555 temporarily deployed to Test Pull Request August 30, 2023 13:40 — with GitHub Actions Inactive

Bravo555 temporarily deployed to Test Pull Request August 30, 2023 13:57 — with GitHub Actions Inactive

Bravo555 force-pushed the remove-webpki-crate branch from 7d86d37 to 6141405 Compare August 30, 2023 14:05

Bravo555 had a problem deploying to Test Pull Request August 30, 2023 14:14 — with GitHub Actions Failure

Bravo555 temporarily deployed to Test Pull Request August 30, 2023 14:31 — with GitHub Actions Inactive

Bravo555 temporarily deployed to Test Pull Request August 30, 2023 14:43 — with GitHub Actions Inactive

Bravo555 force-pushed the remove-webpki-crate branch from d8ed0aa to 30a3c7b Compare August 31, 2023 10:45

Bravo555 had a problem deploying to Test Pull Request August 31, 2023 10:55 — with GitHub Actions Failure

Bravo555 force-pushed the remove-webpki-crate branch from 30a3c7b to 896e441 Compare August 31, 2023 11:06

Bravo555 had a problem deploying to Test Pull Request August 31, 2023 11:15 — with GitHub Actions Failure

Bravo555 force-pushed the remove-webpki-crate branch from 896e441 to 4b149d9 Compare August 31, 2023 11:18

Bravo555 had a problem deploying to Test Pull Request August 31, 2023 11:27 — with GitHub Actions Failure

Bravo555 force-pushed the remove-webpki-crate branch from 4b149d9 to a435068 Compare August 31, 2023 11:35

Bravo555 had a problem deploying to Test Pull Request August 31, 2023 11:45 — with GitHub Actions Failure

Ignore tests that fail due to lack of disk space

3cf6498

Bravo555 force-pushed the remove-webpki-crate branch from a435068 to 3cf6498 Compare August 31, 2023 12:06

Bravo555 temporarily deployed to Test Pull Request August 31, 2023 12:21 — with GitHub Actions Inactive

didier-wenzek approved these changes Aug 31, 2023

View reviewed changes

Bravo555 merged commit c07cf47 into thin-edge:main Aug 31, 2023
18 checks passed

Bravo555 deleted the remove-webpki-crate branch October 19, 2023 10:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove webpki crate - resolves #2183 #2185

Remove webpki crate - resolves #2183 #2185

Bravo555 commented Aug 24, 2023 •

edited

github-actions bot commented Aug 24, 2023 •

edited

codecov bot commented Aug 27, 2023 •

edited

didier-wenzek left a comment

didier-wenzek Aug 29, 2023

Bravo555 Aug 29, 2023

Bravo555 Aug 29, 2023

didier-wenzek Aug 29, 2023

didier-wenzek left a comment

Bravo555 commented Aug 30, 2023

didier-wenzek commented Aug 30, 2023

Bravo555 commented Aug 31, 2023

didier-wenzek left a comment

Remove webpki crate - resolves #2183 #2185

Remove webpki crate - resolves #2183 #2185

Conversation

Bravo555 commented Aug 24, 2023 • edited

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

github-actions bot commented Aug 24, 2023 • edited

Robot Results

codecov bot commented Aug 27, 2023 • edited

Codecov Report

didier-wenzek left a comment

Choose a reason for hiding this comment

didier-wenzek Aug 29, 2023

Choose a reason for hiding this comment

Bravo555 Aug 29, 2023

Choose a reason for hiding this comment

Bravo555 Aug 29, 2023

Choose a reason for hiding this comment

didier-wenzek Aug 29, 2023

Choose a reason for hiding this comment

didier-wenzek left a comment

Choose a reason for hiding this comment

Bravo555 commented Aug 30, 2023

didier-wenzek commented Aug 30, 2023

Bravo555 commented Aug 31, 2023

didier-wenzek left a comment

Choose a reason for hiding this comment

Bravo555 commented Aug 24, 2023 •

edited

github-actions bot commented Aug 24, 2023 •

edited

codecov bot commented Aug 27, 2023 •

edited