Skip to content

remove openssl dependency #2325

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

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

remove openssl dependency #2325

wants to merge 22 commits into from

Conversation

drahnr
Copy link
Collaborator

@drahnr drahnr commented Feb 4, 2025

Continuation of #1742

OpenSSL is a hinderance of both building and distribution, and has been superseeded by rustls and boringssl as safer and alternatives with sufficient subsets for sccache. We hance should not rely on a dependency when there are easier in alternatives.

Particularly difficult use cases when linking against OpenSSL:

  • version mismatches
  • static linkage
  • cross compilation
  • certificate store configuration

@drahnr drahnr changed the title remove openssl remove openssl dependency Feb 4, 2025
@sylvestre
Copy link
Collaborator

In the commit, it would be nice to explicit why we want to remove it :)
Thanks for restarting this

@drahnr
Copy link
Collaborator Author

drahnr commented Feb 5, 2025

Done

@drahnr drahnr force-pushed the bernhard-remove-openssl branch from 0f3f94f to bca3500 Compare February 5, 2025 12:46
@drahnr
Copy link
Collaborator Author

drahnr commented Feb 5, 2025

The issue is somewhere in the delta:

# openssl
[
Extension { extn_id: ObjectIdentifierAsn1(ObjectIdentifier { root: JointIsoItuT, first_node: 5, child_nodes: [29, 17] }), critical: Implicit(false), extn_value: SubjectAltName(OctetStringAsn1Container(Asn1SequenceOf([IpAddress(OctetStringAsn1([127, 0, 0, 1]))]))) },
Extension { extn_id: ObjectIdentifierAsn1(ObjectIdentifier { root: JointIsoItuT, first_node: 5, child_nodes: [29, 37] }), critical: Implicit(false), extn_value: ExtendedKeyUsage(OctetStringAsn1Container(ExtendedKeyUsage(Asn1SequenceOf([ObjectIdentifierAsn1(ObjectIdentifier { root: Iso, first_node: 3, child_nodes: [6, 1, 5, 5, 7, 3, 1] })])))) }
]
 
# picky
[
Extension { extn_id: ObjectIdentifierAsn1(ObjectIdentifier { root: JointIsoItuT, first_node: 5, child_nodes: [29, 17] }), critical: Implicit(true), extn_value: SubjectAltName(OctetStringAsn1Container(Asn1SequenceOf([IpAddress(OctetStringAsn1([127, 0, 0, 1]))]))) }
Extension { extn_id: ObjectIdentifierAsn1(ObjectIdentifier { root: JointIsoItuT, first_node: 5, child_nodes: [29, 37] }), critical: Implicit(true), extn_value: ExtendedKeyUsage(OctetStringAsn1Container(ExtendedKeyUsage(Asn1SequenceOf([ObjectIdentifierAsn1(ObjectIdentifier { root: Iso, first_node: 3, child_nodes: [6, 1, 5, 5, 7, 3, 1] })])))) }
Extension { extn_id: ObjectIdentifierAsn1(ObjectIdentifier { root: JointIsoItuT, first_node: 5, child_nodes: [29, 19] }), critical: Implicit(false), extn_value: BasicConstraints(OctetStringAsn1Container(BasicConstraints { ca: Implicit(Some(false)), path_len_constraint: Implicit(None) })) }
Extension { extn_id: ObjectIdentifierAsn1(ObjectIdentifier { root: JointIsoItuT, first_node: 5, child_nodes: [29, 35] }), critical: Implicit(false), extn_value: AuthorityKeyIdentifier(OctetStringAsn1Container(AuthorityKeyIdentifier { key_identifier: Some(ContextTag0(OctetStringAsn1([53, 4, 192, 142, 244, 179, 95, 190, 50, 190, 70, 215, 188, 117, 9, 37, 50, 147, 131, 167, 10, 120, 7, 227, 65, 73, 228, 209, 66, 145, 160, 107]))), authority_cert_issuer: None, authority_cert_serial_number: None })) }
Extension { extn_id: ObjectIdentifierAsn1(ObjectIdentifier { root: JointIsoItuT, first_node: 5, child_nodes: [29, 14] }), critical: Implicit(false), extn_value: SubjectKeyIdentifier(OctetStringAsn1Container(OctetStringAsn1([53, 4, 192, 142, 244, 179, 95, 190, 50, 190, 70, 215, 188, 117, 9, 37, 50, 147, 131, 167, 10, 120, 7, 227, 65, 73, 228, 209, 66, 145, 160, 107]))) }
]

The basic fields differ in Implicit(true) and Implicit(false), but picky adds 3 extra attributes in CertificateBuilder::build extra extended fields and I have yet to read the spec on it

@drahnr drahnr force-pushed the bernhard-remove-openssl branch from c39828a to bee63dc Compare February 9, 2025 18:40
@drahnr
Copy link
Collaborator Author

drahnr commented Feb 10, 2025

The remaining issues originate from briansmith/ring#1167 (comment) and will be mitigated once hickory-dns gets a 0.25.0 release hickory-dns/hickory-dns#2206

@drahnr
Copy link
Collaborator Author

drahnr commented Feb 19, 2025

hickory-dns 0.24.4 got released

@drahnr drahnr force-pushed the bernhard-remove-openssl branch from 2d6e57e to 6ed6bee Compare February 25, 2025 08:12
@@ -286,7 +286,7 @@ where
.ok()
.map(|f| {
f.flatten()
.filter(|f| f.path().extension().map_or(false, |ext| ext == "bc"))
.filter(|f| f.path().extension().is_some_and(|ext| ext == "bc"))
Copy link
Contributor

Choose a reason for hiding this comment

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

What about migrating OpenSSL-unrelated changes to the dedicated PR?
I don't think that changes breaks CI, but this split will minify change set, that is related to failure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure

@drahnr
Copy link
Collaborator Author

drahnr commented Feb 25, 2025

I cannot reproduce test_dist_restartedserver issues CI is encountering. I've seen over PRs where re-trying the CI ultimately succeeded.

@drahnr
Copy link
Collaborator Author

drahnr commented Mar 5, 2025

I can occassionally repro the issue locally

@drahnr drahnr force-pushed the bernhard-remove-openssl branch 2 times, most recently from 91efbbf to c8811ad Compare May 9, 2025 12:10
@codecov-commenter
Copy link

codecov-commenter commented May 9, 2025

Codecov Report

Attention: Patch coverage is 0% with 107 lines in your changes missing coverage. Please review.

Project coverage is 67.07%. Comparing base (f162d6e) to head (67b074d).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/dist/http.rs 0.00% 89 Missing ⚠️
src/util.rs 0.00% 11 Missing ⚠️
src/bin/sccache-dist/token_check.rs 0.00% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2325      +/-   ##
==========================================
- Coverage   71.42%   67.07%   -4.35%     
==========================================
  Files          65       64       -1     
  Lines       36360    35230    -1130     
==========================================
- Hits        25970    23632    -2338     
- Misses      10390    11598    +1208     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

drahnr and others added 10 commits May 12, 2025 13:38
 - 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
 - Remove `openssl` pulled in `[dev-dependencies]`
 - Bump reqwest from 0.11.17 => 0.11.18

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]>
@drahnr drahnr force-pushed the bernhard-remove-openssl branch from c8811ad to 70fac0a Compare May 12, 2025 11:40
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.

5 participants