ethereum / solidity

Solidity, the Smart Contract Programming Language
https://soliditylang.org
GNU General Public License v3.0
22.66k stars 5.62k forks source link

Add authentication header to circleci api #15151

Closed r0qs closed 1 month ago

r0qs commented 1 month ago

Attempt to fix c_ext_benchmarks job when downloading artifacts from CircleCI API: https://app.circleci.com/pipelines/github/ethereum/solidity/34295/workflows/0cf0489b-11bd-4eae-8a23-6951de527304/jobs/1552691

The problem is only with CircleCI API that apparently started to require authenticated requests. Maybe this changed recently in the v2 API, I honestly don't know. I couldn't find any information about it.

Also, note that CircleCI v2 API does not support project API tokens and, currently, recommends the use of personal API tokens (see: https://circleci.com/docs/api/v2/index.html#section/Authentication).

Furthermore, the way it is currently done in this PR, reads a personal token from the environment variable that is set in the project settings to build the http header, this method has some risks of exposing the token, but we already do similar thing in our CI: https://github.com/ethereum/solidity/blob/develop/.circleci/config.yml#L146

nikola-matic commented 1 month ago

reads a personal token from the environment variable that is set in the project settings to build the http header, this method has some risks of exposing the token

Isn't this in general the way things are done - i.e. store token in an ENV variable, and then read and use from a script? I.e. I don't know how much better we can make this, or what the alternative approaches are?

r0qs commented 1 month ago

reads a personal token from the environment variable that is set in the project settings to build the http header, this method has some risks of exposing the token

Isn't this in general the way things are done - i.e. store token in an ENV variable, and then read and use from a script? I.e. I don't know how much better we can make this, or what the alternative approaches are?

Yes, I think so. When used in the CI I don't see alternatives to manage those secrets other than what we already do (https://circleci.com/docs/security-recommendations/). But I wanted to point it out just in case ;)

r0qs commented 1 month ago

Just to let it documented, as stated by CircleCI docs, they do not expose those variables unless the project is explicitly set to share it with forks, which we don't.

Environment variables set in the CircleCI application are hidden from the public,
these variables will not be shared in forked PRs unless explicitly enabled.

Source: https://circleci.com/docs/oss/#security

This means that PRs from external contributors will still fail after this is merged if they not add to their CircleCI's fork config a environment variable named CIRCLECI_TOKEN with their CircleCI's personal token.

cameel commented 1 month ago

As discussed on the chat, let's add a check in c_ext_benchmarks definition to skip the script if the token is not available. Then it won't be failing for external contributors.