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

Add strong name signing #2575

Merged
merged 1 commit into from
Oct 10, 2022
Merged

Add strong name signing #2575

merged 1 commit into from
Oct 10, 2022

Conversation

AndreyTretyak
Copy link
Contributor

It's a follow-up after my comment here #1604 (comment) that intended to resolve #1604, #2382, and similar issues. I would be grateful if you would consider this change since it would simplify my team's life significantly.

Main justification points:

  • Decision on strong naming here was to sign Octokit before version 1.0, and currently we are at version 3.0, so I hope we can do this long overdue change.
  • Other Octokit packages like Octokit.GraphQL are signed for 5 years already (PR link) and it would be nice to make them consistent.
  • Using StrongNamer as a workaround is a constant source of build and debug problems, since it's essentially decompiling Octokit.dll and compiling it again with the strong name during each build. So customers running not the dll that you've shipped.
  • Strong naming required for multiple use cases for example Visual Studio extensions.

Implementations details:
Signing logic is done in the same way as in octokit.graphql.net using the same key for consistency and also without versioning changes.

Since signing is enabled in the Directory.Build.props it would be applied by default to every new project created in the future in this repository without additional effort.

If for some reason there is a need of disabling strong naming for the specific project this could be done by setting <SignAssembly>false</SignAssembly> in the project. As currently done for Build.csproj and Octokit.Tests.Integration.csproj that reference unsigned dlls.

Another slight change is in applying InternalsVisibleTo attribute for signed dlls. It should be done in the project file directly and with $(StrongNameSuffix) after dll name (to include the public key without duplicating this horrible string).

Side note: One of the options discussed in the epic thread here was to change dll version only on major version changes of NuGet package (Newtonsoft.Json model) and if needed this could be easily achieved by using declaring AssemblyFileVersion MSBuild property with the required dll version. Although I'm not sure how useful it is now when SDK-based projects are able to automatically generate binding redirects for executables during the build.

@nickfloyd
Copy link
Contributor

Hey @AndreyTretyak Thank you for the work here ❤️. I feel like a lot of folks have been waiting for this.

I think I'd like to push this after our next release just so that it is an encapsulated change - I'll most likely release it as a breaking change as well, given the type of change it is.

@AndreyTretyak
Copy link
Contributor Author

Hey @AndreyTretyak Thank you for the work here ❤️. I feel like a lot of folks have been waiting for this.

I think I'd like to push this after our next release just so that it is an encapsulated change - I'll most likely release it as a breaking change as well, given the type of change it is.

Thank you, Nick. I'm looking forward to this. Are there any plans for when the next and following releases might happen?

@nickfloyd
Copy link
Contributor

Thank you, Nick. I'm looking forward to this. Are there any plans for when the next and following releases might happen?

Hey @AndreyTretyak, We'll try to drop what is in main out there tomorrow and most likely push your work here on Monday, tentatively.

@nickfloyd
Copy link
Contributor

Hey @AndreyTretyak, We just dropped our last release before we get this merged in on Monday. Just a quick question - was there any reason you didn't end up signing Octokit.Reactive?

If not, let me know if you'd be able to get that one done or if you'd like me to do it in your branch. I'd like to get both done, so we only have to release one breaking change. Let me know what you think.

Thanks again for getting this hammered out!

@AndreyTretyak
Copy link
Contributor Author

Hey @AndreyTretyak, We just dropped our last release before we get this merged in on Monday. Just a quick question - was there any reason you didn't end up signing Octokit.Reactive?

If not, let me know if you'd be able to get that one done or if you'd like me to do it in your branch. I'd like to get both done, so we only have to release one breaking change. Let me know what you think.

Thanks again for getting this hammered out!

Just checked and Octokit.Reactive.dll would be signed as well.
Setting to enable signing is in the Directory.Build.props file in the root of the repo, so by default all current and future projects would be signed, unless we disable it explicitly by setting <SignAssembly>false</SignAssembly> in the project file the way I've done for Octokit.Tests.Integration.csproj.

@nickfloyd
Copy link
Contributor

Just checked and Octokit.Reactive.dll would be signed as well.

Perfect. Thanks for validating that... we should be g2g on Monday then. 💥

@nickfloyd nickfloyd merged commit 40b2111 into octokit:main Oct 10, 2022
@AndreyTretyak AndreyTretyak deleted the add-dll-sign branch October 10, 2022 17:21
@nickfloyd
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Breaking change Used to note any change that requires a major version bump Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Strongly named assembly issue
2 participants