WIP: quic integration #1466
WIP: quic integration #1466
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
| 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 11 days ago
To fix the problem, we need to ensure that TLS certificate verification is enabled and properly implemented. This involves:
- Removing the
InsecureSkipVerify: truesetting. - Implementing the
VerifyPeerCertificatefunction 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: trueline. - Implement the
VerifyPeerCertificatefunction to verify the certificate chain.
| @@ -67,7 +67,26 @@ | ||
| 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") | ||
| VerifyPeerCertificate: func(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error { | ||
| certs := make([]*x509.Certificate, len(rawCerts)) | ||
| for i, rawCert := range rawCerts { | ||
| cert, err := x509.ParseCertificate(rawCert) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| certs[i] = cert | ||
| } | ||
| opts := x509.VerifyOptions{ | ||
| Roots: x509.NewCertPool(), | ||
| Intermediates: x509.NewCertPool(), | ||
| } | ||
| for _, cert := range certs { | ||
| if cert.IsCA { | ||
| opts.Roots.AddCert(cert) | ||
| } else { | ||
| opts.Intermediates.AddCert(cert) | ||
| } | ||
| } | ||
| _, err := certs[0].Verify(opts) | ||
| return err | ||
| }, |
There was a problem hiding this comment.
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.
Send is called concurrently, so access to streams must be synchronized.
There was a problem hiding this comment.
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.
Before continuing to listen for new messages, it must check whether the given chID is supported.
p2p/netaddress.go
Outdated
| 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.
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.
this function is not used anywhere, that's why I didn't fix its implementation. Maybe later
Description
This is just a prototype, doesn't need to be reviewed by automatic reviewers