[main] Skip auth filter for cellnet protocol-level bye messages

[nvshaxie]

Forward-port of #4569 (which fixed NVBug 6154369 on the 2.8 branch) to main, plus a security tightening of the bye auth-bypass guard (per review).

On main, validate_auth_headers() still exempts only Register/Challenge, not Bye, so any cell shutdown (e.g. nvflare poc stop) logs a spurious unauthenticated msg (channel='cellnet.channel' topic='bye') ... missing client name ERROR, and the receiver's _peer_goodbye never runs (agent cleanup waits on the heartbeat timeout instead of firing immediately).

Changes

• defs.py: add CellChannel.CELLNET + CellChannelTopic.Bye canonical constants (from [2.8] Skip auth filter for cellnet protocol-level bye messages #4569).
• core_cell.py: use the canonical constants instead of local string literals (from [2.8] Skip auth filter for cellnet protocol-level bye messages #4569).
• authenticator.py: bypass auth for a cellnet bye only when the message actually terminates at this cell, i.e. DESTINATION == local_cell_fqcn. validate_auth_headers() gains a local_cell_fqcn parameter, supplied at all three call sites (server via self.cell, relay via my_fqcn, admin HCI client via self.cell); it fails closed when the FQCN is unknown.

Why the guard differs from the 2.8 original

The shipped 2.8 fix (b4fb37c4) guarded the bypass with to_cell == destination. That compares two sender-controlled headers to each other and never checks that the message terminates here — and because _forward rewrites TO_CELL at each hop, the equality also holds at the terminal hop of a forwarded bye, so a crafted bye addressed to another cell could still slip through and evict that cell's upstream agent. This PR replaces that guard with DESTINATION == local_cell_fqcn: the incoming filter runs before CoreCell's forward decision, so a bye whose DESTINATION is this cell is handled locally and never forwarded, while one naming another cell falls through to normal auth. Legitimate byes are always direct-neighbor (Cell.stop() broadcasts to directly-connected agents, so DESTINATION is the receiving cell), so real byes are unaffected — verified end-to-end (the "missing client name" ERROR stays gone).

Tests

• Added unit tests in authenticator_test.py: a bye terminating here is bypassed; a forwarded/forged bye (including one with TO_CELL == DESTINATION pointing at another cell) is rejected; no local FQCN fails closed.
• Fixed two existing test doubles (__new__-built FederatedServer, HCI _FakeCell) that didn't expose the cell FQCN the guard now reads.

Follow-up (not this PR): main's guard is now strictly stronger than the 2.8 branch's shipped to_cell == destination; the same strengthening should be back-ported to 2.8.

Types of changes

• Non-breaking change (fix or new feature that would not break existing functionality).
• Breaking change (fix or new feature that would cause existing functionality to change).
• New tests added to cover the changes.
• Quick tests passed locally (unit tests + black/isort/flake8 on changed files).
• In-line docstrings updated.
• Documentation updated.

[greptile-apps]

Greptile Summary

This PR forward-ports the cellnet bye-message auth-bypass fix from the 2.8 branch to main, and strengthens the guard by comparing the message DESTINATION header against the receiver's own FQCN rather than two sender-controlled headers. All three validate_auth_headers call sites (server, relay, admin HCI client) are updated to supply the new local_cell_fqcn parameter, and test doubles that would otherwise fail at cell.get_fqcn() are patched.

• defs.py / core_cell.py: canonical CellChannel.CELLNET and CellChannelTopic.Bye constants replace the local _CHANNEL/_TOPIC_BYE string literals; values are identical so no behavioural change.
• authenticator.py: new bypass branch triggers only when topic == Bye, channel == CELLNET, local_cell_fqcn is not None, and DESTINATION == local_cell_fqcn; fails closed (normal auth) whenever the FQCN is unknown or the destination is a different cell.
• Tests: four new cases in authenticator_test.py cover the bypass, the forwarded-bye rejection, the old TO_CELL-equals-DESTINATION forgery, and the fail-closed path; admin_api_test.py adds get_fqcn() to the fake cell stub.

Confidence Score: 5/5

Safe to merge; the change is a focused, well-tested auth-filter fix with no data-path side effects.

The bypass guard is anchored to a receiver-controlled value (local_cell_fqcn) rather than attacker-controlled headers, all three registration call sites are updated, the fail-closed default (None) is correctly handled, and four unit tests directly cover the security boundary including the old TO_CELL forgery vector.

No files require special attention; the implementation is straightforward and the test coverage directly targets the security-relevant branches.

Important Files Changed

Filename	Overview
nvflare/private/fed/authenticator.py	Adds local_cell_fqcn parameter to validate_auth_headers; bypass logic correctly compares DESTINATION to receiver's own FQCN rather than sender-controlled headers, and fails closed when FQCN is unknown.
nvflare/fuel/f3/cellnet/defs.py	Adds canonical constants CellChannel.CELLNET and CellChannelTopic.Bye, matching the former local literals removed from core_cell.py.
nvflare/fuel/f3/cellnet/core_cell.py	Replaces file-local _CHANNEL and _TOPIC_BYE string literals with the new canonical constants; string values are identical so no behavioural change.
nvflare/private/fed/server/fed_server.py	Passes local_cell_fqcn to validate_auth_headers; uses getattr(self, cell, None) guard to handle new-built test instances where cell may not be set, failing closed to None.
nvflare/private/fed/app/relay/relay.py	Threads local_cell_fqcn=my_fqcn through the relay's _validate_auth_headers wrapper and into validate_auth_headers; docstring corrected from server to relay.
nvflare/fuel/hci/client/api.py	Adds local_cell_fqcn=self.cell.get_fqcn() when registering the admin client's incoming auth filter.
tests/unit_test/private/fed/authenticator_test.py	Adds four targeted tests: legitimate bye bypass, forwarded-bye rejection, forged-TO_CELL-equals-DESTINATION rejection, and fail-closed when FQCN is None.
tests/unit_test/fuel/hci/admin_api_test.py	Adds get_fqcn() to the _FakeCell test double so the admin API's self.cell.get_fqcn() call no longer raises AttributeError.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Peer as Peer Cell (Cell.stop())
    participant Filter as incoming auth filter (validate_auth_headers)
    participant CoreCell as CoreCell
    participant PeerGoodbye as _peer_goodbye

    Peer->>Filter: "bye msg [DESTINATION=this cell, CHANNEL=cellnet.channel]"
    Filter->>Filter: "topic==Bye AND channel==CELLNET?"
    Filter->>Filter: local_cell_fqcn is not None?
    Filter->>Filter: "DESTINATION == local_cell_fqcn?"
    Filter-->>CoreCell: return None (bypass auth)
    CoreCell->>PeerGoodbye: dispatch (agent cleanup fires immediately)

    Note over Peer,PeerGoodbye: Forged / forwarded bye path
    Peer->>Filter: "bye msg [DESTINATION=other-cell, CHANNEL=cellnet.channel]"
    Filter->>Filter: "DESTINATION != local_cell_fqcn, fall through"
    Filter-->>CoreCell: return UNAUTHENTICATED reply

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Peer as Peer Cell (Cell.stop())
    participant Filter as incoming auth filter (validate_auth_headers)
    participant CoreCell as CoreCell
    participant PeerGoodbye as _peer_goodbye

    Peer->>Filter: "bye msg [DESTINATION=this cell, CHANNEL=cellnet.channel]"
    Filter->>Filter: "topic==Bye AND channel==CELLNET?"
    Filter->>Filter: local_cell_fqcn is not None?
    Filter->>Filter: "DESTINATION == local_cell_fqcn?"
    Filter-->>CoreCell: return None (bypass auth)
    CoreCell->>PeerGoodbye: dispatch (agent cleanup fires immediately)

    Note over Peer,PeerGoodbye: Forged / forwarded bye path
    Peer->>Filter: "bye msg [DESTINATION=other-cell, CHANNEL=cellnet.channel]"
    Filter->>Filter: "DESTINATION != local_cell_fqcn, fall through"
    Filter-->>CoreCell: return UNAUTHENTICATED reply

Loading

_{Reviews (4): Last reviewed commit: "Merge branch 'main' into fix/cellnet-bye..." | Re-trigger Greptile}

[codecov-commenter]

Codecov Report

❌ Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.75%. Comparing base (846fc79) to head (c707d24).

Files with missing lines	Patch %	Lines
nvflare/private/fed/app/relay/relay.py	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4863      +/-   ##
==========================================
+ Coverage   60.73%   60.75%   +0.01%     
==========================================
  Files         977      977              
  Lines       93135    93139       +4     
==========================================
+ Hits        56568    56589      +21     
+ Misses      36567    36550      -17     

Flag	Coverage Δ
unit-tests	60.75% <80.00%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[YuanTingHsieh]

The forward-port itself is faithful and the mechanical parts are clean: the authenticator.py block is byte-identical to the 2.8 original b4fb37c4, the constant values are preserved ("cellnet.channel"/"bye") so registered callbacks still match, there are no leftover references to the removed _CHANNEL/_TOPIC_BYE constants, imports are present, and in-scope unit tests pass.

One thing worth addressing while we're touching this guard — flagging rather than hard-blocking, since it's pre-existing in the shipped 2.8 fix, not introduced here (see the inline comment). In short: the bye auth-bypass guard checks to_cell == destination, which compares two sender-controlled headers to each other and never verifies the message actually terminates at the receiving cell, so it doesn't achieve the "direct-neighbor only" property its own comment claims. This is the right moment to tighten it rather than port the gap verbatim — and 2.8 would want the same change.

[YuanTingHsieh]

The fix itself looks good — I verified the guard end-to-end: the incoming filter runs before CoreCell's forward decision (so crafted byes addressed to other cells are rejected mid-transit), _peer_goodbye evicts only by the connection-stamped ENDPOINT prop (forged headers can't name a victim), all three validate_auth_headers call sites are covered with a fail-closed default, and 449 unit tests pass locally plus adversarial probes (bye on a non-cellnet channel, quit on the cellnet channel, case variants) all reject correctly.

One thing to fix before merge: the PR description no longer matches the code. It says the bypass is "guarded by to_cell is not None and to_cell == destination" and calls this a clean cherry-pick of b4fb37c — but ac7c79b deliberately replaced that guard with the stronger DESTINATION == local_cell_fqcn check. The PR's own test (test_validate_auth_headers_rejects_bye_forged_to_cell_matching_destination) shows why: TO_CELL and DESTINATION are both sender-controlled, and _forward rewrites TO_CELL at each hop, so the old equality also holds at the terminal hop of any forwarded message. Please update the body so the merge-commit record describes the guard that actually shipped — this is a security fix and the description is what gets read later.

Related follow-up (not this PR): since main's guard is now strictly stronger, the 2.8 branch's shipped to_cell == destination guard deserves the same strengthening back-ported.

[nvshaxie]

Thanks @YuanTingHsieh! Updated the PR description to match the shipped code: it now documents the actual guard (DESTINATION == local_cell_fqcn) instead of the old to_cell == destination, and explains why it's stronger than the 2.8 original (TO_CELL/DESTINATION are both sender-controlled and _forward rewrites TO_CELL at each hop, so the old equality also holds at a forwarded bye's terminal hop). Good catch on the merge-record accuracy.

Agreed on the follow-up — I'll back-port the same DESTINATION == local_cell_fqcn strengthening to the 2.8 branch once this lands.