[services] Tunnelbroker - Fix an OpenSSL crate collision
ClosedPublic
Actions

Authored by • max on Sep 6 2022, 4:54 AM.

Details

Reviewers

• jon
• karol
tomek
varun
ashoat

Commits

rCOMM9ec57df50f22: [services] Tunnelbroker - Fix an OpenSSL crate collision

Summary

This diff introduces a fix to resolve conflict in Rust crates which uses the OpenSSL library.
The fix is to use OpenSSL as **vendored** according to the OpenSSL crate documentation.

The full context is available in the Linear task.

Related Linear task: ENG-1738

Test Plan

Call Rust's FCM, A2, Reqwest functions which use OpenSSL from the Tunnelbroker C++ codebase without OpenSSL-related errors.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• max created this revision.Sep 6 2022, 4:54 AM

• max held this revision as a draft.

Herald added a reviewer: • jon. · View Herald TranscriptSep 6 2022, 4:54 AM

Herald added subscribers: • abosh, • karol, atul and 2 others. · View Herald Transcript

Harbormaster completed remote builds in B11915: Diff 16358.Sep 6 2022, 5:02 AM

• max published this revision for review.Sep 6 2022, 5:07 AM

• max edited the summary of this revision. (Show Details)

• max edited the test plan for this revision. (Show Details)

• max added reviewers: • karol, tomek, varun.

tomek accepted this revision.Sep 8 2022, 8:38 AM

tomek added a reviewer: ashoat.

tomek added inline comments.

services/tunnelbroker/rust-notifications/Cargo.toml
18 ↗	(On Diff #16358)	What happens if a machine doesn't have OpenSSL installed?

• karol accepted this revision.Sep 9 2022, 7:50 AM

• jon added inline comments.Sep 9 2022, 11:25 AM

services/tunnelbroker/rust-notifications/Cargo.toml
18 ↗	(On Diff #16358)	vendoring a dependency usually means that a package will use it's own packaging to indirectly expose it. Essentially, it shouldn't matter from a "does someone need openssl" standpoint. The bigger concern is that if there is a big openssl vulnerability, then vendored dependencies have to be updated through the dependency which vendored it. Making a wider window in which we have potential vulnerabilities in production.

This isn't necessary on the nix develop side, but this probably fixes the docker build as it has an older version of openssl available.

• max marked 2 inline comments as done.Sep 12 2022, 11:55 AM

• max added inline comments.

services/tunnelbroker/rust-notifications/Cargo.toml
18 ↗	(On Diff #16358)	What happens if a machine doesn't have OpenSSL installed? We have OpenSSL installed in a Docker dev environment, and macOS have it too. Also, when installing dependencies for our dev environment OpenSSL will be installed anyway. So it looks very unlikely.
18 ↗	(On Diff #16358)	vendoring a dependency usually means that a package will use it's own packaging to indirectly expose it. Essentially, it shouldn't matter from a "does someone need openssl" standpoint. The bigger concern is that if there is a big openssl vulnerability, then vendored dependencies have to be updated through the dependency which vendored it. Making a wider window in which we have potential vulnerabilities in production. In case of a big openssl vulnerability (like openssl had in the past), we should check all the dependencies for an `openssl` anyway and update `version` in `vendored` as well.

In D5064#149334, @jon wrote:

This isn't necessary on the nix develop side, but this probably fixes the docker build as it has an older version of openssl available.

Correct! More context in the task comments.

Pinging @ashoat for a final blocking review because this is a dependency change.

ashoat accepted this revision.Sep 15 2022, 8:33 AM

This revision is now accepted and ready to land.Sep 15 2022, 8:33 AM

Closed by commit rCOMM9ec57df50f22: [services] Tunnelbroker - Fix an OpenSSL crate collision (authored by • max). · Explain WhySep 15 2022, 4:06 PM

This revision was automatically updated to reflect the committed changes.

• max added a commit: rCOMM9ec57df50f22: [services] Tunnelbroker - Fix an OpenSSL crate collision.

Sorry I took so long to review here!!