Add support for public sign to csc #6776

agocke · 2015-11-13T23:59:06Z

Sometimes called "fake sign" or "OSS sign" public signing is including
the public key in an output assembly and setting the "signed" flag, but
not actually signing the assembly with a private key. This is useful for
open source projects where people want to build assemblies which are
compatible with the released "fully signed" assemblies, but don't have
access to the private key used to sign the assemblies. Since almost no
consumers actually need to check if the assembly is fully signed, these
publicly built assemblies are useable in almost every scenario that the
fully signed one would be used in.

This PR implements support only for C# -- VB will be added soon. If
being used at the command line, the /publicsign flag can be passed to
csc and the /keyfile flag can specify the public key. Unlike fully
signing, a full key pair encoded in the SNK file format is not currently
supported. When using /publicsign, just the public key must be in the
/keyfile file.

When using the API, the public key can be passed directly using the
CryptoPublicKey CompilationOption.

Sometimes called "fake sign" or "OSS sign" public signing is including the public key in an output assembly and setting the "signed" flag, but not actually signing the assembly with a private key. This is useful for open source projects where people want to build assemblies which are compatible with the released "fully signed" assemblies, but don't have access to the private key used to sign the assemblies. Since almost no consumers actually need to check if the assembly is fully signed, these publicly built assemblies are useable in almost every scenario that the fully signed one would be used in. This PR implements support only for C# -- VB will be added soon. If being used at the command line, the /publicsign flag can be passed to csc and the /keyfile flag can specify the public key. Unlike fully signing, a full key pair encoded in the SNK file format is not currently supported. When using /publicsign, just the public key must be in the /keyfile file. When using the API, the public key can be passed directly using the CryptoPublicKey CompilationOption.

agocke · 2015-11-13T23:59:20Z

@dotnet/roslyn-compiler @tmat Please review

agocke · 2015-11-14T00:00:33Z

@davidfowl FYI. This change also requires the PublicSign option to be set, so if you were using just the CryptoPublicKey option to indicate that you wanted OSS signing, you'll need to add this flag to ASP.NET

davidfowl · 2015-11-14T15:40:26Z

@agocke That's a breaking change. Can we infer public sign from providing the bytes only? It's going to break omnisharp as well (well only if we update roslyn there)

/cc @moozzyk

davidfowl · 2015-11-14T15:41:45Z

@agocke Where is the code that extracts only the public key from the keyfile? Specifically https://github.com/aspnet/dnx/blob/72b0f325959a06d7b9d92aa438df0dc889a73ec3/src/Microsoft.Dnx.Compilation.CSharp.Common/SnkUtils.cs

agocke · 2015-11-14T19:37:08Z

@davidfowl It is a breaking change, but we never intended for that to be the indicator that public signing was enabled.

For the parser splitting, there is no code for that yet. I'm currently writing a parser that should get rid of all our uses of IStrongName.GetPublicKey, but this is the quickest way to get the functionality without the parser -- the public key must just be written to the file, instead of the public/private key pair.

davidfowl · 2015-11-14T20:28:21Z

@agocke So to get the same functionality we have in dnx today there needs to be some other process that extracts the public key from a real snk file, writes it on disk, and then hand it into csc?

tmat · 2015-11-14T20:33:04Z

@agocke I would still prefer not to have an extra option and do OSS signing automatically when the .snk file only contains public key. To keep it simple. The chances of using accidentally .snk file with public key only instead of full key are low, same as accidentally using a different key than intended.

agocke · 2015-11-14T21:22:59Z

@davidfowl I believe you already have an SNK parser and pass the key to the CryptoPublicKey option. You can do the same thing now, just also set PublicKey sign to true. Is that a big ask?

davidfowl · 2015-11-14T21:27:27Z

I'd rather not write a temp file to disk. The compiler can extract it from the existing file on disk and do that in memory.

agocke · 2015-11-14T21:31:00Z

@davidfowl Are we still talking about DNX? Since you construct your compilation options in memory you don't have to write the public key out to disk -- just pass it to CryptoPublickKey here:

roslyn/src/Compilers/Core/Portable/Compilation/CompilationOptions.cs

Line 55 in 7ef5f23

public ImmutableArray<byte> CryptoPublicKey { get; protected set; }

agocke · 2015-11-14T21:32:20Z

@davidfowl I would also say that most public sign consumers would just store the public key in a file split out from the snk file, since the snk file couldn't be checked into version control, but the public key can.

agocke · 2015-11-14T21:34:28Z

test prtest/win/dbg/unit64 please

agocke · 2015-11-16T07:22:14Z

@tmat @davidfowl Added a key blob parser.

Unfortunately, completely removing the StrongNameProvider.CreateKeys API is more complicated -- DesktopStrongNameProvider takes an optional list of key file search paths, which are searched for the key files. Since DesktopStrongNameProvider is public, removing this method of signing may be a breaking change. I may need to go to the compat council for this one.

davidfowl · 2015-11-16T07:33:08Z

src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs

+            parsedArgs = DefaultParse(new[] { "/publicsign:-", "a.cs" }, _baseDirectory);
+            parsedArgs.Errors.Verify(
+    // error CS2007: Unrecognized option: '/publicsign:-'
+    Diagnostic(ErrorCode.ERR_BadSwitch).WithArguments("/publicsign:-").WithLocation(1, 1));


davidfowl · 2015-11-16T13:30:49Z

src/Compilers/Core/Portable/StrongName/DesktopStrongNameProvider.cs

@@ -9,6 +9,7 @@
 using Microsoft.CodeAnalysis.Collections;
 using Microsoft.CodeAnalysis.Interop;
 using Roslyn.Utilities;
+using System.Threading;


nit: sort usings

jaredpar · 2015-11-16T21:14:49Z

src/Compilers/Core/Portable/StrongName/CryptoBlobParser.cs

@@ -0,0 +1,312 @@
+using Microsoft.CodeAnalysis.Collections;


Missing copyright

agocke · 2015-11-16T22:41:32Z

@dotnet/roslyn-compiler Ping -- please review

AlekseyTs · 2015-11-16T22:49:26Z

LGTM

TyOverby · 2015-11-16T23:33:52Z

src/Compilers/VisualBasic/Portable/VisualBasicCompilationOptions.vb

+                generalDiagnosticOption,
+                specificDiagnosticOptions,
+                concurrentBuild,
+                deterministic:=False,' TODO: fix this


Maybe have a issue number for this TODO?

The TODO was there when I copied this code. I don't know what's to be done here -- @TyOverby did you originally leave this TODO?

jaredpar · 2015-11-17T00:03:54Z

src/Compilers/CSharp/Portable/CSharpCompilationOptions.cs

@@ -473,6 +492,8 @@ protected override CompilationOptions CommonWithPlatform(Platform platform)
            return WithPlatform(platform);
        }

+        protected override CompilationOptions CommonWithPublicSign(bool publicSign) => WithPublicSign(publicSign);


I don't have a strong preference between => and {} for single line methods. But it does irk the OCD part of my brain when we have types that mix the two styles with identical code. I think we should keep the existing style in this file or move everything to =>

jaredpar · 2015-11-17T00:07:14Z

Is there a bug filed for the MSBuild work needed as the follow up to this change?

moozzyk · 2015-11-17T00:11:32Z

LGTM

agocke · 2015-11-17T00:45:14Z

@jaredpar I was going to file bugs for the follow-on work after it was merged.

agocke · 2015-11-17T05:46:03Z

@jaredpar Was that a +1 ;)

jaredpar · 2015-11-17T16:35:27Z

@agocke it was a 👍 assuming you fixed the other feedback but it oloks like you have now so yes 👍

Add support for public sign to csc

dnfclas added the cla-already-signed label Nov 13, 2015

Remove in-progress snk parser

5725a38

Also remove the parser file from the project

6959692

Add a CryptoBlobParser for parsing public keys out of crypto blobs

30d4099

davidfowl reviewed Nov 16, 2015
View reviewed changes

Remove extraneous error numbers and correct formatting

4f08a88

davidfowl reviewed Nov 16, 2015
View reviewed changes

Respond to PR comments

e8bbea8

jaredpar reviewed Nov 16, 2015
View reviewed changes

Revert refactoring that broke tests

afdc0fd

TyOverby reviewed Nov 16, 2015
View reviewed changes

jaredpar reviewed Nov 17, 2015
View reviewed changes

Respond to PR comments

066eeb6

agocke added a commit that referenced this pull request Nov 17, 2015

Merge pull request #6776 from agocke/AddFakeSign

7672019

Add support for public sign to csc

agocke merged commit 7672019 into dotnet:master Nov 17, 2015

agocke deleted the AddFakeSign branch November 17, 2015 18:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for public sign to csc #6776

Add support for public sign to csc #6776

agocke commented Nov 13, 2015

agocke commented Nov 13, 2015

agocke commented Nov 14, 2015

davidfowl commented Nov 14, 2015

davidfowl commented Nov 14, 2015

agocke commented Nov 14, 2015

davidfowl commented Nov 14, 2015

tmat commented Nov 14, 2015

agocke commented Nov 14, 2015

davidfowl commented Nov 14, 2015

agocke commented Nov 14, 2015

agocke commented Nov 14, 2015

agocke commented Nov 14, 2015

agocke commented Nov 16, 2015

davidfowl Nov 16, 2015

davidfowl Nov 16, 2015

jaredpar Nov 16, 2015

agocke commented Nov 16, 2015

AlekseyTs commented Nov 16, 2015

TyOverby Nov 16, 2015

jaredpar Nov 17, 2015

agocke Nov 17, 2015

jaredpar Nov 17, 2015

jaredpar commented Nov 17, 2015

moozzyk commented Nov 17, 2015

agocke commented Nov 17, 2015

agocke commented Nov 17, 2015

jaredpar commented Nov 17, 2015

Add support for public sign to csc #6776

Add support for public sign to csc #6776

Conversation

agocke commented Nov 13, 2015

agocke commented Nov 13, 2015

agocke commented Nov 14, 2015

davidfowl commented Nov 14, 2015

davidfowl commented Nov 14, 2015

agocke commented Nov 14, 2015

davidfowl commented Nov 14, 2015

tmat commented Nov 14, 2015

agocke commented Nov 14, 2015

davidfowl commented Nov 14, 2015

agocke commented Nov 14, 2015

agocke commented Nov 14, 2015

agocke commented Nov 14, 2015

agocke commented Nov 16, 2015

davidfowl Nov 16, 2015

Choose a reason for hiding this comment

davidfowl Nov 16, 2015

Choose a reason for hiding this comment

jaredpar Nov 16, 2015

Choose a reason for hiding this comment

agocke commented Nov 16, 2015

AlekseyTs commented Nov 16, 2015

TyOverby Nov 16, 2015

Choose a reason for hiding this comment

jaredpar Nov 17, 2015

Choose a reason for hiding this comment

agocke Nov 17, 2015

Choose a reason for hiding this comment

jaredpar Nov 17, 2015

Choose a reason for hiding this comment

jaredpar commented Nov 17, 2015

moozzyk commented Nov 17, 2015

agocke commented Nov 17, 2015

agocke commented Nov 17, 2015

jaredpar commented Nov 17, 2015