Always pin the server, let the registry own pins, and survive a revoke mid-upgrade

Found in the Windows VM re-test.

- A CA-flagged server certificate (common with a plain openssl self-signed
  cert) produced an installer package with no pin at all, so agent 1.20
  refused the server. The package now always carries the SPKI pin of the
  certificate PCs receive; the CA flag only skips the trusted-root import.
  When an uploaded CA-flagged certificate differs from the server's own, both
  pins are included. The build response carries X-ShopDB-Build-Warning and the
  installer logs what older agents need.
- Pins written into appsettings.json by the installer were merged with the
  registry, so removing a pin from HKLM never took effect. When the ServerPins
  value exists in the registry it is now authoritative, even when empty;
  appsettings pins apply only when it is absent. Set-ShopDBServerPins.ps1
  -Clear deletes the value; new -None writes an empty value (no pins).
- A script or package download sent with a credential revoked moments earlier
  got 401 CREDENTIAL_INVALID and failed the command. The agent now retries that
  download once with the shared key and records the fallback in the result.

agent/ShopDbEndpointAgent.Core/AgentConfig.cs

@@ -14,13 +14,15 @@ public sealed class AgentConfig
     [JsonPropertyName("cadenceseconds")] public int CadenceSeconds { get; set; } = 900;
 
     // Legacy pin: SHA-1 thumbprint of the server leaf certificate (hex, no
-    // separators). Still honoured next to ServerPins; breaks on every cert
-    // renewal, so prefer an SPKI pin. See AgentHttpFactory.
+    // separators). Honoured with serverpins only while the registry ServerPins
+    // value is absent; breaks on every cert renewal, so prefer an SPKI pin.
+    // See AgentHttpFactory.
     [JsonPropertyName("servercertthumbprint")] public string? ServerCertThumbprint { get; set; }
 
     // Server public key pins: lowercase hex SHA-256 of the leaf certificate's
-    // SubjectPublicKeyInfo. Merged with HKLM\SOFTWARE\GE\ShopDB ServerPins
-    // (REG_MULTI_SZ). Any pin configured = the leaf key must match one; chain
+    // SubjectPublicKeyInfo. Fallback only: HKLM\SOFTWARE\GE\ShopDB ServerPins
+    // (REG_MULTI_SZ), when it exists in either view, replaces these entirely,
+    // even when empty. Any pin in effect = the leaf key must match one; chain
     // and name errors are then ignored. None = normal Windows chain validation.
     [JsonPropertyName("serverpins")] public List<string> ServerPins { get; set; } = new();
 

agent/ShopDbEndpointAgent.Core/AgentHttpFactory.cs

@@ -172,43 +172,55 @@ public sealed record AgentServerError(string Code, string Message)
     }
 }
 
-// Server certificate trust. Pins are hex SHA-256 of the leaf SubjectPublicKeyInfo,
-// merged from HKLM\SOFTWARE\GE\ShopDB ServerPins (both views) and appsettings
-// serverpins; the legacy SHA-1 servercertthumbprint still counts. With any pin
-// configured only a matching leaf is accepted and chain/name errors are ignored
-// (self-signed servers on the air gap). Without pins: SslPolicyErrors.None.
-// Registry pins re-read at most every RefreshInterval on the handshake path, so a
-// GE-Enforce fix or a "next" rotation pin lands without a service restart.
+// Server certificate trust. Pins are hex SHA-256 of the leaf SubjectPublicKeyInfo.
+// HKLM\SOFTWARE\GE\ShopDB ServerPins is AUTHORITATIVE whenever the value exists
+// in either view, even empty (empty = no pins, Windows chain validation), so a
+// removed or rotated-out pin really goes away. Only when the value is absent in
+// both views do appsettings serverpins and the legacy SHA-1 servercertthumbprint
+// apply. With any pin in effect only a matching leaf is accepted and chain/name
+// errors are ignored (self-signed servers on the air gap). Without pins:
+// SslPolicyErrors.None. Registry re-read at most every RefreshInterval on the
+// handshake path, so a GE-Enforce fix or a rotation lands without a restart.
 public sealed class ServerPinSet
 {
     public const string RegistryValue = "ServerPins";
     public static readonly TimeSpan RefreshInterval = TimeSpan.FromSeconds(60);
 
-    private sealed record Snapshot(HashSet<string> SpkiPins, string RawRegistry)
+    // Present = the value exists in at least one view.
+    private sealed record RegistryRead(bool Present, List<string> Values)
     {
-        public string Signature => string.Join(",", SpkiPins.OrderBy(p => p, StringComparer.Ordinal));
+        public string Raw => Present ? "registry:" + string.Join("|", Values) : "absent";
+    }
+
+    private sealed record Snapshot(HashSet<string> SpkiPins, string? LegacyThumbprint, bool FromRegistry, string RawRegistry)
+    {
+        public string Signature => (FromRegistry ? "registry:" : "appsettings:")
+            + string.Join(",", SpkiPins.OrderBy(p => p, StringComparer.Ordinal))
+            + (LegacyThumbprint is null ? "" : "+legacy");
+        public bool Pinned => SpkiPins.Count > 0 || LegacyThumbprint is not null;
+        public string Source => FromRegistry ? "registry" : "appsettings.json";
     }
 
     private readonly HashSet<string> configPins;
     private readonly string? legacyThumbprint;
     private readonly ILogger logger;
-    private readonly Func<bool, List<string>?> readRegistry;
+    private readonly Func<bool, RegistryRead?> readRegistry;
     private readonly object refreshLock = new();
     private volatile Snapshot current;
     private long nextRefreshTicks;
 
     private ServerPinSet(HashSet<string> configPins, string? legacyThumbprint, ILogger logger,
-        Func<bool, List<string>?> readRegistry)
+        Func<bool, RegistryRead?> readRegistry)
     {
         this.configPins = configPins;
         this.legacyThumbprint = legacyThumbprint;
         this.logger = logger;
         this.readRegistry = readRegistry;
-        current = Build(readRegistry(false) ?? new List<string>());
+        current = Build(readRegistry(false) ?? new RegistryRead(false, new List<string>()));
         nextRefreshTicks = Environment.TickCount64 + (long)RefreshInterval.TotalMilliseconds;
     }
 
-    public bool IsPinned => Current().SpkiPins.Count > 0 || legacyThumbprint is not null;
+    public bool IsPinned => Current().Pinned;
 
     public static ServerPinSet Load(AgentConfig config, ILogger logger)
     {
@@ -221,11 +233,15 @@ public sealed class ServerPinSet
             thumbprint = config.ServerCertThumbprint.Replace(":", "").Replace(" ", "").Trim().ToUpperInvariant();
 
         var set = new ServerPinSet(pins, thumbprint, logger, refresh => ReadRegistryPins(logger, refresh));
-        var count = set.current.SpkiPins.Count;
+        var snapshot = set.current;
         logger.LogInformation(
-            "Server certificate trust: {Pins} SPKI pin(s){Legacy}; {Mode}.",
-            count, thumbprint is null ? "" : " + legacy thumbprint",
-            count > 0 || thumbprint is not null ? "pinned" : "Windows chain validation");
+            "Server certificate trust from {Source}: {Pins} SPKI pin(s){Legacy}; {Mode}.",
+            snapshot.Source, snapshot.SpkiPins.Count,
+            snapshot.LegacyThumbprint is null ? "" : " + legacy thumbprint",
+            snapshot.Pinned ? "pinned" : "Windows chain validation");
+        if (snapshot.FromRegistry && (pins.Count > 0 || thumbprint is not null))
+            logger.LogInformation(
+                "Registry {Value} is set; ignoring appsettings.json serverpins/servercertthumbprint.", RegistryValue);
         return set;
     }
 
@@ -238,33 +254,38 @@ public sealed class ServerPinSet
         {
             if (Environment.TickCount64 < nextRefreshTicks)
                 return current;
-            var raw = readRegistry(true);
+            var read = readRegistry(true);
             Interlocked.Exchange(ref nextRefreshTicks,
                 Environment.TickCount64 + (long)RefreshInterval.TotalMilliseconds);
             var previous = current;
-            if (raw is null || string.Join("|", raw) == previous.RawRegistry)
+            if (read is null || read.Raw == previous.RawRegistry)
                 return previous;
 
-            var next = Build(raw);
+            var next = Build(read);
             current = next;
             if (next.Signature != previous.Signature)
                 logger.LogInformation(
-                    "Server pins changed: {Count} SPKI pin(s) [{Pins}]{Legacy}; {Mode}.",
+                    "Server pins changed, source {Source}: {Count} SPKI pin(s) [{Pins}]{Legacy}; {Mode}.",
+                    next.Source,
                     next.SpkiPins.Count,
                     string.Join(", ", next.SpkiPins.OrderBy(p => p, StringComparer.Ordinal).Select(p => p[..12])),
-                    legacyThumbprint is null ? "" : " + legacy thumbprint",
-                    next.SpkiPins.Count > 0 || legacyThumbprint is not null ? "pinned" : "Windows chain validation");
+                    next.LegacyThumbprint is null ? "" : " + legacy thumbprint",
+                    next.Pinned ? "pinned" : "Windows chain validation");
             return next;
         }
     }
 
     // Malformed-pin warnings only fire when the raw registry text changes.
-    private Snapshot Build(List<string> raw)
+    private Snapshot Build(RegistryRead read)
     {
-        var pins = new HashSet<string>(configPins, StringComparer.Ordinal);
-        foreach (var pin in raw)
+        if (!read.Present)
+            return new Snapshot(new HashSet<string>(configPins, StringComparer.Ordinal),
+                legacyThumbprint, FromRegistry: false, read.Raw);
+
+        var pins = new HashSet<string>(StringComparer.Ordinal);
+        foreach (var pin in read.Values)
             AddPin(pins, pin, "registry", logger);
-        return new Snapshot(pins, string.Join("|", raw));
+        return new Snapshot(pins, LegacyThumbprint: null, FromRegistry: true, read.Raw);
     }
 
     public bool Validate(X509Certificate? certificate, SslPolicyErrors errors, AgentHttpChannel channel)
@@ -276,7 +297,7 @@ public sealed class ServerPinSet
         }
 
         var snapshot = Current();
-        if (snapshot.SpkiPins.Count == 0 && legacyThumbprint is null)
+        if (!snapshot.Pinned)
         {
             if (errors == SslPolicyErrors.None)
                 return true;
@@ -293,8 +314,8 @@ public sealed class ServerPinSet
             var spki = SpkiSha256(leaf);
             if (snapshot.SpkiPins.Contains(spki))
                 return true;
-            if (legacyThumbprint is not null
-                && string.Equals(leaf.GetCertHashString(), legacyThumbprint, StringComparison.OrdinalIgnoreCase))
+            if (snapshot.LegacyThumbprint is not null
+                && string.Equals(leaf.GetCertHashString(), snapshot.LegacyThumbprint, StringComparison.OrdinalIgnoreCase))
                 return true;
 
             logger.LogError(
@@ -327,9 +348,10 @@ public sealed class ServerPinSet
     }
 
     // Startup keeps whatever views read. A refresh returns null on any failed view
-    // so a transient denial cannot silently drop pins (and the server with them).
-    private static List<string>? ReadRegistryPins(ILogger logger, bool refresh)
+    // so a transient denial cannot silently swap the pin source (and the server with it).
+    private static RegistryRead? ReadRegistryPins(ILogger logger, bool refresh)
     {
+        var present = false;
         var found = new List<string>();
         foreach (var view in new[] { RegistryView.Registry64, RegistryView.Registry32 })
         {
@@ -337,7 +359,10 @@ public sealed class ServerPinSet
             {
                 using var machine = RegistryKey.OpenBaseKey(RegistryHive.LocalMachine, view);
                 using var key = machine.OpenSubKey(AgentConfigProvider.RegistryPath);
-                switch (key?.GetValue(RegistryValue))
+                var value = key?.GetValue(RegistryValue);
+                if (value is not null)
+                    present = true;
+                switch (value)
                 {
                     case string[] many:
                         found.AddRange(many);
@@ -357,6 +382,6 @@ public sealed class ServerPinSet
                 logger.LogWarning(error, "Registry read of {Value} in {View} view failed", RegistryValue, view);
             }
         }
-        return found;
+        return new RegistryRead(present, found);
     }
 }

agent/ShopDbEndpointAgent.Core/CommandExecutor.cs

@@ -145,10 +145,16 @@ public sealed class CommandExecutor
             // installer pinned by SHA-256 and runs it silently. Both download
             // the body themselves with the collector credential and fail closed.
             case "run_script":
+            {
+                var auth = new DownloadAuth();
                 return RequireTarget(action, target)
-                    ?? await RunScript(target, claim.Script, token);
+                    ?? auth.Stamp(await RunScript(target, claim.Script, auth, token));
+            }
             case "update_app":
-                return await UpdateApp(claim.App, token);
+            {
+                var auth = new DownloadAuth();
+                return auth.Stamp(await UpdateApp(claim.App, auth, token));
+            }
 
             // Fixed destructive action: reboot this endpoint immediately. Runs
             // as the SYSTEM agent, so shutdown does not need a user session.
@@ -1158,7 +1164,7 @@ public sealed class CommandExecutor
     // is to trigger something already installed on the endpoint, use
     // scheduledtask_run instead: it is a much smaller grant.
     private async Task<CommandResult> RunScript(
-        string name, ScriptDeliveryClaim? script, CancellationToken token)
+        string name, ScriptDeliveryClaim? script, DownloadAuth auth, CancellationToken token)
     {
         if (script is null || string.IsNullOrWhiteSpace(script.InstallPath))
         {
@@ -1211,10 +1217,7 @@ public sealed class CommandExecutor
 
             using var client = httpFactory.CreateClient(
                 AgentHttpChannel.Download, TimeSpan.FromMinutes(5));
-            using var httpRequest = new HttpRequestMessage(HttpMethod.Get, scriptUrl);
-
-            using var response = await client.SendAsync(
-                httpRequest, HttpCompletionOption.ResponseHeadersRead, token);
+            using var response = await SendDownloadAsync(client, scriptUrl, "run_script", auth, token);
             if (!response.IsSuccessStatusCode)
             {
                 return new CommandResult
@@ -1872,7 +1875,8 @@ public sealed class CommandExecutor
     // credential, verifies the SHA-256 (when the server pinned one), then runs
     // an MSI via msiexec /qn or an EXE with the version's SilentArgs. Anything
     // missing or mismatched fails closed.
-    private async Task<CommandResult> UpdateApp(AppDeliveryClaim? app, CancellationToken token)
+    private async Task<CommandResult> UpdateApp(
+        AppDeliveryClaim? app, DownloadAuth auth, CancellationToken token)
     {
         if (app is null || string.IsNullOrWhiteSpace(app.InstallPath))
         {
@@ -1910,10 +1914,7 @@ public sealed class CommandExecutor
 
             using var client = httpFactory.CreateClient(
                 AgentHttpChannel.Download, TimeSpan.FromMinutes(10));
-            using var httpRequest = new HttpRequestMessage(HttpMethod.Get, packageUrl);
-
-            using var response = await client.SendAsync(
-                httpRequest, HttpCompletionOption.ResponseHeadersRead, token);
+            using var response = await SendDownloadAsync(client, packageUrl, "update_app", auth, token);
             if (!response.IsSuccessStatusCode)
             {
                 return new CommandResult
@@ -2010,6 +2011,64 @@ public sealed class CommandExecutor
         }
     }
 
+    // What a download had to do about a revoked credential, stamped onto the
+    // command result (message suffix + detail.authfallback) however it ends.
+    private sealed class DownloadAuth
+    {
+        public string? Note { get; set; }
+
+        public CommandResult Stamp(CommandResult result)
+        {
+            if (Note is null)
+                return result;
+            result.Message = $"{result.Message} ({Note})";
+            result.Detail ??= new Dictionary<string, object>();
+            result.Detail["authfallback"] = Note;
+            return result;
+        }
+    }
+
+    // GET for run_script/update_app bodies. A command claimed before a revoke
+    // downloads with the revoked credential and gets 401 CREDENTIAL_INVALID; the
+    // auth handler has already deleted it and woken enrollment. Resend ONCE,
+    // which picks up the shared key (or a credential enrolled meanwhile).
+    private async Task<HttpResponseMessage> SendDownloadAsync(
+        HttpClient client, string url, string action, DownloadAuth auth, CancellationToken token)
+    {
+        using var request = new HttpRequestMessage(HttpMethod.Get, url);
+        var response = await client.SendAsync(
+            request, HttpCompletionOption.ResponseHeadersRead, token);
+        if (response.StatusCode != System.Net.HttpStatusCode.Unauthorized
+            || !request.Headers.Contains(AgentAuthHandler.CredentialHeader))
+            return response;
+
+        // Buffered by the auth handler on 401.
+        var body = await response.Content.ReadAsStringAsync(token);
+        if (AgentServerError.TryParse(body)?.Code != AgentAuthErrorCodes.CredentialInvalid)
+            return response;
+
+        var credentials = httpFactory.Credentials;
+        if (credentials.Current is null && credentials.SharedKey is null)
+        {
+            auth.Note = "endpoint credential was revoked and no shared key is configured to retry the download; "
+                + "the PC must re-enroll";
+            logger.LogWarning("{Action} download refused the revoked endpoint credential; no shared key to retry with",
+                action);
+            return response;
+        }
+
+        response.Dispose();
+        using var retry = new HttpRequestMessage(HttpMethod.Get, url);
+        var second = await client.SendAsync(
+            retry, HttpCompletionOption.ResponseHeadersRead, token);
+        var via = retry.Headers.Contains(AgentAuthHandler.SharedKeyHeader)
+            ? "the shared key" : "the newly enrolled credential";
+        auth.Note = $"endpoint credential was revoked; download retried once with {via}: HTTP {(int)second.StatusCode}";
+        logger.LogWarning("{Action} download refused the revoked endpoint credential; retried with {Via}: HTTP {Status}",
+            action, via, (int)second.StatusCode);
+        return second;
+    }
+
     // MSI installs run through msiexec /i <pkg> /qn (any SilentArgs appended);
     // an EXE runs directly with its version's SilentArgs (homegrown EXEs vary by
     // developer, so the switches are per-version server data, not guessed here).

agent/ShopDbEndpointAgent/ShopDbEndpointAgent.csproj

@@ -274,8 +274,10 @@
 	     fallback until the PC is enrolled, so the build also runs against an
 	     older server (a 404 on enroll parks enrollment for 6 hours). A revoked
 	     credential (CREDENTIAL_INVALID) is deleted and the PC re-enrolls without
-	     a restart. Server trust moves to SPKI SHA-256 pins (ServerPins registry
-	     value and serverpins in appsettings.json), checked by one AgentHttpFactory
+	     a restart, and a script or package download refused that way retries
+	     once. Server trust moves to SPKI SHA-256 pins (the ServerPins registry
+	     value when it exists, even empty; otherwise serverpins in
+	     appsettings.json), checked by one AgentHttpFactory
 	     that now builds every HttpClient, including script and package downloads
 	     that previously had their own unpinned handlers. The tray holds no
 	     secret any more: it reaches the service over the local named pipe