From 39c744ca4e86872a744d5f6ca48b107ac95930f1 Mon Sep 17 00:00:00 2001 From: John-Michael Faircloth Date: Wed, 28 Jul 2021 20:34:52 -0500 Subject: [PATCH] identity: do not allow a role's token_ttl to be longer than verification_ttl (#12151) * do not allow token_ttl to be longer than verification_ttl * add verification when updating an existing key When updating a key, ensure any roles referencing the key do not already have a token_ttl greater than the key's verification_ttl * add changelog * remove unneeded UT check and comment * refactor based on PR comments - remove make slice in favor of var delcaration - remove unneeded if check - validate expiry value during token generation - update changelog as bug * refactor get roles referencing target key names logic * add note about thread safety to helper func * update func comment * sort array and refactor func names * add warning to return response * remove unnecessary code from unit test * Update vault/identity_store_oidc.go Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com> Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com> --- changelog/12151.txt | 3 + vault/identity_store_oidc.go | 135 ++++++++++++++++++++++-------- vault/identity_store_oidc_test.go | 86 +++++++++++++++++++ 3 files changed, 190 insertions(+), 34 deletions(-) create mode 100644 changelog/12151.txt diff --git a/changelog/12151.txt b/changelog/12151.txt new file mode 100644 index 000000000..2d2a8342d --- /dev/null +++ b/changelog/12151.txt @@ -0,0 +1,3 @@ +```release-note:bug +identity: do not allow a role's token_ttl to be longer than the signing key's verification_ttl +``` diff --git a/vault/identity_store_oidc.go b/vault/identity_store_oidc.go index 924050a90..5c70edc0f 100644 --- a/vault/identity_store_oidc.go +++ b/vault/identity_store_oidc.go @@ -11,6 +11,7 @@ import ( "errors" "fmt" "net/url" + "sort" "strings" "time" @@ -472,6 +473,25 @@ func (i *IdentityStore) pathOIDCCreateUpdateKey(ctx context.Context, req *logica return logical.ErrorResponse("verification_ttl cannot be longer than 10x rotation_period"), nil } + if req.Operation == logical.UpdateOperation { + // ensure any roles referencing this key do not already have a token_ttl + // greater than the key's verification_ttl + roles, err := i.rolesReferencingTargetKeyName(ctx, req, name) + if err != nil { + return nil, err + } + for _, role := range roles { + if role.TokenTTL > key.VerificationTTL { + errorMessage := fmt.Sprintf( + "unable to update key %q because it is currently referenced by one or more roles with a token ttl greater than %d seconds", + name, + key.VerificationTTL/time.Second, + ) + return logical.ErrorResponse(errorMessage), nil + } + } + } + if allowedClientIDsRaw, ok := d.GetOk("allowed_client_ids"); ok { key.AllowedClientIDs = allowedClientIDsRaw.([]string) } else if req.Operation == logical.CreateOperation { @@ -556,6 +576,51 @@ func (i *IdentityStore) pathOIDCReadKey(ctx context.Context, req *logical.Reques }, nil } +// rolesReferencingTargetKeyName returns a map of role names to roles referenced by targetKeyName. +// Note: this is not threadsafe. It is to be called with Lock already held. +func (i *IdentityStore) rolesReferencingTargetKeyName(ctx context.Context, req *logical.Request, targetKeyName string) (map[string]role, error) { + roleNames, err := req.Storage.List(ctx, roleConfigPath) + if err != nil { + return nil, err + } + + var tempRole role + roles := make(map[string]role) + for _, roleName := range roleNames { + entry, err := req.Storage.Get(ctx, roleConfigPath+roleName) + if err != nil { + return nil, err + } + if entry != nil { + if err := entry.DecodeJSON(&tempRole); err != nil { + return nil, err + } + if tempRole.Key == targetKeyName { + roles[roleName] = tempRole + } + } + } + + return roles, nil +} + +// roleNamesReferencingTargetKeyName returns a slice of strings of role +// names referenced by targetKeyName. +// Note: this is not threadsafe. It is to be called with Lock already held. +func (i *IdentityStore) roleNamesReferencingTargetKeyName(ctx context.Context, req *logical.Request, targetKeyName string) ([]string, error) { + roles, err := i.rolesReferencingTargetKeyName(ctx, req, targetKeyName) + if err != nil { + return nil, err + } + + var names []string + for key, _ := range roles { + names = append(names, key) + } + sort.Strings(names) + return names, nil +} + // handleOIDCDeleteKey is used to delete a key func (i *IdentityStore) pathOIDCDeleteKey(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { ns, err := namespace.FromContext(ctx) @@ -567,35 +632,14 @@ func (i *IdentityStore) pathOIDCDeleteKey(ctx context.Context, req *logical.Requ i.oidcLock.Lock() - // it is an error to delete a key that is actively referenced by a role - roleNames, err := req.Storage.List(ctx, roleConfigPath) + roleNames, err := i.roleNamesReferencingTargetKeyName(ctx, req, targetKeyName) if err != nil { - i.oidcLock.Unlock() return nil, err } - var role *role - rolesReferencingTargetKeyName := make([]string, 0) - for _, roleName := range roleNames { - entry, err := req.Storage.Get(ctx, roleConfigPath+roleName) - if err != nil { - i.oidcLock.Unlock() - return nil, err - } - if entry != nil { - if err := entry.DecodeJSON(&role); err != nil { - i.oidcLock.Unlock() - return nil, err - } - if role.Key == targetKeyName { - rolesReferencingTargetKeyName = append(rolesReferencingTargetKeyName, roleName) - } - } - } - - if len(rolesReferencingTargetKeyName) > 0 { + if len(roleNames) > 0 { errorMessage := fmt.Sprintf("unable to delete key %q because it is currently referenced by these roles: %s", - targetKeyName, strings.Join(rolesReferencingTargetKeyName, ", ")) + targetKeyName, strings.Join(roleNames, ", ")) i.oidcLock.Unlock() return logical.ErrorResponse(errorMessage), logical.ErrInvalidRequest } @@ -747,13 +791,21 @@ func (i *IdentityStore) pathOIDCGenerateToken(ctx context.Context, req *logical. return nil, err } + retResp := &logical.Response{} + expiry := role.TokenTTL + if expiry > key.VerificationTTL { + expiry = key.VerificationTTL + retResp.AddWarning(fmt.Sprintf("a role's token ttl cannot be longer "+ + "than the verification_ttl of the key it references, setting token ttl to %d", expiry)) + } + now := time.Now() idToken := idToken{ Issuer: config.effectiveIssuer, Namespace: ns.ID, Subject: req.EntityID, Audience: role.ClientID, - Expiry: now.Add(role.TokenTTL).Unix(), + Expiry: now.Add(expiry).Unix(), IssuedAt: now.Unix(), } @@ -782,13 +834,12 @@ func (i *IdentityStore) pathOIDCGenerateToken(ctx context.Context, req *logical. return nil, fmt.Errorf("error signing OIDC token: %w", err) } - return &logical.Response{ - Data: map[string]interface{}{ - "token": signedIdToken, - "client_id": role.ClientID, - "ttl": int64(role.TokenTTL.Seconds()), - }, - }, nil + retResp.Data = map[string]interface{}{ + "token": signedIdToken, + "client_id": role.ClientID, + "ttl": int64(role.TokenTTL.Seconds()), + } + return retResp, nil } func (tok *idToken) generatePayload(logger hclog.Logger, template string, entity *identity.Entity, groups []*identity.Group) ([]byte, error) { @@ -867,7 +918,7 @@ func (i *IdentityStore) pathOIDCRoleExistenceCheck(ctx context.Context, req *log return role != nil, nil } -// handleOIDCCreateRole is used to create a new role or update an existing one +// pathOIDCCreateUpdateRole is used to create a new role or update an existing one func (i *IdentityStore) pathOIDCCreateUpdateRole(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { ns, err := namespace.FromContext(ctx) if err != nil { @@ -938,6 +989,22 @@ func (i *IdentityStore) pathOIDCCreateUpdateRole(ctx context.Context, req *logic role.TokenTTL = time.Duration(d.Get("ttl").(int)) * time.Second } + // get the key referenced by this role + var key namedKey + entry, err := req.Storage.Get(ctx, namedKeyConfigPath+role.Key) + if err != nil { + return nil, err + } + if entry != nil { + if err := entry.DecodeJSON(&key); err != nil { + return nil, err + } + } + + if role.TokenTTL > key.VerificationTTL { + return logical.ErrorResponse("a role's token ttl cannot be longer than the verification_ttl of the key it references"), nil + } + if clientID, ok := d.GetOk("client_id"); ok { role.ClientID = clientID.(string) } @@ -952,7 +1019,7 @@ func (i *IdentityStore) pathOIDCCreateUpdateRole(ctx context.Context, req *logic } // store role (which was either just created or updated) - entry, err := logical.StorageEntryJSON(roleConfigPath+name, role) + entry, err = logical.StorageEntryJSON(roleConfigPath+name, role) if err != nil { return nil, err } diff --git a/vault/identity_store_oidc_test.go b/vault/identity_store_oidc_test.go index e49eb48ba..a334393ca 100644 --- a/vault/identity_store_oidc_test.go +++ b/vault/identity_store_oidc_test.go @@ -113,6 +113,48 @@ func TestOIDC_Path_OIDCRoleRole(t *testing.T) { } } +// TestOIDC_Path_OIDCRole_InvalidTokenTTL tests the TokenTTL validation +func TestOIDC_Path_OIDCRole_InvalidTokenTTL(t *testing.T) { + c, _, _ := TestCoreUnsealed(t) + ctx := namespace.RootContext(nil) + storage := &logical.InmemStorage{} + + // Create a test key "test-key" + c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "oidc/key/test-key", + Operation: logical.CreateOperation, + Data: map[string]interface{}{ + "verification_ttl": int64(60), + }, + Storage: storage, + }) + + // Create a test role "test-role1" with a ttl longer than the + // verification_ttl -- should fail with error + resp, err := c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "oidc/role/test-role1", + Operation: logical.CreateOperation, + Data: map[string]interface{}{ + "key": "test-key", + "ttl": int64(3600), + }, + Storage: storage, + }) + expectError(t, resp, err) + + // Read "test-role1" + respReadTestRole1, err3 := c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "oidc/role/test-role1", + Operation: logical.ReadOperation, + Storage: storage, + }) + // Ensure that "test-role1" was not created + expectSuccess(t, respReadTestRole1, err3) + if respReadTestRole1 != nil { + t.Fatalf("Expected a nil response but instead got:\n%#v", respReadTestRole1) + } +} + // TestOIDC_Path_OIDCRole tests the List operation for roles func TestOIDC_Path_OIDCRole(t *testing.T) { c, _, _ := TestCoreUnsealed(t) @@ -284,6 +326,49 @@ func TestOIDC_Path_OIDCKeyKey(t *testing.T) { expectSuccess(t, resp, err) } +// TestOIDC_Path_OIDCKey_InvalidTokenTTL tests the TokenTTL validation +func TestOIDC_Path_OIDCKey_InvalidTokenTTL(t *testing.T) { + c, _, _ := TestCoreUnsealed(t) + ctx := namespace.RootContext(nil) + storage := &logical.InmemStorage{} + + // Create a test key "test-key" -- should succeed + resp, err := c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "oidc/key/test-key", + Operation: logical.CreateOperation, + Data: map[string]interface{}{ + "verification_ttl": "4m", + }, + Storage: storage, + }) + expectSuccess(t, resp, err) + + // Create a role that depends on test key + resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "oidc/role/allowed-test-role", + Operation: logical.UpdateOperation, + Data: map[string]interface{}{ + "key": "test-key", + "ttl": "4m", + }, + Storage: storage, + }) + expectSuccess(t, resp, err) + + // Update "test-key" -- should fail since allowed-test-role ttl is less than 2m + resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "oidc/key/test-key", + Operation: logical.UpdateOperation, + Data: map[string]interface{}{ + "rotation_period": "10m", + "verification_ttl": "2m", + "allowed_client_ids": "allowed-test-role", + }, + Storage: storage, + }) + expectError(t, resp, err) +} + // TestOIDC_Path_OIDCKey tests the List operation for keys func TestOIDC_Path_OIDCKey(t *testing.T) { c, _, _ := TestCoreUnsealed(t) @@ -1100,6 +1185,7 @@ func expectSuccess(t *testing.T, resp *logical.Response, err error) { } func expectError(t *testing.T, resp *logical.Response, err error) { + t.Helper() if err == nil { if resp == nil || !resp.IsError() { t.Fatalf("expected error but got success; error:\n%v\nresp: %#v", err, resp)