Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Desktop System.Globalization test need strong name skip verification entries to run #17313

Closed
Priya91 opened this issue May 17, 2016 · 37 comments

Comments

@Priya91
Copy link
Contributor

Priya91 commented May 17, 2016

All failing tests Log here

System.IO.FileLoadException : Could not load file or assembly 'System.Globalization.Extensions, Version=4.0.1.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' or one of its dependencies. Strong name validation failed. (Exception from HRESULT: 0x8013141A)\r\n---- System.Security.SecurityException : Strong name validation failed. (Exception from HRESULT: 0x8013141A)
+++++++++++++++++++
STACK TRACE:
at System.Globalization.Tests.GetStringComparerTests.Compare(String x, String y, String cultureName, CompareOptions options, Int32 expectedWindows, Int32 expectedICU) ----- Inner Stack Trace -----
@ellismg ellismg assigned tarekgh and unassigned ellismg May 17, 2016
@ellismg
Copy link
Contributor

ellismg commented May 17, 2016

@tarekgh Could this be related to desktop testing?

@tarekgh
Copy link
Member

tarekgh commented May 17, 2016

I'll take a look but I am wondering is this first time the test runs on Windows 7? I think this was working before. right?

@tarekgh
Copy link
Member

tarekgh commented May 17, 2016

@tarekgh Could this be related to desktop testing?

@ellismg yes looks so.

@stephentoub what version of framework installed on that Windows 7 machine?

@Priya91
Copy link
Contributor Author

Priya91 commented May 17, 2016

@tarekgh 4.6.1 full framework, release is 394271

@tarekgh
Copy link
Member

tarekgh commented May 18, 2016

@krwq the problem is not api-*, the problem is on desktop, the signing verification is applied and because our assemblies are not fully signed we fail to load it. the problem doesn't happen in most of the machines because on these machine the signing verification is skipped by setting some registry key.

@ellismg is going to patch the failing machines for now to include these registry keys to unblock the ci.
@ellismg do you think we need to keep this for RTM or we can push to 1.1.0? as you know there is no easy solution here so it'll need to have some more investigations.

@ellismg
Copy link
Contributor

ellismg commented May 18, 2016

@ellismg do you think we need to keep this for RTM or we can push to 1.1.0? as you know there is no easy solution here so it'll need to have some more investigations.

The thing we should ensure, before RTM, is that these tests work when using a fully signed version of the assembly when skip verification is not enabled. If we do that manually, I think that's sufficient.

I'll take this work item to track fixing CI, and we should file two more, one to do the manual validation and one to figure out how we want this to work long term.

@ellismg ellismg assigned ellismg and unassigned tarekgh May 18, 2016
@tarekgh
Copy link
Member

tarekgh commented May 18, 2016

@ellismg

The thing we should ensure, before RTM, is that these tests work when using a fully signed version of the assembly when skip verification is not enabled. If we do that manually, I think that's sufficient.

I have tried to run the test with the fully signed library (got it from myget.org) and it ran without any complains. I tried that on the test failing with this issue. The question now how we'll force our runs to use the official packages instead of the locally built assemblies. that is I don't know yet.

CC @ericstj to get his opinion.

@ericstj
Copy link
Member

ericstj commented May 18, 2016

Shouldn't OSS-signing alleviate the need for even skip verification? That was the whole point.

@ellismg
Copy link
Contributor

ellismg commented May 18, 2016

Shouldn't OSS-signing alleviate the need for even skip verification? That was the whole point.

Do you understand the cases where that doesn't work, on Desktop? I thought that OSS-signing was not fully supported on desktop, just on CoreCLR.

@ericstj
Copy link
Member

ericstj commented May 18, 2016

@gkhanna79 can fill in. My impression was that it worked for Desktop so long as you don't try to install to the GAC.

@gkhanna79
Copy link
Member

I am not aware of the details of OSS signing and how it works on Desktop. @jkotas Do you know? @terrajobst may know as well.

@tarekgh
Copy link
Member

tarekgh commented May 18, 2016

cc @weshaggard

To clarify, when building a library we'll use MSFT.snk to sign the assembly with SignType=oss. the produced assembly cannot get loaded on the desktop without skipping the signing verification.

for the test assemblies we sign it with Test.snk and having SignType=real, this one seems to load on the desktop.

@tarekgh
Copy link
Member

tarekgh commented May 19, 2016

cc @nguerrera too

@nguerrera
Copy link
Contributor

nguerrera commented May 19, 2016

In addition to not being able to GAC, at least shadow-copying does not work with "OSS" signing. How are these tests getting executed on desktop. xunit defaults to shadow copying. The command line runner has a flag to prevent it and you can also put the following in an app.config of your test project for the VS integration to honor it:

<appSettings> 
  <add key="xunit.shadowCopy" value="false" /> 
</appSettings> 

@tarekgh
Copy link
Member

tarekgh commented May 19, 2016

@nguerrera we run xunit.console.exe and passing the parameters -noshadow which take care with this issue.

@nguerrera
Copy link
Contributor

@tarekgh Odd. Did something in the build switch the signing from "OSS" to delay-signed mistakenly?

@nguerrera
Copy link
Contributor

IOW, are we sure SignType=oss is doing what we think it does?

@tarekgh
Copy link
Member

tarekgh commented May 19, 2016

I checked the assembly signing bit and it shows it is signed but yet fail to pass the signing verification.

@nguerrera
Copy link
Contributor

nguerrera commented May 19, 2016

So 'OSS' signed binaries work on desktop to the extent that signing verification is skipped as a performance optimization. If we hit a code path where verification kicks in, then it will (of course!) fail. I am curious what is special about this case that causes the verification to kick in.

We have had a good deal of success elsewhere (e.g. Roslyn) running unit tests on desktop with OSS-signed bits and no skip verification entries registered. (Shadow copying was the only pain-point there.)

@tarekgh
Copy link
Member

tarekgh commented May 19, 2016

the interesting thing is the test assembly load fine. so the issue looks the difference between how we sign our library and how we sign the test. obviously we use different keys for signing and also we set the sign type to OSS for the library and we set it to real for the tests.

here is the stack that do the signing and then fail.

clr!AssemblyNative::LoadFile
clr!AssemblyNative::GetPostPolicyAssembly
clr!AppDomain::LoadDomainAssemblyInternal
clr!AppDomain::LoadDomainFile
clr!AppDomain::TryIncrementalLoad
clr!DomainAssembly::Allocate
clr!AssemblySecurityDescriptor::CheckAllowAssemblyLoad
clr!AssemblySecurityDescriptor::Resolve
clr!PEAssembly::VerifyStrongName
clr!PEImage::VerifyStrongName
clr!StrongNameSignatureVerification
clr!VerifySignature

note that the stack is upside down :-)

@tarekgh
Copy link
Member

tarekgh commented May 19, 2016

one more thing to mention is the library that failing the test is partial façade which has

[assembly: TypeForwardedTo(typeof(IdnMapping))]
[assembly: TypeForwardedTo(typeof(NormalizationForm))]

maybe this a factor in why the signing validation is triggered too but I am not sure yet.

@ellismg
Copy link
Contributor

ellismg commented May 19, 2016

I added skip verification entries to our Win7 machines to unblock outerloop CI. I'm keeping this bug open so we can figure out the right way to solve this issue, but I'm moving it to 1.1.

@ellismg ellismg changed the title System.Globalization tests failing in windows 7 with assembly strongname signing errors Desktop System.Globalization test need strong name skip verification entries to run May 19, 2016
@tarekgh
Copy link
Member

tarekgh commented May 19, 2016

with more investigation I found the library doesn't have the strong name signing flag

COMIMAGE_FLAGS_STRONGNAMESIGNED     =0x00000008

while corflags showing

Signed : 1

so I'll look why this bit is not set and I think this should take care with the signing problem

@tarekgh
Copy link
Member

tarekgh commented May 19, 2016

ok I think I understand what is going on here. the problem is the target CopyFilesToOutputDirectory get executed before the target OpenSourceSign. which mean we copy the assembly to the out folder before signing it.

CopyFilesToOutputDirectory get executed first because ValidateApiCompatForSrc depends on it. looks we need to try reorder the targets execution to fix this issue. the interesting thing, in the sign.tagets files I am seeing the comment

* BeforeTargets=CopyFilesToOutputDirectory -> does not work on Mono.

and we do the following

   <PrepareForRunDependsOn>OpenSourceSign;$(PrepareForRunDependsOn)</PrepareForRunDependsOn>

will try to look more but I hope @weshaggard will have a chance to recommend the best way we can fix that.

@nguerrera
Copy link
Contributor

nguerrera commented May 19, 2016

Are tools sufficiently updated in corefx build to replace the buildtools bit-twiddling with csc /publicsign?

@tarekgh
Copy link
Member

tarekgh commented May 19, 2016

@nguerrera is there specific csc version I can check?

@nguerrera
Copy link
Contributor

@tarekgh See if csc /? lists /publicsign? (cc @jaredpar)

@tarekgh
Copy link
Member

tarekgh commented May 19, 2016

looks we are using ""C:\Program Files (x86)\MSBuild\14.0\bin\csc.exe"" and the one I have is supporting /publicsign

@tarekgh
Copy link
Member

tarekgh commented May 19, 2016

By the way adding OpenSourceSign to the depends on targets of ValidateApiCompatForSrc fixed the issue. I'll let @weshaggard comment on that.

  <!-- ApiCompat for Implementation Assemblies  -->
  <Target Name="ValidateApiCompatForSrc"
          DependsOnTargets="OpenSourceSign;CopyFilesToOutputDirectory"
          Condition="'$(RunApiCompatForSrc)' == 'true' AND '$(RunApiCompat)' == 'true' AND Exists('$(ContractProject)')" >

@Priya91
Copy link
Contributor Author

Priya91 commented May 19, 2016

These failures are happening on Windows10 outerloop tests as well..

@tarekgh
Copy link
Member

tarekgh commented May 19, 2016

@Priya91 yes this can happen if the skip signing verification is not there. we can patch the machine till we submit the fix. I just want to @weshaggard review as he knows all details around the apicompat and signing targets

@ellismg
Copy link
Contributor

ellismg commented May 19, 2016

I added the skip entries and I am going to restart the W10 outer loop jobs.

@tarekgh
Copy link
Member

tarekgh commented May 20, 2016

thanks @ellismg for your help.

@tarekgh
Copy link
Member

tarekgh commented May 20, 2016

Talked to Wes offline and he mentioned the APICompat target causing other problems too. so he is going to fix that.

CC @ericeil

@tarekgh tarekgh assigned weshaggard and unassigned ellismg May 20, 2016
@weshaggard
Copy link
Member

I've got PR dotnet/buildtools#737 which should fix the Copy target from running before signing.

@weshaggard
Copy link
Member

Corefx should be fixed with dotnet/corefx#8720

@tarekgh
Copy link
Member

tarekgh commented May 20, 2016

thanks @weshaggard

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 1.0.0-rtm milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants