Further steps for CI functions with GH actions

[bekuno]

After merging PR #15529 the configuration needs some tuning.

• configure secrets for normal workflow
• configure secrets for workflows from dependabot (if necessary)
• clarify, if workflow(s) should not run from cloned repositories - and if yes, configure it
• create documentation to configure secrets for cloned repositories

(Feel free to extend the list)

[bekuno]

For failing standard test see https://github.com/cgeo/cgeo/actions/runs/8693327234/job/23839868312?pr=15607

[Lineflyer]

AFAICS the secrets are already stored in our repo key/secrests storage.
If this problem only comes up for builds on forks (looks a bit like that) we should disable this feature. We do not need that and it blocks resources.

[bekuno]

AFAICS the secrets are already stored in our repo key/secrests storage. If this problem only comes up for builds on forks (looks a bit like that) we should disable this feature. We do not need that and it blocks resources.

The failing test above links to a PR in the c:geo repo for master against the source in a cloned repo (typically for a normal PR).

[Lineflyer]

mmh...thats indeed a pretty normal case.
I would have thought, that it uses the secret of the target repo rather than requiring the secrets to be located in the source/fork repo.

[fm-sys]

Might be a security feature. Otherwise developers might introduce malicious code to retrieve the secrets... Still problematic for our workflow.

[bekuno]

What I found at GH to the problem is: https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks.

[fm-sys]

The relevant part out of it:

workflows from forks do not have access to sensitive data such as secrets

[kumy]

Yes this is our issue, a security issue.
@bekuno From our mails, you wanted to disable such jobs for PR from external repositories, am I right?

I did a small POC to test the expression required in such case

• main repo: https://github.com/geokrety/just-a-test
• forked repo: https://github.com/kumy/just-a-test

It allows:

• all jobs to execute on PR from the same repo
• All jobs on push
• PR from forks repo can skip same "secured steps"
• On forks side, PR from fork to fork executes all steps

The expression I came to:

github.event_name == 'push' || (github.event_name == 'pull_request' && github.event.pull_request.base.repo.id == github.event.pull_request.head.repo.id)

We should add some well placed one to our workflows.
We should also check if the secret is empty and provide a better way of failing

[kumy]

(I didn't remembered I had set the action cgeo-preferences on all jobs.)

Does someone know if graddle tasks below really require the app to be fully configured? Is it something that we can skip?

• :main:countBasicDebugDexMethods
• :main:checkstyle
• :main:lintBasicDebug

[kumy]

That would give PR #15626

[Lineflyer]

I am not sure if skipping part of tests is meaningful.
The remaining problem is, that secrets are not shared, which will prevent a full test of PRs from forked repositories.

From what I read here https://securitylab.github.com/research/github-actions-preventing-pwn-requests/ (not sure if outdated but sounds logical from security perspective) we could also make use of pull_request_target in combination with a simple approval workflow e.g. by setting a label Safe to test or OK to test.

As this trigger allows to use secrets of the target repo we should be able to fully test the PRs.
The safety measure is, that labels can only be set by people with write access to the target repo.

[kumy]

I will read the article soon. I agree we should have way to test incoming PRs.

On PR #15625 was afraid you'll activate pull_request_target blindly. As long as each PR is reviewed before the WF can run, we should be safe.

[Lineflyer]

On PR #15625 was afraid you'll activate pull_request_target blindly. As long as each PR is reviewed before the WF can run, we should be safe.

That part (label trigger) was not yet included as I did not (out of the box) know how to do this...but its mentioned in the article.
Help appreciated if you think this method is meaningful to achieve our goal.

[Lineflyer]

Regarding:

configure secrets for workflows from dependabot (if necessary)

Further reading:
https://docs.github.com/en/code-security/dependabot/working-with-dependabot/automating-dependabot-with-github-actions#accessing-secrets

[kumy]

If you accept the security risk you can redefine them all in the dependabot secret section

its mentioned in the article.

I'll read it and see how we can implement their proposal

(heading to bed now)

[Lineflyer]

If you accept the security risk you can redefine them all in the dependabot secret section

That brings me to the point, that I do not have any of the secrets. They have ever only been stored on the CI and in the hands of @mucek4
@kumy If there is no objection form others, please kindly forward the whole set to me in a safe way.

[kumy]

I didn't stored them. I will need to extract them from Jenkins again 😉🫣

[Lineflyer]

clarify, if workflow(s) should not run from cloned repositories - and if yes, configure it

IMHO they should not run in cloned repositories.
For PR and pushes:
We want to have this quality gate for PRs and pushes to our repo. Developers usually have their SDK and emulator to run the tests they need (with own keys and credentials).
For watchdog:
It makes no sense if watchdog tests run scheduled in all forks.

The short cut-off method is to disable actions in your forks using the repository settings.
but (@kumy) I guess it would be possible by adding a condition to the workflows to only run if triggered in or for our main repo.

[kumy]

That brings me to the point, that I do not have any of the secrets. They have ever only been stored on the CI and in the hands of @mucek4
@kumy If there is no objection form others, please kindly forward the whole set to me in a safe way.

@Lineflyer I've sent you a mail

[kumy]

secrets added for dependabot ✔️
That job is un-stuck, but it may also encounter the same issue as in #15630 (comment)

[kumy]

@Lineflyer please see PR #15631 to

• Only run watchdog from cgeo repo
• Check for secret presence and fail is missing

[kumy]

@Lineflyer here is what I understood from the link you sent earlier (untested)
#15632

Also note from the document

Warning

Note that this kind of label based verification is still prone to a race condition in which the attacker may push new changes after the workflow was approved (labeled), but has not started yet.

(Heading to bed now)