From bfe49f287513336b66e016fe4f990b624d216d07 Mon Sep 17 00:00:00 2001 From: Guilherme Santos <157053549+gsantos-hc@users.noreply.github.com> Date: Wed, 3 Jul 2024 15:29:44 -0400 Subject: [PATCH] feat: add explicit_max_ttl to azure secrets (#199) Add `explicit_max_ttl` to Azure role attributes. If set, Application Secrets in Azure AD will be created with a maximum lifetime equal to `explicit_max_ttl` instead of the hard-coded 10-year default in effect until now. Leases are renewable unless or until the remaining Azure-side lifetime is shorter than the role's configured TTL. Marking a lease as non-renewable signals to clients that they must obtain a new lease/secret when the existing one is approaching the limit that was originally set through `explicit_max_ttl`. Fixes #178 Fixes VAULT-12316 --- CHANGELOG.md | 3 ++ backend_test.go | 24 ++++++++-------- client.go | 17 +++++++----- path_roles.go | 30 ++++++++++++++++++-- path_roles_test.go | 35 ++++++++++++++++------- path_service_principal.go | 38 ++++++++++++++++++++++--- path_service_principal_test.go | 51 ++++++++++++++++++++++++++++++++++ 7 files changed, 164 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 582f67c6..97627277 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,8 @@ ## Unreleased +FEATURES: +* Adds ability to limit the lifetime of service principal secrets in Azure through `explicit_max_ttl` on roles ([GH-199](https://github.com/hashicorp/vault-plugin-secrets-azure/pull/199)) + ## v0.19.0 IMPROVEMENTS: diff --git a/backend_test.go b/backend_test.go index 6dd1f037..c8b96ea4 100644 --- a/backend_test.go +++ b/backend_test.go @@ -16,10 +16,11 @@ import ( ) const ( - defaultLeaseTTLHr = 1 * time.Hour - maxLeaseTTLHr = 12 * time.Hour - defaultTestTTL = 300 - defaultTestMaxTTL = 3600 + defaultLeaseTTLHr = 1 * time.Hour + maxLeaseTTLHr = 12 * time.Hour + defaultTestTTL = 300 + defaultTestMaxTTL = 3600 + defaultTestExplicitMaxTTL = 7200 ) var ( @@ -59,13 +60,14 @@ func getTestBackendMocked(t *testing.T, initConfig bool) (*azureSecretBackend, l if initConfig { cfg := map[string]interface{}{ - "subscription_id": generateUUID(), - "tenant_id": generateUUID(), - "client_id": testClientID, - "client_secret": testClientSecret, - "environment": "AZURECHINACLOUD", - "ttl": defaultTestTTL, - "max_ttl": defaultTestMaxTTL, + "subscription_id": generateUUID(), + "tenant_id": generateUUID(), + "client_id": testClientID, + "client_secret": testClientSecret, + "environment": "AZURECHINACLOUD", + "ttl": defaultTestTTL, + "max_ttl": defaultTestMaxTTL, + "explicit_max_ttl": defaultTestExplicitMaxTTL, } testConfigCreate(t, b, config.StorageView, cfg, false) diff --git a/client.go b/client.go index 0fbe7f6d..ace4bb54 100644 --- a/client.go +++ b/client.go @@ -80,16 +80,18 @@ func (c *client) createAppWithName(ctx context.Context, rolename string, signInA func (c *client) createSP( ctx context.Context, app api.Application, - duration time.Duration) (spID string, password string, err error) { + duration time.Duration) (spID string, password string, endDate time.Time, err error) { type idPass struct { ID string Password string + EndDate time.Time } resultRaw, err := retry(ctx, func() (interface{}, bool, error) { now := time.Now() - spID, password, err := c.provider.CreateServicePrincipal(ctx, app.AppID, now, now.Add(duration)) + endDate := now.Add(duration) + spID, password, err := c.provider.CreateServicePrincipal(ctx, app.AppID, now, endDate) // Propagation delays within Azure can cause this error occasionally, so don't quit on it. if err != nil && (strings.Contains(err.Error(), errInvalidApplicationObject)) { @@ -99,32 +101,33 @@ func (c *client) createSP( result := idPass{ ID: spID, Password: password, + EndDate: endDate, } return result, true, err }) if err != nil { - return "", "", fmt.Errorf("error creating service principal: %w", err) + return "", "", time.Time{}, fmt.Errorf("error creating service principal: %w", err) } result := resultRaw.(idPass) - return result.ID, result.Password, nil + return result.ID, result.Password, result.EndDate, nil } // addAppPassword adds a new password to an App's credentials list. -func (c *client) addAppPassword(ctx context.Context, appObjID string, expiresIn time.Duration) (string, string, error) { +func (c *client) addAppPassword(ctx context.Context, appObjID string, expiresIn time.Duration) (string, string, time.Time, error) { exp := time.Now().Add(expiresIn) resp, err := c.provider.AddApplicationPassword(ctx, appObjID, "vault-plugin-secrets-azure", exp) if err != nil { if strings.Contains(err.Error(), "size of the object has exceeded its limit") { err = errors.New("maximum number of Application passwords reached") } - return "", "", fmt.Errorf("error updating credentials: %w", err) + return "", "", time.Time{}, fmt.Errorf("error updating credentials: %w", err) } - return resp.KeyID, resp.SecretText, nil + return resp.KeyID, resp.SecretText, resp.EndDate, nil } // deleteAppPassword removes a password, if present, from an App's credentials list. diff --git a/path_roles.go b/path_roles.go index d07595be..716ea45d 100644 --- a/path_roles.go +++ b/path_roles.go @@ -36,6 +36,7 @@ type roleEntry struct { Tags []string `json:"tags"` TTL time.Duration `json:"ttl"` MaxTTL time.Duration `json:"max_ttl"` + ExplicitMaxTTL time.Duration `json:"explicit_max_ttl"` PermanentlyDelete bool `json:"permanently_delete"` PersistApp bool `json:"persist_app"` @@ -106,6 +107,10 @@ func pathsRole(b *azureSecretBackend) []*framework.Path { Type: framework.TypeDurationSecond, Description: "Maximum time a service principal. If not set or set to 0, will use system default.", }, + "explicit_max_ttl": { + Type: framework.TypeDurationSecond, + Description: "Maximum lifetime of the lease and service principal. If not set or set to 0, will use the system default.", + }, "permanently_delete": { Type: framework.TypeBool, Description: "Indicates whether new application objects should be permanently deleted. If not set, objects will not be permanently deleted.", @@ -207,10 +212,24 @@ func (b *azureSecretBackend) pathRoleUpdate(ctx context.Context, req *logical.Re role.MaxTTL = time.Duration(d.Get("max_ttl").(int)) * time.Second } + if explicitMaxTTLRaw, ok := d.GetOk("explicit_max_ttl"); ok { + role.ExplicitMaxTTL = time.Duration(explicitMaxTTLRaw.(int)) * time.Second + } else if req.Operation == logical.CreateOperation { + role.ExplicitMaxTTL = time.Duration(d.Get("explicit_max_ttl").(int)) * time.Second + } + if role.MaxTTL != 0 && role.TTL > role.MaxTTL { return logical.ErrorResponse("ttl cannot be greater than max_ttl"), nil } + if role.ExplicitMaxTTL != 0 && role.TTL > role.ExplicitMaxTTL { + return logical.ErrorResponse("ttl cannot be greater than explicit_max_ttl"), nil + } + + if role.ExplicitMaxTTL != 0 && role.MaxTTL > role.ExplicitMaxTTL { + return logical.ErrorResponse("max_ttl cannot be greater than explicit_max_ttl"), nil + } + // load and verify deletion options if permanentlyDeleteRaw, ok := d.GetOk("permanently_delete"); ok { role.PermanentlyDelete = permanentlyDeleteRaw.(bool) @@ -463,8 +482,14 @@ func (b *azureSecretBackend) createPersistedApp(ctx context.Context, req *logica return fmt.Errorf("error writing WAL: %w", err) } - // TODO: should we expire the PW? - spObjID, _, err := c.createSP(ctx, app, spExpiration) + // Determine SP duration + spDuration := spExpiration + if role.ExplicitMaxTTL != 0 { + spDuration = role.ExplicitMaxTTL + } + + // Create the SP + spObjID, _, _, err := c.createSP(ctx, app, spDuration) if err != nil { return err } @@ -520,6 +545,7 @@ func (b *azureSecretBackend) pathRoleRead(ctx context.Context, req *logical.Requ Data: map[string]interface{}{ "ttl": r.TTL / time.Second, "max_ttl": r.MaxTTL / time.Second, + "explicit_max_ttl": r.ExplicitMaxTTL / time.Second, "azure_roles": r.AzureRoles, "azure_groups": r.AzureGroups, "application_object_id": r.ApplicationObjectID, diff --git a/path_roles_test.go b/path_roles_test.go index 42ca7b43..dc5aba9e 100644 --- a/path_roles_test.go +++ b/path_roles_test.go @@ -44,6 +44,7 @@ func TestRoleCreate(t *testing.T) { }]`), "ttl": int64(0), "max_ttl": int64(0), + "explicit_max_ttl": int64(0), "application_object_id": "", "permanently_delete": true, "persist_app": false, @@ -74,6 +75,7 @@ func TestRoleCreate(t *testing.T) { }]`), "ttl": int64(300), "max_ttl": int64(3000), + "explicit_max_ttl": int64(3000), "application_object_id": "", "permanently_delete": true, "persist_app": false, @@ -124,6 +126,7 @@ func TestRoleCreate(t *testing.T) { }]`), "ttl": int64(0), "max_ttl": int64(0), + "explicit_max_ttl": int64(0), "application_object_id": "", "persist_app": true, } @@ -151,6 +154,7 @@ func TestRoleCreate(t *testing.T) { }]`), "ttl": int64(300), "max_ttl": int64(3000), + "explicit_max_ttl": int64(3000), "application_object_id": "", "persist_app": true, } @@ -198,6 +202,7 @@ func TestRoleCreate(t *testing.T) { "application_object_id": "00000000-0000-0000-0000-000000000000", "ttl": int64(300), "max_ttl": int64(3000), + "explicit_max_ttl": int64(3000), "azure_roles": "[]", "azure_groups": "[]", "sign_in_audience": "PersonalMicrosoftAccount", @@ -232,13 +237,14 @@ func TestRoleCreate(t *testing.T) { "persist_app": false, } - // Verify that ttl and max_ttl are 0 if not provided + // Verify that ttl, max_ttl and explicit_max_ttl are 0 if not provided // and that permanently_delete is false if not provided name := generateUUID() testRoleCreate(t, b, s, name, testRole) testRole["ttl"] = int64(0) testRole["max_ttl"] = int64(0) + testRole["explicit_max_ttl"] = int64(0) testRole["permanently_delete"] = false resp, err := testRoleRead(t, b, s, name) @@ -253,16 +259,21 @@ func TestRoleCreate(t *testing.T) { const skip = -999 tests := []struct { - ttl int64 - maxTTL int64 - expError bool + ttl int64 + maxTTL int64 + explicitMaxTTL int64 + expError bool }{ - {5, 10, false}, - {5, skip, false}, - {skip, 10, false}, - {100, 100, false}, - {101, 100, true}, - {101, 0, false}, + {5, 10, skip, false}, + {5, skip, skip, false}, + {skip, 10, skip, false}, + {100, 100, skip, false}, + {101, 100, skip, true}, + {101, 0, skip, false}, + {5, 10, 10, false}, + {10, skip, 5, true}, + {10, 10, 5, true}, + {skip, skip, 10, false}, } for i, test := range tests { @@ -276,6 +287,9 @@ func TestRoleCreate(t *testing.T) { if test.maxTTL != skip { role["max_ttl"] = test.maxTTL } + if test.explicitMaxTTL != skip { + role["explicit_max_ttl"] = test.explicitMaxTTL + } name := generateUUID() resp, err := b.HandleRequest(context.Background(), &logical.Request{ Operation: logical.CreateOperation, @@ -864,4 +878,5 @@ func convertRespTypes(data map[string]interface{}) { } data["ttl"] = int64(data["ttl"].(time.Duration)) data["max_ttl"] = int64(data["max_ttl"].(time.Duration)) + data["explicit_max_ttl"] = int64(data["explicit_max_ttl"].(time.Duration)) } diff --git a/path_service_principal.go b/path_service_principal.go index 60e7bdb6..69c4498b 100644 --- a/path_service_principal.go +++ b/path_service_principal.go @@ -20,6 +20,7 @@ const ( ) // SPs will be created with a far-future expiration in Azure +// unless `explicit_max_ttl` is set in the role var spExpiration = 10 * 365 * 24 * time.Hour func secretServicePrincipal(b *azureSecretBackend) *framework.Secret { @@ -97,6 +98,9 @@ func (b *azureSecretBackend) pathSPRead(ctx context.Context, req *logical.Reques resp.Secret.TTL = role.TTL resp.Secret.MaxTTL = role.MaxTTL + if role.ExplicitMaxTTL != 0 && (role.ExplicitMaxTTL < role.MaxTTL || role.MaxTTL == 0) { + resp.Secret.MaxTTL = role.ExplicitMaxTTL + } return resp, nil } @@ -122,8 +126,14 @@ func (b *azureSecretBackend) createSPSecret(ctx context.Context, s logical.Stora return nil, fmt.Errorf("error writing WAL: %w", err) } + // Determine SP duration + spDuration := spExpiration + if role.ExplicitMaxTTL != 0 { + spDuration = role.ExplicitMaxTTL + } + // Create a service principal associated with the new App - spID, password, err := c.createSP(ctx, app, spExpiration) + spID, password, endDate, err := c.createSP(ctx, app, spDuration) if err != nil { return nil, err } @@ -175,6 +185,7 @@ func (b *azureSecretBackend) createSPSecret(ctx context.Context, s logical.Stora "group_membership_ids": groupObjectIDs(role.AzureGroups), "role": roleName, "permanently_delete": role.PermanentlyDelete, + "key_end_date": endDate.Format(time.RFC3339Nano), } return b.Secret(SecretTypeSP).Response(data, internalData), nil @@ -186,7 +197,13 @@ func (b *azureSecretBackend) createStaticSPSecret(ctx context.Context, c *client lock.Lock() defer lock.Unlock() - keyID, password, err := c.addAppPassword(ctx, role.ApplicationObjectID, spExpiration) + // Determine SP duration + spDuration := spExpiration + if role.ExplicitMaxTTL != 0 { + spDuration = role.ExplicitMaxTTL + } + + keyID, password, endDate, err := c.addAppPassword(ctx, role.ApplicationObjectID, spDuration) if err != nil { return nil, err } @@ -198,6 +215,7 @@ func (b *azureSecretBackend) createStaticSPSecret(ctx context.Context, c *client internalData := map[string]interface{}{ "app_object_id": role.ApplicationObjectID, "key_id": keyID, + "key_end_date": endDate.Format(time.RFC3339Nano), "role": roleName, } @@ -219,9 +237,21 @@ func (b *azureSecretBackend) spRenew(ctx context.Context, req *logical.Request, return nil, nil } + // Determine remaining lifetime of SP secret in Azure + keyEndDateRaw, ok := req.Secret.InternalData["key_end_date"] + if !ok { + return nil, errors.New("internal data 'key_end_date' not found") + } + keyEndDate, err := time.Parse(time.RFC3339Nano, keyEndDateRaw.(string)) + if err != nil { + return nil, errors.New("cannot parse 'key_end_date' to timestamp") + } + keyLifetime := time.Until(keyEndDate) + resp := &logical.Response{Secret: req.Secret} - resp.Secret.TTL = role.TTL - resp.Secret.MaxTTL = role.MaxTTL + resp.Secret.TTL = min(role.TTL, keyLifetime) + resp.Secret.MaxTTL = min(role.MaxTTL, keyLifetime) + resp.Secret.Renewable = role.TTL < keyLifetime // Lease cannot be renewed beyond service-side endDate return resp, nil } diff --git a/path_service_principal_test.go b/path_service_principal_test.go index 8677b3da..39b15992 100644 --- a/path_service_principal_test.go +++ b/path_service_principal_test.go @@ -315,6 +315,23 @@ func TestSPRead(t *testing.T) { equal(t, 20*time.Second, resp.Secret.TTL) equal(t, 30*time.Second, resp.Secret.MaxTTL) + + roleUpdate = map[string]interface{}{ + "ttl": 20, + "explicit_max_ttl": 60, + } + testRoleCreate(t, b, s, name, roleUpdate) + + resp, err = b.HandleRequest(context.Background(), &logical.Request{ + Operation: logical.ReadOperation, + Path: "creds/" + name, + Storage: s, + }) + + assertErrorIsNil(t, err) + + equal(t, 20*time.Second, resp.Secret.TTL) + equal(t, 60*time.Second, resp.Secret.MaxTTL) }) } @@ -389,6 +406,23 @@ func TestStaticSPRead(t *testing.T) { equal(t, 20*time.Second, resp.Secret.TTL) equal(t, 30*time.Second, resp.Secret.MaxTTL) + + roleUpdate = map[string]interface{}{ + "ttl": 20, + "explicit_max_ttl": 60, + } + testRoleCreate(t, b, s, name, roleUpdate) + + resp, err = b.HandleRequest(context.Background(), &logical.Request{ + Operation: logical.ReadOperation, + Path: "creds/" + name, + Storage: s, + }) + + assertErrorIsNil(t, err) + + equal(t, 20*time.Second, resp.Secret.TTL) + equal(t, 60*time.Second, resp.Secret.MaxTTL) }) } @@ -463,6 +497,23 @@ func TestPersistentAppSPRead(t *testing.T) { equal(t, 20*time.Second, resp.Secret.TTL) equal(t, 30*time.Second, resp.Secret.MaxTTL) + + roleUpdate = map[string]interface{}{ + "ttl": 20, + "explicit_max_ttl": 60, + } + testRoleCreate(t, b, s, name, roleUpdate) + + resp, err = b.HandleRequest(context.Background(), &logical.Request{ + Operation: logical.ReadOperation, + Path: "creds/" + name, + Storage: s, + }) + + assertErrorIsNil(t, err) + + equal(t, 20*time.Second, resp.Secret.TTL) + equal(t, 60*time.Second, resp.Secret.MaxTTL) }) }