Skip to content

ROX-29160: Add built-in signature integration for Red Hat signature #15761

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

Draft
wants to merge 6 commits into
base: ROX-17872-Remove-remaining-reference-to-feature-flag
Choose a base branch
from

Conversation

guzalv
Copy link
Contributor

@guzalv guzalv commented Jun 17, 2025

Description

change me!

User-facing documentation

Testing and quality

  • the change is production ready: the change is GA, or otherwise the functionality is gated by a feature flag
  • CI results are inspected

Automated testing

  • added unit tests
  • added e2e tests
  • added regression tests
  • added compatibility tests
  • modified existing tests

How I validated my change

change me!

Copy link

openshift-ci bot commented Jun 17, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@guzalv guzalv force-pushed the ROX-29160-Add-default-signature-integration-rh-rk3 branch from be4a6ad to 23c13a6 Compare June 17, 2025 12:46
@rhacs-bot
Copy link
Contributor

rhacs-bot commented Jun 17, 2025

Images are ready for the commit at e87a45d.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.9.x-98-ge87a45db91.

Copy link

codecov bot commented Jun 17, 2025

Codecov Report

Attention: Patch coverage is 82.50000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 48.81%. Comparing base (c61921b) to head (e87a45d).

Files with missing lines Patch % Lines
...entral/signatureintegration/datastore/singleton.go 53.33% 5 Missing and 2 partials ⚠️
Additional details and impacted files
@@                                  Coverage Diff                                  @@
##           ROX-17872-Remove-remaining-reference-to-feature-flag   #15761   +/-   ##
=====================================================================================
  Coverage                                                 48.81%   48.81%           
=====================================================================================
  Files                                                      2590     2590           
  Lines                                                    190591   190629   +38     
=====================================================================================
+ Hits                                                      93029    93059   +30     
- Misses                                                    90252    90258    +6     
- Partials                                                   7310     7312    +2     
Flag Coverage Δ
go-unit-tests 48.81% <82.50%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@guzalv guzalv force-pushed the ROX-29160-Add-default-signature-integration-rh-rk3 branch 2 times, most recently from c2988af to 306bfc7 Compare June 17, 2025 14:01
@guzalv
Copy link
Contributor Author

guzalv commented Jun 17, 2025

@guzalv guzalv changed the title ROX-29160: Add default signature integration for Red Hat Release Key 3 ROX-29160: Add built-in signature integration for Red Hat Release Key 3 Jun 17, 2025
@guzalv guzalv changed the title ROX-29160: Add built-in signature integration for Red Hat Release Key 3 ROX-29160: Add built-in signature integration for Red Hat signature Jun 17, 2025
@guzalv guzalv force-pushed the ROX-29160-Add-default-signature-integration-rh-rk3 branch 3 times, most recently from e4e0ad1 to 77ffe76 Compare June 19, 2025 11:10
@guzalv guzalv changed the base branch from master to ROX-29160-Add-feature-flag-for-built-in-policy June 19, 2025 12:01
@guzalv guzalv force-pushed the ROX-29160-Add-default-signature-integration-rh-rk3 branch from 77ffe76 to 9ef9557 Compare June 19, 2025 12:01
@guzalv guzalv force-pushed the ROX-29160-Add-feature-flag-for-built-in-policy branch from afc4353 to 2a3f10b Compare June 19, 2025 14:05
@guzalv guzalv force-pushed the ROX-29160-Add-default-signature-integration-rh-rk3 branch from 9ef9557 to b077dd9 Compare June 19, 2025 14:05
@guzalv guzalv force-pushed the ROX-29160-Add-feature-flag-for-built-in-policy branch from 2a3f10b to adab0fb Compare June 19, 2025 14:18
@guzalv guzalv force-pushed the ROX-29160-Add-default-signature-integration-rh-rk3 branch from b077dd9 to 7b8fa74 Compare June 19, 2025 14:18
@stehessel
Copy link
Collaborator

stehessel commented Jun 19, 2025

Not a blocker, but one side effect of a default signature integration that I want to call out: Central currently has an optimization to skip image signature downloads if no signature integration has been added. If we add now a signature by default, this optimization will no longer work as is, and signatures will always be loaded. In the past some users have reported this as an issue because their image registries are self-hosted and are sensitive to additional load.

Some ideas come to my mind to address this issue:

  • Rework the optimization such that image signature fetch is still skipped if the image is not a Red Hat image (and no other signature integrations exist).
  • Remove the optimization and point customers towards disabling image signatures via feature flags if they are very sensitive to registry load. May also simplify the scan flow in general, as with the "skip optimization" users may wonder why no signatures are loaded in some cases (namely when there are no integrations).

As a side note, this optimization only works in Central - when using delegated scanning, image signature are already always fetched.

@guzalv guzalv force-pushed the ROX-29160-Add-feature-flag-for-built-in-policy branch from d7c4ff2 to e394187 Compare June 20, 2025 10:52
@guzalv guzalv changed the base branch from ROX-29160-Add-feature-flag-for-built-in-policy to ROX-17872-Remove-remaining-reference-to-feature-flag June 20, 2025 10:52
@guzalv guzalv force-pushed the ROX-29160-Add-default-signature-integration-rh-rk3 branch from 7b8fa74 to 6e209ef Compare June 20, 2025 10:52
@guzalv
Copy link
Contributor Author

guzalv commented Jun 20, 2025

Not a blocker, but one side effect of a default signature integration that I want to call out: Central currently has an optimization to skip image signature downloads if no signature integration has been added. If we add now a signature by default, this optimization will no longer work as is, and signatures will always be loaded. In the past some users have reported this as an issue because their image registries are self-hosted and are sensitive to additional load.

Some ideas come to my mind to address this issue:

  • Rework the optimization such that image signature fetch is still skipped if the image is not a Red Hat image (and no other signature integrations exist).
  • Remove the optimization and point customers towards disabling image signatures via feature flags if they are very sensitive to registry load. May also simplify the scan flow in general, as with the "skip optimization" users may wonder why no signatures are loaded in some cases (namely when there are no integrations).

As a side note, this optimization only works in Central - when using delegated scanning, image signature are already always fetched.

Thanks! I could have introduced a performance penalty for some customers without knowing.

I'm not a big fan of adding tangling between different areas of the product, special cases etc, which is what reworking the optimization implies. On the other I can imagine that customers who rely on the optimization would not be happy to see that it's been removed and now they need to use feature flags to prevent their registries from getting overloaded. So the tangling sounds better than this.

Are there any other alternatives here? How about reworking the optimization to only fetch signatures of images hosted at registries for which a signature integration exists? Does that sound too difficult/cumbersome?

@stehessel
Copy link
Collaborator

Are there any other alternatives here? How about reworking the optimization to only fetch signatures of images hosted at registries for which a signature integration exists? Does that sound too difficult/cumbersome?

The problem is that signature integrations do not depend on the registry. For example, I may create a signature integration for my public key - but I can use this public key to sign images of any registry. The only exception - more or less - are the Red Hat registries, where we know that all images are supposed to be signed by the golden key, and nobody else have write permissions to create any other signatures. So we could say something like "if only the Red Hat signature integration exists, then only download signatures for Red Hat images".

@guzalv
Copy link
Contributor Author

guzalv commented Jun 20, 2025

Are there any other alternatives here? How about reworking the optimization to only fetch signatures of images hosted at registries for which a signature integration exists? Does that sound too difficult/cumbersome?

The problem is that signature integrations do not depend on the registry. For example, I may create a signature integration for my public key - but I can use this public key to sign images of any registry. The only exception - more or less - are the Red Hat registries, where we know that all images are supposed to be signed by the golden key, and nobody else have write permissions to create any other signatures. So we could say something like "if only the Red Hat signature integration exists, then only download signatures for Red Hat images".

Yeah right, I was mixing up signatures and policies, I meant "only fetch sigs of images hosted at registries for which a signature-verifying policy exists".

I imagine that this is probably a no-go, because we use the signature integration to verify signatures and at least display that info in the UI even if there's no policy involving signatures.

I will give a shot at your proposal, which seems to me like our best bet: "if only the Red Hat signature integration exists, then only download signatures for Red Hat images

@guzalv guzalv force-pushed the ROX-29160-Add-default-signature-integration-rh-rk3 branch from 6e209ef to f271347 Compare June 20, 2025 13:03
@guzalv guzalv force-pushed the ROX-17872-Remove-remaining-reference-to-feature-flag branch from a16cd1a to c61921b Compare June 20, 2025 13:47
@guzalv guzalv force-pushed the ROX-29160-Add-default-signature-integration-rh-rk3 branch 2 times, most recently from cdc9ee6 to d2dbde2 Compare June 23, 2025 09:35
guzalv added 3 commits June 23, 2025 12:01
Just for getting something that works in place
Based on registry and remote heuristics.

This will be used in upcoming changes to keep the optimization at [1]
working even when a signature is present by default:

If only 1 signature integration exists, and that is the built-in red hat
signature integration, and the image is a red hat image, download the
signature. Otherwise skip.

[1] https://github.com/stackrox/stackrox/blob/e3180cf49b7e79c31640af08ea571edfc7ffdd28/pkg/images/enricher/enricher_impl.go#L712
@guzalv guzalv force-pushed the ROX-29160-Add-default-signature-integration-rh-rk3 branch from d2dbde2 to e881962 Compare June 23, 2025 10:02
guzalv added 3 commits June 23, 2025 12:52
This optimization skips signature download when no signature
integrations are present.

Adding a built-in integration would effectively disable the
optimization, and we want to avoid that.

This commit reworks the optimziation so that when only the built in
signature integration is present only signatures for Red Hat images are
downloaded - the rest will still be skipped.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants