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

Strongly named assembly #175

Closed
WhiteBlackGoose opened this issue Sep 3, 2021 · 18 comments · Fixed by #298
Closed

Strongly named assembly #175

WhiteBlackGoose opened this issue Sep 3, 2021 · 18 comments · Fixed by #298

Comments

@WhiteBlackGoose
Copy link
Contributor

Should sign all assemblies in the project (mb except the *.Interactive?) to make sure they can be referenced by strogly named assemblies

@kMutagene
Copy link
Collaborator

Can you give some info on what this means / why it is needed? This is absolutely out of my domain^^

@WhiteBlackGoose
Copy link
Contributor Author

It's a thing which allows to make sure that the used assembly is the right one (e. g. it comes in when there are assemblies with the same name). Not very much of a use, and it is only useful for .NET Framework. But those users who do rely on this thing, they have to have all their deps strongly named too. So if your assembly is not strongly named, they won't be able to use the lib at all.

I'm not saying it's a big issue though, and definitely not urgent. But a day may come when someone won't be able to use the lib because of it.

@JakeRadMSFT
Copy link
Contributor

Hello!

I'm Jake from the ML.NET team (one of .NET's Machine Learning libraries) - we're creating a bunch of sample notebooks and we'd love to use Plotly.NET in some of our Interactive Extensions. Unfortunately, we can't take dependency on Plotly.NET until it's signed.

Notebooks are an important part of our strategy going forward -

We have them in Visual Studio
https://marketplace.visualstudio.com/items?itemName=MLNET.notebook

and we're working on adding a collection of getting started with ML notebooks here:
https://github.com/dotnet/csharp-notebooks

We'd also like to use Plotly.NET in Model Builder (That is what LittleLittleCloud was asking about above).
https://dotnet.microsoft.com/en-us/apps/machinelearning-ai/ml-dotnet/model-builder

How can we help move this forward? I'm hoping this would help both of our products grow :).

@kMutagene
Copy link
Collaborator

kMutagene commented May 19, 2022

Hi @JakeRadMSFT , thanks for reaching out! Integration with ML.NET samples is definitely something that increases the priority of this issue quite a bit!

What i don't want to happen is implementing this while not understanding it and then have issues down the line due to it. I have read a bit into strong named assemblies and i am a little bit confused about the concept, so here are some questions/things i need to understand before implementing this.:

  • In general, newer MS tutorials and docs (e.g. here or here) seem to indicate that this is only concerned with giving the assembly a persistent identifier, and as a consequence recommends to check the key used for signing the assembly into source control. Is every assembly signed with this key, or do we have to use one key per assembly?
  • I am not sure if i have seen a F# project doing this (might be possible that i have seen it and just not realized what was going on though). Can you link me an example?
  • Hows the process like? Is it literally just adding <AssemblyOriginatorKeyFile>YouKey.snk</AssemblyOriginatorKeyFile and <SignAssembly>True</SignAssembly> to the project file and checking in the key? If this is so easy and has no disadvantages, why does it seem to be always such a discussion/long process until this is done for OSS libraries (see threads linked below, which have been active for years before the signing happened)?
  • This seems to impose the requirement of all dependencies being strongly named as well (use strong name #240). We have control of one of them, but there are several third party libraries used:
    • Newtonsoft.Json, which seems to cause all kinds of problems with strong naming i have to admit i do not understand, but that might be outdated.
    • Several Dotnet interactive libraries , but i suspect those are signed as i bet you have to use those for your notebooks anyways
    • Puppeteersharp (although only a dependency of the Plotly.NET.ImageExport package) which does not seem to have a strong name.

Also, i have read in some threads that this basically breaks all dependent projects once, not sure if that's true. If so, this would require a major version increase i guess?

All together, the essence of threads such as this one or this one seems to be that this is legacy stuff but it cannot hurt to support it.

The thing is that i cannot control that for third party libraries. This seems like the exact thing that causes development workflow issues down the line. How should we handle third-party dependencies?

@kMutagene
Copy link
Collaborator

On another note, from your link it looks like you will write the notebooks in C#. Depending on the complexity of visualizations you want to show, you might run into the issues outlined in #285. Input (or even help) on that issue from the C# user perspective would be awesome.

@JakeRadMSFT
Copy link
Contributor

JakeRadMSFT commented May 24, 2022

Thanks @kMutagene - I'll be honest - I've never signed an assembly outside of Microsoft. This would be a good learning experience for me on best practices.

Also, i have read in some threads that this basically breaks all dependent projects once, not sure if that's true. If so, this would require a major version increase i guess?

This is sort of implied from above but I've also never tried to transition a NuGet to signed. Perhaps there are issues there - - that might be why libraries like Markdig have a signed and unsigned version.

https://www.nuget.org/packages/Markdig/
https://www.nuget.org/packages/Markdig.Signed/

Newtonsoft.Json, which seems to cause all kinds of problems with strong naming i have to admit i do not understand, but that might be outdated.

Newtonsoft.Json certainly causes some issues. It's bitten me more than a few times :). It essentially comes down to Dependency A wants Newtonsoft 9.0 Dependency B wants Newtonsoft 10.0 ... something needs to resolve the discrepancy. These are generally resolved with binding redirects and it's a pain when it gets all the way to runtime and you hit these issue. Usually this happens in dynamically loading assemblies or other extensibility situations. I'm sure I could find someone with a better explanation if you'd like.

@kMutagene What are your thoughts on having two NuGets? Should I try to wire that up? That way we don't break existing users.

@kMutagene
Copy link
Collaborator

before we get deeper into this, what is the exact reason why the assembly must be signed? Are you making non-.NET(core) notebooks? If i understood this right, strong names are only relevant for netfx:

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.

https://docs.microsoft.com/en-us/dotnet/standard/assembly/strong-named#why-strong-name-your-assemblies

@kMutagene
Copy link
Collaborator

also the official guidelines discourage publishing two versions of the package: https://docs.microsoft.com/en-us/dotnet/standard/library-guidance/strong-naming (although this looks like the easiest solution for me).

Anyways, we still have the problem of third-party libraries we are using not being signed.

@JakeRadMSFT
Copy link
Contributor

JakeRadMSFT commented May 25, 2022

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.

Let me dig in on this with the experts internally.

Ultimately, there is a policy in the https://github.com/dotnet org's repos and builds that enforces signing. We also want to use this inside of Visual Studio's Model Builder and that is .NET Framework ... which requires strong-named.

also the official guidelines discourage publishing two versions of the package: https://docs.microsoft.com/en-us/dotnet/standard/library-guidance/strong-naming (although this looks like the easiest solution for me).

I'll learn more about this too.

@JakeRadMSFT
Copy link
Contributor

JakeRadMSFT commented May 25, 2022

Anyways, we still have the problem of third-party libraries we are using not being signed.

It looks like you guys don't have too many!

@JakeRadMSFT
Copy link
Contributor

JakeRadMSFT commented May 25, 2022

Okay - I've reached out to some experts and re-read the documentation.

My Thoughts:

  • Let's strong-name sign assembly
    • They main goal of this is as an identifier/differentiator.
    • I'm thinking of this as mainly making sure we have a public key token in the Full Qualified Assembly Name - https://docs.microsoft.com/en-us/dotnet/standard/assembly/names myTypes, Version=1.0.1234.0, Culture=en-US, PublicKeyToken=b77a5c561934e089c, ProcessorArchitecture=msil
  • Let's stick with the guidance and just sign the existing NuGet/Assembly
  • Let's do it w/ Major Version Bump since the Fully Qualified Assembly Name will change.
  • Let's get DynamicObj signed too.
  • Let's just check in a key file like other open source projects - PuppeteerSharp, SharpZipLib
    • This would just be one key used for all assemblies in the project.
    • Some project don't check it in ... but I don't believe there is any benefit for hiding the private keys in this instance. Strong-name signing is not for security ... it's for identity.

Let me know your thoughts! I'm happy to help wire this up for Plotly.NET and DynamicObj. I believe this is a one time cost (and maybe some issue handling) and shouldn't have any more work associated with it.

FYI - I don't have strong opinions on versioning or shipping multiple NuGets. If you like your versioning ( and don't want to bump to 3 just for signing) ... the two NuGet way works for me too.

