octokit / octokit.net

A GitHub API client library for .NET
https://octokitnet.readthedocs.io/en/latest/
MIT License
2.67k stars 1.08k forks source link

Signed Octokit Releases - Yea or Nay? #405

Closed shiftkey closed 10 years ago

shiftkey commented 10 years ago

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

SimonCropp commented 10 years ago

@hazzik evidence is not required to "not do something". @shiftkey is asking for "reasons to SN" that have a stronger backing "because I need to SN".

Most orgs think SN provides some kind of extra security which is false. We already have agreement from some MS guys to remove mentions of the word "security" from the strong naming doco on MSDN.

Also here https://github.com/octokit/octokit.net/issues/405#issuecomment-41351198 @shiftkey has articulated the two legit reasons for people wanting SN. that being

I will add another one

All of the above cases can be worked around using several mechanisms

So the problem is just made up by people who are to scared to press one button in VS to sign an assembly, or by people who "suffer" from binding redirects "pain" (never had this before)

I suffer the pain of SN and binding redirects regularly. My paying clients also suffer this pain with regular support incidents basically being "you need to fix your binding redirects".

So the pain it causes is real, just as the pain of not SN is real for others. This discussion is about which pain is better suffered and how many people will suffer that pain

From my experience, once you have ruled out the people who are requesting SN for security reasons, the number of people who legitimately need SN is so small that it is not worth SNing

Also I should note that, as the author of a MSBuild Task, I am one of the people who has a legitimate cause to use SN.

hazzik commented 10 years ago

All of the above cases can be worked around using several mechanisms

I don't want to do any workarounds, I just want it to work without any efforts.

Just because all these "work-arounds" make much more problems than "binding redirects" issue. Just because you do not upgrade your dependencies every minute, but build your software every minute.

So let's discuss initial issue and work-arounds.

  1. Binding redirect issue.
    • Happens some times when you upgrade your dependencies
    • requires 5 seconds to fix.
  2. ILMerge
    • Need to ILMerge every time you try to release
    • Consumes 1-5 minutes each time your build your release
    • Needs at least one hour to configure
  3. SNing the assembly yourself
    • Need to sign the assembly every time you want to upgrade your dependency
    • You can not use nuget
    • Needs some time to implement
  4. Wrapper / AppDomain - I'm not such crazy to implement this.
PureKrome commented 10 years ago

@SimonCropp Great comment! :+1: That all made sense :smile:

yes!

From my experience, once you have ruled out the people who are requesting SN for security reasons, the number of people who legitimately need SN is so small that it is not worth SNing

It's metrics like this (if they could be .. grabbed), I think will help the OctoKit Krew determine which way they will decide to go, IMO. I've been under the (non scientific) guess that the numbers of SN people are small (and at times, a vocal minority) - which is why I've been a fan of not SN'ing stuff. And being OSS, these folks can just clone+SN+Build.

So yeah .. who's the majority user base here?

Meanwhile .. while typing this comment

Hazzik commented just now

Just because all these "work-arounds" make much more problems than "binding redirects" issue. Just because you do not upgrade your dependencies every minute.

.

This is what Binding Redirects (code smell) makes me think of...

..

Which leaves us with ... ..

glennblock commented 10 years ago

If you can get away with it, I'd vote nay! Binding redirects and having to deal with GAC issues have caused me so much personal pain in my years of using .NET (which are many). If you have to do it, @JamesNK's approach of only incrementing the assembly version for major versions seems reasonable. So far we've been able to skirt having to deal with this on the projects I work on. I hope sincerely we can continue to do that.

hazzik commented 10 years ago

(code smell)

HA-HA-HA

Fck it is so ridiculous.

hazzik commented 10 years ago

A long time ago I was working on a project when we were signing projects, and so we had to decide to do this "workarounds" or not to use the fcking unsigned library. Guess what my team chose?

So my IMO: if you are doing some public shit - sign the fcking assembly. You can do whatever you want with your private stuff: ILMerging, include as source, include as source intro resource and compile, encode as brainfuck and execute with interpreter written in javascript.

SimonCropp commented 10 years ago

@glennblock it should be noted that the approach taken by @JamesNK has negative effects on those people who actually do need to GAC. Since the version is not updated on every release it is a last one wins case. So you could have a case where two apps install on one machine, both GAC diff file-versions but same assembly-version and now one app wont work.

Not saying this is a bad approach, just that you should be aware of the side effects.

I wonder if @JamesNK had had many issue with the above effecting his users??

SimonCropp commented 10 years ago

@hazzik of the "reasons for needing SN" i listed above which one do you most regularly fall into?

phillip-haydon commented 10 years ago

Pretty sure he falls into the "I feel it gives me a sense of security and because my boss told me so"

glennblock commented 10 years ago

@SimonCropp I agree it's not perfect. It does address the virality of signing, but I can see how it doesn't completely solve the GAC issue / creates new issues.

hazzik commented 10 years ago

@SimonCropp by the reason: I do public available assembly and for whatever reasons some of my users want it to be signed.

anaisbetts commented 10 years ago

@hazzik Be nice. This is your last warning.

SimonCropp commented 10 years ago

@hazzik can you contact some of your users and find out why they need things to be signed? it is the actual users we want feedback from.

I would appreciate it.

glennblock commented 10 years ago

Isn't the more common issue here the virality? That a strong-named assembly can only reference other strong-named assemblies i.e. if we ship a library that is SN, it can't depend on Oktokit unless Oktokit itself is also SN?

glennblock commented 10 years ago

I remember when I was in patterns & practices we had so many people wanting us to have signed Prism assemblies for this reason. It had nothing to do with GAC and everything to do with the fact that signed assemblies wanted to depend on Prism.

hazzik commented 10 years ago

@paulcbetts I'm nicer than ever.

PS: I'm Russian and you can not scare my by any warnings.

SimonCropp commented 10 years ago

@glennblock

Isn't the more common issue here the virality

Disagree.

Removing the virility would most likely address the artificial requirements of "we need thing to be SNed for security" or "because my manager told me to". Since these teams will be allowed to SN their assemblies in ignorance of the fact that things they reference are not SNed

However it would not address and of the legitimate uses that i mentioned here https://github.com/octokit/octokit.net/issues/405#issuecomment-41640976

But I admit removing the virality would prob make the arguments disappear. the people who understand what SN is actually useful and really need it can use the work-arounds. the rest can continue under the perception of "SN is about security and I am being secure by using it"

is that too cynical?

phillip-haydon commented 10 years ago

@paulcbetts sorry its my fault. I'm winding him up.

glennblock commented 10 years ago

@SimonCropp I see your point on not solving the VS issue. My point is that the majority of requests from SN (in terms of numbers) don't stem from VS extension authoring requirements (at least based on what I have seen).

hazzik commented 10 years ago

If some one really suffers from fixing binding redirects errors, or his religion does not allow to use binding redirects at all, then he needs to use

sn.exe -Vr <assembly>

thecodejunkie commented 10 years ago

The Nancy nugets have well over 300k downloads and every now and then we get a question about strong-naming. Not a single one of them have been able to provide any tangible resigning beyond that a work policy requires it (for security reasons or other) or that they have a dependency on another strong named assembly. For the "security related" cases, not a single one have been able to provide us with an explanation was a signed assembly, from a couple of random people, would be able to provide them in terms of security - we're a not a "trusted authority" in any way + many OSS projects that sign assemblies put the key in the repository - further negating the security aspect.

The headache grows when the strong-named assembly is frequently updated (and version bumped). I would advice against SN, but if you have to then please know your audience and make the path that causes the least amount of pain for the majority. Without having any firm metrics to fall back on, I am pretty sure that you have more non-SN-shop users than SN-shop users (due to the nature of the library). It is my personal opinion that VS extension authors are also in a clear minority and want this for convenience reasons..

The "Assembly X is strong-named so I need your assembly to be signed as well"-argument is the worst one. I for one won't help fuel a fire that someone else started

thecodejunkie commented 10 years ago

@hazzik or (if we flip the coin) if anyone really suffers from the lack of strong-naming they can sign it themselves?

distantcam commented 10 years ago

@thecodejunkie I think the point against "they can sign it themselves" is then it's harder to consume the library than simply invoking nuget.

thecodejunkie commented 10 years ago

@distantcam vs. the friction that strong-naming does cause in many scenarios (as mentioned by previous posters) + you have to realise that a vast majority of signed assemblies has not taken the signing approach chosen by JSON.NET. Even if they had, it is not without friction. Jeremy Miller wrote a post about it recently http://jeremydmiller.com/2014/04/28/fubumvc-lessons-learned-strong-naming-woes-and-workarounds/

aaronpowell commented 10 years ago

Recently the Lucene.NET developers have been talking about ditching strong naming from the project starting with the next version - http://mail-archives.apache.org/mod_mbox/lucenenet-dev/201404.mbox/%3CCAHTr4ZubJCApjQF32noJ_i1nwbDp_pdweFa9FCeNaEEfacYRRA%40mail.gmail.com%3E and it could be worth reading through the thread on there as well to get their perspective on the problem.

For the record it is actually at a vote now - http://mail-archives.apache.org/mod_mbox/lucenenet-dev/201404.mbox/%3CCAHTr4ZsWmJ8VT9NpoHkhza93DH%3Dnu_BedYcUg9Rx1KUQ1VX2Qg%40mail.gmail.com%3E

And that's the end of that chapter

And that's the end of that chapter

mythz commented 10 years ago

I've also wasted more time on strong naming binding redirects than I care to repeat, and I've been strongly opposed to strong naming ever since which IMO it's incompatible with OSS and continuous delivery that disrupts the iterative OSS workflow, introducing friction in forking, making use of customized builds and contributing back changes.

If you make your SN key public, just so forks can actually test their customized builds than that pretty much negates one of the major reasons why CIO's were asking for it in the first place (i.e. under the perception of increased security), in this case all we're left with is that we need to strong name to support existing users who can only use strong named dlls, with the original rationale negated.

Eventually I've had to add support for strong naming in ServiceStack v4 as it's a popular requested feature of ServiceStack's enterprise customers who wish to use it in enterprise products (e.g. SharePoint) that mandate it. But as I didn't want to burden the majority of the user-base (myself included) who don't want it, I've added a parallel track of Signed packages with a .Signed suffix (https://servicestack.net/download#signed). It takes more effort on my side, but with this approach we can mitigate the effects of strong naming to only customers who really want it and are more likely hardened to deal with their effects.

The one exception is the ServiceStack.Interfaces project which is always signed as it provided the most value with the least impact since only the binaries that requires sharing is the DTO dll which only needs a dependency on SS.Interfaces, and the only way for them to be shared with both Signed/Unsigned projects is to be signed. It's done in the least intrusive way where we keep the [AssemblyVersion] constant and resort to updating [AssemblyFileVersion] to report the true version number. Because its Signed only we're realistically able to make "official" changes and builds of ServiceStack.Interfaces and not any of the forks who are maintaining their own customized builds. This is only manageable because SS.Interfaces is rarely updated so has created minimal friction, it does mean anyone else either have to workaround solutions not making changes to SS.Interfaces or propose changes that have to be implemented on my side, then notify them when updated builds are available.

I would've preferred it if strong naming had never existed, if it didn't CIO's would've thought twice if it was justified, if it was concluded some verification was needed, then the simplest solution that worked would've been created and not the unnecessary complexity imposed on us today causing an artificial divide between signed and unsigned packages and the support burden it generates. It doesn't in most other platforms and they're better off without this imposed artificial complexity that continues to plague .NET to this day.

In future I see both the GAC and strong naming being marginalized whose benefits doesn't justify their existence. SN makes more sense if software was sealed and delivered waterfall, but by its nature software is continually evolving and updating where we're striving for fast iteration times, continuous delivery and replay-able automation.

We're pretty far from this in .NET (and may never get there during our Careers), but I see the nirvana dev environment is to move to a more live-reload source-based module system where we can simply reference GitHub repos as dependencies and be able to run C#/F# as source files directly (i.e. by-passing devs seeing/dealing with dlls or other interim artefacts themselves) which I also see as one of the major appeals of projects like ScriptCS. Making a change in this world is just editing source code + hitting replay, likewise making a change upstream is simply updating modules after merging your pull-request (or can even switch reference to use your own fork). There's no place for strong naming in this world, it only hinders it.

glennblock commented 10 years ago

"I would've preferred it if strong naming had never existed" :clap:

haacked commented 10 years ago

I go play :soccer: and this is what I come back to?

No matter what you do no one will be completely happy but that is the best solution I have found for keeping the most people happy.

@JamesNK, are you happy with this? Would you do it again? Would you do it for a library like Octokit.net which is different in nature than JSON.NET. I'm just curious about your opinion given your experience.

I mentioned that Octokit.net is different in nature and I should explain what I mean. JSON.net is a pretty fundamental library. It's the type of library that many other libraries need to reference. Thus the applications that reference those libraries are affected if JSON.NET doesn't strong name.

But I don't expect many other shared libraries will reference Octokit.net. I'd expect it would be referenced directly by applications. Therefore my gut is that the need to strong name is not as strong.

If my assumption is correct, strong name isn't the only solution to the issues raised here. We could offer another Octokit.Source package that contained the source code for Octokit.net which would then get compiled into your own application. The strong name issue in that case goes away.

paulbatum commented 10 years ago

Going to ask a stupid question and I am not even going to use any funny gifs. My team at Microsoft builds an Azure service and we ship our sdks on NuGet. Our sdks are integrated into the tooling experience in Visual Studio so they have to be strong name signed - we have no choice in the matter.

If you trace the dependency chain for one of our libraries, you will find it includes several projects discussed in this thread. Automapper, json.net, etc.

My stupid question: what would you have my team do if the libraries in question stopped distributing strong named versions on NuGet? The only answer I can glean so far from this thread is to ILmerge. I'll admit I haven't actually gone through the exercise of trying this out, but doesn't this have some major problems? Firstly I don't think this works for frameworks that support rich extensibility. E.g if we IL merge in some version of automapper, how can our users reference and configure automapper themselves and have our framework pickup those changes? Secondly, I no longer get the very nice behavior of the user being prompted to accept the license of each of our dependencies.

I genuinely am not interested into getting involved in the religious debate here. I just want to understand How To Make My Stuff Work(tm).

phillip-haydon commented 10 years ago

@paulbatum when you ILMerge you would flag it as internalise which would hide the API from the user, allowing the user to continue to reference what ever version they wanted.

This is what RavenDB had to do when JSON.net became signed. (if my understanding of the issue was correct)

paulbatum commented 10 years ago

But that doesn't work. Now there are two automappers. There needs to be just one, or the developer experience suffers. Automapper isn't an implementation detail for us - it's an important part of the dev experience.

On Tuesday, April 29, 2014, Phillip Haydon notifications@github.com wrote:

@paulbatum https://github.com/paulbatum when you ILMerge you would flag it as internalise which would hide the API from the user, allowing the user to continue to reference what ever version they wanted.

This is what RavenDB had to do when JSON.net became signed. (if my understanding of the issue was correct)

— Reply to this email directly or view it on GitHubhttps://github.com/octokit/octokit.net/issues/405#issuecomment-41650587 .

phillip-haydon commented 10 years ago

Of course there's two, there's your one that you use internally for your library, and the one the consumer of your library uses, you don't use the consumers version, you use the version you internalised.

The person consuming your library doesn't even know you're using automapper.

paulbatum commented 10 years ago

I am not sure how to state it any clearer: Automapper is not an implementation detail for us. Our framework is not a black box.

On Tuesday, April 29, 2014, Phillip Haydon notifications@github.com wrote:

Of course there's two, there's your one that you use internally for your library, and the one the consumer of your library uses, you don't use the consumers version, you use the version you internalised.

The person consuming your library doesn't even know you're using automapper.

— Reply to this email directly or view it on GitHubhttps://github.com/octokit/octokit.net/issues/405#issuecomment-41650986 .

phillip-haydon commented 10 years ago

Then you've designed your API poorly in my opinion, you would need to expose your API for writing the consumers models to your implementation of AutoMapper, you're only using AutoMapper for your internal conversion of their DTOs to your own correct? So I would assume you only need them to register the classes that need to be converted from/to.

But right now it sounds like you've forced the dependency of AutoMapper onto the consumer, not for your internal use, that's bad because you're forced into a life long dependency on AutoMapper. If you wanted to change AutoMapper for ValueInjector, you can't because you've given an explicit dependency to the consumer to configure on AutoMapper.

paulbatum commented 10 years ago

Unfortunately I haven't given you the necessary context because I am in bed on my phone and I should be sleeping. Can we avoid the design debate and continue the discussion with my constraints as described? If we throw an argument about how to and how not to design a framework into the mix this thread will probably gain sentience and destroy us all.

On Tuesday, April 29, 2014, Phillip Haydon notifications@github.com wrote:

Then you've designed your API poorly in my opinion, you would need to expose your API for writing the consumers models to your implementation of AutoMapper, you're only using AutoMapper for your internal conversion of their DTOs to your own correct? So I would assume you only need them to register the classes that need to be converted from/to.

But right now it sounds like you've forced the dependency of AutoMapper onto the consumer, not for your internal use, that's bad because you're forced into a life long dependency on AutoMapper. If you wanted to change AutoMapper for ValueInjector, you can't because you've given an explicit dependency to the consumer to configure on AutoMapper.

— Reply to this email directly or view it on GitHubhttps://github.com/octokit/octokit.net/issues/405#issuecomment-41651585 .

phillip-haydon commented 10 years ago

I can only comment on what I know, you asked about AutoMapper, I'm saying that you can internalise AutoMapper for your internal use, and this fixes the SN issue.

But you're saying that the user will have 2 AutoMappers now, that's simply not true. The only reason they would have two AutoMappers is because you are exposing the AutoMapper dependency to the consumer, so your design prevents you from being able internalise.

This is a problem with your design, not with the fact that SN can be solved by internalising AutoMapper.

paulbatum commented 10 years ago

@phillip-haydon Try to imagine an opinionated .net based web stack that is hosted as a service and you might understand our scenario better. I think your assumption about hiding the dependency makes sense if you are talking about a library that acts as a black box, but that is simply not the case here.

@simoncropp wrong team, and not constructive to the purpose of this thread.

PaulStovell commented 10 years ago

@paulbatum one example is in RavenDB where some parts of JSON.NET are interalized, but public and nested under their namespace. So when declaring properties you can do:

[Newtonsoft.Json.JsonProperty]   // For the real JSON.NET
[Raven.Imports.Newtonsoft.Json.JsonProperty] // For Raven's
public string Foo { get; set; }

It's actually nice because you can specify different serialisation options for different use cases. Likewise, AutoMapper uses a lot of static configuration (IIRC, it has been a while) so having a separate copy from a separate namespace might allow people to customise the way you use it, without affecting other libraries that use it.

Not suggesting that this is the answer to "should we SN or not", just pointing out a shade of grey between "reference a strong-named assembly" and "ILMerge and internalize". I think it would be quite bad if everyone did this.

davidfowl commented 10 years ago

I'm going to suggest something radical because it's 2 AM and I should be sleeping. Everyone in the .NET OSS community should just stop strong naming everything. This will have network effects and eventually the GAC and strong naming in general will die on its own :smile:

Somebody should make a logo for this movement.

PaulStovell commented 10 years ago

@davidfowl eventually that will happen. Unfortunately there's a large company located somewhere in Redmond that make a lot of platforms that still demand strong named assemblies :)

damianh commented 10 years ago

ILMerge is still an MSR project that I think are run by the same people that run code contracts and pex. i.e. it's a tough call to rely on this stuff. (ILRepack seems to be showing some promise)

\ Really wish ILMerge / declaring references as private / internal was a first-class compile-time concern **

SimonCropp commented 10 years ago

@damianh ILMerge can be called as a .net assembly and u can use c# code from within MSBuild ;)

damianh commented 10 years ago

@SimonCropp Yeah, but not quite first-class up-in-your-face enough (imho)

SimonCropp commented 10 years ago

@damianh lol. no. hence the ;)

jen20 commented 10 years ago

My 2¢ - if someone finds a library useful enough to want to use and wants to strong name (for whatever reason), it should be them that feels 100% of the pain involved in strong naming it - they should absolutely have to build it themselves and strong name it in order to extract whatever benefit they perceive there being.

jchannon commented 10 years ago

What @jen20 said!

iancooper commented 10 years ago

I ban strong naming and the GAC everywhere I work - it's a trade-off between maintainability and security, and the security benefits, particularly for OSS libraries are so weak that they don't override the maintainability benefits of not having SN. I'm honestly gobsmacked to discover that people still use SN and the GAC. No one I know has been using a SN/GAC approach post the first five years or so of .NET where they all got burned badly enough to kick it out. My estimation of anyone using SN falls, very rapidly.

SimonCropp commented 10 years ago

@iancooper re "the security benefits," http://stackoverflow.com/a/22977177/53158

the Runtime only checks the strong name signing key/cert but does not Hash the DLL/EXE to match the key

akoeplinger commented 10 years ago

I basically agree with what @jen20 said, however I think we can make it less painful for people that still need SN - by improving the tooling. NuGet could simply apply SN with a provided key to any non-SN assemblies when it detects you want to install it into a SN-project.

This was already discussed at length in this thread a few years ago and the conclusion by @Haacked and @davidebbo was that auto-signing was a viable option. Did anybody of you change your opinion on this since then?

Granted, there will be issues and edge cases, but IMHO in the majority of cases this should it make it less painful.

GeertvanHorrik commented 10 years ago

I think we haven't reached a consensus yet, and I hope we can do that in a professional (non personal opinionated) way. I will try to list the pros & cons again. If you have additional pros and/or cons, please list them here:

Cons 1) Assembly redirects can be a pain 2) You cannot update assemblies separately. For example, you have product X which uses Y. Both are strong named and both have a newer version (thus X has a dependency on Y vNext). With strong naming you cannot handle this scenario (but as a developer, I would strongly recommend to update Y anyways).

Pros 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) 3) Assemblies can be used in the GAC 4) You can load multiple assemblies with different versions (think for example of serializable objects (please, don't start a discussion on serialization techniques here) 5) You can use the product with NuGet in products that require strong named assemblies (Sharepoint for example if I am not mistaken) 6) No need to mess around with ILMerge

I hear some people that see strong naming as a major drawback / pain. I hope they can add their cons to the list so the decision is not taken on personal emotions. Note that I didn't add security as pros because I don't think (and already mentioned in this thread) strong naming != security.

In the end I really don't care what it will be, as long as it is best for most people.