From 9ca5276f26e6a588ab4308e89a1e198961bbe1dc Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Tue, 7 Jul 2026 13:39:43 +0000 Subject: [PATCH 1/3] [main] Skip auth filter for cellnet protocol-level bye messages Forward-port of #4569 (which fixed NVBug 6154369 on the 2.8 branch) to main. The cellnet protocol-level bye auth-bypass fix landed on 2.8 only; on main, validate_auth_headers() still exempts only Register/Challenge but 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 executing immediately). Clean cherry-pick of #4569: - defs.py: add CellChannel.CELLNET + CellChannelTopic.Bye constants - core_cell.py: use the canonical constants - authenticator.py: bypass auth for the direct-neighbor bye, guarded by `to_cell is not None and to_cell == destination` to prevent forged-bye attacks Cherry-picked from commit b4fb37c4 on the 2.8 branch. --- nvflare/fuel/f3/cellnet/core_cell.py | 14 +++++++------- nvflare/fuel/f3/cellnet/defs.py | 2 ++ nvflare/private/fed/authenticator.py | 21 +++++++++++++++++++++ 3 files changed, 30 insertions(+), 7 deletions(-) diff --git a/nvflare/fuel/f3/cellnet/core_cell.py b/nvflare/fuel/f3/cellnet/core_cell.py index 52fdb75396..e242ffd90c 100644 --- a/nvflare/fuel/f3/cellnet/core_cell.py +++ b/nvflare/fuel/f3/cellnet/core_cell.py @@ -28,6 +28,8 @@ from nvflare.fuel.f3.cellnet.defs import ( AbortRun, AuthenticationError, + CellChannel, + CellChannelTopic, CellPropertyKey, InvalidRequest, InvalidSession, @@ -60,9 +62,7 @@ from nvflare.fuel.utils.log_utils import get_obj_logger from nvflare.security.logging import secure_format_exception, secure_format_traceback -_CHANNEL = "cellnet.channel" _TOPIC_BULK = "bulk" -_TOPIC_BYE = "bye" _SM_CHANNEL = "credential_manager" _SM_TOPIC = "key_exchange" @@ -197,7 +197,7 @@ def send(self): tms = [m.to_dict() for m in messages_to_send] bulk_msg = Message(None, tms) send_errs = self.cell.fire_and_forget( - channel=_CHANNEL, topic=_TOPIC_BULK, targets=[self.target], message=bulk_msg + channel=CellChannel.CELLNET, topic=_TOPIC_BULK, targets=[self.target], message=bulk_msg ) if send_errs[self.target]: log_messaging_error( @@ -497,8 +497,8 @@ def __init__( self.adhoc_connector_lock = threading.Lock() self.root_change_lock = threading.Lock() - self.register_request_cb(channel=_CHANNEL, topic=_TOPIC_BULK, cb=self._receive_bulk_message) - self.register_request_cb(channel=_CHANNEL, topic=_TOPIC_BYE, cb=self._peer_goodbye) + self.register_request_cb(channel=CellChannel.CELLNET, topic=_TOPIC_BULK, cb=self._receive_bulk_message) + self.register_request_cb(channel=CellChannel.CELLNET, topic=CellChannelTopic.Bye, cb=self._peer_goodbye) self.cleanup_waiter = None self.msg_stats_pool = StatsPoolManager.add_time_hist_pool( @@ -974,8 +974,8 @@ def stop(self): targets = [peer_name for peer_name in self.agents.keys()] self.logger.debug(f"broadcasting goodbye to {targets}") self.broadcast_request( - channel=_CHANNEL, - topic=_TOPIC_BYE, + channel=CellChannel.CELLNET, + topic=CellChannelTopic.Bye, targets=targets, request=Message(), timeout=0.5, diff --git a/nvflare/fuel/f3/cellnet/defs.py b/nvflare/fuel/f3/cellnet/defs.py index ced7c817ed..45c1ce61a1 100644 --- a/nvflare/fuel/f3/cellnet/defs.py +++ b/nvflare/fuel/f3/cellnet/defs.py @@ -167,12 +167,14 @@ class CellChannel: RETURN_ONLY = "return_only" EDGE_REQUEST = "edge_request" HCI = "hci_channel" + CELLNET = "cellnet.channel" class CellChannelTopic: Challenge = "challenge" Register = "register" + Bye = "bye" Quit = "quit" GET_TASK = "get_task" SUBMIT_RESULT = "submit_result" diff --git a/nvflare/private/fed/authenticator.py b/nvflare/private/fed/authenticator.py index dcef96eaa2..70a86f285b 100644 --- a/nvflare/private/fed/authenticator.py +++ b/nvflare/private/fed/authenticator.py @@ -372,6 +372,27 @@ def validate_auth_headers( logger.debug(f"skip special message {topic=} {channel=}") return None + # Cellnet protocol-level goodbye is broadcast by Cell.stop() with an empty + # Message() that carries no FL-level auth headers. Only bypass auth when the + # message is for the immediate (direct-neighbor) recipient, i.e. TO_CELL == + # DESTINATION. If it would be forwarded onward (TO_CELL != DESTINATION), a + # remote unauthenticated peer could craft a bye with DESTINATION pointing + # at a downstream cell whose _peer_goodbye would then drop the immediate + # endpoint (the server/relay) from its agents dict — fall through to the + # normal auth check in that case. The explicit ``to_cell is not None`` + # check rejects crafted messages that omit both headers (None == None + # would otherwise pass). + to_cell = message.get_header(MessageHeaderKey.TO_CELL) + destination = message.get_header(MessageHeaderKey.DESTINATION) + if ( + topic == CellChannelTopic.Bye + and channel == CellChannel.CELLNET + and to_cell is not None + and to_cell == destination + ): + logger.debug(f"skip direct-neighbor cellnet bye {topic=} {channel=}") + return None + client_name = message.get_header(CellMessageHeaderKeys.CLIENT_NAME) err_text = f"unauthenticated msg ({channel=} {topic=}) received from {origin}" if not client_name: From ac7c79be4362bedbc131f3efe9f0f3607af34b3b Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Thu, 9 Jul 2026 02:05:47 +0000 Subject: [PATCH 2/3] Scope the cellnet bye auth bypass to byes terminating at this cell MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses @YuanTingHsieh's review on #4863: the bye auth-bypass guard compared two sender-controlled headers (TO_CELL == DESTINATION) and never verified the message actually terminates at the receiving cell, so it did not enforce the "direct-neighbor only" property its comment claimed. A peer that can inject into the incoming pipeline without a valid FL token could craft `topic=bye, TO_CELL=X, DESTINATION=X` for another cell X; the guard passed it, and since DESTINATION != this cell's FQCN the message was then FORWARDED to X, whose _peer_goodbye evicts its upstream agent — an unauthenticated peer-eviction / routing disruption. Fix: bypass auth only when DESTINATION == this cell's own FQCN. The in-filter runs before CoreCell's forward decision (`if destination != my_fqcn: forward`), so a bye addressed to this cell is handled locally and never forwarded, while a bye 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 this never breaks a real bye. 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). When it is None the bypass fails closed. Added unit tests: a bye terminating here is bypassed; a forwarded/forged bye (incl. one with TO_CELL == DESTINATION pointing at another cell) is rejected; no local FQCN fails closed. Co-Authored-By: Claude Fable 5 --- nvflare/fuel/hci/client/api.py | 1 + nvflare/private/fed/app/relay/relay.py | 10 ++-- nvflare/private/fed/authenticator.py | 30 ++++++----- nvflare/private/fed/server/fed_server.py | 1 + .../private/fed/authenticator_test.py | 53 ++++++++++++++++++- 5 files changed, 79 insertions(+), 16 deletions(-) diff --git a/nvflare/fuel/hci/client/api.py b/nvflare/fuel/hci/client/api.py index 350075acb9..e71772678a 100644 --- a/nvflare/fuel/hci/client/api.py +++ b/nvflare/fuel/hci/client/api.py @@ -459,6 +459,7 @@ def connect(self, timeout=None): cb=validate_auth_headers, token_verifier=token_verifier, logger=self.logger, + local_cell_fqcn=self.cell.get_fqcn(), ) self.debug(f"Successfully authenticated to {self.server_identity}: {token=} {ssid=}") diff --git a/nvflare/private/fed/app/relay/relay.py b/nvflare/private/fed/app/relay/relay.py index d6115c4352..07be0e1cd9 100644 --- a/nvflare/private/fed/app/relay/relay.py +++ b/nvflare/private/fed/app/relay/relay.py @@ -221,6 +221,7 @@ def main(args): cb=_validate_auth_headers, token_verifier=token_verifier, logger=logger, + local_cell_fqcn=my_fqcn, ) logger.info(f"Successfully authenticated to {server_identity}: {token=} {ssid=}") @@ -232,13 +233,16 @@ def main(args): logger.info(f"Relay {my_fqcn} stopped.") -def _validate_auth_headers(message: CellMessage, token_verifier: TokenVerifier, logger): - """Validate auth headers from messages that go through the server. +def _validate_auth_headers(message: CellMessage, token_verifier: TokenVerifier, logger, local_cell_fqcn=None): + """Validate auth headers from messages that go through the relay. Args: message: the message to validate + token_verifier: verifier for the token and signature + logger: logger + local_cell_fqcn: FQCN of this relay's cell, used to scope the cellnet bye auth bypass Returns: """ - return validate_auth_headers(message, token_verifier, logger) + return validate_auth_headers(message, token_verifier, logger, local_cell_fqcn=local_cell_fqcn) if __name__ == "__main__": diff --git a/nvflare/private/fed/authenticator.py b/nvflare/private/fed/authenticator.py index 70a86f285b..ae184323da 100644 --- a/nvflare/private/fed/authenticator.py +++ b/nvflare/private/fed/authenticator.py @@ -348,6 +348,7 @@ def validate_auth_headers( token_verifier: TokenVerifier, logger, client_fqcn_resolver: Optional[Callable[[str, str], Optional[str]]] = None, + local_cell_fqcn: Optional[str] = None, ): """Validate auth headers from messages that go through the server. @@ -357,6 +358,9 @@ def validate_auth_headers( client_fqcn_resolver: optional resolver used to bind a client token to its registered CellNet origin. Return None only when the token/name cannot be resolved; return MISSING_CLIENT_FQCN for a registered client with no stored origin so validation fails closed. + local_cell_fqcn: the FQCN of the cell that owns this auth filter. Used to bypass auth ONLY for a + cellnet ``bye`` that actually terminates at this cell (DESTINATION == local_cell_fqcn). When None, + the bye bypass never triggers and byes fall through to the normal auth check. Returns: """ @@ -374,23 +378,25 @@ def validate_auth_headers( # Cellnet protocol-level goodbye is broadcast by Cell.stop() with an empty # Message() that carries no FL-level auth headers. Only bypass auth when the - # message is for the immediate (direct-neighbor) recipient, i.e. TO_CELL == - # DESTINATION. If it would be forwarded onward (TO_CELL != DESTINATION), a - # remote unauthenticated peer could craft a bye with DESTINATION pointing - # at a downstream cell whose _peer_goodbye would then drop the immediate - # endpoint (the server/relay) from its agents dict — fall through to the - # normal auth check in that case. The explicit ``to_cell is not None`` - # check rejects crafted messages that omit both headers (None == None - # would otherwise pass). - to_cell = message.get_header(MessageHeaderKey.TO_CELL) + # bye actually TERMINATES at this cell, i.e. DESTINATION == this cell's own + # FQCN. This is the true "direct-neighbor" property: the in-filter runs + # before CoreCell's forward decision (``if destination != my_fqcn: forward``), + # so a bye whose DESTINATION is this cell is handled locally by + # _peer_goodbye and never forwarded. A crafted bye that names some OTHER + # cell as DESTINATION (to have this cell forward it and evict that cell's + # upstream agent) has DESTINATION != local_cell_fqcn and falls through to the + # normal auth check. Comparing DESTINATION to a sender-controlled TO_CELL + # header would NOT give this guarantee, since both are attacker-controlled; + # only comparing against the receiver's own FQCN does. When local_cell_fqcn + # is unknown (None), the bypass never triggers (byes get normal auth). destination = message.get_header(MessageHeaderKey.DESTINATION) if ( topic == CellChannelTopic.Bye and channel == CellChannel.CELLNET - and to_cell is not None - and to_cell == destination + and local_cell_fqcn is not None + and destination == local_cell_fqcn ): - logger.debug(f"skip direct-neighbor cellnet bye {topic=} {channel=}") + logger.debug(f"skip direct-neighbor cellnet bye {topic=} {channel=} {destination=}") return None client_name = message.get_header(CellMessageHeaderKeys.CLIENT_NAME) diff --git a/nvflare/private/fed/server/fed_server.py b/nvflare/private/fed/server/fed_server.py index c3b03650f3..91a55bc466 100644 --- a/nvflare/private/fed/server/fed_server.py +++ b/nvflare/private/fed/server/fed_server.py @@ -454,6 +454,7 @@ def _validate_auth_headers(self, message: Message): token_verifier=token_verifier, logger=self.logger, client_fqcn_resolver=self._resolve_client_fqcn_for_auth, + local_cell_fqcn=self.cell.get_fqcn() if self.cell else None, ) if not reply: self._strip_peer_transit_reply_auth_headers(message) diff --git a/tests/unit_test/private/fed/authenticator_test.py b/tests/unit_test/private/fed/authenticator_test.py index e6ffc4d10d..ccdb952ca3 100644 --- a/tests/unit_test/private/fed/authenticator_test.py +++ b/tests/unit_test/private/fed/authenticator_test.py @@ -17,7 +17,7 @@ import pytest from nvflare.apis.fl_constant import ServerCommandNames -from nvflare.fuel.f3.cellnet.defs import CellChannel, MessageHeaderKey, ReturnCode +from nvflare.fuel.f3.cellnet.defs import CellChannel, CellChannelTopic, MessageHeaderKey, ReturnCode from nvflare.fuel.f3.message import Message from nvflare.fuel.f3.streaming.stream_const import STREAM_CHANNEL, STREAM_DATA_TOPIC from nvflare.private.defs import CellMessageHeaderKeys @@ -143,3 +143,54 @@ def test_validate_auth_headers_rejects_registered_token_with_missing_origin_bind def test_validate_auth_headers_keeps_compatibility_when_origin_cannot_be_resolved(): assert _validate("site-b", lambda _client_name, _token: None) is None + + +def _make_bye_message(destination, to_cell=None): + # Cellnet Cell.stop() broadcasts an empty Message() carrying no FL auth headers; + # the transport stamps DESTINATION/TO_CELL. A crafted bye can set them to anything. + headers = { + MessageHeaderKey.CHANNEL: CellChannel.CELLNET, + MessageHeaderKey.TOPIC: CellChannelTopic.Bye, + MessageHeaderKey.ORIGIN: "attacker", + MessageHeaderKey.DESTINATION: destination, + } + if to_cell is not None: + headers[MessageHeaderKey.TO_CELL] = to_cell + return Message(headers=headers) + + +def _validate_bye(destination, local_cell_fqcn, to_cell=None): + return validate_auth_headers( + message=_make_bye_message(destination, to_cell=to_cell), + token_verifier=_TokenVerifier(), + logger=logging.getLogger(__name__), + local_cell_fqcn=local_cell_fqcn, + ) + + +def test_validate_auth_headers_bypasses_bye_terminating_at_this_cell(): + # A legitimate bye is a direct-neighbor message: DESTINATION == the receiving cell's own FQCN. + assert _validate_bye(destination="server", local_cell_fqcn="server") is None + + +def test_validate_auth_headers_rejects_forwarded_bye_for_other_cell(): + # A crafted bye that names another cell as DESTINATION (so this cell would forward it and evict + # that cell's upstream agent) must NOT bypass auth — DESTINATION != this cell's FQCN. + reply = _validate_bye(destination="site-1", local_cell_fqcn="server") + + assert reply.get_header(MessageHeaderKey.RETURN_CODE) == ReturnCode.UNAUTHENTICATED + + +def test_validate_auth_headers_rejects_bye_forged_to_cell_matching_destination(): + # The old guard compared two sender-controlled headers (TO_CELL == DESTINATION); an attacker can + # satisfy that while DESTINATION still points at another cell. The FQCN-based guard rejects it. + reply = _validate_bye(destination="site-1", local_cell_fqcn="server", to_cell="site-1") + + assert reply.get_header(MessageHeaderKey.RETURN_CODE) == ReturnCode.UNAUTHENTICATED + + +def test_validate_auth_headers_does_not_bypass_bye_when_local_fqcn_unknown(): + # Without a known local FQCN the bypass must not trigger (fail closed). + reply = _validate_bye(destination="server", local_cell_fqcn=None) + + assert reply.get_header(MessageHeaderKey.RETURN_CODE) == ReturnCode.UNAUTHENTICATED From 96c07ebb39a29d3067babd63bd9e2ac8ca5f4eb6 Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Thu, 9 Jul 2026 02:18:57 +0000 Subject: [PATCH 3/3] Fix unit tests broken by the local_cell_fqcn addition Two existing test doubles didn't provide the cell FQCN that validate_auth_headers now reads: - fed_server.py: use getattr(self, "cell", None) so the auth filter is robust when the server cell isn't set yet (authn_test builds a FederatedServer via __new__ without __init__, so it has no .cell attribute). - admin_api_test.py: give the _FakeCell a get_fqcn() (the real Cell has one). Co-Authored-By: Claude Fable 5 --- nvflare/private/fed/server/fed_server.py | 2 +- tests/unit_test/fuel/hci/admin_api_test.py | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/nvflare/private/fed/server/fed_server.py b/nvflare/private/fed/server/fed_server.py index 91a55bc466..6cc7ede923 100644 --- a/nvflare/private/fed/server/fed_server.py +++ b/nvflare/private/fed/server/fed_server.py @@ -454,7 +454,7 @@ def _validate_auth_headers(self, message: Message): token_verifier=token_verifier, logger=self.logger, client_fqcn_resolver=self._resolve_client_fqcn_for_auth, - local_cell_fqcn=self.cell.get_fqcn() if self.cell else None, + local_cell_fqcn=self.cell.get_fqcn() if getattr(self, "cell", None) else None, ) if not reply: self._strip_peer_transit_reply_auth_headers(message) diff --git a/tests/unit_test/fuel/hci/admin_api_test.py b/tests/unit_test/fuel/hci/admin_api_test.py index 9ad4d7e8fd..e230f01b6b 100644 --- a/tests/unit_test/fuel/hci/admin_api_test.py +++ b/tests/unit_test/fuel/hci/admin_api_test.py @@ -32,6 +32,9 @@ def __init__(self, **kwargs): def register_request_cb(self, **kwargs): pass + def get_fqcn(self): + return "admin@nvidia.com" + def start(self): pass