From 32398d98e9d02e1f3934d6f366fb9ca2b2712529 Mon Sep 17 00:00:00 2001 From: Ben Ash <32777270+benashz@users.noreply.github.com> Date: Tue, 2 May 2023 12:18:13 -0400 Subject: [PATCH 1/6] Ensure that the auth_login honours the provider's namespace (#1830) Previously, when any auth_login was configured with provider.namespace, the login would not be done in the correct namespace. This was a big problem for HCP Vault users, since the default namespace for on an HCP Vault cluster is admin. * Only set the parent client's namespace once. --- go.mod | 1 + go.sum | 1 + internal/consts/consts.go | 2 +- internal/provider/meta.go | 104 +++++++++++++++++++++++--------------- vault/provider.go | 2 +- vault/provider_test.go | 25 +++++---- 6 files changed, 78 insertions(+), 57 deletions(-) diff --git a/go.mod b/go.mod index 2553a331b..a4f043aae 100644 --- a/go.mod +++ b/go.mod @@ -53,4 +53,5 @@ require ( google.golang.org/api v0.98.0 google.golang.org/genproto v0.0.0-20221010155953-15ba04fc1c0e google.golang.org/grpc v1.50.0 // indirect + k8s.io/utils v0.0.0-20220210201930-3a6ce19ff2f9 ) diff --git a/go.sum b/go.sum index abe561866..64dab3390 100644 --- a/go.sum +++ b/go.sum @@ -2862,6 +2862,7 @@ k8s.io/utils v0.0.0-20200324210504-a9aa75ae1b89/go.mod h1:sZAwmy6armz5eXlNoLmJcl k8s.io/utils v0.0.0-20201110183641-67b214c5f920/go.mod h1:jPW/WVKK9YHAvNhRxK0md/EJ228hCsBRufyofKtW8HA= k8s.io/utils v0.0.0-20210819203725-bdf08cb9a70a/go.mod h1:jPW/WVKK9YHAvNhRxK0md/EJ228hCsBRufyofKtW8HA= k8s.io/utils v0.0.0-20210930125809-cb0fa318a74b/go.mod h1:jPW/WVKK9YHAvNhRxK0md/EJ228hCsBRufyofKtW8HA= +k8s.io/utils v0.0.0-20220210201930-3a6ce19ff2f9 h1:HNSDgDCrr/6Ly3WEGKZftiE7IY19Vz2GdbOCyI4qqhc= k8s.io/utils v0.0.0-20220210201930-3a6ce19ff2f9/go.mod h1:jPW/WVKK9YHAvNhRxK0md/EJ228hCsBRufyofKtW8HA= layeh.com/radius v0.0.0-20190322222518-890bc1058917/go.mod h1:fywZKyu//X7iRzaxLgPWsvc0L26IUpVvE/aeIL2JtIQ= mvdan.cc/gofumpt v0.1.1/go.mod h1:yXG1r1WqZVKWbVRtBWKWX9+CxGYfA51nSomhM0woR48= diff --git a/internal/consts/consts.go b/internal/consts/consts.go index 311ae6ede..92de0575e 100644 --- a/internal/consts/consts.go +++ b/internal/consts/consts.go @@ -217,7 +217,7 @@ const ( FieldIPAddresses = "ip_addresses" FieldCIDRBlocks = "cidr_blocks" FieldProjectRoles = "project_roles" - + FieldSkipChildToken = "skip_child_token" /* common environment variables */ diff --git a/internal/provider/meta.go b/internal/provider/meta.go index 865966758..fdb7af774 100644 --- a/internal/provider/meta.go +++ b/internal/provider/meta.go @@ -20,6 +20,7 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" "github.com/hashicorp/vault/api" "github.com/hashicorp/vault/command/config" + "k8s.io/utils/pointer" "github.com/hashicorp/terraform-provider-vault/helper" "github.com/hashicorp/terraform-provider-vault/internal/consts" @@ -66,11 +67,11 @@ func (p *ProviderMeta) GetNSClient(ns string) (*api.Client, error) { return nil, err } + ns = strings.Trim(ns, "/") if ns == "" { return nil, fmt.Errorf("empty namespace not allowed") } - ns = strings.Trim(ns, "/") if root, ok := p.resourceData.GetOk(consts.FieldNamespace); ok && root.(string) != "" { ns = fmt.Sprintf("%s/%s", root, ns) } @@ -216,40 +217,73 @@ func NewProviderMeta(d *schema.ResourceData) (interface{}, error) { MaxHTTPRetriesCCC = d.Get("max_retries_ccc").(int) - // Try and get the token from the config or token helper - token, err := GetToken(d) - if err != nil { - return nil, err - } + // Set the namespace to the requested namespace, if provided + namespace := d.Get(consts.FieldNamespace).(string) authLogin, err := GetAuthLogin(d) if err != nil { return nil, err } + var token string if authLogin != nil { - client.SetNamespace(authLogin.Namespace()) - secret, err := authLogin.Login(client) + clone, err := client.Clone() + if err != nil { + return nil, err + } + + if authLogin.Namespace() != "" { + // the namespace configured on the auth_login takes precedence over the provider's + clone.SetNamespace(authLogin.Namespace()) + } else if namespace != "" { + // authenticate to the engine in the provider's namespace + clone.SetNamespace(namespace) + } + + secret, err := authLogin.Login(clone) if err != nil { return nil, err } token = secret.Auth.ClientToken + } else { + // try and get the token from the config or token helper + token, err = GetToken(d) + if err != nil { + return nil, err + } } if token != "" { client.SetToken(token) } if client.Token() == "" { - return nil, errors.New("no vault token found") + return nil, errors.New("no vault token set on Client") } - skipChildToken := d.Get("skip_child_token").(bool) - if !skipChildToken { - err := setChildToken(d, client) + if !d.Get(consts.FieldSkipChildToken).(bool) { + tokenInfo, err := client.Auth().Token().LookupSelf() + if err != nil { + return nil, fmt.Errorf("failed to lookup token, err=%w", err) + } + + var tokenNamespace string + if tokenNamespaceRaw, ok := tokenInfo.Data[consts.FieldNamespacePath]; ok { + tokenNamespace = strings.Trim(tokenNamespaceRaw.(string), "/") + } + + // a child token is always created in the namespace of the parent token. + token, err = createChildToken(d, client, tokenNamespace) if err != nil { return nil, err } + + client.SetToken(token) + } + + if namespace != "" { + // set the namespace on the parent + client.SetNamespace(namespace) } var vaultVersion *version.Version @@ -268,11 +302,6 @@ func NewProviderMeta(d *schema.ResourceData) (interface{}, error) { } vaultVersion = ver } - // Set the namespace to the requested namespace, if provided - namespace := d.Get(consts.FieldNamespace).(string) - if namespace != "" { - client.SetNamespace(namespace) - } return &ProviderMeta{ resourceData: d, @@ -388,47 +417,41 @@ func getVaultVersion(client *api.Client) (*version.Version, error) { return version.Must(version.NewSemver(resp.Version)), nil } -func setChildToken(d *schema.ResourceData, c *api.Client) error { +func createChildToken(d *schema.ResourceData, c *api.Client, namespace string) (string, error) { tokenName := d.Get("token_name").(string) if tokenName == "" { tokenName = "terraform" } + clone, err := c.Clone() + if err != nil { + return "", err + } + + if namespace != "" { + log.Printf("[INFO] Creating child token, namespace=%q", namespace) + clone.SetNamespace(namespace) + } // In order to enforce our relatively-short lease TTL, we derive a - // temporary child token that inherits all of the policies of the + // temporary child token that inherits all the policies of the // token we were given but expires after max_lease_ttl_seconds. // // The intent here is that Terraform will need to re-fetch any - // secrets on each run and so we limit the exposure risk of secrets + // secrets on each run, so we limit the exposure risk of secrets // that end up stored in the Terraform state, assuming that they are // credentials that Vault is able to revoke. // // Caution is still required with state files since not all secrets // can explicitly be revoked, and this limited scope won't apply to // any secrets that are *written* by Terraform to Vault. - - // Set the namespace to the token's namespace only for the - // child token creation - tokenInfo, err := c.Auth().Token().LookupSelf() - if err != nil { - return err - } - if tokenNamespaceRaw, ok := tokenInfo.Data["namespace_path"]; ok { - tokenNamespace := tokenNamespaceRaw.(string) - if tokenNamespace != "" { - c.SetNamespace(tokenNamespace) - } - } - - renewable := false - childTokenLease, err := c.Auth().Token().Create(&api.TokenCreateRequest{ + childTokenLease, err := clone.Auth().Token().Create(&api.TokenCreateRequest{ DisplayName: tokenName, TTL: fmt.Sprintf("%ds", d.Get("max_lease_ttl_seconds").(int)), ExplicitMaxTTL: fmt.Sprintf("%ds", d.Get("max_lease_ttl_seconds").(int)), - Renewable: &renewable, + Renewable: pointer.Bool(false), }) if err != nil { - return fmt.Errorf("failed to create limited child token: %s", err) + return "", fmt.Errorf("failed to create limited child token: %s", err) } childToken := childTokenLease.Auth.ClientToken @@ -436,10 +459,7 @@ func setChildToken(d *schema.ResourceData, c *api.Client) error { log.Printf("[INFO] Using Vault token with the following policies: %s", strings.Join(policies, ", ")) - // Set the token to the generated child token - c.SetToken(childToken) - - return nil + return childToken, nil } func GetToken(d *schema.ResourceData) (string, error) { diff --git a/vault/provider.go b/vault/provider.go index c92e61291..ef86c0cbf 100644 --- a/vault/provider.go +++ b/vault/provider.go @@ -84,7 +84,7 @@ func Provider() *schema.Provider { DefaultFunc: schema.EnvDefaultFunc("VAULT_TOKEN_NAME", ""), Description: "Token name to use for creating the Vault child token.", }, - "skip_child_token": { + consts.FieldSkipChildToken: { Type: schema.TypeBool, Optional: true, DefaultFunc: schema.EnvDefaultFunc("TERRAFORM_VAULT_SKIP_CHILD_TOKEN", false), diff --git a/vault/provider_test.go b/vault/provider_test.go index 2c6b6f528..5ff5ec9dd 100644 --- a/vault/provider_test.go +++ b/vault/provider_test.go @@ -157,6 +157,7 @@ func TestTokenReadProviderConfigureWithHeaders(t *testing.T) { func TestAccNamespaceProviderConfigure(t *testing.T) { testutil.SkipTestAccEnt(t) + testutil.SkipTestAcc(t) rootProvider := Provider() rootProviderResource := &schema.Resource{ @@ -168,21 +169,19 @@ func TestAccNamespaceProviderConfigure(t *testing.T) { } namespacePath := acctest.RandomWithPrefix("test-namespace") + client := testProvider.Meta().(*provider.ProviderMeta).GetClient() - // Create a test namespace and make sure it stays there - resource.Test(t, resource.TestCase{ - PreCheck: func() { testutil.TestAccPreCheck(t) }, - Providers: map[string]*schema.Provider{ - "vault": rootProvider, - }, - Steps: []resource.TestStep{ - { - Config: testNamespaceConfig(namespacePath), - Check: testNamespaceCheckAttrs(), - }, - }, + t.Cleanup(func() { + if _, err := client.Logical().Delete(SysNamespaceRoot + namespacePath); err != nil { + t.Errorf("failed to delete parent namespace %q, err=%s", namespacePath, err) + } }) + // create the namespace for the provider + if _, err := client.Logical().Write(SysNamespaceRoot+namespacePath, nil); err != nil { + t.Fatal(err) + } + nsProvider := Provider() nsProviderResource := &schema.Resource{ Schema: nsProvider.Schema, @@ -644,7 +643,7 @@ func TestAccChildToken(t *testing.T) { } } }, - Config: testProviderConfig(test.useChildTokenSchema, `skip_child_token = `+test.skipChildTokenSchema), + Config: testProviderConfig(test.useChildTokenSchema, consts.FieldSkipChildToken+` = `+test.skipChildTokenSchema), Check: checkTokenUsed(test.expectChildToken), }, }, From 22da6855ac1a01ee624f1203905d6f5d7e55cda1 Mon Sep 17 00:00:00 2001 From: Ben Ash Date: Thu, 4 May 2023 10:42:37 -0400 Subject: [PATCH 2/6] Add back support for deriving the provider namespace from the Vault token's --- internal/provider/meta.go | 41 +++++++++++++++++++++++++++++---------- 1 file changed, 31 insertions(+), 10 deletions(-) diff --git a/internal/provider/meta.go b/internal/provider/meta.go index fdb7af774..639f42c3f 100644 --- a/internal/provider/meta.go +++ b/internal/provider/meta.go @@ -227,6 +227,7 @@ func NewProviderMeta(d *schema.ResourceData) (interface{}, error) { var token string if authLogin != nil { + // the clone is only used to auth to Vault clone, err := client.Clone() if err != nil { return nil, err @@ -234,6 +235,7 @@ func NewProviderMeta(d *schema.ResourceData) (interface{}, error) { if authLogin.Namespace() != "" { // the namespace configured on the auth_login takes precedence over the provider's + // for authentication only. clone.SetNamespace(authLogin.Namespace()) } else if namespace != "" { // authenticate to the engine in the provider's namespace @@ -257,21 +259,22 @@ func NewProviderMeta(d *schema.ResourceData) (interface{}, error) { if token != "" { client.SetToken(token) } + if client.Token() == "" { return nil, errors.New("no vault token set on Client") } - if !d.Get(consts.FieldSkipChildToken).(bool) { - tokenInfo, err := client.Auth().Token().LookupSelf() - if err != nil { - return nil, fmt.Errorf("failed to lookup token, err=%w", err) - } + tokenInfo, err := client.Auth().Token().LookupSelf() + if err != nil { + return nil, fmt.Errorf("failed to lookup token, err=%w", err) + } - var tokenNamespace string - if tokenNamespaceRaw, ok := tokenInfo.Data[consts.FieldNamespacePath]; ok { - tokenNamespace = strings.Trim(tokenNamespaceRaw.(string), "/") - } + var tokenNamespace string + if v, ok := tokenInfo.Data[consts.FieldNamespacePath]; ok { + tokenNamespace = strings.Trim(v.(string), "/") + } + if !d.Get(consts.FieldSkipChildToken).(bool) { // a child token is always created in the namespace of the parent token. token, err = createChildToken(d, client, tokenNamespace) if err != nil { @@ -281,8 +284,25 @@ func NewProviderMeta(d *schema.ResourceData) (interface{}, error) { client.SetToken(token) } + if namespace == "" && tokenNamespace != "" { + // set the provider namespace to the token's namespace + // this is here to ensure that do not break any configurations that are relying on the + // token's namespace being used for resource provisioning. + // In the future we should drop support for this behaviour. + log.Printf("[WARN] The provider namespace should be set when using namespaced auth tokens. "+ + "Please update your provider configuration's namespace to be %q. "+ + "Future releases may not support this type of configuration.", tokenNamespace) + + namespace = tokenNamespace + // set the namespace on the provider to ensure that all child + // namespace paths are properly honoured. + if err := d.Set(consts.FieldNamespace, namespace); err != nil { + return nil, err + } + } + if namespace != "" { - // set the namespace on the parent + // set the namespace on the parent client client.SetNamespace(namespace) } @@ -423,6 +443,7 @@ func createChildToken(d *schema.ResourceData, c *api.Client, namespace string) ( tokenName = "terraform" } + // the clone is only used to auth to Vault clone, err := c.Clone() if err != nil { return "", err From ffb9b4c7b07e1222d7078365a9c03f4b54d4689a Mon Sep 17 00:00:00 2001 From: Ben Ash Date: Sun, 4 Jun 2023 14:13:29 -0400 Subject: [PATCH 3/6] Add namespace tests for NewProviderMeta() --- internal/consts/consts.go | 6 +- internal/provider/meta.go | 9 +- internal/provider/meta_test.go | 219 +++++++++++++++++++++++++++++++ vault/provider_test.go | 4 +- vault/resource_namespace.go | 12 +- vault/resource_namespace_test.go | 2 +- 6 files changed, 236 insertions(+), 16 deletions(-) diff --git a/internal/consts/consts.go b/internal/consts/consts.go index 910af0e49..05115da81 100644 --- a/internal/consts/consts.go +++ b/internal/consts/consts.go @@ -253,6 +253,7 @@ const ( FieldServiceAccountNames = "service_account_names" FieldDisableCheckInEnforcement = "disable_check_in_enforcement" FieldSkipChildToken = "skip_child_token" + FieldTokenPolicies = "token_policies" /* common environment variables @@ -338,6 +339,7 @@ const ( /* misc. path related constants */ - PathDelim = "/" - VaultAPIV1Root = "/v1" + PathDelim = "/" + VaultAPIV1Root = "/v1" + SysNamespaceRoot = "sys/namespaces/" ) diff --git a/internal/provider/meta.go b/internal/provider/meta.go index 639f42c3f..476bc8e74 100644 --- a/internal/provider/meta.go +++ b/internal/provider/meta.go @@ -142,6 +142,9 @@ func (p *ProviderMeta) validate() error { // NewProviderMeta sets up the Provider to service Vault requests. // It is meant to be used as a schema.ConfigureFunc. func NewProviderMeta(d *schema.ResourceData) (interface{}, error) { + if d == nil { + return nil, fmt.Errorf("nil ResourceData provided") + } clientConfig := api.DefaultConfig() addr := d.Get(consts.FieldAddress).(string) if addr != "" { @@ -287,10 +290,10 @@ func NewProviderMeta(d *schema.ResourceData) (interface{}, error) { if namespace == "" && tokenNamespace != "" { // set the provider namespace to the token's namespace // this is here to ensure that do not break any configurations that are relying on the - // token's namespace being used for resource provisioning. + // token's namespace being used during resource provisioning. // In the future we should drop support for this behaviour. - log.Printf("[WARN] The provider namespace should be set when using namespaced auth tokens. "+ - "Please update your provider configuration's namespace to be %q. "+ + log.Printf("[WARN] The provider namespace should be set whenever using namespaced auth tokens. "+ + "You may want to update your provider configuration's namespace to be %q, before executing terraform."+ "Future releases may not support this type of configuration.", tokenNamespace) namespace = tokenNamespace diff --git a/internal/provider/meta_test.go b/internal/provider/meta_test.go index 1828ac7d1..d91cd7f5d 100644 --- a/internal/provider/meta_test.go +++ b/internal/provider/meta_test.go @@ -10,14 +10,18 @@ import ( "reflect" "sync" "testing" + "time" + "github.com/cenkalti/backoff/v4" "github.com/hashicorp/go-version" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" "github.com/hashicorp/vault/api" vault_consts "github.com/hashicorp/vault/sdk/helper/consts" "github.com/hashicorp/terraform-provider-vault/internal/consts" + "github.com/hashicorp/terraform-provider-vault/testutil" ) func TestProviderMeta_GetNSClient(t *testing.T) { @@ -532,3 +536,218 @@ func TestIsEnterpriseSupported(t *testing.T) { }) } } + +func TestNewProviderMeta(t *testing.T) { + testutil.SkipTestAcc(t) + testutil.SkipTestAccEnt(t) + testutil.TestAccPreCheck(t) + + nsPrefix := acctest.RandomWithPrefix("ns") + + defaultUser := "alice" + defaultPassword := "f00bazB1ff" + + rootProvider := NewProvider(nil, nil) + pr := &schema.Resource{ + Schema: rootProvider.Schema, + } + + tests := []struct { + name string + d *schema.ResourceData + data map[string]interface{} + wantNamespace string + tokenNamespace string + authLoginNamespace string + wantErr bool + }{ + { + name: "invalid-nil-ResourceData", + d: nil, + wantErr: true, + }, + { + // expect provider namespace set. + name: "with-provider-ns-only", + d: pr.TestResourceData(), + data: map[string]interface{}{ + consts.FieldNamespace: nsPrefix + "prov", + consts.FieldSkipGetVaultVersion: true, + }, + wantNamespace: nsPrefix + "prov", + wantErr: false, + }, + { + // expect token namespace set + name: "with-token-ns-only", + d: pr.TestResourceData(), + data: map[string]interface{}{ + consts.FieldSkipGetVaultVersion: true, + consts.FieldSkipChildToken: true, + }, + tokenNamespace: nsPrefix + "token-ns-only", + wantNamespace: nsPrefix + "token-ns-only", + wantErr: false, + }, + { + // expect provider namespace set. + name: "with-provider-ns-and-token-ns", + d: pr.TestResourceData(), + data: map[string]interface{}{ + consts.FieldNamespace: nsPrefix + "prov-and-token", + consts.FieldSkipGetVaultVersion: true, + consts.FieldSkipChildToken: true, + }, + tokenNamespace: nsPrefix + "token-ns", + wantNamespace: nsPrefix + "prov-and-token", + wantErr: false, + }, + { + // expect auth_login namespace set. + name: "with-auth-login-and-ns", + d: pr.TestResourceData(), + data: map[string]interface{}{ + consts.FieldSkipGetVaultVersion: true, + consts.FieldSkipChildToken: true, + consts.FieldAuthLoginUserpass: []map[string]interface{}{ + { + consts.FieldNamespace: nsPrefix + "auth-ns", + consts.FieldMount: consts.MountTypeUserpass, + consts.FieldUsername: defaultUser, + consts.FieldPassword: defaultPassword, + }, + }, + }, + authLoginNamespace: nsPrefix + "auth-ns", + wantNamespace: nsPrefix + "auth-ns", + wantErr: false, + }, + { + // expect provider namespace set. + name: "with-provider-ns-and-auth-login-with-ns", + d: pr.TestResourceData(), + data: map[string]interface{}{ + consts.FieldNamespace: nsPrefix + "prov-ns-auth-ns", + consts.FieldSkipGetVaultVersion: true, + consts.FieldSkipChildToken: true, + consts.FieldAuthLoginUserpass: []map[string]interface{}{ + { + consts.FieldNamespace: nsPrefix + "auth-ns", + consts.FieldMount: consts.MountTypeUserpass, + consts.FieldUsername: defaultUser, + consts.FieldPassword: defaultPassword, + }, + }, + }, + authLoginNamespace: nsPrefix + "auth-ns", + wantNamespace: nsPrefix + "prov-ns-auth-ns", + wantErr: false, + }, + } + + createNamespace := func(t *testing.T, client *api.Client, ns string) { + t.Helper() + t.Cleanup(func() { + err := backoff.Retry(func() error { + _, err := client.Logical().Delete(consts.SysNamespaceRoot + ns) + return err + }, backoff.WithMaxRetries(backoff.NewConstantBackOff(time.Microsecond*500), 10)) + if err != nil { + t.Fatalf("failed to delete namespace %q, err=%s", ns, err) + } + }) + if _, err := client.Logical().Write( + consts.SysNamespaceRoot+ns, nil); err != nil { + t.Fatalf("failed to create namespace, err=%s", err) + } + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + config := api.DefaultConfig() + config.CloneToken = true + client, err := api.NewClient(config) + if err != nil { + t.Fatalf("failed to create Vault client, err=%s", err) + } + + if tt.authLoginNamespace != "" { + createNamespace(t, client, tt.authLoginNamespace) + options := &api.EnableAuthOptions{ + Type: consts.MountTypeUserpass, + Description: "test auth_userpass", + Local: true, + } + + clone, err := client.Clone() + if err != nil { + t.Fatalf("failed to clone Vault client, err=%s", err) + } + + clone.SetNamespace(tt.authLoginNamespace) + if err := clone.Sys().EnableAuthWithOptions(consts.MountTypeUserpass, options); err != nil { + t.Fatalf("failed to enable auth, err=%s", err) + } + + if _, err := clone.Logical().Write("auth/userpass/users/alice", + map[string]interface{}{ + consts.FieldPassword: defaultPassword, + consts.FieldTokenPolicies: []string{"admin", "default"}, + }); err != nil { + t.Fatalf("failed to create user, err=%s", err) + } + } + + if tt.tokenNamespace != "" { + if tt.data == nil { + t.Fatal("test data cannot be nil when tokenNamespace set") + } + + createNamespace(t, client, tt.tokenNamespace) + clone, err := client.Clone() + if err != nil { + t.Fatalf("failed to clone Vault client, err=%s", err) + } + + clone.SetNamespace(tt.tokenNamespace) + resp, err := clone.Auth().Token().Create(&api.TokenCreateRequest{}) + if err != nil { + t.Fatalf("failed to create Vault token, err=%s", err) + } + tt.data[consts.FieldToken] = resp.Auth.ClientToken + } + + for k, v := range tt.data { + if err := tt.d.Set(k, v); err != nil { + t.Fatalf("failed to set resource data, key=%s, value=%#v", k, v) + } + } + + got, err := NewProviderMeta(tt.d) + if (err != nil) != tt.wantErr { + t.Errorf("NewProviderMeta() error = %v, wantErr %v", err, tt.wantErr) + return + } + + if err != nil { + if got != nil { + t.Errorf("NewProviderMeta() got = %v, want nil", got) + } + return + } + + p, ok := got.(*ProviderMeta) + if !ok { + t.Fatalf("invalid type got %T, expected %T", got, &ProviderMeta{}) + } + + if !reflect.DeepEqual(p.client.Namespace(), tt.wantNamespace) { + t.Errorf("NewProviderMeta() got ns = %v, want ns %v", p.client.Namespace(), tt.wantNamespace) + } + + if client.Token() == "" { + t.Errorf("NewProviderMeta() got empty Client token") + } + }) + } +} diff --git a/vault/provider_test.go b/vault/provider_test.go index 5ff5ec9dd..12dfa0ff2 100644 --- a/vault/provider_test.go +++ b/vault/provider_test.go @@ -172,13 +172,13 @@ func TestAccNamespaceProviderConfigure(t *testing.T) { client := testProvider.Meta().(*provider.ProviderMeta).GetClient() t.Cleanup(func() { - if _, err := client.Logical().Delete(SysNamespaceRoot + namespacePath); err != nil { + if _, err := client.Logical().Delete(consts.SysNamespaceRoot + namespacePath); err != nil { t.Errorf("failed to delete parent namespace %q, err=%s", namespacePath, err) } }) // create the namespace for the provider - if _, err := client.Logical().Write(SysNamespaceRoot+namespacePath, nil); err != nil { + if _, err := client.Logical().Write(consts.SysNamespaceRoot+namespacePath, nil); err != nil { t.Fatal(err) } diff --git a/vault/resource_namespace.go b/vault/resource_namespace.go index 991e4248c..4b4cb19bc 100644 --- a/vault/resource_namespace.go +++ b/vault/resource_namespace.go @@ -19,10 +19,6 @@ import ( "github.com/hashicorp/terraform-provider-vault/util" ) -const ( - SysNamespaceRoot = "sys/namespaces/" -) - func namespaceResource() *schema.Resource { return &schema.Resource{ Create: namespaceCreate, @@ -65,7 +61,7 @@ func namespaceCreate(d *schema.ResourceData, meta interface{}) error { path := d.Get(consts.FieldPath).(string) log.Printf("[DEBUG] Creating namespace %s in Vault", path) - _, err := client.Logical().Write(SysNamespaceRoot+path, nil) + _, err := client.Logical().Write(consts.SysNamespaceRoot+path, nil) if err != nil { return fmt.Errorf("error writing to Vault: %s", err) } @@ -84,7 +80,7 @@ func namespaceDelete(d *schema.ResourceData, meta interface{}) error { log.Printf("[DEBUG] Deleting namespace %s from Vault", path) deleteNS := func() error { - if _, err := client.Logical().Delete(SysNamespaceRoot + path); err != nil { + if _, err := client.Logical().Delete(consts.SysNamespaceRoot + path); err != nil { // child namespaces exist under path "test-namespace-2161440981046539760/", cannot remove if respErr, ok := err.(*api.ResponseError); ok && (respErr.StatusCode == http.StatusBadRequest) { return err @@ -108,7 +104,7 @@ func namespaceDelete(d *schema.ResourceData, meta interface{}) error { // wait for the namespace to be gone... return backoff.RetryNotify(func() error { - if resp, _ := client.Logical().Read(SysNamespaceRoot + path); resp != nil { + if resp, _ := client.Logical().Read(consts.SysNamespaceRoot + path); resp != nil { return fmt.Errorf("namespace %q still exists", path) } return nil @@ -132,7 +128,7 @@ func namespaceRead(d *schema.ResourceData, meta interface{}) error { path := d.Id() - resp, err := client.Logical().Read(SysNamespaceRoot + path) + resp, err := client.Logical().Read(consts.SysNamespaceRoot + path) if err != nil { return fmt.Errorf("error reading from Vault: %s", err) } diff --git a/vault/resource_namespace_test.go b/vault/resource_namespace_test.go index 5c1819664..0b2705060 100644 --- a/vault/resource_namespace_test.go +++ b/vault/resource_namespace_test.go @@ -117,7 +117,7 @@ func testNamespaceDestroy(path string) resource.TestCheckFunc { return func(s *terraform.State) error { client := testProvider.Meta().(*provider.ProviderMeta).GetClient() - namespaceRef, err := client.Logical().Read(fmt.Sprintf("%s/%s", SysNamespaceRoot, path)) + namespaceRef, err := client.Logical().Read(fmt.Sprintf("%s/%s", consts.SysNamespaceRoot, path)) if err != nil { return fmt.Errorf("error reading back configuration: %s", err) } From f5f553cb09e5d8453afc8ae39a2887f6d228ee2a Mon Sep 17 00:00:00 2001 From: Ben Ash Date: Mon, 5 Jun 2023 11:27:15 -0400 Subject: [PATCH 4/6] Log a warning if the non-root token TTL is below 5m --- internal/provider/meta.go | 25 +++++++++++++++++++++++++ internal/provider/meta_test.go | 9 ++++++--- 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/internal/provider/meta.go b/internal/provider/meta.go index 476bc8e74..f04877703 100644 --- a/internal/provider/meta.go +++ b/internal/provider/meta.go @@ -11,6 +11,7 @@ import ( "os" "strings" "sync" + "time" "github.com/hashicorp/go-hclog" "github.com/hashicorp/go-version" @@ -39,6 +40,8 @@ var ( VaultVersion111 = version.Must(version.NewSemver(consts.VaultVersion111)) VaultVersion112 = version.Must(version.NewSemver(consts.VaultVersion112)) VaultVersion113 = version.Must(version.NewSemver(consts.VaultVersion113)) + + TokenTTLMinRecommended = time.Minute * 5 ) // ProviderMeta provides resources with access to the Vault client and @@ -277,6 +280,8 @@ func NewProviderMeta(d *schema.ResourceData) (interface{}, error) { tokenNamespace = strings.Trim(v.(string), "/") } + warnMinTokenTTL(tokenInfo) + if !d.Get(consts.FieldSkipChildToken).(bool) { // a child token is always created in the namespace of the parent token. token, err = createChildToken(d, client, tokenNamespace) @@ -333,6 +338,26 @@ func NewProviderMeta(d *schema.ResourceData) (interface{}, error) { }, nil } +func warnMinTokenTTL(tokenInfo *api.Secret) { + if policies, err := tokenInfo.TokenPolicies(); err == nil { + for _, v := range policies { + if v == "root" { + return + } + } + } + + // we can ignore the error here, any issue with the token will be handled later + // on during resource provisioning + if tokenTTL, err := tokenInfo.TokenTTL(); err == nil { + if tokenTTL < TokenTTLMinRecommended { + log.Printf("[WARN] The token TTL %s is below the minimum "+ + "recommended value of %s, this can result in unexpected Vault "+ + "provisioning failures e.g. 403 permission denied", tokenTTL, TokenTTLMinRecommended) + } + } +} + // GetClient is meant to be called from a schema.Resource function. // It ensures that the returned api.Client's matches the resource's configured // namespace. The value for the namespace is resolved from any of string, diff --git a/internal/provider/meta_test.go b/internal/provider/meta_test.go index d91cd7f5d..fd5df145e 100644 --- a/internal/provider/meta_test.go +++ b/internal/provider/meta_test.go @@ -632,14 +632,14 @@ func TestNewProviderMeta(t *testing.T) { consts.FieldSkipChildToken: true, consts.FieldAuthLoginUserpass: []map[string]interface{}{ { - consts.FieldNamespace: nsPrefix + "auth-ns", + consts.FieldNamespace: nsPrefix + "auth-ns-prov-ns", consts.FieldMount: consts.MountTypeUserpass, consts.FieldUsername: defaultUser, consts.FieldPassword: defaultPassword, }, }, }, - authLoginNamespace: nsPrefix + "auth-ns", + authLoginNamespace: nsPrefix + "auth-ns-prov-ns", wantNamespace: nsPrefix + "prov-ns-auth-ns", wantErr: false, }, @@ -709,8 +709,11 @@ func TestNewProviderMeta(t *testing.T) { t.Fatalf("failed to clone Vault client, err=%s", err) } + tokenTTL := time.Minute * 6 clone.SetNamespace(tt.tokenNamespace) - resp, err := clone.Auth().Token().Create(&api.TokenCreateRequest{}) + resp, err := clone.Auth().Token().Create(&api.TokenCreateRequest{ + TTL: tokenTTL.String(), + }) if err != nil { t.Fatalf("failed to create Vault token, err=%s", err) } From a019af08500a2bafa81a7ef0def0129a77527d73 Mon Sep 17 00:00:00 2001 From: Ben Ash Date: Mon, 5 Jun 2023 11:41:34 -0400 Subject: [PATCH 5/6] Add note about issues with short Token TTLs --- website/docs/index.html.markdown | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/website/docs/index.html.markdown b/website/docs/index.html.markdown index 1cd73fc6d..13bd34b13 100644 --- a/website/docs/index.html.markdown +++ b/website/docs/index.html.markdown @@ -41,7 +41,7 @@ populate it with secrets. In this case, the state and any plans associated with the configuration must be stored and communicated with care, since they will contain in cleartext any values that were written into Vault. -Currently Terraform has no mechanism to redact or protect secrets +Currently, Terraform has no mechanism to redact or protect secrets that are provided via configuration, so teams choosing to use Terraform for populating Vault secrets should pay careful attention to the notes on each resource's documentation page about how any secrets are persisted @@ -55,13 +55,20 @@ vs. writing and thus limit the exposure of a compromised token. ## Using Vault credentials in Terraform configuration +~> **Important** It is important to ensure that the Vault token +has a long enough `time-to-live` to allow for all Vault resources to +be successfully provisioned. In the case where the `TTL` is insufficient, +you may encounter unexpected permission denied errors. +See [Vault Token TTLs](https://developer.hashicorp.com/vault/docs/concepts/tokens#token-time-to-live-periodic-tokens-and-explicit-max-ttls) +for more details. + Most Terraform providers require credentials to interact with a third-party service that they wrap. This provider allows such credentials to be obtained from Vault, which means that operators or systems running Terraform need only access to a suitably-privileged Vault token in order to temporarily lease the credentials for other providers. -Currently Terraform has no mechanism to redact or protect secrets that +Currently, Terraform has no mechanism to redact or protect secrets that are returned via data sources, so secrets read via this provider will be persisted into the Terraform state, into any plan files, and in some cases in the console output produced while planning and applying. These artifacts @@ -76,7 +83,7 @@ those stored in Vault's "generic" secret backend. The requested token TTL can be controlled by the `max_lease_ttl_seconds` provider argument described below. It is important to consider that Terraform reads from data sources during the `plan` phase and writes the result into -the plan. Thus a subsequent `apply` will likely fail if it is run after the +the plan. Thus, a subsequent `apply` will likely fail if it is run after the intermediate token has expired, due to the revocation of the secrets that are stored in the plan. From 31207b8ac3d7427add96d5ce50f11c3e8d6d966e Mon Sep 17 00:00:00 2001 From: Ben Ash Date: Mon, 5 Jun 2023 15:05:02 -0400 Subject: [PATCH 6/6] Post review fixups --- internal/provider/meta.go | 13 ++++++++----- internal/provider/meta_test.go | 3 ++- vault/provider_test.go | 6 ++++-- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/internal/provider/meta.go b/internal/provider/meta.go index f04877703..36c758d0b 100644 --- a/internal/provider/meta.go +++ b/internal/provider/meta.go @@ -41,7 +41,7 @@ var ( VaultVersion112 = version.Must(version.NewSemver(consts.VaultVersion112)) VaultVersion113 = version.Must(version.NewSemver(consts.VaultVersion113)) - TokenTTLMinRecommended = time.Minute * 5 + TokenTTLMinRecommended = time.Minute * 15 ) // ProviderMeta provides resources with access to the Vault client and @@ -275,13 +275,13 @@ func NewProviderMeta(d *schema.ResourceData) (interface{}, error) { return nil, fmt.Errorf("failed to lookup token, err=%w", err) } + warnMinTokenTTL(tokenInfo) + var tokenNamespace string if v, ok := tokenInfo.Data[consts.FieldNamespacePath]; ok { tokenNamespace = strings.Trim(v.(string), "/") } - warnMinTokenTTL(tokenInfo) - if !d.Get(consts.FieldSkipChildToken).(bool) { // a child token is always created in the namespace of the parent token. token, err = createChildToken(d, client, tokenNamespace) @@ -297,8 +297,9 @@ func NewProviderMeta(d *schema.ResourceData) (interface{}, error) { // this is here to ensure that do not break any configurations that are relying on the // token's namespace being used during resource provisioning. // In the future we should drop support for this behaviour. - log.Printf("[WARN] The provider namespace should be set whenever using namespaced auth tokens. "+ - "You may want to update your provider configuration's namespace to be %q, before executing terraform."+ + log.Printf("[WARN] The provider namespace should be set whenever "+ + "using namespaced auth tokens. You may want to update your provider "+ + "configuration's namespace to be %q, before executing terraform. "+ "Future releases may not support this type of configuration.", tokenNamespace) namespace = tokenNamespace @@ -339,6 +340,8 @@ func NewProviderMeta(d *schema.ResourceData) (interface{}, error) { } func warnMinTokenTTL(tokenInfo *api.Secret) { + // tokens with "root" policies tend to have no TTL set, so there should be no + // need to warn in this case. if policies, err := tokenInfo.TokenPolicies(); err == nil { for _, v := range policies { if v == "root" { diff --git a/internal/provider/meta_test.go b/internal/provider/meta_test.go index fd5df145e..900b75913 100644 --- a/internal/provider/meta_test.go +++ b/internal/provider/meta_test.go @@ -709,7 +709,8 @@ func TestNewProviderMeta(t *testing.T) { t.Fatalf("failed to clone Vault client, err=%s", err) } - tokenTTL := time.Minute * 6 + // in order not to trigger the min TTL warning we can add some time to the min. + tokenTTL := TokenTTLMinRecommended + time.Second*10 clone.SetNamespace(tt.tokenNamespace) resp, err := clone.Auth().Token().Create(&api.TokenCreateRequest{ TTL: tokenTTL.String(), diff --git a/vault/provider_test.go b/vault/provider_test.go index 12dfa0ff2..b0c750fcb 100644 --- a/vault/provider_test.go +++ b/vault/provider_test.go @@ -643,8 +643,10 @@ func TestAccChildToken(t *testing.T) { } } }, - Config: testProviderConfig(test.useChildTokenSchema, consts.FieldSkipChildToken+` = `+test.skipChildTokenSchema), - Check: checkTokenUsed(test.expectChildToken), + Config: testProviderConfig(test.useChildTokenSchema, + consts.FieldSkipChildToken+` = `+test.skipChildTokenSchema, + ), + Check: checkTokenUsed(test.expectChildToken), }, }, })