Skip to content

Conversation

@macladson
Copy link
Member

@macladson macladson commented Feb 7, 2026

Issue Addressed

#8756

Proposed Changes

Only the Web3Signer actually needs OpenSSL in order to parse PKCS12 certificates. This updates the function to instead manually parse the cert (using the p12-keystore crate) and converts it to a PEM certificate (using the pem crate) which can be directly converted to a reqwest::tls::Identity as this can be done directly in rustls.

Additional Info

There may be some situations where a .p12 certificate could be strangely formatted (for example, if not generated by openssl) and break this function where it would succeed before. I'm not sure how to quantify the risks here but we could consider adding more test cases.

@macladson macladson added work-in-progress PR is a work-in-progress code-quality labels Feb 7, 2026
Comment on lines -3 to -8
# The lighthouse/key_legacy.p12 file is generated specifically for macOS because the default `openssl pkcs12` encoding
# algorithm in OpenSSL v3 is not compatible with the PKCS algorithm used by the Apple Security Framework. The client
# side (using the reqwest crate) relies on the Apple Security Framework to parse PKCS files.
# We don't need to generate web3signer/key_legacy.p12 because the compatibility issue doesn't occur on the web3signer
# side. It seems that web3signer (Java) uses its own implementation to parse PKCS files.
# See https://github.com/sigp/lighthouse/issues/6442#issuecomment-2469252651
Copy link
Member Author

Choose a reason for hiding this comment

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

I believe now that we are using pure Rust, we no longer rely on the Apple Security Framework and so can support OpenSSL v3 even on MacOS

@macladson macladson added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Feb 7, 2026
Copy link
Member

@ackintosh ackintosh left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@michaelsproul
Copy link
Member

Can we add openssl to the list of banned crates here?

lighthouse/deny.toml

Lines 7 to 19 in edba56b

deny = [
{ crate = "ethers", reason = "legacy Ethereum crate, use alloy instead" },
{ crate = "ethereum-types", reason = "legacy Ethereum crate, use alloy-primitives instead" },
{ crate = "protobuf", reason = "use quick-protobuf instead" },
{ crate = "derivative", reason = "use educe or derive_more instead" },
{ crate = "ark-ff", reason = "present in Cargo.lock but not needed by Lighthouse" },
{ crate = "strum", deny-multiple-versions = true, reason = "takes a long time to compile" },
{ crate = "reqwest", deny-multiple-versions = true, reason = "takes a long time to compile" },
{ crate = "aes", deny-multiple-versions = true, reason = "takes a long time to compile" },
{ crate = "sha2", deny-multiple-versions = true, reason = "takes a long time to compile" },
{ crate = "pbkdf2", deny-multiple-versions = true, reason = "takes a long time to compile" },
{ crate = "scrypt", deny-multiple-versions = true, reason = "takes a long time to compile" },
]

@jimmygchen jimmygchen added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Feb 9, 2026
@macladson macladson added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Feb 9, 2026
Copy link
Member

@pawanjay176 pawanjay176 left a comment

Choose a reason for hiding this comment

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

LGTM. The web3 signer tests pass so seem like this is good to go.

Copy link
Collaborator

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

LGTM

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Feb 10, 2026
@mergify mergify bot added the queued label Feb 10, 2026
@mergify
Copy link

mergify bot commented Feb 10, 2026

Merge Queue Status

Rule: default


This pull request spent 31 minutes 52 seconds in the queue, including 30 minutes 41 seconds running CI.

Required conditions to merge
  • check-success=local-testnet-success
  • check-success=test-suite-success

mergify bot added a commit that referenced this pull request Feb 10, 2026
@mergify mergify bot merged commit 286b67f into sigp:unstable Feb 10, 2026
36 checks passed
@mergify mergify bot removed the queued label Feb 10, 2026
@macladson macladson deleted the remove-openssl branch February 10, 2026 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code-quality ready-for-merge This PR is ready to merge.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants