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.

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

tls: fetch full certificate chain in case of failure #1551

Open
bassosimone opened this issue May 21, 2020 · 4 comments
Open

tls: fetch full certificate chain in case of failure #1551

bassosimone opened this issue May 21, 2020 · 4 comments
Assignees
Labels
GSoC GSoC related issues methodology issues related to the testing methodology priority/low

Comments

@bassosimone
Copy link
Contributor

This seems to be doable by using the following code:

		opts := x509.VerifyOptions{
			Roots:         c.config.RootCAs,
			CurrentTime:   c.config.time(),
			DNSName:       c.config.ServerName,
			Intermediates: x509.NewCertPool(),
		}
		for _, cert := range certs[1:] {
			opts.Intermediates.AddCert(cert)
		}
		var err error
		c.verifiedChains, err = certs[0].Verify(opts)
		if err != nil {
			c.sendAlert(alertBadCertificate)
			return err
		}

copied from the standard library as the body of the VerifyConnection callback of tls.Config, as shown in the following example code:

package main

import (
	"crypto/tls"
	"crypto/x509"
	"errors"
)

func main() {
	// VerifyPeerCertificate can be used to replace and customize certificate
	// verification. This example shows a VerifyPeerCertificate implementation
	// that will be approximately equivalent to what crypto/tls does normally.

	config := &tls.Config{
		// Set InsecureSkipVerify to skip the default validation we are
		// replacing. This will not disable VerifyPeerCertificate.
		InsecureSkipVerify: true,

		// While packages like net/http will implicitly set ServerName, the
		// VerifyPeerCertificate callback can't access that value, so it has to be set
		// explicitly here or in VerifyPeerCertificate on the client side. If in
		// an http.Transport DialTLS callback, this can be obtained by passing
		// the addr argument to net.SplitHostPort.
		ServerName: "example.com",

		// On the server side, set ClientAuth to require client certificates (or
		// VerifyPeerCertificate will run anyway and panic accessing certs[0])
		// but not verify them with the default verifier.
		// ClientAuth: tls.RequireAnyClientCert,
	}

	config.VerifyPeerCertificate = func(certificates [][]byte, _ [][]*x509.Certificate) error {
		certs := make([]*x509.Certificate, len(certificates))
		for i, asn1Data := range certificates {
			cert, err := x509.ParseCertificate(asn1Data)
			if err != nil {
				return errors.New("tls: failed to parse certificate from server: " + err.Error())
			}
			certs[i] = cert
		}

		opts := x509.VerifyOptions{
			Roots:         config.RootCAs, // On the server side, use config.ClientCAs.
			DNSName:       config.ServerName,
			Intermediates: x509.NewCertPool(),
			// On the server side, set KeyUsages to ExtKeyUsageClientAuth. The
			// default value is appropriate for clients side verification.
			// KeyUsages:     []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth},
		}
		for _, cert := range certs[1:] {
			opts.Intermediates.AddCert(cert)
		}
		_, err := certs[0].Verify(opts)
		return err
	}

	// Note that when InsecureSkipVerify and VerifyPeerCertificate are in use,
	// ConnectionState.VerifiedChains will be nil.
}

Instead of c.config.time() we could directly use time.Now(), in fact:

func (c *Config) time() time.Time {
	t := c.Time
	if t == nil {
		t = time.Now
	}
	return t()
}

or we can just leave it empty as in the example, because:

	// CurrentTime is used to check the validity of all certificates in the
	// chain. If zero, the current time is used.
	CurrentTime time.Time
@bassosimone bassosimone self-assigned this May 21, 2020
@bassosimone
Copy link
Contributor Author

Reduced effort because https://app.zenhub.com/workspace/o/ooni/probe-engine/issues/626 required more work than I anticipated.

@bassosimone
Copy link
Contributor Author

Effort increased with 3 points from #886.

@bassosimone
Copy link
Contributor Author

Dropping from milestone and moving the 8 points of this issue to ooni/probe-engine#619, which has required a significant data analysis.

@bassosimone
Copy link
Contributor Author

We're doing some work with @kelmenhorst that is useful to fix this issue.

@bassosimone bassosimone transferred this issue from ooni/probe-engine Jun 7, 2021
@bassosimone bassosimone added GSoC GSoC related issues methodology issues related to the testing methodology labels Feb 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GSoC GSoC related issues methodology issues related to the testing methodology priority/low
Projects
Status: No status
Development

No branches or pull requests

1 participant