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

changelog/12151.txt

@@ -0,0 +1,3 @@
+```release-note:improvement
+identity: do not allow a role's token_ttl to be longer than the signing key's verification_ttl
+```

vault/identity_store_oidc.go

@@ -472,6 +472,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
+		rolesReferencingTargetKeyName, err := i.getRolesReferencingTargetKeyName(ctx, req, name)
+		if err != nil {
+			return nil, err
+		}
+		for _, role := range rolesReferencingTargetKeyName {
+			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 +575,34 @@ func (i *IdentityStore) pathOIDCReadKey(ctx context.Context, req *logical.Reques
 	}, nil
 }
 
+// getRolesReferencingTargetKeyName returns a slice of roles referenced by targetKeyName.
+// Note: this is not threadsafe. It is to be called with Lock already held.
+func (i *IdentityStore) getRolesReferencingTargetKeyName(ctx context.Context, req *logical.Request, targetKeyName string) ([]role, error) {
+	roleNames, err := req.Storage.List(ctx, roleConfigPath)
+	if err != nil {
+		return nil, err
+	}
+
+	var tempRole role
+	rolesReferencingTargetKeyName := make([]role, 0)
+	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 {
+				rolesReferencingTargetKeyName = append(rolesReferencingTargetKeyName, tempRole)
+			}
+		}
+	}
+
+	return rolesReferencingTargetKeyName, 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)
@@ -867,7 +914,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 +985,24 @@ 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
+	if req.Operation == logical.CreateOperation || req.Operation == logical.UpdateOperation {
+		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)
 	}

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,64 @@ 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)
+
+	// Delete allowed-test-role
+	c.identityStore.HandleRequest(ctx, &logical.Request{
+		Path:      "oidc/role/allowed-test-role",
+		Operation: logical.DeleteOperation,
+		Storage:   storage,
+	})
+
+	// Delete test-key
+	resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{
+		Path:      "oidc/key/test-key",
+		Operation: logical.DeleteOperation,
+		Storage:   storage,
+	})
+	expectSuccess(t, resp, err)
+}
+
 // TestOIDC_Path_OIDCKey tests the List operation for keys
 func TestOIDC_Path_OIDCKey(t *testing.T) {
 	c, _, _ := TestCoreUnsealed(t)
@@ -1100,6 +1200,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)