lf-edge / eden

Eden is where EVE and Adam get tried and tested:
https://projecteve.dev
Apache License 2.0
49 stars 47 forks source link

GitHub Actions use org secrets #999

Closed deitch closed 1 month ago

deitch commented 1 month ago

lfedge created a new user for all of the workflows, and saved the credentials as org secrets, specifically RELEASE_DOCKERHUB_ACCOUNT and RELEASE_DOCKERHUB_TOKEN. This PR removes the different name in the flows and uses the org standard secrets.RELEASE_DOCKERHUB_ACCOUNT and secrets.RELEASE_DOCKERHUB_TOKEN.

Eden actually was using both, RELEASE_DOCKERHUB_* and DOCKERHUB_*, with only a few left using the DOCKERHUB_*. This unifies it all.

milan-zededa commented 1 month ago

Note that for tests running for an open PR this dockerhub credentials will not be used unless we switch pull_request to pull_request_target. But I think that in this repo it is not so important (we do not open PRs that often here) and it has security risks. I'm not sure about pull_request_review that we use in the eve repo - does it have access to secrets?

milan-zededa commented 1 month ago

However, it seems that in each test workflow the only non-OSS pull is for the nginx container (or is it OSS, how can I check?). Hence it is not so import to use the dockerhub account for eden tests.

deitch commented 1 month ago

However, it seems that in each test workflow the only non-OSS pull is for the nginx container (or is it OSS, how can I check?).

nginx is not OSS. You look at the repo. For example, lfedge/eve has this:

Screenshot 2024-07-10 at 11 38 34 AM

While nginx does not:

Screenshot 2024-07-10 at 11 39 07 AM

"Docker official images" explicitly are subject to pull limits.

Hence it is not so import to use the dockerhub account for eden tests.

So maybe we do not need it at all? Perhaps. Let's this one in, which just switches the secret used to unify it. Then we can remove secret access entirely in a separate one if we want.

milan-zededa commented 1 month ago

So maybe we do not need it at all? Perhaps. Let's this one in, which just switches the secret used to unify it. Then we can remove secret access entirely in a separate one if we want.

Note that I'm not against using docker login in tests. It is just that these secrets are not available to an open PR (unless we use pull_request_target) and thus we will continue running them with anonymous docker user.

uncleDecart commented 1 month ago

Eden was using inherited secrets from EVE, hence the naming release. We used secrets instead of inputs as per GitHub guidelines. I wonder though if any repository using reusable Eden workflow will have access to this credentials

milan-zededa commented 1 month ago

Eden was using inherited secrets from EVE, hence the naming release. We used secrets instead of inputs as per GitHub guidelines. I wonder though if any repository using reusable Eden workflow will have access to this credentials

The real problem is here: https://github.com/lf-edge/eve/blob/master/.github/workflows/eden.yml#L14 pull_request_review does not grant access to secrets, hence no secrets are propagated to eden (those fields are inherited empty).