Fix MySQL legacy username regression (#3141)

* Fix the mysql legacy username length

* Remove boolean parameter

* Add a MySQL 5.6 container to test the legacy MySQL plugin against

* Add database plugins to the make file

* Fix credsutil test

Makefile

@@ -79,5 +79,25 @@ fmtcheck:
 fmt:
 	gofmt -w $(GOFMT_FILES)
 
+mysql-database-plugin:
+	@CGO_ENABLED=0 go build -o bin/mysql-database-plugin ./plugins/database/mysql/mysql-database-plugin
 
-.PHONY: bin default generate test vet bootstrap fmt fmtcheck
+mysql-legacy-database-plugin:
+	@CGO_ENABLED=0 go build -o bin/mysql-legacy-database-plugin ./plugins/database/mysql/mysql-legacy-database-plugin
+
+cassandra-database-plugin:
+	@CGO_ENABLED=0 go build -o bin/cassandra-database-plugin ./plugins/database/cassandra/cassandra-database-plugin
+
+postgresql-database-plugin:
+	@CGO_ENABLED=0 go build -o bin/postgresql-database-plugin ./plugins/database/postgresql/postgresql-database-plugin
+
+mssql-database-plugin:
+	@CGO_ENABLED=0 go build -o bin/mssql-database-plugin ./plugins/database/mssql/mssql-database-plugin
+
+hana-database-plugin:
+	@CGO_ENABLED=0 go build -o bin/hana-database-plugin ./plugins/database/hana/hana-database-plugin
+
+mongodb-database-plugin:
+	@CGO_ENABLED=0 go build -o bin/mongodb-database-plugin ./plugins/database/mongodb/mongodb-database-plugin
+
+.PHONY: bin default generate test vet bootstrap fmt fmtcheck mysql-database-plugin mysql-legacy-database-plugin cassandra-database-plugin postgresql-database-plugin mssql-database-plugin hana-database-plugin mongodb-database-plugin

helper/builtinplugins/builtin.go

@@ -7,6 +7,7 @@ import (
 	"github.com/hashicorp/vault/plugins/database/mssql"
 	"github.com/hashicorp/vault/plugins/database/mysql"
 	"github.com/hashicorp/vault/plugins/database/postgresql"
+	"github.com/hashicorp/vault/plugins/helper/database/credsutil"
 )
 
 // BuiltinFactory is the func signature that should be returned by
@@ -16,10 +17,10 @@ type BuiltinFactory func() (interface{}, error)
 var plugins = map[string]BuiltinFactory{
 	// These four plugins all use the same mysql implementation but with
 	// different username settings passed by the constructor.
-	"mysql-database-plugin":        mysql.New(mysql.MetadataLen, mysql.UsernameLen),
-	"mysql-aurora-database-plugin": mysql.New(mysql.LegacyMetadataLen, mysql.LegacyUsernameLen),
-	"mysql-rds-database-plugin":    mysql.New(mysql.LegacyMetadataLen, mysql.LegacyUsernameLen),
-	"mysql-legacy-database-plugin": mysql.New(mysql.LegacyMetadataLen, mysql.LegacyUsernameLen),
+	"mysql-database-plugin":        mysql.New(mysql.MetadataLen, mysql.MetadataLen, mysql.UsernameLen),
+	"mysql-aurora-database-plugin": mysql.New(credsutil.NoneLength, mysql.LegacyMetadataLen, mysql.LegacyUsernameLen),
+	"mysql-rds-database-plugin":    mysql.New(credsutil.NoneLength, mysql.LegacyMetadataLen, mysql.LegacyUsernameLen),
+	"mysql-legacy-database-plugin": mysql.New(credsutil.NoneLength, mysql.LegacyMetadataLen, mysql.LegacyUsernameLen),
 
 	"postgresql-database-plugin": postgresql.New,
 	"mssql-database-plugin":      mssql.New,

plugins/database/mysql/mysql-legacy-database-plugin/main.go

@@ -0,0 +1,21 @@
+package main
+
+import (
+	"log"
+	"os"
+
+	"github.com/hashicorp/vault/helper/pluginutil"
+	"github.com/hashicorp/vault/plugins/database/mysql"
+)
+
+func main() {
+	apiClientMeta := &pluginutil.APIClientMeta{}
+	flags := apiClientMeta.FlagSet()
+	flags.Parse(os.Args)
+
+	err := mysql.RunLegacy(apiClientMeta.GetTLSConfig())
+	if err != nil {
+		log.Println(err)
+		os.Exit(1)
+	}
+}

plugins/database/mysql/mysql.go

@@ -36,14 +36,14 @@ type MySQL struct {
 }
 
 // New implements builtinplugins.BuiltinFactory
-func New(metadataLen, usernameLen int) func() (interface{}, error) {
+func New(displayNameLen, roleNameLen, usernameLen int) func() (interface{}, error) {
 	return func() (interface{}, error) {
 		connProducer := &connutil.SQLConnectionProducer{}
 		connProducer.Type = mySQLTypeName
 
 		credsProducer := &credsutil.SQLCredentialsProducer{
-			DisplayNameLen: metadataLen,
-			RoleNameLen:    metadataLen,
+			DisplayNameLen: displayNameLen,
+			RoleNameLen:    roleNameLen,
 			UsernameLen:    usernameLen,
 			Separator:      "-",
 		}
@@ -59,7 +59,21 @@ func New(metadataLen, usernameLen int) func() (interface{}, error) {
 
 // Run instantiates a MySQL object, and runs the RPC server for the plugin
 func Run(apiTLSConfig *api.TLSConfig) error {
-	f := New(MetadataLen, UsernameLen)
+	return runCommon(false, apiTLSConfig)
+}
+
+// Run instantiates a MySQL object, and runs the RPC server for the plugin
+func RunLegacy(apiTLSConfig *api.TLSConfig) error {
+	return runCommon(true, apiTLSConfig)
+}
+
+func runCommon(legacy bool, apiTLSConfig *api.TLSConfig) error {
+	var f func() (interface{}, error)
+	if legacy {
+		f = New(credsutil.NoneLength, LegacyMetadataLen, LegacyUsernameLen)
+	} else {
+		f = New(MetadataLen, MetadataLen, UsernameLen)
+	}
 	dbType, err := f()
 	if err != nil {
 		return err

plugins/database/mysql/mysql_test.go

@@ -5,19 +5,15 @@ import (
 	"fmt"
 	"os"
 	"strings"
-	"sync"
 	"testing"
 	"time"
 
 	"github.com/hashicorp/vault/builtin/logical/database/dbplugin"
 	"github.com/hashicorp/vault/plugins/helper/database/connutil"
+	"github.com/hashicorp/vault/plugins/helper/database/credsutil"
 	dockertest "gopkg.in/ory-am/dockertest.v3"
 )
 
-var (
-	testMySQLImagePull sync.Once
-)
-
 func prepareMySQLTestContainer(t *testing.T) (cleanup func(), retURL string) {
 	if os.Getenv("MYSQL_URL") != "" {
 		return func() {}, os.Getenv("MYSQL_URL")
@@ -58,6 +54,47 @@ func prepareMySQLTestContainer(t *testing.T) (cleanup func(), retURL string) {
 	return
 }
 
+func prepareMySQLLegacyTestContainer(t *testing.T) (cleanup func(), retURL string) {
+	if os.Getenv("MYSQL_URL") != "" {
+		return func() {}, os.Getenv("MYSQL_URL")
+	}
+
+	pool, err := dockertest.NewPool("")
+	if err != nil {
+		t.Fatalf("Failed to connect to docker: %s", err)
+	}
+
+	// Mysql 5.6 is the last MySQL version to limit usernames to 16 characters.
+	resource, err := pool.Run("mysql", "5.6", []string{"MYSQL_ROOT_PASSWORD=secret"})
+	if err != nil {
+		t.Fatalf("Could not start local MySQL docker container: %s", err)
+	}
+
+	cleanup = func() {
+		err := pool.Purge(resource)
+		if err != nil {
+			t.Fatalf("Failed to cleanup local container: %s", err)
+		}
+	}
+
+	retURL = fmt.Sprintf("root:secret@(localhost:%s)/mysql?parseTime=true", resource.GetPort("3306/tcp"))
+
+	// exponential backoff-retry
+	if err = pool.Retry(func() error {
+		var err error
+		var db *sql.DB
+		db, err = sql.Open("mysql", retURL)
+		if err != nil {
+			return err
+		}
+		return db.Ping()
+	}); err != nil {
+		t.Fatalf("Could not connect to MySQL docker container: %s", err)
+	}
+
+	return
+}
+
 func TestMySQL_Initialize(t *testing.T) {
 	cleanup, connURL := prepareMySQLTestContainer(t)
 	defer cleanup()
@@ -66,7 +103,7 @@ func TestMySQL_Initialize(t *testing.T) {
 		"connection_url": connURL,
 	}
 
-	f := New(MetadataLen, UsernameLen)
+	f := New(MetadataLen, MetadataLen, UsernameLen)
 	dbRaw, _ := f()
 	db := dbRaw.(*MySQL)
 	connProducer := db.ConnectionProducer.(*connutil.SQLConnectionProducer)
@@ -105,7 +142,7 @@ func TestMySQL_CreateUser(t *testing.T) {
 		"connection_url": connURL,
 	}
 
-	f := New(MetadataLen, UsernameLen)
+	f := New(MetadataLen, MetadataLen, UsernameLen)
 	dbRaw, _ := f()
 	db := dbRaw.(*MySQL)
 
@@ -115,8 +152,8 @@ func TestMySQL_CreateUser(t *testing.T) {
 	}
 
 	usernameConfig := dbplugin.UsernameConfig{
-		DisplayName: "test",
-		RoleName:    "test",
+		DisplayName: "test-long-displayname",
+		RoleName:    "test-long-rolename",
 	}
 
 	// Test with no configured Creation Statememt
@@ -137,6 +174,68 @@ func TestMySQL_CreateUser(t *testing.T) {
 	if err := testCredsExist(t, connURL, username, password); err != nil {
 		t.Fatalf("Could not connect with new credentials: %s", err)
 	}
+
+	// Test a second time to make sure usernames don't collide
+	username, password, err = db.CreateUser(statements, usernameConfig, time.Now().Add(time.Minute))
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	if err := testCredsExist(t, connURL, username, password); err != nil {
+		t.Fatalf("Could not connect with new credentials: %s", err)
+	}
+}
+
+func TestMySQL_CreateUser_Legacy(t *testing.T) {
+	cleanup, connURL := prepareMySQLLegacyTestContainer(t)
+	defer cleanup()
+
+	connectionDetails := map[string]interface{}{
+		"connection_url": connURL,
+	}
+
+	f := New(credsutil.NoneLength, LegacyMetadataLen, LegacyUsernameLen)
+	dbRaw, _ := f()
+	db := dbRaw.(*MySQL)
+
+	err := db.Initialize(connectionDetails, true)
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	usernameConfig := dbplugin.UsernameConfig{
+		DisplayName: "test-long-displayname",
+		RoleName:    "test-long-rolename",
+	}
+
+	// Test with no configured Creation Statememt
+	_, _, err = db.CreateUser(dbplugin.Statements{}, usernameConfig, time.Now().Add(time.Minute))
+	if err == nil {
+		t.Fatal("Expected error when no creation statement is provided")
+	}
+
+	statements := dbplugin.Statements{
+		CreationStatements: testMySQLRoleWildCard,
+	}
+
+	username, password, err := db.CreateUser(statements, usernameConfig, time.Now().Add(time.Minute))
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	if err := testCredsExist(t, connURL, username, password); err != nil {
+		t.Fatalf("Could not connect with new credentials: %s", err)
+	}
+
+	// Test a second time to make sure usernames don't collide
+	username, password, err = db.CreateUser(statements, usernameConfig, time.Now().Add(time.Minute))
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	if err := testCredsExist(t, connURL, username, password); err != nil {
+		t.Fatalf("Could not connect with new credentials: %s", err)
+	}
 }
 
 func TestMySQL_RevokeUser(t *testing.T) {
@@ -147,7 +246,7 @@ func TestMySQL_RevokeUser(t *testing.T) {
 		"connection_url": connURL,
 	}
 
-	f := New(MetadataLen, UsernameLen)
+	f := New(MetadataLen, MetadataLen, UsernameLen)
 	dbRaw, _ := f()
 	db := dbRaw.(*MySQL)
 

plugins/helper/database/credsutil/credsutil.go

@@ -27,15 +27,21 @@ const (
 // of the provided length. The string generated takes up to 4 characters
 // of space that are predefined and prepended to ensure password
 // character requirements. It also requires a min length of 10 characters.
-func RandomAlphaNumeric(length int) (string, error) {
+func RandomAlphaNumeric(length int, prependA1a bool) (string, error) {
 	if length < minStrLen {
 		return "", fmt.Errorf("minimum length of %d is required", minStrLen)
 	}
-	size := len(reqStr)
 
-	retBytes := make([]byte, length-size)
+	var size int
+	var retBytes []byte
+	if prependA1a {
+		size = len(reqStr)
+		retBytes = make([]byte, length-size)
 		// Enforce alphanumeric requirements
 		retBytes = append([]byte(reqStr), retBytes...)
+	} else {
+		retBytes = make([]byte, length)
+	}
 
 	for size < length {
 		// Extend the len of the random byte slice to lower odds of having to

plugins/helper/database/credsutil/credsutil_test.go

@@ -6,7 +6,7 @@ import (
 )
 
 func TestRandomAlphaNumeric(t *testing.T) {
-	s, err := RandomAlphaNumeric(10)
+	s, err := RandomAlphaNumeric(10, true)
 	if err != nil {
 		t.Fatalf("Unexpected error: %s", err)
 	}
@@ -14,7 +14,7 @@ func TestRandomAlphaNumeric(t *testing.T) {
 		t.Fatalf("Unexpected length of string, expected 10, got string: %s", s)
 	}
 
-	s, err = RandomAlphaNumeric(20)
+	s, err = RandomAlphaNumeric(20, true)
 	if err != nil {
 		t.Fatalf("Unexpected error: %s", err)
 	}
@@ -25,4 +25,16 @@ func TestRandomAlphaNumeric(t *testing.T) {
 	if !strings.Contains(s, reqStr) {
 		t.Fatalf("Expected %s to contain %s", s, reqStr)
 	}
+
+	s, err = RandomAlphaNumeric(20, false)
+	if err != nil {
+		t.Fatalf("Unexpected error: %s", err)
+	}
+	if len(s) != 20 {
+		t.Fatalf("Unexpected length of string, expected 20, got string: %s", s)
+	}
+
+	if strings.Contains(s, reqStr) {
+		t.Fatalf("Expected %s not to contain %s", s, reqStr)
+	}
 }

plugins/helper/database/credsutil/sql.go

@@ -2,12 +2,15 @@ package credsutil
 
 import (
 	"fmt"
-	"strings"
 	"time"
 
 	"github.com/hashicorp/vault/builtin/logical/database/dbplugin"
 )
 
+const (
+	NoneLength int = -1
+)
+
 // SQLCredentialsProducer implements CredentialsProducer and provides a generic credentials producer for most sql database types.
 type SQLCredentialsProducer struct {
 	DisplayNameLen int
@@ -17,21 +20,37 @@ type SQLCredentialsProducer struct {
 }
 
 func (scp *SQLCredentialsProducer) GenerateUsername(config dbplugin.UsernameConfig) (string, error) {
+	username := "v"
+
 	displayName := config.DisplayName
 	if scp.DisplayNameLen > 0 && len(displayName) > scp.DisplayNameLen {
 		displayName = displayName[:scp.DisplayNameLen]
+	} else if scp.DisplayNameLen == NoneLength {
+		displayName = ""
 	}
+
+	if len(displayName) > 0 {
+		username = fmt.Sprintf("%s%s%s", username, scp.Separator, displayName)
+	}
+
 	roleName := config.RoleName
 	if scp.RoleNameLen > 0 && len(roleName) > scp.RoleNameLen {
 		roleName = roleName[:scp.RoleNameLen]
+	} else if scp.RoleNameLen == NoneLength {
+		roleName = ""
 	}
 
-	userUUID, err := RandomAlphaNumeric(20)
+	if len(roleName) > 0 {
+		username = fmt.Sprintf("%s%s%s", username, scp.Separator, roleName)
+	}
+
+	userUUID, err := RandomAlphaNumeric(20, false)
 	if err != nil {
 		return "", err
 	}
 
-	username := strings.Join([]string{"v", displayName, roleName, userUUID, fmt.Sprint(time.Now().UTC().Unix())}, scp.Separator)
+	username = fmt.Sprintf("%s%s%s", username, scp.Separator, userUUID)
+	username = fmt.Sprintf("%s%s%s", username, scp.Separator, fmt.Sprint(time.Now().UTC().Unix()))
 	if scp.UsernameLen > 0 && len(username) > scp.UsernameLen {
 		username = username[:scp.UsernameLen]
 	}
@@ -40,7 +59,7 @@ func (scp *SQLCredentialsProducer) GenerateUsername(config dbplugin.UsernameConf
 }
 
 func (scp *SQLCredentialsProducer) GeneratePassword() (string, error) {
-	password, err := RandomAlphaNumeric(20)
+	password, err := RandomAlphaNumeric(20, true)
 	if err != nil {
 		return "", err
 	}