NetFX compatibility fixes for X500DistinguishedName. #30572

filipnavara · 2018-06-21T08:51:48Z

Fixes https://github.com/dotnet/corefx/issues/27466.

filipnavara · 2018-06-21T11:17:49Z

@bartonjs, can you review please?

bartonjs · 2018-06-21T15:14:35Z

...Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/X500NameEncoder.cs

@@ -51,7 +51,7 @@ internal static partial class X500NameEncoder
            {
                dnSeparator = "; ";
            }
-            else if ((flags & X500DistinguishedNameFlags.UseNewLines) == X500DistinguishedNameFlags.UseNewLines)
+            else if ((flags & (X500DistinguishedNameFlags.UseNewLines | X500DistinguishedNameFlags.UseCommas)) == X500DistinguishedNameFlags.UseNewLines)


Please add a comment to explain that explicit commas beats newline, but that commas are the implicit default.

bartonjs · 2018-06-21T15:16:39Z

src/System.Security.Cryptography.X509Certificates/tests/X500DistinguishedNameTests.cs

+            new object[]
+            {
+                "CN=GrapeCity inc., OU=Tools Development, O=GrapeCity inc., L=Sendai Izumi-ku, S=Miyagi, C=JP",
+                "308186310b3009060355040613024a50310f300d060355040813064d6979616769311830160603550407130f53656e64616920497a756d692d6b7531173015060355040a140e47726170654369747920696e632e311a3018060355040b1411546f6f6c7320446576656c6f706d656e74311730150603550403140e47726170654369747920696e632e"


Please break these long constants up into multiple lines.

bartonjs · 2018-06-21T15:18:26Z

src/System.Security.Cryptography.X509Certificates/tests/X500DistinguishedNameTests.cs

+            // Mono test case taken from old bug report
+            new object[]
+            {
+                "SERIALNUMBER=CVR:13471967-UID:121212121212, E=vhm@use.test.dk, CN=Hedeby's Møbelhandel - Salgsafdelingen, O=Hedeby's Møbelhandel // CVR:13471967, C=DK",


I don't know how reliably o-slash is going to roundtrip through various git clients and text editors. Please replace it with \u00d8.

Good point!

bartonjs · 2018-06-21T17:27:16Z

src/Common/src/System/Security/Cryptography/DerSequenceReader.cs

+            // Technically the T.61 encoding (code page 20261) should be used here, but many
+            // implementations don't follow that and use ISO 8859-1 instead. This matches the
+            // behavior of CryptoAPI on NetFX (https://github.com/dotnet/corefx/issues/27466).
+            string t61String = System.Text.Encoding.GetEncoding("ISO-8859-1").GetString(_data, _position, contentLength);


I'm guessing that it's actually Windows-1252, or, more transparently:

char[] chars = new char[contentLength]; for (int i = 0; i < contentLength; i++) { chars[i] = (char)_data[_position + i]; } string t61string = new string(chars);

I actually tried couple of 1252 characters on Windows that are missing in iso-8859-1 and they were missing from the output.

Sounds like a test to add 😄

Turns out it is neither 1252 nor iso-8859-1. Windows actually interprets it as UTF-8. I will add test case for that.

bartonjs · 2018-06-21T19:33:20Z

src/System.Security.Cryptography.X509Certificates/tests/X500DistinguishedNameTests.cs

+            new object[]
+            {
+                "CN=\u00A2",
+                "300D310B300906035504061402C2A2"


0603550406 is "2.5.4.6" => id-countryName ("C=", not "CN=").

bartonjs · 2018-06-21T19:48:16Z

on macOS the "Møbelhandel" didn't decode correctly (it got a FFFD replacement character).

So maybe it's something more nuanced. UTF-8 unless it had a decode error (e.g. had to add an FFFD), and Windows-1252 otherwise.

Looks like, maybe, this could be

private static Text.Encoding MakeT61Encoding()
{
    // Using Clone() allows DecoderFallback to be assigned.
    var utf8 = Text.Encoding.Utf8.Clone();
    utf8.DecoderFallback = new TheFallbackWouldHaveToBeWritten();
    // Until further need.
    utf8.EncoderFallback = EncoderFallback.ExceptionFallback;
}

Or

s_t61Utf8 = new Utf8Encoding(false, true);

try
{
    s_t61Utf8.GetWhatever(...);
}
catch (DecoderFallbackException)
{
    bytes-to-chars it is, then();
}

depending on if the fallback is "whole string" or "per character".

filipnavara · 2018-06-21T20:08:41Z

Looks like macOS is not the only problematic one. The OpenSSL decoder fails on the UTF-8 string. It could be easier to just find out what OpenSSL does and mimic that instead of Windows.

filipnavara · 2018-06-21T22:06:22Z

The NETFX test failure is unrelated to the changes.

bartonjs · 2018-06-22T15:16:38Z

src/System.Security.Cryptography.X509Certificates/tests/X500DistinguishedNameTests.cs

+            new object[]
+            {
+                "C=\u00C2\u00A2",
+                "300D310B300906035504061402C2A2"


What happens if the payload had a UTF-8 multi-byte character followed by an invalid UTF-8 sequence?

300E310C300A06035504071403C2A2F8 -> L=Â¢ø (https://dotnetfiddle.net/ERdAyB), will add it to the tests

bartonjs · 2018-06-22T15:20:38Z

src/System.Security.Cryptography.X509Certificates/tests/X500DistinguishedNameTests.cs

+        [Theory]
+        [MemberData(nameof(T61CasesLatin1))]
+        [PlatformSpecific(TestPlatforms.AnyUnix & ~TestPlatforms.OSX)]
+        public static void T61StringsLatin1(string expected, string hexEncoded)


I think I'm fine with dumping X500NameEncoder.OpenSslDecode.cs as part of this change, and just using the same managed implementation on macOS and Linux. It'll cut down on a lot of SafeHandles and P/Invokes, as well as unify the behaviors.

bartonjs · 2018-06-22T23:32:33Z

@dotnet-bot Test NETFX x86 Release Build please (now that #30565 has merged and X509Certificates tests are enabled for netfx)

bartonjs · 2018-06-23T00:44:35Z

src/Common/src/System/Security/Cryptography/DerSequenceReader.cs

+            try
+            {
+                var utf8 = (System.Text.Encoding)System.Text.Encoding.UTF8.Clone();
+                utf8.DecoderFallback = System.Text.DecoderFallback.ExceptionFallback;


This could be simplified to new Utf8Encoding(false, true) since it's using the ExceptionFallback.

I'd probably promote this and the ISO-8859-1 encoding objects to statics, no strong opinion on fieldinit or LazyInitializer.EnsureInitialized or just if (s_utf8WithExceptionsEncoding == null) { s_utf8WithExceptionsEncoding = new Utf8Encoding(false, true); }

I didn't use new Utf8Encoding(false, true) the first time because of the additional reference to System.Text.Encoding.Extensions. LazyInitializer.EnsureInitialized seems like a reasonable option since it's already used elsewhere in the class.

bartonjs · 2018-06-23T00:45:15Z

Aside from the last comment about the two Encoding objects I think I'm happy with it. @stephentoub would you mind giving it a once-over?

stephentoub

Other than the one issue about not creating a new Encoding for each use, LGTM.

bartonjs · 2018-06-24T04:30:14Z

Thanks, @filipnavara :)

filipnavara mentioned this pull request Jun 21, 2018

WIP: Use AsnEncodedData, X.509 extension and X.500 name classes from CoreFX. mono/mono#9250

Closed

3 tasks

NetFX compatibility fixes for X500DistinguishedName.

3f3e204

filipnavara force-pushed the x500name branch from 8be71a2 to 3f3e204 Compare June 21, 2018 08:58

filipnavara added 2 commits June 21, 2018 11:19

Fix build.

11d1ef5

Fix test cases and OpenSSL implementation.

127d402

filipnavara changed the title ~~WIP: NetFX compatibility fixes for X500DistinguishedName.~~ NetFX compatibility fixes for X500DistinguishedName. Jun 21, 2018

bartonjs reviewed Jun 21, 2018

View reviewed changes

Address review comments and fix T61 implementation.

4c5be32

bartonjs reviewed Jun 21, 2018

View reviewed changes

Fix cut & paste error.

12de5a7

filipnavara added 3 commits June 21, 2018 22:57

Another attempt to make the T61 unit tests pass.

b33dab0

Add T61 unit test for OpenSSL.

eb3304c

Fix build on CoreFX.

a3a8881

bartonjs reviewed Jun 22, 2018

View reviewed changes

filipnavara added 2 commits June 22, 2018 18:45

Drop X500NameEncoder.OpenSslDecode and update unit tests.

c9c5272

Update .csproj file.

e876d81

bartonjs reviewed Jun 23, 2018

View reviewed changes

stephentoub approved these changes Jun 23, 2018

View reviewed changes

filipnavara added 2 commits June 23, 2018 10:04

Use lazy initialization for encoding instances in DerSequenceReader.

416415b

Fix build.

a5892e7

bartonjs approved these changes Jun 24, 2018

View reviewed changes

bartonjs merged commit 5e3aecf into dotnet:master Jun 24, 2018

filipnavara mentioned this pull request Jul 6, 2018

OpenSSL: Drop pal_asn1_print in favor of the managed code #30868

Merged

karelz added this to the 3.0 milestone Jul 8, 2018

baulig mentioned this pull request Jul 24, 2018

Make CertificateData.ReadReverseRdns() accept T61String. #31333

Merged

bartonjs mentioned this pull request Sep 13, 2018

Remove the ActiveIssue attribute from a disabled-but-passing test #32286

Merged

omajid mentioned this pull request Jan 19, 2019

[release/2.1] Add hybrid support for OpenSSL 1.0 and 1.1 #34443

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NetFX compatibility fixes for X500DistinguishedName. #30572

NetFX compatibility fixes for X500DistinguishedName. #30572

filipnavara commented Jun 21, 2018 •

edited

Loading

filipnavara commented Jun 21, 2018

bartonjs Jun 21, 2018

bartonjs Jun 21, 2018

bartonjs Jun 21, 2018

filipnavara Jun 21, 2018

bartonjs Jun 21, 2018

filipnavara Jun 21, 2018

bartonjs Jun 21, 2018

filipnavara Jun 21, 2018

bartonjs Jun 21, 2018

bartonjs commented Jun 21, 2018

filipnavara commented Jun 21, 2018

filipnavara commented Jun 21, 2018

bartonjs Jun 22, 2018

filipnavara Jun 22, 2018

bartonjs Jun 22, 2018

filipnavara Jun 22, 2018

bartonjs commented Jun 22, 2018

bartonjs Jun 23, 2018

filipnavara Jun 23, 2018

bartonjs commented Jun 23, 2018

stephentoub left a comment

bartonjs commented Jun 24, 2018

NetFX compatibility fixes for X500DistinguishedName. #30572

NetFX compatibility fixes for X500DistinguishedName. #30572

Conversation

filipnavara commented Jun 21, 2018 • edited Loading

filipnavara commented Jun 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartonjs commented Jun 21, 2018

filipnavara commented Jun 21, 2018

filipnavara commented Jun 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartonjs commented Jun 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartonjs commented Jun 23, 2018

stephentoub left a comment

Choose a reason for hiding this comment

bartonjs commented Jun 24, 2018

filipnavara commented Jun 21, 2018 •

edited

Loading