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 SELinux issues when downloading Python packages. #2382

Closed

Conversation

jdoss
Copy link

@jdoss jdoss commented Oct 4, 2023

This PR creates a directory inside the cache directory that is volume mounted and then tells pip to use that directory instead of /tmp to avoid SELinux drama.

Fixes #2381

@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2023

CLA Assistant Lite bot:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


You can retrigger this bot by commenting recheck in this Pull Request

@rubenfiszel
Copy link
Contributor

I'm not sure to understand how this could work given that this script is only ever used in an nsjail context and in nsjail, /tmp/windmill/cache is not available directly in the sandbox that run that script: https://github.com/windmill-labs/windmill/blob/main/backend/windmill-worker/nsjail/download.py.config.proto

@jdoss
Copy link
Author

jdoss commented Oct 4, 2023

I'm not sure to understand how this could work given that this script is only ever used in an nsjail context and in nsjail, /tmp/windmill/cache is not available directly in the sandbox that run that script: https://github.com/windmill-labs/windmill/blob/main/backend/windmill-worker/nsjail/download.py.config.proto

Isn't it available in the jail here?
https://github.com/windmill-labs/windmill/blob/main/backend/windmill-worker/nsjail/download.py.config.proto#L75-L80

I am not sure what value CACHE_DIR is set to but in order for the script being run in the nsjail, it would have to have write access to the shared cache directory otherwise it wouldn't work based on what I am seeing.

When it runs download.py.sh that script runs in the context of the nsjail, which from what I can tell gets the main container's SELinux context. This makes pip expand the python modules in /tmp (inside the nsjail/container context) and then it moves those files into the shared volume that is bind mounted to the shared cache directory.

@jdoss
Copy link
Author

jdoss commented Oct 4, 2023

CLA Assistant Lite bot: Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.

I have read the CLA Document and I hereby sign the CLA

You can retrigger this bot by commenting recheck in this Pull Request

Unfortunately I am done signing CLAs that take away my rights as an open source contributor (Say thanks to Hashicorp). The only way I will sign a CLA is if it clearly states that the original AGPL project can never have it's license changed away from the AGPL and it will forever remain FOSS.

@rubenfiszel
Copy link
Contributor

Isn't it available in the jail here?
https://github.com/windmill-labs/windmill/blob/main/backend/windmill-worker/nsjail/download.py.config.proto#L75-L80

Yes ... you are right :)

Unfortunately I am done signing CLAs that take away my rights as an open source contributor (Say thanks to Hashicorp). The only way I will sign a CLA is if it clearly states that the original AGPL project can never have it's license changed away from the AGPL and it will forever remain FOSS.

I understand where you're coming from but there are very boring reasons why on our end it would create enormous amount of complexity to make such legal statement. You may have my word for it but that is not legally binding.

@jdoss
Copy link
Author

jdoss commented Oct 4, 2023

I understand where you're coming from but there are very boring reasons why on our end it would create enormous amount of complexity to make such legal statement. You may have my word for it but that is not legally binding.

But it is possible to make such legal statements. It just requires time, money and unfortunately talking to lawyers. I get it. I understand you folks need to get paid for EE in order to support CE and in order to have EE you need a CLA that lets you re-license the FOSS contributions. If I have commercial uses for Windmill, I will for sure pony up the cash for EE. It's great software and I have no problem paying for only commercial features.

I am no longer signing away my rights to my Copyright (even if it is a two line fix like in this PR) only to be Hashicorp'ed in the future without assurances in the CLA that the original Windmill project will forever remain AGPL. That said, I will close this PR out if signing the CLA is a hard requirement for this contribution.

@rubenfiszel
Copy link
Contributor

Right now we do not have the bandwidth to set such legal agreement for this PR. I completely understand your viewpoint however. May we open the same PR on our end ?

@rubenfiszel rubenfiszel closed this Oct 4, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Oct 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

bug: Python packages get the wrong SELinux context when being downloaded by workers
2 participants