Skip to content

feat(policy-engine): add safe affinity-based V2 - #3

Merged
Tetrax merged 5 commits into
mainfrom
feat/policy-engine-v2
Aug 20, 2026
Merged

feat(policy-engine): add safe affinity-based V2#3
Tetrax merged 5 commits into
mainfrom
feat/policy-engine-v2

Conversation

@Tetrax

@Tetrax Tetrax commented Aug 20, 2026

Copy link
Copy Markdown
Owner

Résumé

  • introduit FlowAtom, l’affinité source/destination/service et un optimizer déterministe par rectangles sûrs ;
  • calcule coverage, missing, unexpected et expansion globalement et par policy ;
  • ajoute les profils Recommandé, Strict, Synthétique et Expert ;
  • normalise les services FortiGate, y compris ICMP nommé et type/code, sans fallback ALL/ALL_ICMP ;
  • intègre le moteur au chemin API → analyse → sélection → preflight → CLI ;
  • reprend l’UX Déployer avec métriques de sécurité et matrice destination × service ;
  • conserve le runtime de référence et les données existantes.

Findings de la review finale — corrigés

  • C1 interfaces : l’agrégation Séquence est désactivée pour V2 ; le preflight final détecte aussi toute perte de partition d’interfaces.
  • H1 sélection : coverage/missing/unexpected sont recalculés sur les FlowAtoms déployables après sélection utilisateur ; toute omission bloque le preflight.
  • H2 ICMP : un objet nommé doit avoir le même protocole et, si présents, les mêmes type/code ; les correspondances ambiguës restent bloquées.
  • H3/H4 main : branche réconciliée avec main@17187da, hardening, dépendances, Node 26, rate limiting et tests sécurité conservés.
  • M1 API : endpoint V2 placé derrière le rate limiter et résultat mis en cache par session/profil/flux/configuration.
  • Performance : buildAffinityViews() utilise un index service → destinations, sans group.some() imbriqué.

Vérifications locales

Tests Node                     103/103 pass
Syntaxe JavaScript             PASS
Docker Compose config          PASS
npm audit CRITICAL gate        PASS
Image candidate                fortiflow:pr3-867c7a5043ef
Image digest                   sha256:2b009eb0b74cb519da674d14d8228a424481acb64c73112fa6e7f8ad375ff8ba
HTTP / healthcheck             PASS
Tests dans le conteneur        101 pass / 2 skips attendus hors contexte repo

Benchmark matrice d’affinité :

250 policies      25 ms
500 policies      54 ms
1 000 policies   207 ms

Tous les benchmarks de sécurité restent à missing=0 et unexpected=0 pour Recommandé/Strict.

Dataset réel

Dernière baseline complète préservée :

Policies legacy              1 793
Policies V2 recommended        388
Observed required tuples     7 230
Covered required tuples      7 230
Missing required tuples          0
Unexpected allowed tuples        0
Coverage                    100 %
Expansion                     0 %
Blocked required tuples          7
Deployable required tuples   7 223

Les 7 tuples ICMP sans preuve suffisante restent visibles et bloqués ; aucun élargissement automatique n’est ajouté. Le cache de session complet utilisé pour cette baseline n’est plus présent sur le runtime, donc cette mesure historique n’est pas réinventée depuis le workspace legacy incomplet.

Déploiement

Après merge et CI verte : construire une image immuable depuis le SHA mergé, déployer en parallèle sur un port alternatif avec données isolées, exécuter healthchecks/smoke/dataset disponible, puis basculer la stack existante avec fortiflow:17187da conservée comme rollback.

@Tetrax Tetrax left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final technical review — BLOCKED

Review boundary: PR #3 at b0a28eef201f3e9fdbb752014936553b3ad77928 against current main@17187da75fccc56d413c2824c0c7028aab2f4a11.

