Implement workflow to push container security manifests #184

querti · 2023-08-31T10:57:13Z

The workflow can be described as follows: Get security manifest for each architecture of a given push item. Then check if an attestation of this archtecture already exists in the destination. If it does, determine if a new product name will be added to the properties. If it does, overwrite the attestation with the new product name included. If it doesn't, skip uploading the attestation, as no new information will be added. If an attestation doesn't exist in the destination, just upload the security manifest.

The workflow gets the product name from the push item, which gets it from Errata via pushsource. PushDocker method doesn't have an associated advisory, so there is not enough information to get the associated product. The workflow supports not adding product name to the security manifest in such cases.

Only multiarch push items and source push items are supported. Other image types (for example v2s1) are assumed to not contain a security manifest and are skipped. In fact, many source and multiarch images don't contain security manifests either, which is why no error is raised and the push continues without uploading them.

Cosign has several limitations:

It can only generate attestation images to images that are already pushed to the destination. This means that if the push fails after an image is pushed, but before the attestation image is uploaded, an image will exist without a security manifest. This was deemed acceptable since the affected tags won't be visible in the catalog because Metaxor only processes sucessful pub tasks.
It can only use the default docker auth config file location to get credentials. Pub currently has two config files. The file in the default location (/.docker/config.json) contains pre-filled data and is immutable because it's mounted from a Kubernetes resource. The second config file is only used by skopeo and its location is specified with the REGISTRY_AUTH_FILE env variable. Skopeo login writes the acquired credentials to this file at runtime. The issue for cosign is that the destination credentials (needed to get and push attestations) are target-specific, and thus can only be set at runtime. It is resolved by making the default config file writable (mounting the Kubernetes resource to a different file and copying the contents to /.docker/config.json at startup) and by invoking skopeo login at the start of the security manifest pushing stage.
It cannot easily overwrite existing attestations. When attempting to push a security manifest when one was already pushed, the resulting attestation will containg two security manifests, which is obviously incorrect. If attempting to use the --replace flag, the command fails. To resolve it, the existing attestation is maually removed before the new one is pushed. This may pose issues if a push fails between removing the old and pushing the new attestations, since a published image may now not have a security manifest. In the following section I will explain why I consider this acceptable.

It is very unlikely that a container image can be associated with multiple products. This means that the workflow where an existing attestation image is removed and merged will almost never occur. Instead, attestation images will simply be copied to the destination.

Refers to CLOUDDST-19704

codecov · 2023-08-31T11:03:12Z

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (8a585ea) 100.00% compared to head (2d4f3f4) 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##            master      #184    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           28        29     +1     
  Lines         3110      3299   +189     
==========================================
+ Hits          3110      3299   +189

