fix(server): carry the tester's logical address into ClientConnectionInfo

ARCHITECTURE.md

@@ -573,9 +573,14 @@ choosing the crate; the mechanics are here. No TLS; no unsolicited UDP vehicle a
 binds TCP alone — and only the broadcast `0x0001` form, since `Payload::decode`
 discards the EID/VIN the directed forms name); the server's accept loop serves
 one TCP connection at a time; entity status and vehicle identification requests
-over TCP are silently dropped; `ClientConnectionInfo::logical_address` is
-hard-coded to `0x0000` because the server tracks no per-connection state; the
-handler passed to `Server::new` is not validated.
+over TCP are silently dropped; the handler passed to `Server::new` is not
+validated.
+
+`RoutingActivationRequest::encode` omits the optional vehicle-manufacturer
+field when it is `None`, writing 7 bytes instead of 11. That is what the
+optionality means, and every golden vector agrees — but no vector exercises a
+peer that requires the long form, so if one turns up, this is the function to
+look at (`src/messages/routing_activation_request.rs`).
 
 A failed `accept()` no longer panics the server task — as of 0.4.0 both the TCP
 accept loop and the UDP responder log the error, sleep briefly, and continue, so

CHANGELOG.md

@@ -23,6 +23,14 @@ changed rather than what was announced at the time.
   org-wide reusable workflow, matching `uds_protocol` and
   `automotive_wire_codec`. There is no release workflow in this repo.
 
+### Fixed
+
+- `ClientConnectionInfo::logical_address` carries the address the tester
+  activated routing with, instead of always being `0x0000`. A handler can now
+  tell which tester is asking, and the default `alive_check` answers with the
+  right source address. It stays `0x0000` before activation and after an
+  activation the handler denied.
+
 ### Changed
 
 - docs.rs now builds with all features, so the `client`, `server` and `codec`

README.md

@@ -25,7 +25,6 @@ case, within these bounds:
   `DiagnosticMessage` that arrives while the client is waiting for the
   acknowledgement is dropped, so a peer that answers first appears never to
   answer at all.
-- **`ClientConnectionInfo::logical_address` is not yet tracked per connection.**
 
 [`ARCHITECTURE.md`](ARCHITECTURE.md) §7 has the mechanics behind each of these,
 and the deferred work around them.

src/connection.rs

@@ -85,8 +85,11 @@ pub trait Connector {
     ) -> Result<(OwnedReadHalf, OwnedWriteHalf), crate::Error>;
 }
 
-// TODO: Move this to a config file
-/// Buffer size for the TCP socket
+/// Send and receive buffer size requested on the diagnostic TCP socket.
+///
+/// Fixed rather than configurable: `Connector` is the seam for a caller that
+/// needs different socket options, so a `ConnectorSocket` knob would duplicate
+/// it. Implement [`Connector`] to size these yourself.
 const BUFFER_SIZE: u32 = 1024 * 64;
 
 /// ISO 13400-2:2012 Connection to the gateway node via port 13400

src/messages/routing_activation_request.rs

@@ -145,7 +145,6 @@ impl Encode for RoutingActivationRequest {
     ///
     /// # Errors
     /// Returns [`MessageError::Io`] if the writer fails.
-    // TODO: Investigate if we should write the optional vehicle manufacturer specific data if none
     fn encode(&self, writer: &mut impl embedded_io::Write) -> Result<usize, MessageError> {
         write_u16_be(writer, self.source_address.into())?;
         write_u8(writer, self.activation_type.into())?;

src/server.rs

@@ -11,7 +11,8 @@ use crate::{
     messages::{
         Decode, DiagnosticMessage, DiagnosticPowerModeCode, Encode, FurtherActionRequired, Message,
         OwnedMessage, OwnedPayload, Payload, PayloadType, ProtocolVersion,
-        RoutingActivationRequest, VehicleIdentificationResponse, VinGidSyncStatus,
+        RoutingActivationRequest, RoutingActivationResponseCode, VehicleIdentificationResponse,
+        VinGidSyncStatus,
     },
 };
 use async_trait::async_trait;
@@ -48,14 +49,16 @@ const ACCEPT_ERROR_BACKOFF: Duration = Duration::from_millis(100);
 pub struct ClientConnectionInfo {
     /// IP address of the tester's end of the TCP connection.
     pub ip_address: IpAddr,
-    /// Intended to carry the logical address the tester identified itself with
-    /// during routing activation.
+    /// The logical address the tester identified itself with when it activated
+    /// routing on this connection.
     ///
-    /// **Currently always `0x0000`.** The server does not yet track per-connection
-    /// state, so the tester's routing activation source address is never
-    /// propagated here and this field is hard-coded. As a consequence the default
-    /// [`ServerConnectionHandler::alive_check`] implementation answers with source
-    /// address `0x0000`. Do not treat this field as carrying real data.
+    /// `0x0000` until routing activation succeeds, and after an activation the
+    /// handler denied. That sentinel is unambiguous: ISO 13400-2 reserves
+    /// everything below [`LogicalAddress::MIN_CLIENT_ADDRESS`], so `0x0000` can
+    /// never be a tester's own address.
+    ///
+    /// The UDP identification path has no connection and therefore no routing
+    /// activation to learn an address from, so it always reports `0x0000`.
     pub logical_address: LogicalAddress,
 }
 
@@ -285,7 +288,6 @@ where
     /// # Errors
     /// Returns an [`Error`] if the server cannot be initialized
     pub fn new(connection_handler: T) -> Result<Self, Error> {
-        // TODO: Validate the provided handler
         Ok(Server {
             connection_handler: Arc::new(connection_handler),
             active_connections: AtomicUsize::new(0),
@@ -327,10 +329,6 @@ where
     /// # Errors
     /// Returns an [`Error`] if the TCP listener cannot be bound.
     pub async fn run_server(&self) -> Result<(), Error> {
-        // TODO: unsolicited Vehicle Announcement over UDP at power-on. Answering
-        // vehicle identification requests already exists as
-        // `run_udp_responder`, which the caller drives with its own socket.
-
         let tcp_listener = TcpListener::bind(("0.0.0.0", TCP_PORT)).await?;
         self.run_server_with_listener(tcp_listener).await
     }
@@ -583,11 +581,21 @@ where
         let mut read_stream = FramedRead::new(rx, MessageCodec::new());
         let mut write_sink = FramedWrite::new(tx, MessageCodec::new());
 
+        // Learned from the routing activation this connection carries, and only
+        // once the handler accepts it. Per-connection rather than per-server:
+        // routing activation applies to the socket it arrived on.
+        let mut tester_logical_address: Option<LogicalAddress> = None;
+
         loop {
             match read_stream.next().await {
                 Some(Ok(message)) => {
                     if let Some(response) = self
-                        .handle_client_message(client_socket_addr, message, &mut write_sink)
+                        .handle_client_message(
+                            client_socket_addr,
+                            message,
+                            &mut write_sink,
+                            &mut tester_logical_address,
+                        )
                         .await?
                     {
                         write_sink.send(&response).await?;
@@ -620,15 +628,14 @@ where
         client_socket_addr: SocketAddr,
         request_message: OwnedMessage,
         write_sink: &mut FramedWrite<W, MessageCodec>,
+        tester_logical_address: &mut Option<LogicalAddress>,
     ) -> Result<Option<OwnedMessage>, Error>
     where
         W: tokio::io::AsyncWrite + Unpin + Send,
     {
-        // TODO: Need to handle active sockets by adding clients to a map
-        // client count should come from that map, as well as the logical address missing below
         let connection_info = ClientConnectionInfo {
             ip_address: client_socket_addr.ip(),
-            logical_address: LogicalAddress(0x0000), // TODO fix this constant
+            logical_address: tester_logical_address.unwrap_or(LogicalAddress(0x0000)),
         };
 
         match request_message.payload {
@@ -650,11 +657,24 @@ where
                 );
                 Ok(None)
             }
-            OwnedPayload::RoutingActivationRequest(request) => self
-                .connection_handler
-                .routing_activation(&request)
-                .await
-                .map(Some),
+            OwnedPayload::RoutingActivationRequest(request) => {
+                let response = self.connection_handler.routing_activation(&request).await?;
+                // Record the address only once the handler has accepted the
+                // activation. A denied tester is not activated, so attributing
+                // its claimed address to the connection would report an
+                // identity the entity refused.
+                if let OwnedPayload::RoutingActivationResponse(activation_response) =
+                    &response.payload
+                    && matches!(
+                        activation_response.routing_activation_response_code,
+                        RoutingActivationResponseCode::RoutingSuccessfullyActivated
+                            | RoutingActivationResponseCode::RoutingSuccessfullyActivatedConfirmationRequired
+                    )
+                {
+                    *tester_logical_address = Some(request.source_address);
+                }
+                Ok(Some(response))
+            }
             OwnedPayload::RoutingActivationResponse(_routing_activation_response) => {
                 warn!(
                     "Client sent a server-role RoutingActivationResponse message, source: {client_socket_addr}"

tests/integration_test.rs

@@ -1666,3 +1666,134 @@ async fn a_failed_connection_reports_the_connect_error_not_socket_not_bound() {
         "expected the underlying connect error to survive, got: {error:?}"
     );
 }
+
+/// Handler whose routing activation either succeeds or is denied, so a test can
+/// pin what the server does with the tester's claimed address in both cases.
+/// Everything else, including `alive_check`, is the trait default.
+struct ActivationOutcomeHandler {
+    accept: bool,
+}
+
+#[async_trait]
+impl ServerConnectionHandler for ActivationOutcomeHandler {
+    fn get_vin(&self) -> [u8; 17] {
+        [0x00; 17]
+    }
+
+    fn get_logical_address(&self) -> LogicalAddress {
+        SERVER_LOGICAL_ADDRESS
+    }
+
+    fn get_entity_id(&self) -> [u8; 6] {
+        [0x00; 6]
+    }
+
+    fn get_group_id(&self) -> Option<[u8; 6]> {
+        None
+    }
+
+    async fn routing_activation(
+        &self,
+        request: &RoutingActivationRequest,
+    ) -> Result<OwnedMessage, Error> {
+        let code = if self.accept {
+            RoutingActivationResponseCode::RoutingSuccessfullyActivated
+        } else {
+            RoutingActivationResponseCode::DeniedUnknownSourceAddress
+        };
+        Ok(OwnedMessage::routing_activation_response(
+            self.protocol_version(),
+            request.source_address,
+            self.get_logical_address(),
+            code,
+            [0; 4],
+            None,
+        ))
+    }
+
+    async fn diagnostic_message(
+        &self,
+        _message: &DiagnosticMessage<'_>,
+        _responses: &mut dyn ResponseWriter,
+    ) -> Result<(), Error> {
+        Ok(())
+    }
+}
+
+/// Read the source address out of an alive check response, which the default
+/// `alive_check` fills from `ClientConnectionInfo::logical_address` — so it is
+/// the observable end of what the server learned about the tester.
+fn alive_check_source_address(message: &OwnedMessage) -> LogicalAddress {
+    match &message.payload {
+        OwnedPayload::AliveCheckResponse(response) => response.source_address,
+        other => panic!("expected an alive check response, got {other:?}"),
+    }
+}
+
+/// The server must carry the logical address a tester activated routing with
+/// into `ClientConnectionInfo`, so a handler can tell which peer is asking.
+/// It reported `0x0000` for every connection before this was tracked.
+#[tokio::test]
+async fn alive_check_reports_the_activated_tester_logical_address() {
+    let (server_addr, accept_loop) =
+        start_server_with(ActivationOutcomeHandler { accept: true }).await;
+    let mut stream = with_timeout("connect", TcpStream::connect(server_addr))
+        .await
+        .expect("connect to test server");
+    let (rx, tx) = stream.split();
+    let mut reader = FramedRead::new(rx, MessageCodec::new());
+    let mut writer = FramedWrite::new(tx, MessageCodec::new());
+
+    send_routing_activation(&mut writer, CLIENT_LOGICAL_ADDRESS).await;
+    let _activation = read_message(&mut reader).await;
+
+    with_timeout(
+        "send alive check",
+        writer.send(&OwnedMessage::alive_check_request(ProtocolVersion::V2012)),
+    )
+    .await
+    .expect("send alive check");
+    let response = read_message(&mut reader).await;
+
+    assert_eq!(
+        alive_check_source_address(&response),
+        CLIENT_LOGICAL_ADDRESS,
+        "the alive check must name the address the tester activated with"
+    );
+
+    accept_loop.abort();
+}
+
+/// A denied activation leaves the tester unactivated, so its claimed address
+/// must not be attributed to the connection — reporting it would name an
+/// identity the entity refused.
+#[tokio::test]
+async fn a_denied_activation_does_not_record_the_testers_address() {
+    let (server_addr, accept_loop) =
+        start_server_with(ActivationOutcomeHandler { accept: false }).await;
+    let mut stream = with_timeout("connect", TcpStream::connect(server_addr))
+        .await
+        .expect("connect to test server");
+    let (rx, tx) = stream.split();
+    let mut reader = FramedRead::new(rx, MessageCodec::new());
+    let mut writer = FramedWrite::new(tx, MessageCodec::new());
+
+    send_routing_activation(&mut writer, CLIENT_LOGICAL_ADDRESS).await;
+    let _denial = read_message(&mut reader).await;
+
+    with_timeout(
+        "send alive check",
+        writer.send(&OwnedMessage::alive_check_request(ProtocolVersion::V2012)),
+    )
+    .await
+    .expect("send alive check");
+    let response = read_message(&mut reader).await;
+
+    assert_eq!(
+        alive_check_source_address(&response),
+        LogicalAddress(0x0000),
+        "a refused tester must not have its claimed address reported back"
+    );
+
+    accept_loop.abort();
+}