Merge pull request #390 from daniel-nichter/fix-389-socket-auth-fail-broken-pipe-error

Fixes #389 by not sending COM_QUIT until authenticated. Also refactor…

Author: julienschmidt

AUTHORS

@@ -15,6 +15,7 @@ Aaron Hopkins <go-sql-driver at die.net>
 Arne Hormann <arnehormann at gmail.com>
 Carlos Nieto <jose.carlos at menteslibres.net>
 Chris Moos <chris at tech9computers.com>
+Daniel Nichter <nil at codenode.com>
 DisposaBoy <disposaboy at dby.me>
 Frederick Mayle <frederickmayle at gmail.com>
 Gustavo Kristic <gkristic at gmail.com>
@@ -25,6 +26,7 @@ INADA Naoki <songofacandy at gmail.com>
 James Harr <james.harr at gmail.com>
 Jian Zhen <zhenjl at gmail.com>
 Joshua Prunier <joshua.prunier at gmail.com>
+Julien Lefevre <julien.lefevr at gmail.com>
 Julien Schmidt <go-sql-driver at julienschmidt.com>
 Kamil Dziedzic <kamil at klecza.pl>
 Kevin Malachowski <kevin at chowski.com>
@@ -38,7 +40,6 @@ Soroush Pour <me at soroushjp.com>
 Stan Putrya <root.vagner at gmail.com>
 Xiaobing Jiang <s7v7nislands at gmail.com>
 Xiuming Chen <cc at cxm.cc>
-Julien Lefevre <julien.lefevr at gmail.com>
 
 # Organizations
 

connection.go

