[issue-405] Add external built image integrity validation #535

treblereel · 2024-09-20T03:40:50Z

Fixes #405

ps: tests will be added in the next commit to this pr

treblereel · 2024-09-20T04:04:35Z

sonataflow_image.yaml.zip

this yaml can be used to test the PR. It contained image declaration with sonataflow project, published to DH

ricardozanini

It seems pretty nice! Just a few minor comments.

internal/controller/validation/Validator.go

internal/controller/validation/ImageValidator.go

ricardozanini · 2024-10-01T18:06:09Z

The e2e tests have been fixed, please rebase your branch 🙏

treblereel · 2024-10-01T18:07:48Z

The e2e tests have been fixed, please rebase your branch 🙏
Thanks

Unfortunately, I have to fix test/testdata/sonataflow.org_v1alpha08_sonataflow-simpleops.yaml first, because of image: quay.io/kiegroup/sonataflow-minimal-example:latest

ricardozanini · 2024-10-01T18:15:06Z

@treblereel I'm currently working to fix this one.

treblereel · 2024-10-01T18:20:23Z

sonataflow-minimal-example

The problem here is sonataflow-minimal-example has no workflow.sw.json in it, so I have to build and publish it to temp repo before tests ...

treblereel · 2024-10-07T07:14:32Z

Currently, to verify that the image corresponds to the deployment, it is checked that /deployments/app/workflow.sw.json in the image matches the workflow in the YAML.

Comments:

This approach aligns with how kn-workflow-plugin generates new projects.
If this approach is acceptable, kn-workflow-plugin needs to be updated (update Dockerfile.*).
Other naming options are possible, such as metadata.name.
It's breaking change, if it is merged, all deployments with image declaration will fail

An additional step has been added that replaces kind-registry in spec.podTemplate.container.image with the IP address of kind-registry in *operatorapi.SonataFlow during the deployment phase, because the container builder cannot resolve the image URI by hostname in it's kind-registry. This issue does not occur with an external Docker registry.

.github/workflows/e2e.yml

internal/controller/validation/image_url_sanitizer.go

internal/controller/validation/image_validator.go

test/testdata/sonataflow.org_v1alpha08_sonataflow-simpleops.yaml

ricardozanini

Also please remove the container-builder/bin folder. Not sure why they are there. 🤔

internal/controller/validation/image_validator.go

internal/controller/validation/common.go

ricardozanini · 2024-10-09T16:27:54Z

@treblereel I think we are closer to merging this PR. Can you please also provide the docs on incubator-kie-kogito-docs? I believe this feature deserves a new page under Cloud.

@wmedvede @jakubschwan , do you mind taking a look?

ricardozanini

@treblereel controller-gen is still there. Please also add an entry to .gitignore bin.

In the docs, we must add details of this verification and instruct users how to build or customize a dockerfile to build a valid image.

ricardozanini · 2024-10-11T13:53:12Z

Waiting for QE verification @jakubschwan and @wmedvede's review.

@treblereel the docs can be pushed later.

jakubschwan

Changes in PR looks good to me

treblereel · 2024-10-11T14:48:28Z

Waiting for QE verification @jakubschwan and @wmedvede's review.

@treblereel the docs can be pushed later.

apache/incubator-kie-kogito-docs#673

wmedvede · 2024-10-14T10:31:43Z

internal/controller/validation/image_validator.go

+}
+
+func validateImage(ctx context.Context, sonataflow *operatorapi.SonataFlow) (bool, error) {
+	isInKindRegistry, _, err := imageStoredInKindRegistry(ctx, sonataflow.Spec.PodTemplate.Container.Image)


Do this check mean that we are trying to cover the scenario where the operator is executing in kind?

If true, do we need an equivalent verification when installed in minikube? (considering that most of our examples executes in minikube)

@wmedvede As I can see, we use kind-registry and sonataflow.Spec.PodTemplate.Container.Image only in the tests, where data.localRegistryHosting.v1.hostFromClusterNetwork set. In case we want to use the same scenario with the minikube, it's better to add to install-minikube.sh registry and ConfigMap from the create-kind-cluster-with-registry.sh.

wmedvede · 2024-10-14T10:34:14Z

internal/controller/validation/image_validator.go

+	if isInKindRegistry {
+		ref, err = kindRegistryImage(sonataflow)
+	} else {
+		ref, err = remoteImage(sonataflow)


So here we get the remoteImage from Docker registry.

Did you guys @treblereel @jakubschwan verify that this code executes well when the image is read form Quay.io ? I think it's worth verify it works too.

@wmedvede @ricardozanini

The whole point of the isInKindRegistry check is to determine whether the image is in the kind-registry or not. As far as I understand, this only makes sense for tests and only in the case where the 'data.localRegistryHosting.v1.hostFromClusterNetwork' flag is set in the ConfigMap. Kind-registry is insecure. Overall, I don't like the idea of adding checks to the production code just to simplify testing (I'd prefer to remove this check), but the strategy for fetching an image from a secure and insecure registry is different

One option would be to add certificates (self-signed ones are not suitable) to make the kind-registry secure

This does not interfere with minikube because we don't use kind-registry in it

For testing, I use this image from Docker: docker.io/treblereel/sonataflow-workflow-demo:latest. I don't think there should be any issues with quay.io, but I will check further

ok, thanks, we must be sure that quay.io works well, and openshift installations too.

@wmedvede quay.io works
sonataflow_image_quay.yaml.zip

wmedvede · 2024-10-14T10:43:44Z

internal/controller/sonataflow_controller.go

@@ -98,6 +100,11 @@ func (r *SonataFlowReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 		return ctrl.Result{}, err
 	}

+	if err := r.Validate(ctx, workflow, req); err != nil {
+		klog.V(log.E).ErrorS(err, "Failed to validate SonataFlow")
+		return reconcile.Result{}, nil


If we have the err, I think it makes sense to return it?

@wmedvede @ricardozanini

This is quite a tricky situation: if we return the error, the deployment will be retried until the limit is reached. A common pattern to solve this problem is to return from Reconcile and log the error to the console.

internal/controller/validation/common.go

internal/controller/validation/image_validator.go

internal/controller/validation/validator.go

wmedvede · 2024-10-14T11:06:50Z

Hi @treblereel , I have left some minor comments/nitpicks.

Before merging I thing guys @jakubschwan @treblereel we must be sure this was tested in:

minikube
openshift

and also using images from docker.io and quay.io.

Many thanks!

internal/controller/validation/validator.go

internal/controller/validation/image_validator.go

internal/controller/validation/common_test.go

wmedvede · 2024-10-30T17:17:30Z

@treblereel I think this one needs resolving the merge conflicts and verification that it works in OpenShift.
Would you mind to check please @jakubschwan ?

…ty validation

treblereel requested a review from ricardozanini as a code owner September 20, 2024 03:40

treblereel force-pushed the 405 branch 2 times, most recently from 2e50511 to 4b5da8e Compare September 20, 2024 03:53

treblereel force-pushed the 405 branch 4 times, most recently from f6214ed to 6df7c95 Compare September 27, 2024 22:45

ricardozanini reviewed Sep 30, 2024

View reviewed changes

treblereel force-pushed the 405 branch 2 times, most recently from b838cd2 to 4cce57d Compare October 6, 2024 19:53

treblereel force-pushed the 405 branch 3 times, most recently from 453c24d to ac78e6b Compare October 7, 2024 17:55

ricardozanini reviewed Oct 8, 2024

View reviewed changes

ricardozanini reviewed Oct 9, 2024

View reviewed changes

internal/controller/validation/image_validator.go Show resolved Hide resolved

internal/controller/validation/common.go Outdated Show resolved Hide resolved

ricardozanini requested review from wmedvede and jakubschwan October 9, 2024 16:28

ricardozanini reviewed Oct 10, 2024

View reviewed changes

treblereel changed the title ~~kie-kogito-serverless-operator-405: Add external built image integri…~~ [issue-405] Add external built image integri… Oct 11, 2024

treblereel changed the title ~~[issue-405] Add external built image integri…~~ [issue-405] Add external built image integrity validation Oct 11, 2024

jakubschwan reviewed Oct 11, 2024

View reviewed changes

wmedvede requested changes Oct 14, 2024

View reviewed changes

internal/controller/validation/validator.go Outdated Show resolved Hide resolved

internal/controller/validation/image_validator.go Outdated Show resolved Hide resolved

internal/controller/validation/common_test.go Outdated Show resolved Hide resolved

treblereel force-pushed the 405 branch from e36bf2e to 7d21157 Compare October 15, 2024 20:35

treblereel mentioned this pull request Oct 16, 2024

Add Image Verification process and restrictions apache/incubator-kie-kogito-docs#675

Open

kie-kogito-serverless-operator-405: Add external built image integri…

f228a51

…ty validation

treblereel force-pushed the 405 branch from 7d21157 to f228a51 Compare October 30, 2024 21:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[issue-405] Add external built image integrity validation #535

[issue-405] Add external built image integrity validation #535

treblereel commented Sep 20, 2024 •

edited by ricardozanini

Loading

treblereel commented Sep 20, 2024

ricardozanini left a comment •

edited

Loading

ricardozanini commented Oct 1, 2024

treblereel commented Oct 1, 2024

ricardozanini commented Oct 1, 2024

treblereel commented Oct 1, 2024

treblereel commented Oct 7, 2024

ricardozanini left a comment

ricardozanini commented Oct 9, 2024

ricardozanini left a comment

ricardozanini commented Oct 11, 2024

jakubschwan left a comment

treblereel commented Oct 11, 2024

wmedvede Oct 14, 2024

treblereel Oct 15, 2024

wmedvede Oct 14, 2024

treblereel Oct 15, 2024

wmedvede Oct 16, 2024

treblereel Oct 16, 2024

wmedvede Oct 14, 2024

treblereel Oct 15, 2024

wmedvede commented Oct 14, 2024 •

edited

Loading

wmedvede commented Oct 30, 2024

[issue-405] Add external built image integrity validation #535

Are you sure you want to change the base?

[issue-405] Add external built image integrity validation #535

Conversation

treblereel commented Sep 20, 2024 • edited by ricardozanini Loading

treblereel commented Sep 20, 2024

ricardozanini left a comment • edited Loading

Choose a reason for hiding this comment

ricardozanini commented Oct 1, 2024

treblereel commented Oct 1, 2024

ricardozanini commented Oct 1, 2024

treblereel commented Oct 1, 2024

treblereel commented Oct 7, 2024

ricardozanini left a comment

Choose a reason for hiding this comment

ricardozanini commented Oct 9, 2024

ricardozanini left a comment

Choose a reason for hiding this comment

ricardozanini commented Oct 11, 2024

jakubschwan left a comment

Choose a reason for hiding this comment

treblereel commented Oct 11, 2024

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

wmedvede commented Oct 14, 2024 • edited Loading

wmedvede commented Oct 30, 2024

treblereel commented Sep 20, 2024 •

edited by ricardozanini

Loading

ricardozanini left a comment •

edited

Loading

wmedvede commented Oct 14, 2024 •

edited

Loading