diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 1587f28..d58b978 100644 --- a/ARCHITECTURE.md +++ b/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 diff --git a/CHANGELOG.md b/CHANGELOG.md index 57fee90..25481e2 100644 --- a/CHANGELOG.md +++ b/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` diff --git a/README.md b/README.md index 11fa10c..3f4e7d1 100644 --- a/README.md +++ b/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. diff --git a/src/connection.rs b/src/connection.rs index 1a22001..abb70b5 100644 --- a/src/connection.rs +++ b/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 diff --git a/src/messages/routing_activation_request.rs b/src/messages/routing_activation_request.rs index 9c6c88a..a1aaeba 100644 --- a/src/messages/routing_activation_request.rs +++ b/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 { write_u16_be(writer, self.source_address.into())?; write_u8(writer, self.activation_type.into())?; diff --git a/src/server.rs b/src/server.rs index 7d654ef..0cce9d7 100644 --- a/src/server.rs +++ b/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 { - // 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 = 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, + tester_logical_address: &mut Option, ) -> Result, 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}" diff --git a/tests/integration_test.rs b/tests/integration_test.rs index f89458b..6aedaae 100644 --- a/tests/integration_test.rs +++ b/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 { + 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(); +}