@kMutagene
Copy link
Collaborator

Thanks for the insights!

I think my preferred approach would be:

  • Signing DynamicObj and all Plotly.NET assemblies
  • bump the major version for all packages

If you could help wiring that up, that would be great. I think we could start with DynamicObj, basically using it for a first dry-run of this process and then apply the same procedure to Plotly.NET.

@kMutagene
Copy link
Collaborator

kMutagene commented May 30, 2022

@JakeRadMSFT I think we are already quite close for wrapping this up for DynamicObj. My last question concerns the AssemblyVersion. Per https://docs.microsoft.com/en-us/dotnet/standard/library-guidance/versioning:

CONSIDER only including a major version in the AssemblyVersion.
e.g. Library 1.0 and Library 1.0.1 both have an AssemblyVersion of 1.0.0.0, while Library 2.0 has AssemblyVersion of 2.0.0.0. When the assembly version changes less often, it reduces binding redirects.

How do we achieve this decoupling of nuget and assembly version? The current build pipeline increases both the assembly and package version (i have tested this by locally bumping the version to 2.0.1):

image

You can also see that the dependencies are all doing this version decoupling (FSharp.Core, netstandard, Newtonsoft.Json)

So i think the last roadblock here is:

  • Is it important to decouple these versions, or can we keep assembly and package version identical?
  • If it is important, how is it done?

@JakeRadMSFT
Copy link
Contributor

@kMutagene let me ask some folks! Sorry I missed these messages!!

@JakeRadMSFT
Copy link
Contributor

JakeRadMSFT commented Jun 6, 2022

My guess is that you're build process is somehow setting the version property via msbuild somewhere which then overrides all the versions.

Packages like newtonsoft set each version:
https://github.com/JamesNK/Newtonsoft.Json/blob/master/Src/Newtonsoft.Json/Newtonsoft.Json.csproj#L7

I believe we can do this from msbuild by setting each version property "AssemblyVersion", "FileVersion" ... instead of setting "version".

https://github.com/JamesNK/Newtonsoft.Json/blob/bf2e2a78e8febf0006ec647f9bde3aa5bbe0ce72/Build/build.ps1#L164

exec { & $script:msBuildPath "/t:build" "/v:$msbuildVerbosity" $projectPath "/p:Configuration=Release" "/p:LibraryFrameworks="$libraryFrameworks"" "/p:TestFrameworks="$testFrameworks"" "/p:AssemblyOriginatorKeyFile=$signKeyPath" "/p:SignAssembly=$signAssemblies" "/p:TreatWarningsAsErrors=$treatWarningsAsErrors" "/p:AdditionalConstants=$additionalConstants" "/p:GeneratePackageOnBuild=$buildNuGet" "/p:ContinuousIntegrationBuild=true" "/p:PackageId=$packageId" "/p:VersionPrefix=$majorWithReleaseVersion" "/p:VersionSuffix=$nugetPrerelease" "/p:AssemblyVersion=$assemblyVersion" "/p:FileVersion=$version" "/m" }

@JakeRadMSFT
Copy link
Contributor

I've never used the build tools you're using (FAKE) but I think it's this:

Properties =
([
"Version", stableVersionTag
"PackageReleaseNotes", (release.Notes |> String.concat "\r\n")
]
@ p.MSBuildParams.Properties)

It looks like it's setting all versions based on NuGet symver ...

let stableVersion =
SemVer.parse release.NugetVersion
let stableVersionTag =
(sprintf "%i.%i.%i" stableVersion.Major stableVersion.Minor stableVersion.Patch)

@kMutagene
Copy link
Collaborator

@JakeRadMSFT exactly, both versions can be set via msbuild params, that was the missing link. The signed version of DynamicObj is live. We can do the same for Plotly.NET soon, i am just wondering if it would be better to get #294 merged first and release the Plotly.NET.CSharp package alongside the 3.0 packages. So this kind of goes in the direction of my question in #285: What's the minimum functionality you would need for getting the notebooks running? Specific charts only? Is default styling enough?

@JakeRadMSFT
Copy link
Contributor

I'll reply in #285 :)

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

Successfully merging a pull request may close this issue.

3 participants