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

SQSCANGHA-56 Support GitHub self-hosted runners without keytool #149

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

antonioaversa
Copy link
Contributor

@antonioaversa antonioaversa commented Nov 21, 2024

Successful run here

This PR uses the keytool embedded in the JRE inside the scanner, instead of relying on the keytool command being installed on the environment running the action.

The PR also introduces QA tests checking the full SSL flow (previously QA only checked that the p12 file was in the right place).
The test is done via an nginx serving https port 4443 with a self-signed certificate (generated on the fly).
The nginx instance forwards to a local SQ instance on port 9000.

The PR merges into dev (which currently points to v4.0.0) since master should stay on v3.0.0 until December 9th.

@antonioaversa antonioaversa force-pushed the antonio/support-for-missing-keytool branch from 5b83703 to 36e580d Compare November 21, 2024 16:52
@antonioaversa antonioaversa force-pushed the antonio/support-for-missing-keytool branch 13 times, most recently from 0e85316 to e3a4f6c Compare November 25, 2024 13:52
@antonioaversa antonioaversa marked this pull request as ready for review November 25, 2024 14:01
@antonioaversa antonioaversa requested review from a team, Godin and henryju and removed request for a team November 25, 2024 14:02
@antonioaversa antonioaversa force-pushed the antonio/support-for-missing-keytool branch 2 times, most recently from 3c40241 to f45d12d Compare November 25, 2024 17:48
@antonioaversa antonioaversa force-pushed the antonio/support-for-missing-keytool branch from f45d12d to 0da496f Compare November 25, 2024 17:51
@antonioaversa
Copy link
Contributor Author

@Godin @henryju I have made updates to the PR, to fix the healthcheck of nginx, which was just waiting for 60 seconds and then moving on. I have now added a health endpoint on port 8080, which ensures that docker compose up waits for nginx to be in an operational state, before proceeding with the rest of the action.
The PR is ready, and you can proceed with the review.

Copy link
Member

@henryju henryju left a comment

Choose a reason for hiding this comment

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

Very nice, I like the use of nginx to test SSL, this is something I will likely reuse ;)

I think you can remove the generation of the client SSL certificate, but otherwise, LGTM!

run-sonar-scanner.sh Show resolved Hide resolved
.github/qa-sq-behind-ngix/generate-certificates.sh Outdated Show resolved Hide resolved
@antonioaversa antonioaversa force-pushed the antonio/support-for-missing-keytool branch 3 times, most recently from 72ef13a to d398b15 Compare November 26, 2024 10:26
@antonioaversa antonioaversa force-pushed the antonio/support-for-missing-keytool branch from d398b15 to 9cd13fb Compare November 26, 2024 10:28
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.

2 participants