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

Remove assembly signing #1050

Open
fubar-coder opened this issue Aug 6, 2019 · 13 comments
Open

Remove assembly signing #1050

fubar-coder opened this issue Aug 6, 2019 · 13 comments
Labels
discussion Discussion required

Comments

@fubar-coder
Copy link
Member

Our assemblies currently have a "strong name". Do we want to keep this?

Pro

  • Allows users to use FluentMigrator to make/keep their assemblies strong-named
  • Get rid of possible source of errors (signing problems when being built with mono)

Contra

  • We cannot upgrade to the latest Snowflake.Data ADO.NET driver

.NET Core doesn't check the integrity of signed assemblies, so this would only affect users of the .NET Framework. Do we want to support them?

What are your thoughts, @jzabroski ?

@fubar-coder fubar-coder added the discussion Discussion required label Aug 6, 2019
@jzabroski
Copy link
Collaborator

I wrote up a best practice around this somewhere with my full thoughts. Suffice to say, best practice is to open source the strong name key. Authenticode is the secure way to sign assemblies.

I am blanking on why "We cannot upgrade to the latest Snowflake.Data ADO.NET driver"?

The neutral way to do this is to have two versions of every FM nuget package: signed and unsigned. Signed would be for .NET 4 legacy, unsigned for .NET Core and .NET 5

@fubar-coder
Copy link
Member Author

fubar-coder commented Aug 28, 2019

My wording was a bit off. You cannot directly reference a non-signed assembly from your signed assembly, but you can load it later via Assembly.Load (for example), so we'd have to load the Snowflake ADO.NET driver and use reflection to find the classes instead of referencing the new package directly.

I don't like having two different sets of assemblies, because it might confuse the users of this library.

Possible solutions

.NET Framework has a different package name, but same assembly name

Causes problems when your main project depends on FluentMigrator-NET4 (for the signed assembly) and references a package which depends on FluentMigrator.Abstractions, because the main project would also reference FluentMigrator.Abstractions-NET4 and we're now having assemblies with the same name from two different references.

.NET Framework has a different package name and a different assembly name

Creation of a library for both .NET Core and .NET Framework will only be possible if there are two packages for this library, one for .NET Framework and the other for .NET Core. This would cause a similar situation where you start using async/await and realize, that - when you convert one function - you need to convert others too.

We're removing assembly signing

FluentMigrator cannot be used from strong-named assemblies and cannot be put into the GAC anymore.

We keep assembly signing

We load the ADO.NET drivers via reflection.

@jzabroski
Copy link
Collaborator

jzabroski commented Oct 8, 2019

@fubar-coder Just thinking... why can't we use ALC (AssemblyLoadConext) to sidestep this problem on .NET Core?

Pros

  • Just works :)

Cons

Even if we don't immediately choose ALC, I think that solution should color any interim solution we put in place. I believe that will side-step the problem entirely. It is effectively your solution - We keep assembly signing && We load ADO.NET drivers via reflection - but with a systematic approach using a PluginLoader API similar to Nate McMaster's DotNetCorePlugins nuget package.

Separately, once we have docker tests running, it will mitigate the risk of PlatformNotSupportedException(s) occuring during assembly load, which the C# compiler would otherwise catch if not loaded dynamically. Right?

@antrv
Copy link

antrv commented Oct 16, 2019

Please keep the assemblies signed. Would it be better asking the contributors of Snowflake.Data ADO.NET driver to sign it?

@fubar-coder
Copy link
Member Author

I wish that we'd be able to off-load the hassle to load the ADO.NET drivers (via reflection) to the FM user. That way we'd avoid problems with the GAC (on .NET Framework) or with the missing GAC (on .NET Core) and all other problems surrounding missing dependencies...

@jzabroski
Copy link
Collaborator

jzabroski commented Oct 16, 2019

I think that's why I was wondering if we can load ADO.NET Drivers indirectly via reflection, as opposed to directly via reflection. The reason is that historically, Nuget package authors had the responsibility for certifying the platform target for their packages (AnyCPU, x86, x64, ARM...). As platform targets have increased and deployment option complexity has increased (think AOT requirements for compilation for UWP, multiple OSes, etc), the need to have the packaging sub-system self-certify some of these requirements has become a necessity.

It's not immediately clear to me how much of this the .NET Toolchain does for us vs. we have to do for ourselves as package authors. It's not really a NuGet issue so much as an SDK-Nuget Interop "representation independence" issue.

I hope I'm making sense - the idea is that ideally we should not be directly tied to salient features but the build system should resolve those. Why we are in the year 2019 and developers are still struggling with this vs. a language (MSBuild xml / Common Project System) is mind boggling. I think, if presented correctly, this could be pushed to the .NET Foundation to address instead of us to solve (why is assembly signing something not automateable as part of <PackageReference> element).

@jzabroski
Copy link
Collaborator

Related: #1016

@jzabroski
Copy link
Collaborator

@fubar-coder It seems Microsoft's opens source library guidance is explicitly DO NOT remove assembly signing. They raise a very simple point:

❌ DO NOT add, remove, or change the strong naming key.

Modifying an assembly's strong naming key changes the assembly's identity and breaks compiled code that uses it. For more information, see binary breaking changes.

@fubar-coder
Copy link
Member Author

Yes, it's a breaking change, which is the reason why I suggested the removal for the next major version.

@jzabroski
Copy link
Collaborator

See also: StephenCleary/AsyncEx#196 (comment)

@jzabroski
Copy link
Collaborator

I think we may want to prototype creating a MSBuild shim for DesktopStrongNameProvider - but I am a bit leary of why it has such a weird name? What am I missing?

@fubar-coder
Copy link
Member Author

It seems that public signing doesn't work for us as it wouldn't work on .NET Framework, which does a strong name validation.

@jzabroski
Copy link
Collaborator

jzabroski commented Nov 8, 2019

I just started using the latest version of StrongNamer after Oren Novotny fixed issues with it for .NET Core. Latest package is only 5 days old. Works well so far. A little non-intuitive, but usable.

If we make progress on using dotnet templates (#1074), we can just bundle as part of the template for Snowflake a StrongNamer in the InProcess toolchain. This would leave issues with #1016 potentially. The solution there would be that you'd have to include StrongNamer in all your assemblies that reference a [MigrationAttribute] or IMigration.

Just thinking out loud :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Discussion required
Projects
None yet
Development

No branches or pull requests

3 participants