diff --git a/command/agent/approle_end_to_end_test.go b/command/agent/approle_end_to_end_test.go index bd9f9cc0a0..692a4de767 100644 --- a/command/agent/approle_end_to_end_test.go +++ b/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) { - testAppRoleEndToEnd(t, true) - }) + testCases := []struct { + 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{}{ - "bind_secret_id": "true", + _, err = client.Logical().Write("auth/approle/role/test1", addConstraints(!bindSecretID, map[string]interface{}{ + "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) - if err != nil { - t.Fatal(err) + secret := "" + secretID1 := "" + 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{}{ - "bind_secret_id": "true", + _, err = client.Logical().Write("auth/approle/role/test2", addConstraints(!bindSecretID, map[string]interface{}{ + "bind_secret_id": bindSecretID, "token_ttl": "3s", "token_max_ttl": "10s", - }) + })) if err != nil { t.Fatal(err) } - - resp, err = client.Logical().Write("auth/approle/role/test2/secret-id", nil) - if err != nil { - t.Fatal(err) + if bindSecretID { + resp, err = client.Logical().Write("auth/approle/role/test2/secret-id", nil) + if err != nil { + 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) - - secretf, err := ioutil.TempFile("", "auth.secret-id.test.") - if err != nil { - t.Fatal(err) + if bindSecretID { + secretf, err := ioutil.TempFile("", "auth.secret-id.test.") + 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 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 { - t.Fatal(err) + if bindSecretID { + 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") } - - _, err = os.Stat(secret) - switch { - case removeSecretIDFile && err == nil: - t.Fatal("secret file exists but was supposed to be removed") - case !removeSecretIDFile && err != nil: - t.Fatal("secret ID file does not exist but was not supposed to be removed") + if !secretIDLess { + _, err = os.Stat(secretFromAgent) + switch { + case removeSecretIDFile && err == nil: + t.Fatal("secret file exists but was supposed to be removed") + case !removeSecretIDFile && err != nil: + 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 { - t.Fatal(err) + if bindSecretID { + 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{}{ - "bind_secret_id": "true", + _, err = client.Logical().Write("auth/approle/role/test1", addConstraints(!bindSecretID, map[string]interface{}{ + "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) - if err != nil { - t.Fatal(err) + secret := "" + secretID1 := "" + 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 err != nil { - t.Fatal(err) + if bindSecretID { + secretf, err := ioutil.TempFile("", "auth.secret-id.test.") + 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 { - t.Fatal(err) + if bindSecretID { + 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 _, err := os.Stat(secret); err == nil { - t.Fatal("secret ID file does not exist but was not supposed to be removed") + if !secretIDLess { + if _, err := os.Stat(secret); err == nil { + 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) - if err != nil { - t.Fatal(err) - } - secretID2 := resp.WrapInfo.Token - if err := ioutil.WriteFile(secret, []byte(secretID2), 0600); err != nil { - t.Fatal(err) + logger.Trace("origToken set into client", "origToken", origToken) + + if bindSecretID { + resp, err = client.Logical().Write("auth/approle/role/test1/secret-id", nil) + if err != nil { + 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 +} diff --git a/command/agent/auth/approle/approle.go b/command/agent/auth/approle/approle.go index 0413835acf..a3477136aa 100644 --- a/command/agent/auth/approle/approle.go +++ b/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) - 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) + a.secretIDFilePath, ok = secretIDFilePathRaw.(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 == "" { - return nil, errors.New("'secret_id_response_wrapping_path' value is empty") + if a.secretIDFilePath == "" { + 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") } diff --git a/website/source/docs/agent/autoauth/methods/approle.html.md b/website/source/docs/agent/autoauth/methods/approle.html.md index d50ca38834..3cfeb6a7c9 100644 --- a/website/source/docs/agent/autoauth/methods/approle.html.md +++ b/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 - ID +* `secret_id_file_path` `(string: optional)` - The path to the file with secret + 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