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

Fix docker:S2612 while setting permissions to SQ folders #663

Conversation

carminevassallo
Copy link
Collaborator

No description provided.

@jCOTINEAU
Copy link
Collaborator

jCOTINEAU commented Jan 16, 2024

I think we need to extend a bit the permission system, as the following block

chown -R sonarqube:sonarqube "${SQ_DATA_DIR}" "${SQ_EXTENSIONS_DIR}" "${SQ_LOGS_DIR}" "${SQ_TEMP_DIR}"; \
chmod -R 774 "${SQ_DATA_DIR}" "${SQ_EXTENSIONS_DIR}" "${SQ_LOGS_DIR}" "${SQ_TEMP_DIR}"; \

will break installation where uid is not 1000 by not allowing random uid to write into the temp folders.

docker run --rm -it -u 1001 will raise permission issue.

While looking at openshift documentation, which seems to be the best practice in terms of random uid setup

It seems we should be using the root group as a fixed entity to give permissions to. while being more elegant than having full open, I don't really see how it improves security ?

If another app that follows the same permission pattern manages to escape to the host, then they will have access to your filesystem anyway by being part of the root group.

And if it is your app that has a flaw then anyway the intruder will have the same permission as the user running the app, therefore gaining access.

Anyway we should follow best practices wdyt ?

@carminevassallo
Copy link
Collaborator Author

Mmm, very good catch! Supporting Openshift's arbitrary user ids means we need to add the sonarqube user to the root group and then only assign permissions at the group level.

I would vote for changing the permissions of those folders for two reasons:

  • as of now, we grant full permissions to other users on the folders. So, adding the sonarqube user to the root group and assigning full permission to the root group only will be an improvement, as we exclude others from having full access (therefore, fix the hotspot).
  • we don't explicit support the arbitrary user ids, that is crucial in Openshift

@carminevassallo
Copy link
Collaborator Author

I drafted a change in the 9/community/Dockerfile only. Let me know what you think @jCOTINEAU, however, we need to adapt parts of the helm chart probably...

@jCOTINEAU
Copy link
Collaborator

jCOTINEAU commented Jan 17, 2024

hmm that's true, in order to not change too much the helm chart I would suggest something like this:

    chown sonarqube:root ${SONARQUBE_HOME}; \
    chmod -R 550 ${SONARQUBE_HOME}; \
    chown -R sonarqube:root "${SQ_DATA_DIR}" "${SQ_EXTENSIONS_DIR}" "${SQ_LOGS_DIR}" "${SQ_TEMP_DIR}"; \
    chmod -R 770 "${SQ_DATA_DIR}" "${SQ_EXTENSIONS_DIR}" "${SQ_LOGS_DIR}" "${SQ_TEMP_DIR}"; \

With that, the default helm chart parameter should work (have not tested yet) but if a user changed both runAsUser and runAsGroup in the helm chart it will break.

I would suggest using this setup and changing helm default with runAsUser: 1000 runAsGroup: 0 to follow closely what would happen on OpenShift while still having broad compatibility.

It would look like the best tradeoff in terms of user-friendliness and security, but we will have to clearly communicate the change as it matters a lot

Wdyt ?

@carminevassallo carminevassallo force-pushed the bug/cv/SONAR-21432-address-docker-s-2612-in-sq-docker-images branch 2 times, most recently from d5a7f55 to 8b5d1aa Compare January 18, 2024 18:13
Copy link
Collaborator

@jCOTINEAU jCOTINEAU left a comment

Choose a reason for hiding this comment

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

lgtm.

@carminevassallo carminevassallo force-pushed the bug/cv/SONAR-21432-address-docker-s-2612-in-sq-docker-images branch from 8b5d1aa to 3ce4e3f Compare January 22, 2024 15:25
Copy link

@carminevassallo carminevassallo merged commit 8e556d6 into master Jan 22, 2024
31 checks passed
@carminevassallo carminevassallo deleted the bug/cv/SONAR-21432-address-docker-s-2612-in-sq-docker-images branch January 22, 2024 16:42
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