ci: improve handling of public-key-check #2847

mqasimsarfraz · 2024-05-14T21:39:58Z

Currently, in order to publish ig-k8s image / gadget images on a fork users will have to:

Set secrets for cosign (COSIGN_PRIVATE_KEY/COSIGN_PASSWORD)
Update the public key in source (pkg/resources/inspektor-gadget.pub)

Otherwise public-key-check job will fail and ig-k8s image /gadgets images won't be published. The idea of this PR is to improve this workflow by not signing images by default on forks. public-key-check is removed from the needs of both the image publishing steps to allow images to still be published but not signed on forks. Having said that, public-key-check is moved to release to ensure if incorrect keys are configured we won't be able to release. So it isn't a fatal job for CI runs on main but only for releases.

Also tests need to be adapted to allow running without verification on a fork!

Testing Done

CI is green on the fork now

Currently, in order to publish ig-k8s image / gadget images on a fork users will have to: - Set secrets for cosign (COSIGN_PRIVATE_KEY/COSIGN_PASSWORD) - Update the public key in source (pkg/resources/inspektor-gadget.pub) the idea of this change is to improve this workflow by not signing images by default on forks. We won't depend on public-key-check when publishing images, but add it in the release job to ensure images are published with correct keys. Signed-off-by: Qasim Sarfraz <qasimsarfraz@microsoft.com>

Signed-off-by: Qasim Sarfraz <qasimsarfraz@microsoft.com>

eiffel-fl

Hi!

I am not convinced by the approach with regard to signing.
Moreover, all the remaining parts regarding testing can be part of a dedicated PR.

Best regards.

eiffel-fl · 2024-05-15T08:44:52Z

.github/workflows/inspektor-gadget.yml

-    if: github.event_name == 'push' || (github.event.pull_request.head.repo.full_name == github.repository && github.actor != 'dependabot[bot]') && needs.check-secrets.outputs.cosign == 'true'
+    # we merge something on main. Also, we ensure this job will always run on
+    # for 'inspektor-gadget/inspektor-gadget' respository
+    if: needs.check-secrets.outputs.cosign == 'true' || (github.event_name == 'push' && github.repository == 'inspektor-gadget/inspektor-gadget')


The comment refers to dependabot, but it was removed from the if, I suspect it will fail when run by the bot.

Moreover, this totally blocks forks from signing their image without tweaking the CI.
So, before they had to tweak the public key, now they have to tweak the CI, in the end they still have something to tweak.

I should have explained the approach bit better 🙈

The comment refers to dependabot, but it was removed from the if, I suspect it will fail when run by the bot.

Actually the first check needs.check-secrets.outputs.cosign == 'true' ensures that the job will only run if secrets are configured so if no secrets are configured the CI won't run!

Moreover, this totally blocks forks from signing their image without tweaking the CI.

Do you mean they need to modify (github.event_name == 'push' && github.repository == 'inspektor-gadget/inspektor-gadget') ?

I added this explicit check for our main repository so if somehow we delete cosign secrets the job will start to fail but for forks this check is ignored. I just tried it on a fork and configured the secrets and it seem to be working fine.

I should have explained the approach bit better 🙈

The comment refers to dependabot, but it was removed from the if, I suspect it will fail when run by the bot.

Actually the first check needs.check-secrets.outputs.cosign == 'true' ensures that the job will only run if secrets are configured so if no secrets are configured the CI won't run!

The secret exists, but dependabot has not access to it and I do not want to give it access.
So, we need this github.actor != 'dependabot[bot]'.

Moreover, this totally blocks forks from signing their image without tweaking the CI.

Do you mean they need to modify (github.event_name == 'push' && github.repository == 'inspektor-gadget/inspektor-gadget') ?

Indeed.

I added this explicit check for our main repository so if somehow we delete cosign secrets the job will start to fail but for forks this check is ignored. I just tried it on a fork and configured the secrets and it seem to be working fine.

Did you try to remove the secret?
Also, please share more information on the fork, because it is not clear why the job is run.
Actually, the goal of this PR is to avoid forks to tweak anything to have the CI running, but in your case the fork CI is failing because the secret does not correspond to the source public key, which is exactly like the current situation.

eiffel-fl · 2024-05-15T08:45:47Z

.github/workflows/inspektor-gadget.yml

@@ -715,7 +715,6 @@ jobs:
    if: github.event_name != 'pull_request'
    needs:
      - build-gadget-container-images
-      - public-key-check


We sign our images on main, so we should not remove this from the needs here.

See the comment below.

eiffel-fl · 2024-05-15T08:46:00Z

.github/workflows/inspektor-gadget.yml

@@ -1303,7 +1303,6 @@ jobs:
      - build-ig
      - build-helper-images
      - check-secrets
-      - public-key-check


I know it isn't a nice part but the idea was to remove this dependency from here so the entire job won't be skipped and gadget images will be published without signing if secrets aren't configured. The other approach I have in mind is to split this job into two. 1) gadget pushing 2) gadget signing. The only tricky bit is how to share the gadgets digests between the jobs (I know how to do it but would require more changes) but in that case we can move the need of public-key-check to gadget-signing job. What do you say?

If I understand correctly, if the public key check goes wrong, the gadgets will still be pushed.
On one hand, it seems "OK", on the other, I have the feeling this give a wrong feeling of security as people would just push unsigned gadget, which would end up being refused by the CLI (unless --verify-image=false is set).
I guess we do not have other possibilities than to split these jobs.

eiffel-fl · 2024-05-15T08:49:40Z

pkg/testing/ig/ig.go

+	verifyImage := true
+	if os.Getenv("GADGET_VERIFY_IMAGE") == "false" {
+		verifyImage = false
+	}


Suggested change

verifyImage := true

if os.Getenv("GADGET_VERIFY_IMAGE") == "false" {

verifyImage = false

}

verifyImage := os.Getenv("GADGET_VERIFY_IMAGE") == "true"

Unless you want to ensure the two possible values are "false" and "true".

eiffel-fl · 2024-05-15T08:51:26Z

integration/k8s/run_top_file_test.go

@@ -103,7 +103,7 @@ func TestRunTopFile(t *testing.T) {
 		RunTestSteps(commands, t, WithCbBeforeCleanup(PrintLogsFn(ns)))
 	})

-	cmd := fmt.Sprintf("$KUBECTL_GADGET run %s/top_file:%s -n %s -o json", *gadgetRepository, *gadgetTag, ns)
+	cmd := fmt.Sprintf("$KUBECTL_GADGET run %s/top_file:%s --verify-image=%t -n %s -o json", *gadgetRepository, *gadgetTag, *gadgetVerifyImage, ns)


OK for now, but we should definitely have a way to define and tweak this globally rather than having to do so in each file.

Agreed, we can probably use config for this in future.

mqasimsarfraz added 3 commits May 14, 2024 18:27

gadgets/test: Add an option skip image verification

68b12c7

Signed-off-by: Qasim Sarfraz <qasimsarfraz@microsoft.com>

integration/kubectl-gadget: Add an option to skip image verification

9c840e6

Signed-off-by: Qasim Sarfraz <qasimsarfraz@microsoft.com>

mqasimsarfraz requested review from blanquicet, mauriciovasquezbernal, eiffel-fl and alban as code owners May 14, 2024 21:39

eiffel-fl reviewed May 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: improve handling of public-key-check #2847

ci: improve handling of public-key-check #2847

mqasimsarfraz commented May 14, 2024 •

edited

eiffel-fl left a comment

eiffel-fl May 15, 2024

eiffel-fl May 15, 2024

mqasimsarfraz May 17, 2024

eiffel-fl May 21, 2024

eiffel-fl May 15, 2024

mqasimsarfraz May 17, 2024

eiffel-fl May 15, 2024

mqasimsarfraz May 17, 2024 •

edited

eiffel-fl May 21, 2024

eiffel-fl May 15, 2024

eiffel-fl May 15, 2024

mqasimsarfraz May 17, 2024

ci: improve handling of public-key-check #2847

Are you sure you want to change the base?

ci: improve handling of public-key-check #2847

Conversation

mqasimsarfraz commented May 14, 2024 • edited

Testing Done

eiffel-fl left a 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

mqasimsarfraz May 17, 2024 • edited

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

mqasimsarfraz commented May 14, 2024 •

edited

mqasimsarfraz May 17, 2024 •

edited