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

Signed Octokit Releases - Yea or Nay? #405

Closed
shiftkey opened this issue Feb 28, 2014 · 181 comments
Closed

Signed Octokit Releases - Yea or Nay? #405

shiftkey opened this issue Feb 28, 2014 · 181 comments

Comments

@shiftkey
Copy link
Member

So #404 landed, and I wanted to have a proper discussion about whether we should do this in an official capacity for Octokit releases.

So let the fur fly (remember, bonus points for good reasoning and being gentlemanly) and I'll check back in a few days to see who is left standing...

fight3
fight2
fight4
fight1

@rprouse
Copy link

rprouse commented Feb 28, 2014

I submitted the pull request, so I will be the first to jump into the fray.

First, let's make sure it is clear what we are talking about. This issue is about Strong-Name Signing, not signing the code with an SSL certificate purchased from a signing authority.

Strong-Name signing just provides managed assemblies with a globally unique identity, whereas the second is used to verify the publisher of the application, in this case GitHub. The signing key for strong naming is just a key that a developer creates on the command line and checks into the repository. Ideally they would be secured, but that does not work for open source projects.

Public libraries should always be strong named otherwise they cannot be used by applications that are strong named or in Click-Once deployments. In my case, I am working on a Visual Studio extension which should be strong named. I can get around it, but that will limit what I can do in the extension.

Look around at most of the most popular non-Microsoft open source libraries, they are all signed and are not putting artificial limitations on their users. Here is a short list, Newtonsoft.Json, NUnit, NLog, Ninject, DotNetOpenAuth, log4net, AutoMapper, Castle (Windsor, MonoRail, etc). In all cases, their keys are in their repositories.

If you want to troll around your bin directory and see what is signed, sn -T Ninject.dll

So, to sum up, Strong-Name signing costs you nothing and allows your library to be used everywhere and play nice with others.

Okay, who's next? Gloves off, Internet fight time...

Internet Fight

@phantomtypist
Copy link
Contributor

LOL.

This is an issue on another public project I try to help with.

Let me throw this idea out there that I've been toying around with. (Disclaimer: I've seen the idea on a few projects so it's not an original idea.)

What if we were to set up the build server to generate two different sets of assemblies, one Strong-Name signed and one not signed? As a byproduct it would also generate two sets of NuGet packages. The unsigned NuGet package would continue to have the same name. The signed NuGet package would be named something along the lines of "Octokit-Signed".

The one downside to introducing signed assemblies, only, is that it could lead to assembly version redirect issues for end-users. log4net ran into this issue and got a little backlash from it.

I'd like to think it makes people on both sides of the fence happy. Thoughts?

tumblr_mh3kaa1r0m1qffb31o4_400

@haacked
Copy link
Contributor

haacked commented Feb 28, 2014

Two sets of NuGet packages is very bad for libraries that other libraries depend on. What package should they reference? This might not be as big a deal for Octokit.net.

I believe that eventually, strong naming as we know it will be burnt down by the CLR team and replaced with something better.

Using Octokit in Visual Studio seems like a core scenario to me. I would be 👍 to taking the JSON.NET approach.

  1. Strong name Octokit.net and Octokit.Reactive.
  2. Freeze the assembly version at 1.0.0. The CLR assembly version of these DLLs would never change again.
  3. Continue to increment the NuGet version.

In this plan, NuGet becomes the unit of deployment, not the assembly as has traditionally been the case with .NET.

By freezing the assembly version, we don't have to worry about binding redirect problems in the future. Octokit is pre-release right now so I don't care about introducing breaking changes like this at the moment.

@haacked
Copy link
Contributor

haacked commented Feb 28, 2014

Also, while you should never rely on strong name keys to verify an assembly, I do worry that people still do. So we should consider having a set of private keys in the project for builds and another set for the official releases that we keep private.

The alternative is just to have one set of SN keys and have them in the project and hope people by now understand SN verification is a bad idea.

@phantomtypist
Copy link
Contributor

@haacked, I do agree that two NuGet packages are a bad idea. That's why the other project I'm involved in hasn't proceeded in any direction yet.

Freezing the assembly versions would work to resolve the binding redirect (I forgot the name for that) problems. I can't think of any downside to this off the top of my mind, but can you think of any?

@rprouse
Copy link

rprouse commented Feb 28, 2014

@haacked, I also agree that two sets of NuGet packages is a very bad idea, especially if you do freeze the assembly version number which takes away the only downside that I know of. We've been having the same discussions over on the NUnit team recently, but at least we have the advantage that unit tests are recompiled often.

As for keeping the key private, if you want to, you could have the build check for the existence of a key and generate one if it doesn't exist. Then you just need to drop the key in on the build server.

That said, from my experience, most open source projects just check it into the repository. I think the downsides of that are much lower than the consequence of ever losing the official key which would be a breaking change if you had to regenerate it. Plus there is just the headache of keeping track of the official key and making sure it is always used for the production builds.

@akoeplinger
Copy link
Contributor

+1 on freezing the AssemblyVersion number. The AssemblyFileVersion can be used to store the real version number instead.

If I'm not mistaken, you could increment the assembly version on major releases (i.e. 1.0.0, 2.0.0, 3.0.0) as those would be breaking and require recompilation anyway when following SemVer?

@rprouse
Copy link

rprouse commented Mar 1, 2014

I was thinking more about freezing the assembly version number and wonder if it is necessary for this project. There are usually only problems for libraries that tend to get pulled in from other libraries. For example, when a project uses two NuGet packages that reference different versions of log4net.

I don't think that will be a problem for this project as it will probably be a top level package.

@rprouse
Copy link

rprouse commented Mar 1, 2014

If this ends up getting the thumbs up, I would be happy to take my original pull request and incorporate whatever is decided here. Just let me know.

@pmacn
Copy link

pmacn commented Mar 1, 2014

If the version number was to be frozen. What would the benefit be to freeze it indefinitely as opposed to updating it for major releases (i.e. breaking changes)? Don't we want it to break at a predictable time if the assemblies are not actually compatible?

Also this 👍

I don't think that will be a problem for this project as it will probably be a top level package.

Was going to say that I would also rather see the key be public and checked into the repository. But considering the above statement, using a private key becomes much less of an issue and I really have no preference one way or another re: public vs. private 🔑

In summary I would say Sign it!
If we have to freeze the version number (not convinced that we do) at least update for major releases.
No preference on public/private key for official releases.

@shiftkey
Copy link
Member Author

I hate it when @paulcbetts writes what I wanted to

reactiveui/ReactiveUI#43 (comment)

Every time a new contributor comes along, they now have to have an extremely demotivating fight with Visual Studio to get the project to build, or to replace the official binaries with ones they've built for testing.

Anyone have a sample/blog post around about doing this without touching the csproj files so we can avoid this pain?

But I'd lean towards Authenticode signing for this sort of thing, because It's The Real Deal.

@haacked
Copy link
Contributor

haacked commented Mar 13, 2014

@shiftkey what paul writes about is a non-issue if we just include the official snk in the solution. Problem solved.

Remember, we're not doing this for security reasons. We're doing it so people can use Octokit.net within a Visual Studio extension. I bet you can see the appeal of that to me. 😉

@anaisbetts
Copy link
Contributor

@haacked I mean, I currently use ReactiveUI in a Visual Studio Extension, I don't know where this "You must SN every binary" is coming from

@haacked
Copy link
Contributor

haacked commented Mar 13, 2014

I mean, I currently use ReactiveUI in a Visual Studio Extension, I don't know where this "You must SN every binary" is coming from

People keep telling me this 💩. If we don't need to sign it, then let's not. What the hell? Can I get some good intel here? 😏

@shiftkey
Copy link
Member Author

Can I get some good intel here? 😏

This seems like an interesting breakdown of the VSIX situation: http://stackoverflow.com/a/16452046/1363815

Reasons for strong naming:

Also, if you didn't strong name sign, you do risk a name collision with another extension.

template wizards do require strong name signing, but only because they must be installed into the GAC

If your package exposes a public API which other extensions consume, you might be referencing a common DLL as the other people consuming your public API.

If you created a DLL called "Package.dll" and another extension also did, and neither of you strong name signed your binaries, it's possible the CLR will get a bit confused here. So if you weren't strong name signing, make sure your assembly name is "unique enough" to avoid this risk.

Reasons against:

If you are already VSIX-deployable and don't need anything in the GAC, then no, Visual Studio makes no requirement that you are strong name signed.

Emphasis mine

@anaisbetts
Copy link
Contributor

Also, if you didn't strong name sign, you do risk a name collision with another extension.

lol ok

template wizards do require strong name signing, but only because they must be installed into the GAC

If you install Octokit into the GAC you've got bigger problems

If your package exposes a public API which other extensions consume, you might be referencing a common DLL as the other people consuming your public API.

This has happened before with libgit2sharp, it's the one legitimate reason you might want to SN.

@rprouse
Copy link

rprouse commented Mar 13, 2014

I am the one working on the Visual Studio extension. Not signing the extension is not causing any major problems for me and as long as I keep my assembly names unique, I shouldn't have a problem.

It does prevent anyone from creating a Visual Studio template extension with wizards which is a possible usage, but to be honest I can't think of an Octokit.net use case for a wizard. Maybe automatically create and populate a Github repository for new solutions? Actually, that would be kind of cool 👍

@paulcbetts, I've done template wizards before. It isn't Octokit that needs to be in the GAC, just the wizard, but it means that the rest of the extension (not in the GAC) needs to be signed and therefore all the assemblies you use with it.

If anyone is interested in checking out or helping me beta test the Visual Studio extension, add a comment to rprouse/GitHubExtension#47 and I will get in touch.

@fahadash
Copy link

We have a huge application that we are trying to use ReactiveUI in. Our application is strongly named and it is up to our architects to allow removal of strong-naming which they refuse to. Now we are having a catch-22 with this ReactiveUI. Paul (I respect this guy) has not come up with a good reason not to have ReactiveUI strongly-named.

I have never had trouble getting public libraries from NuGet because they generally are all strongly-named. We cannot maintain a branch just for strong-naming, it would be an extra effort for not a good reason.

@SimonCropp
Copy link
Contributor

note the VSIX+share appdomain issue also exists for msbuild tasks. in some cases you can have multiple slns sharing the same msbuild appdomain. SN does avoid this.

But this should be a deployment concern for people writing a task or VSIX.

Options

  • ILMerge your dependencies and then SN the result
  • manage your own appdomain to ensure isolation.

@jbogard
Copy link

jbogard commented Apr 23, 2014

I tried creating an unsigned package. People unhappy. I tried creating two packages. People unhappy. Eventually I just went with signing the assembly, freezing the assembly version at just major/minor versions, and nobody has complained since. I think the reality is this is the most accessible, least bad choice.

@anaisbetts
Copy link
Contributor

If someone's going to bear the suffering of Strong Naming, it's going to be the people who want it, not the Open Source community and everyone else sane who doesn't want it. -:100: to strong naming.

@jbogard
Copy link

jbogard commented Apr 23, 2014

Maybe my project is different, but I've never received in 6 years anyone asking to remove strong naming. Only to put it in. I error'd on the side of inclusion and maybe (?) pain for some, versus exclusion.

I also dual licensed my project, which I didn't care about, but also led to inclusion for folks that can only accept MIT or Apache. But what can I say, I'm a pleaser.

@GeertvanHorrik
Copy link

I agree with @jbogard. I was strong naming first, nobody complained. Then someone convinced me that strong naming was bad (at least, it should be banned). However I get more and more requests to strong name it again. The downsides of strong naming are:

  1. ???

And the pros are:

  1. People can decide whether they want to strong name themselves (both ways are working with strong named assemblies)
  2. You can use assembly redirects in your config files (and then it is up to the user whether he wants to redirect breaking changes versions (as in 3.x => 4.x)

I know strong naming is nonsense for some, but as long as the users are happy, why not? It's not that it costs you money, takes a lot of time or is very complex.

@jbogard
Copy link

jbogard commented Apr 23, 2014

@GeertvanHorrik to add to those pros:
3) Strong-named assemblies can only reference other strong-named assemblies. For me, this meant I locked out a ton of folks from using my library
4) Some companies have arcane, insane policies around strong-naming ALL THE THINGS. It's not the developer's policy, so why punish them.

Ultimately, my assembly versioning policy removed most (or nearly all) the pain of strong naming. I reversed my position as it was really pride that was the only thing keeping me from strong-naming. But, this really only applies to my library and how it was being used, referenced and applied. Dunno about OctoKit but thought I'd share my experiences.

@GeertvanHorrik
Copy link

@jbogard

3 == 1
4) Agree, but is probably the reason why we "need" 1 ;-)

@anaisbetts
Copy link
Contributor

I'm reminded of a Richard M. Stallman Quote:

"This is where I am great. I am great at being very, very stubborn and ignoring all sorts of reasons why you should change your goal, reasons that many other people will be susceptible to. Many people want to be on the winning side. I didn't give a damn about that. I wanted to be on the side that was right, and even if I didn't win, at least I was going to give it a good try."

@nickfloyd
Copy link
Contributor

Hey Friends,

As a factor of closing the loop on this - we have just shipped the first signed Octokit .NET. Regardless of where you might've been originally on this approach - after a few years, we have completed what we set out to do after this conversation.

❤️ Thank you to everyone who contributed to the above and to Octokit .NET itself - we could not do any of this without y'all!

Special hat-tip to @shiftkey for having the courage to begin this discussion and making the decision and to @AndreyTretyak for implementing it.

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

No branches or pull requests