From 9c347c1b4233d5943f927fd68897743565754443 Mon Sep 17 00:00:00 2001 From: iBinh Date: Tue, 25 Aug 2026 15:20:31 +0700 Subject: [PATCH 1/2] fix(onvif): answer digest challenges, retry SOAP 1.1, read nested values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A Hikvision dome failed every probe with "GetCapabilities: empty SOAP body" — HTTP 200, zero bytes, no fault to explain itself. Three separate causes, each of which produces that same unhelpful result. The client carried no credentials on its handler, so a 401 challenge was never answered. All it sent was a preemptive Basic header, and Hikvision wants Digest for ONVIF; the request was simply unauthorized and the camera declined to say so. Each (host, user) now gets an HttpClient whose handler holds the credentials, which is what lets HttpClient satisfy Basic or Digest as the camera asks. PreAuthenticate stays off so the camera states its terms first, and the preemptive Basic header stays for onvif_simple_server, which enforces Basic at the transport and never challenges. Every request went out as SOAP 1.2 only. Several firmwares are built for 1.1 and answer 1.2 with nothing at all. A response with no usable envelope is now retried once as SOAP 1.1 — different content type, action moved into its own SOAPAction header. A fault counts as an answer, so a camera that says why it refused is not asked twice. And GetStreamUri read Uri as a direct child of the response, which only matches the flatter shape onvif_simple_server sends. The spec nests it as MediaUri/Uri, so a compliant camera looked like it had no stream at all; SetPreset's token is nested the same way on some firmwares. Both now search the response instead of assuming its depth. When both versions come back empty the error names what to check — ONVIF switched off on the camera, or an account without ONVIF rights, which is what an empty body from a working camera almost always means — and the response is logged at debug level. Covered by a stub camera over a real socket, so the client's own HTTP stack does the work: the 401 handshake, the content types and the SOAPAction header are all exercised rather than mocked. --- .../Onvif/SoapOnvifClient.cs | 149 ++++++++++++++---- .../Onvif/SoapOnvifClientInteropTests.cs | 147 +++++++++++++++++ .../Onvif/StubCamera.cs | 133 ++++++++++++++++ 3 files changed, 400 insertions(+), 29 deletions(-) create mode 100644 tests/OpenIPC.Viewer.Devices.Tests/Onvif/SoapOnvifClientInteropTests.cs create mode 100644 tests/OpenIPC.Viewer.Devices.Tests/Onvif/StubCamera.cs diff --git a/src/OpenIPC.Viewer.Devices/Onvif/SoapOnvifClient.cs b/src/OpenIPC.Viewer.Devices/Onvif/SoapOnvifClient.cs index 1b408f7..52962e4 100644 --- a/src/OpenIPC.Viewer.Devices/Onvif/SoapOnvifClient.cs +++ b/src/OpenIPC.Viewer.Devices/Onvif/SoapOnvifClient.cs @@ -3,6 +3,7 @@ using System.Collections.Generic; using System.Globalization; using System.Linq; +using System.Net; using System.Net.Http; using System.Net.Http.Headers; using System.Security; @@ -25,14 +26,18 @@ namespace OpenIPC.Viewer.Devices.Onvif; /// the "XmlType reflection error" on Onvif.Core.Client.Common.DeviceEntity. /// Same contract, so the swap is one DI registration. /// -/// Auth mirrors the old builder: preemptive HTTP Basic (OpenIPC's -/// onvif_simple_server enforces it at the transport) plus a WS-Security -/// UsernameToken password digest. GetSystemDateAndTime (unauthenticated) yields -/// the camera clock offset the digest's Created stamp needs; it's cached per host. +/// Auth is three things at once, because cameras disagree about which they +/// want: preemptive HTTP Basic (OpenIPC's onvif_simple_server enforces it at +/// the transport and never challenges), HTTP Digest answered on a 401 by an +/// whose handler carries the credentials, and a +/// WS-Security UsernameToken password digest in the envelope. +/// GetSystemDateAndTime (unauthenticated) yields the camera clock offset the +/// token's Created stamp needs; it's cached per host. /// public sealed class SoapOnvifClient : IOnvifClient { private const string Soap = "http://www.w3.org/2003/05/soap-envelope"; + private const string Soap11 = "http://schemas.xmlsoap.org/soap/envelope/"; private const string Tds = "http://www.onvif.org/ver10/device/wsdl"; private const string Trt = "http://www.onvif.org/ver10/media/wsdl"; private const string Tptz = "http://www.onvif.org/ver20/ptz/wsdl"; @@ -52,19 +57,43 @@ public sealed class SoapOnvifClient : IOnvifClient // first authed call, refreshed on an auth fault. private readonly ConcurrentDictionary _shiftByHost = new(StringComparer.OrdinalIgnoreCase); + // One client per (host, user). A handler that carries credentials is what + // lets HttpClient answer a 401 challenge on its own, which is the only way + // to satisfy a camera that asks for Digest rather than Basic. + private readonly ConcurrentDictionary _authedClients = new(StringComparer.Ordinal); + public SoapOnvifClient(ILogger logger) { _logger = logger; + _http = NewClient(credentials: null); + } + + private static HttpClient NewClient(NetworkCredential? credentials) + { // onvif_simple_server is CGI-style: one request per connection, then it // closes the socket. Disable pooling so we never reuse a dead socket. - _http = new HttpClient(new SocketsHttpHandler + var handler = new SocketsHttpHandler { PooledConnectionLifetime = TimeSpan.Zero, ConnectTimeout = CallTimeout, - }) - { - Timeout = CallTimeout, }; + if (credentials is not null) + { + handler.Credentials = credentials; + // Let the camera state its terms first: preemptive auth would send + // Basic to a device that only accepts Digest. + handler.PreAuthenticate = false; + } + return new HttpClient(handler) { Timeout = CallTimeout }; + } + + private HttpClient ClientFor(Uri service, CameraCredentials? credentials) + { + if (credentials is not { } c || string.IsNullOrEmpty(c.Username)) return _http; + + var key = $"{service.Host}:{service.Port}\u0000{c.Username}\u0000{c.Password}"; + return _authedClients.GetOrAdd(key, _ => + NewClient(new NetworkCredential(c.Username, c.Password ?? string.Empty))); } // --- Device service ----------------------------------------------------- @@ -126,7 +155,11 @@ public async Task GetStreamUriAsync(OnvifEndpoint endpoint, string profileT $"{Escape(profileToken)}"; var body = await CallAuthedAsync(media, endpoint, $"{Trt}/GetStreamUri", reqBody, ct).ConfigureAwait(false); - var uri = Value(body, "Uri"); + // The spec nests this as MediaUri/Uri, and Hikvision (among others) sends + // exactly that. Reading it as a direct child only matched the flatter + // shape onvif_simple_server returns, so a compliant camera looked like + // it had answered with no stream at all. + var uri = Descendant(body, "Uri")?.Value; if (string.IsNullOrWhiteSpace(uri)) throw new InvalidOperationException($"GetStreamUri returned no URI for profile {profileToken}"); return new Uri(uri, UriKind.Absolute); @@ -194,7 +227,8 @@ public async Task SetPresetAsync(OnvifEndpoint endpoint, string profileT $"{Escape(profileToken)}" + $"{Escape(name)}"; var body = await CallAuthedAsync(ptz, endpoint, $"{Tptz}/SetPreset", reqBody, ct).ConfigureAwait(false); - return Value(body, "PresetToken") ?? string.Empty; + // Nested the same way on some firmwares, for the same reason. + return Descendant(body, "PresetToken")?.Value ?? string.Empty; } public async Task RemovePresetAsync(OnvifEndpoint endpoint, string profileToken, string presetToken, CancellationToken ct) @@ -282,28 +316,21 @@ private async Task GetTimeShiftAsync(Uri deviceService, CancellationTo private async Task CallAsync(Uri service, string action, string body, CameraCredentials? credentials, TimeSpan shift, CancellationToken ct) { - var header = SecurityHeader(credentials, shift); - var envelope = - "" + - $"{header}{body}"; + // SOAP 1.2 first — the version ONVIF specifies. A camera that answers + // it with nothing usable gets one retry as SOAP 1.1, which several + // firmwares are built for and which costs one request to find out. + var (status, text) = await SendAsync(service, action, body, credentials, shift, soap12: true, ct) + .ConfigureAwait(false); - using var req = new HttpRequestMessage(HttpMethod.Post, service); - req.Headers.ConnectionClose = true; - if (credentials is { } c && !string.IsNullOrEmpty(c.Username)) + if (!IsUsable(text)) { - var basic = Convert.ToBase64String(Encoding.UTF8.GetBytes($"{c.Username}:{c.Password}")); - req.Headers.Authorization = new AuthenticationHeaderValue("Basic", basic); + _logger.LogDebug("ONVIF {Action}: SOAP 1.2 gave HTTP {Status} and {Length} bytes; retrying as SOAP 1.1", + action, (int)status, text.Length); + (status, text) = await SendAsync(service, action, body, credentials, shift, soap12: false, ct) + .ConfigureAwait(false); } - var content = new StringContent(envelope, Encoding.UTF8); - content.Headers.ContentType = new MediaTypeHeaderValue("application/soap+xml") { CharSet = "utf-8" }; - content.Headers.ContentType.Parameters.Add(new NameValueHeaderValue("action", $"\"{action}\"")); - req.Content = content; - - using var resp = await _http.SendAsync(req, HttpCompletionOption.ResponseContentRead, ct).ConfigureAwait(false); - var text = await resp.Content.ReadAsStringAsync(ct).ConfigureAwait(false); - if (string.IsNullOrWhiteSpace(text)) - throw new InvalidOperationException($"ONVIF {action}: empty response (HTTP {(int)resp.StatusCode})"); + if (string.IsNullOrWhiteSpace(text)) throw EmptyBody(action, status); XElement root; try { root = XDocument.Parse(text).Root!; } @@ -311,7 +338,11 @@ private async Task CallAsync(Uri service, string action, string body, var bodyEl = Child(Child(root, "Body"), null); if (bodyEl is null) - throw new InvalidOperationException($"ONVIF {action}: empty SOAP body"); + { + _logger.LogDebug("ONVIF {Action}: HTTP {Status}, body: {Body}", + action, (int)status, text.Length > 400 ? text[..400] : text); + throw EmptyBody(action, status); + } if (bodyEl.Name.LocalName == "Fault") { var reason = Descendant(bodyEl, "Text")?.Value @@ -322,6 +353,66 @@ private async Task CallAsync(Uri service, string action, string body, return bodyEl; } + private async Task<(HttpStatusCode Status, string Text)> SendAsync( + Uri service, string action, string body, CameraCredentials? credentials, + TimeSpan shift, bool soap12, CancellationToken ct) + { + var header = SecurityHeader(credentials, shift); + var envelope = + "" + + $"{header}{body}"; + + using var req = new HttpRequestMessage(HttpMethod.Post, service); + req.Headers.ConnectionClose = true; + if (credentials is { } c && !string.IsNullOrEmpty(c.Username)) + { + // Preemptive Basic for onvif_simple_server, which enforces it at the + // transport and never challenges. A camera that wants Digest answers + // 401 instead, and the handler's credentials settle that exchange. + var basic = Convert.ToBase64String(Encoding.UTF8.GetBytes($"{c.Username}:{c.Password}")); + req.Headers.Authorization = new AuthenticationHeaderValue("Basic", basic); + } + + var content = new StringContent(envelope, Encoding.UTF8); + if (soap12) + { + content.Headers.ContentType = new MediaTypeHeaderValue("application/soap+xml") { CharSet = "utf-8" }; + content.Headers.ContentType.Parameters.Add(new NameValueHeaderValue("action", $"\"{action}\"")); + } + else + { + // SOAP 1.1 has no action parameter on the content type; it travels + // in a header of its own. + content.Headers.ContentType = new MediaTypeHeaderValue("text/xml") { CharSet = "utf-8" }; + req.Headers.TryAddWithoutValidation("SOAPAction", $"\"{action}\""); + } + req.Content = content; + + using var resp = await ClientFor(service, credentials) + .SendAsync(req, HttpCompletionOption.ResponseContentRead, ct).ConfigureAwait(false); + return (resp.StatusCode, await resp.Content.ReadAsStringAsync(ct).ConfigureAwait(false) ?? string.Empty); + } + + // Worth reading: it parses, and its Body holds something. A firmware built + // for SOAP 1.1 typically answers a 1.2 request with no bytes at all or with + // an envelope whose Body is empty, and both mean "ask again differently". + // A fault is a usable answer — a camera that says why it refused is not + // asked twice. + private static bool IsUsable(string text) + { + if (string.IsNullOrWhiteSpace(text)) return false; + try { return Child(Child(XDocument.Parse(text).Root!, "Body"), null) is not null; } + catch (Exception) { return false; } + } + + // An empty body is what a camera sends when it will not say why. In + // practice it means ONVIF is switched off in the camera's own settings or + // the account has no ONVIF rights — neither of which arrives as a fault, so + // the message has to name them. The status code is the only other clue. + private static InvalidOperationException EmptyBody(string action, HttpStatusCode status) => + new($"ONVIF {action}: the camera returned an empty SOAP body (HTTP {(int)status}). " + + "Check that ONVIF is enabled on the camera and that this account may use it."); + private static string SecurityHeader(CameraCredentials? credentials, TimeSpan shift) { if (credentials is not { } c || string.IsNullOrEmpty(c.Username)) diff --git a/tests/OpenIPC.Viewer.Devices.Tests/Onvif/SoapOnvifClientInteropTests.cs b/tests/OpenIPC.Viewer.Devices.Tests/Onvif/SoapOnvifClientInteropTests.cs new file mode 100644 index 0000000..30a190c --- /dev/null +++ b/tests/OpenIPC.Viewer.Devices.Tests/Onvif/SoapOnvifClientInteropTests.cs @@ -0,0 +1,147 @@ +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Extensions.Logging.Abstractions; +using OpenIPC.Viewer.Core.Entities; +using OpenIPC.Viewer.Devices.Onvif; + +namespace OpenIPC.Viewer.Devices.Tests.Onvif; + +// Interop with firmwares that do not behave like onvif_simple_server: they want +// Digest rather than Basic, or SOAP 1.1 rather than 1.2, or they nest the +// stream URI where the spec says it goes. All three fail the same unhelpful +// way — HTTP 200 with an empty SOAP body — so each is reproduced against a stub +// camera rather than taken on trust. +// +// Every call is preceded by an unauthenticated GetSystemDateAndTime (the clock +// probe the WS-Security digest needs), so assertions count the requests that +// carry the action under test rather than all of them. +public sealed class SoapOnvifClientInteropTests +{ + private static SoapOnvifClient NewClient() => new(NullLogger.Instance); + + [Fact] + public async Task ACameraThatAnswersSoap12WithNothing_IsRetriedAsSoap11() + { + using var camera = StubCamera.Start(req => + req.IsSoap12 ? (string.Empty, 200) : (Envelope11(Capabilities()), 200)); + + var caps = await NewClient().GetCapabilitiesAsync(camera.Endpoint(null), CancellationToken.None); + + Assert.NotNull(caps); + var attempts = camera.Requests.Where(r => r.Is("GetCapabilities")).ToList(); + Assert.Equal(2, attempts.Count); + Assert.True(attempts[0].IsSoap12); + Assert.True(attempts[1].IsSoap11); + } + + // SOAP 1.1 carries the action in a header of its own rather than as a + // parameter on the content type. A camera that reads SOAPAction and finds + // nothing there rejects the call, so the retry would be pointless without it. + [Fact] + public async Task TheSoap11Retry_CarriesTheActionInItsOwnHeader() + { + using var camera = StubCamera.Start(req => + req.IsSoap12 ? (string.Empty, 200) : (Envelope11(Capabilities()), 200)); + + await NewClient().GetCapabilitiesAsync(camera.Endpoint(null), CancellationToken.None); + + var retry = camera.Requests.Last(r => r.Is("GetCapabilities")); + Assert.Contains("GetCapabilities", retry.SoapAction ?? "", StringComparison.Ordinal); + } + + // The Hikvision case. The camera refuses the preemptive Basic header and + // challenges for Digest; answering that is HttpClient's job, but only when + // the handler holds the credentials. + [Fact] + public async Task ADigestChallenge_IsAnswered() + { + using var camera = StubCamera.Start( + req => (req.Authorization ?? "").StartsWith("Digest", StringComparison.OrdinalIgnoreCase) + ? (Envelope12(Capabilities()), 200) + : (string.Empty, 401), + challenge: "Digest realm=\"IP Camera\", qop=\"auth\", nonce=\"4f3a2b1c\", stale=\"FALSE\""); + + var caps = await NewClient().GetCapabilitiesAsync( + camera.Endpoint(new CameraCredentials("admin", "secret")), CancellationToken.None); + + Assert.NotNull(caps); + Assert.Contains(camera.Requests, r => + r.Is("GetCapabilities") + && (r.Authorization ?? "").StartsWith("Digest", StringComparison.OrdinalIgnoreCase)); + } + + // Neither version got anywhere. "Empty SOAP body" is not something a user + // can act on; the two things worth checking on the camera are. + [Fact] + public async Task ACameraThatSaysNothingAtAll_FailsWithSomethingActionable() + { + using var camera = StubCamera.Start(_ => (string.Empty, 200)); + + var ex = await Assert.ThrowsAsync(() => + NewClient().GetCapabilitiesAsync(camera.Endpoint(null), CancellationToken.None)); + + Assert.Contains("ONVIF is enabled", ex.Message, StringComparison.OrdinalIgnoreCase); + Assert.Contains("account", ex.Message, StringComparison.OrdinalIgnoreCase); + } + + // A fault is an answer. Asking again in another dialect would waste a round + // trip and bury what the camera actually said. + [Fact] + public async Task AFault_IsReportedAsWorded_AndNeverRetriedAsSoap11() + { + using var camera = StubCamera.Start(_ => (Envelope12( + "" + + "Sender not authorized"), 400)); + + // The fault type is private to the client, so the assertion is on what + // reaches the caller: the camera's own wording. + var ex = await Assert.ThrowsAnyAsync(() => + NewClient().GetCapabilitiesAsync(camera.Endpoint(null), CancellationToken.None)); + + Assert.Contains("Sender not authorized", ex.Message, StringComparison.Ordinal); + Assert.DoesNotContain(camera.Requests, r => r.IsSoap11); + } + + // GetStreamUriResponse/MediaUri/Uri is what the spec defines and what most + // cameras send. Reading Uri as a direct child matched only the flatter + // shape onvif_simple_server returns, so a working camera looked like it had + // no stream at all. + [Fact] + public async Task TheStreamUri_IsReadFromWhereTheSpecPutsIt() + { + StubCamera? camera = null; + camera = StubCamera.Start(req => req.Is("GetCapabilities") + // The media service has to be advertised somewhere the client can + // actually follow — this stub. + ? (Envelope12(Capabilities($"http://127.0.0.1:{camera!.Port}/onvif/media")), 200) + : (Envelope12( + "" + + "rtsp://10.16.33.231:554/Streaming/Channels/101" + + "false" + + ""), 200)); + using var _ = camera; + + var uri = await NewClient().GetStreamUriAsync(camera.Endpoint(null), "Profile_1", CancellationToken.None); + + Assert.Equal("rtsp://10.16.33.231:554/Streaming/Channels/101", uri.ToString()); + } + + // --- helpers ------------------------------------------------------------ + + private static string Capabilities(string mediaXAddr = "http://127.0.0.1:1/onvif/media") => + "" + + $"{mediaXAddr}" + + ""; + + private static string Envelope12(string body) => + "" + + "" + + $"{body}"; + + private static string Envelope11(string body) => + "" + + "" + + $"{body}"; +} diff --git a/tests/OpenIPC.Viewer.Devices.Tests/Onvif/StubCamera.cs b/tests/OpenIPC.Viewer.Devices.Tests/Onvif/StubCamera.cs new file mode 100644 index 0000000..61a16ee --- /dev/null +++ b/tests/OpenIPC.Viewer.Devices.Tests/Onvif/StubCamera.cs @@ -0,0 +1,133 @@ +using System.Collections.Concurrent; +using System.IO; +using System.Net; +using System.Net.Sockets; +using System.Text; +using System.Threading.Tasks; +using OpenIPC.Viewer.Core.Entities; +using OpenIPC.Viewer.Core.Onvif; + +namespace OpenIPC.Viewer.Devices.Tests.Onvif; + +// What the client saw arrive. The body is read once, here, so a test can look +// at it without racing the handler for the request stream. +internal sealed record StubRequest( + string Body, + string? ContentType, + string? SoapAction, + string? Authorization) +{ + public bool IsSoap12 => + (ContentType ?? "").Contains("application/soap+xml", StringComparison.OrdinalIgnoreCase); + + public bool IsSoap11 => + (ContentType ?? "").Contains("text/xml", StringComparison.OrdinalIgnoreCase); + + public bool Is(string action) => Body.Contains(action, StringComparison.Ordinal); +} + +// A camera that answers however a test needs it to, over a real socket, so the +// client's own HTTP stack does the work — content types, SOAPAction, and the +// 401 handshake included. None of that would be exercised by a mocked handler. +internal sealed class StubCamera : IDisposable +{ + private readonly HttpListener _listener; + private readonly ConcurrentQueue _requests = new(); + + private StubCamera(HttpListener listener, int port) + { + _listener = listener; + Port = port; + } + + public int Port { get; } + + public IReadOnlyList Requests => _requests.ToArray(); + + public OnvifEndpoint Endpoint(CameraCredentials? credentials) => + OnvifEndpoint.FromHost("127.0.0.1", Port, credentials); + + // `challenge`, when set, is sent as WWW-Authenticate with any 401 the + // responder returns — that is what makes HttpClient try again with Digest. + public static StubCamera Start( + Func respond, + string? challenge = null) + { + var port = FreePort(); + var listener = new HttpListener(); + listener.Prefixes.Add($"http://127.0.0.1:{port}/"); + listener.Start(); + + var camera = new StubCamera(listener, port); + _ = Task.Run(() => camera.LoopAsync(respond, challenge)); + return camera; + } + + private async Task LoopAsync(Func respond, string? challenge) + { + while (_listener.IsListening) + { + HttpListenerContext ctx; + try { ctx = await _listener.GetContextAsync().ConfigureAwait(false); } + catch (Exception) { return; } // disposed mid-wait + + try + { + string body; + using (var reader = new StreamReader(ctx.Request.InputStream, Encoding.UTF8)) + body = await reader.ReadToEndAsync().ConfigureAwait(false); + + var request = new StubRequest( + body, + ctx.Request.ContentType, + ctx.Request.Headers["SOAPAction"], + ctx.Request.Headers["Authorization"]); + _requests.Enqueue(request); + + var (payload, status) = respond(request); + ctx.Response.StatusCode = status; + if (status == 401 && challenge is not null) + ctx.Response.AddHeader("WWW-Authenticate", challenge); + + if (payload.Length > 0) + { + var bytes = Encoding.UTF8.GetBytes(payload); + ctx.Response.ContentType = "application/soap+xml; charset=utf-8"; + ctx.Response.ContentLength64 = bytes.Length; + await ctx.Response.OutputStream.WriteAsync(bytes).ConfigureAwait(false); + } + else + { + // The failure this suite is about: a status, and no body to + // explain it. + ctx.Response.ContentLength64 = 0; + } + } + catch (Exception) + { + // A test that tore the camera down mid-request is not a failure. + } + finally + { + try { ctx.Response.Close(); } catch (Exception) { /* already gone */ } + } + } + } + + // Ask the OS for a port, then hand it to HttpListener. Racy in principle, + // never in practice on a test host. + private static int FreePort() + { + var probe = new TcpListener(IPAddress.Loopback, 0); + probe.Start(); + var port = ((IPEndPoint)probe.LocalEndpoint).Port; + probe.Stop(); + return port; + } + + public void Dispose() + { + try { _listener.Stop(); } catch (Exception) { /* nothing to stop */ } + try { _listener.Close(); } catch (Exception) { /* already closed */ } + } +} From adf7be83f9f8522e04aa7f79530a3c8bced49e80 Mon Sep 17 00:00:00 2001 From: iBinh Date: Tue, 25 Aug 2026 16:00:51 +0700 Subject: [PATCH 2/2] =?UTF-8?q?fix(onvif):=20address=20review=20=E2=80=94?= =?UTF-8?q?=20dialect=20learned=20once,=20mutations=20never=20re-sent,=20a?= =?UTF-8?q?uthed=20clients=20recycled?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two holes the review caught, both real. The SOAP 1.1 fallback retried every call whose response was unusable, including SetPreset and RemovePreset. An unusable response does not prove the request was not executed — a camera that ran SetPreset and answered garbage would get a duplicate preset from the resend, and a resend after a successful but unreadable remove would fault on the now-missing preset and report failure for a removal that worked. The dialect a host speaks is now learned once and remembered: the first time a host answers 1.2 with nothing usable and 1.1 with something, the flip is cached and later calls lead with 1.1. Since every authed operation is preceded by the unauthenticated clock probe on first contact, the dialect is already known by the time any mutation goes out. Mutations never cross-dialect retry — they use what the host's reads taught and fail honestly otherwise. The clock-skew retry stays, for mutations too: a fault means the camera refused the request, not that it ran it. And the per-credential HttpClient cache was keyed by host, user and password together with no eviction: every password a camera has ever had kept a live handler — old secret included — for the rest of the process, and two threads missing the cache at once could each construct a client only one of which was ever stored. Keyed by host:port now, since a camera has one credential at a time; a lookup that finds a different credential swaps the entry and disposes the superseded client (a request in flight on it was sent with the old password and failing anyway), and a plain lock replaces GetOrAdd so the losing constructor of a concurrent miss never exists. Growth is bounded by the camera addresses spoken to. Two tests pin the retry behaviour: the dialect is remembered (exactly one 1.2 request ever reaches a 1.1-only host), and a SetPreset whose response is empty is reported as failed after exactly one attempt. --- .../Onvif/SoapOnvifClient.cs | 98 +++++++++++++++---- .../Onvif/SoapOnvifClientInteropTests.cs | 47 ++++++++- 2 files changed, 120 insertions(+), 25 deletions(-) diff --git a/src/OpenIPC.Viewer.Devices/Onvif/SoapOnvifClient.cs b/src/OpenIPC.Viewer.Devices/Onvif/SoapOnvifClient.cs index 52962e4..aa37465 100644 --- a/src/OpenIPC.Viewer.Devices/Onvif/SoapOnvifClient.cs +++ b/src/OpenIPC.Viewer.Devices/Onvif/SoapOnvifClient.cs @@ -57,10 +57,24 @@ public sealed class SoapOnvifClient : IOnvifClient // first authed call, refreshed on an auth fault. private readonly ConcurrentDictionary _shiftByHost = new(StringComparer.OrdinalIgnoreCase); - // One client per (host, user). A handler that carries credentials is what + // Hosts that turned out to speak SOAP 1.1 only. Learned from the retry the + // first time a host answers 1.2 with nothing usable, then used as the first + // choice — so the discovery costs one extra request per host, ever, and a + // state-changing call is never the one doing the discovering. + private readonly ConcurrentDictionary _soap11Hosts = new(StringComparer.OrdinalIgnoreCase); + + // One client per camera address. A handler that carries credentials is what // lets HttpClient answer a 401 challenge on its own, which is the only way // to satisfy a camera that asks for Digest rather than Basic. - private readonly ConcurrentDictionary _authedClients = new(StringComparer.Ordinal); + // + // Keyed by host:port alone — a camera has one credential at a time — with + // the credential kept beside the client so a password change swaps the + // entry and disposes the superseded one, instead of caching every password + // this process has ever seen. Growth is bounded by the number of camera + // addresses. A plain lock rather than GetOrAdd: it also stops a concurrent + // miss from constructing a second client that nothing would ever dispose. + private readonly object _clientsGate = new(); + private readonly Dictionary _authedClients = new(StringComparer.Ordinal); public SoapOnvifClient(ILogger logger) { @@ -91,9 +105,23 @@ private HttpClient ClientFor(Uri service, CameraCredentials? credentials) { if (credentials is not { } c || string.IsNullOrEmpty(c.Username)) return _http; - var key = $"{service.Host}:{service.Port}\u0000{c.Username}\u0000{c.Password}"; - return _authedClients.GetOrAdd(key, _ => - NewClient(new NetworkCredential(c.Username, c.Password ?? string.Empty))); + var key = $"{service.Host}:{service.Port}"; + var credential = $"{c.Username}\u0000{c.Password}"; + lock (_clientsGate) + { + if (_authedClients.TryGetValue(key, out var entry)) + { + if (entry.Credential == credential) return entry.Client; + // The password changed. A request in flight on the old client + // was sent with the old password and is failing anyway, so + // disposing under it loses nothing. + entry.Client.Dispose(); + } + + var client = NewClient(new NetworkCredential(c.Username, c.Password ?? string.Empty)); + _authedClients[key] = (credential, client); + return client; + } } // --- Device service ----------------------------------------------------- @@ -226,7 +254,9 @@ public async Task SetPresetAsync(OnvifEndpoint endpoint, string profileT $"" + $"{Escape(profileToken)}" + $"{Escape(name)}"; - var body = await CallAuthedAsync(ptz, endpoint, $"{Tptz}/SetPreset", reqBody, ct).ConfigureAwait(false); + // retryable: false — if the camera ran the request and answered + // garbage, a resend would create a second preset. + var body = await CallAuthedAsync(ptz, endpoint, $"{Tptz}/SetPreset", reqBody, ct, retryable: false).ConfigureAwait(false); // Nested the same way on some firmwares, for the same reason. return Descendant(body, "PresetToken")?.Value ?? string.Empty; } @@ -238,7 +268,10 @@ public async Task RemovePresetAsync(OnvifEndpoint endpoint, string profileToken, $"" + $"{Escape(profileToken)}" + $"{Escape(presetToken)}"; - await CallAuthedAsync(ptz, endpoint, $"{Tptz}/RemovePreset", reqBody, ct).ConfigureAwait(false); + // retryable: false — a resend after a successful-but-unreadable remove + // would fault on the now-missing preset and report failure for a + // removal that worked. + await CallAuthedAsync(ptz, endpoint, $"{Tptz}/RemovePreset", reqBody, ct, retryable: false).ConfigureAwait(false); } // --- Transport ---------------------------------------------------------- @@ -265,25 +298,30 @@ private async Task ResolveServiceAsync(OnvifEndpoint endpoint, ServiceKind // Authenticated call with a per-host clock shift; on a fault, refresh the // shift once and retry (covers a stale/absent offset causing digest rejection). - private async Task CallAuthedAsync(Uri service, OnvifEndpoint endpoint, string action, string body, CancellationToken ct) + private async Task CallAuthedAsync(Uri service, OnvifEndpoint endpoint, string action, string body, CancellationToken ct, bool retryable = true) { var host = endpoint.DeviceServiceUri.Host; if (!_shiftByHost.TryGetValue(host, out var shift)) { + // Also where the host's SOAP dialect gets discovered, since this + // probe runs before the first real call — so by the time a mutation + // goes out, the dialect is already known. shift = await GetTimeShiftAsync(endpoint.DeviceServiceUri, ct).ConfigureAwait(false); _shiftByHost[host] = shift; } try { - return await CallAsync(service, action, body, endpoint.Credentials, shift, ct).ConfigureAwait(false); + return await CallAsync(service, action, body, endpoint.Credentials, shift, retryable, ct).ConfigureAwait(false); } catch (OnvifFaultException) { - // Maybe the clock drifted / the first shift was wrong — recompute and retry once. + // Maybe the clock drifted / the first shift was wrong — recompute and + // retry once. Safe for mutations too: a fault means the camera + // refused the request, not that it ran it. var fresh = await GetTimeShiftAsync(endpoint.DeviceServiceUri, ct).ConfigureAwait(false); _shiftByHost[host] = fresh; - return await CallAsync(service, action, body, endpoint.Credentials, fresh, ct).ConfigureAwait(false); + return await CallAsync(service, action, body, endpoint.Credentials, fresh, retryable, ct).ConfigureAwait(false); } } @@ -293,7 +331,7 @@ private async Task GetTimeShiftAsync(Uri deviceService, CancellationTo { var body = await CallAsync(deviceService, $"{Tds}/GetSystemDateAndTime", $"", - credentials: null, shift: TimeSpan.Zero, ct).ConfigureAwait(false); + credentials: null, shift: TimeSpan.Zero, retryable: true, ct).ConfigureAwait(false); var utc = Descendant(body, "UTCDateTime"); var date = Child(utc, "Date"); @@ -314,20 +352,38 @@ private async Task GetTimeShiftAsync(Uri deviceService, CancellationTo } } - private async Task CallAsync(Uri service, string action, string body, CameraCredentials? credentials, TimeSpan shift, CancellationToken ct) + private async Task CallAsync(Uri service, string action, string body, CameraCredentials? credentials, TimeSpan shift, bool retryable, CancellationToken ct) { - // SOAP 1.2 first — the version ONVIF specifies. A camera that answers - // it with nothing usable gets one retry as SOAP 1.1, which several - // firmwares are built for and which costs one request to find out. - var (status, text) = await SendAsync(service, action, body, credentials, shift, soap12: true, ct) + // SOAP 1.2 first — the version ONVIF specifies — unless this host has + // already shown it only answers 1.1. A camera that answers the first + // choice with nothing usable gets one retry in the other dialect, which + // several firmwares need and which costs one request to find out. The + // winner is remembered per host, so the discovery happens once. + // + // Except for mutations (retryable: false). An unusable response does + // not prove the request was not executed — a camera that ran SetPreset + // and then answered garbage would get a duplicate preset from a resend. + // Mutations rely on the dialect already learned from this host's + // earlier read calls (the clock probe at minimum) and fail honestly + // rather than guessing. + var soap12First = !_soap11Hosts.ContainsKey(service.Host); + var (status, text) = await SendAsync(service, action, body, credentials, shift, soap12: soap12First, ct) .ConfigureAwait(false); - if (!IsUsable(text)) + if (!IsUsable(text) && retryable) { - _logger.LogDebug("ONVIF {Action}: SOAP 1.2 gave HTTP {Status} and {Length} bytes; retrying as SOAP 1.1", - action, (int)status, text.Length); - (status, text) = await SendAsync(service, action, body, credentials, shift, soap12: false, ct) + _logger.LogDebug("ONVIF {Action}: SOAP {First} gave HTTP {Status} and {Length} bytes; retrying as SOAP {Second}", + action, soap12First ? "1.2" : "1.1", (int)status, text.Length, soap12First ? "1.1" : "1.2"); + (status, text) = await SendAsync(service, action, body, credentials, shift, soap12: !soap12First, ct) .ConfigureAwait(false); + + if (IsUsable(text)) + { + // The other dialect is the one this host speaks; remember it in + // whichever direction the flip went. + if (soap12First) _soap11Hosts[service.Host] = 1; + else _soap11Hosts.TryRemove(service.Host, out _); + } } if (string.IsNullOrWhiteSpace(text)) throw EmptyBody(action, status); diff --git a/tests/OpenIPC.Viewer.Devices.Tests/Onvif/SoapOnvifClientInteropTests.cs b/tests/OpenIPC.Viewer.Devices.Tests/Onvif/SoapOnvifClientInteropTests.cs index 30a190c..b428f0b 100644 --- a/tests/OpenIPC.Viewer.Devices.Tests/Onvif/SoapOnvifClientInteropTests.cs +++ b/tests/OpenIPC.Viewer.Devices.Tests/Onvif/SoapOnvifClientInteropTests.cs @@ -28,10 +28,15 @@ public async Task ACameraThatAnswersSoap12WithNothing_IsRetriedAsSoap11() var caps = await NewClient().GetCapabilitiesAsync(camera.Endpoint(null), CancellationToken.None); Assert.NotNull(caps); - var attempts = camera.Requests.Where(r => r.Is("GetCapabilities")).ToList(); - Assert.Equal(2, attempts.Count); - Assert.True(attempts[0].IsSoap12); - Assert.True(attempts[1].IsSoap11); + // The host's first exchange — the clock probe — is where the flip + // happens: 1.2, nothing usable, one retry as 1.1. Everything after + // leads with what that taught, so GetCapabilities is 1.1 on the first + // try rather than failing 1.2 again. + Assert.True(camera.Requests[0].IsSoap12); + Assert.True(camera.Requests[1].IsSoap11); + var capabilities = camera.Requests.Where(r => r.Is("GetCapabilities")).ToList(); + Assert.Single(capabilities); + Assert.True(capabilities[0].IsSoap11); } // SOAP 1.1 carries the action in a header of its own rather than as a @@ -127,6 +132,40 @@ public async Task TheStreamUri_IsReadFromWhereTheSpecPutsIt() Assert.Equal("rtsp://10.16.33.231:554/Streaming/Channels/101", uri.ToString()); } + // The discovery costs one request per host, ever: once a host has answered + // 1.1 after failing 1.2, later calls lead with 1.1 instead of failing 1.2 + // again first. + [Fact] + public async Task TheWorkingDialectIsRemembered() + { + using var camera = StubCamera.Start(req => + req.IsSoap12 ? (string.Empty, 200) : (Envelope11(Capabilities()), 200)); + + var client = NewClient(); + await client.GetCapabilitiesAsync(camera.Endpoint(null), CancellationToken.None); + await client.GetCapabilitiesAsync(camera.Endpoint(null), CancellationToken.None); + + // Only the very first request on the host — the clock probe — went out + // as 1.2; everything after used what that probe learned. + Assert.Equal(1, camera.Requests.Count(r => r.IsSoap12)); + } + + // An unusable response does not prove the request was not executed. A + // camera that ran SetPreset and answered garbage must not be asked again — + // the resend would create a second preset — so mutations fail honestly + // instead of retrying in the other dialect. + [Fact] + public async Task AMutation_IsNeverRetriedInAnotherDialect() + { + using var camera = StubCamera.Start(req => + req.Is("SetPreset") ? (string.Empty, 200) : (Envelope12(Capabilities()), 200)); + + await Assert.ThrowsAsync(() => + NewClient().SetPresetAsync(camera.Endpoint(null), "Profile_1", "Gate", CancellationToken.None)); + + Assert.Equal(1, camera.Requests.Count(r => r.Is("SetPreset"))); + } + // --- helpers ------------------------------------------------------------ private static string Capabilities(string mediaXAddr = "http://127.0.0.1:1/onvif/media") =>