fix: skip subjects with odd-length backslash runs in sync guard (#28)

CHANGELOG.md

@@ -1,8 +1,7 @@
-<<<<<<< release-1.3
+- 1.3.4
+  - Sync subject guard now also skips certificates whose subject contains an odd-length run of literal backslash bytes (e.g. a CN ending in one `\`). The 1.3.3 guard admitted these and they aborted Command's Full Scan with "badly formatted directory string" once the gateway re-parsed the un-escaped subject (issue #28).
 - 1.3.3
   - Sync now skips bad/unparseable certificates returned from Google CAS instead of failing the sync.
-=======
->>>>>>> main
 - 1.3.2
   - Fixed Sans Being passed through Extensions Data, Google does not like this.
 - 1.3.1

GCPCAS.Tests/SubjectGuardTests.cs

@@ -0,0 +1,118 @@
+// Copyright 2025 Keyfactor
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+using System.Security.Cryptography;
+using System.Security.Cryptography.X509Certificates;
+using Keyfactor.Extensions.CAPlugin.GCPCAS.Client;
+
+namespace Keyfactor.Extensions.CAPlugin.GCPCASTests;
+
+/// <summary>
+/// Regression tests for the sync subject guard (issue #28). The 1.3.3 guard only ran
+/// <c>new X509Name(true, netCert.Subject)</c>, which does not throw on a CN containing an odd-length run of
+/// literal backslash bytes. Such a subject was therefore admitted, persisted, and then aborted Command's Full
+/// Scan with "badly formatted directory string" when the gateway un-escaped and re-parsed it on its search
+/// response.
+///
+/// Subject strings below are in .NET's <see cref="X509Certificate2.Subject"/> form, where literal backslash
+/// bytes appear verbatim (empirically verified - .NET does not backslash-double them and quotes
+/// separator-bearing values instead). These are pure unit tests and run under a plain <c>dotnet test</c>.
+/// </summary>
+public class SubjectGuardTests
+{
+    [Theory]
+    // Well-formed subjects survive the round trip.
+    [InlineData("CN=baseline-app-01.lab.test", true)]
+    [InlineData("CN=wellformed.lab.test", true)]
+    [InlineData("CN=host.lab.test, OU=PKI, O=Keyfactor Labs, C=US", true)]
+    // shape1: CN ends in ONE literal backslash (odd run) -> gateway un-escapes to a dangling "\" and the
+    // re-parse throws. This is the regression: must be rejected now (1.3.3 admitted it).
+    [InlineData(@"CN=shape1.lab.test\", false)]
+    // shape2: CN ends in TWO literal backslashes (even run) -> round-trips as a valid escaped pair. Survives,
+    // matching the lab reproduction, which saw only shape1 abort the scan.
+    [InlineData(@"CN=shape2.lab.test\\", true)]
+    // shape3: FOUR literal backslashes (even run) ahead of a real RDN separator -> survives.
+    [InlineData(@"CN=shape3.lab.test\\\\, OU=PKI, O=Keyfactor Labs", true)]
+    // Odd run in the middle of a value is just as unsafe as a trailing one.
+    [InlineData(@"CN=sha\pe.lab.test", false)]
+    [InlineData(@"CN=sha\\\pe.lab.test", false)]
+    public void SubjectSurvivesGatewayRoundTrip_ClassifiesSubjects(string dotNetSubject, bool expectedSurvives)
+    {
+        bool survives = GCPCASClient.SubjectSurvivesGatewayRoundTrip(dotNetSubject, out string failureReason);
+
+        Assert.Equal(expectedSurvives, survives);
+        if (expectedSurvives)
+        {
+            Assert.Null(failureReason);
+        }
+        else
+        {
+            Assert.False(string.IsNullOrEmpty(failureReason));
+        }
+    }
+
+    [Theory]
+    [InlineData("CN=nobackslash.lab.test", false)]
+    [InlineData(@"CN=one\backslash", true)]        // single -> odd
+    [InlineData(@"CN=two\\backslash", false)]      // pair -> even
+    [InlineData(@"CN=three\\\backslash", true)]    // three -> odd
+    [InlineData(@"CN=four\\\\backslash", false)]   // four -> even
+    [InlineData(@"CN=trailing\", true)]            // single trailing -> odd
+    [InlineData(@"CN=a\b\c", true)]                // two separate single (odd) runs
+    [InlineData(@"CN=a\\b\\c", false)]             // two separate pair (even) runs
+    public void HasOddBackslashRun_DetectsOddRuns(string value, bool expected)
+    {
+        Assert.Equal(expected, GCPCASClient.HasOddBackslashRun(value));
+    }
+
+    [Fact]
+    public void RealCertificate_WithTrailingBackslashCn_IsRejected()
+    {
+        // End-to-end shape check: build a real cert whose CN ends in a literal backslash byte (as GCP CAS
+        // would accept and issue), then feed the .NET subject string through the guard. This is the exact
+        // shape from the lab reproduction (shape1).
+        string pem = SelfSignedPemWithCommonName(@"shape1.lab.test\");
+        using X509Certificate2 cert = X509Certificate2.CreateFromPem(pem);
+
+        // Sanity: .NET renders the single literal backslash verbatim (not doubled).
+        Assert.EndsWith(@"\", cert.Subject);
+        Assert.DoesNotContain(@"\\", cert.Subject);
+
+        Assert.False(GCPCASClient.SubjectSurvivesGatewayRoundTrip(cert.Subject, out string failureReason));
+        Assert.False(string.IsNullOrEmpty(failureReason));
+    }
+
+    [Fact]
+    public void RealCertificate_WithWellFormedCn_IsAccepted()
+    {
+        string pem = SelfSignedPemWithCommonName("baseline-app-01.lab.test");
+        using X509Certificate2 cert = X509Certificate2.CreateFromPem(pem);
+
+        Assert.True(GCPCASClient.SubjectSurvivesGatewayRoundTrip(cert.Subject, out string failureReason));
+        Assert.Null(failureReason);
+    }
+
+    private static string SelfSignedPemWithCommonName(string commonNameValue)
+    {
+        var builder = new X500DistinguishedNameBuilder();
+        builder.AddCommonName(commonNameValue);
+        X500DistinguishedName subject = builder.Build();
+
+        using RSA rsa = RSA.Create(2048);
+        var request = new CertificateRequest(subject, rsa, HashAlgorithmName.SHA256, RSASignaturePadding.Pkcs1);
+        using X509Certificate2 cert = request.CreateSelfSigned(
+            DateTimeOffset.UtcNow.AddDays(-1), DateTimeOffset.UtcNow.AddDays(1));
+        return cert.ExportCertificatePem();
+    }
+}

GCPCAS/Client/GCPCASClient.cs

@@ -271,7 +271,6 @@ public async Task<int> DownloadAllIssuedCertificates(BlockingCollection<AnyCAPlu
                             continue;
                         }
                     }
-<<<<<<< release-1.3
                     AnyCAPluginCertificate pluginCertificate = AnyCAPluginCertificateFromGCPCertificate(certificate);
 
                     // Mirror the subject handling the AnyCA Gateway performs when it builds the
@@ -287,9 +286,6 @@ public async Task<int> DownloadAllIssuedCertificates(BlockingCollection<AnyCAPlu
                     }
 
                     certificatesBuffer.Add(pluginCertificate);
-=======
-                    certificatesBuffer.Add(AnyCAPluginCertificateFromGCPCertificate(certificate));
->>>>>>> main
                     numberOfCertificates++;
                     _logger.LogDebug($"Found Certificate with name {certificate.CertificateName.CertificateId} {this.ToString()}");
                 }
@@ -317,10 +313,7 @@ public async Task<int> DownloadAllIssuedCertificates(BlockingCollection<AnyCAPlu
         {
             certificatesBuffer.CompleteAdding();
             _logger.LogDebug($"Fetched {certificatesBuffer.Count} certificates from GCP over {pageNumber} pages.");
-<<<<<<< release-1.3
             _logger.LogInformation($"[SYNC-DIAG] Handed {numberOfCertificates} certificate(s) to the AnyCA Gateway buffer; skipped {skippedCertificates} certificate(s) with subjects the gateway cannot parse. Review the per-record [SYNC-DIAG]/[SYNC-SKIP] lines above for details.");
-=======
->>>>>>> main
         }
         _logger.MethodExit();
         return numberOfCertificates;
@@ -380,7 +373,6 @@ private AnyCAPluginCertificate AnyCAPluginCertificateFromGCPCertificate(Certific
             status = EndEntityStatus.REVOKED;
             revocationReason = (int)certificate.RevocationDetails.RevocationState;
         }
-<<<<<<< release-1.3
 
         string caRequestId = certificate.CertificateName.CertificateId;
         string pem = certificate.PemCertificate;
@@ -391,8 +383,6 @@ private AnyCAPluginCertificate AnyCAPluginCertificateFromGCPCertificate(Certific
         // compare against what the Gateway stores / returns to Command on the /v2/certificate/search response.
         LogCertificateContentDiagnostics(caRequestId, pem, status, revocationDate, revocationReason);
 
-=======
->>>>>>> main
         _logger.MethodExit();
         return new AnyCAPluginCertificate
         {
@@ -443,12 +433,21 @@ private void LogCertificateContentDiagnostics(string caRequestId, string pem, En
     }
 
     /// <summary>
-    /// Mirrors the subject parsing the AnyCA Gateway performs when it builds the /v2/certificate/search
-    /// response: <c>new Org.BouncyCastle.Asn1.X509.X509Name(true, netCert.Subject)</c>. That call throws on
-    /// subjects BouncyCastle cannot re-parse from .NET's string representation, which 500s the entire gateway
-    /// search page and aborts Command's CA sync. Returning <see langword="false"/> lets the sync skip the
-    /// certificate so it never enters the gateway database and can never break the downstream Command sync.
+    /// Mirrors the subject parsing the AnyCA Gateway performs on the /v2/certificate/search response and
+    /// returns <see langword="false"/> for subjects that would abort Command's CA sync, so the plugin can
+    /// skip the certificate before it ever enters the gateway database.
     /// </summary>
+    /// <remarks>
+    /// The gateway exercises the subject twice, and the two operations do not see the same string. On the way
+    /// in it stores the subject with literal backslash bytes escape-doubled; on the way out (the search
+    /// response) it un-escapes that stored form one level and hands the result to
+    /// <c>new Org.BouncyCastle.Asn1.X509.X509Name(true, ...)</c>. A value containing an odd-length run of
+    /// literal backslashes (e.g. a CN ending in one <c>\</c>) survives storage but un-escapes back to a
+    /// dangling escape (<c>CN=...\</c>), which <c>X509NameTokenizer</c> rejects with
+    /// "badly formatted directory string": the search page 500s and Command's sync aborts. The 1.3.3 guard
+    /// only ran <c>new X509Name(true, netCert.Subject)</c>, which does not throw on these shapes, so they were
+    /// admitted. <see cref="SubjectSurvivesGatewayRoundTrip"/> now checks the shape structurally instead.
+    /// </remarks>
     /// <param name="pem">The PEM certificate content that will be handed to the gateway.</param>
     /// <param name="subject">The parsed .NET subject string, when available (for logging).</param>
     /// <param name="failureReason">The exception message when parsing fails.</param>
@@ -461,15 +460,99 @@ private bool GatewayCanParseSubject(string pem, out string subject, out string f
         {
             using X509Certificate2 netCert = X509Certificate2.CreateFromPem(pem);
             subject = netCert.Subject;
-            // This is the exact operation the gateway performs and that throws on problematic subjects.
-            _ = new Org.BouncyCastle.Asn1.X509.X509Name(true, subject);
-            return true;
         }
         catch (Exception ex)
         {
             failureReason = ex.Message;
             return false;
         }
+
+        return SubjectSurvivesGatewayRoundTrip(subject, out failureReason);
+    }
+
+    /// <summary>
+    /// Returns whether an RFC 4514 subject string as produced by <see cref="X509Certificate2.Subject"/>
+    /// survives the AnyCA Gateway's store-then-re-parse round trip without aborting Command's CA sync.
+    /// </summary>
+    /// <remarks>
+    /// The gateway stores the subject with its literal backslash bytes escape-doubled, then un-escapes one
+    /// level and re-parses the result on its /v2/certificate/search response. A value containing a run of
+    /// literal backslashes of <em>odd</em> length collapses to an unbalanced escape on that round trip
+    /// (a lone trailing <c>\</c>, or a <c>\</c> in front of a non-escapable character), which the gateway's
+    /// <c>X509Name</c> tokenizer rejects with "badly formatted directory string" - 500ing the search page and
+    /// aborting the sync. Runs of even length round-trip cleanly (each pair is a valid escaped backslash),
+    /// which is why the lab reproduction saw only the single-backslash shape abort while the two- and
+    /// four-backslash shapes synced.
+    ///
+    /// Note this is a structural check, not a mirror of one BouncyCastle call: .NET's
+    /// <see cref="X509Certificate2.Subject"/> renders literal backslash bytes verbatim (it quotes values
+    /// containing separators rather than backslash-escaping them), and the exact throw lives in the gateway's
+    /// own BouncyCastle build, so checking the shape is more reliable across gateway/BouncyCastle versions
+    /// than re-running the gateway's parse locally. A defensive local parse is still attempted as a second
+    /// net.
+    /// </remarks>
+    /// <param name="dotNetSubject">The subject string in .NET's RFC 4514 form.</param>
+    /// <param name="failureReason">The reason the subject is unsafe; otherwise <see langword="null"/>.</param>
+    /// <returns><see langword="true"/> if the subject survives the round trip; otherwise <see langword="false"/>.</returns>
+    internal static bool SubjectSurvivesGatewayRoundTrip(string dotNetSubject, out string failureReason)
+    {
+        failureReason = null;
+
+        if (HasOddBackslashRun(dotNetSubject))
+        {
+            failureReason = "subject contains an attribute value with an odd-length run of literal backslash bytes; " +
+                "the AnyCA Gateway cannot round-trip this on its search response and it would abort Command's CA sync";
+            return false;
+        }
+
+        // Defense in depth: also reject anything BouncyCastle itself refuses to parse from the .NET string form.
+        try
+        {
+            _ = new Org.BouncyCastle.Asn1.X509.X509Name(true, dotNetSubject);
+        }
+        catch (Exception ex)
+        {
+            failureReason = ex.Message;
+            return false;
+        }
+
+        return true;
+    }
+
+    /// <summary>
+    /// Returns <see langword="true"/> if <paramref name="value"/> contains at least one maximal run of
+    /// backslash (<c>\</c>) characters whose length is odd. Such a run is what breaks the gateway's
+    /// escape-double / un-escape round trip. See <see cref="SubjectSurvivesGatewayRoundTrip"/>.
+    /// </summary>
+    internal static bool HasOddBackslashRun(string value)
+    {
+        if (string.IsNullOrEmpty(value))
+        {
+            return false;
+        }
+
+        int i = 0;
+        while (i < value.Length)
+        {
+            if (value[i] != '\\')
+            {
+                i++;
+                continue;
+            }
+
+            int runStart = i;
+            while (i < value.Length && value[i] == '\\')
+            {
+                i++;
+            }
+
+            if (((i - runStart) & 1) == 1)
+            {
+                return true;
+            }
+        }
+
+        return false;
     }
     /// <summary>
     /// Enrolls a certificate using a configured <see cref="ICreateCertificateRequestBuilder"/> and returns the result.

GCPCAS/GCPCAS.csproj

@@ -6,6 +6,9 @@
 		<RootNamespace>Keyfactor.Extensions.CAPlugin.GCPCAS</RootNamespace>
 		<AssemblyName>GCPCASCAPlugin</AssemblyName>
 	</PropertyGroup>
+	<ItemGroup>
+		<InternalsVisibleTo Include="GCPCAS.Tests" />
+	</ItemGroup>
 	<ItemGroup>
 		<PackageReference Include="Google.Cloud.Security.PrivateCA.V1" Version="3.9.0" />
 		<PackageReference Include="Keyfactor.AnyGateway.IAnyCAPlugin" Version="3.0.0" />