@@ -120,18 +120,27 @@ func (mc *mysqlConn) Close() (err error) {
 	// Makes Close idempotent
 	if mc.netConn != nil {
 		err = mc.writeCommandPacket(comQuit)
-		if err == nil {
-			err = mc.netConn.Close()
-		} else {
-			mc.netConn.Close()
+	}
+
+	mc.cleanup()
+
+	return
+}
+
+// Closes the network connection and unsets internal variables. Do not call this
+// function after successfully authentication, call Close instead. This function
+// is called before auth or on auth failure because MySQL will have already
+// closed the network connection.
+func (mc *mysqlConn) cleanup() {
+	// Makes cleanup idempotent
+	if mc.netConn != nil {
+		if err := mc.netConn.Close(); err != nil {
+			errLog.Print(err)
 		}
 		mc.netConn = nil
 	}
-
 	mc.cfg = nil
 	mc.buf.rd = nil
-
-	return
 }
 
 func (mc *mysqlConn) Prepare(query string) (driver.Stmt, error) {

driver.go

@@ -84,43 +84,23 @@ func (d MySQLDriver) Open(dsn string) (driver.Conn, error) {
 	// Reading Handshake Initialization Packet
 	cipher, err := mc.readInitPacket()
 	if err != nil {
-		mc.Close()
+		mc.cleanup()
 		return nil, err
 	}
 
 	// Send Client Authentication Packet
 	if err = mc.writeAuthPacket(cipher); err != nil {
-		mc.Close()
+		mc.cleanup()
 		return nil, err
 	}
 
-	// Read Result Packet
-	err = mc.readResultOK()
-	if err != nil {
-		// Retry with old authentication method, if allowed
-		if mc.cfg != nil && mc.cfg.allowOldPasswords && err == ErrOldPassword {
-			if err = mc.writeOldAuthPacket(cipher); err != nil {
-				mc.Close()
-				return nil, err
-			}
-			if err = mc.readResultOK(); err != nil {
-				mc.Close()
-				return nil, err
-			}
-		} else if mc.cfg != nil && mc.cfg.allowCleartextPasswords && err == ErrCleartextPassword {
-			if err = mc.writeClearAuthPacket(); err != nil {
-				mc.Close()
-				return nil, err
-			}
-			if err = mc.readResultOK(); err != nil {
-				mc.Close()
-				return nil, err
-			}
-		} else {
-			mc.Close()
-			return nil, err
-		}
-
+	// Handle response to auth packet, switch methods if possible
+	if err = handleAuthResult(mc, cipher); err != nil {
+		// Authentication failed and MySQL has already closed the connection
+		// (https://dev.mysql.com/doc/internals/en/authentication-fails.html).
+		// Do not send COM_QUIT, just cleanup and return the error.
+		mc.cleanup()
+		return nil, err
 	}
 
 	// Get max allowed packet size
@@ -144,6 +124,38 @@ func (d MySQLDriver) Open(dsn string) (driver.Conn, error) {
 	return mc, nil
 }
 
+func handleAuthResult(mc *mysqlConn, cipher []byte) error {
+	// Read Result Packet
+	err := mc.readResultOK()
+	if err == nil {
+		return nil // auth successful
+	}
+
+	if mc.cfg == nil {
+		return err // auth failed and retry not possible
+	}
+
+	// Retry auth if configured to do so.
+	if mc.cfg.allowOldPasswords && err == ErrOldPassword {
+		// Retry with old authentication method. Note: there are edge cases
+		// where this should work but doesn't; this is currently "wontfix":
+		// https://github.com/go-sql-driver/mysql/issues/184
+		if err = mc.writeOldAuthPacket(cipher); err != nil {
+			return err
+		}
+		err = mc.readResultOK()
+	} else if mc.cfg.allowCleartextPasswords && err == ErrCleartextPassword {
+		// Retry with clear text password for
+		// http://dev.mysql.com/doc/refman/5.7/en/cleartext-authentication-plugin.html
+		// http://dev.mysql.com/doc/refman/5.7/en/pam-authentication-plugin.html
+		if err = mc.writeClearAuthPacket(); err != nil {
+			return err
+		}
+		err = mc.readResultOK()
+	}
+	return err
+}
+
 func init() {
 	sql.Register("mysql", &MySQLDriver{})
 }

driver_test.go

@@ -9,12 +9,14 @@
 package mysql
 
 import (
+	"bytes"
 	"crypto/tls"
 	"database/sql"
 	"database/sql/driver"
 	"fmt"
 	"io"
 	"io/ioutil"
+	"log"
 	"net"
 	"net/url"
 	"os"
@@ -1679,3 +1681,50 @@ func TestInsertRetrieveEscapedData(t *testing.T) {
 		runTests(t, testdsn, testData)
 	}
 }
+
+func TestUnixSocketAuthFail(t *testing.T) {
+	runTests(t, dsn, func(dbt *DBTest) {
+		// Save the current logger so we can restore it.
+		oldLogger := errLog
+
+		// Set a new logger so we can capture its output.
+		buffer := bytes.NewBuffer(make([]byte, 0, 64))
+		newLogger := log.New(buffer, "prefix: ", 0)
+		SetLogger(newLogger)
+
+		// Restore the logger.
+		defer SetLogger(oldLogger)
+
+		// Make a new DSN that uses the MySQL socket file and a bad password, which
+		// we can make by simply appending any character to the real password.
+		badPass := pass + "x"
+		socket := ""
+		if prot == "unix" {
+			socket = addr
+		} else {
+			// Get socket file from MySQL.
+			err := dbt.db.QueryRow("SELECT @@socket").Scan(&socket)
+			if err != nil {
+				t.Fatalf("Error on SELECT @@socket: %s", err.Error())
+			}
+		}
+		t.Logf("socket: %s", socket)
+		badDSN := fmt.Sprintf("%s:%s@unix(%s)/%s?timeout=30s&strict=true", user, badPass, socket, dbname)
+		db, err := sql.Open("mysql", badDSN)
+		if err != nil {
+			t.Fatalf("Error connecting: %s", err.Error())
+		}
+		defer db.Close()
+
+		// Connect to MySQL for real. This will cause an auth failure.
+		err = db.Ping()
+		if err == nil {
+			t.Error("expected Ping() to return an error")
+		}
+
+		// The driver should not log anything.
+		if actual := buffer.String(); actual != "" {
+			t.Errorf("expected no output, got %q", actual)
+		}
+	})
+}