Blocking security findings

  1. Selected-policy coverage is not recomputed. Selecting one of two required V2 policies still returns preflight exact with 0 errors. The final gate proves every selected rule is observed, but not that all required tuples remain covered. A required flow can therefore be omitted while certification stays exact.
  2. Sequence mode reintroduces an interface Cartesian product. Two exact policies on interface pairs A→X and B→Y are aggregated into src interfaces [A,B] and dst interfaces [X,Y], which authorizes four pairs. Preflight checks interface existence but evidence matching ignores interfaces and returns exact.
  3. Named ICMP can be falsely certified exact. A log service=PING with no type/code is accepted against any configured ICMP object with the same name, even when that object is type 3/code 1. The probe returned 0 blockers, coverage 100 % and preflight exact.

Delivery blockers

  1. GitHub reports the PR CONFLICTING/DIRTY. Current main contains security hardening absent from the branch. git merge-tree reports a content conflict in app/web/test/server-dependencies.test.js; resolution must preserve both the V2 endpoint/import assertions and the main-branch reverse-proxy security assertion.
  2. There are no GitHub checks. The 85 green tests apply to the stale branch head, while current main's 63 tests apply separately. The combined Node 26 / updated dependency / security workflow result has not been executed.
  3. The isolated Docker image/smoke evidence is tagged bd15702, while the PR head is b0a28ee; it does not contain the final ICMP commit or current main's Node 26 security base image.

Medium

  • /api/policy-engine/v2 synchronously recomputes the full engine on the Node HTTP thread and is absent from main's costly-route rate-limiter list. The real dataset takes about 1.5 s per call; repeated requests can stall health/API handling.

Gates observed

  • PR head tests: 85/85 pass.
  • Current main tests in isolated archive: 63/63 pass.
  • JavaScript syntax, Compose rendering, diff checks and added-line secret/static scans pass.
  • npm audit --audit-level=critical passes but reports 11 non-critical findings on the stale branch dependencies; current main updates Multer/ws and adds the critical Trivy/npm gates.
  • GitHub CI: no checks reported.

Required before merge

  • reconcile the branch with current main and resolve the test conflict additively;
  • gate selected-policy coverage against the complete required tuple set;
  • remove/disable V2 sequence aggregation or include exact interface-pair evidence in grouping, metrics and preflight;
  • require ICMP semantic equality (type/code or another authoritative fingerprint), not name/protocol alone;
  • rate-limit/cache/offload the V2 endpoint;
  • run the full combined suite and GitHub security workflow.
  • build and smoke an immutable image from the final reconciled merge SHA.

No code was modified during this review.

@Tetrax

Tetrax commented Aug 20, 2026

Copy link
Copy Markdown
Owner Author

Addendum to final review — confirmed performance blocker

The interrupted contradictory review contained a 1,000-atom timing anomaly. I reproduced and isolated it to buildAffinityViews() (app/web/lib/policy-engine-v2.js:698-739).

For one source with n destinations and n distinct services/policies, the matrix loops over every service × destination and calls group.some(...) across all policies (:715-717). This produces cubic-like growth:

250 atoms/policies     135 ms
500 atoms/policies   1 494 ms
1,000 atoms/policies 11 713 ms

All runs still returned missing=0 and unexpected=0; the issue is availability/performance, not tuple correctness.

Because /api/policy-engine/v2 executes synchronously on the Node HTTP thread, a valid dataset with a large affinity group can stall the whole application for seconds or minutes. The previous 100k/1M benchmark did not cover this topology.

Severity: High / merge-blocking for the stated large-dataset requirement.

Required targeted correction: construct an indexed service → destination membership map once per affinity group instead of scanning every policy inside the nested matrix loops, then add a regression benchmark with this sparse diagonal topology. No optimizer or policy-count change is required.

@Tetrax
Tetrax marked this pull request as ready for review August 20, 2026 13:53
@Tetrax
Tetrax merged commit f8f3c3d into main Aug 20, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant