Skip to content

[Core feature request] Pinning action versions to commit hashes updateable by bots #1691

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

Open
krzema12 opened this issue Oct 24, 2024 · 8 comments
Assignees
Labels
enhancement New feature or request

Comments

@krzema12
Copy link
Member

krzema12 commented Oct 24, 2024

What feature do you need?
By default, when using a binding provided by the bindings server, we refer by the major or full version. It can be a branch or a tag. While major version tags/branches change and it's expected, full versions shouldn't. However, technically nothing stops the action owner to hard-reset some full version branch/tag to point to a different commit, and no one will notice it.

That's why, as a part of security hardening, some workflow owners use full SHA-1 of commits they want to use for each action. It guarantees the action's code won't silently change.

Users of github-workflows-kt can already do it using _customVersion constructor argument:

UploadArtifact(
    // ...
    _customVersion = "actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11",
)

However, dependency updating bots cannot update such commit hashes.

In theory we could try allowing such format when specifying a dependency on an action:

@file:DependsOn("actions:checkout:b4ffde65f46336ab88eb53be808477a3936bae11")

but then, even if we make this commit hash be updated to the right value, there's no mechanism to keep the full version in the comment, like shown in the below example.

Do you have an example usage?

uses: 'actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11' # v4.1.1

Is there a workaround for not having this feature? If yes, please describe it.
No way to make the dependency updating bots work, just specifying the commit hash as version.

@krzema12 krzema12 added the enhancement New feature or request label Oct 24, 2024
@szpak
Copy link

szpak commented Apr 26, 2025

Looking at the tj-actions/changed-files attack, it would be great to have:

@file:DependsOn("actions:checkout:b4ffde65f46336ab88eb53be808477a3936bae11")

generates:

uses: 'actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11' # v4.1.1

Of course, it still has to be checked/verified at the PR level (e.g. when generated by Renovate or something), but a comment is a guide when briefly checking what version am I using.

@Vampire
Copy link
Collaborator

Vampire commented Apr 26, 2025

You currently can do

@file:DependsOn("actions:checkout:b4ffde65f46336ab88eb53be808477a3936bae11")

but you only get the untyped binding if the types are coming from the catalog.

The bindings server would need to find out the nearest version in the ancestry of the Git history to then use the according types information from the catalog to get typed bindings.


Alternatively, we could support a syntax like

@file:DependsOn("actions:checkout:4.1.1__b4ffde65f46336ab88eb53be808477a3936bae11")

or

@file:DependsOn("actions:checkout:b4ffde65f46336ab88eb53be808477a3936bae11__4.1.1")

or similar to have both information in the binding.
The first part can then be used in the YAML as version, the second as comment and to get the typings from the catalog.


To make this coooperate with dependency update bots, we would need to mark the group or name so that the bindings server knows what to put to the metadata.xml.
So for example

@file:DependsOn("actions:checkout___commit:4.1.1__b4ffde65f46336ab88eb53be808477a3936bae11")

so that the metadata.xml can contain the <version>__<sha1> pairs as version.

@krzema12
Copy link
Member Author

I think we should think about implementing this because there's fairly strong signal (compared to other feature requests) that users need it - I recall several related discussions.

The bindings server would need to find out the nearest version in the ancestry of the Git history to then use the according types information from the catalog to get typed bindings.

Right - in fact, it's a task on its own, even if we don't consider referring to action versions by (version, commit hash) pairs.

API-wise, I'm leaning towards

@file:DependsOn("actions:checkout:4.1.1__b4ffde65f46336ab88eb53be808477a3936bae11")

because it's sortable, and the user first sees the version which is more informative.

I think we'd also need a separate server route to return the right entries in metadata.xml - only the ones with the version and the commit hash.

@Vampire
Copy link
Collaborator

Vampire commented Apr 26, 2025

Right - in fact, it's a task on its own, even if we don't consider referring to action versions by (version, commit hash) pairs.

While of course, this means it could again there deliver different results, depending on external state. 🤷‍♂️
And yes, it is not "even if don't consider" but completely orthogonal.
If the version/hash pair is given, there is no need to find the nearest version even if typings need to be taken from the catalog.

because it's sortable

Yes, also for returning multiple results and relying on proper sorting this would work better,
that's why I also prefer version-first if using a pair like that.

I think we'd also need a separate server route to return the right entries in metadata.xml - only the ones with the version and the commit hash.

I don't think "server route" if that is really what you meant.
Imho this should be controllable per dependency.
And that's what I suggested @file:DependsOn("actions:checkout___commit:4.1.1__b4ffde65f46336ab88eb53be808477a3936bae11") for, so that like with ___major and ___minor we just have a third option ___commit (also three underscores) which then tells the metadata.xml generation to put the pairs there instead of versions.

It's open for discussion whether then with actions:checkout:4.1.1__b4ffde65f46336ab88eb53be808477a3936bae11 the version part would be used as-is just like it is now and return 404 and only treat it as pair when actions:checkout___commit:4.1.1__b4ffde65f46336ab88eb53be808477a3936bae11 is used, or whether the pair should be auto-detected and then if a dep-update bot is used just not work as the metadata file contains the wrong content (only the major versions) or actually it would work, but suggest updating to the following major versions as version 5 is bigger than 4.1.1_....

So in the light of the thoughts in the last paragraph I think we should only treat the version as version/commit pair if the ___commit suffix is used. This makes sure there is no conflict with ___major or ___minor (what would happen with actions:checkout___minor:4.1.1__b4ffde65f46336ab88eb53be808477a3936bae11 otherwise), is just more explicit, and the user anyway has to use a special syntax, so he can also add the ___commit.

@krzema12
Copy link
Member Author

I don't think "server route" if that is really what you meant. Imho this should be controllable per dependency. And that's what I suggested @file:DependsOn("actions:checkout___commit:4.1.1__b4ffde65f46336ab88eb53be808477a3936bae11") for, so that like with ___major and ___minor we just have a third option ___commit (also three underscores) which then tells the metadata.xml generation to put the pairs there instead of versions.

Ah yes, it makes perfect sense. I forgot about our already introduced approach for listing versions optimized for version ranges.

It's open for discussion whether then with actions:checkout:4.1.1__b4ffde65f46336ab88eb53be808477a3936bae11 the version part would be used as-is just like it is now and return 404 and only treat it as pair when actions:checkout___commit:4.1.1__b4ffde65f46336ab88eb53be808477a3936bae11 is used, or whether the pair should be auto-detected and then if a dep-update bot is used just not work as the metadata file contains the wrong content (only the major versions) or actually it would work, but suggest updating to the following major versions as version 5 is bigger than 4.1.1_....

So in the light of the thoughts in the last paragraph I think we should only treat the version as version/commit pair if the ___commit suffix is used. This makes sure there is no conflict with ___major or ___minor (what would happen with actions:checkout___minor:4.1.1__b4ffde65f46336ab88eb53be808477a3936bae11 otherwise), is just more explicit, and the user anyway has to use a special syntax, so he can also add the ___commit.

I agree the more explicit ___commit approach is better, at least easier to implement correctly. We should return 404 if the pair is requested for a ___commit artifact. All of this is pretty hacky anyway because we need to convey extra info/mode in artifact name, but I think it's the best we can get.

If the version/hash pair is given, there is no need to find the nearest version even if typings need to be taken from the catalog.

Not sure if I understand it. We should ensure that the pair is valid, that is: the version tag points to the commit hash specified in the pair. We don't want a situation where the commit hash points to v1.2.3 and the given version is v4.5.6, even if the typings happen to be compatible.

@Vampire
Copy link
Collaborator

Vampire commented Apr 26, 2025

We should return 404 if the pair is requested for a ___commit artifact

I hope you miss a "not", otherwise I don't get what you mean.

If the version/hash pair is given, there is no need to find the nearest version even if typings need to be taken from the catalog.

Not sure if I understand it.

You said "in fact, it's a task on its own, even if we don't consider referring to action versions by (version, commit hash) pairs.".
I say if we have the version / hash pair we do not need to lookup the nearest version as the version is already specified and we only need to find the nearest version if the given hash does not include typing information so we need to get the typing for the version from the catalog.

We should ensure that the pair is valid, that is: the version tag points to the commit hash specified in the pair. We don't want a situation where the commit hash points to v1.2.3 and the given version is v4.5.6, even if the typings happen to be compatible.

I'm not so sure about that.
You might want to use a newer hash of an action that does not have any tag, or you might want to use the parent of commit 1.2.3 as the last commit introduced a bug, but still want to use the 1.2.3 bindings.

So we could be reluctant and have the user be responsible to give a senseful pair.
We just generate typings for the given version, write the version to the comment and use the hash as version.

Or we can be restrictive and verify that the given version is the version of that commit or the nearest version in the ancestry.
But given the described attack vector from the linked article, suddenly the versions would point somewhere else and suddenly the binding could no longer be generated and the workflow script execution fails.

Or some user has 1.2.3_hashOf1.2.3 and the action maintainer finds that v1.2.3 has substantialy security defects in certain situations and decides to remove that tag. The user maybe is not affected as he only uses another part, but the tag is gone and so generation will fail and the script again be broken.

So I'm not so sure whether it might be better to here just trust what the user specified.
It is anyway just put to a comment for actions providing typings and otherwise used to generate typing from catalog if not.
Worst that could happen is, that the binding does so much not match the specified hash's code, that the action fails to execute and then the user can correct his error. 🤷‍♂️

@krzema12
Copy link
Member Author

krzema12 commented Apr 27, 2025

I hope you miss a "not", otherwise I don't get what you mean.

Of course, sorry for that!

Regarding validating the pair, thanks for outlining some problems that may indeed occur with it. While we can go with how vanilla GitHub Actions work and not validate the tag at all, I think we're in an unique position to provide some validation before even the workflow runs, and it would be a waste to not use this opportunity in some way.

My intuition tells me that most of the time, the corresponding version tag exists, and if it ceases to exist, the user should know about it. For example, if the tag was removed because of a security concern, failing fast (not even letting the workflow to run) gives the user a chance to decide what to do in such case.

This shifts the tradeoff towards correctness and safety by failing the workflow, which some users may not like. That's why while I'd push towards having the validation in the default, more convenient notation (__commit), we could provide a slightly longer modifier like __commit_lenient (TBD) that would enable behavior resembling the one for YAML, that the version is just a comment and nothing else. Think of it as of let and let mut in Rust where we have immutability with a shorter, more convenient keyword, and still allow mutability with extra ceremony.

I think what we should do next is to put together something like an Architectural Decision Record that would describe various use cases and proposed API, what's not allowed, what the tradeoffs are, and so on. Inferring the final decisions from this discussion becomes more challenging, and such a summarizing step will be good also retrospectively. I can describe it and then we can move iterating on the API there, as PR comments.

@krzema12
Copy link
Member Author

@Vampire @szpak here's an initial version of a design doc, to make the discussion and the description of the proposed approach more structured: https://github.com/typesafegithub/design-docs/blob/enhancements-commit-hash/2025-04-27%20Enhancements%20to%20referring%20to%20actions%20by%20commit%20hash.md. It still misses some details, or comparison of pros/cons, but I think it's a good starting point.

We can comment on it in this PR: typesafegithub/design-docs#1

@krzema12 krzema12 self-assigned this Apr 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants