-
Notifications
You must be signed in to change notification settings - Fork 331
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
[Signing] Maintaining SignToolData file #58
Comments
One thing we tried to do in KoreBuild was generate the sign data automatically from the .csproj. https://github.com/aspnet/BuildTools/blob/dev/docs/Signing.md#config-via-csproj For the most part, aspnet packages follow standard conventions, and we can auto-generate all the metadata we need for signing. I'm not sure if this would be flexible enough for other teams. |
Why do you think dropping a file next to binary that it should be signed is more reliable than updating a file? In both cases it requires developer effort to achieve. In general we haven't really had the problem you specified with SignTool. While the individual binaries within a repository do get added / deleted with some regularity the consumable outputs of the repository change much less often. For example the set of VSIX / NuGet packages Roslyn produces changes a couple of times a year while the set of projects changes with some regularity. SignTool pretty aggressively digs through these VSIX / NuGet packages and requires that every item contained in them be accounted for. Either by explicitly labeling it as not needing to be signed or by verifying it's in the list of binaries to be signed. This makes it quite hard to sneak in a signing error. The moment a new DLL is embedded in any of our VSIX / NuGet it gets caught in Jenkins PR that it's not properly accounted for. Note: the necessity of listing binaries like System.* as not needing to be signed is tedious. We have a work item to remove it now. Probably one of the first changes I'll do after I move it over to arcade. |
The file is generated as part of the build based on a set of projects that set a signing property that we set in our common dir.props. My main goal is that we have some strategy that makee maintaining this simple/automatic, and ideally a side-effect of the build as opposed to being committed and manually authored. |
Why do you think developers are more likely to set this property than update the json file? My viewpoint on this: any manual action by developers is equally likely to be forgotten. Don't care what it is. Even if you make it central and on by default then developers will forget to turn it off and you'll end up signing and publishing code you never should have. The reason we went with this approach is the following:
I agree that there is a whole though. If you add a new project and it's not included in one of your existing assets then it can slip through the cracks. In practice though that hasn't happened since we moved to this system. |
Because the property is automatically set in the common props file that is included in all the shipping library projects. So as long as folks just copy/paste a project file and follow the same convention it will just happen for them. I don't know how many times I've had to fix official build breaks because binaries that should be signed weren't correctly added to the signing list so as part of our move to github/open source I made sure we would minimize those type of issues by not requiring developers to do anything special to get signed. |
I tend to prefer towards the side where signing is just a by-product of the build and the right things happen versus having to maintain a config file. It's also nice to have the code itself handle the unique cases rather than a separate config. Either way, there are certainly ways that things can fall through the cracks or be signed incorrectly or whatever. The main reason I can get behind the "SignTool" concept is for potential prodcon scenarios where signing is a separate process from the repo build itself and occurs over a composed set of repos. I know there used to be discussion about this, I'm not certain if that is still a goal or non-goal for prod-con.
If the build itself generated the config file (and you maybe commit it to the repo), that would provide many of the same benefits. |
How do you propose the build "does the right things"? It requires some sort of manual intervention. Better to have it in a declarative form that can be tooled. Bears repeating I guess but we had all of the hypothetical problems you all listed when we used your recommended approach (derived from build). Since switching we've had none of them.
Disagree. One of the other benefits of the config file is it's declarative in history. When files are added, or removed, from signing it's visible in the PR. Changes to build which accidentally remove signing (through subtle build interactions) are not visible. |
I agree with your principal, and I suppose we're talking about an implementation detail, but I'd expect a build generated file that's intended for commit to still have to go through pr. Ie. It'd be weird if an official build generated a file different than what was in the repo.
I imagine you'd still need something in the build (msbuild properties or Metadata). Something more akin to aspnets implementation which @natemcmaster referenced https://github.com/aspnet/BuildTools/blob/dev/docs/Signing.md#config-via-csproj |
aspnet's solution is very similar to the core repo's solution as well. See https://github.com/dotnet/buildtools/blob/master/src/Microsoft.DotNet.Build.Tasks/PackageFiles/sign.targets. A library project can opt-in to signing (which is the default for src projects) and can set the strong-name key and authenticode signatures (also defaulted). |
Also FWIW: a large number of repos use SignToolData.json. There have been zero complaints so far about the need to maintain the file (other than the exclusion lists). It's also lead to a big jump in reliability, errors caught at PR time and build throughput. Definitely we're going to be doing more initial maintenance in this repo at the start. But this isn't a normal developer cycle. We're in the process of merging a lot of existing and well formed tools into a central repository. Edits to SignToolData will certainly be higher here at first but this is also an abnormal level of churn. |
I'm fine if folks would like to manually maintain the file but I don't think it should be a requirement and I'm ok with the format I just want to enable an option to generate as part of the build instead of manually maintaining it. |
I have another suggestion: please try this before you reject it. Literally the only people who want this option have not tried it. Why create deviations on the repositories based on a fear of a solution you haven't tried? |
We have tried it for many years in the TFS world and I worked extremely hard to eliminate maintaining this file list as part of our move to the open. Sorry but this is something I feel strongly about not having. |
Then the implementation is up to you. This is a case where CoreFx is going to deviate from the rest of the organization, without every trying the solution at all. Work and maintenance is on you. Got to say though this is very disappointing to see. The point of arcade is to unify our solution, not deviate. Kicking off the effort by deviating before trying is not a good start. |
Why is it that you guys don't want to try the approach that both core and aspnet are already using? |
It doesn't meet the needs of our repositories:
|
To update this thread, @jaredpar, @weshaggard, and I have agreed on the following requirements. @maririos - please be sure and add these to the overall signtool requirement doc which is being created.
|
Jumping a little bit late to the discussion but hopefully not too late :). @weshaggard If I understand your concern correctly it's the build breaks that you had to fix that bother you. Is that the only issue you have with the existing implementation? If so I'd like to emphasize Jared's point on local verification and verification in Jenkins. The CI (PR and official) builds can't get broken because Jenkins will validate that all assets are signed that need signing. The output of a repo is a set of packages. That's what's being published. The sign tool validates that all binaries in these packages are listed in the .json file and thus will be signed in official build. That said, I wonder if we could do the following: Would that work for everyone? |
I like the idea of signing validation during both CI (currently don't do in corefx) and Official Builds (we do this) but I don't believe that requires a checked in file list it just requires the list of what we consider shipping bits, as you put it. That list is easily gathered from our builds today based on the project metadata. From there we can verify that all the files in our containers (msi's, vsix, nupkgs, etc) are in the list of shipping bits to be signed. We are planning to convert (or at least create a new) version of the signtool that is an msbuild task and ideally everyone would start using that version of the tool and the only pivot is how repo's decide they wish to pass in the list of files to it. That list can be a static json file as it is today (or perhaps a static file list in an msbuild props file), or it can generated as part of the build and passed to the tool. For the core repo's the plan will be for any shipping library we will produce a ".requires_signing" file next to the binary in the output which will contain it's strong name and cert ids. Then when we hit the signing phase we will gather all those require_signing files and produce the list of things we need to pass to the signing service. For validation we will pass that list as well as a list of all our containers (mostly nupkgs) to the signtool task so it can validate that all assets we are going to ship are planned to be signed for CI, or are actually signed in the case of official builds. |
Why are If each project that produces a shipping package outputs that package to an artifacts directory dedicated for shipping packages, while non-shipping packages are placed to a different directory, then the requirement is that all artifacts (the packages as well as the binaries contained in those packages) in the shipping directory must be signed. The same logic can be applied to symbol publishing. All symbols contained in packages in the shipping directory must be published. In addition, we would use the set of shipping packages found in the shipping directory for nuget repacking. NuGet repacking changes the per-build pre-release version of a NuGet package to a release version, which is readily available for publishing if we decide we need to publish the build. Roslyn (and other repos) use repacking since we don't know upfront which build is gonna be the final build. So we produce final packages for every build with extra information that tells us whether these packages can be published (i.e. whether they do not have pre-release dependencies). |
@tmat - I like the fact that you had a good suggestion as to how further unification might be accomplished. However, at the end of the day, I think this is an area where we can leave the decision up to the repo owner regarding if it's explicit or implicit. The point of convergence is that the regardless the path - there's still a "manifest" of what should have got signed. Is there something specific that you think blocks moving forward with the following?
|
I don't think we disagree on the above bullet points. I'm just saying that if we structure the package output directory based on whether package is shipping or not we won't need the explicit manifest, nor we would need extra ".requires_signing" files. At that point I don't see why we wouldn't use implicit one in all repos. I'm currently working on incorporating the package shipping/non-shipping classification into RepoToolset, since it's needed for NuGet and Symbol publishing. The publishing logic needs to know which packages are shipping and which are not. The easiest way to do that, imo, is to separate them into directories during the build and let each project that produces a package decide to which directory to put the package (based on |
I see @tmat. So you're saying that as part of the publishing work you're doing in repotoolset could potentially be used to determine what needs to be signed? |
Exactly. |
That is exactly what do today for our shipping packages and how we gather them to publish and validate. However one key missing piece of data is figuring out what strong name and certificate to use for a given binary. The certificate is generally the same but the strong name varies. If we can figure out a way to get all the necessary information (perhaps we could crack open the files and read the public key token to figure out the strong name) then I'm all for eliminating the requires_signing marker files as well. I think we still have to work out some of the details about unpacking/signing/repacking so I'm not sure if that approach will fully work but it is worth exploring. One thing we need to be careful with is that some binaries ship in multiple containers but today are only signed once if we move to a model of unpack/sign/repack then we may end up signing the same binary multiple times. That may not be that common and isn't the end of the world but something to keep in mind. |
I think that's a good idea. Let's try to read the strong name from the metadata.
Absolutely. I believe the signtool has this built-in already. It calculates checksum of each artifacts to see if it matches the file that was listed in the .json. |
Indeed this is the case. The tool itself primarily operates on checksums of artifacts for identity. The only time names are used is when printing out error messages to developers. |
Changing to always generate manifest during build to reflect the decision that happened recently in #58
* Update signtool plan Changing to always generate manifest during build to reflect the decision that happened recently in #58 * change the wording
Shorten error messages associated with missing resources
What is the strategy for maintaining the SignToolData.json (https://github.com/dotnet/arcade/blob/master/build/SignToolData.json) file? I'm not a fan of manually maintaining a list of files that we need to sign. People always forget to update it and we have build failures because some binary is unsigned. In the core repo's we have moved to a model of generating and dropping files next to the binaries (foo.dll.requires_signing) based on a property, as part of the build process and then gather the list of files to sign from those. It has worked pretty well over the last few years of doing it.
The text was updated successfully, but these errors were encountered: