fix(dotnet-agent-identity)!: harden issuance and nonce retention

docs/agent-identity/INTENT.agent-identity.md

@@ -164,7 +164,7 @@ Not intended:
   Authority-issued nonces would tolerate a skewed agent clock, but they cost a round trip after an idle period, stall a retry after a lost response, and need issuance state shared across authority nodes.
 - The authority records a nonce only after the request signature and any body digest are valid.
   Requests with an invalid signature or body digest therefore cannot consume nonce storage.
-- The authority keeps each accepted nonce until 60 s after its signature expires, the last moment the freshness window could accept the same signature again.
+- The authority keeps each accepted nonce at least until 60 s after its signature expires, the last moment the freshness window could accept the same signature again.
 - Published trust anchors help third parties validate certificate chains, but they carry no revocation information.
 
 ### Agent channel

dotnet/Devolutions.Agent.Identity.Authority/Devolutions.Agent.Identity.Authority.Tests/Authentication/AuthenticationTypeTests.cs

@@ -3,7 +3,7 @@
 namespace Devolutions.Agent.Identity.Authority.Tests;
 
 /// <summary>
-/// The types that only request authentication, the channel handshake and the enrollment rules create.
+/// The types that only request authentication, the channel handshake, and the enrollment and renewal rules create.
 /// </summary>
 /// <remarks>
 /// INTENT.md: code that acts for an identity takes it as a type only the signature check can create,
@@ -23,6 +23,10 @@ public class AuthenticationTypeTests
         // so a decision built by a caller would certify a key for an identity that no enrollment created.
         // oracle: intent dotnet/Devolutions.Agent.Identity.Authority/INTENT.md §Authenticated identity
         typeof(EnrollmentDecision),
+        // A renewal's certificate is issued from the decision to issue it, which carries the renewing identity,
+        // so a decision built by a caller would certify a key for an identity that no renew request proved.
+        // oracle: intent dotnet/Devolutions.Agent.Identity.Authority/INTENT.md §Authenticated identity
+        typeof(RenewalDecision),
         // oracle: mutant a public AuthenticationResult constructor
         typeof(AuthenticationResult),
         // oracle: mutant a public ChannelOpeningResult constructor
@@ -55,7 +59,7 @@ public void TypeHasNoPublicOrProtectedConstructor(Type type)
     // The intent forbids creating an identity outside that check but not forwarding one that it created,
     // so the list of members allowed to produce one is the library's own.
     // oracle: mutant a public member that builds an AuthenticatedIdentity outside the signature check,
-    // or an EnrollmentDecision outside the enrollment rules
+    // or an EnrollmentDecision or a RenewalDecision outside the enrollment or renewal rules
     [Fact]
     public void NoOtherPublicMemberProducesTheseTypes()
     {
@@ -82,6 +86,7 @@ public void NoOtherPublicMemberProducesTheseTypes()
                 "ChannelOpening.VerifyHelloAsync",
                 "ChannelOpeningResult.get_Opening",
                 "EnrollmentRules.Decide",
+                "RenewalRules.Decide",
                 "RequestAuthenticator.AuthenticateAsync",
                 "RequestAuthenticator.AuthenticateChannelOpeningAsync",
             ],

dotnet/Devolutions.Agent.Identity.Authority/Devolutions.Agent.Identity.Authority.Tests/Authentication/RequestAuthenticatorTests.cs

