Fix syncing cosign signature tags when using remote include_tags field#2328
Conversation
gerrod3
force-pushed
the
sig-tag-sync-fix
branch
2 times, most recently
from
17b7227 to
9f0c946
Compare
gerrod3
force-pushed
the
sig-tag-sync-fix
branch
4 times, most recently
from
2c2c13e to
a1dde1b
Compare
| self._full_tag_list, self.remote.include_tags, exclude_tags_and_cosign | ||
| ) | ||
| else: | ||
| tag_list = self._full_tag_list |
There was a problem hiding this comment.
Could you comment this inline a bit better?
e.g. if the goal was to fix the cosign tags being silently skipped, then why are the cosign tags being added to the exclusion list here?
There was a problem hiding this comment.
Is the point just to make a separate call to self._process_tags() w/ a separate log message?
There was a problem hiding this comment.
Yep, basically if we are using includes/excludes then I want to process all non cosign tags first, figure out every manifest that we synced, then go through and sync every cosign tag that matches a synced manifest. This is to ensure we only sync the cosign tags of the filtered tags the user requested. The process needs to be done in two steps to make sure we sync everything needed, but nothing extra.
If no includes/excludes then we can grab everything in one go, no need to differentiate between the two.
| companion_tags, signature_source, msg="Processing Cosign Companion Tags" | ||
| ) | ||
|
|
||
| def _find_cosign_companion_tags(self): |
There was a problem hiding this comment.
Likewise some comments about the structure of the tags and the transformations going on here would be nice.
| return True | ||
| return False | ||
|
|
||
| def _is_cosign_companion_tag(self, tag_name, media_type, content_data): |
There was a problem hiding this comment.
Could be unit tested? self isn't actually required.
There was a problem hiding this comment.
Unit testing _has_cosign_signature and _find_cosign_companion_tags is likely possible also, just more difficult.
But in any case you can skip the unit tests if you think they aren't needed.
| def _is_cosign_companion_tag(self, tag_name, media_type, content_data): | ||
| """Check if a fetched tag is a cosign companion tag.""" | ||
| if tag_name.startswith("sha256-"): | ||
| if len(tag_name) == 71: |
There was a problem hiding this comment.
What is the significance of "71", specifically?
There was a problem hiding this comment.
(I know what it is from context elsewhere in the PR, but it's not obvious locally)
There was a problem hiding this comment.
Do you want me to change it to len("sha256-") + 64? Re:significance - v3 tags are just sha256- no suffix.
|
|
||
| tags = container_tag_api.list(repository_version=synced_repo.latest_version_href) | ||
| manifests = container_manifest_api.list(repository_version=synced_repo.latest_version_href) | ||
| assert tags.count == 3 # manifest_a, sha256-<a-digest>.sig, sha256-<a-digest>.att |
There was a problem hiding this comment.
Is there any way to check the actual names and not just the count?
fixes: pulp#2096 Assisted by: claude-opus-4.6
| self.stage._cosign_tags = [f"{cosign_key}.sig"] | ||
|
|
||
| self.assertTrue(await self.stage._has_cosign_signature(digest)) | ||
| self.stage.remote.get_downloader.assert_not_called() |
There was a problem hiding this comment.
Is this actually valuable / relevant?
There was a problem hiding this comment.
For v2 tags we can just check the naming scheme of the tag since they have special suffixes. For v3 tags we need to check the manifest of the tag to make sure its internals are for a v3 tag. In theory we could check the internals of v2 to be safe or skip the internal check of v3 if the name is sha256-<digest>, but that's what this check is testing. V2 is just a name check, v3 is a manifest check and thus requires a request.
Backport to 2.26: 💚 backport PR created✅ Backport PR branch: Backported as #2361 🤖 @patchback |
Backport to 2.27: 💚 backport PR created✅ Backport PR branch: Backported as #2360 🤖 @patchback |
2 participants
fixes: #2096
Assisted by: claude-opus-4.6
Need to work on tests for this, going to update our local GH packages to include some cosign attachments.
📜 Checklist
See: Pull Request Walkthrough