diff --git a/CHANGELOG.md b/CHANGELOG.md index f6dd435..77b4ae0 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/GCPCAS.Tests/SubjectGuardTests.cs b/GCPCAS.Tests/SubjectGuardTests.cs new file mode 100644 index 0000000..bb72baf --- /dev/null +++ b/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; + +/// +/// Regression tests for the sync subject guard (issue #28). The 1.3.3 guard only ran +/// new X509Name(true, netCert.Subject), 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 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 dotnet test. +/// +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(); + } +} diff --git a/GCPCAS/Client/GCPCASClient.cs b/GCPCAS/Client/GCPCASClient.cs index 01320c0..061cc7e 100644 --- a/GCPCAS/Client/GCPCASClient.cs +++ b/GCPCAS/Client/GCPCASClient.cs @@ -271,7 +271,6 @@ public async Task DownloadAllIssuedCertificates(BlockingCollection DownloadAllIssuedCertificates(BlockingCollection>>>>>> main numberOfCertificates++; _logger.LogDebug($"Found Certificate with name {certificate.CertificateName.CertificateId} {this.ToString()}"); } @@ -317,10 +313,7 @@ public async Task DownloadAllIssuedCertificates(BlockingCollection>>>>>> 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 } /// - /// Mirrors the subject parsing the AnyCA Gateway performs when it builds the /v2/certificate/search - /// response: new Org.BouncyCastle.Asn1.X509.X509Name(true, netCert.Subject). 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 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 for subjects that would abort Command's CA sync, so the plugin can + /// skip the certificate before it ever enters the gateway database. /// + /// + /// 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 + /// new Org.BouncyCastle.Asn1.X509.X509Name(true, ...). A value containing an odd-length run of + /// literal backslashes (e.g. a CN ending in one \) survives storage but un-escapes back to a + /// dangling escape (CN=...\), which X509NameTokenizer rejects with + /// "badly formatted directory string": the search page 500s and Command's sync aborts. The 1.3.3 guard + /// only ran new X509Name(true, netCert.Subject), which does not throw on these shapes, so they were + /// admitted. now checks the shape structurally instead. + /// /// The PEM certificate content that will be handed to the gateway. /// The parsed .NET subject string, when available (for logging). /// The exception message when parsing fails. @@ -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); + } + + /// + /// Returns whether an RFC 4514 subject string as produced by + /// survives the AnyCA Gateway's store-then-re-parse round trip without aborting Command's CA sync. + /// + /// + /// 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 odd length collapses to an unbalanced escape on that round trip + /// (a lone trailing \, or a \ in front of a non-escapable character), which the gateway's + /// X509Name 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 + /// 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. + /// + /// The subject string in .NET's RFC 4514 form. + /// The reason the subject is unsafe; otherwise . + /// if the subject survives the round trip; otherwise . + 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; + } + + /// + /// Returns if contains at least one maximal run of + /// backslash (\) characters whose length is odd. Such a run is what breaks the gateway's + /// escape-double / un-escape round trip. See . + /// + 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; } /// /// Enrolls a certificate using a configured and returns the result. diff --git a/GCPCAS/GCPCAS.csproj b/GCPCAS/GCPCAS.csproj index 366c8df..eacf9da 100644 --- a/GCPCAS/GCPCAS.csproj +++ b/GCPCAS/GCPCAS.csproj @@ -6,6 +6,9 @@ Keyfactor.Extensions.CAPlugin.GCPCAS GCPCASCAPlugin + + +