@@ -262,17 +262,53 @@ public async Task RenewGraceIsTheCertificateLifetimeAfterItsNotAfter(
     // CONTRACT.agent-identity.md §6 states the window of step 3 and the certificate rule of step 4
     // against the same `now`.
     // A second reading of the clock between the window and the certificate check
-    // would judge the certificate a day later, after it expired.
+    // would judge the certificate a second later, after it expired, though the window still holds then.
     // oracle: contract docs/agent-identity/CONTRACT.agent-identity.md §signatures
     [Fact]
     public async Task WindowAndCertificateAreJudgedAtOneReadingOfTheClock()
     {
         DateTimeOffset first = DeviceACertificate.NotAfter;
-        TestAuthority authority = new(first, clock: new SteppingTimeProvider(first, first.AddDays(1)));
+        TestAuthority authority = new(first, clock: new SteppingTimeProvider(first, first.AddSeconds(1)));
 
         Assert.Equal("ok", await authority.OutcomeAsync(authority.Valid(AgentOperation.Confirm)));
     }
 
+    // The shared intent keeps each accepted nonce until 60 s after its signature expires, the last instant at which
+    // step 3 accepts the signature, so a store may have discarded the pair after that instant.
+    // A request whose window closes while the registry answers would reach the store after it,
+    // where a copy of the request could find no pair and be accepted twice, so step 3 fails at that later now.
+    // The request expires 60 s after Now, so its window closes 120 s after Now; the lookup ends one tick later.
+    // oracle: intent docs/agent-identity/INTENT.agent-identity.md §Request authentication
+    // oracle: contract docs/agent-identity/CONTRACT.agent-identity.md §signatures
+    [Theory]
+    [MemberData(nameof(Operations))]
+    public async Task RequestWhoseWindowClosesBeforeItsNonceIsRecordedIsClockSkew(AgentOperation operation)
+    {
+        FixedTimeProvider clock = new(Now);
+        TestAuthority authority = new(Now, clock: clock);
+        authority.Registry.OnLookup = () => clock.Now = Now.AddSeconds(120).AddTicks(1);
+
+        string outcome = await authority.OutcomeAsync(authority.Valid(operation));
+
+        Assert.Equal("clock_skew", outcome);
+        Assert.Empty(authority.Nonces.Calls);
+    }
+
+    // Step 3 accepts a signature until 60 s after it expires, that instant included,
+    // so a request still inside its window when its nonce is recorded is accepted, however long the registry took.
+    // The request expires 60 s after Now, so its window closes 120 s after Now, when the lookup ends.
+    // oracle: contract docs/agent-identity/CONTRACT.agent-identity.md §signatures
+    [Theory]
+    [MemberData(nameof(Operations))]
+    public async Task RequestInsideItsWindowWhenItsNonceIsRecordedIsAccepted(AgentOperation operation)
+    {
+        FixedTimeProvider clock = new(Now);
+        TestAuthority authority = new(Now, clock: clock);
+        authority.Registry.OnLookup = () => clock.Now = Now.AddSeconds(120);
+
+        Assert.Equal("ok", await authority.OutcomeAsync(authority.Valid(operation)));
+    }
+
     // Step 3: 0 < expires − created ≤ 300.
     // oracle: contract docs/agent-identity/CONTRACT.agent-identity.md §signatures
     [Fact]

dotnet/Devolutions.Agent.Identity.Authority/Devolutions.Agent.Identity.Authority.Tests/Authentication/RequestAuthenticatorVectorTests.cs

@@ -135,13 +135,15 @@ public async Task FailingCaseDetailHoldsNoRequestData(string name)
         Assert.All(headers.Concat(values), value => Assert.DoesNotContain(value, outcome.FailureDetail));
     }
 
-    // CONTRACT.agent-identity.md §6 step 6 and the shared intent: the authority keeps each accepted nonce
-    // until 60 s after its signature expires, the last moment the window could accept the signature again.
+    // CONTRACT.agent-identity.md §6 step 6 accepts each (keyid, nonce) pair once, and the shared intent keeps each
+    // accepted nonce until 60 s after its signature expires, the last instant at which step 3 accepts the signature.
+    // The store call follows the last check of the window and may discard the pair once retainUntil has passed,
+    // so a copy of the signature accepted at that instant reaches the store after it, and the pair must outlive it.
     // oracle: contract docs/agent-identity/CONTRACT.agent-identity.md §signatures
     // oracle: intent docs/agent-identity/INTENT.agent-identity.md §Request authentication
     [Theory]
     [MemberData(nameof(OkCaseNames))]
-    public async Task OkCaseRecordsItsNonceOnceUntilExpiresPlus60Seconds(string name)
+    public async Task OkCaseRecordsItsNonceOnceUntilAfterItsWindowCloses(string name)
     {
         HttpSignatureCaseVector vector = Vectors.Case(name);
         VectorAuthority authority = new();
@@ -150,11 +152,12 @@ public async Task OkCaseRecordsItsNonceOnceUntilExpiresPlus60Seconds(string name
 
         string signatureInput = vector.Headers.SignatureInput!;
         long expires = long.Parse(Parameter(signatureInput, "expires"), CultureInfo.InvariantCulture);
-        (string, string, DateTimeOffset) recorded = (
-            Parameter(signatureInput, "keyid"),
-            Parameter(signatureInput, "nonce"),
-            DateTimeOffset.FromUnixTimeSeconds(expires + 60));
-        Assert.Equal([recorded], authority.Nonces.Calls);
+        DateTimeOffset windowEnd = DateTimeOffset.FromUnixTimeSeconds(expires + 60);
+        (string KeyId, string Nonce, DateTimeOffset RetainUntil) call = Assert.Single(authority.Nonces.Calls);
+        Assert.Equal(Parameter(signatureInput, "keyid"), call.KeyId);
+        Assert.Equal(Parameter(signatureInput, "nonce"), call.Nonce);
+        // oracle: mutant a retainUntil whose margin past the window's end is not 60 s, such as one tick
+        Assert.Equal(windowEnd.AddSeconds(60), call.RetainUntil);
     }
 
     // Code that acts for an identity relies on these facts,

dotnet/Devolutions.Agent.Identity.Authority/Devolutions.Agent.Identity.Authority.Tests/Certificates/IdentityCertificateIssuerTests.cs

@@ -42,10 +42,11 @@ public sealed class IdentityCertificateIssuerTests : IDisposable
     public static TheoryData<string> SerialNumberDrawNames => new(SerialNumberDraws.Keys);
 
     // A token that can create an identity.
-    // The `u` of the token that enrolls the identity, and the CSR of the enrollment, which names it.
+    // The `u` of the token that enrolls the identity, which the CSR of each enrollment names.
     private const string TokenUrl = "https://host/dvls";
 
-    private static readonly CertificateSigningRequest EnrollingCsr = TestEnrollmentUrl.Csr(TokenUrl);
+    // The CSR of another device enrolling with the same token: another key, the same URL.
+    private static readonly CertificateSigningRequest OtherKeyCsr = TestEnrollmentUrl.Csr(TokenUrl);
 
     private static EnrollmentTokenState UsableToken =>
         new(false, TokenUrl, Now.AddDays(1), maxUses: 10, identitiesCreated: 0);
@@ -56,24 +57,37 @@ public sealed class IdentityCertificateIssuerTests : IDisposable
         [EnrollmentOutcome.Reject] =
             EnrollmentRules.Decide(
                 new(false, TokenUrl, Now, maxUses: 10, identitiesCreated: 0),
-                EnrollingCsr,
+                OtherKeyCsr,
                 null,
                 Now),
         [EnrollmentOutcome.ReturnExistingIdentity] = EnrollmentRules.Decide(
             UsableToken,
-            EnrollingCsr,
+            OtherKeyCsr,
             IssuedKeyMatch.ForIdentity(Guid.Parse(IdentityIdText), true, false, CertificateStatus.Current),
             Now),
     };
 
+    // For each renewal outcome but a new pending certificate, the issued key whose CSR gets it:
+    // the renewing identity's pending key gets that certificate again, and its current key gets invalid_request.
+    private static Dictionary<RenewalOutcome, Func<Guid, IssuedKeyMatch>> KeysButANewPendingCertificate => new()
+    {
+        [RenewalOutcome.ReturnPendingCertificate] = identityId =>
+            IssuedKeyMatch.ForIdentity(identityId, false, false, CertificateStatus.Pending),
+        [RenewalOutcome.Reject] = identityId =>
+            IssuedKeyMatch.ForIdentity(identityId, false, false, CertificateStatus.Current),
+    };
+
     private readonly ECDsa _rootKey = ECDsa.Create(ECCurve.NamedCurves.nistP256);
     private readonly ECDsa _agentKey = ECDsa.Create(ECCurve.NamedCurves.nistP256);
     private readonly RandomNumberGenerator _random = RandomNumberGenerator.Create();
     private readonly X509Certificate2 _root;
+
+    // The CSR of the agent key, which names the token's URL, and which each test issues for unless it passes another.
     private readonly CertificateSigningRequest _csr;
 
-    // The decision that creates the identity whose certificate each test issues, unless the test passes another.
-    private readonly EnrollmentDecision _creating = EnrollmentRules.Decide(UsableToken, EnrollingCsr, null, Now);
+    // The decision that creates the identity whose certificate each test issues, unless the test passes another,
+    // made for the CSR of the agent key.
+    private readonly EnrollmentDecision _creating;
 
     // The ID that the decision minted, as CONTRACT.agent-identity.md §1 writes IDs:
     // Guid.ToString("D") writes a UUID lowercase and hyphenated.
@@ -82,9 +96,8 @@ public sealed class IdentityCertificateIssuerTests : IDisposable
     public IdentityCertificateIssuerTests()
     {
         _root = CreateRoot(Now.AddDays(-1), Now.AddYears(10));
-
-        byte[] csr = new CertificateRequest("CN=agent", _agentKey, HashAlgorithmName.SHA256).CreateSigningRequest();
-        _csr = CertificateSigningRequest.Validate(csr).Csr!;
+        _csr = TestEnrollmentUrl.Csr(_agentKey, TokenUrl);
+        _creating = EnrollmentRules.Decide(UsableToken, _csr, null, Now);
     }
 
     public void Dispose()
@@ -604,59 +617,88 @@ public async Task RenewalCertifiesTheKeyOfTheCsrForTheIdentityThatSignedTheRenew
         Assert.Equal(_agentKey.ExportSubjectPublicKeyInfo(), leaf.PublicKey.ExportSubjectPublicKeyInfo());
     }
 
-    // Code that acts for an identity takes the identity that the signature check produced,
-    // and only a renew request proves the identity that a renewal acts for.
-    // An identity that proved its key for another operation never had a renew request accepted for it.
-    // oracle: intent dotnet/Devolutions.Agent.Identity.Authority/INTENT.md §Authenticated identity
+    // Every issued public key is unique by SPKI SHA-256, so a certificate is issued only for the key whose lookup
+    // decided to issue it: another key may already be issued, or be decided on by a concurrent request.
+    // A host that issues from one enrollment's decision with another request's CSR would certify a key twice.
+    // oracle: contract docs/agent-identity/CONTRACT.agent-identity.md §token-rules
+    // oracle: spec Framework Design Guidelines §Validating Arguments
+    [Fact]
+    public void ThrowsForTheCsrOfAnotherKeyThanTheEnrollmentDecision()
+    {
+        ArgumentException exception = Assert.Throws<ArgumentException>(() => Issue(csr: OtherKeyCsr));
+
+        // oracle: spec Framework Design Guidelines §Using Standard Exception Types
+        Assert.Equal("csr", exception.ParamName);
+    }
+
+    // Every issued public key is unique by SPKI SHA-256, and a renew request whose CSR key was never issued gets a new
+    // pending certificate for that key, so a renewal decision issues for its own CSR key only.
+    // oracle: contract docs/agent-identity/CONTRACT.agent-identity.md §token-rules
     // oracle: contract docs/agent-identity/CONTRACT.agent-identity.md §renew
     // oracle: spec Framework Design Guidelines §Validating Arguments
-    [Theory]
-    [InlineData(nameof(IdentityCertificateIssuer), AgentOperation.Confirm)]
-    [InlineData(nameof(IdentityCertificateIssuer), AgentOperation.CheckIn)]
-    [InlineData(nameof(IdentityCertificateIssuer), AgentOperation.Connect)]
-    [InlineData(nameof(RenewalRules), AgentOperation.Confirm)]
-    [InlineData(nameof(RenewalRules), AgentOperation.CheckIn)]
-    [InlineData(nameof(RenewalRules), AgentOperation.Connect)]
-    public async Task RenewalThrowsForAnIdentityAuthenticatedForAnotherOperation(
-        string renewal,
-        AgentOperation operation)
+    [Fact]
+    public async Task RenewalThrowsForTheCsrOfAnotherKeyThanTheDecision()
     {
-        AuthenticatedIdentity identity = await TestIdentity.AuthenticateAsync(operation);
+        AuthenticatedIdentity renewing = await TestIdentity.AuthenticateAsync(AgentOperation.Renew);
 
-        ArgumentException exception = Assert.Throws<ArgumentException>(() => Renewals[renewal](identity));
+        ArgumentException exception =
+            Assert.Throws<ArgumentException>(() => IssueForRenewal(renewing, csr: OtherKeyCsr));
 
         // oracle: spec Framework Design Guidelines §Using Standard Exception Types
-        Assert.Equal("renewing", exception.ParamName);
+        Assert.Equal("csr", exception.ParamName);
     }
 
-    // INTENT.md: a renewal acts for the identity that a renew request proved, and null is no identity;
-    // the .NET Framework Design Guidelines throw ArgumentNullException for a null argument that is not allowed.
-    // oracle: intent dotnet/Devolutions.Agent.Identity.Authority/INTENT.md §Authenticated identity
+    // The CSR key of the identity's pending certificate gets that pending chain again, and the key of any other issued
+    // certificate gets invalid_request: neither issues a certificate, which would certify an issued key twice.
+    // oracle: contract docs/agent-identity/CONTRACT.agent-identity.md §renew
+    // oracle: contract docs/agent-identity/CONTRACT.agent-identity.md §token-rules
     // oracle: spec Framework Design Guidelines §Validating Arguments
     [Theory]
-    [InlineData(nameof(IdentityCertificateIssuer))]
-    [InlineData(nameof(RenewalRules))]
-    public void RenewalThrowsForANullIdentity(string renewal)
+    [InlineData(RenewalOutcome.ReturnPendingCertificate)]
+    [InlineData(RenewalOutcome.Reject)]
+    public async Task RenewalThrowsForADecisionThatIssuesNoCertificate(RenewalOutcome outcome)
     {
-        ArgumentNullException exception = Assert.Throws<ArgumentNullException>(() => Renewals[renewal](null!));
+        AuthenticatedIdentity renewing = await TestIdentity.AuthenticateAsync(AgentOperation.Renew);
+        RenewalDecision decision =
+            RenewalRules.Decide(renewing, _csr, KeysButANewPendingCertificate[outcome](renewing.IdentityId));
+
+        ArgumentException exception = Assert.Throws<ArgumentException>(() => IssueForRenewal(decision));
 
         // oracle: spec Framework Design Guidelines §Using Standard Exception Types
-        Assert.Equal("renewing", exception.ParamName);
+        Assert.Equal("issuing", exception.ParamName);
     }
 
-    // The two calls that act for a renewing identity, named by the class of each.
-    private Dictionary<string, Action<AuthenticatedIdentity>> Renewals => new()
+    // A null argument throws ArgumentNullException, whose ParamName names it.
+    // oracle: spec Framework Design Guidelines §Validating Arguments
+    [Fact]
+    public void RenewalThrowsForANullDecision()
     {
-        [nameof(IdentityCertificateIssuer)] = renewing => IssueForRenewal(renewing).Dispose(),
-        [nameof(RenewalRules)] = renewing => RenewalRules.Decide(renewing, null),
-    };
+        ArgumentNullException exception =
+            Assert.Throws<ArgumentNullException>(() => IssueForRenewal((RenewalDecision)null!));
 
-    private X509Certificate2 IssueForRenewal(AuthenticatedIdentity renewing, string enrollmentUrl = TokenUrl)
+        // oracle: spec Framework Design Guidelines §Using Standard Exception Types
+        Assert.Equal("issuing", exception.ParamName);
+    }
+
+    // Decides a renew request of the identity for the CSR of the agent key,
+    // then issues its pending certificate with the given CSR, by default that one.
+    private X509Certificate2 IssueForRenewal(
+        AuthenticatedIdentity renewing,
+        string enrollmentUrl = TokenUrl,
+        CertificateSigningRequest? csr = null)
+    {
+        return IssueForRenewal(RenewalRules.Decide(renewing, _csr, null), enrollmentUrl, csr);
+    }
+
+    private X509Certificate2 IssueForRenewal(
+        RenewalDecision issuing,
+        string enrollmentUrl = TokenUrl,
+        CertificateSigningRequest? csr = null)
     {
         return IdentityCertificateIssuer.Issue(
-            renewing,
+            issuing,
             enrollmentUrl,
-            _csr,
+            csr ?? _csr,
             new TestRootSigner(_root, _rootKey),
             Lifetime,
             new FixedTimeProvider(Now),