Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

disable rustls tls12 feature #28956

Merged
merged 1 commit into from
Jan 12, 2023
Merged

disable rustls tls12 feature #28956

merged 1 commit into from
Jan 12, 2023

Conversation

ripatel-fd
Copy link
Contributor

@ripatel-fd ripatel-fd commented Nov 25, 2022 •
edited
Loading

Problem

Currently, Solana advertises support for legacy TLS 1.2 cipher suites (0xc0??).
TLS 1.3 is widely supported at this point -- for security and simplicity, support for TLS 1.2 should be disabled.

Cipher Suites (10 suites)
    Cipher Suite: TLS_AES_256_GCM_SHA384 (0x1302)
    Cipher Suite: TLS_AES_128_GCM_SHA256 (0x1301)
    Cipher Suite: TLS_CHACHA20_POLY1305_SHA256 (0x1303)
    Cipher Suite: TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384 (0xc02c)
    Cipher Suite: TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 (0xc02b)
    Cipher Suite: TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256 (0xcca9)
    Cipher Suite: TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (0xc030)
    Cipher Suite: TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (0xc02f)
    Cipher Suite: TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 (0xcca8)
    Cipher Suite: TLS_EMPTY_RENEGOTIATION_INFO_SCSV (0x00ff)

Summary of Changes

Disables the default tls12 feature on the rustls dependency.

See solana-foundation/specs#21

Fixes #

@mergify mergify bot added the community Community contribution label Nov 25, 2022
@mergify mergify bot requested a review from a team November 25, 2022 00:44
@github-actions github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Dec 29, 2022
@github-actions github-actions bot closed this Jan 6, 2023
@ripatel-fd
Copy link
Contributor Author

@github-actions, that's not very nice

@behzadnouri behzadnouri reopened this Jan 10, 2023
@behzadnouri behzadnouri added do-not-close Add this tag to exempt an issue/PR from being closed by the stalebot and removed stale [bot only] Added to stale content; results in auto-close after a week. labels Jan 10, 2023
@behzadnouri
Copy link
Contributor

@ripatel-jump Please tag appropriate folks to review the change.
Otherwise it might be missed.

@ripatel-fd
Copy link
Contributor Author

Not sure who to ping, but I've shared this change in the network-protocols channel ~3 weeks ago. It's not very urgent but would be good to keep open. I'll resolve the conflicts.

@behzadnouri
Copy link
Contributor

@t-nelson @pgarg66 @leoluk
can any of you please take a look and stamp or comment?

@pgarg66
Copy link
Contributor

pgarg66 commented Jan 10, 2023

@ripatel-jump, the PR needs a rebase. Could you rebase and push?

@mvines mvines removed the do-not-close Add this tag to exempt an issue/PR from being closed by the stalebot label Jan 11, 2023
@ripatel-fd
Copy link
Contributor Author

Thanks for taking a look @pgarg66, just rebased.

@pgarg66
Copy link
Contributor

pgarg66 commented Jan 11, 2023

Could this change cause a compatibility issue between client running on older version vs streamer running on the updated version (or vice versa), since we are disabling some features in rustls?

@ripatel-fd
Copy link
Contributor Author

ripatel-fd commented Jan 11, 2023 •
edited
Loading

Could this change cause a compatibility issue between client running on older version vs streamer running on the updated version (or vice versa), since we are disabling some features in rustls?

@pgarg66 This will not cause any compatibility issues between any existing Labs validators because it only restricts the available cipher suites and TLS versions, but not add any new ones. The TLS handshake protocol is designed to gracefully handle changes like this PR.

With or without this patch, Solana validators will negotiate TLS 1.3 with TLS_AES_256_GCM_SHA384 cipher suite and X25519 key exchange.

The only incompatibility issue that could be caused by this patch is breakage of clients that don't support TLS 1.3 yet. Sadly, some stacks can only speak TLS 1.2. That said, TLS 1.3 (first conceived 2014, finalized August 2018) predates the entire Solana protocol. So IMO, it is reasonable to enforce TLS 1.3 for additional security and protocol simplification. Further reasons are in the linked specs PR.

@ripatel-fd ripatel-fd mentioned this pull request Jan 11, 2023
Closed
@pgarg66 pgarg66 added the CI Pull Request is ready to enter CI label Jan 11, 2023
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Jan 11, 2023
t-nelson
t-nelson approved these changes Jan 12, 2023
View reviewed changes
Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

lgtm! r+ ci (pretty sure only blocked on flake)

@pgarg66 pgarg66 merged commit 1b6024a into solana-labs:master Jan 12, 2023
@ripatel-fd ripatel-fd deleted the ripatel/no-tls12 branch January 13, 2023 11:27
@aochagavia aochagavia mentioned this pull request Aug 29, 2024
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@t-nelson t-nelson t-nelson approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
community Community contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ripatel-fd @behzadnouri @pgarg66 @t-nelson @mvines @solana-grimes @riptl