diff --git a/CHANGELOG.md b/CHANGELOG.md index b80acd1..71ee12f 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 pairing's state is now kept in a file of its own, beside the key store and under the same permissions, so what a server believes about a pairing survives a restart instead of living only in whatever object happened to hold 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..5f9b0f1 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,219 @@ 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(Skewed(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(Skewed(1, 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(Skewed(direction, FreshnessWindow.WindowSeconds))), + At); + + var past = Plane().Serve( + PairingMessage.Exchange, + Signed( + PairingMessage.Exchange, + carries: FreshNonce(), + stamp: Stamp(Skewed(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 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. + /// + /// 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 +730,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 +755,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..00d7add 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,58 @@ 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) + { + // 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. + /// + /// 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 +502,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 +521,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 ef9b2c5..03fe505 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..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:124: 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 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 909f2df..c4a6868 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 @@ -982,10 +987,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. THIS LIST CARRIED WHAT IDENTIFIER HOLDS A PAIRING IN `Offered` AND IT IS DECIDED. A record is written, under a provisional identifier that no peer can name and that diff --git a/docs/threat-model.md b/docs/threat-model.md index fd4566f..b14ae09 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 @@ -754,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: @@ -854,11 +873,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