search

webispy / checkpatch-action

Github action for checkpatch.pl
7 stars 6 forks source link

Pin docker image digest #21

Closed nikolaik closed 1 year ago

nikolaik commented 1 year ago

This reduces the consequence of a base image for this action being tampered with, since action consumers can pin the action sha and be a bit more sure what code is running.

Are you open for this change, do you want to move the github workflows to the master branch as well?

webispy commented 1 year ago

Thanks for your pull request! I understand your suggestion, but the reason I separated the docker layer is for performance. (more than 5 minutes -> about 10 seconds) Since there is a lot of overhead to perform apt-get update, apt-get install and locale-gen whenever the checkpatch action is performed, I think it is much more efficient to use a pre-built image.

nikolaik commented 1 year ago

Thanks for explaining, that makes perfect sense.

What do you think about pining the docker image digest and possibly automate updating it?

Getting the digest

❯ docker pull webispy/checkpatch:latest
(...)
❯ docker inspect webispy/checkpatch:latest --format '{{index .RepoDigests 0}}'
webispy/checkpatch@sha256:e84b5049be1410395ecca4547e14b1d209fb9f3327383bef83b25aeb666a8304

So the change would be

@@ -1,4 +1,4 @@
-FROM webispy/checkpatch
+FROM webispy/checkpatch@sha256:e84b5049be1410395ecca4547e14b1d209fb9f3327383bef83b25aeb666a8304

 COPY entrypoint.sh /entrypoint.sh
 COPY review.sh /review.sh
webispy commented 1 year ago

How about using the tag name rather than using the full SHA string? The version tag will be reflected through the PR https://github.com/webispy/checkpatch-action/pull/22 When the Docker image changes, I plan to update the version tag as well.

nikolaik commented 1 year ago

A docker tag is still mutable so that would unfortunately not help things securitywise. There is a similar discussion here https://github.com/lycheeverse/lychee-action/issues/19 that describes the issue I'm trying to solve. They eventually solved it by rewriting it as a nodejs action with all the dependencies bundled, which is hard to do for this plugin. I believe pinning is the right, albeit cumbersome, way to do it.

webispy commented 1 year ago

Thanks for sharing links to similar discussions. I found the Using third-party actions Github guide document through the link you shared and I think your suggestion is correct.

So, if you update this PR or upload a new PR, I will merge it. Thank you!

nikolaik commented 1 year ago

Thanks for sharing links to similar discussions. I found the Using third-party actions Github guide document through the link you shared and I think your suggestion is correct.

So, if you update this PR or upload a new PR, I will merge it. Thank you!

I've updated the PR now, thanks for helping and understading! After this PR I plan to submit another one to allow using the uboot specific version of chechpatch.pl found here https://github.com/u-boot/u-boot/blob/master/scripts/checkpatch.pl

webispy commented 1 year ago

Ok. Thank you for update. When the new pull request you described is uploaded, I will review it.