-
Notifications
You must be signed in to change notification settings - Fork 264
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.
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
WIP: quic integration #1466
base: v0.34.x-celestia
Are you sure you want to change the base?
WIP: quic integration #1466
Conversation
} | ||
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
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 7 days ago
To fix the problem, we need to ensure that the TLS certificate verification is properly handled. This involves:
- Removing the
InsecureSkipVerify: true
setting. - 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.
-
Copy modified lines R67-R87
@@ -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 | ||
}, |
There was a problem hiding this comment.
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
Outdated
labels := []string{ | ||
"peer_id", string(p.ID()), | ||
"chID", fmt.Sprintf("%#x", chID), | ||
stream, has := p.streams[chID] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
msg, err = w.Unwrap() | ||
if err != nil { | ||
panic(fmt.Errorf("unwrapping message: %s", err)) | ||
// start accepting data |
There was a problem hiding this comment.
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.
func (na *NetAddress) Dial(ctx context.Context) (quic.Connection, error) { | ||
tlsConfig := tls.Config{ | ||
MinVersion: tls.VersionTLS13, | ||
InsecureSkipVerify: true, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
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
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 7 days 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.
-
General Fix Approach:
- Set
InsecureSkipVerify
tofalse
. - Load the system's root CA certificates or specify a custom CA if needed.
- Set
-
Detailed Fix:
- Modify the
tls.Config
initialization to setInsecureSkipVerify
tofalse
. - Optionally, load the system's root CA certificates using
x509.SystemCertPool()
.
- Modify the
-
Specific Changes:
- Update the
tlsConfig
initialization in theDial
function. - Add necessary imports and error handling for loading the CA certificates.
- Update the
-
Copy modified lines R266-R269 -
Copy modified lines R272-R273
@@ -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, | ||
} |
c627746
to
21d8e38
Compare
Description
This is just a prototype, doesn't need to be reviewed by automatic reviewers