don't sign X.509 certs #34202

ripatel-fd · 2023-11-22T00:04:52Z

Problem

Addresses a TODO comment regarding a vulnerability
Removes the pkcs8 dependency
Removes logic to create X.509 signatures (X.509 signatures are ignored)
Hardcodes the SAN (SAN is ignored)
Removes the rcgen dependency

Summary of Changes

Fixes #

ripatel-fd · 2023-11-22T01:00:24Z

Related Firedancer patch: firedancer-io/firedancer@851e73f

lijunwangs · 2023-11-22T17:07:42Z

Can you elaborate on what problems this PR solves? Why do we need this?

ripatel-fd · 2023-11-22T17:53:15Z

@lijunwangs There is lots of background here. It starts with the messy integration of QUIC into Solana's peer-to-peer protocols. The Rust ecosystem still lacks supports for some modern TLS standards, like RFC 7250 RawPublicKeys. So, in order to establish a TLS connection, the validator needs to work with X.509 certificates. Currently, it uses self-signed certificates. These certs are almost entirely redundant, except for the fact that they carry the peer's Ed25519 public key.

Ever since QUIC was first added to the validator, there has been a historical security concern with how these certificates were created. Currently, the Labs validator uses the node identity key to sign X.509 certificates. The signature is useless however. Validators don't verify it. This form of signing introduces risk though: It exposes the node's private key to a handful of unaudited third-party dependencies. The signing payload could also be ambiguous with other messages signed by the identity key, such as Solana gossip packets or vote transactions.

