Merge pull request #734 from ainar-g/nopanicssl

Don't panic in SSL code

Author: maddyblue

conn.go

@@ -340,15 +340,19 @@ func DialOpen(d Dialer, name string) (_ driver.Conn, err error) {
 		return nil, err
 	}
 
-	// cn.ssl and cn.startup panic on error. Make sure we don't leak cn.c.
+	err = cn.ssl(o)
+	if err != nil {
+		return nil, err
+	}
+
+	// cn.startup panics on error. Make sure we don't leak cn.c.
 	panicking := true
 	defer func() {
 		if panicking {
 			cn.c.Close()
 		}
 	}()
 
-	cn.ssl(o)
 	cn.buf = bufio.NewReader(cn.c)
 	cn.startup(o)
 
@@ -1029,30 +1033,35 @@ func (cn *conn) recv1() (t byte, r *readBuf) {
 	return t, r
 }
 
-func (cn *conn) ssl(o values) {
-	upgrade := ssl(o)
+func (cn *conn) ssl(o values) error {
+	upgrade, err := ssl(o)
+	if err != nil {
+		return err
+	}
+
 	if upgrade == nil {
 		// Nothing to do
-		return
+		return nil
 	}
 
 	w := cn.writeBuf(0)
 	w.int32(80877103)
-	if err := cn.sendStartupPacket(w); err != nil {
-		panic(err)
+	if err = cn.sendStartupPacket(w); err != nil {
+		return err
 	}
 
 	b := cn.scratch[:1]
-	_, err := io.ReadFull(cn.c, b)
+	_, err = io.ReadFull(cn.c, b)
 	if err != nil {
-		panic(err)
+		return err
 	}
 
 	if b[0] != 'S' {
-		panic(ErrSSLNotSupported)
+		return ErrSSLNotSupported
 	}
 
-	cn.c = upgrade(cn.c)
+	cn.c, err = upgrade(cn.c)
+	return err
 }
 
 // isDriverSetting returns true iff a setting is purely for configuring the

conn_go18.go

@@ -108,7 +108,10 @@ func (cn *conn) cancel() error {
 		can := conn{
 			c: c,
 		}
-		can.ssl(cn.opts)
+		err = can.ssl(cn.opts)
+		if err != nil {
+			return err
+		}
 
 		w := can.writeBuf(0)
 		w.int32(80877102) // cancel request code

error.go

@@ -460,6 +460,11 @@ func errorf(s string, args ...interface{}) {
 	panic(fmt.Errorf("pq: %s", fmt.Sprintf(s, args...)))
 }
 
+// TODO(ainar-g) Rename to errorf after removing panics.
+func fmterrorf(s string, args ...interface{}) error {
+	return fmt.Errorf("pq: %s", fmt.Sprintf(s, args...))
+}
+
 func errRecoverNoErrBadConn(err *error) {
 	e := recover()
 	if e == nil {

ssl.go

@@ -12,7 +12,7 @@ import (
 
 // ssl generates a function to upgrade a net.Conn based on the "sslmode" and
 // related settings. The function is nil when no upgrade should take place.
-func ssl(o values) func(net.Conn) net.Conn {
+func ssl(o values) (func(net.Conn) (net.Conn, error), error) {
 	verifyCaOnly := false
 	tlsConf := tls.Config{}
 	switch mode := o["sslmode"]; mode {
@@ -45,29 +45,38 @@ func ssl(o values) func(net.Conn) net.Conn {
 	case "verify-full":
 		tlsConf.ServerName = o["host"]
 	case "disable":
-		return nil
+		return nil, nil
 	default:
-		errorf(`unsupported sslmode %q; only "require" (default), "verify-full", "verify-ca", and "disable" supported`, mode)
+		return nil, fmterrorf(`unsupported sslmode %q; only "require" (default), "verify-full", "verify-ca", and "disable" supported`, mode)
 	}
 
-	sslClientCertificates(&tlsConf, o)
-	sslCertificateAuthority(&tlsConf, o)
+	err := sslClientCertificates(&tlsConf, o)
+	if err != nil {
+		return nil, err
+	}
+	err = sslCertificateAuthority(&tlsConf, o)
+	if err != nil {
+		return nil, err
+	}
 	sslRenegotiation(&tlsConf)
 
-	return func(conn net.Conn) net.Conn {
+	return func(conn net.Conn) (net.Conn, error) {
 		client := tls.Client(conn, &tlsConf)
 		if verifyCaOnly {
-			sslVerifyCertificateAuthority(client, &tlsConf)
+			err := sslVerifyCertificateAuthority(client, &tlsConf)
+			if err != nil {
+				return nil, err
+			}
 		}
-		return client
-	}
+		return client, nil
+	}, nil
 }
 
 // sslClientCertificates adds the certificate specified in the "sslcert" and
 // "sslkey" settings, or if they aren't set, from the .postgresql directory
 // in the user's home directory. The configured files must exist and have
 // the correct permissions.
-func sslClientCertificates(tlsConf *tls.Config, o values) {
+func sslClientCertificates(tlsConf *tls.Config, o values) error {
 	// user.Current() might fail when cross-compiling. We have to ignore the
 	// error and continue without home directory defaults, since we wouldn't
 	// know from where to load them.
@@ -82,13 +91,13 @@ func sslClientCertificates(tlsConf *tls.Config, o values) {
 	}
 	// https://github.com/postgres/postgres/blob/REL9_6_2/src/interfaces/libpq/fe-secure-openssl.c#L1045
 	if len(sslcert) == 0 {
-		return
+		return nil
 	}
 	// https://github.com/postgres/postgres/blob/REL9_6_2/src/interfaces/libpq/fe-secure-openssl.c#L1050:L1054
 	if _, err := os.Stat(sslcert); os.IsNotExist(err) {
-		return
+		return nil
 	} else if err != nil {
-		panic(err)
+		return err
 	}
 
 	// In libpq, the ssl key is only loaded if the setting is not blank.
@@ -101,19 +110,21 @@ func sslClientCertificates(tlsConf *tls.Config, o values) {
 
 	if len(sslkey) > 0 {
 		if err := sslKeyPermissions(sslkey); err != nil {
-			panic(err)
+			return err
 		}
 	}
 
 	cert, err := tls.LoadX509KeyPair(sslcert, sslkey)
 	if err != nil {
-		panic(err)
+		return err
 	}
+
 	tlsConf.Certificates = []tls.Certificate{cert}
+	return nil
 }
 
 // sslCertificateAuthority adds the RootCA specified in the "sslrootcert" setting.
-func sslCertificateAuthority(tlsConf *tls.Config, o values) {
+func sslCertificateAuthority(tlsConf *tls.Config, o values) error {
 	// In libpq, the root certificate is only loaded if the setting is not blank.
 	//
 	// https://github.com/postgres/postgres/blob/REL9_6_2/src/interfaces/libpq/fe-secure-openssl.c#L950-L951
@@ -122,22 +133,24 @@ func sslCertificateAuthority(tlsConf *tls.Config, o values) {
 
 		cert, err := ioutil.ReadFile(sslrootcert)
 		if err != nil {
-			panic(err)
+			return err
 		}
 
 		if !tlsConf.RootCAs.AppendCertsFromPEM(cert) {
-			errorf("couldn't parse pem in sslrootcert")
+			return fmterrorf("couldn't parse pem in sslrootcert")
 		}
 	}
+
+	return nil
 }
 
 // sslVerifyCertificateAuthority carries out a TLS handshake to the server and
 // verifies the presented certificate against the CA, i.e. the one specified in
 // sslrootcert or the system CA if sslrootcert was not specified.
-func sslVerifyCertificateAuthority(client *tls.Conn, tlsConf *tls.Config) {
+func sslVerifyCertificateAuthority(client *tls.Conn, tlsConf *tls.Config) error {
 	err := client.Handshake()
 	if err != nil {
-		panic(err)
+		return err
 	}
 	certs := client.ConnectionState().PeerCertificates
 	opts := x509.VerifyOptions{
@@ -152,7 +165,5 @@ func sslVerifyCertificateAuthority(client *tls.Conn, tlsConf *tls.Config) {
 		opts.Intermediates.AddCert(cert)
 	}
 	_, err = certs[0].Verify(opts)
-	if err != nil {
-		panic(err)
-	}
+	return err
 }