agent: allow AppRole Auto-Auth when bind_secret_id = false (#6324)

* agent: allow AppRole Auto-Auth when bind_secret_id = false

command/agent/approle_end_to_end_test.go

@@ -3,6 +3,7 @@ package agent
 import (
 	"context"
 	"encoding/json"
+	"fmt"
 	"io/ioutil"
 	"os"
 	"testing"
@@ -22,16 +23,43 @@ import (
 )
 
 func TestAppRoleEndToEnd(t *testing.T) {
-	t.Run("preserve_secret_id_file", func(t *testing.T) {
-		testAppRoleEndToEnd(t, false)
-	})
 
-	t.Run("remove_secret_id_file", func(t *testing.T) {
+	testCases := []struct {
-		testAppRoleEndToEnd(t, true)
+		removeSecretIDFile bool
-	})
+		bindSecretID       bool
+		secretIDLess       bool
+		expectToken        bool
+	}{
+		//default behaviour => token expected
+		{false, true, false, true},
+		{true, true, false, true},
+
+		//bindSecretID=false, wrong secret provided => token expected
+		//(vault ignores the supplied secret_id if bind_secret_id=false)
+		{false, false, false, true},
+		{true, false, false, true},
+
+		//bindSecretID=false, secret not provided => token expected
+		{false, false, true, true},
+		{true, false, true, true},
+
+		//bindSecretID=true, secret not provided => token not expected
+		{false, true, true, false},
+		{true, true, true, false},
+	}
+
+	for _, tc := range testCases {
+		secretFileAction := "preserve"
+		if tc.removeSecretIDFile {
+			secretFileAction = "remove"
+		}
+		t.Run(fmt.Sprintf("%s_secret_id_file bindSecretID=%v secretIDLess=%v expectToken=%v", secretFileAction, tc.bindSecretID, tc.secretIDLess, tc.expectToken), func(t *testing.T) {
+			testAppRoleEndToEnd(t, tc.removeSecretIDFile, tc.bindSecretID, tc.secretIDLess, tc.expectToken)
+		})
+	}
 }
 
-func testAppRoleEndToEnd(t *testing.T, removeSecretIDFile bool) {
+func testAppRoleEndToEnd(t *testing.T, removeSecretIDFile bool, bindSecretID bool, secretIDLess bool, expectToken bool) {
 	var err error
 	logger := logging.NewVaultLogger(log.Trace)
 	coreConfig := &vault.CoreConfig{
@@ -63,42 +91,53 @@ func testAppRoleEndToEnd(t *testing.T, removeSecretIDFile bool) {
 		t.Fatal(err)
 	}
 
-	_, err = client.Logical().Write("auth/approle/role/test1", map[string]interface{}{
+	_, err = client.Logical().Write("auth/approle/role/test1", addConstraints(!bindSecretID, map[string]interface{}{
-		"bind_secret_id": "true",
+		"bind_secret_id": bindSecretID,
 		"token_ttl":      "3s",
 		"token_max_ttl":  "10s",
-	})
+	}))
+
+	logger.Trace("vault configured with", "bind_secret_id", bindSecretID)
+
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	resp, err := client.Logical().Write("auth/approle/role/test1/secret-id", nil)
+	secret := ""
-	if err != nil {
+	secretID1 := ""
-		t.Fatal(err)
+	secretID2 := ""
+	if bindSecretID {
+		resp, err := client.Logical().Write("auth/approle/role/test1/secret-id", nil)
+		if err != nil {
+			t.Fatal(err)
+		}
+		secretID1 = resp.Data["secret_id"].(string)
+	} else {
+		logger.Trace("skipped write to auth/approle/role/test1/secret-id")
 	}
-	secretID1 := resp.Data["secret_id"].(string)
+	resp, err := client.Logical().Read("auth/approle/role/test1/role-id")
-
-	resp, err = client.Logical().Read("auth/approle/role/test1/role-id")
 	if err != nil {
 		t.Fatal(err)
 	}
 	roleID1 := resp.Data["role_id"].(string)
 
-	_, err = client.Logical().Write("auth/approle/role/test2", map[string]interface{}{
+	_, err = client.Logical().Write("auth/approle/role/test2", addConstraints(!bindSecretID, map[string]interface{}{
-		"bind_secret_id": "true",
+		"bind_secret_id": bindSecretID,
 		"token_ttl":      "3s",
 		"token_max_ttl":  "10s",
-	})
+	}))
 	if err != nil {
 		t.Fatal(err)
 	}
-
+	if bindSecretID {
-	resp, err = client.Logical().Write("auth/approle/role/test2/secret-id", nil)
+		resp, err = client.Logical().Write("auth/approle/role/test2/secret-id", nil)
-	if err != nil {
+		if err != nil {
-		t.Fatal(err)
+			t.Fatal(err)
+		}
+		secretID2 = resp.Data["secret_id"].(string)
+	} else {
+		logger.Trace("skipped write to auth/approle/role/test2/secret-id")
 	}
-	secretID2 := resp.Data["secret_id"].(string)
-
 	resp, err = client.Logical().Read("auth/approle/role/test2/role-id")
 	if err != nil {
 		t.Fatal(err)
@@ -113,16 +152,18 @@ func testAppRoleEndToEnd(t *testing.T, removeSecretIDFile bool) {
 	rolef.Close() // WriteFile doesn't need it open
 	defer os.Remove(role)
 	t.Logf("input role_id_file_path: %s", role)
-
+	if bindSecretID {
-	secretf, err := ioutil.TempFile("", "auth.secret-id.test.")
+		secretf, err := ioutil.TempFile("", "auth.secret-id.test.")
-	if err != nil {
+		if err != nil {
-		t.Fatal(err)
+			t.Fatal(err)
+		}
+		secret = secretf.Name()
+		secretf.Close()
+		defer os.Remove(secret)
+		t.Logf("input secret_id_file_path: %s", secret)
+	} else {
+		logger.Trace("skipped writing tempfile auth.secret-id.test.")
 	}
-	secret := secretf.Name()
-	secretf.Close()
-	defer os.Remove(secret)
-	t.Logf("input secret_id_file_path: %s", secret)
-
 	// We close these right away because we're just basically testing
 	// permissions and finding a usable file name
 	ouf, err := ioutil.TempFile("", "auth.tokensink.test.")
@@ -140,14 +181,34 @@ func testAppRoleEndToEnd(t *testing.T, removeSecretIDFile bool) {
 	})
 	defer timer.Stop()
 
+	secretFromAgent := secret
+	if secretIDLess {
+		secretFromAgent = ""
+	}
+	if !bindSecretID && !secretIDLess {
+		logger.Trace("agent is providing an invalid secret that should be ignored")
+		secretf, err := ioutil.TempFile("", "auth.secret-id.test.")
+		if err != nil {
+			t.Fatal(err)
+		}
+		secretFromAgent = secretf.Name()
+		secretf.Close()
+		defer os.Remove(secretFromAgent)
+		//if the token is empty, auth.approle would fail reporting the error
+		if err := ioutil.WriteFile(secretFromAgent, []byte("wrong-secret"), 0600); err != nil {
+			t.Fatal(err)
+		} else {
+			logger.Trace("wrote secret_id_file_path with wrong-secret", "path", secretFromAgent)
+		}
+	}
 	conf := map[string]interface{}{
 		"role_id_file_path":   role,
-		"secret_id_file_path": secret,
+		"secret_id_file_path": secretFromAgent,
 	}
+	logger.Trace("agent configured with", "conf", conf)
 	if !removeSecretIDFile {
 		conf["remove_secret_id_file_after_reading"] = removeSecretIDFile
 	}
-
 	am, err := agentapprole.NewApproleAuthMethod(&auth.AuthConfig{
 		Logger:    logger.Named("auth.approle"),
 		MountPath: "auth/approle",
@@ -205,17 +266,24 @@ func testAppRoleEndToEnd(t *testing.T, removeSecretIDFile bool) {
 		logger.Trace("wrote test role 1", "path", role)
 	}
 
-	if err := ioutil.WriteFile(secret, []byte(secretID1), 0600); err != nil {
+	if bindSecretID {
-		t.Fatal(err)
+		if err := ioutil.WriteFile(secret, []byte(secretID1), 0600); err != nil {
+			t.Fatal(err)
+		} else {
+			logger.Trace("wrote test secret 1", "path", secret)
+		}
 	} else {
-		logger.Trace("wrote test secret 1", "path", secret)
+		logger.Trace("skipped writing test secret 1")
 	}
 
 	checkToken := func() string {
 		timeout := time.Now().Add(10 * time.Second)
 		for {
 			if time.Now().After(timeout) {
-				t.Fatal("did not find a written token after timeout")
+				if expectToken {
+					t.Fatal("did not find a written token after timeout")
+				}
+				return ""
 			}
 			val, err := ioutil.ReadFile(out)
 			if err == nil {
@@ -223,15 +291,15 @@ func testAppRoleEndToEnd(t *testing.T, removeSecretIDFile bool) {
 				if len(val) == 0 {
 					t.Fatal("written token was empty")
 				}
-
+				if !secretIDLess {
-				_, err = os.Stat(secret)
+					_, err = os.Stat(secretFromAgent)
-				switch {
+					switch {
-				case removeSecretIDFile && err == nil:
+					case removeSecretIDFile && err == nil:
-					t.Fatal("secret file exists but was supposed to be removed")
+						t.Fatal("secret file exists but was supposed to be removed")
-				case !removeSecretIDFile && err != nil:
+					case !removeSecretIDFile && err != nil:
-					t.Fatal("secret ID file does not exist but was not supposed to be removed")
+						t.Fatal("secret ID file does not exist but was not supposed to be removed")
+					}
 				}
-
 				client.SetToken(string(val))
 				secret, err := client.Auth().Token().LookupSelf()
 				if err != nil {
@@ -243,6 +311,13 @@ func testAppRoleEndToEnd(t *testing.T, removeSecretIDFile bool) {
 		}
 	}
 	origEntity := checkToken()
+	if !expectToken && origEntity != "" {
+		t.Fatal("did not expect a token to be written: " + origEntity)
+	}
+	if !expectToken && origEntity == "" {
+		logger.Trace("skipping entities comparison as we are not expecting tokens to be written")
+		return
+	}
 
 	// Make sure it gets renewed
 	timeout := time.Now().Add(4 * time.Second)
@@ -270,10 +345,14 @@ func testAppRoleEndToEnd(t *testing.T, removeSecretIDFile bool) {
 		logger.Trace("wrote test role 2", "path", role)
 	}
 
-	if err := ioutil.WriteFile(secret, []byte(secretID2), 0600); err != nil {
+	if bindSecretID {
-		t.Fatal(err)
+		if err := ioutil.WriteFile(secret, []byte(secretID2), 0600); err != nil {
+			t.Fatal(err)
+		} else {
+			logger.Trace("wrote test secret 2", "path", secret)
+		}
 	} else {
-		logger.Trace("wrote test secret 2", "path", secret)
+		logger.Trace("skipped writing test secret 2")
 	}
 
 	newEntity := checkToken()
@@ -301,6 +380,29 @@ func testAppRoleEndToEnd(t *testing.T, removeSecretIDFile bool) {
 }
 
 func TestAppRoleWithWrapping(t *testing.T) {
+	testCases := []struct {
+		bindSecretID bool
+		secretIDLess bool
+		expectToken  bool
+	}{
+		//default behaviour => token expected
+		{true, false, true},
+
+		//bindSecretID=false, wrong secret provided, wrapping_path provided => token not expected
+		//(wrapping token is not valid or does not exist)
+		{false, false, false},
+
+		//bindSecretID=false, no secret provided, wrapping_path provided but ignored => token expected
+		{false, true, true},
+	}
+	for _, tc := range testCases {
+		t.Run(fmt.Sprintf("bindSecretID=%v secretIDLess=%v expectToken=%v", tc.bindSecretID, tc.secretIDLess, tc.expectToken), func(t *testing.T) {
+			testAppRoleWithWrapping(t, tc.bindSecretID, tc.secretIDLess, tc.expectToken)
+		})
+	}
+}
+
+func testAppRoleWithWrapping(t *testing.T, bindSecretID bool, secretIDLess bool, expectToken bool) {
 	var err error
 	logger := logging.NewVaultLogger(log.Trace)
 	coreConfig := &vault.CoreConfig{
@@ -333,11 +435,11 @@ func TestAppRoleWithWrapping(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	_, err = client.Logical().Write("auth/approle/role/test1", map[string]interface{}{
+	_, err = client.Logical().Write("auth/approle/role/test1", addConstraints(!bindSecretID, map[string]interface{}{
-		"bind_secret_id": "true",
+		"bind_secret_id": bindSecretID,
 		"token_ttl":      "3s",
 		"token_max_ttl":  "10s",
-	})
+	}))
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -349,13 +451,18 @@ func TestAppRoleWithWrapping(t *testing.T) {
 		return ""
 	})
 
-	resp, err := client.Logical().Write("auth/approle/role/test1/secret-id", nil)
+	secret := ""
-	if err != nil {
+	secretID1 := ""
-		t.Fatal(err)
+	if bindSecretID {
+		resp, err := client.Logical().Write("auth/approle/role/test1/secret-id", nil)
+		if err != nil {
+			t.Fatal(err)
+		}
+		secretID1 = resp.WrapInfo.Token
+	} else {
+		logger.Trace("skipped write to auth/approle/role/test1/secret-id")
 	}
-	secretID1 := resp.WrapInfo.Token
+	resp, err := client.Logical().Read("auth/approle/role/test1/role-id")
-
-	resp, err = client.Logical().Read("auth/approle/role/test1/role-id")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -370,14 +477,18 @@ func TestAppRoleWithWrapping(t *testing.T) {
 	defer os.Remove(role)
 	t.Logf("input role_id_file_path: %s", role)
 
-	secretf, err := ioutil.TempFile("", "auth.secret-id.test.")
+	if bindSecretID {
-	if err != nil {
+		secretf, err := ioutil.TempFile("", "auth.secret-id.test.")
-		t.Fatal(err)
+		if err != nil {
+			t.Fatal(err)
+		}
+		secret = secretf.Name()
+		secretf.Close()
+		defer os.Remove(secret)
+		t.Logf("input secret_id_file_path: %s", secret)
+	} else {
+		logger.Trace("skipped writing tempfile auth.secret-id.test.")
 	}
-	secret := secretf.Name()
-	secretf.Close()
-	defer os.Remove(secret)
-	t.Logf("input secret_id_file_path: %s", secret)
 
 	// We close these right away because we're just basically testing
 	// permissions and finding a usable file name
@@ -396,12 +507,33 @@ func TestAppRoleWithWrapping(t *testing.T) {
 	})
 	defer timer.Stop()
 
+	secretFromAgent := secret
+	if secretIDLess {
+		secretFromAgent = ""
+	}
+	if !bindSecretID && !secretIDLess {
+		logger.Trace("agent is providing an invalid secret that should be ignored")
+		secretf, err := ioutil.TempFile("", "auth.secret-id.test.")
+		if err != nil {
+			t.Fatal(err)
+		}
+		secretFromAgent = secretf.Name()
+		secretf.Close()
+		defer os.Remove(secretFromAgent)
+		//if the token is empty, auth.approle would fail reporting the error
+		if err := ioutil.WriteFile(secretFromAgent, []byte("wrong-secret"), 0600); err != nil {
+			t.Fatal(err)
+		} else {
+			logger.Trace("wrote secret_id_file_path with wrong-secret", "path", secretFromAgent)
+		}
+	}
 	conf := map[string]interface{}{
 		"role_id_file_path":                   role,
-		"secret_id_file_path":                 secret,
+		"secret_id_file_path":                 secretFromAgent,
 		"secret_id_response_wrapping_path":    "auth/approle/role/test1/secret-id",
 		"remove_secret_id_file_after_reading": true,
 	}
+	logger.Trace("agent configured with", "conf", conf)
 
 	am, err := agentapprole.NewApproleAuthMethod(&auth.AuthConfig{
 		Logger:    logger.Named("auth.approle"),
@@ -460,17 +592,26 @@ func TestAppRoleWithWrapping(t *testing.T) {
 		logger.Trace("wrote test role 1", "path", role)
 	}
 
-	if err := ioutil.WriteFile(secret, []byte(secretID1), 0600); err != nil {
+	if bindSecretID {
-		t.Fatal(err)
+		logger.Trace("WRITING TO auth.secret-id.test.", "secret", secret, "secretID1", secretID1)
+
+		if err := ioutil.WriteFile(secret, []byte(secretID1), 0600); err != nil {
+			t.Fatal(err)
+		} else {
+			logger.Trace("wrote test secret 1", "path", secret)
+		}
 	} else {
-		logger.Trace("wrote test secret 1", "path", secret)
+		logger.Trace("skipped writing test secret 1")
 	}
 
 	checkToken := func() string {
 		timeout := time.Now().Add(10 * time.Second)
 		for {
 			if time.Now().After(timeout) {
-				t.Fatal("did not find a written token after timeout")
+				if expectToken {
+					t.Fatal("did not find a written token after timeout")
+				}
+				return ""
 			}
 			val, err := ioutil.ReadFile(out)
 			if err == nil {
@@ -478,9 +619,10 @@ func TestAppRoleWithWrapping(t *testing.T) {
 				if len(val) == 0 {
 					t.Fatal("written token was empty")
 				}
-
+				if !secretIDLess {
-				if _, err := os.Stat(secret); err == nil {
+					if _, err := os.Stat(secret); err == nil {
-					t.Fatal("secret ID file does not exist but was not supposed to be removed")
+						t.Fatal("secret ID file does not exist but was not supposed to be removed")
+					}
 				}
 
 				client.SetToken(string(val))
@@ -494,6 +636,15 @@ func TestAppRoleWithWrapping(t *testing.T) {
 		}
 	}
 	origEntity := checkToken()
+	logger.Trace("cheking token", "origEntity", origEntity)
+
+	if !expectToken && origEntity != "" {
+		t.Fatal("did not expect a token to be written: " + origEntity)
+	}
+	if !expectToken && origEntity == "" {
+		logger.Trace("skipping entities comparison as we are not expecting tokens to be written")
+		return
+	}
 
 	// Make sure it gets renewed
 	timeout := time.Now().Add(4 * time.Second)
@@ -516,15 +667,21 @@ func TestAppRoleWithWrapping(t *testing.T) {
 
 	// Write new values
 	client.SetToken(origToken)
-	resp, err = client.Logical().Write("auth/approle/role/test1/secret-id", nil)
+	logger.Trace("origToken set into client", "origToken", origToken)
-	if err != nil {
+
-		t.Fatal(err)
+	if bindSecretID {
-	}
+		resp, err = client.Logical().Write("auth/approle/role/test1/secret-id", nil)
-	secretID2 := resp.WrapInfo.Token
+		if err != nil {
-	if err := ioutil.WriteFile(secret, []byte(secretID2), 0600); err != nil {
+			t.Fatal(err)
-		t.Fatal(err)
+		}
+		secretID2 := resp.WrapInfo.Token
+		if err := ioutil.WriteFile(secret, []byte(secretID2), 0600); err != nil {
+			t.Fatal(err)
+		} else {
+			logger.Trace("wrote test secret 2", "path", secret)
+		}
 	} else {
-		logger.Trace("wrote test secret 2", "path", secret)
+		logger.Trace("skipped writing test secret 2")
 	}
 
 	newEntity := checkToken()
@@ -550,3 +707,17 @@ func TestAppRoleWithWrapping(t *testing.T) {
 		}
 	}
 }
+
+func addConstraints(add bool, cfg map[string]interface{}) map[string]interface{} {
+	if add {
+		//extraConstraints to add when bind_secret_id=false (otherwise Vault would fail with: "at least one constraint should be enabled on the role")
+		extraConstraints := map[string]interface{}{
+			"secret_id_bound_cidrs": "127.0.0.1/32",
+			"token_bound_cidrs":     "127.0.0.1/32",
+		}
+		for k, v := range extraConstraints {
+			cfg[k] = v
+		}
+	}
+	return cfg
+}

command/agent/auth/approle/approle.go

@@ -54,34 +54,33 @@ func NewApproleAuthMethod(conf *auth.AuthConfig) (auth.AuthMethod, error) {
 	}
 
 	secretIDFilePathRaw, ok := conf.Config["secret_id_file_path"]
-	if !ok {
-		return nil, errors.New("missing 'secret_id_file_path' value")
-	}
-	a.secretIDFilePath, ok = secretIDFilePathRaw.(string)
-	if !ok {
-		return nil, errors.New("could not convert 'secret_id_file_path' config value to string")
-	}
-	if a.secretIDFilePath == "" {
-		return nil, errors.New("'secret_id_file_path' value is empty")
-	}
-
-	removeSecretIDFileAfterReadingRaw, ok := conf.Config["remove_secret_id_file_after_reading"]
 	if ok {
-		removeSecretIDFileAfterReading, err := parseutil.ParseBool(removeSecretIDFileAfterReadingRaw)
+		a.secretIDFilePath, ok = secretIDFilePathRaw.(string)
-		if err != nil {
-			return nil, errwrap.Wrapf("error parsing 'remove_secret_id_file_after_reading' value: {{err}}", err)
-		}
-		a.removeSecretIDFileAfterReading = removeSecretIDFileAfterReading
-	}
-
-	secretIDResponseWrappingPathRaw, ok := conf.Config["secret_id_response_wrapping_path"]
-	if ok {
-		a.secretIDResponseWrappingPath, ok = secretIDResponseWrappingPathRaw.(string)
 		if !ok {
-			return nil, errors.New("could not convert 'secret_id_response_wrapping_path' config value to string")
+			return nil, errors.New("could not convert 'secret_id_file_path' config value to string")
 		}
-		if a.secretIDResponseWrappingPath == "" {
+		if a.secretIDFilePath == "" {
-			return nil, errors.New("'secret_id_response_wrapping_path' value is empty")
+			return a, nil
+		}
+
+		removeSecretIDFileAfterReadingRaw, ok := conf.Config["remove_secret_id_file_after_reading"]
+		if ok {
+			removeSecretIDFileAfterReading, err := parseutil.ParseBool(removeSecretIDFileAfterReadingRaw)
+			if err != nil {
+				return nil, errwrap.Wrapf("error parsing 'remove_secret_id_file_after_reading' value: {{err}}", err)
+			}
+			a.removeSecretIDFileAfterReading = removeSecretIDFileAfterReading
+		}
+
+		secretIDResponseWrappingPathRaw, ok := conf.Config["secret_id_response_wrapping_path"]
+		if ok {
+			a.secretIDResponseWrappingPath, ok = secretIDResponseWrappingPathRaw.(string)
+			if !ok {
+				return nil, errors.New("could not convert 'secret_id_response_wrapping_path' config value to string")
+			}
+			if a.secretIDResponseWrappingPath == "" {
+				return nil, errors.New("'secret_id_response_wrapping_path' value is empty")
+			}
 		}
 	}
 
@@ -106,6 +105,17 @@ func (a *approleMethod) Authenticate(ctx context.Context, client *api.Client) (s
 			a.cachedRoleID = strings.TrimSpace(string(roleID))
 		}
 	}
+
+	if a.cachedRoleID == "" {
+		return "", nil, errors.New("no known role ID")
+	}
+
+	if a.secretIDFilePath == "" {
+		return fmt.Sprintf("%s/login", a.mountPath), map[string]interface{}{
+			"role_id": a.cachedRoleID,
+		}, nil
+	}
+
 	if _, err := os.Stat(a.secretIDFilePath); err == nil {
 		secretID, err := ioutil.ReadFile(a.secretIDFilePath)
 		if err != nil {
@@ -180,9 +190,6 @@ func (a *approleMethod) Authenticate(ctx context.Context, client *api.Client) (s
 		}
 	}
 
-	if a.cachedRoleID == "" {
-		return "", nil, errors.New("no known role ID")
-	}
 	if a.cachedSecretID == "" {
 		return "", nil, errors.New("no known secret ID")
 	}

website/source/docs/agent/autoauth/methods/approle.html.md

@@ -23,8 +23,10 @@ cached.
 
 * `role_id_file_path` `(string: required)` - The path to the file with role ID
 
-* `secret_id_file_path` `(string: required)` - The path to the file with secret
+* `secret_id_file_path` `(string: optional)` - The path to the file with secret
-  ID
+  ID.
+  If not set, only the `role-id` will be used. \
+  In that case, the AppRole should have `bind_secret_id` set to `false` otherwise Vault Agent wouldn't be able to login.
 
 * `remove_secret_id_file_after_reading` `(bool: optional, defaults to true)` -
   This can be set to `false` to disable the default behavior of removing the