diff --git a/nvflare/fuel/f3/cellnet/core_cell.py b/nvflare/fuel/f3/cellnet/core_cell.py index 2584830749..68b7da18f0 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/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 eb3d734d21..cc8b93b45b 100644 --- a/nvflare/private/fed/authenticator.py +++ b/nvflare/private/fed/authenticator.py @@ -355,6 +355,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. @@ -364,6 +365,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: """ @@ -379,6 +383,29 @@ 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 + # 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 local_cell_fqcn is not None + and destination == local_cell_fqcn + ): + logger.debug(f"skip direct-neighbor cellnet bye {topic=} {channel=} {destination=}") + return None + client_name = message.get_header(CellMessageHeaderKeys.CLIENT_NAME) err_text = f"unauthenticated msg ({channel=} {topic=}) received from {origin}" if not client_name: diff --git a/nvflare/private/fed/server/fed_server.py b/nvflare/private/fed/server/fed_server.py index c3b03650f3..6cc7ede923 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 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 diff --git a/tests/unit_test/private/fed/authenticator_test.py b/tests/unit_test/private/fed/authenticator_test.py index c72fb4eb05..6c48f9f3f6 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 @@ -186,3 +186,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