Skip to content

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Oct 8, 2025

  • Fix the deserialization bug in X509CertificateEndpointIdentity.cs by replacing reader.ReadContentAsString() with reader.ReadElementString()
  • Create unit test to verify X509Certificate element can be correctly deserialized from XML
  • Verify the fix aligns with the working implementation in dotnet-svcutil fork
  • Update tests to manually construct XML instead of depending on WriteContentsTo
  • Fix xunit analyzer error by using Assert.Single for single item collection check
  • Fix MacOS test failure by exporting certificates without private keys to avoid keychain issues
  • Fix SYSLIB0057 warning by using X509CertificateLoader.LoadCertificate instead of obsolete X509Certificate2(byte[]) constructor
Original prompt

This section details on the original issue you should resolve

<issue_title>The certificate specified in the X509Certificate EndpointIdentity can never be deserialized on the client side.</issue_title>
<issue_description>
EndpointIdentity in the server configuration:

<identity>
    <certificateReference x509FindType="FindByThumbprint" findValue="key" />
</identity>

Fragment of response with client side:

<Identity xmlns="http://schemas.xmlsoap.org/ws/2006/02/addressingidentity">
    <KeyInfo xmlns="http://www.w3.org/2000/09/xmldsig#">
        <X509Data>
            <X509Certificate>content</X509Certificate>
        </X509Data>
    </KeyInfo>
</Identity>

Deserialization code:
https://github.com/dotnet/wcf/blob/main/src/System.ServiceModel.Primitives/src/System/ServiceModel/X509CertificateEndpointIdentity.cs#L62-L65

while (reader.IsStartElement(XD.XmlSignatureDictionary.X509Certificate, XD.XmlSignatureDictionary.Namespace))
{
    reader.MoveToContent();
    X509Certificate2 certificate = new X509Certificate2(Convert.FromBase64String(reader.ReadContentAsString()));

reader.MoveToContent does not go to X509Certificate content, ReadContentAsString will always return null

This leads to System.Security.Cryptography.CryptographicException: "m_safeCertContext is an invalid handle."
</issue_description>

<agent_instructions>Add unit tests to make sure it can correctly read an X509 based entity record.</agent_instructions>

Comments on the Issue (you are @copilot in this section)

@mconnew That code should have been ```c# X509Certificate2 certificate = new X509Certificate2(Convert.FromBase64String(reader.ReadElementString())); ``` This is most likely an artifact from when we first started porting WCF, a lot of the XmlReader api's were missing and we replaced them with what we thought were equivalents. Looks like we got this one wrong.
Fixes #5838

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@mconnew
Copy link
Member

mconnew commented Oct 8, 2025

@copilot, the tests depend on X509CertificateEndpointIdentity.WriteContentsTo having a correct implementation. Modify the tests to not depend on the writing implementation being correct.

@Copilot Copilot AI changed the title [WIP] Fix deserialization issue for X509Certificate EndpointIdentity Fix X509CertificateEndpointIdentity deserialization bug causing CryptographicException Oct 8, 2025
@Copilot Copilot AI requested a review from mconnew October 8, 2025 23:29
Copilot finished work on behalf of mconnew October 8, 2025 23:29
Copy link
Contributor Author

Copilot AI commented Oct 8, 2025

@copilot, the tests depend on X509CertificateEndpointIdentity.WriteContentsTo having a correct implementation. Modify the tests to not depend on the writing implementation being correct.

Modified the tests to manually construct the XML instead of using WriteContentsTo. Both tests now create the XML strings directly with the expected structure and base64-encoded certificate data. Commit: e6b6329

Copilot finished work on behalf of mconnew October 8, 2025 23:33
@mconnew
Copy link
Member

mconnew commented Oct 8, 2025

@copilot, an xunit code analyzer generated the following build error:

Do not use Assert.Equal() to check for collection size. Use Assert.Single instead. (https://xunit.net/xunit.analyzers/rules/xUnit2013)

Copy link
Contributor Author

Copilot AI commented Oct 8, 2025

@copilot, an xunit code analyzer generated the following build error:

Do not use Assert.Equal() to check for collection size. Use Assert.Single instead. (https://xunit.net/xunit.analyzers/rules/xUn...

Fixed by replacing Assert.Equal(1, deserializedIdentity.Certificates.Count) with Assert.Single(deserializedIdentity.Certificates) which also simplifies the test. Commit: d2bc70b

Copilot finished work on behalf of mconnew October 8, 2025 23:39
@mconnew
Copy link
Member

mconnew commented Oct 9, 2025

@copilot, the test is failing on MacOS with the following error:

X509CertificateEndpointIdentityTest.X509Certificate_Multiple_RoundTrip_Succeeds [FAIL]
      Interop+AppleCrypto+AppleCommonCryptoCryptographicException : The specified item is no longer valid. It may have been deleted from the keychain.
      Stack Trace:
           at Interop.AppleCrypto.X509CopyWithPrivateKey(SafeSecCertificateHandle certHandle, SafeSecKeyRefHandle privateKeyHandle, SafeKeychainHandle targetKeychain)
           at System.Security.Cryptography.X509Certificates.AppleCertificatePal.CopyWithPrivateKey(SafeSecKeyRefHandle privateKey)
           at System.Security.Cryptography.X509Certificates.AppleCertificatePal.CopyWithPrivateKey(RSA privateKey)
           at System.Security.Cryptography.X509Certificates.RSACertificateExtensions.CopyWithPrivateKey(X509Certificate2 certificate, RSA privateKey)
           at System.Security.Cryptography.X509Certificates.CertificateRequest.CreateSelfSigned(DateTimeOffset notBefore, DateTimeOffset notAfter)
        /_/src/System.ServiceModel.Primitives/tests/ServiceModel/X509CertificateEndpointIdentityTest.cs(100,0): at X509CertificateEndpointIdentityTest.CreateTestCertificate()
        /_/src/System.ServiceModel.Primitives/tests/ServiceModel/X509CertificateEndpointIdentityTest.cs(55,0): at X509CertificateEndpointIdentityTest.X509Certificate_Multiple_RoundTrip_Succeeds()
           at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
           at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

Copy link
Contributor Author

Copilot AI commented Oct 9, 2025

@copilot, the test is failing on MacOS with the following error:

X509CertificateEndpointIdentityTest.X509Certificate_Multiple_RoundTrip_Succeeds [FAIL]

...

Fixed by exporting and reimporting the certificates without private keys. The tests only need the certificate data for deserialization testing, not the private keys, so this avoids MacOS keychain issues. Commit: 28e9559

Copilot finished work on behalf of mconnew October 9, 2025 00:03
@mconnew
Copy link
Member

mconnew commented Oct 9, 2025

@copilot, building the tests is failing due to the following code analyzer build error:

'X509Certificate2.X509Certificate2(byte[])' is obsolete: 'Loading certificate data through the constructor or Import is obsolete. Use X509CertificateLoader instead to load certificates.' (https://aka.ms/dotnet-warnings/SYSLIB0057)

Copy link
Contributor Author

Copilot AI commented Oct 9, 2025

@copilot, building the tests is failing due to the following code analyzer build error:

'X509Certificate2.X509Certificate2(byte[])' is obsolete: 'Loading certificate data through the constructor...

Fixed by replacing new X509Certificate2(certData) with X509CertificateLoader.LoadCertificate(certData). Commit: b4a75a4

Copilot finished work on behalf of mconnew October 9, 2025 00:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The certificate specified in the X509Certificate EndpointIdentity can never be deserialized on the client side.

2 participants