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.

Sign up for GitHub

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

Jump to bottom

remove openssl dependency #2325

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

remove openssl dependency #2325

wants to merge 19 commits into from
+1,029 −351

Conversation

drahnr
Copy link
Collaborator

@drahnr drahnr commented Feb 4, 2025 •
edited
Loading

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

AJIOB
AJIOB reviewed Feb 6, 2025
View reviewed changes
@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 drahnr mentioned this pull request Feb 10, 2025
Merged
@drahnr drahnr force-pushed the bernhard-remove-openssl branch from 9518a7e to 6777c63 Compare February 19, 2025 10:47
@drahnr
Copy link
Collaborator Author

drahnr commented Feb 19, 2025

hickory-dns 0.24.4 got released

drahnr and others added 19 commits February 25, 2025 09:12
@drahnr
feat: remove openssl dependency
2c58fa9 
 - 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]>
@NobodyXu @drahnr
dep: Upgrade trust-dns-resolver v0.22 to hickory-resolver v0.24
20ad1b1 
Signed-off-by: Jiahao XU <[email protected]>
@NobodyXu @drahnr
Fix line ending of dynamic key generated
68ce924 
Use CRLF on windows and `\n` on Linux.

Also fix formatting of `Cargo.toml`

Signed-off-by: Jiahao XU <[email protected]>
@NobodyXu @drahnr
Fix typo: Default() => default()
9d79401 
Signed-off-by: Jiahao XU <[email protected]>
@drahnr
fix: toml formating
69ee0c3
@drahnr
use artifacts v4
ee723f5
@drahnr
add certificate equiv test, move to sha256, newer openssl distributio…
8092460 
…ns do not contain sha1 support by default
@drahnr
clippy
ea73c9a
@drahnr
fmt again
a1b5b68
@drahnr
remvoe flipper RNG + guard test module to dist-{client,server}
cf92d3e
@drahnr
don't add mask to ip address authority
b1ba2fa
@drahnr
use vendored openssl for tests
9fdc365
@drahnr
toml fmt
bde1b79
@drahnr
clippy was wrong
e24929d
@drahnr
dep: force hickory-dns >= 0.24.4 or higher
0a1bfb0 
Uses `ring >= 0.7.0` which contains a fix for win on aarch64
@drahnr
ignore openssl cert congruence test
8e920d7 
It will never work with `picky`, the `openssl` generated certs were invalid
in the first place.
@drahnr
fmt
bb93592
@drahnr
lock
408b87c
@drahnr
remove openssl integrity test, we can't ensure it
6ed6bee 
Simplifies CI
@drahnr drahnr force-pushed the bernhard-remove-openssl branch from 2d6e57e to 6ed6bee Compare February 25, 2025 08:12
AJIOB
AJIOB reviewed Feb 25, 2025
View reviewed changes
src/compiler/c.rs
@@ -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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AJIOB AJIOB AJIOB left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@drahnr @sylvestre @AJIOB @NobodyXu