feat: include auth token when cloning actions from own instance #1249
mfenniak
viceice
Labels
No labels
FreeBSD
Kind/Breaking
Kind/Bug
Kind/Chore
Kind/DependencyUpdate
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
Windows
linux-powerpc64le
linux-riscv64
linux-s390x
run-end-to-end-tests
run-forgejo-tests
No milestone
No project
No assignees
4 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
forgejo/runner!1249
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "JohnMoon-VTS/runner:token-auth"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Currently, when using an instance with
REQUIRE_SIGNIN_VIEW = true, itis impossible to self-host repos containing Forgejo Actions as the runner
does not pass any auth token when it attempts to clone the action.
Let's enable this workflow by passing along the default
github.Tokentoken when we're trying to clone the action in question from our own
instance. This can happen in two cases:
actions/checkout@v5) and the defaultactions URL is set to one's own instance.
https://forgejo.my-instance.com/actions/checkout@v5).While the
github.Tokenscope is limited to the repo where the actionis running, it does authenticate the runner to the server and allow it
to clone actions repos that are "public" on the instance. However, this
is only true since Forgejo v13.0.0 1. As such, only set the token if
the server version is reported as >= 13.0.0.
For more discussion on this feature, see:
forgejo/forgejo-actions-feature-requests#3
isn't the token still limited to the triggering repo for forgejo below v14? I remember some token rights expand in v14
I am not positive what the changes are in v14, but I did test this with my 13.0.3 instance and it does work.
To be clear, my instance has
REQUIRE_SIGNIN_VIEW = trueand the actions repo the workflow clones is "public" within the instance.it's in v13, you need to make this compatible with v11 because it's lts. see below
needs to be compatible with all supported versions
@viceice, I'm not sure what the ask is here. If we merge this PR as is, the runner will include the token if the actions URL is set to one's own instance.
Without this PR:
I must be missing something... if we merged this PR, what would be the incompatibility for users on v11?
@JohnMoon-VTS wrote in #1249 (comment):
it would fail on v11 if the instance and the action repo is public and it's now passing the token which doesn't have access to public repo.
Understood - thank you for clarifying.
So, the two paths forward:
I'm happy to implement option 2. I could also see the version number context being helpful for these sorts of issues in the future, so maybe it'd be good to land that before the next LTS. Does that approach sound reasonable to you @viceice?
@JohnMoon-VTS wrote in #1249 (comment):
yes. option 2 sounds good as long term solution. otherwise we're blocked again in future for sure.
I would help you to get it into v14 if you're able to get it done fast. otherwise it'll come in v15.
@mfenniak wdyt? maybe it should be a protocol extension, so the runner knows the server version after first contact on start-up? I don't like to post it on every job context.
It's a great catch in review. 👍 Even understanding the issue I would have mistaken believed the behaviour would be fine on v11. Thanks for noting it, @viceice. @JohnMoon-VTS and I did a lot of discussing to end up at the same design as #855, so I think we can also celebrate that it's the right choice if it was arrived at independently twice. 🤣
I like @JohnMoon-VTS's idea of adding version info to the context.
FORGEJO_VERSION/${{ forgejo.version }}being accessible in an action or reusable workflow would allow reasonable conditional logic like, "ah, this API isn't available yet, this API has a known bug, etc."It's also a minor enough change that it should be reasonable to backport for Forgejo v14's release.
@mfenniak sounds reasonable and I agree
Adding it to the context sounds good. However, it has to be done in such a way that it does not end up in
githubto prevent compatibility problems down the road. That might be tricky as long asforgejois an alias ofgithub.c59539d36b32cd48ac3732cd48ac37d3c3e2fc43This PR should be in good shape to re-review. I've added the context on the server side here: https://codeberg.org/forgejo/forgejo/pulls/10642.
In addition to the integration tests in both, I manually spun up a new server with the change and added a log message on the runner side to verify the version number is passed correctly through the context.
@ -312,0 +321,4 @@// actions from "public" repos.// See: https://codeberg.org/forgejo/forgejo/pulls/8889func tokenAuthSupported(serverVersion string) (bool, error) {v, err := semver.NewVersion(serverVersion)I'd feel more comfortable with this if it had handling for
serverVersion == ""(eg. the task came from Forgejo v11, so we can definitely returnfalse), and separately ifsemver.NewVersion()returned an error that we actually treated that as an error. I can imagine subtle cases may exist if, for some reason, an invalid version came through that thesemverlibrary couldn't parse... it would just silently drop token auth support rather than surfacing that error state for resolution.Let me know if the latest push resolves this concern!
@aahlenst wrote in #1249 (comment):
@JohnMoon-VTS's PR update, if I'm reading it accurately, uses the value only for detection of whether the new token auth capability is supported or not. I believe it is not yet exposed in
forgejocontext, which is good for the scope of this PR. We can expose it in the near future and make sure it doesn't leak intogithubcontext at that time.d3c3e2fc43d8ff254aee