dedis / onet

Overlay Network for distributed protocols
GNU Lesser General Public License v3.0
50 stars 29 forks source link

TLS handshake fails with Go 1.15 #653

Closed jeffallen closed 4 years ago

jeffallen commented 4 years ago

The tests in onet/network di not pass with Go 1.15.

Investigation notes

At least one problem is that the server returns an internal error right after the call to c.config.getCertificate. And that's because our GetCertificate callback is returning error x509: only CAs are allowed to specify MaxPathLen. So when we remove our use of MaxPathLen, then the server does not stop the handshake, but then the client fails the handshake with connecting: connecting: tcp connection: dial: certificate verification: x509: certificate relies on legacy Common Name field, use SANs or temporarily enable Common Name matching with GODEBUG=x509ignoreCN=0.

That environment variable does fix the problem. It is not possible for onet to set this variable itself, because it is only checked by the Go standard library at init time. Also, the docs say, "This setting might be removed in the future".

The (backwards compatible) solution to that one is possible to imagine... we'd need to put the CN side of the protocol into the SAN, put a second copy into the CN (for previous versions of onet to find).

ineiti commented 4 years ago

Would it be possible to use SANs for #652 , and then fix both in one PR?

jeffallen commented 4 years ago

Here's a very early draft at a fix for this, where we tunnel the response to the challenge back through the URIs field of the certificate:

diff --git a/network/tls.go b/network/tls.go
index b3e7af7..83d9100 100644
--- a/network/tls.go
+++ b/network/tls.go
@@ -10,8 +10,10 @@ import (
    "crypto/x509/pkix"
    "encoding/asn1"
    "encoding/hex"
+   "fmt"
    "math/big"
    "net"
+   "net/url"
    "time"

    "go.dedis.ch/kyber/v3"
@@ -146,16 +148,22 @@ func (cm *certMaker) get(nonce []byte) (*tls.Certificate, error) {
    r := random.Bits(128, true, random.New())
    serial.SetBytes(r)

+   uri, err := url.Parse(fmt.Sprintf("onet-response:%v", cm.subj.CommonName))
+   if err != nil {
+       return nil, err
+   }
+
    tmpl := &x509.Certificate{
        BasicConstraintsValid: true,
-       MaxPathLen:            1,
-       IsCA:                  false,
-       ExtKeyUsage:           []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth},
-       NotAfter:              time.Now().Add(2 * time.Hour),
-       NotBefore:             time.Now().Add(-5 * time.Minute),
-       SerialNumber:          serial,
-       SignatureAlgorithm:    x509.ECDSAWithSHA384,
-       Subject:               cm.subj,
+       //MaxPathLen:            1,
+       IsCA:               false,
+       ExtKeyUsage:        []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth},
+       NotAfter:           time.Now().Add(2 * time.Hour),
+       NotBefore:          time.Now().Add(-5 * time.Minute),
+       SerialNumber:       serial,
+       SignatureAlgorithm: x509.ECDSAWithSHA384,
+       Subject:            cm.subj,
+       URIs:               []*url.URL{uri},
        ExtraExtensions: []pkix.Extension{
            {
                Id:       oidDedisSig,
@@ -323,9 +331,17 @@ func makeVerifier(suite Suite, them *ServerIdentity) (verifier, []byte) {
        // When we know who we are connecting to (e.g. client mode):
        // Check that the CN is the same as the public key.
        if them != nil {
-           err = cert.VerifyHostname(pubToCN(them.Public))
-           if err != nil {
-               return xerrors.Errorf("certificate verification: %v", err)
+           cn := pubToCN(them.Public)
+           if len(cert.URIs) > 0 {
+               if cert.URIs[0].Scheme != "onet-response" ||
+                   cert.URIs[0].Opaque != cn {
+                   return xerrors.Errorf("certificate uri %#v wrong", cert.URIs[0])
+               }
+           } else {
+               err = cert.VerifyHostname(cn)
+               if err != nil {
+                   return xerrors.Errorf("certificate verification: %v", err)
+               }
            }
        }

I need to sleep on this and think about the backwards compatibility issues a bit more, but there's clearly a nice path forward here by stepping off of the "sneak it through the hostname" treadmill, where we could get caught up later again.

ineiti commented 4 years ago

Just for anybody else reading this: the golang implementation seems to support only four Subject Alternate Name values (https://godoc.org/crypto/x509#Certificate):

So the choice of URIs seems to be the best one.