Skip to content

support Github Actions OIDC#1054

Closed
zetaab wants to merge 1 commit intocodecov:mainfrom
elisa-actions:oidc
Closed

support Github Actions OIDC#1054
zetaab wants to merge 1 commit intocodecov:mainfrom
elisa-actions:oidc

Conversation

@zetaab
Copy link

@zetaab zetaab commented Aug 22, 2023 •
edited
Loading

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

zetaab
zetaab commented Aug 22, 2023
View reviewed changes
zetaab
zetaab commented Aug 22, 2023
View reviewed changes
@juho9000 juho9000 mentioned this pull request Aug 22, 2023
Closed
@zetaab zetaab force-pushed the oidc branch 2 times, most recently from d3656aa to b9fd881 Compare August 23, 2023 04:38
@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

thomasrockhu-codecov commented Sep 5, 2023

@zetaab I'll take a look today

@codecov
Copy link

codecov bot commented Sep 6, 2023 •
edited
Loading

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

thomasrockhu-codecov
thomasrockhu-codecov requested changes Sep 6, 2023
View reviewed changes
@rohan-at-sentry rohan-at-sentry mentioned this pull request Sep 6, 2023
Closed
@webknjaz
Copy link
Contributor

webknjaz commented Sep 26, 2023

@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

s0undt3ch commented Sep 26, 2023

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
Contributor

webknjaz commented Sep 27, 2023

@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 commented Sep 27, 2023

@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
zetaab
zetaab commented Oct 20, 2023
View reviewed changes
src/index.ts
/**
* Main function of the codecov-action
*/
async function run(): Promise<void> {
Copy link
Author

@zetaab zetaab Oct 20, 2023 •
edited
Loading

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 •
edited
Loading

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)

@zetaab zetaab mentioned this pull request Oct 20, 2023
Closed
@zetaab zetaab mentioned this pull request Nov 14, 2023
Merged
@LecrisUT
Copy link
Contributor

LecrisUT commented Dec 18, 2023

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

@nora-shap nora-shap mentioned this pull request Mar 12, 2024
Merged
This was referenced Mar 17, 2024
Merged
Merged
@thomasrockhu-codecov
Copy link
Contributor

thomasrockhu-codecov commented Mar 17, 2024

@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

Reviewers

@thomasrockhu-codecov thomasrockhu-codecov thomasrockhu-codecov requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@zetaab @thomasrockhu-codecov @webknjaz @s0undt3ch @LecrisUT