search

dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.04k stars 4.03k forks source link

"Invalid public key" error with newer versions of csc #32502

Open kg opened 5 years ago

kg commented 5 years ago

Version Used: git 9e501093bc44a15361704aa057ebbdaf477fa9a2

Steps to Reproduce:

  1. Build csc
  2. Use roslyn csc and/or compiler server to build mono

Expected Behavior: Building for all assemblies should work

Actual Behavior: Building fails because csc can no longer load the signing key for nunit

--- PID=111075 TID=667 Ticks=232118991: ****Running C# compiler...
--- PID=111075 TID=667 Ticks=232119027: ****C# Compilation complete.
****Return code: 1
****Output:
error CS7027: Error signing output with public key from file '/home/kate/Projects/mono/mcs/nunit24/nunit.snk' -- Invalid public key.
error CS8102: Public signing was specified and requires a public key, but no public key was specified.
--- PID=111075 TID=667 Ticks=232119027: Client 349: End compilation

Visual studio & windows msbuild already have had this problem for some time, when I was updating the .csproj generator for mono I had to disable signing. It appears that command-line csc now has this problem as well. It was working for me a few weeks ago on an older rev of roslyn, though it's possible the signing just wasn't happening for some reason until now.

The key can be found here: https://github.com/mono/mono/blob/master/mcs/nunit24/nunit.snk

We currently keep a specific version of roslyn in-tree to use for mono builds, so this only shows up if you manually use a new version (I'm working on updating us to a new version & adding compiler server support)

kg commented 5 years ago

cc @jaredpar

kg commented 5 years ago

Can confirm from testing that our mono.snk file seems to still be parsed fine, so it's just nunit.snk. That matches my experience with VS on Windows. If I disable all signing (just disabling nunit doesn't seem to be enough) I can build again using current roslyn. So it's just a signing issue.

jaredpar commented 5 years ago

@kg can you try adding the following command line parameter and see if that changes things: /features:UseLegacyStrongNameProvider

kg commented 5 years ago

error CS2007: Unrecognized option: '/feature:UseLegacyStrongNameProvider'

jaredpar commented 5 years ago

@kg sorry I was missing an "s" on "features". Updated my previous comment with the correct argument.

Also can you provide some context to the origin of the nunit.snk file. How was it generated?

kg commented 5 years ago

It was probably generated by one of the nunit maintainers, they have one in their repo as well: https://github.com/nunit/nunit/blob/master/src/nunit.snk I know zero about all this signing stuff so I can't really come up with a reasonable guess.

kg commented 5 years ago

I'm having trouble with that switch turned on but I'll keep testing it. I got this error once:

Microsoft (R) Visual C# Compiler version 2.8.2.62916 (2ad4aabc)
Copyright (C) Microsoft Corporation. All rights reserved.

CSC     [build-linux] gensources.exe
CSC     [build-linux] Mono.Cecil.dll
error CS7027: Error signing output with public key from file '../mono.snk' -- mscoree.dll
../../build/library.make:335: recipe for target '../../class/lib/build-linux/tmp/Mono.Cecil.dll' failed

If I try building again, I get further and then Mono.Cecil is clearly a corrupt binary:

CSC     [build-linux] upload-to-sentry.exe
error CS0009: Metadata file '/home/kate/Projects/mono/mcs/class/lib/build-linux/Mono.Cecil.dll' could not be opened -- PE image doesn't contain managed metadata.

I'm not sure what to make of an error message that's just 'mscoree.dll'.

I'll try only setting that flag for nunit instead of for all compilations.

0xd4d commented 5 years ago

The snk file is missing bytes (private exponent). You can generate a new snk file or use the latest nunit snk file which should have the correct size.

kg commented 5 years ago

Thanks, I'll assume the file is corrupt and update it. It would be great if roslyn could produce better error messages for this, but it sounds like if it's a "regression" it's just that the signing path used to be more permissive.

jaredpar commented 5 years ago

@kg were you able to get it to sign using /features:UseLegacyStrongNameProvider?

If so then I know what change caused that problem and I can follow up on making the new code path as permissive as the old one. If there is still an error, even if it's different, then I'd like to keep digging on this because that's very unexpected.

kg commented 5 years ago

Sorry for the delay, each iteration on this takes 30+ minutes. Hooray, build systems!

MONO_PATH="./../../../class/lib/build:$MONO_PATH" CSC_SDK_PATH_DISABLED= /home/kate/Projects/mono/runtime/mono-wrapper  --aot-path=/home/kate/Projects/mono/mcs/class/lib/build --gc-params=nursery-size=64m /home/kate/Projects/roslyn/artifacts/bin/csc/Debug/net472/csc.exe /shared:monomake /codepage:65001 /nologo /noconfig /deterministic   -d:NET_4_0 -d:NET_4_5 -d:NET_4_6 -d:MONO -d:WIN_PLATFORM -nowarn:1699 -nostdlib /debug:portable -optimize /features:peverify-compat /langversion:latest  /d:StronglyNamedAssembly -warn:1 /publicsign /features:UseLegacyStrongNameProvider /keyfile:../../nunit.snk -r:./../../../class/lib/net_4_x-linux/System.Xml.dll -r:./../../../class/lib/net_4_x-linux/System.dll -r:./../../../class/lib/net_4_x-linux/mscorlib.dll  -sourcelink:./../../../build/common/sourcelink.json   -target:library -out:../../../class/lib/net_4_x-linux/nunit.framework.dll  @./../../../build/deps/linux_net_4_x__NUnit.Framework.dll.response
error CS7027: Error signing output with public key from file '/home/kate/Projects/mono/mcs/nunit24/nunit.snk' -- Invalid public key.
error CS8102: Public signing was specified and requires a public key, but no public key was specified.
../../../build/library.make:335: recipe for target '../../../class/lib/net_4_x-linux/nunit.framework.dll' failed
make[8]: *** [../../../class/lib/net_4_x-linux/nunit.framework.dll] Error 1
make[8]: Leaving directory '/home/kate/Projects/mono/mcs/nunit24/NUnitFramework/framework'
../../../build/rules.make:223: recipe for target 'do-all' failed

/home/kate/Projects/roslyn/artifacts/bin/csc/Debug/net472/csc.exe here is git 9e50109, as is the VBCSCompiler.exe that it's talking to over the monomake pipe. I think the command line is correct, unless having two /features switches is wrong and I need to comma (semicolon?) separate them? If two is wrong the 2nd should throw a command line parsing error, ideally.

jaredpar commented 5 years ago

@kg

Sorry for the delay, each iteration on this takes 30+ minutes

No problem. I just wanted to make sure we got to the bottom of this. This area has been problematic for us before and I just did a big switch here. Wanted to make sure I wasn't breaking a Mono scenario when I did it.

Hmm. Looks like even in legacy mode this still happens. That's truly odd here. There are a couple of explanations I can think of:

  1. The legacy mode just doesn't have the same fidelity that it was intended to.
  2. Mono wasn't actually signing before and it's just a strange coincedence that you fixed signing about the same time I checked my change in.

I'm betting on 1 being the case here. Let me play with this a bit.

jaredpar commented 5 years ago

@kg

Did some testing and I think that nunit.snk is just a corrupted key and it's never been used for signing. As an experiment I tried the native C# compiler (pre-roslyn) and even it can't process this key:

E:\temp> nativecsc /nologo /keyfile:nunit.snk test.cs
error CS1548: Cryptographic failure while signing assembly 'e:\temp\test.exe' -- 'Error signing assembly -- The parameter is
        incorrect. '

I think tried the some code path with no key and with a fresh key and both work. 

E:\temp> nativecsc /nologo .\test.cs
E:\temp> nativecsc /nologo .\test.cs /keyfile:test.snk
kg commented 5 years ago

Odd, some set of switches must have been causing the keyfile to be ignored until now. That explains why it was already failing in VS months ago. In that case, I'll treat this as an error on our end. It would be great to get a better error message (I was totally stumped when I hit this in VS and couldn't figure out how to troubleshoot it) but it seems like the root cause is just that this file was always busted.

Thanks for investigating!

jaredpar commented 5 years ago

@kg

In that case, I'll treat this as an error on our end.

Okay. I'm still somewhat paranoid that this issue was filed shortly after I made our strong naming switch over. If you end up finding evidence that this may still be our problem please let me know.

It would be great to get a better error message

For strong naming we're mostly shelling out to external APIs here and just re-surfacing the messages that they provide. I agree their messages could be a lot clearer here 😦

akoeplinger commented 5 years ago

@jaredpar looks like we figured it out, the nunit folks ran into the issue in 2014 too: https://groups.google.com/d/msg/nunit-developer/VdI88Y94zLY/r1SLuxbROCUJ

Quoting the relevant pieces (emphasis mine):

I have tried building on three Windows 8.1 machines with VS2010 & VS2013 and it fails on all of them with this error,

1>CSC : error CS1548: Cryptographic failure while signing assembly 'c:\Users\rob.prouse\Documents\GitHub\nunit-framework\src\framework\obj\Debug\net-2.0\nunit.framework.dll' -- 'Error signing assembly -- The parameter is incorrect. '

It builds fine in VS2013 on Windows Server 2008 R2 though, so I am guessing that it is a Windows 8.1 issue. My guess (based on the size of the SNK) is that it is in an older format that is no longer supported in Windows 8.x.

jaredpar commented 5 years ago

@akoeplinger thanks for providing that info. The implementation of strong name signing in that environment is done completely in the desktop CLR COM APIs. It's possible they depend on the OS here and we're just seeing that play out as the operating system updates.

@davidwrighton may be able to provide more context here.

I'm not sure there is much else we can do here then if this is due to an OS change.

davidwrighton commented 5 years ago

I don't really know how our strongname signing works or why it would have stopped working as of Windows 8.1 However, @bartonjs would be more likely to know.

bartonjs commented 5 years ago

I don't really see anything wrong with the nunit.snk file. It and the mono.snk file agree on the header contents (PRIVATEKEYBLOB, VERSION_02, 0, CALG_RSA_SIGN, RSA_PRIV_MAGIC, 1024-bit, e=0x00010001); and they have the same length (both of which are right for 1024-bit keys).

My little utilities that I've written say that the nunit RSA parameters are valid (PQ = Modulus, DE === 1 mod Phi, etc) and I double checked that P and Q are (probably) prime.

There is an oddity in the key, in that the two different formulae for producing the private exponent (D) value produced the same result... I've never seen that before. Maybe that caused a problem for Windows 8.1 (but somehow not Server 2012 R2)?

FWIW, I was able to run my test on Windows 10 passing the SNK bytes to RSACryptoServiceProvider.ImportCspBlob, and it worked... though I can't confirm that Win8.1 fails, since I don't have the client OS anywhere.

akoeplinger commented 5 years ago

@bartonjs so you're saying that it is a bug in Roslyn that it can't sign with the nunit.snk key?

jaredpar commented 5 years ago

@akoeplinger, @bartonjs

so you're saying that it is a bug in Roslyn that it can't sign with the nunit.snk key?

Doesn't seem possible it's a bug in Roslyn. The Roslyn code base as well as the pre-roslyn C# compiler both exhibit this behavior with this key. Both code bases use the CLR COM APIs for signing and that just amounts to passing a file path along. The error is coming from the COM APIs here.

bartonjs commented 5 years ago

@akoeplinger No, I'm saying that I think it was an OS bug somehow only in the client SKU of Windows 8.1; Win7 and Win10 client SKUs can both load it, and the server SKU of Win8.1 (Server 2012 R2) could also.

akoeplinger commented 5 years ago

@bartonjs but according to @jaredpar's tests he does still hit this and I can't imagine he's still using Windows 8.1 😄