Skip to content
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

support Github Actions OIDC #1054

Closed
wants to merge 1 commit into from
Closed

Conversation

zetaab
Copy link

@zetaab zetaab commented Aug 22, 2023

adds support for using Github OIDC that was requested codecov/feedback#53

so after this, the codecov-action itself supports following:

jobs:
  build:
    permissions: # these are needed after oidc change
      id-token: write
      contents: read
    steps:
    ...
    - uses: codecov/codecov-action@v3

However, this still needs support to codecov-api (codecov/codecov-api#90)

This is tested in following scenarios:

  • without token and without GHA permissions -> it will continue posting to Codecov without any token
  • without token and with GHA permission -> it will fetch Github JWT and post that to Codecov
  • with token and without GHA permission -> it will post original token to Codecov
  • with token and with GHA permission -> it will post original token to Codecov

This can be merged before the codecov-api has changes or updated version running. If someone will start using this before it will not just work. It should not break backwards compatibility

src/buildExec.ts Outdated Show resolved Hide resolved
src/buildExec.ts Outdated Show resolved Hide resolved
@zetaab
Copy link
Author

zetaab commented Aug 30, 2023

@thomasrockhu-codecov is there any timeline to get some feedback for the pr?

@thomasrockhu-codecov
Copy link
Contributor

@zetaab I'll take a look today

@codecov
Copy link

codecov bot commented Sep 6, 2023

Codecov Report

Merging #1054 (b9fd881) into main (525bbff) will increase coverage by 1.10%.
The diff coverage is 100.00%.

❗ Current head b9fd881 differs from pull request most recent head 8049f50. Consider uploading reports for the commit 8049f50 to get more accurate results

@@            Coverage Diff             @@
##             main    #1054      +/-   ##
==========================================
+ Coverage   94.83%   95.94%   +1.10%     
==========================================
  Files           4        4              
  Lines         213      345     +132     
  Branches       60      103      +43     
==========================================
+ Hits          202      331     +129     
- Misses         10       14       +4     
+ Partials        1        0       -1     
Flag Coverage Δ
demo 80.00% <ø> (-14.84%) ⬇️
macos-latest 95.94% <100.00%> (+1.10%) ⬆️
script 96.66% <100.00%> (+1.83%) ⬆️
ubuntu-latest 95.94% <100.00%> (+1.10%) ⬆️
version 80.00% <ø> (-14.84%) ⬇️
windows-latest 95.94% <100.00%> (+1.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/buildExec.ts 97.29% <100.00%> (+0.46%) ⬆️

... and 1 file with indirect coverage changes

src/buildExec.ts Outdated Show resolved Hide resolved
src/buildExec.ts Outdated Show resolved Hide resolved
@webknjaz
Copy link

@zetaab this is cool and it's nice to have. I just want to point out that OIDC won't work for PRs from forks because of the limited privileges the pull_request event-triggered GHA jobs have. So this might be something to document.

@s0undt3ch
Copy link

I just want to point out that OIDC won't work for PRs from forks because of the limited privileges the pull_request event-triggered GHA jobs have.

Bummer, then this does not cover our personal use case... We'll need tokenless support.

@webknjaz
Copy link

@s0undt3ch yep, that's definitely annoying. Though, you can use pull_request_target instead, if you're ready to take on more responsibility that GitHub currently has, security-wise. But don't forget to read all the security considerations — they have a doc on that somewhere.

@s0undt3ch
Copy link

@s0undt3ch yep, that's definitely annoying. Though, you can use pull_request_target instead, if you're ready to take on more responsibility that GitHub currently has, security-wise. But don't forget to read all the security considerations — they have a doc on that somewhere.

Nope 😁

What we really need is tokenless uploads, though the new codecov-cli does not support them 😞

@zetaab zetaab force-pushed the oidc branch 2 times, most recently from 156ba26 to 07e2b13 Compare October 20, 2023 07:18
/**
* Main function of the codecov-action
*/
async function run(): Promise<void> {
Copy link
Author

@zetaab zetaab Oct 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

basically this file is linted and added async + await to funcs

@zetaab zetaab force-pushed the oidc branch 3 times, most recently from 9756dc8 to 161a2a5 Compare October 20, 2023 08:01
@zetaab
Copy link
Author

zetaab commented Oct 20, 2023

This PR will work as is. However, newer codecov cli is now forcing token to be UUID

Error: Invalid value for '-t' / '--token': '\*\*\*' is not a valid UUID.

which should not be the case anymore after codecov/codecov-api#177. So we need another PR to codecov-cli to fix this issue.

(and seems that codecov-action does not work anymore against 0.6.x versions)

@LecrisUT
Copy link
Contributor

I guess this PR fixes codecov/feedback#112? What's the status for this?

@thomasrockhu-codecov
Copy link
Contributor

@zetaab I'm running into some issues pushing some changes here. We are looking to implement in the next 2 weeks. You can find the changes

#1330
#1329

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants