identity: do not allow a role's token_ttl to be longer than verificat…

…ion_ttl (#12151) (#12213) * 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 <[email protected]> Co-authored-by: Austin Gebauer <[email protected]> Co-authored-by: John-Michael Faircloth <[email protected]> Co-authored-by: Austin Gebauer <[email protected]>
hashicorp · Jul 30, 2021 · f882564 · f882564
1 parent 571a30c
commit f882564
Show file tree

Hide file tree

Showing 3 changed files with 188 additions and 32 deletions.
diff --git a/changelog/12151.txt 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
@@ -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,46 +576,70 @@ func (i *IdentityStore) pathOIDCReadKey(ctx context.Context, req *logical.Reques
 	}, 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)
-	if err != nil {
-		return nil, err
-	}
-
-	targetKeyName := d.Get("name").(string)
-
-	i.oidcLock.Lock()
-
-	// it is an error to delete a key that is actively referenced by a role
+// 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 {
-		i.oidcLock.Unlock()
 		return nil, err
 	}
 
-	var role *role
-	rolesReferencingTargetKeyName := make([]string, 0)
+	var tempRole role
+	roles := make(map[string]role)
 	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()
+			if err := entry.DecodeJSON(&tempRole); err != nil {
 				return nil, err
 			}
-			if role.Key == targetKeyName {
-				rolesReferencingTargetKeyName = append(rolesReferencingTargetKeyName, roleName)
+			if tempRole.Key == targetKeyName {
+				roles[roleName] = tempRole
 			}
 		}
 	}
 
-	if len(rolesReferencingTargetKeyName) > 0 {
+	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)
+	if err != nil {
+		return nil, err
+	}
+
+	targetKeyName := d.Get("name").(string)
+
+	i.oidcLock.Lock()
+
+	roleNames, err := i.roleNamesReferencingTargetKeyName(ctx, req, targetKeyName)
+	if err != nil {
+		return nil, err
+	}
+
+	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
@@ -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)