-
Notifications
You must be signed in to change notification settings - Fork 1
feat(DEV-1582): New fetch-github-token-js action #8
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
base: main
Are you sure you want to change the base?
Conversation
Introduces a new GitHub Action to fetch ephemeral GitHub tokens from Vault using OIDC authentication.
Introduces a 'skip-token-revoke' input and adds a post step to revoke the Vault token after the action completes. Renames dist/index.js to dist/main.js and adds dist/revoke.js for post-job cleanup. Updates main action logic to save state needed for token revocation.
|
Random thought I just had: does the revocation counts as a request towards the GitHub App? As we know, we can only make up to 15k requests per hours so I was wondering if this will affect this. |
esenmarti
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I misclicked on "approve". I have some questions first 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new GitHub Action to securely fetch ephemeral GitHub tokens from Vault using OIDC authentication. The action retrieves tokens from either ci-dev or ci-prod Vault instances and automatically revokes them after workflow completion.
Key Changes:
- Implements OIDC-based authentication with Vault to fetch ephemeral GitHub tokens
- Adds automatic token revocation via post-action script with configurable skip option
- Includes token validation using GitHub CLI before exposing to workflows
Reviewed Changes
Copilot reviewed 5 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| action.yml | Defines the action interface with inputs for Vault configuration and outputs for the token |
| main.js | Implements OIDC authentication, token fetching, and validation logic |
| revoke.js | Handles post-action token revocation with optional skip functionality |
| package.json | Declares project dependencies and build configuration |
| README.md | Provides usage documentation and input/output specifications |
Files not reviewed (1)
- fetch-github-token-js/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Replaces 'skip-token-revoke' with 'revoke' input for clarity and reverses its logic in revoke.js and action.yml. Updates Node.js engine requirement to >=24 and sets main entry to dist/main.js in package.json. Improves vault role generation and updates documentation and workflow usage instructions.
v1v
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a few comments. In general, I think this action could be added to the existing one, use new dependencies, and use a more TypeScript-native approach for interacting with the GitHub context. Finally I miss some UTs and e2e tests here.
I plan to test it out in one of our playgrounds
fetch-github-token-js/README.md
Outdated
| @@ -0,0 +1,34 @@ | |||
| # <!--name-->Get Ephemeral GitHub Token from CI Vault<!--/name--> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the user's point of view, I think this should keep the same name as previously; it's a new feature, so v1.1.0 could be the semver here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean replacing the other old GH action with this one, but reflecting like only an upgrade from the users POV?
Edit: Never mind, just seen that part on the more general comment about adding it to the existing GH Action.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved fetch-github-token-js to the currently existing folder fetch-github-token to replace the old action once this is merged and a new release is out with the new minor upgrade.
fetch-github-token-js/main.js
Outdated
|
|
||
| // Generate vault role if not provided | ||
| if (!vaultRole) { | ||
| const workflowRef = process.env.GITHUB_WORKFLOW_REF; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wonder if using a more native support instead of env variables can be done here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried using the @actions/github npm package but I haven't found an easy way of retrieving the workflow_ref as I can do with process.env
Co-authored-by: Victor Martinez <[email protected]>
Upgraded @actions/core to 1.11.1 and axios to 1.13.1 in package.json and package-lock.json. Changed Node.js engine requirement to >=20.
|
While testing this feature branch at here (only accessible by Elastic employees) I've found the below deprecation/warning: |
Introduces a GitHub Actions workflow to run unit tests on pull requests to main. Refactors fetch-github-token-js to export the run function for unit testing, adds a Jest test file, and updates dependencies to include Jest. Also removes unnecessary permissions from test-fetch_token.yaml.
|
@copilot , you are a GitHub actions expert and now about GitHub composite actions. When using the action in this PR it prints the below warning: Can you analyse the reason for the warning? and whether the existing dependencies in the package.json or the generated files in the Please provide a path forward to solve the deprecation warning |
I asked copilot and: Source: follow-redirects package in the and suggested: Replace https://nodejs.org/en/learn/getting-started/fetch
|
Replaces axios with fetch in files for Vault authentication and secret retrieval. Updates response handling to match fetch API, improves error reporting, and adjusts token extraction logic. Adds permissions to GitHub Actions workflow. Adds new dist files for main and revoke entry points, and updates dependencies in package.json.
Deleted fetch-github-token-js/dist/101.main.js and 101.revoke.js as they are no longer needed. Updated main.js and revoke.js in dist and source, and made minor package.json and package-lock.json changes to reflect the new build output.
Replaces CommonJS-style Webpack bootstrap code in dist/main.js and dist/revoke.js with ESM-compatible imports and exports. Adds dist/package.json with "type": "module" to enable ESM. Updates references to Node.js built-in modules for ESM. Migrates test file from .test.js to .test.mjs for ESM support.
Thank you for all this information, I've revamped it to use native fetch instead of axios. Also, I've revamped the tests as I needed to modify it from commonJS to ES Module as the latest version of OctoKit is of type ESM. |
Removed fetch-github-token-js directory and moved all files to fetch-github-token.
| import { Octokit } from '@octokit/core'; | ||
| import crypto from 'crypto'; | ||
|
|
||
| export async function run() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aggregate Cyclomatic Complexity: 12 (Complex, high risk category)
Maintainability Index: 69.01 (Moderately maintainable)
Main Function Complexity: The run function has a complexity of 11 with 105 lines of code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @kuisathaverat. thank you for your comment and for this helpful info, could we also know which parts of the code are the ones that execution has identified as complex?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function has nine returns and an uncountable nested if/try/for clauses. The only way to reduce the complexity (paths) is to split the function into meaningful functions of 10-20 lines with no more than three levels of nested if/try/for and with a single point of return on each. In the current implementation, it is tough to identify and fix an error, as you cannot fully understand the implications of a change in that code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I've revamped the code logic into modular functions and adapted the unit tests.
fetch-github-token/main.js
Outdated
| } | ||
|
|
||
| core.saveState('github-ephemeral-token', githubToken); | ||
| core.setOutput('token', githubToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the token is not masked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added last commit with core.setSecret() to mask that value.
fetch-github-token/main.js
Outdated
|
|
||
| // Generate vault role if not provided | ||
| if (!vaultRole) { | ||
| const workflowRef = process.env.GITHUB_WORKFLOW_REF; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can potentially get any token for any repo from any repo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, correct me if I'm wrong here, but even if you could mangle the env variable value, you couldn't login into Vault, as Vault would retrieve the workflow_ref value from the JWT token presented, and would only let it pass if the workflow_ref matches the value that the requested role has as refs allowed to login.
This logic is only to let it generate the vault-role value to ask the Vault instance to log in with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is related to allowing workflows that must not have access to other repos/resources, not about taking over the vault. Thus, a workflow that can access a token can access any token. That's my point, it is a security risk.
Added a call to core.setSecret to mask the GitHub token retrieved from Vault, ensuring it is not exposed in logs. Updated tests to mock and verify setSecret usage.
| throw new Error('No GitHub token found in Vault secret response.'); | ||
| } | ||
|
|
||
| core.setSecret(githubToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
| } | ||
|
|
||
| export function generateVaultRole() { | ||
| const workflowRef = process.env.GITHUB_WORKFLOW_REF; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is possible to make a cross-repo token access. A repository repoA in the workflowA can use the environment variable GITHUB_WORKFLOW_REF to inject the workflow ref of the workflowB in the repoB, accessing the token in repoB and all its privileges. The only way to avoid it with the current implementation is by treating this${hash} part of the token-policy-${hash} as a code that must be stored in GitHub secrets and must not be predictable (like a password), so users cannot access those policies without knowing the ${hash} part and they cannot predict it. On the downside, this limits the action's use to feature branches because forks cannot access GitHub secrets in our org.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I can illustrate this in one of our existing GH workflows in a private repository.
BTW, actions/toolkit#1264 (comment) could be a way to infer the WORKFLOW_REF using JWT as long as https://github.com/actions/toolkit/blob/main/packages/github/src/context.ts does not provide the support for the worklfow_ref.
Or maybe, GITHUB_WORKFLOW_REF is an immutable env variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe, GITHUB_WORKFLOW_REF is an immutable env variable
GITHUB_WORKFLOW_REF is immutable when using env, while it can be mutable when using GITHUB_WORKFLOW_REF=.... <command> in the run step
- name: print GITHUB_WORKFLOW_REF
run: echo "$GITHUB_WORKFLOW_REF"
- name: GITHUB_WORKFLOW_REF cannot be overridden in env
run: echo "$GITHUB_WORKFLOW_REF"
env:
GITHUB_WORKFLOW_REF: elastic/ci-gh-actions/.github/workflows/test-fetch_token.yaml@refs/pull/5/merge
- name: GITHUB_WORKFLOW_REF can be overridden in run
run: |
export GITHUB_WORKFLOW_REF=elastic/ci-gh-actions/.github/workflows/test-fetch_token.yaml@refs/pull/5/merge
echo "$GITHUB_WORKFLOW_REF"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you overwrite it on the GITHUB_ENV?
- name: GITHUB_WORKFLOW_REF can be overridden in GITHUB_ENV?
run: |
echo GITHUB_WORKFLOW_REF=elastic/ci-gh-actions/.github/workflows/test-fetch_token.yaml@refs/pull/5/merge >> "${GITHUB_ENV}"
- name: GITHUB_WORKFLOW_REF can be overridden in GITHUB_ENV?
run: |
echo "$GITHUB_WORKFLOW_REF"There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me try that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried
- name: I'm evil and I am injecting GITHUB_WORKFLOW_REF via a wrapper script
run: |
NODE="$(which node)"
cat <<EOF >node
#!/bin/bash
export GITHUB_WORKFLOW_REF=elastic/ci-gh-actions/.github/workflows/test-fetch_token.yaml@refs/pull/5/merge
echo "I am evel and I am injecting \$GITHUB_WORKFLOW_REF"
echo "$NODE will not be honoured but $(pwd)/node"
"$NODE" "\$@"
EOF
chmod +x ./node
echo "$(pwd)" >> $GITHUB_PATH
- name: Check node version
run: node --version
As Ivan mentioned so I could inject my changes, however, for a node24 composite action, the node24 version is installed somewhere else:
I couldn't find the absolute path or whether it's read-only, but I think it's important to note that anyone with write access could potentially change the env variable on the fly, even though it's meant to be immutable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for trying it @v1v, but correct me if I'm wrong, as I think the GitHub OIDC provider auto-generates the token at the start of each run, would the default OIDC token of the workflow have this mangled workflow_ref? could you try this action after mangling it to check if the claim from the OIDC token is the correct one or not? https://github.com/github/actions-oidc-debugger?tab=readme-ov-file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you suggesting that the token policy could be altered by manipulating the GITHUB_WORKFLOW_REF environment variable? Ultimately, the GitHub OIDC token will act as the gatekeeper, ensuring that the workflow_ref matches properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, reading what Ivan said:
It is possible to make a cross-repo token access. A repository repoA in the workflowA can use the environment variable GITHUB_WORKFLOW_REF to inject the workflow ref of the workflowB in the repoB, accessing the token in repoB and all its privileges. The only way to avoid it with the current implementation is by treating this${hash} part of the token-policy-${hash} as a code that must be stored in GitHub secrets and must not be predictable (like a password), so users cannot access those policies without knowing the ${hash} part and they cannot predict it. On the downside, this limits the action's use to feature branches because forks cannot access GitHub secrets in our org.
I think that this security risk could only be the case "if" we could mangle the workflow_ref that is presented with the GitHub OIDC token to the Vault instance, if that's not the case, even if we modify the env variable workflow_ref, when the GitHub OIDC token is presented to the Vault instance, it will check that the calling GH workflow it's not allowed for that role and will not let it login.
I'm not understanding what would be the risk here if not for this, "if" it could be mangled on the GitHub OIDC Token too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addresses DEV-1582.
This pull request refactors the
fetch-github-tokenGitHub Action from a composite implementation to a js-based action for improved features like the possibility of using post-actions (to be able to revoke tokens at will). It introduces a new package, updates documentation, and adds automated unit testing. The most important changes are grouped below:Action Implementation Migration
main.jsand post-action logic (token revocation) inrevoke.js. The action now uses the Node.js 24 runtime for execution. (fetch-github-token/action.yml,fetch-github-token/main.js,fetch-github-token/revoke.js) [1] [2] [3]Configuration and Package Management
package.jsonto define the Node.js package, dependencies (such as@actions/coreand@octokit/core), and test scripts for the action. (fetch-github-token/package.json)Testing and Quality Assurance
fetch-github-token/tests/main.test.mjs)CI Integration
.github/workflows/run-unit-tests.yml) to automatically run unit tests for all packages when a pull request is opened againstmain.