Organize some of the duplicated TLS code into a separate crate #3835

alexpyattaev · 2024-11-28T15:48:33Z

Problem

A whole bunch of places in the code have been defining the same structure SkipServerVerification for use in quic related code. There were at least 3 independent definitions for this struct with zero functional differences in different project locations.

This created unnecessary duplication as well as strange code dependencies where turbine would depend on quic-client for this one struct, even though logically it makes no sense for turbine to depend on quic-client.

Summary of Changes

The following moved to tls-utils crate:

SkipServerVerification
SkipClientVerification
new_dummy_x509_certificate

All references to them redirected. Multiple redundant definitions removed.

KirillLykov · 2024-12-02T08:54:08Z

tls-utils/Cargo.toml

+
+[dependencies]
+solana-sdk = { workspace = true }
+


dependencies should be sorted

have you pushed your changes? I still see space and unsorted dependencies.

yes, just did

KirillLykov · 2024-12-02T08:54:20Z

tls-utils/Cargo.toml

+solana-signer = { workspace = true }
+solana-pubkey = { workspace = true }
+
+


extra space

KirillLykov · 2024-12-02T08:54:56Z

tls-utils/src/lib.rs

@@ -0,0 +1,14 @@
+//! Collection of TLS related code fragments that end up popping up everywhere where quic is used.
+//! Aggregated here to avoid bugs due to conflicting implementations of the same functionality


Suggested change

//! Aggregated here to avoid bugs due to conflicting implementations of the same functionality

//! Aggregated here to avoid bugs due to conflicting implementations of the same functionality.

tls-utils/README

KirillLykov · 2024-12-02T08:55:47Z

Looks like a good change to me

KirillLykov · 2024-12-02T13:58:32Z

I think you can press now "ready for review"

alexpyattaev · 2024-12-02T14:00:07Z

Thanks, will do.

…

I think you can press now "ready for review"

gregcusack

just a few initial thoughts on this! thank you for making these changes. making sure the crate isn't published will help get this PR past CI

gregcusack · 2024-12-02T17:33:40Z

quic-client/Cargo.toml

 solana-transaction-error = { workspace = true }
 thiserror = { workspace = true }
 tokio = { workspace = true, features = ["full"] }
-


should be a new line here

gregcusack · 2024-12-02T17:36:42Z

tls-utils/src/tls_certificates.rs

@@ -1,3 +1,4 @@
+#![allow(clippy::arithmetic_side_effects)]


is this addition needed? isn't the file just moved to a new location?

Yes, this is necessary. A similar "allow" was on the crate level in the original crate. Clippy gets confused with benign addition for a buffer size computation otherwise. If it is not benign then please let me know why. I've changed the code to appease clippy without the #allow, though I feel like in this case it was overly protective.

The bigger problem is the streamer crate from which the code was taken, where the same clippy warning is triggered many many times if the allow line is removed. I can look into that in a different PR.

looks like you removed this?

tls-utils/README

gregcusack · 2024-12-02T17:46:18Z

tls-utils/Cargo.toml

+description = "Solana TLS utilities"
+documentation = "https://docs.rs/solana-tls-utils"
+
+version.workspace = true


Suggested change

description = "Solana TLS utilities"

documentation = "https://docs.rs/solana-tls-utils"

version.workspace = true

description = "Solana TLS utilities"

documentation = "https://docs.rs/solana-tls-utils"

publish = false

version.workspace = true

set to publish=false as suggested

lijunwangs · 2024-12-02T19:19:02Z

quic-client/tests/quic_client.rs

        },
+        solana_tls_utils::{QuicClientCertificate,new_dummy_x509_certificate},


space after 'QuicClientCertificate', can you do a "cargo fmt"

thanks, cargo fmt done as suggested on all files.

KirillLykov · 2024-12-03T16:32:52Z

tls-utils/Cargo.toml

+description = "Solana TLS utilities"
+documentation = "https://docs.rs/solana-tls-utils"
+publish = false
+


extra line here

publish=false will have to go away after this is merged as far as I understand.

ya are we going to publish this on crates.io? or no?

I have no idea if anyone would ever use it there. Probably there is no benefit to that in this case.

alexpyattaev · 2024-12-03T18:09:02Z

Yes, I have changed the code to remove the #allow directive, even though it was present in the streamer crate, as it could be masking other possible bugs (and still probably does in streamer). 3 Dec 2024 19.19.36 Greg Cusack ***@***.***>:

…

***@***.**** commented on this pull request. ---------------------------------------- In tls-utils/src/tls_certificates.rs[#3835 (comment)]: > @@ -1,3 +1,4 @@ +#![allow(clippy::arithmetic_side_effects)] looks like you removed this? — Reply to this email directly, view it on GitHub[#3835 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/ABNIL3UROYIOELDJ33LDQBL2DXR2RAVCNFSM6AAAAABSVNUOZGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDINZWGMZTQMZQGI]. You are receiving this because you authored the thread. [Tracking image][https://github.com/notifications/beacon/ABNIL3WHRMVNVU7P6ZENOUD2DXR2RA5CNFSM6AAAAABSVNUOZGWGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTUTTHWH4.gif]

lijunwangs · 2024-12-03T21:44:08Z

tls-utils/src/tls_certificates.rs

+        if keypair_secret_len != 32 {
+            panic!("Unexpected secret key length!");
+        }
+        let need_buffer = PKCS8_PREFIX


Suggested change

let need_buffer = PKCS8_PREFIX

let buffer_size = PKCS8_PREFIX

corrected as suggested.

lijunwangs · 2024-12-03T21:44:39Z

tls-utils/src/tls_certificates.rs

+            .len()
+            .checked_add(keypair_secret_len) //clippy being overly guarded here but optimizer will elide checked_add
+            .expect("Unexpected secret key length!");
+        let mut key_pkcs8_der = Vec::<u8>::with_capacity(need_buffer);


Suggested change

let mut key_pkcs8_der = Vec::<u8>::with_capacity(need_buffer);

let mut key_pkcs8_der = Vec::<u8>::with_capacity(buffer_size);

corrected as suggested.

gregcusack

looks good. just need to remove the extra blank line in tls-utils/Cargo.toml

gregcusack · 2024-12-03T21:50:09Z

tls-utils/Cargo.toml

+edition = { workspace = true }
+
+[dependencies]
+


nit: shouldn't be a blank line at line 15

… to avoid duplication. Also move SkipClientVerification

gregcusack

lgtm!

joncinque · 2024-12-04T10:49:45Z

This crate needs to be published -- solana-quic-client depends on it, which is used by the CLI and many other clients. We can also remove the dependency on solana-sdk. I'll put in a PR for all that

KirillLykov reviewed Dec 2, 2024

View reviewed changes

tls-utils/README Outdated Show resolved Hide resolved

alexpyattaev force-pushed the organize_tls_utils branch from b2c6e92 to 56af8a2 Compare December 2, 2024 13:43

alexpyattaev marked this pull request as ready for review December 2, 2024 14:56

alexpyattaev requested review from gregcusack and lijunwangs December 2, 2024 14:57

alexpyattaev force-pushed the organize_tls_utils branch from 9e69118 to 9288522 Compare December 2, 2024 14:59

gregcusack reviewed Dec 2, 2024

View reviewed changes

lijunwangs reviewed Dec 2, 2024

View reviewed changes

alexpyattaev force-pushed the organize_tls_utils branch 4 times, most recently from 0474c3f to 87b4eae Compare December 3, 2024 16:27

KirillLykov reviewed Dec 3, 2024

View reviewed changes

Alex Pyattaev added 3 commits December 3, 2024 16:33

Add a place to store common TLS-related code that has no other home

109c178

remove SkipServerVerification from tpu-client-next

0737bd6

move TLS manipulation stuff from streamer to solana-tls-utils

f634e82

alexpyattaev force-pushed the organize_tls_utils branch from 87b4eae to 3becfc7 Compare December 3, 2024 16:33

KirillLykov previously approved these changes Dec 3, 2024

View reviewed changes

gregcusack self-requested a review December 3, 2024 21:25

lijunwangs reviewed Dec 3, 2024

View reviewed changes

gregcusack reviewed Dec 3, 2024

View reviewed changes

alexpyattaev dismissed KirillLykov’s stale review via 3f96c5c December 3, 2024 22:12

lijunwangs approved these changes Dec 3, 2024

View reviewed changes

Alex Pyattaev added 4 commits December 3, 2024 22:25

move SkipServerVerification and ClientCertificate to solana-tls-utils…

2aa7c35

… to avoid duplication. Also move SkipClientVerification

Formatting and CI prep

0764456

propagate changes to quic-client-certificate constructor from upstream

a6133f6

better naming for buffer size variable in tls_certificate.rs

f884350

alexpyattaev force-pushed the organize_tls_utils branch from 3f96c5c to f884350 Compare December 3, 2024 22:32

gregcusack self-requested a review December 4, 2024 00:17

gregcusack approved these changes Dec 4, 2024

View reviewed changes

alexpyattaev merged commit 1d3c961 into anza-xyz:master Dec 4, 2024
51 checks passed

		solana-signer = { workspace = true }
		solana-pubkey = { workspace = true }

		@@ -0,0 +1,14 @@
		//! Collection of TLS related code fragments that end up popping up everywhere where quic is used.
		//! Aggregated here to avoid bugs due to conflicting implementations of the same functionality

		},
		solana_tls_utils::{QuicClientCertificate,new_dummy_x509_certificate},

	let need_buffer = PKCS8_PREFIX
	let buffer_size = PKCS8_PREFIX

	let mut key_pkcs8_der = Vec::<u8>::with_capacity(need_buffer);
	let mut key_pkcs8_der = Vec::<u8>::with_capacity(buffer_size);


		[dependencies]
		solana-sdk = { workspace = true }

		edition = { workspace = true }

		[dependencies]

Organize some of the duplicated TLS code into a separate crate #3835

Organize some of the duplicated TLS code into a separate crate #3835

Conversation

alexpyattaev commented Nov 28, 2024

Problem

Summary of Changes

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KirillLykov commented Dec 2, 2024

KirillLykov commented Dec 2, 2024

alexpyattaev commented Dec 2, 2024 via email • edited by KirillLykov Loading

gregcusack left a 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

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

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

alexpyattaev commented Dec 3, 2024 via email

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

gregcusack left a comment • edited Loading

Choose a reason for hiding this comment

gregcusack Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregcusack left a comment

Choose a reason for hiding this comment

joncinque commented Dec 4, 2024

alexpyattaev commented Dec 2, 2024 via email •

edited by KirillLykov

Loading

gregcusack left a comment •

edited

Loading

gregcusack Dec 3, 2024 •

edited

Loading