[feature] Improve package signing plugin integration: new commands, tools and output format #18785
Conversation
| cli_out_write(f"[Package signing plugin] {title} results:", fg=Color.BRIGHT_BLUE) | ||
| for ref, result in elements.items(): | ||
| cli_out_write(f" {ref}", fg=Color.BRIGHT_BLUE) | ||
| if result is None: |
There was a problem hiding this comment.
Assuming here that if the pkg signing plugin of the user only uses return, this means that it was correctly signed or verified
| if sign_tools.is_pkg_signed(): | ||
| summary = sign_tools.load_summary() | ||
| if summary.get("provider") != "conan-client": | ||
| return "Warn: Package already signed by another provider" |
There was a problem hiding this comment.
This could be a warn or an error depending on the context or maybe there is some functionality still missing like conan cache sign --force to resign packages (user's plugin implementation). Also a conan cache sign --clear could be useful to "unsign" packages. WDYT?
| file_path = os.path.join(self._artifacts_folder, fname) | ||
| if os.path.isfile(file_path): | ||
| sha256 = sha256sum(file_path) | ||
| checksums[fname] = sha256 | ||
| sorted_checksums = dict(sorted(checksums.items())) | ||
| content = copy.deepcopy(self.SIGN_SUMMARY_CONTENT) |
There was a problem hiding this comment.
This is doing a checksum of all artifacts in the package?
What exactly for?
There was a problem hiding this comment.
We agreed that this should be the way the plugin should work. Get the cheksums of all the contetns of the package, create a summary file with filenames and checkesums and use that file to sign the package
There was a problem hiding this comment.
But maybe, this is the kind of thing that shouldn't belong then to user space? This is something that Conan should kind of enforce in the plugin? Or do we still envision other possible signing approaches that do not sign this file?
| metadata_sign = os.path.join(folder, METADATA, "sign") | ||
| mkdir(metadata_sign) | ||
| self._plugin_sign_function(ref, artifacts_folder=folder, signature_folder=metadata_sign) | ||
| sign_tools = PkgSignaturesTools(folder, metadata_sign) |
There was a problem hiding this comment.
Why instantiating here PkgSignaturesTools with those arguments, then passing it to self._plugin_sign_function() as an argument if the function can create the tools inside?
I am not convinced by this API, passing an object with data this way is not that common, in general Conan defines helpers/tools that users can instantiate and use them themselves, or provide an input-output interface, in which Conan would automatically manage the resource, in this case sign-summary.json. If this file must exist for any signing plugin, then why not mandating the return interface of sign() to return the necessary data so Conan can enforce the correct generation of sign-summary.json?
| cache = PkgCache(self._conan_api.cache_folder, self._api_helpers.global_conf) | ||
| pkg_signer = PkgSignaturesPlugin(cache, self._conan_api.home_folder) | ||
| results = pkg_signer.verify_pkglist(package_list, context="cache") | ||
| return {"results": results, "context": "cache", "action": "verify"} |
There was a problem hiding this comment.
Why returning the "action"? It is clear that you called the verify() method, in the same way you are not returning the package_list as it is an input, both the context and action seem redundant, they are already define by the action of calling verify(pkg_list).
There was a problem hiding this comment.
I might sound redundant, but I think that having the information saved as a "log" might be useful if the json report is saved for later consumption or just for auditing
|
@danimtb what about something like this for the output? I think it may be more readable? |
Co-authored-by: Carlos Zoido <[email protected]>
4 participants
Changelog: Feature: Improve package signing plugin integration.
Changelog: Feature: Add
conan cache signandconan cache verifycommands for package signing.Changelog Feature: Standardize package summary format with files, checksums, signing method and signing provider.
Docs: missing