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

WIP: quic integration #1466

Draft
wants to merge 23 commits into
base: v0.34.x-celestia
Choose a base branch
from
Draft

WIP: quic integration #1466

wants to merge 23 commits into from

Conversation

rach-id
Copy link
Member

@rach-id rach-id commented Aug 27, 2024

Description

This is just a prototype, doesn't need to be reviewed by automatic reviewers

@rach-id rach-id requested a review from Wondertan August 27, 2024 19:28
@rach-id rach-id self-assigned this Aug 27, 2024
@rach-id rach-id requested a review from a team as a code owner August 27, 2024 19:28
@rach-id rach-id requested review from rootulp and staheri14 and removed request for a team, rootulp and staheri14 August 27, 2024 19:28
@rach-id rach-id marked this pull request as draft August 27, 2024 19:28
github-advanced-security[bot]
github-advanced-security bot found potential problems Aug 27, 2024
View reviewed changes
p2p/certificate.go
}
return &tls.Config{
MinVersion: tls.VersionTLS13,
InsecureSkipVerify: true, // This is not insecure here. We will verify the cert chain ourselves.

Check failure

Code scanning / CodeQL

Disabled TLS certificate check High

InsecureSkipVerify should not be used in production code.

Copilot Autofix AI about 15 hours ago

To fix the problem, we need to ensure that the TLS certificate verification is properly handled. This involves:

  1. Removing the InsecureSkipVerify: true setting.
  2. Implementing the VerifyPeerCertificate function to perform the necessary certificate chain verification.

The changes will be made in the NewTLSConfig function in the p2p/certificate.go file. We will:

  • Remove the InsecureSkipVerify: true line.
  • Implement the VerifyPeerCertificate function to verify the certificate chain.
p2p/certificate.go

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/p2p/certificate.go b/p2p/certificate.go
--- a/p2p/certificate.go
+++ b/p2p/certificate.go
@@ -66,8 +66,23 @@
 	return &tls.Config{
-		MinVersion:         tls.VersionTLS13,
-		InsecureSkipVerify: true, // This is not insecure here. We will verify the cert chain ourselves.
-		ClientAuth:         tls.RequireAnyClientCert,
-		Certificates:       []tls.Certificate{*cert},
-		VerifyPeerCertificate: func(_ [][]byte, _ [][]*x509.Certificate) error {
-			panic("tls config not specialized for peer")
+		MinVersion:   tls.VersionTLS13,
+		ClientAuth:   tls.RequireAnyClientCert,
+		Certificates: []tls.Certificate{*cert},
+		VerifyPeerCertificate: func(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error {
+			// Implement certificate chain verification
+			roots := x509.NewCertPool()
+			// Add the root certificates to the pool (this should be configured appropriately)
+			// roots.AppendCertsFromPEM(rootPEM) // Example: Add root certificates from PEM
+			opts := x509.VerifyOptions{
+				Roots:         roots,
+				Intermediates: x509.NewCertPool(),
+			}
+			for _, rawCert := range rawCerts {
+				cert, err := x509.ParseCertificate(rawCert)
+				if err != nil {
+					return err
+				}
+				opts.Intermediates.AddCert(cert)
+			}
+			_, err := verifiedChains[0][0].Verify(opts)
+			return err
 		},
EOF
@@ -66,8 +66,23 @@
return &tls.Config{
MinVersion: tls.VersionTLS13,
InsecureSkipVerify: true, // This is not insecure here. We will verify the cert chain ourselves.
ClientAuth: tls.RequireAnyClientCert,
Certificates: []tls.Certificate{*cert},
VerifyPeerCertificate: func(_ [][]byte, _ [][]*x509.Certificate) error {
panic("tls config not specialized for peer")
MinVersion: tls.VersionTLS13,
ClientAuth: tls.RequireAnyClientCert,
Certificates: []tls.Certificate{*cert},
VerifyPeerCertificate: func(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error {
// Implement certificate chain verification
roots := x509.NewCertPool()
// Add the root certificates to the pool (this should be configured appropriately)
// roots.AppendCertsFromPEM(rootPEM) // Example: Add root certificates from PEM
opts := x509.VerifyOptions{
Roots: roots,
Intermediates: x509.NewCertPool(),
}
for _, rawCert := range rawCerts {
cert, err := x509.ParseCertificate(rawCert)
if err != nil {
return err
}
opts.Intermediates.AddCert(cert)
}
_, err := verifiedChains[0][0].Verify(opts)
return err
},
Copilot is powered by AI and may make mistakes. Always verify output.
p2p/netaddress.go Fixed Show fixed Hide fixed
Wondertan
Wondertan reviewed Aug 28, 2024
View reviewed changes
Copy link
Member

@Wondertan Wondertan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

My current goal is to grasp the approach here, think about it, compare it with alternatives, and contribute with pros and cons for us to compare. The selected approach looks promising and simple. However, a few things are missing for the final version, like stream management, that will add some more complexity.

The first superficial look includes comments on major DOS and synchronization issues that must be fixed.

p2p/peer.go Show resolved Hide resolved
p2p/peer.go
labels := []string{
"peer_id", string(p.ID()),
"chID", fmt.Sprintf("%#x", chID),
stream, has := p.streams[chID]
Copy link
Member

@Wondertan Wondertan Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Send is called concurrently, so access to streams must be synchronized.

Copy link
Member

Choose a reason for hiding this comment

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

Besides, dead and duplicate stream cases must be handled as well

p2p/peer.go
msg, err = w.Unwrap()
if err != nil {
panic(fmt.Errorf("unwrapping message: %s", err))
// start accepting data
Copy link
Member

Choose a reason for hiding this comment

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

Before continuing to listen for new messages, it must check whether the given chID is supported.

p2p/netaddress.go
func (na *NetAddress) Dial(ctx context.Context) (quic.Connection, error) {
tlsConfig := tls.Config{
MinVersion: tls.VersionTLS13,
InsecureSkipVerify: true,
Copy link
Member

@Wondertan Wondertan Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

IIUC, this means TLS is disabled, and according to recent additions to the code you meant to enable it, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

this function is not used anywhere, that's why I didn't fix its implementation. Maybe later

github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 20, 2024
View reviewed changes
p2p/netaddress.go
func (na *NetAddress) Dial(ctx context.Context) (quic.Connection, error) {
tlsConfig := tls.Config{
MinVersion: tls.VersionTLS13,
InsecureSkipVerify: true,

Check failure

Code scanning / CodeQL

Disabled TLS certificate check High

InsecureSkipVerify should not be used in production code.

Copilot Autofix AI about 15 hours ago

To fix the problem, we need to ensure that the TLS configuration does not disable certificate verification. This involves setting InsecureSkipVerify to false and properly configuring the TLS settings to use a valid certificate authority (CA) for verification.

  1. General Fix Approach:

    • Set InsecureSkipVerify to false.
    • Load the system's root CA certificates or specify a custom CA if needed.

  2. Detailed Fix:

    • Modify the tls.Config initialization to set InsecureSkipVerify to false.
    • Optionally, load the system's root CA certificates using x509.SystemCertPool().

  3. Specific Changes:

    • Update the tlsConfig initialization in the Dial function.
    • Add necessary imports and error handling for loading the CA certificates.
p2p/netaddress.go

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/p2p/netaddress.go b/p2p/netaddress.go
--- a/p2p/netaddress.go
+++ b/p2p/netaddress.go
@@ -265,5 +265,10 @@
 func (na *NetAddress) Dial(ctx context.Context) (quic.Connection, error) {
+	rootCAs, err := x509.SystemCertPool()
+	if err != nil {
+		return nil, fmt.Errorf("failed to load system root CAs: %v", err)
+	}
 	tlsConfig := tls.Config{
 		MinVersion:         tls.VersionTLS13,
-		InsecureSkipVerify: true,
+		RootCAs:            rootCAs,
+		InsecureSkipVerify: false,
 	}
EOF
@@ -265,5 +265,10 @@
func (na *NetAddress) Dial(ctx context.Context) (quic.Connection, error) {
rootCAs, err := x509.SystemCertPool()
if err != nil {
return nil, fmt.Errorf("failed to load system root CAs: %v", err)
}
tlsConfig := tls.Config{
MinVersion: tls.VersionTLS13,
InsecureSkipVerify: true,
RootCAs: rootCAs,
InsecureSkipVerify: false,
}
Copilot is powered by AI and may make mistakes. Always verify output.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Wondertan Wondertan Wondertan left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees

@rach-id rach-id

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rach-id @Wondertan