Build on macOS Mojave DP fails

[filipnavara]

Unfortunately the SecCertificateCopyPublicKey API has been deprecated and causes build failure due to the following warning (and -Werror):

/Users/filipnavara/Documents/GitHub/corefx/src/Native/Unix/System.Security.Cryptography.Native.Apple/pal_x509.c:55:21: error: 
      'SecCertificateCopyPublicKey' is deprecated: first deprecated in macOS 10.14
      [-Werror,-Wdeprecated-declarations]
    *pOSStatusOut = SecCertificateCopyPublicKey(cert, pPublicKeyOut);
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~
                    SecCertificateCopyKey
/Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/System/Library/Frameworks/Security.framework/Headers/SecCertificate.h:180:10: note: 
      'SecCertificateCopyPublicKey' has been explicitly marked deprecated here
OSStatus SecCertificateCopyPublicKey(SecCertificateRef certificate, SecKeyRef * __n...
         ^
1 error generated.

[filipnavara]

Workaround is to add add_compile_options(-Wno-deprecated-declarations) in src/Native/Unix/CMakeLists.txt.

[danmoseley]

@maryamariyan

[maryamariyan]

I tried the workaround as explained above as well as another approach where I copied over the dylib/a files for System.Security.Cryptography.Native.Apple from macOS High Sierra to Mojave to just run the tests.
With both approaches I successfully got around the compilation issue but I still get test failure on System.Security.Cryptography.X509Certificates test project.
Below is the callstack to the test failure on Mojave:

  Discovering: System.Security.Cryptography.X509Certificates.Tests
  Assertion Failed
  X509ChainGetStatusAtIndex returned unexpected error 2
 
     at Internal.Cryptography.Pal.SecTrustChainPal.ParseResults(SafeX509ChainHandle chainHandle, X509RevocationMode revocationMode) in /Users/dotnet-bot/git/corefx/src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/ChainPal.cs:line 247
     at Internal.Cryptography.Pal.SecTrustChainPal.Execute(DateTime verificationTime, Boolean allowNetwork, OidCollection applicationPolicy, OidCollection certificatePolicy, X509RevocationFlag revocationFlag) in /Users/dotnet-bot/git/corefx/src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/ChainPal.cs:line 210
     at Internal.Cryptography.Pal.ChainPal.BuildChain(Boolean useMachineContext, ICertificatePal cert, X509Certificate2Collection extraStore, OidCollection applicationPolicy, OidCollection certificatePolicy, X509RevocationMode revocationMode, X509RevocationFlag revocationFlag, DateTime verificationTime, TimeSpan timeout) in /Users/dotnet-bot/git/corefx/src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/ChainPal.cs:line 571
     at System.Security.Cryptography.X509Certificates.X509Chain.Build(X509Certificate2 certificate, Boolean throwOnException) in /Users/dotnet-bot/git/corefx/src/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/X509Chain.cs:line 118
     at System.Security.Cryptography.X509Certificates.X509Chain.Build(X509Certificate2 certificate) in /Users/dotnet-bot/git/corefx/src/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/X509Chain.cs:line 105
     at System.Security.Cryptography.X509Certificates.Tests.ChainTests.get_TrustsMicrosoftDotComRoot() in /Users/dotnet-bot/git/corefx/src/System.Security.Cryptography.X509Certificates/tests/ChainTests.cs:line 33
     at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor, Boolean wrapExceptions)
     at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture) in /Users/buildagent/agent/_work/482/s/src/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.cs:line 485
     at Xunit.NetCore.Extensions.ConditionalTestDiscoverer.Discover(ITestFrameworkDiscoveryOptions discoveryOptions, IMessageSink diagnosticMessageSink, ITestMethod testMethod, IEnumerable`1 testCases, Object[] conditionArguments)
     at Xunit.Sdk.XunitTestFrameworkDiscoverer.FindTestsForMethod(ITestMethod testMethod, Boolean includeSourceInformation, IMessageBus messageBus, ITestFrameworkDiscoveryOptions discoveryOptions)
     at Xunit.Sdk.XunitTestFrameworkDiscoverer.FindTestsForType(ITestClass testClass, Boolean includeSourceInformation, IMessageBus messageBus, ITestFrameworkDiscoveryOptions discoveryOptions)
     at Xunit.Sdk.TestFrameworkDiscoverer.FindTestsForTypeAndWrapExceptions(ITestClass testClass, Boolean includeSourceInformation, IMessageBus messageBus, ITestFrameworkDiscoveryOptions discoveryOptions)
     at Xunit.Sdk.TestFrameworkDiscoverer.<>c__DisplayClass26_0.<Find>b__0()
     at Xunit.Sdk.XunitWorkerThread.<>c.<QueueUserWorkItem>b__5_0(Object _)
     at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state) in /Users/buildagent/agent/_work/482/s/src/System.Private.CoreLib/shared/System/Threading/ExecutionContext.cs:line 166
     at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Task& currentTaskSlot) in /Users/buildagent/agent/_work/482/s/src/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs:line 2436
     at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state) in /Users/buildagent/agent/_work/482/s/src/System.Private.CoreLib/shared/System/Threading/ExecutionContext.cs:line 166
  /Users/dotnet-bot/git/corefx/bin/tests/System.Security.Cryptography.X509Certificates.Tests/netcoreapp-OSX-Debug-x64/RunTests.sh: line 124:  9740 Abort trap: 6           (core dumped) $RUNTIME_PATH/dotnet xunit.console.netcore.exe System.Security.Cryptography.X509Certificates.Tests.dll -xml testResults.xml -notrait Benchmark=true -notrait category=nonnetcoreapptests -notrait category=nonosxtests -notrait category=OuterLoop -notrait category=failing
  ~/git/corefx/src/System.Security.Cryptography.X509Certificates/tests
  ----- end 18:11:53 ----- exit code 134 ----------------------------------------------------------
  exit code 134 means SIGABRT Abort. Managed or native assert, or runtime check such as heap corruption, caused call to abort(). Core dumped.
/Users/dotnet-bot/git/corefx/Tools/tests.targets(492,5): warning MSB3073: The command "/Users/dotnet-bot/git/corefx/bin/tests/System.Security.Cryptography.X509Certificates.Tests/netcoreapp-OSX-Debug-x64/RunTests.sh /Users/dotnet-bot/git/corefx/bin/testhost/netcoreapp-OSX-Debug-x64/" exited with code 134. [/Users/dotnet-bot/git/corefx/src/System.Security.Cryptography.X509Certificates/tests/System.Security.Cryptography.X509Certificates.Tests.csproj]
/Users/dotnet-bot/git/corefx/Tools/tests.targets(500,5): error : One or more tests failed while running tests from 'System.Security.Cryptography.X509Certificates.Tests' please check /Users/dotnet-bot/git/corefx/bin/tests/System.Security.Cryptography.X509Certificates.Tests/netcoreapp-OSX-Debug-x64/testResults.xml for details! [/Users/dotnet-bot/git/corefx/src/System.Security.Cryptography.X509Certificates/tests/System.Security.Cryptography.X509Certificates.Tests.csproj]

The commit I prepared here shows (1) a diff with the workaround to the compilation issue, (2) ActiveIssue added to the failing test, and (3) points to the Debug.Fail statement that we reach only on Mojave (Removing it will help fix the tests but I wouldn’t be confident that this would be a proper fix, but simply pointing to the code snippet with the problem.)

I'm going to also run outerloop tests on Mojave and report on that in this issue.

cc: @danmosemsft @bartonjs

[danmoseley]

@bartonjs when you have time could you please finish the change to make this build successfully? Although ,at this point it is only for community members like @filipnavara since we won't switch our build machines until Mohave ships.

[bartonjs]

That means that the "I don't know what this code means" code tripped.

Defining DEBUGGING_UNKNOWN_VALUE will make it print the unknown value to the console right before asserting out.

https://github.com/dotnet/corefx/blob/master/src/Native/Unix/System.Security.Cryptography.Native.Apple/pal_x509chain.c#L195-L199

[bartonjs]

(And whatever the unknown codes are will need to be put into existing LTS builds to prevent failures on OS upgrade)

[filipnavara]

The problematic string is "GrayListedKey". Apparently the error existed since at least macOS 10.9 (according to the open source part of the security framework), possibly along with others:

CFStringRef kSecPolicyCheckBlackListedLeaf = CFSTR("BlackListedLeaf");
CFStringRef kSecPolicyCheckGrayListedLeaf  = CFSTR("GrayListedLeaf");
CFStringRef kSecPolicyCheckGrayListedKey   = CFSTR("GrayListedKey");
CFStringRef kSecPolicyCheckBlackListedKey  = CFSTR("BlackListedKey");

[maryamariyan]

I could avoid hitting the Debug.Fail statement by making this change.

Regardless, the test VerifyExpiration_LocalTime would still fail on Mojave on Assert.Equal (expecting true but is false).

@bartonjs Do you have any ideas on how to fix this test?

[danmoseley]

The Assert.Equal is in a test that operates on various test data. Which of the inputs (DateTime verificationTime, bool shouldBeValid, DateTimeKind kind) are failing?

[maryamariyan]

Outerloop tests: Interop+AppleCrypto+AppleCommonCryptoCryptographicException : User interaction is not allowed.

     System.Security.Cryptography.X509Certificates.Tests.X509StoreMutableTests_OSX.VerifyRemove [FAIL]
     System.Security.Cryptography.X509Certificates.Tests.X509StoreMutableTests_OSX.AddToStoreTwice [FAIL]
     System.Security.Cryptography.X509Certificates.Tests.X509StoreMutableTests_OSX.AddPrivateAfterPublic [FAIL]
     System.Security.Cryptography.X509Certificates.Tests.X509StoreMutableTests_OSX.AddToStore_Exportable [FAIL]
     System.Security.Cryptography.X509Certificates.Tests.X509StoreMutableTests_OSX.AddPublicAfterPrivate [FAIL]
     System.Security.Cryptography.X509Certificates.Tests.X509StoreMutableTests_OSX.RemovePublicDeletesPrivateKey [FAIL]
     System.Security.Cryptography.X509Certificates.Tests.X509StoreMutableTests_OSX.PersistKeySet_OSX [FAIL]

Assertion failure for the remaining tests:

     System.Security.Cryptography.X509Certificates.Tests.ChainTests.VerifyExpiration_LocalTime(verificationTime: 2014-10-15T00:00:00.0000000Z, shouldBeValid: True, kind: Utc) [FAIL]
     System.Security.Cryptography.X509Certificates.Tests.ChainTests.VerifyExpiration_LocalTime(verificationTime: 2014-10-14T17:00:00.0000000-07:00, shouldBeValid: True, kind: Local) [FAIL]
     System.Security.Cryptography.X509Certificates.Tests.ChainTests.VerifyExpiration_LocalTime(verificationTime: 2014-10-14T17:00:00.0000000, shouldBeValid: True, kind: Unspecified) [FAIL]
     System.Security.Cryptography.X509Certificates.Tests.ChainTests.VerifyExpiration_LocalTime(verificationTime: 2016-10-15T23:59:58.0000000Z, shouldBeValid: True, kind: Utc) [FAIL]
     System.Security.Cryptography.X509Certificates.Tests.ChainTests.VerifyExpiration_LocalTime(verificationTime: 2016-10-15T16:59:58.0000000-07:00, shouldBeValid: True, kind: Local) [FAIL]
     System.Security.Cryptography.X509Certificates.Tests.ChainTests.VerifyExpiration_LocalTime(verificationTime: 2016-10-15T16:59:58.0000000, shouldBeValid: True, kind: Unspecified) [FAIL]

     System.Security.Cryptography.X509Certificates.Tests.ChainTests.BuildChain [FAIL]
     System.Security.Cryptography.X509Certificates.Tests.CollectionTests.X509ChainElementCollection_IndexerVsEnumerator [FAIL]

[bartonjs]

I'm installing Mojave to see what's what.

[bartonjs]

On Mojave public beta (18A314k) I see the (inner and) outerloop tests passing provided I edit the shim to say that GrayListedKey is an ignored code. (After checking with the UI in Keychain Access that it reported the same cert/chain as trustworthy)

[maryamariyan]

@bartonjs thanks for checking. I'll push up the test fix we talked about into my currently open PR dotnet/corefx#30716

[danmoseley]

@bartonjs what about the other three codes noted above pulled from Apple sources? Do we need to add those in case they appear in some case we don't test yet?

[bartonjs]

Do we need to add those in case they appear in some case we don't test yet?

Very much no :).

Until we can see a thing that produces a particular code we don't know how it's actually handled, and what to map it to.

[danmoseley]

@bartonjs we should also decide when to flip our test matrix. It looks like 10.12 still has significant market share http://gs.statcounter.com/os-version-market-share/macos/desktop/worldwide . We could keep 10.12 and swap 10.14 for 10.13, or we could drop 10.12. Thoughts?

[karelz]

@maryamariyan did PRs dotnet/corefx#30716 and dotnet/corefx#30744 fix only part of the problem?
Do you expect more changes (e.g. PR dotnet/corefx#30815)? Do you expect those changes to flow into release/2.1 as well?