From e624245a71e557356306a4d48057a413d571e1e3 Mon Sep 17 00:00:00 2001 From: Nils Lehnen <30603423+iderex@users.noreply.github.com> Date: Tue, 1 Sep 2026 01:37:26 +0200 Subject: [PATCH 1/4] Judge an arriving request's freshness once it has verified (#26) The freshness window and the nonce store have been in the tree since #152, and no route consulted them, so a request captured off the wire and sent again was served exactly like the first copy of it. The peer plane consults one now. What changed, in one sentence: `PeerPlane.Serve` judges freshness after the signature verifies and before the transition table answers, and a request that is stale, replayed, or arriving with no room left to remember its nonce is refused with the code the taxonomy in `docs/protocol.md` gives it. ## Where it sits, and why there Freshness is judged AFTER verification. That is the security property rather than an ordering convenience. `docs/threat-model.md` keeps one distinction on this plane deliberately - a refusal caused by clock skew says `clock` instead of reading as a signature failure - and the argument for keeping it is that the caller who sees it has already proved it holds the pairing's key, so the one bit it hands over is a bit the specification hands over anyway. Judging freshness first would hand that bit to a stranger. `replay` and `busy` sit behind the same door for the same reason. A request refused for freshness hands nothing on, though its body verified. Verification says the bytes are authentic; freshness says they are not this request. ## The one place a cause becomes a code `PeerPlane.Refuse` no longer names a code at all. It asks `RefusalCounters.CodeFor`, which is the same method the diagnostics payload sums by, so a site cannot answer one code while counting a cause that carries another. That is a stronger version of the property the old remark described, not a weaker one: the site does not choose a code. Three causes are added beside it, and each is reachable, which the theory walking every cause enforces: `timestamp-outside-the-window`, `nonce-already-seen` and `no-room-to-remember-the-nonce`. Every cause a caller without a verifying key can reach still collapses into `refused`. ## Registered once per server The window holds the nonces already seen, so a second instance remembers none of them. A per-caller window is not a weaker replay guard, it is no replay guard, and nothing about the plane's own behaviour would say so. The skew it runs on is the operator's, read through `ConfigurationReading.NewFreshnessWindow`, whose only caller until now was the test proving it worked. ## The proof Nine plants, each applied, built, run and reverted. Baseline 859 passed, 6 skipped, 0 failed. the plane stops judging freshness at all reds 13, leaves 844 a freshness refusal hands its verified body on reds 12, leaves 847 every refusal answers the undistinguished code again reds 9, leaves 848 the window is applied to the future only reds 6, leaves 853 a stale request is counted as one that did not verify reds 6, leaves 851 a replayed request is handed on instead of refused reds 5, leaves 854 freshness is judged before the signature is reds 4, leaves 853 a clock refusal is spelled as a replay to the peer reds 4, leaves 853 each caller gets a freshness window of its own reds 1, leaves 858 The second-to-last is the one worth knowing about: moving the judgement above the verification leaves the plane refusing exactly the same requests and reds only the cases about what a stranger is told, which is the whole of what the ordering buys. The last one red nothing until this change added the case that holds it. That is disclosed rather than repaired quietly: a per-caller window passes every case that drives one plane, because such a case holds one window either way. ## Cases the change moved rather than added `ADuplicatedMessageArrivesTwiceAndNothingRefusesTheSecondCopy` asserted, over the two-instance harness, that nothing refuses the second copy. It said so as the state of the protocol rather than as something it endorsed, and that state has moved; it is renamed and now asserts the replay is counted and the first copy is not. `TheAnswerIsTheSameRefusalWhateverTheCause` could not survive a taxonomy in which three causes answer their own code. It asserts the code the cause maps to instead, and the property it was really about - that a caller holding no key tells no two refusals apart - is asserted over exactly those callers in a new case rather than being dropped. Several cases sent one request repeatedly to reach the arrival limit. Under a nonce store every send after the first was a replay, so they now carry one nonce each, which is what the specification says two requests differing in nothing else must do. ## Documents Four pasted readings moved with the lines they name and are re-derived rather than adjusted by eye. Four passages said the plane consults no freshness window; each says what it said before and then says what is true now, in the register this repository uses for a claim that has stopped holding. ## Not claimed Nothing was run on a Jellyfin server. No key was written by an enrolment, no request was judged against a real clock on one, and no replay was ever presented to a running server - on a server today the key store is empty, so every request is refused before its freshness is reached and the window judges nothing. What is proved is proved against the types and against the in-process two-instance harness. Two servers with different skews are still not reconciled: each judges what arrives against its own setting, and no route here makes them agree. No case runs two threads, so the lock guarding the nonce store has still not been exercised under contention. The `net9.0` line of the suite did not run on the machine this was written on, which carries no .NET 9 runtime; only `net10.0` is measured above. The six skipped cases are the Unix-mode key store cases, skipped on Windows. Signed-off-by: Nils Lehnen <30603423+iderex@users.noreply.github.com> --- CHANGELOG.md | 13 ++ .../Api/PeerPlaneControllerTests.cs | 4 +- .../Api/PeerPlaneTests.cs | 214 ++++++++++++++++-- .../Api/RefusalCountersTests.cs | 176 ++++++++++++-- .../Harness/PairedInstance.cs | 20 +- .../Harness/PairedInstancesTests.cs | 30 ++- .../KeyStore/EndpointKeyMaterialTests.cs | 2 +- .../Protocol/RevocationTests.cs | 108 +++++++-- .../ServiceRegistrationTests.cs | 56 +++++ .../Api/PeerPlane.cs | 91 +++++++- .../Api/RefusalCause.cs | 42 +++- .../Api/RefusalCode.cs | 34 +-- .../Api/RefusalCounters.cs | 16 +- .../Configuration/ConfigurationReading.cs | 16 +- .../PluginServiceRegistrator.cs | 11 + .../Protocol/FreshnessOutcome.cs | 6 +- .../Protocol/FreshnessWindow.cs | 4 +- docs/configuration.md | 18 +- docs/lifecycle.md | 2 +- docs/logging.md | 2 +- docs/protocol.md | 31 ++- docs/threat-model.md | 55 +++-- 22 files changed, 814 insertions(+), 137 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 75ce910..81ae21c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,19 @@ there is no page and no endpoint an administrator can open an enrolment from. Two servers cannot be paired with this version. The `changelog` field in `build.yaml` and `build.net10.0.yaml` carries that paragraph in the same words. +- [protocol] A request that arrives with a correct signature is now judged for + freshness before this server acts on it. One whose timestamp is further from this + server's clock than the tolerated skew is answered `clock`, one carrying a nonce + already seen for that pairing is answered `replay`, and one arriving when that + pairing has no room left to remember another nonce is answered `busy`. A captured + request sent again is refused instead of being served. Only a peer that has + proved it holds the pairing's key is told which of the three happened; every + other caller gets the same refusal as before, because freshness is judged after + the signature and never before it. An operator whose two servers disagree about + the time now reads a clock refusal rather than debugging a signature error. As + with every other line here, nothing on a server produces this yet: no route puts + a key into a key store, so nothing verifies, and what changed is what a server + will answer rather than what one answers today. - [protocol] A refusal that says the two servers have no protocol version in common now carries the range this server speaks, as `versionLow` and `versionHigh` beside the code, in the same member names a pairing request uses diff --git a/Jellyfin.Plugin.ServerPairing.Tests/Api/PeerPlaneControllerTests.cs b/Jellyfin.Plugin.ServerPairing.Tests/Api/PeerPlaneControllerTests.cs index 6e4be97..8209112 100644 --- a/Jellyfin.Plugin.ServerPairing.Tests/Api/PeerPlaneControllerTests.cs +++ b/Jellyfin.Plugin.ServerPairing.Tests/Api/PeerPlaneControllerTests.cs @@ -366,7 +366,7 @@ private static byte[] Filled(int length) public async Task TheInstantAnArrivalIsJudgedAtIsReadFromTheClockPerRequest() { var limit = new ArrivalLimit(); - var plane = new PeerPlane(new RequestAuthenticator(new StoreBackedKeys(new InMemoryPairingKeyStore())), limit); + var plane = new PeerPlane(new RequestAuthenticator(new StoreBackedKeys(new InMemoryPairingKeyStore())), limit, new FreshnessWindow()); var clock = new MovableClock(DateTimeOffset.FromUnixTimeSeconds(1786000000)); await Over(plane, clock).Hello().ConfigureAwait(true); @@ -437,7 +437,7 @@ private static PeerPlaneController ControllerOver( feature.RawTarget = rawTarget!; } - return new PeerPlaneController(new PeerPlane(new RequestAuthenticator(new StoreBackedKeys(new InMemoryPairingKeyStore())), new ArrivalLimit()), TimeProvider.System, logger) + return new PeerPlaneController(new PeerPlane(new RequestAuthenticator(new StoreBackedKeys(new InMemoryPairingKeyStore())), new ArrivalLimit(), new FreshnessWindow()), TimeProvider.System, logger) { ControllerContext = new ControllerContext { HttpContext = context }, }; diff --git a/Jellyfin.Plugin.ServerPairing.Tests/Api/PeerPlaneTests.cs b/Jellyfin.Plugin.ServerPairing.Tests/Api/PeerPlaneTests.cs index 7c46fec..4c9acf8 100644 --- a/Jellyfin.Plugin.ServerPairing.Tests/Api/PeerPlaneTests.cs +++ b/Jellyfin.Plugin.ServerPairing.Tests/Api/PeerPlaneTests.cs @@ -1,4 +1,5 @@ using System; +using System.Globalization; using System.Linq; using System.Security.Cryptography; using System.Text; @@ -445,10 +446,10 @@ public void AnArrivalPastTheLimitIsRefusedBeforeItIsVerified(PairingMessage mess for (var i = 0; i < ArrivalLimit.ArrivalsPerPairing; i++) { - Assert.True(plane.Serve(message, Signed(message), At).BodyWasHandedOn); + Assert.True(plane.Serve(message, Signed(message, carries: FreshNonce()), At).BodyWasHandedOn); } - var past = plane.Serve(message, Signed(message), At); + var past = plane.Serve(message, Signed(message, carries: FreshNonce()), At); Assert.False(past.BodyWasHandedOn); Assert.Equal(RefusalCode.Refused, past.Code); @@ -469,11 +470,11 @@ public void TheAllowanceComesBackAWindowLater(PairingMessage message) for (var i = 0; i < ArrivalLimit.ArrivalsPerPairing; i++) { - plane.Serve(message, Signed(message), At); + plane.Serve(message, Signed(message, carries: FreshNonce()), At); } - Assert.False(plane.Serve(message, Signed(message), At).BodyWasHandedOn); - Assert.True(plane.Serve(message, Signed(message), At.AddSeconds(ArrivalLimit.WindowSeconds)).BodyWasHandedOn); + Assert.False(plane.Serve(message, Signed(message, carries: FreshNonce()), At).BodyWasHandedOn); + Assert.True(plane.Serve(message, Signed(message, carries: FreshNonce()), At.AddSeconds(ArrivalLimit.WindowSeconds)).BodyWasHandedOn); } /// @@ -507,21 +508,203 @@ public void AFloodOnTheEnrolmentIdentifierLeavesAPairingsAllowanceAlone(PairingM public void AnArrivalPastTheLimitCostsNoVerification(PairingMessage message) { var keys = new KnownKeys(PairingId, Key); - var plane = new PeerPlane(new RequestAuthenticator(keys), new ArrivalLimit()); + var plane = new PeerPlane(new RequestAuthenticator(keys), new ArrivalLimit(), new FreshnessWindow()); for (var i = 0; i < ArrivalLimit.ArrivalsPerPairing; i++) { - plane.Serve(message, Signed(message), At); + plane.Serve(message, Signed(message, carries: FreshNonce()), At); } var asked = keys.Asked; Assert.Equal(ArrivalLimit.ArrivalsPerPairing, asked); - Assert.False(plane.Serve(message, Signed(message), At).BodyWasHandedOn); + Assert.False(plane.Serve(message, Signed(message, carries: FreshNonce()), At).BodyWasHandedOn); Assert.Equal(asked, keys.Asked); } - private static PeerPlane Plane() => new PeerPlane(new RequestAuthenticator(new KnownKeys(PairingId, Key)), new ArrivalLimit()); + /// + /// A peer whose clock is outside the tolerated skew is refused for the clock, and that is a + /// different answer from the one a bad signature gets. This is the fourth done condition of + /// issue #26, and the distinction is the one docs/threat-model.md keeps on this plane + /// deliberately rather than collapsing into the undistinguished refusal. + /// + /// Which side of this server's clock the peer is on. + /// + /// Both directions, because a request from the future is as suspicious as one from the past, + /// and a window applied in one direction only would pass a case that drove the other. + /// + [Theory] + [InlineData(1)] + [InlineData(-1)] + public void ASkewedPeerIsRefusedForTheClockRatherThanForItsSignature(int direction) + { + var stamp = Stamp(At.AddSeconds(direction * (FreshnessWindow.WindowSeconds + 1))); + + var skewed = Plane().Serve( + PairingMessage.Exchange, + Signed(PairingMessage.Exchange, carries: FreshNonce(), stamp: stamp), + At); + + Assert.Equal(RefusalCode.Clock, skewed.Code); + Assert.Equal("{\"code\":\"clock\"}", Refusal.Body(skewed.Code)); + Assert.False(skewed.BodyWasHandedOn); + Assert.True(skewed.VerifiedBody.IsEmpty); + + // The same skew, presented by somebody who does not hold the key. What comes back is the + // undistinguished refusal, so an operator reading the two answers is told which of them + // happened rather than being left to guess. + var unsigned = Plane().Serve( + PairingMessage.Exchange, + Signed(PairingMessage.Exchange, signature: null, carries: FreshNonce(), stamp: stamp), + At); + + Assert.Equal(RefusalCode.Refused, unsigned.Code); + Assert.NotEqual(Refusal.Body(skewed.Code), Refusal.Body(unsigned.Code)); + } + + /// + /// A caller holding no verifying key learns nothing from a skew. A request that is both + /// stale and unsigned is answered exactly as one that is merely unsigned, because freshness + /// is judged after verification and never before it. + /// + /// + /// This is the sentence docs/threat-model.md closes its oracle section with, made + /// into a case. Without it the clock refusal would hand every stranger one bit about this + /// server's window, and the argument for keeping the distinction at all rests on their not + /// getting it. + /// + [Fact] + public void AStrangerLearnsNothingFromASkewBecauseFreshnessIsJudgedAfterVerification() + { + var stale = Plane().Serve( + PairingMessage.Exchange, + Signed( + PairingMessage.Exchange, + signature: null, + carries: FreshNonce(), + stamp: Stamp(At.AddSeconds(FreshnessWindow.WindowSeconds + 1))), + At); + + var fresh = Plane().Serve( + PairingMessage.Exchange, + Signed(PairingMessage.Exchange, signature: null, carries: FreshNonce()), + At); + + Assert.Equal(RefusalCode.Refused, stale.Code); + Assert.Equal(Refusal.Body(fresh.Code), Refusal.Body(stale.Code)); + Assert.False(stale.BodyWasHandedOn); + } + + /// + /// The edge of the window is inside it and the second past it is not, in both directions. A + /// window compared with the wrong operator passes every case that stays well away from its + /// edge, so these sit on it. + /// + /// Which side of this server's clock the peer is on. + [Theory] + [InlineData(1)] + [InlineData(-1)] + public void TheEdgeOfTheWindowIsInsideItAndTheSecondPastItIsNot(int direction) + { + var edge = Plane().Serve( + PairingMessage.Exchange, + Signed( + PairingMessage.Exchange, + carries: FreshNonce(), + stamp: Stamp(At.AddSeconds(direction * FreshnessWindow.WindowSeconds))), + At); + + var past = Plane().Serve( + PairingMessage.Exchange, + Signed( + PairingMessage.Exchange, + carries: FreshNonce(), + stamp: Stamp(At.AddSeconds(direction * (FreshnessWindow.WindowSeconds + 1)))), + At); + + // Inside the window, so it reaches the transition table and is refused by that instead. + Assert.Equal(RefusalCode.Refused, edge.Code); + Assert.True(edge.BodyWasHandedOn); + + Assert.Equal(RefusalCode.Clock, past.Code); + Assert.False(past.BodyWasHandedOn); + } + + /// + /// The same request sent twice is refused the second time, and the answer says replay. A + /// correctly signed request that is captured and sent again is still correctly signed, which + /// is why no signature check refuses one and why the nonce store exists at all. + /// + [Fact] + public void TheSameRequestSentTwiceIsRefusedAsAReplayTheSecondTime() + { + var plane = Plane(); + var once = Signed(PairingMessage.Exchange, carries: FreshNonce()); + + var first = plane.Serve(PairingMessage.Exchange, once, At); + var second = plane.Serve(PairingMessage.Exchange, once, At); + + Assert.Equal(RefusalCode.Refused, first.Code); + Assert.True(first.BodyWasHandedOn); + + Assert.Equal(RefusalCode.Replay, second.Code); + Assert.Equal("{\"code\":\"replay\"}", Refusal.Body(second.Code)); + Assert.False(second.BodyWasHandedOn); + Assert.True(second.VerifiedBody.IsEmpty); + } + + /// + /// A nonce is remembered under the pairing it arrived on rather than for the whole server, + /// so a nonce seen once does not refuse a second pairing that carries the same one. + /// + /// + /// Remembering across pairings would let one peer end another's traffic by sending the + /// nonces it expects that peer to use, which is a denial the store would have created rather + /// than refused. The second request here does not verify, and that is the point: it is + /// refused before freshness is reached, so what this asserts is that it was not refused as a + /// replay of the first. + /// + [Fact] + public void ANonceIsRememberedForThePairingItArrivedUnderAndNotForTheServer() + { + var plane = Plane(); + var carries = FreshNonce(); + var once = Signed(PairingMessage.Exchange, carries: carries); + + plane.Serve(PairingMessage.Exchange, once, At); + + Assert.Equal(RefusalCode.Replay, plane.Serve(PairingMessage.Exchange, once, At).Code); + + var elsewhere = plane.Serve( + PairingMessage.Exchange, + Signed(PairingMessage.Exchange, pairingId: "0011223344556677889900aabbccddee", carries: carries), + At); + + Assert.Equal(RefusalCode.Refused, elsewhere.Code); + } + + /// + /// The timestamp a peer whose clock reads this instant puts on a request. + /// + /// The peer's clock. + /// The timestamp, as it is spelled on the wire. + private static string Stamp(DateTimeOffset at) => + at.ToUnixTimeSeconds().ToString(CultureInfo.InvariantCulture); + + private static PeerPlane Plane() => new PeerPlane(new RequestAuthenticator(new KnownKeys(PairingId, Key)), new ArrivalLimit(), new FreshnessWindow()); + + /// + /// A nonce no other request in a case carries, of the shape the specification fixes. + /// + /// + /// A case that sends several requests to reach a limit is sending several REQUESTS, and the + /// specification says two that differ in nothing else must differ here. Reusing one nonce + /// makes every send after the first a replay, which is a different refusal from the one + /// those cases are about, so they would pass or fail for the wrong reason. + /// + /// The nonce. + private static string FreshNonce() => + Convert.ToHexString(RandomNumberGenerator.GetBytes(FieldShape.HexFieldLength / 2)).ToLowerInvariant(); private static ArrivingRequest Signed( PairingMessage message, @@ -531,15 +714,18 @@ private static ArrivingRequest Signed( string? signature = "", string? drop = null, byte[]? body = null, - bool exceeded = false) + bool exceeded = false, + string? carries = null, + string stamp = Timestamp) { var path = PeerPlane.PathFor(message); var bytes = body ?? Array.Empty(); + var carried = carries ?? Nonce; var id = string.Equals(drop, "id", StringComparison.Ordinal) ? null : pairingId; var version = string.Equals(drop, "version", StringComparison.Ordinal) ? null : Version; - var timestamp = string.Equals(drop, "timestamp", StringComparison.Ordinal) ? null : Timestamp; - var nonce = string.Equals(drop, "nonce", StringComparison.Ordinal) ? null : Nonce; + var timestamp = string.Equals(drop, "timestamp", StringComparison.Ordinal) ? null : stamp; + var nonce = string.Equals(drop, "nonce", StringComparison.Ordinal) ? null : carried; // An empty signature argument means "sign this properly", so every case that is not // about the signature carries one that verifies and the request fails for its own @@ -553,8 +739,8 @@ private static ArrivingRequest Signed( path, pairingId ?? string.Empty, Version, - Timestamp, - Nonce, + stamp, + carried, bytes); presented = RequestAuthenticator.Sign(signable, Key); diff --git a/Jellyfin.Plugin.ServerPairing.Tests/Api/RefusalCountersTests.cs b/Jellyfin.Plugin.ServerPairing.Tests/Api/RefusalCountersTests.cs index 0f7962a..8b8bc3a 100644 --- a/Jellyfin.Plugin.ServerPairing.Tests/Api/RefusalCountersTests.cs +++ b/Jellyfin.Plugin.ServerPairing.Tests/Api/RefusalCountersTests.cs @@ -165,19 +165,31 @@ public void EachCauseIsCountedByTheSiteThatRefusesIt(RefusalCause cause) } /// - /// Counting changes nothing a caller is told. Every cause is answered with the same code, - /// which is the property the wire rests on: an operator gains the split and a stranger - /// gains nothing. + /// What a caller is told is the code the cause maps to, at every site, driven through the + /// plane rather than read off the mapping. /// /// The cause the request is built to produce. /// - /// The body is handed on for one of them, and that is the transition table refusing a - /// caller it authenticated rather than anything this file adds. The assertion says so - /// rather than leaving it out. + /// THIS CASE ASSERTED THAT EVERY CAUSE IS ANSWERED WITH THE SAME CODE. Three are not: the + /// plane judges freshness after verification, and the taxonomy in docs/protocol.md + /// allows a distinguishable code to a caller that has proved it holds the key. The property + /// that mattered is not weakened by the narrowing, it is moved to + /// , which asserts it over + /// exactly the callers it was ever about. + /// + /// Asserting against rather than against + /// a literal is what makes this a case about the site: the mapping is one method, and a site + /// that answered a code while counting a cause carrying another would fail here. + /// + /// + /// The body is handed on for one of them, and that is the transition table refusing a caller + /// it authenticated rather than anything this file adds. A freshness refusal hands nothing + /// on, though its body verified, which the assertion says rather than leaving out. + /// /// [Theory] [MemberData(nameof(EveryCause))] - public void TheAnswerIsTheSameRefusalWhateverTheCause(RefusalCause cause) + public void TheAnswerIsTheCodeTheCauseMapsTo(RefusalCause cause) { var plane = PlaneFor(new RefusalCounters(), cause); @@ -185,10 +197,57 @@ public void TheAnswerIsTheSameRefusalWhateverTheCause(RefusalCause cause) var outcome = Final(plane, cause); - Assert.Equal(RefusalCode.Refused, outcome.Code); + Assert.Equal(RefusalCounters.CodeFor(cause), outcome.Code); Assert.Equal(cause == RefusalCause.NotAcceptedInThisState, outcome.BodyWasHandedOn); } + /// + /// Every refusal a caller holding no verifying key can reach is the same bytes. That is the + /// oracle property, stated over the callers it is about rather than over every cause. + /// + /// + /// The five below are the causes reachable before verification has succeeded, so they are + /// what a stranger walking this server can produce. A caller that reaches any of them learns + /// which of the five it met only if the bytes differ, and they do not. + /// + /// The list is written out rather than derived from the code each cause maps to. Deriving it + /// would ask the mapping whether the mapping is right, and this case exists to hold the + /// mapping to something: a change putting a distinguishable code on a cause a stranger can + /// reach fails here rather than passing because it moved both sides at once. + /// + /// + [Fact] + public void EveryRefusalACallerWithoutAKeyCanReachIsTheSameBytes() + { + RefusalCause[] withoutAKey = + [ + RefusalCause.NotOnThisPlane, + RefusalCause.BodyOverItsLimit, + RefusalCause.ArrivalAllowanceSpent, + RefusalCause.NoRoomToCountTheArrival, + RefusalCause.DidNotVerify, + ]; + + var answers = new List(); + + foreach (var cause in withoutAKey) + { + var plane = PlaneFor(new RefusalCounters(), cause); + + Setup(plane, cause); + + var outcome = Final(plane, cause); + + Assert.False(outcome.BodyWasHandedOn); + + answers.Add(Refusal.Body(outcome.Code)); + } + + Assert.Equal(withoutAKey.Length, answers.Count); + Assert.Single(answers.Distinct()); + Assert.Equal("{\"code\":\"refused\"}", answers[0]); + } + /// /// The plane counts into the object it was handed, which is what makes the number the /// diagnostics action renders the same number the plane wrote. A plane built without one @@ -204,7 +263,7 @@ public void ThePlaneCountsIntoTheCounterItWasHanded() Assert.Same(counters, plane.Refusals); Assert.Equal(1, counters.Counted(RefusalCause.NotOnThisPlane)); - Assert.Equal(0, new PeerPlane(new RequestAuthenticator(new KnownKeys()), new ArrivalLimit()) + Assert.Equal(0, new PeerPlane(new RequestAuthenticator(new KnownKeys()), new ArrivalLimit(), new FreshnessWindow()) .Refusals.Counted(RefusalCause.NotOnThisPlane)); } @@ -224,7 +283,7 @@ public void ThePayloadReportsWhatThePlaneRefused() { var counters = new RefusalCounters(); var arrivals = new ArrivalLimit(); - var plane = new PeerPlane(new RequestAuthenticator(new KnownKeys()), arrivals, counters); + var plane = new PeerPlane(new RequestAuthenticator(new KnownKeys()), arrivals, new FreshnessWindow(), counters); plane.Serve(PairingMessage.Hello, Arriving(target: "/ServerPairing/elsewhere"), At); plane.Serve(PairingMessage.Hello, Arriving(target: "/ServerPairing/elsewhere"), At); @@ -286,11 +345,23 @@ private static PeerPlane PlaneFor(RefusalCounters counters, RefusalCause cause) { // An allowance of one is what makes the second arrival the refusal, rather than sending // a full allowance and counting every admission on the way to it. - var arrivals = cause == RefusalCause.ArrivalAllowanceSpent - ? new ArrivalLimit(ArrivalLimit.WindowSeconds, 1, 1) - : new ArrivalLimit(); - - return new PeerPlane(new RequestAuthenticator(new KnownKeys()), arrivals, counters); + // + // Filling the nonce store needs the opposite. The store holds more nonces for one + // pairing than the default allowance admits requests, so the arrival limit would refuse + // long before the store filled and the case would assert the wrong cause. The allowance + // is widened for that one cause rather than the store being made smaller, because the + // store's bound is the constant the specification names. + var arrivals = cause switch + { + RefusalCause.ArrivalAllowanceSpent => new ArrivalLimit(ArrivalLimit.WindowSeconds, 1, 1), + RefusalCause.NoRoomToRememberTheNonce => new ArrivalLimit( + ArrivalLimit.WindowSeconds, + ArrivalLimit.MaximumArrivals, + ArrivalLimit.ArrivalsPerEnrolment), + _ => new ArrivalLimit(), + }; + + return new PeerPlane(new RequestAuthenticator(new KnownKeys()), arrivals, new FreshnessWindow(), counters); } /// @@ -314,6 +385,22 @@ private static void Setup(PeerPlane plane, RefusalCause cause) break; + case RefusalCause.NonceAlreadySeen: + // Seen once, so the request the case is about is the second copy of it. Signed, + // because a nonce is only ever remembered for a request that verified. + plane.Serve(PairingMessage.Hello, Arriving(sign: true), At); + break; + + case RefusalCause.NoRoomToRememberTheNonce: + // One distinct nonce per request, all of them verifying, until the store holds + // as many as it will for one pairing. + for (var i = 0; i < FreshnessWindow.NoncesPerPairing; i++) + { + plane.Serve(PairingMessage.Hello, Arriving(sign: true, carries: FreshNonce()), Filling(i)); + } + + break; + default: break; } @@ -340,9 +427,50 @@ private static void Setup(PeerPlane plane, RefusalCause cause) plane.Serve(PairingMessage.Hello, Arriving(), At), RefusalCause.NotAcceptedInThisState => plane.Serve(PairingMessage.Hello, Arriving(sign: true), At), + RefusalCause.TimestampOutsideTheWindow => + plane.Serve( + PairingMessage.Hello, + Arriving(sign: true, stamp: Skewed(FreshnessWindow.WindowSeconds + 1)), + At), + RefusalCause.NonceAlreadySeen => + plane.Serve(PairingMessage.Hello, Arriving(sign: true), At), + RefusalCause.NoRoomToRememberTheNonce => + plane.Serve( + PairingMessage.Hello, + Arriving(sign: true, carries: FreshNonce()), + Filling(FreshnessWindow.NoncesPerPairing)), _ => throw new ArgumentOutOfRangeException(nameof(cause)), }; + /// + /// The instant the request at this position in the fill is served at. + /// + /// Which request of the fill. + /// The instant. + /// + /// The store holds more nonces for one pairing than the widest arrival allowance this server + /// accepts admits requests in one window, so a fill cannot happen at a single instant: the + /// allowance is spent and comes back a window later. The clock moves rather than the + /// allowance being raised past its own maximum, because a case that had to exceed a bound + /// the plugin refuses would be proving something no server can be in. + /// + /// The whole fill spans one arrival window, which is well inside how long a nonce is + /// remembered, so nothing put in the store at the start has aged out of it by the end. It is + /// also well inside the tolerated skew, so every request carries the same timestamp and none + /// of them is refused for the clock instead. + /// + /// + private static DateTimeOffset Filling(int which) => + At.AddSeconds((which / ArrivalLimit.MaximumArrivals) * ArrivalLimit.WindowSeconds); + + /// + /// A timestamp this many seconds later than the instant every case judges at. + /// + /// How far ahead. + /// The timestamp, as it is spelled on the wire. + private static string Skewed(int seconds) => + (At.ToUnixTimeSeconds() + seconds).ToString(CultureInfo.InvariantCulture); + /// /// An identifier of the right shape that no other case uses. /// @@ -366,14 +494,17 @@ private static ArrivingRequest Arriving( string? target = null, string pairingId = PairingId, bool sign = false, - bool exceeded = false) + bool exceeded = false, + string? carries = null, + string stamp = Timestamp) { var path = PeerPlane.PathFor(PairingMessage.Hello); var body = Array.Empty(); + var carried = carries ?? Nonce; var presented = sign ? RequestAuthenticator.Sign( - new PairingRequest(PeerPlane.Method, path, pairingId, Version, Timestamp, Nonce, body), + new PairingRequest(PeerPlane.Method, path, pairingId, Version, stamp, carried, body), Key) : NotASignature; @@ -382,13 +513,20 @@ private static ArrivingRequest Arriving( PeerPlane.Method, pairingId, Version, - Timestamp, - Nonce, + stamp, + carried, presented, body, exceeded); } + /// + /// A nonce no other request in a case carries, of the shape the specification fixes. + /// + /// The nonce. + private static string FreshNonce() => + Convert.ToHexString(RandomNumberGenerator.GetBytes(FieldShape.HexFieldLength / 2)).ToLowerInvariant(); + /// /// A key source holding the one key this file signs with. /// diff --git a/Jellyfin.Plugin.ServerPairing.Tests/Harness/PairedInstance.cs b/Jellyfin.Plugin.ServerPairing.Tests/Harness/PairedInstance.cs index 8b9cd3f..8393b10 100644 --- a/Jellyfin.Plugin.ServerPairing.Tests/Harness/PairedInstance.cs +++ b/Jellyfin.Plugin.ServerPairing.Tests/Harness/PairedInstance.cs @@ -79,6 +79,7 @@ public PairedInstance(string name, PeerAddress address, DateTimeOffset startsAt, Log = new CapturedLog(); Configuration = new PluginConfiguration(); Refusals = new RefusalCounters(); + Freshness = new FreshnessWindow(); // A directory of this side's own under the platform's temporary path, so the two // stores cannot meet and nothing is written where a real server would keep one. @@ -96,7 +97,7 @@ public PairedInstance(string name, PeerAddress address, DateTimeOffset startsAt, KeyStoreFile = Path.Join(_directory, KeyStorePath.FileName); Keys = new FilePairingKeyStore(KeyStoreFile); - Plane = new PeerPlane(new RequestAuthenticator(new StoreBackedKeys(Keys)), Arrivals, Refusals); + Plane = new PeerPlane(new RequestAuthenticator(new StoreBackedKeys(Keys)), Arrivals, Freshness, Refusals); } /// @@ -134,6 +135,17 @@ public PairedInstance(string name, PeerAddress address, DateTimeOffset startsAt, /// public ArrivalLimit Arrivals { get; } + /// + /// Gets the timestamp window and nonce store this side judges a verified request against. + /// + /// + /// One per side, built on the default skew, and held here so a case can see that the two + /// sides remember their own nonces rather than a shared set. A second one on either side + /// would remember nothing the first had seen, which is a replay window opened by + /// construction rather than by a peer. + /// + public FreshnessWindow Freshness { get; } + /// /// Gets what this side has refused and why, since it was built. /// @@ -144,7 +156,11 @@ public PairedInstance(string name, PeerAddress address, DateTimeOffset startsAt, /// that verified from one that did not. /// is recorded only after verification /// succeeded and only when it failed, so the pair - /// is what a case asserts on. + /// is what a case asserts on. THIS REMARK SAID EVERY ANSWER ON THIS PLANE IS THE SAME + /// REFUSAL BY DESIGN. Three answers are not, and they are the three the plane reaches only + /// after verification, so what a sender receives now separates a stale or replayed request + /// from one that verified and was refused for its state. It still separates nothing before + /// verification, which is the property that sentence was written for. /// public RefusalCounters Refusals { get; } diff --git a/Jellyfin.Plugin.ServerPairing.Tests/Harness/PairedInstancesTests.cs b/Jellyfin.Plugin.ServerPairing.Tests/Harness/PairedInstancesTests.cs index 4d7bb0e..8f39def 100644 --- a/Jellyfin.Plugin.ServerPairing.Tests/Harness/PairedInstancesTests.cs +++ b/Jellyfin.Plugin.ServerPairing.Tests/Harness/PairedInstancesTests.cs @@ -185,17 +185,27 @@ public async Task ADelayedMessageIsJudgedAtTheLaterInstant() } /// - /// A duplicated message arrives twice, and both copies verify. The second half is the state - /// of this protocol today rather than something this case endorses: nothing refuses a - /// replay, and this is the point in the harness where that refusal will be proved when it - /// exists. THIS REMARK NAMED ISSUE #21 FOR IT AND THAT WAS THE WRONG ISSUE. The window and - /// the nonce store that would judge a replay are landed and are #21's; what no route does - /// is consult them on this plane, and a refusal on this plane that names the clock and is - /// told apart from a signature failure is the fourth done condition of issue #26. + /// A duplicated message arrives twice, both copies verify, and the second is refused as a + /// replay. This is the end-to-end half of the fourth done condition of issue #26: the plane + /// consults a freshness window, so the nonce store the window carries now judges what + /// arrives instead of judging nothing. /// + /// + /// THIS CASE ASSERTED THAT NOTHING REFUSES THE SECOND COPY, and said so as the state of the + /// protocol rather than as something it endorsed. That state has moved. What made the old + /// assertion possible is that both copies reached the transition table and were refused + /// there for the same reason, so a replay was indistinguishable from a first arrival in the + /// only instrument this harness has. It is distinguishable now, and the two counters below + /// are what separate them. + /// + /// Nothing about VERIFICATION moves here, which the third assertion holds: a replayed + /// request is correctly signed, which is exactly why a signature check cannot refuse one and + /// why the nonce store exists at all. + /// + /// /// The running case. [Fact] - public async Task ADuplicatedMessageArrivesTwiceAndNothingRefusesTheSecondCopy() + public async Task ADuplicatedMessageArrivesTwiceAndTheSecondCopyIsRefusedAsAReplay() { using var both = new PairedInstances(Start); @@ -212,8 +222,10 @@ public async Task ADuplicatedMessageArrivesTwiceAndNothingRefusesTheSecondCopy() // The same bytes both times, which is what makes it a duplicate rather than a second // send: a second send would carry a fresh nonce and a later timestamp. - Assert.Equal(2L, both.Right.Refusals.Counted(RefusalCause.NotAcceptedInThisState)); + Assert.Equal(1L, both.Right.Refusals.Counted(RefusalCause.NotAcceptedInThisState)); + Assert.Equal(1L, both.Right.Refusals.Counted(RefusalCause.NonceAlreadySeen)); Assert.Equal(0L, both.Right.Refusals.Counted(RefusalCause.DidNotVerify)); + Assert.Equal(0L, both.Right.Refusals.Counted(RefusalCause.TimestampOutsideTheWindow)); } /// diff --git a/Jellyfin.Plugin.ServerPairing.Tests/KeyStore/EndpointKeyMaterialTests.cs b/Jellyfin.Plugin.ServerPairing.Tests/KeyStore/EndpointKeyMaterialTests.cs index 81733e9..cd50045 100644 --- a/Jellyfin.Plugin.ServerPairing.Tests/KeyStore/EndpointKeyMaterialTests.cs +++ b/Jellyfin.Plugin.ServerPairing.Tests/KeyStore/EndpointKeyMaterialTests.cs @@ -272,7 +272,7 @@ private static PeerPlaneController Controller(string path) feature.RawTarget = path; } - return new PeerPlaneController(new PeerPlane(new RequestAuthenticator(new StoreBackedKeys(new InMemoryPairingKeyStore())), new ArrivalLimit()), TimeProvider.System, NullLogger.Instance) + return new PeerPlaneController(new PeerPlane(new RequestAuthenticator(new StoreBackedKeys(new InMemoryPairingKeyStore())), new ArrivalLimit(), new FreshnessWindow()), TimeProvider.System, NullLogger.Instance) { ControllerContext = new ControllerContext { HttpContext = context }, }; diff --git a/Jellyfin.Plugin.ServerPairing.Tests/Protocol/RevocationTests.cs b/Jellyfin.Plugin.ServerPairing.Tests/Protocol/RevocationTests.cs index 5a511b5..0797865 100644 --- a/Jellyfin.Plugin.ServerPairing.Tests/Protocol/RevocationTests.cs +++ b/Jellyfin.Plugin.ServerPairing.Tests/Protocol/RevocationTests.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Globalization; using System.IO; using System.Linq; using System.Security.Cryptography; @@ -178,14 +179,25 @@ public void RevokingDuringARotationDestroysTheSupersededKeyAsWell(int secondsFro store.Replace(PairingId, KeyMaterial.From(replacement), endsAt); var plane = PlaneOver(store); - var underTheOldKey = Signed(PairingMessage.Revoke, superseded); + + // Stamped at the instant it is judged at, which is what a peer whose clock agrees with + // this server sends. The rotation this case is about ends two hours out, so a request + // still carrying the fixed timestamp every other case uses would be refused for the + // clock long before the key it is signed with was reached. + var stamp = StampAt(judgedAt); + var underTheOldKey = Signed(PairingMessage.Revoke, superseded, carries: FreshNonce(), stamp: stamp); Assert.True(plane.Serve(PairingMessage.Revoke, underTheOldKey, judgedAt).BodyWasHandedOn); store.Destroy(PairingId); Assert.False(plane.Serve(PairingMessage.Revoke, underTheOldKey, judgedAt).BodyWasHandedOn); - Assert.False(plane.Serve(PairingMessage.Revoke, Signed(PairingMessage.Revoke, replacement), judgedAt).BodyWasHandedOn); + Assert.False(plane + .Serve( + PairingMessage.Revoke, + Signed(PairingMessage.Revoke, replacement, carries: FreshNonce(), stamp: stamp), + judgedAt) + .BodyWasHandedOn); } /// @@ -203,17 +215,15 @@ public void RevokingOnePairingLeavesEveryOtherPairingVerifying() store.Add(AnotherPairing, KeyMaterial.From(kept)); var plane = PlaneOver(store); - var theOther = Arriving( - PairingMessage.Exchange, - AnotherPairing, - RequestAuthenticator.Sign(Signable(PairingMessage.Exchange, AnotherPairing, Array.Empty()), kept), - Array.Empty()); - Assert.True(plane.Serve(PairingMessage.Exchange, theOther, At).BodyWasHandedOn); + // Two requests rather than one sent twice. A peer sending a second request carries a + // fresh nonce, and re-serving the same bytes would be a replay, which is refused for a + // reason that has nothing to do with the pairing this case is about. + Assert.True(plane.Serve(PairingMessage.Exchange, FromTheOther(kept), At).BodyWasHandedOn); store.Destroy(PairingId); - Assert.True(plane.Serve(PairingMessage.Exchange, theOther, At).BodyWasHandedOn); + Assert.True(plane.Serve(PairingMessage.Exchange, FromTheOther(kept), At).BodyWasHandedOn); Assert.Equal(new[] { AnotherPairing }, store.Pairings()); } @@ -256,15 +266,22 @@ public void ARevokedPairingIsAnsweredAsOneThisServerNeverHeldAKeyFor() /// The message. /// The identifier the request claims. /// The body. + /// The nonce it carries. + /// The timestamp it carries. /// The request. - private static PairingRequest Signable(PairingMessage message, string pairingId, byte[] body) + private static PairingRequest Signable( + PairingMessage message, + string pairingId, + byte[] body, + string? carries = null, + string stamp = Timestamp) => new PairingRequest( PeerPlane.Method, PeerPlane.PathFor(message), pairingId, Version, - Timestamp, - Nonce, + stamp, + carries ?? Nonce, body); /// @@ -274,15 +291,23 @@ private static PairingRequest Signable(PairingMessage message, string pairingId, /// The identifier the request claims. /// The signature presented, which may be nothing at all. /// The body. + /// The nonce it carries. + /// The timestamp it carries. /// The arriving request. - private static ArrivingRequest Arriving(PairingMessage message, string pairingId, string? signature, byte[] body) + private static ArrivingRequest Arriving( + PairingMessage message, + string pairingId, + string? signature, + byte[] body, + string? carries = null, + string stamp = Timestamp) => new ArrivingRequest( PeerPlane.PathFor(message), PeerPlane.Method, pairingId, Version, - Timestamp, - Nonce, + stamp, + carries ?? Nonce, signature, body, false); @@ -294,11 +319,58 @@ private static ArrivingRequest Arriving(PairingMessage message, string pairingId /// The key the peer signs with. /// The body, empty where a case does not care about one. /// The arriving request. - private static ArrivingRequest Signed(PairingMessage message, byte[] key, byte[]? body = null) + private static ArrivingRequest Signed( + PairingMessage message, + byte[] key, + byte[]? body = null, + string? carries = null, + string stamp = Timestamp) { var bytes = body ?? Array.Empty(); - return Arriving(message, PairingId, RequestAuthenticator.Sign(Signable(message, PairingId, bytes), key), bytes); + return Arriving( + message, + PairingId, + RequestAuthenticator.Sign(Signable(message, PairingId, bytes, carries, stamp), key), + bytes, + carries, + stamp); + } + + /// + /// A nonce no other request in a case carries, of the shape the specification fixes. + /// + /// The nonce. + private static string FreshNonce() => + Convert.ToHexString(RandomNumberGenerator.GetBytes(FieldShape.HexFieldLength / 2)).ToLowerInvariant(); + + /// + /// The timestamp a peer whose clock agrees with this server's puts on a request judged at + /// this instant. + /// + /// The instant the request is judged at. + /// The timestamp, as it is spelled on the wire. + private static string StampAt(DateTimeOffset at) => + at.ToUnixTimeSeconds().ToString(CultureInfo.InvariantCulture); + + /// + /// A request from the second pairing, signed under its own key and carrying a nonce no + /// other request in the case carries. + /// + /// The key that pairing holds. + /// The arriving request. + private static ArrivingRequest FromTheOther(byte[] key) + { + var carries = FreshNonce(); + + return Arriving( + PairingMessage.Exchange, + AnotherPairing, + RequestAuthenticator.Sign( + Signable(PairingMessage.Exchange, AnotherPairing, Array.Empty(), carries), + key), + Array.Empty(), + carries); } /// @@ -307,7 +379,7 @@ private static ArrivingRequest Signed(PairingMessage message, byte[] key, byte[] /// The key store. /// The plane. private static PeerPlane PlaneOver(IPairingKeyStore store) - => new PeerPlane(new RequestAuthenticator(new StoreBackedKeys(store)), new ArrivalLimit()); + => new PeerPlane(new RequestAuthenticator(new StoreBackedKeys(store)), new ArrivalLimit(), new FreshnessWindow()); /// /// A path to a store file in a directory of its own, removed when the class is disposed. diff --git a/Jellyfin.Plugin.ServerPairing.Tests/ServiceRegistrationTests.cs b/Jellyfin.Plugin.ServerPairing.Tests/ServiceRegistrationTests.cs index c2e4db8..68e8dd9 100644 --- a/Jellyfin.Plugin.ServerPairing.Tests/ServiceRegistrationTests.cs +++ b/Jellyfin.Plugin.ServerPairing.Tests/ServiceRegistrationTests.cs @@ -2,6 +2,7 @@ using System.Linq; using Jellyfin.Plugin.ServerPairing.Api; using Jellyfin.Plugin.ServerPairing.Configuration; +using Jellyfin.Plugin.ServerPairing.Protocol; using MediaBrowser.Common.Configuration; using MediaBrowser.Controller; using MediaBrowser.Controller.Plugins; @@ -133,6 +134,61 @@ public void ThePeerPlaneGetsOneLimitRatherThanOnePerCaller() Assert.Same(provider.GetRequiredService(), provider.GetRequiredService()); } + /// + /// One freshness window per server rather than one per caller. The reason is stronger than + /// the one above it: what this object holds is the nonces already seen, so a second instance + /// remembers none of them and every replay is fresh to it. + /// + /// + /// A per-caller window is not a weaker replay guard, it is no replay guard, and nothing about + /// the plane's own behaviour would say so - every case driving one plane holds one window and + /// passes either way. This is the only assertion in the tree that the server gets one. + /// + [Fact] + public void ThePeerPlaneGetsOneFreshnessWindowRatherThanOnePerCaller() + { + var services = new ServiceCollection(); + + var paths = Substitute.For(); + paths.DataPath.Returns(System.IO.Path.GetTempPath()); + services.AddSingleton(paths); + services.AddLogging(); + + new PluginServiceRegistrator().RegisterServices(services, Substitute.For()); + + using var provider = services.BuildServiceProvider(); + + Assert.Same( + provider.GetRequiredService(), + provider.GetRequiredService()); + } + + /// + /// The skew an operator set is the skew a verified request is judged against. Without this + /// the setting is a number that is read, refused out of range, and handed to a window the + /// plane never sees. + /// + [Fact] + public void ThePeerPlaneIsGivenTheSkewTheConfigurationCarries() + { + var services = new ServiceCollection(); + + var paths = Substitute.For(); + paths.DataPath.Returns(System.IO.Path.GetTempPath()); + services.AddSingleton(paths); + services.AddLogging(); + + // Registered before the registrator runs, which is what the TryAdd in it is for. The + // value is none of the defaults, so a window built on the default would fail here. + services.AddSingleton(new PluginConfiguration { TimestampWindowSeconds = 42 }); + + new PluginServiceRegistrator().RegisterServices(services, Substitute.For()); + + using var provider = services.BuildServiceProvider(); + + Assert.Equal(42, provider.GetRequiredService().AcceptedSkewSeconds); + } + /// /// The check above runs over whatever the registrator added, so it is empty either /// because everything resolved or because there was nothing to resolve, and the result diff --git a/Jellyfin.Plugin.ServerPairing/Api/PeerPlane.cs b/Jellyfin.Plugin.ServerPairing/Api/PeerPlane.cs index f7bbf80..f672ebd 100644 --- a/Jellyfin.Plugin.ServerPairing/Api/PeerPlane.cs +++ b/Jellyfin.Plugin.ServerPairing/Api/PeerPlane.cs @@ -42,6 +42,8 @@ public sealed class PeerPlane private readonly ArrivalLimit _arrivals; + private readonly FreshnessWindow _freshness; + private readonly RefusalCounters _refusals; /// @@ -49,15 +51,26 @@ public sealed class PeerPlane /// /// What decides whether an arriving request is authentic. /// How much of this plane one claimed identifier may use. + /// + /// The timestamp window and the nonce store a verified request is judged against. One per + /// server rather than one per caller: what it holds is the nonces already seen, and a + /// second instance would remember none of them, which is a replay window opened by + /// construction. + /// /// /// Where a refusal is counted for this server's own administrator. A plane built without one /// gets a counter of its own that nothing reads, so counting can never change what a caller /// is told; the composition root hands in the one the diagnostics action renders. /// - public PeerPlane(RequestAuthenticator authenticator, ArrivalLimit arrivals, RefusalCounters? refusals = null) + public PeerPlane( + RequestAuthenticator authenticator, + ArrivalLimit arrivals, + FreshnessWindow freshness, + RefusalCounters? refusals = null) { _authenticator = authenticator ?? throw new ArgumentNullException(nameof(authenticator)); _arrivals = arrivals ?? throw new ArgumentNullException(nameof(arrivals)); + _freshness = freshness ?? throw new ArgumentNullException(nameof(freshness)); _refusals = refusals ?? new RefusalCounters(); } @@ -131,10 +144,28 @@ public PeerPlane(RequestAuthenticator authenticator, ArrivalLimit arrivals, Refu /// every site answers the same code and a count taken from the answer would be one number. /// /// - /// Every answer today is , and that is the transition - /// table rather than a placeholder. No record store exists, so every pairing is - /// , and the Absent row of that table is the - /// undistinguished refusal for all five messages. + /// FRESHNESS IS JUDGED AFTER VERIFICATION AND THAT POSITION IS THE ORACLE ARGUMENT RATHER + /// THAN AN ORDERING CONVENIENCE. docs/threat-model.md keeps one distinction on this + /// plane deliberately: a refusal caused by clock skew says clock rather than reading as a + /// signature failure, which costs a caller one bit that the specification already gives + /// them and saves an operator an evening on two home servers whose clocks disagree. It is + /// affordable only because a caller that reaches it has already proved it holds the + /// pairing's key, so judging freshness before verifying would hand that bit to a stranger + /// and is the mistake this ordering exists against. The same holds for + /// and . + /// + /// + /// A request refused for freshness hands nothing on, even though its body verified. + /// Verification says the bytes are authentic and freshness says they are not this request, + /// and acting on a replayed body is exactly what the nonce store exists to stop. + /// + /// + /// THIS PARAGRAPH SAID EVERY ANSWER TODAY IS . A verified + /// request that is stale, replayed or arriving with no room left to remember its nonce is + /// answered with its own code. What is unchanged is the answer to everything before + /// verification, and the answer to a request that is verified and fresh: no record store + /// exists, so every pairing is , and the Absent row + /// of that table is the undistinguished refusal for all five messages. /// PeerPlaneTests.TheAbsentRowRefusesEveryMessage is the assertion that ties this /// answer to the table instead of to this sentence. /// @@ -197,6 +228,16 @@ public PeerPlaneOutcome Serve(PairingMessage message, ArrivingRequest arrived, D return Refuse(RefusalCause.DidNotVerify); } + // Only now, and never earlier. Everything below this line answers a caller that has + // proved it holds the pairing's key, which is what lets these three refusals be told + // apart from one another at all. + var freshness = _freshness.Judge(request.PairingId, request.Nonce, request.Timestamp, at); + + if (freshness != FreshnessOutcome.Fresh) + { + return Refuse(CauseOf(freshness)); + } + _refusals.Record(RefusalCause.NotAcceptedInThisState); return new PeerPlaneOutcome(RefusalCode.Refused, true, verified); @@ -206,16 +247,46 @@ public PeerPlaneOutcome Serve(PairingMessage message, ArrivingRequest arrived, D /// Counts a refusal and answers it. /// /// Why the request is refused. - /// The undistinguished refusal, with no body handed on. + /// The refusal the cause carries, with no body handed on. /// - /// One place builds the answer so that counting cannot drift from refusing. What the caller - /// receives does not depend on the cause and this method is where that is visible: the code - /// is the same constant whatever is passed in. + /// One place builds the answer so that counting cannot drift from refusing. THIS REMARK + /// SAID THE CODE IS THE SAME CONSTANT WHATEVER IS PASSED IN. It is derived from the cause + /// now, through , which is the same + /// method the diagnostics payload sums by. That is a stronger version of the property the + /// sentence it replaced was about rather than a weaker one: a site cannot answer one code + /// while counting a cause that maps to another, because it does not choose a code at all. + /// + /// Which causes still collapse into is that method's to + /// say, and all of the ones reached before verification do. + /// /// private PeerPlaneOutcome Refuse(RefusalCause cause) { _refusals.Record(cause); - return new PeerPlaneOutcome(RefusalCode.Refused, false, ReadOnlyMemory.Empty); + return new PeerPlaneOutcome(RefusalCounters.CodeFor(cause), false, ReadOnlyMemory.Empty); } + + /// + /// The cause this server counts for a freshness judgement that is not fresh. + /// + /// What judging the request's freshness produced. + /// The cause. + /// + /// is not reachable from this plane and is mapped + /// rather than thrown on. runs inside + /// verification, before any key is fetched, and refuses exactly the two field shapes this + /// outcome is about, so a request reaching the judgement has already passed them. It is + /// answered as the undistinguished refusal because the alternative is an exception on a + /// request path, and it is counted as rather than + /// under a cause of its own because a cause no site can reach is a number an operator reads + /// as a measurement and is not one. + /// + private static RefusalCause CauseOf(FreshnessOutcome freshness) => freshness switch + { + FreshnessOutcome.OutsideTheWindow => RefusalCause.TimestampOutsideTheWindow, + FreshnessOutcome.AlreadySeen => RefusalCause.NonceAlreadySeen, + FreshnessOutcome.NoRoomToRemember => RefusalCause.NoRoomToRememberTheNonce, + _ => RefusalCause.DidNotVerify, + }; } diff --git a/Jellyfin.Plugin.ServerPairing/Api/RefusalCause.cs b/Jellyfin.Plugin.ServerPairing/Api/RefusalCause.cs index 515a94c..5398b4a 100644 --- a/Jellyfin.Plugin.ServerPairing/Api/RefusalCause.cs +++ b/Jellyfin.Plugin.ServerPairing/Api/RefusalCause.cs @@ -21,8 +21,14 @@ namespace Jellyfin.Plugin.ServerPairing.Api; /// that map to it, which cannot move when a cause is added beside them. /// /// -/// Every member below maps to today, because that is the only -/// code any site in this tree produces. A member is added here when a site can distinguish it, +/// THIS PARAGRAPH SAID EVERY MEMBER BELOW MAPS TO , BECAUSE +/// THAT WAS THE ONLY CODE ANY SITE IN THIS TREE PRODUCED. Three of them no longer do. The plane +/// judges freshness once a request has verified, so a caller that has already proved it holds +/// the pairing's key is told which of the three it met. The taxonomy in docs/protocol.md +/// is what allows that and bounds it: a distinguishable code to a caller holding the key, and +/// none to anyone else. Every other member still maps to , and +/// is the one place that says which does +/// which. A member is added here when a site can distinguish it, /// never ahead of one: a cause nothing produces is a number an operator reads as a measurement /// and is not one. What each site refuses is , in the order that /// method fixes, and that order is the security property rather than a style. @@ -75,4 +81,36 @@ public enum RefusalCause /// is issue #287 and is not decided here. /// NotAcceptedInThisState = 5, + + /// + /// The signature verified and the timestamp is further from this server's clock than the + /// tolerated skew allows, in either direction. A request from the future is as suspicious + /// as one from the past, so both directions are this one cause. + /// + /// + /// This is the one distinction docs/threat-model.md keeps deliberately rather than + /// collapsing. It hands a caller one bit, which is whether their timestamp was inside this + /// server's window, and that bit is in the specification already; what it buys is an + /// operator on two home servers reading a clock refusal instead of debugging a signature + /// failure that is really a clock error. It is reached only after verification, so nobody + /// without a verifying key ever sees it. + /// + TimestampOutsideTheWindow = 6, + + /// + /// The signature verified, the timestamp is inside the window, and this nonce has already + /// been seen for this pairing. What this counts is a correctly signed request that was + /// captured and sent again, so a number here that is not zero says something none of the + /// others do. + /// + NonceAlreadySeen = 7, + + /// + /// The signature verified, the request is fresh, and this pairing has no room left to + /// remember another nonce, so it is refused rather than remembered. Separated from the + /// member above for the reason is separated from + /// : a peer replaying and this server having run out of + /// room are repaired in opposite directions. + /// + NoRoomToRememberTheNonce = 8, } diff --git a/Jellyfin.Plugin.ServerPairing/Api/RefusalCode.cs b/Jellyfin.Plugin.ServerPairing/Api/RefusalCode.cs index fec09b2..d932e81 100644 --- a/Jellyfin.Plugin.ServerPairing/Api/RefusalCode.cs +++ b/Jellyfin.Plugin.ServerPairing/Api/RefusalCode.cs @@ -10,12 +10,18 @@ namespace Jellyfin.Plugin.ServerPairing.Api; /// peer can interpret. /// /// Which members this tree can currently produce is a smaller set than this enumeration, and -/// deliberately so. produces and nothing else, -/// because every pairing is while no record store -/// exists, and the Absent row of the transition table is the undistinguished refusal -/// for all five messages. THIS SENTENCE SAID NO KEY STORE EXISTS EITHER, and one does and is -/// read on that path, which is issue #287: a request signed under a pairing's key verifies -/// there and is still answered with the row above. The rest are named here so the taxonomy has +/// deliberately so. THIS PARAGRAPH SAID PRODUCES +/// AND NOTHING ELSE. It produces four codes now: the plane judges freshness once a request has +/// verified, so , and are answered +/// to a caller that has proved it holds the pairing's key. What is unchanged is what an +/// unauthenticated caller gets, which is and only that, because freshness +/// is judged after verification and never before it. +/// +/// +/// The Absent row of the transition table is still the undistinguished refusal for all +/// five messages, so a request that is fresh and verified is answered +/// while no record store exists. THIS SENTENCE SAID NO KEY STORE EXISTS EITHER, and one does +/// and is read on that path, which is issue #287. The rest are named here so the taxonomy has /// one expression in code rather than a partial one that grows a second. /// /// @@ -29,8 +35,10 @@ public enum RefusalCode /// /// The signature verified and the timestamp is outside the freshness window. Only a - /// caller holding a verifying key ever sees it. No site produces it yet; the freshness - /// window is landed and nothing on this plane consults it. + /// caller holding a verifying key ever sees it. THIS SENTENCE SAID NO SITE PRODUCES IT AND + /// THAT NOTHING ON THIS PLANE CONSULTS THE WINDOW. consults + /// one, and this is what it answers where the timestamp is further from this server's clock + /// than the tolerated skew allows. /// Clock = 1, @@ -54,16 +62,16 @@ public enum RefusalCode Malformed = 4, /// - /// The signature verified, the request is fresh, and this nonce has already been seen for - /// this pairing. Only a caller holding a verifying key ever sees it. No site produces it - /// yet. + /// The signature verified, the timestamp is inside the window, and this nonce has already + /// been seen for this pairing. Only a caller holding a verifying key ever sees it. + /// produces it. /// Replay = 5, /// /// The signature verified, the request is fresh, and this pairing has no room left to - /// remember another nonce. Only a caller holding a verifying key ever sees it. No site - /// produces it yet. + /// remember another nonce. Only a caller holding a verifying key ever sees it. + /// produces it. /// Busy = 6, } diff --git a/Jellyfin.Plugin.ServerPairing/Api/RefusalCounters.cs b/Jellyfin.Plugin.ServerPairing/Api/RefusalCounters.cs index 7fc9c81..014e44b 100644 --- a/Jellyfin.Plugin.ServerPairing/Api/RefusalCounters.cs +++ b/Jellyfin.Plugin.ServerPairing/Api/RefusalCounters.cs @@ -65,10 +65,12 @@ public sealed class RefusalCounters /// The code. /// The cause is not one of the defined values. /// - /// Every cause maps to , which is what - /// answers at every one of its refusal sites. This method is - /// the one place that says so, so a site that ever answers a different code is one edit - /// here rather than a payload that quietly disagrees with the wire. + /// THIS REMARK SAID EVERY CAUSE MAPS TO . Three do not, + /// and they are the three the plane reaches only after a request has verified. This method + /// is still the one place that says which code a cause answers, and it is now load-bearing + /// rather than a single constant: builds its answer by asking + /// this rather than by naming a code beside a cause, so counting cannot drift from + /// refusing in either direction. /// public static RefusalCode CodeFor(RefusalCause cause) => cause switch { @@ -78,6 +80,9 @@ or RefusalCause.ArrivalAllowanceSpent or RefusalCause.NoRoomToCountTheArrival or RefusalCause.DidNotVerify or RefusalCause.NotAcceptedInThisState => RefusalCode.Refused, + RefusalCause.TimestampOutsideTheWindow => RefusalCode.Clock, + RefusalCause.NonceAlreadySeen => RefusalCode.Replay, + RefusalCause.NoRoomToRememberTheNonce => RefusalCode.Busy, _ => throw new ArgumentOutOfRangeException(nameof(cause)), }; @@ -101,6 +106,9 @@ or RefusalCause.DidNotVerify RefusalCause.NoRoomToCountTheArrival => "no-room-to-count-the-arrival", RefusalCause.DidNotVerify => "did-not-verify", RefusalCause.NotAcceptedInThisState => "not-accepted-in-this-state", + RefusalCause.TimestampOutsideTheWindow => "timestamp-outside-the-window", + RefusalCause.NonceAlreadySeen => "nonce-already-seen", + RefusalCause.NoRoomToRememberTheNonce => "no-room-to-remember-the-nonce", _ => throw new ArgumentOutOfRangeException(nameof(cause)), }; diff --git a/Jellyfin.Plugin.ServerPairing/Configuration/ConfigurationReading.cs b/Jellyfin.Plugin.ServerPairing/Configuration/ConfigurationReading.cs index 736dd06..c5979f0 100644 --- a/Jellyfin.Plugin.ServerPairing/Configuration/ConfigurationReading.cs +++ b/Jellyfin.Plugin.ServerPairing/Configuration/ConfigurationReading.cs @@ -276,12 +276,16 @@ public EnrolmentWindow NewEnrolmentWindow(IPairedPeers paired) /// /// A window that refuses a timestamp further out than the configured skew. /// - /// NOTHING ON THE PEER PLANE CONSULTS A FRESHNESS WINDOW YET, which the refusal taxonomy - /// says of the clock code in as many words. So the skew is refused out of range and - /// reaches a window only here and in the test that proves it does. THIS REMARK NAMED ISSUE - /// #21 FOR THE WIRING AND THAT WAS THE WRONG ISSUE: #21 owns how the window and the nonce - /// store judge, and a refusal that names the clock and is told apart from a signature - /// failure is the fourth done condition of issue #26. + /// THIS REMARK SAID NOTHING ON THE PEER PLANE CONSULTS A FRESHNESS WINDOW YET, AND THAT + /// THE SKEW REACHED A WINDOW ONLY HERE AND IN THE TEST THAT PROVES IT DOES. The registrator + /// resolves this once per server and hands the window to , so + /// the skew an operator sets is what a verified request is judged against. + /// + /// One per server rather than one per caller, which the registrator holds to by registering + /// it once. The reason is stronger here than for the arrival limit beside it: what this + /// holds is the nonces already seen, so a second instance remembers none of them and every + /// replay is fresh to it. + /// /// public FreshnessWindow NewFreshnessWindow() => new FreshnessWindow(TimestampWindowSeconds); diff --git a/Jellyfin.Plugin.ServerPairing/PluginServiceRegistrator.cs b/Jellyfin.Plugin.ServerPairing/PluginServiceRegistrator.cs index 4674422..c508ea9 100644 --- a/Jellyfin.Plugin.ServerPairing/PluginServiceRegistrator.cs +++ b/Jellyfin.Plugin.ServerPairing/PluginServiceRegistrator.cs @@ -80,9 +80,20 @@ public void RegisterServices(IServiceCollection serviceCollection, IServerApplic // administrative plane reads it, which is the only path between the two planes and // carries numbers rather than anything a caller supplied. serviceCollection.AddSingleton(); + + // Once, and for a reason that is not the same as the two above even though the word is. + // What this holds is the nonces already seen for each pairing, so a second instance + // remembers none of them and every replay it judges is fresh to it. A per-caller + // freshness window is not a weaker limit, it is no limit. The span it runs on is the + // operator's, read through the same reading every other setting comes through, so a + // refused skew is named at Error and the plane runs on the span a server nobody + // configured runs on. + serviceCollection.AddSingleton(services => + services.GetRequiredService().NewFreshnessWindow()); serviceCollection.AddSingleton(services => new PeerPlane( services.GetRequiredService(), services.GetRequiredService(), + services.GetRequiredService(), services.GetRequiredService())); // The one place in this plugin that reads a real clock. Everything downstream judges diff --git a/Jellyfin.Plugin.ServerPairing/Protocol/FreshnessOutcome.cs b/Jellyfin.Plugin.ServerPairing/Protocol/FreshnessOutcome.cs index cbf00fd..c13c080 100644 --- a/Jellyfin.Plugin.ServerPairing/Protocol/FreshnessOutcome.cs +++ b/Jellyfin.Plugin.ServerPairing/Protocol/FreshnessOutcome.cs @@ -8,8 +8,10 @@ namespace Jellyfin.Plugin.ServerPairing.Protocol; /// by a caller holding the pairing's key. That is what makes it safe for them to differ: the /// error taxonomy in docs/protocol.md allows a distinguishable code to a caller that /// already proved it holds the key, and allows none to anyone else. Which code each of these -/// becomes on the wire is fixed by that taxonomy rather than here, and nothing in this tree -/// performs the mapping, because there is no endpoint that would. +/// becomes on the wire is fixed by that taxonomy rather than here. THIS SENTENCE SAID NOTHING +/// IN THIS TREE PERFORMS THE MAPPING, BECAUSE THERE IS NO ENDPOINT THAT WOULD. There is one: +/// judges freshness after verification and maps every member +/// below onto an , which is what carries it to a code. /// public enum FreshnessOutcome { diff --git a/Jellyfin.Plugin.ServerPairing/Protocol/FreshnessWindow.cs b/Jellyfin.Plugin.ServerPairing/Protocol/FreshnessWindow.cs index 36ba802..5ea6a86 100644 --- a/Jellyfin.Plugin.ServerPairing/Protocol/FreshnessWindow.cs +++ b/Jellyfin.Plugin.ServerPairing/Protocol/FreshnessWindow.cs @@ -29,7 +29,9 @@ namespace Jellyfin.Plugin.ServerPairing.Protocol; /// /// /// Nothing here reads a clock. The instant to judge against is an argument, so a skew is -/// testable without waiting for one, and which clock supplies it is issue #26. +/// testable without waiting for one. What supplies it on the request path is the clock the +/// controller reads once and hands down, so the window, the arrival limit and the key store are +/// judged against one reading of the time rather than three. /// /// /// The store is not persisted. A restart forgets it, and a request replayed across a restart diff --git a/docs/configuration.md b/docs/configuration.md index 1e0b0f4..f4b4d82 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -165,13 +165,17 @@ in both directions and is derived from it, because two numbers an operator can s apart are two numbers they can set into a state where the store forgets a replay it exists to refuse. -NOTHING ON THE PEER PLANE CONSULTS A FRESHNESS WINDOW YET, which the refusal -taxonomy says of its `clock` code in as many words, so the skew is refused out of -range and reaches a window nothing asks. THIS SENTENCE NAMED ISSUE #21 FOR THE -WIRING AND THAT WAS THE WRONG ISSUE. #21 owns the window and the nonce store, and -its done conditions are about how those judge; asserting that a skewed peer -produces a refusal naming the clock, told apart from a signature failure, is the -fourth done condition of issue #26, which is where the wiring is. +THIS PARAGRAPH SAID NOTHING ON THE PEER PLANE CONSULTS A FRESHNESS WINDOW YET, AND +THAT THE SKEW REACHED A WINDOW NOTHING ASKS. The plane consults one. The window a +server builds from this setting is the window an arriving request is judged +against once its signature has verified, so a peer whose clock is further out than +the number below is told so, and is told it in a different answer from the one a +bad signature gets. + +What that does NOT mean is that a request has ever been judged this way on a +running server. Nothing puts a key into a key store yet, so nothing verifies, and +freshness is judged after verification and never before it. Setting this on a +server today changes what that server will do rather than what it does. The three `PeerPlane` settings are the arrival allowance the peer plane runs on: how long an allowance is counted over, how many requests one pairing identifier diff --git a/docs/lifecycle.md b/docs/lifecycle.md index 59c2978..6096a3f 100644 --- a/docs/lifecycle.md +++ b/docs/lifecycle.md @@ -131,7 +131,7 @@ no code ran at startup. git grep -n 'AddHostedService' -- Jellyfin.Plugin.ServerPairing/PluginServiceRegistrator.cs Jellyfin.Plugin.ServerPairing/PluginServiceRegistrator.cs:50: serviceCollection.AddHostedService(); - Jellyfin.Plugin.ServerPairing/PluginServiceRegistrator.cs:124: serviceCollection.AddHostedService(); + Jellyfin.Plugin.ServerPairing/PluginServiceRegistrator.cs:145: serviceCollection.AddHostedService(); The second of those two is this reader. The first is the one that says a setting was refused, which is a different thing that also runs at startup, and this block diff --git a/docs/logging.md b/docs/logging.md index 4653e29..82b3cb9 100644 --- a/docs/logging.md +++ b/docs/logging.md @@ -165,7 +165,7 @@ reading rather than counted here. origin/master:Jellyfin.Plugin.ServerPairing/KeyStore/StoreAtStartup.cs:82: if (_logger.IsEnabled(LogLevel.Information)) origin/master:Jellyfin.Plugin.ServerPairing/KeyStore/StoreAtStartup.cs:86: _logger.LogInformation( origin/master:Jellyfin.Plugin.ServerPairing/KeyStore/StoreAtStartup.cs:96: _logger.LogError(fault, "The key store could not be read at startup, so what it holds is unknown and no pairing will work. The server is left running."); - origin/master:Jellyfin.Plugin.ServerPairing/PluginServiceRegistrator.cs:113: services.GetRequiredService>())); + origin/master:Jellyfin.Plugin.ServerPairing/PluginServiceRegistrator.cs:124: services.GetRequiredService>())); THIS BLOCK WENT STALE TWICE AND NO RUN ON THIS REPOSITORY SAW EITHER TIME. It pasted two types, then four, and the command returns five; the registration line diff --git a/docs/protocol.md b/docs/protocol.md index c174d3b..db51d29 100644 --- a/docs/protocol.md +++ b/docs/protocol.md @@ -477,8 +477,8 @@ skew: ``` git grep -n "const int NoncesPerPairing\|const int MaximumWindowSeconds" origin/master -- Jellyfin.Plugin.ServerPairing/Protocol/FreshnessWindow.cs -origin/master:Jellyfin.Plugin.ServerPairing/Protocol/FreshnessWindow.cs:59: public const int MaximumWindowSeconds = 900; -origin/master:Jellyfin.Plugin.ServerPairing/Protocol/FreshnessWindow.cs:71: public const int NoncesPerPairing = 4096; +origin/master:Jellyfin.Plugin.ServerPairing/Protocol/FreshnessWindow.cs:61: public const int MaximumWindowSeconds = 900; +origin/master:Jellyfin.Plugin.ServerPairing/Protocol/FreshnessWindow.cs:73: public const int NoncesPerPairing = 4096; ``` What that refusal says on the wire is the taxonomy below. THIS SENTENCE SAID THE @@ -496,8 +496,13 @@ Both numbers are constants of the specification rather than secrets, so a caller learns nothing by discovering them that reading this document would not have told them. What makes them testable is already here: every judgement takes the instant as an argument, so a case chooses the moment rather than waiting for it, -and nothing is injected. Issue #26 owns the skew policy the refusal below rests -on. +and nothing is injected. + +THIS PARAGRAPH ENDED BY SAYING ISSUE #26 OWNS THE SKEW POLICY THE REFUSAL BELOW +RESTS ON. It owns it and has taken it. The plane consults a window: a request is +judged for freshness once its signature has verified, and the three answers below +are what a caller holding the key is told. The order is the security property and +is stated where the taxonomy is, not here. ## The arrival limit @@ -943,10 +948,20 @@ than here, and what is lost is stated there as plainly as it can be. It is off this list because a decided question left on a list of undecided ones is read as open by everybody who does not open the section it points at. -What no route on this plane consults is a freshness window at all, so nothing in -this section is refused by anything today. That is the fourth done condition of -issue #26 rather than a question this document leaves open, and the `clock` code -in the taxonomy above says the same of itself. +THIS LIST ALSO CARRIED THAT NO ROUTE ON THIS PLANE CONSULTS A FRESHNESS WINDOW AT +ALL, so that nothing in this section was refused by anything. One does. A request +that has verified is judged against the window and the nonce store, and a stale +one, a replayed one and one arriving with no room left to remember its nonce are +each answered with their own code out of the taxonomy above. It is off this list +for the same reason the restart question is: it was the fourth done condition of +issue #26 rather than a question this document left open, and it has been met. + +What is NOT claimed by that, and is the half to read carefully: no request has +ever reached this on a running server. Nothing puts a key into a key store, so +nothing verifies there, and a request that does not verify is refused before its +timestamp is considered. What is proved is proved against the types and against +the two-instance harness, and what a server does is unchanged until an enrolment +exists, which is issue #18. What identifier holds a pairing in `Offered`, which is set out under the local events table above with both answers and what each costs. It is written there diff --git a/docs/threat-model.md b/docs/threat-model.md index 8affedd..cf919b7 100644 --- a/docs/threat-model.md +++ b/docs/threat-model.md @@ -351,22 +351,33 @@ one. A request from the network now reaches one of the three, the request authenticator, through the plane; it reaches neither the freshness window nor the peer address, because the plane is given neither of them to consult: - git grep -n 'public PeerPlane(' origin/master -- Jellyfin.Plugin.ServerPairing/Api/PeerPlane.cs - origin/master:Jellyfin.Plugin.ServerPairing/Api/PeerPlane.cs:57: public PeerPlane(RequestAuthenticator authenticator, ArrivalLimit arrivals, RefusalCounters? refusals = null) - -THE PASTE ABOVE CARRIED TWO PARAMETERS AND CARRIES THREE. The third is where a -refusal is counted for this server's own administrator, which is issue #51, and -it moves nothing in this section: it holds one number per member of two -enumerations, it is written to and never read on this path, and no caller -reaches it. The sentence it corrects said the authenticator and the bound on -arrivals and nothing else, and what that sentence is about is unchanged - the -plane is still given neither the freshness window nor the peer address, so it -still consults neither. And what the one it does reach -answers with is a refusal for every caller, because the key source it is given -holds no keys, which is the reading pasted above under what exists today. So the -first paragraph above still describes a design position rather than a measured -property, and this sentence is meant to stay until something reaches those types -from the network and verifies. + +``` +git grep -n 'public PeerPlane(' origin/master -- Jellyfin.Plugin.ServerPairing/Api/PeerPlane.cs +``` + +THE PLANE IS GIVEN THE FRESHNESS WINDOW NOW, AND THIS PASSAGE SAID TWICE THAT IT +IS NOT. It said the plane is given neither the freshness window nor the peer +address and consults neither, and it corrected itself once about a third +constructor parameter while leaving that half standing. The parameter list is no +longer pasted here, because a paste of it goes stale every time an argument is +added and the command above is what a reader should run. + +What is true now: a request from the network reaches the request authenticator +through the plane, and, once it has verified, the freshness window as well. So a +captured request replayed to this server is refused by the nonce store rather than +by nothing, which is the limit this section claims and previously could not point +at. The peer address is unchanged - the plane is given none and consults none, +which is issue #22. + +WHAT THAT DOES NOT BUY IS THE MEASUREMENT, and it is the same absence as before. +The key source the plane is given holds no keys, because nothing puts one there, +so on a server today every request is refused before its freshness is judged and +the window judges nothing. The reach is proved against the types and against the +two-instance harness rather than against a server. So the first paragraph above +still describes a design position rather than a measured property, and this +sentence is meant to stay until something reaches those types from the network on +a running server and verifies. ### A2, someone on the network path, active @@ -854,11 +865,21 @@ two reasons. The window is a documented constant rather than a secret, so a caller can learn the same bit by reading the specification. And the alternative costs an operator an evening of debugging a signature error that is really a clock error, on two home servers where one of them has no time source. Issue #26 -owns the skew policy and the test for that distinction. +owns the skew policy and the test for that distinction, and both have landed: the +plane judges freshness against a window built from the operator's setting, and +`PeerPlaneTests.ASkewedPeerIsRefusedForTheClockRatherThanForItsSignature` is where +the two answers are shown to differ. An unauthenticated caller does not get that distinction, because a request that fails signature verification is refused before its timestamp is considered. So the clock refusal is only ever reported to a caller that already holds the key. +That ordering is what the whole argument rests on, so it is held by a case rather +than by this paragraph: + + +``` +git grep -n 'public void AStrangerLearnsNothingFromASkewBecauseFreshnessIsJudgedAfterVerification' origin/master -- Jellyfin.Plugin.ServerPairing.Tests/Api/PeerPlaneTests.cs +``` ## Out of scope From e999faecbcc523de3559b547bc14fd5ddc256895 Mon Sep 17 00:00:00 2001 From: Nils Lehnen <30603423+iderex@users.noreply.github.com> Date: Tue, 1 Sep 2026 02:07:42 +0200 Subject: [PATCH 2/4] Re-derive the reading the freshness outcomes moved under (#26) `docs/threat-model.md` pastes the two `FreshnessOutcome` members that separate a stale request from a replayed one, at lines 33 and 38. The change on this branch added two lines to that file's header remark, so both moved to 35 and 40, and the paste stopped reproducing. git grep -n "AlreadySeen = \|OutsideTheWindow = " -- Jellyfin.Plugin.ServerPairing/Protocol/FreshnessOutcome.cs Jellyfin.Plugin.ServerPairing/Protocol/FreshnessOutcome.cs:35: OutsideTheWindow = 2, Jellyfin.Plugin.ServerPairing/Protocol/FreshnessOutcome.cs:40: AlreadySeen = 3, How it was found is the part worth recording. I did grep the documents for pastes naming the files this branch changes, and I read the result through `head -20`, which cut the list one line above this block. So the survey was right and its output was truncated, and `Reproduce every pasted reading` on the pull request is what caught it - the same check could not be run to completion on this machine, where it dies on the temporary directory it makes. The paragraph carrying that block also said the sixth of six limits is a landed type. It is a consulted one now, which is what this branch is about, so the paragraph says that as well, with the same bound every other passage here carries: no key reaches a key store, so nothing has been judged this way on a server. sh .github/link-check.sh link-check: 157 checked, 6 not in this mode, 0 bad. Signed-off-by: Nils Lehnen <30603423+iderex@users.noreply.github.com> --- docs/threat-model.md | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/docs/threat-model.md b/docs/threat-model.md index cf919b7..01075d9 100644 --- a/docs/threat-model.md +++ b/docs/threat-model.md @@ -765,10 +765,18 @@ window from a nonce already seen: ``` git grep -n "AlreadySeen = \|OutsideTheWindow = " origin/master -- Jellyfin.Plugin.ServerPairing/Protocol/FreshnessOutcome.cs -origin/master:Jellyfin.Plugin.ServerPairing/Protocol/FreshnessOutcome.cs:33: OutsideTheWindow = 2, -origin/master:Jellyfin.Plugin.ServerPairing/Protocol/FreshnessOutcome.cs:38: AlreadySeen = 3, +origin/master:Jellyfin.Plugin.ServerPairing/Protocol/FreshnessOutcome.cs:35: OutsideTheWindow = 2, +origin/master:Jellyfin.Plugin.ServerPairing/Protocol/FreshnessOutcome.cs:40: AlreadySeen = 3, ``` +IT IS ALSO THE ONE OF THE SIX THAT IS NOW CONSULTED RATHER THAN ONLY LANDED, AND +THIS PARAGRAPH SAID ONLY THAT THE TYPE EXISTS. The peer plane judges an arriving +request against that window once its signature has verified, so a captured request +sent again is refused by the store rather than by nothing. What that does not buy +is the measurement: nothing puts a key into a key store, so on a server today +every request is refused before its freshness is reached, and the reach is proved +against the types and the two-instance harness rather than against a server. + The third and the fourth are the two that are owed in full, and this paragraph said that was because the tree held neither a mapping table nor a key store. It holds both: From b8c97ce3e264ba0c0d83c6fbb30d1be3434a2b22 Mon Sep 17 00:00:00 2001 From: Nils Lehnen <30603423+iderex@users.noreply.github.com> Date: Tue, 1 Sep 2026 02:15:46 +0200 Subject: [PATCH 3/4] Take the skew arithmetic out of int so nothing is cast from a product (#26) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five `cs/loss-of-precision` alerts arrived with the cases this branch added, on four sites in two files. Each is `AddSeconds` handed the result of an integer multiplication, and one of them is also an integer division whose fraction is dropped on purpose. gh api "repos/Flowfin/jellyfin-plugin-server-pairing/code-scanning/alerts?state=open&ref=refs/pull/310/merge&per_page=100" --jq '.[] | "\(.rule.id)\t\(.most_recent_instance.location.path):\(.most_recent_instance.location.start_line)"' cs/loss-of-precision Jellyfin.Plugin.ServerPairing.Tests/Api/RefusalCountersTests.cs:464 cs/loss-of-precision Jellyfin.Plugin.ServerPairing.Tests/Api/RefusalCountersTests.cs:464 cs/loss-of-precision Jellyfin.Plugin.ServerPairing.Tests/Api/PeerPlaneTests.cs:622 cs/loss-of-precision Jellyfin.Plugin.ServerPairing.Tests/Api/PeerPlaneTests.cs:614 cs/loss-of-precision Jellyfin.Plugin.ServerPairing.Tests/Api/PeerPlaneTests.cs:541 None of the four could overflow: every factor is a compile-time constant of this specification or the 1 and -1 a theory hands in. That is an argument about these instances rather than about the shape, and the shape is what a later case copies, so the sites are written so the question cannot be asked instead of being answered once. The skew sites take their product in `double`, through one helper that says why. The fill site keeps its integer division and holds it in an `int` of its own, which is where the intent belongs: an allowance is spent every `MaximumArrivals` requests, so the window a request falls in is the floor of its position over that allowance, and written as one expression that floor reads as a fraction dropped by accident. dotnet build 0 warnings, 0 errors dotnet test -f net10.0 Bestanden! : Fehler: 0, erfolgreich: 859, übersprungen: 6, gesamt: 865 Not claimed: the alert set at the merge of this pull request is what the analysis says after the change, and it is read there rather than here. Nothing was run on a Jellyfin server. Signed-off-by: Nils Lehnen <30603423+iderex@users.noreply.github.com> --- .../Api/PeerPlaneTests.cs | 24 +++++++++++++++---- .../Api/RefusalCountersTests.cs | 12 ++++++++-- 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/Jellyfin.Plugin.ServerPairing.Tests/Api/PeerPlaneTests.cs b/Jellyfin.Plugin.ServerPairing.Tests/Api/PeerPlaneTests.cs index 4c9acf8..5f9b0f1 100644 --- a/Jellyfin.Plugin.ServerPairing.Tests/Api/PeerPlaneTests.cs +++ b/Jellyfin.Plugin.ServerPairing.Tests/Api/PeerPlaneTests.cs @@ -538,7 +538,7 @@ public void AnArrivalPastTheLimitCostsNoVerification(PairingMessage message) [InlineData(-1)] public void ASkewedPeerIsRefusedForTheClockRatherThanForItsSignature(int direction) { - var stamp = Stamp(At.AddSeconds(direction * (FreshnessWindow.WindowSeconds + 1))); + var stamp = Stamp(Skewed(direction, FreshnessWindow.WindowSeconds + 1)); var skewed = Plane().Serve( PairingMessage.Exchange, @@ -582,7 +582,7 @@ public void AStrangerLearnsNothingFromASkewBecauseFreshnessIsJudgedAfterVerifica PairingMessage.Exchange, signature: null, carries: FreshNonce(), - stamp: Stamp(At.AddSeconds(FreshnessWindow.WindowSeconds + 1))), + stamp: Stamp(Skewed(1, FreshnessWindow.WindowSeconds + 1))), At); var fresh = Plane().Serve( @@ -611,7 +611,7 @@ public void TheEdgeOfTheWindowIsInsideItAndTheSecondPastItIsNot(int direction) Signed( PairingMessage.Exchange, carries: FreshNonce(), - stamp: Stamp(At.AddSeconds(direction * FreshnessWindow.WindowSeconds))), + stamp: Stamp(Skewed(direction, FreshnessWindow.WindowSeconds))), At); var past = Plane().Serve( @@ -619,7 +619,7 @@ public void TheEdgeOfTheWindowIsInsideItAndTheSecondPastItIsNot(int direction) Signed( PairingMessage.Exchange, carries: FreshNonce(), - stamp: Stamp(At.AddSeconds(direction * (FreshnessWindow.WindowSeconds + 1)))), + stamp: Stamp(Skewed(direction, FreshnessWindow.WindowSeconds + 1))), At); // Inside the window, so it reaches the transition table and is refused by that instead. @@ -683,6 +683,22 @@ public void ANonceIsRememberedForThePairingItArrivedUnderAndNotForTheServer() Assert.Equal(RefusalCode.Refused, elsewhere.Code); } + /// + /// The instant on a peer's clock this many seconds to one side of this server's. + /// + /// Which side, as 1 for the future and -1 for the past. + /// How far. + /// The instant. + /// + /// The product is taken in rather than in . Both + /// factors are small constants and neither could overflow, but an integer multiplication + /// whose result is handed to a parameter taking a double is a shape the analysis refuses on + /// sight, and writing it so that it cannot be wrong costs less than arguing that this + /// instance is safe. + /// + private static DateTimeOffset Skewed(int direction, int seconds) => + At.AddSeconds(direction * (double)seconds); + /// /// The timestamp a peer whose clock reads this instant puts on a request. /// diff --git a/Jellyfin.Plugin.ServerPairing.Tests/Api/RefusalCountersTests.cs b/Jellyfin.Plugin.ServerPairing.Tests/Api/RefusalCountersTests.cs index 8b8bc3a..00d7add 100644 --- a/Jellyfin.Plugin.ServerPairing.Tests/Api/RefusalCountersTests.cs +++ b/Jellyfin.Plugin.ServerPairing.Tests/Api/RefusalCountersTests.cs @@ -460,8 +460,16 @@ private static void Setup(PeerPlane plane, RefusalCause cause) /// of them is refused for the clock instead. /// /// - private static DateTimeOffset Filling(int which) => - At.AddSeconds((which / ArrivalLimit.MaximumArrivals) * ArrivalLimit.WindowSeconds); + private static DateTimeOffset Filling(int which) + { + // Integer division on purpose, and held in an int so that it says so: an allowance is + // spent every MaximumArrivals requests, so the window a request falls in is the floor of + // its position over that allowance. Written as one expression it reads to the analysis + // as a fraction being dropped by accident. + var window = which / ArrivalLimit.MaximumArrivals; + + return At.AddSeconds(window * (double)ArrivalLimit.WindowSeconds); + } /// /// A timestamp this many seconds later than the instant every case judges at. From 58971d9e0ffd4e291189cc9d4954b1f91a90b410 Mon Sep 17 00:00:00 2001 From: Nils Lehnen <30603423+iderex@users.noreply.github.com> Date: Tue, 1 Sep 2026 18:09:17 +0200 Subject: [PATCH 4/4] Re-derive the startup-reader line the record store moved (#26) Merging the mainline into this branch added the pairing record store's registration to `PluginServiceRegistrator`, which pushed the hosted service that reads the key store at startup from line 145 to line 160. The reading in `docs/lifecycle.md` names no `origin/master`, so `.github/reading-check.sh` does not walk it and nothing would have said the paste had gone stale. Re-derived at this head rather than adjusted by eye: git grep -n 'AddHostedService' -- Jellyfin.Plugin.ServerPairing/PluginServiceRegistrator.cs Signed-off-by: Nils Lehnen <30603423+iderex@users.noreply.github.com> --- docs/lifecycle.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/lifecycle.md b/docs/lifecycle.md index 6096a3f..37496d1 100644 --- a/docs/lifecycle.md +++ b/docs/lifecycle.md @@ -131,7 +131,7 @@ no code ran at startup. git grep -n 'AddHostedService' -- Jellyfin.Plugin.ServerPairing/PluginServiceRegistrator.cs Jellyfin.Plugin.ServerPairing/PluginServiceRegistrator.cs:50: serviceCollection.AddHostedService(); - Jellyfin.Plugin.ServerPairing/PluginServiceRegistrator.cs:145: serviceCollection.AddHostedService(); + Jellyfin.Plugin.ServerPairing/PluginServiceRegistrator.cs:160: serviceCollection.AddHostedService(); The second of those two is this reader. The first is the one that says a setting was refused, which is a different thing that also runs at startup, and this block