Files Changed	Coverage Δ
pubtools/_quay/push_docker.py	`100.00% <100.00%> (ø)`
pubtools/_quay/security_manifest_pusher.py	`100.00% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pubtools/_quay/security_manifest_pusher.py

midnightercz · 2023-09-04T14:07:54Z

pubtools/_quay/security_manifest_pusher.py

+            "--key",
+            self.cosign_public_key_path,
+            image_ref,
+            "--output-file",


I wonder if there's a reason why stdout cannot be used instead of file

I think it can, but should it?

I personally prefer solutions which doesn't produce mess. For example: cosign_get_existing_attestation takes output file as attribute it's called in merge_and_push_security_manifest but I don't see any cleanup there.

The entire workflow happens inside a TemporaryDirectory (see method push_item_security_manifests). The directory along with its contents is deleted once the code is outside of its context.

pubtools/_quay/security_manifest_pusher.py

midnightercz · 2023-09-04T14:36:07Z

pubtools/_quay/security_manifest_pusher.py

+            is_multiach_image = True
+
+        with tempfile.TemporaryDirectory(prefix="security_manifest_") as tmp_dir:
+            if is_source_image:


ok, I don't understand why there's extra handling for the source containers. Shouldn't there be just
if multi_arch:
...
else:
...

and then skip if digest manifest is empty

This logic is analogous to ContainerImagePusher's push_container_images method. To my knowledge, security manifests can only be present in multiarch and (possibly) V2S2 source images. Could there be other image types?

I think it's dangerous to write code where we don't even understand what type of data we might receive. So I think it's better to explicitly declare what type of images we support.

I think security manifest can be present in multiarch or v2sch2 regardless of if it's source container or not. Note that only difference between copy_source_push_item and copy_v1_push_item is log message.

To my knowledge, new push items can only be multiarch or source images. And old items don't contain security manifests because they only started to be generated recently. I can't think of a scenario where other image types may contain security manifests. If new image types are added, I imagine this change will have to be reflected in our implementation as well. Do you disagree with this approach?

My point is, there's no extra logic needed to handle source containers compared to other v2sch2 containers, so there's no reason to restrict the code to push only source containers.

In my opinion, we should have an understanding what data we'll be working with and our code should reflect our expectations of that data. If we write our code in a way that "sort of maybe" supports other types of data, I think we're setting ourselves up for failure. Besides, I've written the code to be easily extensible if other push item types need to be supported.

If you still insist that this method should support "other" v2s2 push items then I will change it, but I still don't think it's a good idea.

Thoughts?

My point is that there's basically no difference between source containers and regular containers when we compare them in format. Source containers include just different content but there's no extra handling needed for them. Then imagine build team decide to ship secure manifests for v2ch2 and they will obviously do that without asking anyone because it doesn't make sense to be able to process v2ch2 source containers and forbid processing of regular v2ch2. You're creating useless limitation

I don't understand your point. Build team doesn't ship v2s2 containers at all so how can they decide to ship secure manifests for them?

If you check ContainerImagePusher, the code doesn't expect v2s2 to be shipped at all. Due to poorly written condition, the method "copy_v1_push_item" will be called if non-source v2s2 is encountered, which is obviously wrong. The rest of the code base doesn't support normal v2s2, so I argue that security_manifest_pusher shouldn't either.

emilyzheng · 2023-09-06T03:57:47Z

The pre-filled data in /.docker/config.json were used by pub-docker-daemon (docker-pulp scenarios) and are not read any more. I think we can stop mounting it and setting REGISTRY_AUTH_FILE, just make the default config file ($HOME/.docker/config.json) writable.

querti · 2023-09-06T06:54:41Z

The pre-filled data in /.docker/config.json were used by pub-docker-daemon (docker-pulp scenarios) and are not read any more. I think we can stop mounting it and setting REGISTRY_AUTH_FILE, just make the default config file ($HOME/.docker/config.json) writable.

Should we create a new story for this?

emilyzheng · 2023-09-06T07:59:33Z

The pre-filled data in /.docker/config.json were used by pub-docker-daemon (docker-pulp scenarios) and are not read any more. I think we can stop mounting it and setting REGISTRY_AUTH_FILE, just make the default config file ($HOME/.docker/config.json) writable.

Should we create a new story for this?

Yeah, it's better to clean pre-filled data in another task. But for the change in pubtools/_quay/command_executor.py, is it made because REGISTRY_AUTH_FILE is set to a non-default path? Since the default config file is made writable in playbook, REGISTRY_AUTH_FILE doesn't need to be set and command_executor.py can be kept what it is.

querti · 2023-09-06T10:24:20Z

The pre-filled data in /.docker/config.json were used by pub-docker-daemon (docker-pulp scenarios) and are not read any more. I think we can stop mounting it and setting REGISTRY_AUTH_FILE, just make the default config file ($HOME/.docker/config.json) writable.

Should we create a new story for this?

Yeah, it's better to clean pre-filled data in another task. But for the change in pubtools/_quay/command_executor.py, is it made because REGISTRY_AUTH_FILE is set to a non-default path? Since the default config file is made writable in playbook, REGISTRY_AUTH_FILE doesn't need to be set and command_executor.py can be kept what it is.

I wonder if we also have to remove the default config for this to work. Won't the skopeo login be skipped if there are already quay credentials in the config.json? In the playbooks, the config.json currently contains credentials for rh-osbs.

If so, should I include this change in this story, or should we create a new story to consolidate all logins into one file?

emilyzheng · 2023-09-06T10:42:55Z

I wonder if we also have to remove the default config for this to work. Won't the skopeo login be skipped if there are already quay credentials in the config.json? In the playbooks, the config.json currently contains credentials for rh-osbs.

It's skipped only when username is in output of get login.

querti · 2023-09-06T11:55:05Z

I wonder if we also have to remove the default config for this to work. Won't the skopeo login be skipped if there are already quay credentials in the config.json? In the playbooks, the config.json currently contains credentials for rh-osbs.

It's skipped only when username is in output of get login.

I have removed the authfile from this PR, and changed the playbooks MR to not set REGISTRY_AUTH_FILE.

pubtools/_quay/security_manifest_pusher.py

midnightercz · 2023-09-11T10:44:13Z

pubtools/_quay/security_manifest_pusher.py

+            is_multiach_image = True
+
+        with tempfile.TemporaryDirectory(prefix="security_manifest_") as tmp_dir:
+            if is_source_image:


I think security manifest can be present in multiarch or v2sch2 regardless of if it's source container or not. Note that only difference between copy_source_push_item and copy_v1_push_item is log message.

The workflow can be described as follows: Get security manifest for each architecture of a given push item. Then check if an attestation of this archtecture already exists in the destination. If it does, determine if a new product name will be added to the properties. If it does, overwrite the attestation with the new product name included. If it doesn't, skip uploading the attestation, as no new information will be added. If an attestation doesn't exist in the destination, just upload the security manifest. The workflow gets the product name from the push item, which gets it from Errata via pushsource. PushDocker method doesn't have an associated advisory, so there is not enough information to get the associated product. The workflow supports not adding product name to the security manifest in such cases. Only multiarch push items and source push items are supported. Other image types (for example v2s1) are assumed to not contain a security manifest and are skipped. In fact, many source and multiarch images don't contain security manifests either, which is why no error is raised and the push continues without uploading them. Cosign has several limitations: - It can only generate attestation images to images that are already pushed to the destination. This means that if the push fails after an image is pushed, but before the attestation image is uploaded, an image will exist without a security manifest. This was deemed acceptable since the affected tags won't be visible in the catalog because Metaxor only processes sucessful pub tasks. - It can only use the default docker auth config file location to get credentials. Pub currently has two config files. The file in the default location (/.docker/config.json) contains pre-filled data and is immutable because it's mounted from a Kubernetes resource. The second config file is only used by skopeo and its location is specified with the REGISTRY_AUTH_FILE env variable. Skopeo login writes the acquired credentials to this file at runtime. The issue for cosign is that the destination credentials (needed to get and push attestations) are target-specific, and thus can only be set at runtime. It is resolved by making the default config file writable (mounting the Kubernetes resource to a different file and copying the contents to /.docker/config.json at startup) and by invoking skopeo login at the start of the security manifest pushing stage. - It cannot easily overwrite existing attestations. When attempting to push a security manifest when one was already pushed, the resulting attestation will containg two security manifests, which is obviously incorrect. If attempting to use the --replace flag, the command fails. To resolve it, the existing attestation is maually removed before the new one is pushed. This may pose issues if a push fails between removing the old and pushing the new attestations, since a published image may now not have a security manifest. In the following section I will explain why I consider this acceptable. It is very unlikely that a container image can be associated with multiple products. This means that the workflow where an existing attestation image is removed and merged will almost never occur. Instead, attestation images will simply be copied to the destination.

querti force-pushed the push-security-manifests branch from da78732 to 2b47753 Compare August 31, 2023 10:58

querti force-pushed the push-security-manifests branch 2 times, most recently from 3ff8d25 to bf1ac8b Compare August 31, 2023 11:17

querti marked this pull request as ready for review September 1, 2023 09:28

querti requested a review from midnightercz as a code owner September 1, 2023 09:28

querti requested review from zxiong, emilyzheng and lrossi-dev September 1, 2023 09:29

midnightercz requested changes Sep 4, 2023

View reviewed changes

querti force-pushed the push-security-manifests branch from bf1ac8b to 39ca958 Compare September 6, 2023 10:58

midnightercz requested changes Sep 11, 2023

View reviewed changes

querti force-pushed the push-security-manifests branch 5 times, most recently from 219e6d3 to 8ddfbf6 Compare September 12, 2023 07:14

querti force-pushed the push-security-manifests branch from 8ddfbf6 to c102628 Compare September 25, 2023 07:33

querti force-pushed the push-security-manifests branch from c102628 to 2d4f3f4 Compare September 25, 2023 07:36

emilyzheng approved these changes Sep 25, 2023

View reviewed changes

midnightercz self-requested a review September 25, 2023 14:09

midnightercz approved these changes Sep 25, 2023

View reviewed changes

querti merged commit 917672f into release-engineering:master Sep 26, 2023
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement workflow to push container security manifests #184

Implement workflow to push container security manifests #184

querti commented Aug 31, 2023

codecov bot commented Aug 31, 2023 •

edited

Loading

midnightercz Sep 4, 2023

querti Sep 5, 2023

midnightercz Sep 5, 2023

querti Sep 5, 2023

midnightercz Sep 4, 2023

querti Sep 5, 2023

midnightercz Sep 11, 2023

querti Sep 11, 2023

midnightercz Sep 12, 2023

querti Sep 12, 2023

midnightercz Sep 12, 2023

querti Sep 25, 2023

emilyzheng commented Sep 6, 2023

querti commented Sep 6, 2023

emilyzheng commented Sep 6, 2023

querti commented Sep 6, 2023

emilyzheng commented Sep 6, 2023

querti commented Sep 6, 2023

midnightercz Sep 11, 2023

Implement workflow to push container security manifests #184

Implement workflow to push container security manifests #184

Conversation

querti commented Aug 31, 2023

codecov bot commented Aug 31, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilyzheng commented Sep 6, 2023

querti commented Sep 6, 2023

emilyzheng commented Sep 6, 2023

querti commented Sep 6, 2023

emilyzheng commented Sep 6, 2023

querti commented Sep 6, 2023

Choose a reason for hiding this comment

codecov bot commented Aug 31, 2023 •

edited

Loading