Feature/assembly strongname

[anderslaub]

Motivation

Many solutions based on .NET Framework (net4x) has both direct and inherited dependencies on GraphQl-dotnet.

Since the currently released assemblies does not have a strongly typed name, developers who work on such solutions are forced to depend on a compatible version of graphql-dotnet as their inherited dependency. This often means v2.4.0 due to breaking changes introduced in later major versions.

By strong name signing the assemblies that target netstandard2.0/1, developers who are currently stuck on an old inherited version will be able to do codebase versioned assembly binding.

example:

            <dependentAssembly xdt:Transform="InsertIfMissing"  xdt:Locator="Condition(asmv1:assemblyIdentity/@name='GraphQl')">
                <assemblyIdentity name="GraphQl"  publicKeyToken="e6238258560628ee" />
                <codeBase version="5.0.0.0" href="bin\deps\GraphQL.dll" />
            </dependentAssembly>

            <dependentAssembly xdt:Transform="InsertIfMissing"  xdt:Locator="Condition(asmv1:assemblyIdentity/@name='GraphQl.DataLoader')">
                <assemblyIdentity name="GraphQl.DataLoader" publicKeyToken="e6238258560628ee" />
                <codeBase version="5.0.0.0" href="bin\deps\GraphQl.DataLoader.dll" />
            </dependentAssembly>

            <dependentAssembly xdt:Transform="InsertIfMissing"  xdt:Locator="Condition(asmv1:assemblyIdentity/@name='GraphQl.NewtonsoftJson')">
                <assemblyIdentity name="GraphQL.NewtonsoftJson" publicKeyToken="e6238258560628ee" />
                <codeBase version="5.0.0.0" href="bin\deps\GraphQL.NewtonsoftJson.dll" />
            </dependentAssembly>

            <dependentAssembly xdt:Transform="InsertIfMissing"  xdt:Locator="Condition(asmv1:assemblyIdentity/@name='GraphQLParser')">
                <assemblyIdentity name="GraphQLParser" publicKeyToken="e6238258560628ee" />
                <codeBase version="8.0.0.0" href="bin\deps\GraphQLParser.dll" />
            </dependentAssembly>

Signing the assemblies also enable other use-cases which has otherwise been prevented by legacy requirements for strong-named assembly (such as Visual Studio extension development).

The lack of a strong name on current releases prevents developers from transitioning their codebases to later versions of GraphQl leaving many projects stuck on graphql-dotnet v2.4.0

As support for this thesis, the nuget.org download stats clearly shows that v2.4.0 is by far the most downloaded nuget and continues to have a very significant amount of daily downloads.

Notes

Pushing the private key used for signing the assemblies is the recommended approach for open-source projects and is not a security concern.

We can all agree that signing assemblies for managing dependencies should never have been a thing or should at least be a thing of the past. This is unfortunately not the reality yet and leaving this out on popular frameworks such as this makes modernizing code-bases even harder.

Considerations

Assembly sigining could alternatively be done in a Github action.

The reason for having it as part of the build is that it provide more flexibility and also enable local builds to be signed with the same key as the officially released assemblies - making testing and extending less prone to error.

Important

Since strong named assemblies only can reference other strongly named assemblies. These 2 PRs are dependent:

• Feature/assembly strongname parser#262
• Feature/assembly strongname #3265

References

Microsoft recommend publicly published .NET libraries and open-source maintainers to sign their assemblies for best cross-runtime compatibility.

• https://github.com/dotnet/runtime/blob/main/docs/project/strong-name-signing.md
• https://github.com/dotnet/project-system/blob/main/eng/imports/StrongName.targets

[ghost]

Desperately need this!

[Shane32]

I looked through much of the available MS documentation on this topic. I cannot find any significant reason not to sign the assembly.

Here's what I noticed:

• On .NET Framework, having the assembly strongly named is useful to other strongly-named assemblies
• On .NET Framework, if we sign the assembly, it will require binding redirects in some situations (probably often), which are generally automatically created by Visual Studio
• All .NET Core assemblies are strongly named
• Having the private key stored in the repository is recommended and desired
• It should not affect .NET Core applications, per the snip below:

