mfa: code cleanup

builtin/credential/userpass/cli.go

@@ -29,12 +29,12 @@ func (h *CLIHandler) Auth(c *api.Client, m map[string]string) (string, error) {
 	}
 	if data.Password == "" {
 		fmt.Printf("Password (will be hidden): ")
-		var err error
-		data.Password, err = pwd.Read(os.Stdin)
+		password, err := pwd.Read(os.Stdin)
 		fmt.Println()
 		if err != nil {
 			return "", err
 		}
+		data.Password = password
 	}
 	if data.Mount == "" {
 		data.Mount = "userpass"

helper/mfa/duo/duo.go

@@ -40,24 +40,38 @@ func DuoHandler(req *logical.Request, d *framework.FieldData, resp *logical.Resp
 		return logical.ErrorResponse("Could not read username for MFA"), nil
 	}
 
-	method := d.Get("method").(string)
-	passcode := d.Get("passcode").(string)
+	var request *duoAuthRequest = &duoAuthRequest{}
+	request.successResp = resp
+	request.username = username
+	request.method = d.Get("method").(string)
+	request.passcode = d.Get("passcode").(string)
+	request.ipAddr = req.Connection.RemoteAddr
 
-	return duoHandler(duoConfig, duoAuthClient, resp,
-		username, method, passcode, req.Connection.RemoteAddr)
+	return duoHandler(duoConfig, duoAuthClient, request)
 }
 
-func duoHandler(
-	duoConfig *DuoConfig, duoAuthClient AuthClient, successResp *logical.Response,
-	username string, method string, passcode string, ipAddr string) (*logical.Response, error) {
+type duoAuthRequest struct {
+	successResp *logical.Response
+	username string
+	method string
+	passcode string
+	ipAddr string
+}
 
-	duoUser := fmt.Sprintf(duoConfig.UsernameFormat, username)
+func duoHandler(duoConfig *DuoConfig, duoAuthClient AuthClient, request *duoAuthRequest) (
+	*logical.Response, error) {
+
+	duoUser := fmt.Sprintf(duoConfig.UsernameFormat, request.username)
 
 	preauth, err := duoAuthClient.Preauth(
 		authapi.PreauthUsername(duoUser),
-		authapi.PreauthIpAddr(ipAddr),
+		authapi.PreauthIpAddr(request.ipAddr),
 	)
 
+	if err != nil || preauth == nil {
+		return logical.ErrorResponse("Could not call Duo preauth"), nil
+	}
+
 	if preauth.StatResult.Stat != "OK" {
 		errorMsg := "Could not look up Duo user information"
 		if preauth.StatResult.Message != nil {
@@ -71,7 +85,7 @@ func duoHandler(
 
 	switch preauth.Response.Result {
 	case "allow":
-		return successResp, err
+		return request.successResp, err
 	case "deny":
 		return logical.ErrorResponse(preauth.Response.Status_Msg), nil
 	case "enroll":
@@ -80,22 +94,25 @@ func duoHandler(
 			preauth.Response.Enroll_Portal_Url)), nil
 	case "auth":
 		break
+	default:
+		return logical.ErrorResponse(fmt.Sprintf("Invalid Duo preauth response: %s",
+			preauth.Response.Result)), nil
 	}
 
 	options := []func(*url.Values){authapi.AuthUsername(duoUser)}
-	if method == "" {
-		method = "auto"
+	if request.method == "" {
+		request.method = "auto"
 	}
-	if passcode != "" {
-		method = "passcode"
-		options = append(options, authapi.AuthPasscode(passcode))
+	if request.passcode != "" {
+		request.method = "passcode"
+		options = append(options, authapi.AuthPasscode(request.passcode))
 	} else {
 		options = append(options, authapi.AuthDevice("auto"))
 	}
 
-	result, err := duoAuthClient.Auth(method, options...)
+	result, err := duoAuthClient.Auth(request.method, options...)
 
-	if err != nil {
+	if err != nil || result == nil {
 		return logical.ErrorResponse("Could not call Duo auth"), nil
 	}
 
@@ -114,5 +131,5 @@ func duoHandler(
 		return logical.ErrorResponse(result.Response.Status_Msg), nil
 	}
 
-	return successResp, err
+	return request.successResp, nil
 }

helper/mfa/duo/duo_test.go

@@ -37,29 +37,31 @@ func MockGetDuoAuthClient(data *MockClientData) func (*logical.Request, *DuoConf
 
 func getDuoAuthClient(data *MockClientData) AuthClient {
 	var c MockAuthClient
-	// set default response to auth user 
+	// set default response to be successful  
+	preauthSuccessJSON := `
+	{
+	  "Stat": "OK",
+	  "Response": {
+	    "Result": "auth",
+	    "Status_Msg": "Needs authentication",
+	    "Devices": []
+	  }
+	}`
 	if data.PreauthData == nil {
 		data.PreauthData = &authapi.PreauthResult{}
-		json.Unmarshal([]byte(`
-{
-  "Stat": "OK",
-  "Response": {
-    "Result": "auth",
-    "Status_Msg": "Needs authentication",
-    "Devices": []
-  }
-}`), data.PreauthData)
+		json.Unmarshal([]byte(preauthSuccessJSON), data.PreauthData)
 	}
 
+	authSuccessJSON := `
+	{
+	  "Stat": "OK",
+	  "Response": {
+	    "Result": "allow"
+	  }
+	}`
 	if data.AuthData == nil {
 		data.AuthData = &authapi.AuthResult{}
-		json.Unmarshal([]byte(`
-{
-  "Stat": "OK",
-  "Response": {
-    "Result": "allow"
-  }
-}`), data.AuthData)
+		json.Unmarshal([]byte(authSuccessJSON), data.AuthData)
 	}
 
 	c.MockData = data
@@ -74,7 +76,10 @@ func TestDuoHandlerSuccess(t *testing.T) {
 		UsernameFormat: "%s",
 	}
 	duoAuthClient := getDuoAuthClient(&MockClientData{})
-	resp, err := duoHandler(duoConfig, duoAuthClient, successResp, "user", "", "", "")
+	resp, err := duoHandler(duoConfig, duoAuthClient, &duoAuthRequest {
+		successResp: successResp,
+		username: "",
+	})
 	if err != nil {
 		t.Fatalf(err.Error())
 	}
@@ -85,14 +90,15 @@ func TestDuoHandlerSuccess(t *testing.T) {
 
 func TestDuoHandlerReject(t *testing.T) {
 	AuthData := &authapi.AuthResult{}
-		json.Unmarshal([]byte(`
-{
-  "Stat": "OK",
-  "Response": {
-    "Result": "deny",
-    "Status_Msg": "Invalid auth"
-  }
-}`), AuthData)
+	authRejectJSON := `
+	{
+	  "Stat": "OK",
+	  "Response": {
+	    "Result": "deny",
+	    "Status_Msg": "Invalid auth"
+	  }
+	}`
+	json.Unmarshal([]byte(authRejectJSON), AuthData)
 	successResp := &logical.Response{
 		Auth: &logical.Auth{},
 	}
@@ -103,7 +109,10 @@ func TestDuoHandlerReject(t *testing.T) {
 	duoAuthClient := getDuoAuthClient(&MockClientData{
 		AuthData: AuthData,
 	})
-	resp, err := duoHandler(duoConfig, duoAuthClient, successResp, "user", "", "", "")
+	resp, err := duoHandler(duoConfig, duoAuthClient, &duoAuthRequest {
+		successResp: successResp,
+		username: "user",
+	})
 	if err != nil {
 		t.Fatalf(err.Error())
 	}

helper/mfa/duo/path_duo_config.go

@@ -1,7 +1,7 @@
 package duo
 
 import (
-	"fmt"
+	"errors"
 	"strings"
 
 	"github.com/hashicorp/vault/logical"
@@ -51,7 +51,7 @@ func pathDuoConfigWrite(
 	req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
 	username_format := d.Get("username_format").(string)
 	if !strings.Contains(username_format, "%s") {
-		return nil, fmt.Errorf("username_format must include username ('%s')")
+		return nil, errors.New("username_format must include username ('%s')")
 	}
 	entry, err := logical.StorageEntryJSON("duo/config", DuoConfig{
 		UsernameFormat: username_format,