So, this PR removes a lot of complexity. It removes several third-party crates used to construct the X.509 certificate with a single hardcoded template. (As mentioned above, the content of the cert doesn't matter, so it is fine to hardcode). The signature contained in the certificate is now deliberately invalid, as nodes don't verify it anyways. For similar reasons, the SAN and serial number are removed too.

What remains is a template holding a byte array with a DER serialization of an X.509 certificate. The pubkey field is then patched with the node's identity key as needed.

t-nelson · 2023-11-22T16:13:04Z

quic-client/src/lib.rs

@@ -97,8 +93,7 @@ pub struct QuicConfig {

 impl NewConnectionConfig for QuicConfig {
    fn new() -> Result<Self, ClientError> {


looks like this becomes infallible?

@t-nelson No, unfortunately not. There are some trait impls of NewConnectionConfig for the legacy UDP code path that can still return an IOError.

quic-client/src/lib.rs

streamer/src/tls_certificates.rs

t-nelson

changes lgtm now. gonna tap in @lijunwangs and @behzadnouri for rationale

behzadnouri · 2023-11-29T01:31:20Z

Can you please confirm that this change is backward compatible with current mainnet?
i.e. if I spin up a staked node with this code on mainnet, other nodes still accept connections from my node and correctly recognize its pubkey?
This is probably a dumb question, but regarding

The signature contained in the certificate is now deliberately invalid, as nodes don't verify it anyways.

what exactly prevents pubkey spoofing with this code? i.e. a node claiming a pubkey which they don't possess the private key of.

ripatel-fd · 2023-11-29T10:01:13Z

Can you please confirm that this change is backward compatible with current mainnet?
i.e. if I spin up a staked node with this code on mainnet, other nodes still accept connections from my node and correctly recognize its pubkey?

I'm pretty sure it's safe, but I cannot 100% confirm this, as I haven't tested a build from this PR against an old Solana build.

Is there a simple CLI tool I can run that spins up a solana-streamer client or server? That would help with testing.
Ideally the test setup is reproducible so you can confirm yourself as well.

This is probably a dumb question, but regarding

The signature contained in the certificate is now deliberately invalid, as nodes don't verify it anyways.

what exactly prevents pubkey spoofing with this code? i.e. a node claiming a pubkey which they don't possess the private key of.

That's a great question, and I should have clarified! The TLS 1.3 CertificateVerify handshake message protects the connection. It is created via an overcomplicated hash tree that includes the ECDH key exchange secret and a hash of all of the incoming and outgoing TLS messages, called the "transcript hash". The transcript hash is then signed with Ed25519. Both parties verify that the signature matches the public key in the peer's X.509 certificate (in the "SubjectPublicKeyInfo" field of the "TBSCertificate" structure)

Both client and server deterministically compute the transcript hash and then do the verification.
So, in the case of a MitM attack where some handshake messages are modified or the pubkeys are swapped, the signature verification fails, and the QUIC connection terminates.

The client and server also put in a 32 byte random value in their ClientHello and ServerHello messages to prevent replay attacks. (So it has some properties of a challenge-response mechanism)

All TLS connections do this procedure described above. X.509 signatures are only used for the certificate chain, but they do not secure the connection itself. (e.g. I could easily take the github.com certificate and signatures and put it in my own web server. The peer will think the certificate is valid. But since I don't have the private key for it, I can't sign a valid TLS 1.3 CertificateVerify message)

In the web, we need both TLS CertificateVerifys (to prove cert ownership) and X.509 certs (to prove cert authenticity against the root CAs via cert chains). In P2P, we only need TLS CertificateVerify, as the blockchain state does the authentication part.

See RFC 8446:

4.4.3. Certificate Verify

This message is used to provide explicit proof that an endpoint
possesses the private key corresponding to its certificate. The
CertificateVerify message also provides integrity for the handshake
up to this point. Servers MUST send this message when authenticating
via a certificate. Clients MUST send this message whenever
authenticating via a certificate (i.e., when the Certificate message
is non-empty). When sent, this message MUST appear immediately after
the Certificate message and immediately prior to the Finished
message.

For completeness, here is a pretty picture describing exactly how the TLS CertificateVerify is created:

behzadnouri · 2023-11-29T13:38:48Z

Is there a simple CLI tool I can run that spins up a solana-streamer client or server? That would help with testing.
Ideally the test setup is reproducible so you can confirm yourself as well.

I think the easiest would be to run a staked node on testnet and verify quic connections are ok, and other nodes extract the correct pubkey from the connection.

core/src/repair/quic_endpoint.rs

turbine/src/quic_endpoint.rs

codecov · 2023-11-29T20:48:11Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (935e06f) 81.9% compared to head (57a35df) 81.9%.
Report is 426 commits behind head on master.

❗ Current head 57a35df differs from pull request most recent head 2b1c177. Consider uploading reports for the commit 2b1c177 to get more accurate results

Additional details and impacted files

@@            Coverage Diff            @@
##           master   #34202     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         819      819             
  Lines      219765   219689     -76     
=========================================
- Hits       180094   179987    -107     
- Misses      39671    39702     +31

lijunwangs · 2023-11-29T22:00:59Z

streamer/src/tls_certificates.rs

-        .to_der()
-        .expect("Failed to convert keypair to DER")
-        .to_der();
+    //


Can you add a link to where the prefix is documented?

I've added a link to https://www.rfc-editor.org/rfc/rfc8410#section-7.

https://www.rfc-editor.org/rfc/rfc8410#section-10.3 is probably a better example reference.

Nodes currently don't verify X.509 self-signed certificates because peer authentication is done via TLS 1.3 CertificateVerify. Thus, encodes an invalid signature in the X.509 certificate instead.

lijunwangs · 2023-11-30T00:55:17Z

streamer/src/tls_certificates.rs

-        .to_der()
-        .expect("Failed to convert keypair to DER")
-        .to_der();
+    //


https://www.rfc-editor.org/rfc/rfc8410#section-10.3 is probably a better example reference.

lijunwangs · 2023-11-30T01:40:25Z

streamer/src/tls_certificates.rs

-    cert_params
-        .distinguished_name
-        .push(DnType::CommonName, "Solana node");
+    let mut cert_der = Vec::<u8>::with_capacity(0xf4);


Reference link please for the following sections. And instead of hard coding a certificate, can we not generate it without giving the key_pair?

What do you mean with reference link? The X.509 spec? This code is functionally no different from what was there before. The previous version just used a crate to produce the same cert on all nodes, with just the serial number, SAN, and public key different. Here, we do the same, but precompute the ASN.1 serialization. I honestly thought this was worth getting rid of thousands of lines of dependency code, considering using X.509 is a mistake in the first place.

can we not generate it without giving the key_pair

Holding the public key is the only purpose of the cert, everything else is arbitrary. Without it, authentication is not possible. So we cannot remove the public key.

Yes the reference doc's link, like ITU-T X.690. This hard code is just very hard to review. I really need to understand each code to approve this change.

@lijunwangs Maybe it would help to have a test case that asserts the hardcoded serialization decodes to the structure mentions in the comment. We can use a dev-dependency for this. What do you think?

ripatel-fd · 2023-12-01T15:12:03Z

@lijunwangs I wrote up an explanation of this change: https://forum.solana.com/t/deprecate-x-509-certs-for-p2p-connections/762. We could link it in the code if you think it helps.

lijunwangs

It took me a while to fully review the changes in this PR and test the changes to verify they are safe and working:

I tested against the mainnet -- connections to the existing nodes are okay
Tested pubkey spoofing in the certificate (impersonating someone else's pubkey in the X.509 cert) -- the validator is correctly rejecting with BadSignature during TLS1.3 handshaking.
I created a manual test tool to compare the cert and key generated using this code and without the code and use openssl to compare the generated DER file. Do not see worthy aberrations other than the issuer name.
The PR while making it safer by eliminating a third party crate does make the code a lot harder to review and maintain -- so the referential links are important.
The code need to be rebased.

lijunwangs · 2023-12-13T22:45:28Z

streamer/src/tls_certificates.rs

+    //          RelativeDistinguishedName SET (1 elem)
+    //            AttributeTypeAndValue SEQUENCE (2 elem)
+    //              type AttributeType OBJECT IDENTIFIER 2.5.4.3 commonName (X.520 DN component)
+    //              value AttributeValue [?] UTF8String Solana


Can we keep using the old "Solana node" -- it is more appropriate. It is issued by this node -- not Solana networks.

lijunwangs · 2024-01-30T08:13:03Z

Superseded by #34896. Thanks for your efforts @ripatel-fd

mergify bot added community Community contribution need:merge-assist labels Nov 22, 2023

mergify bot requested a review from a team November 22, 2023 00:05

ripatel-fd force-pushed the ripatel/better-pkcs8 branch from 9829bfe to ef4dea3 Compare November 22, 2023 00:13

ripatel-fd changed the title ~~streamer: remove pkcs8 dep and clarify self-signing~~ don't sign X.509 certs Nov 22, 2023

t-nelson reviewed Nov 22, 2023

View reviewed changes

ripatel-fd force-pushed the ripatel/better-pkcs8 branch from 468aea6 to fe5c638 Compare November 22, 2023 20:53

ripatel-fd requested a review from t-nelson November 22, 2023 20:53

t-nelson reviewed Nov 28, 2023

View reviewed changes

t-nelson requested review from behzadnouri and lijunwangs November 28, 2023 01:21

behzadnouri previously approved these changes Nov 29, 2023

View reviewed changes

core/src/repair/quic_endpoint.rs Outdated Show resolved Hide resolved

turbine/src/quic_endpoint.rs Outdated Show resolved Hide resolved

ripatel-fd dismissed behzadnouri’s stale review via 57a35df November 29, 2023 18:22

ripatel-fd force-pushed the ripatel/better-pkcs8 branch from fe5c638 to 57a35df Compare November 29, 2023 18:22

ripatel-fd requested a review from behzadnouri November 29, 2023 18:22

behzadnouri previously approved these changes Nov 29, 2023

View reviewed changes

lijunwangs added the CI Pull Request is ready to enter CI label Nov 29, 2023

solana-grimes removed the CI Pull Request is ready to enter CI label Nov 29, 2023

lijunwangs reviewed Nov 29, 2023

View reviewed changes

riptl added 2 commits November 29, 2023 23:35

streamer: remove pkcs8 dep and clarify self-signing

ceb0103

don't sign X.509 certs

2b1c177

Nodes currently don't verify X.509 self-signed certificates because peer authentication is done via TLS 1.3 CertificateVerify. Thus, encodes an invalid signature in the X.509 certificate instead.

ripatel-fd dismissed behzadnouri’s stale review via 2b1c177 November 29, 2023 23:35

ripatel-fd force-pushed the ripatel/better-pkcs8 branch from 57a35df to 2b1c177 Compare November 29, 2023 23:35

ripatel-fd requested review from behzadnouri and lijunwangs November 29, 2023 23:53

lijunwangs reviewed Nov 30, 2023

View reviewed changes

lijunwangs reviewed Dec 13, 2023

View reviewed changes

github-actions bot added stale [bot only] Added to stale content; results in auto-close after a week. and removed stale [bot only] Added to stale content; results in auto-close after a week. labels Dec 28, 2023

github-actions bot added stale [bot only] Added to stale content; results in auto-close after a week. and removed stale [bot only] Added to stale content; results in auto-close after a week. labels Jan 12, 2024

lijunwangs mentioned this pull request Jan 23, 2024

don't sign X.509 certs #34896

Merged

github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Jan 30, 2024

lijunwangs closed this Jan 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

don't sign X.509 certs #34202

don't sign X.509 certs #34202

ripatel-fd commented Nov 22, 2023 •

edited

ripatel-fd commented Nov 22, 2023

lijunwangs commented Nov 22, 2023

ripatel-fd commented Nov 22, 2023

t-nelson Nov 22, 2023

ripatel-fd Nov 22, 2023

t-nelson left a comment

behzadnouri commented Nov 29, 2023

ripatel-fd commented Nov 29, 2023

behzadnouri commented Nov 29, 2023

codecov bot commented Nov 29, 2023 •

edited

lijunwangs Nov 29, 2023

ripatel-fd Nov 29, 2023

lijunwangs Nov 30, 2023

lijunwangs Nov 30, 2023

lijunwangs Nov 30, 2023

ripatel-fd Nov 30, 2023 •

edited

ripatel-fd Nov 30, 2023

lijunwangs Nov 30, 2023

ripatel-fd Dec 1, 2023

ripatel-fd commented Dec 1, 2023

lijunwangs left a comment

lijunwangs Dec 13, 2023

lijunwangs commented Jan 30, 2024

		@@ -97,8 +93,7 @@ pub struct QuicConfig {

		impl NewConnectionConfig for QuicConfig {
		fn new() -> Result<Self, ClientError> {

don't sign X.509 certs #34202

don't sign X.509 certs #34202

Conversation

ripatel-fd commented Nov 22, 2023 • edited

Problem

Summary of Changes

ripatel-fd commented Nov 22, 2023

lijunwangs commented Nov 22, 2023

ripatel-fd commented Nov 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-nelson left a comment

Choose a reason for hiding this comment

behzadnouri commented Nov 29, 2023

ripatel-fd commented Nov 29, 2023

behzadnouri commented Nov 29, 2023

codecov bot commented Nov 29, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ripatel-fd Nov 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ripatel-fd commented Dec 1, 2023

lijunwangs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lijunwangs commented Jan 30, 2024

ripatel-fd commented Nov 22, 2023 •

edited

codecov bot commented Nov 29, 2023 •

edited

ripatel-fd Nov 30, 2023 •

edited