For .NET Core and .NET 5+, strong-named assemblies do not provide material benefits. The runtime never validates the strong-name signature, nor does it use the strong-name for assembly binding.

I support these changes. However, I suggest that we target the develop branch and release it with 7.0.0.

@anderslaub Can you modify your PR to target the develop branch? Thanks!

[Shane32]

@anderslaub Also, can you add a PR targeting the develop branch of the GraphQL.NET Server repository (https://github.com/graphql-dotnet/server) for matching changes? New for v7, GraphQL.NET Server supports ASP.NET Core 2.1 running on .NET Framework 4.6.1+ or .NET Core 2.1.

[anderslaub]

@anderslaub Can you modify your PR to target the develop branch? Thanks!

@Shane32 I've changed PR to target develop and did a merge local to fix the failing tests.

Now I noticed the dependencies from the server repo, so I've made a fork and introduced the same assembly signing setup there and to make it build I've added a local folder as nuget source for signed versions similar to what I did in this PR.

However, I can't help notice the cyclic dependency flow.

I can't build the full graphql-dotnet:develop solution before I have signed assemblies from server:develop and I can't build and sign server:develop before I have signed assemblies from graphql-dotnet:develop

It's not a blocker at all; The needed references from graphql-dotnet:develop is just packed individually.

My point is really just that to me this indicates that GraphQl.Harness maybe should be somewhere else. Probably in it's own repo or alternately in the server repo.

PR coming up in server shortly followed by updates to this.

[Shane32]

You make a very good point; I've been thinking that we should stop publishing samples that do not use the server project, as many people copy from these samples which contain non-recommended design patterns. However, I don't think this will be updated prior to 7.0 release. cc: @sungam3r

[anderslaub]

@Shane32 Pragmatically; it works, so..

Another question, server:develop referenced 6.0.0-preview-612

I've made signed nugets from graphql-dotnet:develop for now just called 7.0.0-strongname and referenced these instead - all the projects that I need builds but the test projects does not.

It wouldn't be a problem for me to make some new temporary nugets that has strong named assemblies from another branch - but I can't find any branches or tags for 6.0.0-preview-612 - is it just graphql-dotnet:master?

Or should I just leave it and PR into server:develop with failing test builds? I guess these have to be updated soon anyway.

[Shane32]

It's the develop branch from graphql-dotnet/graphql-dotnet.

[anderslaub]

graphql-dotnet/src/Directory.Build.props

Line 4 in 2ff8a9a

<VersionPrefix>7.0.0-preview</VersionPrefix>

this is called 7.0.0-preview

[anderslaub]

and the reference in server:develop is https://github.com/graphql-dotnet/server/blob/240e112f329e3eec13254e59d58180a815700ede/Directory.Build.props#L31 6.0.0-preview-612

[Shane32]

If all is well and @sungam3r has no comments, I'll merge and release the parser 8.1.0, then you can fix up the GraphQL.NET PR, and we will merge that, and hopefully release 7.0. Once 7.0 is released, you can update the PR for the GraphQL.NET Server PR, and I'll merge and release 7.0 of GraphQL.NET Server.

We are just waiting for PR #3245 to be finished before we can release 7.0.

[Shane32]

graphql-dotnet/src/Directory.Build.props

Line 4 in 2ff8a9a

<VersionPrefix>7.0.0-preview</VersionPrefix>

this is called 7.0.0-preview

We renamed 6.x to 7.x to align with server repo. There is no 6.x. Probably it's just an older beta build. Here's all the latest beta builds of GraphQL.NET. You'll notice 5.x or 7.x builds based on when a PR got merged to either master or develop last. So the latest 7.x preview is 7.0.0-preview-634 and really the server repo should be referencing that rather than 6.0.0-preview-612.

https://github.com/graphql-dotnet/graphql-dotnet/packages/433360/versions

[Shane32]

When we release GraphQL.NET 7.0.0 we will update the reference in the server repo to 7.0.0 of course.

[Shane32]

Once the PR in the parser library is merged, even before 8.1.0 is released, we can reference the beta parser lib in GraphQL.NET so it is referencing a strongly-named assembly. Ditto for GraphQL.NET/Server.

[anderslaub]

Sure okay, then I just wonder why the test projects on server:develop doesn't build. But I leave it for now and wait for the releases.

Btw; do you think there is any chance of getting a 2.4.1 release out with strong names. 2.4.0 seems to be where a lot of net framework solutions are stuck.

I can do the signing etc. in a hotfix branch off 2.4.0 + in parser if it would be possible to get it released.

[Shane32]

Sure okay, then I just wonder why the test projects on server:develop doesn't build. But I leave it for now and wait for the releases.

Btw; do you think there is any chance of getting a 2.4.1 release out with strong names. 2.4.0 seems to be where a lot of net framework solutions are stuck.

I can do the signing etc. in a hotfix branch off 2.4.0 + in parser if it would be possible to get it released.

I fail to understand why we would do that, or what basis there is for saying that .NET Framework solutions are either stuck or using 2.4.0. The current version of GraphQL.NET only requires .NET Standard 2.0, which is supported by .NET Framework 4.6.1 and later. Support for .NET Framework 4.6.1 and prior versions ended in April. .NET Framework 4.6.2, still supported, is compatible with .NET Standard 2.0 and hence compatible with GraphQL.NET. So while lack of support for .NET Framework 4.5 may have been a blocker in the past, it seems a bit irrelevant now.

We can also view the NuGet statistics for GraphQL.NET which show that in the last 6 weeks, while there were 134,000 downloads of v2.4.0 (which was the current version for a 2-year period), there was 450,000 downloads of newer versions (also covering about a 2-year period). So while there are a significant number of people still on 2.4.0, the number seems about right considering the length of time it was the active version.

Also, 2.4.0 was missing some major features; 3.0.0 was a huge step forward in many ways, as was 4.0 for speed and 4.6 for ease of configuration and 5.0 for type-driven graphs. With the upcoming 7.x version, it will be easier than ever before to spin up a new GraphQL server within .NET -- and for .NET Framework users especially, with the server's project support of ASP.NET Core 2.1 running on .NET Framework.

As it pertains to a hotfix, I have no problem issuing hotfixes for older versions of GraphQL.NET. However, the deployment scripts currently in use were written for 3.1.0 and later, so it is not easy/practical to release a hotfix for a version prior to 3.x. To put it simply, we can't issue a 2.4.1.

I also fail to see the logic here; anyone using 2.4.0 now is likely using it because they have a current deployment utilizing that version. A strong name is not going to help them. Whereas a newcomer to GraphQL.NET that may require a strong-named assembly is not going to select 2.4.1 over the current version.

[anderslaub]

Sure okay, then I just wonder why the test projects on server:develop doesn't build. But I leave it for now and wait for the releases.
Btw; do you think there is any chance of getting a 2.4.1 release out with strong names. 2.4.0 seems to be where a lot of net framework solutions are stuck.
I can do the signing etc. in a hotfix branch off 2.4.0 + in parser if it would be possible to get it released.

I fail to understand why we would do that, or what basis there is for saying that .NET Framework solutions are either stuck or using 2.4.0. The current version of GraphQL.NET only requires .NET Standard 2.0, which is supported by .NET Framework 4.6.1 and later. Support for .NET Framework 4.6.1 and prior versions ended in April. .NET Framework 4.6.2, still supported, is compatible with .NET Standard 2.0 and hence compatible with GraphQL.NET. So while lack of support for .NET Framework 4.5 may have been a blocker in the past, it seems a bit irrelevant now.

We can also view the NuGet statistics for GraphQL.NET which show that in the last 6 weeks, while there were 134,000 downloads of v2.4.0 (which was the current version for a 2-year period), there was 450,000 downloads of newer versions (also covering about a 2-year period). So while there are a significant number of people still on 2.4.0, the number seems about right considering the length of time it was the active version.

Also, 2.4.0 was missing some major features; 3.0.0 was a huge step forward in many ways, as was 4.0 for speed and 4.6 for ease of configuration and 5.0 for type-driven graphs. With the upcoming 7.x version, it will be easier than ever before to spin up a new GraphQL server within .NET -- and for .NET Framework users especially, with the server's project support of ASP.NET Core 2.1 running on .NET Framework.

As it pertains to a hotfix, I have no problem issuing hotfixes for older versions of GraphQL.NET. However, the deployment scripts currently in use were written for 3.1.0 and later, so it is not easy/practical to release a hotfix for a version prior to 3.x. To put it simply, we can't issue a 2.4.1.

I also fail to see the logic here; anyone using 2.4.0 now is likely using it because they have a current deployment utilizing that version. A strong name is not going to help them. Whereas a newcomer to GraphQL.NET that may require a strong-named assembly is not going to select 2.4.1 over the current version.

@Shane32 Let me try to explain; It's not about the current version or lack of .NET Framework support at all.

It is inherited dependencies combined with the legacy of .NET Framework assembly binding that is the PITA.

The problem is as you know, that without a signed "strong" name on the assembly you cannot do any version assembly binding.

.NET Framework ignores the version of an assembly if it is not signed. This is really good if there are no breaking changes between versions - but if there is and you have external dependencies that depend on a specific version then you are locked to that version or you break your dependencies at runtime.

To use my own situation as an example;

I am upgrading a custom codebase that is built for a closed-source .NET Framework based CMS.

The custom codebase that I am upgrading use GraphQl v4.5. The version of the CMS that it was built for did not depend on GraphQl - however, the new version of the CMS has new modules that depend on GraphQl v2.4.

So without a strongname on neither v2.4 or v4.5 there would be nothing to do but downgrade the codebase to target v2.4.0 - which would be a lot of work and really just plain stupid - or alternately, try to convince the CMS vendor to upgrade their dependency but they might also be locked in on v2.4.0 due to one of their dependencies - or at least due to internal dependencies that all would have to be upgraded at the same time too.

To fix this I've simply packed up a custom signed v4.5.0.0 - which is what brought me here in the first place.

With a strongname on the v4.5.0 assembly I can do a runtime version based assembly binding like this:

            <dependentAssembly xdt:Transform="InsertIfMissing"  xdt:Locator="Condition(asmv1:assemblyIdentity/@name='GraphQl')">
                <assemblyIdentity name="GraphQl"  publicKeyToken="e6238258560628ee" />
                <codeBase version="4.5.0.0" href="bin\deps\GraphQL.dll" />
            </dependentAssembly>

And this just works - the inherited old unsigned v2.4.0 dll remains in the bin/ folder and when the code requests a v4.5.0 it use the assembly from the specified location.

This solved my specific issue in this particular situation but it doesn't help me next time and it doesn't help others who are in a similar situation. They would also have to "roll their own" signed version and fiddle around with the build output before deploying their code.

For the CMS vendor, they have a significant large task in upgrading their dependency on v2.4, especially since they have several modules which all have to be upgraded at once since the assemblies are unsigned. Without a strong name, they cannot do it one module at a time using codebase version bindings.

With a signed v2.4.1 this would be much easier for them and it would also be somewhat easier for us "consumers" to assembly bind the other way. Move the old version out of the bin/ folder for legacy dependencies, rather than moving the new version out.

Makes sense?

I don't know if my thesis about many .NET Frameworks being stuck on v2.4.0 is correct.

But I do know a multitude of solutions that are build on the same CMS which are currently stuck. They cannot use a newer version of GraphQl before a signed version is released. I doubt that this particular vendor is the only .NET Framework based platform who has been using GraphQl since v2.4.0 and haven't upgraded their codebase to a newer version yet - thus are making all solutions built on top of their platforms stuck on that version.

[Shane32]

As it relates to your previous comment, I think I follow. It is possible to release a 2.4.1 but we would need a PR to bring in the proper GitHub Action workflows from 3.1.0 or newer, and possibly update them to work with the older .NET Framework referenced by the project. That may be a bit pointless unless we first release GraphQL-Parser 3.0.1 with a strong name.

[Shane32]

@anderslaub If you bump GraphQL-Parser to 8.0.0-preview-153, which should have a strong name, then you should be able to make the necessary changes in this repository for all the build checks to pass. Let me know when to review again. We will bump GraphQL-Parser 8.0.0-preview-153 to 8.1.0 when we release v7.

[codecov-commenter]

Codecov Report

Merging #3265 (07ae7f7) into develop (1fab95b) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #3265      +/-   ##
===========================================
- Coverage    84.21%   84.20%   -0.02%     
===========================================
  Files          374      374              
  Lines        16075    16075              
  Branches      2588     2588              
===========================================
- Hits         13538    13536       -2     
- Misses        1917     1918       +1     
- Partials       620      621       +1     

Impacted Files	Coverage Δ
src/GraphQL/Types/Schema.cs	78.99% <0.00%> (-0.92%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[Shane32]

Well, all the .NET Framework 4.8 tests failed (which only run on Windows), with an error about the strong name not matching.

[xUnit.net 00:00:01.83] Skipping: GraphQL.Tests (could not find dependent assembly 'GraphQL, Version=7.0.0')
No test is available in D:\a\graphql-dotnet\graphql-dotnet\src\GraphQL.Tests\bin\Debug\net48\GraphQL.Tests.dll. Make sure that test discoverer & executors are registered and platform & framework version settings are appropriate and try again.

[xUnit.net 00:00:01.83] Skipping: GraphQL.DataLoader.Tests (could not find dependent assembly 'GraphQL, Version=7.0.0')
No test is available in D:\a\graphql-dotnet\graphql-dotnet\src\GraphQL.DataLoader.Tests\bin\Debug\net48\GraphQL.DataLoader.Tests.dll. Make sure that test discoverer & executors are registered and platform & framework version settings are appropriate and try again.

[xUnit.net 00:00:00.54] GraphQL.MicrosoftDI.Tests: Exception during discovery:
System.IO.FileLoadException: Could not load file or assembly 'GraphQL, Version=7.0.0.0, Culture=neutral, PublicKeyToken=e6238258560628ee' or one of its dependencies. Strong name signature could not be verified.  The assembly may have been tampered with, or it was delay signed but not fully signed with the correct private key. (Exception from HRESULT: 0x80131045)

.NET Core doesn't check strong names, so all those tests passed.

[Shane32]

@anderslaub I merged in #3278 and then merged develop into your branch. Then I fixed the workflows in your branch here so that it would pass the tests. Apparently when calculating the code coverage, something breaks the strong name. But since we don't use the code coverage reports on Windows, I disabled the build process from calculating the code coverage on Windows -- code coverage is still calculated on Linux and uploaded as it has always done.

[Shane32]

Also I took another pass at simplifying some of the setup, along the lines that were discussed in the parser project. You can see it in #3281 . If you agree with the changes, you can merge the branch strongnamechanges in here to apply them all to your PR, and I'll close #3281. Let me know - thanks!

[anderslaub]

@Shane32 merged in #3281

[Shane32]

@sungam3r Please review if you like

[Shane32]

@sungam3r I'm merging this so we can get the strong name PR done in the server repo. Lmk if you have any comments.

[sungam3r]

You make a very good point; I've been thinking that we should stop publishing samples that do not use the server project, as many people copy from these samples which contain non-recommended design patterns. However, I don't think this will be updated prior to 7.0 release

And... it has nothing to do with signing problem. Regarding samples with or withous server project - I'm 50/50 here and have no strong opinion.

[sungam3r]

Apparently when calculating the code coverage, something breaks the strong name

This is coverlet magic, it instruments assembly to insert special instructions to track code coverage.

[sungam3r]

Interesting. I did not notice these two deleted files.