Add guidance on pinning GitHub Actions and container images

[pellared]

Why

A bad actor can force push a tag so that GitHub Action to do some malicious actions.

A bad actor can push a malicious container image under the same name.

What

We should use digest pinning to mitigate the possibility of using a malicious GitHub Actions and container images.
It should also make the build more reproducible.
Some references:

• https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#using-third-party-actions
• https://candrews.integralblue.com/2023/09/always-use-docker-image-digests/

When using GitHub Actions we can add a comment with a after the digest (e.g. actions/checkout@01aecccf739ca6ff86c0539fbc67a7a5007bbc81 # v2.1.0).
Both Renovate and Dependabot can bump both the digest and the tag in the comment:

• For actions that are pinned-by-hash, bump the human readable version number in the code comment dependabot/dependabot-core#4691 (comment)
• https://docs.renovatebot.com/modules/manager/github-actions/

When using container images we can add the digest at the end (e.g. node:14.15.1@sha256:d938c1761e3afbae9242848ffbb95b9cc1cb0a24d889f8bd955204d347a7266e).
Both Renovate and Dependabot can bump the image name and the digest as well:

• Don't try to resolve pinned sha's to versions when updating docker images dependabot/dependabot-core#6150
• https://docs.renovatebot.com/docker/

[pellared]

I propose to add recommendation somewhere under /docs that explain how to pin the container images and GitHub Actions and how to configure Dependabot and Renovate to have them being bumped.

@open-telemetry/governance-committee @open-telemetry/technical-committee @open-telemetry/sig-security-maintainers, thoughts?

[svrnm]

Thanks for raising this, as far as I remember the OpenSSF Scorecard Automation should catch that as well and raise an issue on repositories

[yurishkuro]

The automation creates a PR that changes all actions to pinned. But personally I think it goes overboard and in Jaeger we went back to semver refs in some places. Eg you want pinned versions in a workflow that performs a release, but not in the integration tests.

[jpkrohling]

@yurishkuro, do you mind sharing why did you go back to semver? The recommendation, as I understand it, is to have this:

      - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7

Source: https://github.com/open-telemetry/opentelemetry-collector-releases/blob/2dd46ba38de4affe3ff923959d23291f5c89aca9/.github/workflows/base-release.yaml#L38

Both tools can then open a PR bumping that upon a new version: https://github.com/open-telemetry/opentelemetry-collector-releases/pull/578/files

I'm probably missing something, but if your integration tests are using semver with 4.1 and 4.1.7 was just released, you'd automatically use this new 4.1.7 without being aware of that. Beyond the security aspects, wouldn't it be bad in case of test failures introduced by that change? Is it about reducing the review burden on maintainers?

[yurishkuro]

@jpkrohling 99% of the time there is no benefit for me as a maintainer in minor bumps of the versions of GH actions, but it's a constant maintenance burden to merge those PRs. For example, for all integration tests, it makes no difference if the setup-go action is v4, v4.1, or v4.1.2. So I can define it as v4 semver and never worry about bumps PRs from dependabot (I just ran a test for last year of commits in Jaeger, 30% of them were from dep bots). But I cannot do v4 if it's pinned by hash.

[jpkrohling]

it's a constant maintenance burden to merge those PRs

Sounds like an automation issue? Renovate has a "noise reduction" feature, so that it would group related dependencies (like "all github actions") into one single PR, and you can adjust the frequency: https://docs.renovatebot.com/noise-reduction/

[trask]

I just ran a test for last year of commits in Jaeger, 30% of them were from dep bots

I agree these PRs create a lot of clutter if nothing else

Renovate has a "noise reduction" feature, so that it would group related dependencies (like "all github actions") into one single PR, and you can adjust the frequency: https://docs.renovatebot.com/noise-reduction/

thanks, I'm trying this out: open-telemetry/opentelemetry-java-instrumentation#11829

[yurishkuro]

Sounds like an automation issue? Renovate has a "noise reduction" feature, so that it would group related dependencies (like "all github actions") into one single PR

We are already using the grouping of dependencies in Jaeger, but it's not enough - if a project depends on a dozen GH actions there's a high chance at least one will have a new patch every week, which will create a new PR that provides exactly 0 value to me. So another thing I do is restrict certain groups to minor version bumps only & ignore patches.

[pellared]

So another thing I do is restrict certain groups to minor version bumps only & ignore patches.

Patches use to have bugfixes (including security fixes). Not bumping them may not appropriate from security perspective.

[Kielek]

After introducing support for digest in testcoinares,
we have pinned both GH actions and docker images to SHA in OTel .NET Auto repo. No issues so far.

Ref:

• GH action GH Action - pin actions to GH commits opentelemetry-dotnet-instrumentation#3505
• Docker: Pin docker images to exact digest opentelemetry-dotnet-instrumentation#3514