feat: add OIDC workload identity federation support #1232
No reviewers
mfenniak
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
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
forgejo/runner!1232
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "mpminardi/runner:mpminardi/workload-identity"
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?
Add support for OIDC workload identity federation in a shape similar to
what GitHub supports.
Add a new workflow and job-level setting named "allow-generating-id-tokens"
that enables ID token generation at the workflow or specific job level.
Export ACTIONS_ID_TOKEN_REQUEST_TOKEN and ACTIONS_ID_TOKEN_REQUEST_URL in
the runner environment. These are populated from the task context keys
forgejo_actions_id_token_request_token and
forgejo_actions_id_token_request_url respectively which are only set when
the aforementioned "allow-generating-id-tokens" setting is enabled.
Required by https://codeberg.org/forgejo/forgejo/pulls/10481
Signed-off-by: Mario Minardi [email protected]
2f6e9dd7e27cf87484a67cf87484a68c434e940b@mpminardi I would happily permit this PR to run CI so that you can see test results, but I'm unable to due to the conflicting files. If you'd like the CI run, please rebase/merge main and let me know, and I will permit it.
8c434e940ba535809b35@mfenniak thank you! Have rebased the PR on top of main
@ -53,2 +54,4 @@"boolean": {}},"workflow-allow-generating-id-tokens": {"description": "Allow all jobs in the workflow to generate an OIDC compliant ID token.\n\n[Documentation](https://forgejo.org/docs/TODO)",TODO: need to actually have this and the below point to docs before merging (or omit the docs link / add later).
a535809b351fc525ff601fc525ff60d35bdda98dWIP: feat: add OIDC workload identity federation supportto feat: add OIDC workload identity federation supportThis looks really good, with some minor comments for us to resolve.
I think the workflow entry name can be better. I'd propose:
enable-openid-connect: true.enable-*is consistent withenable-email-notificationsopenid-connect(or maybeenable-oidc🤷) is a little more specific about what is being turned on, rather than "tokens" which in this context is a term pretty disconnected from what you're doingI'm not firm on
enable-openid-connectand I'm happy to discuss... although we shouldn't burn too many cycles on it either... but I quite dislikeallow-generating-id-tokens(said with respect).@ -31,1 +30,3 @@RawConcurrency *RawConcurrency `yaml:"concurrency"` // For GiteaRawNotifications yaml.Node `yaml:"enable-email-notifications"`RawConcurrency *RawConcurrency `yaml:"concurrency"` // For GiteaRawAllowGeneratingIDTokens yaml.Node `yaml:"allow-generating-id-tokens"`From the usage, it seems like these
RawAllowGeneratingIDTokensstruct members could be defined as*boolrather thanyaml.Node, eliminating the need for the yaml type check functions.yaml.Nodeshould only be needed when the input could be multiple different types of yaml node and the runner needs to standardize it -- eg.runs-oncan be a scalar, an array, or a mapping, and then is standardized for code to understand.@ -313,2 +313,4 @@runEnvs["ACTIONS_RUNTIME_TOKEN"] = runtimeTokenrunEnvs["ACTIONS_ID_TOKEN_REQUEST_TOKEN"] = taskContext["forgejo_actions_id_token_request_token"].GetStringValue()runEnvs["ACTIONS_ID_TOKEN_REQUEST_URL"] = taskContext["forgejo_actions_id_token_request_url"].GetStringValue()This should also be covered with an automated test, but I am assuming that you're expecting to cover it in the end-to-end test. Please confirm.
Yup this should be covered by the end-to-end test! Can look at adding some simple regression tests here as well though.
@ -62,2 +62,4 @@masker.add(v)}if v := task.Context.Fields["forgejo_actions_id_token_request_token"].GetStringValue(); v != "" {masker.add(v)Please cover with an automated test -- it would be security sensitive for this to regress.
It raises the question of whether this belongs in
${{ secrets... }}and not in the environment. What do you think?Whoops good catch! Will add some testing around this.
That is also a fair point! The thinking here was to have maximum compatibility with the patterns GitHub uses for this functionality which exports these into the runner environment and documents doing:
We could instead put these values into secrets (and vars potentially for the URL) and drop the extra bits added to the
ACTIONS_ID_TOKEN_REQUEST_URLunder the hood and have something like:Downside here being that it will force more of a change for users migrating from GitHub / require changes in reusable third-party actions that end up being ported over to Forgejo vs having things "just work".
I don't have super strong preferences around this though! If we do want to diverge from the GitHub patterns for this I don't think it will be a huge amount of friction / it could be explained in docs pretty easily, but it will add more friction than keeping the patterns here as-is.
OK, let's leave it as-is (w/ test automation around the masking). I think compatibility is a good reason, and other than the enforced masking of secrets, there's no significant value. 👍
@mfenniak wrote in #1232 (comment):
enable-openid-connectsounds good to me!d35bdda98dbf63f59d4ebf63f59d4e351e4eef4aGreat! I'll tag a release of the runner in a day or two so that the forgejo side's
jobparserupgrade can be done, unblocking the forgejo PR. Thank you for the contribution.